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