[PATCH v2 0/8] PCI: brcmstb: Misc small tweaks and fixes

[PATCH v2 0/8] PCI: brcmstb: Misc small tweaks and fixes

[Jim Quinlan]

V2: Changes from V1

  o All Commits: Wrap all commit messages to 75 cols (Bjorn)
  o Commit: Refactor max speed limit functionality
    -- Split into three commits (Bjorn)
  o Commit: "Fix error path upon call of regulator_bulk_get()"
    -- Changed so regulator_bulk_xxx() funcs do not get called with
       num_regulators==0 (Bjorn)
  o Commit: "Fix potential premature regluator disabling"
    -- s/regluator/regulator/ (Bjorn)
  o Commit: "Cast an int variable to an irq_hw_number_t"
    -- Change commit subject line (Stephan)


V1:

  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 (8):
  PCI: brcmstb: Set gen limitation before link, not after
  PCI: brcmstb: Write to internal register to change link cap
  PCI: brcmstb: Do not assume that reg field starts at LSB
  PCI: brcmstb: Fix error path upon call of regulator_bulk_get()
  PCI: brcmstb: Fix potential premature regulator disabling
  PCI: brcmstb: Use same constant table for config space access
  PCI: brcmstb: Make two changes in MDIO register fields
  PCI: brcmstb: Clarify conversion of irq_domain_set_info() param

 drivers/pci/controller/pcie-brcmstb.c | 40 ++++++++++++++-------------
 1 file changed, 21 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 v2 1/8] PCI: brcmstb: Set gen limitation before link, not after

[Jim Quinlan]

When the user elects to limit the PCIe generation via the appropriate DT
property, apply the settings before the PCIe link-up, not after.

Fixes: c0452137034bda8f686dd9a2e167949bfffd6776 ("PCI: brcmstb: Add Broadcom STB PCIe host controller driver")

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

diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
index 546056f7f0d3..64a7511e66a8 100644
--- a/drivers/pci/controller/pcie-brcmstb.c
+++ b/drivers/pci/controller/pcie-brcmstb.c
@@ -1324,6 +1324,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 +1354,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

[PATCH v2 2/8] PCI: brcmstb: Write to internal register to change link cap

[Jim Quinlan]

The driver was mistakenly writing to a RO config-space register
(PCI_EXP_LNKCAP).  Although harmless in this case, the proper destination
is an internal RW register that is reflected by PCI_EXP_LNKCAP.

Fixes: c0452137034bda8f686dd9a2e167949bfffd6776 ("PCI: brcmstb: Add Broadcom STB PCIe host controller driver")

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

diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
index 64a7511e66a8..98542e74aa16 100644
--- a/drivers/pci/controller/pcie-brcmstb.c
+++ b/drivers/pci/controller/pcie-brcmstb.c
@@ -413,10 +413,10 @@ 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);
+	writel(lnkcap, pcie->base + PCIE_RC_CFG_PRIV1_LINK_CAPABILITY);
 
 	lnkctl2 = (lnkctl2 & ~0xf) | gen;
 	writew(lnkctl2, pcie->base + BRCM_PCIE_CAP_REGS + PCI_EXP_LNKCTL2);
-- 
2.43.0

[PATCH v2 3/8] PCI: brcmstb: Do not assume that reg field starts at LSB

[Jim Quinlan]

When setting a register field it was assumed that the field started at the
lsb of the register.  Although the masks do indeed start at the lsb, and
this will probably not change, it is prudent to use a method that makes no
assumption about the mask's placement in the register.

The uXXp_replace_bits() calls are used since they are already prevalent
in this driver.

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

diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
index 98542e74aa16..e0b20f58c604 100644
--- a/drivers/pci/controller/pcie-brcmstb.c
+++ b/drivers/pci/controller/pcie-brcmstb.c
@@ -415,10 +415,10 @@ 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 + PCIE_RC_CFG_PRIV1_LINK_CAPABILITY);
 
-	lnkcap = (lnkcap & ~PCI_EXP_LNKCAP_SLS) | gen;
+	u32p_replace_bits(&lnkcap, gen, PCI_EXP_LNKCAP_SLS);
 	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);
 }
 
-- 
2.43.0

[PATCH v2 4/8] 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 e0b20f58c604..56b49d3cae19 100644
--- a/drivers/pci/controller/pcie-brcmstb.c
+++ b/drivers/pci/controller/pcie-brcmstb.c
@@ -1416,7 +1416,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);
+			pcie->sr = NULL;
 			goto no_regulators;
 		}
 
