[PATCH v4 00/12] PCI: brcnstb: Enable STB 7712 SOC

[PATCH v4 00/12] PCI: brcnstb: Enable STB 7712 SOC

[Jim Quinlan]

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

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 (12):
  dt-bindings: PCI: Cleanup of brcmstb YAML and add 7712 SoC
  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 BARs
  PCI: brcmstb: Check return value of all reset_control_xxx calls
  PCI: brcmstb: Change field name from 'type' to 'model'
  PCI: brcmstb: Enable 7712 SOCs

 .../bindings/pci/brcm,stb-pcie.yaml           |  50 +-
 drivers/pci/controller/pcie-brcmstb.c         | 485 +++++++++++++-----
 2 files changed, 400 insertions(+), 135 deletions(-)


base-commit: 55027e689933ba2e64f3d245fb1ff185b3e7fc81
-- 
2.17.1


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

[PATCH v4 01/12] dt-bindings: PCI: Cleanup of brcmstb YAML and add 7712 SoC

[Jim Quinlan]

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

o Change order of the compatible strings to be alphabetical

o Describe resets/reset-names before using them in rules

o Add minItems/maxItems where needed.

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

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

diff --git a/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml b/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
index 11f8ea33240c..692f7ed7c98e 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
@@ -95,6 +95,18 @@ properties:
       minItems: 1
       maxItems: 3
 
+  resets:
+    minItems: 1
+    items:
+      - description: reset for external PCIe PERST# signal # perst
+      - description: reset for phy reset calibration       # rescal
+
+  reset-names:
+    minItems: 1
+    items:
+      - const: perst
+      - const: rescal
+
 required:
   - compatible
   - reg
@@ -118,8 +130,8 @@ allOf:
     then:
       properties:
         resets:
-          items:
-            - description: reset controller handling the PERST# signal
+          minItems: 1
+          maxItems: 1
 
         reset-names:
           items:
@@ -136,8 +148,8 @@ allOf:
     then:
       properties:
         resets:
-          items:
-            - description: phandle pointing to the RESCAL reset controller
+          minItems: 1
+          maxItems: 1
 
         reset-names:
           items:
-- 
2.17.1


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

[PATCH v4 02/12] dt-bindings: PCI: brcmstb: Add 7712 SoC description

[Jim Quinlan]

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

This adds the description for the 7712 SoC, a Broadcom
STB sibling chip of the RPi 5.  Two new reset controllers
are described.

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

diff --git a/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml b/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
index 692f7ed7c98e..90683a0df2c5 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
@@ -100,12 +101,16 @@ properties:
     items:
       - description: reset for external PCIe PERST# signal # perst
       - description: reset for phy reset calibration       # rescal
+      - description: reset for PCIe/CPU bus bridge         # bridge
+      - description: reset for soft PCIe core reset        # swinit
 
   reset-names:
     minItems: 1
     items:
       - const: perst
       - const: rescal
+      - const: bridge
+      - const: swinit
 
 required:
   - compatible
@@ -159,6 +164,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


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

[PATCH v4 03/12] PCI: brcmstb: Use common error handling code in brcm_pcie_probe()

[Jim Quinlan]

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

o Move the clk_prepare_enable() below the resource allocations.
o Add a jump target (clk_out) so that a bit of exception handling can be
  better reused at the end of this function implementation.

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 | 29 +++++++++++++++------------
 1 file changed, 16 insertions(+), 13 deletions(-)

diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
index c08683febdd4..c257434edc08 100644
--- a/drivers/pci/controller/pcie-brcmstb.c
+++ b/drivers/pci/controller/pcie-brcmstb.c
@@ -1613,31 +1613,30 @@ 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 = clk_prepare_enable(pcie->clk);
+	if (ret) {
+		dev_err(&pdev->dev, "could not enable clock\n");
+		return ret;
 	}
 
 	ret = reset_control_reset(pcie->rescal);
-	if (ret)
+	if (ret) {
 		dev_err(&pdev->dev, "failed to deassert 'rescal'\n");
+		goto clk_out;
+	}
 
 	ret = brcm_phy_start(pcie);
 	if (ret) {
 		reset_control_rearm(pcie->rescal);
-		clk_disable_unprepare(pcie->clk);
-		return ret;
+		goto clk_out;
 	}
 
 	ret = brcm_pcie_setup(pcie);
@@ -1676,6 +1675,10 @@ static int brcm_pcie_probe(struct platform_device *pdev)
 
 	return 0;
 
+clk_out:
+	clk_disable_unprepare(pcie->clk);
+	return ret;
+
 fail:
 	__brcm_pcie_remove(pcie);
 	return ret;
-- 
2.17.1


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

[PATCH v4 04/12] PCI: brcmstb: Use bridge reset if available

[Jim Quinlan]

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

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

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 | 22 +++++++++++++++++-----
 1 file changed, 17 insertions(+), 5 deletions(-)

diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
index c257434edc08..92816d8d215a 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;
 	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 (pcie->bridge) {
+		if (val)
+			reset_control_assert(pcie->bridge);
+		else
+			reset_control_deassert(pcie->bridge);
+	} else {
+		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));
+		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,6 +1629,10 @@ static int brcm_pcie_probe(struct platform_device *pdev)
 	if (IS_ERR(pcie->perst_reset))
 		return PTR_ERR(pcie->perst_reset);
 
