Hi Stefan,
On Sat, Jul 31, 2021 at 12:31 AM Stefan Roese <s...@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 <mibo...@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