-- 
2.43.0

[PATCH v2 5/8] PCI: brcmstb: Fix potential premature regulator 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 56b49d3cae19..e1059e3365bd 100644
--- a/drivers/pci/controller/pcie-brcmstb.c
+++ b/drivers/pci/controller/pcie-brcmstb.c
@@ -1440,7 +1440,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

[PATCH v2 6/8] 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 e1059e3365bd..923ac1a03f85 100644
--- a/drivers/pci/controller/pcie-brcmstb.c
+++ b/drivers/pci/controller/pcie-brcmstb.c
@@ -150,9 +150,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
 
@@ -727,8 +724,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,
@@ -1711,7 +1708,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,
 };
@@ -1719,7 +1716,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,
 };
@@ -1733,8 +1730,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 v2 7/8] 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 923ac1a03f85..cb897d4b2579 100644
--- a/drivers/pci/controller/pcie-brcmstb.c
+++ b/drivers/pci/controller/pcie-brcmstb.c
@@ -175,8 +175,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
@@ -327,6 +328,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 v2 8/8] PCI: brcmstb: Clarify conversion of irq_domain_set_info() param

[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 cb897d4b2579..f790d5252e9f 100644
--- a/drivers/pci/controller/pcie-brcmstb.c
+++ b/drivers/pci/controller/pcie-brcmstb.c
@@ -559,7 +559,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 v2 1/8] PCI: brcmstb: Set gen limitation before link, not after

[Florian Fainelli]

On 2/14/2025 9:39 AM, Jim Quinlan wrote:
> When the user elects to limit the PCIe generation via the appropriate DT
> property, apply the settings before the PCIe link-up, not after.
> 
> Fixes: c0452137034bda8f686dd9a2e167949bfffd6776 ("PCI: brcmstb: Add Broadcom STB PCIe host controller driver")

Looks like this commit ID should be shortened to 12 digits. Other than that:

Reviewed-by: Florian Fainelli <florian.fainelli@broadcom.com>
-- 
Florian

Re: [PATCH v2 2/8] PCI: brcmstb: Write to internal register to change link cap

[Florian Fainelli]

On 2/14/2025 9:39 AM, Jim Quinlan wrote:
> The driver was mistakenly writing to a RO config-space register
> (PCI_EXP_LNKCAP).  Although harmless in this case, the proper destination
> is an internal RW register that is reflected by PCI_EXP_LNKCAP.
> 
> Fixes: c0452137034bda8f686dd9a2e167949bfffd6776 ("PCI: brcmstb: Add Broadcom STB PCIe host controller driver")

Likewise this needs to be shortened to 12 digits, with that fixed:

Reviewed-by: Florian Fainelli <florian.fainelli@broadcom.com>
-- 
Florian

Re: [PATCH v2 3/8] PCI: brcmstb: Do not assume that reg field starts at LSB

[Florian Fainelli]

On 2/14/2025 9:39 AM, Jim Quinlan wrote:
> When setting a register field it was assumed that the field started at the
> lsb of the register.  Although the masks do indeed start at the lsb, and
> this will probably not change, it is prudent to use a method that makes no
> assumption about the mask's placement in the register.
> 
> The uXXp_replace_bits() calls are used since they are already prevalent
> in this driver.
> 
> Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com>

Reviewed-by: Florian Fainelli <florian.fainelli@broadcom.com>
-- 
Florian

Re: [PATCH v2 4/8] PCI: brcmstb: Fix error path upon call of regulator_bulk_get()

[Florian Fainelli]

On 2/14/2025 9:39 AM, 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>

Reviewed-by: Florian Fainelli <florian.fainelli@broadcom.com>
-- 
Florian

Re: [PATCH v2 5/8] PCI: brcmstb: Fix potential premature regulator disabling

[Florian Fainelli]

On 2/14/2025 9:39 AM, 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.
> 
> Fixes: 9e6be018b263 ("PCI: brcmstb: Enable child bus device regulators from DT")
> Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com>

Reviewed-by: Florian Fainelli <florian.fainelli@broadcom.com>
-- 
Florian

Re: [PATCH v2 6/8] PCI: brcmstb: Use same constant table for config space access

[Florian Fainelli]

On 2/14/2025 9:39 AM, Jim Quinlan wrote:
> 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>

Reviewed-by: Florian Fainelli <florian.fainelli@broadcom.com>
-- 
Florian

Re: [PATCH v2 7/8] PCI: brcmstb: Make two changes in MDIO register fields

[Florian Fainelli]

On 2/14/2025 9:39 AM, Jim Quinlan wrote:
> 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>

Reviewed-by: Florian Fainelli <florian.fainelli@broadcom.com>
-- 
Florian

Re: [PATCH v2 8/8] PCI: brcmstb: Clarify conversion of irq_domain_set_info() param

[Florian Fainelli]

On 2/14/2025 9:39 AM, Jim Quinlan wrote:
> 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>

Reviewed-by: Florian Fainelli <florian.fainelli@broadcom.com>
-- 
Florian

Re: [PATCH v2 0/8] PCI: brcmstb: Misc small tweaks and fixes

[Krzysztof Wilczyński]

Hello,

[...]
>   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).

Applied to controller/brcmstb, thank you!

	Krzysztof

Re: [PATCH v2 4/8] PCI: brcmstb: Fix error path upon call of regulator_bulk_get()

[Bjorn Helgaas]

On Fri, Feb 14, 2025 at 12:39:32PM -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 e0b20f58c604..56b49d3cae19 100644
> --- a/drivers/pci/controller/pcie-brcmstb.c
> +++ b/drivers/pci/controller/pcie-brcmstb.c
> @@ -1416,7 +1416,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);
> +			pcie->sr = NULL;

Is alloc_subdev_regulators() buying us something useful?  It seems
like it would be simpler to have:

  struct brcm_pcie {
    ...
    struct regulator_bulk_data supplies[3];
    ...
  };

I think that's what most callers of devm_regulator_bulk_get() do.

>  			goto no_regulators;
>  		}
>  
> -- 
> 2.43.0
> 

Re: [PATCH v2 4/8] PCI: brcmstb: Fix error path upon call of regulator_bulk_get()

[Jim Quinlan]

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

On Mon, Mar 3, 2025 at 1:40 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
>
> On Fri, Feb 14, 2025 at 12:39:32PM -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 e0b20f58c604..56b49d3cae19 100644
> > --- a/drivers/pci/controller/pcie-brcmstb.c
> > +++ b/drivers/pci/controller/pcie-brcmstb.c
> > @@ -1416,7 +1416,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);
> > +                     pcie->sr = NULL;
>
> Is alloc_subdev_regulators() buying us something useful?  It seems
> like it would be simpler to have:
>
>   struct brcm_pcie {
>     ...
>     struct regulator_bulk_data supplies[3];
>     ...
>   };
>
> I think that's what most callers of devm_regulator_bulk_get() do.

Ack.

Jim Quinlan
Broadcom STB/CM
>
> >                       goto no_regulators;
> >               }
> >
> > --
> > 2.43.0
> >

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

Re: [PATCH v2 1/8] PCI: brcmstb: Set gen limitation before link, not after

[Manivannan Sadhasivam]

On Fri, Feb 14, 2025 at 12:39:29PM -0500, Jim Quinlan wrote:
> When the user elects to limit the PCIe generation via the appropriate DT
> property, apply the settings before the PCIe link-up, not after.
> 
> Fixes: c0452137034bda8f686dd9a2e167949bfffd6776 ("PCI: brcmstb: Add Broadcom STB PCIe host controller driver")
> 

Common practice is to use the 12 chars SHA for Fixes tag and not the entire 40
chars. Like,

Fixes: c0452137034b ("PCI: brcmstb: Add Broadcom STB PCIe host controller driver")

And no need of extra newline here.

> Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com>

Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

- Mani

> ---
>  drivers/pci/controller/pcie-brcmstb.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
> index 546056f7f0d3..64a7511e66a8 100644
> --- a/drivers/pci/controller/pcie-brcmstb.c
> +++ b/drivers/pci/controller/pcie-brcmstb.c
> @@ -1324,6 +1324,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 +1354,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 v2 2/8] PCI: brcmstb: Write to internal register to change link cap

[Manivannan Sadhasivam]

On Fri, Feb 14, 2025 at 12:39:30PM -0500, Jim Quinlan wrote:
> The driver was mistakenly writing to a RO config-space register
> (PCI_EXP_LNKCAP).  Although harmless in this case, the proper destination
> is an internal RW register that is reflected by PCI_EXP_LNKCAP.
> 

You mean to say that writing to RO register doesn't cause any issue, but what
about the link capability not getting updated? It is a bug nevertheless.

> Fixes: c0452137034bda8f686dd9a2e167949bfffd6776 ("PCI: brcmstb: Add Broadcom STB PCIe host controller driver")
> 

Same comment as previous patch.

> Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com>

Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

- Mani

> ---
>  drivers/pci/controller/pcie-brcmstb.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
> index 64a7511e66a8..98542e74aa16 100644
> --- a/drivers/pci/controller/pcie-brcmstb.c
> +++ b/drivers/pci/controller/pcie-brcmstb.c
> @@ -413,10 +413,10 @@ 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);
> +	writel(lnkcap, pcie->base + PCIE_RC_CFG_PRIV1_LINK_CAPABILITY);
>  
>  	lnkctl2 = (lnkctl2 & ~0xf) | gen;
>  	writew(lnkctl2, pcie->base + BRCM_PCIE_CAP_REGS + PCI_EXP_LNKCTL2);
> -- 
> 2.43.0
> 

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

Re: [PATCH v2 3/8] PCI: brcmstb: Do not assume that reg field starts at LSB

[Manivannan Sadhasivam]

On Fri, Feb 14, 2025 at 12:39:31PM -0500, Jim Quinlan wrote:
> When setting a register field it was assumed that the field started at the

s/a register field/LNKCAP and LNKCTL2 register fields,

> lsb of the register.  Although the masks do indeed start at the lsb, and
> this will probably not change, it is prudent to use a method that makes no
> assumption about the mask's placement in the register.
> 
> The uXXp_replace_bits() calls are used since they are already prevalent
> in this driver.
> 
> Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com>

Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

- Mani

> ---
>  drivers/pci/controller/pcie-brcmstb.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
> index 98542e74aa16..e0b20f58c604 100644
> --- a/drivers/pci/controller/pcie-brcmstb.c
> +++ b/drivers/pci/controller/pcie-brcmstb.c
> @@ -415,10 +415,10 @@ 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 + PCIE_RC_CFG_PRIV1_LINK_CAPABILITY);
>  
> -	lnkcap = (lnkcap & ~PCI_EXP_LNKCAP_SLS) | gen;
> +	u32p_replace_bits(&lnkcap, gen, PCI_EXP_LNKCAP_SLS);
>  	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);
>  }
>  
> -- 
> 2.43.0
> 

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

Re: [PATCH v2 4/8] PCI: brcmstb: Fix error path upon call of regulator_bulk_get()

[Manivannan Sadhasivam]

On Fri, Feb 14, 2025 at 12:39:32PM -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 e0b20f58c604..56b49d3cae19 100644
> --- a/drivers/pci/controller/pcie-brcmstb.c
> +++ b/drivers/pci/controller/pcie-brcmstb.c
> @@ -1416,7 +1416,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);

Why is this dev_info() instead of dev_err()?

> +			pcie->sr = NULL;

Why can't you set 'pcie->sr' after successfull regulator_bulk_get()?

- Mani

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

Re: [PATCH v2 5/8] PCI: brcmstb: Fix potential premature regulator disabling

[Manivannan Sadhasivam]

On Fri, Feb 14, 2025 at 12:39:33PM -0500, Jim Quinlan wrote:
> Our system for enabling and disabling regulators is designed to work only

'system'? Perhaps 'logic'?

> 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>

Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

- Mani

> ---
>  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 56b49d3cae19..e1059e3365bd 100644
> --- a/drivers/pci/controller/pcie-brcmstb.c
> +++ b/drivers/pci/controller/pcie-brcmstb.c
> @@ -1440,7 +1440,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 v2 6/8] PCI: brcmstb: Use same constant table for config space access

[Manivannan Sadhasivam]

On Fri, Feb 14, 2025 at 12:39:34PM -0500, Jim Quinlan wrote:
> 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.
> 

What is the effect of this change? Why is it required? Sounds like it got
sneaked in.

> 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 e1059e3365bd..923ac1a03f85 100644
> --- a/drivers/pci/controller/pcie-brcmstb.c
> +++ b/drivers/pci/controller/pcie-brcmstb.c
> @@ -150,9 +150,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
>  
> @@ -727,8 +724,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,
> @@ -1711,7 +1708,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,
>  };
> @@ -1719,7 +1716,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,
>  };
> @@ -1733,8 +1730,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
> 

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

Re: [PATCH v2 7/8] PCI: brcmstb: Make two changes in MDIO register fields

[Manivannan Sadhasivam]

On Fri, Feb 14, 2025 at 12:39:35PM -0500, Jim Quinlan wrote:
> 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>

Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

- Mani

> ---
>  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 923ac1a03f85..cb897d4b2579 100644
> --- a/drivers/pci/controller/pcie-brcmstb.c
> +++ b/drivers/pci/controller/pcie-brcmstb.c
> @@ -175,8 +175,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
> @@ -327,6 +328,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
> 

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

