Re: [PATCH 1/1] PCI/ASPM: Add a fix for an erratum of the PI7C9X111SLB PCI-to-PCIe bridge

["Stefan Mätje" <Stefan.Maetje@esd.eu>]

Am 01.11.18 um 21:06 schrieb Sinan Kaya:
> On 11/1/2018 3:22 PM, Stefan Mätje wrote:
>> + b/drivers/pci/pcie/aspm.c
>> @@ -268,6 +268,15 @@ static void pcie_aspm_configure_common_clock(struct pcie_link_state *link)
>>   	/* Retrain link */
>>   	reg16 |= PCI_EXP_LNKCTL_RL;
>>   	pcie_capability_write_word(parent, PCI_EXP_LNKCTL, reg16);
>> +	if (0x12d8 == parent->vendor && 0xe111 == parent->device) {
>> +		/*
>> +		 * Due to an erratum in the Pericom PI7C9X111SLB bridge in
>> +		 * reverse mode the retrain link bit needs to be cleared
>> +		 * again manually to allow the link training to succeed.
>> +		 */
>> +		reg16 &= ~PCI_EXP_LNKCTL_RL;
>> +		pcie_capability_write_word(parent, PCI_EXP_LNKCTL, reg16);
>> +	}
> 
> The typical model is to abstract quirk work into quirks.c and add some
> callbacks from the actual code.

Yes, I'm aware of the quirks.c code. But I don't believe the problem can be solved
by a quirk function that is run via pci_fixup_device() at certain points of the
PCI scan (i. e. pci_fixup_pass like pci_fixup_early / pci_fixup_header ...) after 
pcie_aspm_cap_init() has run.

Let's have a look at the function pcie_aspm_cap_init() from where pcie_aspm_configure_common_clock()
is called (where the patch was included). Be aware of the fact that the PCI express link downstream 
is broken after leaving that function without the patch. But looking in pcie_aspm_cap_init() you can 
see that it is reloading the ASPM registers from the child device after returning from 
pcie_aspm_configure_common_clock() and from this point on it is working on bogus ASPM register 
contents.

Therefore I think the rest of pcie_aspm_cap_init() is doing nothing sensible for the downstream 
PCIe tree. Also I think that pcie_aspm_configure_common_clock() must be fixed in a way that after
leaving that function the PCIe downstream link is still working. This is what my patch is good for.

Best regards,
    Stefan