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@xxxxxxxxxxxxx/T/#m697d2121463a4c946730e6b83940e12d6d7e6700
--
Damien Le Moal
Western Digital Research
