[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CANCKTBsLuxGq1fBNcasLRuStELSknDGH3C-e+EbKy-rfw4L18Q@mail.gmail.com>
Date: Thu, 6 Apr 2023 16:03:05 -0400
From: Jim Quinlan <jim2101024@...il.com>
To: Bjorn Helgaas <helgaas@...nel.org>
Cc: linux-pci@...r.kernel.org,
Nicolas Saenz Julienne <nsaenz@...nel.org>,
Bjorn Helgaas <bhelgaas@...gle.com>,
Lorenzo Pieralisi <lorenzo.pieralisi@....com>,
Cyril Brulebois <kibi@...ian.org>,
Phil Elwell <phil@...pberrypi.com>,
bcm-kernel-feedback-list@...adcom.com, james.quinlan@...adcom.com,
Florian Fainelli <f.fainelli@...il.com>,
Lorenzo Pieralisi <lpieralisi@...nel.org>,
Krzysztof Wilczyński <kw@...ux.com>,
Rob Herring <robh@...nel.org>,
"moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE"
<linux-rpi-kernel@...ts.infradead.org>,
"moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE"
<linux-arm-kernel@...ts.infradead.org>,
open list <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v1 2/3] PCI: brcmstb: Clkreq# accomodations of downstream device
On Thu, Apr 6, 2023 at 3:09 PM Bjorn Helgaas <helgaas@...nel.org> wrote:
>
> On Thu, Apr 06, 2023 at 08:46:23AM -0400, Jim Quinlan wrote:
> > The Broadcom STB/CM PCIe HW core, which is also used in RPi SOCs, may be
> > set into three mutually exclusive modes:
> >
> > (a) No clkreq# expected or required, refclk is always available.
> > (b) Clkreq# is expected to be driven by downstream device when needed.
> > (c) Bidirectional clkreq# for L1SS capable devices.
> >
> > Previously, only (b) was supported by the driver, as almost all STB boards
> > operate in this mode. But now there is interest in activating L1SS power
> > savings from STB customers, and also interest in accomodating mode (a) for
> > designs such as the RPi CM4 with IO board.
> >
> > The HW can tell us when mode (a) mode is needed. But there is no easy way
> > to tell if L1SS mode is needed. Unfortunately, getting this wrong causes a
> > panic during boot time. So we rely on the DT prop "brcm,enable-l1ss" to
> > tell us when mode (c) is desired. This property has already been in
> > use by Raspian Linux, but this immplementation adds more details and
> > discerns between (a) and (b) automatically.
> >
> > Link: https://bugzilla.kernel.org/show_bug.cgi?id=217276
> > Signed-off-by: Jim Quinlan <jim2101024@...il.com>
>
> > + * We have "seen" clkreq# if it is asserted or has been in the past.
> > + * Note that the CLKREQ_IN_MASK is 1 if clkreq# is asserted.
>
> "CLKREQ#" to match PCIe spec and comments below.
Will do.
>
> > + if (l1ss && IS_ENABLED(CONFIG_PCIEASPM)) {
> > + /*
> > + * Note: This (l1ss) mode may not meet requirement for
> > + * Endpoints that require CLKREQ# assertion to clock active
> > + * within 400ns.
> > + */
> > + clkreq_set |= PCIE_MISC_HARD_PCIE_HARD_DEBUG_L1SS_ENABLE_MASK;
> > + dev_info(pcie->dev, "bi-dir clkreq; l1ss-capable devs only\n");
> > + dev_info(pcie->dev, "ASPM policy must be set to powersupersave\n");
>
> Seems problematic since L1SS can be enabled/disabled at run-time:
>
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/ABI/testing/sysfs-bus-pci?id=v6.2#n420
Yes it is problematic. AFAICT there are no notifier chain for
changing modes; you probably don't want me to add one and neither do I
:-).
Our HW should shift gears when there is a change the standard L1SS
control field, but it does not -- more on this below.
>
> The simplistic answer is to advertise L1SS support if and only if you
> can safely support it.
We can only safely support it if we know a priori that (a) the
downstream device is ASPM capable and (b) the policy is set to
"power_supersave" and will not be changed.
For (a), I thought about having the RC probe "peek" at the downstream
devices capabilities, but that would require it to go through the
capability linked-list. I have a feeling that would not be approved
by you folks.
For (b), there is no way to know at RC probe-time that the policy is
going to be "power_supersave". This calculation happens after the RC
probe exits, and besides, pcie_aspm_get_policy() is a static function.
And as you mentioned, the ASPM policy may be changed at runtime.
That is the reason for the "brcm,enable-l1ss" property.
>
> I don't know why this is an issue for this device but not others. Is
> it because there's some problem in the way the board is designed? Or
> (after skimming the bugzilla) maybe a problem with the plug-in cards?
FWIW, it's not clear that all of the devices for drivers in
drivers/pci/controller/ support L1SS -- our driver had no mention of
ti until now.
However, I asked the PCIe HW design engineer a question similar to
yours; i.e. from looking at all of the drivers' code as well as
pcie/asmp.c, there doesn't seem to be any issue wrt seamlessly
switching between uni-dir and bi-dir CLKREQ# in response to ASPM
control settings. He just answered something on the lines that for
this design, one has to make a deliberate choice of L1SS or !L1SS and
stick with it.
Regards,
Jim Quinlan
Broadcom STB
>
> Bjorn
Powered by blists - more mailing lists