Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20230406124625.41325-3-jim2101024@gmail.com> <20230406190953.GA3723665@bhelgaas>
In-Reply-To: <20230406190953.GA3723665@bhelgaas>
From:   Jim Quinlan <jim2101024@gmail.com>
Date:   Thu, 6 Apr 2023 16:03:05 -0400
Message-ID: <CANCKTBsLuxGq1fBNcasLRuStELSknDGH3C-e+EbKy-rfw4L18Q@mail.gmail.com>
Subject: Re: [PATCH v1 2/3] PCI: brcmstb: Clkreq# accomodations of downstream device
To:     Bjorn Helgaas <helgaas@kernel.org>
Cc:     linux-pci@vger.kernel.org,
        Nicolas Saenz Julienne <nsaenz@kernel.org>,
        Bjorn Helgaas <bhelgaas@google.com>,
        Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>,
        Cyril Brulebois <kibi@debian.org>,
        Phil Elwell <phil@raspberrypi.com>,
        bcm-kernel-feedback-list@broadcom.com, james.quinlan@broadcom.com,
        Florian Fainelli <f.fainelli@gmail.com>,
        Lorenzo Pieralisi <lpieralisi@kernel.org>,
        =?UTF-8?Q?Krzysztof_Wilczy=C5=84ski?= <kw@linux.com>,
        Rob Herring <robh@kernel.org>,
        "moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE" 
        <linux-rpi-kernel@lists.infradead.org>,
        "moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE" 
        <linux-arm-kernel@lists.infradead.org>,
        open list <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Thu, Apr 6, 2023 at 3:09=E2=80=AFPM Bjorn Helgaas <helgaas@kernel.org> w=
rote:
>
> 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 b=
e
> > 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 boa=
rds
> > operate in this mode.  But now there is interest in activating L1SS pow=
er
> > 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 caus=
es a
> > panic during boot time.  So we rely on the DT prop "brcm,enable-l1ss" t=
o
> > 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=3D217276
> > Signed-off-by: Jim Quinlan <jim2101024@gmail.com>
>
> > +      * We have "seen" clkreq# if it is asserted or has been in the pa=
st.
> > +      * 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 acti=
ve
> > +              * within 400ns.
> > +              */
> > +             clkreq_set |=3D PCIE_MISC_HARD_PCIE_HARD_DEBUG_L1SS_ENABL=
E_MASK;
> > +             dev_info(pcie->dev, "bi-dir clkreq; l1ss-capable devs onl=
y\n");
> > +             dev_info(pcie->dev, "ASPM policy must be set to powersupe=
rsave\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/D=
ocumentation/ABI/testing/sysfs-bus-pci?id=3Dv6.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