Re: [PATCH v2 8/8] PCI: brcmstb: Clarify conversion of irq_domain_set_info() param

[Manivannan Sadhasivam]

On Fri, Feb 14, 2025 at 12:39:36PM -0500, Jim Quinlan wrote:
> 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>

Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

- Mani

> ---
>  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 cb897d4b2579..f790d5252e9f 100644
> --- a/drivers/pci/controller/pcie-brcmstb.c
> +++ b/drivers/pci/controller/pcie-brcmstb.c
> @@ -559,7 +559,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 v2 0/8] PCI: brcmstb: Misc small tweaks and fixes

[Krzysztof Wilczyński]

Hello,

> >   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).
> 
> Applied to controller/brcmstb, thank you!

Updated with missing "Reviewed-by" tags that Mani recently offered.

Thank you!

	Krzysztof

Re: [PATCH v2 6/8] PCI: brcmstb: Use same constant table for config space access

[Jim Quinlan]

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

On Tue, Mar 4, 2025 at 10:08 AM Manivannan Sadhasivam
<manivannan.sadhasivam@linaro.org> wrote:
>
> On Fri, Feb 14, 2025 at 12:39:34PM -0500, Jim Quinlan wrote:
> > 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.
> >
>
> What is the effect of this change? Why is it required? Sounds like it got
> sneaked in.

Hello,
There is no functional difference with this commit -- the code will
behave the same.  A previous commit set up the "EXT_CFG_DATA" and
"EXT_CFG_INDEX" constants in the offset table but one of the map_bus()
methods did not use them, instead it relied on old generic #define
constants.  This commit uses them and gets rid of the old #defines.

I didn't add a "Fixes" line because there is no functional change but
I can if you want.

Regards,
Jim Quinlan
Broadcom STB/CM

>
> > 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 e1059e3365bd..923ac1a03f85 100644
> > --- a/drivers/pci/controller/pcie-brcmstb.c
> > +++ b/drivers/pci/controller/pcie-brcmstb.c
> > @@ -150,9 +150,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
> >
> > @@ -727,8 +724,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,
> > @@ -1711,7 +1708,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,
> >  };
> > @@ -1719,7 +1716,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,
> >  };
> > @@ -1733,8 +1730,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
> >
>
> --
> மணிவண்ணன் சதாசிவம்

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

Re: [PATCH v2 4/8] PCI: brcmstb: Fix error path upon call of regulator_bulk_get()

[Jim Quinlan]

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

On Tue, Mar 4, 2025 at 10:03 AM Manivannan Sadhasivam
<manivannan.sadhasivam@linaro.org> wrote:
>
> On Fri, Feb 14, 2025 at 12:39:32PM -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 e0b20f58c604..56b49d3cae19 100644
> > --- a/drivers/pci/controller/pcie-brcmstb.c
> > +++ b/drivers/pci/controller/pcie-brcmstb.c
> > @@ -1416,7 +1416,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);
>
> Why is this dev_info() instead of dev_err()?

I will change this.
>
> > +                     pcie->sr = NULL;
>
> Why can't you set 'pcie->sr' after successfull regulator_bulk_get()?

Not sure I understand -- it is already set before a  successful
regulator_bulk_get() call.
I set it to NULL after an unsuccessful result so the structure will
not be passed to subsequent calls.

Regards,
Jim Quinlan
Broadcom STB/CM

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

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

Re: [PATCH v2 6/8] PCI: brcmstb: Use same constant table for config space access

[Manivannan Sadhasivam]

On Tue, Mar 04, 2025 at 11:37:14AM -0500, Jim Quinlan wrote:
> On Tue, Mar 4, 2025 at 10:08 AM Manivannan Sadhasivam
> <manivannan.sadhasivam@linaro.org> wrote:
> >
> > On Fri, Feb 14, 2025 at 12:39:34PM -0500, Jim Quinlan wrote:
> > > 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.
> > >
> >
> > What is the effect of this change? Why is it required? Sounds like it got
> > sneaked in.
> 
> Hello,
> There is no functional difference with this commit -- the code will
> behave the same.  A previous commit set up the "EXT_CFG_DATA" and
> "EXT_CFG_INDEX" constants in the offset table but one of the map_bus()
> methods did not use them, instead it relied on old generic #define
> constants.  This commit uses them and gets rid of the old #defines.
> 

My comment was about the change that modified the offset of EXT_CFG_DATA. This
was not justified properly.

- Mani

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

Re: [PATCH v2 4/8] PCI: brcmstb: Fix error path upon call of regulator_bulk_get()

[Manivannan Sadhasivam]

On Tue, Mar 04, 2025 at 11:55:05AM -0500, Jim Quinlan wrote:
> On Tue, Mar 4, 2025 at 10:03 AM Manivannan Sadhasivam
> <manivannan.sadhasivam@linaro.org> wrote:
> >
> > On Fri, Feb 14, 2025 at 12:39:32PM -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 e0b20f58c604..56b49d3cae19 100644
> > > --- a/drivers/pci/controller/pcie-brcmstb.c
> > > +++ b/drivers/pci/controller/pcie-brcmstb.c
> > > @@ -1416,7 +1416,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);
> >
> > Why is this dev_info() instead of dev_err()?
> 
> I will change this.
> >
> > > +                     pcie->sr = NULL;
> >
> > Why can't you set 'pcie->sr' after successfull regulator_bulk_get()?
> 
> Not sure I understand -- it is already set before a  successful
> regulator_bulk_get() call.

Didn't I say 'after'?

> I set it to NULL after an unsuccessful result so the structure will
> not be passed to subsequent calls.
> 

If you set the pointer after a successful regulator_bulk_get(), you do not need
to set it to NULL for a failure.

- Mani

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

Re: [PATCH v2 4/8] PCI: brcmstb: Fix error path upon call of regulator_bulk_get()

[Jim Quinlan]

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

On Tue, Mar 4, 2025 at 12:07 PM Manivannan Sadhasivam
<manivannan.sadhasivam@linaro.org> wrote:
>
> On Tue, Mar 04, 2025 at 11:55:05AM -0500, Jim Quinlan wrote:
> > On Tue, Mar 4, 2025 at 10:03 AM Manivannan Sadhasivam
> > <manivannan.sadhasivam@linaro.org> wrote:
> > >
> > > On Fri, Feb 14, 2025 at 12:39:32PM -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 e0b20f58c604..56b49d3cae19 100644
> > > > --- a/drivers/pci/controller/pcie-brcmstb.c
> > > > +++ b/drivers/pci/controller/pcie-brcmstb.c
> > > > @@ -1416,7 +1416,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);
> > >
> > > Why is this dev_info() instead of dev_err()?
> >
> > I will change this.
> > >
> > > > +                     pcie->sr = NULL;
> > >
> > > Why can't you set 'pcie->sr' after successfull regulator_bulk_get()?
> >
> > Not sure I understand -- it is already set before a  successful
> > regulator_bulk_get() call.
>
> Didn't I say 'after'?

Sorry, I misinterpreted your question.  I can change it but it would
just be churn because a new commit is going to refactor this function.
However,
I can set  pcie->num_regulators "after" in the new commit.

Regards,
Jim Quinlan
Broadcom STB/CM

>
> > I set it to NULL after an unsuccessful result so the structure will
> > not be passed to subsequent calls.
> >
>
> If you set the pointer after a successful regulator_bulk_get(), you do not need
> to set it to NULL for a failure.
>
> - Mani
>
> --
> மணிவண்ணன் சதாசிவம்

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

Re: [PATCH v2 4/8] PCI: brcmstb: Fix error path upon call of regulator_bulk_get()

[Manivannan Sadhasivam]

On Tue, Mar 04, 2025 at 12:24:56PM -0500, Jim Quinlan wrote:
> On Tue, Mar 4, 2025 at 12:07 PM Manivannan Sadhasivam
> <manivannan.sadhasivam@linaro.org> wrote:
> >
> > On Tue, Mar 04, 2025 at 11:55:05AM -0500, Jim Quinlan wrote:
> > > On Tue, Mar 4, 2025 at 10:03 AM Manivannan Sadhasivam
> > > <manivannan.sadhasivam@linaro.org> wrote:
> > > >
> > > > On Fri, Feb 14, 2025 at 12:39:32PM -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 e0b20f58c604..56b49d3cae19 100644
> > > > > --- a/drivers/pci/controller/pcie-brcmstb.c
> > > > > +++ b/drivers/pci/controller/pcie-brcmstb.c
> > > > > @@ -1416,7 +1416,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);
> > > >
> > > > Why is this dev_info() instead of dev_err()?
> > >
> > > I will change this.
> > > >
> > > > > +                     pcie->sr = NULL;
> > > >
> > > > Why can't you set 'pcie->sr' after successfull regulator_bulk_get()?
> > >
> > > Not sure I understand -- it is already set before a  successful
> > > regulator_bulk_get() call.
> >
> > Didn't I say 'after'?
> 
> Sorry, I misinterpreted your question.  I can change it but it would
> just be churn because a new commit is going to refactor this function.
> However,
> I can set  pcie->num_regulators "after" in the new commit.
> 

If a new patch is going to change the beahvior, then I'm fine with it for now.
After all, this series is already merged.

- Mani

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

Re: [PATCH v2 6/8] PCI: brcmstb: Use same constant table for config space access

[Jim Quinlan]

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

On Tue, Mar 4, 2025 at 11:58 AM Manivannan Sadhasivam
<manivannan.sadhasivam@linaro.org> wrote:
>
> On Tue, Mar 04, 2025 at 11:37:14AM -0500, Jim Quinlan wrote:
> > On Tue, Mar 4, 2025 at 10:08 AM Manivannan Sadhasivam
> > <manivannan.sadhasivam@linaro.org> wrote:
> > >
> > > On Fri, Feb 14, 2025 at 12:39:34PM -0500, Jim Quinlan wrote:
> > > > 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.
> > > >
> > >
> > > What is the effect of this change? Why is it required? Sounds like it got
> > > sneaked in.
> >
> > Hello,
> > There is no functional difference with this commit -- the code will
> > behave the same.  A previous commit set up the "EXT_CFG_DATA" and
> > "EXT_CFG_INDEX" constants in the offset table but one of the map_bus()
> > methods did not use them, instead it relied on old generic #define
> > constants.  This commit uses them and gets rid of the old #defines.
> >
>
> My comment was about the change that modified the offset of EXT_CFG_DATA. This
> was not justified properly.

Okay, got it.  You are referring to (for example)
-      [EXT_CFG_DATA]          = 0x9004,
+       [EXT_CFG_DATA]          = 0x8000,

We have two ways of accessing the config space: (1) by writing a full
index and reading a  designated register (0x9004) and (2) by writing
the index and then reading from a 4k register region (0x8000 +
offset).  We previously used (1).  An update was made to use (2) but
instead of updating EXT_CFG_DATA from 0x9004 to 0x8000,
PCIE_EXT_CFG_DATA (0x8000) was used by the code of one of the map_bus
methods.

This commit changes the code in the offending map_bus method to use
the offset table for (2) and updates the offset table EXT_CFG_DATA to
its proper value.

If you want me to expand the commit message with the above text I can do that.

Regards,
Jim Quiinlan
Broadcom STB/CM

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

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

Re: [PATCH v2 4/8] PCI: brcmstb: Fix error path upon call of regulator_bulk_get()

[Krzysztof Wilczyński]

Hello,

[...]
> > > > > > -                     dev_info(dev, "No regulators for downstream device\n");
> > > > > > +                     dev_info(dev, "Did not get regulators; err=%d\n", ret);
> > > > >
> > > > > Why is this dev_info() instead of dev_err()?
> > > >
> > > > I will change this.

I can update this directly on the branch if you want.

> > > > >
> > > > > > +                     pcie->sr = NULL;
> > > > >
> > > > > Why can't you set 'pcie->sr' after successfull regulator_bulk_get()?
> > > >
> > > > Not sure I understand -- it is already set before a  successful
> > > > regulator_bulk_get() call.
> > >
> > > Didn't I say 'after'?
> > 
> > Sorry, I misinterpreted your question.  I can change it but it would
> > just be churn because a new commit is going to refactor this function.
> > However,
> > I can set  pcie->num_regulators "after" in the new commit.
> > 
> 
> If a new patch is going to change the beahvior, then I'm fine with it for now.
> After all, this series is already merged.

There is still time if a follow-up patch would be of benefit there.

Same goes for

	Krzysztof

Re: [PATCH v2 6/8] PCI: brcmstb: Use same constant table for config space access

[Manivannan Sadhasivam]

On Tue, Mar 04, 2025 at 12:37:26PM -0500, Jim Quinlan wrote:
> On Tue, Mar 4, 2025 at 11:58 AM Manivannan Sadhasivam
> <manivannan.sadhasivam@linaro.org> wrote:
> >
> > On Tue, Mar 04, 2025 at 11:37:14AM -0500, Jim Quinlan wrote:
> > > On Tue, Mar 4, 2025 at 10:08 AM Manivannan Sadhasivam
> > > <manivannan.sadhasivam@linaro.org> wrote:
> > > >
> > > > On Fri, Feb 14, 2025 at 12:39:34PM -0500, Jim Quinlan wrote:
> > > > > 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.
> > > > >
> > > >
> > > > What is the effect of this change? Why is it required? Sounds like it got
> > > > sneaked in.
> > >
> > > Hello,
> > > There is no functional difference with this commit -- the code will
> > > behave the same.  A previous commit set up the "EXT_CFG_DATA" and
> > > "EXT_CFG_INDEX" constants in the offset table but one of the map_bus()
> > > methods did not use them, instead it relied on old generic #define
> > > constants.  This commit uses them and gets rid of the old #defines.
> > >
> >
> > My comment was about the change that modified the offset of EXT_CFG_DATA. This
> > was not justified properly.
> 
> Okay, got it.  You are referring to (for example)
> -      [EXT_CFG_DATA]          = 0x9004,
> +       [EXT_CFG_DATA]          = 0x8000,
> 
> We have two ways of accessing the config space: (1) by writing a full
> index and reading a  designated register (0x9004) and (2) by writing
> the index and then reading from a 4k register region (0x8000 +
> offset).  We previously used (1).  An update was made to use (2) but
> instead of updating EXT_CFG_DATA from 0x9004 to 0x8000,
> PCIE_EXT_CFG_DATA (0x8000) was used by the code of one of the map_bus
> methods.
> 
> This commit changes the code in the offending map_bus method to use
> the offset table for (2) and updates the offset table EXT_CFG_DATA to
> its proper value.
> 

Ok, this clarifies.

> If you want me to expand the commit message with the above text I can do that.
> 

No need. Krzysztof should be able to ammend the commit message for you.

- Mani

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

Re: [PATCH v2 4/8] PCI: brcmstb: Fix error path upon call of regulator_bulk_get()

[Jim Quinlan]

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

On Tue, Mar 4, 2025 at 9:49 AM Jim Quinlan <james.quinlan@broadcom.com> wrote:
>
> On Mon, Mar 3, 2025 at 1:40 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
> >
> > On Fri, Feb 14, 2025 at 12:39:32PM -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 e0b20f58c604..56b49d3cae19 100644
> > > --- a/drivers/pci/controller/pcie-brcmstb.c
> > > +++ b/drivers/pci/controller/pcie-brcmstb.c
> > > @@ -1416,7 +1416,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);
> > > +                     pcie->sr = NULL;
> >
> > Is alloc_subdev_regulators() buying us something useful?  It seems
> > like it would be simpler to have:
> >
> >   struct brcm_pcie {
> >     ...
> >     struct regulator_bulk_data supplies[3];
> >     ...
> >   };
> >
> > I think that's what most callers of devm_regulator_bulk_get() do.
>
> Ack.

Hi Bjorn,

Manivannan stated that this series has already been merged.  So shall
I implement your comments with a commit sometime in the future?

Thanks,
Jim Quinlan
Broadcom STB/CM

>
> Jim Quinlan
> Broadcom STB/CM
> >
> > >                       goto no_regulators;
> > >               }
> > >
> > > --
> > > 2.43.0
> > >

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

Re: [PATCH v2 4/8] PCI: brcmstb: Fix error path upon call of regulator_bulk_get()

[Bjorn Helgaas]

On Thu, Mar 06, 2025 at 10:24:56AM -0500, Jim Quinlan wrote:
> On Tue, Mar 4, 2025 at 9:49 AM Jim Quinlan <james.quinlan@broadcom.com> wrote:
> > On Mon, Mar 3, 2025 at 1:40 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
> > > On Fri, Feb 14, 2025 at 12:39:32PM -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 e0b20f58c604..56b49d3cae19 100644
> > > > --- a/drivers/pci/controller/pcie-brcmstb.c
> > > > +++ b/drivers/pci/controller/pcie-brcmstb.c
> > > > @@ -1416,7 +1416,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);
> > > > +                     pcie->sr = NULL;
> > >
> > > Is alloc_subdev_regulators() buying us something useful?  It seems
> > > like it would be simpler to have:
> > >
> > >   struct brcm_pcie {
> > >     ...
> > >     struct regulator_bulk_data supplies[3];
> > >     ...
> > >   };
> > >
> > > I think that's what most callers of devm_regulator_bulk_get() do.
> 
> Manivannan stated that this series has already been merged.  So shall
> I implement your comments with a commit sometime in the future?

Sorry, I should have mentioned this would be something possible for
the future.  This current series is all set to go.

Bjorn