Re: [PATCH 2/2] PCI: brcmstb: Use "num-lanes" DT property if present

[Jim Quinlan]

On Sat, May 31, 2025 at 2:34 AM Manivannan Sadhasivam
<manivannan.sadhasivam@xxxxxxxxxx> wrote:
>
> On Fri, May 30, 2025 at 06:40:33PM -0400, Jim Quinlan wrote:
> > By default, we use automatic HW negotiation to ascertain the number of
> > lanes of the PCIe connection.  If the "num-lanes" DT property is present,
> > assume that the chip's built-in capability information is incorrect or
> > undesired, and use the specified value instead.
> >
> > Signed-off-by: Jim Quinlan <james.quinlan@xxxxxxxxxxxx>
> > ---
> >  drivers/pci/controller/pcie-brcmstb.c | 26 +++++++++++++++++++++++++-
> >  1 file changed, 25 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
> > index e19628e13898..79fc6d00b7bc 100644
> > --- a/drivers/pci/controller/pcie-brcmstb.c
> > +++ b/drivers/pci/controller/pcie-brcmstb.c
> > @@ -46,6 +46,7 @@
> >  #define  PCIE_RC_CFG_PRIV1_ID_VAL3_CLASS_CODE_MASK   0xffffff
> >
> >  #define PCIE_RC_CFG_PRIV1_LINK_CAPABILITY                    0x04dc
> > +#define  PCIE_RC_CFG_PRIV1_LINK_CAPABILITY_MAX_LINK_WIDTH_MASK       0x1f0
> >  #define  PCIE_RC_CFG_PRIV1_LINK_CAPABILITY_ASPM_SUPPORT_MASK 0xc00
> >
> >  #define PCIE_RC_CFG_PRIV1_ROOT_CAP                   0x4f8
> > @@ -55,6 +56,9 @@
> >  #define PCIE_RC_DL_MDIO_WR_DATA                              0x1104
> >  #define PCIE_RC_DL_MDIO_RD_DATA                              0x1108
> >
> > +#define PCIE_RC_PL_REG_PHY_CTL_1                     0x1804
> > +#define  PCIE_RC_PL_REG_PHY_CTL_1_REG_P2_POWERDOWN_ENA_NOSYNC_MASK   0x8
> > +
> >  #define PCIE_RC_PL_PHY_CTL_15                                0x184c
> >  #define  PCIE_RC_PL_PHY_CTL_15_DIS_PLL_PD_MASK               0x400000
> >  #define  PCIE_RC_PL_PHY_CTL_15_PM_CLK_PERIOD_MASK    0xff
> > @@ -1072,7 +1076,7 @@ static int brcm_pcie_setup(struct brcm_pcie *pcie)
> >       void __iomem *base = pcie->base;
> >       struct pci_host_bridge *bridge;
> >       struct resource_entry *entry;
> > -     u32 tmp, burst, aspm_support;
> > +     u32 tmp, burst, aspm_support, num_lanes, num_lanes_cap;
> >       u8 num_out_wins = 0;
> >       int num_inbound_wins = 0;
> >       int memc, ret;
> > @@ -1180,6 +1184,26 @@ static int brcm_pcie_setup(struct brcm_pcie *pcie)
> >               PCIE_RC_CFG_PRIV1_LINK_CAPABILITY_ASPM_SUPPORT_MASK);
> >       writel(tmp, base + PCIE_RC_CFG_PRIV1_LINK_CAPABILITY);
> >
> > +     /* 'tmp' still holds the contents of PRIV1_LINK_CAPABILITY */
> > +     num_lanes_cap = u32_get_bits(tmp, PCIE_RC_CFG_PRIV1_LINK_CAPABILITY_MAX_LINK_WIDTH_MASK);
> > +     num_lanes = 0;
> > +     /*
> > +      * Use automatic num-lanes HW negotiation by default.  If the
>
> "Use hardware negotiated Max Link Width value by default."
>
> > +      * "num-lanes" DT property is present, assume that the chip's
> > +      * built-in link width capability information is
> > +      * incorrect/undesired and use the specified value instead.
> > +      */
> > +     if (!of_property_read_u32(pcie->np, "num-lanes", &num_lanes) &&
> > +         num_lanes && num_lanes <= 4 && num_lanes_cap != num_lanes) {
>
> I think you should drop the 'num_lanes && num_lanes <= 4' check since the DT
> binding should take care of that. Otherwise, once link width gets increased, you
> need to update both binding and the driver, which is redundant.
Not all Linux release configuration systems run a comprehensive DT
validator  before execution.  Our bootloader modifies the DT blob on
the fly and also permits -- with restrictions -- customers to modify
the DT at the bootloader command line.  Yes, we can do partial a
priori DT validation, but there is still value to checking the params
in the driver code, at least for us.

>
> - Mani
>
> --
> மணிவண்ணன் சதாசிவம்

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