| Message ID | 20211229161119.1006-3-pmenzel@molgen.mpg.de |
|---|---|
| State | New |
| Headers | show |
| Series | [v3,1/3] PCI: Add device code for AMD FCH SATA Controller in AHCI mode | expand |
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 */
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
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.
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
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.
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
[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
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
[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
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.
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 */