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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CA+-6iNxioAQH8vsdPxhjP6gHzhzy3EAKtgOCMncCqgMSMZNRPg@mail.gmail.com>
Date: Mon, 2 Jun 2025 11:36:45 -0400
From: Jim Quinlan <james.quinlan@...adcom.com>
To: Manivannan Sadhasivam <manivannan.sadhasivam@...aro.org>
Cc: linux-pci@...r.kernel.org, Nicolas Saenz Julienne <nsaenz@...nel.org>, 
	Bjorn Helgaas <bhelgaas@...gle.com>, Lorenzo Pieralisi <lorenzo.pieralisi@....com>, 
	bcm-kernel-feedback-list@...adcom.com, jim2101024@...il.com, 
	Florian Fainelli <florian.fainelli@...adcom.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 2/2] PCI: brcmstb: Use "num-lanes" DT property if present

On Sat, May 31, 2025 at 2:34 AM Manivannan Sadhasivam
<manivannan.sadhasivam@...aro.org> 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@...adcom.com>
> > ---
> >  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
>
> --
> மணிவண்ணன் சதாசிவம்

Download attachment "smime.p7s" of type "application/pkcs7-signature" (4197 bytes)