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

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

[Jim Quinlan]

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

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

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

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


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


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

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

Jim Quinlan (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 windows
  PCI: brcmstb: Check return value of all reset_control_xxx calls
  PCI: brcmstb: Change field name from 'type' to 'soc_base'
  PCI: brcmstb: Enable 7712 SOCs

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


base-commit: 94ede2a3e9135764736221c080ac7c0ad993dc2d
-- 
2.17.1

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

[Jim Quinlan]

o Change order of the compatible strings to be alphabetical
o Use "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>
---
 .../devicetree/bindings/pci/brcm,stb-pcie.yaml | 18 +++++++++++-------
 1 file changed, 11 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..7d2552192153 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,12 @@ properties:
       minItems: 1
       maxItems: 3
 
+  resets:
+    maxItems: 1
+
+  reset-names:
+    maxItems: 1
+
 required:
   - compatible
   - reg
@@ -118,8 +124,7 @@ allOf:
     then:
       properties:
         resets:
-          items:
-            - description: reset controller handling the PERST# signal
+          maxItems: 1
 
         reset-names:
           items:
@@ -136,8 +141,7 @@ allOf:
     then:
       properties:
         resets:
-          items:
-            - description: phandle pointing to the RESCAL reset controller
+          maxItems: 1
 
         reset-names:
           items:
-- 
2.17.1

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

[Florian Fainelli]

On 7/31/24 15:28, Jim Quinlan wrote:
> o Change order of the compatible strings to be alphabetical
> o Use "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>

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

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

[Krzysztof Kozlowski]

On 01/08/2024 00:28, Jim Quinlan wrote:
> o Change order of the compatible strings to be alphabetical
> o Use "maxItems" where needed.

I asked at v3 and then in v4 about splitting this. You never responded
to that comment, so sorry I won't be repeating the same thing in v5.

NAK.

Best regards,
Krzysztof

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

[Jim Quinlan]

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

On Fri, Aug 2, 2024 at 2:43 AM Krzysztof Kozlowski <krzk@kernel.org> wrote:
>
> On 01/08/2024 00:28, Jim Quinlan wrote:
> > o Change order of the compatible strings to be alphabetical
> > o Use "maxItems" where needed.
>
> I asked at v3 and then in v4 about splitting this. You never responded
> to that comment, so sorry I won't be repeating the same thing in v5.

I'm sorry Krzyszof, but I just reviewed your responses in V3 and V4
and I can't find you saying anything about splitting off the above two
bullet points.  Perhaps I am somehow losing email responses but all I
see is this in V3 is the following, where you ask me to do a squash,
not a commit:

    [JQ] o Change order of the compatible strings to be alphabetical
    []KK] That's a cleanup. You can squash it with a previous patch.

Now you did say in V3

    [JQ] o Describe resets/reset-names before using them in rules
    [KK] That's a new commit.

but this bullet item does not relate to the bullet points you have
highlighted in this email.    As for your responses to V4, I don't see
anything about splitting anything.  Again, perhaps I have somehow
missed an email.

Rather than do more round trips of email, can you confirm that this is
what you want:

A new "dt bindings" commit that only includes the changes of
    o Change order of the compatible strings to be alphabetical
    o Use "maxItems" where needed.

If the above is not what you want, please tell me unequivocally what
you would like changed, even if you think you are repeating yourself.

Regards and thanks,
Jim Quinlan
Broadcom STB/CM



>
> NAK.
>
> Best regards,
> Krzysztof
>

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

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

[Krzysztof Kozlowski]

On 13/08/2024 00:07, Jim Quinlan wrote:
> On Fri, Aug 2, 2024 at 2:43 AM Krzysztof Kozlowski <krzk@kernel.org> wrote:
>>
>> On 01/08/2024 00:28, Jim Quinlan wrote:
>>> o Change order of the compatible strings to be alphabetical
>>> o Use "maxItems" where needed.
>>
>> I asked at v3 and then in v4 about splitting this. You never responded
>> to that comment, so sorry I won't be repeating the same thing in v5.
> 
> I'm sorry Krzyszof, but I just reviewed your responses in V3 and V4
> and I can't find you saying anything about splitting off the above two
> bullet points.  Perhaps I am somehow losing email responses but all I
> see is this in V3 is the following, where you ask me to do a squash,
> not a commit:
> 
>     [JQ] o Change order of the compatible strings to be alphabetical
>     []KK] That's a cleanup. You can squash it with a previous patch.
> 
> Now you did say in V3
> 
>     [JQ] o Describe resets/reset-names before using them in rules
>     [KK] That's a new commit.
> 
> but this bullet item does not relate to the bullet points you have
> highlighted in this email.    As for your responses to V4, I don't see

It exactly relates to the quoted part. The comment is ALWAYS under exact
part of your patch being questioned/commented.

So first I said one part is cleanup and should be moved away. Then I
explained that this part is a NEW COMMIT. New, so one more, different.

I understand that this was not clear, but you never came with a question
what did I mean.

Best regards,
Krzysztof

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

[Jim Quinlan]

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

On Tue, Aug 13, 2024 at 4:27 AM Krzysztof Kozlowski <krzk@kernel.org> wrote:
>
> On 13/08/2024 00:07, Jim Quinlan wrote:
> > On Fri, Aug 2, 2024 at 2:43 AM Krzysztof Kozlowski <krzk@kernel.org> wrote:
> >>
> >> On 01/08/2024 00:28, Jim Quinlan wrote:
> >>> o Change order of the compatible strings to be alphabetical
> >>> o Use "maxItems" where needed.
> >>
> >> I asked at v3 and then in v4 about splitting this. You never responded
> >> to that comment, so sorry I won't be repeating the same thing in v5.
> >
> > I'm sorry Krzyszof, but I just reviewed your responses in V3 and V4
> > and I can't find you saying anything about splitting off the above two
> > bullet points.  Perhaps I am somehow losing email responses but all I
> > see is this in V3 is the following, where you ask me to do a squash,
> > not a commit:
> >
> >     [JQ] o Change order of the compatible strings to be alphabetical
> >     []KK] That's a cleanup. You can squash it with a previous patch.
> >
> > Now you did say in V3
> >
> >     [JQ] o Describe resets/reset-names before using them in rules
> >     [KK] That's a new commit.
> >
> > but this bullet item does not relate to the bullet points you have
> > highlighted in this email.    As for your responses to V4, I don't see
>
> It exactly relates to the quoted part. The comment is ALWAYS under exact
> part of your patch being questioned/commented.
>
> So first I said one part is cleanup and should be moved away. Then I
> explained that this part is a NEW COMMIT. New, so one more, different.
>
> I understand that this was not clear, but you never came with a question
> what did I mean.

Hello Krzysztof,

Sorry to bother you again but I want this to be clear.  This commit
contains the changes:

1. alphabetical order of compatible strings
2. maxItems
3.  maintainer change

Do you want  me to split off 3 since it is not a "cleanup"?  If not,
please specify.

Regards,
Jim Quinlan
Broadcom STB/CM

>
> Best regards,
> Krzysztof
>

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

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

[Krzysztof Kozlowski]

On 14/08/2024 19:35, Jim Quinlan wrote:
>> I understand that this was not clear, but you never came with a question
>> what did I mean.
> 
> Hello Krzysztof,
> 
> Sorry to bother you again but I want this to be clear.  This commit
> contains the changes:
> 
> 1. alphabetical order of compatible strings
> 2. maxItems

Only this one to new commit, because this is a functional change.

> 3.  maintainer change

All others are rather trivial or organizational, so they could stay in
one commit.

> 
> Do you want  me to split off 3 since it is not a "cleanup"?  If not,
> please specify.
> 
Best regards,
Krzysztof

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

[Jim Quinlan]

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

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

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

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

[Florian Fainelli]

On 7/31/24 15:28, Jim Quinlan wrote:
> Add description for the 7712 SoC, a Broadcom STB sibling chip of the RPi 5.
> The 7712 uses three reset controllers: rescal, for phy reset calibration;
> bridge, for the bridge between the PCIe bus and the memory bus; and swinit,
> which is a "soft" initialization of the PCIe HW.
> 
> Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com>

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

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

[Krzysztof Kozlowski]

On 01/08/2024 00:28, Jim Quinlan wrote:
> Add description for the 7712 SoC, a Broadcom STB sibling chip of the RPi 5.
> The 7712 uses three reset controllers: rescal, for phy reset calibration;
> bridge, for the bridge between the PCIe bus and the memory bus; and swinit,
> which is a "soft" initialization of the PCIe HW.
> 
> Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com>

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

Best regards,
Krzysztof

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

[Jim Quinlan]

o Move the clk_prepare_enable() below the resource allocations.
o Move the clk_prepare_enable() out of __brcm_pcie_remove() but
  add it to the end of brcm_pcie_remove().
o Add a jump target (clk_disable_unprepare) so that a bit of exception
  handling can be better reused at the end of this function implementation.
o Use dev_err_probe() where it makes sense.

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

diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
index c08683febdd4..7595e7009192 100644
--- a/drivers/pci/controller/pcie-brcmstb.c
+++ b/drivers/pci/controller/pcie-brcmstb.c
@@ -1473,7 +1473,6 @@ static void __brcm_pcie_remove(struct brcm_pcie *pcie)
 		dev_err(pcie->dev, "Could not stop phy\n");
 	if (reset_control_rearm(pcie->rescal))
 		dev_err(pcie->dev, "Could not rearm rescal reset\n");
-	clk_disable_unprepare(pcie->clk);
 }
 
 static void brcm_pcie_remove(struct platform_device *pdev)
@@ -1484,6 +1483,7 @@ static void brcm_pcie_remove(struct platform_device *pdev)
 	pci_stop_root_bus(bridge->bus);
 	pci_remove_root_bus(bridge->bus);
 	__brcm_pcie_remove(pcie);
+	clk_disable_unprepare(pcie->clk);
 }
 
 static const int pcie_offsets[] = {
@@ -1613,31 +1613,26 @@ static int brcm_pcie_probe(struct platform_device *pdev)
 
 	pcie->ssc = of_property_read_bool(np, "brcm,enable-ssc");
 
-	ret = clk_prepare_enable(pcie->clk);
-	if (ret) {
-		dev_err(&pdev->dev, "could not enable clock\n");
-		return ret;
-	}
 	pcie->rescal = devm_reset_control_get_optional_shared(&pdev->dev, "rescal");
-	if (IS_ERR(pcie->rescal)) {
-		clk_disable_unprepare(pcie->clk);
+	if (IS_ERR(pcie->rescal))
 		return PTR_ERR(pcie->rescal);
-	}
+
 	pcie->perst_reset = devm_reset_control_get_optional_exclusive(&pdev->dev, "perst");
-	if (IS_ERR(pcie->perst_reset)) {
-		clk_disable_unprepare(pcie->clk);
+	if (IS_ERR(pcie->perst_reset))
 		return PTR_ERR(pcie->perst_reset);
-	}
 
-	ret = reset_control_reset(pcie->rescal);
+	ret = clk_prepare_enable(pcie->clk);
 	if (ret)
-		dev_err(&pdev->dev, "failed to deassert 'rescal'\n");
+		return dev_err_probe(&pdev->dev, ret, "could not enable clock\n");
+
+	ret = reset_control_reset(pcie->rescal);
+	if (dev_err_probe(&pdev->dev, ret, "failed to deassert 'rescal'\n"))
+		goto clk_disable_unprepare;
 
 	ret = brcm_phy_start(pcie);
 	if (ret) {
 		reset_control_rearm(pcie->rescal);
-		clk_disable_unprepare(pcie->clk);
-		return ret;
+		goto clk_disable_unprepare;
 	}
 
 	ret = brcm_pcie_setup(pcie);
@@ -1654,10 +1649,8 @@ static int brcm_pcie_probe(struct platform_device *pdev)
 	msi_np = of_parse_phandle(pcie->np, "msi-parent", 0);
 	if (pci_msi_enabled() && msi_np == pcie->np) {
 		ret = brcm_pcie_enable_msi(pcie);
-		if (ret) {
-			dev_err(pcie->dev, "probe of internal MSI failed");
+		if (dev_err_probe(pcie->dev, ret, "probe of internal MSI failed"))
 			goto fail;
-		}
 	}
 
 	bridge->ops = pcie->type == BCM7425 ? &brcm7425_pcie_ops : &brcm_pcie_ops;
@@ -1678,6 +1671,9 @@ static int brcm_pcie_probe(struct platform_device *pdev)
 
 fail:
 	__brcm_pcie_remove(pcie);
+clk_disable_unprepare:
+	clk_disable_unprepare(pcie->clk);
+
 	return ret;
 }
 
-- 
2.17.1

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

[Florian Fainelli]

On 7/31/24 15:28, Jim Quinlan wrote:
> o Move the clk_prepare_enable() below the resource allocations.
> o Move the clk_prepare_enable() out of __brcm_pcie_remove() but
>    add it to the end of brcm_pcie_remove().
> o Add a jump target (clk_disable_unprepare) so that a bit of exception
>    handling can be better reused at the end of this function implementation.
> o Use dev_err_probe() where it makes sense.
> 
> Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com>

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

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

[Manivannan Sadhasivam]

On Wed, Jul 31, 2024 at 06:28:17PM -0400, Jim Quinlan wrote:
> o Move the clk_prepare_enable() below the resource allocations.
> o Move the clk_prepare_enable() out of __brcm_pcie_remove() but
>   add it to the end of brcm_pcie_remove().
> o Add a jump target (clk_disable_unprepare) so that a bit of exception
>   handling can be better reused at the end of this function implementation.
> o Use dev_err_probe() where it makes sense.
> 
> Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com>
> ---
>  drivers/pci/controller/pcie-brcmstb.c | 34 ++++++++++++---------------
>  1 file changed, 15 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
> index c08683febdd4..7595e7009192 100644
> --- a/drivers/pci/controller/pcie-brcmstb.c
> +++ b/drivers/pci/controller/pcie-brcmstb.c
> @@ -1473,7 +1473,6 @@ static void __brcm_pcie_remove(struct brcm_pcie *pcie)
>  		dev_err(pcie->dev, "Could not stop phy\n");
>  	if (reset_control_rearm(pcie->rescal))
>  		dev_err(pcie->dev, "Could not rearm rescal reset\n");
> -	clk_disable_unprepare(pcie->clk);
>  }
>  
>  static void brcm_pcie_remove(struct platform_device *pdev)
> @@ -1484,6 +1483,7 @@ static void brcm_pcie_remove(struct platform_device *pdev)
>  	pci_stop_root_bus(bridge->bus);
>  	pci_remove_root_bus(bridge->bus);
>  	__brcm_pcie_remove(pcie);
> +	clk_disable_unprepare(pcie->clk);
>  }
>  
>  static const int pcie_offsets[] = {
> @@ -1613,31 +1613,26 @@ static int brcm_pcie_probe(struct platform_device *pdev)
>  
>  	pcie->ssc = of_property_read_bool(np, "brcm,enable-ssc");
>  
> -	ret = clk_prepare_enable(pcie->clk);
> -	if (ret) {
> -		dev_err(&pdev->dev, "could not enable clock\n");
> -		return ret;
> -	}
>  	pcie->rescal = devm_reset_control_get_optional_shared(&pdev->dev, "rescal");
> -	if (IS_ERR(pcie->rescal)) {
> -		clk_disable_unprepare(pcie->clk);
> +	if (IS_ERR(pcie->rescal))
>  		return PTR_ERR(pcie->rescal);
> -	}
> +
>  	pcie->perst_reset = devm_reset_control_get_optional_exclusive(&pdev->dev, "perst");
> -	if (IS_ERR(pcie->perst_reset)) {
> -		clk_disable_unprepare(pcie->clk);
> +	if (IS_ERR(pcie->perst_reset))
>  		return PTR_ERR(pcie->perst_reset);
> -	}
>  
> -	ret = reset_control_reset(pcie->rescal);
> +	ret = clk_prepare_enable(pcie->clk);
>  	if (ret)
> -		dev_err(&pdev->dev, "failed to deassert 'rescal'\n");
> +		return dev_err_probe(&pdev->dev, ret, "could not enable clock\n");
> +
> +	ret = reset_control_reset(pcie->rescal);
> +	if (dev_err_probe(&pdev->dev, ret, "failed to deassert 'rescal'\n"))
> +		goto clk_disable_unprepare;
>  
>  	ret = brcm_phy_start(pcie);
>  	if (ret) {
>  		reset_control_rearm(pcie->rescal);
> -		clk_disable_unprepare(pcie->clk);
> -		return ret;
> +		goto clk_disable_unprepare;
>  	}
>  
>  	ret = brcm_pcie_setup(pcie);
> @@ -1654,10 +1649,8 @@ static int brcm_pcie_probe(struct platform_device *pdev)
>  	msi_np = of_parse_phandle(pcie->np, "msi-parent", 0);
>  	if (pci_msi_enabled() && msi_np == pcie->np) {
>  		ret = brcm_pcie_enable_msi(pcie);
> -		if (ret) {
> -			dev_err(pcie->dev, "probe of internal MSI failed");
> +		if (dev_err_probe(pcie->dev, ret, "probe of internal MSI failed"))
>  			goto fail;
> -		}
>  	}
>  
>  	bridge->ops = pcie->type == BCM7425 ? &brcm7425_pcie_ops : &brcm_pcie_ops;
> @@ -1678,6 +1671,9 @@ static int brcm_pcie_probe(struct platform_device *pdev)
>  
>  fail:
>  	__brcm_pcie_remove(pcie);
> +clk_disable_unprepare:
> +	clk_disable_unprepare(pcie->clk);
> +

TBH, this is not improving the code readability. __brcm_pcie_remove() used to
free all the resources and now you just moved clk_disable_unprepare() out of it
to save 2 lines in probe(). And you ended up calling clk_disable_unprepare()
separately in brcm_pcie_remove().

So please remove the label and call clk_disable_unprepare() in the error path
(just 2 instances) and continue to use __brcm_pcie_remove() to free all
resources (I would've preferred to have separate error labels instead of calling
__brcm_pcie_remove() though, but not this).

- Mani

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

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

[Jim Quinlan]

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

On Tue, Aug 6, 2024 at 10:53 PM Manivannan Sadhasivam
<manivannan.sadhasivam@linaro.org> wrote:
>
> On Wed, Jul 31, 2024 at 06:28:17PM -0400, Jim Quinlan wrote:
> > o Move the clk_prepare_enable() below the resource allocations.
> > o Move the clk_prepare_enable() out of __brcm_pcie_remove() but
> >   add it to the end of brcm_pcie_remove().
> > o Add a jump target (clk_disable_unprepare) so that a bit of exception
> >   handling can be better reused at the end of this function implementation.
> > o Use dev_err_probe() where it makes sense.
> >
> > Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com>
> > ---
> >  drivers/pci/controller/pcie-brcmstb.c | 34 ++++++++++++---------------
> >  1 file changed, 15 insertions(+), 19 deletions(-)
> >
> > diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
> > index c08683febdd4..7595e7009192 100644
> > --- a/drivers/pci/controller/pcie-brcmstb.c
> > +++ b/drivers/pci/controller/pcie-brcmstb.c
> > @@ -1473,7 +1473,6 @@ static void __brcm_pcie_remove(struct brcm_pcie *pcie)
> >               dev_err(pcie->dev, "Could not stop phy\n");
> >       if (reset_control_rearm(pcie->rescal))
> >               dev_err(pcie->dev, "Could not rearm rescal reset\n");
> > -     clk_disable_unprepare(pcie->clk);
> >  }
> >
> >  static void brcm_pcie_remove(struct platform_device *pdev)
> > @@ -1484,6 +1483,7 @@ static void brcm_pcie_remove(struct platform_device *pdev)
> >       pci_stop_root_bus(bridge->bus);
> >       pci_remove_root_bus(bridge->bus);
> >       __brcm_pcie_remove(pcie);
> > +     clk_disable_unprepare(pcie->clk);
> >  }
> >
> >  static const int pcie_offsets[] = {
> > @@ -1613,31 +1613,26 @@ static int brcm_pcie_probe(struct platform_device *pdev)
> >
> >       pcie->ssc = of_property_read_bool(np, "brcm,enable-ssc");
> >
> > -     ret = clk_prepare_enable(pcie->clk);
> > -     if (ret) {
> > -             dev_err(&pdev->dev, "could not enable clock\n");
> > -             return ret;
> > -     }
> >       pcie->rescal = devm_reset_control_get_optional_shared(&pdev->dev, "rescal");
> > -     if (IS_ERR(pcie->rescal)) {
> > -             clk_disable_unprepare(pcie->clk);
> > +     if (IS_ERR(pcie->rescal))
> >               return PTR_ERR(pcie->rescal);
> > -     }
> > +
> >       pcie->perst_reset = devm_reset_control_get_optional_exclusive(&pdev->dev, "perst");
> > -     if (IS_ERR(pcie->perst_reset)) {
> > -             clk_disable_unprepare(pcie->clk);
> > +     if (IS_ERR(pcie->perst_reset))
> >               return PTR_ERR(pcie->perst_reset);
> > -     }
> >
> > -     ret = reset_control_reset(pcie->rescal);
> > +     ret = clk_prepare_enable(pcie->clk);
> >       if (ret)
> > -             dev_err(&pdev->dev, "failed to deassert 'rescal'\n");
> > +             return dev_err_probe(&pdev->dev, ret, "could not enable clock\n");
> > +
> > +     ret = reset_control_reset(pcie->rescal);
> > +     if (dev_err_probe(&pdev->dev, ret, "failed to deassert 'rescal'\n"))
> > +             goto clk_disable_unprepare;
> >
> >       ret = brcm_phy_start(pcie);
> >       if (ret) {
> >               reset_control_rearm(pcie->rescal);
> > -             clk_disable_unprepare(pcie->clk);
> > -             return ret;
> > +             goto clk_disable_unprepare;
> >       }
> >
> >       ret = brcm_pcie_setup(pcie);
> > @@ -1654,10 +1649,8 @@ static int brcm_pcie_probe(struct platform_device *pdev)
> >       msi_np = of_parse_phandle(pcie->np, "msi-parent", 0);
> >       if (pci_msi_enabled() && msi_np == pcie->np) {
> >               ret = brcm_pcie_enable_msi(pcie);
> > -             if (ret) {
> > -                     dev_err(pcie->dev, "probe of internal MSI failed");
> > +             if (dev_err_probe(pcie->dev, ret, "probe of internal MSI failed"))
> >                       goto fail;
> > -             }
> >       }
> >
> >       bridge->ops = pcie->type == BCM7425 ? &brcm7425_pcie_ops : &brcm_pcie_ops;
> > @@ -1678,6 +1671,9 @@ static int brcm_pcie_probe(struct platform_device *pdev)
> >
> >  fail:
> >       __brcm_pcie_remove(pcie);
> > +clk_disable_unprepare:
> > +     clk_disable_unprepare(pcie->clk);
> > +
>
> TBH, this is not improving the code readability. __brcm_pcie_remove() used to
> free all the resources and now you just moved clk_disable_unprepare() out of it
> to save 2 lines in probe(). And you ended up calling clk_disable_unprepare()
> separately in brcm_pcie_remove().

Actually it saves more lines than that; the "swinit reset" commit adds
two more "goto clk_disable_unprepare" instances.

Nonetheless I will  make the change


Regards,
Jim Quinlan
Broadcom STB/CM




>
> So please remove the label and call clk_disable_unprepare() in the error path
> (just 2 instances) and continue to use __brcm_pcie_remove() to free all
> resources (I would've preferred to have separate error labels instead of calling
> __brcm_pcie_remove() though, but not this).
>
> - Mani
>
> --
> மணிவண்ணன் சதாசிவம்

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

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

[Manivannan Sadhasivam]

On Wed, Jul 31, 2024 at 06:28:17PM -0400, Jim Quinlan wrote:
> o Move the clk_prepare_enable() below the resource allocations.
> o Move the clk_prepare_enable() out of __brcm_pcie_remove() but
>   add it to the end of brcm_pcie_remove().
> o Add a jump target (clk_disable_unprepare) so that a bit of exception
>   handling can be better reused at the end of this function implementation.
> o Use dev_err_probe() where it makes sense.
> 

Thanks for using the imperative tone. It would be nice to have the patch
description as a single paragraph in a continuous manner instead of bullet
points.

- Mani

> Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com>
> ---
>  drivers/pci/controller/pcie-brcmstb.c | 34 ++++++++++++---------------
>  1 file changed, 15 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
> index c08683febdd4..7595e7009192 100644
> --- a/drivers/pci/controller/pcie-brcmstb.c
> +++ b/drivers/pci/controller/pcie-brcmstb.c
> @@ -1473,7 +1473,6 @@ static void __brcm_pcie_remove(struct brcm_pcie *pcie)
>  		dev_err(pcie->dev, "Could not stop phy\n");
>  	if (reset_control_rearm(pcie->rescal))
>  		dev_err(pcie->dev, "Could not rearm rescal reset\n");
> -	clk_disable_unprepare(pcie->clk);
>  }
>  
>  static void brcm_pcie_remove(struct platform_device *pdev)
> @@ -1484,6 +1483,7 @@ static void brcm_pcie_remove(struct platform_device *pdev)
>  	pci_stop_root_bus(bridge->bus);
>  	pci_remove_root_bus(bridge->bus);
>  	__brcm_pcie_remove(pcie);
> +	clk_disable_unprepare(pcie->clk);
>  }
>  
>  static const int pcie_offsets[] = {
> @@ -1613,31 +1613,26 @@ static int brcm_pcie_probe(struct platform_device *pdev)
>  
>  	pcie->ssc = of_property_read_bool(np, "brcm,enable-ssc");
>  
> -	ret = clk_prepare_enable(pcie->clk);
> -	if (ret) {
> -		dev_err(&pdev->dev, "could not enable clock\n");
> -		return ret;
> -	}
>  	pcie->rescal = devm_reset_control_get_optional_shared(&pdev->dev, "rescal");
> -	if (IS_ERR(pcie->rescal)) {
> -		clk_disable_unprepare(pcie->clk);
> +	if (IS_ERR(pcie->rescal))
>  		return PTR_ERR(pcie->rescal);
> -	}
> +
>  	pcie->perst_reset = devm_reset_control_get_optional_exclusive(&pdev->dev, "perst");
> -	if (IS_ERR(pcie->perst_reset)) {
> -		clk_disable_unprepare(pcie->clk);
> +	if (IS_ERR(pcie->perst_reset))
>  		return PTR_ERR(pcie->perst_reset);
> -	}
>  
> -	ret = reset_control_reset(pcie->rescal);
> +	ret = clk_prepare_enable(pcie->clk);
>  	if (ret)
> -		dev_err(&pdev->dev, "failed to deassert 'rescal'\n");
> +		return dev_err_probe(&pdev->dev, ret, "could not enable clock\n");
> +
> +	ret = reset_control_reset(pcie->rescal);
> +	if (dev_err_probe(&pdev->dev, ret, "failed to deassert 'rescal'\n"))
> +		goto clk_disable_unprepare;
>  
>  	ret = brcm_phy_start(pcie);
>  	if (ret) {
>  		reset_control_rearm(pcie->rescal);
> -		clk_disable_unprepare(pcie->clk);
> -		return ret;
> +		goto clk_disable_unprepare;
>  	}
>  
>  	ret = brcm_pcie_setup(pcie);
> @@ -1654,10 +1649,8 @@ static int brcm_pcie_probe(struct platform_device *pdev)
>  	msi_np = of_parse_phandle(pcie->np, "msi-parent", 0);
>  	if (pci_msi_enabled() && msi_np == pcie->np) {
>  		ret = brcm_pcie_enable_msi(pcie);
> -		if (ret) {
> -			dev_err(pcie->dev, "probe of internal MSI failed");
> +		if (dev_err_probe(pcie->dev, ret, "probe of internal MSI failed"))
>  			goto fail;
> -		}
>  	}
>  
>  	bridge->ops = pcie->type == BCM7425 ? &brcm7425_pcie_ops : &brcm_pcie_ops;
> @@ -1678,6 +1671,9 @@ static int brcm_pcie_probe(struct platform_device *pdev)
>  
>  fail:
>  	__brcm_pcie_remove(pcie);
> +clk_disable_unprepare:
> +	clk_disable_unprepare(pcie->clk);
> +
>  	return ret;
>  }
>  
> -- 
> 2.17.1
> 

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

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

[Stanimir Varbanov]

Hi Jim,

On 8/1/24 01:28, Jim Quinlan wrote:
> o Move the clk_prepare_enable() below the resource allocations.
> o Move the clk_prepare_enable() out of __brcm_pcie_remove() but
>   add it to the end of brcm_pcie_remove().
> o Add a jump target (clk_disable_unprepare) so that a bit of exception
>   handling can be better reused at the end of this function implementation.
> o Use dev_err_probe() where it makes sense.

Those dev_err_probe produce these errors on RPi5:

rpi5:~ # dmesg -l err
[    1.004960] brcm-pcie 1000110000.pcie: error 0000000000000000: could
not assert reset 'swinit'
[    1.013812] brcm-pcie 1000110000.pcie: error 0000000000000000: could
not de-assert reset 'swinit' after asserting
[    1.024222] brcm-pcie 1000110000.pcie: error 0000000000000000: failed
to deassert 'rescal'
[    1.533839] brcm-pcie 1000110000.pcie: link down
[    1.627564] brcm-pcie 1000120000.pcie: error 0000000000000000: could
not assert reset 'swinit'
[    1.636415] brcm-pcie 1000120000.pcie: error 0000000000000000: could
not de-assert reset 'swinit' after asserting
[    1.646829] brcm-pcie 1000120000.pcie: error 0000000000000000: failed
to deassert 'rescal'

... as you can see there is no error at all.

~Stan

> 
> Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com>
> ---
>  drivers/pci/controller/pcie-brcmstb.c | 34 ++++++++++++---------------
>  1 file changed, 15 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
> index c08683febdd4..7595e7009192 100644
> --- a/drivers/pci/controller/pcie-brcmstb.c
> +++ b/drivers/pci/controller/pcie-brcmstb.c
> @@ -1473,7 +1473,6 @@ static void __brcm_pcie_remove(struct brcm_pcie *pcie)
>  		dev_err(pcie->dev, "Could not stop phy\n");
>  	if (reset_control_rearm(pcie->rescal))
>  		dev_err(pcie->dev, "Could not rearm rescal reset\n");
> -	clk_disable_unprepare(pcie->clk);
>  }
>  
>  static void brcm_pcie_remove(struct platform_device *pdev)
> @@ -1484,6 +1483,7 @@ static void brcm_pcie_remove(struct platform_device *pdev)
>  	pci_stop_root_bus(bridge->bus);
>  	pci_remove_root_bus(bridge->bus);
>  	__brcm_pcie_remove(pcie);
> +	clk_disable_unprepare(pcie->clk);
>  }
>  
>  static const int pcie_offsets[] = {
> @@ -1613,31 +1613,26 @@ static int brcm_pcie_probe(struct platform_device *pdev)
>  
>  	pcie->ssc = of_property_read_bool(np, "brcm,enable-ssc");
>  
> -	ret = clk_prepare_enable(pcie->clk);
> -	if (ret) {
> -		dev_err(&pdev->dev, "could not enable clock\n");
> -		return ret;
> -	}
>  	pcie->rescal = devm_reset_control_get_optional_shared(&pdev->dev, "rescal");
> -	if (IS_ERR(pcie->rescal)) {
> -		clk_disable_unprepare(pcie->clk);
> +	if (IS_ERR(pcie->rescal))
>  		return PTR_ERR(pcie->rescal);
> -	}
> +
>  	pcie->perst_reset = devm_reset_control_get_optional_exclusive(&pdev->dev, "perst");
> -	if (IS_ERR(pcie->perst_reset)) {
> -		clk_disable_unprepare(pcie->clk);
> +	if (IS_ERR(pcie->perst_reset))
>  		return PTR_ERR(pcie->perst_reset);
> -	}
>  
> -	ret = reset_control_reset(pcie->rescal);
> +	ret = clk_prepare_enable(pcie->clk);
>  	if (ret)
> -		dev_err(&pdev->dev, "failed to deassert 'rescal'\n");
> +		return dev_err_probe(&pdev->dev, ret, "could not enable clock\n");
> +
> +	ret = reset_control_reset(pcie->rescal);
> +	if (dev_err_probe(&pdev->dev, ret, "failed to deassert 'rescal'\n"))
> +		goto clk_disable_unprepare;
>  
>  	ret = brcm_phy_start(pcie);
>  	if (ret) {
>  		reset_control_rearm(pcie->rescal);
> -		clk_disable_unprepare(pcie->clk);
> -		return ret;
> +		goto clk_disable_unprepare;
>  	}
>  
>  	ret = brcm_pcie_setup(pcie);
> @@ -1654,10 +1649,8 @@ static int brcm_pcie_probe(struct platform_device *pdev)
>  	msi_np = of_parse_phandle(pcie->np, "msi-parent", 0);
>  	if (pci_msi_enabled() && msi_np == pcie->np) {
>  		ret = brcm_pcie_enable_msi(pcie);
> -		if (ret) {
> -			dev_err(pcie->dev, "probe of internal MSI failed");
> +		if (dev_err_probe(pcie->dev, ret, "probe of internal MSI failed"))
>  			goto fail;
> -		}
>  	}
>  
>  	bridge->ops = pcie->type == BCM7425 ? &brcm7425_pcie_ops : &brcm_pcie_ops;
> @@ -1678,6 +1671,9 @@ static int brcm_pcie_probe(struct platform_device *pdev)
>  
>  fail:
>  	__brcm_pcie_remove(pcie);
> +clk_disable_unprepare:
> +	clk_disable_unprepare(pcie->clk);
> +
>  	return ret;
>  }
>  

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

[James Quinlan]

On 8/13/24 12:45, Stanimir Varbanov wrote:
> Hi Jim,
>
> On 8/1/24 01:28, Jim Quinlan wrote:
>> o Move the clk_prepare_enable() below the resource allocations.
>> o Move the clk_prepare_enable() out of __brcm_pcie_remove() but
>>    add it to the end of brcm_pcie_remove().
>> o Add a jump target (clk_disable_unprepare) so that a bit of exception
>>    handling can be better reused at the end of this function implementation.
>> o Use dev_err_probe() where it makes sense.PASSWORD
> Those dev_err_probe produce these errors on RPi5:

Hi Stan,

Sorry, I clearly  missed that.  My perception of the dev_err_probe() 
semantics were incorrect -- I thought it didn't print anything at all  
if the "ret" param was zero, which would allow it to be used as an "if" 
conditional, as I am doing. Obviously not the case.

Will fix.

BTW I do not yet possess a CM5 so my testing is done on a CM4, 7712, and 
other STB chips.

Regards and thanks,

Jim Quinlan

Broadcom STB/CM

>
> rpi5:~ # dmesg -l err
> [    1.004960] brcm-pcie 1000110000.pcie: error 0000000000000000: could
> not assert reset 'swinit'
> [    1.013812] brcm-pcie 1000110000.pcie: error 0000000000000000: could
> not de-assert reset 'swinit' after asserting
> [    1.024222] brcm-pcie 1000110000.pcie: error 0000000000000000: failed
> to deassert 'rescal'
> [    1.533839] brcm-pcie 1000110000.pcie: link down
> [    1.627564] brcm-pcie 1000120000.pcie: error 0000000000000000: could
> not assert reset 'swinit'
> [    1.636415] brcm-pcie 1000120000.pcie: error 0000000000000000: could
> not de-assert reset 'swinit' after asserting
> [    1.646829] brcm-pcie 1000120000.pcie: error 0000000000000000: failed
> to deassert 'rescal'
>
> ... as you can see there is no error at all.
>
> ~Stan
>
>> Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com>
>> ---
>>   drivers/pci/controller/pcie-brcmstb.c | 34 ++++++++++++---------------
>>   1 file changed, 15 insertions(+), 19 deletions(-)
>>
>> diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
>> index c08683febdd4..7595e7009192 100644
>> --- a/drivers/pci/controller/pcie-brcmstb.c
>> +++ b/drivers/pci/controller/pcie-brcmstb.c
>> @@ -1473,7 +1473,6 @@ static void __brcm_pcie_remove(struct brcm_pcie *pcie)
>>   		dev_err(pcie->dev, "Could not stop phy\n");
>>   	if (reset_control_rearm(pcie->rescal))
>>   		dev_err(pcie->dev, "Could not rearm rescal reset\n");
>> -	clk_disable_unprepare(pcie->clk);
>>   }
>>   
>>   static void brcm_pcie_remove(struct platform_device *pdev)
>> @@ -1484,6 +1483,7 @@ static void brcm_pcie_remove(struct platform_device *pdev)
>>   	pci_stop_root_bus(bridge->bus);
>>   	pci_remove_root_bus(bridge->bus);
>>   	__brcm_pcie_remove(pcie);
>> +	clk_disable_unprepare(pcie->clk);
>>   }
>>   
>>   static const int pcie_offsets[] = {
>> @@ -1613,31 +1613,26 @@ static int brcm_pcie_probe(struct platform_device *pdev)
>>   
>>   	pcie->ssc = of_property_read_bool(np, "brcm,enable-ssc");
>>   
>> -	ret = clk_prepare_enable(pcie->clk);
>> -	if (ret) {
>> -		dev_err(&pdev->dev, "could not enable clock\n");
>> -		return ret;
>> -	}
>>   	pcie->rescal = devm_reset_control_get_optional_shared(&pdev->dev, "rescal");
>> -	if (IS_ERR(pcie->rescal)) {
>> -		clk_disable_unprepare(pcie->clk);
>> +	if (IS_ERR(pcie->rescal))
>>   		return PTR_ERR(pcie->rescal);
>> -	}
>> +
>>   	pcie->perst_reset = devm_reset_control_get_optional_exclusive(&pdev->dev, "perst");
>> -	if (IS_ERR(pcie->perst_reset)) {
>> -		clk_disable_unprepare(pcie->clk);
>> +	if (IS_ERR(pcie->perst_reset))
>>   		return PTR_ERR(pcie->perst_reset);
>> -	}
>>   
>> -	ret = reset_control_reset(pcie->rescal);
>> +	ret = clk_prepare_enable(pcie->clk);
>>   	if (ret)
>> -		dev_err(&pdev->dev, "failed to deassert 'rescal'\n");
>> +		return dev_err_probe(&pdev->dev, ret, "could not enable clock\n");
>> +
>> +	ret = reset_control_reset(pcie->rescal);
>> +	if (dev_err_probe(&pdev->dev, ret, "failed to deassert 'rescal'\n"))
>> +		goto clk_disable_unprepare;
>>   
>>   	ret = brcm_phy_start(pcie);
>>   	if (ret) {
>>   		reset_control_rearm(pcie->rescal);
>> -		clk_disable_unprepare(pcie->clk);
>> -		return ret;
>> +		goto clk_disable_unprepare;
>>   	}
>>   
>>   	ret = brcm_pcie_setup(pcie);
>> @@ -1654,10 +1649,8 @@ static int brcm_pcie_probe(struct platform_device *pdev)
>>   	msi_np = of_parse_phandle(pcie->np, "msi-parent", 0);
>>   	if (pci_msi_enabled() && msi_np == pcie->np) {
>>   		ret = brcm_pcie_enable_msi(pcie);
>> -		if (ret) {
>> -			dev_err(pcie->dev, "probe of internal MSI failed");
>> +		if (dev_err_probe(pcie->dev, ret, "probe of internal MSI failed"))
>>   			goto fail;
>> -		}
>>   	}
>>   
>>   	bridge->ops = pcie->type == BCM7425 ? &brcm7425_pcie_ops : &brcm_pcie_ops;
>> @@ -1678,6 +1671,9 @@ static int brcm_pcie_probe(struct platform_device *pdev)
>>   
>>   fail:
>>   	__brcm_pcie_remove(pcie);
>> +clk_disable_unprepare:
>> +	clk_disable_unprepare(pcie->clk);
>> +
>>   	return ret;
>>   }
>>   

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

[Jim Quinlan]

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

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

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

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

[Florian Fainelli]

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

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

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

[Manivannan Sadhasivam]

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

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

- Mani

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

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

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

[Stanimir Varbanov]

Hi Jim,

On 8/1/24 01:28, Jim Quinlan wrote:
> The 7712 SOC has a bridge reset which can be described in the device tree.
> Use it if present.  Otherwise, continue to use the legacy method to reset
> the bridge.
> 
> Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com>
> ---
>  drivers/pci/controller/pcie-brcmstb.c | 24 +++++++++++++++++++-----
>  1 file changed, 19 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
> index 7595e7009192..4d68fe318178 100644
> --- a/drivers/pci/controller/pcie-brcmstb.c
> +++ b/drivers/pci/controller/pcie-brcmstb.c
> @@ -265,6 +265,7 @@ struct brcm_pcie {
>  	enum pcie_type		type;
>  	struct reset_control	*rescal;
>  	struct reset_control	*perst_reset;
> +	struct reset_control	*bridge_reset;
>  	int			num_memc;
>  	u64			memc_size[PCIE_BRCM_MAX_MEMC];
>  	u32			hw_rev;
> @@ -732,12 +733,19 @@ static void __iomem *brcm7425_pcie_map_bus(struct pci_bus *bus,
>  
>  static void brcm_pcie_bridge_sw_init_set_generic(struct brcm_pcie *pcie, u32 val)
>  {
> -	u32 tmp, mask =  RGR1_SW_INIT_1_INIT_GENERIC_MASK;
> -	u32 shift = RGR1_SW_INIT_1_INIT_GENERIC_SHIFT;
> +	if (val)
> +		reset_control_assert(pcie->bridge_reset);
> +	else
> +		reset_control_deassert(pcie->bridge_reset);
>  
> -	tmp = readl(pcie->base + PCIE_RGR1_SW_INIT_1(pcie));
> -	tmp = (tmp & ~mask) | ((val << shift) & mask);
> -	writel(tmp, pcie->base + PCIE_RGR1_SW_INIT_1(pcie));
> +	if (!pcie->bridge_reset) {
> +		u32 tmp, mask =  RGR1_SW_INIT_1_INIT_GENERIC_MASK;
> +		u32 shift = RGR1_SW_INIT_1_INIT_GENERIC_SHIFT;
> +
> +		tmp = readl(pcie->base + PCIE_RGR1_SW_INIT_1(pcie));
> +		tmp = (tmp & ~mask) | ((val << shift) & mask);
> +		writel(tmp, pcie->base + PCIE_RGR1_SW_INIT_1(pcie));
> +	}
>  }
>  
>  static void brcm_pcie_bridge_sw_init_set_7278(struct brcm_pcie *pcie, u32 val)
> @@ -1621,10 +1629,16 @@ static int brcm_pcie_probe(struct platform_device *pdev)
>  	if (IS_ERR(pcie->perst_reset))
>  		return PTR_ERR(pcie->perst_reset);
>  
> +	pcie->bridge_reset = devm_reset_control_get_optional_exclusive(&pdev->dev, "bridge");

Shouldn't this be devm_reset_control_get_optional_shared? See more below.

> +	if (IS_ERR(pcie->bridge_reset))
> +		return PTR_ERR(pcie->bridge_reset);
> +
>  	ret = clk_prepare_enable(pcie->clk);
>  	if (ret)
>  		return dev_err_probe(&pdev->dev, ret, "could not enable clock\n");
>  
> +	pcie->bridge_sw_init_set(pcie, 0);

According to reset_control_get_shared description looks like this
.deassert is satisfying the requirements for _shared reset-control API
variant.
Is that the intention to call reset_control_deassert() here?

> +
>  	ret = reset_control_reset(pcie->rescal);
>  	if (dev_err_probe(&pdev->dev, ret, "failed to deassert 'rescal'\n"))
>  		goto clk_disable_unprepare;

~Stan

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

[Jim Quinlan]

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

On Fri, Aug 9, 2024 at 7:16 AM Stanimir Varbanov <svarbanov@suse.de> wrote:
>
> Hi Jim,
>
> On 8/1/24 01:28, Jim Quinlan wrote:
> > The 7712 SOC has a bridge reset which can be described in the device tree.
> > Use it if present.  Otherwise, continue to use the legacy method to reset
> > the bridge.
> >
> > Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com>
> > ---
> >  drivers/pci/controller/pcie-brcmstb.c | 24 +++++++++++++++++++-----
> >  1 file changed, 19 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
> > index 7595e7009192..4d68fe318178 100644
> > --- a/drivers/pci/controller/pcie-brcmstb.c
> > +++ b/drivers/pci/controller/pcie-brcmstb.c
> > @@ -265,6 +265,7 @@ struct brcm_pcie {
> >       enum pcie_type          type;
> >       struct reset_control    *rescal;
> >       struct reset_control    *perst_reset;
> > +     struct reset_control    *bridge_reset;
> >       int                     num_memc;
> >       u64                     memc_size[PCIE_BRCM_MAX_MEMC];
> >       u32                     hw_rev;
> > @@ -732,12 +733,19 @@ static void __iomem *brcm7425_pcie_map_bus(struct pci_bus *bus,
> >
> >  static void brcm_pcie_bridge_sw_init_set_generic(struct brcm_pcie *pcie, u32 val)
> >  {
> > -     u32 tmp, mask =  RGR1_SW_INIT_1_INIT_GENERIC_MASK;
> > -     u32 shift = RGR1_SW_INIT_1_INIT_GENERIC_SHIFT;
> > +     if (val)
> > +             reset_control_assert(pcie->bridge_reset);
> > +     else
> > +             reset_control_deassert(pcie->bridge_reset);
> >
> > -     tmp = readl(pcie->base + PCIE_RGR1_SW_INIT_1(pcie));
> > -     tmp = (tmp & ~mask) | ((val << shift) & mask);
> > -     writel(tmp, pcie->base + PCIE_RGR1_SW_INIT_1(pcie));
> > +     if (!pcie->bridge_reset) {
> > +             u32 tmp, mask =  RGR1_SW_INIT_1_INIT_GENERIC_MASK;
> > +             u32 shift = RGR1_SW_INIT_1_INIT_GENERIC_SHIFT;
> > +
> > +             tmp = readl(pcie->base + PCIE_RGR1_SW_INIT_1(pcie));
> > +             tmp = (tmp & ~mask) | ((val << shift) & mask);
> > +             writel(tmp, pcie->base + PCIE_RGR1_SW_INIT_1(pcie));
> > +     }
> >  }
> >
> >  static void brcm_pcie_bridge_sw_init_set_7278(struct brcm_pcie *pcie, u32 val)
> > @@ -1621,10 +1629,16 @@ static int brcm_pcie_probe(struct platform_device *pdev)
> >       if (IS_ERR(pcie->perst_reset))
> >               return PTR_ERR(pcie->perst_reset);
> >
> > +     pcie->bridge_reset = devm_reset_control_get_optional_exclusive(&pdev->dev, "bridge");
>
> Shouldn't this be devm_reset_control_get_optional_shared? See more below.

Hi Stan,
Yes you are correct, thank you.  It is shared with the driver in
drivers/ata/ahci_brcm.c.  I don't know how this got changed
to exclusive.  Will fix.

Regards,
Jim Quinlan
Broadcom STB/CM

>
> > +     if (IS_ERR(pcie->bridge_reset))
> > +             return PTR_ERR(pcie->bridge_reset);
> > +
> >       ret = clk_prepare_enable(pcie->clk);
> >       if (ret)
> >               return dev_err_probe(&pdev->dev, ret, "could not enable clock\n");
> >
> > +     pcie->bridge_sw_init_set(pcie, 0);
>
> According to reset_control_get_shared description looks like this
> .deassert is satisfying the requirements for _shared reset-control API
> variant.
> Is that the intention to call reset_control_deassert() here?
>
> > +
> >       ret = reset_control_reset(pcie->rescal);
> >       if (dev_err_probe(&pdev->dev, ret, "failed to deassert 'rescal'\n"))
> >               goto clk_disable_unprepare;
>
> ~Stan

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

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

[Jim Quinlan]

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

On Fri, Aug 9, 2024 at 7:16 AM Stanimir Varbanov <svarbanov@suse.de> wrote:
>
> Hi Jim,
>
> On 8/1/24 01:28, Jim Quinlan wrote:
> > The 7712 SOC has a bridge reset which can be described in the device tree.
> > Use it if present.  Otherwise, continue to use the legacy method to reset
> > the bridge.
> >
> > Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com>
> > ---
> >  drivers/pci/controller/pcie-brcmstb.c | 24 +++++++++++++++++++-----
> >  1 file changed, 19 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
> > index 7595e7009192..4d68fe318178 100644
> > --- a/drivers/pci/controller/pcie-brcmstb.c
> > +++ b/drivers/pci/controller/pcie-brcmstb.c
> > @@ -265,6 +265,7 @@ struct brcm_pcie {
> >       enum pcie_type          type;
> >       struct reset_control    *rescal;
> >       struct reset_control    *perst_reset;
> > +     struct reset_control    *bridge_reset;
> >       int                     num_memc;
> >       u64                     memc_size[PCIE_BRCM_MAX_MEMC];
> >       u32                     hw_rev;
> > @@ -732,12 +733,19 @@ static void __iomem *brcm7425_pcie_map_bus(struct pci_bus *bus,
> >
> >  static void brcm_pcie_bridge_sw_init_set_generic(struct brcm_pcie *pcie, u32 val)
> >  {
> > -     u32 tmp, mask =  RGR1_SW_INIT_1_INIT_GENERIC_MASK;
> > -     u32 shift = RGR1_SW_INIT_1_INIT_GENERIC_SHIFT;
> > +     if (val)
> > +             reset_control_assert(pcie->bridge_reset);
> > +     else
> > +             reset_control_deassert(pcie->bridge_reset);
> >
> > -     tmp = readl(pcie->base + PCIE_RGR1_SW_INIT_1(pcie));
> > -     tmp = (tmp & ~mask) | ((val << shift) & mask);
> > -     writel(tmp, pcie->base + PCIE_RGR1_SW_INIT_1(pcie));
> > +     if (!pcie->bridge_reset) {
> > +             u32 tmp, mask =  RGR1_SW_INIT_1_INIT_GENERIC_MASK;
> > +             u32 shift = RGR1_SW_INIT_1_INIT_GENERIC_SHIFT;
> > +
> > +             tmp = readl(pcie->base + PCIE_RGR1_SW_INIT_1(pcie));
> > +             tmp = (tmp & ~mask) | ((val << shift) & mask);
> > +             writel(tmp, pcie->base + PCIE_RGR1_SW_INIT_1(pcie));
> > +     }
> >  }
> >
> >  static void brcm_pcie_bridge_sw_init_set_7278(struct brcm_pcie *pcie, u32 val)
> > @@ -1621,10 +1629,16 @@ static int brcm_pcie_probe(struct platform_device *pdev)
> >       if (IS_ERR(pcie->perst_reset))
> >               return PTR_ERR(pcie->perst_reset);
> >
> > +     pcie->bridge_reset = devm_reset_control_get_optional_exclusive(&pdev->dev, "bridge");
>
> Shouldn't this be devm_reset_control_get_optional_shared? See more below.
>
> > +     if (IS_ERR(pcie->bridge_reset))
> > +             return PTR_ERR(pcie->bridge_reset);
> > +
> >       ret = clk_prepare_enable(pcie->clk);
> >       if (ret)
> >               return dev_err_probe(&pdev->dev, ret, "could not enable clock\n");
> >
> > +     pcie->bridge_sw_init_set(pcie, 0);
>
> According to reset_control_get_shared description looks like this
> .deassert is satisfying the requirements for _shared reset-control API
> variant.
> Is that the intention to call reset_control_deassert() here?

Hi Stan,
Now I'm not sure I understand you.  All of the resets (bridge, perst,
swinit) are exclusive, except for the "rescal" reset, which is shared.

On the exclusive resets I am using reset_control_assert() and
reset_control_deasssert().  On the shared reset I am using
reset_control_rearm() and reset_control_reset().   Why do
you think the calls I am using are incongruent with the shard/exclusive
status?

Regards,
Jim Quinlan
Broadcom STB/CM

>
> > +
> >       ret = reset_control_reset(pcie->rescal);
> >       if (dev_err_probe(&pdev->dev, ret, "failed to deassert 'rescal'\n"))
> >               goto clk_disable_unprepare;
>
> ~Stan

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

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

[Stanimir Varbanov]

Hi Jim,

On 8/12/24 18:46, Jim Quinlan wrote:
> On Fri, Aug 9, 2024 at 7:16 AM Stanimir Varbanov <svarbanov@suse.de> wrote:
>>
>> Hi Jim,
>>
>> On 8/1/24 01:28, Jim Quinlan wrote:
>>> The 7712 SOC has a bridge reset which can be described in the device tree.
>>> Use it if present.  Otherwise, continue to use the legacy method to reset
>>> the bridge.
>>>
>>> Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com>
>>> ---
>>>  drivers/pci/controller/pcie-brcmstb.c | 24 +++++++++++++++++++-----
>>>  1 file changed, 19 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
>>> index 7595e7009192..4d68fe318178 100644
>>> --- a/drivers/pci/controller/pcie-brcmstb.c
>>> +++ b/drivers/pci/controller/pcie-brcmstb.c
>>> @@ -265,6 +265,7 @@ struct brcm_pcie {
>>>       enum pcie_type          type;
>>>       struct reset_control    *rescal;
>>>       struct reset_control    *perst_reset;
>>> +     struct reset_control    *bridge_reset;
>>>       int                     num_memc;
>>>       u64                     memc_size[PCIE_BRCM_MAX_MEMC];
>>>       u32                     hw_rev;
>>> @@ -732,12 +733,19 @@ static void __iomem *brcm7425_pcie_map_bus(struct pci_bus *bus,
>>>
>>>  static void brcm_pcie_bridge_sw_init_set_generic(struct brcm_pcie *pcie, u32 val)
>>>  {
>>> -     u32 tmp, mask =  RGR1_SW_INIT_1_INIT_GENERIC_MASK;
>>> -     u32 shift = RGR1_SW_INIT_1_INIT_GENERIC_SHIFT;
>>> +     if (val)
>>> +             reset_control_assert(pcie->bridge_reset);
>>> +     else
>>> +             reset_control_deassert(pcie->bridge_reset);
>>>
>>> -     tmp = readl(pcie->base + PCIE_RGR1_SW_INIT_1(pcie));
>>> -     tmp = (tmp & ~mask) | ((val << shift) & mask);
>>> -     writel(tmp, pcie->base + PCIE_RGR1_SW_INIT_1(pcie));
>>> +     if (!pcie->bridge_reset) {
>>> +             u32 tmp, mask =  RGR1_SW_INIT_1_INIT_GENERIC_MASK;
>>> +             u32 shift = RGR1_SW_INIT_1_INIT_GENERIC_SHIFT;
>>> +
>>> +             tmp = readl(pcie->base + PCIE_RGR1_SW_INIT_1(pcie));
>>> +             tmp = (tmp & ~mask) | ((val << shift) & mask);
>>> +             writel(tmp, pcie->base + PCIE_RGR1_SW_INIT_1(pcie));
>>> +     }
>>>  }
>>>
>>>  static void brcm_pcie_bridge_sw_init_set_7278(struct brcm_pcie *pcie, u32 val)
>>> @@ -1621,10 +1629,16 @@ static int brcm_pcie_probe(struct platform_device *pdev)
>>>       if (IS_ERR(pcie->perst_reset))
>>>               return PTR_ERR(pcie->perst_reset);
>>>
>>> +     pcie->bridge_reset = devm_reset_control_get_optional_exclusive(&pdev->dev, "bridge");
>>
>> Shouldn't this be devm_reset_control_get_optional_shared? See more below.
>>
>>> +     if (IS_ERR(pcie->bridge_reset))
>>> +             return PTR_ERR(pcie->bridge_reset);
>>> +
>>>       ret = clk_prepare_enable(pcie->clk);
>>>       if (ret)
>>>               return dev_err_probe(&pdev->dev, ret, "could not enable clock\n");
>>>
>>> +     pcie->bridge_sw_init_set(pcie, 0);

^^^ this was missing in v4

>>
>> According to reset_control_get_shared description looks like this
>> .deassert is satisfying the requirements for _shared reset-control API
>> variant.
>> Is that the intention to call reset_control_deassert() here?
> 
> Hi Stan,
> Now I'm not sure I understand you.  All of the resets (bridge, perst,
> swinit) are exclusive, except for the "rescal" reset, which is shared.

... the call of pcie->bridge_sw_init_set(pcie, 0) from brcm_pcie_probe()
was missing in previous v4 and I'm wondering what changed in v5.

And because of _shared reset-control description [1] I decided (wrongly)
that the bridge reset could be _shared_.

> 
> On the exclusive resets I am using reset_control_assert() and
> reset_control_deasssert().  On the shared reset I am using
> reset_control_rearm() and reset_control_reset().   Why do
> you think the calls I am using are incongruent with the shard/exclusive
> status?

I'd argue that rescal reset is not correctly used in brcm_pcie_probe(),
see [2] and according to [1] "Calling reset_control_reset is also not
allowed on a shared reset control."


[1]
https://elixir.bootlin.com/linux/v6.11-rc3/source/include/linux/reset.h#L338

[2]
https://elixir.bootlin.com/linux/v6.11-rc3/source/drivers/pci/controller/pcie-brcmstb.c#L1632

~Stan

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

[James Quinlan]

On 8/12/24 18:28, Stanimir Varbanov wrote:
> Hi Jim,
>
> On 8/12/24 18:46, Jim Quinlan wrote:
>> On Fri, Aug 9, 2024 at 7:16 AM Stanimir Varbanov <svarbanov@suse.de> wrote:
>>> Hi Jim,
>>>
>>> On 8/1/24 01:28, Jim Quinlan wrote:
>>>> The 7712 SOC has a bridge reset which can be described in the device tree.
>>>> Use it if present.  Otherwise, continue to use the legacy method to reset
>>>> the bridge.
>>>>
>>>> Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com>
>>>> ---
>>>>   drivers/pci/controller/pcie-brcmstb.c | 24 +++++++++++++++++++-----
>>>>   1 file changed, 19 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
>>>> index 7595e7009192..4d68fe318178 100644
>>>> --- a/drivers/pci/controller/pcie-brcmstb.c
>>>> +++ b/drivers/pci/controller/pcie-brcmstb.c
>>>> @@ -265,6 +265,7 @@ struct brcm_pcie {
>>>>        enum pcie_type          type;
>>>>        struct reset_control    *rescal;
>>>>        struct reset_control    *perst_reset;
>>>> +     struct reset_control    *bridge_reset;
>>>>        int                     num_memc;
>>>>        u64                     memc_size[PCIE_BRCM_MAX_MEMC];
>>>>        u32                     hw_rev;
>>>> @@ -732,12 +733,19 @@ static void __iomem *brcm7425_pcie_map_bus(struct pci_bus *bus,
>>>>
>>>>   static void brcm_pcie_bridge_sw_init_set_generic(struct brcm_pcie *pcie, u32 val)
>>>>   {
>>>> -     u32 tmp, mask =  RGR1_SW_INIT_1_INIT_GENERIC_MASK;
>>>> -     u32 shift = RGR1_SW_INIT_1_INIT_GENERIC_SHIFT;
>>>> +     if (val)
>>>> +             reset_control_assert(pcie->bridge_reset);
>>>> +     else
>>>> +             reset_control_deassert(pcie->bridge_reset);
>>>>
>>>> -     tmp = readl(pcie->base + PCIE_RGR1_SW_INIT_1(pcie));
>>>> -     tmp = (tmp & ~mask) | ((val << shift) & mask);
>>>> -     writel(tmp, pcie->base + PCIE_RGR1_SW_INIT_1(pcie));
>>>> +     if (!pcie->bridge_reset) {
>>>> +             u32 tmp, mask =  RGR1_SW_INIT_1_INIT_GENERIC_MASK;
>>>> +             u32 shift = RGR1_SW_INIT_1_INIT_GENERIC_SHIFT;
>>>> +
>>>> +             tmp = readl(pcie->base + PCIE_RGR1_SW_INIT_1(pcie));
>>>> +             tmp = (tmp & ~mask) | ((val << shift) & mask);
>>>> +             writel(tmp, pcie->base + PCIE_RGR1_SW_INIT_1(pcie));
>>>> +     }
>>>>   }
>>>>
>>>>   static void brcm_pcie_bridge_sw_init_set_7278(struct brcm_pcie *pcie, u32 val)
>>>> @@ -1621,10 +1629,16 @@ static int brcm_pcie_probe(struct platform_device *pdev)
>>>>        if (IS_ERR(pcie->perst_reset))
>>>>                return PTR_ERR(pcie->perst_reset);
>>>>
>>>> +     pcie->bridge_reset = devm_reset_control_get_optional_exclusive(&pdev->dev, "bridge");
>>> Shouldn't this be devm_reset_control_get_optional_shared? See more below.
>>>
>>>> +     if (IS_ERR(pcie->bridge_reset))
>>>> +             return PTR_ERR(pcie->bridge_reset);
>>>> +
>>>>        ret = clk_prepare_enable(pcie->clk);
>>>>        if (ret)
>>>>                return dev_err_probe(&pdev->dev, ret, "could not enable clock\n");
>>>>
>>>> +     pcie->bridge_sw_init_set(pcie, 0);
> ^^^ this was missing in v4

Hi Stan,

You are correct and I was remiss in not mentioning this in the cover 
letter.  After my changes on V4 I discovered that my driver failed the 
rebind in my unbind/rebind test and this line was required.

>
>>> According to reset_control_get_shared description looks like this
>>> .deassert is satisfying the requirements for _shared reset-control API
>>> variant.
>>> Is that the intention to call reset_control_deassert() here?
>> Hi Stan,
>> Now I'm not sure I understand you.  All of the resets (bridge, perst,
>> swinit) are exclusive, except for the "rescal" reset, which is shared.
> ... the call of pcie->bridge_sw_init_set(pcie, 0) from brcm_pcie_probe()
> was missing in previous v4 and I'm wondering what changed in v5.
>
> And because of _shared reset-control description [1] I decided (wrongly)
> that the bridge reset could be _shared_.
>
>> On the exclusive resets I am using reset_control_assert() and
>> reset_control_deasssert().  On the shared reset I am using
>> reset_control_rearm() and reset_control_reset().   Why do
>> you think the calls I am using are incongruent with the shard/exclusive
>> status?
> I'd argue that rescal reset is not correctly used in brcm_pcie_probe(),
> see [2] and according to [1] "Calling reset_control_reset is also not
> allowed on a shared reset control."

This is interesting because in reset/core.c [1]  the comment says that 
"reset_control_rearm", which is clearly used for shared resets, must be 
paired with calls to "reset_control_reset".

Regards,

Jim Quinlan

Broadcom STB/CM

[1] 
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/reset/core.c?h=v6.11-rc3#n412

>
>
> [1]
> https://elixir.bootlin.com/linux/v6.11-rc3/source/include/linux/reset.h#L338
>
> [2]
> https://elixir.bootlin.com/linux/v6.11-rc3/source/drivers/pci/controller/pcie-brcmstb.c#L1632
>
> ~Stan

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

[Jim Quinlan]

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

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

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

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

[Florian Fainelli]

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

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

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

[Manivannan Sadhasivam]

On Wed, Jul 31, 2024 at 06:28:19PM -0400, Jim Quinlan wrote:
> The 7712 SOC adds a software init reset device for the PCIe HW.
> If found in the DT node, use it.
> 
> Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com>
> ---
>  drivers/pci/controller/pcie-brcmstb.c | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
> 
> diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
> index 4d68fe318178..948fd4d176bc 100644
> --- a/drivers/pci/controller/pcie-brcmstb.c
> +++ b/drivers/pci/controller/pcie-brcmstb.c
> @@ -266,6 +266,7 @@ struct brcm_pcie {
>  	struct reset_control	*rescal;
>  	struct reset_control	*perst_reset;
>  	struct reset_control	*bridge_reset;
> +	struct reset_control	*swinit_reset;
>  	int			num_memc;
>  	u64			memc_size[PCIE_BRCM_MAX_MEMC];
>  	u32			hw_rev;
> @@ -1633,12 +1634,30 @@ static int brcm_pcie_probe(struct platform_device *pdev)
>  	if (IS_ERR(pcie->bridge_reset))
>  		return PTR_ERR(pcie->bridge_reset);
>  
> +	pcie->swinit_reset = devm_reset_control_get_optional_exclusive(&pdev->dev, "swinit");
> +	if (IS_ERR(pcie->swinit_reset))
> +		return PTR_ERR(pcie->swinit_reset);
> +
>  	ret = clk_prepare_enable(pcie->clk);
>  	if (ret)
>  		return dev_err_probe(&pdev->dev, ret, "could not enable clock\n");
>  
>  	pcie->bridge_sw_init_set(pcie, 0);
>  
> +	if (pcie->swinit_reset) {

You already have a callback called 'bridge_sw_init_set', so this 'swinit_reset'
is different from 'bridge_sw_init'? If so, does it make sense to move this into
the callback itself to have all reset sequence in one place?

- Mani

> +		ret = reset_control_assert(pcie->swinit_reset);
> +		if (dev_err_probe(&pdev->dev, ret, "could not assert reset 'swinit'\n"))
> +			goto clk_disable_unprepare;
> +
> +		/* HW team recommends 1us for proper sync and propagation of reset */
> +		udelay(1);
> +
> +		ret = reset_control_deassert(pcie->swinit_reset);
> +		if (dev_err_probe(&pdev->dev, ret,
> +				  "could not de-assert reset 'swinit' after asserting\n"))
> +			goto clk_disable_unprepare;
> +	}
> +
>  	ret = reset_control_reset(pcie->rescal);
>  	if (dev_err_probe(&pdev->dev, ret, "failed to deassert 'rescal'\n"))
>  		goto clk_disable_unprepare;
> -- 
> 2.17.1
> 

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

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

[Jim Quinlan]

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

On Tue, Aug 6, 2024 at 11:03 PM Manivannan Sadhasivam
<manivannan.sadhasivam@linaro.org> wrote:
>
> On Wed, Jul 31, 2024 at 06:28:19PM -0400, Jim Quinlan wrote:
> > The 7712 SOC adds a software init reset device for the PCIe HW.
> > If found in the DT node, use it.
> >
> > Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com>
> > ---
> >  drivers/pci/controller/pcie-brcmstb.c | 19 +++++++++++++++++++
> >  1 file changed, 19 insertions(+)
> >
> > diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
> > index 4d68fe318178..948fd4d176bc 100644
> > --- a/drivers/pci/controller/pcie-brcmstb.c
> > +++ b/drivers/pci/controller/pcie-brcmstb.c
> > @@ -266,6 +266,7 @@ struct brcm_pcie {
> >       struct reset_control    *rescal;
> >       struct reset_control    *perst_reset;
> >       struct reset_control    *bridge_reset;
> > +     struct reset_control    *swinit_reset;
> >       int                     num_memc;
> >       u64                     memc_size[PCIE_BRCM_MAX_MEMC];
> >       u32                     hw_rev;
> > @@ -1633,12 +1634,30 @@ static int brcm_pcie_probe(struct platform_device *pdev)
> >       if (IS_ERR(pcie->bridge_reset))
> >               return PTR_ERR(pcie->bridge_reset);
> >
> > +     pcie->swinit_reset = devm_reset_control_get_optional_exclusive(&pdev->dev, "swinit");
> > +     if (IS_ERR(pcie->swinit_reset))
> > +             return PTR_ERR(pcie->swinit_reset);
> > +
> >       ret = clk_prepare_enable(pcie->clk);
> >       if (ret)
> >               return dev_err_probe(&pdev->dev, ret, "could not enable clock\n");
> >
> >       pcie->bridge_sw_init_set(pcie, 0);
> >
> > +     if (pcie->swinit_reset) {
>
> You already have a callback called 'bridge_sw_init_set', so this 'swinit_reset'
> is different from 'bridge_sw_init'?
Yes.  The swinit_reset is a soft reset of the entire core while
bridge_sw_init reset is only for the bridge to system memory.

If so, does it make sense to move this into
> the callback itself to have all reset sequence in one place?

The order and placement of the resets can sometimes be fragile and I
would prefer to leave them where they are.

Regards,
Jim Quinlan
Broadcom STB/CM

>
> - Mani
>
> > +             ret = reset_control_assert(pcie->swinit_reset);
> > +             if (dev_err_probe(&pdev->dev, ret, "could not assert reset 'swinit'\n"))
> > +                     goto clk_disable_unprepare;
> > +
> > +             /* HW team recommends 1us for proper sync and propagation of reset */
> > +             udelay(1);
> > +
> > +             ret = reset_control_deassert(pcie->swinit_reset);
> > +             if (dev_err_probe(&pdev->dev, ret,
> > +                               "could not de-assert reset 'swinit' after asserting\n"))
> > +                     goto clk_disable_unprepare;
> > +     }
> > +
> >       ret = reset_control_reset(pcie->rescal);
> >       if (dev_err_probe(&pdev->dev, ret, "failed to deassert 'rescal'\n"))
> >               goto clk_disable_unprepare;
> > --
> > 2.17.1
> >
>
> --
> மணிவண்ணன் சதாசிவம்

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

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

[Stanimir Varbanov]

Hi Jim,

On 8/1/24 01:28, 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>
> ---
>  drivers/pci/controller/pcie-brcmstb.c | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
> 
> diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
> index 4d68fe318178..948fd4d176bc 100644
> --- a/drivers/pci/controller/pcie-brcmstb.c
> +++ b/drivers/pci/controller/pcie-brcmstb.c
> @@ -266,6 +266,7 @@ struct brcm_pcie {
>  	struct reset_control	*rescal;
>  	struct reset_control	*perst_reset;
>  	struct reset_control	*bridge_reset;
> +	struct reset_control	*swinit_reset;
>  	int			num_memc;
>  	u64			memc_size[PCIE_BRCM_MAX_MEMC];
>  	u32			hw_rev;
> @@ -1633,12 +1634,30 @@ static int brcm_pcie_probe(struct platform_device *pdev)
>  	if (IS_ERR(pcie->bridge_reset))
>  		return PTR_ERR(pcie->bridge_reset);
>  
> +	pcie->swinit_reset = devm_reset_control_get_optional_exclusive(&pdev->dev, "swinit");
> +	if (IS_ERR(pcie->swinit_reset))
> +		return PTR_ERR(pcie->swinit_reset);
> +
>  	ret = clk_prepare_enable(pcie->clk);
>  	if (ret)
>  		return dev_err_probe(&pdev->dev, ret, "could not enable clock\n");
>  
>  	pcie->bridge_sw_init_set(pcie, 0);
>  
> +	if (pcie->swinit_reset) {
> +		ret = reset_control_assert(pcie->swinit_reset);
> +		if (dev_err_probe(&pdev->dev, ret, "could not assert reset 'swinit'\n"))
> +			goto clk_disable_unprepare;
> +
> +		/* HW team recommends 1us for proper sync and propagation of reset */
> +		udelay(1);

Hmm, shouldn't this delay be part of .assert/.deassert reset_control
driver?  I think this detail is reset-control hw specific and the
consumers does not need to know it.

> +
> +		ret = reset_control_deassert(pcie->swinit_reset);
> +		if (dev_err_probe(&pdev->dev, ret,
> +				  "could not de-assert reset 'swinit' after asserting\n"))
> +			goto clk_disable_unprepare;
> +	}
> +
>  	ret = reset_control_reset(pcie->rescal);
>  	if (dev_err_probe(&pdev->dev, ret, "failed to deassert 'rescal'\n"))
>  		goto clk_disable_unprepare;

~Stan

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

[Jim Quinlan]

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

On Fri, Aug 9, 2024 at 5:53 AM Stanimir Varbanov <svarbanov@suse.de> wrote:
>
> Hi Jim,
>
> On 8/1/24 01:28, 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>
> > ---
> >  drivers/pci/controller/pcie-brcmstb.c | 19 +++++++++++++++++++
> >  1 file changed, 19 insertions(+)
> >
> > diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
> > index 4d68fe318178..948fd4d176bc 100644
> > --- a/drivers/pci/controller/pcie-brcmstb.c
> > +++ b/drivers/pci/controller/pcie-brcmstb.c
> > @@ -266,6 +266,7 @@ struct brcm_pcie {
> >       struct reset_control    *rescal;
> >       struct reset_control    *perst_reset;
> >       struct reset_control    *bridge_reset;
> > +     struct reset_control    *swinit_reset;
> >       int                     num_memc;
> >       u64                     memc_size[PCIE_BRCM_MAX_MEMC];
> >       u32                     hw_rev;
> > @@ -1633,12 +1634,30 @@ static int brcm_pcie_probe(struct platform_device *pdev)
> >       if (IS_ERR(pcie->bridge_reset))
> >               return PTR_ERR(pcie->bridge_reset);
> >
> > +     pcie->swinit_reset = devm_reset_control_get_optional_exclusive(&pdev->dev, "swinit");
> > +     if (IS_ERR(pcie->swinit_reset))
> > +             return PTR_ERR(pcie->swinit_reset);
> > +
> >       ret = clk_prepare_enable(pcie->clk);
> >       if (ret)
> >               return dev_err_probe(&pdev->dev, ret, "could not enable clock\n");
> >
> >       pcie->bridge_sw_init_set(pcie, 0);
> >
> > +     if (pcie->swinit_reset) {
> > +             ret = reset_control_assert(pcie->swinit_reset);
> > +             if (dev_err_probe(&pdev->dev, ret, "could not assert reset 'swinit'\n"))
> > +                     goto clk_disable_unprepare;
> > +
> > +             /* HW team recommends 1us for proper sync and propagation of reset */
> > +             udelay(1);
>
> Hmm, shouldn't this delay be part of .assert/.deassert reset_control
> driver?  I think this detail is reset-control hw specific and the
> consumers does not need to know it.

This was discussed previously.  I pointed out that we use a reset
provider that governs dozens of devices.  The only thing that the
provider could do is to employ a  worst case delay used for all
resets.  This is unacceptable; we have certain devices that may have
to invoke
reset often and require timely action, and we do not want them having
to wait the same amount of worst case delay as for example, a UART device reset.

Further, if I do a "grep reset_control_assert -A 10 drivers"  I see
plenty of existing drivers that use usleep/msleep/udelay after the call to
reset_control_assert, just as I am doing now.

As far as my opinion goes (FWIW) I think the delay is more apt to
be present in the consumer driver and not the provider driver.  To
ascertain this specific delay I had to consult with the PCIe HW team,
not the HW team that implemented the reset controller.

Regards,
Jim Quinlan
Broadcom

>
> > +
> > +             ret = reset_control_deassert(pcie->swinit_reset);
> > +             if (dev_err_probe(&pdev->dev, ret,
> > +                               "could not de-assert reset 'swinit' after asserting\n"))
> > +                     goto clk_disable_unprepare;
> > +     }
> > +
> >       ret = reset_control_reset(pcie->rescal);
> >       if (dev_err_probe(&pdev->dev, ret, "failed to deassert 'rescal'\n"))
> >               goto clk_disable_unprepare;
>
> ~Stan

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

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

[Manivannan Sadhasivam]

On Mon, Aug 12, 2024 at 09:43:46AM -0400, Jim Quinlan wrote:
> On Fri, Aug 9, 2024 at 5:53 AM Stanimir Varbanov <svarbanov@suse.de> wrote:
> >
> > Hi Jim,
> >
> > On 8/1/24 01:28, 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>
> > > ---
> > >  drivers/pci/controller/pcie-brcmstb.c | 19 +++++++++++++++++++
> > >  1 file changed, 19 insertions(+)
> > >
> > > diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
> > > index 4d68fe318178..948fd4d176bc 100644
> > > --- a/drivers/pci/controller/pcie-brcmstb.c
> > > +++ b/drivers/pci/controller/pcie-brcmstb.c
> > > @@ -266,6 +266,7 @@ struct brcm_pcie {
> > >       struct reset_control    *rescal;
> > >       struct reset_control    *perst_reset;
> > >       struct reset_control    *bridge_reset;
> > > +     struct reset_control    *swinit_reset;
> > >       int                     num_memc;
> > >       u64                     memc_size[PCIE_BRCM_MAX_MEMC];
> > >       u32                     hw_rev;
> > > @@ -1633,12 +1634,30 @@ static int brcm_pcie_probe(struct platform_device *pdev)
> > >       if (IS_ERR(pcie->bridge_reset))
> > >               return PTR_ERR(pcie->bridge_reset);
> > >
> > > +     pcie->swinit_reset = devm_reset_control_get_optional_exclusive(&pdev->dev, "swinit");
> > > +     if (IS_ERR(pcie->swinit_reset))
> > > +             return PTR_ERR(pcie->swinit_reset);
> > > +
> > >       ret = clk_prepare_enable(pcie->clk);
> > >       if (ret)
> > >               return dev_err_probe(&pdev->dev, ret, "could not enable clock\n");
> > >
> > >       pcie->bridge_sw_init_set(pcie, 0);
> > >
> > > +     if (pcie->swinit_reset) {
> > > +             ret = reset_control_assert(pcie->swinit_reset);
> > > +             if (dev_err_probe(&pdev->dev, ret, "could not assert reset 'swinit'\n"))
> > > +                     goto clk_disable_unprepare;
> > > +
> > > +             /* HW team recommends 1us for proper sync and propagation of reset */
> > > +             udelay(1);
> >
> > Hmm, shouldn't this delay be part of .assert/.deassert reset_control
> > driver?  I think this detail is reset-control hw specific and the
> > consumers does not need to know it.
> 
> This was discussed previously.  I pointed out that we use a reset
> provider that governs dozens of devices.  The only thing that the
> provider could do is to employ a  worst case delay used for all
> resets.  This is unacceptable; we have certain devices that may have
> to invoke
> reset often and require timely action, and we do not want them having
> to wait the same amount of worst case delay as for example, a UART device reset.
> 
> Further, if I do a "grep reset_control_assert -A 10 drivers"  I see
> plenty of existing drivers that use usleep/msleep/udelay after the call to
> reset_control_assert, just as I am doing now.
> 
> As far as my opinion goes (FWIW) I think the delay is more apt to
> be present in the consumer driver and not the provider driver.  To
> ascertain this specific delay I had to consult with the PCIe HW team,
> not the HW team that implemented the reset controller.
> 

Yeah. Often the reset controller won't have any idea about the delay required
between assert + deassert, unless the reset controller is closely tied to the
peripheral. So keeping the delay in consumer drivers is the right thing to do.

- Mani

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

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

[Stanimir Varbanov]

Hi,

On 8/12/24 16:43, Jim Quinlan wrote:
> On Fri, Aug 9, 2024 at 5:53 AM Stanimir Varbanov <svarbanov@suse.de> wrote:
>>
>> Hi Jim,
>>
>> On 8/1/24 01:28, 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>
>>> ---
>>>  drivers/pci/controller/pcie-brcmstb.c | 19 +++++++++++++++++++
>>>  1 file changed, 19 insertions(+)
>>>
>>> diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
>>> index 4d68fe318178..948fd4d176bc 100644
>>> --- a/drivers/pci/controller/pcie-brcmstb.c
>>> +++ b/drivers/pci/controller/pcie-brcmstb.c
>>> @@ -266,6 +266,7 @@ struct brcm_pcie {
>>>       struct reset_control    *rescal;
>>>       struct reset_control    *perst_reset;
>>>       struct reset_control    *bridge_reset;
>>> +     struct reset_control    *swinit_reset;
>>>       int                     num_memc;
>>>       u64                     memc_size[PCIE_BRCM_MAX_MEMC];
>>>       u32                     hw_rev;
>>> @@ -1633,12 +1634,30 @@ static int brcm_pcie_probe(struct platform_device *pdev)
>>>       if (IS_ERR(pcie->bridge_reset))
>>>               return PTR_ERR(pcie->bridge_reset);
>>>
>>> +     pcie->swinit_reset = devm_reset_control_get_optional_exclusive(&pdev->dev, "swinit");
>>> +     if (IS_ERR(pcie->swinit_reset))
>>> +             return PTR_ERR(pcie->swinit_reset);
>>> +
>>>       ret = clk_prepare_enable(pcie->clk);
>>>       if (ret)
>>>               return dev_err_probe(&pdev->dev, ret, "could not enable clock\n");
>>>
>>>       pcie->bridge_sw_init_set(pcie, 0);
>>>
>>> +     if (pcie->swinit_reset) {
>>> +             ret = reset_control_assert(pcie->swinit_reset);
>>> +             if (dev_err_probe(&pdev->dev, ret, "could not assert reset 'swinit'\n"))
>>> +                     goto clk_disable_unprepare;
>>> +
>>> +             /* HW team recommends 1us for proper sync and propagation of reset */
>>> +             udelay(1);
>>
>> Hmm, shouldn't this delay be part of .assert/.deassert reset_control
>> driver?  I think this detail is reset-control hw specific and the
>> consumers does not need to know it.
> 
> This was discussed previously.  I pointed out that we use a reset

Sorry, I missed that discussion.

> provider that governs dozens of devices.  The only thing that the
> provider could do is to employ a  worst case delay used for all
> resets.  This is unacceptable; we have certain devices that may have
> to invoke
> reset often and require timely action, and we do not want them having
> to wait the same amount of worst case delay as for example, a UART device reset.
> 
> Further, if I do a "grep reset_control_assert -A 10 drivers"  I see
> plenty of existing drivers that use usleep/msleep/udelay after the call to
> reset_control_assert, just as I am doing now.

Yes, I saw them.

> 
> As far as my opinion goes (FWIW) I think the delay is more apt to
> be present in the consumer driver and not the provider driver.  To
> ascertain this specific delay I had to consult with the PCIe HW team,
> not the HW team that implemented the reset controller.
> 

Thank you for the explanation!

~Stan

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

[Jim Quinlan]

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

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

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

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

[Jim Quinlan]

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

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

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

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

[Jim Quinlan]

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

Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com>
Reviewed-by: Stanimir Varbanov <svarbanov@suse.de>
Reviewed-by: Florian Fainelli <florian.fainelli@broadcom.com>
---
 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 1ae66c639186..4659208ae8da 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

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

[Manivannan Sadhasivam]

On Wed, Jul 31, 2024 at 06:28:22PM -0400, Jim Quinlan wrote:
> Add a "has_phy" field indicating that the internal phy has SW control that
> requires configuration.  Some previous chips only required the firing of
> the "rescal" reset controller.  This change requires us to give the 7216
> SoC its own cfg_data structure.
> 
> Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com>

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

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

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

[Jim Quinlan]

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

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

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

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

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

diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
index 4659208ae8da..0ecca3d9576f 100644
--- a/drivers/pci/controller/pcie-brcmstb.c
+++ b/drivers/pci/controller/pcie-brcmstb.c
@@ -75,15 +75,19 @@
 #define PCIE_MEM_WIN0_HI(win)	\
 		PCIE_MISC_CPU_2_PCIE_MEM_WIN0_HI + ((win) * 8)
 
+/*
+ * NOTE: You may see the term "BAR" in a number of register names used by
+ *   this driver.  The term is an artifact of when the HW core was an
+ *   endpoint device (EP).  Now it is a root complex (RC) and anywhere a
+ *   register has the term "BAR" it is related to an inbound window.
+ */
+
+#define PCIE_BRCM_MAX_INBOUND_WINS			16
 #define PCIE_MISC_RC_BAR1_CONFIG_LO			0x402c
 #define  PCIE_MISC_RC_BAR1_CONFIG_LO_SIZE_MASK		0x1f
 
-#define PCIE_MISC_RC_BAR2_CONFIG_LO			0x4034
-#define  PCIE_MISC_RC_BAR2_CONFIG_LO_SIZE_MASK		0x1f
-#define PCIE_MISC_RC_BAR2_CONFIG_HI			0x4038
+#define PCIE_MISC_RC_BAR4_CONFIG_LO			0x40d4
 
-#define PCIE_MISC_RC_BAR3_CONFIG_LO			0x403c
-#define  PCIE_MISC_RC_BAR3_CONFIG_LO_SIZE_MASK		0x1f
 
 #define PCIE_MISC_MSI_BAR_CONFIG_LO			0x4044
 #define PCIE_MISC_MSI_BAR_CONFIG_HI			0x4048
@@ -130,6 +134,10 @@
 	  (PCIE_MISC_HARD_PCIE_HARD_DEBUG_CLKREQ_DEBUG_ENABLE_MASK | \
 	   PCIE_MISC_HARD_PCIE_HARD_DEBUG_L1SS_ENABLE_MASK)
 
+#define PCIE_MISC_UBUS_BAR1_CONFIG_REMAP			0x40ac
+#define  PCIE_MISC_UBUS_BAR1_CONFIG_REMAP_ACCESS_EN_MASK	BIT(0)
+#define PCIE_MISC_UBUS_BAR4_CONFIG_REMAP			0x410c
+
 #define PCIE_MSI_INTR2_BASE		0x4500
 
 /* Offsets from INTR2_CPU and MSI_INTR2 BASE offsets */
@@ -217,12 +225,20 @@ enum pcie_type {
 	BCM4908,
 	BCM7278,
 	BCM2711,
+	BCM7712,
+};
+
+struct inbound_win {
+	u64 size;
+	u64 pci_offset;
+	u64 cpu_addr;
 };
 
 struct pcie_cfg_data {
 	const int *offsets;
 	const enum pcie_type type;
 	const bool has_phy;
+	unsigned int num_inbound_wins;
 	void (*perst_set)(struct brcm_pcie *pcie, u32 val);
 	void (*bridge_sw_init_set)(struct brcm_pcie *pcie, u32 val);
 };
@@ -274,6 +290,7 @@ struct brcm_pcie {
 	struct subdev_regulators *sr;
 	bool			ep_wakeup_capable;
 	bool			has_phy;
+	int			num_inbound_wins;
 };
 
 static inline bool is_bmips(const struct brcm_pcie *pcie)
@@ -789,23 +806,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 inbound_win *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 inbound_win inbound_wins[])
 {
 	struct pci_host_bridge *bridge = pci_host_bridge_from_priv(pcie);
+	u64 pci_offset, cpu_addr, size = 0, tot_size = 0;
 	struct resource_entry *entry;
 	struct device *dev = pcie->dev;
 	u64 lowest_pcie_addr = ~(u64)0;
-	int ret, i = 0;
-	u64 size = 0;
+	int ret, i = 0, n = 0;
+
+	/*
+	 * The HW registers (and PCIe) use order-1 numbering for BARs. As
+	 * such, we have inbound_wins[0] unused and BAR1 starts at inbound_wins[1].
+	 */
+	struct inbound_win *b_begin = &inbound_wins[1];
+	struct inbound_win *b = b_begin;
+
+	/*
+	 * STB chips beside 7712 disable the first inbound window default.
+	 * Rather being mapped to system memory it is mapped to the
+	 * internal registers of the SoC.  This feature is deprecated, has
+	 * security considerations, and is not implemented in our modern
+	 * SoCs.
+	 */
+	if (pcie->type != BCM7712)
+		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_wins)
+			break;
 	}
 
 	if (lowest_pcie_addr == ~(u64)0) {
@@ -813,13 +868,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 +890,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 +933,90 @@ static int brcm_pcie_get_rc_bar2_size_and_offset(struct brcm_pcie *pcie,
 	 *   outbound memory @ 3GB). So instead it will  start at the 1x
 	 *   multiple of its size
 	 */
-	if (!*rc_bar2_size || (*rc_bar2_offset & (*rc_bar2_size - 1)) ||
-	    (*rc_bar2_offset < SZ_4G && *rc_bar2_offset > SZ_2G)) {
-		dev_err(dev, "Invalid rc_bar2_offset/size: size 0x%llx, off 0x%llx\n",
-			*rc_bar2_size, *rc_bar2_offset);
+	if (!size || (pci_offset & (size - 1)) ||
+	    (pci_offset < SZ_4G && pci_offset > SZ_2G)) {
+		dev_err(dev, "Invalid inbound_win2_offset/size: size 0x%llx, off 0x%llx\n",
+			size, pci_offset);
 		return -EINVAL;
 	}
 
-	return 0;
+	/* Enable inbound window 2, the main inbound window for STB chips */
+	set_bar(b++, &n, size, cpu_addr, pci_offset);
+
+	/*
+	 * Disable inbound window 3.  On some chips presents the same
+	 * window as #2 but the data appears in a settable endianness.
+	 */
+	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 inbound_win *inbound_wins,
+				      int num_inbound_wins)
+{
+	void __iomem *base = pcie->base;
+	int i;
+
+	for (i = 1; i <= num_inbound_wins; i++) {
+		u64 pci_offset = inbound_wins[i].pci_offset;
+		u64 cpu_addr = inbound_wins[i].cpu_addr;
+		u64 size = inbound_wins[i].size;
+		u32 reg_offset = brcm_bar_reg_offset(i);
+		u32 tmp = lower_32_bits(pci_offset);
+
+		u32p_replace_bits(&tmp, brcm_pcie_encode_ibar_size(size),
+				  PCIE_MISC_RC_BAR1_CONFIG_LO_SIZE_MASK);
+
+		/* Write low */
+		writel(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 inbound_win inbound_wins[PCIE_BRCM_MAX_INBOUND_WINS];
 	void __iomem *base = pcie->base;
 	struct pci_host_bridge *bridge;
 	struct resource_entry *entry;
 	u32 tmp, burst, aspm_support;
-	int num_out_wins = 0;
-	int ret, memc;
+	int num_out_wins = 0, num_rc_bars = 0;
+	int memc;
 
 	/* Reset the bridge */
 	pcie->bridge_sw_init_set(pcie, 1);
@@ -933,17 +1065,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, inbound_wins);
+	if (num_rc_bars < 0)
+		return num_rc_bars;
+
+	set_inbound_win_registers(pcie, inbound_wins, 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);
+	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 +1096,12 @@ static int brcm_pcie_setup(struct brcm_pcie *pcie)
 	 * 4GB or when the inbound area is smaller than 4GB (taking into
 	 * account the rounding-up we're forced to perform).
 	 */
-	if (rc_bar2_offset >= SZ_4G || (rc_bar2_size + rc_bar2_offset) < SZ_4G)
+	if (inbound_wins[2].pci_offset >= SZ_4G ||
+	    (inbound_wins[2].size + inbound_wins[2].pci_offset) < SZ_4G)
 		pcie->msi_target_addr = BRCM_MSI_TARGET_ADDR_LT_4GB;
 	else
 		pcie->msi_target_addr = BRCM_MSI_TARGET_ADDR_GT_4GB;
 
-	if (!brcm_pcie_rc_mode(pcie)) {
-		dev_err(pcie->dev, "PCIe RC controller misconfigured as Endpoint\n");
-		return -EINVAL;
-	}
-
-	/* disable the PCIe->GISB memory window (RC_BAR1) */
-	tmp = readl(base + PCIE_MISC_RC_BAR1_CONFIG_LO);
-	tmp &= ~PCIE_MISC_RC_BAR1_CONFIG_LO_SIZE_MASK;
-	writel(tmp, base + PCIE_MISC_RC_BAR1_CONFIG_LO);
-
-	/* disable the PCIe->SCB memory window (RC_BAR3) */
-	tmp = readl(base + PCIE_MISC_RC_BAR3_CONFIG_LO);
-	tmp &= ~PCIE_MISC_RC_BAR3_CONFIG_LO_SIZE_MASK;
-	writel(tmp, base + PCIE_MISC_RC_BAR3_CONFIG_LO);
 
 	/* Don't advertise L0s capability if 'aspm-no-l0s' */
 	aspm_support = PCIE_LINK_STATE_L1;
@@ -1034,7 +1152,7 @@ static int brcm_pcie_setup(struct brcm_pcie *pcie)
 		num_out_wins++;
 	}
 
-	/* PCIe->SCB endian mode for BAR */
+	/* PCIe->SCB endian mode for inbound window */
 	tmp = readl(base + PCIE_RC_CFG_VENDOR_VENDOR_SPECIFIC_REG1);
 	u32p_replace_bits(&tmp, PCIE_RC_CFG_VENDOR_SPCIFIC_REG1_LITTLE_ENDIAN,
 		PCIE_RC_CFG_VENDOR_VENDOR_SPECIFIC_REG1_ENDIAN_MODE_BAR2_MASK);
@@ -1516,6 +1634,7 @@ static const struct pcie_cfg_data generic_cfg = {
 	.type		= GENERIC,
 	.perst_set	= brcm_pcie_perst_set_generic,
 	.bridge_sw_init_set = brcm_pcie_bridge_sw_init_set_generic,
+	.num_inbound_wins = 3,
 };
 
 static const struct pcie_cfg_data bcm7425_cfg = {
@@ -1523,6 +1642,7 @@ static const struct pcie_cfg_data bcm7425_cfg = {
 	.type		= BCM7425,
 	.perst_set	= brcm_pcie_perst_set_generic,
 	.bridge_sw_init_set = brcm_pcie_bridge_sw_init_set_generic,
+	.num_inbound_wins = 3,
 };
 
 static const struct pcie_cfg_data bcm7435_cfg = {
@@ -1530,6 +1650,7 @@ static const struct pcie_cfg_data bcm7435_cfg = {
 	.type		= BCM7435,
 	.perst_set	= brcm_pcie_perst_set_generic,
 	.bridge_sw_init_set = brcm_pcie_bridge_sw_init_set_generic,
+	.num_inbound_wins = 3,
 };
 
 static const struct pcie_cfg_data bcm4908_cfg = {
@@ -1537,6 +1658,7 @@ static const struct pcie_cfg_data bcm4908_cfg = {
 	.type		= BCM4908,
 	.perst_set	= brcm_pcie_perst_set_4908,
 	.bridge_sw_init_set = brcm_pcie_bridge_sw_init_set_generic,
+	.num_inbound_wins = 3,
 };
 
 static const int pcie_offset_bcm7278[] = {
@@ -1552,6 +1674,7 @@ static const struct pcie_cfg_data bcm7278_cfg = {
 	.type		= BCM7278,
 	.perst_set	= brcm_pcie_perst_set_7278,
 	.bridge_sw_init_set = brcm_pcie_bridge_sw_init_set_7278,
+	.num_inbound_wins = 3,
 };
 
 static const struct pcie_cfg_data bcm2711_cfg = {
@@ -1559,6 +1682,7 @@ static const struct pcie_cfg_data bcm2711_cfg = {
 	.type		= BCM2711,
 	.perst_set	= brcm_pcie_perst_set_generic,
 	.bridge_sw_init_set = brcm_pcie_bridge_sw_init_set_generic,
+	.num_inbound_wins = 3,
 };
 
 static const struct pcie_cfg_data bcm7216_cfg = {
@@ -1567,6 +1691,7 @@ static const struct pcie_cfg_data bcm7216_cfg = {
 	.perst_set	= brcm_pcie_perst_set_7278,
 	.bridge_sw_init_set = brcm_pcie_bridge_sw_init_set_7278,
 	.has_phy	= true,
+	.num_inbound_wins = 3,
 };
 
 static const struct of_device_id brcm_pcie_match[] = {
@@ -1623,6 +1748,7 @@ static int brcm_pcie_probe(struct platform_device *pdev)
 	pcie->perst_set = data->perst_set;
 	pcie->bridge_sw_init_set = data->bridge_sw_init_set;
 	pcie->has_phy = data->has_phy;
+	pcie->num_inbound_wins = data->num_inbound_wins;
 
 	pcie->base = devm_platform_ioremap_resource(pdev, 0);
 	if (IS_ERR(pcie->base))
-- 
2.17.1

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

[Florian Fainelli]

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

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

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

[Stanimir Varbanov]

Hi Jim,


On 8/1/24 01:28, Jim Quinlan wrote:
> Provide support for new chips with multiple inbound windows while
> keeping the legacy support for the older chips.
> 
> In existing chips there are three inbound windows with fixed purposes: the
> first was for mapping SoC internal registers, the second was for memory,
> and the third was for memory but with the endian swapped.  Typically, only
> one window was used.
> 
> Complicating the inbound window usage was the fact that the PCIe HW would
> do a baroque internal mapping of system memory, and concatenate the regions
> of multiple memory controllers.
> 
> Newer chips such as the 7712 and Cable Modem SOCs take a step forward and
> drop the internal mapping while providing for multiple inbound windows.
> This works in concert with the dma-ranges property, where each provided
> range becomes an inbound window.
> 
> Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com>
> ---
>  drivers/pci/controller/pcie-brcmstb.c | 228 ++++++++++++++++++++------
>  1 file changed, 177 insertions(+), 51 deletions(-)
> 
> diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
> index 4659208ae8da..0ecca3d9576f 100644
> --- a/drivers/pci/controller/pcie-brcmstb.c
> +++ b/drivers/pci/controller/pcie-brcmstb.c
> @@ -75,15 +75,19 @@
>  #define PCIE_MEM_WIN0_HI(win)	\
>  		PCIE_MISC_CPU_2_PCIE_MEM_WIN0_HI + ((win) * 8)
>  
> +/*
> + * NOTE: You may see the term "BAR" in a number of register names used by
> + *   this driver.  The term is an artifact of when the HW core was an
> + *   endpoint device (EP).  Now it is a root complex (RC) and anywhere a
> + *   register has the term "BAR" it is related to an inbound window.
> + */
> +
> +#define PCIE_BRCM_MAX_INBOUND_WINS			16
>  #define PCIE_MISC_RC_BAR1_CONFIG_LO			0x402c
>  #define  PCIE_MISC_RC_BAR1_CONFIG_LO_SIZE_MASK		0x1f
>  
> -#define PCIE_MISC_RC_BAR2_CONFIG_LO			0x4034
> -#define  PCIE_MISC_RC_BAR2_CONFIG_LO_SIZE_MASK		0x1f
> -#define PCIE_MISC_RC_BAR2_CONFIG_HI			0x4038
> +#define PCIE_MISC_RC_BAR4_CONFIG_LO			0x40d4
>  
> -#define PCIE_MISC_RC_BAR3_CONFIG_LO			0x403c
> -#define  PCIE_MISC_RC_BAR3_CONFIG_LO_SIZE_MASK		0x1f
>  
>  #define PCIE_MISC_MSI_BAR_CONFIG_LO			0x4044
>  #define PCIE_MISC_MSI_BAR_CONFIG_HI			0x4048
> @@ -130,6 +134,10 @@
>  	  (PCIE_MISC_HARD_PCIE_HARD_DEBUG_CLKREQ_DEBUG_ENABLE_MASK | \
>  	   PCIE_MISC_HARD_PCIE_HARD_DEBUG_L1SS_ENABLE_MASK)
>  
> +#define PCIE_MISC_UBUS_BAR1_CONFIG_REMAP			0x40ac
> +#define  PCIE_MISC_UBUS_BAR1_CONFIG_REMAP_ACCESS_EN_MASK	BIT(0)
> +#define PCIE_MISC_UBUS_BAR4_CONFIG_REMAP			0x410c
> +
>  #define PCIE_MSI_INTR2_BASE		0x4500
>  
>  /* Offsets from INTR2_CPU and MSI_INTR2 BASE offsets */
> @@ -217,12 +225,20 @@ enum pcie_type {
>  	BCM4908,
>  	BCM7278,
>  	BCM2711,
> +	BCM7712,
> +};
> +
> +struct inbound_win {
> +	u64 size;
> +	u64 pci_offset;
> +	u64 cpu_addr;
>  };
>  
>  struct pcie_cfg_data {
>  	const int *offsets;
>  	const enum pcie_type type;
>  	const bool has_phy;
> +	unsigned int num_inbound_wins;
>  	void (*perst_set)(struct brcm_pcie *pcie, u32 val);
>  	void (*bridge_sw_init_set)(struct brcm_pcie *pcie, u32 val);
>  };
> @@ -274,6 +290,7 @@ struct brcm_pcie {
>  	struct subdev_regulators *sr;
>  	bool			ep_wakeup_capable;
>  	bool			has_phy;
> +	int			num_inbound_wins;

Could you unify the type of num_inbound_wins field. I think u8 should be
enough?

>  };
>  
>  static inline bool is_bmips(const struct brcm_pcie *pcie)
> @@ -789,23 +806,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 inbound_win *b, int *count, u64 size,

The number of BARs cannot be more than 256, could you make "count" an u8
like the num_inbound_wins.

> +			   u64 cpu_addr, u64 pci_offset)
> +{
> +	b->size = size;
> +	b->cpu_addr = cpu_addr;
> +	b->pci_offset = pci_offset;
> +	(*count)++;
> +}
> +

<cut>

In case you send a new version please address the comments, otherwise

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

~Stan

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

[Manivannan Sadhasivam]

On Wed, Jul 31, 2024 at 06:28:23PM -0400, Jim Quinlan wrote:
> Provide support for new chips with multiple inbound windows while
> keeping the legacy support for the older chips.
> 
> In existing chips there are three inbound windows with fixed purposes: the
> first was for mapping SoC internal registers, the second was for memory,
> and the third was for memory but with the endian swapped.  Typically, only
> one window was used.
> 
> Complicating the inbound window usage was the fact that the PCIe HW would
> do a baroque internal mapping of system memory, and concatenate the regions
> of multiple memory controllers.
> 
> Newer chips such as the 7712 and Cable Modem SOCs take a step forward and
> drop the internal mapping while providing for multiple inbound windows.
> This works in concert with the dma-ranges property, where each provided
> range becomes an inbound window.
> 
> Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com>
> ---
>  drivers/pci/controller/pcie-brcmstb.c | 228 ++++++++++++++++++++------
>  1 file changed, 177 insertions(+), 51 deletions(-)
> 
> diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
> index 4659208ae8da..0ecca3d9576f 100644
> --- a/drivers/pci/controller/pcie-brcmstb.c
> +++ b/drivers/pci/controller/pcie-brcmstb.c
> @@ -75,15 +75,19 @@
>  #define PCIE_MEM_WIN0_HI(win)	\
>  		PCIE_MISC_CPU_2_PCIE_MEM_WIN0_HI + ((win) * 8)
>  
> +/*
> + * NOTE: You may see the term "BAR" in a number of register names used by
> + *   this driver.  The term is an artifact of when the HW core was an
> + *   endpoint device (EP).  Now it is a root complex (RC) and anywhere a
> + *   register has the term "BAR" it is related to an inbound window.
> + */
> +
> +#define PCIE_BRCM_MAX_INBOUND_WINS			16
>  #define PCIE_MISC_RC_BAR1_CONFIG_LO			0x402c
>  #define  PCIE_MISC_RC_BAR1_CONFIG_LO_SIZE_MASK		0x1f
>  
> -#define PCIE_MISC_RC_BAR2_CONFIG_LO			0x4034
> -#define  PCIE_MISC_RC_BAR2_CONFIG_LO_SIZE_MASK		0x1f
> -#define PCIE_MISC_RC_BAR2_CONFIG_HI			0x4038
> +#define PCIE_MISC_RC_BAR4_CONFIG_LO			0x40d4
>  
> -#define PCIE_MISC_RC_BAR3_CONFIG_LO			0x403c
> -#define  PCIE_MISC_RC_BAR3_CONFIG_LO_SIZE_MASK		0x1f
>  
>  #define PCIE_MISC_MSI_BAR_CONFIG_LO			0x4044
>  #define PCIE_MISC_MSI_BAR_CONFIG_HI			0x4048
> @@ -130,6 +134,10 @@
>  	  (PCIE_MISC_HARD_PCIE_HARD_DEBUG_CLKREQ_DEBUG_ENABLE_MASK | \
>  	   PCIE_MISC_HARD_PCIE_HARD_DEBUG_L1SS_ENABLE_MASK)
>  
> +#define PCIE_MISC_UBUS_BAR1_CONFIG_REMAP			0x40ac
> +#define  PCIE_MISC_UBUS_BAR1_CONFIG_REMAP_ACCESS_EN_MASK	BIT(0)
> +#define PCIE_MISC_UBUS_BAR4_CONFIG_REMAP			0x410c
> +
>  #define PCIE_MSI_INTR2_BASE		0x4500
>  
>  /* Offsets from INTR2_CPU and MSI_INTR2 BASE offsets */
> @@ -217,12 +225,20 @@ enum pcie_type {
>  	BCM4908,
>  	BCM7278,
>  	BCM2711,
> +	BCM7712,
> +};
> +
> +struct inbound_win {
> +	u64 size;
> +	u64 pci_offset;
> +	u64 cpu_addr;
>  };
>  
>  struct pcie_cfg_data {
>  	const int *offsets;
>  	const enum pcie_type type;
>  	const bool has_phy;
> +	unsigned int num_inbound_wins;
>  	void (*perst_set)(struct brcm_pcie *pcie, u32 val);
>  	void (*bridge_sw_init_set)(struct brcm_pcie *pcie, u32 val);
>  };
> @@ -274,6 +290,7 @@ struct brcm_pcie {
>  	struct subdev_regulators *sr;
>  	bool			ep_wakeup_capable;
>  	bool			has_phy;
> +	int			num_inbound_wins;
>  };
>  
>  static inline bool is_bmips(const struct brcm_pcie *pcie)
> @@ -789,23 +806,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 inbound_win *b, int *count, u64 size,
> +			   u64 cpu_addr, u64 pci_offset)

There is no need to pass 'inline' keyword in a .c file. Making a function inline
is upto the discretion of the compiler.

Also, set_bar() is quite misleading as you are not setting any BAR but just
populating the inbound_win struct. So how about, "add_inbound_window()"?

> +{
> +	b->size = size;
> +	b->cpu_addr = cpu_addr;
> +	b->pci_offset = pci_offset;
> +	(*count)++;
> +}
> +
> +static int brcm_pcie_get_inbound_wins(struct brcm_pcie *pcie,
> +				      struct inbound_win inbound_wins[])
>  {
>  	struct pci_host_bridge *bridge = pci_host_bridge_from_priv(pcie);
> +	u64 pci_offset, cpu_addr, size = 0, tot_size = 0;
>  	struct resource_entry *entry;
>  	struct device *dev = pcie->dev;
>  	u64 lowest_pcie_addr = ~(u64)0;
> -	int ret, i = 0;
> -	u64 size = 0;
> +	int ret, i = 0, n = 0;
> +
> +	/*
> +	 * The HW registers (and PCIe) use order-1 numbering for BARs. As
> +	 * such, we have inbound_wins[0] unused and BAR1 starts at inbound_wins[1].
> +	 */

Instead of wasting one array entry, you can start the array from 0 and just
decrement the index where needed? Like,

	reg_offset = brcm_bar_reg_offset(i - 1);

> +	struct inbound_win *b_begin = &inbound_wins[1];
> +	struct inbound_win *b = b_begin;
> +
> +	/*
> +	 * STB chips beside 7712 disable the first inbound window default.
> +	 * Rather being mapped to system memory it is mapped to the
> +	 * internal registers of the SoC.  This feature is deprecated, has
> +	 * security considerations, and is not implemented in our modern
> +	 * SoCs.
> +	 */
> +	if (pcie->type != BCM7712)
> +		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;

What does 'beg' mean?

>  
> -		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_wins)
> +			break;
>  	}
>  
>  	if (lowest_pcie_addr == ~(u64)0) {
> @@ -813,13 +868,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 +890,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 +933,90 @@ static int brcm_pcie_get_rc_bar2_size_and_offset(struct brcm_pcie *pcie,
>  	 *   outbound memory @ 3GB). So instead it will  start at the 1x
>  	 *   multiple of its size
>  	 */
> -	if (!*rc_bar2_size || (*rc_bar2_offset & (*rc_bar2_size - 1)) ||
> -	    (*rc_bar2_offset < SZ_4G && *rc_bar2_offset > SZ_2G)) {
> -		dev_err(dev, "Invalid rc_bar2_offset/size: size 0x%llx, off 0x%llx\n",
> -			*rc_bar2_size, *rc_bar2_offset);
> +	if (!size || (pci_offset & (size - 1)) ||
> +	    (pci_offset < SZ_4G && pci_offset > SZ_2G)) {
> +		dev_err(dev, "Invalid inbound_win2_offset/size: size 0x%llx, off 0x%llx\n",
> +			size, pci_offset);
>  		return -EINVAL;
>  	}
>  
> -	return 0;
> +	/* Enable inbound window 2, the main inbound window for STB chips */
> +	set_bar(b++, &n, size, cpu_addr, pci_offset);
> +
> +	/*
> +	 * Disable inbound window 3.  On some chips presents the same
> +	 * window as #2 but the data appears in a settable endianness.
> +	 */
> +	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 inbound_win *inbound_wins,
> +				      int num_inbound_wins)
> +{
> +	void __iomem *base = pcie->base;
> +	int i;
> +
> +	for (i = 1; i <= num_inbound_wins; i++) {
> +		u64 pci_offset = inbound_wins[i].pci_offset;
> +		u64 cpu_addr = inbound_wins[i].cpu_addr;
> +		u64 size = inbound_wins[i].size;
> +		u32 reg_offset = brcm_bar_reg_offset(i);
> +		u32 tmp = lower_32_bits(pci_offset);
> +
> +		u32p_replace_bits(&tmp, brcm_pcie_encode_ibar_size(size),
> +				  PCIE_MISC_RC_BAR1_CONFIG_LO_SIZE_MASK);
> +
> +		/* Write low */
> +		writel(tmp, base + reg_offset);

Can you use writel_relaxed() instead? Here and below. I don't see a necessity to
use the barrier that comes with non-relaxed version of writel.

- Mani

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

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

[Florian Fainelli]

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

[snip]

>> +static void set_inbound_win_registers(struct brcm_pcie *pcie,
>> +				      const struct inbound_win *inbound_wins,
>> +				      int num_inbound_wins)
>> +{
>> +	void __iomem *base = pcie->base;
>> +	int i;
>> +
>> +	for (i = 1; i <= num_inbound_wins; i++) {
>> +		u64 pci_offset = inbound_wins[i].pci_offset;
>> +		u64 cpu_addr = inbound_wins[i].cpu_addr;
>> +		u64 size = inbound_wins[i].size;
>> +		u32 reg_offset = brcm_bar_reg_offset(i);
>> +		u32 tmp = lower_32_bits(pci_offset);
>> +
>> +		u32p_replace_bits(&tmp, brcm_pcie_encode_ibar_size(size),
>> +				  PCIE_MISC_RC_BAR1_CONFIG_LO_SIZE_MASK);
>> +
>> +		/* Write low */
>> +		writel(tmp, base + reg_offset);
> 
> Can you use writel_relaxed() instead? Here and below. I don't see a necessity to
> use the barrier that comes with non-relaxed version of writel.

Out of curiosity what is the reasoning here for asking to use 
writel_relaxed(), this is not a hot path, this is a configuration path 
anyway. I am not certain clear on the implication of using 
writel_relaxed() on systems like 7712/2712 where the busing is different 
from the other STB chips.
-- 
Florian

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

[Manivannan Sadhasivam]

On Wed, Aug 07, 2024 at 07:16:44AM -0700, Florian Fainelli wrote:
> 
> 
> On 8/7/2024 7:04 AM, Manivannan Sadhasivam wrote:
> > On Wed, Jul 31, 2024 at 06:28:23PM -0400, Jim Quinlan wrote:
> > > Provide support for new chips with multiple inbound windows while
> > > keeping the legacy support for the older chips.
> > > 
> > > In existing chips there are three inbound windows with fixed purposes: the
> > > first was for mapping SoC internal registers, the second was for memory,
> > > and the third was for memory but with the endian swapped.  Typically, only
> > > one window was used.
> > > 
> > > Complicating the inbound window usage was the fact that the PCIe HW would
> > > do a baroque internal mapping of system memory, and concatenate the regions
> > > of multiple memory controllers.
> > > 
> > > Newer chips such as the 7712 and Cable Modem SOCs take a step forward and
> > > drop the internal mapping while providing for multiple inbound windows.
> > > This works in concert with the dma-ranges property, where each provided
> > > range becomes an inbound window.
> > > 
> > > Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com>
> > > ---
> 
> [snip]
> 
> > > +static void set_inbound_win_registers(struct brcm_pcie *pcie,
> > > +				      const struct inbound_win *inbound_wins,
> > > +				      int num_inbound_wins)
> > > +{
> > > +	void __iomem *base = pcie->base;
> > > +	int i;
> > > +
> > > +	for (i = 1; i <= num_inbound_wins; i++) {
> > > +		u64 pci_offset = inbound_wins[i].pci_offset;
> > > +		u64 cpu_addr = inbound_wins[i].cpu_addr;
> > > +		u64 size = inbound_wins[i].size;
> > > +		u32 reg_offset = brcm_bar_reg_offset(i);
> > > +		u32 tmp = lower_32_bits(pci_offset);
> > > +
> > > +		u32p_replace_bits(&tmp, brcm_pcie_encode_ibar_size(size),
> > > +				  PCIE_MISC_RC_BAR1_CONFIG_LO_SIZE_MASK);
> > > +
> > > +		/* Write low */
> > > +		writel(tmp, base + reg_offset);
> > 
> > Can you use writel_relaxed() instead? Here and below. I don't see a necessity to
> > use the barrier that comes with non-relaxed version of writel.
> 
> Out of curiosity what is the reasoning here for asking to use
> writel_relaxed(), this is not a hot path, this is a configuration path
> anyway. I am not certain clear on the implication of using writel_relaxed()
> on systems like 7712/2712 where the busing is different from the other STB
> chips.

It is the general recommendation (although not documented). If the code path do
not need ordering/barrier, then there is no need for non-relaxed variants.

Btw, if the register accesses are to the same domain (like PCIe), then certainly
you do not need barrier as writes to the same domain are ordered.

Problem with readl/writel is that the drivers started using non-relaxed variants
as if it is a norm and completely ignored the relaxed variants.

- Mani

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

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

[Jim Quinlan]

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

On Wed, Aug 7, 2024 at 10:04 AM Manivannan Sadhasivam
<manivannan.sadhasivam@linaro.org> wrote:
>
> On Wed, Jul 31, 2024 at 06:28:23PM -0400, Jim Quinlan wrote:
> > Provide support for new chips with multiple inbound windows while
> > keeping the legacy support for the older chips.
> >
> > In existing chips there are three inbound windows with fixed purposes: the
> > first was for mapping SoC internal registers, the second was for memory,
> > and the third was for memory but with the endian swapped.  Typically, only
> > one window was used.
> >
> > Complicating the inbound window usage was the fact that the PCIe HW would
> > do a baroque internal mapping of system memory, and concatenate the regions
> > of multiple memory controllers.
> >
> > Newer chips such as the 7712 and Cable Modem SOCs take a step forward and
> > drop the internal mapping while providing for multiple inbound windows.
> > This works in concert with the dma-ranges property, where each provided
> > range becomes an inbound window.
> >
> > Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com>
> > ---
> >  drivers/pci/controller/pcie-brcmstb.c | 228 ++++++++++++++++++++------
> >  1 file changed, 177 insertions(+), 51 deletions(-)
> >
> > diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
> > index 4659208ae8da..0ecca3d9576f 100644
> > --- a/drivers/pci/controller/pcie-brcmstb.c
> > +++ b/drivers/pci/controller/pcie-brcmstb.c
> > @@ -75,15 +75,19 @@
> >  #define PCIE_MEM_WIN0_HI(win)        \
> >               PCIE_MISC_CPU_2_PCIE_MEM_WIN0_HI + ((win) * 8)
> >
> > +/*
> > + * NOTE: You may see the term "BAR" in a number of register names used by
> > + *   this driver.  The term is an artifact of when the HW core was an
> > + *   endpoint device (EP).  Now it is a root complex (RC) and anywhere a
> > + *   register has the term "BAR" it is related to an inbound window.
> > + */
> > +
> > +#define PCIE_BRCM_MAX_INBOUND_WINS                   16
> >  #define PCIE_MISC_RC_BAR1_CONFIG_LO                  0x402c
> >  #define  PCIE_MISC_RC_BAR1_CONFIG_LO_SIZE_MASK               0x1f
> >
> > -#define PCIE_MISC_RC_BAR2_CONFIG_LO                  0x4034
> > -#define  PCIE_MISC_RC_BAR2_CONFIG_LO_SIZE_MASK               0x1f
> > -#define PCIE_MISC_RC_BAR2_CONFIG_HI                  0x4038
> > +#define PCIE_MISC_RC_BAR4_CONFIG_LO                  0x40d4
> >
> > -#define PCIE_MISC_RC_BAR3_CONFIG_LO                  0x403c
> > -#define  PCIE_MISC_RC_BAR3_CONFIG_LO_SIZE_MASK               0x1f
> >
> >  #define PCIE_MISC_MSI_BAR_CONFIG_LO                  0x4044
> >  #define PCIE_MISC_MSI_BAR_CONFIG_HI                  0x4048
> > @@ -130,6 +134,10 @@
> >         (PCIE_MISC_HARD_PCIE_HARD_DEBUG_CLKREQ_DEBUG_ENABLE_MASK | \
> >          PCIE_MISC_HARD_PCIE_HARD_DEBUG_L1SS_ENABLE_MASK)
> >
> > +#define PCIE_MISC_UBUS_BAR1_CONFIG_REMAP                     0x40ac
> > +#define  PCIE_MISC_UBUS_BAR1_CONFIG_REMAP_ACCESS_EN_MASK     BIT(0)
> > +#define PCIE_MISC_UBUS_BAR4_CONFIG_REMAP                     0x410c
> > +
> >  #define PCIE_MSI_INTR2_BASE          0x4500
> >
> >  /* Offsets from INTR2_CPU and MSI_INTR2 BASE offsets */
> > @@ -217,12 +225,20 @@ enum pcie_type {
> >       BCM4908,
> >       BCM7278,
> >       BCM2711,
> > +     BCM7712,
> > +};
> > +
> > +struct inbound_win {
> > +     u64 size;
> > +     u64 pci_offset;
> > +     u64 cpu_addr;
> >  };
> >
> >  struct pcie_cfg_data {
> >       const int *offsets;
> >       const enum pcie_type type;
> >       const bool has_phy;
> > +     unsigned int num_inbound_wins;
> >       void (*perst_set)(struct brcm_pcie *pcie, u32 val);
> >       void (*bridge_sw_init_set)(struct brcm_pcie *pcie, u32 val);
> >  };
> > @@ -274,6 +290,7 @@ struct brcm_pcie {
> >       struct subdev_regulators *sr;
> >       bool                    ep_wakeup_capable;
> >       bool                    has_phy;
> > +     int                     num_inbound_wins;
> >  };
> >
> >  static inline bool is_bmips(const struct brcm_pcie *pcie)
> > @@ -789,23 +806,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 inbound_win *b, int *count, u64 size,
> > +                        u64 cpu_addr, u64 pci_offset)
>
> There is no need to pass 'inline' keyword in a .c file. Making a function inline
> is upto the discretion of the compiler.
>
> Also, set_bar() is quite misleading as you are not setting any BAR but just
> populating the inbound_win struct. So how about, "add_inbound_window()"?
>
> > +{
> > +     b->size = size;
> > +     b->cpu_addr = cpu_addr;
> > +     b->pci_offset = pci_offset;
> > +     (*count)++;
> > +}
> > +
> > +static int brcm_pcie_get_inbound_wins(struct brcm_pcie *pcie,
> > +                                   struct inbound_win inbound_wins[])
> >  {
> >       struct pci_host_bridge *bridge = pci_host_bridge_from_priv(pcie);
> > +     u64 pci_offset, cpu_addr, size = 0, tot_size = 0;
> >       struct resource_entry *entry;
> >       struct device *dev = pcie->dev;
> >       u64 lowest_pcie_addr = ~(u64)0;
> > -     int ret, i = 0;
> > -     u64 size = 0;
> > +     int ret, i = 0, n = 0;
> > +
> > +     /*
> > +      * The HW registers (and PCIe) use order-1 numbering for BARs. As
> > +      * such, we have inbound_wins[0] unused and BAR1 starts at inbound_wins[1].
> > +      */
>
> Instead of wasting one array entry, you can start the array from 0 and just
> decrement the index where needed? Like,
>
>         reg_offset = brcm_bar_reg_offset(i - 1);

This is intentional -- IMO it is worth the waste of one entry to
improve the readability and avoid the "- 1" everywhere an index is
used.

Further, I am only wasting an entry on the stack for a brief moment of
a function that is only called once, this does not affect memory usage
at all.

If you insist on this I will change it but I prefer it this way.




>
> > +     struct inbound_win *b_begin = &inbound_wins[1];
> > +     struct inbound_win *b = b_begin;
> > +
> > +     /*
> > +      * STB chips beside 7712 disable the first inbound window default.
> > +      * Rather being mapped to system memory it is mapped to the
> > +      * internal registers of the SoC.  This feature is deprecated, has
> > +      * security considerations, and is not implemented in our modern
> > +      * SoCs.
> > +      */
> > +     if (pcie->type != BCM7712)
> > +             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;
>
> What does 'beg' mean?

"beg" => "begin".  I will change it to 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_wins)
> > +                     break;
> >       }
> >
> >       if (lowest_pcie_addr == ~(u64)0) {
> > @@ -813,13 +868,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 +890,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 +933,90 @@ static int brcm_pcie_get_rc_bar2_size_and_offset(struct brcm_pcie *pcie,
> >        *   outbound memory @ 3GB). So instead it will  start at the 1x
> >        *   multiple of its size
> >        */
> > -     if (!*rc_bar2_size || (*rc_bar2_offset & (*rc_bar2_size - 1)) ||
> > -         (*rc_bar2_offset < SZ_4G && *rc_bar2_offset > SZ_2G)) {
> > -             dev_err(dev, "Invalid rc_bar2_offset/size: size 0x%llx, off 0x%llx\n",
> > -                     *rc_bar2_size, *rc_bar2_offset);
> > +     if (!size || (pci_offset & (size - 1)) ||
> > +         (pci_offset < SZ_4G && pci_offset > SZ_2G)) {
> > +             dev_err(dev, "Invalid inbound_win2_offset/size: size 0x%llx, off 0x%llx\n",
> > +                     size, pci_offset);
> >               return -EINVAL;
> >       }
> >
> > -     return 0;
> > +     /* Enable inbound window 2, the main inbound window for STB chips */
> > +     set_bar(b++, &n, size, cpu_addr, pci_offset);
> > +
> > +     /*
> > +      * Disable inbound window 3.  On some chips presents the same
> > +      * window as #2 but the data appears in a settable endianness.
> > +      */
> > +     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 inbound_win *inbound_wins,
> > +                                   int num_inbound_wins)
> > +{
> > +     void __iomem *base = pcie->base;
> > +     int i;
> > +
> > +     for (i = 1; i <= num_inbound_wins; i++) {
> > +             u64 pci_offset = inbound_wins[i].pci_offset;
> > +             u64 cpu_addr = inbound_wins[i].cpu_addr;
> > +             u64 size = inbound_wins[i].size;
> > +             u32 reg_offset = brcm_bar_reg_offset(i);
> > +             u32 tmp = lower_32_bits(pci_offset);
> > +
> > +             u32p_replace_bits(&tmp, brcm_pcie_encode_ibar_size(size),
> > +                               PCIE_MISC_RC_BAR1_CONFIG_LO_SIZE_MASK);
> > +
> > +             /* Write low */
> > +             writel(tmp, base + reg_offset);
>
> Can you use writel_relaxed() instead? Here and below. I don't see a necessity to
> use the barrier that comes with non-relaxed version of writel.

Yep.

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

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

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

[Jim Quinlan]

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

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

diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
index 0ecca3d9576f..c4ceb1823a79 100644
--- a/drivers/pci/controller/pcie-brcmstb.c
+++ b/drivers/pci/controller/pcie-brcmstb.c
@@ -239,8 +239,8 @@ struct pcie_cfg_data {
 	const enum pcie_type type;
 	const bool has_phy;
 	unsigned int num_inbound_wins;
-	void (*perst_set)(struct brcm_pcie *pcie, u32 val);
-	void (*bridge_sw_init_set)(struct brcm_pcie *pcie, u32 val);
+	int (*perst_set)(struct brcm_pcie *pcie, u32 val);
+	int (*bridge_sw_init_set)(struct brcm_pcie *pcie, u32 val);
 };
 
 struct subdev_regulators {
@@ -285,8 +285,8 @@ struct brcm_pcie {
 	int			num_memc;
 	u64			memc_size[PCIE_BRCM_MAX_MEMC];
 	u32			hw_rev;
-	void			(*perst_set)(struct brcm_pcie *pcie, u32 val);
-	void			(*bridge_sw_init_set)(struct brcm_pcie *pcie, u32 val);
+	int			(*perst_set)(struct brcm_pcie *pcie, u32 val);
+	int			(*bridge_sw_init_set)(struct brcm_pcie *pcie, u32 val);
 	struct subdev_regulators *sr;
 	bool			ep_wakeup_capable;
 	bool			has_phy;
@@ -749,12 +749,18 @@ static void __iomem *brcm7425_pcie_map_bus(struct pci_bus *bus,
 	return base + DATA_ADDR(pcie);
 }
 
-static void brcm_pcie_bridge_sw_init_set_generic(struct brcm_pcie *pcie, u32 val)
+static int brcm_pcie_bridge_sw_init_set_generic(struct brcm_pcie *pcie, u32 val)
 {
+	int ret = 0;
+
 	if (val)
-		reset_control_assert(pcie->bridge_reset);
+		ret = reset_control_assert(pcie->bridge_reset);
 	else
-		reset_control_deassert(pcie->bridge_reset);
+		ret = reset_control_deassert(pcie->bridge_reset);
+
+	if (ret)
+		dev_err(pcie->dev, "failed to %s 'bridge' reset, err=%d\n",
+			val ? "assert" : "deassert", ret);
 
 	if (!pcie->bridge_reset) {
 		u32 tmp, mask =  RGR1_SW_INIT_1_INIT_GENERIC_MASK;
@@ -764,9 +770,11 @@ static void brcm_pcie_bridge_sw_init_set_generic(struct brcm_pcie *pcie, u32 val
 		tmp = (tmp & ~mask) | ((val << shift) & mask);
 		writel(tmp, pcie->base + PCIE_RGR1_SW_INIT_1(pcie));
 	}
+
+	return ret;
 }
 
-static void brcm_pcie_bridge_sw_init_set_7278(struct brcm_pcie *pcie, u32 val)
+static int brcm_pcie_bridge_sw_init_set_7278(struct brcm_pcie *pcie, u32 val)
 {
 	u32 tmp, mask =  RGR1_SW_INIT_1_INIT_7278_MASK;
 	u32 shift = RGR1_SW_INIT_1_INIT_7278_SHIFT;
@@ -774,20 +782,29 @@ static void brcm_pcie_bridge_sw_init_set_7278(struct brcm_pcie *pcie, u32 val)
 	tmp = readl(pcie->base + PCIE_RGR1_SW_INIT_1(pcie));
 	tmp = (tmp & ~mask) | ((val << shift) & mask);
 	writel(tmp, pcie->base + PCIE_RGR1_SW_INIT_1(pcie));
+
+	return 0;
 }
 
-static void brcm_pcie_perst_set_4908(struct brcm_pcie *pcie, u32 val)
+static int brcm_pcie_perst_set_4908(struct brcm_pcie *pcie, u32 val)
 {
+	int ret;
+
 	if (WARN_ONCE(!pcie->perst_reset, "missing PERST# reset controller\n"))
-		return;
+		return -EINVAL;
 
 	if (val)
-		reset_control_assert(pcie->perst_reset);
+		ret = reset_control_assert(pcie->perst_reset);
 	else
-		reset_control_deassert(pcie->perst_reset);
+		ret = reset_control_deassert(pcie->perst_reset);
+
+	if (ret)
+		dev_err(pcie->dev, "failed to %s 'perst' reset, err=%d\n",
+			val ? "assert" : "deassert", ret);
+	return ret;
 }
 
-static void brcm_pcie_perst_set_7278(struct brcm_pcie *pcie, u32 val)
+static int brcm_pcie_perst_set_7278(struct brcm_pcie *pcie, u32 val)
 {
 	u32 tmp;
 
@@ -795,15 +812,19 @@ static void brcm_pcie_perst_set_7278(struct brcm_pcie *pcie, u32 val)
 	tmp = readl(pcie->base + PCIE_MISC_PCIE_CTRL);
 	u32p_replace_bits(&tmp, !val, PCIE_MISC_PCIE_CTRL_PCIE_PERSTB_MASK);
 	writel(tmp, pcie->base +  PCIE_MISC_PCIE_CTRL);
+
+	return 0;
 }
 
-static void brcm_pcie_perst_set_generic(struct brcm_pcie *pcie, u32 val)
+static int brcm_pcie_perst_set_generic(struct brcm_pcie *pcie, u32 val)
 {
 	u32 tmp;
 
 	tmp = readl(pcie->base + PCIE_RGR1_SW_INIT_1(pcie));
 	u32p_replace_bits(&tmp, val, PCIE_RGR1_SW_INIT_1_PERST_MASK);
 	writel(tmp, pcie->base + PCIE_RGR1_SW_INIT_1(pcie));
+
+	return 0;
 }
 
 static inline void set_bar(struct inbound_win *b, int *count, u64 size,
@@ -1016,19 +1037,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))
@@ -1247,7 +1277,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
@@ -1439,15 +1471,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);
@@ -1460,7 +1494,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)
@@ -1478,9 +1514,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
@@ -1509,7 +1548,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;
 			}
 		}
@@ -1524,7 +1566,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);
@@ -1586,7 +1628,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

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

[Manivannan Sadhasivam]

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

One comment below. With that addressed,

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

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

[...]

>  static int pci_dev_may_wakeup(struct pci_dev *dev, void *data)
> @@ -1478,9 +1514,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
> @@ -1509,7 +1548,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);

I don't think it is really necessary to capture the return value in err path.
Unable to turn off the regulator itself is fatal, so we could just assert reset
and hope for the best.

- Mani

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

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

[Jim Quinlan]

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

On Wed, Aug 7, 2024 at 10:11 AM Manivannan Sadhasivam
<manivannan.sadhasivam@linaro.org> wrote:
>
> On Wed, Jul 31, 2024 at 06:28:24PM -0400, Jim Quinlan wrote:
> > Always check the return value for invocations of reset_control_xxx() and
> > propagate the error to the next level.  Although the current functions
> > in reset-brcmstb.c cannot fail, this may someday change.
> >
> > Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com>
>
> One comment below. With that addressed,
>
> Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
>
> > Reviewed-by: Stanimir Varbanov <svarbanov@suse.de>
> > Reviewed-by: Florian Fainelli <florian.fainelli@broadcom.com>
> > ---
> >  drivers/pci/controller/pcie-brcmstb.c | 102 ++++++++++++++++++--------
> >  1 file changed, 73 insertions(+), 29 deletions(-)
> >
> > diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
> > index 0ecca3d9576f..c4ceb1823a79 100644
> > --- a/drivers/pci/controller/pcie-brcmstb.c
> > +++ b/drivers/pci/controller/pcie-brcmstb.c
>
> [...]
>
> >  static int pci_dev_may_wakeup(struct pci_dev *dev, void *data)
> > @@ -1478,9 +1514,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
> > @@ -1509,7 +1548,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);
>
> I don't think it is really necessary to capture the return value in err path.
> Unable to turn off the regulator itself is fatal, so we could just assert reset
> and hope for the best.

Hi Mani,

I'm not sure what you would like changed.  Failure to turn off the
regulators isn't  necessarily fatal, it's when they cannot be turned
on that is fatal.

There can be two failures here: failure to turn off regulators and
failure to reset the "rascal" reset.  Since I can only return one
error value, I print the other value via dev_err().

Regards,
Jim Quinlan
Broadcom STB/CM


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

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

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

[Jim Quinlan]

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

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

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

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

[Florian Fainelli]

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

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

Personally, I preferred the 'type' name, but I really have no dog in 
this fight and today's color might as well be blue.
-- 
Florian

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

[Jim Quinlan]

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

Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com>
Reviewed-by: Stanimir Varbanov <svarbanov@suse.de>
Reviewed-by: Florian Fainelli <florian.fainelli@broadcom.com>
---
 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 4623b70f9ad8..44b323a13357 100644
--- a/drivers/pci/controller/pcie-brcmstb.c
+++ b/drivers/pci/controller/pcie-brcmstb.c
@@ -1202,6 +1202,10 @@ static void brcm_extend_rbus_timeout(struct brcm_pcie *pcie)
 	const unsigned int REG_OFFSET = PCIE_RGR1_SW_INIT_1(pcie) - 8;
 	u32 timeout_us = 4000000; /* 4 seconds, our setting for L1SS */
 
+	/* 7712 does not have this (RGR1) timer */
+	if (pcie->soc_base == BCM7712)
+		return;
+
 	/* Each unit in timeout register is 1/216,000,000 seconds */
 	writel(216 * timeout_us, pcie->base + REG_OFFSET);
 }
@@ -1673,6 +1677,13 @@ static const int pcie_offsets_bmips_7425[] = {
 	[PCIE_INTR2_CPU_BASE] = 0x4300,
 };
 
+static const int pcie_offset_bcm7712[] = {
+	[EXT_CFG_INDEX]  = 0x9000,
+	[EXT_CFG_DATA]   = 0x9004,
+	[PCIE_HARD_DEBUG] = 0x4304,
+	[PCIE_INTR2_CPU_BASE] = 0x4400,
+};
+
 static const struct pcie_cfg_data generic_cfg = {
 	.offsets	= pcie_offsets,
 	.soc_base	= GENERIC,
@@ -1738,6 +1749,14 @@ static const struct pcie_cfg_data bcm7216_cfg = {
 	.num_inbound_wins = 3,
 };
 
+static const struct pcie_cfg_data bcm7712_cfg = {
+	.offsets	= pcie_offset_bcm7712,
+	.perst_set	= brcm_pcie_perst_set_7278,
+	.bridge_sw_init_set = brcm_pcie_bridge_sw_init_set_generic,
+	.soc_base	= BCM7712,
+	.num_inbound_wins = 10,
+};
+
 static const struct of_device_id brcm_pcie_match[] = {
 	{ .compatible = "brcm,bcm2711-pcie", .data = &bcm2711_cfg },
 	{ .compatible = "brcm,bcm4908-pcie", .data = &bcm4908_cfg },
@@ -1747,6 +1766,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 v5 12/12] PCI: brcmstb: Enable 7712 SOCs

[Manivannan Sadhasivam]

On Wed, Jul 31, 2024 at 06:28:26PM -0400, Jim Quinlan wrote:
> The Broadcom STB 7712 is the sibling chip of the RPi 5 (2712).
> It has one PCIe controller with a single port, supports gen2
> and one lane only.  The current revision of the chip is "C0"
> or "C1".
> 
> Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com>

Reviewed-by: 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 | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
> 
> diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
> index 4623b70f9ad8..44b323a13357 100644
> --- a/drivers/pci/controller/pcie-brcmstb.c
> +++ b/drivers/pci/controller/pcie-brcmstb.c
> @@ -1202,6 +1202,10 @@ static void brcm_extend_rbus_timeout(struct brcm_pcie *pcie)
>  	const unsigned int REG_OFFSET = PCIE_RGR1_SW_INIT_1(pcie) - 8;
>  	u32 timeout_us = 4000000; /* 4 seconds, our setting for L1SS */
>  
> +	/* 7712 does not have this (RGR1) timer */
> +	if (pcie->soc_base == BCM7712)
> +		return;
> +
>  	/* Each unit in timeout register is 1/216,000,000 seconds */
>  	writel(216 * timeout_us, pcie->base + REG_OFFSET);
>  }
> @@ -1673,6 +1677,13 @@ static const int pcie_offsets_bmips_7425[] = {
>  	[PCIE_INTR2_CPU_BASE] = 0x4300,
>  };
>  
> +static const int pcie_offset_bcm7712[] = {
> +	[EXT_CFG_INDEX]  = 0x9000,
> +	[EXT_CFG_DATA]   = 0x9004,
> +	[PCIE_HARD_DEBUG] = 0x4304,
> +	[PCIE_INTR2_CPU_BASE] = 0x4400,
> +};
> +
>  static const struct pcie_cfg_data generic_cfg = {
>  	.offsets	= pcie_offsets,
>  	.soc_base	= GENERIC,
> @@ -1738,6 +1749,14 @@ static const struct pcie_cfg_data bcm7216_cfg = {
>  	.num_inbound_wins = 3,
>  };
>  
> +static const struct pcie_cfg_data bcm7712_cfg = {
> +	.offsets	= pcie_offset_bcm7712,
> +	.perst_set	= brcm_pcie_perst_set_7278,
> +	.bridge_sw_init_set = brcm_pcie_bridge_sw_init_set_generic,
> +	.soc_base	= BCM7712,
> +	.num_inbound_wins = 10,
> +};
> +
>  static const struct of_device_id brcm_pcie_match[] = {
>  	{ .compatible = "brcm,bcm2711-pcie", .data = &bcm2711_cfg },
>  	{ .compatible = "brcm,bcm4908-pcie", .data = &bcm4908_cfg },
> @@ -1747,6 +1766,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
> 

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