Hi Simon, On Tue, Dec 30, 2014 at 12:33 AM, Simon Glass <s...@chromium.org> wrote: > 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?
No, it doesn't affect this patch. Yes, we could improve those places with a separate 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, Bin _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot