[PATCH v1 0/6] PCI: brcmstb: Misc small tweaks and fixes

[PATCH v1 0/6] PCI: brcmstb: Misc small tweaks and fixes

[Jim Quinlan]

Six small fixes and improvements for the driver.  This may be applied
before or after Stan's V5 [1] on pci-next (they should not conflict).

[1] https://lore.kernel.org/linux-pci/20250127113251.b2tqacoalcjrtcap@localhost.localdomain/T/#rfe31466507e3e540ad681278924e0ae4e0b8a727

Jim Quinlan (6):
  PCI: brcmstb: Refactor max speed limit functionality
  PCI: brcmstb: Fix error path upon call of regulator_bulk_get()
  PCI: brcmstb: Fix potential premature regluator disabling
  PCI: brcmstb: Use same constant table for config space access
  PCI: brcmstb: Make two changes in MDIO register fields
  PCI: brcmstb: Cast an int variable to an irq_hw_number_t

 drivers/pci/controller/pcie-brcmstb.c | 41 ++++++++++++++-------------
 1 file changed, 22 insertions(+), 19 deletions(-)


base-commit: 647d69605c70368d54fc012fce8a43e8e5955b04
prerequisite-patch-id: 17728ad3425bcbd72e06271f2537a5aeec9ec0f2
prerequisite-patch-id: fab98130bbf5ffc881704b02153e387aa4a08e87
prerequisite-patch-id: 0d2d8d02821ca742aed46f16e9ed60db2ead359d
prerequisite-patch-id: 8cddbbc69bd26c0284784d29f5abcc30db1c8327
prerequisite-patch-id: b5d9165e3627079a44c08faaa306c17ef735fc9f
prerequisite-patch-id: 95858e79f69b693a7955cf12549ff1ce8df27699
prerequisite-patch-id: cc66e7df1fc7acef4ce10f99033a39359b6d57ab
prerequisite-patch-id: af00ddbf164acf1742020616d8c0f05cbd5c1fa0
prerequisite-patch-id: e37b800216e856a32ec7e7231d5191f17026ebc9
prerequisite-patch-id: edeed902cf9a962e3431132dacbd95781b1cf970
prerequisite-patch-id: da4f731901ffc44f2f1a3e69c1c35213d531fcae
-- 
2.43.0

[PATCH v1 1/6] PCI: brcmstb: Refactor max speed limit functionality

[Jim Quinlan]

Make changes to the code that limits the PCIe max speed.

(1) Do the changes before link-up, not after.  We do not want
    to temporarily rise to a higher speed than desired.
(2) Use constants from pci_reg.h when possible
(3) Use uXX_replace_bits(...) for setting a register field.
(4) Use the internal link capabilities register for writing
    the max speed, not the official config space register
    where the speed field is RO.  Updating this field is
    not necessary to limit the speed so this mistake was
    harmless.

Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com>
---
 drivers/pci/controller/pcie-brcmstb.c | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
index 546056f7f0d3..f8fc3d620ee2 100644
--- a/drivers/pci/controller/pcie-brcmstb.c
+++ b/drivers/pci/controller/pcie-brcmstb.c
@@ -47,6 +47,7 @@
 
 #define PCIE_RC_CFG_PRIV1_LINK_CAPABILITY			0x04dc
 #define  PCIE_RC_CFG_PRIV1_LINK_CAPABILITY_ASPM_SUPPORT_MASK	0xc00
+#define  PCIE_RC_CFG_PRIV1_LINK_CAPABILITY_MAX_LINK_SPEED_MASK	0xf
 
 #define PCIE_RC_CFG_PRIV1_ROOT_CAP			0x4f8
 #define  PCIE_RC_CFG_PRIV1_ROOT_CAP_L1SS_MODE_MASK	0xf8
@@ -413,12 +414,12 @@ static int brcm_pcie_set_ssc(struct brcm_pcie *pcie)
 static void brcm_pcie_set_gen(struct brcm_pcie *pcie, int gen)
 {
 	u16 lnkctl2 = readw(pcie->base + BRCM_PCIE_CAP_REGS + PCI_EXP_LNKCTL2);
-	u32 lnkcap = readl(pcie->base + BRCM_PCIE_CAP_REGS + PCI_EXP_LNKCAP);
+	u32 lnkcap = readl(pcie->base + PCIE_RC_CFG_PRIV1_LINK_CAPABILITY);
 
-	lnkcap = (lnkcap & ~PCI_EXP_LNKCAP_SLS) | gen;
-	writel(lnkcap, pcie->base + BRCM_PCIE_CAP_REGS + PCI_EXP_LNKCAP);
+	u32p_replace_bits(&lnkcap, gen, PCIE_RC_CFG_PRIV1_LINK_CAPABILITY_MAX_LINK_SPEED_MASK);
+	writel(lnkcap, pcie->base + PCIE_RC_CFG_PRIV1_LINK_CAPABILITY);
 
-	lnkctl2 = (lnkctl2 & ~0xf) | gen;
+	u16p_replace_bits(&lnkctl2, gen, PCI_EXP_LNKCTL2_TLS);
 	writew(lnkctl2, pcie->base + BRCM_PCIE_CAP_REGS + PCI_EXP_LNKCTL2);
 }
 
@@ -1324,6 +1325,10 @@ static int brcm_pcie_start_link(struct brcm_pcie *pcie)
 	bool ssc_good = false;
 	int ret, i;
 
+	/* Limit the generation if specified */
+	if (pcie->gen)
+		brcm_pcie_set_gen(pcie, pcie->gen);
+
 	/* Unassert the fundamental reset */
 	ret = pcie->cfg->perst_set(pcie, 0);
 	if (ret)
@@ -1350,9 +1355,6 @@ static int brcm_pcie_start_link(struct brcm_pcie *pcie)
 
 	brcm_config_clkreq(pcie);
 
-	if (pcie->gen)
-		brcm_pcie_set_gen(pcie, pcie->gen);
-
 	if (pcie->ssc) {
 		ret = brcm_pcie_set_ssc(pcie);
 		if (ret == 0)
-- 
2.43.0

Re: [PATCH v1 1/6] PCI: brcmstb: Refactor max speed limit functionality

[Bjorn Helgaas]

On Wed, Feb 05, 2025 at 02:12:01PM -0500, Jim Quinlan wrote:
> Make changes to the code that limits the PCIe max speed.
> 
> (1) Do the changes before link-up, not after.  We do not want
>     to temporarily rise to a higher speed than desired.

This is a functional change that should be split into its own patch.
That will also make it obvious that this is not simple refactoring as
the subject line advertises.

> (2) Use constants from pci_reg.h when possible
> (3) Use uXX_replace_bits(...) for setting a register field.

> (4) Use the internal link capabilities register for writing
>     the max speed, not the official config space register
>     where the speed field is RO.  Updating this field is
>     not necessary to limit the speed so this mistake was
>     harmless.

Also a bug fix (though harmless in this case) that deserves to be
split out so the distinction between the internal and the architected
paths to the register is highlighted and may help prevent the same
mistake in the future.

> Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com>
> ---
>  drivers/pci/controller/pcie-brcmstb.c | 16 +++++++++-------
>  1 file changed, 9 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
> index 546056f7f0d3..f8fc3d620ee2 100644
> --- a/drivers/pci/controller/pcie-brcmstb.c
> +++ b/drivers/pci/controller/pcie-brcmstb.c
> @@ -47,6 +47,7 @@
>  
>  #define PCIE_RC_CFG_PRIV1_LINK_CAPABILITY			0x04dc
>  #define  PCIE_RC_CFG_PRIV1_LINK_CAPABILITY_ASPM_SUPPORT_MASK	0xc00
> +#define  PCIE_RC_CFG_PRIV1_LINK_CAPABILITY_MAX_LINK_SPEED_MASK	0xf

If the format of this internal register is different, of course we
need a new #define for this.  But if this is just a different path to
LNKCAP, and both paths read the same bits in the same format, I don't
see the point of a new #define.

>  #define PCIE_RC_CFG_PRIV1_ROOT_CAP			0x4f8
>  #define  PCIE_RC_CFG_PRIV1_ROOT_CAP_L1SS_MODE_MASK	0xf8
> @@ -413,12 +414,12 @@ static int brcm_pcie_set_ssc(struct brcm_pcie *pcie)
>  static void brcm_pcie_set_gen(struct brcm_pcie *pcie, int gen)
>  {
>  	u16 lnkctl2 = readw(pcie->base + BRCM_PCIE_CAP_REGS + PCI_EXP_LNKCTL2);
> -	u32 lnkcap = readl(pcie->base + BRCM_PCIE_CAP_REGS + PCI_EXP_LNKCAP);
> +	u32 lnkcap = readl(pcie->base + PCIE_RC_CFG_PRIV1_LINK_CAPABILITY);
>  
> -	lnkcap = (lnkcap & ~PCI_EXP_LNKCAP_SLS) | gen;
> -	writel(lnkcap, pcie->base + BRCM_PCIE_CAP_REGS + PCI_EXP_LNKCAP);
> +	u32p_replace_bits(&lnkcap, gen, PCIE_RC_CFG_PRIV1_LINK_CAPABILITY_MAX_LINK_SPEED_MASK);
> +	writel(lnkcap, pcie->base + PCIE_RC_CFG_PRIV1_LINK_CAPABILITY);
>  
> -	lnkctl2 = (lnkctl2 & ~0xf) | gen;
> +	u16p_replace_bits(&lnkctl2, gen, PCI_EXP_LNKCTL2_TLS);


OK.  I am not really a fan of the uXX_replace_bits() thing because
it's not widely used (I found 341 instances tree-wide, compared to
14000+ for FIELD_PREP()), grep can't find the definition easily, and
stylistically it doesn't fit with GENMASK(), FIELD_PREP(), etc.

But it's already used throughout brcmstb.c, so we should be
consistent.

Re: [PATCH v1 1/6] PCI: brcmstb: Refactor max speed limit functionality

[Jim Quinlan]

[-- Attachment #1: Type: text/plain, Size: 3578 bytes --]

On Thu, Feb 6, 2025 at 12:04 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
>
> On Wed, Feb 05, 2025 at 02:12:01PM -0500, Jim Quinlan wrote:
> > Make changes to the code that limits the PCIe max speed.
> >
> > (1) Do the changes before link-up, not after.  We do not want
> >     to temporarily rise to a higher speed than desired.
>
> This is a functional change that should be split into its own patch.
> That will also make it obvious that this is not simple refactoring as
> the subject line advertises.
ack
>
> > (2) Use constants from pci_reg.h when possible
> > (3) Use uXX_replace_bits(...) for setting a register field.
>
> > (4) Use the internal link capabilities register for writing
> >     the max speed, not the official config space register
> >     where the speed field is RO.  Updating this field is
> >     not necessary to limit the speed so this mistake was
> >     harmless.
>
> Also a bug fix (though harmless in this case) that deserves to be
> split out so the distinction between the internal and the architected
> paths to the register is highlighted and may help prevent the same
> mistake in the future.
ack
>
> > Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com>
> > ---
> >  drivers/pci/controller/pcie-brcmstb.c | 16 +++++++++-------
> >  1 file changed, 9 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
> > index 546056f7f0d3..f8fc3d620ee2 100644
> > --- a/drivers/pci/controller/pcie-brcmstb.c
> > +++ b/drivers/pci/controller/pcie-brcmstb.c
> > @@ -47,6 +47,7 @@
> >
> >  #define PCIE_RC_CFG_PRIV1_LINK_CAPABILITY                    0x04dc
> >  #define  PCIE_RC_CFG_PRIV1_LINK_CAPABILITY_ASPM_SUPPORT_MASK 0xc00
> > +#define  PCIE_RC_CFG_PRIV1_LINK_CAPABILITY_MAX_LINK_SPEED_MASK       0xf
>
> If the format of this internal register is different, of course we
> need a new #define for this.  But if this is just a different path to
> LNKCAP, and both paths read the same bits in the same format, I don't
> see the point of a new #define.
ack
>
> >  #define PCIE_RC_CFG_PRIV1_ROOT_CAP                   0x4f8
> >  #define  PCIE_RC_CFG_PRIV1_ROOT_CAP_L1SS_MODE_MASK   0xf8
> > @@ -413,12 +414,12 @@ static int brcm_pcie_set_ssc(struct brcm_pcie *pcie)
> >  static void brcm_pcie_set_gen(struct brcm_pcie *pcie, int gen)
> >  {
> >       u16 lnkctl2 = readw(pcie->base + BRCM_PCIE_CAP_REGS + PCI_EXP_LNKCTL2);
> > -     u32 lnkcap = readl(pcie->base + BRCM_PCIE_CAP_REGS + PCI_EXP_LNKCAP);
> > +     u32 lnkcap = readl(pcie->base + PCIE_RC_CFG_PRIV1_LINK_CAPABILITY);
> >
> > -     lnkcap = (lnkcap & ~PCI_EXP_LNKCAP_SLS) | gen;
> > -     writel(lnkcap, pcie->base + BRCM_PCIE_CAP_REGS + PCI_EXP_LNKCAP);
> > +     u32p_replace_bits(&lnkcap, gen, PCIE_RC_CFG_PRIV1_LINK_CAPABILITY_MAX_LINK_SPEED_MASK);
> > +     writel(lnkcap, pcie->base + PCIE_RC_CFG_PRIV1_LINK_CAPABILITY);
> >
> > -     lnkctl2 = (lnkctl2 & ~0xf) | gen;
> > +     u16p_replace_bits(&lnkctl2, gen, PCI_EXP_LNKCTL2_TLS);
>
>
> OK.  I am not really a fan of the uXX_replace_bits() thing because
> it's not widely used (I found 341 instances tree-wide, compared to
> 14000+ for FIELD_PREP()), grep can't find the definition easily, and
> stylistically it doesn't fit with GENMASK(), FIELD_PREP(), etc.
>
> But it's already used throughout brcmstb.c, so we should be
> consistent.

And here I thought that uXX_replace_bits()  was the up-and-coming
solution to be used :-)

Regards,
Jim Quinlan
Broadcom STB/CM

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4210 bytes --]

Re: [PATCH v1 1/6] PCI: brcmstb: Refactor max speed limit functionality

[Bjorn Helgaas]

On Thu, Feb 06, 2025 at 01:27:44PM -0500, Jim Quinlan wrote:
> On Thu, Feb 6, 2025 at 12:04 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
> > On Wed, Feb 05, 2025 at 02:12:01PM -0500, Jim Quinlan wrote:
> > > Make changes to the code that limits the PCIe max speed.
> ...

> And here I thought that uXX_replace_bits() was the up-and-coming
> solution to be used :-)

:)  Yeah, I was kind of surprised that it isn't used more, given that
it was merged in 2017.  And it *does* reduce some repetition, which is
definitely an advantage.  But the fact that those functions are hard
to find with grep is a big turnoff for me.

I wasn't really a fan of GENMASK() and FIELD_PREP() at first either,
but now I'm a big fan because you don't need _SHIFT #defines and it
reduces shift/mask errors.  So probably the same will happen with
uXX_replace_bits().

[PATCH v1 2/6] PCI: brcmstb: Fix error path upon call of regulator_bulk_get()

[Jim Quinlan]

If regulator_bulk_get() returns an error, no regulators are
created and we need to set their number to zero.  If we do
not do this and the PCIe link-up fails, regulator_bulk_free()
will be invoked and effect a panic.

Also print out the error value, as we cannot return an error
upwards as Linux will WARN on an error from add_bus().

Fixes: 9e6be018b263 ("PCI: brcmstb: Enable child bus device regulators from DT")
Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com>
---
 drivers/pci/controller/pcie-brcmstb.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
index f8fc3d620ee2..bf919467cbcd 100644
--- a/drivers/pci/controller/pcie-brcmstb.c
+++ b/drivers/pci/controller/pcie-brcmstb.c
@@ -1417,7 +1417,8 @@ static int brcm_pcie_add_bus(struct pci_bus *bus)
 
 		ret = regulator_bulk_get(dev, sr->num_supplies, sr->supplies);
 		if (ret) {
-			dev_info(dev, "No regulators for downstream device\n");
+			dev_info(dev, "Did not get regulators; err=%d\n", ret);
+			sr->num_supplies = 0;
 			goto no_regulators;
 		}
 
-- 
2.43.0

Re: [PATCH v1 2/6] PCI: brcmstb: Fix error path upon call of regulator_bulk_get()

[Bjorn Helgaas]

On Wed, Feb 05, 2025 at 02:12:02PM -0500, Jim Quinlan wrote:
> If regulator_bulk_get() returns an error, no regulators are
> created and we need to set their number to zero.  If we do
> not do this and the PCIe link-up fails, regulator_bulk_free()
> will be invoked and effect a panic.
> 
> Also print out the error value, as we cannot return an error
> upwards as Linux will WARN on an error from add_bus().

Wrap all these commit logs to fill 75 columns.  No point in leaving
unused space when most things are formatted to fill 80ish columns or
more.

