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> <CAL_Jsq+2svQ+1GOw2HcxR88dHog7V=5dbCVgi37Dw6ariZsiqQ@mail.gmail.com>
 <f3c5f961-d06c-47e2-453f-ec48e758a684@marcan.st>
In-Reply-To: <f3c5f961-d06c-47e2-453f-ec48e758a684@marcan.st>
From:   Rob Herring <robh@kernel.org>
Date:   Fri, 3 Dec 2021 08:21:14 -0600
Message-ID: <CAL_JsqK5_iy0b4ayj26-V+r3i_E9RbxCGKhY_ssBW2bjbCn98A@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 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.

Rob