[U-Boot] [PATCH v1] x86: ahci: Make sure interface is not busy after enabling the port

[Simon Glass]

Hi Bin,

On 23 December 2014 at 18:42, Bin Meng <bmeng.cn at gmail.com> wrote:
> Hi Simon,
>
> On Wed, Dec 24, 2014 at 3:42 AM, Simon Glass <sjg at chromium.org> wrote:
>> Hi Bin,
>>
>> On 23 December 2014 at 01:01, Bin Meng <bmeng.cn at 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.cn at 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