[PATCH v3 1/3] PCI: Add device code for AMD FCH SATA Controller in AHCI mode

[PATCH v3 1/3] PCI: Add device code for AMD FCH SATA Controller in AHCI mode

[Paul Menzel]

The ASUS F2A85-M PRO with the fusion controller hub (FCH) AMD A85
(Hudson D4) has the SATA controller below.

    $ lspci -s 00:11.0
    00:11.0 SATA controller [0106]: Advanced Micro Devices, Inc. [AMD] FCH SATA Controller [AHCI mode] [1022:7801] (rev 40)

Add the ID for it, when in AHCI mode.

Signed-off-by: Paul Menzel <pmenzel@molgen.mpg.de>
---
 include/linux/pci_ids.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/linux/pci_ids.h b/include/linux/pci_ids.h
index 011f2f1ea5bb..fe944b44858a 100644
--- a/include/linux/pci_ids.h
+++ b/include/linux/pci_ids.h
@@ -602,6 +602,7 @@
 #define PCI_DEVICE_ID_AMD_LX_VIDEO  0x2081
 #define PCI_DEVICE_ID_AMD_LX_AES    0x2082
 #define PCI_DEVICE_ID_AMD_HUDSON2_SATA_IDE	0x7800
+#define PCI_DEVICE_ID_AMD_HUDSON2_SATA_AHCI	0x7801
 #define PCI_DEVICE_ID_AMD_HUDSON2_SMBUS		0x780b
 #define PCI_DEVICE_ID_AMD_HUDSON2_IDE		0x780c
 #define PCI_DEVICE_ID_AMD_KERNCZ_SMBUS  0x790b
-- 
2.30.2

[PATCH v3 2/3] ahci: Use macro PCI_DEVICE_ID_AMD_HUDSON2_SATA_IDE

[Paul Menzel]

Use the defined macro from `include/linux/pci_ids.h`.

Signed-off-by: Paul Menzel <pmenzel@molgen.mpg.de>
---
 drivers/ata/ahci.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
index 1e1167e725a4..6a2432e4adda 100644
--- a/drivers/ata/ahci.c
+++ b/drivers/ata/ahci.c
@@ -436,7 +436,7 @@ static const struct pci_device_id ahci_pci_tbl[] = {
 		.class_mask = 0xffffff,
 		board_ahci_al },
 	/* AMD */
-	{ PCI_VDEVICE(AMD, 0x7800), board_ahci }, /* AMD Hudson-2 */
+	{ PCI_VDEVICE(AMD, PCI_DEVICE_ID_AMD_HUDSON2_SATA_IDE), board_ahci },
 	{ PCI_VDEVICE(AMD, 0x7900), board_ahci }, /* AMD CZ */
 	{ PCI_VDEVICE(AMD, 0x7901), board_ahci_mobile }, /* AMD Green Sardine */
 	/* AMD is using RAID class only for ahci controllers */
-- 
2.30.2

Re: [PATCH v3 2/3] ahci: Use macro PCI_DEVICE_ID_AMD_HUDSON2_SATA_IDE

[Damien Le Moal]

On 12/30/21 01:11, Paul Menzel wrote:
> Use the defined macro from `include/linux/pci_ids.h`.
> 
> Signed-off-by: Paul Menzel <pmenzel@molgen.mpg.de>
> ---
>  drivers/ata/ahci.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
> index 1e1167e725a4..6a2432e4adda 100644
> --- a/drivers/ata/ahci.c
> +++ b/drivers/ata/ahci.c
> @@ -436,7 +436,7 @@ static const struct pci_device_id ahci_pci_tbl[] = {
>  		.class_mask = 0xffffff,
>  		board_ahci_al },
>  	/* AMD */
> -	{ PCI_VDEVICE(AMD, 0x7800), board_ahci }, /* AMD Hudson-2 */
> +	{ PCI_VDEVICE(AMD, PCI_DEVICE_ID_AMD_HUDSON2_SATA_IDE), board_ahci },

That breaks the style of the declarations here... And since
PCI_DEVICE_ID_AMD_HUDSON2_SATA_IDE is used only in drivers/pci/quirks.c,
I do not really see the point of this change.

>  	{ PCI_VDEVICE(AMD, 0x7900), board_ahci }, /* AMD CZ */
>  	{ PCI_VDEVICE(AMD, 0x7901), board_ahci_mobile }, /* AMD Green Sardine */
>  	/* AMD is using RAID class only for ahci controllers */


-- 
Damien Le Moal
Western Digital Research

Re: [PATCH v3 2/3] ahci: Use macro PCI_DEVICE_ID_AMD_HUDSON2_SATA_IDE

[Paul Menzel]

Dear Damien,


Am 30.12.21 um 03:13 schrieb Damien Le Moal:
> On 12/30/21 01:11, Paul Menzel wrote:
>> Use the defined macro from `include/linux/pci_ids.h`.
>>
>> Signed-off-by: Paul Menzel <pmenzel@molgen.mpg.de>
>> ---
>>   drivers/ata/ahci.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
>> index 1e1167e725a4..6a2432e4adda 100644
>> --- a/drivers/ata/ahci.c
>> +++ b/drivers/ata/ahci.c
>> @@ -436,7 +436,7 @@ static const struct pci_device_id ahci_pci_tbl[] = {
>>   		.class_mask = 0xffffff,
>>   		board_ahci_al },
>>   	/* AMD */
>> -	{ PCI_VDEVICE(AMD, 0x7800), board_ahci }, /* AMD Hudson-2 */
>> +	{ PCI_VDEVICE(AMD, PCI_DEVICE_ID_AMD_HUDSON2_SATA_IDE), board_ahci }, >
> That breaks the style of the declarations here... And since
> PCI_DEVICE_ID_AMD_HUDSON2_SATA_IDE is used only in drivers/pci/quirks.c,
> I do not really see the point of this change.

1.  It makes the comment superfluous.
2.  `git grep 0x7800` in the Linux source returns 1034 hits, so getting 
the macro name from the header `pci_ids.h` and then grepping for that is 
more straight forward.

I agree, that it breaks the style, and, if you agree, I could try to fix 
up the whole file.

>>   	{ PCI_VDEVICE(AMD, 0x7900), board_ahci }, /* AMD CZ */
>>   	{ PCI_VDEVICE(AMD, 0x7901), board_ahci_mobile }, /* AMD Green Sardine */
>>   	/* AMD is using RAID class only for ahci controllers */


Kind regards,

Paul

Re: [PATCH v3 2/3] ahci: Use macro PCI_DEVICE_ID_AMD_HUDSON2_SATA_IDE

[Damien Le Moal]

On 12/30/21 20:01, Paul Menzel wrote:
> Dear Damien,
> 
> 
> Am 30.12.21 um 03:13 schrieb Damien Le Moal:
>> On 12/30/21 01:11, Paul Menzel wrote:
>>> Use the defined macro from `include/linux/pci_ids.h`.
>>>
>>> Signed-off-by: Paul Menzel <pmenzel@molgen.mpg.de>
>>> ---
>>>   drivers/ata/ahci.c | 2 +-
>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
>>> index 1e1167e725a4..6a2432e4adda 100644
>>> --- a/drivers/ata/ahci.c
>>> +++ b/drivers/ata/ahci.c
>>> @@ -436,7 +436,7 @@ static const struct pci_device_id ahci_pci_tbl[] = {
>>>   		.class_mask = 0xffffff,
>>>   		board_ahci_al },
>>>   	/* AMD */
>>> -	{ PCI_VDEVICE(AMD, 0x7800), board_ahci }, /* AMD Hudson-2 */
>>> +	{ PCI_VDEVICE(AMD, PCI_DEVICE_ID_AMD_HUDSON2_SATA_IDE), board_ahci }, >
>> That breaks the style of the declarations here... And since
>> PCI_DEVICE_ID_AMD_HUDSON2_SATA_IDE is used only in drivers/pci/quirks.c,
>> I do not really see the point of this change.
> 
> 1.  It makes the comment superfluous.
> 2.  `git grep 0x7800` in the Linux source returns 1034 hits, so getting 
> the macro name from the header `pci_ids.h` and then grepping for that is 
> more straight forward.
> 
> I agree, that it breaks the style, and, if you agree, I could try to fix 
> up the whole file.

Arg, please no ! That would need defining a ton of PCI IDs for using
them only here. Let's not do that.

