Hi Bin, On 23 December 2014 at 18:42, Bin Meng <bmeng...@gmail.com> wrote: > Hi Simon, > > On Wed, Dec 24, 2014 at 3:42 AM, Simon Glass <s...@chromium.org> wrote: >> Hi Bin, >> >> On 23 December 2014 at 01:01, Bin Meng <bmeng...@gmail.com> wrote: >>> >>> Each time U-Boot boots on Intel Crown Bay board, the displayed hard >>> drive information is wrong. It could be either wrong capacity or just >>> a 'Capacity: not available' message. After enabling the debug switch, >>> we can see the scsi inquiry command did not execute successfully. >>> However, doing a 'scsi scan' in the U-Boot shell does not expose >>> this issue. >> >> This sounds like an error condition is not being propagated. > > Actually an error condition not checked. > >>> >>> SCSI: Target spinup took 0 ms. >>> SATA link 1 timeout. >>> AHCI 0001.0100 32 slots 2 ports 3 Gbps 0x3 impl SATA mode >>> flags: ncq stag pm led clo only pmp pio slum part ccc apst >>> scanning bus for devices... >>> ahci_device_data_io: 0 byte transferred. <--- scsi inquiry fails >>> ahci_device_data_io: 512 byte transferred. >>> ahci_device_data_io: 512 byte transferred. >>> ahci_device_data_io: 512 byte transferred. >>> Device 0: (0:0) Vendor: ATA Prod.: Rev: ?8 >>> Type: Hard Disk >>> Capacity: 912968.3 MB = 891.5 GB (1869759264 x >>> 512) >>> Found 1 device(s). >>> >>> So uninitialized contents on the stack were passed to dev_print() to >>> display those wrong information. >>> >>> The symptom were observed on two hard drives (one is Seagate, the >>> other one is Western Digital). The fix is to make sure the AHCI >>> interface is not busy by checking the error and status information >>> from task file register after enabling the port in ahci_port_start() >>> before proceeding other operations like scsi_scan(). >>> >>> Signed-off-by: Bin Meng <bmeng...@gmail.com> >>> >>> --- >>> >>> drivers/block/ahci.c | 15 ++++++++++++++- >>> 1 file changed, 14 insertions(+), 1 deletion(-) >>> >>> diff --git a/drivers/block/ahci.c b/drivers/block/ahci.c >>> index c9a3beb..7ca8f40 100644 >>> --- a/drivers/block/ahci.c >>> +++ b/drivers/block/ahci.c >>> @@ -505,8 +505,9 @@ static int ahci_port_start(u8 port) >>> { >>> struct ahci_ioports *pp = &(probe_ent->port[port]); >>> volatile u8 *port_mmio = (volatile u8 *)pp->port_mmio; >>> - u32 port_status; >>> + u32 port_status, tf_data; >>> u32 mem; >>> + int i = 0; >>> >>> debug("Enter start port: %d\n", port); >>> port_status = readl(port_mmio + PORT_SCR_STAT); >>> @@ -564,6 +565,18 @@ static int ahci_port_start(u8 port) >>> PORT_CMD_POWER_ON | PORT_CMD_SPIN_UP | >>> PORT_CMD_START, port_mmio + PORT_CMD); >>> >>> + /* >>> + * Make sure interface is not busy based on error and status >>> + * information from task file data register before proceeding >>> + */ >>> + while (i < WAIT_MS_SPINUP) { >>> + tf_data = readl(port_mmio + PORT_TFDATA); >>> + if (!(tf_data & ATA_BUSY)) >>> + break; >>> + udelay(1000); >>> + i++; >>> + } >>> + >> >> Doesn't this fall through after a timeout and fail to report an error? > > Ah, yes! We should return error code when timeout. But some other > routines in the scsi initialization path do no check return values, > like initr_scsi()->scsi_init()->scsi_low_level_init().
I suppose that could be improved separately but does it affect this patch? > >> As a matter of style I think something like the below is better >> because it the timeout will be more accurate in the case where you >> have a lot of processing each time around the loop. >> >> static int wait_spinup(void) >> { >> ulong start; >> >> start = get_timer(0); >> do { >> tf_data = ...; >> if (!((tf_data & ATA_BUSY)) >> return 0; >> while (get_timer(start) < WAIT_MS_SPINUP); >> return -ETIMEDOUT; >> } > > Looks like the original codes there are using i++ style for the > timeout check instead of get_timer(). > >> Also how does this relate to the existing spinup delay code in >> ahci_host_init()? > > This seems to me they are not related. Per my understanding, the check > we need here is because we write something to the port command > register, but we missed the task file data busy indication. > > writel_with_flush(PORT_CMD_ICC_ACTIVE | PORT_CMD_FIS_RX | > PORT_CMD_POWER_ON | PORT_CMD_SPIN_UP | > PORT_CMD_START, port_mmio + PORT_CMD); > > [snip] > OK. Regards, Simon _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot