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

[Stefan Roese <sr@denx.de>]

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