See Sergey and Hannes comment too. Let's keep the current style. Note
that with this change, AMD_HUDSON2_SATA_IDE PCI ID would be used both
here and in drivers/pci/quirks.c, so 2 places. But given that the ID
here is only for the table entry and used nowhere else, using the
numeral is better to preserve the style. So let's drop this patch.

> 
>>>   	{ PCI_VDEVICE(AMD, 0x7900), board_ahci }, /* AMD CZ */
>>>   	{ PCI_VDEVICE(AMD, 0x7901), board_ahci_mobile }, /* AMD Green Sardine */
>>>   	/* AMD is using RAID class only for ahci controllers */
> 
> 
> Kind regards,
> 
> Paul


-- 
Damien Le Moal
Western Digital Research

Re: [PATCH v3 2/3] ahci: Use macro PCI_DEVICE_ID_AMD_HUDSON2_SATA_IDE

[Hannes Reinecke]

On 12/29/21 5:11 PM, Paul Menzel wrote:
> Use the defined macro from `include/linux/pci_ids.h`.
> 
> Signed-off-by: Paul Menzel <pmenzel@molgen.mpg.de>
> ---
>   drivers/ata/ahci.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
> index 1e1167e725a4..6a2432e4adda 100644
> --- a/drivers/ata/ahci.c
> +++ b/drivers/ata/ahci.c
> @@ -436,7 +436,7 @@ static const struct pci_device_id ahci_pci_tbl[] = {
>   		.class_mask = 0xffffff,
>   		board_ahci_al },
>   	/* AMD */
> -	{ PCI_VDEVICE(AMD, 0x7800), board_ahci }, /* AMD Hudson-2 */
> +	{ PCI_VDEVICE(AMD, PCI_DEVICE_ID_AMD_HUDSON2_SATA_IDE), board_ahci },
>   	{ PCI_VDEVICE(AMD, 0x7900), board_ahci }, /* AMD CZ */
>   	{ PCI_VDEVICE(AMD, 0x7901), board_ahci_mobile }, /* AMD Green Sardine */
>   	/* AMD is using RAID class only for ahci controllers */
> 
Weelll ... there are defines for AMD Hudson-2 and similar in the pci_ids 
file, yet these definitions are not used here.
I'd vote keeping the style for all entries, ie either convert all 
entries here to use #defines, or stay with the numeral.

But we shouldn't mix them.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                Kernel Storage Architect
hare@suse.de                              +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Felix Imendörffer

[PATCH v3 3/3] ahci: AMD A85 FCH (Hudson D4): Skip 200 ms debounce delay in `sata_link_resume()`

[Paul Menzel]

Since the first commit 1da177e4c3 (Linux-2.6.12-rc2) in the Linux git
repository, `sata_link_resume()` contains a 200 ms delay with the comment
below.

>    /*
>     * Some PHYs react badly if SStatus is pounded
>     * immediately after resuming.  Delay 200ms before
>     * debouncing.
>     */

A lot of PHYs do not have that problem though, so delaying 200 ms increases
the boot time by 30 percent unnecessarily for a lot of systems, making
“instant booting” quite hard.

As it’s unknown for what PHY the delay was added, create a new board
`board_ahci_nodbdelay` with the link flag `ATA_LFLAG_NO_DB_DELAY,`, and,
for now, configure the AMD A85 FCH (Hudson D4) to use it.

On the ASUS F2A85-M PRO it reduces the Linux kernel boot time by the
expected 200 ms from 787 ms to 585 ms.

Tested on ASUS F2A85-M PRO:

Without patch, i. e., with 200 ms debounce delay:

    […]
    [    0.000000] DMI: ASUS F2A85-M_PRO/F2A85-M_PRO, BIOS 4.15-671-g7b043ef855 12/27/2021
    […]
    [    0.404885] ahci 0000:00:11.0: version 3.0
    [    0.405466] ahci 0000:00:11.0: AHCI 0001.0300 32 slots 8 ports 6 Gbps 0x40 impl SATA mode
    [    0.405470] ahci 0000:00:11.0: flags: 64bit ncq sntf ilck led clo pio
    [    0.408036] scsi host0: ahci
    [    0.408537] scsi host1: ahci
    [    0.408932] scsi host2: ahci
    [    0.409444] scsi host3: ahci
    [    0.409841] scsi host4: ahci
    [    0.410266] scsi host5: ahci
    [    0.410661] scsi host6: ahci
    [    0.411052] scsi host7: ahci
    [    0.411284] ata1: DUMMY
    [    0.411286] ata2: DUMMY
    [    0.411286] ata3: DUMMY
    [    0.411287] ata4: DUMMY
    [    0.411288] ata5: DUMMY
    [    0.411289] ata6: DUMMY
    [    0.411291] ata7: SATA max UDMA/133 abar m2048@0xf01cc000 port 0xf01cc400 irq 19
    [    0.411292] ata8: DUMMY
    […]
    [    0.422362] Key type encrypted registered
    [    0.424903] PM:   Magic number: 1:28:636
    [    0.723979] ata7: SATA link up 6.0 Gbps (SStatus 133 SControl 300)
    [    0.724268] ata7.00: ATA-9: SanDisk SDSSDP064G, 2.0.0, max UDMA/133
    [    0.724271] ata7.00: 125045424 sectors, multi 1: LBA48 NCQ (depth 32)
    [    0.725537] ata7.00: configured for UDMA/133
    [    0.725898] scsi 6:0:0:0: Direct-Access     ATA      SanDisk SDSSDP06 0    PQ: 0 ANSI: 5
    [    0.726428] sd 6:0:0:0: [sda] 125045424 512-byte logical blocks: (64.0 GB/59.6 GiB)
    [    0.726442] sd 6:0:0:0: [sda] Write Protect is off
    [    0.726446] sd 6:0:0:0: [sda] Mode Sense: 00 3a 00 00
    [    0.726464] sd 6:0:0:0: [sda] Write cache: enabled, read cache: enabled, doesn't support DPO or FUA
    [    0.727985]  sda: sda1 sda2 sda3
    [    0.728588] sd 6:0:0:0: [sda] Attached SCSI disk
    [    0.738495] EXT4-fs (sda3): mounted filesystem with ordered data mode. Opts: (null). Quota mode: none.
    […]
    [    0.786812] Run /sbin/init as init process

With patch, i. e., skipping the debounce delay saves 200 ms from the boot
as expected.

    […]
    [    0.000000] DMI: ASUS F2A85-M_PRO/F2A85-M_PRO, BIOS 4.15-671-g7b043ef855 12/27/2021
    […]
    [    0.407372] ahci 0000:00:11.0: version 3.0
    [    0.407909] ahci 0000:00:11.0: AHCI 0001.0300 32 slots 8 ports 6 Gbps 0x40 impl SATA mode
    [    0.407913] ahci 0000:00:11.0: flags: 64bit ncq sntf ilck led clo pio
    [    0.410520] scsi host0: ahci
    [    0.411017] scsi host1: ahci
    [    0.411418] scsi host2: ahci
    [    0.411810] scsi host3: ahci
    [    0.412225] scsi host4: ahci
    [    0.412614] scsi host5: ahci
    [    0.413005] scsi host6: ahci
    [    0.413488] scsi host7: ahci
    [    0.413713] ata1: DUMMY
    [    0.413715] ata2: DUMMY
    [    0.413716] ata3: DUMMY
    [    0.413716] ata4: DUMMY
    [    0.413717] ata5: DUMMY
    [    0.413718] ata6: DUMMY
    [    0.413720] ata7: SATA max UDMA/133 abar m2048@0xf01cc000 port 0xf01cc400 irq 19
    [    0.413722] ata8: DUMMY
    […]
    [    0.425414] Key type encrypted registered
    [    0.427873] PM:   Magic number: 1:234:838
    [    0.522131] ata7: SATA link up 6.0 Gbps (SStatus 133 SControl 300)
    [    0.522415] ata7.00: ATA-9: SanDisk SDSSDP064G, 2.0.0, max UDMA/133
    [    0.522418] ata7.00: 125045424 sectors, multi 1: LBA48 NCQ (depth 32)
    [    0.523636] ata7.00: configured for UDMA/133
    [    0.523993] scsi 6:0:0:0: Direct-Access     ATA      SanDisk SDSSDP06 0    PQ: 0 ANSI: 5
    [    0.524497] sd 6:0:0:0: [sda] 125045424 512-byte logical blocks: (64.0 GB/59.6 GiB)
    [    0.524511] sd 6:0:0:0: [sda] Write Protect is off
    [    0.524515] sd 6:0:0:0: [sda] Mode Sense: 00 3a 00 00
    [    0.524534] sd 6:0:0:0: [sda] Write cache: enabled, read cache: enabled, doesn't support DPO or FUA
    [    0.525953]  sda: sda1 sda2 sda3
    [    0.526541] sd 6:0:0:0: [sda] Attached SCSI disk
    [    0.536245] EXT4-fs (sda3): mounted filesystem with ordered data mode. Opts: (null). Quota mode: none.
    […]
    [    0.585327] Run /sbin/init as init process

