Re: [PATCH v7 2/3] PCI: brcmstb: Configure HW CLKREQ# mode appropriate for downstream device

[Jim Quinlan]

On Thu, Nov 9, 2023 at 4:27 PM Bjorn Helgaas <helgaas@xxxxxxxxxx> wrote:
>
> On Thu, Nov 09, 2023 at 02:13:53PM -0500, Jim Quinlan wrote:
> > The Broadcom STB/CM PCIe HW core, which is also used in RPi SOCs, must be
> > deliberately set by the PCIe RC HW into one of three mutually exclusive
> > modes:
> >
> > "safe" -- No CLKREQ# expected or required, refclk is always provided.  This
> >     mode should work for all devices but is not be capable of any refclk
> >     power savings.
> >
> > "no-l1ss" -- CLKREQ# is expected to be driven by the downstream device for
> >     CPM and ASPM L0s and L1.  Provides Clock Power Management, L0s, and L1,
> >     but cannot provide L1 substate (L1SS) power savings. If the downstream
> >     device connected to the RC is L1SS capable AND the OS enables L1SS, all
> >     PCIe traffic may abruptly halt, potentially hanging the system.
> >
> > "default" -- Bidirectional CLKREQ# between the RC and downstream device.
> >     Provides ASPM L0s, L1, and L1SS, but not compliant to provide Clock
> >     Power Management; specifically, may not be able to meet the Tclron max
> >     timing of 400ns as specified in "Dynamic Clock Control", section
> >     3.2.5.2.2 of the PCIe spec.  This situation is atypical and should
> >     happen only with older devices.
>
> The PCIe base spec r6.0 has no section 3.2.5.2.2.  Looks like this
> could be:
>
>   PCIe Mini CEM r2.1, sec 3.2.5.2.2 (December, 2016), or
>   PCIe CEM r5.1, sec 2.8.2 (August, 2023)
>
> I don't know the relationship between the "Mini CEM" and the "CEM"
> specs, but CEM r5.1 seems to have the same text as the Mini CEM r2.1
> about Dynamic Clock Control.
>
> We're hampered by the lack of clear subscripts here, but the text in
> both capitalizes the "CRL" part, e.g., "T_CLRon".
>
> > Previously, this driver always set the mode to "no-l1ss", as almost all
> > STB/CM boards operate in this mode.  But now there is interest in
> > activating L1SS power savings from STB/CM customers, which requires "aspm"
> > mode.  In addition, a bug was filed for RPi4 CM platform because most
> > devices did not work in "no-l1ss" mode.
> >
> > Note that the mode is specified by the DT property "brcm,clkreq-mode".  If
> > this property is omitted, then "default" mode is chosen.
> >
> > Link: https://bugzilla.kernel.org/show_bug.cgi?id=217276
> >
> > Signed-off-by: Jim Quinlan <james.quinlan@xxxxxxxxxxxx>
> > ---
> >  drivers/pci/controller/pcie-brcmstb.c | 65 ++++++++++++++++++++++-----
> >  1 file changed, 55 insertions(+), 10 deletions(-)
> >
> > diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
> > index f9dd6622fe10..f45c5d0168d3 100644
> > --- a/drivers/pci/controller/pcie-brcmstb.c
> > +++ b/drivers/pci/controller/pcie-brcmstb.c
> > @@ -121,9 +121,12 @@
> >
> >  #define PCIE_MISC_HARD_PCIE_HARD_DEBUG                                       0x4204
> >  #define  PCIE_MISC_HARD_PCIE_HARD_DEBUG_CLKREQ_DEBUG_ENABLE_MASK     0x2
> > +#define  PCIE_MISC_HARD_PCIE_HARD_DEBUG_L1SS_ENABLE_MASK             0x200000
> >  #define  PCIE_MISC_HARD_PCIE_HARD_DEBUG_SERDES_IDDQ_MASK             0x08000000
> >  #define  PCIE_BMIPS_MISC_HARD_PCIE_HARD_DEBUG_SERDES_IDDQ_MASK               0x00800000
> > -
> > +#define  PCIE_CLKREQ_MASK \
> > +       (PCIE_MISC_HARD_PCIE_HARD_DEBUG_CLKREQ_DEBUG_ENABLE_MASK | \
> > +        PCIE_MISC_HARD_PCIE_HARD_DEBUG_L1SS_ENABLE_MASK)
> >
> >  #define PCIE_INTR2_CPU_BASE          0x4300
> >  #define PCIE_MSI_INTR2_BASE          0x4500
> > @@ -1028,13 +1031,61 @@ static int brcm_pcie_setup(struct brcm_pcie *pcie)
> >       return 0;
> >  }
> >
> > +static void brcm_config_clkreq(struct brcm_pcie *pcie)
> > +{
> > +     static const char err_msg[] = "invalid 'brcm,clkreq-mode' DT string\n";
> > +     const char *mode = "default";
> > +     u32 clkreq_cntl;
> > +     int ret;
> > +
> > +     ret = of_property_read_string(pcie->np, "brcm,clkreq-mode", &mode);
> > +     if (ret && ret != -EINVAL) {
> > +             dev_err(pcie->dev, err_msg);
> > +             mode = "safe";
> > +     }
> > +
> > +     /* Start out assuming safe mode (both mode bits cleared) */
> > +     clkreq_cntl = readl(pcie->base + PCIE_MISC_HARD_PCIE_HARD_DEBUG);
> > +     clkreq_cntl &= ~PCIE_CLKREQ_MASK;
> > +
> > +     if (strcmp(mode, "no-l1ss") == 0) {
> > +             /*
> > +              * "no-l1ss" -- Provides Clock Power Management, L0s, and
> > +              * L1, but cannot provide L1 substate (L1SS) power
> > +              * savings. If the downstream device connected to the RC is
> > +              * L1SS capable AND the OS enables L1SS, all PCIe traffic
> > +              * may abruptly halt, potentially hanging the system.
> > +              */
> > +             clkreq_cntl |= PCIE_MISC_HARD_PCIE_HARD_DEBUG_CLKREQ_DEBUG_ENABLE_MASK;
>
> Does this somehow change the features advertised by the Root Port,
> e.g., does it hide the L1 PM Substates Capability completely, or at
> least clear the L1 PM Substates Supported bit?

>
> It it doesn't, the PCI core may enable L1SS and cause this hang.
> Every feature advertised in config space is expected to work.
Agree, I'll put this back in and also reconcile your other comments.

BTW, besides the RPi4, I haven't been able to find a Linux platform
where I can do

        echo $POLICY > /sys/module/pcie_aspm/parameters/policy

It seems that the FW/ACPI typically locks this down.  I did see a
comment somewhere that
said that the reason it was locked down is because too many devices
cannot handle it.
FWIW.

Regards,
Jim Quinlan
Broadcom STB/CM

>
> > +     } else if (strcmp(mode, "default") == 0) {
> > +             /*
> > +              * "default" -- Provides L0s, L1, and L1SS, but not
> > +              * compliant to provide Clock Power Management;
> > +              * specifically, may not be able to meet the Tclron max
> > +              * timing of 400ns as specified in "Dynamic Clock Control",
> > +              * section 3.2.5.2.2 of the PCIe spec.  This situation is
> > +              * atypical and should happen only with older devices.
> > +              */
> > +             clkreq_cntl |= PCIE_MISC_HARD_PCIE_HARD_DEBUG_L1SS_ENABLE_MASK;
> > +     } else {
> > +             /*
> > +              * "safe" -- No power savings; refclk is driven by RC
> > +              * unconditionally.
> > +              */
> > +             if (strcmp(mode, "safe") != 0)
> > +                     dev_err(pcie->dev, err_msg);
> > +             mode = "safe";
> > +     }
> > +     writel(clkreq_cntl, pcie->base + PCIE_MISC_HARD_PCIE_HARD_DEBUG);
> > +     dev_info(pcie->dev, "clkreq-mode set to %s\n", mode);
> > +}
> > +
> >  static int brcm_pcie_start_link(struct brcm_pcie *pcie)
> >  {
> >       struct device *dev = pcie->dev;
> >       void __iomem *base = pcie->base;
> >       u16 nlw, cls, lnksta;
> >       bool ssc_good = false;
> > -     u32 tmp;
> >       int ret, i;
> >
> >       /* Unassert the fundamental reset */
> > @@ -1059,6 +1110,8 @@ static int brcm_pcie_start_link(struct brcm_pcie *pcie)
> >               return -ENODEV;
> >       }
> >
> > +     brcm_config_clkreq(pcie);
> > +
> >       if (pcie->gen)
> >               brcm_pcie_set_gen(pcie, pcie->gen);
> >
> > @@ -1077,14 +1130,6 @@ static int brcm_pcie_start_link(struct brcm_pcie *pcie)
> >                pci_speed_string(pcie_link_speed[cls]), nlw,
> >                ssc_good ? "(SSC)" : "(!SSC)");
> >
> > -     /*
> > -      * Refclk from RC should be gated with CLKREQ# input when ASPM L0s,L1
> > -      * is enabled => setting the CLKREQ_DEBUG_ENABLE field to 1.
> > -      */
> > -     tmp = readl(base + PCIE_MISC_HARD_PCIE_HARD_DEBUG);
> > -     tmp |= PCIE_MISC_HARD_PCIE_HARD_DEBUG_CLKREQ_DEBUG_ENABLE_MASK;
> > -     writel(tmp, base + PCIE_MISC_HARD_PCIE_HARD_DEBUG);
> > -
> >       return 0;
> >  }

Attachment: smime.p7s
Description: S/MIME Cryptographic Signature