+	pcie->bridge = devm_reset_control_get_optional_exclusive(&pdev->dev, "bridge");
+	if (IS_ERR(pcie->bridge))
+		return PTR_ERR(pcie->bridge);
+
 	ret = clk_prepare_enable(pcie->clk);
 	if (ret) {
 		dev_err(&pdev->dev, "could not enable clock\n");
-- 
2.17.1


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

[PATCH v4 05/12] PCI: brcmstb: Use swinit reset if available

[Jim Quinlan]

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

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: Stanimir Varbanov <svarbanov@suse.de>
Reviewed-by: Florian Fainelli <florian.fainelli@broadcom.com>
---
 drivers/pci/controller/pcie-brcmstb.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
index 92816d8d215a..4dc2ff7f3167 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;
+	struct reset_control	*swinit;
 	int			num_memc;
 	u64			memc_size[PCIE_BRCM_MAX_MEMC];
 	u32			hw_rev;
@@ -1633,12 +1634,27 @@ static int brcm_pcie_probe(struct platform_device *pdev)
 	if (IS_ERR(pcie->bridge))
 		return PTR_ERR(pcie->bridge);
 
+	pcie->swinit = devm_reset_control_get_optional_exclusive(&pdev->dev, "swinit");
+	if (IS_ERR(pcie->swinit))
+		return PTR_ERR(pcie->swinit);
+
 	ret = clk_prepare_enable(pcie->clk);
 	if (ret) {
 		dev_err(&pdev->dev, "could not enable clock\n");
 		return ret;
 	}
 
+	ret = reset_control_assert(pcie->swinit);
+	if (ret) {
+		dev_err_probe(&pdev->dev, ret, "could not assert reset 'swinit'\n");
+		goto clk_out;
+	}
+	ret = reset_control_deassert(pcie->swinit);
+	if (ret) {
+		dev_err(&pdev->dev, "could not de-assert reset 'swinit' after asserting\n");
+		goto clk_out;
+	}
+
 	ret = reset_control_reset(pcie->rescal);
 	if (ret) {
 		dev_err(&pdev->dev, "failed to deassert 'rescal'\n");
-- 
2.17.1


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

[PATCH v4 06/12] PCI: brcmstb: PCI: brcmstb: Make HARD_DEBUG, INTR2_CPU_BASE offsets SoC-specific

[Jim Quinlan]

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

Our HW design has again changed a register offset which used to be standard
for all Broadcom SOCs with PCIe cores.  This difference is now reconciled
for 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>
---
 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 4dc2ff7f3167..073d790d97b7 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


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

[PATCH v4 07/12] PCI: brcmstb: Remove two unused constants from driver

[Jim Quinlan]

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

Two constants in the driver, RGR1_SW_INIT_1_INIT_MASK and
RGR1_SW_INIT_1_INIT_SHIFT are no longer used and are removed.

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 | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
index 073d790d97b7..dfb404748ad8 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


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

[PATCH v4 08/12] PCI: brcmstb: Don't conflate the reset rescal with phy ctrl

[Jim Quinlan]

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

We've been assuming that if an SOC has a "rescal" reset controller that we
should automatically invoke brcm_phy_cntl(...).  This will not be true in
future SOCs, so we create a bool "has_phy" and adjust the cfg_data
appropriately (we need to give 7216 its own cfg_data structure instead of
sharing one).

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 | 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 dfb404748ad8..8ab5a8ca05b4 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


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

[PATCH v4 09/12] PCI: brcmstb: Refactor for chips with many regular inbound BARs

[Jim Quinlan]

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

Previously, our chips provided three inbound "BARS" 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.  We
typically only used one of these BARs.

Complicating that BARs 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 have taken a step forward
and now provide multiple inbound BARs.  This works in concert with the
dma-ranges property, where each provided range becomes an inbound BAR.

This commit provides support for these new chips and their multiple
inbound BARs but also keeps the legacy support for the older system.

Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com>
Reviewed-by: Stanimir Varbanov <svarbanov@suse.de>
---
 drivers/pci/controller/pcie-brcmstb.c | 216 ++++++++++++++++++++------
 1 file changed, 167 insertions(+), 49 deletions(-)

diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
index 8ab5a8ca05b4..c44a92217855 100644
--- a/drivers/pci/controller/pcie-brcmstb.c
+++ b/drivers/pci/controller/pcie-brcmstb.c
@@ -75,15 +75,12 @@
 #define PCIE_MEM_WIN0_HI(win)	\
 		PCIE_MISC_CPU_2_PCIE_MEM_WIN0_HI + ((win) * 8)
 
+#define PCIE_BRCM_MAX_RC_BARS				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 +127,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 +218,20 @@ enum pcie_type {
 	BCM4908,
 	BCM7278,
 	BCM2711,
+	BCM7712,
+};
+
+struct rc_bar {
+	u64 size;
+	u64 pci_offset;
+	u64 cpu_addr;
 };
 
 struct pcie_cfg_data {
 	const int *offsets;
 	const enum pcie_type type;
 	const bool has_phy;
+	unsigned int num_inbound;
 	void (*perst_set)(struct brcm_pcie *pcie, u32 val);
 	void (*bridge_sw_init_set)(struct brcm_pcie *pcie, u32 val);
 };
@@ -274,6 +283,7 @@ struct brcm_pcie {
 	struct subdev_regulators *sr;
 	bool			ep_wakeup_capable;
 	bool			has_phy;
+	int			num_inbound;
 };
 
 static inline bool is_bmips(const struct brcm_pcie *pcie)
@@ -789,23 +799,61 @@ 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 inline void set_bar(struct rc_bar *b, int *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 rc_bar rc_bars[])
 {
 	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;
+	int ret, i = 0, n = 0;
+
+	/*
+	 * The HW registers (and PCIe) use order-1 numbering for BARs. As
+	 * such, we have rc_bars[0] unused and BAR1 starts at rc_bars[1].
+	 */
+	struct rc_bar *b_begin = &rc_bars[1];
+	struct rc_bar *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)
+		set_bar(b++, &n, 0, 0, 0);
 
 	resource_list_for_each_entry(entry, &bridge->dma_ranges) {
 		u64 pcie_beg = entry->res->start - entry->offset;
+		u64 cpu_beg = entry->res->start;
 
-		size += entry->res->end - entry->res->start + 1;
+		size = resource_size(entry->res);
+		tot_size += size;
 		if (pcie_beg < lowest_pcie_addr)
 			lowest_pcie_addr = pcie_beg;
+		/*
+		 * 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)
+			set_bar(b++, &n, size, cpu_beg, pcie_beg);
+
+		if (n > pcie->num_inbound)
+			break;
 	}
 
 	if (lowest_pcie_addr == ~(u64)0) {
@@ -813,13 +861,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 +883,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 +926,89 @@ 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)) {
+	if (!size || (pci_offset & (size - 1)) ||
+	    (pci_offset < SZ_4G && pci_offset > SZ_2G)) {
 		dev_err(dev, "Invalid rc_bar2_offset/size: size 0x%llx, off 0x%llx\n",
-			*rc_bar2_size, *rc_bar2_offset);
+			size, pci_offset);
 		return -EINVAL;
 	}
 
-	return 0;
+	/* Enable BAR2, the inbound window for STB chips */
+	set_bar(b++, &n, size, cpu_addr, pci_offset);
+
+	/*
+	 * Disable BAR3.  On some chips presents the same window as BAR2
+	 * but the data appears in a settable endianness.
+	 */
+	set_bar(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 rc_bar *rc_bars,
+				      int num_rc_bars)
+{
+	void __iomem *base = pcie->base;
+	int i;
+
+	for (i = 1; i <= num_rc_bars; i++) {
+		u64 pci_offset = rc_bars[i].pci_offset;
+		u64 cpu_addr = rc_bars[i].cpu_addr;
+		u64 size = rc_bars[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(tmp, base + reg_offset);
+		/* Write high */
+		writel(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(tmp, base + reg_offset);
+			tmp = upper_32_bits(cpu_addr);
+			writel(tmp, base + reg_offset + 4);
+		}
+	}
 }
 
 static int brcm_pcie_setup(struct brcm_pcie *pcie)
 {
-	u64 rc_bar2_offset, rc_bar2_size;
+	struct rc_bar rc_bars[PCIE_BRCM_MAX_RC_BARS];
 	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;
+	int num_out_wins = 0, num_rc_bars = 0;
+	int memc;
 
 	/* Reset the bridge */
 	pcie->bridge_sw_init_set(pcie, 1);
@@ -933,17 +1057,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_rc_bars = brcm_pcie_get_inbound_wins(pcie, rc_bars);
+	if (num_rc_bars < 0)
+		return num_rc_bars;
 
-	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);
+	set_inbound_win_registers(pcie, rc_bars, num_rc_bars);
+
+	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 +1088,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 (rc_bars[2].pci_offset >= SZ_4G ||
+	    (rc_bars[2].size + rc_bars[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;
@@ -1516,6 +1626,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	= 3,
 };
 
 static const struct pcie_cfg_data bcm7425_cfg = {
@@ -1523,6 +1634,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	= 3,
 };
 
 static const struct pcie_cfg_data bcm7435_cfg = {
@@ -1530,6 +1642,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	= 3,
 };
 
 static const struct pcie_cfg_data bcm4908_cfg = {
@@ -1537,6 +1650,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	= 3,
 };
 
 static const int pcie_offset_bcm7278[] = {
@@ -1552,6 +1666,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	= 3,
 };
 
 static const struct pcie_cfg_data bcm2711_cfg = {
@@ -1559,6 +1674,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	= 3,
 };
 
 static const struct pcie_cfg_data bcm7216_cfg = {
@@ -1567,6 +1683,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	= 3,
 };
 
 static const struct of_device_id brcm_pcie_match[] = {
@@ -1623,6 +1740,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 = data->num_inbound;
 
 	pcie->base = devm_platform_ioremap_resource(pdev, 0);
 	if (IS_ERR(pcie->base))
-- 
2.17.1


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

[PATCH v4 10/12] PCI: brcmstb: Check return value of all reset_control_xxx calls

[Jim Quinlan]

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

In some cases the result of a reset_control_xxx() call have been ignored.
Now we check all return values of such functions and propagate the error to
the next level.

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 | 101 ++++++++++++++++++--------
 1 file changed, 72 insertions(+), 29 deletions(-)

diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
index c44a92217855..2fe1f2a26697 100644
--- a/drivers/pci/controller/pcie-brcmstb.c
+++ b/drivers/pci/controller/pcie-brcmstb.c
@@ -232,8 +232,8 @@ struct pcie_cfg_data {
 	const enum pcie_type type;
 	const bool has_phy;
 	unsigned int num_inbound;
-	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 {
@@ -278,8 +278,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;
@@ -742,13 +742,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 (pcie->bridge) {
 		if (val)
-			reset_control_assert(pcie->bridge);
+			ret = reset_control_assert(pcie->bridge);
 		else
-			reset_control_deassert(pcie->bridge);
+			ret = reset_control_deassert(pcie->bridge);
+		if (ret)
+			dev_err(pcie->dev, "failed to %s 'bridge' reset, err=%d\n",
+				val ? "assert" : "deassert", ret);
 	} else {
 		u32 tmp, mask =  RGR1_SW_INIT_1_INIT_GENERIC_MASK;
 		u32 shift = RGR1_SW_INIT_1_INIT_GENERIC_SHIFT;
@@ -757,9 +762,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;
@@ -767,20 +774,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;
 
@@ -788,15 +804,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 inline void set_bar(struct rc_bar *b, int *count, u64 size,
@@ -1008,19 +1028,28 @@ static int brcm_pcie_setup(struct brcm_pcie *pcie)
 	struct resource_entry *entry;
 	u32 tmp, burst, aspm_support;
 	int num_out_wins = 0, num_rc_bars = 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))
@@ -1239,7 +1268,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
@@ -1431,15 +1462,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);
@@ -1452,7 +1485,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)
@@ -1470,9 +1505,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
@@ -1501,7 +1539,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;
 			}
 		}
@@ -1516,7 +1557,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);
@@ -1578,7 +1619,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


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

[PATCH v4 11/12] PCI: brcmstb: Change field name from 'type' to 'model'

[Jim Quinlan]

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

The 'type' field used in the driver to discern SoC differences is confusing
so change it to the more apt 'model'.  We considered using 'family' but
this conflicts with Broadcom's conception of a family; for example, 7216a0
and 7216b0 chips are both considered separate families as each has multiple
derivative product chips based on the original design.

Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com>
Reviewed-by: Florian Fainelli <florian.fainelli@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 2fe1f2a26697..fa5616a56383 100644
--- a/drivers/pci/controller/pcie-brcmstb.c
+++ b/drivers/pci/controller/pcie-brcmstb.c
@@ -211,7 +211,7 @@ enum {
 	PCIE_INTR2_CPU_BASE,
 };
 
-enum pcie_type {
+enum pcie_model {
 	GENERIC,
 	BCM7425,
 	BCM7435,
@@ -229,7 +229,7 @@ struct rc_bar {
 
 struct pcie_cfg_data {
 	const int *offsets;
-	const enum pcie_type type;
+	const enum pcie_model model;
 	const bool has_phy;
 	unsigned int num_inbound;
 	int (*perst_set)(struct brcm_pcie *pcie, u32 val);
@@ -270,7 +270,7 @@ struct brcm_pcie {
 	u64			msi_target_addr;
 	struct brcm_msi		*msi;
 	const int		*reg_offsets;
-	enum pcie_type		type;
+	enum pcie_model		model;
 	struct reset_control	*rescal;
 	struct reset_control	*perst_reset;
 	struct reset_control	*bridge;
@@ -288,7 +288,7 @@ struct brcm_pcie {
 
 static inline bool is_bmips(const struct brcm_pcie *pcie)
 {
-	return pcie->type == BCM7435 || pcie->type == BCM7425;
+	return pcie->model == BCM7435 || pcie->model == BCM7425;
 }
 
 /*
@@ -852,7 +852,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->model != BCM7712)
 		set_bar(b++, &n, 0, 0, 0);
 
 	resource_list_for_each_entry(entry, &bridge->dma_ranges) {
@@ -869,7 +869,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->model == BCM7712)
 			set_bar(b++, &n, size, cpu_beg, pcie_beg);
 
 		if (n > pcie->num_inbound)
@@ -886,7 +886,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->model == BCM7712)
 		return n;
 
 	ret = of_property_read_variable_u64_array(pcie->np, "brcm,scb-sizes", pcie->memc_size, 1,
@@ -1008,7 +1008,7 @@ static void set_inbound_win_registers(struct brcm_pcie *pcie, const struct rc_ba
 		 * 7712:
 		 *     All of their BARs need to be set.
 		 */
-		if (pcie->type == BCM7712) {
+		if (pcie->model == BCM7712) {
 			/* BUS remap register settings */
 			reg_offset = brcm_ubus_reg_offset(i);
 			tmp = lower_32_bits(cpu_addr) & ~0xfff;
@@ -1036,7 +1036,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->model == BCM2711) {
 		ret = pcie->perst_set(pcie, 1);
 		if (ret) {
 			pcie->bridge_sw_init_set(pcie, 0);
@@ -1067,9 +1067,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->model == BCM2711)
 		burst = 0x0; /* 128 bytes */
-	else if (pcie->type == BCM7278)
+	else if (pcie->model == BCM7278)
 		burst = 0x3; /* 512 bytes */
 	else
 		burst = 0x2; /* 512 bytes */
@@ -1666,7 +1666,7 @@ static const int pcie_offsets_bmips_7425[] = {
 
 static const struct pcie_cfg_data generic_cfg = {
 	.offsets	= pcie_offsets,
-	.type		= GENERIC,
+	.model		= GENERIC,
 	.perst_set	= brcm_pcie_perst_set_generic,
 	.bridge_sw_init_set = brcm_pcie_bridge_sw_init_set_generic,
 	.num_inbound	= 3,
@@ -1674,7 +1674,7 @@ static const struct pcie_cfg_data generic_cfg = {
 
 static const struct pcie_cfg_data bcm7425_cfg = {
 	.offsets	= pcie_offsets_bmips_7425,
-	.type		= BCM7425,
+	.model		= BCM7425,
 	.perst_set	= brcm_pcie_perst_set_generic,
 	.bridge_sw_init_set = brcm_pcie_bridge_sw_init_set_generic,
 	.num_inbound	= 3,
@@ -1682,7 +1682,7 @@ static const struct pcie_cfg_data bcm7425_cfg = {
 
 static const struct pcie_cfg_data bcm7435_cfg = {
 	.offsets	= pcie_offsets,
-	.type		= BCM7435,
+	.model		= BCM7435,
 	.perst_set	= brcm_pcie_perst_set_generic,
 	.bridge_sw_init_set = brcm_pcie_bridge_sw_init_set_generic,
 	.num_inbound	= 3,
@@ -1690,7 +1690,7 @@ static const struct pcie_cfg_data bcm7435_cfg = {
 
 static const struct pcie_cfg_data bcm4908_cfg = {
 	.offsets	= pcie_offsets,
-	.type		= BCM4908,
+	.model		= BCM4908,
 	.perst_set	= brcm_pcie_perst_set_4908,
 	.bridge_sw_init_set = brcm_pcie_bridge_sw_init_set_generic,
 	.num_inbound	= 3,
@@ -1706,7 +1706,7 @@ static const int pcie_offset_bcm7278[] = {
 
 static const struct pcie_cfg_data bcm7278_cfg = {
 	.offsets	= pcie_offset_bcm7278,
-	.type		= BCM7278,
+	.model		= BCM7278,
 	.perst_set	= brcm_pcie_perst_set_7278,
 	.bridge_sw_init_set = brcm_pcie_bridge_sw_init_set_7278,
 	.num_inbound	= 3,
@@ -1714,7 +1714,7 @@ static const struct pcie_cfg_data bcm7278_cfg = {
 
 static const struct pcie_cfg_data bcm2711_cfg = {
 	.offsets	= pcie_offsets,
-	.type		= BCM2711,
+	.model		= BCM2711,
 	.perst_set	= brcm_pcie_perst_set_generic,
 	.bridge_sw_init_set = brcm_pcie_bridge_sw_init_set_generic,
 	.num_inbound	= 3,
@@ -1722,7 +1722,7 @@ static const struct pcie_cfg_data bcm2711_cfg = {
 
 static const struct pcie_cfg_data bcm7216_cfg = {
 	.offsets	= pcie_offset_bcm7278,
-	.type		= BCM7278,
+	.model		= BCM7278,
 	.perst_set	= brcm_pcie_perst_set_7278,
 	.bridge_sw_init_set = brcm_pcie_bridge_sw_init_set_7278,
 	.has_phy	= true,
@@ -1779,7 +1779,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->model = data->model;
 	pcie->perst_set = data->perst_set;
 	pcie->bridge_sw_init_set = data->bridge_sw_init_set;
 	pcie->has_phy = data->has_phy;
@@ -1848,7 +1848,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->model == 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;
@@ -1863,7 +1863,7 @@ static int brcm_pcie_probe(struct platform_device *pdev)
 		}
 	}
 
-	bridge->ops = pcie->type == BCM7425 ? &brcm7425_pcie_ops : &brcm_pcie_ops;
+	bridge->ops = pcie->model == BCM7425 ? &brcm7425_pcie_ops : &brcm_pcie_ops;
 	bridge->sysdata = pcie;
 
 	platform_set_drvdata(pdev, pcie);
-- 
2.17.1


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

[PATCH v4 12/12] PCI: brcmstb: Enable 7712 SOCs

[Jim Quinlan]

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

The Broadcom STB 7712 is the sibling chip of the RPi 5 (2712).

Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com>
Reviewed-by: Stanimir Varbanov <svarbanov@suse.de>
Reviewed-by: Florian Fainelli <florian.fainelli@broadcom.com>
Tested-by: Florian Fainelli <florian.fainelli@broadcom.com>
---
 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 fa5616a56383..7debb3599789 100644
--- a/drivers/pci/controller/pcie-brcmstb.c
+++ b/drivers/pci/controller/pcie-brcmstb.c
@@ -1193,6 +1193,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->model == BCM7712)
+		return;
+
 	/* Each unit in timeout register is 1/216,000,000 seconds */
 	writel(216 * timeout_us, pcie->base + REG_OFFSET);
 }
@@ -1664,6 +1668,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,
 	.model		= GENERIC,
@@ -1729,6 +1740,14 @@ static const struct pcie_cfg_data bcm7216_cfg = {
 	.num_inbound	= 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,
+	.model		= BCM7712,
+	.num_inbound	= 10,
+};
+
 static const struct of_device_id brcm_pcie_match[] = {
 	{ .compatible = "brcm,bcm2711-pcie", .data = &bcm2711_cfg },
 	{ .compatible = "brcm,bcm4908-pcie", .data = &bcm4908_cfg },
@@ -1738,6 +1757,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


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

Re: [PATCH v4 01/12] dt-bindings: PCI: Cleanup of brcmstb YAML and add 7712 SoC

[Krzysztof Kozlowski]

On 16/07/2024 23:31, Jim Quinlan wrote:
> o Change order of the compatible strings to be alphabetical
> 
> o Describe resets/reset-names before using them in rules
> 

<form letter>
This is a friendly reminder during the review process.

It seems my or other reviewer's previous comments were not fully
addressed. Maybe the feedback got lost between the quotes, maybe you
just forgot to apply it. Please go back to the previous discussion and
either implement all requested changes or keep discussing them.

Thank you.
</form letter>

> o Add minItems/maxItems where needed.
> 
> o 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.
> 
> Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com>
> ---
>  .../bindings/pci/brcm,stb-pcie.yaml           | 26 ++++++++++++++-----
>  1 file changed, 19 insertions(+), 7 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml b/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
> index 11f8ea33240c..692f7ed7c98e 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
> @@ -95,6 +95,18 @@ properties:
>        minItems: 1
>        maxItems: 3
>  
> +  resets:
> +    minItems: 1
> +    items:
> +      - description: reset for external PCIe PERST# signal # perst
> +      - description: reset for phy reset calibration       # rescal
> +
> +  reset-names:
> +    minItems: 1
> +    items:
> +      - const: perst
> +      - const: rescal

There are no devices with two resets. Anyway, this does not match one of
your variants which have first element as rescal.

Best regards,
Krzysztof

Re: [PATCH v4 02/12] dt-bindings: PCI: brcmstb: Add 7712 SoC description

[Krzysztof Kozlowski]

On 16/07/2024 23:31, Jim Quinlan wrote:
> This adds the description for the 7712 SoC, a Broadcom
> STB sibling chip of the RPi 5.  Two new reset controllers
> are described.
> 
> Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com>
> ---
>  .../bindings/pci/brcm,stb-pcie.yaml           | 26 +++++++++++++++++++
>  1 file changed, 26 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml b/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
> index 692f7ed7c98e..90683a0df2c5 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
> @@ -100,12 +101,16 @@ properties:
>      items:
>        - description: reset for external PCIe PERST# signal # perst
>        - description: reset for phy reset calibration       # rescal
> +      - description: reset for PCIe/CPU bus bridge         # bridge
> +      - description: reset for soft PCIe core reset        # swinit
>  
>    reset-names:
>      minItems: 1
>      items:
>        - const: perst
>        - const: rescal
> +      - const: bridge
> +      - const: swinit

This does not match at all what you have in allOf:if:then section.

>  
>  required:
>    - compatible
> @@ -159,6 +164,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

Look - here it is rescal. Before you said it must be perst.

https://elixir.bootlin.com/linux/v6.8/source/Documentation/devicetree/bindings/ufs/qcom,ufs.yaml#L132

Best regards,
Krzysztof

Re: [PATCH v4 01/12] dt-bindings: PCI: Cleanup of brcmstb YAML and add 7712 SoC

[Jim Quinlan]

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

On Wed, Jul 17, 2024 at 2:51 AM Krzysztof Kozlowski <krzk@kernel.org> wrote:
>
> On 16/07/2024 23:31, Jim Quinlan wrote:
> > o Change order of the compatible strings to be alphabetical
> >
> > o Describe resets/reset-names before using them in rules
> >
>
> <form letter>
> This is a friendly reminder during the review process.
>
> It seems my or other reviewer's previous comments were not fully
> addressed. Maybe the feedback got lost between the quotes, maybe you
> just forgot to apply it. Please go back to the previous discussion and
> either implement all requested changes or keep discussing them.
>
> Thank you.
> </form letter>

I'm sorry Krzysztof, but AFAICT I paid attention to all of your comments and
tried to answer them as best as I could.  Please do not resort to a
form letter; if
you think I missed something(s) please oblige me and say what it is rather than
having  me search for something that you know and I do not.

>
> > o Add minItems/maxItems where needed.
> >
> > o 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.
> >
> > Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com>
> > ---
> >  .../bindings/pci/brcm,stb-pcie.yaml           | 26 ++++++++++++++-----
> >  1 file changed, 19 insertions(+), 7 deletions(-)
> >
> > diff --git a/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml b/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
> > index 11f8ea33240c..692f7ed7c98e 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
> > @@ -95,6 +95,18 @@ properties:
> >        minItems: 1
> >        maxItems: 3
> >
> > +  resets:
> > +    minItems: 1
> > +    items:
> > +      - description: reset for external PCIe PERST# signal # perst
> > +      - description: reset for phy reset calibration       # rescal
> > +
> > +  reset-names:
> > +    minItems: 1
> > +    items:
> > +      - const: perst
> > +      - const: rescal
>
> There are no devices with two resets. Anyway, this does not match one of
> your variants which have first element as rescal.

The 4908 chip exclusively uses the "perst" reset, and the  7216 chip
exclusively uses the "rescal" reset.   The rest of the chips use zero
resets.   All together, there are two resets.

You are the one that wanted me to first list all resets at the top,
and refer to them by the conditional rules.

What am I missing here?

Regards,
Jim Quinlan
Broadcom STB/CM
>
> Best regards,
> Krzysztof
>

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

Re: [PATCH v4 01/12] dt-bindings: PCI: Cleanup of brcmstb YAML and add 7712 SoC

[Krzysztof Kozlowski]

On 17/07/2024 15:20, Jim Quinlan wrote:
> On Wed, Jul 17, 2024 at 2:51 AM Krzysztof Kozlowski <krzk@kernel.org> wrote:
>>
>> On 16/07/2024 23:31, Jim Quinlan wrote:
>>> o Change order of the compatible strings to be alphabetical
>>>
>>> o Describe resets/reset-names before using them in rules
>>>
>>
>> <form letter>
>> This is a friendly reminder during the review process.
>>
>> It seems my or other reviewer's previous comments were not fully
>> addressed. Maybe the feedback got lost between the quotes, maybe you
>> just forgot to apply it. Please go back to the previous discussion and
>> either implement all requested changes or keep discussing them.
>>
>> Thank you.
>> </form letter>
> 
> I'm sorry Krzysztof, but AFAICT I paid attention to all of your comments and
> tried to answer them as best as I could.  Please do not resort to a
> form letter; if
> you think I missed something(s) please oblige me and say what it is rather than
> having  me search for something that you know and I do not.

I do not see your response at all to my comments on patch #2.

> 
>>
>>> o Add minItems/maxItems where needed.
>>>
>>> o 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.
>>>
>>> Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com>
>>> ---
>>>  .../bindings/pci/brcm,stb-pcie.yaml           | 26 ++++++++++++++-----
>>>  1 file changed, 19 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml b/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
>>> index 11f8ea33240c..692f7ed7c98e 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
>>> @@ -95,6 +95,18 @@ properties:
>>>        minItems: 1
>>>        maxItems: 3
>>>
>>> +  resets:
>>> +    minItems: 1
>>> +    items:
>>> +      - description: reset for external PCIe PERST# signal # perst
>>> +      - description: reset for phy reset calibration       # rescal
>>> +
>>> +  reset-names:
>>> +    minItems: 1
>>> +    items:
>>> +      - const: perst
>>> +      - const: rescal
>>
>> There are no devices with two resets. Anyway, this does not match one of
>> your variants which have first element as rescal.
> 
> The 4908 chip exclusively uses the "perst" reset, and the  7216 chip
> exclusively uses the "rescal" reset.   The rest of the chips use zero
> resets.   All together, there are two resets.

This is not enum, but a list. What you do mean overall two resets? You
have a chip which is both 4908 and 7216 at the same time? How is this
possible?

> 
> You are the one that wanted me to first list all resets at the top,
> and refer to them by the conditional rules.

No, I wanted widest constraints at the top.

My comment at v2 was already saying this:

"This does not match what you have in conditional, so just keep min and
max Items here."

and you have numerous examples in the code for this.

Best regards,
Krzysztof

Re: [PATCH v4 01/12] dt-bindings: PCI: Cleanup of brcmstb YAML and add 7712 SoC

[Florian Fainelli]

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

On 7/16/24 23:51, Krzysztof Kozlowski wrote:
> On 16/07/2024 23:31, Jim Quinlan wrote:
>> o Change order of the compatible strings to be alphabetical
>>
>> o Describe resets/reset-names before using them in rules
>>
> 
> <form letter>
> This is a friendly reminder during the review process.
> 
> It seems my or other reviewer's previous comments were not fully
> addressed. Maybe the feedback got lost between the quotes, maybe you
> just forgot to apply it. Please go back to the previous discussion and
> either implement all requested changes or keep discussing them.

Maybe the feedback was not clear, the fault cannot always be in the 
submitter, right?

> 
> Thank you.
> </form letter>
> 
>> o Add minItems/maxItems where needed.
>>
>> o 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.
>>
>> Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com>
>> ---
>>   .../bindings/pci/brcm,stb-pcie.yaml           | 26 ++++++++++++++-----
>>   1 file changed, 19 insertions(+), 7 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml b/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
>> index 11f8ea33240c..692f7ed7c98e 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
>> @@ -95,6 +95,18 @@ properties:
>>         minItems: 1
>>         maxItems: 3
>>   
>> +  resets:
>> +    minItems: 1
>> +    items:
>> +      - description: reset for external PCIe PERST# signal # perst
>> +      - description: reset for phy reset calibration       # rescal
>> +
>> +  reset-names:
>> +    minItems: 1
>> +    items:
>> +      - const: perst
>> +      - const: rescal
> 
> There are no devices with two resets. Anyway, this does not match one of
> your variants which have first element as rescal.

Just to be clear, is the diff below what you would expect to see when 
applying both patch 1 and 4 in succession, that is the reset properties 
are described "generically" and the conditional section only describes 
the order and values:

diff --git a/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml 
b/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
index 11f8ea33240c..faab7291d722 100644
--- a/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
+++ b/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
@@ -95,6 +95,28 @@ properties:
        minItems: 1
        maxItems: 3

+  resets:
+    minItems: 1
+    maxItems: 3
+    oneOf:
+      - description: reset controller handling the PERST# signal
+      - description: phandle pointing to the RESCAL reset controller
+      - items:
+          - description: phandle pointing to the RESCAL reset controller
+          - description: reset for PCIe/CPU bus bridge
+          - description: reset for soft PCIe core reset
+
+  reset-names:
+    minItems: 1
+    maxItems: 3
+    oneOf:
+      - const: perst
+      - const: rescal
+      - items:
+          - const: rescal
+          - const: bridge
+          - const: swinit
+
  required:
    - compatible
    - reg
@@ -118,8 +140,7 @@ allOf:
      then:
        properties:
          resets:
-          items:
-            - description: reset controller handling the PERST# signal
+          maxItems: 1

          reset-names:
            items:
@@ -136,8 +157,7 @@ allOf:
      then:
        properties:
          resets:
-          items:
-            - description: phandle pointing to the RESCAL reset controller
+          maxItems: 1

          reset-names:
            items:
@@ -146,6 +166,22 @@ allOf:
        required:
          - resets
          - reset-names
+  - if:
+      properties:
+        compatible:
+          contains:
+            const: brcm,bcm7712-pcie
+    then:
+      properties:
+        resets:
+          minItems: 3
+
+        reset-names:
+          minItems: 3
+
+      required:
+        - resets
+        - reset-names

  unevaluatedProperties: false

Thanks!
--
Florian


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

Re: [PATCH v4 01/12] dt-bindings: PCI: Cleanup of brcmstb YAML and add 7712 SoC

[Krzysztof Kozlowski]

On 17/07/2024 23:06, Florian Fainelli wrote:
>>> +  resets:
>>> +    minItems: 1
>>> +    items:
>>> +      - description: reset for external PCIe PERST# signal # perst
>>> +      - description: reset for phy reset calibration       # rescal
>>> +
>>> +  reset-names:
>>> +    minItems: 1
>>> +    items:
>>> +      - const: perst
>>> +      - const: rescal
>>
>> There are no devices with two resets. Anyway, this does not match one of
>> your variants which have first element as rescal.
> 
> Just to be clear, is the diff below what you would expect to see when 
> applying both patch 1 and 4 in succession, that is the reset properties 
> are described "generically" and the conditional section only describes 
> the order and values:

Yeah, this would work. I still propose the format I provided link to
(here in this thread or in my talks).

Best regards,
Krzysztof

Re: [PATCH v4 01/12] dt-bindings: PCI: Cleanup of brcmstb YAML and add 7712 SoC

[Krzysztof Kozlowski]

On 17/07/2024 23:06, Florian Fainelli wrote:
> On 7/16/24 23:51, Krzysztof Kozlowski wrote:
>> On 16/07/2024 23:31, Jim Quinlan wrote:
>>> o Change order of the compatible strings to be alphabetical
>>>
>>> o Describe resets/reset-names before using them in rules
>>>
>>
>> <form letter>
>> This is a friendly reminder during the review process.
>>
>> It seems my or other reviewer's previous comments were not fully
>> addressed. Maybe the feedback got lost between the quotes, maybe you
>> just forgot to apply it. Please go back to the previous discussion and
>> either implement all requested changes or keep discussing them.
> 
> Maybe the feedback was not clear, the fault cannot always be in the 
> submitter, right?

That why it is nice to ack each reviewers comment. If reviewers comments
are somehow trickier, then such acks or further clarifications are even
more useful.

Did it happen for v3? No.

Best regards,
Krzysztof

Re: [PATCH v4 01/12] dt-bindings: PCI: Cleanup of brcmstb YAML and add 7712 SoC

[Jim Quinlan]

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

On Wed, Jul 17, 2024 at 2:51 AM Krzysztof Kozlowski <krzk@kernel.org> wrote:
>
> On 16/07/2024 23:31, Jim Quinlan wrote:
> > o Change order of the compatible strings to be alphabetical
> >
> > o Describe resets/reset-names before using them in rules
> >
>
> <form letter>
> This is a friendly reminder during the review process.
>
> It seems my or other reviewer's previous comments were not fully
> addressed. Maybe the feedback got lost between the quotes, maybe you
> just forgot to apply it. Please go back to the previous discussion and
> either implement all requested changes or keep discussing them.
>
> Thank you.
> </form letter>
>
> > o Add minItems/maxItems where needed.
> >
> > o 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.
> >
> > Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com>
> > ---
> >  .../bindings/pci/brcm,stb-pcie.yaml           | 26 ++++++++++++++-----
> >  1 file changed, 19 insertions(+), 7 deletions(-)
> >
> > diff --git a/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml b/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
> > index 11f8ea33240c..692f7ed7c98e 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
> > @@ -95,6 +95,18 @@ properties:
> >        minItems: 1
> >        maxItems: 3
> >
> > +  resets:
> > +    minItems: 1
> > +    items:
> > +      - description: reset for external PCIe PERST# signal # perst
> > +      - description: reset for phy reset calibration       # rescal
> > +
> > +  reset-names:
> > +    minItems: 1
> > +    items:
> > +      - const: perst
> > +      - const: rescal
>
> There are no devices with two resets. Anyway, this does not match one of
> your variants which have first element as rescal.


Hello Krzysztof,

At this commit there are two resets; the 4908 requires "perst" and the
7216 requires "rescal".   I now think what you are looking for is the
top-level
description of something like

resets:
  maxItems: 1
    oneOf:
      - description: reset controller handling the PERST# signal
      - description: phandle pointing to the RESCAL reset controller

reset-names:
  maxItems: 1
    oneOf:
      - const: perst
      - const: rescal

I left out minItems because imItems==maxItems=1

Before I was giving both of them as the "potential candidates list"
that will be used further on, but this is not how Yaml should be used.

Is the above in the right direction?

Regards,
Jim Quinlan
Broadcom STB/CM

>
> Best regards,
> Krzysztof
>

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

Re: [PATCH v4 01/12] dt-bindings: PCI: Cleanup of brcmstb YAML and add 7712 SoC

[Jim Quinlan]

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

On Wed, Jul 17, 2024 at 9:30 AM Krzysztof Kozlowski <krzk@kernel.org> wrote:
>
> On 17/07/2024 15:20, Jim Quinlan wrote:
> > On Wed, Jul 17, 2024 at 2:51 AM Krzysztof Kozlowski <krzk@kernel.org> wrote:
> >>
> >> On 16/07/2024 23:31, Jim Quinlan wrote:
> >>> o Change order of the compatible strings to be alphabetical
> >>>
> >>> o Describe resets/reset-names before using them in rules
> >>>
> >>
> >> <form letter>
> >> This is a friendly reminder during the review process.
> >>
> >> It seems my or other reviewer's previous comments were not fully
> >> addressed. Maybe the feedback got lost between the quotes, maybe you
> >> just forgot to apply it. Please go back to the previous discussion and
> >> either implement all requested changes or keep discussing them.
> >>
> >> Thank you.
> >> </form letter>
> >
> > I'm sorry Krzysztof, but AFAICT I paid attention to all of your comments and
> > tried to answer them as best as I could.  Please do not resort to a
> > form letter; if
> > you think I missed something(s) please oblige me and say what it is rather than
> > having  me search for something that you know and I do not.
>
> I do not see your response at all to my comments on patch #2.
>
> >
> >>
> >>> o Add minItems/maxItems where needed.
> >>>
> >>> o 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.
> >>>
> >>> Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com>
> >>> ---
> >>>  .../bindings/pci/brcm,stb-pcie.yaml           | 26 ++++++++++++++-----
> >>>  1 file changed, 19 insertions(+), 7 deletions(-)
> >>>
> >>> diff --git a/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml b/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
> >>> index 11f8ea33240c..692f7ed7c98e 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
> >>> @@ -95,6 +95,18 @@ properties:
> >>>        minItems: 1
> >>>        maxItems: 3
> >>>
> >>> +  resets:
> >>> +    minItems: 1
> >>> +    items:
> >>> +      - description: reset for external PCIe PERST# signal # perst
> >>> +      - description: reset for phy reset calibration       # rescal
> >>> +
> >>> +  reset-names:
> >>> +    minItems: 1
> >>> +    items:
> >>> +      - const: perst
> >>> +      - const: rescal
> >>
> >> There are no devices with two resets. Anyway, this does not match one of
> >> your variants which have first element as rescal.
> >
> > The 4908 chip exclusively uses the "perst" reset, and the  7216 chip
> > exclusively uses the "rescal" reset.   The rest of the chips use zero
> > resets.   All together, there are two resets.
>
> This is not enum, but a list. What you do mean overall two resets? You
> have a chip which is both 4908 and 7216 at the same time? How is this
> possible?
>
> >
> > You are the one that wanted me to first list all resets at the top,
> > and refer to them by the conditional rules.
>
> No, I wanted widest constraints at the top.
Can you explain what you mean by "widest constraint"?  I did not see
the word "wide" in any of the Linux YAML documentation.

>
> My comment at v2 was already saying this:
>
> "This does not match what you have in conditional, so just keep min and
> max Items here."
>
Okay, what I was referring to was your V1 response:

"Fix the binding first - properties should be defined in top level
"properties:" and then customized."

I think this is correct but I have not been describing the
reset/reset-names properties properly at the top level; i.e.  I have
been giving a full list instead of using "oneOf".

Regards,
Jim Quinlan
Broadcom STB/CM

> and you have numerous examples in the code for this.
>
> Best regards,
> Krzysztof
>

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

Re: [PATCH v4 02/12] dt-bindings: PCI: brcmstb: Add 7712 SoC description

[Jim Quinlan]

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

On Wed, Jul 17, 2024 at 2:53 AM Krzysztof Kozlowski <krzk@kernel.org> wrote:
>
> On 16/07/2024 23:31, Jim Quinlan wrote:
> > This adds the description for the 7712 SoC, a Broadcom
> > STB sibling chip of the RPi 5.  Two new reset controllers
> > are described.
> >
> > Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com>
> > ---
> >  .../bindings/pci/brcm,stb-pcie.yaml           | 26 +++++++++++++++++++
> >  1 file changed, 26 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml b/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
> > index 692f7ed7c98e..90683a0df2c5 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
> > @@ -100,12 +101,16 @@ properties:
> >      items:
> >        - description: reset for external PCIe PERST# signal # perst
> >        - description: reset for phy reset calibration       # rescal
> > +      - description: reset for PCIe/CPU bus bridge         # bridge
> > +      - description: reset for soft PCIe core reset        # swinit
> >
> >    reset-names:
> >      minItems: 1
> >      items:
> >        - const: perst
> >        - const: rescal
> > +      - const: bridge
> > +      - const: swinit
>
> This does not match at all what you have in allOf:if:then section.
>
> >
> >  required:
> >    - compatible
> > @@ -159,6 +164,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
>
> Look - here it is rescal. Before you said it must be perst.
>
> https://elixir.bootlin.com/linux/v6.8/source/Documentation/devicetree/bindings/ufs/qcom,ufs.yaml#L132

Hello Krzysztof,

The difference between my commits and the above example is that the
above example has no "desc" line(s) to describe the clocks  -- How
would you add this?  Or are you okay with (a) no description or (b)
using a "#comment..." next to the clock's name?

Regards,
Jim Quinlan
Broadcom STB/CM
>
> Best regards,
> Krzysztof
>

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

Re: [PATCH v4 02/12] dt-bindings: PCI: brcmstb: Add 7712 SoC description

[Krzysztof Kozlowski]

On 23/07/2024 23:03, Jim Quinlan wrote:
> On Wed, Jul 17, 2024 at 2:53 AM Krzysztof Kozlowski <krzk@kernel.org> wrote:
>>
>> On 16/07/2024 23:31, Jim Quinlan wrote:
>>> This adds the description for the 7712 SoC, a Broadcom
>>> STB sibling chip of the RPi 5.  Two new reset controllers
>>> are described.
>>>
>>> Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com>
>>> ---
>>>  .../bindings/pci/brcm,stb-pcie.yaml           | 26 +++++++++++++++++++
>>>  1 file changed, 26 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml b/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
>>> index 692f7ed7c98e..90683a0df2c5 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
>>> @@ -100,12 +101,16 @@ properties:
>>>      items:
>>>        - description: reset for external PCIe PERST# signal # perst
>>>        - description: reset for phy reset calibration       # rescal
>>> +      - description: reset for PCIe/CPU bus bridge         # bridge
>>> +      - description: reset for soft PCIe core reset        # swinit
>>>
>>>    reset-names:
>>>      minItems: 1
>>>      items:
>>>        - const: perst
>>>        - const: rescal
>>> +      - const: bridge
>>> +      - const: swinit
>>
>> This does not match at all what you have in allOf:if:then section.
>>
>>>
>>>  required:
>>>    - compatible
>>> @@ -159,6 +164,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
>>
>> Look - here it is rescal. Before you said it must be perst.
>>
>> https://elixir.bootlin.com/linux/v6.8/source/Documentation/devicetree/bindings/ufs/qcom,ufs.yaml#L132
> 
> Hello Krzysztof,
> 
> The difference between my commits and the above example is that the
> above example has no "desc" line(s) to describe the clocks  -- How

Which does not really matter to illustrate the concept where you define
the widest constraints and where you narrow them.

> would you add this?  Or are you okay with (a) no description or (b)
> using a "#comment..." next to the clock's name?



Best regards,
Krzysztof

Re: [PATCH v4 01/12] dt-bindings: PCI: Cleanup of brcmstb YAML and add 7712 SoC

[Krzysztof Kozlowski]

On 23/07/2024 20:44, Jim Quinlan wrote:
> On Wed, Jul 17, 2024 at 2:51 AM Krzysztof Kozlowski <krzk@kernel.org> wrote:
>>
>> On 16/07/2024 23:31, Jim Quinlan wrote:
>>> o Change order of the compatible strings to be alphabetical
>>>
>>> o Describe resets/reset-names before using them in rules
>>>
>>
>> <form letter>
>> This is a friendly reminder during the review process.
>>
>> It seems my or other reviewer's previous comments were not fully
>> addressed. Maybe the feedback got lost between the quotes, maybe you
>> just forgot to apply it. Please go back to the previous discussion and
>> either implement all requested changes or keep discussing them.
>>
>> Thank you.
>> </form letter>
>>
>>> o Add minItems/maxItems where needed.
>>>
>>> o 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.
>>>
>>> Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com>
>>> ---
>>>  .../bindings/pci/brcm,stb-pcie.yaml           | 26 ++++++++++++++-----
>>>  1 file changed, 19 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml b/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
>>> index 11f8ea33240c..692f7ed7c98e 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
>>> @@ -95,6 +95,18 @@ properties:
>>>        minItems: 1
>>>        maxItems: 3
>>>
>>> +  resets:
>>> +    minItems: 1
>>> +    items:
>>> +      - description: reset for external PCIe PERST# signal # perst
>>> +      - description: reset for phy reset calibration       # rescal
>>> +
>>> +  reset-names:
>>> +    minItems: 1
>>> +    items:
>>> +      - const: perst
>>> +      - const: rescal
>>
>> There are no devices with two resets. Anyway, this does not match one of
>> your variants which have first element as rescal.
> 
> 
> Hello Krzysztof,
> 
> At this commit there are two resets; the 4908 requires "perst" and the
> 7216 requires "rescal".   I now think what you are looking for is the
> top-level
> description of something like
> 
> resets:
>   maxItems: 1
>     oneOf:
>       - description: reset controller handling the PERST# signal
>       - description: phandle pointing to the RESCAL reset controller

Now tell me, what sort of new information comes with this description?
"Phandle"? It cannot be anything else. Redundant. "Pointing to"?
Redundant. "reset-controller"? Well, resets always point to reset
controller.

So what is the point of this description? Any point?

> 
> reset-names:
>   maxItems: 1
>     oneOf:
>       - const: perst
>       - const: rescal
> 
> I left out minItems because imItems==maxItems=1
> 
> Before I was giving both of them as the "potential candidates list"
> that will be used further on, but this is not how Yaml should be used.
> 
> Is the above in the right direction?

It's over complicated. First maxItems are redundant, because you define
number of items in items. Second, you have EXACTLY the same case as the
hardware for which I gave you bindings to use. I don't understand why
you insist on doing things differently, but you can. Take a look at many
other bindings how they achieve this - there are many, many examples.
But do not invent third or fourth method...

Best regards,
Krzysztof

Re: [PATCH v4 01/12] dt-bindings: PCI: Cleanup of brcmstb YAML and add 7712 SoC

[Jim Quinlan]

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

On Wed, Jul 24, 2024 at 4:05 AM Krzysztof Kozlowski <krzk@kernel.org> wrote:
>
> On 23/07/2024 20:44, Jim Quinlan wrote:
> > On Wed, Jul 17, 2024 at 2:51 AM Krzysztof Kozlowski <krzk@kernel.org> wrote:
> >>
> >> On 16/07/2024 23:31, Jim Quinlan wrote:
> >>> o Change order of the compatible strings to be alphabetical
> >>>
> >>> o Describe resets/reset-names before using them in rules
> >>>
> >>
> >> <form letter>
> >> This is a friendly reminder during the review process.
> >>
> >> It seems my or other reviewer's previous comments were not fully
> >> addressed. Maybe the feedback got lost between the quotes, maybe you
> >> just forgot to apply it. Please go back to the previous discussion and
> >> either implement all requested changes or keep discussing them.
> >>
> >> Thank you.
> >> </form letter>
> >>
> >>> o Add minItems/maxItems where needed.
> >>>
> >>> o 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.
> >>>
> >>> Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com>
> >>> ---
> >>>  .../bindings/pci/brcm,stb-pcie.yaml           | 26 ++++++++++++++-----
> >>>  1 file changed, 19 insertions(+), 7 deletions(-)
> >>>
> >>> diff --git a/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml b/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
> >>> index 11f8ea33240c..692f7ed7c98e 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
> >>> @@ -95,6 +95,18 @@ properties:
> >>>        minItems: 1
> >>>        maxItems: 3
> >>>
> >>> +  resets:
> >>> +    minItems: 1
> >>> +    items:
> >>> +      - description: reset for external PCIe PERST# signal # perst
> >>> +      - description: reset for phy reset calibration       # rescal
> >>> +
> >>> +  reset-names:
> >>> +    minItems: 1
> >>> +    items:
> >>> +      - const: perst
> >>> +      - const: rescal
> >>
> >> There are no devices with two resets. Anyway, this does not match one of
> >> your variants which have first element as rescal.
> >
> >
> > Hello Krzysztof,
> >
> > At this commit there are two resets; the 4908 requires "perst" and the
> > 7216 requires "rescal".   I now think what you are looking for is the
> > top-level
> > description of something like
> >
> > resets:
> >   maxItems: 1
> >     oneOf:
> >       - description: reset controller handling the PERST# signal
> >       - description: phandle pointing to the RESCAL reset controller
>
> Now tell me, what sort of new information comes with this description?
> "Phandle"? It cannot be anything else. Redundant. "Pointing to"?
> Redundant. "reset-controller"? Well, resets always point to reset
> controller.
>
> So what is the point of this description? Any point?
>
> >
> > reset-names:
> >   maxItems: 1
> >     oneOf:
> >       - const: perst
> >       - const: rescal
> >
> > I left out minItems because imItems==maxItems=1
> >
> > Before I was giving both of them as the "potential candidates list"
> > that will be used further on, but this is not how Yaml should be used.
> >
> > Is the above in the right direction?
>
> It's over complicated. First maxItems are redundant, because you define
> number of items in items. Second, you have EXACTLY the same case as the
> hardware for which I gave you bindings to use. I don't understand why
> you insist on doing things differently, but you can. Take a look at many
> other bindings how they achieve this - there are many, many examples.
> But do not invent third or fourth method...

ack, I will follow the qcom,ufs.yaml file you referenced.

>
> Best regards,
> Krzysztof
>

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

Re: [PATCH v4 03/12] PCI: brcmstb: Use common error handling code in brcm_pcie_probe()

[Manivannan Sadhasivam]

On Tue, Jul 16, 2024 at 05:31:18PM -0400, Jim Quinlan wrote:
> o Move the clk_prepare_enable() below the resource allocations.
> o Add a jump target (clk_out) so that a bit of exception handling can be
>   better reused at the end of this function implementation.
> 
> 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 | 29 +++++++++++++++------------
>  1 file changed, 16 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
> index c08683febdd4..c257434edc08 100644
> --- a/drivers/pci/controller/pcie-brcmstb.c
> +++ b/drivers/pci/controller/pcie-brcmstb.c
> @@ -1613,31 +1613,30 @@ 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 = clk_prepare_enable(pcie->clk);
> +	if (ret) {
> +		dev_err(&pdev->dev, "could not enable clock\n");
> +		return ret;
>  	}
>  
>  	ret = reset_control_reset(pcie->rescal);
> -	if (ret)
> +	if (ret) {
>  		dev_err(&pdev->dev, "failed to deassert 'rescal'\n");
> +		goto clk_out;

Please use a descriptive name for the err labels. Here this err path disables
and unprepares the clk, so use 'clk_disable_unprepare'.

> +	}
>  
>  	ret = brcm_phy_start(pcie);
>  	if (ret) {
>  		reset_control_rearm(pcie->rescal);
> -		clk_disable_unprepare(pcie->clk);
> -		return ret;
> +		goto clk_out;
>  	}
>  
>  	ret = brcm_pcie_setup(pcie);
> @@ -1676,6 +1675,10 @@ static int brcm_pcie_probe(struct platform_device *pdev)
>  
>  	return 0;
>  
> +clk_out:
> +	clk_disable_unprepare(pcie->clk);
> +	return ret;
> +

This is leaking the resources. Move this new label below 'fail'.

- Mani

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

Re: [PATCH v4 04/12] PCI: brcmstb: Use bridge reset if available

[Manivannan Sadhasivam]

On Tue, Jul 16, 2024 at 05:31:19PM -0400, Jim Quinlan wrote:
> The 7712 SOC has a bridge reset which can be described in the device tree.
> If it is present, use it. Otherwise, continue to use the legacy method to
> reset the bridge.
> 
> 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 | 22 +++++++++++++++++-----
>  1 file changed, 17 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
> index c257434edc08..92816d8d215a 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;

'bridge' is an opaque name. Use something like '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 (pcie->bridge) {
> +		if (val)
> +			reset_control_assert(pcie->bridge);
> +		else
> +			reset_control_deassert(pcie->bridge);

Well, these APIs will bail out if the reset_control pointer is NULL. So you can
just call them directly instead of if..else condition.

- Mani

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

Re: [PATCH v4 05/12] PCI: brcmstb: Use swinit reset if available

[Manivannan Sadhasivam]

On Tue, Jul 16, 2024 at 05:31:20PM -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: Stanimir Varbanov <svarbanov@suse.de>
> Reviewed-by: Florian Fainelli <florian.fainelli@broadcom.com>
> ---
>  drivers/pci/controller/pcie-brcmstb.c | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
> 
> diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
> index 92816d8d215a..4dc2ff7f3167 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;
> +	struct reset_control	*swinit;

Same comment as previous patch.

>  	int			num_memc;
>  	u64			memc_size[PCIE_BRCM_MAX_MEMC];
>  	u32			hw_rev;
> @@ -1633,12 +1634,27 @@ static int brcm_pcie_probe(struct platform_device *pdev)
>  	if (IS_ERR(pcie->bridge))
>  		return PTR_ERR(pcie->bridge);
>  
> +	pcie->swinit = devm_reset_control_get_optional_exclusive(&pdev->dev, "swinit");
> +	if (IS_ERR(pcie->swinit))
> +		return PTR_ERR(pcie->swinit);
> +
>  	ret = clk_prepare_enable(pcie->clk);
>  	if (ret) {
>  		dev_err(&pdev->dev, "could not enable clock\n");
>  		return ret;
>  	}
>  
> +	ret = reset_control_assert(pcie->swinit);
> +	if (ret) {
> +		dev_err_probe(&pdev->dev, ret, "could not assert reset 'swinit'\n");
> +		goto clk_out;
> +	}

No delay required?

- Mani

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

Re: [PATCH v4 06/12] PCI: brcmstb: PCI: brcmstb: Make HARD_DEBUG, INTR2_CPU_BASE offsets SoC-specific

[Manivannan Sadhasivam]

On Tue, Jul 16, 2024 at 05:31:21PM -0400, Jim Quinlan wrote:
> Our HW design has again changed a register offset which used to be standard

Changed for which SoC? Please mention that this is a preparatory work.

> for all Broadcom SOCs with PCIe cores.  This difference is now reconciled
> for the registers HARD_DEBUG and INTR2_CPU_BASE.
> 
> Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com>

With that,

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

- Mani

> Reviewed-by: Stanimir Varbanov <svarbanov@suse.de>
> Reviewed-by: Florian Fainelli <florian.fainelli@broadcom.com>
> ---
>  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 4dc2ff7f3167..073d790d97b7 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
> 



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

Re: [PATCH v4 07/12] PCI: brcmstb: Remove two unused constants from driver

[Manivannan Sadhasivam]

On Tue, Jul 16, 2024 at 05:31:22PM -0400, Jim Quinlan wrote:
> Two constants in the driver, RGR1_SW_INIT_1_INIT_MASK and
> RGR1_SW_INIT_1_INIT_SHIFT are no longer used and are removed.
> 
> Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com>

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

- Mani

> Reviewed-by: Stanimir Varbanov <svarbanov@suse.de>
> Reviewed-by: Florian Fainelli <florian.fainelli@broadcom.com>
> ---
>  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 073d790d97b7..dfb404748ad8 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
> 



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

Re: [PATCH v4 08/12] PCI: brcmstb: Don't conflate the reset rescal with phy ctrl

[Manivannan Sadhasivam]

On Tue, Jul 16, 2024 at 05:31:23PM -0400, Jim Quinlan wrote:
> We've been assuming that if an SOC has a "rescal" reset controller that we
> should automatically invoke brcm_phy_cntl(...).  This will not be true in
> future SOCs, so we create a bool "has_phy" and adjust the cfg_data
> appropriately (we need to give 7216 its own cfg_data structure instead of
> sharing one).
> 

In all commit messages, use imperative tone as per kernel documentation:

"Describe your changes in imperative mood, e.g. "make xyzzy do frotz"
instead of "[This patch] makes xyzzy do frotz" or "[I] changed xyzzy
to do frotz", as if you are giving orders to the codebase to change
its behaviour."

> 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 | 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 dfb404748ad8..8ab5a8ca05b4 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;

'has_phy' means the controller supports PHY and the new SoC doesn't have a PHY
for the controller?

- Mani

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

Re: [PATCH v4 09/12] PCI: brcmstb: Refactor for chips with many regular inbound BARs

[Manivannan Sadhasivam]

On Tue, Jul 16, 2024 at 05:31:24PM -0400, Jim Quinlan wrote:
> Previously, our chips provided three inbound "BARS" 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.  We
> typically only used one of these BARs.
> 
> Complicating that BARs 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 have taken a step forward
> and now provide multiple inbound BARs.  This works in concert with the
> dma-ranges property, where each provided range becomes an inbound BAR.
> 
> This commit provides support for these new chips and their multiple
> inbound BARs but also keeps the legacy support for the older system.
> 

BAR belongs to the endpoints not to the RC. How can the RC have 'BARs'? RC can
only map endpoint BARs to MEM region. What you are referring to is 'MEM region'
maybe?

- Mani

> Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com>
> Reviewed-by: Stanimir Varbanov <svarbanov@suse.de>
> ---
>  drivers/pci/controller/pcie-brcmstb.c | 216 ++++++++++++++++++++------
>  1 file changed, 167 insertions(+), 49 deletions(-)
> 
> diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
> index 8ab5a8ca05b4..c44a92217855 100644
> --- a/drivers/pci/controller/pcie-brcmstb.c
> +++ b/drivers/pci/controller/pcie-brcmstb.c
> @@ -75,15 +75,12 @@
>  #define PCIE_MEM_WIN0_HI(win)	\
>  		PCIE_MISC_CPU_2_PCIE_MEM_WIN0_HI + ((win) * 8)
>  
> +#define PCIE_BRCM_MAX_RC_BARS				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 +127,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 +218,20 @@ enum pcie_type {
>  	BCM4908,
>  	BCM7278,
>  	BCM2711,
> +	BCM7712,
> +};
> +
> +struct rc_bar {
> +	u64 size;
> +	u64 pci_offset;
> +	u64 cpu_addr;
>  };
>  
>  struct pcie_cfg_data {
>  	const int *offsets;
>  	const enum pcie_type type;
>  	const bool has_phy;
> +	unsigned int num_inbound;
>  	void (*perst_set)(struct brcm_pcie *pcie, u32 val);
>  	void (*bridge_sw_init_set)(struct brcm_pcie *pcie, u32 val);
>  };
> @@ -274,6 +283,7 @@ struct brcm_pcie {
>  	struct subdev_regulators *sr;
>  	bool			ep_wakeup_capable;
>  	bool			has_phy;
> +	int			num_inbound;
>  };
>  
>  static inline bool is_bmips(const struct brcm_pcie *pcie)
> @@ -789,23 +799,61 @@ 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 inline void set_bar(struct rc_bar *b, int *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 rc_bar rc_bars[])
>  {
>  	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;
> +	int ret, i = 0, n = 0;
> +
> +	/*
> +	 * The HW registers (and PCIe) use order-1 numbering for BARs. As
> +	 * such, we have rc_bars[0] unused and BAR1 starts at rc_bars[1].
> +	 */
> +	struct rc_bar *b_begin = &rc_bars[1];
> +	struct rc_bar *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)
> +		set_bar(b++, &n, 0, 0, 0);
>  
>  	resource_list_for_each_entry(entry, &bridge->dma_ranges) {
>  		u64 pcie_beg = entry->res->start - entry->offset;
> +		u64 cpu_beg = entry->res->start;
>  
> -		size += entry->res->end - entry->res->start + 1;
> +		size = resource_size(entry->res);
> +		tot_size += size;
>  		if (pcie_beg < lowest_pcie_addr)
>  			lowest_pcie_addr = pcie_beg;
> +		/*
> +		 * 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)
> +			set_bar(b++, &n, size, cpu_beg, pcie_beg);
> +
> +		if (n > pcie->num_inbound)
> +			break;
>  	}
>  
>  	if (lowest_pcie_addr == ~(u64)0) {
> @@ -813,13 +861,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 +883,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 +926,89 @@ 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)) {
> +	if (!size || (pci_offset & (size - 1)) ||
> +	    (pci_offset < SZ_4G && pci_offset > SZ_2G)) {
>  		dev_err(dev, "Invalid rc_bar2_offset/size: size 0x%llx, off 0x%llx\n",
> -			*rc_bar2_size, *rc_bar2_offset);
> +			size, pci_offset);
>  		return -EINVAL;
>  	}
>  
> -	return 0;
> +	/* Enable BAR2, the inbound window for STB chips */
> +	set_bar(b++, &n, size, cpu_addr, pci_offset);
> +
> +	/*
> +	 * Disable BAR3.  On some chips presents the same window as BAR2
> +	 * but the data appears in a settable endianness.
> +	 */
> +	set_bar(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 rc_bar *rc_bars,
> +				      int num_rc_bars)
> +{
> +	void __iomem *base = pcie->base;
> +	int i;
> +
> +	for (i = 1; i <= num_rc_bars; i++) {
> +		u64 pci_offset = rc_bars[i].pci_offset;
> +		u64 cpu_addr = rc_bars[i].cpu_addr;
> +		u64 size = rc_bars[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(tmp, base + reg_offset);
> +		/* Write high */
> +		writel(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(tmp, base + reg_offset);
> +			tmp = upper_32_bits(cpu_addr);
> +			writel(tmp, base + reg_offset + 4);
> +		}
> +	}
>  }
>  
>  static int brcm_pcie_setup(struct brcm_pcie *pcie)
>  {
> -	u64 rc_bar2_offset, rc_bar2_size;
> +	struct rc_bar rc_bars[PCIE_BRCM_MAX_RC_BARS];
>  	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;
> +	int num_out_wins = 0, num_rc_bars = 0;
> +	int memc;
>  
>  	/* Reset the bridge */
>  	pcie->bridge_sw_init_set(pcie, 1);
> @@ -933,17 +1057,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_rc_bars = brcm_pcie_get_inbound_wins(pcie, rc_bars);
> +	if (num_rc_bars < 0)
> +		return num_rc_bars;
>  
> -	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);
> +	set_inbound_win_registers(pcie, rc_bars, num_rc_bars);
> +
> +	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 +1088,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 (rc_bars[2].pci_offset >= SZ_4G ||
> +	    (rc_bars[2].size + rc_bars[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;
> @@ -1516,6 +1626,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	= 3,
>  };
>  
>  static const struct pcie_cfg_data bcm7425_cfg = {
> @@ -1523,6 +1634,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	= 3,
>  };
>  
>  static const struct pcie_cfg_data bcm7435_cfg = {
> @@ -1530,6 +1642,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	= 3,
>  };
>  
>  static const struct pcie_cfg_data bcm4908_cfg = {
> @@ -1537,6 +1650,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	= 3,
>  };
>  
>  static const int pcie_offset_bcm7278[] = {
> @@ -1552,6 +1666,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	= 3,
>  };
>  
>  static const struct pcie_cfg_data bcm2711_cfg = {
> @@ -1559,6 +1674,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	= 3,
>  };
>  
>  static const struct pcie_cfg_data bcm7216_cfg = {
> @@ -1567,6 +1683,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	= 3,
>  };
>  
>  static const struct of_device_id brcm_pcie_match[] = {
> @@ -1623,6 +1740,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 = data->num_inbound;
>  
>  	pcie->base = devm_platform_ioremap_resource(pdev, 0);
>  	if (IS_ERR(pcie->base))
> -- 
> 2.17.1
> 



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

Re: [PATCH v4 11/12] PCI: brcmstb: Change field name from 'type' to 'model'

[Manivannan Sadhasivam]

On Tue, Jul 16, 2024 at 05:31:26PM -0400, Jim Quinlan wrote:
> The 'type' field used in the driver to discern SoC differences is confusing
> so change it to the more apt 'model'.  We considered using 'family' but
> this conflicts with Broadcom's conception of a family; for example, 7216a0
> and 7216b0 chips are both considered separate families as each has multiple
> derivative product chips based on the original design.
> 

TBH, 'model' is also confusing :) Why can't you just use 'soc' as you are
referrring to the SoC name.

- Mani

> Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com>
> Reviewed-by: Florian Fainelli <florian.fainelli@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 2fe1f2a26697..fa5616a56383 100644
> --- a/drivers/pci/controller/pcie-brcmstb.c
> +++ b/drivers/pci/controller/pcie-brcmstb.c
> @@ -211,7 +211,7 @@ enum {
>  	PCIE_INTR2_CPU_BASE,
>  };
>  
> -enum pcie_type {
> +enum pcie_model {
>  	GENERIC,
>  	BCM7425,
>  	BCM7435,
> @@ -229,7 +229,7 @@ struct rc_bar {
>  
>  struct pcie_cfg_data {
>  	const int *offsets;
> -	const enum pcie_type type;
> +	const enum pcie_model model;
>  	const bool has_phy;
>  	unsigned int num_inbound;
>  	int (*perst_set)(struct brcm_pcie *pcie, u32 val);
> @@ -270,7 +270,7 @@ struct brcm_pcie {
>  	u64			msi_target_addr;
>  	struct brcm_msi		*msi;
>  	const int		*reg_offsets;
> -	enum pcie_type		type;
> +	enum pcie_model		model;
>  	struct reset_control	*rescal;
>  	struct reset_control	*perst_reset;
>  	struct reset_control	*bridge;
> @@ -288,7 +288,7 @@ struct brcm_pcie {
>  
>  static inline bool is_bmips(const struct brcm_pcie *pcie)
>  {
> -	return pcie->type == BCM7435 || pcie->type == BCM7425;
> +	return pcie->model == BCM7435 || pcie->model == BCM7425;
>  }
>  
>  /*
> @@ -852,7 +852,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->model != BCM7712)
>  		set_bar(b++, &n, 0, 0, 0);
>  
>  	resource_list_for_each_entry(entry, &bridge->dma_ranges) {
> @@ -869,7 +869,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->model == BCM7712)
>  			set_bar(b++, &n, size, cpu_beg, pcie_beg);
>  
>  		if (n > pcie->num_inbound)
> @@ -886,7 +886,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->model == BCM7712)
>  		return n;
>  
>  	ret = of_property_read_variable_u64_array(pcie->np, "brcm,scb-sizes", pcie->memc_size, 1,
> @@ -1008,7 +1008,7 @@ static void set_inbound_win_registers(struct brcm_pcie *pcie, const struct rc_ba
>  		 * 7712:
>  		 *     All of their BARs need to be set.
>  		 */
> -		if (pcie->type == BCM7712) {
> +		if (pcie->model == BCM7712) {
>  			/* BUS remap register settings */
>  			reg_offset = brcm_ubus_reg_offset(i);
>  			tmp = lower_32_bits(cpu_addr) & ~0xfff;
> @@ -1036,7 +1036,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->model == BCM2711) {
>  		ret = pcie->perst_set(pcie, 1);
>  		if (ret) {
>  			pcie->bridge_sw_init_set(pcie, 0);
> @@ -1067,9 +1067,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->model == BCM2711)
>  		burst = 0x0; /* 128 bytes */
> -	else if (pcie->type == BCM7278)
> +	else if (pcie->model == BCM7278)
>  		burst = 0x3; /* 512 bytes */
>  	else
>  		burst = 0x2; /* 512 bytes */
> @@ -1666,7 +1666,7 @@ static const int pcie_offsets_bmips_7425[] = {
>  
>  static const struct pcie_cfg_data generic_cfg = {
>  	.offsets	= pcie_offsets,
> -	.type		= GENERIC,
> +	.model		= GENERIC,
>  	.perst_set	= brcm_pcie_perst_set_generic,
>  	.bridge_sw_init_set = brcm_pcie_bridge_sw_init_set_generic,
>  	.num_inbound	= 3,
> @@ -1674,7 +1674,7 @@ static const struct pcie_cfg_data generic_cfg = {
>  
>  static const struct pcie_cfg_data bcm7425_cfg = {
>  	.offsets	= pcie_offsets_bmips_7425,
> -	.type		= BCM7425,
> +	.model		= BCM7425,
>  	.perst_set	= brcm_pcie_perst_set_generic,
>  	.bridge_sw_init_set = brcm_pcie_bridge_sw_init_set_generic,
>  	.num_inbound	= 3,
> @@ -1682,7 +1682,7 @@ static const struct pcie_cfg_data bcm7425_cfg = {
>  
>  static const struct pcie_cfg_data bcm7435_cfg = {
>  	.offsets	= pcie_offsets,
> -	.type		= BCM7435,
> +	.model		= BCM7435,
>  	.perst_set	= brcm_pcie_perst_set_generic,
>  	.bridge_sw_init_set = brcm_pcie_bridge_sw_init_set_generic,
>  	.num_inbound	= 3,
> @@ -1690,7 +1690,7 @@ static const struct pcie_cfg_data bcm7435_cfg = {
>  
>  static const struct pcie_cfg_data bcm4908_cfg = {
>  	.offsets	= pcie_offsets,
> -	.type		= BCM4908,
> +	.model		= BCM4908,
>  	.perst_set	= brcm_pcie_perst_set_4908,
>  	.bridge_sw_init_set = brcm_pcie_bridge_sw_init_set_generic,
>  	.num_inbound	= 3,
> @@ -1706,7 +1706,7 @@ static const int pcie_offset_bcm7278[] = {
>  
>  static const struct pcie_cfg_data bcm7278_cfg = {
>  	.offsets	= pcie_offset_bcm7278,
> -	.type		= BCM7278,
> +	.model		= BCM7278,
>  	.perst_set	= brcm_pcie_perst_set_7278,
>  	.bridge_sw_init_set = brcm_pcie_bridge_sw_init_set_7278,
>  	.num_inbound	= 3,
> @@ -1714,7 +1714,7 @@ static const struct pcie_cfg_data bcm7278_cfg = {
>  
>  static const struct pcie_cfg_data bcm2711_cfg = {
>  	.offsets	= pcie_offsets,
> -	.type		= BCM2711,
> +	.model		= BCM2711,
>  	.perst_set	= brcm_pcie_perst_set_generic,
>  	.bridge_sw_init_set = brcm_pcie_bridge_sw_init_set_generic,
>  	.num_inbound	= 3,
> @@ -1722,7 +1722,7 @@ static const struct pcie_cfg_data bcm2711_cfg = {
>  
>  static const struct pcie_cfg_data bcm7216_cfg = {
>  	.offsets	= pcie_offset_bcm7278,
> -	.type		= BCM7278,
> +	.model		= BCM7278,
>  	.perst_set	= brcm_pcie_perst_set_7278,
>  	.bridge_sw_init_set = brcm_pcie_bridge_sw_init_set_7278,
>  	.has_phy	= true,
> @@ -1779,7 +1779,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->model = data->model;
>  	pcie->perst_set = data->perst_set;
>  	pcie->bridge_sw_init_set = data->bridge_sw_init_set;
>  	pcie->has_phy = data->has_phy;
> @@ -1848,7 +1848,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->model == 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;
> @@ -1863,7 +1863,7 @@ static int brcm_pcie_probe(struct platform_device *pdev)
>  		}
>  	}
>  
> -	bridge->ops = pcie->type == BCM7425 ? &brcm7425_pcie_ops : &brcm_pcie_ops;
> +	bridge->ops = pcie->model == BCM7425 ? &brcm7425_pcie_ops : &brcm_pcie_ops;
>  	bridge->sysdata = pcie;
>  
>  	platform_set_drvdata(pdev, pcie);
> -- 
> 2.17.1
> 



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

Re: [PATCH v4 12/12] PCI: brcmstb: Enable 7712 SOCs

[Manivannan Sadhasivam]

On Tue, Jul 16, 2024 at 05:31:27PM -0400, Jim Quinlan wrote:
> The Broadcom STB 7712 is the sibling chip of the RPi 5 (2712).
> 

Could you please add more info about this SoC? What PCIe Gen it supports, lanes,
IP revision etc...

- Mani

> Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com>
> Reviewed-by: Stanimir Varbanov <svarbanov@suse.de>
> Reviewed-by: Florian Fainelli <florian.fainelli@broadcom.com>
> Tested-by: Florian Fainelli <florian.fainelli@broadcom.com>
> ---
>  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 fa5616a56383..7debb3599789 100644
> --- a/drivers/pci/controller/pcie-brcmstb.c
> +++ b/drivers/pci/controller/pcie-brcmstb.c
> @@ -1193,6 +1193,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->model == BCM7712)
> +		return;
> +
>  	/* Each unit in timeout register is 1/216,000,000 seconds */
>  	writel(216 * timeout_us, pcie->base + REG_OFFSET);
>  }
> @@ -1664,6 +1668,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,
>  	.model		= GENERIC,
> @@ -1729,6 +1740,14 @@ static const struct pcie_cfg_data bcm7216_cfg = {
>  	.num_inbound	= 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,
> +	.model		= BCM7712,
> +	.num_inbound	= 10,
> +};
> +
>  static const struct of_device_id brcm_pcie_match[] = {
>  	{ .compatible = "brcm,bcm2711-pcie", .data = &bcm2711_cfg },
>  	{ .compatible = "brcm,bcm4908-pcie", .data = &bcm4908_cfg },
> @@ -1738,6 +1757,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 v4 00/12] PCI: brcnstb: Enable STB 7712 SOC

[Manivannan Sadhasivam]

On Tue, Jul 16, 2024 at 05:31:15PM -0400, Jim Quinlan wrote:
> 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)
> 

Rpi mailing list owners: Could you please make the list 'unmoderated'? I haven't
subscribe to this list, so I just keep getting the 'Your message to
linux-rpi-kernel awaits moderator approval' for every review comment I post,
which is a spam to me.

Also I won't subscribe to this list unless I work on Rpi. So using moderated
list in LKML is just spamming the reviewers.

- Mani

> 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 (12):
>   dt-bindings: PCI: Cleanup of brcmstb YAML and add 7712 SoC
>   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 BARs
>   PCI: brcmstb: Check return value of all reset_control_xxx calls
>   PCI: brcmstb: Change field name from 'type' to 'model'
>   PCI: brcmstb: Enable 7712 SOCs
> 
>  .../bindings/pci/brcm,stb-pcie.yaml           |  50 +-
>  drivers/pci/controller/pcie-brcmstb.c         | 485 +++++++++++++-----
>  2 files changed, 400 insertions(+), 135 deletions(-)
> 
> 
> base-commit: 55027e689933ba2e64f3d245fb1ff185b3e7fc81
> -- 
> 2.17.1
> 



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

Re: [PATCH v4 03/12] PCI: brcmstb: Use common error handling code in brcm_pcie_probe()

[Jim Quinlan]

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

On Thu, Jul 25, 2024 at 12:31 AM Manivannan Sadhasivam
<manivannan.sadhasivam@linaro.org> wrote:
>
> On Tue, Jul 16, 2024 at 05:31:18PM -0400, Jim Quinlan wrote:
> > o Move the clk_prepare_enable() below the resource allocations.
> > o Add a jump target (clk_out) so that a bit of exception handling can be
> >   better reused at the end of this function implementation.
> >
> > 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 | 29 +++++++++++++++------------
> >  1 file changed, 16 insertions(+), 13 deletions(-)
> >
> > diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
> > index c08683febdd4..c257434edc08 100644
> > --- a/drivers/pci/controller/pcie-brcmstb.c
> > +++ b/drivers/pci/controller/pcie-brcmstb.c
> > @@ -1613,31 +1613,30 @@ 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 = clk_prepare_enable(pcie->clk);
> > +     if (ret) {
> > +             dev_err(&pdev->dev, "could not enable clock\n");
> > +             return ret;
> >       }
> >
> >       ret = reset_control_reset(pcie->rescal);
> > -     if (ret)
> > +     if (ret) {
> >               dev_err(&pdev->dev, "failed to deassert 'rescal'\n");
> > +             goto clk_out;
>
> Please use a descriptive name for the err labels. Here this err path disables
> and unprepares the clk, so use 'clk_disable_unprepare'.
ack
>
> > +     }
> >
> >       ret = brcm_phy_start(pcie);
> >       if (ret) {
> >               reset_control_rearm(pcie->rescal);
> > -             clk_disable_unprepare(pcie->clk);
> > -             return ret;
> > +             goto clk_out;
> >       }
> >
> >       ret = brcm_pcie_setup(pcie);
> > @@ -1676,6 +1675,10 @@ static int brcm_pcie_probe(struct platform_device *pdev)
> >
> >       return 0;
> >
> > +clk_out:
> > +     clk_disable_unprepare(pcie->clk);
> > +     return ret;
> > +
>
> This is leaking the resources. Move this new label below 'fail'.
What resources is it leaking?  At "clk_out" the return value will be negative
and only managed resources have been allocated at that juncture.

Regards,
Jim Quinlan
Broadcom STB/CM

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

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

Re: [PATCH v4 09/12] PCI: brcmstb: Refactor for chips with many regular inbound BARs

[Jim Quinlan]

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

On Thu, Jul 25, 2024 at 12:53 AM Manivannan Sadhasivam
<manivannan.sadhasivam@linaro.org> wrote:
>
> On Tue, Jul 16, 2024 at 05:31:24PM -0400, Jim Quinlan wrote:
> > Previously, our chips provided three inbound "BARS" 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.  We
> > typically only used one of these BARs.
> >
> > Complicating that BARs 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 have taken a step forward
> > and now provide multiple inbound BARs.  This works in concert with the
> > dma-ranges property, where each provided range becomes an inbound BAR.
> >
> > This commit provides support for these new chips and their multiple
> > inbound BARs but also keeps the legacy support for the older system.
> >
>
> BAR belongs to the endpoints not to the RC. How can the RC have 'BARs'? RC can
> only map endpoint BARs to MEM region. What you are referring to is 'MEM region'
> maybe?

Agreed, it is confusing.  Long story short, the HW team gave the
inbound windows the label "BAR".   We will still have to use their
register names,
e.g. PCIE_MISC_RC_BAR4_CONFIG_LO, but what I can do is change
for example "struct rc_bar" to "struct inbound_win" as well as make similar
changes to the code and function names.

Let's assume you will be okay with my plan above; if not, please tell
me what you would prefer.

Regards,
Jim Quinlan
Broadcom STB/CM
>
> - Mani
>
> > Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com>
> > Reviewed-by: Stanimir Varbanov <svarbanov@suse.de>
> > ---
> >  drivers/pci/controller/pcie-brcmstb.c | 216 ++++++++++++++++++++------
> >  1 file changed, 167 insertions(+), 49 deletions(-)
> >
> > diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
> > index 8ab5a8ca05b4..c44a92217855 100644
> > --- a/drivers/pci/controller/pcie-brcmstb.c
> > +++ b/drivers/pci/controller/pcie-brcmstb.c
> > @@ -75,15 +75,12 @@
> >  #define PCIE_MEM_WIN0_HI(win)        \
> >               PCIE_MISC_CPU_2_PCIE_MEM_WIN0_HI + ((win) * 8)
> >
> > +#define PCIE_BRCM_MAX_RC_BARS                                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 +127,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 +218,20 @@ enum pcie_type {
> >       BCM4908,
> >       BCM7278,
> >       BCM2711,
> > +     BCM7712,
> > +};
> > +
> > +struct rc_bar {
> > +     u64 size;
> > +     u64 pci_offset;
> > +     u64 cpu_addr;
> >  };
> >
> >  struct pcie_cfg_data {
> >       const int *offsets;
> >       const enum pcie_type type;
> >       const bool has_phy;
> > +     unsigned int num_inbound;
> >       void (*perst_set)(struct brcm_pcie *pcie, u32 val);
> >       void (*bridge_sw_init_set)(struct brcm_pcie *pcie, u32 val);
> >  };
> > @@ -274,6 +283,7 @@ struct brcm_pcie {
> >       struct subdev_regulators *sr;
> >       bool                    ep_wakeup_capable;
> >       bool                    has_phy;
> > +     int                     num_inbound;
> >  };
> >
> >  static inline bool is_bmips(const struct brcm_pcie *pcie)
> > @@ -789,23 +799,61 @@ 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 inline void set_bar(struct rc_bar *b, int *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 rc_bar rc_bars[])
> >  {
> >       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;
> > +     int ret, i = 0, n = 0;
> > +
> > +     /*
> > +      * The HW registers (and PCIe) use order-1 numbering for BARs. As
> > +      * such, we have rc_bars[0] unused and BAR1 starts at rc_bars[1].
> > +      */
> > +     struct rc_bar *b_begin = &rc_bars[1];
> > +     struct rc_bar *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)
> > +             set_bar(b++, &n, 0, 0, 0);
> >
> >       resource_list_for_each_entry(entry, &bridge->dma_ranges) {
> >               u64 pcie_beg = entry->res->start - entry->offset;
> > +             u64 cpu_beg = entry->res->start;
> >
> > -             size += entry->res->end - entry->res->start + 1;
> > +             size = resource_size(entry->res);
> > +             tot_size += size;
> >               if (pcie_beg < lowest_pcie_addr)
> >                       lowest_pcie_addr = pcie_beg;
> > +             /*
> > +              * 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)
> > +                     set_bar(b++, &n, size, cpu_beg, pcie_beg);
> > +
> > +             if (n > pcie->num_inbound)
> > +                     break;
> >       }
> >
> >       if (lowest_pcie_addr == ~(u64)0) {
> > @@ -813,13 +861,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 +883,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 +926,89 @@ 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)) {
> > +     if (!size || (pci_offset & (size - 1)) ||
> > +         (pci_offset < SZ_4G && pci_offset > SZ_2G)) {
> >               dev_err(dev, "Invalid rc_bar2_offset/size: size 0x%llx, off 0x%llx\n",
> > -                     *rc_bar2_size, *rc_bar2_offset);
> > +                     size, pci_offset);
> >               return -EINVAL;
> >       }
> >
> > -     return 0;
> > +     /* Enable BAR2, the inbound window for STB chips */
> > +     set_bar(b++, &n, size, cpu_addr, pci_offset);
> > +
> > +     /*
> > +      * Disable BAR3.  On some chips presents the same window as BAR2
> > +      * but the data appears in a settable endianness.
> > +      */
> > +     set_bar(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 rc_bar *rc_bars,
> > +                                   int num_rc_bars)
> > +{
> > +     void __iomem *base = pcie->base;
> > +     int i;
> > +
> > +     for (i = 1; i <= num_rc_bars; i++) {
> > +             u64 pci_offset = rc_bars[i].pci_offset;
> > +             u64 cpu_addr = rc_bars[i].cpu_addr;
> > +             u64 size = rc_bars[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(tmp, base + reg_offset);
> > +             /* Write high */
> > +             writel(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(tmp, base + reg_offset);
> > +                     tmp = upper_32_bits(cpu_addr);
> > +                     writel(tmp, base + reg_offset + 4);
> > +             }
> > +     }
> >  }
> >
> >  static int brcm_pcie_setup(struct brcm_pcie *pcie)
> >  {
> > -     u64 rc_bar2_offset, rc_bar2_size;
> > +     struct rc_bar rc_bars[PCIE_BRCM_MAX_RC_BARS];
> >       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;
> > +     int num_out_wins = 0, num_rc_bars = 0;
> > +     int memc;
> >
> >       /* Reset the bridge */
> >       pcie->bridge_sw_init_set(pcie, 1);
> > @@ -933,17 +1057,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_rc_bars = brcm_pcie_get_inbound_wins(pcie, rc_bars);
> > +     if (num_rc_bars < 0)
> > +             return num_rc_bars;
> >
> > -     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);
> > +     set_inbound_win_registers(pcie, rc_bars, num_rc_bars);
> > +
> > +     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 +1088,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 (rc_bars[2].pci_offset >= SZ_4G ||
> > +         (rc_bars[2].size + rc_bars[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;
> > @@ -1516,6 +1626,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    = 3,
> >  };
> >
> >  static const struct pcie_cfg_data bcm7425_cfg = {
> > @@ -1523,6 +1634,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    = 3,
> >  };
> >
> >  static const struct pcie_cfg_data bcm7435_cfg = {
> > @@ -1530,6 +1642,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    = 3,
> >  };
> >
> >  static const struct pcie_cfg_data bcm4908_cfg = {
> > @@ -1537,6 +1650,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    = 3,
> >  };
> >
> >  static const int pcie_offset_bcm7278[] = {
> > @@ -1552,6 +1666,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    = 3,
> >  };
> >
> >  static const struct pcie_cfg_data bcm2711_cfg = {
> > @@ -1559,6 +1674,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    = 3,
> >  };
> >
> >  static const struct pcie_cfg_data bcm7216_cfg = {
> > @@ -1567,6 +1683,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    = 3,
> >  };
> >
> >  static const struct of_device_id brcm_pcie_match[] = {
> > @@ -1623,6 +1740,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 = data->num_inbound;
> >
> >       pcie->base = devm_platform_ioremap_resource(pdev, 0);
> >       if (IS_ERR(pcie->base))
> > --
> > 2.17.1
> >
>
>
>
> --
> மணிவண்ணன் சதாசிவம்

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

Re: [PATCH v4 11/12] PCI: brcmstb: Change field name from 'type' to 'model'

[Jim Quinlan]

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

On Thu, Jul 25, 2024 at 12:58 AM Manivannan Sadhasivam
<manivannan.sadhasivam@linaro.org> wrote:
>
> On Tue, Jul 16, 2024 at 05:31:26PM -0400, Jim Quinlan wrote:
> > The 'type' field used in the driver to discern SoC differences is confusing
> > so change it to the more apt 'model'.  We considered using 'family' but
> > this conflicts with Broadcom's conception of a family; for example, 7216a0
> > and 7216b0 chips are both considered separate families as each has multiple
> > derivative product chips based on the original design.
> >
>
> TBH, 'model' is also confusing :) Why can't you just use 'soc' as you are
> referrring to the SoC name.

Hello,

Well, the "model" we assign is not necessarily the same as the SoC.
If a new SoC has the same characteristics as a previous "model", we
will not create a new model but rather use the existing one.   For example,
the bcm7216_cfg structure, which is for the 7216 SoC uses the model "BCM7278".

I agree that this is not crystal clear but using SoC could be
considered misleading.

Regards,
Jim Quinlan
Broadcom STB/CM

>
> - Mani
>
> > Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com>
> > Reviewed-by: Florian Fainelli <florian.fainelli@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 2fe1f2a26697..fa5616a56383 100644
> > --- a/drivers/pci/controller/pcie-brcmstb.c
> > +++ b/drivers/pci/controller/pcie-brcmstb.c
> > @@ -211,7 +211,7 @@ enum {
> >       PCIE_INTR2_CPU_BASE,
> >  };
> >
> > -enum pcie_type {
> > +enum pcie_model {
> >       GENERIC,
> >       BCM7425,
> >       BCM7435,
> > @@ -229,7 +229,7 @@ struct rc_bar {
> >
> >  struct pcie_cfg_data {
> >       const int *offsets;
> > -     const enum pcie_type type;
> > +     const enum pcie_model model;
> >       const bool has_phy;
> >       unsigned int num_inbound;
> >       int (*perst_set)(struct brcm_pcie *pcie, u32 val);
> > @@ -270,7 +270,7 @@ struct brcm_pcie {
> >       u64                     msi_target_addr;
> >       struct brcm_msi         *msi;
> >       const int               *reg_offsets;
> > -     enum pcie_type          type;
> > +     enum pcie_model         model;
> >       struct reset_control    *rescal;
> >       struct reset_control    *perst_reset;
> >       struct reset_control    *bridge;
> > @@ -288,7 +288,7 @@ struct brcm_pcie {
> >
> >  static inline bool is_bmips(const struct brcm_pcie *pcie)
> >  {
> > -     return pcie->type == BCM7435 || pcie->type == BCM7425;
> > +     return pcie->model == BCM7435 || pcie->model == BCM7425;
> >  }
> >
> >  /*
> > @@ -852,7 +852,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->model != BCM7712)
> >               set_bar(b++, &n, 0, 0, 0);
> >
> >       resource_list_for_each_entry(entry, &bridge->dma_ranges) {
> > @@ -869,7 +869,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->model == BCM7712)
> >                       set_bar(b++, &n, size, cpu_beg, pcie_beg);
> >
> >               if (n > pcie->num_inbound)
> > @@ -886,7 +886,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->model == BCM7712)
> >               return n;
> >
> >       ret = of_property_read_variable_u64_array(pcie->np, "brcm,scb-sizes", pcie->memc_size, 1,
> > @@ -1008,7 +1008,7 @@ static void set_inbound_win_registers(struct brcm_pcie *pcie, const struct rc_ba
> >                * 7712:
> >                *     All of their BARs need to be set.
> >                */
> > -             if (pcie->type == BCM7712) {
> > +             if (pcie->model == BCM7712) {
> >                       /* BUS remap register settings */
> >                       reg_offset = brcm_ubus_reg_offset(i);
> >                       tmp = lower_32_bits(cpu_addr) & ~0xfff;
> > @@ -1036,7 +1036,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->model == BCM2711) {
> >               ret = pcie->perst_set(pcie, 1);
> >               if (ret) {
> >                       pcie->bridge_sw_init_set(pcie, 0);
> > @@ -1067,9 +1067,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->model == BCM2711)
> >               burst = 0x0; /* 128 bytes */
> > -     else if (pcie->type == BCM7278)
> > +     else if (pcie->model == BCM7278)
> >               burst = 0x3; /* 512 bytes */
> >       else
> >               burst = 0x2; /* 512 bytes */
> > @@ -1666,7 +1666,7 @@ static const int pcie_offsets_bmips_7425[] = {
> >
> >  static const struct pcie_cfg_data generic_cfg = {
> >       .offsets        = pcie_offsets,
> > -     .type           = GENERIC,
> > +     .model          = GENERIC,
> >       .perst_set      = brcm_pcie_perst_set_generic,
> >       .bridge_sw_init_set = brcm_pcie_bridge_sw_init_set_generic,
> >       .num_inbound    = 3,
> > @@ -1674,7 +1674,7 @@ static const struct pcie_cfg_data generic_cfg = {
> >
> >  static const struct pcie_cfg_data bcm7425_cfg = {
> >       .offsets        = pcie_offsets_bmips_7425,
> > -     .type           = BCM7425,
> > +     .model          = BCM7425,
> >       .perst_set      = brcm_pcie_perst_set_generic,
> >       .bridge_sw_init_set = brcm_pcie_bridge_sw_init_set_generic,
> >       .num_inbound    = 3,
> > @@ -1682,7 +1682,7 @@ static const struct pcie_cfg_data bcm7425_cfg = {
> >
> >  static const struct pcie_cfg_data bcm7435_cfg = {
> >       .offsets        = pcie_offsets,
> > -     .type           = BCM7435,
> > +     .model          = BCM7435,
> >       .perst_set      = brcm_pcie_perst_set_generic,
> >       .bridge_sw_init_set = brcm_pcie_bridge_sw_init_set_generic,
> >       .num_inbound    = 3,
> > @@ -1690,7 +1690,7 @@ static const struct pcie_cfg_data bcm7435_cfg = {
> >
> >  static const struct pcie_cfg_data bcm4908_cfg = {
> >       .offsets        = pcie_offsets,
> > -     .type           = BCM4908,
> > +     .model          = BCM4908,
> >       .perst_set      = brcm_pcie_perst_set_4908,
> >       .bridge_sw_init_set = brcm_pcie_bridge_sw_init_set_generic,
> >       .num_inbound    = 3,
> > @@ -1706,7 +1706,7 @@ static const int pcie_offset_bcm7278[] = {
> >
> >  static const struct pcie_cfg_data bcm7278_cfg = {
> >       .offsets        = pcie_offset_bcm7278,
> > -     .type           = BCM7278,
> > +     .model          = BCM7278,
> >       .perst_set      = brcm_pcie_perst_set_7278,
> >       .bridge_sw_init_set = brcm_pcie_bridge_sw_init_set_7278,
> >       .num_inbound    = 3,
> > @@ -1714,7 +1714,7 @@ static const struct pcie_cfg_data bcm7278_cfg = {
> >
> >  static const struct pcie_cfg_data bcm2711_cfg = {
> >       .offsets        = pcie_offsets,
> > -     .type           = BCM2711,
> > +     .model          = BCM2711,
> >       .perst_set      = brcm_pcie_perst_set_generic,
> >       .bridge_sw_init_set = brcm_pcie_bridge_sw_init_set_generic,
> >       .num_inbound    = 3,
> > @@ -1722,7 +1722,7 @@ static const struct pcie_cfg_data bcm2711_cfg = {
> >
> >  static const struct pcie_cfg_data bcm7216_cfg = {
> >       .offsets        = pcie_offset_bcm7278,
> > -     .type           = BCM7278,
> > +     .model          = BCM7278,
> >       .perst_set      = brcm_pcie_perst_set_7278,
> >       .bridge_sw_init_set = brcm_pcie_bridge_sw_init_set_7278,
> >       .has_phy        = true,
> > @@ -1779,7 +1779,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->model = data->model;
> >       pcie->perst_set = data->perst_set;
> >       pcie->bridge_sw_init_set = data->bridge_sw_init_set;
> >       pcie->has_phy = data->has_phy;
> > @@ -1848,7 +1848,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->model == 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;
> > @@ -1863,7 +1863,7 @@ static int brcm_pcie_probe(struct platform_device *pdev)
> >               }
> >       }
> >
> > -     bridge->ops = pcie->type == BCM7425 ? &brcm7425_pcie_ops : &brcm_pcie_ops;
> > +     bridge->ops = pcie->model == BCM7425 ? &brcm7425_pcie_ops : &brcm_pcie_ops;
> >       bridge->sysdata = pcie;
> >
> >       platform_set_drvdata(pdev, pcie);
> > --
> > 2.17.1
> >
>
>
>
> --
> மணிவண்ணன் சதாசிவம்

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

Re: [PATCH v4 03/12] PCI: brcmstb: Use common error handling code in brcm_pcie_probe()

[Manivannan Sadhasivam]

On Thu, Jul 25, 2024 at 03:45:59PM -0400, Jim Quinlan wrote:
> On Thu, Jul 25, 2024 at 12:31 AM Manivannan Sadhasivam
> <manivannan.sadhasivam@linaro.org> wrote:
> >
> > On Tue, Jul 16, 2024 at 05:31:18PM -0400, Jim Quinlan wrote:
> > > o Move the clk_prepare_enable() below the resource allocations.
> > > o Add a jump target (clk_out) so that a bit of exception handling can be
> > >   better reused at the end of this function implementation.
> > >
> > > 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 | 29 +++++++++++++++------------
> > >  1 file changed, 16 insertions(+), 13 deletions(-)
> > >
> > > diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
> > > index c08683febdd4..c257434edc08 100644
> > > --- a/drivers/pci/controller/pcie-brcmstb.c
> > > +++ b/drivers/pci/controller/pcie-brcmstb.c
> > > @@ -1613,31 +1613,30 @@ 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 = clk_prepare_enable(pcie->clk);
> > > +     if (ret) {
> > > +             dev_err(&pdev->dev, "could not enable clock\n");
> > > +             return ret;
> > >       }
> > >
> > >       ret = reset_control_reset(pcie->rescal);
> > > -     if (ret)
> > > +     if (ret) {
> > >               dev_err(&pdev->dev, "failed to deassert 'rescal'\n");
> > > +             goto clk_out;
> >
> > Please use a descriptive name for the err labels. Here this err path disables
> > and unprepares the clk, so use 'clk_disable_unprepare'.
> ack
> >
> > > +     }
> > >
> > >       ret = brcm_phy_start(pcie);
> > >       if (ret) {
> > >               reset_control_rearm(pcie->rescal);
> > > -             clk_disable_unprepare(pcie->clk);
> > > -             return ret;
> > > +             goto clk_out;
> > >       }
> > >
> > >       ret = brcm_pcie_setup(pcie);
> > > @@ -1676,6 +1675,10 @@ static int brcm_pcie_probe(struct platform_device *pdev)
> > >
> > >       return 0;
> > >
> > > +clk_out:
> > > +     clk_disable_unprepare(pcie->clk);
> > > +     return ret;
> > > +
> >
> > This is leaking the resources. Move this new label below 'fail'.
> What resources is it leaking?  At "clk_out" the return value will be negative
> and only managed resources have been allocated at that juncture.
> 

Right, but what about the err path below this one? If that path is taken, then
clks won't be released, right?

It is not a good design to return from each err labels. There should be only one
return for all err labels at the end and those labels need to be in reverse
order w.r.t the actual code.

- Mani

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

Re: [PATCH v4 09/12] PCI: brcmstb: Refactor for chips with many regular inbound BARs

[Manivannan Sadhasivam]

On Thu, Jul 25, 2024 at 04:29:56PM -0400, Jim Quinlan wrote:
> On Thu, Jul 25, 2024 at 12:53 AM Manivannan Sadhasivam
> <manivannan.sadhasivam@linaro.org> wrote:
> >
> > On Tue, Jul 16, 2024 at 05:31:24PM -0400, Jim Quinlan wrote:
> > > Previously, our chips provided three inbound "BARS" 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.  We
> > > typically only used one of these BARs.
> > >
> > > Complicating that BARs 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 have taken a step forward
> > > and now provide multiple inbound BARs.  This works in concert with the
> > > dma-ranges property, where each provided range becomes an inbound BAR.
> > >
> > > This commit provides support for these new chips and their multiple
> > > inbound BARs but also keeps the legacy support for the older system.
> > >
> >
> > BAR belongs to the endpoints not to the RC. How can the RC have 'BARs'? RC can
> > only map endpoint BARs to MEM region. What you are referring to is 'MEM region'
> > maybe?
> 
> Agreed, it is confusing.  Long story short, the HW team gave the
> inbound windows the label "BAR".   We will still have to use their
> register names,

Wow, such an inventive naming :)

> e.g. PCIE_MISC_RC_BAR4_CONFIG_LO, but what I can do is change
> for example "struct rc_bar" to "struct inbound_win" as well as make similar
> changes to the code and function names.
> 
> Let's assume you will be okay with my plan above; if not, please tell
> me what you would prefer.
> 

Yes please. Just keep BAR in the register name and use 'inbound_win' elsewhere.
Even better, add a comment at the top of these register names to clarify that
these refer to inbound windows.

- Mani

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

Re: [PATCH v4 11/12] PCI: brcmstb: Change field name from 'type' to 'model'

[Manivannan Sadhasivam]

On Thu, Jul 25, 2024 at 04:38:12PM -0400, Jim Quinlan wrote:
> On Thu, Jul 25, 2024 at 12:58 AM Manivannan Sadhasivam
> <manivannan.sadhasivam@linaro.org> wrote:
> >
> > On Tue, Jul 16, 2024 at 05:31:26PM -0400, Jim Quinlan wrote:
> > > The 'type' field used in the driver to discern SoC differences is confusing
> > > so change it to the more apt 'model'.  We considered using 'family' but
> > > this conflicts with Broadcom's conception of a family; for example, 7216a0
> > > and 7216b0 chips are both considered separate families as each has multiple
> > > derivative product chips based on the original design.
> > >
> >
> > TBH, 'model' is also confusing :) Why can't you just use 'soc' as you are
> > referrring to the SoC name.
> 
> Hello,
> 
> Well, the "model" we assign is not necessarily the same as the SoC.
> If a new SoC has the same characteristics as a previous "model", we
> will not create a new model but rather use the existing one.   For example,
> the bcm7216_cfg structure, which is for the 7216 SoC uses the model "BCM7278".
> 
> I agree that this is not crystal clear but using SoC could be
> considered misleading.
> 

Ok, thanks for clarifying. Still I think you can use 'soc' prefix.

For naming, how about 'soc_base'? This specifies the SoC baseline used by *this*
Soc.

- Mani

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

Re: [PATCH v4 03/12] PCI: brcmstb: Use common error handling code in brcm_pcie_probe()

[Jim Quinlan]

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

On Fri, Jul 26, 2024 at 1:04 AM Manivannan Sadhasivam
<manivannan.sadhasivam@linaro.org> wrote:
>
> On Thu, Jul 25, 2024 at 03:45:59PM -0400, Jim Quinlan wrote:
> > On Thu, Jul 25, 2024 at 12:31 AM Manivannan Sadhasivam
> > <manivannan.sadhasivam@linaro.org> wrote:
> > >
> > > On Tue, Jul 16, 2024 at 05:31:18PM -0400, Jim Quinlan wrote:
> > > > o Move the clk_prepare_enable() below the resource allocations.
> > > > o Add a jump target (clk_out) so that a bit of exception handling can be
> > > >   better reused at the end of this function implementation.
> > > >
> > > > 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 | 29 +++++++++++++++------------
> > > >  1 file changed, 16 insertions(+), 13 deletions(-)
> > > >
> > > > diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
> > > > index c08683febdd4..c257434edc08 100644
> > > > --- a/drivers/pci/controller/pcie-brcmstb.c
> > > > +++ b/drivers/pci/controller/pcie-brcmstb.c
> > > > @@ -1613,31 +1613,30 @@ 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 = clk_prepare_enable(pcie->clk);
> > > > +     if (ret) {
> > > > +             dev_err(&pdev->dev, "could not enable clock\n");
> > > > +             return ret;
> > > >       }
> > > >
> > > >       ret = reset_control_reset(pcie->rescal);
> > > > -     if (ret)
> > > > +     if (ret) {
> > > >               dev_err(&pdev->dev, "failed to deassert 'rescal'\n");
> > > > +             goto clk_out;
> > >
> > > Please use a descriptive name for the err labels. Here this err path disables
> > > and unprepares the clk, so use 'clk_disable_unprepare'.
> > ack
> > >
> > > > +     }
> > > >
> > > >       ret = brcm_phy_start(pcie);
> > > >       if (ret) {
> > > >               reset_control_rearm(pcie->rescal);
> > > > -             clk_disable_unprepare(pcie->clk);
> > > > -             return ret;
> > > > +             goto clk_out;
> > > >       }
> > > >
> > > >       ret = brcm_pcie_setup(pcie);
> > > > @@ -1676,6 +1675,10 @@ static int brcm_pcie_probe(struct platform_device *pdev)
> > > >
> > > >       return 0;
> > > >
> > > > +clk_out:
> > > > +     clk_disable_unprepare(pcie->clk);
> > > > +     return ret;
> > > > +
> > >
> > > This is leaking the resources. Move this new label below 'fail'.
> > What resources is it leaking?  At "clk_out" the return value will be negative
> > and only managed resources have been allocated at that juncture.
> >
>
> Right, but what about the err path below this one? If that path is taken, then
> clks won't be released, right?
No, that is the same situation.  The clock is originally allocated
with "devm_clk_get_optional()", i.e. it is a managed resource.
 If the probe fails, and it does in both of these error paths,
Linux deallocates the newly formed device structure and all of its resources.
Perhaps I am missing something?

>
> It is not a good design to return from each err labels. There should be only one
> return for all err labels at the end and those labels need to be in reverse
> order w.r.t the actual code.

Agreed.

Regards,
Jim Quinlan
Broadcom STB/CM
>
> - Mani
>
> --
> மணிவண்ணன் சதாசிவம்

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

Re: [PATCH v4 08/12] PCI: brcmstb: Don't conflate the reset rescal with phy ctrl

[Jim Quinlan]

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

On Thu, Jul 25, 2024 at 12:48 AM Manivannan Sadhasivam
<manivannan.sadhasivam@linaro.org> wrote:
>
> On Tue, Jul 16, 2024 at 05:31:23PM -0400, Jim Quinlan wrote:
> > We've been assuming that if an SOC has a "rescal" reset controller that we
> > should automatically invoke brcm_phy_cntl(...).  This will not be true in
> > future SOCs, so we create a bool "has_phy" and adjust the cfg_data
> > appropriately (we need to give 7216 its own cfg_data structure instead of
> > sharing one).
> >
>
> In all commit messages, use imperative tone as per kernel documentation:
>
> "Describe your changes in imperative mood, e.g. "make xyzzy do frotz"
> instead of "[This patch] makes xyzzy do frotz" or "[I] changed xyzzy
> to do frotz", as if you are giving orders to the codebase to change
> its behaviour."

ack

>
> > 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 | 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 dfb404748ad8..8ab5a8ca05b4 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;
>
> 'has_phy' means the controller supports PHY and the new SoC doesn't have a PHY
> for the controller?

Well they all have a phy but in this case parts of it are exposed to
SW.  Perhaps "has_phy_cntl" would suffice?

Regards,
Jim Quinlan
Broadcom STB/CM
>
> - Mani
>
> --
> மணிவண்ணன் சதாசிவம்

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

Re: [PATCH v4 03/12] PCI: brcmstb: Use common error handling code in brcm_pcie_probe()

[Manivannan Sadhasivam]

On Fri, Jul 26, 2024 at 02:34:54PM -0400, Jim Quinlan wrote:
> On Fri, Jul 26, 2024 at 1:04 AM Manivannan Sadhasivam
> <manivannan.sadhasivam@linaro.org> wrote:
> >
> > On Thu, Jul 25, 2024 at 03:45:59PM -0400, Jim Quinlan wrote:
> > > On Thu, Jul 25, 2024 at 12:31 AM Manivannan Sadhasivam
> > > <manivannan.sadhasivam@linaro.org> wrote:
> > > >
> > > > On Tue, Jul 16, 2024 at 05:31:18PM -0400, Jim Quinlan wrote:
> > > > > o Move the clk_prepare_enable() below the resource allocations.
> > > > > o Add a jump target (clk_out) so that a bit of exception handling can be
> > > > >   better reused at the end of this function implementation.
> > > > >
> > > > > 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 | 29 +++++++++++++++------------
> > > > >  1 file changed, 16 insertions(+), 13 deletions(-)
> > > > >
> > > > > diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
> > > > > index c08683febdd4..c257434edc08 100644
> > > > > --- a/drivers/pci/controller/pcie-brcmstb.c
> > > > > +++ b/drivers/pci/controller/pcie-brcmstb.c
> > > > > @@ -1613,31 +1613,30 @@ 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 = clk_prepare_enable(pcie->clk);
> > > > > +     if (ret) {
> > > > > +             dev_err(&pdev->dev, "could not enable clock\n");
> > > > > +             return ret;
> > > > >       }
> > > > >
> > > > >       ret = reset_control_reset(pcie->rescal);
> > > > > -     if (ret)
> > > > > +     if (ret) {
> > > > >               dev_err(&pdev->dev, "failed to deassert 'rescal'\n");
> > > > > +             goto clk_out;
> > > >
> > > > Please use a descriptive name for the err labels. Here this err path disables
> > > > and unprepares the clk, so use 'clk_disable_unprepare'.
> > > ack
> > > >
> > > > > +     }
> > > > >
> > > > >       ret = brcm_phy_start(pcie);
> > > > >       if (ret) {
> > > > >               reset_control_rearm(pcie->rescal);
> > > > > -             clk_disable_unprepare(pcie->clk);
> > > > > -             return ret;
> > > > > +             goto clk_out;
> > > > >       }
> > > > >
> > > > >       ret = brcm_pcie_setup(pcie);
> > > > > @@ -1676,6 +1675,10 @@ static int brcm_pcie_probe(struct platform_device *pdev)
> > > > >
> > > > >       return 0;
> > > > >
> > > > > +clk_out:
> > > > > +     clk_disable_unprepare(pcie->clk);
> > > > > +     return ret;
> > > > > +
> > > >
> > > > This is leaking the resources. Move this new label below 'fail'.
> > > What resources is it leaking?  At "clk_out" the return value will be negative
> > > and only managed resources have been allocated at that juncture.
> > >
> >
> > Right, but what about the err path below this one? If that path is taken, then
> > clks won't be released, right?
> No, that is the same situation.  The clock is originally allocated
> with "devm_clk_get_optional()", i.e. it is a managed resource.
>  If the probe fails, and it does in both of these error paths,
> Linux deallocates the newly formed device structure and all of its resources.
> Perhaps I am missing something?
> 

No, I missed the fact that __brcm_pcie_remove() is freeing all resources. But
grouping all release functions in a single helper and using it in multiple err
paths even when the err path need not release everything the helper is
releasing, warrants trouble.

- Mani

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

Re: [PATCH v4 03/12] PCI: brcmstb: Use common error handling code in brcm_pcie_probe()

[Jim Quinlan]

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

On Sat, Jul 27, 2024 at 2:40 AM Manivannan Sadhasivam
<manivannan.sadhasivam@linaro.org> wrote:
>
> On Fri, Jul 26, 2024 at 02:34:54PM -0400, Jim Quinlan wrote:
> > On Fri, Jul 26, 2024 at 1:04 AM Manivannan Sadhasivam
> > <manivannan.sadhasivam@linaro.org> wrote:
> > >
> > > On Thu, Jul 25, 2024 at 03:45:59PM -0400, Jim Quinlan wrote:
> > > > On Thu, Jul 25, 2024 at 12:31 AM Manivannan Sadhasivam
> > > > <manivannan.sadhasivam@linaro.org> wrote:
> > > > >
> > > > > On Tue, Jul 16, 2024 at 05:31:18PM -0400, Jim Quinlan wrote:
> > > > > > o Move the clk_prepare_enable() below the resource allocations.
> > > > > > o Add a jump target (clk_out) so that a bit of exception handling can be
> > > > > >   better reused at the end of this function implementation.
> > > > > >
> > > > > > 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 | 29 +++++++++++++++------------
> > > > > >  1 file changed, 16 insertions(+), 13 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
> > > > > > index c08683febdd4..c257434edc08 100644
> > > > > > --- a/drivers/pci/controller/pcie-brcmstb.c
> > > > > > +++ b/drivers/pci/controller/pcie-brcmstb.c
> > > > > > @@ -1613,31 +1613,30 @@ 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 = clk_prepare_enable(pcie->clk);
> > > > > > +     if (ret) {
> > > > > > +             dev_err(&pdev->dev, "could not enable clock\n");
> > > > > > +             return ret;
> > > > > >       }
> > > > > >
> > > > > >       ret = reset_control_reset(pcie->rescal);
> > > > > > -     if (ret)
> > > > > > +     if (ret) {
> > > > > >               dev_err(&pdev->dev, "failed to deassert 'rescal'\n");
> > > > > > +             goto clk_out;
> > > > >
> > > > > Please use a descriptive name for the err labels. Here this err path disables
> > > > > and unprepares the clk, so use 'clk_disable_unprepare'.
> > > > ack
> > > > >
> > > > > > +     }
> > > > > >
> > > > > >       ret = brcm_phy_start(pcie);
> > > > > >       if (ret) {
> > > > > >               reset_control_rearm(pcie->rescal);
> > > > > > -             clk_disable_unprepare(pcie->clk);
> > > > > > -             return ret;
> > > > > > +             goto clk_out;
> > > > > >       }
> > > > > >
> > > > > >       ret = brcm_pcie_setup(pcie);
> > > > > > @@ -1676,6 +1675,10 @@ static int brcm_pcie_probe(struct platform_device *pdev)
> > > > > >
> > > > > >       return 0;
> > > > > >
> > > > > > +clk_out:
> > > > > > +     clk_disable_unprepare(pcie->clk);
> > > > > > +     return ret;
> > > > > > +
> > > > >
> > > > > This is leaking the resources. Move this new label below 'fail'.
> > > > What resources is it leaking?  At "clk_out" the return value will be negative
> > > > and only managed resources have been allocated at that juncture.
> > > >
> > >
> > > Right, but what about the err path below this one? If that path is taken, then
> > > clks won't be released, right?
> > No, that is the same situation.  The clock is originally allocated
> > with "devm_clk_get_optional()", i.e. it is a managed resource.
> >  If the probe fails, and it does in both of these error paths,
> > Linux deallocates the newly formed device structure and all of its resources.
> > Perhaps I am missing something?
> >
>
> No, I missed the fact that __brcm_pcie_remove() is freeing all resources. But
> grouping all release functions in a single helper and using it in multiple err
> paths even when the err path need not release everything the helper is
> releasing, warrants trouble.

Got it, I will address this.

Regards,
Jim Quinlan
Broadcom STB/CM
>
> - Mani
>
> --
> மணிவண்ணன் சதாசிவம்

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

Re: [PATCH v4 05/12] PCI: brcmstb: Use swinit reset if available

[Jim Quinlan]

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

On Thu, Jul 25, 2024 at 12:39 AM Manivannan Sadhasivam
<manivannan.sadhasivam@linaro.org> wrote:
>
> On Tue, Jul 16, 2024 at 05:31:20PM -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: Stanimir Varbanov <svarbanov@suse.de>
> > Reviewed-by: Florian Fainelli <florian.fainelli@broadcom.com>
> > ---
> >  drivers/pci/controller/pcie-brcmstb.c | 16 ++++++++++++++++
> >  1 file changed, 16 insertions(+)
> >
> > diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
> > index 92816d8d215a..4dc2ff7f3167 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;
> > +     struct reset_control    *swinit;
>
> Same comment as previous patch.
>
> >       int                     num_memc;
> >       u64                     memc_size[PCIE_BRCM_MAX_MEMC];
> >       u32                     hw_rev;
> > @@ -1633,12 +1634,27 @@ static int brcm_pcie_probe(struct platform_device *pdev)
> >       if (IS_ERR(pcie->bridge))
> >               return PTR_ERR(pcie->bridge);
> >
> > +     pcie->swinit = devm_reset_control_get_optional_exclusive(&pdev->dev, "swinit");
> > +     if (IS_ERR(pcie->swinit))
> > +             return PTR_ERR(pcie->swinit);
> > +
> >       ret = clk_prepare_enable(pcie->clk);
> >       if (ret) {
> >               dev_err(&pdev->dev, "could not enable clock\n");
> >               return ret;
> >       }
> >
> > +     ret = reset_control_assert(pcie->swinit);
> > +     if (ret) {
> > +             dev_err_probe(&pdev->dev, ret, "could not assert reset 'swinit'\n");
> > +             goto clk_out;
> > +     }
>
> No delay required?

Unfortunately I am waiting for HW to answer this.

Regards,
Jim Quinlan
Broadcom STB/CM
>
> - Mani
>
> --
> மணிவண்ணன் சதாசிவம்

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