> Fixes: 9e6be018b263 ("PCI: brcmstb: Enable child bus device regulators from DT")
> Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com>
> ---
>  drivers/pci/controller/pcie-brcmstb.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
> index f8fc3d620ee2..bf919467cbcd 100644
> --- a/drivers/pci/controller/pcie-brcmstb.c
> +++ b/drivers/pci/controller/pcie-brcmstb.c
> @@ -1417,7 +1417,8 @@ static int brcm_pcie_add_bus(struct pci_bus *bus)
>  
>  		ret = regulator_bulk_get(dev, sr->num_supplies, sr->supplies);
>  		if (ret) {
> -			dev_info(dev, "No regulators for downstream device\n");
> +			dev_info(dev, "Did not get regulators; err=%d\n", ret);
> +			sr->num_supplies = 0;
>  			goto no_regulators;

I think it might have been better if we could do the
regulator_bulk_get() separately, before pci_host_probe(), so that if 
this error happens, we can deal with it more easily.

Setting num_supplies = 0 is an unusual way of handling this error, and
if this pattern of managing PCIe regulators spreads to other drivers,
we might trip over this again.

Not asking for a redesign here, and maybe it wouldn't even be
possible, but it kind of fits with thinking about splitting Root Port
support from the Root Complex/host bridge support.

Bjorn

Re: [PATCH v1 2/6] PCI: brcmstb: Fix error path upon call of regulator_bulk_get()

[Jim Quinlan]

[-- Attachment #1: Type: text/plain, Size: 2833 bytes --]

On Thu, Feb 6, 2025 at 12:29 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
>
> On Wed, Feb 05, 2025 at 02:12:02PM -0500, Jim Quinlan wrote:
> > If regulator_bulk_get() returns an error, no regulators are
> > created and we need to set their number to zero.  If we do
> > not do this and the PCIe link-up fails, regulator_bulk_free()
> > will be invoked and effect a panic.
> >
> > Also print out the error value, as we cannot return an error
> > upwards as Linux will WARN on an error from add_bus().
>
> Wrap all these commit logs to fill 75 columns.  No point in leaving
> unused space when most things are formatted to fill 80ish columns or
> more.
ack
>
> > Fixes: 9e6be018b263 ("PCI: brcmstb: Enable child bus device regulators from DT")
> > Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com>
> > ---
> >  drivers/pci/controller/pcie-brcmstb.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
> > index f8fc3d620ee2..bf919467cbcd 100644
> > --- a/drivers/pci/controller/pcie-brcmstb.c
> > +++ b/drivers/pci/controller/pcie-brcmstb.c
> > @@ -1417,7 +1417,8 @@ static int brcm_pcie_add_bus(struct pci_bus *bus)
> >
> >               ret = regulator_bulk_get(dev, sr->num_supplies, sr->supplies);
> >               if (ret) {
> > -                     dev_info(dev, "No regulators for downstream device\n");
> > +                     dev_info(dev, "Did not get regulators; err=%d\n", ret);
> > +                     sr->num_supplies = 0;
> >                       goto no_regulators;
>
> I think it might have been better if we could do the
> regulator_bulk_get() separately, before pci_host_probe(), so that if
> this error happens, we can deal with it more easily.

Hi Bjorn,

Keep in mind that brcm_pcie_add_bus() cannot return a non-zero error
code, as it will get a WARN() from the caller if it does.

Would you  accept deallocating the "sr" array and setting it to NULL
like the following error condition does?  In that way we would not be
invoking any regulator_bulk_xxxx() functions with nr==0.  I'm really
wary of moving things around...

>
> Setting num_supplies = 0 is an unusual way of handling this error, and
> if this pattern of managing PCIe regulators spreads to other drivers,
> we might trip over this again.
Understood.
>
> Not asking for a redesign here, and maybe it wouldn't even be
> possible, but it kind of fits with thinking about splitting Root Port
> support from the Root Complex/host bridge support.

IIRC there was a submission having custom/modified port drivers owning
regulators, but I don't think it got accepted.  FWIW, we were
considering  switching to that.

Regards,
Jim Quinlan
Broadcom STB/CM

>
> Bjorn

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4210 bytes --]

Re: [PATCH v1 2/6] PCI: brcmstb: Fix error path upon call of regulator_bulk_get()

[Bjorn Helgaas]

On Thu, Feb 06, 2025 at 01:22:54PM -0500, Jim Quinlan wrote:
> On Thu, Feb 6, 2025 at 12:29 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
> > On Wed, Feb 05, 2025 at 02:12:02PM -0500, Jim Quinlan wrote:
> > > If regulator_bulk_get() returns an error, no regulators are
> > > created and we need to set their number to zero.  If we do
> > > not do this and the PCIe link-up fails, regulator_bulk_free()
> > > will be invoked and effect a panic.
> > >
> > > Also print out the error value, as we cannot return an error
> > > upwards as Linux will WARN on an error from add_bus().

> > > Fixes: 9e6be018b263 ("PCI: brcmstb: Enable child bus device regulators from DT")
> > > Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com>
> > > ---
> > >  drivers/pci/controller/pcie-brcmstb.c | 3 ++-
> > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
> > > index f8fc3d620ee2..bf919467cbcd 100644
> > > --- a/drivers/pci/controller/pcie-brcmstb.c
> > > +++ b/drivers/pci/controller/pcie-brcmstb.c
> > > @@ -1417,7 +1417,8 @@ static int brcm_pcie_add_bus(struct pci_bus *bus)
> > >
> > >               ret = regulator_bulk_get(dev, sr->num_supplies, sr->supplies);
> > >               if (ret) {
> > > -                     dev_info(dev, "No regulators for downstream device\n");
> > > +                     dev_info(dev, "Did not get regulators; err=%d\n", ret);
> > > +                     sr->num_supplies = 0;
> > >                       goto no_regulators;
> >
> > I think it might have been better if we could do the
> > regulator_bulk_get() separately, before pci_host_probe(), so that if
> > this error happens, we can deal with it more easily.
> 
> Keep in mind that brcm_pcie_add_bus() cannot return a non-zero error
> code, as it will get a WARN() from the caller if it does.
> 
> Would you  accept deallocating the "sr" array and setting it to NULL
> like the following error condition does?  In that way we would not be
> invoking any regulator_bulk_xxxx() functions with nr==0.  I'm really
> wary of moving things around...

Yeah, don't move anything around right now.  My wondering is just
about whether the alloc and bulk_get() could be done earlier, leaving
only the bulk_enable() to be done in brcm_pcie_add_bus().

The alloc and bulk_get() depend on DT things and it's nice to catch
those errors earlier.

Bjorn

Re: [PATCH v1 2/6] PCI: brcmstb: Fix error path upon call of regulator_bulk_get()

[Jim Quinlan]

[-- Attachment #1: Type: text/plain, Size: 3083 bytes --]

On Thu, Feb 6, 2025 at 1:34 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
>
> On Thu, Feb 06, 2025 at 01:22:54PM -0500, Jim Quinlan wrote:
> > On Thu, Feb 6, 2025 at 12:29 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
> > > On Wed, Feb 05, 2025 at 02:12:02PM -0500, Jim Quinlan wrote:
> > > > If regulator_bulk_get() returns an error, no regulators are
> > > > created and we need to set their number to zero.  If we do
> > > > not do this and the PCIe link-up fails, regulator_bulk_free()
> > > > will be invoked and effect a panic.
> > > >
> > > > Also print out the error value, as we cannot return an error
> > > > upwards as Linux will WARN on an error from add_bus().
>
> > > > Fixes: 9e6be018b263 ("PCI: brcmstb: Enable child bus device regulators from DT")
> > > > Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com>
> > > > ---
> > > >  drivers/pci/controller/pcie-brcmstb.c | 3 ++-
> > > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
> > > > index f8fc3d620ee2..bf919467cbcd 100644
> > > > --- a/drivers/pci/controller/pcie-brcmstb.c
> > > > +++ b/drivers/pci/controller/pcie-brcmstb.c
> > > > @@ -1417,7 +1417,8 @@ static int brcm_pcie_add_bus(struct pci_bus *bus)
> > > >
> > > >               ret = regulator_bulk_get(dev, sr->num_supplies, sr->supplies);
> > > >               if (ret) {
> > > > -                     dev_info(dev, "No regulators for downstream device\n");
> > > > +                     dev_info(dev, "Did not get regulators; err=%d\n", ret);
> > > > +                     sr->num_supplies = 0;
> > > >                       goto no_regulators;
> > >
> > > I think it might have been better if we could do the
> > > regulator_bulk_get() separately, before pci_host_probe(), so that if
> > > this error happens, we can deal with it more easily.
> >
> > Keep in mind that brcm_pcie_add_bus() cannot return a non-zero error
> > code, as it will get a WARN() from the caller if it does.
> >
> > Would you  accept deallocating the "sr" array and setting it to NULL
> > like the following error condition does?  In that way we would not be
> > invoking any regulator_bulk_xxxx() functions with nr==0.  I'm really
> > wary of moving things around...
>
> Yeah, don't move anything around right now.  My wondering is just
> about whether the alloc and bulk_get() could be done earlier, leaving
> only the bulk_enable() to be done in brcm_pcie_add_bus().
>
> The alloc and bulk_get() depend on DT things and it's nice to catch
> those errors earlier.

IIRC, I believe the reason that this code is in
brcm_pcie_{add,remove}_bus() is because the reviewers wanted the code
outside of the RC driver, as it was the port driver that "owned'  the
regulator.  I also vaguely recall one of my submissions that had
generic pci_subdev_regulators_{add,remove}_bus()  calls in pcie/bus.c,
but  was  later redirected back to the brcmstb RC driver.

Regards,
Jim Quinlan
Broadcom STB/CM
>
> Bjorn

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4210 bytes --]

[PATCH v1 3/6] PCI: brcmstb: Fix potential premature regluator disabling

[Jim Quinlan]

Our system for enabling and disabling regulators is designed to work
only on the port driver below the root complex.  The conditions to
discriminate for this case should be the same when we are adding or
removing the bus.  Without this change the regulators may be disabled
prematurely when a bus further down the tree is removed.

Fixes: 9e6be018b263 ("PCI: brcmstb: Enable child bus device regulators from DT")
Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com>
---
 drivers/pci/controller/pcie-brcmstb.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
index bf919467cbcd..4f5d751cbdd7 100644
--- a/drivers/pci/controller/pcie-brcmstb.c
+++ b/drivers/pci/controller/pcie-brcmstb.c
@@ -1441,7 +1441,7 @@ static void brcm_pcie_remove_bus(struct pci_bus *bus)
 	struct subdev_regulators *sr = pcie->sr;
 	struct device *dev = &bus->dev;
 
-	if (!sr)
+	if (!sr || !bus->parent || !pci_is_root_bus(bus->parent))
 		return;
 
 	if (regulator_bulk_disable(sr->num_supplies, sr->supplies))
-- 
2.43.0

Re: [PATCH v1 3/6] PCI: brcmstb: Fix potential premature regluator disabling

[Bjorn Helgaas]

In subject,

s/regluator/regulator/

On Wed, Feb 05, 2025 at 02:12:03PM -0500, Jim Quinlan wrote:
> Our system for enabling and disabling regulators is designed to work
> only on the port driver below the root complex.  The conditions to
> discriminate for this case should be the same when we are adding or
> removing the bus.  Without this change the regulators may be disabled
> prematurely when a bus further down the tree is removed.

What did the user do to cause the bus to be removed?  I'm wondering if
we turn off the power while the link is still up.  If we do, how does
it get turned back on?  Does that require a manual rescan, and the
scan of the child bus gets the power turned back on?

> Fixes: 9e6be018b263 ("PCI: brcmstb: Enable child bus device regulators from DT")
> Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com>
> ---
>  drivers/pci/controller/pcie-brcmstb.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
> index bf919467cbcd..4f5d751cbdd7 100644
> --- a/drivers/pci/controller/pcie-brcmstb.c
> +++ b/drivers/pci/controller/pcie-brcmstb.c
> @@ -1441,7 +1441,7 @@ static void brcm_pcie_remove_bus(struct pci_bus *bus)
>  	struct subdev_regulators *sr = pcie->sr;
>  	struct device *dev = &bus->dev;
>  
> -	if (!sr)
> +	if (!sr || !bus->parent || !pci_is_root_bus(bus->parent))
>  		return;
>  
>  	if (regulator_bulk_disable(sr->num_supplies, sr->supplies))
> -- 
> 2.43.0
> 

Re: [PATCH v1 3/6] PCI: brcmstb: Fix potential premature regluator disabling

[Jim Quinlan]

[-- Attachment #1: Type: text/plain, Size: 2476 bytes --]

On Thu, Feb 6, 2025 at 12:33 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
>
> In subject,
>
> s/regluator/regulator/

ack
>
>
> On Wed, Feb 05, 2025 at 02:12:03PM -0500, Jim Quinlan wrote:
> > Our system for enabling and disabling regulators is designed to work
> > only on the port driver below the root complex.  The conditions to
> > discriminate for this case should be the same when we are adding or
> > removing the bus.  Without this change the regulators may be disabled
> > prematurely when a bus further down the tree is removed.
>
> What did the user do to cause the bus to be removed?  I'm wondering if
> we turn off the power while the link is still up.  If we do, how does
> it get turned back on?  Does that require a manual rescan, and the
> scan of the child bus gets the power turned back on?

Hi Bjorn,

We only support having a regulator  for the device immediately
connected to the RC.  If the entire RC driver is unbound or rmod'd for
example, a device further down in the tree would be dismantled first
and it would turn off the regulator for the device closest to the RC.
This could cause errors during the removal of the closest device, as
it would be unable to access its registers.

Note that we do not support hotplug so there are no rescans going on
in our world.  We do have customers whose chip has several PCIe
controllers and they do unbind some of their drivers during power
critical scenarios.

The RPi folks do not use this regulator mechanism AFAICT.

Regards,
Jim Quiinlan
Broadcom STB/CM
>
>
> > Fixes: 9e6be018b263 ("PCI: brcmstb: Enable child bus device regulators from DT")
> > Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com>
> > ---
> >  drivers/pci/controller/pcie-brcmstb.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
> > index bf919467cbcd..4f5d751cbdd7 100644
> > --- a/drivers/pci/controller/pcie-brcmstb.c
> > +++ b/drivers/pci/controller/pcie-brcmstb.c
> > @@ -1441,7 +1441,7 @@ static void brcm_pcie_remove_bus(struct pci_bus *bus)
> >       struct subdev_regulators *sr = pcie->sr;
> >       struct device *dev = &bus->dev;
> >
> > -     if (!sr)
> > +     if (!sr || !bus->parent || !pci_is_root_bus(bus->parent))
> >               return;
> >
> >       if (regulator_bulk_disable(sr->num_supplies, sr->supplies))
> > --
> > 2.43.0
> >

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4210 bytes --]

[PATCH v1 4/6] PCI: brcmstb: Use same constant table for config space access

[Jim Quinlan]

The constants EXT_CFG_DATA and EXT_CFG_INDEX vary by SOC. One of the
map_bus methods used these constants, the other used different
constants.  Fortunately there was no problem because the SoCs that used
the latter map_bus method all had the same register constants.

Remove the redundant constants and adjust the code to use them.
In addition, update EXT_CFG_DATA to use the 4k-page based config
space access system, which is what the second map_bus method was
already using.

Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com>
---
 drivers/pci/controller/pcie-brcmstb.c | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
index 4f5d751cbdd7..2d1969d7fd30 100644
--- a/drivers/pci/controller/pcie-brcmstb.c
+++ b/drivers/pci/controller/pcie-brcmstb.c
@@ -151,9 +151,6 @@
 #define  MSI_INT_MASK_SET		0x10
 #define  MSI_INT_MASK_CLR		0x14
 
-#define PCIE_EXT_CFG_DATA				0x8000
-#define PCIE_EXT_CFG_INDEX				0x9000
-
 #define  PCIE_RGR1_SW_INIT_1_PERST_MASK			0x1
 #define  PCIE_RGR1_SW_INIT_1_PERST_SHIFT		0x0
 
@@ -728,8 +725,8 @@ static void __iomem *brcm_pcie_map_bus(struct pci_bus *bus,
 
 	/* For devices, write to the config space index register */
 	idx = PCIE_ECAM_OFFSET(bus->number, devfn, 0);
-	writel(idx, pcie->base + PCIE_EXT_CFG_INDEX);
-	return base + PCIE_EXT_CFG_DATA + PCIE_ECAM_REG(where);
+	writel(idx, base + IDX_ADDR(pcie));
+	return base + DATA_ADDR(pcie) + PCIE_ECAM_REG(where);
 }
 
 static void __iomem *brcm7425_pcie_map_bus(struct pci_bus *bus,
@@ -1712,7 +1709,7 @@ static void brcm_pcie_remove(struct platform_device *pdev)
 static const int pcie_offsets[] = {
 	[RGR1_SW_INIT_1]	= 0x9210,
 	[EXT_CFG_INDEX]		= 0x9000,
-	[EXT_CFG_DATA]		= 0x9004,
+	[EXT_CFG_DATA]		= 0x8000,
 	[PCIE_HARD_DEBUG]	= 0x4204,
 	[PCIE_INTR2_CPU_BASE]	= 0x4300,
 };
@@ -1720,7 +1717,7 @@ static const int pcie_offsets[] = {
 static const int pcie_offsets_bcm7278[] = {
 	[RGR1_SW_INIT_1]	= 0xc010,
 	[EXT_CFG_INDEX]		= 0x9000,
-	[EXT_CFG_DATA]		= 0x9004,
+	[EXT_CFG_DATA]		= 0x8000,
 	[PCIE_HARD_DEBUG]	= 0x4204,
 	[PCIE_INTR2_CPU_BASE]	= 0x4300,
 };
@@ -1734,8 +1731,9 @@ static const int pcie_offsets_bcm7425[] = {
 };
 
 static const int pcie_offsets_bcm7712[] = {
+	[RGR1_SW_INIT_1]	= 0x9210,
 	[EXT_CFG_INDEX]		= 0x9000,
-	[EXT_CFG_DATA]		= 0x9004,
+	[EXT_CFG_DATA]		= 0x8000,
 	[PCIE_HARD_DEBUG]	= 0x4304,
 	[PCIE_INTR2_CPU_BASE]	= 0x4400,
 };
-- 
2.43.0

[PATCH v1 5/6] PCI: brcmstb: Make two changes in MDIO register fields

[Jim Quinlan]

The HW team has decided to "tighten" some field definitions
in the MDIO packet format.  Fortunately these two changes may
be made in a backwards compatible manner.

The CMD field used to be 12 bits and now is one.  This change is
backwards compatible because the field's starting bit position is
unchanged and the only commands we've used have values 0 and 1.

The PORT field's width has been changed from four to five bits.  When
written, the new bit is not contiguous with the other four.
Fortunately, this change is backwards compatible because we have never
used anything other than 0 for the port field's value.

Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com>
---
 drivers/pci/controller/pcie-brcmstb.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
index 2d1969d7fd30..da7b10036948 100644
--- a/drivers/pci/controller/pcie-brcmstb.c
+++ b/drivers/pci/controller/pcie-brcmstb.c
@@ -176,8 +176,9 @@
 #define MDIO_PORT0			0x0
 #define MDIO_DATA_MASK			0x7fffffff
 #define MDIO_PORT_MASK			0xf0000
+#define MDIO_PORT_EXT_MASK		0x200000
 #define MDIO_REGAD_MASK			0xffff
-#define MDIO_CMD_MASK			0xfff00000
+#define MDIO_CMD_MASK			0x00100000
 #define MDIO_CMD_READ			0x1
 #define MDIO_CMD_WRITE			0x0
 #define MDIO_DATA_DONE_MASK		0x80000000
@@ -328,6 +329,7 @@ static u32 brcm_pcie_mdio_form_pkt(int port, int regad, int cmd)
 {
 	u32 pkt = 0;
 
+	pkt |= FIELD_PREP(MDIO_PORT_EXT_MASK, port >> 4);
 	pkt |= FIELD_PREP(MDIO_PORT_MASK, port);
 	pkt |= FIELD_PREP(MDIO_REGAD_MASK, regad);
 	pkt |= FIELD_PREP(MDIO_CMD_MASK, cmd);
-- 
2.43.0

[PATCH v1 6/6] PCI: brcmstb: Cast an int variable to an irq_hw_number_t

[Jim Quinlan]

Just make it clear to the reader that there is a conversion happening,
in this case from an int type to an irq_hw_number_t, an unsigned long int.

Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com>
---
 drivers/pci/controller/pcie-brcmstb.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
index da7b10036948..1e24e7fc895c 100644
--- a/drivers/pci/controller/pcie-brcmstb.c
+++ b/drivers/pci/controller/pcie-brcmstb.c
@@ -560,7 +560,7 @@ static int brcm_irq_domain_alloc(struct irq_domain *domain, unsigned int virq,
 		return hwirq;
 
 	for (i = 0; i < nr_irqs; i++)
-		irq_domain_set_info(domain, virq + i, hwirq + i,
+		irq_domain_set_info(domain, virq + i, (irq_hw_number_t)hwirq + i,
 				    &brcm_msi_bottom_irq_chip, domain->host_data,
 				    handle_edge_irq, NULL, NULL);
 	return 0;
-- 
2.43.0

Re: [PATCH v1 6/6] PCI: brcmstb: Cast an int variable to an irq_hw_number_t

[Stefan Wahren]

Hi Jim,

Am 05.02.25 um 20:12 schrieb Jim Quinlan:
> Just make it clear to the reader that there is a conversion happening,
> in this case from an int type to an irq_hw_number_t, an unsigned long int.
I'm not a fan of this generic subject. A possible suggestion might be:

PCI: brcmstb: Clarify conversion by irq_domain_set_info

Regards
>
> Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com>
> ---
>   drivers/pci/controller/pcie-brcmstb.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
> index da7b10036948..1e24e7fc895c 100644
> --- a/drivers/pci/controller/pcie-brcmstb.c
> +++ b/drivers/pci/controller/pcie-brcmstb.c
> @@ -560,7 +560,7 @@ static int brcm_irq_domain_alloc(struct irq_domain *domain, unsigned int virq,
>   		return hwirq;
>
>   	for (i = 0; i < nr_irqs; i++)
> -		irq_domain_set_info(domain, virq + i, hwirq + i,
> +		irq_domain_set_info(domain, virq + i, (irq_hw_number_t)hwirq + i,
>   				    &brcm_msi_bottom_irq_chip, domain->host_data,
>   				    handle_edge_irq, NULL, NULL);
>   	return 0;

Re: [PATCH v1 6/6] PCI: brcmstb: Cast an int variable to an irq_hw_number_t

[Jim Quinlan]

[-- Attachment #1: Type: text/plain, Size: 1385 bytes --]

On Thu, Feb 6, 2025 at 5:20 PM Stefan Wahren <wahrenst@gmx.net> wrote:
>
> Hi Jim,
>
> Am 05.02.25 um 20:12 schrieb Jim Quinlan:
> > Just make it clear to the reader that there is a conversion happening,
> > in this case from an int type to an irq_hw_number_t, an unsigned long int.
> I'm not a fan of this generic subject. A possible suggestion might be:
>
> PCI: brcmstb: Clarify conversion by irq_domain_set_info
ack
>
> Regards
> >
> > Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com>
> > ---
> >   drivers/pci/controller/pcie-brcmstb.c | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
> > index da7b10036948..1e24e7fc895c 100644
> > --- a/drivers/pci/controller/pcie-brcmstb.c
> > +++ b/drivers/pci/controller/pcie-brcmstb.c
> > @@ -560,7 +560,7 @@ static int brcm_irq_domain_alloc(struct irq_domain *domain, unsigned int virq,
> >               return hwirq;
> >
> >       for (i = 0; i < nr_irqs; i++)
> > -             irq_domain_set_info(domain, virq + i, hwirq + i,
> > +             irq_domain_set_info(domain, virq + i, (irq_hw_number_t)hwirq + i,
> >                                   &brcm_msi_bottom_irq_chip, domain->host_data,
> >                                   handle_edge_irq, NULL, NULL);
> >       return 0;
>

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4210 bytes --]