[PATCH] arm: mvebu: sata_mv failed to identify HDDs during cold start
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)
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
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
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)
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
participants (2)
-
Stefan Roese -
Tony Dinh