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

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

[Tony Dinh]

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.

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;
 }
 
 static void mv_sata_xfer_mode(struct udevice *dev, int port, u16 *id)
-- 
2.20.1

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

[Stefan Roese]

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?

Thanks,
Stefan

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

[Tony Dinh]

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

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

[Tony Dinh]

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.

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.

Reviewed-by: Stefan Roese <sr@denx.de>
Signed-off-by: Tony Dinh <mibodhi@gmail.com>
---

Changes in v2:
Return value -ENODEV if mv_identify() failed to identify the drive

 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..dadb2c7c2e 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 -ENODEV;
 }
 
 static void mv_sata_xfer_mode(struct udevice *dev, int port, u16 *id)
-- 
2.20.1

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

[Stefan Roese]

On 01.08.21 05:29, 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.
> 
> 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.
> 
> Reviewed-by: Stefan Roese <sr@denx.de>
> Signed-off-by: Tony Dinh <mibodhi@gmail.com>

Applied to u-boot-marvell/master

Thanks,
Stefan

> ---
> 
> Changes in v2:
> Return value -ENODEV if mv_identify() failed to identify the drive
> 
>   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..dadb2c7c2e 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 -ENODEV;
>   }
>   
>   static void mv_sata_xfer_mode(struct udevice *dev, int port, u16 *id)
> 


Viele Grüße,
Stefan

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-51 Fax: (+49)-8142-66989-80 Email: sr@denx.de