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

[Bin Meng]

Hi Simon,

On Tue, Dec 30, 2014 at 12:33 AM, Simon Glass <sjg at chromium.org> wrote:
> 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?

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