[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 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.

>
> 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?

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;
}

Also how does this relate to the existing spinup delay code in ahci_host_init()?

>
>         debug("Exit start port %d\n", port);
>
>         return 0;
> --
> 1.8.2.1
>

Regards,
Simon