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

From: Hector Martin
Date: Tue Dec 07 2021 - 00:07:50 EST

On 03/12/2021 23.27, Hector Martin wrote:
On 03/12/2021 23.21, Rob Herring wrote:
On Fri, Dec 3, 2021 at 7:47 AM Hector Martin <marcan@xxxxxxxxx> 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 :)

This is now done in m1n1 (abee2ec).

Interestingly, the ADT downgrades the link to the SD card reader to Gen1, even though both sides can do Gen2. I wonder why that is the case. Perhaps they didn't need the extra speed and Gen1 saves power?

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