[PATCH v6 00/13] PCI: brcnstb: Enable STB 7712 SOC

[PATCH v6 00/13] PCI: brcnstb: Enable STB 7712 SOC

[Jim Quinlan]

V6 Changes
  o Commit "Refactor for chips with many regular inbound windows"
    -- Use u8 for anything storing/counting # inbound windows (Stan)
    -- s/set_bar/add_inbound_win/g (Manivannan)
    -- Drop use of "inline" (Manivannan)
    -- Change cpu_beg to cpu_start, same with pcie_beg. (Manivannan)
    -- Used writel_relaxed() (Manivannan)
  o Use swinit reset if available
    -- Proper use of dev_err_probe() (Stan)
  o Commit "Use common error handling code in brcm_pcie_probe()"
    -- Rewrite commit msg in paragraph form (Manivannan)
    -- Refactor error path at end of probe func (Manivannan)
    -- Proper use of dev_err_probe() (Stan)
  o New commit "dt-bindings: PCI: Change brcmstb maintainer and cleanup"
    -- Break out maintainer change and small cleanup into a
       separate commit (Krzysztof)

V5 Changes:
  o All commits: Use imperative voice in commit subjects/messages
       (Manivannan)
  o Commit "PCI: brcmstb: Enable 7712 SOCs"
    -- Augment commit message to include PCIe details and revision.
       (Manivannan)
  o Commit "PCI: brcmstb: Change field name from 'type' to 'model'"
    -- Instead of "model" use "soc_base" (Manivannan)
  o Commit "PCI: brcmstb: Refactor for chips with many regular inbound BARs"
    -- Get rid of the confusing "BAR" variable names and types and use
       something like "inbound_win". (Manivannan)
  o Commit "PCI: brcmstb: PCI: brcmstb: Make HARD_DEBUG, INTR2_CPU_BASE..."
    -- Mention in the commit message that this change is in preparation
       for the 7712 SoC. (Manivannan)
  o Commit: "PCI: brcmstb: Use swinit reset if available"
    -- Change reset name "swinit" to "swinit_reset" (Manivannan)
    -- Add 1us delay for reset (Manivannan)
    -- Use dev_err_probe() (Multiple reviewers)
  o Commit "PCI: brcmstb: Use bridge reset if available"
    -- Change reset name "bridge" to "bridge_reset" (Manivannan)
    -- The Reset API can take NULL so need need to test variable
       before calling (Manivannan)
    -- Added a call to bridge_sw_init_set() method in probe()
       as some registers cannot be accessed w/o this. (JQ)
  o Commit "PCI: brcmstb: Use common error handling code in ..."
    -- Use more descriptive goto label (Manivannan)
    -- Refactor error paths to be less encumbered (Manivannan)
    -- Use dev_err_probe() (Multiple reviewers)
  o Commits "dt-bindings: PCI: brcmstb: ..."
    -- Specify the "resets" and "reset-names" in the same manner
       as does qcom,ufs.yaml specifies "clocks" and
       "clock-names" (Krzysztof)
    -- Drop reset desccriptions as they were pretty content-free
       anyhow. (Krzysztof)

V4 Changes:
  o Commit "Check return value of all reset_control_xxx calls"
    -- Blank line before "return" (Stan)
  o Commit "Use common error handling code in brcmstb_probe()"
    -- Drop the "Fixes" tag (Stan)
  o Commit "dt-bindings: PCI ..."
    -- Separate the main commit into two: cleanup and adding the
       7712 SoC (Krzysztof)
    -- Fold maintainer change commit into cleanup change (Krzysztof)
    -- Use minItems/maxItems where appropriate (Krzysztof)
    -- Consistent order of resets/reset-names in decl and usage
       (Krzysztof)

