Messages in this thread | | | Date | Tue, 4 Jan 2022 09:49:29 +0100 | Subject | Re: [PATCH v3 3/3] ahci: AMD A85 FCH (Hudson D4): Skip 200 ms debounce delay in `sata_link_resume()` | From | 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
| |