Re: [PATCH] PCI: apple: Configure link speeds properly

[Hector Martin <marcan@marcan.st>]

On 03/12/2021 23.21, Rob Herring wrote:
> On Fri, Dec 3, 2021 at 7:47 AM Hector Martin <marcan@marcan.st> wrote:
>>
>> On 02/12/2021 23.33, Rob Herring wrote:
>>>>>>>> +       max_gen = of_pci_get_max_link_speed(port->np);
>>>>>>>> +       if (max_gen < 0) {
>>>>>>>> +               dev_err(port->pcie->dev, "max link speed not specified\n");
>>>>>>>
>>>>>>> Better to fail than limp along in gen1? Though you don't check the
>>>>>>> return value...
>>>>>>>
>>>>>>> Usually, the DT property is there to limit the speed when there's a
>>>>>>> board limitation.
>>>>>>
>>>>>> The default *setting* is actually Gen4, but without
>>>>>> PCIE_LINK_WIDTH_SPEED_CONTROL poked it always trains at Gen1. Might make
>>>>>> more sense to only set the LNKCTL field if max-link-speed is specified,
>>>>>> and unconditionally poke that bit. That'll get us Gen4 by default (or
>>>>>> even presumably Gen5 in future controllers, if everything else stays
>>>>>> compatible).
>>>>>
>>>>> You already do some setup in firmware for ECAM, right? I think it
>>>>> would be better if you can do any default setup there and then
>>>>> max-link-speed is only an override for the kernel.
>>>>
>>>> I thought the PCIE_LINK_WIDTH_SPEED_CONTROL thing had to be set later,
>>>> but trying it now I realized we were missing a bit of initialization
>>>> that was causing it not to work. Indeed it can be done there and we can
>>>> drop it from the kernel.
>>>>
>>>> We could even do the max-link-speed thing in m1n1 if we want. It has
>>>> access to the value from the ADT directly, which to be correct we'd have
>>>> to dynamically transplant to the DT, since there's at least one device
>>>> that has different PCIe devices on one port depending on hardware
>>>> variant, while sharing a devicetree. If we're okay with the kernel just
>>>> not implementing this feature for now, we can say it's the bootloader's job.
>>>>
>>>> Ultimately we ship the DTs along with m1n1, so there's an argument that
>>>> if some day we need to override the max-link-speed for whatever reason
>>>> over what the ADT says, well, we'd be shipping the updated DT along with
>>>> m1n1 anyway, so we might as well make m1n1 do it... if so, it might make
>>>> sense to drop those properties from the actual DTs we ship altogether,
>>>> at least for now.
>>>>
>>>> If we decide to make it m1n1's job entirely, we can drop this patch
>>>> altogether, at least for now (I can't say how this will interact with
>>>> suspend/resume and other power management, and hotplug... but we'll open
>>>> that can of worms when we get there).
>>>
>>> Shouldn't you be setting PCI_EXP_LNKCAP_SLS and/or PCI_EXP_LNKCAP2 if
>>> you need to limit the max speed and then you can use that instead of
>>> max-link-speed? If that's lost in low power modes, the driver just has
>>> to save and restore it.
>>
>> Those registers aren't writable as far as I can tell. All we can do is
>> set LNKCTL2 to tell the hardware what actual max speed to use, the same
>> thing this patch does.
> 
> I believe they are if you set the PCIE_DBI_RO_WR_EN bit. Multiple DWC
> drivers write PCI_EXP_LNKCAP.

Oh, indeed, that works. I figured that DBI stuff was only for the higher 
registers, not the core PCIe ones. Sounds like a plan then! We might 
still want to set LNKCTL2 though, since there's no point in that 
specifying a higher speed than what we are now claiming the hardware 
supports.

We can drop this patch then, and probably the max-link-speed props in 
the DTs altogether. More stuff in m1n1 is good, makes it easier to keep 
older kernel support with new hardware :)

-- 
Hector Martin (marcan@marcan.st)
Public Key: https://mrcn.st/pub