MIME-Version: 1.0
References: <20211122111332.72264-1-marcan@marcan.st> <CAL_Jsq+vFbFN+WQhi3dRicW+kaP1Oi9JPSmnAFL7XAc0eCvgrA@mail.gmail.com>
 <8e881b80-614c-dccf-ddaf-895d1acf26c7@marcan.st> <CAL_JsqLMgkTdbKNP=kAvwKFQp+m330ztTX8v_UFmj2zvzsB-KA@mail.gmail.com>
 <7f6928fc-97fd-cbe8-f7e9-954945b4574b@marcan.st>
In-Reply-To: <7f6928fc-97fd-cbe8-f7e9-954945b4574b@marcan.st>
From:   Rob Herring <robh@kernel.org>
Date:   Thu, 2 Dec 2021 08:33:56 -0600
Message-ID: <CAL_Jsq+2svQ+1GOw2HcxR88dHog7V=5dbCVgi37Dw6ariZsiqQ@mail.gmail.com>
Subject: Re: [PATCH] PCI: apple: Configure link speeds properly
To:     Hector Martin <marcan@marcan.st>
Cc:     Marc Zyngier <maz@kernel.org>,
        Alyssa Rosenzweig <alyssa@rosenzweig.io>,
        Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>,
        =?UTF-8?Q?Krzysztof_Wilczy=C5=84ski?= <kw@linux.com>,
        Bjorn Helgaas <bhelgaas@google.com>,
        PCI <linux-pci@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Wed, Dec 1, 2021 at 10:55 PM Hector Martin <marcan@marcan.st> wrote:
>
> On 30/11/2021 00.02, Rob Herring wrote:
> >> Sure, it just means I have to reinvent the PCI capability lookup wheel
> >> again. I'd love to use the regular accessors, but the infrastructure
> >> isn't up to the point where we can do that yet yere. DWC also reinvents
> >> this wheel, but we can't reuse that code because it pokes these
> >> registers through a separate reg range, not config space (even though it
> >> seems like they should be the same thing? I'm not sure what's going on
> >> in the DWC devices... for the Apple controller it's just the ECAM).
> >
> > Since it is just ECAM, can you use the regular config space accessors?
>
> The problem is this is before the PCI objects are created, so those
> wouldn't work since they expect to be called on a pci_dev and such.

Ah, right. That's something I want to fix because lots of drivers have
2 sets of accessors for this reason.

> >>>> +       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.

All this link handling is also something we want to make common as
much as possible. There's a lot of drivers poking these registers in
different ways and it's not clear whether they really need to be
different. So the less you do in the kernel the better.

Rob