V3 Changes:
  o Commit "Enable 7712 SOCs"
    -- Move "model" check from outside to inside func (Stan)
  o Commit "Check return value of all reset_control_xxx calls"
    -- Propagate errors up the chain instead of ignoring them (Stan)
  o Commit "Refactor for chips with many regular inbound BARs"
    -- Nine suggestions given, nine implemented (Stan)
  o Commit "Make HARD_DEBUG, INTR2_CPU_BASE offsets SoC-specific"
    -- Drop tab, add parens around macro params in expression (Stan)
  o Commit "Use swinit reset if available"
    -- Treat swinit the same as other reset controllers (Stan)
       Stan suggested to use dev_err_probe() for getting resources
       but I will defer that to future series (if that's okay).
  o Commit "Get resource before we start asserting resets"
    -- Squash this with previous commit (Stan)
  o Commit "Use "clk_out" error path label"
    -- Move clk_prepare_enable() after getting resouurces (Stan)
    -- Change subject to "Use more common error handling code in
       brcm_pcie_probe()" (Markus)
    -- Use imperative commit description (Markus)
    -- "Fixes:" tag added for missing error return. (Markus)
  o Commit "dt-bindings: PCI ..."
    -- Split off maintainer change in separate commit.
    -- Tried to accomodate Krzysztof's requests, I'm not sure I
       have succeeded.  Krzysztof, please see [1] below.
  
  [1] Wrt the YAML of brcmstb PCIe resets, here is what I am trying
      to describe:

      CHIP       NUM_RESETS    NAMES
      ====       ==========    =====
      4908       1             perst
      7216       1             rescal
      7712       3             rescal, bridge, swinit
      Others     0             -


V2 Changes (note: four new commits):
  o Commit "dt-bindings: PCI ..."
    -- s/Adds/Add/, fix spelling error (Bjorn)
    -- Order compatible strings alphabetically (Krzysztof)
    -- Give definitions first then rules (Krzysztof)
    -- Add reason for change in maintainer (Krzysztof)
  o Commit "Use swinit reset if available"
    -- no need for "else" clause (Philipp)
    -- fix improper use of dev_err_probe() (Philipp) 
  o Commit "Use "clk_out" error path label"
    -- Improve commit message (Bjorn)
  o Commit "PCI: brcmstb: Make HARD_DEBUG, INTR2_CPU_BASE offsets SoC-specific"
    -- Improve commit subject line (Bjorn)
  o Commit (NEW) -- Change field name from 'type' to 'model'
    -- Added as requested (Stanimir)
  o Commit (NEW) -- Check return value of all reset_control_xxx calls
    -- Added as requested (Stanimir)
  o Commit (NEW) "Get resource before we start asserting reset controllers"
    -- Added as requested (Stanimir)
  o Commit (NEW) -- "Remove two unused constants from driver"


V1:
  This submission is for the Broadcom STB 7712, sibling SOC of the RPi5 chip.
  Stanimir has already submitted a patch "Add PCIe support for bcm2712" for
  the RPi version of the SOC.  It is hoped that Stanimir will allow us to
  submit this series first and subsequently rebase his patch(es).

  The largest commit, "Refactor for chips with many regular inbound BARs"
  affects both the STB and RPi SOCs.  It allows for multiple inbound ranges
  where previously only one was effectively used.  This feature will also
  be present in future STB chips, as well as Broadcom's Cable Modem group.

Jim Quinlan (13):
  dt-bindings: PCI: Change brcmstb maintainer and cleanup
  dt-bindings: PCI: Use maxItems for reset controllers
  dt-bindings: PCI: brcmstb: Add 7712 SoC description
  PCI: brcmstb: Use common error handling code in brcm_pcie_probe()
  PCI: brcmstb: Use bridge reset if available
  PCI: brcmstb: Use swinit reset if available
  PCI: brcmstb: PCI: brcmstb: Make HARD_DEBUG, INTR2_CPU_BASE offsets
    SoC-specific
  PCI: brcmstb: Remove two unused constants from driver
  PCI: brcmstb: Don't conflate the reset rescal with phy ctrl
  PCI: brcmstb: Refactor for chips with many regular inbound windows
  PCI: brcmstb: Check return value of all reset_control_xxx calls
  PCI: brcmstb: Change field name from 'type' to 'soc_base'
  PCI: brcmstb: Enable 7712 SOCs

 .../bindings/pci/brcm,stb-pcie.yaml           |  40 +-
 drivers/pci/controller/pcie-brcmstb.c         | 513 +++++++++++++-----
 2 files changed, 412 insertions(+), 141 deletions(-)


base-commit: e724918b3786252b985b0c2764c16a57d1937707
-- 
2.17.1

[PATCH v6 01/13] dt-bindings: PCI: Change brcmstb maintainer and cleanup

[Jim Quinlan]

Change maintainer: Nicolas has not been active for a while.
It also makes sense for a Broadcom employee to be the
maintainer as many of the details are privy to Broadcom.

Also, alphabetize the compatible strings.

Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com>
---
 Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml b/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
index 11f8ea33240c..a95760357335 100644
--- a/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
+++ b/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
@@ -7,7 +7,7 @@ $schema: http://devicetree.org/meta-schemas/core.yaml#
 title: Brcmstb PCIe Host Controller
 
 maintainers:
-  - Nicolas Saenz Julienne <nsaenzjulienne@suse.de>
+  - Jim Quinlan <james.quinlan@broadcom.com>
 
 properties:
   compatible:
@@ -16,11 +16,11 @@ properties:
           - brcm,bcm2711-pcie # The Raspberry Pi 4
           - brcm,bcm4908-pcie
           - brcm,bcm7211-pcie # Broadcom STB version of RPi4
-          - brcm,bcm7278-pcie # Broadcom 7278 Arm
           - brcm,bcm7216-pcie # Broadcom 7216 Arm
-          - brcm,bcm7445-pcie # Broadcom 7445 Arm
+          - brcm,bcm7278-pcie # Broadcom 7278 Arm
           - brcm,bcm7425-pcie # Broadcom 7425 MIPs
           - brcm,bcm7435-pcie # Broadcom 7435 MIPs
+          - brcm,bcm7445-pcie # Broadcom 7445 Arm
 
   reg:
     maxItems: 1
-- 
2.17.1

[PATCH v6 02/13] dt-bindings: PCI: Use maxItems for reset controllers

[Jim Quinlan]

Provide the maxItem property for the reset controllers and drop their
superfluous descriptions.

Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com>
---
 .../devicetree/bindings/pci/brcm,stb-pcie.yaml       | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml b/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
index a95760357335..7d2552192153 100644
--- a/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
+++ b/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
@@ -95,6 +95,12 @@ properties:
       minItems: 1
       maxItems: 3
 
+  resets:
+    maxItems: 1
+
+  reset-names:
+    maxItems: 1
+
 required:
   - compatible
   - reg
@@ -118,8 +124,7 @@ allOf:
     then:
       properties:
         resets:
-          items:
-            - description: reset controller handling the PERST# signal
+          maxItems: 1
 
         reset-names:
           items:
@@ -136,8 +141,7 @@ allOf:
     then:
       properties:
         resets:
-          items:
-            - description: phandle pointing to the RESCAL reset controller
+          maxItems: 1
 
         reset-names:
           items:
-- 
2.17.1

[PATCH v6 03/13] dt-bindings: PCI: brcmstb: Add 7712 SoC description

[Jim Quinlan]

Add description for the 7712 SoC, a Broadcom STB sibling chip of the RPi 5.
The 7712 uses three reset controllers: rescal, for phy reset calibration;
bridge, for the bridge between the PCIe bus and the memory bus; and swinit,
which is a "soft" initialization of the PCIe HW.

Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com>
Reviewed-by: Florian Fainelli <florian.fainelli@broadcom.com>
Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
---
 .../bindings/pci/brcm,stb-pcie.yaml           | 28 +++++++++++++++++--
 1 file changed, 26 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml b/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
index 7d2552192153..0925c520195a 100644
--- a/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
+++ b/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
@@ -21,6 +21,7 @@ properties:
           - brcm,bcm7425-pcie # Broadcom 7425 MIPs
           - brcm,bcm7435-pcie # Broadcom 7435 MIPs
           - brcm,bcm7445-pcie # Broadcom 7445 Arm
+          - brcm,bcm7712-pcie # Broadcom STB sibling of Rpi 5
 
   reg:
     maxItems: 1
@@ -96,10 +97,12 @@ properties:
       maxItems: 3
 
   resets:
-    maxItems: 1
+    minItems: 1
+    maxItems: 3
 
   reset-names:
-    maxItems: 1
+    minItems: 1
+    maxItems: 3
 
 required:
   - compatible
@@ -151,6 +154,27 @@ allOf:
         - resets
         - reset-names
 
+  - if:
+      properties:
+        compatible:
+          contains:
+            const: brcm,bcm7712-pcie
+    then:
+      properties:
+        resets:
+          minItems: 3
+          maxItems: 3
+
+        reset-names:
+          items:
+            - const: rescal
+            - const: bridge
+            - const: swinit
+
+      required:
+        - resets
+        - reset-names
+
 unevaluatedProperties: false
 
 examples:
-- 
2.17.1

[PATCH v6 04/13] PCI: brcmstb: Use common error handling code in brcm_pcie_probe()

[Jim Quinlan]

Refactor the error handling in the bottom half of the probe
function for readability.  The invocation of clk_prepare_enable()
is moved lower in the function and this simplifies a couple
of return paths.  dev_err_probe() is also used when it is apt.

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

diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
index c08683febdd4..790a149f6581 100644
--- a/drivers/pci/controller/pcie-brcmstb.c
+++ b/drivers/pci/controller/pcie-brcmstb.c
@@ -1613,25 +1613,23 @@ static int brcm_pcie_probe(struct platform_device *pdev)
 
 	pcie->ssc = of_property_read_bool(np, "brcm,enable-ssc");
 
-	ret = clk_prepare_enable(pcie->clk);
-	if (ret) {
-		dev_err(&pdev->dev, "could not enable clock\n");
-		return ret;
-	}
 	pcie->rescal = devm_reset_control_get_optional_shared(&pdev->dev, "rescal");
-	if (IS_ERR(pcie->rescal)) {
-		clk_disable_unprepare(pcie->clk);
+	if (IS_ERR(pcie->rescal))
 		return PTR_ERR(pcie->rescal);
-	}
+
 	pcie->perst_reset = devm_reset_control_get_optional_exclusive(&pdev->dev, "perst");
-	if (IS_ERR(pcie->perst_reset)) {
-		clk_disable_unprepare(pcie->clk);
+	if (IS_ERR(pcie->perst_reset))
 		return PTR_ERR(pcie->perst_reset);
-	}
 
-	ret = reset_control_reset(pcie->rescal);
+	ret = clk_prepare_enable(pcie->clk);
 	if (ret)
-		dev_err(&pdev->dev, "failed to deassert 'rescal'\n");
+		return dev_err_probe(&pdev->dev, ret, "could not enable clock\n");
+
+	ret = reset_control_reset(pcie->rescal);
+	if (ret) {
+		clk_disable_unprepare(pcie->clk);
+		return dev_err_probe(&pdev->dev, ret, "failed to deassert 'rescal'\n");
+	}
 
 	ret = brcm_phy_start(pcie);
 	if (ret) {
@@ -1678,6 +1676,7 @@ static int brcm_pcie_probe(struct platform_device *pdev)
 
 fail:
 	__brcm_pcie_remove(pcie);
+
 	return ret;
 }
 
-- 
2.17.1

[PATCH v6 05/13] PCI: brcmstb: Use bridge reset if available

[Jim Quinlan]

The 7712 SOC has a bridge reset which can be described in the device tree.
Use it if present.  Otherwise, continue to use the legacy method to reset
the bridge.

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

diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
index 790a149f6581..af14debd81d0 100644
--- a/drivers/pci/controller/pcie-brcmstb.c
+++ b/drivers/pci/controller/pcie-brcmstb.c
@@ -265,6 +265,7 @@ struct brcm_pcie {
 	enum pcie_type		type;
 	struct reset_control	*rescal;
 	struct reset_control	*perst_reset;
+	struct reset_control	*bridge_reset;
 	int			num_memc;
 	u64			memc_size[PCIE_BRCM_MAX_MEMC];
 	u32			hw_rev;
@@ -732,12 +733,19 @@ static void __iomem *brcm7425_pcie_map_bus(struct pci_bus *bus,
 
 static void brcm_pcie_bridge_sw_init_set_generic(struct brcm_pcie *pcie, u32 val)
 {
-	u32 tmp, mask =  RGR1_SW_INIT_1_INIT_GENERIC_MASK;
-	u32 shift = RGR1_SW_INIT_1_INIT_GENERIC_SHIFT;
+	if (val)
+		reset_control_assert(pcie->bridge_reset);
+	else
+		reset_control_deassert(pcie->bridge_reset);
 
-	tmp = readl(pcie->base + PCIE_RGR1_SW_INIT_1(pcie));
-	tmp = (tmp & ~mask) | ((val << shift) & mask);
-	writel(tmp, pcie->base + PCIE_RGR1_SW_INIT_1(pcie));
+	if (!pcie->bridge_reset) {
+		u32 tmp, mask =  RGR1_SW_INIT_1_INIT_GENERIC_MASK;
+		u32 shift = RGR1_SW_INIT_1_INIT_GENERIC_SHIFT;
+
+		tmp = readl(pcie->base + PCIE_RGR1_SW_INIT_1(pcie));
+		tmp = (tmp & ~mask) | ((val << shift) & mask);
+		writel(tmp, pcie->base + PCIE_RGR1_SW_INIT_1(pcie));
+	}
 }
 
 static void brcm_pcie_bridge_sw_init_set_7278(struct brcm_pcie *pcie, u32 val)
@@ -1621,10 +1629,16 @@ static int brcm_pcie_probe(struct platform_device *pdev)
 	if (IS_ERR(pcie->perst_reset))
 		return PTR_ERR(pcie->perst_reset);
 
+	pcie->bridge_reset = devm_reset_control_get_optional_exclusive(&pdev->dev, "bridge");
+	if (IS_ERR(pcie->bridge_reset))
+		return PTR_ERR(pcie->bridge_reset);
+
 	ret = clk_prepare_enable(pcie->clk);
 	if (ret)
 		return dev_err_probe(&pdev->dev, ret, "could not enable clock\n");
 
+	pcie->bridge_sw_init_set(pcie, 0);
+
 	ret = reset_control_reset(pcie->rescal);
 	if (ret) {
 		clk_disable_unprepare(pcie->clk);
-- 
2.17.1

[PATCH v6 06/13] PCI: brcmstb: Use swinit reset if available

[Jim Quinlan]

The 7712 SOC adds a software init reset device for the PCIe HW.
If found in the DT node, use it.

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

diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
index af14debd81d0..aa21c4c7b7f7 100644
--- a/drivers/pci/controller/pcie-brcmstb.c
+++ b/drivers/pci/controller/pcie-brcmstb.c
@@ -266,6 +266,7 @@ struct brcm_pcie {
 	struct reset_control	*rescal;
 	struct reset_control	*perst_reset;
 	struct reset_control	*bridge_reset;
+	struct reset_control	*swinit_reset;
 	int			num_memc;
 	u64			memc_size[PCIE_BRCM_MAX_MEMC];
 	u32			hw_rev;
@@ -1633,12 +1634,35 @@ static int brcm_pcie_probe(struct platform_device *pdev)
 	if (IS_ERR(pcie->bridge_reset))
 		return PTR_ERR(pcie->bridge_reset);
 
+	pcie->swinit_reset = devm_reset_control_get_optional_exclusive(&pdev->dev, "swinit");
+	if (IS_ERR(pcie->swinit_reset))
+		return PTR_ERR(pcie->swinit_reset);
+
 	ret = clk_prepare_enable(pcie->clk);
 	if (ret)
 		return dev_err_probe(&pdev->dev, ret, "could not enable clock\n");
 
 	pcie->bridge_sw_init_set(pcie, 0);
 
+	if (pcie->swinit_reset) {
+		ret = reset_control_assert(pcie->swinit_reset);
+		if (ret) {
+			clk_disable_unprepare(pcie->clk);
+			return dev_err_probe(&pdev->dev, ret,
+					     "could not assert reset 'swinit'\n");
+		}
+
+		/* HW team recommends 1us for proper sync and propagation of reset */
+		udelay(1);
+
+		ret = reset_control_deassert(pcie->swinit_reset);
+		if (ret) {
+			clk_disable_unprepare(pcie->clk);
+			return dev_err_probe(&pdev->dev, ret,
+					     "could not de-assert reset 'swinit'\n");
+		}
+	}
+
 	ret = reset_control_reset(pcie->rescal);
 	if (ret) {
 		clk_disable_unprepare(pcie->clk);
-- 
2.17.1

[PATCH v6 07/13] PCI: brcmstb: PCI: brcmstb: Make HARD_DEBUG, INTR2_CPU_BASE offsets SoC-specific

[Jim Quinlan]

Do prepatory work for the 7712 SoC, which is introduced in a future commit.
Our HW design has changed two register offsets for the 7712, where
previously it was a common value for all Broadcom SOCs with PCIe cores.
Specifically, the two offsets are to the registers HARD_DEBUG and
INTR2_CPU_BASE.

Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com>
Reviewed-by: Stanimir Varbanov <svarbanov@suse.de>
Reviewed-by: Florian Fainelli <florian.fainelli@broadcom.com>
Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---
 drivers/pci/controller/pcie-brcmstb.c | 39 ++++++++++++++++-----------
 1 file changed, 24 insertions(+), 15 deletions(-)

diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
index aa21c4c7b7f7..1444f2a9c21e 100644
--- a/drivers/pci/controller/pcie-brcmstb.c
+++ b/drivers/pci/controller/pcie-brcmstb.c
@@ -122,7 +122,6 @@
 #define PCIE_MEM_WIN0_LIMIT_HI(win)	\
 		PCIE_MISC_CPU_2_PCIE_MEM_WIN0_LIMIT_HI + ((win) * 8)
 
-#define PCIE_MISC_HARD_PCIE_HARD_DEBUG					0x4204
 #define  PCIE_MISC_HARD_PCIE_HARD_DEBUG_CLKREQ_DEBUG_ENABLE_MASK	0x2
 #define  PCIE_MISC_HARD_PCIE_HARD_DEBUG_L1SS_ENABLE_MASK		0x200000
 #define  PCIE_MISC_HARD_PCIE_HARD_DEBUG_SERDES_IDDQ_MASK		0x08000000
@@ -131,9 +130,9 @@
 	  (PCIE_MISC_HARD_PCIE_HARD_DEBUG_CLKREQ_DEBUG_ENABLE_MASK | \
 	   PCIE_MISC_HARD_PCIE_HARD_DEBUG_L1SS_ENABLE_MASK)
 
-#define PCIE_INTR2_CPU_BASE		0x4300
 #define PCIE_MSI_INTR2_BASE		0x4500
-/* Offsets from PCIE_INTR2_CPU_BASE and PCIE_MSI_INTR2_BASE */
+
+/* Offsets from INTR2_CPU and MSI_INTR2 BASE offsets */
 #define  MSI_INT_STATUS			0x0
 #define  MSI_INT_CLR			0x8
 #define  MSI_INT_MASK_SET		0x10
@@ -184,9 +183,11 @@
 #define SSC_STATUS_PLL_LOCK_MASK	0x800
 #define PCIE_BRCM_MAX_MEMC		3
 
-#define IDX_ADDR(pcie)			(pcie->reg_offsets[EXT_CFG_INDEX])
-#define DATA_ADDR(pcie)			(pcie->reg_offsets[EXT_CFG_DATA])
-#define PCIE_RGR1_SW_INIT_1(pcie)	(pcie->reg_offsets[RGR1_SW_INIT_1])
+#define IDX_ADDR(pcie)			((pcie)->reg_offsets[EXT_CFG_INDEX])
+#define DATA_ADDR(pcie)			((pcie)->reg_offsets[EXT_CFG_DATA])
+#define PCIE_RGR1_SW_INIT_1(pcie)	((pcie)->reg_offsets[RGR1_SW_INIT_1])
+#define HARD_DEBUG(pcie)		((pcie)->reg_offsets[PCIE_HARD_DEBUG])
+#define INTR2_CPU_BASE(pcie)		((pcie)->reg_offsets[PCIE_INTR2_CPU_BASE])
 
 /* Rescal registers */
 #define PCIE_DVT_PMU_PCIE_PHY_CTRL				0xc700
@@ -205,6 +206,8 @@ enum {
 	RGR1_SW_INIT_1,
 	EXT_CFG_INDEX,
 	EXT_CFG_DATA,
+	PCIE_HARD_DEBUG,
+	PCIE_INTR2_CPU_BASE,
 };
 
 enum {
@@ -651,7 +654,7 @@ static int brcm_pcie_enable_msi(struct brcm_pcie *pcie)
 	BUILD_BUG_ON(BRCM_INT_PCI_MSI_LEGACY_NR > BRCM_INT_PCI_MSI_NR);
 
 	if (msi->legacy) {
-		msi->intr_base = msi->base + PCIE_INTR2_CPU_BASE;
+		msi->intr_base = msi->base + INTR2_CPU_BASE(pcie);
 		msi->nr = BRCM_INT_PCI_MSI_LEGACY_NR;
 		msi->legacy_shift = 24;
 	} else {
@@ -898,12 +901,12 @@ static int brcm_pcie_setup(struct brcm_pcie *pcie)
 	/* Take the bridge out of reset */
 	pcie->bridge_sw_init_set(pcie, 0);
 
-	tmp = readl(base + PCIE_MISC_HARD_PCIE_HARD_DEBUG);
+	tmp = readl(base + HARD_DEBUG(pcie));
 	if (is_bmips(pcie))
 		tmp &= ~PCIE_BMIPS_MISC_HARD_PCIE_HARD_DEBUG_SERDES_IDDQ_MASK;
 	else
 		tmp &= ~PCIE_MISC_HARD_PCIE_HARD_DEBUG_SERDES_IDDQ_MASK;
-	writel(tmp, base + PCIE_MISC_HARD_PCIE_HARD_DEBUG);
+	writel(tmp, base + HARD_DEBUG(pcie));
 	/* Wait for SerDes to be stable */
 	usleep_range(100, 200);
 
@@ -1072,7 +1075,7 @@ static void brcm_config_clkreq(struct brcm_pcie *pcie)
 	}
 
 	/* Start out assuming safe mode (both mode bits cleared) */
-	clkreq_cntl = readl(pcie->base + PCIE_MISC_HARD_PCIE_HARD_DEBUG);
+	clkreq_cntl = readl(pcie->base + HARD_DEBUG(pcie));
 	clkreq_cntl &= ~PCIE_CLKREQ_MASK;
 
 	if (strcmp(mode, "no-l1ss") == 0) {
@@ -1115,7 +1118,7 @@ static void brcm_config_clkreq(struct brcm_pcie *pcie)
 			dev_err(pcie->dev, err_msg);
 		mode = "safe";
 	}
-	writel(clkreq_cntl, pcie->base + PCIE_MISC_HARD_PCIE_HARD_DEBUG);
+	writel(clkreq_cntl, pcie->base + HARD_DEBUG(pcie));
 
 	dev_info(pcie->dev, "clkreq-mode set to %s\n", mode);
 }
@@ -1337,9 +1340,9 @@ static void brcm_pcie_turn_off(struct brcm_pcie *pcie)
 	writel(tmp, base + PCIE_MISC_PCIE_CTRL);
 
 	/* Turn off SerDes */
-	tmp = readl(base + PCIE_MISC_HARD_PCIE_HARD_DEBUG);
+	tmp = readl(base + HARD_DEBUG(pcie));
 	u32p_replace_bits(&tmp, 1, PCIE_MISC_HARD_PCIE_HARD_DEBUG_SERDES_IDDQ_MASK);
-	writel(tmp, base + PCIE_MISC_HARD_PCIE_HARD_DEBUG);
+	writel(tmp, base + HARD_DEBUG(pcie));
 
 	/* Shutdown PCIe bridge */
 	pcie->bridge_sw_init_set(pcie, 1);
@@ -1425,9 +1428,9 @@ static int brcm_pcie_resume_noirq(struct device *dev)
 	pcie->bridge_sw_init_set(pcie, 0);
 
 	/* SERDES_IDDQ = 0 */
-	tmp = readl(base + PCIE_MISC_HARD_PCIE_HARD_DEBUG);
+	tmp = readl(base + HARD_DEBUG(pcie));
 	u32p_replace_bits(&tmp, 0, PCIE_MISC_HARD_PCIE_HARD_DEBUG_SERDES_IDDQ_MASK);
-	writel(tmp, base + PCIE_MISC_HARD_PCIE_HARD_DEBUG);
+	writel(tmp, base + HARD_DEBUG(pcie));
 
 	/* wait for serdes to be stable */
 	udelay(100);
@@ -1499,12 +1502,16 @@ static const int pcie_offsets[] = {
 	[RGR1_SW_INIT_1] = 0x9210,
 	[EXT_CFG_INDEX]  = 0x9000,
 	[EXT_CFG_DATA]   = 0x9004,
+	[PCIE_HARD_DEBUG] = 0x4204,
+	[PCIE_INTR2_CPU_BASE] = 0x4300,
 };
 
 static const int pcie_offsets_bmips_7425[] = {
 	[RGR1_SW_INIT_1] = 0x8010,
 	[EXT_CFG_INDEX]  = 0x8300,
 	[EXT_CFG_DATA]   = 0x8304,
+	[PCIE_HARD_DEBUG] = 0x4204,
+	[PCIE_INTR2_CPU_BASE] = 0x4300,
 };
 
 static const struct pcie_cfg_data generic_cfg = {
@@ -1539,6 +1546,8 @@ static const int pcie_offset_bcm7278[] = {
 	[RGR1_SW_INIT_1] = 0xc010,
 	[EXT_CFG_INDEX] = 0x9000,
 	[EXT_CFG_DATA] = 0x9004,
+	[PCIE_HARD_DEBUG] = 0x4204,
+	[PCIE_INTR2_CPU_BASE] = 0x4300,
 };
 
 static const struct pcie_cfg_data bcm7278_cfg = {
-- 
2.17.1

[PATCH v6 08/13] PCI: brcmstb: Remove two unused constants from driver

[Jim Quinlan]

Remove two constants in the driver which are no longer
used: RGR1_SW_INIT_1_INIT_MASK and RGR1_SW_INIT_1_INIT_SHIFT.

Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com>
Reviewed-by: Stanimir Varbanov <svarbanov@suse.de>
Reviewed-by: Florian Fainelli <florian.fainelli@broadcom.com>
Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---
 drivers/pci/controller/pcie-brcmstb.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
index 1444f2a9c21e..51b715fbf3a9 100644
--- a/drivers/pci/controller/pcie-brcmstb.c
+++ b/drivers/pci/controller/pcie-brcmstb.c
@@ -210,11 +210,6 @@ enum {
 	PCIE_INTR2_CPU_BASE,
 };
 
-enum {
-	RGR1_SW_INIT_1_INIT_MASK,
-	RGR1_SW_INIT_1_INIT_SHIFT,
-};
-
 enum pcie_type {
 	GENERIC,
 	BCM7425,
-- 
2.17.1

[PATCH v6 09/13] PCI: brcmstb: Don't conflate the reset rescal with phy ctrl

[Jim Quinlan]

Add a "has_phy" field indicating that the internal phy has SW control that
requires configuration.  Some previous chips only required the firing of
the "rescal" reset controller.  This change requires us to give the 7216
SoC its own cfg_data structure.

Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com>
Reviewed-by: Stanimir Varbanov <svarbanov@suse.de>
Reviewed-by: Florian Fainelli <florian.fainelli@broadcom.com>
Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---
 drivers/pci/controller/pcie-brcmstb.c | 17 ++++++++++++++---
 1 file changed, 14 insertions(+), 3 deletions(-)

diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
index 51b715fbf3a9..2431c5a75cde 100644
--- a/drivers/pci/controller/pcie-brcmstb.c
+++ b/drivers/pci/controller/pcie-brcmstb.c
@@ -222,6 +222,7 @@ enum pcie_type {
 struct pcie_cfg_data {
 	const int *offsets;
 	const enum pcie_type type;
+	const bool has_phy;
 	void (*perst_set)(struct brcm_pcie *pcie, u32 val);
 	void (*bridge_sw_init_set)(struct brcm_pcie *pcie, u32 val);
 };
@@ -272,6 +273,7 @@ struct brcm_pcie {
 	void			(*bridge_sw_init_set)(struct brcm_pcie *pcie, u32 val);
 	struct subdev_regulators *sr;
 	bool			ep_wakeup_capable;
+	bool			has_phy;
 };
 
 static inline bool is_bmips(const struct brcm_pcie *pcie)
@@ -1311,12 +1313,12 @@ static int brcm_phy_cntl(struct brcm_pcie *pcie, const int start)
 
 static inline int brcm_phy_start(struct brcm_pcie *pcie)
 {
-	return pcie->rescal ? brcm_phy_cntl(pcie, 1) : 0;
+	return pcie->has_phy ? brcm_phy_cntl(pcie, 1) : 0;
 }
 
 static inline int brcm_phy_stop(struct brcm_pcie *pcie)
 {
-	return pcie->rescal ? brcm_phy_cntl(pcie, 0) : 0;
+	return pcie->has_phy ? brcm_phy_cntl(pcie, 0) : 0;
 }
 
 static void brcm_pcie_turn_off(struct brcm_pcie *pcie)
@@ -1559,12 +1561,20 @@ static const struct pcie_cfg_data bcm2711_cfg = {
 	.bridge_sw_init_set = brcm_pcie_bridge_sw_init_set_generic,
 };
 
+static const struct pcie_cfg_data bcm7216_cfg = {
+	.offsets	= pcie_offset_bcm7278,
+	.type		= BCM7278,
+	.perst_set	= brcm_pcie_perst_set_7278,
+	.bridge_sw_init_set = brcm_pcie_bridge_sw_init_set_7278,
+	.has_phy	= true,
+};
+
 static const struct of_device_id brcm_pcie_match[] = {
 	{ .compatible = "brcm,bcm2711-pcie", .data = &bcm2711_cfg },
 	{ .compatible = "brcm,bcm4908-pcie", .data = &bcm4908_cfg },
 	{ .compatible = "brcm,bcm7211-pcie", .data = &generic_cfg },
 	{ .compatible = "brcm,bcm7278-pcie", .data = &bcm7278_cfg },
-	{ .compatible = "brcm,bcm7216-pcie", .data = &bcm7278_cfg },
+	{ .compatible = "brcm,bcm7216-pcie", .data = &bcm7216_cfg },
 	{ .compatible = "brcm,bcm7445-pcie", .data = &generic_cfg },
 	{ .compatible = "brcm,bcm7435-pcie", .data = &bcm7435_cfg },
 	{ .compatible = "brcm,bcm7425-pcie", .data = &bcm7425_cfg },
@@ -1612,6 +1622,7 @@ static int brcm_pcie_probe(struct platform_device *pdev)
 	pcie->type = data->type;
 	pcie->perst_set = data->perst_set;
 	pcie->bridge_sw_init_set = data->bridge_sw_init_set;
+	pcie->has_phy = data->has_phy;
 
 	pcie->base = devm_platform_ioremap_resource(pdev, 0);
 	if (IS_ERR(pcie->base))
-- 
2.17.1

[PATCH v6 10/13] PCI: brcmstb: Refactor for chips with many regular inbound windows

[Jim Quinlan]

Provide support for new chips with multiple inbound windows while
keeping the legacy support for the older chips.

In existing chips there are three inbound windows with fixed purposes: the
first was for mapping SoC internal registers, the second was for memory,
and the third was for memory but with the endian swapped.  Typically, only
one window was used.

Complicating the inbound window usage was the fact that the PCIe HW would
do a baroque internal mapping of system memory, and concatenate the regions
of multiple memory controllers.

Newer chips such as the 7712 and Cable Modem SOCs take a step forward and
drop the internal mapping while providing for multiple inbound windows.
This works in concert with the dma-ranges property, where each provided
range becomes an inbound window.

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

diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
index 2431c5a75cde..c5d3a5e9e0fc 100644
--- a/drivers/pci/controller/pcie-brcmstb.c
+++ b/drivers/pci/controller/pcie-brcmstb.c
@@ -75,15 +75,19 @@
 #define PCIE_MEM_WIN0_HI(win)	\
 		PCIE_MISC_CPU_2_PCIE_MEM_WIN0_HI + ((win) * 8)
 
+/*
+ * NOTE: You may see the term "BAR" in a number of register names used by
+ *   this driver.  The term is an artifact of when the HW core was an
+ *   endpoint device (EP).  Now it is a root complex (RC) and anywhere a
+ *   register has the term "BAR" it is related to an inbound window.
+ */
+
+#define PCIE_BRCM_MAX_INBOUND_WINS			16
 #define PCIE_MISC_RC_BAR1_CONFIG_LO			0x402c
 #define  PCIE_MISC_RC_BAR1_CONFIG_LO_SIZE_MASK		0x1f
 
-#define PCIE_MISC_RC_BAR2_CONFIG_LO			0x4034
-#define  PCIE_MISC_RC_BAR2_CONFIG_LO_SIZE_MASK		0x1f
-#define PCIE_MISC_RC_BAR2_CONFIG_HI			0x4038
+#define PCIE_MISC_RC_BAR4_CONFIG_LO			0x40d4
 
-#define PCIE_MISC_RC_BAR3_CONFIG_LO			0x403c
-#define  PCIE_MISC_RC_BAR3_CONFIG_LO_SIZE_MASK		0x1f
 
 #define PCIE_MISC_MSI_BAR_CONFIG_LO			0x4044
 #define PCIE_MISC_MSI_BAR_CONFIG_HI			0x4048
@@ -130,6 +134,10 @@
 	  (PCIE_MISC_HARD_PCIE_HARD_DEBUG_CLKREQ_DEBUG_ENABLE_MASK | \
 	   PCIE_MISC_HARD_PCIE_HARD_DEBUG_L1SS_ENABLE_MASK)
 
+#define PCIE_MISC_UBUS_BAR1_CONFIG_REMAP			0x40ac
+#define  PCIE_MISC_UBUS_BAR1_CONFIG_REMAP_ACCESS_EN_MASK	BIT(0)
+#define PCIE_MISC_UBUS_BAR4_CONFIG_REMAP			0x410c
+
 #define PCIE_MSI_INTR2_BASE		0x4500
 
 /* Offsets from INTR2_CPU and MSI_INTR2 BASE offsets */
@@ -217,12 +225,20 @@ enum pcie_type {
 	BCM4908,
 	BCM7278,
 	BCM2711,
+	BCM7712,
+};
+
+struct inbound_win {
+	u64 size;
+	u64 pci_offset;
+	u64 cpu_addr;
 };
 
 struct pcie_cfg_data {
 	const int *offsets;
 	const enum pcie_type type;
 	const bool has_phy;
+	u8 num_inbound_wins;
 	void (*perst_set)(struct brcm_pcie *pcie, u32 val);
 	void (*bridge_sw_init_set)(struct brcm_pcie *pcie, u32 val);
 };
@@ -274,6 +290,7 @@ struct brcm_pcie {
 	struct subdev_regulators *sr;
 	bool			ep_wakeup_capable;
 	bool			has_phy;
+	u8			num_inbound_wins;
 };
 
 static inline bool is_bmips(const struct brcm_pcie *pcie)
@@ -396,7 +413,7 @@ static void brcm_pcie_set_gen(struct brcm_pcie *pcie, int gen)
 }
 
 static void brcm_pcie_set_outbound_win(struct brcm_pcie *pcie,
-				       unsigned int win, u64 cpu_addr,
+				       u8 win, u64 cpu_addr,
 				       u64 pcie_addr, u64 size)
 {
 	u32 cpu_addr_mb_high, limit_addr_mb_high;
@@ -789,23 +806,62 @@ static void brcm_pcie_perst_set_generic(struct brcm_pcie *pcie, u32 val)
 	writel(tmp, pcie->base + PCIE_RGR1_SW_INIT_1(pcie));
 }
 
-static int brcm_pcie_get_rc_bar2_size_and_offset(struct brcm_pcie *pcie,
-							u64 *rc_bar2_size,
-							u64 *rc_bar2_offset)
+static void add_inbound_win(struct inbound_win *b, u8 *count, u64 size,
+			    u64 cpu_addr, u64 pci_offset)
+{
+	b->size = size;
+	b->cpu_addr = cpu_addr;
+	b->pci_offset = pci_offset;
+	(*count)++;
+}
+
+static int brcm_pcie_get_inbound_wins(struct brcm_pcie *pcie,
+				      struct inbound_win inbound_wins[])
 {
 	struct pci_host_bridge *bridge = pci_host_bridge_from_priv(pcie);
+	u64 pci_offset, cpu_addr, size = 0, tot_size = 0;
 	struct resource_entry *entry;
 	struct device *dev = pcie->dev;
 	u64 lowest_pcie_addr = ~(u64)0;
 	int ret, i = 0;
-	u64 size = 0;
+	u8 n = 0;
+
+	/*
+	 * The HW registers (and PCIe) use order-1 numbering for BARs. As
+	 * such, we have inbound_wins[0] unused and BAR1 starts at inbound_wins[1].
+	 */
+	struct inbound_win *b_begin = &inbound_wins[1];
+	struct inbound_win *b = b_begin;
+
+	/*
+	 * STB chips beside 7712 disable the first inbound window default.
+	 * Rather being mapped to system memory it is mapped to the
+	 * internal registers of the SoC.  This feature is deprecated, has
+	 * security considerations, and is not implemented in our modern
+	 * SoCs.
+	 */
+	if (pcie->type != BCM7712)
+		add_inbound_win(b++, &n, 0, 0, 0);
 
 	resource_list_for_each_entry(entry, &bridge->dma_ranges) {
-		u64 pcie_beg = entry->res->start - entry->offset;
+		u64 pcie_start = entry->res->start - entry->offset;
+		u64 cpu_start = entry->res->start;
 
-		size += entry->res->end - entry->res->start + 1;
-		if (pcie_beg < lowest_pcie_addr)
-			lowest_pcie_addr = pcie_beg;
+		size = resource_size(entry->res);
+		tot_size += size;
+		if (pcie_start < lowest_pcie_addr)
+			lowest_pcie_addr = pcie_start;
+		/*
+		 * 7712 and newer chips may have many BARs, with each
+		 * offering a non-overlapping viewport to system memory.
+		 * That being said, each BARs size must still be a power of
+		 * two.
+		 */
+		if (pcie->type == BCM7712)
+			add_inbound_win(b++, &n, size, cpu_start, pcie_start);
+
+		if (n > pcie->num_inbound_wins)
+			break;
 	}
 
 	if (lowest_pcie_addr == ~(u64)0) {
@@ -813,13 +869,20 @@ static int brcm_pcie_get_rc_bar2_size_and_offset(struct brcm_pcie *pcie,
 		return -EINVAL;
 	}
 
+	/*
+	 * 7712 and newer chips do not have an internal memory mapping system
+	 * that enables multiple memory controllers.  As such, it can return
+	 * now w/o doing special configuration.
+	 */
+	if (pcie->type == BCM7712)
+		return n;
+
 	ret = of_property_read_variable_u64_array(pcie->np, "brcm,scb-sizes", pcie->memc_size, 1,
 						  PCIE_BRCM_MAX_MEMC);
-
 	if (ret <= 0) {
 		/* Make an educated guess */
 		pcie->num_memc = 1;
-		pcie->memc_size[0] = 1ULL << fls64(size - 1);
+		pcie->memc_size[0] = 1ULL << fls64(tot_size - 1);
 	} else {
 		pcie->num_memc = ret;
 	}
@@ -828,10 +891,15 @@ static int brcm_pcie_get_rc_bar2_size_and_offset(struct brcm_pcie *pcie,
 	for (i = 0, size = 0; i < pcie->num_memc; i++)
 		size += pcie->memc_size[i];
 
-	/* System memory starts at this address in PCIe-space */
-	*rc_bar2_offset = lowest_pcie_addr;
-	/* The sum of all memc views must also be a power of 2 */
-	*rc_bar2_size = 1ULL << fls64(size - 1);
+	/* Our HW mandates that the window size must be a power of 2 */
+	size = 1ULL << fls64(size - 1);
+
+	/*
+	 * For STB chips, the BAR2 cpu_addr is hardwired to the start
+	 * of system memory, so we set it to 0.
+	 */
+	cpu_addr = 0;
+	pci_offset = lowest_pcie_addr;
 
 	/*
 	 * We validate the inbound memory view even though we should trust
@@ -866,25 +934,90 @@ static int brcm_pcie_get_rc_bar2_size_and_offset(struct brcm_pcie *pcie,
 	 *   outbound memory @ 3GB). So instead it will  start at the 1x
 	 *   multiple of its size
 	 */
-	if (!*rc_bar2_size || (*rc_bar2_offset & (*rc_bar2_size - 1)) ||
-	    (*rc_bar2_offset < SZ_4G && *rc_bar2_offset > SZ_2G)) {
-		dev_err(dev, "Invalid rc_bar2_offset/size: size 0x%llx, off 0x%llx\n",
-			*rc_bar2_size, *rc_bar2_offset);
+	if (!size || (pci_offset & (size - 1)) ||
+	    (pci_offset < SZ_4G && pci_offset > SZ_2G)) {
+		dev_err(dev, "Invalid inbound_win2_offset/size: size 0x%llx, off 0x%llx\n",
+			size, pci_offset);
 		return -EINVAL;
 	}
 
-	return 0;
+	/* Enable inbound window 2, the main inbound window for STB chips */
+	add_inbound_win(b++, &n, size, cpu_addr, pci_offset);
+
+	/*
+	 * Disable inbound window 3.  On some chips presents the same
+	 * window as #2 but the data appears in a settable endianness.
+	 */
+	add_inbound_win(b++, &n, 0, 0, 0);
+
+	return n;
+}
+
+static u32 brcm_bar_reg_offset(int bar)
+{
+	if (bar <= 3)
+		return PCIE_MISC_RC_BAR1_CONFIG_LO + 8 * (bar - 1);
+	else
+		return PCIE_MISC_RC_BAR4_CONFIG_LO + 8 * (bar - 4);
+}
+
+static u32 brcm_ubus_reg_offset(int bar)
+{
+	if (bar <= 3)
+		return PCIE_MISC_UBUS_BAR1_CONFIG_REMAP + 8 * (bar - 1);
+	else
+		return PCIE_MISC_UBUS_BAR4_CONFIG_REMAP + 8 * (bar - 4);
+}
+
+static void set_inbound_win_registers(struct brcm_pcie *pcie,
+				      const struct inbound_win *inbound_wins,
+				      u8 num_inbound_wins)
+{
+	void __iomem *base = pcie->base;
+	int i;
+
+	for (i = 1; i <= num_inbound_wins; i++) {
+		u64 pci_offset = inbound_wins[i].pci_offset;
+		u64 cpu_addr = inbound_wins[i].cpu_addr;
+		u64 size = inbound_wins[i].size;
+		u32 reg_offset = brcm_bar_reg_offset(i);
+		u32 tmp = lower_32_bits(pci_offset);
+
+		u32p_replace_bits(&tmp, brcm_pcie_encode_ibar_size(size),
+				  PCIE_MISC_RC_BAR1_CONFIG_LO_SIZE_MASK);
+
+		/* Write low */
+		writel_relaxed(tmp, base + reg_offset);
+		/* Write high */
+		writel_relaxed(upper_32_bits(pci_offset), base + reg_offset + 4);
+
+		/*
+		 * Most STB chips:
+		 *     Do nothing.
+		 * 7712:
+		 *     All of their BARs need to be set.
+		 */
+		if (pcie->type == BCM7712) {
+			/* BUS remap register settings */
+			reg_offset = brcm_ubus_reg_offset(i);
+			tmp = lower_32_bits(cpu_addr) & ~0xfff;
+			tmp |= PCIE_MISC_UBUS_BAR1_CONFIG_REMAP_ACCESS_EN_MASK;
+			writel_relaxed(tmp, base + reg_offset);
+			tmp = upper_32_bits(cpu_addr);
+			writel_relaxed(tmp, base + reg_offset + 4);
+		}
+	}
 }
 
 static int brcm_pcie_setup(struct brcm_pcie *pcie)
 {
-	u64 rc_bar2_offset, rc_bar2_size;
+	struct inbound_win inbound_wins[PCIE_BRCM_MAX_INBOUND_WINS];
 	void __iomem *base = pcie->base;
 	struct pci_host_bridge *bridge;
 	struct resource_entry *entry;
 	u32 tmp, burst, aspm_support;
-	int num_out_wins = 0;
-	int ret, memc;
+	u8 num_out_wins = 0, num_inbound_wins = 0;
+	int memc;
 
 	/* Reset the bridge */
 	pcie->bridge_sw_init_set(pcie, 1);
@@ -933,17 +1066,16 @@ static int brcm_pcie_setup(struct brcm_pcie *pcie)
 	u32p_replace_bits(&tmp, 1, PCIE_MISC_MISC_CTRL_PCIE_RCB_64B_MODE_MASK);
 	writel(tmp, base + PCIE_MISC_MISC_CTRL);
 
-	ret = brcm_pcie_get_rc_bar2_size_and_offset(pcie, &rc_bar2_size,
-						    &rc_bar2_offset);
-	if (ret)
-		return ret;
+	num_inbound_wins = brcm_pcie_get_inbound_wins(pcie, inbound_wins);
+	if (num_inbound_wins < 0)
+		return num_inbound_wins;
+
+	set_inbound_win_registers(pcie, inbound_wins, num_inbound_wins);
 
-	tmp = lower_32_bits(rc_bar2_offset);
-	u32p_replace_bits(&tmp, brcm_pcie_encode_ibar_size(rc_bar2_size),
-			  PCIE_MISC_RC_BAR2_CONFIG_LO_SIZE_MASK);
-	writel(tmp, base + PCIE_MISC_RC_BAR2_CONFIG_LO);
-	writel(upper_32_bits(rc_bar2_offset),
-	       base + PCIE_MISC_RC_BAR2_CONFIG_HI);
+	if (!brcm_pcie_rc_mode(pcie)) {
+		dev_err(pcie->dev, "PCIe RC controller misconfigured as Endpoint\n");
+		return -EINVAL;
+	}
 
 	tmp = readl(base + PCIE_MISC_MISC_CTRL);
 	for (memc = 0; memc < pcie->num_memc; memc++) {
@@ -965,25 +1097,12 @@ static int brcm_pcie_setup(struct brcm_pcie *pcie)
 	 * 4GB or when the inbound area is smaller than 4GB (taking into
 	 * account the rounding-up we're forced to perform).
 	 */
-	if (rc_bar2_offset >= SZ_4G || (rc_bar2_size + rc_bar2_offset) < SZ_4G)
+	if (inbound_wins[2].pci_offset >= SZ_4G ||
+	    (inbound_wins[2].size + inbound_wins[2].pci_offset) < SZ_4G)
 		pcie->msi_target_addr = BRCM_MSI_TARGET_ADDR_LT_4GB;
 	else
 		pcie->msi_target_addr = BRCM_MSI_TARGET_ADDR_GT_4GB;
 
-	if (!brcm_pcie_rc_mode(pcie)) {
-		dev_err(pcie->dev, "PCIe RC controller misconfigured as Endpoint\n");
-		return -EINVAL;
-	}
-
-	/* disable the PCIe->GISB memory window (RC_BAR1) */
-	tmp = readl(base + PCIE_MISC_RC_BAR1_CONFIG_LO);
-	tmp &= ~PCIE_MISC_RC_BAR1_CONFIG_LO_SIZE_MASK;
-	writel(tmp, base + PCIE_MISC_RC_BAR1_CONFIG_LO);
-
-	/* disable the PCIe->SCB memory window (RC_BAR3) */
-	tmp = readl(base + PCIE_MISC_RC_BAR3_CONFIG_LO);
-	tmp &= ~PCIE_MISC_RC_BAR3_CONFIG_LO_SIZE_MASK;
-	writel(tmp, base + PCIE_MISC_RC_BAR3_CONFIG_LO);
 
 	/* Don't advertise L0s capability if 'aspm-no-l0s' */
 	aspm_support = PCIE_LINK_STATE_L1;
@@ -1034,7 +1153,7 @@ static int brcm_pcie_setup(struct brcm_pcie *pcie)
 		num_out_wins++;
 	}
 
-	/* PCIe->SCB endian mode for BAR */
+	/* PCIe->SCB endian mode for inbound window */
 	tmp = readl(base + PCIE_RC_CFG_VENDOR_VENDOR_SPECIFIC_REG1);
 	u32p_replace_bits(&tmp, PCIE_RC_CFG_VENDOR_SPCIFIC_REG1_LITTLE_ENDIAN,
 		PCIE_RC_CFG_VENDOR_VENDOR_SPECIFIC_REG1_ENDIAN_MODE_BAR2_MASK);
@@ -1516,6 +1635,7 @@ static const struct pcie_cfg_data generic_cfg = {
 	.type		= GENERIC,
 	.perst_set	= brcm_pcie_perst_set_generic,
 	.bridge_sw_init_set = brcm_pcie_bridge_sw_init_set_generic,
+	.num_inbound_wins = 3,
 };
 
 static const struct pcie_cfg_data bcm7425_cfg = {
@@ -1523,6 +1643,7 @@ static const struct pcie_cfg_data bcm7425_cfg = {
 	.type		= BCM7425,
 	.perst_set	= brcm_pcie_perst_set_generic,
 	.bridge_sw_init_set = brcm_pcie_bridge_sw_init_set_generic,
+	.num_inbound_wins = 3,
 };
 
 static const struct pcie_cfg_data bcm7435_cfg = {
@@ -1530,6 +1651,7 @@ static const struct pcie_cfg_data bcm7435_cfg = {
 	.type		= BCM7435,
 	.perst_set	= brcm_pcie_perst_set_generic,
 	.bridge_sw_init_set = brcm_pcie_bridge_sw_init_set_generic,
+	.num_inbound_wins = 3,
 };
 
 static const struct pcie_cfg_data bcm4908_cfg = {
@@ -1537,6 +1659,7 @@ static const struct pcie_cfg_data bcm4908_cfg = {
 	.type		= BCM4908,
 	.perst_set	= brcm_pcie_perst_set_4908,
 	.bridge_sw_init_set = brcm_pcie_bridge_sw_init_set_generic,
+	.num_inbound_wins = 3,
 };
 
 static const int pcie_offset_bcm7278[] = {
@@ -1552,6 +1675,7 @@ static const struct pcie_cfg_data bcm7278_cfg = {
 	.type		= BCM7278,
 	.perst_set	= brcm_pcie_perst_set_7278,
 	.bridge_sw_init_set = brcm_pcie_bridge_sw_init_set_7278,
+	.num_inbound_wins = 3,
 };
 
 static const struct pcie_cfg_data bcm2711_cfg = {
@@ -1559,6 +1683,7 @@ static const struct pcie_cfg_data bcm2711_cfg = {
 	.type		= BCM2711,
 	.perst_set	= brcm_pcie_perst_set_generic,
 	.bridge_sw_init_set = brcm_pcie_bridge_sw_init_set_generic,
+	.num_inbound_wins = 3,
 };
 
 static const struct pcie_cfg_data bcm7216_cfg = {
@@ -1567,6 +1692,7 @@ static const struct pcie_cfg_data bcm7216_cfg = {
 	.perst_set	= brcm_pcie_perst_set_7278,
 	.bridge_sw_init_set = brcm_pcie_bridge_sw_init_set_7278,
 	.has_phy	= true,
+	.num_inbound_wins = 3,
 };
 
 static const struct of_device_id brcm_pcie_match[] = {
@@ -1623,6 +1749,7 @@ static int brcm_pcie_probe(struct platform_device *pdev)
 	pcie->perst_set = data->perst_set;
 	pcie->bridge_sw_init_set = data->bridge_sw_init_set;
 	pcie->has_phy = data->has_phy;
+	pcie->num_inbound_wins = data->num_inbound_wins;
 
 	pcie->base = devm_platform_ioremap_resource(pdev, 0);
 	if (IS_ERR(pcie->base))
-- 
2.17.1

[PATCH v6 11/13] PCI: brcmstb: Check return value of all reset_control_xxx calls

[Jim Quinlan]

Always check the return value for invocations of reset_control_xxx() and
propagate the error to the next level.  Although the current functions
in reset-brcmstb.c cannot fail, this may someday change.

Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com>
Reviewed-by: Stanimir Varbanov <svarbanov@suse.de>
Reviewed-by: Florian Fainelli <florian.fainelli@broadcom.com>
---
 drivers/pci/controller/pcie-brcmstb.c | 102 ++++++++++++++++++--------
 1 file changed, 73 insertions(+), 29 deletions(-)

diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
index c5d3a5e9e0fc..d19eeeed623b 100644
--- a/drivers/pci/controller/pcie-brcmstb.c
+++ b/drivers/pci/controller/pcie-brcmstb.c
@@ -239,8 +239,8 @@ struct pcie_cfg_data {
 	const enum pcie_type type;
 	const bool has_phy;
 	u8 num_inbound_wins;
-	void (*perst_set)(struct brcm_pcie *pcie, u32 val);
-	void (*bridge_sw_init_set)(struct brcm_pcie *pcie, u32 val);
+	int (*perst_set)(struct brcm_pcie *pcie, u32 val);
+	int (*bridge_sw_init_set)(struct brcm_pcie *pcie, u32 val);
 };
 
 struct subdev_regulators {
@@ -285,8 +285,8 @@ struct brcm_pcie {
 	int			num_memc;
 	u64			memc_size[PCIE_BRCM_MAX_MEMC];
 	u32			hw_rev;
-	void			(*perst_set)(struct brcm_pcie *pcie, u32 val);
-	void			(*bridge_sw_init_set)(struct brcm_pcie *pcie, u32 val);
+	int			(*perst_set)(struct brcm_pcie *pcie, u32 val);
+	int			(*bridge_sw_init_set)(struct brcm_pcie *pcie, u32 val);
 	struct subdev_regulators *sr;
 	bool			ep_wakeup_capable;
 	bool			has_phy;
@@ -749,12 +749,18 @@ static void __iomem *brcm7425_pcie_map_bus(struct pci_bus *bus,
 	return base + DATA_ADDR(pcie);
 }
 
-static void brcm_pcie_bridge_sw_init_set_generic(struct brcm_pcie *pcie, u32 val)
+static int brcm_pcie_bridge_sw_init_set_generic(struct brcm_pcie *pcie, u32 val)
 {
+	int ret = 0;
+
 	if (val)
-		reset_control_assert(pcie->bridge_reset);
+		ret = reset_control_assert(pcie->bridge_reset);
 	else
-		reset_control_deassert(pcie->bridge_reset);
+		ret = reset_control_deassert(pcie->bridge_reset);
+
+	if (ret)
+		dev_err(pcie->dev, "failed to %s 'bridge' reset, err=%d\n",
+			val ? "assert" : "deassert", ret);
 
 	if (!pcie->bridge_reset) {
 		u32 tmp, mask =  RGR1_SW_INIT_1_INIT_GENERIC_MASK;
@@ -764,9 +770,11 @@ static void brcm_pcie_bridge_sw_init_set_generic(struct brcm_pcie *pcie, u32 val
 		tmp = (tmp & ~mask) | ((val << shift) & mask);
 		writel(tmp, pcie->base + PCIE_RGR1_SW_INIT_1(pcie));
 	}
+
+	return ret;
 }
 
-static void brcm_pcie_bridge_sw_init_set_7278(struct brcm_pcie *pcie, u32 val)
+static int brcm_pcie_bridge_sw_init_set_7278(struct brcm_pcie *pcie, u32 val)
 {
 	u32 tmp, mask =  RGR1_SW_INIT_1_INIT_7278_MASK;
 	u32 shift = RGR1_SW_INIT_1_INIT_7278_SHIFT;
@@ -774,20 +782,29 @@ static void brcm_pcie_bridge_sw_init_set_7278(struct brcm_pcie *pcie, u32 val)
 	tmp = readl(pcie->base + PCIE_RGR1_SW_INIT_1(pcie));
 	tmp = (tmp & ~mask) | ((val << shift) & mask);
 	writel(tmp, pcie->base + PCIE_RGR1_SW_INIT_1(pcie));
+
+	return 0;
 }
 
-static void brcm_pcie_perst_set_4908(struct brcm_pcie *pcie, u32 val)
+static int brcm_pcie_perst_set_4908(struct brcm_pcie *pcie, u32 val)
 {
+	int ret;
+
 	if (WARN_ONCE(!pcie->perst_reset, "missing PERST# reset controller\n"))
-		return;
+		return -EINVAL;
 
 	if (val)
-		reset_control_assert(pcie->perst_reset);
+		ret = reset_control_assert(pcie->perst_reset);
 	else
-		reset_control_deassert(pcie->perst_reset);
+		ret = reset_control_deassert(pcie->perst_reset);
+
+	if (ret)
+		dev_err(pcie->dev, "failed to %s 'perst' reset, err=%d\n",
+			val ? "assert" : "deassert", ret);
+	return ret;
 }
 
-static void brcm_pcie_perst_set_7278(struct brcm_pcie *pcie, u32 val)
+static int brcm_pcie_perst_set_7278(struct brcm_pcie *pcie, u32 val)
 {
 	u32 tmp;
 
@@ -795,15 +812,19 @@ static void brcm_pcie_perst_set_7278(struct brcm_pcie *pcie, u32 val)
 	tmp = readl(pcie->base + PCIE_MISC_PCIE_CTRL);
 	u32p_replace_bits(&tmp, !val, PCIE_MISC_PCIE_CTRL_PCIE_PERSTB_MASK);
 	writel(tmp, pcie->base +  PCIE_MISC_PCIE_CTRL);
+
+	return 0;
 }
 
-static void brcm_pcie_perst_set_generic(struct brcm_pcie *pcie, u32 val)
+static int brcm_pcie_perst_set_generic(struct brcm_pcie *pcie, u32 val)
 {
 	u32 tmp;
 
 	tmp = readl(pcie->base + PCIE_RGR1_SW_INIT_1(pcie));
 	u32p_replace_bits(&tmp, val, PCIE_RGR1_SW_INIT_1_PERST_MASK);
 	writel(tmp, pcie->base + PCIE_RGR1_SW_INIT_1(pcie));
+
+	return 0;
 }
 
 static void add_inbound_win(struct inbound_win *b, u8 *count, u64 size,
@@ -1017,19 +1038,28 @@ static int brcm_pcie_setup(struct brcm_pcie *pcie)
 	struct resource_entry *entry;
 	u32 tmp, burst, aspm_support;
 	u8 num_out_wins = 0, num_inbound_wins = 0;
-	int memc;
+	int memc, ret;
 
 	/* Reset the bridge */
-	pcie->bridge_sw_init_set(pcie, 1);
+	ret = pcie->bridge_sw_init_set(pcie, 1);
+	if (ret)
+		return ret;
 
 	/* Ensure that PERST# is asserted; some bootloaders may deassert it. */
-	if (pcie->type == BCM2711)
-		pcie->perst_set(pcie, 1);
+	if (pcie->type == BCM2711) {
+		ret = pcie->perst_set(pcie, 1);
+		if (ret) {
+			pcie->bridge_sw_init_set(pcie, 0);
+			return ret;
+		}
+	}
 
 	usleep_range(100, 200);
 
 	/* Take the bridge out of reset */
-	pcie->bridge_sw_init_set(pcie, 0);
+	ret = pcie->bridge_sw_init_set(pcie, 0);
+	if (ret)
+		return ret;
 
 	tmp = readl(base + HARD_DEBUG(pcie));
 	if (is_bmips(pcie))
@@ -1248,7 +1278,9 @@ static int brcm_pcie_start_link(struct brcm_pcie *pcie)
 	int ret, i;
 
 	/* Unassert the fundamental reset */
-	pcie->perst_set(pcie, 0);
+	ret = pcie->perst_set(pcie, 0);
+	if (ret)
+		return ret;
 
 	/*
 	 * Wait for 100ms after PERST# deassertion; see PCIe CEM specification
@@ -1440,15 +1472,17 @@ static inline int brcm_phy_stop(struct brcm_pcie *pcie)
 	return pcie->has_phy ? brcm_phy_cntl(pcie, 0) : 0;
 }
 
-static void brcm_pcie_turn_off(struct brcm_pcie *pcie)
+static int brcm_pcie_turn_off(struct brcm_pcie *pcie)
 {
 	void __iomem *base = pcie->base;
-	int tmp;
+	int tmp, ret;
 
 	if (brcm_pcie_link_up(pcie))
 		brcm_pcie_enter_l23(pcie);
 	/* Assert fundamental reset */
-	pcie->perst_set(pcie, 1);
+	ret = pcie->perst_set(pcie, 1);
+	if (ret)
+		return ret;
 
 	/* Deassert request for L23 in case it was asserted */
 	tmp = readl(base + PCIE_MISC_PCIE_CTRL);
@@ -1461,7 +1495,9 @@ static void brcm_pcie_turn_off(struct brcm_pcie *pcie)
 	writel(tmp, base + HARD_DEBUG(pcie));
 
 	/* Shutdown PCIe bridge */
-	pcie->bridge_sw_init_set(pcie, 1);
+	ret = pcie->bridge_sw_init_set(pcie, 1);
+
+	return ret;
 }
 
 static int pci_dev_may_wakeup(struct pci_dev *dev, void *data)
@@ -1479,9 +1515,12 @@ static int brcm_pcie_suspend_noirq(struct device *dev)
 {
 	struct brcm_pcie *pcie = dev_get_drvdata(dev);
 	struct pci_host_bridge *bridge = pci_host_bridge_from_priv(pcie);
-	int ret;
+	int ret, rret;
+
+	ret = brcm_pcie_turn_off(pcie);
+	if (ret)
+		return ret;
 
-	brcm_pcie_turn_off(pcie);
 	/*
 	 * If brcm_phy_stop() returns an error, just dev_err(). If we
 	 * return the error it will cause the suspend to fail and this is a
@@ -1510,7 +1549,10 @@ static int brcm_pcie_suspend_noirq(struct device *dev)
 						     pcie->sr->supplies);
 			if (ret) {
 				dev_err(dev, "Could not turn off regulators\n");
-				reset_control_reset(pcie->rescal);
+				rret = reset_control_reset(pcie->rescal);
+				if (rret)
+					dev_err(dev, "failed to reset 'rascal' controller ret=%d\n",
+						rret);
 				return ret;
 			}
 		}
@@ -1525,7 +1567,7 @@ static int brcm_pcie_resume_noirq(struct device *dev)
 	struct brcm_pcie *pcie = dev_get_drvdata(dev);
 	void __iomem *base;
 	u32 tmp;
-	int ret;
+	int ret, rret;
 
 	base = pcie->base;
 	ret = clk_prepare_enable(pcie->clk);
@@ -1587,7 +1629,9 @@ static int brcm_pcie_resume_noirq(struct device *dev)
 	if (pcie->sr)
 		regulator_bulk_disable(pcie->sr->num_supplies, pcie->sr->supplies);
 err_reset:
-	reset_control_rearm(pcie->rescal);
+	rret = reset_control_rearm(pcie->rescal);
+	if (rret)
+		dev_err(pcie->dev, "failed to rearm 'rescal' reset, err=%d\n", rret);
 err_disable_clk:
 	clk_disable_unprepare(pcie->clk);
 	return ret;
-- 
2.17.1

[PATCH v6 12/13] PCI: brcmstb: Change field name from 'type' to 'soc_base'

[Jim Quinlan]

The 'type' field used in the driver to discern SoC differences is
confusing; change it to the more apt 'soc_base'.  The 'base' is because
some SoCs have the same characteristics as previous SoCs so it is
convenient to classify them in the same group.

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

diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
index d19eeeed623b..26e8f544da4c 100644
--- a/drivers/pci/controller/pcie-brcmstb.c
+++ b/drivers/pci/controller/pcie-brcmstb.c
@@ -218,7 +218,7 @@ enum {
 	PCIE_INTR2_CPU_BASE,
 };
 
-enum pcie_type {
+enum pcie_soc_base {
 	GENERIC,
 	BCM7425,
 	BCM7435,
@@ -236,7 +236,7 @@ struct inbound_win {
 
 struct pcie_cfg_data {
 	const int *offsets;
-	const enum pcie_type type;
+	const enum pcie_soc_base soc_base;
 	const bool has_phy;
 	u8 num_inbound_wins;
 	int (*perst_set)(struct brcm_pcie *pcie, u32 val);
@@ -277,7 +277,7 @@ struct brcm_pcie {
 	u64			msi_target_addr;
 	struct brcm_msi		*msi;
 	const int		*reg_offsets;
-	enum pcie_type		type;
+	enum pcie_soc_base	soc_base;
 	struct reset_control	*rescal;
 	struct reset_control	*perst_reset;
 	struct reset_control	*bridge_reset;
@@ -295,7 +295,7 @@ struct brcm_pcie {
 
 static inline bool is_bmips(const struct brcm_pcie *pcie)
 {
-	return pcie->type == BCM7435 || pcie->type == BCM7425;
+	return pcie->soc_base == BCM7435 || pcie->soc_base == BCM7425;
 }
 
 /*
@@ -861,7 +861,7 @@ static int brcm_pcie_get_inbound_wins(struct brcm_pcie *pcie,
 	 * security considerations, and is not implemented in our modern
 	 * SoCs.
 	 */
-	if (pcie->type != BCM7712)
+	if (pcie->soc_base != BCM7712)
 		add_inbound_win(b++, &n, 0, 0, 0);
 
 	resource_list_for_each_entry(entry, &bridge->dma_ranges) {
@@ -878,7 +878,7 @@ static int brcm_pcie_get_inbound_wins(struct brcm_pcie *pcie,
 		 * That being said, each BARs size must still be a power of
 		 * two.
 		 */
-		if (pcie->type == BCM7712)
+		if (pcie->soc_base == BCM7712)
 			add_inbound_win(b++, &n, size, cpu_start, pcie_start);
 
 		if (n > pcie->num_inbound_wins)
@@ -895,7 +895,7 @@ static int brcm_pcie_get_inbound_wins(struct brcm_pcie *pcie,
 	 * that enables multiple memory controllers.  As such, it can return
 	 * now w/o doing special configuration.
 	 */
-	if (pcie->type == BCM7712)
+	if (pcie->soc_base == BCM7712)
 		return n;
 
 	ret = of_property_read_variable_u64_array(pcie->np, "brcm,scb-sizes", pcie->memc_size, 1,
@@ -1018,7 +1018,7 @@ static void set_inbound_win_registers(struct brcm_pcie *pcie,
 		 * 7712:
 		 *     All of their BARs need to be set.
 		 */
-		if (pcie->type == BCM7712) {
+		if (pcie->soc_base == BCM7712) {
 			/* BUS remap register settings */
 			reg_offset = brcm_ubus_reg_offset(i);
 			tmp = lower_32_bits(cpu_addr) & ~0xfff;
@@ -1046,7 +1046,7 @@ static int brcm_pcie_setup(struct brcm_pcie *pcie)
 		return ret;
 
 	/* Ensure that PERST# is asserted; some bootloaders may deassert it. */
-	if (pcie->type == BCM2711) {
+	if (pcie->soc_base == BCM2711) {
 		ret = pcie->perst_set(pcie, 1);
 		if (ret) {
 			pcie->bridge_sw_init_set(pcie, 0);
@@ -1077,9 +1077,9 @@ static int brcm_pcie_setup(struct brcm_pcie *pcie)
 	 */
 	if (is_bmips(pcie))
 		burst = 0x1; /* 256 bytes */
-	else if (pcie->type == BCM2711)
+	else if (pcie->soc_base == BCM2711)
 		burst = 0x0; /* 128 bytes */
-	else if (pcie->type == BCM7278)
+	else if (pcie->soc_base == BCM7278)
 		burst = 0x3; /* 512 bytes */
 	else
 		burst = 0x2; /* 512 bytes */
@@ -1676,7 +1676,7 @@ static const int pcie_offsets_bmips_7425[] = {
 
 static const struct pcie_cfg_data generic_cfg = {
 	.offsets	= pcie_offsets,
-	.type		= GENERIC,
+	.soc_base	= GENERIC,
 	.perst_set	= brcm_pcie_perst_set_generic,
 	.bridge_sw_init_set = brcm_pcie_bridge_sw_init_set_generic,
 	.num_inbound_wins = 3,
@@ -1684,7 +1684,7 @@ static const struct pcie_cfg_data generic_cfg = {
 
 static const struct pcie_cfg_data bcm7425_cfg = {
 	.offsets	= pcie_offsets_bmips_7425,
-	.type		= BCM7425,
+	.soc_base	= BCM7425,
 	.perst_set	= brcm_pcie_perst_set_generic,
 	.bridge_sw_init_set = brcm_pcie_bridge_sw_init_set_generic,
 	.num_inbound_wins = 3,
@@ -1692,7 +1692,7 @@ static const struct pcie_cfg_data bcm7425_cfg = {
 
 static const struct pcie_cfg_data bcm7435_cfg = {
 	.offsets	= pcie_offsets,
-	.type		= BCM7435,
+	.soc_base	= BCM7435,
 	.perst_set	= brcm_pcie_perst_set_generic,
 	.bridge_sw_init_set = brcm_pcie_bridge_sw_init_set_generic,
 	.num_inbound_wins = 3,
@@ -1700,7 +1700,7 @@ static const struct pcie_cfg_data bcm7435_cfg = {
 
 static const struct pcie_cfg_data bcm4908_cfg = {
 	.offsets	= pcie_offsets,
-	.type		= BCM4908,
+	.soc_base	= BCM4908,
 	.perst_set	= brcm_pcie_perst_set_4908,
 	.bridge_sw_init_set = brcm_pcie_bridge_sw_init_set_generic,
 	.num_inbound_wins = 3,
@@ -1716,7 +1716,7 @@ static const int pcie_offset_bcm7278[] = {
 
 static const struct pcie_cfg_data bcm7278_cfg = {
 	.offsets	= pcie_offset_bcm7278,
-	.type		= BCM7278,
+	.soc_base	= BCM7278,
 	.perst_set	= brcm_pcie_perst_set_7278,
 	.bridge_sw_init_set = brcm_pcie_bridge_sw_init_set_7278,
 	.num_inbound_wins = 3,
@@ -1724,7 +1724,7 @@ static const struct pcie_cfg_data bcm7278_cfg = {
 
 static const struct pcie_cfg_data bcm2711_cfg = {
 	.offsets	= pcie_offsets,
-	.type		= BCM2711,
+	.soc_base	= BCM2711,
 	.perst_set	= brcm_pcie_perst_set_generic,
 	.bridge_sw_init_set = brcm_pcie_bridge_sw_init_set_generic,
 	.num_inbound_wins = 3,
@@ -1732,7 +1732,7 @@ static const struct pcie_cfg_data bcm2711_cfg = {
 
 static const struct pcie_cfg_data bcm7216_cfg = {
 	.offsets	= pcie_offset_bcm7278,
-	.type		= BCM7278,
+	.soc_base	= BCM7278,
 	.perst_set	= brcm_pcie_perst_set_7278,
 	.bridge_sw_init_set = brcm_pcie_bridge_sw_init_set_7278,
 	.has_phy	= true,
@@ -1789,7 +1789,7 @@ static int brcm_pcie_probe(struct platform_device *pdev)
 	pcie->dev = &pdev->dev;
 	pcie->np = np;
 	pcie->reg_offsets = data->offsets;
-	pcie->type = data->type;
+	pcie->soc_base = data->soc_base;
 	pcie->perst_set = data->perst_set;
 	pcie->bridge_sw_init_set = data->bridge_sw_init_set;
 	pcie->has_phy = data->has_phy;
@@ -1867,7 +1867,7 @@ static int brcm_pcie_probe(struct platform_device *pdev)
 		goto fail;
 
 	pcie->hw_rev = readl(pcie->base + PCIE_MISC_REVISION);
-	if (pcie->type == BCM4908 && pcie->hw_rev >= BRCM_PCIE_HW_REV_3_20) {
+	if (pcie->soc_base == BCM4908 && pcie->hw_rev >= BRCM_PCIE_HW_REV_3_20) {
 		dev_err(pcie->dev, "hardware revision with unsupported PERST# setup\n");
 		ret = -ENODEV;
 		goto fail;
@@ -1882,7 +1882,7 @@ static int brcm_pcie_probe(struct platform_device *pdev)
 		}
 	}
 
-	bridge->ops = pcie->type == BCM7425 ? &brcm7425_pcie_ops : &brcm_pcie_ops;
+	bridge->ops = pcie->soc_base == BCM7425 ? &brcm7425_pcie_ops : &brcm_pcie_ops;
 	bridge->sysdata = pcie;
 
 	platform_set_drvdata(pdev, pcie);
-- 
2.17.1

[PATCH v6 13/13] PCI: brcmstb: Enable 7712 SOCs

[Jim Quinlan]

The Broadcom STB 7712 is the sibling chip of the RPi 5 (2712).
It has one PCIe controller with a single port, supports gen2
and one lane only.  The current revision of the chip is "C0"
or "C1".

Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com>
Reviewed-by: Stanimir Varbanov <svarbanov@suse.de>
Reviewed-by: Florian Fainelli <florian.fainelli@broadcom.com>
Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---
 drivers/pci/controller/pcie-brcmstb.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
index 26e8f544da4c..21e692a57882 100644
--- a/drivers/pci/controller/pcie-brcmstb.c
+++ b/drivers/pci/controller/pcie-brcmstb.c
@@ -1203,6 +1203,10 @@ static void brcm_extend_rbus_timeout(struct brcm_pcie *pcie)
 	const unsigned int REG_OFFSET = PCIE_RGR1_SW_INIT_1(pcie) - 8;
 	u32 timeout_us = 4000000; /* 4 seconds, our setting for L1SS */
 
+	/* 7712 does not have this (RGR1) timer */
+	if (pcie->soc_base == BCM7712)
+		return;
+
 	/* Each unit in timeout register is 1/216,000,000 seconds */
 	writel(216 * timeout_us, pcie->base + REG_OFFSET);
 }
@@ -1674,6 +1678,13 @@ static const int pcie_offsets_bmips_7425[] = {
 	[PCIE_INTR2_CPU_BASE] = 0x4300,
 };
 
+static const int pcie_offset_bcm7712[] = {
+	[EXT_CFG_INDEX]  = 0x9000,
+	[EXT_CFG_DATA]   = 0x9004,
+	[PCIE_HARD_DEBUG] = 0x4304,
+	[PCIE_INTR2_CPU_BASE] = 0x4400,
+};
+
 static const struct pcie_cfg_data generic_cfg = {
 	.offsets	= pcie_offsets,
 	.soc_base	= GENERIC,
@@ -1739,6 +1750,14 @@ static const struct pcie_cfg_data bcm7216_cfg = {
 	.num_inbound_wins = 3,
 };
 
+static const struct pcie_cfg_data bcm7712_cfg = {
+	.offsets	= pcie_offset_bcm7712,
+	.perst_set	= brcm_pcie_perst_set_7278,
+	.bridge_sw_init_set = brcm_pcie_bridge_sw_init_set_generic,
+	.soc_base	= BCM7712,
+	.num_inbound_wins = 10,
+};
+
 static const struct of_device_id brcm_pcie_match[] = {
 	{ .compatible = "brcm,bcm2711-pcie", .data = &bcm2711_cfg },
 	{ .compatible = "brcm,bcm4908-pcie", .data = &bcm4908_cfg },
@@ -1748,6 +1767,7 @@ static const struct of_device_id brcm_pcie_match[] = {
 	{ .compatible = "brcm,bcm7445-pcie", .data = &generic_cfg },
 	{ .compatible = "brcm,bcm7435-pcie", .data = &bcm7435_cfg },
 	{ .compatible = "brcm,bcm7425-pcie", .data = &bcm7425_cfg },
+	{ .compatible = "brcm,bcm7712-pcie", .data = &bcm7712_cfg },
 	{},
 };
 
-- 
2.17.1

Re: [PATCH v6 01/13] dt-bindings: PCI: Change brcmstb maintainer and cleanup

[Krzysztof Kozlowski]

On 16/08/2024 00:57, Jim Quinlan wrote:
> Change maintainer: Nicolas has not been active for a while.
> It also makes sense for a Broadcom employee to be the
> maintainer as many of the details are privy to Broadcom.
> 
> Also, alphabetize the compatible strings.
> 
> Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com>

Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Best regards,
Krzysztof

Re: [PATCH v6 02/13] dt-bindings: PCI: Use maxItems for reset controllers

[Krzysztof Kozlowski]

On 16/08/2024 00:57, Jim Quinlan wrote:
> Provide the maxItem property for the reset controllers and drop their
> superfluous descriptions.
> 
> Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com>

Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Best regards,
Krzysztof

Re: [PATCH v6 04/13] PCI: brcmstb: Use common error handling code in brcm_pcie_probe()

[Manivannan Sadhasivam]

On Thu, Aug 15, 2024 at 06:57:17PM -0400, Jim Quinlan wrote:
> Refactor the error handling in the bottom half of the probe
> function for readability.  The invocation of clk_prepare_enable()
> is moved lower in the function and this simplifies a couple
> of return paths.  dev_err_probe() is also used when it is apt.
> 
> Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com>

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

> ---
>  drivers/pci/controller/pcie-brcmstb.c | 25 ++++++++++++-------------
>  1 file changed, 12 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
> index c08683febdd4..790a149f6581 100644
> --- a/drivers/pci/controller/pcie-brcmstb.c
> +++ b/drivers/pci/controller/pcie-brcmstb.c
> @@ -1613,25 +1613,23 @@ static int brcm_pcie_probe(struct platform_device *pdev)
>  
>  	pcie->ssc = of_property_read_bool(np, "brcm,enable-ssc");
>  
> -	ret = clk_prepare_enable(pcie->clk);
> -	if (ret) {
> -		dev_err(&pdev->dev, "could not enable clock\n");
> -		return ret;
> -	}
>  	pcie->rescal = devm_reset_control_get_optional_shared(&pdev->dev, "rescal");
> -	if (IS_ERR(pcie->rescal)) {
> -		clk_disable_unprepare(pcie->clk);
> +	if (IS_ERR(pcie->rescal))
>  		return PTR_ERR(pcie->rescal);
> -	}
> +
>  	pcie->perst_reset = devm_reset_control_get_optional_exclusive(&pdev->dev, "perst");
> -	if (IS_ERR(pcie->perst_reset)) {
> -		clk_disable_unprepare(pcie->clk);
> +	if (IS_ERR(pcie->perst_reset))
>  		return PTR_ERR(pcie->perst_reset);
> -	}
>  
> -	ret = reset_control_reset(pcie->rescal);
> +	ret = clk_prepare_enable(pcie->clk);
>  	if (ret)
> -		dev_err(&pdev->dev, "failed to deassert 'rescal'\n");
> +		return dev_err_probe(&pdev->dev, ret, "could not enable clock\n");
> +
> +	ret = reset_control_reset(pcie->rescal);
> +	if (ret) {
> +		clk_disable_unprepare(pcie->clk);
> +		return dev_err_probe(&pdev->dev, ret, "failed to deassert 'rescal'\n");
> +	}
>  
>  	ret = brcm_phy_start(pcie);
>  	if (ret) {
> @@ -1678,6 +1676,7 @@ static int brcm_pcie_probe(struct platform_device *pdev)
>  
>  fail:
>  	__brcm_pcie_remove(pcie);
> +

Irrelevant change in this patch.

- Mani

>  	return ret;
>  }
>  
> -- 
> 2.17.1
> 

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

Re: [PATCH v6 05/13] PCI: brcmstb: Use bridge reset if available

[Manivannan Sadhasivam]

On Thu, Aug 15, 2024 at 06:57:18PM -0400, Jim Quinlan wrote:
> The 7712 SOC has a bridge reset which can be described in the device tree.
> Use it if present.  Otherwise, continue to use the legacy method to reset
> the bridge.
> 
> Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com>

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

- Mani

> ---
>  drivers/pci/controller/pcie-brcmstb.c | 24 +++++++++++++++++++-----
>  1 file changed, 19 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
> index 790a149f6581..af14debd81d0 100644
> --- a/drivers/pci/controller/pcie-brcmstb.c
> +++ b/drivers/pci/controller/pcie-brcmstb.c
> @@ -265,6 +265,7 @@ struct brcm_pcie {
>  	enum pcie_type		type;
>  	struct reset_control	*rescal;
>  	struct reset_control	*perst_reset;
> +	struct reset_control	*bridge_reset;
>  	int			num_memc;
>  	u64			memc_size[PCIE_BRCM_MAX_MEMC];
>  	u32			hw_rev;
> @@ -732,12 +733,19 @@ static void __iomem *brcm7425_pcie_map_bus(struct pci_bus *bus,
>  
>  static void brcm_pcie_bridge_sw_init_set_generic(struct brcm_pcie *pcie, u32 val)
>  {
> -	u32 tmp, mask =  RGR1_SW_INIT_1_INIT_GENERIC_MASK;
> -	u32 shift = RGR1_SW_INIT_1_INIT_GENERIC_SHIFT;
> +	if (val)
> +		reset_control_assert(pcie->bridge_reset);
> +	else
> +		reset_control_deassert(pcie->bridge_reset);
>  
> -	tmp = readl(pcie->base + PCIE_RGR1_SW_INIT_1(pcie));
> -	tmp = (tmp & ~mask) | ((val << shift) & mask);
> -	writel(tmp, pcie->base + PCIE_RGR1_SW_INIT_1(pcie));
> +	if (!pcie->bridge_reset) {
> +		u32 tmp, mask =  RGR1_SW_INIT_1_INIT_GENERIC_MASK;
> +		u32 shift = RGR1_SW_INIT_1_INIT_GENERIC_SHIFT;
> +
> +		tmp = readl(pcie->base + PCIE_RGR1_SW_INIT_1(pcie));
> +		tmp = (tmp & ~mask) | ((val << shift) & mask);
> +		writel(tmp, pcie->base + PCIE_RGR1_SW_INIT_1(pcie));
> +	}
>  }
>  
>  static void brcm_pcie_bridge_sw_init_set_7278(struct brcm_pcie *pcie, u32 val)
> @@ -1621,10 +1629,16 @@ static int brcm_pcie_probe(struct platform_device *pdev)
>  	if (IS_ERR(pcie->perst_reset))
>  		return PTR_ERR(pcie->perst_reset);
>  
> +	pcie->bridge_reset = devm_reset_control_get_optional_exclusive(&pdev->dev, "bridge");
> +	if (IS_ERR(pcie->bridge_reset))
> +		return PTR_ERR(pcie->bridge_reset);
> +
>  	ret = clk_prepare_enable(pcie->clk);
>  	if (ret)
>  		return dev_err_probe(&pdev->dev, ret, "could not enable clock\n");
>  
> +	pcie->bridge_sw_init_set(pcie, 0);
> +
>  	ret = reset_control_reset(pcie->rescal);
>  	if (ret) {
>  		clk_disable_unprepare(pcie->clk);
> -- 
> 2.17.1
> 

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

Re: [PATCH v6 06/13] PCI: brcmstb: Use swinit reset if available

[Manivannan Sadhasivam]

On Thu, Aug 15, 2024 at 06:57:19PM -0400, Jim Quinlan wrote:
> The 7712 SOC adds a software init reset device for the PCIe HW.
> If found in the DT node, use it.
> 
> Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com>

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

- Mani

> ---
>  drivers/pci/controller/pcie-brcmstb.c | 24 ++++++++++++++++++++++++
>  1 file changed, 24 insertions(+)
> 
> diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
> index af14debd81d0..aa21c4c7b7f7 100644
> --- a/drivers/pci/controller/pcie-brcmstb.c
> +++ b/drivers/pci/controller/pcie-brcmstb.c
> @@ -266,6 +266,7 @@ struct brcm_pcie {
>  	struct reset_control	*rescal;
>  	struct reset_control	*perst_reset;
>  	struct reset_control	*bridge_reset;
> +	struct reset_control	*swinit_reset;
>  	int			num_memc;
>  	u64			memc_size[PCIE_BRCM_MAX_MEMC];
>  	u32			hw_rev;
> @@ -1633,12 +1634,35 @@ static int brcm_pcie_probe(struct platform_device *pdev)
>  	if (IS_ERR(pcie->bridge_reset))
>  		return PTR_ERR(pcie->bridge_reset);
>  
> +	pcie->swinit_reset = devm_reset_control_get_optional_exclusive(&pdev->dev, "swinit");
> +	if (IS_ERR(pcie->swinit_reset))
> +		return PTR_ERR(pcie->swinit_reset);
> +
>  	ret = clk_prepare_enable(pcie->clk);
>  	if (ret)
>  		return dev_err_probe(&pdev->dev, ret, "could not enable clock\n");
>  
>  	pcie->bridge_sw_init_set(pcie, 0);
>  
> +	if (pcie->swinit_reset) {
> +		ret = reset_control_assert(pcie->swinit_reset);
> +		if (ret) {
> +			clk_disable_unprepare(pcie->clk);
> +			return dev_err_probe(&pdev->dev, ret,
> +					     "could not assert reset 'swinit'\n");
> +		}
> +
> +		/* HW team recommends 1us for proper sync and propagation of reset */
> +		udelay(1);
> +
> +		ret = reset_control_deassert(pcie->swinit_reset);
> +		if (ret) {
> +			clk_disable_unprepare(pcie->clk);
> +			return dev_err_probe(&pdev->dev, ret,
> +					     "could not de-assert reset 'swinit'\n");
> +		}
> +	}
> +
>  	ret = reset_control_reset(pcie->rescal);
>  	if (ret) {
>  		clk_disable_unprepare(pcie->clk);
> -- 
> 2.17.1
> 

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

Re: [PATCH v6 10/13] PCI: brcmstb: Refactor for chips with many regular inbound windows

[Manivannan Sadhasivam]

On Thu, Aug 15, 2024 at 06:57:23PM -0400, Jim Quinlan wrote:
> Provide support for new chips with multiple inbound windows while
> keeping the legacy support for the older chips.
> 
> In existing chips there are three inbound windows with fixed purposes: the
> first was for mapping SoC internal registers, the second was for memory,
> and the third was for memory but with the endian swapped.  Typically, only
> one window was used.
> 
> Complicating the inbound window usage was the fact that the PCIe HW would
> do a baroque internal mapping of system memory, and concatenate the regions
> of multiple memory controllers.
> 
> Newer chips such as the 7712 and Cable Modem SOCs take a step forward and
> drop the internal mapping while providing for multiple inbound windows.
> This works in concert with the dma-ranges property, where each provided
> range becomes an inbound window.
> 
> Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com>

LGTM!

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

It would be great if someone with knowledge of Broadcom chipset could review
this patch.

- Mani

> ---
>  drivers/pci/controller/pcie-brcmstb.c | 235 ++++++++++++++++++++------
>  1 file changed, 181 insertions(+), 54 deletions(-)
> 
> diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
> index 2431c5a75cde..c5d3a5e9e0fc 100644
> --- a/drivers/pci/controller/pcie-brcmstb.c
> +++ b/drivers/pci/controller/pcie-brcmstb.c
> @@ -75,15 +75,19 @@
>  #define PCIE_MEM_WIN0_HI(win)	\
>  		PCIE_MISC_CPU_2_PCIE_MEM_WIN0_HI + ((win) * 8)
>  
> +/*
> + * NOTE: You may see the term "BAR" in a number of register names used by
> + *   this driver.  The term is an artifact of when the HW core was an
> + *   endpoint device (EP).  Now it is a root complex (RC) and anywhere a
> + *   register has the term "BAR" it is related to an inbound window.
> + */
> +
> +#define PCIE_BRCM_MAX_INBOUND_WINS			16
>  #define PCIE_MISC_RC_BAR1_CONFIG_LO			0x402c
>  #define  PCIE_MISC_RC_BAR1_CONFIG_LO_SIZE_MASK		0x1f
>  
> -#define PCIE_MISC_RC_BAR2_CONFIG_LO			0x4034
> -#define  PCIE_MISC_RC_BAR2_CONFIG_LO_SIZE_MASK		0x1f
> -#define PCIE_MISC_RC_BAR2_CONFIG_HI			0x4038
> +#define PCIE_MISC_RC_BAR4_CONFIG_LO			0x40d4
>  
> -#define PCIE_MISC_RC_BAR3_CONFIG_LO			0x403c
> -#define  PCIE_MISC_RC_BAR3_CONFIG_LO_SIZE_MASK		0x1f
>  
>  #define PCIE_MISC_MSI_BAR_CONFIG_LO			0x4044
>  #define PCIE_MISC_MSI_BAR_CONFIG_HI			0x4048
> @@ -130,6 +134,10 @@
>  	  (PCIE_MISC_HARD_PCIE_HARD_DEBUG_CLKREQ_DEBUG_ENABLE_MASK | \
>  	   PCIE_MISC_HARD_PCIE_HARD_DEBUG_L1SS_ENABLE_MASK)
>  
> +#define PCIE_MISC_UBUS_BAR1_CONFIG_REMAP			0x40ac
> +#define  PCIE_MISC_UBUS_BAR1_CONFIG_REMAP_ACCESS_EN_MASK	BIT(0)
> +#define PCIE_MISC_UBUS_BAR4_CONFIG_REMAP			0x410c
> +
>  #define PCIE_MSI_INTR2_BASE		0x4500
>  
>  /* Offsets from INTR2_CPU and MSI_INTR2 BASE offsets */
> @@ -217,12 +225,20 @@ enum pcie_type {
>  	BCM4908,
>  	BCM7278,
>  	BCM2711,
> +	BCM7712,
> +};
> +
> +struct inbound_win {
> +	u64 size;
> +	u64 pci_offset;
> +	u64 cpu_addr;
>  };
>  
>  struct pcie_cfg_data {
>  	const int *offsets;
>  	const enum pcie_type type;
>  	const bool has_phy;
> +	u8 num_inbound_wins;
>  	void (*perst_set)(struct brcm_pcie *pcie, u32 val);
>  	void (*bridge_sw_init_set)(struct brcm_pcie *pcie, u32 val);
>  };
> @@ -274,6 +290,7 @@ struct brcm_pcie {
>  	struct subdev_regulators *sr;
>  	bool			ep_wakeup_capable;
>  	bool			has_phy;
> +	u8			num_inbound_wins;
>  };
>  
>  static inline bool is_bmips(const struct brcm_pcie *pcie)
> @@ -396,7 +413,7 @@ static void brcm_pcie_set_gen(struct brcm_pcie *pcie, int gen)
>  }
>  
>  static void brcm_pcie_set_outbound_win(struct brcm_pcie *pcie,
> -				       unsigned int win, u64 cpu_addr,
> +				       u8 win, u64 cpu_addr,
>  				       u64 pcie_addr, u64 size)
>  {
>  	u32 cpu_addr_mb_high, limit_addr_mb_high;
> @@ -789,23 +806,62 @@ static void brcm_pcie_perst_set_generic(struct brcm_pcie *pcie, u32 val)
>  	writel(tmp, pcie->base + PCIE_RGR1_SW_INIT_1(pcie));
>  }
>  
> -static int brcm_pcie_get_rc_bar2_size_and_offset(struct brcm_pcie *pcie,
> -							u64 *rc_bar2_size,
> -							u64 *rc_bar2_offset)
> +static void add_inbound_win(struct inbound_win *b, u8 *count, u64 size,
> +			    u64 cpu_addr, u64 pci_offset)
> +{
> +	b->size = size;
> +	b->cpu_addr = cpu_addr;
> +	b->pci_offset = pci_offset;
> +	(*count)++;
> +}
> +
> +static int brcm_pcie_get_inbound_wins(struct brcm_pcie *pcie,
> +				      struct inbound_win inbound_wins[])
>  {
>  	struct pci_host_bridge *bridge = pci_host_bridge_from_priv(pcie);
> +	u64 pci_offset, cpu_addr, size = 0, tot_size = 0;
>  	struct resource_entry *entry;
>  	struct device *dev = pcie->dev;
>  	u64 lowest_pcie_addr = ~(u64)0;
>  	int ret, i = 0;
> -	u64 size = 0;
> +	u8 n = 0;
> +
> +	/*
> +	 * The HW registers (and PCIe) use order-1 numbering for BARs. As
> +	 * such, we have inbound_wins[0] unused and BAR1 starts at inbound_wins[1].
> +	 */
> +	struct inbound_win *b_begin = &inbound_wins[1];
> +	struct inbound_win *b = b_begin;
> +
> +	/*
> +	 * STB chips beside 7712 disable the first inbound window default.
> +	 * Rather being mapped to system memory it is mapped to the
> +	 * internal registers of the SoC.  This feature is deprecated, has
> +	 * security considerations, and is not implemented in our modern
> +	 * SoCs.
> +	 */
> +	if (pcie->type != BCM7712)
> +		add_inbound_win(b++, &n, 0, 0, 0);
>  
>  	resource_list_for_each_entry(entry, &bridge->dma_ranges) {
> -		u64 pcie_beg = entry->res->start - entry->offset;
> +		u64 pcie_start = entry->res->start - entry->offset;
> +		u64 cpu_start = entry->res->start;
>  
> -		size += entry->res->end - entry->res->start + 1;
> -		if (pcie_beg < lowest_pcie_addr)
> -			lowest_pcie_addr = pcie_beg;
> +		size = resource_size(entry->res);
> +		tot_size += size;
> +		if (pcie_start < lowest_pcie_addr)
> +			lowest_pcie_addr = pcie_start;
> +		/*
> +		 * 7712 and newer chips may have many BARs, with each
> +		 * offering a non-overlapping viewport to system memory.
> +		 * That being said, each BARs size must still be a power of
> +		 * two.
> +		 */
> +		if (pcie->type == BCM7712)
> +			add_inbound_win(b++, &n, size, cpu_start, pcie_start);
> +
> +		if (n > pcie->num_inbound_wins)
> +			break;
>  	}
>  
>  	if (lowest_pcie_addr == ~(u64)0) {
> @@ -813,13 +869,20 @@ static int brcm_pcie_get_rc_bar2_size_and_offset(struct brcm_pcie *pcie,
>  		return -EINVAL;
>  	}
>  
> +	/*
> +	 * 7712 and newer chips do not have an internal memory mapping system
> +	 * that enables multiple memory controllers.  As such, it can return
> +	 * now w/o doing special configuration.
> +	 */
> +	if (pcie->type == BCM7712)
> +		return n;
> +
>  	ret = of_property_read_variable_u64_array(pcie->np, "brcm,scb-sizes", pcie->memc_size, 1,
>  						  PCIE_BRCM_MAX_MEMC);
> -
>  	if (ret <= 0) {
>  		/* Make an educated guess */
>  		pcie->num_memc = 1;
> -		pcie->memc_size[0] = 1ULL << fls64(size - 1);
> +		pcie->memc_size[0] = 1ULL << fls64(tot_size - 1);
>  	} else {
>  		pcie->num_memc = ret;
>  	}
> @@ -828,10 +891,15 @@ static int brcm_pcie_get_rc_bar2_size_and_offset(struct brcm_pcie *pcie,
>  	for (i = 0, size = 0; i < pcie->num_memc; i++)
>  		size += pcie->memc_size[i];
>  
> -	/* System memory starts at this address in PCIe-space */
> -	*rc_bar2_offset = lowest_pcie_addr;
> -	/* The sum of all memc views must also be a power of 2 */
> -	*rc_bar2_size = 1ULL << fls64(size - 1);
> +	/* Our HW mandates that the window size must be a power of 2 */
> +	size = 1ULL << fls64(size - 1);
> +
> +	/*
> +	 * For STB chips, the BAR2 cpu_addr is hardwired to the start
> +	 * of system memory, so we set it to 0.
> +	 */
> +	cpu_addr = 0;
> +	pci_offset = lowest_pcie_addr;
>  
>  	/*
>  	 * We validate the inbound memory view even though we should trust
> @@ -866,25 +934,90 @@ static int brcm_pcie_get_rc_bar2_size_and_offset(struct brcm_pcie *pcie,
>  	 *   outbound memory @ 3GB). So instead it will  start at the 1x
>  	 *   multiple of its size
>  	 */
> -	if (!*rc_bar2_size || (*rc_bar2_offset & (*rc_bar2_size - 1)) ||
> -	    (*rc_bar2_offset < SZ_4G && *rc_bar2_offset > SZ_2G)) {
> -		dev_err(dev, "Invalid rc_bar2_offset/size: size 0x%llx, off 0x%llx\n",
> -			*rc_bar2_size, *rc_bar2_offset);
> +	if (!size || (pci_offset & (size - 1)) ||
> +	    (pci_offset < SZ_4G && pci_offset > SZ_2G)) {
> +		dev_err(dev, "Invalid inbound_win2_offset/size: size 0x%llx, off 0x%llx\n",
> +			size, pci_offset);
>  		return -EINVAL;
>  	}
>  
> -	return 0;
> +	/* Enable inbound window 2, the main inbound window for STB chips */
> +	add_inbound_win(b++, &n, size, cpu_addr, pci_offset);
> +
> +	/*
> +	 * Disable inbound window 3.  On some chips presents the same
> +	 * window as #2 but the data appears in a settable endianness.
> +	 */
> +	add_inbound_win(b++, &n, 0, 0, 0);
> +
> +	return n;
> +}
> +
> +static u32 brcm_bar_reg_offset(int bar)
> +{
> +	if (bar <= 3)
> +		return PCIE_MISC_RC_BAR1_CONFIG_LO + 8 * (bar - 1);
> +	else
> +		return PCIE_MISC_RC_BAR4_CONFIG_LO + 8 * (bar - 4);
> +}
> +
> +static u32 brcm_ubus_reg_offset(int bar)
> +{
> +	if (bar <= 3)
> +		return PCIE_MISC_UBUS_BAR1_CONFIG_REMAP + 8 * (bar - 1);
> +	else
> +		return PCIE_MISC_UBUS_BAR4_CONFIG_REMAP + 8 * (bar - 4);
> +}
> +
> +static void set_inbound_win_registers(struct brcm_pcie *pcie,
> +				      const struct inbound_win *inbound_wins,
> +				      u8 num_inbound_wins)
> +{
> +	void __iomem *base = pcie->base;
> +	int i;
> +
> +	for (i = 1; i <= num_inbound_wins; i++) {
> +		u64 pci_offset = inbound_wins[i].pci_offset;
> +		u64 cpu_addr = inbound_wins[i].cpu_addr;
> +		u64 size = inbound_wins[i].size;
> +		u32 reg_offset = brcm_bar_reg_offset(i);
> +		u32 tmp = lower_32_bits(pci_offset);
> +
> +		u32p_replace_bits(&tmp, brcm_pcie_encode_ibar_size(size),
> +				  PCIE_MISC_RC_BAR1_CONFIG_LO_SIZE_MASK);
> +
> +		/* Write low */
> +		writel_relaxed(tmp, base + reg_offset);
> +		/* Write high */
> +		writel_relaxed(upper_32_bits(pci_offset), base + reg_offset + 4);
> +
> +		/*
> +		 * Most STB chips:
> +		 *     Do nothing.
> +		 * 7712:
> +		 *     All of their BARs need to be set.
> +		 */
> +		if (pcie->type == BCM7712) {
> +			/* BUS remap register settings */
> +			reg_offset = brcm_ubus_reg_offset(i);
> +			tmp = lower_32_bits(cpu_addr) & ~0xfff;
> +			tmp |= PCIE_MISC_UBUS_BAR1_CONFIG_REMAP_ACCESS_EN_MASK;
> +			writel_relaxed(tmp, base + reg_offset);
> +			tmp = upper_32_bits(cpu_addr);
> +			writel_relaxed(tmp, base + reg_offset + 4);
> +		}
> +	}
>  }
>  
>  static int brcm_pcie_setup(struct brcm_pcie *pcie)
>  {
> -	u64 rc_bar2_offset, rc_bar2_size;
> +	struct inbound_win inbound_wins[PCIE_BRCM_MAX_INBOUND_WINS];
>  	void __iomem *base = pcie->base;
>  	struct pci_host_bridge *bridge;
>  	struct resource_entry *entry;
>  	u32 tmp, burst, aspm_support;
> -	int num_out_wins = 0;
> -	int ret, memc;
> +	u8 num_out_wins = 0, num_inbound_wins = 0;
> +	int memc;
>  
>  	/* Reset the bridge */
>  	pcie->bridge_sw_init_set(pcie, 1);
> @@ -933,17 +1066,16 @@ static int brcm_pcie_setup(struct brcm_pcie *pcie)
>  	u32p_replace_bits(&tmp, 1, PCIE_MISC_MISC_CTRL_PCIE_RCB_64B_MODE_MASK);
>  	writel(tmp, base + PCIE_MISC_MISC_CTRL);
>  
> -	ret = brcm_pcie_get_rc_bar2_size_and_offset(pcie, &rc_bar2_size,
> -						    &rc_bar2_offset);
> -	if (ret)
> -		return ret;
> +	num_inbound_wins = brcm_pcie_get_inbound_wins(pcie, inbound_wins);
> +	if (num_inbound_wins < 0)
> +		return num_inbound_wins;
> +
> +	set_inbound_win_registers(pcie, inbound_wins, num_inbound_wins);
>  
> -	tmp = lower_32_bits(rc_bar2_offset);
> -	u32p_replace_bits(&tmp, brcm_pcie_encode_ibar_size(rc_bar2_size),
> -			  PCIE_MISC_RC_BAR2_CONFIG_LO_SIZE_MASK);
> -	writel(tmp, base + PCIE_MISC_RC_BAR2_CONFIG_LO);
> -	writel(upper_32_bits(rc_bar2_offset),
> -	       base + PCIE_MISC_RC_BAR2_CONFIG_HI);
> +	if (!brcm_pcie_rc_mode(pcie)) {
> +		dev_err(pcie->dev, "PCIe RC controller misconfigured as Endpoint\n");
> +		return -EINVAL;
> +	}
>  
>  	tmp = readl(base + PCIE_MISC_MISC_CTRL);
>  	for (memc = 0; memc < pcie->num_memc; memc++) {
> @@ -965,25 +1097,12 @@ static int brcm_pcie_setup(struct brcm_pcie *pcie)
>  	 * 4GB or when the inbound area is smaller than 4GB (taking into
>  	 * account the rounding-up we're forced to perform).
>  	 */
> -	if (rc_bar2_offset >= SZ_4G || (rc_bar2_size + rc_bar2_offset) < SZ_4G)
> +	if (inbound_wins[2].pci_offset >= SZ_4G ||
> +	    (inbound_wins[2].size + inbound_wins[2].pci_offset) < SZ_4G)
>  		pcie->msi_target_addr = BRCM_MSI_TARGET_ADDR_LT_4GB;
>  	else
>  		pcie->msi_target_addr = BRCM_MSI_TARGET_ADDR_GT_4GB;
>  
> -	if (!brcm_pcie_rc_mode(pcie)) {
> -		dev_err(pcie->dev, "PCIe RC controller misconfigured as Endpoint\n");
> -		return -EINVAL;
> -	}
> -
> -	/* disable the PCIe->GISB memory window (RC_BAR1) */
> -	tmp = readl(base + PCIE_MISC_RC_BAR1_CONFIG_LO);
> -	tmp &= ~PCIE_MISC_RC_BAR1_CONFIG_LO_SIZE_MASK;
> -	writel(tmp, base + PCIE_MISC_RC_BAR1_CONFIG_LO);
> -
> -	/* disable the PCIe->SCB memory window (RC_BAR3) */
> -	tmp = readl(base + PCIE_MISC_RC_BAR3_CONFIG_LO);
> -	tmp &= ~PCIE_MISC_RC_BAR3_CONFIG_LO_SIZE_MASK;
> -	writel(tmp, base + PCIE_MISC_RC_BAR3_CONFIG_LO);
>  
>  	/* Don't advertise L0s capability if 'aspm-no-l0s' */
>  	aspm_support = PCIE_LINK_STATE_L1;
> @@ -1034,7 +1153,7 @@ static int brcm_pcie_setup(struct brcm_pcie *pcie)
>  		num_out_wins++;
>  	}
>  
> -	/* PCIe->SCB endian mode for BAR */
> +	/* PCIe->SCB endian mode for inbound window */
>  	tmp = readl(base + PCIE_RC_CFG_VENDOR_VENDOR_SPECIFIC_REG1);
>  	u32p_replace_bits(&tmp, PCIE_RC_CFG_VENDOR_SPCIFIC_REG1_LITTLE_ENDIAN,
>  		PCIE_RC_CFG_VENDOR_VENDOR_SPECIFIC_REG1_ENDIAN_MODE_BAR2_MASK);
> @@ -1516,6 +1635,7 @@ static const struct pcie_cfg_data generic_cfg = {
>  	.type		= GENERIC,
>  	.perst_set	= brcm_pcie_perst_set_generic,
>  	.bridge_sw_init_set = brcm_pcie_bridge_sw_init_set_generic,
> +	.num_inbound_wins = 3,
>  };
>  
>  static const struct pcie_cfg_data bcm7425_cfg = {
> @@ -1523,6 +1643,7 @@ static const struct pcie_cfg_data bcm7425_cfg = {
>  	.type		= BCM7425,
>  	.perst_set	= brcm_pcie_perst_set_generic,
>  	.bridge_sw_init_set = brcm_pcie_bridge_sw_init_set_generic,
> +	.num_inbound_wins = 3,
>  };
>  
>  static const struct pcie_cfg_data bcm7435_cfg = {
> @@ -1530,6 +1651,7 @@ static const struct pcie_cfg_data bcm7435_cfg = {
>  	.type		= BCM7435,
>  	.perst_set	= brcm_pcie_perst_set_generic,
>  	.bridge_sw_init_set = brcm_pcie_bridge_sw_init_set_generic,
> +	.num_inbound_wins = 3,
>  };
>  
>  static const struct pcie_cfg_data bcm4908_cfg = {
> @@ -1537,6 +1659,7 @@ static const struct pcie_cfg_data bcm4908_cfg = {
>  	.type		= BCM4908,
>  	.perst_set	= brcm_pcie_perst_set_4908,
>  	.bridge_sw_init_set = brcm_pcie_bridge_sw_init_set_generic,
> +	.num_inbound_wins = 3,
>  };
>  
>  static const int pcie_offset_bcm7278[] = {
> @@ -1552,6 +1675,7 @@ static const struct pcie_cfg_data bcm7278_cfg = {
>  	.type		= BCM7278,
>  	.perst_set	= brcm_pcie_perst_set_7278,
>  	.bridge_sw_init_set = brcm_pcie_bridge_sw_init_set_7278,
> +	.num_inbound_wins = 3,
>  };
>  
>  static const struct pcie_cfg_data bcm2711_cfg = {
> @@ -1559,6 +1683,7 @@ static const struct pcie_cfg_data bcm2711_cfg = {
>  	.type		= BCM2711,
>  	.perst_set	= brcm_pcie_perst_set_generic,
>  	.bridge_sw_init_set = brcm_pcie_bridge_sw_init_set_generic,
> +	.num_inbound_wins = 3,
>  };
>  
>  static const struct pcie_cfg_data bcm7216_cfg = {
> @@ -1567,6 +1692,7 @@ static const struct pcie_cfg_data bcm7216_cfg = {
>  	.perst_set	= brcm_pcie_perst_set_7278,
>  	.bridge_sw_init_set = brcm_pcie_bridge_sw_init_set_7278,
>  	.has_phy	= true,
> +	.num_inbound_wins = 3,
>  };
>  
>  static const struct of_device_id brcm_pcie_match[] = {
> @@ -1623,6 +1749,7 @@ static int brcm_pcie_probe(struct platform_device *pdev)
>  	pcie->perst_set = data->perst_set;
>  	pcie->bridge_sw_init_set = data->bridge_sw_init_set;
>  	pcie->has_phy = data->has_phy;
> +	pcie->num_inbound_wins = data->num_inbound_wins;
>  
>  	pcie->base = devm_platform_ioremap_resource(pdev, 0);
>  	if (IS_ERR(pcie->base))
> -- 
> 2.17.1
> 

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

Re: [PATCH v6 11/13] PCI: brcmstb: Check return value of all reset_control_xxx calls

[Manivannan Sadhasivam]

On Thu, Aug 15, 2024 at 06:57:24PM -0400, Jim Quinlan wrote:
> Always check the return value for invocations of reset_control_xxx() and
> propagate the error to the next level.  Although the current functions
> in reset-brcmstb.c cannot fail, this may someday change.
> 
> Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com>

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

But one comment below.

> Reviewed-by: Stanimir Varbanov <svarbanov@suse.de>
> Reviewed-by: Florian Fainelli <florian.fainelli@broadcom.com>
> ---
>  drivers/pci/controller/pcie-brcmstb.c | 102 ++++++++++++++++++--------
>  1 file changed, 73 insertions(+), 29 deletions(-)
> 
> diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
> index c5d3a5e9e0fc..d19eeeed623b 100644
> --- a/drivers/pci/controller/pcie-brcmstb.c
> +++ b/drivers/pci/controller/pcie-brcmstb.c

[...]

>  static int pci_dev_may_wakeup(struct pci_dev *dev, void *data)
> @@ -1479,9 +1515,12 @@ static int brcm_pcie_suspend_noirq(struct device *dev)
>  {
>  	struct brcm_pcie *pcie = dev_get_drvdata(dev);
>  	struct pci_host_bridge *bridge = pci_host_bridge_from_priv(pcie);
> -	int ret;
> +	int ret, rret;
> +
> +	ret = brcm_pcie_turn_off(pcie);
> +	if (ret)
> +		return ret;
>  
> -	brcm_pcie_turn_off(pcie);
>  	/*
>  	 * If brcm_phy_stop() returns an error, just dev_err(). If we
>  	 * return the error it will cause the suspend to fail and this is a
> @@ -1510,7 +1549,10 @@ static int brcm_pcie_suspend_noirq(struct device *dev)
>  						     pcie->sr->supplies);
>  			if (ret) {
>  				dev_err(dev, "Could not turn off regulators\n");
> -				reset_control_reset(pcie->rescal);
> +				rret = reset_control_reset(pcie->rescal);
> +				if (rret)
> +					dev_err(dev, "failed to reset 'rascal' controller ret=%d\n",

Do you really mean to say 'rascal'? ;)

- Mani

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

Re: [PATCH v6 12/13] PCI: brcmstb: Change field name from 'type' to 'soc_base'

[Manivannan Sadhasivam]

On Thu, Aug 15, 2024 at 06:57:25PM -0400, Jim Quinlan wrote:
> The 'type' field used in the driver to discern SoC differences is
> confusing; change it to the more apt 'soc_base'.  The 'base' is because
> some SoCs have the same characteristics as previous SoCs so it is
> convenient to classify them in the same group.
> 
> Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com>

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

- Mani

> ---
>  drivers/pci/controller/pcie-brcmstb.c | 42 +++++++++++++--------------
>  1 file changed, 21 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
> index d19eeeed623b..26e8f544da4c 100644
> --- a/drivers/pci/controller/pcie-brcmstb.c
> +++ b/drivers/pci/controller/pcie-brcmstb.c
> @@ -218,7 +218,7 @@ enum {
>  	PCIE_INTR2_CPU_BASE,
>  };
>  
> -enum pcie_type {
> +enum pcie_soc_base {
>  	GENERIC,
>  	BCM7425,
>  	BCM7435,
> @@ -236,7 +236,7 @@ struct inbound_win {
>  
>  struct pcie_cfg_data {
>  	const int *offsets;
> -	const enum pcie_type type;
> +	const enum pcie_soc_base soc_base;
>  	const bool has_phy;
>  	u8 num_inbound_wins;
>  	int (*perst_set)(struct brcm_pcie *pcie, u32 val);
> @@ -277,7 +277,7 @@ struct brcm_pcie {
>  	u64			msi_target_addr;
>  	struct brcm_msi		*msi;
>  	const int		*reg_offsets;
> -	enum pcie_type		type;
> +	enum pcie_soc_base	soc_base;
>  	struct reset_control	*rescal;
>  	struct reset_control	*perst_reset;
>  	struct reset_control	*bridge_reset;
> @@ -295,7 +295,7 @@ struct brcm_pcie {
>  
>  static inline bool is_bmips(const struct brcm_pcie *pcie)
>  {
> -	return pcie->type == BCM7435 || pcie->type == BCM7425;
> +	return pcie->soc_base == BCM7435 || pcie->soc_base == BCM7425;
>  }
>  
>  /*
> @@ -861,7 +861,7 @@ static int brcm_pcie_get_inbound_wins(struct brcm_pcie *pcie,
>  	 * security considerations, and is not implemented in our modern
>  	 * SoCs.
>  	 */
> -	if (pcie->type != BCM7712)
> +	if (pcie->soc_base != BCM7712)
>  		add_inbound_win(b++, &n, 0, 0, 0);
>  
>  	resource_list_for_each_entry(entry, &bridge->dma_ranges) {
> @@ -878,7 +878,7 @@ static int brcm_pcie_get_inbound_wins(struct brcm_pcie *pcie,
>  		 * That being said, each BARs size must still be a power of
>  		 * two.
>  		 */
> -		if (pcie->type == BCM7712)
> +		if (pcie->soc_base == BCM7712)
>  			add_inbound_win(b++, &n, size, cpu_start, pcie_start);
>  
>  		if (n > pcie->num_inbound_wins)
> @@ -895,7 +895,7 @@ static int brcm_pcie_get_inbound_wins(struct brcm_pcie *pcie,
>  	 * that enables multiple memory controllers.  As such, it can return
>  	 * now w/o doing special configuration.
>  	 */
> -	if (pcie->type == BCM7712)
> +	if (pcie->soc_base == BCM7712)
>  		return n;
>  
>  	ret = of_property_read_variable_u64_array(pcie->np, "brcm,scb-sizes", pcie->memc_size, 1,
> @@ -1018,7 +1018,7 @@ static void set_inbound_win_registers(struct brcm_pcie *pcie,
>  		 * 7712:
>  		 *     All of their BARs need to be set.
>  		 */
> -		if (pcie->type == BCM7712) {
> +		if (pcie->soc_base == BCM7712) {
>  			/* BUS remap register settings */
>  			reg_offset = brcm_ubus_reg_offset(i);
>  			tmp = lower_32_bits(cpu_addr) & ~0xfff;
> @@ -1046,7 +1046,7 @@ static int brcm_pcie_setup(struct brcm_pcie *pcie)
>  		return ret;
>  
>  	/* Ensure that PERST# is asserted; some bootloaders may deassert it. */
> -	if (pcie->type == BCM2711) {
> +	if (pcie->soc_base == BCM2711) {
>  		ret = pcie->perst_set(pcie, 1);
>  		if (ret) {
>  			pcie->bridge_sw_init_set(pcie, 0);
> @@ -1077,9 +1077,9 @@ static int brcm_pcie_setup(struct brcm_pcie *pcie)
>  	 */
>  	if (is_bmips(pcie))
>  		burst = 0x1; /* 256 bytes */
> -	else if (pcie->type == BCM2711)
> +	else if (pcie->soc_base == BCM2711)
>  		burst = 0x0; /* 128 bytes */
> -	else if (pcie->type == BCM7278)
> +	else if (pcie->soc_base == BCM7278)
>  		burst = 0x3; /* 512 bytes */
>  	else
>  		burst = 0x2; /* 512 bytes */
> @@ -1676,7 +1676,7 @@ static const int pcie_offsets_bmips_7425[] = {
>  
>  static const struct pcie_cfg_data generic_cfg = {
>  	.offsets	= pcie_offsets,
> -	.type		= GENERIC,
> +	.soc_base	= GENERIC,
>  	.perst_set	= brcm_pcie_perst_set_generic,
>  	.bridge_sw_init_set = brcm_pcie_bridge_sw_init_set_generic,
>  	.num_inbound_wins = 3,
> @@ -1684,7 +1684,7 @@ static const struct pcie_cfg_data generic_cfg = {
>  
>  static const struct pcie_cfg_data bcm7425_cfg = {
>  	.offsets	= pcie_offsets_bmips_7425,
> -	.type		= BCM7425,
> +	.soc_base	= BCM7425,
>  	.perst_set	= brcm_pcie_perst_set_generic,
>  	.bridge_sw_init_set = brcm_pcie_bridge_sw_init_set_generic,
>  	.num_inbound_wins = 3,
> @@ -1692,7 +1692,7 @@ static const struct pcie_cfg_data bcm7425_cfg = {
>  
>  static const struct pcie_cfg_data bcm7435_cfg = {
>  	.offsets	= pcie_offsets,
> -	.type		= BCM7435,
> +	.soc_base	= BCM7435,
>  	.perst_set	= brcm_pcie_perst_set_generic,
>  	.bridge_sw_init_set = brcm_pcie_bridge_sw_init_set_generic,
>  	.num_inbound_wins = 3,
> @@ -1700,7 +1700,7 @@ static const struct pcie_cfg_data bcm7435_cfg = {
>  
>  static const struct pcie_cfg_data bcm4908_cfg = {
>  	.offsets	= pcie_offsets,
> -	.type		= BCM4908,
> +	.soc_base	= BCM4908,
>  	.perst_set	= brcm_pcie_perst_set_4908,
>  	.bridge_sw_init_set = brcm_pcie_bridge_sw_init_set_generic,
>  	.num_inbound_wins = 3,
> @@ -1716,7 +1716,7 @@ static const int pcie_offset_bcm7278[] = {
>  
>  static const struct pcie_cfg_data bcm7278_cfg = {
>  	.offsets	= pcie_offset_bcm7278,
> -	.type		= BCM7278,
> +	.soc_base	= BCM7278,
>  	.perst_set	= brcm_pcie_perst_set_7278,
>  	.bridge_sw_init_set = brcm_pcie_bridge_sw_init_set_7278,
>  	.num_inbound_wins = 3,
> @@ -1724,7 +1724,7 @@ static const struct pcie_cfg_data bcm7278_cfg = {
>  
>  static const struct pcie_cfg_data bcm2711_cfg = {
>  	.offsets	= pcie_offsets,
> -	.type		= BCM2711,
> +	.soc_base	= BCM2711,
>  	.perst_set	= brcm_pcie_perst_set_generic,
>  	.bridge_sw_init_set = brcm_pcie_bridge_sw_init_set_generic,
>  	.num_inbound_wins = 3,
> @@ -1732,7 +1732,7 @@ static const struct pcie_cfg_data bcm2711_cfg = {
>  
>  static const struct pcie_cfg_data bcm7216_cfg = {
>  	.offsets	= pcie_offset_bcm7278,
> -	.type		= BCM7278,
> +	.soc_base	= BCM7278,
>  	.perst_set	= brcm_pcie_perst_set_7278,
>  	.bridge_sw_init_set = brcm_pcie_bridge_sw_init_set_7278,
>  	.has_phy	= true,
> @@ -1789,7 +1789,7 @@ static int brcm_pcie_probe(struct platform_device *pdev)
>  	pcie->dev = &pdev->dev;
>  	pcie->np = np;
>  	pcie->reg_offsets = data->offsets;
> -	pcie->type = data->type;
> +	pcie->soc_base = data->soc_base;
>  	pcie->perst_set = data->perst_set;
>  	pcie->bridge_sw_init_set = data->bridge_sw_init_set;
>  	pcie->has_phy = data->has_phy;
> @@ -1867,7 +1867,7 @@ static int brcm_pcie_probe(struct platform_device *pdev)
>  		goto fail;
>  
>  	pcie->hw_rev = readl(pcie->base + PCIE_MISC_REVISION);
> -	if (pcie->type == BCM4908 && pcie->hw_rev >= BRCM_PCIE_HW_REV_3_20) {
> +	if (pcie->soc_base == BCM4908 && pcie->hw_rev >= BRCM_PCIE_HW_REV_3_20) {
>  		dev_err(pcie->dev, "hardware revision with unsupported PERST# setup\n");
>  		ret = -ENODEV;
>  		goto fail;
> @@ -1882,7 +1882,7 @@ static int brcm_pcie_probe(struct platform_device *pdev)
>  		}
>  	}
>  
> -	bridge->ops = pcie->type == BCM7425 ? &brcm7425_pcie_ops : &brcm_pcie_ops;
> +	bridge->ops = pcie->soc_base == BCM7425 ? &brcm7425_pcie_ops : &brcm_pcie_ops;
>  	bridge->sysdata = pcie;
>  
>  	platform_set_drvdata(pdev, pcie);
> -- 
> 2.17.1
> 

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

Re: [PATCH v6 00/13] PCI: brcnstb: Enable STB 7712 SOC

[Manivannan Sadhasivam]

On Thu, Aug 15, 2024 at 06:57:13PM -0400, Jim Quinlan wrote:
> V6 Changes
>   o Commit "Refactor for chips with many regular inbound windows"
>     -- Use u8 for anything storing/counting # inbound windows (Stan)
>     -- s/set_bar/add_inbound_win/g (Manivannan)
>     -- Drop use of "inline" (Manivannan)
>     -- Change cpu_beg to cpu_start, same with pcie_beg. (Manivannan)
>     -- Used writel_relaxed() (Manivannan)
>   o Use swinit reset if available
>     -- Proper use of dev_err_probe() (Stan)
>   o Commit "Use common error handling code in brcm_pcie_probe()"
>     -- Rewrite commit msg in paragraph form (Manivannan)
>     -- Refactor error path at end of probe func (Manivannan)
>     -- Proper use of dev_err_probe() (Stan)
>   o New commit "dt-bindings: PCI: Change brcmstb maintainer and cleanup"
>     -- Break out maintainer change and small cleanup into a
>        separate commit (Krzysztof)
> 

Looks like you've missed adding the review tags...

- Mani

> V5 Changes:
>   o All commits: Use imperative voice in commit subjects/messages
>        (Manivannan)
>   o Commit "PCI: brcmstb: Enable 7712 SOCs"
>     -- Augment commit message to include PCIe details and revision.
>        (Manivannan)
>   o Commit "PCI: brcmstb: Change field name from 'type' to 'model'"
>     -- Instead of "model" use "soc_base" (Manivannan)
>   o Commit "PCI: brcmstb: Refactor for chips with many regular inbound BARs"
>     -- Get rid of the confusing "BAR" variable names and types and use
>        something like "inbound_win". (Manivannan)
>   o Commit "PCI: brcmstb: PCI: brcmstb: Make HARD_DEBUG, INTR2_CPU_BASE..."
>     -- Mention in the commit message that this change is in preparation
>        for the 7712 SoC. (Manivannan)
>   o Commit: "PCI: brcmstb: Use swinit reset if available"
>     -- Change reset name "swinit" to "swinit_reset" (Manivannan)
>     -- Add 1us delay for reset (Manivannan)
>     -- Use dev_err_probe() (Multiple reviewers)
>   o Commit "PCI: brcmstb: Use bridge reset if available"
>     -- Change reset name "bridge" to "bridge_reset" (Manivannan)
>     -- The Reset API can take NULL so need need to test variable
>        before calling (Manivannan)
>     -- Added a call to bridge_sw_init_set() method in probe()
>        as some registers cannot be accessed w/o this. (JQ)
>   o Commit "PCI: brcmstb: Use common error handling code in ..."
>     -- Use more descriptive goto label (Manivannan)
>     -- Refactor error paths to be less encumbered (Manivannan)
>     -- Use dev_err_probe() (Multiple reviewers)
>   o Commits "dt-bindings: PCI: brcmstb: ..."
>     -- Specify the "resets" and "reset-names" in the same manner
>        as does qcom,ufs.yaml specifies "clocks" and
>        "clock-names" (Krzysztof)
>     -- Drop reset desccriptions as they were pretty content-free
>        anyhow. (Krzysztof)
> 
> V4 Changes:
>   o Commit "Check return value of all reset_control_xxx calls"
>     -- Blank line before "return" (Stan)
>   o Commit "Use common error handling code in brcmstb_probe()"
>     -- Drop the "Fixes" tag (Stan)
>   o Commit "dt-bindings: PCI ..."
>     -- Separate the main commit into two: cleanup and adding the
>        7712 SoC (Krzysztof)
>     -- Fold maintainer change commit into cleanup change (Krzysztof)
>     -- Use minItems/maxItems where appropriate (Krzysztof)
>     -- Consistent order of resets/reset-names in decl and usage
>        (Krzysztof)
> 
> V3 Changes:
>   o Commit "Enable 7712 SOCs"
>     -- Move "model" check from outside to inside func (Stan)
>   o Commit "Check return value of all reset_control_xxx calls"
>     -- Propagate errors up the chain instead of ignoring them (Stan)
>   o Commit "Refactor for chips with many regular inbound BARs"
>     -- Nine suggestions given, nine implemented (Stan)
>   o Commit "Make HARD_DEBUG, INTR2_CPU_BASE offsets SoC-specific"
>     -- Drop tab, add parens around macro params in expression (Stan)
>   o Commit "Use swinit reset if available"
>     -- Treat swinit the same as other reset controllers (Stan)
>        Stan suggested to use dev_err_probe() for getting resources
>        but I will defer that to future series (if that's okay).
>   o Commit "Get resource before we start asserting resets"
>     -- Squash this with previous commit (Stan)
>   o Commit "Use "clk_out" error path label"
>     -- Move clk_prepare_enable() after getting resouurces (Stan)
>     -- Change subject to "Use more common error handling code in
>        brcm_pcie_probe()" (Markus)
>     -- Use imperative commit description (Markus)
>     -- "Fixes:" tag added for missing error return. (Markus)
>   o Commit "dt-bindings: PCI ..."
>     -- Split off maintainer change in separate commit.
>     -- Tried to accomodate Krzysztof's requests, I'm not sure I
>        have succeeded.  Krzysztof, please see [1] below.
>   
>   [1] Wrt the YAML of brcmstb PCIe resets, here is what I am trying
>       to describe:
> 
>       CHIP       NUM_RESETS    NAMES
>       ====       ==========    =====
>       4908       1             perst
>       7216       1             rescal
>       7712       3             rescal, bridge, swinit
>       Others     0             -
> 
> 
> V2 Changes (note: four new commits):
>   o Commit "dt-bindings: PCI ..."
>     -- s/Adds/Add/, fix spelling error (Bjorn)
>     -- Order compatible strings alphabetically (Krzysztof)
>     -- Give definitions first then rules (Krzysztof)
>     -- Add reason for change in maintainer (Krzysztof)
>   o Commit "Use swinit reset if available"
>     -- no need for "else" clause (Philipp)
>     -- fix improper use of dev_err_probe() (Philipp) 
>   o Commit "Use "clk_out" error path label"
>     -- Improve commit message (Bjorn)
>   o Commit "PCI: brcmstb: Make HARD_DEBUG, INTR2_CPU_BASE offsets SoC-specific"
>     -- Improve commit subject line (Bjorn)
>   o Commit (NEW) -- Change field name from 'type' to 'model'
>     -- Added as requested (Stanimir)
>   o Commit (NEW) -- Check return value of all reset_control_xxx calls
>     -- Added as requested (Stanimir)
>   o Commit (NEW) "Get resource before we start asserting reset controllers"
>     -- Added as requested (Stanimir)
>   o Commit (NEW) -- "Remove two unused constants from driver"
> 
> 
> V1:
>   This submission is for the Broadcom STB 7712, sibling SOC of the RPi5 chip.
>   Stanimir has already submitted a patch "Add PCIe support for bcm2712" for
>   the RPi version of the SOC.  It is hoped that Stanimir will allow us to
>   submit this series first and subsequently rebase his patch(es).
> 
>   The largest commit, "Refactor for chips with many regular inbound BARs"
>   affects both the STB and RPi SOCs.  It allows for multiple inbound ranges
>   where previously only one was effectively used.  This feature will also
>   be present in future STB chips, as well as Broadcom's Cable Modem group.
> 
> Jim Quinlan (13):
>   dt-bindings: PCI: Change brcmstb maintainer and cleanup
>   dt-bindings: PCI: Use maxItems for reset controllers
>   dt-bindings: PCI: brcmstb: Add 7712 SoC description
>   PCI: brcmstb: Use common error handling code in brcm_pcie_probe()
>   PCI: brcmstb: Use bridge reset if available
>   PCI: brcmstb: Use swinit reset if available
>   PCI: brcmstb: PCI: brcmstb: Make HARD_DEBUG, INTR2_CPU_BASE offsets
>     SoC-specific
>   PCI: brcmstb: Remove two unused constants from driver
>   PCI: brcmstb: Don't conflate the reset rescal with phy ctrl
>   PCI: brcmstb: Refactor for chips with many regular inbound windows
>   PCI: brcmstb: Check return value of all reset_control_xxx calls
>   PCI: brcmstb: Change field name from 'type' to 'soc_base'
>   PCI: brcmstb: Enable 7712 SOCs
> 
>  .../bindings/pci/brcm,stb-pcie.yaml           |  40 +-
>  drivers/pci/controller/pcie-brcmstb.c         | 513 +++++++++++++-----
>  2 files changed, 412 insertions(+), 141 deletions(-)
> 
> 
> base-commit: e724918b3786252b985b0c2764c16a57d1937707
> -- 
> 2.17.1
> 

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

Re: [PATCH v6 01/13] dt-bindings: PCI: Change brcmstb maintainer and cleanup

[Florian Fainelli]

On 8/15/24 15:57, Jim Quinlan wrote:
> Change maintainer: Nicolas has not been active for a while.
> It also makes sense for a Broadcom employee to be the
> maintainer as many of the details are privy to Broadcom.
> 
> Also, alphabetize the compatible strings.
> 
> Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com>

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

Re: [PATCH v6 02/13] dt-bindings: PCI: Use maxItems for reset controllers

[Florian Fainelli]

On 8/15/24 15:57, Jim Quinlan wrote:
> Provide the maxItem property for the reset controllers and drop their
> superfluous descriptions.
> 
> Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com>

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

Re: [PATCH v6 04/13] PCI: brcmstb: Use common error handling code in brcm_pcie_probe()

[Florian Fainelli]

On 8/15/24 15:57, Jim Quinlan wrote:
> Refactor the error handling in the bottom half of the probe
> function for readability.  The invocation of clk_prepare_enable()
> is moved lower in the function and this simplifies a couple
> of return paths.  dev_err_probe() is also used when it is apt.
> 
> Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com>

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

Re: [PATCH v6 05/13] PCI: brcmstb: Use bridge reset if available

[Florian Fainelli]

On 8/15/24 15:57, Jim Quinlan wrote:
> The 7712 SOC has a bridge reset which can be described in the device tree.
> Use it if present.  Otherwise, continue to use the legacy method to reset
> the bridge.
> 
> Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com>

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

Re: [PATCH v6 06/13] PCI: brcmstb: Use swinit reset if available

[Florian Fainelli]

On 8/15/24 15:57, Jim Quinlan wrote:
> The 7712 SOC adds a software init reset device for the PCIe HW.
> If found in the DT node, use it.
> 
> Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com>

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

Re: [PATCH v6 12/13] PCI: brcmstb: Change field name from 'type' to 'soc_base'

[Florian Fainelli]

On 8/15/24 15:57, Jim Quinlan wrote:
> The 'type' field used in the driver to discern SoC differences is
> confusing; change it to the more apt 'soc_base'.  The 'base' is because
> some SoCs have the same characteristics as previous SoCs so it is
> convenient to classify them in the same group.
> 
> Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com>

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

Re: [PATCH v6 10/13] PCI: brcmstb: Refactor for chips with many regular inbound windows

[Florian Fainelli]

On 8/15/24 15:57, Jim Quinlan wrote:
> Provide support for new chips with multiple inbound windows while
> keeping the legacy support for the older chips.
> 
> In existing chips there are three inbound windows with fixed purposes: the
> first was for mapping SoC internal registers, the second was for memory,
> and the third was for memory but with the endian swapped.  Typically, only
> one window was used.
> 
> Complicating the inbound window usage was the fact that the PCIe HW would
> do a baroque internal mapping of system memory, and concatenate the regions
> of multiple memory controllers.
> 
> Newer chips such as the 7712 and Cable Modem SOCs take a step forward and
> drop the internal mapping while providing for multiple inbound windows.
> This works in concert with the dma-ranges property, where each provided
> range becomes an inbound window.
> 
> Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com>

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

Re: [PATCH v6 10/13] PCI: brcmstb: Refactor for chips with many regular inbound windows

[Stanimir Varbanov]

On 8/16/24 01:57, Jim Quinlan wrote:
> Provide support for new chips with multiple inbound windows while
> keeping the legacy support for the older chips.
> 
> In existing chips there are three inbound windows with fixed purposes: the
> first was for mapping SoC internal registers, the second was for memory,
> and the third was for memory but with the endian swapped.  Typically, only
> one window was used.
> 
> Complicating the inbound window usage was the fact that the PCIe HW would
> do a baroque internal mapping of system memory, and concatenate the regions
> of multiple memory controllers.
> 
> Newer chips such as the 7712 and Cable Modem SOCs take a step forward and
> drop the internal mapping while providing for multiple inbound windows.
> This works in concert with the dma-ranges property, where each provided
> range becomes an inbound window.
> 
> Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com>
> ---
>  drivers/pci/controller/pcie-brcmstb.c | 235 ++++++++++++++++++++------
>  1 file changed, 181 insertions(+), 54 deletions(-)

Reviewed-by: Stanimir Varbanov <svarbanov@suse.de>

~Stan

Re: [PATCH v6 05/13] PCI: brcmstb: Use bridge reset if available

[Stanimir Varbanov]

Hi Jim,

On 8/16/24 01:57, Jim Quinlan wrote:
> The 7712 SOC has a bridge reset which can be described in the device tree.
> Use it if present.  Otherwise, continue to use the legacy method to reset
> the bridge.
> 
> Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com>
> ---
>  drivers/pci/controller/pcie-brcmstb.c | 24 +++++++++++++++++++-----
>  1 file changed, 19 insertions(+), 5 deletions(-)

Reviewed-by: Stanimir Varbanov <svarbanov@suse.de>

One problem though on RPi5 (bcm2712).

With this series applied + my WIP patches for enablement of PCIe on
bcm2712 when enable the pcie1 and pcie2 root ports in dts, I see kernel
boot stuck on pcie2 enumeration and I have to add this [1] to make it
work again.

Some more info about resets used:

pcie0 @ 100000:
	resets = <&bcm_reset 5>, <&bcm_reset 42>, <&pcie_rescal>;
	reset-names = "swinit", "bridge", "rescal";

pcie1 @ 110000:
	resets = <&bcm_reset 7>, <&bcm_reset 43>, <&pcie_rescal>;
	reset-names = "swinit", "bridge", "rescal";

pcie2 @ 120000:
	resets = <&bcm_reset 9>, <&bcm_reset 44>, <&pcie_rescal>;
	reset-names = "swinit", "bridge", "rescal";


I changed "swinit" reset for pcie2 to <&bcm_reset 9> (it is 32 in
downstream rpi kernel) because otherwise I'm unable to enumerate RP1
south bridge at all.

Any help will be appreciated.

~Stan

[1]
https://github.com/raspberrypi/linux/blob/rpi-6.11.y/drivers/pci/controller/pcie-brcmstb.c#L1711

Re: [PATCH v6 00/13] PCI: brcnstb: Enable STB 7712 SOC

[Jim Quinlan]

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

On Fri, Aug 16, 2024 at 3:18 AM Manivannan Sadhasivam
<manivannan.sadhasivam@linaro.org> wrote:
>
> On Thu, Aug 15, 2024 at 06:57:13PM -0400, Jim Quinlan wrote:
> > V6 Changes
> >   o Commit "Refactor for chips with many regular inbound windows"
> >     -- Use u8 for anything storing/counting # inbound windows (Stan)
> >     -- s/set_bar/add_inbound_win/g (Manivannan)
> >     -- Drop use of "inline" (Manivannan)
> >     -- Change cpu_beg to cpu_start, same with pcie_beg. (Manivannan)
> >     -- Used writel_relaxed() (Manivannan)
> >   o Use swinit reset if available
> >     -- Proper use of dev_err_probe() (Stan)
> >   o Commit "Use common error handling code in brcm_pcie_probe()"
> >     -- Rewrite commit msg in paragraph form (Manivannan)
> >     -- Refactor error path at end of probe func (Manivannan)
> >     -- Proper use of dev_err_probe() (Stan)
> >   o New commit "dt-bindings: PCI: Change brcmstb maintainer and cleanup"
> >     -- Break out maintainer change and small cleanup into a
> >        separate commit (Krzysztof)
> >
>
> Looks like you've missed adding the review tags...


I didn't mention this in the cover letter but I update the review tags
at each version.  The problem is that if someone has reviewed a
commit, and then that commit is subsequently changed due to another
reviewer, I have to remove the existing tag of the first reviewer
because the code has changed.

Regards,
Jim Quinlan
Broadcom STB/CM
>
>
> - Mani
>
> > V5 Changes:
> >   o All commits: Use imperative voice in commit subjects/messages
> >        (Manivannan)
> >   o Commit "PCI: brcmstb: Enable 7712 SOCs"
> >     -- Augment commit message to include PCIe details and revision.
> >        (Manivannan)
> >   o Commit "PCI: brcmstb: Change field name from 'type' to 'model'"
> >     -- Instead of "model" use "soc_base" (Manivannan)
> >   o Commit "PCI: brcmstb: Refactor for chips with many regular inbound BARs"
> >     -- Get rid of the confusing "BAR" variable names and types and use
> >        something like "inbound_win". (Manivannan)
> >   o Commit "PCI: brcmstb: PCI: brcmstb: Make HARD_DEBUG, INTR2_CPU_BASE..."
> >     -- Mention in the commit message that this change is in preparation
> >        for the 7712 SoC. (Manivannan)
> >   o Commit: "PCI: brcmstb: Use swinit reset if available"
> >     -- Change reset name "swinit" to "swinit_reset" (Manivannan)
> >     -- Add 1us delay for reset (Manivannan)
> >     -- Use dev_err_probe() (Multiple reviewers)
> >   o Commit "PCI: brcmstb: Use bridge reset if available"
> >     -- Change reset name "bridge" to "bridge_reset" (Manivannan)
> >     -- The Reset API can take NULL so need need to test variable
> >        before calling (Manivannan)
> >     -- Added a call to bridge_sw_init_set() method in probe()
> >        as some registers cannot be accessed w/o this. (JQ)
> >   o Commit "PCI: brcmstb: Use common error handling code in ..."
> >     -- Use more descriptive goto label (Manivannan)
> >     -- Refactor error paths to be less encumbered (Manivannan)
> >     -- Use dev_err_probe() (Multiple reviewers)
> >   o Commits "dt-bindings: PCI: brcmstb: ..."
> >     -- Specify the "resets" and "reset-names" in the same manner
> >        as does qcom,ufs.yaml specifies "clocks" and
> >        "clock-names" (Krzysztof)
> >     -- Drop reset desccriptions as they were pretty content-free
> >        anyhow. (Krzysztof)
> >
> > V4 Changes:
> >   o Commit "Check return value of all reset_control_xxx calls"
> >     -- Blank line before "return" (Stan)
> >   o Commit "Use common error handling code in brcmstb_probe()"
> >     -- Drop the "Fixes" tag (Stan)
> >   o Commit "dt-bindings: PCI ..."
> >     -- Separate the main commit into two: cleanup and adding the
> >        7712 SoC (Krzysztof)
> >     -- Fold maintainer change commit into cleanup change (Krzysztof)
> >     -- Use minItems/maxItems where appropriate (Krzysztof)
> >     -- Consistent order of resets/reset-names in decl and usage
> >        (Krzysztof)
> >
> > V3 Changes:
> >   o Commit "Enable 7712 SOCs"
> >     -- Move "model" check from outside to inside func (Stan)
> >   o Commit "Check return value of all reset_control_xxx calls"
> >     -- Propagate errors up the chain instead of ignoring them (Stan)
> >   o Commit "Refactor for chips with many regular inbound BARs"
> >     -- Nine suggestions given, nine implemented (Stan)
> >   o Commit "Make HARD_DEBUG, INTR2_CPU_BASE offsets SoC-specific"
> >     -- Drop tab, add parens around macro params in expression (Stan)
> >   o Commit "Use swinit reset if available"
> >     -- Treat swinit the same as other reset controllers (Stan)
> >        Stan suggested to use dev_err_probe() for getting resources
> >        but I will defer that to future series (if that's okay).
> >   o Commit "Get resource before we start asserting resets"
> >     -- Squash this with previous commit (Stan)
> >   o Commit "Use "clk_out" error path label"
> >     -- Move clk_prepare_enable() after getting resouurces (Stan)
> >     -- Change subject to "Use more common error handling code in
> >        brcm_pcie_probe()" (Markus)
> >     -- Use imperative commit description (Markus)
> >     -- "Fixes:" tag added for missing error return. (Markus)
> >   o Commit "dt-bindings: PCI ..."
> >     -- Split off maintainer change in separate commit.
> >     -- Tried to accomodate Krzysztof's requests, I'm not sure I
> >        have succeeded.  Krzysztof, please see [1] below.
> >
> >   [1] Wrt the YAML of brcmstb PCIe resets, here is what I am trying
> >       to describe:
> >
> >       CHIP       NUM_RESETS    NAMES
> >       ====       ==========    =====
> >       4908       1             perst
> >       7216       1             rescal
> >       7712       3             rescal, bridge, swinit
> >       Others     0             -
> >
> >
> > V2 Changes (note: four new commits):
> >   o Commit "dt-bindings: PCI ..."
> >     -- s/Adds/Add/, fix spelling error (Bjorn)
> >     -- Order compatible strings alphabetically (Krzysztof)
> >     -- Give definitions first then rules (Krzysztof)
> >     -- Add reason for change in maintainer (Krzysztof)
> >   o Commit "Use swinit reset if available"
> >     -- no need for "else" clause (Philipp)
> >     -- fix improper use of dev_err_probe() (Philipp)
> >   o Commit "Use "clk_out" error path label"
> >     -- Improve commit message (Bjorn)
> >   o Commit "PCI: brcmstb: Make HARD_DEBUG, INTR2_CPU_BASE offsets SoC-specific"
> >     -- Improve commit subject line (Bjorn)
> >   o Commit (NEW) -- Change field name from 'type' to 'model'
> >     -- Added as requested (Stanimir)
> >   o Commit (NEW) -- Check return value of all reset_control_xxx calls
> >     -- Added as requested (Stanimir)
> >   o Commit (NEW) "Get resource before we start asserting reset controllers"
> >     -- Added as requested (Stanimir)
> >   o Commit (NEW) -- "Remove two unused constants from driver"
> >
> >
> > V1:
> >   This submission is for the Broadcom STB 7712, sibling SOC of the RPi5 chip.
> >   Stanimir has already submitted a patch "Add PCIe support for bcm2712" for
> >   the RPi version of the SOC.  It is hoped that Stanimir will allow us to
> >   submit this series first and subsequently rebase his patch(es).
> >
> >   The largest commit, "Refactor for chips with many regular inbound BARs"
> >   affects both the STB and RPi SOCs.  It allows for multiple inbound ranges
> >   where previously only one was effectively used.  This feature will also
> >   be present in future STB chips, as well as Broadcom's Cable Modem group.
> >
> > Jim Quinlan (13):
> >   dt-bindings: PCI: Change brcmstb maintainer and cleanup
> >   dt-bindings: PCI: Use maxItems for reset controllers
> >   dt-bindings: PCI: brcmstb: Add 7712 SoC description
> >   PCI: brcmstb: Use common error handling code in brcm_pcie_probe()
> >   PCI: brcmstb: Use bridge reset if available
> >   PCI: brcmstb: Use swinit reset if available
> >   PCI: brcmstb: PCI: brcmstb: Make HARD_DEBUG, INTR2_CPU_BASE offsets
> >     SoC-specific
> >   PCI: brcmstb: Remove two unused constants from driver
> >   PCI: brcmstb: Don't conflate the reset rescal with phy ctrl
> >   PCI: brcmstb: Refactor for chips with many regular inbound windows
> >   PCI: brcmstb: Check return value of all reset_control_xxx calls
> >   PCI: brcmstb: Change field name from 'type' to 'soc_base'
> >   PCI: brcmstb: Enable 7712 SOCs
> >
> >  .../bindings/pci/brcm,stb-pcie.yaml           |  40 +-
> >  drivers/pci/controller/pcie-brcmstb.c         | 513 +++++++++++++-----
> >  2 files changed, 412 insertions(+), 141 deletions(-)
> >
> >
> > base-commit: e724918b3786252b985b0c2764c16a57d1937707
> > --
> > 2.17.1
> >
>
> --
> மணிவண்ணன் சதாசிவம்

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

Re: [PATCH v6 00/13] PCI: brcnstb: Enable STB 7712 SOC

[Florian Fainelli]

On 8/19/24 10:44, Jim Quinlan wrote:
> On Fri, Aug 16, 2024 at 3:18 AM Manivannan Sadhasivam
> <manivannan.sadhasivam@linaro.org> wrote:
>>
>> On Thu, Aug 15, 2024 at 06:57:13PM -0400, Jim Quinlan wrote:
>>> V6 Changes
>>>    o Commit "Refactor for chips with many regular inbound windows"
>>>      -- Use u8 for anything storing/counting # inbound windows (Stan)
>>>      -- s/set_bar/add_inbound_win/g (Manivannan)
>>>      -- Drop use of "inline" (Manivannan)
>>>      -- Change cpu_beg to cpu_start, same with pcie_beg. (Manivannan)
>>>      -- Used writel_relaxed() (Manivannan)
>>>    o Use swinit reset if available
>>>      -- Proper use of dev_err_probe() (Stan)
>>>    o Commit "Use common error handling code in brcm_pcie_probe()"
>>>      -- Rewrite commit msg in paragraph form (Manivannan)
>>>      -- Refactor error path at end of probe func (Manivannan)
>>>      -- Proper use of dev_err_probe() (Stan)
>>>    o New commit "dt-bindings: PCI: Change brcmstb maintainer and cleanup"
>>>      -- Break out maintainer change and small cleanup into a
>>>         separate commit (Krzysztof)
>>>
>>
>> Looks like you've missed adding the review tags...
> 
> 
> I didn't mention this in the cover letter but I update the review tags
> at each version.  The problem is that if someone has reviewed a
> commit, and then that commit is subsequently changed due to another
> reviewer, I have to remove the existing tag of the first reviewer
> because the code has changed.

It seems to me like you did the right thing here, the changes you did 
between v5 and v6 were substantial enough they invalidated the 
Reviewed-by tag(s) you had gotten previously.
-- 
Florian

Re: [PATCH v6 05/13] PCI: brcmstb: Use bridge reset if available

[Jim Quinlan]

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

On Sat, Aug 17, 2024 at 1:41 PM Stanimir Varbanov <svarbanov@suse.de> wrote:
>
> Hi Jim,
>
> On 8/16/24 01:57, Jim Quinlan wrote:
> > The 7712 SOC has a bridge reset which can be described in the device tree.
> > Use it if present.  Otherwise, continue to use the legacy method to reset
> > the bridge.
> >
> > Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com>
> > ---
> >  drivers/pci/controller/pcie-brcmstb.c | 24 +++++++++++++++++++-----
> >  1 file changed, 19 insertions(+), 5 deletions(-)
>
> Reviewed-by: Stanimir Varbanov <svarbanov@suse.de>
>
> One problem though on RPi5 (bcm2712).
>
> With this series applied + my WIP patches for enablement of PCIe on
> bcm2712 when enable the pcie1 and pcie2 root ports in dts, I see kernel
> boot stuck on pcie2 enumeration and I have to add this [1] to make it
> work again.
>
> Some more info about resets used:
>
> pcie0 @ 100000:
>         resets = <&bcm_reset 5>, <&bcm_reset 42>, <&pcie_rescal>;
>         reset-names = "swinit", "bridge", "rescal";
>
> pcie1 @ 110000:
>         resets = <&bcm_reset 7>, <&bcm_reset 43>, <&pcie_rescal>;
>         reset-names = "swinit", "bridge", "rescal";
>
> pcie2 @ 120000:
>         resets = <&bcm_reset 9>, <&bcm_reset 44>, <&pcie_rescal>;
>         reset-names = "swinit", "bridge", "rescal";
>
>
> I changed "swinit" reset for pcie2 to <&bcm_reset 9> (it is 32 in
> downstream rpi kernel) because otherwise I'm unable to enumerate RP1
> south bridge at all.
>
> Any help will be appreciated.

Hi Stan,
Let me look into this.  Why is one of the controllers turning off --
is it not populated with a device?

As you probably know the 7712 only has access to PCIe1.  But we do
have another chip with two controllers and I will try to reproduce
your failure and get to the bottom of it.

Regards,
Jim Quinlan
Broadcom STB/CM
>
> ~Stan
>
> [1]
> https://github.com/raspberrypi/linux/blob/rpi-6.11.y/drivers/pci/controller/pcie-brcmstb.c#L1711

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

Re: [PATCH v6 05/13] PCI: brcmstb: Use bridge reset if available

[Florian Fainelli]

On 8/17/24 10:41, Stanimir Varbanov wrote:
> Hi Jim,
> 
> On 8/16/24 01:57, Jim Quinlan wrote:
>> The 7712 SOC has a bridge reset which can be described in the device tree.
>> Use it if present.  Otherwise, continue to use the legacy method to reset
>> the bridge.
>>
>> Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com>
>> ---
>>   drivers/pci/controller/pcie-brcmstb.c | 24 +++++++++++++++++++-----
>>   1 file changed, 19 insertions(+), 5 deletions(-)
> 
> Reviewed-by: Stanimir Varbanov <svarbanov@suse.de>
> 
> One problem though on RPi5 (bcm2712).
> 
> With this series applied + my WIP patches for enablement of PCIe on
> bcm2712 when enable the pcie1 and pcie2 root ports in dts, I see kernel
> boot stuck on pcie2 enumeration and I have to add this [1] to make it
> work again.
> 
> Some more info about resets used:
> 
> pcie0 @ 100000:
> 	resets = <&bcm_reset 5>, <&bcm_reset 42>, <&pcie_rescal>;
> 	reset-names = "swinit", "bridge", "rescal";
> 
> pcie1 @ 110000:
> 	resets = <&bcm_reset 7>, <&bcm_reset 43>, <&pcie_rescal>;
> 	reset-names = "swinit", "bridge", "rescal";
> 
> pcie2 @ 120000:
> 	resets = <&bcm_reset 9>, <&bcm_reset 44>, <&pcie_rescal>;
> 	reset-names = "swinit", "bridge", "rescal"; >
> 
> I changed "swinit" reset for pcie2 to <&bcm_reset 9> (it is 32 in
> downstream rpi kernel) because otherwise I'm unable to enumerate RP1
> south bridge at all.

The value 9 is unused, so I suppose it does not really hurt to use it, 
but it is also unlikely to achieve what you desire. 32 is the correct 
value since pcie2_sw_init is bit 0 within SW_INIT_1 (second bank of resets).

The file link you provided appears to be lacking support for the 
"swinit" reset line, is that intentional? I don't think you can assume 
this will work without.
-- 
Florian

Re: [PATCH v6 05/13] PCI: brcmstb: Use bridge reset if available

[Stanimir Varbanov]

Hi Jim,

On 8/19/24 21:09, Jim Quinlan wrote:
> On Sat, Aug 17, 2024 at 1:41 PM Stanimir Varbanov <svarbanov@suse.de> wrote:
>>
>> Hi Jim,
>>
>> On 8/16/24 01:57, Jim Quinlan wrote:
>>> The 7712 SOC has a bridge reset which can be described in the device tree.
>>> Use it if present.  Otherwise, continue to use the legacy method to reset
>>> the bridge.
>>>
>>> Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com>
>>> ---
>>>  drivers/pci/controller/pcie-brcmstb.c | 24 +++++++++++++++++++-----
>>>  1 file changed, 19 insertions(+), 5 deletions(-)
>>
>> Reviewed-by: Stanimir Varbanov <svarbanov@suse.de>
>>
>> One problem though on RPi5 (bcm2712).
>>
>> With this series applied + my WIP patches for enablement of PCIe on
>> bcm2712 when enable the pcie1 and pcie2 root ports in dts, I see kernel
>> boot stuck on pcie2 enumeration and I have to add this [1] to make it
>> work again.
>>
>> Some more info about resets used:
>>
>> pcie0 @ 100000:
>>         resets = <&bcm_reset 5>, <&bcm_reset 42>, <&pcie_rescal>;
>>         reset-names = "swinit", "bridge", "rescal";
>>
>> pcie1 @ 110000:
>>         resets = <&bcm_reset 7>, <&bcm_reset 43>, <&pcie_rescal>;
>>         reset-names = "swinit", "bridge", "rescal";
>>
>> pcie2 @ 120000:
>>         resets = <&bcm_reset 9>, <&bcm_reset 44>, <&pcie_rescal>;
>>         reset-names = "swinit", "bridge", "rescal";
>>
>>
>> I changed "swinit" reset for pcie2 to <&bcm_reset 9> (it is 32 in
>> downstream rpi kernel) because otherwise I'm unable to enumerate RP1
>> south bridge at all.
>>
>> Any help will be appreciated.
> 
> Hi Stan,
> Let me look into this.  Why is one of the controllers turning off --
> is it not populated with a device?

Yes, I enabled pcie1 but no PCI endpoint devices attached on the
expansion connector.

> 
> As you probably know the 7712 only has access to PCIe1.  But we do
> have another chip with two controllers and I will try to reproduce
> your failure and get to the bottom of it.

Thank you for the help.

~Stan

> 
> Regards,
> Jim Quinlan
> Broadcom STB/CM
>>
>> ~Stan
>>
>> [1]
>> https://github.com/raspberrypi/linux/blob/rpi-6.11.y/drivers/pci/controller/pcie-brcmstb.c#L1711

Re: [PATCH v6 05/13] PCI: brcmstb: Use bridge reset if available

[Jim Quinlan]

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

On Mon, Aug 19, 2024 at 3:39 PM Stanimir Varbanov <svarbanov@suse.de> wrote:
>
> Hi Jim,
>
> On 8/19/24 21:09, Jim Quinlan wrote:
> > On Sat, Aug 17, 2024 at 1:41 PM Stanimir Varbanov <svarbanov@suse.de> wrote:
> >>
> >> Hi Jim,
> >>
> >> On 8/16/24 01:57, Jim Quinlan wrote:
> >>> The 7712 SOC has a bridge reset which can be described in the device tree.
> >>> Use it if present.  Otherwise, continue to use the legacy method to reset
> >>> the bridge.
> >>>
> >>> Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com>
> >>> ---
> >>>  drivers/pci/controller/pcie-brcmstb.c | 24 +++++++++++++++++++-----
> >>>  1 file changed, 19 insertions(+), 5 deletions(-)
> >>
> >> Reviewed-by: Stanimir Varbanov <svarbanov@suse.de>
> >>
> >> One problem though on RPi5 (bcm2712).
> >>
> >> With this series applied + my WIP patches for enablement of PCIe on
> >> bcm2712 when enable the pcie1 and pcie2 root ports in dts, I see kernel
> >> boot stuck on pcie2 enumeration and I have to add this [1] to make it
> >> work again.
> >>
> >> Some more info about resets used:
> >>
> >> pcie0 @ 100000:
> >>         resets = <&bcm_reset 5>, <&bcm_reset 42>, <&pcie_rescal>;
> >>         reset-names = "swinit", "bridge", "rescal";
> >>
> >> pcie1 @ 110000:
> >>         resets = <&bcm_reset 7>, <&bcm_reset 43>, <&pcie_rescal>;
> >>         reset-names = "swinit", "bridge", "rescal";
> >>
> >> pcie2 @ 120000:
> >>         resets = <&bcm_reset 9>, <&bcm_reset 44>, <&pcie_rescal>;
> >>         reset-names = "swinit", "bridge", "rescal";
> >>
> >>
> >> I changed "swinit" reset for pcie2 to <&bcm_reset 9> (it is 32 in
> >> downstream rpi kernel) because otherwise I'm unable to enumerate RP1
> >> south bridge at all.
> >>
> >> Any help will be appreciated.
> >
> > Hi Stan,
> > Let me look into this.  Why is one of the controllers turning off --
> > is it not populated with a device?
>
> Yes, I enabled pcie1 but no PCI endpoint devices attached on the
> expansion connector.

Hi Stan,

I looked at our similar STB chip that has a rescal controller and
multiple PCIe controllers and it doesn't have this problem.  Our 7712
doesn't  have this problem either, only because it only has one PCIe
controller.

However, using my 7712 and unbinding the device (invokes
brcm_pcie_remove()) shows me behavior similar to yours (2712).  What I
do is read the rescal registers after the unbind, and they will either
be dead or alive.  If I comment out the
"pcie->bridge_sw_init_set(pcie, 1);" call, the rescal is still dead
after unbind.  However if I comment out that AND the
"clk_disable_unprepare(pcie->clk);" call,  the rescal registers remain
alive after unbind.

Perhaps you don't see the dependence on the PCIe clocks if the 2712
does not give the PCIe node a clock property and instead keeps its
clocks on all of the time.  In that case I would think that your
solution would be fine.

Regards,
Jim Quinlan
Broadcom STB/CM



>
> >
> > As you probably know the 7712 only has access to PCIe1.  But we do
> > have another chip with two controllers and I will try to reproduce
> > your failure and get to the bottom of it.
>
> Thank you for the help.
>
> ~Stan
>
> >
> > Regards,
> > Jim Quinlan
> > Broadcom STB/CM
> >>
> >> ~Stan
> >>
> >> [1]
> >> https://github.com/raspberrypi/linux/blob/rpi-6.11.y/drivers/pci/controller/pcie-brcmstb.c#L1711

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

Re: [PATCH v6 05/13] PCI: brcmstb: Use bridge reset if available

[Stanimir Varbanov]

Hi Florian,

On 8/19/24 22:07, Florian Fainelli wrote:
> On 8/17/24 10:41, Stanimir Varbanov wrote:
>> Hi Jim,
>>
>> On 8/16/24 01:57, Jim Quinlan wrote:
>>> The 7712 SOC has a bridge reset which can be described in the device
>>> tree.
>>> Use it if present.  Otherwise, continue to use the legacy method to
>>> reset
>>> the bridge.
>>>
>>> Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com>
>>> ---
>>>   drivers/pci/controller/pcie-brcmstb.c | 24 +++++++++++++++++++-----
>>>   1 file changed, 19 insertions(+), 5 deletions(-)
>>
>> Reviewed-by: Stanimir Varbanov <svarbanov@suse.de>
>>
>> One problem though on RPi5 (bcm2712).
>>
>> With this series applied + my WIP patches for enablement of PCIe on
>> bcm2712 when enable the pcie1 and pcie2 root ports in dts, I see kernel
>> boot stuck on pcie2 enumeration and I have to add this [1] to make it
>> work again.
>>
>> Some more info about resets used:
>>
>> pcie0 @ 100000:
>>     resets = <&bcm_reset 5>, <&bcm_reset 42>, <&pcie_rescal>;
>>     reset-names = "swinit", "bridge", "rescal";
>>
>> pcie1 @ 110000:
>>     resets = <&bcm_reset 7>, <&bcm_reset 43>, <&pcie_rescal>;
>>     reset-names = "swinit", "bridge", "rescal";
>>
>> pcie2 @ 120000:
>>     resets = <&bcm_reset 9>, <&bcm_reset 44>, <&pcie_rescal>;
>>     reset-names = "swinit", "bridge", "rescal"; >
>>
>> I changed "swinit" reset for pcie2 to <&bcm_reset 9> (it is 32 in
>> downstream rpi kernel) because otherwise I'm unable to enumerate RP1
>> south bridge at all.
> 
> The value 9 is unused, so I suppose it does not really hurt to use it,
> but it is also unlikely to achieve what you desire. 32 is the correct
> value since pcie2_sw_init is bit 0 within SW_INIT_1 (second bank of
> resets).

Good to know that 9 is not the proper reset line, thank you.

Unfortunately, I'm unable to make it work with the proper reset line (32).

> 
> The file link you provided appears to be lacking support for the
> "swinit" reset line, is that intentional? I don't think you can assume

No idea why downstream RPi kernel does not use swinit reset.

> this will work without.

If I do not populate swinit in PCIe DT node it works i.e. PCI
enumeration is working and RP1 south-bridge is functional.

~Stan

Re: [PATCH v6 05/13] PCI: brcmstb: Use bridge reset if available

[Stanimir Varbanov]

Hi Jim,

On 8/20/24 00:49, Jim Quinlan wrote:
> On Mon, Aug 19, 2024 at 3:39 PM Stanimir Varbanov <svarbanov@suse.de> wrote:
>>
>> Hi Jim,
>>
>> On 8/19/24 21:09, Jim Quinlan wrote:
>>> On Sat, Aug 17, 2024 at 1:41 PM Stanimir Varbanov <svarbanov@suse.de> wrote:
>>>>
>>>> Hi Jim,
>>>>
>>>> On 8/16/24 01:57, Jim Quinlan wrote:
>>>>> The 7712 SOC has a bridge reset which can be described in the device tree.
>>>>> Use it if present.  Otherwise, continue to use the legacy method to reset
>>>>> the bridge.
>>>>>
>>>>> Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com>
>>>>> ---
>>>>>  drivers/pci/controller/pcie-brcmstb.c | 24 +++++++++++++++++++-----
>>>>>  1 file changed, 19 insertions(+), 5 deletions(-)
>>>>
>>>> Reviewed-by: Stanimir Varbanov <svarbanov@suse.de>
>>>>
>>>> One problem though on RPi5 (bcm2712).
>>>>
>>>> With this series applied + my WIP patches for enablement of PCIe on
>>>> bcm2712 when enable the pcie1 and pcie2 root ports in dts, I see kernel
>>>> boot stuck on pcie2 enumeration and I have to add this [1] to make it
>>>> work again.
>>>>
>>>> Some more info about resets used:
>>>>
>>>> pcie0 @ 100000:
>>>>         resets = <&bcm_reset 5>, <&bcm_reset 42>, <&pcie_rescal>;
>>>>         reset-names = "swinit", "bridge", "rescal";
>>>>
>>>> pcie1 @ 110000:
>>>>         resets = <&bcm_reset 7>, <&bcm_reset 43>, <&pcie_rescal>;
>>>>         reset-names = "swinit", "bridge", "rescal";
>>>>
>>>> pcie2 @ 120000:
>>>>         resets = <&bcm_reset 9>, <&bcm_reset 44>, <&pcie_rescal>;
>>>>         reset-names = "swinit", "bridge", "rescal";
>>>>
>>>>
>>>> I changed "swinit" reset for pcie2 to <&bcm_reset 9> (it is 32 in
>>>> downstream rpi kernel) because otherwise I'm unable to enumerate RP1
>>>> south bridge at all.
>>>>
>>>> Any help will be appreciated.
>>>
>>> Hi Stan,
>>> Let me look into this.  Why is one of the controllers turning off --
>>> is it not populated with a device?
>>
>> Yes, I enabled pcie1 but no PCI endpoint devices attached on the
>> expansion connector.
> 
> Hi Stan,
> 
> I looked at our similar STB chip that has a rescal controller and
> multiple PCIe controllers and it doesn't have this problem.  Our 7712
> doesn't  have this problem either, only because it only has one PCIe
> controller.
> 
> However, using my 7712 and unbinding the device (invokes
> brcm_pcie_remove()) shows me behavior similar to yours (2712).  What I
> do is read the rescal registers after the unbind, and they will either
> be dead or alive.  If I comment out the
> "pcie->bridge_sw_init_set(pcie, 1);" call, the rescal is still dead
> after unbind.  However if I comment out that AND the
> "clk_disable_unprepare(pcie->clk);" call,  the rescal registers remain
> alive after unbind.

Thank you. No idea why the clock is not used on 2712 (or at least it is
not populated on RPi downstream kernel.

> 
> Perhaps you don't see the dependence on the PCIe clocks if the 2712
> does not give the PCIe node a clock property and instead keeps its
> clocks on all of the time.  In that case I would think that your
> solution would be fine.

What you mean by my solution? The one where avoiding assert of
bridge_reset at link [1] bellow?

If so, I still cannot understand the relation between bridge_reset and
rescal as the comment mentions:

"Shutting down this bridge on pcie1 means accesses to rescal block will
hang the chip if another RC wants to assert/deassert rescal".

~Stan
> 
> Regards,
> Jim Quinlan
> Broadcom STB/CM
> 
> 
> 
>>
>>>
>>> As you probably know the 7712 only has access to PCIe1.  But we do
>>> have another chip with two controllers and I will try to reproduce
>>> your failure and get to the bottom of it.
>>
>> Thank you for the help.
>>
>> ~Stan
>>
>>>
>>> Regards,
>>> Jim Quinlan
>>> Broadcom STB/CM
>>>>
>>>> ~Stan
>>>>
>>>> [1]
>>>> https://github.com/raspberrypi/linux/blob/rpi-6.11.y/drivers/pci/controller/pcie-brcmstb.c#L1711

Re: [PATCH v6 05/13] PCI: brcmstb: Use bridge reset if available

[Jim Quinlan]

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

On Tue, Aug 20, 2024 at 7:38 PM Stanimir Varbanov <svarbanov@suse.de> wrote:
>
> Hi Florian,
>
> On 8/19/24 22:07, Florian Fainelli wrote:
> > On 8/17/24 10:41, Stanimir Varbanov wrote:
> >> Hi Jim,
> >>
> >> On 8/16/24 01:57, Jim Quinlan wrote:
> >>> The 7712 SOC has a bridge reset which can be described in the device
> >>> tree.
> >>> Use it if present.  Otherwise, continue to use the legacy method to
> >>> reset
> >>> the bridge.
> >>>
> >>> Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com>
> >>> ---
> >>>   drivers/pci/controller/pcie-brcmstb.c | 24 +++++++++++++++++++-----
> >>>   1 file changed, 19 insertions(+), 5 deletions(-)
> >>
> >> Reviewed-by: Stanimir Varbanov <svarbanov@suse.de>
> >>
> >> One problem though on RPi5 (bcm2712).
> >>
> >> With this series applied + my WIP patches for enablement of PCIe on
> >> bcm2712 when enable the pcie1 and pcie2 root ports in dts, I see kernel
> >> boot stuck on pcie2 enumeration and I have to add this [1] to make it
> >> work again.
> >>
> >> Some more info about resets used:
> >>
> >> pcie0 @ 100000:
> >>     resets = <&bcm_reset 5>, <&bcm_reset 42>, <&pcie_rescal>;
> >>     reset-names = "swinit", "bridge", "rescal";
> >>
> >> pcie1 @ 110000:
> >>     resets = <&bcm_reset 7>, <&bcm_reset 43>, <&pcie_rescal>;
> >>     reset-names = "swinit", "bridge", "rescal";
> >>
> >> pcie2 @ 120000:
> >>     resets = <&bcm_reset 9>, <&bcm_reset 44>, <&pcie_rescal>;
> >>     reset-names = "swinit", "bridge", "rescal"; >
> >>
> >> I changed "swinit" reset for pcie2 to <&bcm_reset 9> (it is 32 in
> >> downstream rpi kernel) because otherwise I'm unable to enumerate RP1
> >> south bridge at all.
> >
> > The value 9 is unused, so I suppose it does not really hurt to use it,
> > but it is also unlikely to achieve what you desire. 32 is the correct
> > value since pcie2_sw_init is bit 0 within SW_INIT_1 (second bank of
> > resets).
>
> Good to know that 9 is not the proper reset line, thank you.
>
> Unfortunately, I'm unable to make it work with the proper reset line (32).
>
> >
> > The file link you provided appears to be lacking support for the
> > "swinit" reset line, is that intentional? I don't think you can assume
>
> No idea why downstream RPi kernel does not use swinit reset.

I found out accidentally that one does not need this (swinit) to
properly function.
>
> > this will work without.
>
> If I do not populate swinit in PCIe DT node it works i.e. PCI
> enumeration is working and RP1 south-bridge is functional.
>
> ~Stan

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

Re: [PATCH v6 05/13] PCI: brcmstb: Use bridge reset if available

[Jim Quinlan]

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

On Tue, Aug 20, 2024 at 7:42 PM Stanimir Varbanov <svarbanov@suse.de> wrote:
>
> Hi Jim,
>
> On 8/20/24 00:49, Jim Quinlan wrote:
> > On Mon, Aug 19, 2024 at 3:39 PM Stanimir Varbanov <svarbanov@suse.de> wrote:
> >>
> >> Hi Jim,
> >>
> >> On 8/19/24 21:09, Jim Quinlan wrote:
> >>> On Sat, Aug 17, 2024 at 1:41 PM Stanimir Varbanov <svarbanov@suse.de> wrote:
> >>>>
> >>>> Hi Jim,
> >>>>
> >>>> On 8/16/24 01:57, Jim Quinlan wrote:
> >>>>> The 7712 SOC has a bridge reset which can be described in the device tree.
> >>>>> Use it if present.  Otherwise, continue to use the legacy method to reset
> >>>>> the bridge.
> >>>>>
> >>>>> Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com>
> >>>>> ---
> >>>>>  drivers/pci/controller/pcie-brcmstb.c | 24 +++++++++++++++++++-----
> >>>>>  1 file changed, 19 insertions(+), 5 deletions(-)
> >>>>
> >>>> Reviewed-by: Stanimir Varbanov <svarbanov@suse.de>
> >>>>
> >>>> One problem though on RPi5 (bcm2712).
> >>>>
> >>>> With this series applied + my WIP patches for enablement of PCIe on
> >>>> bcm2712 when enable the pcie1 and pcie2 root ports in dts, I see kernel
> >>>> boot stuck on pcie2 enumeration and I have to add this [1] to make it
> >>>> work again.
> >>>>
> >>>> Some more info about resets used:
> >>>>
> >>>> pcie0 @ 100000:
> >>>>         resets = <&bcm_reset 5>, <&bcm_reset 42>, <&pcie_rescal>;
> >>>>         reset-names = "swinit", "bridge", "rescal";
> >>>>
> >>>> pcie1 @ 110000:
> >>>>         resets = <&bcm_reset 7>, <&bcm_reset 43>, <&pcie_rescal>;
> >>>>         reset-names = "swinit", "bridge", "rescal";
> >>>>
> >>>> pcie2 @ 120000:
> >>>>         resets = <&bcm_reset 9>, <&bcm_reset 44>, <&pcie_rescal>;
> >>>>         reset-names = "swinit", "bridge", "rescal";
> >>>>
> >>>>
> >>>> I changed "swinit" reset for pcie2 to <&bcm_reset 9> (it is 32 in
> >>>> downstream rpi kernel) because otherwise I'm unable to enumerate RP1
> >>>> south bridge at all.
> >>>>
> >>>> Any help will be appreciated.
> >>>
> >>> Hi Stan,
> >>> Let me look into this.  Why is one of the controllers turning off --
> >>> is it not populated with a device?
> >>
> >> Yes, I enabled pcie1 but no PCI endpoint devices attached on the
> >> expansion connector.
> >
> > Hi Stan,
> >
> > I looked at our similar STB chip that has a rescal controller and
> > multiple PCIe controllers and it doesn't have this problem.  Our 7712
> > doesn't  have this problem either, only because it only has one PCIe
> > controller.
> >
> > However, using my 7712 and unbinding the device (invokes
> > brcm_pcie_remove()) shows me behavior similar to yours (2712).  What I
> > do is read the rescal registers after the unbind, and they will either
> > be dead or alive.  If I comment out the
> > "pcie->bridge_sw_init_set(pcie, 1);" call, the rescal is still dead
> > after unbind.  However if I comment out that AND the
> > "clk_disable_unprepare(pcie->clk);" call,  the rescal registers remain
> > alive after unbind.
>
> Thank you. No idea why the clock is not used on 2712 (or at least it is
> not populated on RPi downstream kernel.

Hi Stan,

Most of the clocks on the STB chips come up active so one does not
have to turn them on and off to have the device function.  It helps
power savings to do this although I'm not sure it is significant.
>
> >
> > Perhaps you don't see the dependence on the PCIe clocks if the 2712
> > does not give the PCIe node a clock property and instead keeps its
> > clocks on all of the time.  In that case I would think that your
> > solution would be fine.
>
> What you mean by my solution? The one where avoiding assert of
> bridge_reset at link [1] bellow?

Yes.
>
> If so, I still cannot understand the relation between bridge_reset and
> rescal as the comment mentions:
>
> "Shutting down this bridge on pcie1 means accesses to rescal block will
> hang the chip if another RC wants to assert/deassert rescal".

I was just describing my observations; this should not be happening.
I would say it is a HW bug for the 2712.  I can file a bug against the
2712 but that will not help us right now.  From what I was told by HW,
asserting the PCIe1 bridge reset does not affect the rescal settings,
but it does freeze access to the rescal registers, and that is game
over for the other PCIe controllers accessing the rescal registers.

Regards,
Jim Quinlan
Broadcom STB/CM

>
> ~Stan
> you
> > Regards,
> > Jim Quinlan
> > Broadcom STB/CM
> >
> >
> >
> >>
> >>>
> >>> As you probably know the 7712 only has access to PCIe1.  But we do
> >>> have another chip with two controllers and I will try to reproduce
> >>> your failure and get to the bottom of it.
> >>
> >> Thank you for the help.
> >>
> >> ~Stan
> >>
> >>>
> >>> Regards,
> >>> Jim Quinlan
> >>> Broadcom STB/CM
> >>>>
> >>>> ~Stan
> >>>>
> >>>> [1]
> >>>> https://github.com/raspberrypi/linux/blob/rpi-6.11.y/drivers/pci/controller/pcie-brcmstb.c#L1711

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

Re: [PATCH v6 05/13] PCI: brcmstb: Use bridge reset if available

[Stanimir Varbanov]

Hi Jim,

<cut>

> 
> Hi Stan,
> 
> Most of the clocks on the STB chips come up active so one does not
> have to turn them on and off to have the device function.  It helps
> power savings to do this although I'm not sure it is significant.
>>
>>>
>>> Perhaps you don't see the dependence on the PCIe clocks if the 2712
>>> does not give the PCIe node a clock property and instead keeps its
>>> clocks on all of the time.  In that case I would think that your
>>> solution would be fine.
>>
>> What you mean by my solution? The one where avoiding assert of
>> bridge_reset at link [1] bellow?
> 
> Yes.
>>
>> If so, I still cannot understand the relation between bridge_reset and
>> rescal as the comment mentions:
>>
>> "Shutting down this bridge on pcie1 means accesses to rescal block will
>> hang the chip if another RC wants to assert/deassert rescal".
> 
> I was just describing my observations; this should not be happening.
> I would say it is a HW bug for the 2712.  I can file a bug against the
> 2712 but that will not help us right now.  From what I was told by HW,
> asserting the PCIe1 bridge reset does not affect the rescal settings,
> but it does freeze access to the rescal registers, and that is game
> over for the other PCIe controllers accessing the rescal registers.

Good findings, thank you.

The problem comes from this snippet from brcm_pcie_probe() :

	ret = pci_host_probe(bridge);
	if (!ret && !brcm_pcie_link_up(pcie))
		ret = -ENODEV;

	if (ret) {
		brcm_pcie_remove(pdev);
		return ret;
	}

Even when pci_host_probe() is successful the .probe will fail if there
are no endpoint devices on this root port bus. This is the case when
probing pcie1 port which is the one with external connector. Cause the
probe is failing we call reset_control_rearm(rescal) from
brcm_pcie_remove(), after that during .probe of pcie2 (the root port
where RP1 south-bridge is attached) reset_control_reset(rescal) will
issue rescal reset thus rescal-reset driver will stuck on read/write
registers.

I think we have to drop this link-up check and allow the probe to finish
successfully. Even that there no PCI devices attached to bus we want the
root port to be visible by lspci tool. This will solve partially the
issue with accessing rescal reset-controller registers after asserting
bridge_reset. The other part of the problem will be solved by remove the
invocation of reset_control_rearm(rescal) from __brcm_pcie_remove().
That way only the first probed root port will issue rescal reset and
every next probe will not try to reset rescal because we do not call
_rearm(rescal).

What do you think?

~Stan

Re: [PATCH v6 05/13] PCI: brcmstb: Use bridge reset if available

[Jim Quinlan]

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

On Mon, Aug 26, 2024 at 6:43 AM Stanimir Varbanov <svarbanov@suse.de> wrote:
>
> Hi Jim,
>
> <cut>
>
> >
> > Hi Stan,
> >
> > Most of the clocks on the STB chips come up active so one does not
> > have to turn them on and off to have the device function.  It helps
> > power savings to do this although I'm not sure it is significant.
> >>
> >>>
> >>> Perhaps you don't see the dependence on the PCIe clocks if the 2712
> >>> does not give the PCIe node a clock property and instead keeps its
> >>> clocks on all of the time.  In that case I would think that your
> >>> solution would be fine.
> >>
> >> What you mean by my solution? The one where avoiding assert of
> >> bridge_reset at link [1] bellow?
> >
> > Yes.
> >>
> >> If so, I still cannot understand the relation between bridge_reset and
> >> rescal as the comment mentions:
> >>
> >> "Shutting down this bridge on pcie1 means accesses to rescal block will
> >> hang the chip if another RC wants to assert/deassert rescal".
> >
> > I was just describing my observations; this should not be happening.
> > I would say it is a HW bug for the 2712.  I can file a bug against the
> > 2712 but that will not help us right now.  From what I was told by HW,
> > asserting the PCIe1 bridge reset does not affect the rescal settings,
> > but it does freeze access to the rescal registers, and that is game
> > over for the other PCIe controllers accessing the rescal registers.
>
> Good findings, thank you.
>
> The problem comes from this snippet from brcm_pcie_probe() :
>
>         ret = pci_host_probe(bridge);
>         if (!ret && !brcm_pcie_link_up(pcie))
>                 ret = -ENODEV;
>
>         if (ret) {
>                 brcm_pcie_remove(pdev);
>                 return ret;
>         }
>
> Even when pci_host_probe() is successful the .probe will fail if there
> are no endpoint devices on this root port bus. This is the case when
> probing pcie1 port which is the one with external connector. Cause the
> probe is failing we call reset_control_rearm(rescal) from
> brcm_pcie_remove(), after that during .probe of pcie2 (the root port
> where RP1 south-bridge is attached) reset_control_reset(rescal) will
> issue rescal reset thus rescal-reset driver will stuck on read/write
> registers.
>
> I think we have to drop this link-up check and allow the probe to finish
> successfully. Even that there no PCI devices attached to bus we want the
> root port to be visible by lspci tool.

Hi Stan,

What is gained by having only the root bridge shown by lspci?  We do
not support PCIe hotplug so why have lspci reporting a bridge with no
devices?

The reason we do this is to save power -- when we see no device we
turn off the clocks, put things in reset (e.g. bridge), and turn off
the regulators.  We have SoCs with multiple controllers  and they
cannot afford to be supplying power to controllers with non-populated
sockets; these may be products that are trying to conform to mandated
energy-mode specifications.

 This will solve partially the
> issue with accessing rescal reset-controller registers after asserting
> bridge_reset. The other part of the problem will be solved by remove the
> invocation of reset_control_rearm(rescal) from __brcm_pcie_remove().
> That way only the first probed root port will issue rescal reset and
> every next probe will not try to reset rescal because we do not call
> _rearm(rescal).

In theory I agree with the above -- it should probably not be invoking
the rearm() when it is being deactivated.  However, I am concerned
about a controller(s) going into s2 suspend/resume or unbind/bind.

Let me run some tests.

Regards,
Jim Quinlan
Broadcom STB/CM


>
> What do you think?
>
> ~Stan

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

Re: [PATCH v6 05/13] PCI: brcmstb: Use bridge reset if available

[Stanimir Varbanov]

Hi Jim,

On 8/26/24 17:17, Jim Quinlan wrote:
> On Mon, Aug 26, 2024 at 6:43 AM Stanimir Varbanov <svarbanov@suse.de> wrote:
>>
>> Hi Jim,
>>
>> <cut>
>>
>>>
>>> Hi Stan,
>>>
>>> Most of the clocks on the STB chips come up active so one does not
>>> have to turn them on and off to have the device function.  It helps
>>> power savings to do this although I'm not sure it is significant.
>>>>
>>>>>
>>>>> Perhaps you don't see the dependence on the PCIe clocks if the 2712
>>>>> does not give the PCIe node a clock property and instead keeps its
>>>>> clocks on all of the time.  In that case I would think that your
>>>>> solution would be fine.
>>>>
>>>> What you mean by my solution? The one where avoiding assert of
>>>> bridge_reset at link [1] bellow?
>>>
>>> Yes.
>>>>
>>>> If so, I still cannot understand the relation between bridge_reset and
>>>> rescal as the comment mentions:
>>>>
>>>> "Shutting down this bridge on pcie1 means accesses to rescal block will
>>>> hang the chip if another RC wants to assert/deassert rescal".
>>>
>>> I was just describing my observations; this should not be happening.
>>> I would say it is a HW bug for the 2712.  I can file a bug against the
>>> 2712 but that will not help us right now.  From what I was told by HW,
>>> asserting the PCIe1 bridge reset does not affect the rescal settings,
>>> but it does freeze access to the rescal registers, and that is game
>>> over for the other PCIe controllers accessing the rescal registers.
>>
>> Good findings, thank you.
>>
>> The problem comes from this snippet from brcm_pcie_probe() :
>>
>>         ret = pci_host_probe(bridge);
>>         if (!ret && !brcm_pcie_link_up(pcie))
>>                 ret = -ENODEV;
>>
>>         if (ret) {
>>                 brcm_pcie_remove(pdev);
>>                 return ret;
>>         }
>>
>> Even when pci_host_probe() is successful the .probe will fail if there
>> are no endpoint devices on this root port bus. This is the case when
>> probing pcie1 port which is the one with external connector. Cause the
>> probe is failing we call reset_control_rearm(rescal) from
>> brcm_pcie_remove(), after that during .probe of pcie2 (the root port
>> where RP1 south-bridge is attached) reset_control_reset(rescal) will
>> issue rescal reset thus rescal-reset driver will stuck on read/write
>> registers.
>>
>> I think we have to drop this link-up check and allow the probe to finish
>> successfully. Even that there no PCI devices attached to bus we want the
>> root port to be visible by lspci tool.
> 
> Hi Stan,
> 
> What is gained by having only the root bridge shown by lspci?  We do
> not support PCIe hotplug so why have lspci reporting a bridge with no
> devices?
> 
> The reason we do this is to save power -- when we see no device we
> turn off the clocks, put things in reset (e.g. bridge), and turn off
> the regulators.  We have SoCs with multiple controllers  and they
> cannot afford to be supplying power to controllers with non-populated
> sockets; these may be products that are trying to conform to mandated
> energy-mode specifications.

I totally agree, although I do not see power consumption significantly
increased. Also I checked all other PCI controller drivers and no one
else doing this.

> 
>  This will solve partially the
>> issue with accessing rescal reset-controller registers after asserting
>> bridge_reset. The other part of the problem will be solved by remove the
>> invocation of reset_control_rearm(rescal) from __brcm_pcie_remove().
>> That way only the first probed root port will issue rescal reset and
>> every next probe will not try to reset rescal because we do not call
>> _rearm(rescal).
> 
> In theory I agree with the above -- it should probably not be invoking
> the rearm() when it is being deactivated.  However, I am concerned
> about a controller(s) going into s2 suspend/resume or unbind/bind.

In fact not calling rearm() from __brcm_pcie_remove() is not enough
because the .probe will fail thus rescal reset will loose it's instance
and next probed pcie root port will issue rescal reset again.

I'd stick with avoiding assert of bridge_reset from brcm_pcie_turn_off()
for now, and this will be true only for bcm2712.

~Stan

Re: [PATCH v6 05/13] PCI: brcmstb: Use bridge reset if available

[Jim Quinlan]

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

On Tue, Aug 27, 2024 at 8:28 AM Stanimir Varbanov <svarbanov@suse.de> wrote:
>
> Hi Jim,
>
> On 8/26/24 17:17, Jim Quinlan wrote:
> > On Mon, Aug 26, 2024 at 6:43 AM Stanimir Varbanov <svarbanov@suse.de> wrote:
> >>
> >> Hi Jim,
> >>
> >> <cut>
> >>
> >>>
> >>> Hi Stan,
> >>>
> >>> Most of the clocks on the STB chips come up active so one does not
> >>> have to turn them on and off to have the device function.  It helps
> >>> power savings to do this although I'm not sure it is significant.
> >>>>
> >>>>>
> >>>>> Perhaps you don't see the dependence on the PCIe clocks if the 2712
> >>>>> does not give the PCIe node a clock property and instead keeps its
> >>>>> clocks on all of the time.  In that case I would think that your
> >>>>> solution would be fine.
> >>>>
> >>>> What you mean by my solution? The one where avoiding assert of
> >>>> bridge_reset at link [1] bellow?
> >>>
> >>> Yes.
> >>>>
> >>>> If so, I still cannot understand the relation between bridge_reset and
> >>>> rescal as the comment mentions:
> >>>>
> >>>> "Shutting down this bridge on pcie1 means accesses to rescal block will
> >>>> hang the chip if another RC wants to assert/deassert rescal".
> >>>
> >>> I was just describing my observations; this should not be happening.
> >>> I would say it is a HW bug for the 2712.  I can file a bug against the
> >>> 2712 but that will not help us right now.  From what I was told by HW,
> >>> asserting the PCIe1 bridge reset does not affect the rescal settings,
> >>> but it does freeze access to the rescal registers, and that is game
> >>> over for the other PCIe controllers accessing the rescal registers.
> >>
> >> Good findings, thank you.
> >>
> >> The problem comes from this snippet from brcm_pcie_probe() :
> >>
> >>         ret = pci_host_probe(bridge);
> >>         if (!ret && !brcm_pcie_link_up(pcie))
> >>                 ret = -ENODEV;
> >>
> >>         if (ret) {
> >>                 brcm_pcie_remove(pdev);
> >>                 return ret;
> >>         }
> >>
> >> Even when pci_host_probe() is successful the .probe will fail if there
> >> are no endpoint devices on this root port bus. This is the case when
> >> probing pcie1 port which is the one with external connector. Cause the
> >> probe is failing we call reset_control_rearm(rescal) from
> >> brcm_pcie_remove(), after that during .probe of pcie2 (the root port
> >> where RP1 south-bridge is attached) reset_control_reset(rescal) will
> >> issue rescal reset thus rescal-reset driver will stuck on read/write
> >> registers.
> >>
> >> I think we have to drop this link-up check and allow the probe to finish
> >> successfully. Even that there no PCI devices attached to bus we want the
> >> root port to be visible by lspci tool.
> >
> > Hi Stan,
> >
> > What is gained by having only the root bridge shown by lspci?  We do
> > not support PCIe hotplug so why have lspci reporting a bridge with no
> > devices?
> >
> > The reason we do this is to save power -- when we see no device we
> > turn off the clocks, put things in reset (e.g. bridge), and turn off
> > the regulators.  We have SoCs with multiple controllers  and they
> > cannot afford to be supplying power to controllers with non-populated
> > sockets; these may be products that are trying to conform to mandated
> > energy-mode specifications.
>
> I totally agree, although I do not see power consumption significantly
> increased. Also I checked all other PCI controller drivers and no one
> else doing this.

Hi Stan,
I  see a few drivers use runtime_pm but we  do not; our architecture
is old and not amenable to that system.  Discounting runtime_pm, we
seem to be the only controller that uses the .suspend* or .resume*
methods.

I think we are kind of unique in how we turn off/on voltage regulators
for the endpoint device -- we put the regulator node under the port
driver DT node.  This is what customers asked for and what made
customers happy -- they just add a regulator node and ref to the DT we
provide and they don't have to worry about syncing power for their
device on their board.

There was also a recent patch submission that implemented something
similar by making the port driver customizable so that it could pick
up regulators; I don't recall if that ever got accepted.  But if it
was, we were thinking of switching to it.

Let me ask the HW folks if it is acceptable to leave the bridge reset
unasserted on remove() or suspend().  Note that if RPi decides to give
<clocks> and <clock-names>  valid props then problem will still be
there.

Regards,
Jim Quiinlan
Broad


>
> >
> >  This will solve partially the
> >> issue with accessing rescal reset-controller registers after asserting
> >> bridge_reset. The other part of the problem will be solved by remove the
> >> invocation of reset_control_rearm(rescal) from __brcm_pcie_remove().
> >> That way only the first probed root port will issue rescal reset and
> >> every next probe will not try to reset rescal because we do not call
> >> _rearm(rescal).
> >
> > In theory I agree with the above -- it should probably not be invoking
> > the rearm() when it is being deactivated.  However, I am concerned
> > about a controller(s) going into s2 suspend/resume or unbind/bind.
>
> In fact not calling rearm() from __brcm_pcie_remove() is not enough
> because the .probe will fail thus rescal reset will loose it's instance
> and next probed pcie root port will issue rescal reset again.
>
> I'd stick with avoiding assert of bridge_reset from brcm_pcie_turn_off()
> for now, and this will be true only for bcm2712.
>
> ~Stan

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

Re: [PATCH v6 00/13] PCI: brcnstb: Enable STB 7712 SOC

[Krzysztof Wilczyński]

Hello,

>   This submission is for the Broadcom STB 7712, sibling SOC of the RPi5 chip.
>   Stanimir has already submitted a patch "Add PCIe support for bcm2712" for
>   the RPi version of the SOC.  It is hoped that Stanimir will allow us to
>   submit this series first and subsequently rebase his patch(es).
> 
>   The largest commit, "Refactor for chips with many regular inbound BARs"
>   affects both the STB and RPi SOCs.  It allows for multiple inbound ranges
>   where previously only one was effectively used.  This feature will also
>   be present in future STB chips, as well as Broadcom's Cable Modem group.

Applied to controller/brcmstb, thank you!

[01/13] dt-bindings: PCI: brcm,stb-pcie: Change brcmstb maintainer and cleanup
        https://git.kernel.org/pci/pci/c/2cd86a7c2346

[02/13] dt-bindings: PCI: brcm,stb-pcie: Use maxItems for reset controllers
        https://git.kernel.org/pci/pci/c/9014c6b92fbe

[03/13] dt-bindings: PCI: brcm,stb-pcie: Add 7712 SoC description
        https://git.kernel.org/pci/pci/c/154051eae687

[04/13] PCI: brcmstb: Use common error handling code in brcm_pcie_probe()
        https://git.kernel.org/pci/pci/c/5ccf0ade7937

[05/13] PCI: brcmstb: Use bridge reset if available
        https://git.kernel.org/pci/pci/c/996a76b913ab

[06/13] PCI: brcmstb: Use swinit reset if available
        https://git.kernel.org/pci/pci/c/50fd71c2d2fb

[07/13] PCI: brcmstb: PCI: brcmstb: Make HARD_DEBUG, INTR2_CPU_BASE offsets SoC-specific
        https://git.kernel.org/pci/pci/c/e42c556de029

[08/13] PCI: brcmstb: Remove two unused constants from driver
        https://git.kernel.org/pci/pci/c/092001a4ebd0

[09/13] PCI: brcmstb: Don't conflate the reset rescal with PHY ctrl
        https://git.kernel.org/pci/pci/c/1bed07ffeccb

[10/13] PCI: brcmstb: Refactor for chips with many regular inbound windows
        https://git.kernel.org/pci/pci/c/22877c1ac638

[11/13] PCI: brcmstb: Check return value of all reset_control_* calls
        https://git.kernel.org/pci/pci/c/363051fc3ab8

[12/13] PCI: brcmstb: Change field name from 'type' to 'soc_base'
        https://git.kernel.org/pci/pci/c/e78bade02796

[13/13] PCI: brcmstb: Enable 7712 SOCs
        https://git.kernel.org/pci/pci/c/1ae791a877e7

	Krzysztof

Re: [PATCH v6 05/13] PCI: brcmstb: Use bridge reset if available

[Krzysztof Wilczyński]

Hello,

[...]
> Let me ask the HW folks if it is acceptable to leave the bridge reset
> unasserted on remove() or suspend().  Note that if RPi decides to give
> <clocks> and <clock-names>  valid props then problem will still be
> there.

I took this series, but if you have some concerns, especially given the
conversation here, then do let me know... We can always hold for now, and
get whatever changes you would need to be part of v7.

	Krzysztof

Re: [PATCH v6 05/13] PCI: brcmstb: Use bridge reset if available

[Bjorn Helgaas]

On Thu, Aug 15, 2024 at 06:57:18PM -0400, Jim Quinlan wrote:
> The 7712 SOC has a bridge reset which can be described in the device tree.
> Use it if present.  Otherwise, continue to use the legacy method to reset
> the bridge.

>  static void brcm_pcie_bridge_sw_init_set_generic(struct brcm_pcie *pcie, u32 val)
>  {
> -	u32 tmp, mask =  RGR1_SW_INIT_1_INIT_GENERIC_MASK;
> -	u32 shift = RGR1_SW_INIT_1_INIT_GENERIC_SHIFT;
> +	if (val)
> +		reset_control_assert(pcie->bridge_reset);
> +	else
> +		reset_control_deassert(pcie->bridge_reset);
>  
> -	tmp = readl(pcie->base + PCIE_RGR1_SW_INIT_1(pcie));
> -	tmp = (tmp & ~mask) | ((val << shift) & mask);
> -	writel(tmp, pcie->base + PCIE_RGR1_SW_INIT_1(pcie));
> +	if (!pcie->bridge_reset) {
> +		u32 tmp, mask =  RGR1_SW_INIT_1_INIT_GENERIC_MASK;
> +		u32 shift = RGR1_SW_INIT_1_INIT_GENERIC_SHIFT;
> +
> +		tmp = readl(pcie->base + PCIE_RGR1_SW_INIT_1(pcie));
> +		tmp = (tmp & ~mask) | ((val << shift) & mask);
> +		writel(tmp, pcie->base + PCIE_RGR1_SW_INIT_1(pcie));
> +	}

This pattern looks goofy:

  reset_control_assert(pcie->bridge_reset);
  if (!pcie->bridge_reset) {
    ...

If we're going to test pcie->bridge_reset at all, it should be first
so it's obvious what's going on and the reader doesn't have to go
verify that reset_control_assert() ignores and returns success for a
NULL pointer:

  if (pcie->bridge_reset) {
    if (val)
      reset_control_assert(pcie->bridge_reset);
    else
      reset_control_deassert(pcie->bridge_reset);

    return;
  }

  u32 tmp, mask =  RGR1_SW_INIT_1_INIT_GENERIC_MASK;
  ...

Krzysztof, can you amend this on the branch?

It will also make the eventual return checking and error message
simpler because we won't have to initialize "ret" first, and we can
"return 0" directly for the legacy case.

Bjorn

Re: [PATCH v6 07/13] PCI: brcmstb: PCI: brcmstb: Make HARD_DEBUG, INTR2_CPU_BASE offsets SoC-specific

[Bjorn Helgaas]

On Thu, Aug 15, 2024 at 06:57:20PM -0400, Jim Quinlan wrote:
> Do prepatory work for the 7712 SoC, which is introduced in a future commit.
> Our HW design has changed two register offsets for the 7712, where
> previously it was a common value for all Broadcom SOCs with PCIe cores.
> Specifically, the two offsets are to the registers HARD_DEBUG and
> INTR2_CPU_BASE.

> @@ -1499,12 +1502,16 @@ static const int pcie_offsets[] = {
>  	[RGR1_SW_INIT_1] = 0x9210,
>  	[EXT_CFG_INDEX]  = 0x9000,
>  	[EXT_CFG_DATA]   = 0x9004,
> +	[PCIE_HARD_DEBUG] = 0x4204,
> +	[PCIE_INTR2_CPU_BASE] = 0x4300,
>  };
>  
>  static const int pcie_offsets_bmips_7425[] = {
>  	[RGR1_SW_INIT_1] = 0x8010,
>  	[EXT_CFG_INDEX]  = 0x8300,
>  	[EXT_CFG_DATA]   = 0x8304,
> +	[PCIE_HARD_DEBUG] = 0x4204,
> +	[PCIE_INTR2_CPU_BASE] = 0x4300,
>  };
>  
>  static const struct pcie_cfg_data generic_cfg = {
> @@ -1539,6 +1546,8 @@ static const int pcie_offset_bcm7278[] = {
>  	[RGR1_SW_INIT_1] = 0xc010,
>  	[EXT_CFG_INDEX] = 0x9000,
>  	[EXT_CFG_DATA] = 0x9004,
> +	[PCIE_HARD_DEBUG] = 0x4204,
> +	[PCIE_INTR2_CPU_BASE] = 0x4300,
>  };

What's the organization scheme here?  We now have:

  static const int pcie_offsets[] = { ... };
  static const int pcie_offsets_bmips_7425[] = { ... };
  static const int pcie_offset_bcm7712[] = { ... };

  static const struct pcie_cfg_data generic_cfg = { ... };
  static const struct pcie_cfg_data bcm7425_cfg = { ... };
  static const struct pcie_cfg_data bcm7435_cfg = { ... };
  static const struct pcie_cfg_data bcm4908_cfg = { ... };

  static const int pcie_offset_bcm7278[] = { ... };

  static const struct pcie_cfg_data bcm7278_cfg = { ... };
  static const struct pcie_cfg_data bcm2711_cfg = { ... };
  static const struct pcie_cfg_data bcm7216_cfg = { ... };
  static const struct pcie_cfg_data bcm7712_cfg = { ... };

So we have pcie_offsets_bmips_7425[] and pcie_offset_bcm7712[] (with
gratuituously different "offset" vs "offsets") which are all together
before the pcie_cfg_data.

Then we have pcie_offset_bcm7278[] (again gratuitously different
"offset") separately, next to bcm7278_cfg.

It would be nice to pick one scheme and stick to it.

Also a seemingly random order of the pcie_cfg_data structs and
.compatible strings.

Also a little confusing to have "bmips_7425" and "bcm7425" associated
with the same chip.  I suppose there's historical reason for it, but I
don't think it's helpful in this usage.

>  static const struct pcie_cfg_data bcm7278_cfg = {
> -- 
> 2.17.1
> 

Re: [PATCH v6 10/13] PCI: brcmstb: Refactor for chips with many regular inbound windows

[Bjorn Helgaas]

On Thu, Aug 15, 2024 at 06:57:23PM -0400, Jim Quinlan wrote:
> Provide support for new chips with multiple inbound windows while
> keeping the legacy support for the older chips.
> 
> In existing chips there are three inbound windows with fixed purposes: the
> first was for mapping SoC internal registers, the second was for memory,
> and the third was for memory but with the endian swapped.  Typically, only
> one window was used.
> 
> Complicating the inbound window usage was the fact that the PCIe HW would
> do a baroque internal mapping of system memory, and concatenate the regions
> of multiple memory controllers.
> 
> Newer chips such as the 7712 and Cable Modem SOCs take a step forward and
> drop the internal mapping while providing for multiple inbound windows.
> This works in concert with the dma-ranges property, where each provided
> range becomes an inbound window.

Krzysztof, can you touch this up on your branch to s/SOCs/SoCs/ to
match other usage above, and ...

> +	 * The HW registers (and PCIe) use order-1 numbering for BARs. As
> +	 * such, we have inbound_wins[0] unused and BAR1 starts at inbound_wins[1].

Rewrap this to fit in 80 columns like (most of) the rest of the file
instead of 83?  I don't think we gain anything by being wider here.

Re: [PATCH v6 05/13] PCI: brcmstb: Use bridge reset if available

[Jim Quinlan]

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

On Mon, Sep 2, 2024 at 3:18 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
>
> On Thu, Aug 15, 2024 at 06:57:18PM -0400, Jim Quinlan wrote:
> > The 7712 SOC has a bridge reset which can be described in the device tree.
> > Use it if present.  Otherwise, continue to use the legacy method to reset
> > the bridge.
>
> >  static void brcm_pcie_bridge_sw_init_set_generic(struct brcm_pcie *pcie, u32 val)
> >  {
> > -     u32 tmp, mask =  RGR1_SW_INIT_1_INIT_GENERIC_MASK;
> > -     u32 shift = RGR1_SW_INIT_1_INIT_GENERIC_SHIFT;
> > +     if (val)
> > +             reset_control_assert(pcie->bridge_reset);
> > +     else
> > +             reset_control_deassert(pcie->bridge_reset);
> >
> > -     tmp = readl(pcie->base + PCIE_RGR1_SW_INIT_1(pcie));
> > -     tmp = (tmp & ~mask) | ((val << shift) & mask);
> > -     writel(tmp, pcie->base + PCIE_RGR1_SW_INIT_1(pcie));
> > +     if (!pcie->bridge_reset) {
> > +             u32 tmp, mask =  RGR1_SW_INIT_1_INIT_GENERIC_MASK;
> > +             u32 shift = RGR1_SW_INIT_1_INIT_GENERIC_SHIFT;
> > +
> > +             tmp = readl(pcie->base + PCIE_RGR1_SW_INIT_1(pcie));
> > +             tmp = (tmp & ~mask) | ((val << shift) & mask);
> > +             writel(tmp, pcie->base + PCIE_RGR1_SW_INIT_1(pcie));
> > +     }
>
> This pattern looks goofy:
>
>   reset_control_assert(pcie->bridge_reset);
>   if (!pcie->bridge_reset) {
>     ...
>
> If we're going to test pcie->bridge_reset at all, it should be first
> so it's obvious what's going on and the reader doesn't have to go
> verify that reset_control_assert() ignores and returns success for a
> NULL pointer:
>
>   if (pcie->bridge_reset) {
>     if (val)
>       reset_control_assert(pcie->bridge_reset);
>     else
>       reset_control_deassert(pcie->bridge_reset);
>
>     return;
>   }
>
>   u32 tmp, mask =  RGR1_SW_INIT_1_INIT_GENERIC_MASK;
>   ...
>
Will do.

Jim Quinlan
Broadcom STB/CM

> Krzysztof, can you amend this on the branch?
>
> It will also make the eventual return checking and error message
> simpler because we won't have to initialize "ret" first, and we can
> "return 0" directly for the legacy case.
>
> Bjorn

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

Re: [PATCH v6 05/13] PCI: brcmstb: Use bridge reset if available

[Krzysztof Wilczyński]

Hello,

[...]
> > >  static void brcm_pcie_bridge_sw_init_set_generic(struct brcm_pcie *pcie, u32 val)
> > >  {
> > > -     u32 tmp, mask =  RGR1_SW_INIT_1_INIT_GENERIC_MASK;
> > > -     u32 shift = RGR1_SW_INIT_1_INIT_GENERIC_SHIFT;
> > > +     if (val)
> > > +             reset_control_assert(pcie->bridge_reset);
> > > +     else
> > > +             reset_control_deassert(pcie->bridge_reset);
> > >
> > > -     tmp = readl(pcie->base + PCIE_RGR1_SW_INIT_1(pcie));
> > > -     tmp = (tmp & ~mask) | ((val << shift) & mask);
> > > -     writel(tmp, pcie->base + PCIE_RGR1_SW_INIT_1(pcie));
> > > +     if (!pcie->bridge_reset) {
> > > +             u32 tmp, mask =  RGR1_SW_INIT_1_INIT_GENERIC_MASK;
> > > +             u32 shift = RGR1_SW_INIT_1_INIT_GENERIC_SHIFT;
> > > +
> > > +             tmp = readl(pcie->base + PCIE_RGR1_SW_INIT_1(pcie));
> > > +             tmp = (tmp & ~mask) | ((val << shift) & mask);
> > > +             writel(tmp, pcie->base + PCIE_RGR1_SW_INIT_1(pcie));
> > > +     }
> >
> > This pattern looks goofy:
> >
> >   reset_control_assert(pcie->bridge_reset);
> >   if (!pcie->bridge_reset) {
> >     ...
> >
> > If we're going to test pcie->bridge_reset at all, it should be first
> > so it's obvious what's going on and the reader doesn't have to go
> > verify that reset_control_assert() ignores and returns success for a
> > NULL pointer:
> >
> >   if (pcie->bridge_reset) {
> >     if (val)
> >       reset_control_assert(pcie->bridge_reset);
> >     else
> >       reset_control_deassert(pcie->bridge_reset);
> >
> >     return;
> >   }
> >
> >   u32 tmp, mask =  RGR1_SW_INIT_1_INIT_GENERIC_MASK;
> >   ...
> >
> Will do.
[...]

You will do what?  If you don't mind me asking.

	Krzysztof

Re: [PATCH v6 05/13] PCI: brcmstb: Use bridge reset if available

[Bjorn Helgaas]

On Tue, Sep 03, 2024 at 11:46:13PM +0900, Krzysztof Wilczyński wrote:
> Hello,
> 
> [...]
> > > >  static void brcm_pcie_bridge_sw_init_set_generic(struct brcm_pcie *pcie, u32 val)
> > > >  {
> > > > -     u32 tmp, mask =  RGR1_SW_INIT_1_INIT_GENERIC_MASK;
> > > > -     u32 shift = RGR1_SW_INIT_1_INIT_GENERIC_SHIFT;
> > > > +     if (val)
> > > > +             reset_control_assert(pcie->bridge_reset);
> > > > +     else
> > > > +             reset_control_deassert(pcie->bridge_reset);
> > > >
> > > > -     tmp = readl(pcie->base + PCIE_RGR1_SW_INIT_1(pcie));
> > > > -     tmp = (tmp & ~mask) | ((val << shift) & mask);
> > > > -     writel(tmp, pcie->base + PCIE_RGR1_SW_INIT_1(pcie));
> > > > +     if (!pcie->bridge_reset) {
> > > > +             u32 tmp, mask =  RGR1_SW_INIT_1_INIT_GENERIC_MASK;
> > > > +             u32 shift = RGR1_SW_INIT_1_INIT_GENERIC_SHIFT;
> > > > +
> > > > +             tmp = readl(pcie->base + PCIE_RGR1_SW_INIT_1(pcie));
> > > > +             tmp = (tmp & ~mask) | ((val << shift) & mask);
> > > > +             writel(tmp, pcie->base + PCIE_RGR1_SW_INIT_1(pcie));
> > > > +     }
> > >
> > > This pattern looks goofy:
> > >
> > >   reset_control_assert(pcie->bridge_reset);
> > >   if (!pcie->bridge_reset) {
> > >     ...
> > >
> > > If we're going to test pcie->bridge_reset at all, it should be first
> > > so it's obvious what's going on and the reader doesn't have to go
> > > verify that reset_control_assert() ignores and returns success for a
> > > NULL pointer:
> > >
> > >   if (pcie->bridge_reset) {
> > >     if (val)
> > >       reset_control_assert(pcie->bridge_reset);
> > >     else
> > >       reset_control_deassert(pcie->bridge_reset);
> > >
> > >     return;
> > >   }
> > >
> > >   u32 tmp, mask =  RGR1_SW_INIT_1_INIT_GENERIC_MASK;
> > >   ...
> > >
> > Will do.
> [...]
> 
> You will do what?  If you don't mind me asking.

Can you just do the rework on the branch, Krzysztof?  I think that
will be easier/quicker than having Jim repost the entire series.

Bjorn

Re: [PATCH v6 05/13] PCI: brcmstb: Use bridge reset if available

[Krzysztof Wilczyński]

Hello,

[...]
> > > > >  static void brcm_pcie_bridge_sw_init_set_generic(struct brcm_pcie *pcie, u32 val)
> > > > >  {
> > > > > -     u32 tmp, mask =  RGR1_SW_INIT_1_INIT_GENERIC_MASK;
> > > > > -     u32 shift = RGR1_SW_INIT_1_INIT_GENERIC_SHIFT;
> > > > > +     if (val)
> > > > > +             reset_control_assert(pcie->bridge_reset);
> > > > > +     else
> > > > > +             reset_control_deassert(pcie->bridge_reset);
> > > > >
> > > > > -     tmp = readl(pcie->base + PCIE_RGR1_SW_INIT_1(pcie));
> > > > > -     tmp = (tmp & ~mask) | ((val << shift) & mask);
> > > > > -     writel(tmp, pcie->base + PCIE_RGR1_SW_INIT_1(pcie));
> > > > > +     if (!pcie->bridge_reset) {
> > > > > +             u32 tmp, mask =  RGR1_SW_INIT_1_INIT_GENERIC_MASK;
> > > > > +             u32 shift = RGR1_SW_INIT_1_INIT_GENERIC_SHIFT;
> > > > > +
> > > > > +             tmp = readl(pcie->base + PCIE_RGR1_SW_INIT_1(pcie));
> > > > > +             tmp = (tmp & ~mask) | ((val << shift) & mask);
> > > > > +             writel(tmp, pcie->base + PCIE_RGR1_SW_INIT_1(pcie));
> > > > > +     }
> > > >
> > > > This pattern looks goofy:
> > > >
> > > >   reset_control_assert(pcie->bridge_reset);
> > > >   if (!pcie->bridge_reset) {
> > > >     ...
> > > >
> > > > If we're going to test pcie->bridge_reset at all, it should be first
> > > > so it's obvious what's going on and the reader doesn't have to go
> > > > verify that reset_control_assert() ignores and returns success for a
> > > > NULL pointer:
> > > >
> > > >   if (pcie->bridge_reset) {
> > > >     if (val)
> > > >       reset_control_assert(pcie->bridge_reset);
> > > >     else
> > > >       reset_control_deassert(pcie->bridge_reset);
> > > >
> > > >     return;
> > > >   }
> > > >
> > > >   u32 tmp, mask =  RGR1_SW_INIT_1_INIT_GENERIC_MASK;
> > > >   ...
> > > >
> > > Will do.
> > [...]
> > 
> > You will do what?  If you don't mind me asking.
> 
> Can you just do the rework on the branch, Krzysztof?  I think that
> will be easier/quicker than having Jim repost the entire series.

Will do. That was the idea, I believe.

	Krzysztof

Re: [PATCH v6 07/13] PCI: brcmstb: PCI: brcmstb: Make HARD_DEBUG, INTR2_CPU_BASE offsets SoC-specific

[Florian Fainelli]

On 9/2/24 12:46, Bjorn Helgaas wrote:
> On Thu, Aug 15, 2024 at 06:57:20PM -0400, Jim Quinlan wrote:
>> Do prepatory work for the 7712 SoC, which is introduced in a future commit.
>> Our HW design has changed two register offsets for the 7712, where
>> previously it was a common value for all Broadcom SOCs with PCIe cores.
>> Specifically, the two offsets are to the registers HARD_DEBUG and
>> INTR2_CPU_BASE.
> 
>> @@ -1499,12 +1502,16 @@ static const int pcie_offsets[] = {
>>   	[RGR1_SW_INIT_1] = 0x9210,
>>   	[EXT_CFG_INDEX]  = 0x9000,
>>   	[EXT_CFG_DATA]   = 0x9004,
>> +	[PCIE_HARD_DEBUG] = 0x4204,
>> +	[PCIE_INTR2_CPU_BASE] = 0x4300,
>>   };
>>   
>>   static const int pcie_offsets_bmips_7425[] = {
>>   	[RGR1_SW_INIT_1] = 0x8010,
>>   	[EXT_CFG_INDEX]  = 0x8300,
>>   	[EXT_CFG_DATA]   = 0x8304,
>> +	[PCIE_HARD_DEBUG] = 0x4204,
>> +	[PCIE_INTR2_CPU_BASE] = 0x4300,
>>   };
>>   
>>   static const struct pcie_cfg_data generic_cfg = {
>> @@ -1539,6 +1546,8 @@ static const int pcie_offset_bcm7278[] = {
>>   	[RGR1_SW_INIT_1] = 0xc010,
>>   	[EXT_CFG_INDEX] = 0x9000,
>>   	[EXT_CFG_DATA] = 0x9004,
>> +	[PCIE_HARD_DEBUG] = 0x4204,
>> +	[PCIE_INTR2_CPU_BASE] = 0x4300,
>>   };
> 
> What's the organization scheme here?  We now have:
> 
>    static const int pcie_offsets[] = { ... };
>    static const int pcie_offsets_bmips_7425[] = { ... };
>    static const int pcie_offset_bcm7712[] = { ... };
> 
>    static const struct pcie_cfg_data generic_cfg = { ... };
>    static const struct pcie_cfg_data bcm7425_cfg = { ... };
>    static const struct pcie_cfg_data bcm7435_cfg = { ... };
>    static const struct pcie_cfg_data bcm4908_cfg = { ... };
> 
>    static const int pcie_offset_bcm7278[] = { ... };
> 
>    static const struct pcie_cfg_data bcm7278_cfg = { ... };
>    static const struct pcie_cfg_data bcm2711_cfg = { ... };
>    static const struct pcie_cfg_data bcm7216_cfg = { ... };
>    static const struct pcie_cfg_data bcm7712_cfg = { ... };
> 
> So we have pcie_offsets_bmips_7425[] and pcie_offset_bcm7712[] (with
> gratuituously different "offset" vs "offsets") which are all together
> before the pcie_cfg_data.
> 
> Then we have pcie_offset_bcm7278[] (again gratuitously different
> "offset") separately, next to bcm7278_cfg.
> 
> It would be nice to pick one scheme and stick to it.
> 
> Also a seemingly random order of the pcie_cfg_data structs and
> .compatible strings.
> 
> Also a little confusing to have "bmips_7425" and "bcm7425" associated
> with the same chip.  I suppose there's historical reason for it, but I
> don't think it's helpful in this usage.

All fair points, especially the lack of consistency, thanks for cleaning 
that up.
-- 
Florian

Re: [PATCH v6 05/13] PCI: brcmstb: Use bridge reset if available

[Jim Quinlan]

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

On Tue, Sep 3, 2024 at 10:26 AM Jim Quinlan <james.quinlan@broadcom.com> wrote:
>
> On Mon, Sep 2, 2024 at 3:18 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
> >
> > On Thu, Aug 15, 2024 at 06:57:18PM -0400, Jim Quinlan wrote:
> > > The 7712 SOC has a bridge reset which can be described in the device tree.
> > > Use it if present.  Otherwise, continue to use the legacy method to reset
> > > the bridge.
> >
> > >  static void brcm_pcie_bridge_sw_init_set_generic(struct brcm_pcie *pcie, u32 val)
> > >  {
> > > -     u32 tmp, mask =  RGR1_SW_INIT_1_INIT_GENERIC_MASK;
> > > -     u32 shift = RGR1_SW_INIT_1_INIT_GENERIC_SHIFT;
> > > +     if (val)
> > > +             reset_control_assert(pcie->bridge_reset);
> > > +     else
> > > +             reset_control_deassert(pcie->bridge_reset);
> > >
> > > -     tmp = readl(pcie->base + PCIE_RGR1_SW_INIT_1(pcie));
> > > -     tmp = (tmp & ~mask) | ((val << shift) & mask);
> > > -     writel(tmp, pcie->base + PCIE_RGR1_SW_INIT_1(pcie));
> > > +     if (!pcie->bridge_reset) {
> > > +             u32 tmp, mask =  RGR1_SW_INIT_1_INIT_GENERIC_MASK;
> > > +             u32 shift = RGR1_SW_INIT_1_INIT_GENERIC_SHIFT;
> > > +
> > > +             tmp = readl(pcie->base + PCIE_RGR1_SW_INIT_1(pcie));
> > > +             tmp = (tmp & ~mask) | ((val << shift) & mask);
> > > +             writel(tmp, pcie->base + PCIE_RGR1_SW_INIT_1(pcie));
> > > +     }
> >
> > This pattern looks goofy:
> >
> >   reset_control_assert(pcie->bridge_reset);
> >   if (!pcie->bridge_reset) {
> >     ...
> >
> > If we're going to test pcie->bridge_reset at all, it should be first
> > so it's obvious what's going on and the reader doesn't have to go
> > verify that reset_control_assert() ignores and returns success for a
> > NULL pointer:
> >
> >   if (pcie->bridge_reset) {
> >     if (val)
> >       reset_control_assert(pcie->bridge_reset);
> >     else
> >       reset_control_deassert(pcie->bridge_reset);
> >
> >     return;
> >   }
> >
> >   u32 tmp, mask =  RGR1_SW_INIT_1_INIT_GENERIC_MASK;
> >   ...
> >
> Will do.


Hi Bjorn,

It is not clear to me if you want a new series -- which would be V7 --
or you are okay with the current series V6.  If the latter, someone
sent in a fixup commit which must be included.
Please advise.

Jim Quinlan
Broadcom STB/CM

>
>
> Jim Quinlan
> Broadcom STB/CM
>
> > Krzysztof, can you amend this on the branch?
> >
> > It will also make the eventual return checking and error message
> > simpler because we won't have to initialize "ret" first, and we can
> > "return 0" directly for the legacy case.
> >
> > Bjorn

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

Re: [PATCH v6 05/13] PCI: brcmstb: Use bridge reset if available

[Bjorn Helgaas]

On Tue, Sep 10, 2024 at 01:30:41PM -0400, Jim Quinlan wrote:
> On Tue, Sep 3, 2024 at 10:26 AM Jim Quinlan <james.quinlan@broadcom.com> wrote:
> > On Mon, Sep 2, 2024 at 3:18 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
> > > On Thu, Aug 15, 2024 at 06:57:18PM -0400, Jim Quinlan wrote:
> > > > The 7712 SOC has a bridge reset which can be described in the device tree.
> > > > Use it if present.  Otherwise, continue to use the legacy method to reset
> > > > the bridge.
> > >
> > > >  static void brcm_pcie_bridge_sw_init_set_generic(struct brcm_pcie *pcie, u32 val)
> > > >  {
> > > > -     u32 tmp, mask =  RGR1_SW_INIT_1_INIT_GENERIC_MASK;
> > > > -     u32 shift = RGR1_SW_INIT_1_INIT_GENERIC_SHIFT;
> > > > +     if (val)
> > > > +             reset_control_assert(pcie->bridge_reset);
> > > > +     else
> > > > +             reset_control_deassert(pcie->bridge_reset);
> > > >
> > > > -     tmp = readl(pcie->base + PCIE_RGR1_SW_INIT_1(pcie));
> > > > -     tmp = (tmp & ~mask) | ((val << shift) & mask);
> > > > -     writel(tmp, pcie->base + PCIE_RGR1_SW_INIT_1(pcie));
> > > > +     if (!pcie->bridge_reset) {
> > > > +             u32 tmp, mask =  RGR1_SW_INIT_1_INIT_GENERIC_MASK;
> > > > +             u32 shift = RGR1_SW_INIT_1_INIT_GENERIC_SHIFT;
> > > > +
> > > > +             tmp = readl(pcie->base + PCIE_RGR1_SW_INIT_1(pcie));
> > > > +             tmp = (tmp & ~mask) | ((val << shift) & mask);
> > > > +             writel(tmp, pcie->base + PCIE_RGR1_SW_INIT_1(pcie));
> > > > +     }
> > >
> > > This pattern looks goofy:
> > >
> > >   reset_control_assert(pcie->bridge_reset);
> > >   if (!pcie->bridge_reset) {
> > >     ...
> > >
> > > If we're going to test pcie->bridge_reset at all, it should be first
> > > so it's obvious what's going on and the reader doesn't have to go
> > > verify that reset_control_assert() ignores and returns success for a
> > > NULL pointer:
> > >
> > >   if (pcie->bridge_reset) {
> > >     if (val)
> > >       reset_control_assert(pcie->bridge_reset);
> > >     else
> > >       reset_control_deassert(pcie->bridge_reset);
> > >
> > >     return;
> > >   }
> > >
> > >   u32 tmp, mask =  RGR1_SW_INIT_1_INIT_GENERIC_MASK;
> > >   ...
> > >
> > Will do.
> 
> Hi Bjorn,
> 
> It is not clear to me if you want a new series -- which would be V7 --
> or you are okay with the current series V6.  If the latter, someone
> sent in a fixup commit which must be included.
> Please advise.

Krzysztof amended this on the branch.  Take a look here and verify
that it makes sense to you:

  https://git.kernel.org/pub/scm/linux/kernel/git/pci/pci.git/tree/drivers/pci/controller/pcie-brcmstb.c?h=controller/brcmstb#n752

If that looks right to you, no need to post a new v7.

I think Krzysztof also integrated an "int num_inbound_wins" fix; is
that the one you mean?  If I'm thinking of the right one, you can
check that at:

  https://git.kernel.org/pub/scm/linux/kernel/git/pci/pci.git/tree/drivers/pci/controller/pcie-brcmstb.c?h=controller/brcmstb#n1034

> > > Krzysztof, can you amend this on the branch?
> > >
> > > It will also make the eventual return checking and error message
> > > simpler because we won't have to initialize "ret" first, and we can
> > > "return 0" directly for the legacy case.
> > >
> > > Bjorn

Re: [PATCH v6 05/13] PCI: brcmstb: Use bridge reset if available

[Krzysztof Wilczyński]

Hello,

[...]
> > > Will do.
> > 
> > It is not clear to me if you want a new series -- which would be V7 --
> > or you are okay with the current series V6.  If the latter, someone
> > sent in a fixup commit which must be included.
> > Please advise.

Apologies for the confusion.  I though it was obvious that I will go ahead
and fix the code on the branch directly.

> Krzysztof amended this on the branch.  Take a look here and verify
> that it makes sense to you:
> 
>   https://git.kernel.org/pub/scm/linux/kernel/git/pci/pci.git/tree/drivers/pci/controller/pcie-brcmstb.c?h=controller/brcmstb#n752
> 
> If that looks right to you, no need to post a new v7.
> 
> I think Krzysztof also integrated an "int num_inbound_wins" fix; is
> that the one you mean?  If I'm thinking of the right one, you can
> check that at:
> 
>   https://git.kernel.org/pub/scm/linux/kernel/git/pci/pci.git/tree/drivers/pci/controller/pcie-brcmstb.c?h=controller/brcmstb#n1034

Let me know if anything else needs doing.  But, if anything, then we need
to be quick about it.

	Krzysztof

Re: [PATCH v6 05/13] PCI: brcmstb: Use bridge reset if available

[Jim Quinlan]

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

On Tue, Sep 10, 2024 at 1:59 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
>
> On Tue, Sep 10, 2024 at 01:30:41PM -0400, Jim Quinlan wrote:
> > On Tue, Sep 3, 2024 at 10:26 AM Jim Quinlan <james.quinlan@broadcom.com> wrote:
> > > On Mon, Sep 2, 2024 at 3:18 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
> > > > On Thu, Aug 15, 2024 at 06:57:18PM -0400, Jim Quinlan wrote:
> > > > > The 7712 SOC has a bridge reset which can be described in the device tree.
> > > > > Use it if present.  Otherwise, continue to use the legacy method to reset
> > > > > the bridge.
> > > >
> > > > >  static void brcm_pcie_bridge_sw_init_set_generic(struct brcm_pcie *pcie, u32 val)
> > > > >  {
> > > > > -     u32 tmp, mask =  RGR1_SW_INIT_1_INIT_GENERIC_MASK;
> > > > > -     u32 shift = RGR1_SW_INIT_1_INIT_GENERIC_SHIFT;
> > > > > +     if (val)
> > > > > +             reset_control_assert(pcie->bridge_reset);
> > > > > +     else
> > > > > +             reset_control_deassert(pcie->bridge_reset);
> > > > >
> > > > > -     tmp = readl(pcie->base + PCIE_RGR1_SW_INIT_1(pcie));
> > > > > -     tmp = (tmp & ~mask) | ((val << shift) & mask);
> > > > > -     writel(tmp, pcie->base + PCIE_RGR1_SW_INIT_1(pcie));
> > > > > +     if (!pcie->bridge_reset) {
> > > > > +             u32 tmp, mask =  RGR1_SW_INIT_1_INIT_GENERIC_MASK;
> > > > > +             u32 shift = RGR1_SW_INIT_1_INIT_GENERIC_SHIFT;
> > > > > +
> > > > > +             tmp = readl(pcie->base + PCIE_RGR1_SW_INIT_1(pcie));
> > > > > +             tmp = (tmp & ~mask) | ((val << shift) & mask);
> > > > > +             writel(tmp, pcie->base + PCIE_RGR1_SW_INIT_1(pcie));
> > > > > +     }
> > > >
> > > > This pattern looks goofy:
> > > >
> > > >   reset_control_assert(pcie->bridge_reset);
> > > >   if (!pcie->bridge_reset) {
> > > >     ...
> > > >
> > > > If we're going to test pcie->bridge_reset at all, it should be first
> > > > so it's obvious what's going on and the reader doesn't have to go
> > > > verify that reset_control_assert() ignores and returns success for a
> > > > NULL pointer:
> > > >
> > > >   if (pcie->bridge_reset) {
> > > >     if (val)
> > > >       reset_control_assert(pcie->bridge_reset);
> > > >     else
> > > >       reset_control_deassert(pcie->bridge_reset);
> > > >
> > > >     return;
> > > >   }
> > > >
> > > >   u32 tmp, mask =  RGR1_SW_INIT_1_INIT_GENERIC_MASK;
> > > >   ...
> > > >
> > > Will do.
> >
> > Hi Bjorn,
> >
> > It is not clear to me if you want a new series -- which would be V7 --
> > or you are okay with the current series V6.  If the latter, someone
> > sent in a fixup commit which must be included.
> > Please advise.
>
> Krzysztof amended this on the branch.  Take a look here and verify
> that it makes sense to you:
>
>   https://git.kernel.org/pub/scm/linux/kernel/git/pci/pci.git/tree/drivers/pci/controller/pcie-brcmstb.c?h=controller/brcmstb#n752
>
> If that looks right to you, no need to post a new v7.
>
> I think Krzysztof also integrated an "int num_inbound_wins" fix; is
> that the one you mean?  If I'm thinking of the right one, you can
> check that at:
>
>   https://git.kernel.org/pub/scm/linux/kernel/git/pci/pci.git/tree/drivers/pci/controller/pcie-brcmstb.c?h=controller/brcmstb#n1034
>
> > > > Krzysztof, can you amend this on the branch?
> > > >
> > > > It will also make the eventual return checking and error message
> > > > simpler because we won't have to initialize "ret" first, and we can
> > > > "return 0" directly for the legacy case.
> > > >
> > > > Bjorn

Sorry, I didn't see this email until now.  The changes look good,
thanks for making them.

Regards,
Jim
>
>

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