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