Signed-off-by: Paul Menzel <pmenzel@molgen.mpg.de>
Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Cc: Guenter Roeck <groeck@chromium.org>

---

Add the two Chromium OS developers Dmitry and Guenter to Cc, as to my
knowledge Chromium/Chrome OS also tries to boot very fast, and the Chromium
project has some CI infrastructure.
---
 drivers/ata/ahci.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
index 6a2432e4adda..4f3e0603864d 100644
--- a/drivers/ata/ahci.c
+++ b/drivers/ata/ahci.c
@@ -51,6 +51,7 @@ enum board_ids {
 	board_ahci,
 	board_ahci_ign_iferr,
 	board_ahci_mobile,
+	board_ahci_nodbdelay,
 	board_ahci_nomsi,
 	board_ahci_noncq,
 	board_ahci_nosntf,
@@ -141,6 +142,13 @@ static const struct ata_port_info ahci_port_info[] = {
 		.udma_mask	= ATA_UDMA6,
 		.port_ops	= &ahci_ops,
 	},
+	[board_ahci_nodbdelay] = {
+		.flags		= AHCI_FLAG_COMMON,
+		.link_flags	= ATA_LFLAG_NO_DB_DELAY,
+		.pio_mask	= ATA_PIO4,
+		.udma_mask	= ATA_UDMA6,
+		.port_ops	= &ahci_ops,
+	},
 	[board_ahci_nomsi] = {
 		AHCI_HFLAGS	(AHCI_HFLAG_NO_MSI),
 		.flags		= AHCI_FLAG_COMMON,
@@ -437,6 +445,7 @@ static const struct pci_device_id ahci_pci_tbl[] = {
 		board_ahci_al },
 	/* AMD */
 	{ PCI_VDEVICE(AMD, PCI_DEVICE_ID_AMD_HUDSON2_SATA_IDE), board_ahci },
+	{ PCI_VDEVICE(AMD, PCI_DEVICE_ID_AMD_HUDSON2_SATA_AHCI), board_ahci_nodbdelay },
 	{ PCI_VDEVICE(AMD, 0x7900), board_ahci }, /* AMD CZ */
 	{ PCI_VDEVICE(AMD, 0x7901), board_ahci_mobile }, /* AMD Green Sardine */
 	/* AMD is using RAID class only for ahci controllers */
-- 
2.30.2

Re: [PATCH v3 3/3] ahci: AMD A85 FCH (Hudson D4): Skip 200 ms debounce delay in `sata_link_resume()`

[Damien Le Moal]

On 12/30/21 01:11, Paul Menzel wrote:
> Since the first commit 1da177e4c3 (Linux-2.6.12-rc2) in the Linux git
> repository, `sata_link_resume()` contains a 200 ms delay with the comment
> below.
> 
>>    /*
>>     * Some PHYs react badly if SStatus is pounded
>>     * immediately after resuming.  Delay 200ms before
>>     * debouncing.
>>     */

This is code comment so no need to "quote" this in the commit message
(line starting with '>'). Seeing the patch in an email client, it looks
weird :)

> 
> A lot of PHYs do not have that problem though, so delaying 200 ms increases
> the boot time by 30 percent unnecessarily for a lot of systems, making
> “instant booting” quite hard.
> 
> As it’s unknown for what PHY the delay was added, create a new board
> `board_ahci_nodbdelay` with the link flag `ATA_LFLAG_NO_DB_DELAY,`, and,
> for now, configure the AMD A85 FCH (Hudson D4) to use it.
> 
> On the ASUS F2A85-M PRO it reduces the Linux kernel boot time by the
> expected 200 ms from 787 ms to 585 ms.
> 
> Tested on ASUS F2A85-M PRO:
> 
> Without patch, i. e., with 200 ms debounce delay:
> 
>     […]
>     [    0.000000] DMI: ASUS F2A85-M_PRO/F2A85-M_PRO, BIOS 4.15-671-g7b043ef855 12/27/2021
>     […]
>     [    0.404885] ahci 0000:00:11.0: version 3.0
>     [    0.405466] ahci 0000:00:11.0: AHCI 0001.0300 32 slots 8 ports 6 Gbps 0x40 impl SATA mode
>     [    0.405470] ahci 0000:00:11.0: flags: 64bit ncq sntf ilck led clo pio
>     [    0.408036] scsi host0: ahci
>     [    0.408537] scsi host1: ahci
>     [    0.408932] scsi host2: ahci
>     [    0.409444] scsi host3: ahci
>     [    0.409841] scsi host4: ahci
>     [    0.410266] scsi host5: ahci
>     [    0.410661] scsi host6: ahci
>     [    0.411052] scsi host7: ahci
>     [    0.411284] ata1: DUMMY
>     [    0.411286] ata2: DUMMY
>     [    0.411286] ata3: DUMMY
>     [    0.411287] ata4: DUMMY
>     [    0.411288] ata5: DUMMY
>     [    0.411289] ata6: DUMMY
>     [    0.411291] ata7: SATA max UDMA/133 abar m2048@0xf01cc000 port 0xf01cc400 irq 19
>     [    0.411292] ata8: DUMMY
>     […]
>     [    0.422362] Key type encrypted registered
>     [    0.424903] PM:   Magic number: 1:28:636
>     [    0.723979] ata7: SATA link up 6.0 Gbps (SStatus 133 SControl 300)
>     [    0.724268] ata7.00: ATA-9: SanDisk SDSSDP064G, 2.0.0, max UDMA/133
>     [    0.724271] ata7.00: 125045424 sectors, multi 1: LBA48 NCQ (depth 32)
>     [    0.725537] ata7.00: configured for UDMA/133
>     [    0.725898] scsi 6:0:0:0: Direct-Access     ATA      SanDisk SDSSDP06 0    PQ: 0 ANSI: 5
>     [    0.726428] sd 6:0:0:0: [sda] 125045424 512-byte logical blocks: (64.0 GB/59.6 GiB)
>     [    0.726442] sd 6:0:0:0: [sda] Write Protect is off
>     [    0.726446] sd 6:0:0:0: [sda] Mode Sense: 00 3a 00 00
>     [    0.726464] sd 6:0:0:0: [sda] Write cache: enabled, read cache: enabled, doesn't support DPO or FUA
>     [    0.727985]  sda: sda1 sda2 sda3
>     [    0.728588] sd 6:0:0:0: [sda] Attached SCSI disk
>     [    0.738495] EXT4-fs (sda3): mounted filesystem with ordered data mode. Opts: (null). Quota mode: none.
>     […]
>     [    0.786812] Run /sbin/init as init process
> 
> With patch, i. e., skipping the debounce delay saves 200 ms from the boot
> as expected.
> 
>     […]
>     [    0.000000] DMI: ASUS F2A85-M_PRO/F2A85-M_PRO, BIOS 4.15-671-g7b043ef855 12/27/2021
>     […]
>     [    0.407372] ahci 0000:00:11.0: version 3.0
>     [    0.407909] ahci 0000:00:11.0: AHCI 0001.0300 32 slots 8 ports 6 Gbps 0x40 impl SATA mode
>     [    0.407913] ahci 0000:00:11.0: flags: 64bit ncq sntf ilck led clo pio
>     [    0.410520] scsi host0: ahci
>     [    0.411017] scsi host1: ahci
>     [    0.411418] scsi host2: ahci
>     [    0.411810] scsi host3: ahci
>     [    0.412225] scsi host4: ahci
>     [    0.412614] scsi host5: ahci
>     [    0.413005] scsi host6: ahci
>     [    0.413488] scsi host7: ahci
>     [    0.413713] ata1: DUMMY
>     [    0.413715] ata2: DUMMY
>     [    0.413716] ata3: DUMMY
>     [    0.413716] ata4: DUMMY
>     [    0.413717] ata5: DUMMY
>     [    0.413718] ata6: DUMMY
>     [    0.413720] ata7: SATA max UDMA/133 abar m2048@0xf01cc000 port 0xf01cc400 irq 19
>     [    0.413722] ata8: DUMMY
>     […]
>     [    0.425414] Key type encrypted registered
>     [    0.427873] PM:   Magic number: 1:234:838
>     [    0.522131] ata7: SATA link up 6.0 Gbps (SStatus 133 SControl 300)
>     [    0.522415] ata7.00: ATA-9: SanDisk SDSSDP064G, 2.0.0, max UDMA/133
>     [    0.522418] ata7.00: 125045424 sectors, multi 1: LBA48 NCQ (depth 32)
>     [    0.523636] ata7.00: configured for UDMA/133
>     [    0.523993] scsi 6:0:0:0: Direct-Access     ATA      SanDisk SDSSDP06 0    PQ: 0 ANSI: 5
>     [    0.524497] sd 6:0:0:0: [sda] 125045424 512-byte logical blocks: (64.0 GB/59.6 GiB)
>     [    0.524511] sd 6:0:0:0: [sda] Write Protect is off
>     [    0.524515] sd 6:0:0:0: [sda] Mode Sense: 00 3a 00 00
>     [    0.524534] sd 6:0:0:0: [sda] Write cache: enabled, read cache: enabled, doesn't support DPO or FUA
>     [    0.525953]  sda: sda1 sda2 sda3
>     [    0.526541] sd 6:0:0:0: [sda] Attached SCSI disk
>     [    0.536245] EXT4-fs (sda3): mounted filesystem with ordered data mode. Opts: (null). Quota mode: none.
>     […]
>     [    0.585327] Run /sbin/init as init process
> 
> Signed-off-by: Paul Menzel <pmenzel@molgen.mpg.de>
> Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> Cc: Guenter Roeck <groeck@chromium.org>
> 
> ---
> 
> Add the two Chromium OS developers Dmitry and Guenter to Cc, as to my
> knowledge Chromium/Chrome OS also tries to boot very fast, and the Chromium
> project has some CI infrastructure.
> ---
>  drivers/ata/ahci.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
> index 6a2432e4adda..4f3e0603864d 100644
> --- a/drivers/ata/ahci.c
> +++ b/drivers/ata/ahci.c
> @@ -51,6 +51,7 @@ enum board_ids {
>  	board_ahci,
>  	board_ahci_ign_iferr,
>  	board_ahci_mobile,
> +	board_ahci_nodbdelay,

The "nodb" naming is not super clear...
Maybe change the name to:

board_ahci_no_debounce_delay

or

board_ahci_no_resume_delay

?

Longer, but clearer.

>  	board_ahci_nomsi,
>  	board_ahci_noncq,
>  	board_ahci_nosntf,
> @@ -141,6 +142,13 @@ static const struct ata_port_info ahci_port_info[] = {
>  		.udma_mask	= ATA_UDMA6,
>  		.port_ops	= &ahci_ops,
>  	},
> +	[board_ahci_nodbdelay] = {
> +		.flags		= AHCI_FLAG_COMMON,
> +		.link_flags	= ATA_LFLAG_NO_DB_DELAY,
> +		.pio_mask	= ATA_PIO4,
> +		.udma_mask	= ATA_UDMA6,
> +		.port_ops	= &ahci_ops,
> +	},
>  	[board_ahci_nomsi] = {
>  		AHCI_HFLAGS	(AHCI_HFLAG_NO_MSI),
>  		.flags		= AHCI_FLAG_COMMON,
> @@ -437,6 +445,7 @@ static const struct pci_device_id ahci_pci_tbl[] = {
>  		board_ahci_al },
>  	/* AMD */
>  	{ PCI_VDEVICE(AMD, PCI_DEVICE_ID_AMD_HUDSON2_SATA_IDE), board_ahci },
> +	{ PCI_VDEVICE(AMD, PCI_DEVICE_ID_AMD_HUDSON2_SATA_AHCI), board_ahci_nodbdelay },

Patch 1 introduces this macro in pci_ids.h, but it is used only here. So
to keep with the current style in this structure, drop the macro (so
drop patch 1).

>  	{ PCI_VDEVICE(AMD, 0x7900), board_ahci }, /* AMD CZ */
>  	{ PCI_VDEVICE(AMD, 0x7901), board_ahci_mobile }, /* AMD Green Sardine */
>  	/* AMD is using RAID class only for ahci controllers */


-- 
Damien Le Moal
Western Digital Research

Re: [PATCH v3 3/3] ahci: AMD A85 FCH (Hudson D4): Skip 200 ms debounce delay in `sata_link_resume()`

[Paul Menzel]

Dear Damien,


Am 30.12.21 um 03:19 schrieb Damien Le Moal:
> On 12/30/21 01:11, Paul Menzel wrote:
>> Since the first commit 1da177e4c3 (Linux-2.6.12-rc2) in the Linux git
>> repository, `sata_link_resume()` contains a 200 ms delay with the comment
>> below.
>>
>>>     /*
>>>      * Some PHYs react badly if SStatus is pounded
>>>      * immediately after resuming.  Delay 200ms before
>>>      * debouncing.
>>>      */
> 
> This is code comment so no need to "quote" this in the commit message
> (line starting with '>'). Seeing the patch in an email client, it looks
> weird :)

Alright.

>> A lot of PHYs do not have that problem though, so delaying 200 ms increases
>> the boot time by 30 percent unnecessarily for a lot of systems, making
>> “instant booting” quite hard.
>>
>> As it’s unknown for what PHY the delay was added, create a new board
>> `board_ahci_nodbdelay` with the link flag `ATA_LFLAG_NO_DB_DELAY,`, and,
>> for now, configure the AMD A85 FCH (Hudson D4) to use it.
>>
>> On the ASUS F2A85-M PRO it reduces the Linux kernel boot time by the
>> expected 200 ms from 787 ms to 585 ms.
>>
>> Tested on ASUS F2A85-M PRO:
>>
>> Without patch, i. e., with 200 ms debounce delay:
>>
>>      […]
>>      [    0.000000] DMI: ASUS F2A85-M_PRO/F2A85-M_PRO, BIOS 4.15-671-g7b043ef855 12/27/2021
>>      […]
>>      [    0.404885] ahci 0000:00:11.0: version 3.0
>>      [    0.405466] ahci 0000:00:11.0: AHCI 0001.0300 32 slots 8 ports 6 Gbps 0x40 impl SATA mode
>>      [    0.405470] ahci 0000:00:11.0: flags: 64bit ncq sntf ilck led clo pio
>>      [    0.408036] scsi host0: ahci
>>      [    0.408537] scsi host1: ahci
>>      [    0.408932] scsi host2: ahci
>>      [    0.409444] scsi host3: ahci
>>      [    0.409841] scsi host4: ahci
>>      [    0.410266] scsi host5: ahci
>>      [    0.410661] scsi host6: ahci
>>      [    0.411052] scsi host7: ahci
>>      [    0.411284] ata1: DUMMY
>>      [    0.411286] ata2: DUMMY
>>      [    0.411286] ata3: DUMMY
>>      [    0.411287] ata4: DUMMY
>>      [    0.411288] ata5: DUMMY
>>      [    0.411289] ata6: DUMMY
>>      [    0.411291] ata7: SATA max UDMA/133 abar m2048@0xf01cc000 port 0xf01cc400 irq 19
>>      [    0.411292] ata8: DUMMY
>>      […]
>>      [    0.422362] Key type encrypted registered
>>      [    0.424903] PM:   Magic number: 1:28:636
>>      [    0.723979] ata7: SATA link up 6.0 Gbps (SStatus 133 SControl 300)
>>      [    0.724268] ata7.00: ATA-9: SanDisk SDSSDP064G, 2.0.0, max UDMA/133
>>      [    0.724271] ata7.00: 125045424 sectors, multi 1: LBA48 NCQ (depth 32)
>>      [    0.725537] ata7.00: configured for UDMA/133
>>      [    0.725898] scsi 6:0:0:0: Direct-Access     ATA      SanDisk SDSSDP06 0    PQ: 0 ANSI: 5
>>      [    0.726428] sd 6:0:0:0: [sda] 125045424 512-byte logical blocks: (64.0 GB/59.6 GiB)
>>      [    0.726442] sd 6:0:0:0: [sda] Write Protect is off
>>      [    0.726446] sd 6:0:0:0: [sda] Mode Sense: 00 3a 00 00
>>      [    0.726464] sd 6:0:0:0: [sda] Write cache: enabled, read cache: enabled, doesn't support DPO or FUA
>>      [    0.727985]  sda: sda1 sda2 sda3
>>      [    0.728588] sd 6:0:0:0: [sda] Attached SCSI disk
>>      [    0.738495] EXT4-fs (sda3): mounted filesystem with ordered data mode. Opts: (null). Quota mode: none.
>>      […]
>>      [    0.786812] Run /sbin/init as init process
>>
>> With patch, i. e., skipping the debounce delay saves 200 ms from the boot
>> as expected.
>>
>>      […]
>>      [    0.000000] DMI: ASUS F2A85-M_PRO/F2A85-M_PRO, BIOS 4.15-671-g7b043ef855 12/27/2021
>>      […]
>>      [    0.407372] ahci 0000:00:11.0: version 3.0
>>      [    0.407909] ahci 0000:00:11.0: AHCI 0001.0300 32 slots 8 ports 6 Gbps 0x40 impl SATA mode
>>      [    0.407913] ahci 0000:00:11.0: flags: 64bit ncq sntf ilck led clo pio
>>      [    0.410520] scsi host0: ahci
>>      [    0.411017] scsi host1: ahci
>>      [    0.411418] scsi host2: ahci
>>      [    0.411810] scsi host3: ahci
>>      [    0.412225] scsi host4: ahci
>>      [    0.412614] scsi host5: ahci
>>      [    0.413005] scsi host6: ahci
>>      [    0.413488] scsi host7: ahci
>>      [    0.413713] ata1: DUMMY
>>      [    0.413715] ata2: DUMMY
>>      [    0.413716] ata3: DUMMY
>>      [    0.413716] ata4: DUMMY
>>      [    0.413717] ata5: DUMMY
>>      [    0.413718] ata6: DUMMY
>>      [    0.413720] ata7: SATA max UDMA/133 abar m2048@0xf01cc000 port 0xf01cc400 irq 19
>>      [    0.413722] ata8: DUMMY
>>      […]
>>      [    0.425414] Key type encrypted registered
>>      [    0.427873] PM:   Magic number: 1:234:838
>>      [    0.522131] ata7: SATA link up 6.0 Gbps (SStatus 133 SControl 300)
>>      [    0.522415] ata7.00: ATA-9: SanDisk SDSSDP064G, 2.0.0, max UDMA/133
>>      [    0.522418] ata7.00: 125045424 sectors, multi 1: LBA48 NCQ (depth 32)
>>      [    0.523636] ata7.00: configured for UDMA/133
>>      [    0.523993] scsi 6:0:0:0: Direct-Access     ATA      SanDisk SDSSDP06 0    PQ: 0 ANSI: 5
>>      [    0.524497] sd 6:0:0:0: [sda] 125045424 512-byte logical blocks: (64.0 GB/59.6 GiB)
>>      [    0.524511] sd 6:0:0:0: [sda] Write Protect is off
>>      [    0.524515] sd 6:0:0:0: [sda] Mode Sense: 00 3a 00 00
>>      [    0.524534] sd 6:0:0:0: [sda] Write cache: enabled, read cache: enabled, doesn't support DPO or FUA
>>      [    0.525953]  sda: sda1 sda2 sda3
>>      [    0.526541] sd 6:0:0:0: [sda] Attached SCSI disk
>>      [    0.536245] EXT4-fs (sda3): mounted filesystem with ordered data mode. Opts: (null). Quota mode: none.
>>      […]
>>      [    0.585327] Run /sbin/init as init process
>>
>> Signed-off-by: Paul Menzel <pmenzel@molgen.mpg.de>
>> Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
>> Cc: Guenter Roeck <groeck@chromium.org>
>>
>> ---
>>
>> Add the two Chromium OS developers Dmitry and Guenter to Cc, as to my
>> knowledge Chromium/Chrome OS also tries to boot very fast, and the Chromium
>> project has some CI infrastructure.
>> ---
>>   drivers/ata/ahci.c | 9 +++++++++
>>   1 file changed, 9 insertions(+)
>>
>> diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
>> index 6a2432e4adda..4f3e0603864d 100644
>> --- a/drivers/ata/ahci.c
>> +++ b/drivers/ata/ahci.c
>> @@ -51,6 +51,7 @@ enum board_ids {
>>   	board_ahci,
>>   	board_ahci_ign_iferr,
>>   	board_ahci_mobile,
>> +	board_ahci_nodbdelay,
> 
> The "nodb" naming is not super clear...
> Maybe change the name to:
> 
> board_ahci_no_debounce_delay
> 
> or
> 
> board_ahci_no_resume_delay
> 
> ?
> 
> Longer, but clearer.

I agree. I followed the flag name `ATA_LFLAG_NO_DB_DELAY`.

>>   	board_ahci_nomsi,
>>   	board_ahci_noncq,
>>   	board_ahci_nosntf,
>> @@ -141,6 +142,13 @@ static const struct ata_port_info ahci_port_info[] = {
>>   		.udma_mask	= ATA_UDMA6,
>>   		.port_ops	= &ahci_ops,
>>   	},
>> +	[board_ahci_nodbdelay] = {
>> +		.flags		= AHCI_FLAG_COMMON,
>> +		.link_flags	= ATA_LFLAG_NO_DB_DELAY,
>> +		.pio_mask	= ATA_PIO4,
>> +		.udma_mask	= ATA_UDMA6,
>> +		.port_ops	= &ahci_ops,
>> +	},
>>   	[board_ahci_nomsi] = {
>>   		AHCI_HFLAGS	(AHCI_HFLAG_NO_MSI),
>>   		.flags		= AHCI_FLAG_COMMON,
>> @@ -437,6 +445,7 @@ static const struct pci_device_id ahci_pci_tbl[] = {
>>   		board_ahci_al },
>>   	/* AMD */
>>   	{ PCI_VDEVICE(AMD, PCI_DEVICE_ID_AMD_HUDSON2_SATA_IDE), board_ahci },
>> +	{ PCI_VDEVICE(AMD, PCI_DEVICE_ID_AMD_HUDSON2_SATA_AHCI), board_ahci_nodbdelay },
> 
> Patch 1 introduces this macro in pci_ids.h, but it is used only here. So
> to keep with the current style in this structure, drop the macro (so
> drop patch 1).

I wait for your answer of the second patch, and then I am going to sent v4.

>>   	{ PCI_VDEVICE(AMD, 0x7900), board_ahci }, /* AMD CZ */
>>   	{ PCI_VDEVICE(AMD, 0x7901), board_ahci_mobile }, /* AMD Green Sardine */
>>   	/* AMD is using RAID class only for ahci controllers */

Do you have a AHCI device at hand, where you could also test if 
everything works fine without the delay?


Kind regards,

Paul

Re: [PATCH v3 3/3] ahci: AMD A85 FCH (Hudson D4): Skip 200 ms debounce delay in `sata_link_resume()`

[Damien Le Moal]

On 12/30/21 20:08, Paul Menzel wrote:
>>>   	board_ahci_nomsi,
>>>   	board_ahci_noncq,
>>>   	board_ahci_nosntf,
>>> @@ -141,6 +142,13 @@ static const struct ata_port_info ahci_port_info[] = {
>>>   		.udma_mask	= ATA_UDMA6,
>>>   		.port_ops	= &ahci_ops,
>>>   	},
>>> +	[board_ahci_nodbdelay] = {
>>> +		.flags		= AHCI_FLAG_COMMON,
>>> +		.link_flags	= ATA_LFLAG_NO_DB_DELAY,
>>> +		.pio_mask	= ATA_PIO4,
>>> +		.udma_mask	= ATA_UDMA6,
>>> +		.port_ops	= &ahci_ops,
>>> +	},
>>>   	[board_ahci_nomsi] = {
>>>   		AHCI_HFLAGS	(AHCI_HFLAG_NO_MSI),
>>>   		.flags		= AHCI_FLAG_COMMON,
>>> @@ -437,6 +445,7 @@ static const struct pci_device_id ahci_pci_tbl[] = {
>>>   		board_ahci_al },
>>>   	/* AMD */
>>>   	{ PCI_VDEVICE(AMD, PCI_DEVICE_ID_AMD_HUDSON2_SATA_IDE), board_ahci },
>>> +	{ PCI_VDEVICE(AMD, PCI_DEVICE_ID_AMD_HUDSON2_SATA_AHCI), board_ahci_nodbdelay },
>>
>> Patch 1 introduces this macro in pci_ids.h, but it is used only here. So
>> to keep with the current style in this structure, drop the macro (so
>> drop patch 1).
> 
> I wait for your answer of the second patch, and then I am going to sent v4.

Let's use the numeric value. No macro definition needed.

> 
>>>   	{ PCI_VDEVICE(AMD, 0x7900), board_ahci }, /* AMD CZ */
>>>   	{ PCI_VDEVICE(AMD, 0x7901), board_ahci_mobile }, /* AMD Green Sardine */
>>>   	/* AMD is using RAID class only for ahci controllers */
> 
> Do you have a AHCI device at hand, where you could also test if 
> everything works fine without the delay?

Unfortunately, I do not have any board with this adapter.

Cheers.


-- 
Damien Le Moal
Western Digital Research

Re: [PATCH v3 3/3] ahci: AMD A85 FCH (Hudson D4): Skip 200 ms debounce delay in `sata_link_resume()`

[Paul Menzel]

Dear Damien,


Am 31.12.21 um 01:52 schrieb Damien Le Moal:
> On 12/30/21 20:08, Paul Menzel wrote:
>>>>    	board_ahci_nomsi,
>>>>    	board_ahci_noncq,
>>>>    	board_ahci_nosntf,
>>>> @@ -141,6 +142,13 @@ static const struct ata_port_info ahci_port_info[] = {
>>>>    		.udma_mask	= ATA_UDMA6,
>>>>    		.port_ops	= &ahci_ops,
>>>>    	},
>>>> +	[board_ahci_nodbdelay] = {
>>>> +		.flags		= AHCI_FLAG_COMMON,
>>>> +		.link_flags	= ATA_LFLAG_NO_DB_DELAY,
>>>> +		.pio_mask	= ATA_PIO4,
>>>> +		.udma_mask	= ATA_UDMA6,
>>>> +		.port_ops	= &ahci_ops,
>>>> +	},
>>>>    	[board_ahci_nomsi] = {
>>>>    		AHCI_HFLAGS	(AHCI_HFLAG_NO_MSI),
>>>>    		.flags		= AHCI_FLAG_COMMON,
>>>> @@ -437,6 +445,7 @@ static const struct pci_device_id ahci_pci_tbl[] = {
>>>>    		board_ahci_al },
>>>>    	/* AMD */
>>>>    	{ PCI_VDEVICE(AMD, PCI_DEVICE_ID_AMD_HUDSON2_SATA_IDE), board_ahci },
>>>> +	{ PCI_VDEVICE(AMD, PCI_DEVICE_ID_AMD_HUDSON2_SATA_AHCI), board_ahci_nodbdelay },
>>>
>>> Patch 1 introduces this macro in pci_ids.h, but it is used only here. So
>>> to keep with the current style in this structure, drop the macro (so
>>> drop patch 1).
>>
>> I wait for your answer of the second patch, and then I am going to sent v4.
> 
> Let's use the numeric value. No macro definition needed.

Alright. I am going to follow the maintainers wishes.

>>>>    	{ PCI_VDEVICE(AMD, 0x7900), board_ahci }, /* AMD CZ */
>>>>    	{ PCI_VDEVICE(AMD, 0x7901), board_ahci_mobile }, /* AMD Green Sardine */
>>>>    	/* AMD is using RAID class only for ahci controllers */
>>
>> Do you have a AHCI device at hand, where you could also test if
>> everything works fine without the delay?
> 
> Unfortunately, I do not have any board with this adapter.

Sorry, we misunderstand each other. (I wrote a reply to my own patch [1].)

I think the delay is not necessary for any modern AHCI controller. It’d 
be great, if you could test, if it’s also true on the systems you have 
by just skipping the delay.


Kind regards,

Paul


[1]: 
https://lore.kernel.org/linux-ide/20211227162658.11314-2-pmenzel@molgen.mpg.de/T/#m697d2121463a4c946730e6b83940e12d6d7e6700

Re: [PATCH v3 3/3] ahci: AMD A85 FCH (Hudson D4): Skip 200 ms debounce delay in `sata_link_resume()`

[Damien Le Moal]

On 12/31/21 16:08, Paul Menzel wrote:
> Dear Damien,
> 
> 
> Am 31.12.21 um 01:52 schrieb Damien Le Moal:
>> On 12/30/21 20:08, Paul Menzel wrote:
>>>>>    	board_ahci_nomsi,
>>>>>    	board_ahci_noncq,
>>>>>    	board_ahci_nosntf,
>>>>> @@ -141,6 +142,13 @@ static const struct ata_port_info ahci_port_info[] = {
>>>>>    		.udma_mask	= ATA_UDMA6,
>>>>>    		.port_ops	= &ahci_ops,
>>>>>    	},
>>>>> +	[board_ahci_nodbdelay] = {
>>>>> +		.flags		= AHCI_FLAG_COMMON,
>>>>> +		.link_flags	= ATA_LFLAG_NO_DB_DELAY,
>>>>> +		.pio_mask	= ATA_PIO4,
>>>>> +		.udma_mask	= ATA_UDMA6,
>>>>> +		.port_ops	= &ahci_ops,
>>>>> +	},
>>>>>    	[board_ahci_nomsi] = {
>>>>>    		AHCI_HFLAGS	(AHCI_HFLAG_NO_MSI),
>>>>>    		.flags		= AHCI_FLAG_COMMON,
>>>>> @@ -437,6 +445,7 @@ static const struct pci_device_id ahci_pci_tbl[] = {
>>>>>    		board_ahci_al },
>>>>>    	/* AMD */
>>>>>    	{ PCI_VDEVICE(AMD, PCI_DEVICE_ID_AMD_HUDSON2_SATA_IDE), board_ahci },
>>>>> +	{ PCI_VDEVICE(AMD, PCI_DEVICE_ID_AMD_HUDSON2_SATA_AHCI), board_ahci_nodbdelay },
>>>>
>>>> Patch 1 introduces this macro in pci_ids.h, but it is used only here. So
>>>> to keep with the current style in this structure, drop the macro (so
>>>> drop patch 1).
>>>
>>> I wait for your answer of the second patch, and then I am going to sent v4.
>>
>> Let's use the numeric value. No macro definition needed.
> 
> Alright. I am going to follow the maintainers wishes.
> 
>>>>>    	{ PCI_VDEVICE(AMD, 0x7900), board_ahci }, /* AMD CZ */
>>>>>    	{ PCI_VDEVICE(AMD, 0x7901), board_ahci_mobile }, /* AMD Green Sardine */
>>>>>    	/* AMD is using RAID class only for ahci controllers */
>>>
>>> Do you have a AHCI device at hand, where you could also test if
>>> everything works fine without the delay?
>>
>> Unfortunately, I do not have any board with this adapter.
> 
> Sorry, we misunderstand each other. (I wrote a reply to my own patch [1].)
> 
> I think the delay is not necessary for any modern AHCI controller. It’d 
> be great, if you could test, if it’s also true on the systems you have 
> by just skipping the delay.

I need to figure out how to safely test suspend/resume remotely (working
from home) :)

It would indeed be great to have the default as "no delay on resume" and
add the delay only for chipsets that need it. However, it is unclear
which chipset need the delay, right ? So I think we are stuck with
switching chipsets to "no delay" one by one by testing. Once the
majority of drivers are converted, we can reverse the default to be "no
delay" and mark untested drivers as needing the delay.

> 
> 
> Kind regards,
> 
> Paul
> 
> 
> [1]: 
> https://lore.kernel.org/linux-ide/20211227162658.11314-2-pmenzel@molgen.mpg.de/T/#m697d2121463a4c946730e6b83940e12d6d7e6700


-- 
Damien Le Moal
Western Digital Research

Re: [PATCH v3 3/3] ahci: AMD A85 FCH (Hudson D4): Skip 200 ms debounce delay in `sata_link_resume()`

[Paul Menzel]

[cc: -dmitry, -guenter]

Dear Damien,


Am 04.01.22 um 09:36 schrieb Damien Le Moal:
> On 12/31/21 16:08, Paul Menzel wrote:

>> Am 31.12.21 um 01:52 schrieb Damien Le Moal:
>>> On 12/30/21 20:08, Paul Menzel wrote:
>>>>>>     	board_ahci_nomsi,
>>>>>>     	board_ahci_noncq,
>>>>>>     	board_ahci_nosntf,
>>>>>> @@ -141,6 +142,13 @@ static const struct ata_port_info ahci_port_info[] = {
>>>>>>     		.udma_mask	= ATA_UDMA6,
>>>>>>     		.port_ops	= &ahci_ops,
>>>>>>     	},
>>>>>> +	[board_ahci_nodbdelay] = {
>>>>>> +		.flags		= AHCI_FLAG_COMMON,
>>>>>> +		.link_flags	= ATA_LFLAG_NO_DB_DELAY,
>>>>>> +		.pio_mask	= ATA_PIO4,
>>>>>> +		.udma_mask	= ATA_UDMA6,
>>>>>> +		.port_ops	= &ahci_ops,
>>>>>> +	},
>>>>>>     	[board_ahci_nomsi] = {
>>>>>>     		AHCI_HFLAGS	(AHCI_HFLAG_NO_MSI),
>>>>>>     		.flags		= AHCI_FLAG_COMMON,
>>>>>> @@ -437,6 +445,7 @@ static const struct pci_device_id ahci_pci_tbl[] = {
>>>>>>     		board_ahci_al },
>>>>>>     	/* AMD */
>>>>>>     	{ PCI_VDEVICE(AMD, PCI_DEVICE_ID_AMD_HUDSON2_SATA_IDE), board_ahci },
>>>>>> +	{ PCI_VDEVICE(AMD, PCI_DEVICE_ID_AMD_HUDSON2_SATA_AHCI), board_ahci_nodbdelay },
>>>>>
>>>>> Patch 1 introduces this macro in pci_ids.h, but it is used only here. So
>>>>> to keep with the current style in this structure, drop the macro (so
>>>>> drop patch 1).
>>>>
>>>> I wait for your answer of the second patch, and then I am going to sent v4.
>>>
>>> Let's use the numeric value. No macro definition needed.
>>
>> Alright. I am going to follow the maintainers wishes.
>>
>>>>>>     	{ PCI_VDEVICE(AMD, 0x7900), board_ahci }, /* AMD CZ */
>>>>>>     	{ PCI_VDEVICE(AMD, 0x7901), board_ahci_mobile }, /* AMD Green Sardine */
>>>>>>     	/* AMD is using RAID class only for ahci controllers */
>>>>
>>>> Do you have a AHCI device at hand, where you could also test if
>>>> everything works fine without the delay?
>>>
>>> Unfortunately, I do not have any board with this adapter.
>>
>> Sorry, we misunderstand each other. (I wrote a reply to my own patch [1].)
>>
>> I think the delay is not necessary for any modern AHCI controller. It’d
>> be great, if you could test, if it’s also true on the systems you have
>> by just skipping the delay.
> 
> I need to figure out how to safely test suspend/resume remotely (working
> from home) :)

Please note, I tested the cold bootup, where `sata_link_resume()` is 
also run.

> It would indeed be great to have the default as "no delay on resume" and
> add the delay only for chipsets that need it. However, it is unclear
> which chipset need the delay, right?

Yes, it’s unclear for what chipset (PHY?) it was added, as the git 
history i not available in the repository, and I have not found it yet.

> So I think we are stuck with switching chipsets to "no delay" one by
> one by testing. Once the majority of drivers are converted, we can
> reverse the default to be "no delay" and mark untested drivers as
> needing the delay.

For easy testing, a new CLI parameter to skip the delay might be handy.


Kind regards,

Paul


>> [1]: https://lore.kernel.org/linux-ide/20211227162658.11314-2-pmenzel@molgen.mpg.de/T/#m697d2121463a4c946730e6b83940e12d6d7e6700

Re: [PATCH v3 3/3] ahci: AMD A85 FCH (Hudson D4): Skip 200 ms debounce delay in `sata_link_resume()`

[Damien Le Moal]

On 1/4/22 17:49, Paul Menzel wrote:
> [cc: -dmitry, -guenter]
> 
> Dear Damien,
> 
> 
> Am 04.01.22 um 09:36 schrieb Damien Le Moal:
>> On 12/31/21 16:08, Paul Menzel wrote:
> 
>>> Am 31.12.21 um 01:52 schrieb Damien Le Moal:
>>>> On 12/30/21 20:08, Paul Menzel wrote:
>>>>>>>     	board_ahci_nomsi,
>>>>>>>     	board_ahci_noncq,
>>>>>>>     	board_ahci_nosntf,
>>>>>>> @@ -141,6 +142,13 @@ static const struct ata_port_info ahci_port_info[] = {
>>>>>>>     		.udma_mask	= ATA_UDMA6,
>>>>>>>     		.port_ops	= &ahci_ops,
>>>>>>>     	},
>>>>>>> +	[board_ahci_nodbdelay] = {
>>>>>>> +		.flags		= AHCI_FLAG_COMMON,
>>>>>>> +		.link_flags	= ATA_LFLAG_NO_DB_DELAY,
>>>>>>> +		.pio_mask	= ATA_PIO4,
>>>>>>> +		.udma_mask	= ATA_UDMA6,
>>>>>>> +		.port_ops	= &ahci_ops,
>>>>>>> +	},
>>>>>>>     	[board_ahci_nomsi] = {
>>>>>>>     		AHCI_HFLAGS	(AHCI_HFLAG_NO_MSI),
>>>>>>>     		.flags		= AHCI_FLAG_COMMON,
>>>>>>> @@ -437,6 +445,7 @@ static const struct pci_device_id ahci_pci_tbl[] = {
>>>>>>>     		board_ahci_al },
>>>>>>>     	/* AMD */
>>>>>>>     	{ PCI_VDEVICE(AMD, PCI_DEVICE_ID_AMD_HUDSON2_SATA_IDE), board_ahci },
>>>>>>> +	{ PCI_VDEVICE(AMD, PCI_DEVICE_ID_AMD_HUDSON2_SATA_AHCI), board_ahci_nodbdelay },
>>>>>>
>>>>>> Patch 1 introduces this macro in pci_ids.h, but it is used only here. So
>>>>>> to keep with the current style in this structure, drop the macro (so
>>>>>> drop patch 1).
>>>>>
>>>>> I wait for your answer of the second patch, and then I am going to sent v4.
>>>>
>>>> Let's use the numeric value. No macro definition needed.
>>>
>>> Alright. I am going to follow the maintainers wishes.
>>>
>>>>>>>     	{ PCI_VDEVICE(AMD, 0x7900), board_ahci }, /* AMD CZ */
>>>>>>>     	{ PCI_VDEVICE(AMD, 0x7901), board_ahci_mobile }, /* AMD Green Sardine */
>>>>>>>     	/* AMD is using RAID class only for ahci controllers */
>>>>>
>>>>> Do you have a AHCI device at hand, where you could also test if
>>>>> everything works fine without the delay?
>>>>
>>>> Unfortunately, I do not have any board with this adapter.
>>>
>>> Sorry, we misunderstand each other. (I wrote a reply to my own patch [1].)
>>>
>>> I think the delay is not necessary for any modern AHCI controller. It’d
>>> be great, if you could test, if it’s also true on the systems you have
>>> by just skipping the delay.
>>
>> I need to figure out how to safely test suspend/resume remotely (working
>> from home) :)
> 
> Please note, I tested the cold bootup, where `sata_link_resume()` is 
> also run.

OK. So it should be easy to test. Will try to have a look.

> 
>> It would indeed be great to have the default as "no delay on resume" and
>> add the delay only for chipsets that need it. However, it is unclear
>> which chipset need the delay, right?
> 
> Yes, it’s unclear for what chipset (PHY?) it was added, as the git 
> history i not available in the repository, and I have not found it yet.
> 
>> So I think we are stuck with switching chipsets to "no delay" one by
>> one by testing. Once the majority of drivers are converted, we can
>> reverse the default to be "no delay" and mark untested drivers as
>> needing the delay.
> 
> For easy testing, a new CLI parameter to skip the delay might be handy.

You mean a sysfs attribute may be ?
I am not sure it would help: on resume, the sysfs attributes would be
recreated and get the default value, not a new one.

> 
> 
> Kind regards,
> 
> Paul
> 
> 
>>> [1]: https://lore.kernel.org/linux-ide/20211227162658.11314-2-pmenzel@molgen.mpg.de/T/#m697d2121463a4c946730e6b83940e12d6d7e6700


-- 
Damien Le Moal
Western Digital Research

ata: For what PHY was debounce delay introduced? (was: [PATCH v3 3/3] ahci: AMD A85 FCH (Hudson D4): Skip 200 ms debounce delay in `sata_link_resume()`)

[Paul Menzel]

[cc: +jeff, +tejun]

Dear Damien,


Am 04.01.22 um 10:08 schrieb Damien Le Moal:
> On 1/4/22 17:49, Paul Menzel wrote:
>> [cc: -dmitry, -guenter]

>> Am 04.01.22 um 09:36 schrieb Damien Le Moal:
>>> On 12/31/21 16:08, Paul Menzel wrote:
>>
>>>> Am 31.12.21 um 01:52 schrieb Damien Le Moal:
>>>>> On 12/30/21 20:08, Paul Menzel wrote:

[…]

Jeff, Tejun, in `drivers/ata/libata-sata.c` contains a 200 ms sleep 
delaying the boot noticeably for optimized setups, where Linux takes 
less than 500 ms to start:

```
int sata_link_resume(struct ata_link *link, const unsigned long *params,
                      unsigned long deadline)
{
[…]
         /*
          * Writes to SControl sometimes get ignored under certain
          * controllers (ata_piix SIDPR).  Make sure DET actually is
          * cleared.
          */
         do {
                 scontrol = (scontrol & 0x0f0) | 0x300;
                 if ((rc = sata_scr_write(link, SCR_CONTROL, scontrol)))
                         return rc;
                 /*
                  * Some PHYs react badly if SStatus is pounded
                  * immediately after resuming.  Delay 200ms before
                  * debouncing.
                  */
                 if (!(link->flags & ATA_LFLAG_NO_DB_DELAY))
                         ata_msleep(link->ap, 200);

                 /* is SControl restored correctly? */
                 if ((rc = sata_scr_read(link, SCR_CONTROL, &scontrol)))
                         return rc;
         } while ((scontrol & 0xf0f) != 0x300 && --tries);
[…]
}
```

>>> It would indeed be great to have the default as "no delay on resume" and
>>> add the delay only for chipsets that need it. However, it is unclear
>>> which chipset need the delay, right?
>>
>> Yes, it’s unclear for what chipset (PHY?) it was added, as the git
>> history i not available in the repository, and I have not found it yet.

I found the historical git archive [2], and Jeff’s commit 4effb658a0 
from October 2003 [3] with the commit message

> [libata] Merge Serial ATA core, and drivers for:
> 
> Intel ICH5 (production)
> ServerWorks / Apple K2 (beta)
> VIA (beta)
> Silicon Image 3112 (broken!)
> Various Promise (alpha/beta)

adds the code below:

```
void sata_phy_reset(struct ata_port *ap)
{
[…]
	/* wait for phy to become ready, if necessary */
	do {
		msleep(200);
		sstatus = scr_read(ap, SCR_STATUS);
		if ((sstatus & 0xf) != 1)
			break;
	} while (time_before(jiffies, timeout));
[…]
}
```

Later on Tejun refactored the code in commit d7bb4cc75759 ([PATCH] 
libata-hp-prep: implement sata_phy_debounce()) [4], and clarified the 
comment.

(Sorry, if I mis-analyzed anything.)

Jeff, Tejun, do you know, what chipsets/PHYs had trouble with being 
queried right away? Only ata_piix?

>>> So I think we are stuck with switching chipsets to "no delay" one by
>>> one by testing. Once the majority of drivers are converted, we can
>>> reverse the default to be "no delay" and mark untested drivers as
>>> needing the delay.
>>
>> For easy testing, a new CLI parameter to skip the delay might be handy.
> 
> You mean a sysfs attribute may be ?
> I am not sure it would help: on resume, the sysfs attributes would be
> recreated and get the default value, not a new one.

No, I mean a module parameter for `libata` like `ata_probe_timeout`. 
Then users could test it during boot, and, if there are no issue, tell 
us the ID.


Kind regards,

Paul


>>>> [1]: https://lore.kernel.org/linux-ide/20211227162658.11314-2-pmenzel@molgen.mpg.de/T/#m697d2121463a4c946730e6b83940e12d6d7e6700
[2]: https://git.kernel.org/pub/scm/linux/kernel/git/history/history.git/
[3]: 
https://git.kernel.org/pub/scm/linux/kernel/git/history/history.git/commit/?id=4effb658a0f800e159c29a2d881cac76c326087a
[4]: 
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=d7bb4cc7575929a60b0a718daa1bce87bea9a9cc

Re: ata: For what PHY was debounce delay introduced? (was: [PATCH v3 3/3] ahci: AMD A85 FCH (Hudson D4): Skip 200 ms debounce delay in `sata_link_resume()`)

[Tejun Heo]

Hello,

On Wed, Jan 05, 2022 at 03:49:58PM -0500, Jeff Garzik wrote:
> On Tue, Jan 4, 2022 at 6:34 AM Paul Menzel <pmenzel@molgen.mpg.de> wrote:
> 
> > [cc: +jeff, +tejun]
> >
> > void sata_phy_reset(struct ata_port *ap)
> > {
> > […]
> >         /* wait for phy to become ready, if necessary */
> >         do {
> >                 msleep(200);
> >                 sstatus = scr_read(ap, SCR_STATUS);
> >                 if ((sstatus & 0xf) != 1)
> >                         break;
> >         } while (time_before(jiffies, timeout));
> > […]
> > }
> > ```
> >
> 
> The piix did not have SCRs, as I recall, so it wouldn't apply to those
> chips.   I don't recall further than that.
> 
> Presumably just give those early chips a "needs delay" quirk, and then
> start testing newer chips to make sure they survive an immediate bitbang?

I don't remember exactly but most likely the sata_sil chips, I think. But,
yeah, the way forward would be converting it to a quirk and gradually lift
them with tests.

Thanks and happy new year.

-- 
tejun

Re: [PATCH v3 3/3] ahci: AMD A85 FCH (Hudson D4): Skip 200 ms debounce delay in `sata_link_resume()`

[Dmitry Torokhov]

Hi Paul,

On Wed, Dec 29, 2021 at 05:11:18PM +0100, Paul Menzel wrote:
> 
> Add the two Chromium OS developers Dmitry and Guenter to Cc, as to my
> knowledge Chromium/Chrome OS also tries to boot very fast, and the Chromium
> project has some CI infrastructure.

I am not sure if we can be of use here as Chrome OS devices are using
either eMMC or NVME for storage. I do not recall devices using AHCI,
maybe some very old ones way past EOL.

Thanks.

-- 
Dmitry

Re: [PATCH v3 1/3] PCI: Add device code for AMD FCH SATA Controller in AHCI mode

[Sergei Shtylyov]

Hello!

On 12/29/21 7:11 PM, Paul Menzel wrote:

> The ASUS F2A85-M PRO with the fusion controller hub (FCH) AMD A85
> (Hudson D4) has the SATA controller below.
> 
>     $ lspci -s 00:11.0
>     00:11.0 SATA controller [0106]: Advanced Micro Devices, Inc. [AMD] FCH SATA Controller [AHCI mode] [1022:7801] (rev 40)
> 
> Add the ID for it, when in AHCI mode.
> 
> Signed-off-by: Paul Menzel <pmenzel@molgen.mpg.de>
> ---
>  include/linux/pci_ids.h | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/include/linux/pci_ids.h b/include/linux/pci_ids.h
> index 011f2f1ea5bb..fe944b44858a 100644
> --- a/include/linux/pci_ids.h
> +++ b/include/linux/pci_ids.h
> @@ -602,6 +602,7 @@
>  #define PCI_DEVICE_ID_AMD_LX_VIDEO  0x2081
>  #define PCI_DEVICE_ID_AMD_LX_AES    0x2082
>  #define PCI_DEVICE_ID_AMD_HUDSON2_SATA_IDE	0x7800
> +#define PCI_DEVICE_ID_AMD_HUDSON2_SATA_AHCI	0x7801

   We only add device IDs to this file if they are used in 2+ places.

[...]

MBR, Sergey