Re: [PATCH] arm: mvebu: sata_mv failed to identify HDDs during cold start

[Tony Dinh <mibodhi@gmail.com>]

Hi Stefan,


On Sat, Jul 31, 2021 at 12:31 AM Stefan Roese <sr@denx.de> wrote:
>
> Hi Tony,
>
> On 25.07.21 23:57, Tony Dinh wrote:
> > During cold start, with some HDDs, mv_sata_identify() does not populate
> > the ID words on the 1st ATA ID command. In fact, the first ATA ID
> > command will only power up the drive, and then the ATA ID command
> > processing is lost in the process.
> >
> > Tests with:
> >
> > - Seagate ST9250320AS 250GB HDD and Seagate ST4000DM004-2CV104 4TB HDD.
> > - Zyxel NSA310S (Kirkwood 88F6702), Marvell Dreamplug (Kirkwood 88F6281),
> >   Seagate GoFlex Home (Kirkwood 88F6281), Pogoplug V4 (Kirkwood 88F6192).
> >
> > Observation:
> >
> > - The Seagate ST9250320AS 250GB took about 3 seconds to spin up.
> > - The Seagate ST4000DM004-2CV104 4TB took about 8 seconds to spin up.
> > - mv_sata_identify() did not populate the ID words after the call to
> >   mv_ata_exec_ata_cmd_nondma().
> > - Attempt to insert a long delay of 30 seconds, ie. mdelay(30_000), after
> > the call to ata_wait_register() inside mv_ata_exec_ata_cmd_nondma() did
> > not help with the 4TB drive. The ID words were still empty after that 30s
> > delay.
>
> IIRC, I've seen similar issues with a SATA SSD on theadorable before as
> well. Thanks for digging into this.
>
> One some nit below...
>
> > Patch Description:
> >
> > - Added a second ATA ID command in mv_sata_identify(), which will be
> > executed if the 1st ATA ID command did not return with valid ID words.
> > - Use the HDD drive capacity in the ID words as a successful indicator of
> > ATA ID command.
> > - In the scenario where a box is rebooted, the 1st ATA ID command is always
> > successful, so there is no extra time wasted.
> > - In the scenario where a box is cold started, the 1st ATA command is the
> > power up command. The 2nd ATA ID command alleviates the uncertainty of
> > how long we have to wait for the ID words to be populated by the SATA
> > controller.
> >
> > Signed-off-by: Tony Dinh <mibodhi@gmail.com>
> > ---
> >
> >   drivers/ata/sata_mv.c | 29 +++++++++++++++++++++++++++--
> >   1 file changed, 27 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/ata/sata_mv.c b/drivers/ata/sata_mv.c
> > index 1012cb5374..7d1515d5f8 100644
> > --- a/drivers/ata/sata_mv.c
> > +++ b/drivers/ata/sata_mv.c
> > @@ -809,6 +809,7 @@ static int mv_ata_exec_ata_cmd_nondma(struct udevice *dev, int port,
> >   static int mv_sata_identify(struct udevice *dev, int port, u16 *id)
> >   {
> >       struct sata_fis_h2d h2d;
> > +     int len;
> >
> >       memset(&h2d, 0, sizeof(struct sata_fis_h2d));
> >
> > @@ -818,8 +819,32 @@ static int mv_sata_identify(struct udevice *dev, int port, u16 *id)
> >       /* Give device time to get operational */
> >       mdelay(10);
> >
> > -     return mv_ata_exec_ata_cmd_nondma(dev, port, &h2d, (u8 *)id,
> > -                                       ATA_ID_WORDS * 2, READ_CMD);
> > +     /* During cold start, with some HDDs, the first ATA ID command does
> > +      * not populate the ID words. In fact, the first ATA ID
> > +      * command will only power up the drive, and then the ATA ID command
> > +      * processing is lost in the process.
> > +      */
> > +     len = mv_ata_exec_ata_cmd_nondma(dev, port, &h2d, (u8 *)id,
> > +                                      ATA_ID_WORDS * 2, READ_CMD);
> > +
> > +     /* If drive capacity has been filled in, then it was successfully
> > +      * identified (the drive has been powered up before, i.e.
> > +      * this function is invoked during a reboot)
> > +      */
> > +     if (ata_id_n_sectors(id) != 0)
> > +             return len;
> > +
> > +     /* Issue the 2nd ATA ID command to make sure the ID words are
> > +      * populated properly.
> > +      */
> > +     mdelay(10);
> > +     len = mv_ata_exec_ata_cmd_nondma(dev, port, &h2d, (u8 *)id,
> > +                                      ATA_ID_WORDS * 2, READ_CMD);
> > +     if (ata_id_n_sectors(id) != 0)
> > +             return len;
> > +
> > +     printf("Err: Failed to identify SATA device %d\n", port);
> > +     return -1;
>
> Perhaps better to return -ENODEV or something similar instead of the -1
> here?

Much agreed. However, I could not find any other error status that's a
bit more descriptive. So I guess we should use -ENODEV.

I will send a v2 version for this patch.

Thanks,
Tony

>
> Thanks,
> Stefan