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

[Damien Le Moal <damien.lemoal@opensource.wdc.com>]

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