[PATCH v3 0/6] PCI: brcmstb: add EP regulators and panic handler

[PATCH v3 0/6] PCI: brcmstb: add EP regulators and panic handler

[Jim Quinlan]

v3 -- Driver now searches for EP DT subnode for any regulators to turn on.
      If present, these regulators have the property names
      "vpcie12v-supply" and "vpcie3v3-supply".  The existence of these
      regulators in the EP subnode are currently pending as a pullreq
      in pci-bus.yaml at
      https://github.com/devicetree-org/dt-schema/pull/54
      (MarkB, RobH).
   -- Check return of brcm_set_regulators() (Florian)
   -- Specify one regulator string per line for easier update (Florian)
   -- Author/Committer/Signoff email changed from that of V2 from
      'james.quinlan@broadcom.com' to 'jim2101024@gmail.com'.

v2 -- Use regulator bulk API rather than multiple calls (MarkB).

v1 -- Bindings are added for fixed regulators that may power the EP device.
   -- The brcmstb RC driver is modified to control these regulators
      during probe, suspend, and resume.
   -- 7216 type SOCs have additional error reporting HW and a
      panic handler is added to dump its info.
   -- A missing return value check is added.


Jim Quinlan (6):
  dt-bindings: PCI: Add bindings for Brcmstb EP voltage regulators
  PCI: brcmstb: Add control of EP voltage regulators
  PCI: brcmstb: Do not turn off regulators if EP can wake up
  PCI: brcmstb: Give 7216 SOCs their own config type
  PCI: brcmstb: Add panic/die handler to RC driver
  PCI: brcmstb: Check return value of clk_prepare_enable()

 .../bindings/pci/brcm,stb-pcie.yaml           |   6 +
 drivers/pci/controller/pcie-brcmstb.c         | 271 +++++++++++++++++-
 2 files changed, 272 insertions(+), 5 deletions(-)

-- 
2.17.1

[PATCH v3 0/6] PCI: brcmstb: add EP regulators and panic handler

[Jim Quinlan]

v3 -- Driver now searches for EP DT subnode for any regulators to turn on.
      If present, these regulators have the property names
      "vpcie12v-supply" and "vpcie3v3-supply".  The existence of these
      regulators in the EP subnode are currently pending as a pullreq
      in pci-bus.yaml at
      https://github.com/devicetree-org/dt-schema/pull/54
      (MarkB, RobH).
   -- Check return of brcm_set_regulators() (Florian)
   -- Specify one regulator string per line for easier update (Florian)
   -- Author/Committer/Signoff email changed from that of V2 from
      'james.quinlan@broadcom.com' to 'jim2101024@gmail.com'.

v2 -- Use regulator bulk API rather than multiple calls (MarkB).

v1 -- Bindings are added for fixed regulators that may power the EP device.
   -- The brcmstb RC driver is modified to control these regulators
      during probe, suspend, and resume.
   -- 7216 type SOCs have additional error reporting HW and a
      panic handler is added to dump its info.
   -- A missing return value check is added.


Jim Quinlan (6):
  dt-bindings: PCI: Add bindings for Brcmstb EP voltage regulators
  PCI: brcmstb: Add control of EP voltage regulators
  PCI: brcmstb: Do not turn off regulators if EP can wake up
  PCI: brcmstb: Give 7216 SOCs their own config type
  PCI: brcmstb: Add panic/die handler to RC driver
  PCI: brcmstb: Check return value of clk_prepare_enable()

 .../bindings/pci/brcm,stb-pcie.yaml           |   6 +
 drivers/pci/controller/pcie-brcmstb.c         | 271 +++++++++++++++++-
 2 files changed, 272 insertions(+), 5 deletions(-)

-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v3 1/6] dt-bindings: PCI: Add bindings for Brcmstb EP voltage regulators

[Jim Quinlan]

Similar to the regulator bindings found in "rockchip-pcie-host.txt", this
allows optional regulators to be attached and controlled by the PCIe RC
driver.  That being said, this driver searches in the DT subnode (the EP
node, eg pci@0,0) for the regulator property.

The use of a regulator property in the pcie EP subnode such as
"vpcie12v-supply" depends on a pending pullreq to the pci-bus.yaml
file at

https://github.com/devicetree-org/dt-schema/pull/54

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

diff --git a/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml b/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
index f90557f6deb8..ea3e6f55e365 100644
--- a/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
+++ b/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
@@ -156,5 +156,11 @@ examples:
                                  <0x42000000 0x1 0x80000000 0x3 0x00000000 0x0 0x80000000>;
                     brcm,enable-ssc;
                     brcm,scb-sizes =  <0x0000000080000000 0x0000000080000000>;
+
+                    pcie-ep@0,0 {
+                            reg = <0x0 0x0 0x0 0x0 0x0>;
+                            compatible = "pci14e4,1688";
+                            vpcie12v-supply: <&vreg12>;
+                    };
             };
     };
-- 
2.17.1

[PATCH v3 1/6] dt-bindings: PCI: Add bindings for Brcmstb EP voltage regulators

[Jim Quinlan]

Similar to the regulator bindings found in "rockchip-pcie-host.txt", this
allows optional regulators to be attached and controlled by the PCIe RC
driver.  That being said, this driver searches in the DT subnode (the EP
node, eg pci@0,0) for the regulator property.

The use of a regulator property in the pcie EP subnode such as
"vpcie12v-supply" depends on a pending pullreq to the pci-bus.yaml
file at

https://github.com/devicetree-org/dt-schema/pull/54

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

diff --git a/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml b/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
index f90557f6deb8..ea3e6f55e365 100644
--- a/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
+++ b/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
@@ -156,5 +156,11 @@ examples:
                                  <0x42000000 0x1 0x80000000 0x3 0x00000000 0x0 0x80000000>;
                     brcm,enable-ssc;
                     brcm,scb-sizes =  <0x0000000080000000 0x0000000080000000>;
+
+                    pcie-ep@0,0 {
+                            reg = <0x0 0x0 0x0 0x0 0x0>;
+                            compatible = "pci14e4,1688";
+                            vpcie12v-supply: <&vreg12>;
+                    };
             };
     };
-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 1/6] dt-bindings: PCI: Add bindings for Brcmstb EP voltage regulators

[Rob Herring]

On Fri, 26 Mar 2021 15:18:59 -0400, Jim Quinlan wrote:
> Similar to the regulator bindings found in "rockchip-pcie-host.txt", this
> allows optional regulators to be attached and controlled by the PCIe RC
> driver.  That being said, this driver searches in the DT subnode (the EP
> node, eg pci@0,0) for the regulator property.
> 
> The use of a regulator property in the pcie EP subnode such as
> "vpcie12v-supply" depends on a pending pullreq to the pci-bus.yaml
> file at
> 
> https://github.com/devicetree-org/dt-schema/pull/54
> 
> Signed-off-by: Jim Quinlan <jim2101024@gmail.com>
> ---
>  Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml | 6 ++++++
>  1 file changed, 6 insertions(+)
> 

My bot found errors running 'make dt_binding_check' on your patch:

yamllint warnings/errors:

dtschema/dtc warnings/errors:
Error: Documentation/devicetree/bindings/pci/brcm,stb-pcie.example.dts:48.48-49 syntax error
FATAL ERROR: Unable to parse input tree
make[1]: *** [scripts/Makefile.lib:349: Documentation/devicetree/bindings/pci/brcm,stb-pcie.example.dt.yaml] Error 1
make[1]: *** Waiting for unfinished jobs....
make: *** [Makefile:1380: dt_binding_check] Error 2

See https://patchwork.ozlabs.org/patch/1458942

This check can fail if there are any dependencies. The base for a patch
series is generally the most recent rc1.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit.

Re: [PATCH v3 1/6] dt-bindings: PCI: Add bindings for Brcmstb EP voltage regulators

[Rob Herring]

On Fri, 26 Mar 2021 15:18:59 -0400, Jim Quinlan wrote:
> Similar to the regulator bindings found in "rockchip-pcie-host.txt", this
> allows optional regulators to be attached and controlled by the PCIe RC
> driver.  That being said, this driver searches in the DT subnode (the EP
> node, eg pci@0,0) for the regulator property.
> 
> The use of a regulator property in the pcie EP subnode such as
> "vpcie12v-supply" depends on a pending pullreq to the pci-bus.yaml
> file at
> 
> https://github.com/devicetree-org/dt-schema/pull/54
> 
> Signed-off-by: Jim Quinlan <jim2101024@gmail.com>
> ---
>  Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml | 6 ++++++
>  1 file changed, 6 insertions(+)
> 

My bot found errors running 'make dt_binding_check' on your patch:

yamllint warnings/errors:

dtschema/dtc warnings/errors:
Error: Documentation/devicetree/bindings/pci/brcm,stb-pcie.example.dts:48.48-49 syntax error
FATAL ERROR: Unable to parse input tree
make[1]: *** [scripts/Makefile.lib:349: Documentation/devicetree/bindings/pci/brcm,stb-pcie.example.dt.yaml] Error 1
make[1]: *** Waiting for unfinished jobs....
make: *** [Makefile:1380: dt_binding_check] Error 2

See https://patchwork.ozlabs.org/patch/1458942

This check can fail if there are any dependencies. The base for a patch
series is generally the most recent rc1.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit.


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 1/6] dt-bindings: PCI: Add bindings for Brcmstb EP voltage regulators

[Rob Herring]

On Fri, Mar 26, 2021 at 03:18:59PM -0400, Jim Quinlan wrote:
> Similar to the regulator bindings found in "rockchip-pcie-host.txt", this
> allows optional regulators to be attached and controlled by the PCIe RC
> driver.  That being said, this driver searches in the DT subnode (the EP
> node, eg pci@0,0) for the regulator property.
> 
> The use of a regulator property in the pcie EP subnode such as
> "vpcie12v-supply" depends on a pending pullreq to the pci-bus.yaml
> file at
> 
> https://github.com/devicetree-org/dt-schema/pull/54
> 
> Signed-off-by: Jim Quinlan <jim2101024@gmail.com>
> ---
>  Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml b/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
> index f90557f6deb8..ea3e6f55e365 100644
> --- a/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
> +++ b/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
> @@ -156,5 +156,11 @@ examples:
>                                   <0x42000000 0x1 0x80000000 0x3 0x00000000 0x0 0x80000000>;
>                      brcm,enable-ssc;
>                      brcm,scb-sizes =  <0x0000000080000000 0x0000000080000000>;
> +
> +                    pcie-ep@0,0 {
> +                            reg = <0x0 0x0 0x0 0x0 0x0>;
> +                            compatible = "pci14e4,1688";
> +                            vpcie12v-supply: <&vreg12>;

For other cases, these properties are in the host bridge node. If these 
are standard PCI rails, then I think that's where they belong unless we 
define slot nodes.

Rob

Re: [PATCH v3 1/6] dt-bindings: PCI: Add bindings for Brcmstb EP voltage regulators

[Rob Herring]

On Fri, Mar 26, 2021 at 03:18:59PM -0400, Jim Quinlan wrote:
> Similar to the regulator bindings found in "rockchip-pcie-host.txt", this
> allows optional regulators to be attached and controlled by the PCIe RC
> driver.  That being said, this driver searches in the DT subnode (the EP
> node, eg pci@0,0) for the regulator property.
> 
> The use of a regulator property in the pcie EP subnode such as
> "vpcie12v-supply" depends on a pending pullreq to the pci-bus.yaml
> file at
> 
> https://github.com/devicetree-org/dt-schema/pull/54
> 
> Signed-off-by: Jim Quinlan <jim2101024@gmail.com>
> ---
>  Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml b/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
> index f90557f6deb8..ea3e6f55e365 100644
> --- a/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
> +++ b/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
> @@ -156,5 +156,11 @@ examples:
>                                   <0x42000000 0x1 0x80000000 0x3 0x00000000 0x0 0x80000000>;
>                      brcm,enable-ssc;
>                      brcm,scb-sizes =  <0x0000000080000000 0x0000000080000000>;
> +
> +                    pcie-ep@0,0 {
> +                            reg = <0x0 0x0 0x0 0x0 0x0>;
> +                            compatible = "pci14e4,1688";
> +                            vpcie12v-supply: <&vreg12>;

For other cases, these properties are in the host bridge node. If these 
are standard PCI rails, then I think that's where they belong unless we 
define slot nodes.

Rob

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 1/6] dt-bindings: PCI: Add bindings for Brcmstb EP voltage regulators

[Mark Brown]

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

On Tue, Mar 30, 2021 at 10:08:16AM -0500, Rob Herring wrote:
> On Fri, Mar 26, 2021 at 03:18:59PM -0400, Jim Quinlan wrote:

> > +                    pcie-ep@0,0 {
> > +                            reg = <0x0 0x0 0x0 0x0 0x0>;
> > +                            compatible = "pci14e4,1688";
> > +                            vpcie12v-supply: <&vreg12>;

> For other cases, these properties are in the host bridge node. If these 
> are standard PCI rails, then I think that's where they belong unless we 
> define slot nodes.

For a soldered down part I'd expect we'd want both (if the host even
cares) - for anything except a supply that I/O or something else shared
is referenced off there's no great reason why it has to be physically
the same supply going to every device on the bus so each device should
be able to specify separately.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v3 1/6] dt-bindings: PCI: Add bindings for Brcmstb EP voltage regulators

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 852 bytes --]

On Tue, Mar 30, 2021 at 10:08:16AM -0500, Rob Herring wrote:
> On Fri, Mar 26, 2021 at 03:18:59PM -0400, Jim Quinlan wrote:

> > +                    pcie-ep@0,0 {
> > +                            reg = <0x0 0x0 0x0 0x0 0x0>;
> > +                            compatible = "pci14e4,1688";
> > +                            vpcie12v-supply: <&vreg12>;

> For other cases, these properties are in the host bridge node. If these 
> are standard PCI rails, then I think that's where they belong unless we 
> define slot nodes.

For a soldered down part I'd expect we'd want both (if the host even
cares) - for anything except a supply that I/O or something else shared
is referenced off there's no great reason why it has to be physically
the same supply going to every device on the bus so each device should
be able to specify separately.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 1/6] dt-bindings: PCI: Add bindings for Brcmstb EP voltage regulators

[Jim Quinlan]

On Tue, Mar 30, 2021 at 11:30 AM Mark Brown <broonie@kernel.org> wrote:
>10.22.8.121
> On Tue, Mar 30, 2021 at 10:08:16AM -0500, Rob Herring wrote:
> > On Fri, Mar 26, 2021 at 03:18:59PM -0400, Jim Quinlan wrote:
>
> > > +                    pcie-ep@0,0 {
> > > +                            reg = <0x0 0x0 0x0 0x0 0x0>;
> > > +                            compatible = "pci14e4,1688";
> > > +                            vpcie12v-supply: <&vreg12>;
>
> > For other cases, these properties are in the host bridge node. If these
> > are standard PCI rails, then I think that's where they belong unless we10.22.8.121
> > define slot nodes.
>
> For a soldered down part I'd expect we'd want both (if the host even
> cares) - for anything except a supply that I/O or something else shared
> is referenced off there's no great reason why it has to be physically
> the same supply going to every device on the bus so each device should
> be able to specify separately.
Our developer and reference boards frequently have Mini and half-mini
PCIe sockets (a few exceptions), whereas production boards are mostly
soldered down.

If I resubmit this pullreq  so that it  looks for "vpcie12v-supply"
and "vpcie3v3-supply" in the host node, will that be acceptable for
both of you?

Thanks,
Jim Quinlan
Broadcom STB

Re: [PATCH v3 1/6] dt-bindings: PCI: Add bindings for Brcmstb EP voltage regulators

[Jim Quinlan]

On Tue, Mar 30, 2021 at 11:30 AM Mark Brown <broonie@kernel.org> wrote:
>10.22.8.121
> On Tue, Mar 30, 2021 at 10:08:16AM -0500, Rob Herring wrote:
> > On Fri, Mar 26, 2021 at 03:18:59PM -0400, Jim Quinlan wrote:
>
> > > +                    pcie-ep@0,0 {
> > > +                            reg = <0x0 0x0 0x0 0x0 0x0>;
> > > +                            compatible = "pci14e4,1688";
> > > +                            vpcie12v-supply: <&vreg12>;
>
> > For other cases, these properties are in the host bridge node. If these
> > are standard PCI rails, then I think that's where they belong unless we10.22.8.121
> > define slot nodes.
>
> For a soldered down part I'd expect we'd want both (if the host even
> cares) - for anything except a supply that I/O or something else shared
> is referenced off there's no great reason why it has to be physically
> the same supply going to every device on the bus so each device should
> be able to specify separately.
Our developer and reference boards frequently have Mini and half-mini
PCIe sockets (a few exceptions), whereas production boards are mostly
soldered down.

If I resubmit this pullreq  so that it  looks for "vpcie12v-supply"
and "vpcie3v3-supply" in the host node, will that be acceptable for
both of you?

Thanks,
Jim Quinlan
Broadcom STB

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 1/6] dt-bindings: PCI: Add bindings for Brcmstb EP voltage regulators

[Mark Brown]

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

On Tue, Mar 30, 2021 at 12:23:35PM -0400, Jim Quinlan wrote:
> On Tue, Mar 30, 2021 at 11:30 AM Mark Brown <broonie@kernel.org> wrote:

> > For a soldered down part I'd expect we'd want both (if the host even
> > cares) - for anything except a supply that I/O or something else shared
> > is referenced off there's no great reason why it has to be physically
> > the same supply going to every device on the bus so each device should
> > be able to specify separately.

> Our developer and reference boards frequently have Mini and half-mini
> PCIe sockets (a few exceptions), whereas production boards are mostly
> soldered down.

On reflection I think the above probably also applies to sockets - you'd
just have to have a socket visible in the DT.

> If I resubmit this pullreq  so that it  looks for "vpcie12v-supply"
> and "vpcie3v3-supply" in the host node, will that be acceptable for
> both of you?

I think you will need both (assuming the controller actually physically
gets the supplies) - like I say the sockets/devices may not all share
the same 12V and 3.3V rails.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v3 1/6] dt-bindings: PCI: Add bindings for Brcmstb EP voltage regulators

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 1079 bytes --]

On Tue, Mar 30, 2021 at 12:23:35PM -0400, Jim Quinlan wrote:
> On Tue, Mar 30, 2021 at 11:30 AM Mark Brown <broonie@kernel.org> wrote:

> > For a soldered down part I'd expect we'd want both (if the host even
> > cares) - for anything except a supply that I/O or something else shared
> > is referenced off there's no great reason why it has to be physically
> > the same supply going to every device on the bus so each device should
> > be able to specify separately.

> Our developer and reference boards frequently have Mini and half-mini
> PCIe sockets (a few exceptions), whereas production boards are mostly
> soldered down.

On reflection I think the above probably also applies to sockets - you'd
just have to have a socket visible in the DT.

> If I resubmit this pullreq  so that it  looks for "vpcie12v-supply"
> and "vpcie3v3-supply" in the host node, will that be acceptable for
> both of you?

I think you will need both (assuming the controller actually physically
gets the supplies) - like I say the sockets/devices may not all share
the same 12V and 3.3V rails.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v3 2/6] PCI: brcmstb: Add control of EP voltage regulators

[Jim Quinlan]

Control of EP regulators by the RC is needed because of the chicken-and-egg
situation: although the regulator is "owned" by the EP and would be best
handled on its driver, the EP cannot be discovered and probed unless its
regulator is already turned on.

Signed-off-by: Jim Quinlan <jim2101024@gmail.com>
---
 drivers/pci/controller/pcie-brcmstb.c | 90 ++++++++++++++++++++++++++-
 1 file changed, 87 insertions(+), 3 deletions(-)

diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
index e330e6811f0b..b76ec7d9af32 100644
--- a/drivers/pci/controller/pcie-brcmstb.c
+++ b/drivers/pci/controller/pcie-brcmstb.c
@@ -24,6 +24,7 @@
 #include <linux/pci.h>
 #include <linux/pci-ecam.h>
 #include <linux/printk.h>
+#include <linux/regulator/consumer.h>
 #include <linux/reset.h>
 #include <linux/sizes.h>
 #include <linux/slab.h>
@@ -169,6 +170,7 @@
 #define SSC_STATUS_SSC_MASK		0x400
 #define SSC_STATUS_PLL_LOCK_MASK	0x800
 #define PCIE_BRCM_MAX_MEMC		3
+#define PCIE_BRCM_MAX_EP_REGULATORS	4
 
 #define IDX_ADDR(pcie)			(pcie->reg_offsets[EXT_CFG_INDEX])
 #define DATA_ADDR(pcie)			(pcie->reg_offsets[EXT_CFG_DATA])
@@ -295,8 +297,27 @@ struct brcm_pcie {
 	u32			hw_rev;
 	void			(*perst_set)(struct brcm_pcie *pcie, u32 val);
 	void			(*bridge_sw_init_set)(struct brcm_pcie *pcie, u32 val);
+	struct regulator_bulk_data supplies[PCIE_BRCM_MAX_EP_REGULATORS];
+	unsigned int		num_supplies;
 };
 
+static int brcm_set_regulators(struct brcm_pcie *pcie, bool on)
+{
+	struct device *dev = pcie->dev;
+	int ret;
+
+	if (!pcie->num_supplies)
+		return 0;
+	if (on)
+		ret = regulator_bulk_enable(pcie->num_supplies, pcie->supplies);
+	else
+		ret = regulator_bulk_disable(pcie->num_supplies, pcie->supplies);
+	if (ret)
+		dev_err(dev, "failed to %s EP regulators\n",
+			on ? "enable" : "disable");
+	return ret;
+}
+
 /*
  * This is to convert the size of the inbound "BAR" region to the
  * non-linear values of PCIE_X_MISC_RC_BAR[123]_CONFIG_LO.SIZE
@@ -1141,16 +1162,63 @@ static void brcm_pcie_turn_off(struct brcm_pcie *pcie)
 	pcie->bridge_sw_init_set(pcie, 1);
 }
 
+static int brcm_pcie_get_regulators(struct brcm_pcie *pcie)
+{
+	struct device_node *child, *parent = pcie->np;
+	const unsigned int max_name_len = 64 + 4;
+	struct property *pp;
+
+	/* Look for regulator supply property in the EP device subnodes */
+	for_each_available_child_of_node(parent, child) {
+		/*
+		 * Do a santiy test to ensure that this is an EP node
+		 * (e.g. node name: "pci-ep@0,0").  The slot number
+		 * should always be 0 as our controller only has a single
+		 * port.
+		 */
+		const char *p = strstr(child->full_name, "@0");
+
+		if (!p || (p[2] && p[2] != ','))
+			continue;
+
+		/* Now look for regulator supply properties */
+		for_each_property_of_node(child, pp) {
+			int i, n = strnlen(pp->name, max_name_len);
+
+			if (n <= 7 || strncmp("-supply", &pp->name[n - 7], 7))
+				continue;
+
+			/* Make sure this is not a duplicate */
+			for (i = 0; i < pcie->num_supplies; i++)
+				if (strncmp(pcie->supplies[i].supply,
+					    pp->name, max_name_len) == 0)
+					continue;
+
+			if (pcie->num_supplies < PCIE_BRCM_MAX_EP_REGULATORS)
+				pcie->supplies[pcie->num_supplies++].supply = pp->name;
+			else
+				dev_warn(pcie->dev, "No room for EP supply %s\n",
+					 pp->name);
+		}
+	}
+	/*
+	 * Get the regulators that the EP devices require.  We cannot use
+	 * pcie->dev as the device argument in regulator_bulk_get() since
+	 * it will not find the regulators.  Instead, use NULL and the
+	 * regulators are looked up by their name.
+	 */
+	return regulator_bulk_get(NULL, pcie->num_supplies, pcie->supplies);
+}
+
 static int brcm_pcie_suspend(struct device *dev)
 {
 	struct brcm_pcie *pcie = dev_get_drvdata(dev);
-	int ret;
 
 	brcm_pcie_turn_off(pcie);
-	ret = brcm_phy_stop(pcie);
+	brcm_phy_stop(pcie);
 	clk_disable_unprepare(pcie->clk);
 
-	return ret;
+	return brcm_set_regulators(pcie, false);
 }
 
 static int brcm_pcie_resume(struct device *dev)
@@ -1163,6 +1231,10 @@ static int brcm_pcie_resume(struct device *dev)
 	base = pcie->base;
 	clk_prepare_enable(pcie->clk);
 
+	ret = brcm_set_regulators(pcie, true);
+	if (ret)
+		return ret;
+
 	ret = brcm_phy_start(pcie);
 	if (ret)
 		goto err;
@@ -1199,6 +1271,8 @@ static void __brcm_pcie_remove(struct brcm_pcie *pcie)
 	brcm_phy_stop(pcie);
 	reset_control_assert(pcie->rescal);
 	clk_disable_unprepare(pcie->clk);
+	brcm_set_regulators(pcie, false);
+	regulator_bulk_free(pcie->num_supplies, pcie->supplies);
 }
 
 static int brcm_pcie_remove(struct platform_device *pdev)
@@ -1289,6 +1363,16 @@ static int brcm_pcie_probe(struct platform_device *pdev)
 		return ret;
 	}
 
+	ret = brcm_pcie_get_regulators(pcie);
+	if (ret) {
+		dev_err(pcie->dev, "failed to get regulators (err=%d)\n", ret);
+		goto fail;
+	}
+
+	ret = brcm_set_regulators(pcie, true);
+	if (ret)
+		goto fail;
+
 	ret = brcm_pcie_setup(pcie);
 	if (ret)
 		goto fail;
-- 
2.17.1

[PATCH v3 2/6] PCI: brcmstb: Add control of EP voltage regulators

[Jim Quinlan]

Control of EP regulators by the RC is needed because of the chicken-and-egg
situation: although the regulator is "owned" by the EP and would be best
handled on its driver, the EP cannot be discovered and probed unless its
regulator is already turned on.

Signed-off-by: Jim Quinlan <jim2101024@gmail.com>
---
 drivers/pci/controller/pcie-brcmstb.c | 90 ++++++++++++++++++++++++++-
 1 file changed, 87 insertions(+), 3 deletions(-)

diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
index e330e6811f0b..b76ec7d9af32 100644
--- a/drivers/pci/controller/pcie-brcmstb.c
+++ b/drivers/pci/controller/pcie-brcmstb.c
@@ -24,6 +24,7 @@
 #include <linux/pci.h>
 #include <linux/pci-ecam.h>
 #include <linux/printk.h>
+#include <linux/regulator/consumer.h>
 #include <linux/reset.h>
 #include <linux/sizes.h>
 #include <linux/slab.h>
@@ -169,6 +170,7 @@
 #define SSC_STATUS_SSC_MASK		0x400
 #define SSC_STATUS_PLL_LOCK_MASK	0x800
 #define PCIE_BRCM_MAX_MEMC		3
+#define PCIE_BRCM_MAX_EP_REGULATORS	4
 
 #define IDX_ADDR(pcie)			(pcie->reg_offsets[EXT_CFG_INDEX])
 #define DATA_ADDR(pcie)			(pcie->reg_offsets[EXT_CFG_DATA])
@@ -295,8 +297,27 @@ struct brcm_pcie {
 	u32			hw_rev;
 	void			(*perst_set)(struct brcm_pcie *pcie, u32 val);
 	void			(*bridge_sw_init_set)(struct brcm_pcie *pcie, u32 val);
+	struct regulator_bulk_data supplies[PCIE_BRCM_MAX_EP_REGULATORS];
+	unsigned int		num_supplies;
 };
 
+static int brcm_set_regulators(struct brcm_pcie *pcie, bool on)
+{
+	struct device *dev = pcie->dev;
+	int ret;
+
+	if (!pcie->num_supplies)
+		return 0;
+	if (on)
+		ret = regulator_bulk_enable(pcie->num_supplies, pcie->supplies);
+	else
+		ret = regulator_bulk_disable(pcie->num_supplies, pcie->supplies);
+	if (ret)
+		dev_err(dev, "failed to %s EP regulators\n",
+			on ? "enable" : "disable");
+	return ret;
+}
+
 /*
  * This is to convert the size of the inbound "BAR" region to the
  * non-linear values of PCIE_X_MISC_RC_BAR[123]_CONFIG_LO.SIZE
@@ -1141,16 +1162,63 @@ static void brcm_pcie_turn_off(struct brcm_pcie *pcie)
 	pcie->bridge_sw_init_set(pcie, 1);
 }
 
+static int brcm_pcie_get_regulators(struct brcm_pcie *pcie)
+{
+	struct device_node *child, *parent = pcie->np;
+	const unsigned int max_name_len = 64 + 4;
+	struct property *pp;
+
+	/* Look for regulator supply property in the EP device subnodes */
+	for_each_available_child_of_node(parent, child) {
+		/*
+		 * Do a santiy test to ensure that this is an EP node
+		 * (e.g. node name: "pci-ep@0,0").  The slot number
+		 * should always be 0 as our controller only has a single
+		 * port.
+		 */
+		const char *p = strstr(child->full_name, "@0");
+
+		if (!p || (p[2] && p[2] != ','))
+			continue;
+
+		/* Now look for regulator supply properties */
+		for_each_property_of_node(child, pp) {
+			int i, n = strnlen(pp->name, max_name_len);
+
+			if (n <= 7 || strncmp("-supply", &pp->name[n - 7], 7))
+				continue;
+
+			/* Make sure this is not a duplicate */
+			for (i = 0; i < pcie->num_supplies; i++)
+				if (strncmp(pcie->supplies[i].supply,
+					    pp->name, max_name_len) == 0)
+					continue;
+
+			if (pcie->num_supplies < PCIE_BRCM_MAX_EP_REGULATORS)
+				pcie->supplies[pcie->num_supplies++].supply = pp->name;
+			else
+				dev_warn(pcie->dev, "No room for EP supply %s\n",
+					 pp->name);
+		}
+	}
+	/*
+	 * Get the regulators that the EP devices require.  We cannot use
+	 * pcie->dev as the device argument in regulator_bulk_get() since
+	 * it will not find the regulators.  Instead, use NULL and the
+	 * regulators are looked up by their name.
+	 */
+	return regulator_bulk_get(NULL, pcie->num_supplies, pcie->supplies);
+}
+
 static int brcm_pcie_suspend(struct device *dev)
 {
 	struct brcm_pcie *pcie = dev_get_drvdata(dev);
-	int ret;
 
 	brcm_pcie_turn_off(pcie);
-	ret = brcm_phy_stop(pcie);
+	brcm_phy_stop(pcie);
 	clk_disable_unprepare(pcie->clk);
 
-	return ret;
+	return brcm_set_regulators(pcie, false);
 }
 
 static int brcm_pcie_resume(struct device *dev)
@@ -1163,6 +1231,10 @@ static int brcm_pcie_resume(struct device *dev)
 	base = pcie->base;
 	clk_prepare_enable(pcie->clk);
 
+	ret = brcm_set_regulators(pcie, true);
+	if (ret)
+		return ret;
+
 	ret = brcm_phy_start(pcie);
 	if (ret)
 		goto err;
@@ -1199,6 +1271,8 @@ static void __brcm_pcie_remove(struct brcm_pcie *pcie)
 	brcm_phy_stop(pcie);
 	reset_control_assert(pcie->rescal);
 	clk_disable_unprepare(pcie->clk);
+	brcm_set_regulators(pcie, false);
+	regulator_bulk_free(pcie->num_supplies, pcie->supplies);
 }
 
 static int brcm_pcie_remove(struct platform_device *pdev)
@@ -1289,6 +1363,16 @@ static int brcm_pcie_probe(struct platform_device *pdev)
 		return ret;
 	}
 
+	ret = brcm_pcie_get_regulators(pcie);
+	if (ret) {
+		dev_err(pcie->dev, "failed to get regulators (err=%d)\n", ret);
+		goto fail;
+	}
+
+	ret = brcm_set_regulators(pcie, true);
+	if (ret)
+		goto fail;
+
 	ret = brcm_pcie_setup(pcie);
 	if (ret)
 		goto fail;
-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 2/6] PCI: brcmstb: Add control of EP voltage regulators

[Bjorn Helgaas]

On Fri, Mar 26, 2021 at 03:19:00PM -0400, Jim Quinlan wrote:
> Control of EP regulators by the RC is needed because of the chicken-and-egg

Can you expand "EP"?  Not sure if this refers to "endpoint" or
something else.

If this refers to a device in a slot, I guess it isn't necessarily a
PCIe *endpoint*; it could also be a switch upstream port.

> situation: although the regulator is "owned" by the EP and would be best
> handled on its driver, the EP cannot be discovered and probed unless its
> regulator is already turned on.
> 
> Signed-off-by: Jim Quinlan <jim2101024@gmail.com>
> ---
>  drivers/pci/controller/pcie-brcmstb.c | 90 ++++++++++++++++++++++++++-
>  1 file changed, 87 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
> index e330e6811f0b..b76ec7d9af32 100644
> --- a/drivers/pci/controller/pcie-brcmstb.c
> +++ b/drivers/pci/controller/pcie-brcmstb.c
> @@ -24,6 +24,7 @@
>  #include <linux/pci.h>
>  #include <linux/pci-ecam.h>
>  #include <linux/printk.h>
> +#include <linux/regulator/consumer.h>
>  #include <linux/reset.h>
>  #include <linux/sizes.h>
>  #include <linux/slab.h>
> @@ -169,6 +170,7 @@
>  #define SSC_STATUS_SSC_MASK		0x400
>  #define SSC_STATUS_PLL_LOCK_MASK	0x800
>  #define PCIE_BRCM_MAX_MEMC		3
> +#define PCIE_BRCM_MAX_EP_REGULATORS	4
>  
>  #define IDX_ADDR(pcie)			(pcie->reg_offsets[EXT_CFG_INDEX])
>  #define DATA_ADDR(pcie)			(pcie->reg_offsets[EXT_CFG_DATA])
> @@ -295,8 +297,27 @@ struct brcm_pcie {
>  	u32			hw_rev;
>  	void			(*perst_set)(struct brcm_pcie *pcie, u32 val);
>  	void			(*bridge_sw_init_set)(struct brcm_pcie *pcie, u32 val);
> +	struct regulator_bulk_data supplies[PCIE_BRCM_MAX_EP_REGULATORS];
> +	unsigned int		num_supplies;
>  };
>  
> +static int brcm_set_regulators(struct brcm_pcie *pcie, bool on)
> +{
> +	struct device *dev = pcie->dev;
> +	int ret;
> +
> +	if (!pcie->num_supplies)
> +		return 0;
> +	if (on)
> +		ret = regulator_bulk_enable(pcie->num_supplies, pcie->supplies);
> +	else
> +		ret = regulator_bulk_disable(pcie->num_supplies, pcie->supplies);
> +	if (ret)
> +		dev_err(dev, "failed to %s EP regulators\n",
> +			on ? "enable" : "disable");
> +	return ret;
> +}
> +
>  /*
>   * This is to convert the size of the inbound "BAR" region to the
>   * non-linear values of PCIE_X_MISC_RC_BAR[123]_CONFIG_LO.SIZE
> @@ -1141,16 +1162,63 @@ static void brcm_pcie_turn_off(struct brcm_pcie *pcie)
>  	pcie->bridge_sw_init_set(pcie, 1);
>  }
>  
> +static int brcm_pcie_get_regulators(struct brcm_pcie *pcie)
> +{
> +	struct device_node *child, *parent = pcie->np;
> +	const unsigned int max_name_len = 64 + 4;
> +	struct property *pp;
> +
> +	/* Look for regulator supply property in the EP device subnodes */
> +	for_each_available_child_of_node(parent, child) {
> +		/*
> +		 * Do a santiy test to ensure that this is an EP node

s/santiy/sanity/

> +		 * (e.g. node name: "pci-ep@0,0").  The slot number
> +		 * should always be 0 as our controller only has a single
> +		 * port.
> +		 */
> +		const char *p = strstr(child->full_name, "@0");
> +
> +		if (!p || (p[2] && p[2] != ','))
> +			continue;
> +
> +		/* Now look for regulator supply properties */
> +		for_each_property_of_node(child, pp) {
> +			int i, n = strnlen(pp->name, max_name_len);
> +
> +			if (n <= 7 || strncmp("-supply", &pp->name[n - 7], 7))
> +				continue;
> +
> +			/* Make sure this is not a duplicate */
> +			for (i = 0; i < pcie->num_supplies; i++)
> +				if (strncmp(pcie->supplies[i].supply,
> +					    pp->name, max_name_len) == 0)
> +					continue;
> +
> +			if (pcie->num_supplies < PCIE_BRCM_MAX_EP_REGULATORS)
> +				pcie->supplies[pcie->num_supplies++].supply = pp->name;
> +			else
> +				dev_warn(pcie->dev, "No room for EP supply %s\n",
> +					 pp->name);
> +		}
> +	}
> +	/*
> +	 * Get the regulators that the EP devices require.  We cannot use
> +	 * pcie->dev as the device argument in regulator_bulk_get() since
> +	 * it will not find the regulators.  Instead, use NULL and the
> +	 * regulators are looked up by their name.

The comment doesn't explain the interesting part of why you need NULL
instead of "pcie->dev".  I assume it has something to do with the
platform topology and its DT description.

This appears to be the only instance in the whole kernel of a use of
regulator_bulk_get() or devm_regulator_bulk_get() with NULL.  That
definitely warrants a comment, so I'm glad you've got something here.

The regulator_bulk_get() function comment doesn't mention the
possibility of "dev == NULL", although regulator_dev_lookup(),
create_regulator(), device_link_add() do check for it being NULL, so I
guess it's not a surprise.  We may call dev_err(NULL), which I think
will *work* without crashing even though it will look like a mistake
on the output.

> +	 */
> +	return regulator_bulk_get(NULL, pcie->num_supplies, pcie->supplies);

devm_regulator_bulk_get()?

> +}
> +
>  static int brcm_pcie_suspend(struct device *dev)
>  {
>  	struct brcm_pcie *pcie = dev_get_drvdata(dev);
> -	int ret;
>  
>  	brcm_pcie_turn_off(pcie);
> -	ret = brcm_phy_stop(pcie);
> +	brcm_phy_stop(pcie);

If we no longer care whether brcm_phy_stop() returns an error, nobody
looks at the return value and it could be void.

>  	clk_disable_unprepare(pcie->clk);
>  
> -	return ret;
> +	return brcm_set_regulators(pcie, false);
>  }
>  
>  static int brcm_pcie_resume(struct device *dev)
> @@ -1163,6 +1231,10 @@ static int brcm_pcie_resume(struct device *dev)
>  	base = pcie->base;
>  	clk_prepare_enable(pcie->clk);
>  
> +	ret = brcm_set_regulators(pcie, true);
> +	if (ret)
> +		return ret;
> +
>  	ret = brcm_phy_start(pcie);
>  	if (ret)
>  		goto err;
> @@ -1199,6 +1271,8 @@ static void __brcm_pcie_remove(struct brcm_pcie *pcie)
>  	brcm_phy_stop(pcie);
>  	reset_control_assert(pcie->rescal);
>  	clk_disable_unprepare(pcie->clk);
> +	brcm_set_regulators(pcie, false);
> +	regulator_bulk_free(pcie->num_supplies, pcie->supplies);
>  }
>  
>  static int brcm_pcie_remove(struct platform_device *pdev)
> @@ -1289,6 +1363,16 @@ static int brcm_pcie_probe(struct platform_device *pdev)
>  		return ret;
>  	}
>  
> +	ret = brcm_pcie_get_regulators(pcie);
> +	if (ret) {
> +		dev_err(pcie->dev, "failed to get regulators (err=%d)\n", ret);
> +		goto fail;
> +	}
> +
> +	ret = brcm_set_regulators(pcie, true);
> +	if (ret)
> +		goto fail;
> +
>  	ret = brcm_pcie_setup(pcie);
>  	if (ret)
>  		goto fail;
> -- 
> 2.17.1
> 

Re: [PATCH v3 2/6] PCI: brcmstb: Add control of EP voltage regulators

[Bjorn Helgaas]

On Fri, Mar 26, 2021 at 03:19:00PM -0400, Jim Quinlan wrote:
> Control of EP regulators by the RC is needed because of the chicken-and-egg

Can you expand "EP"?  Not sure if this refers to "endpoint" or
something else.

If this refers to a device in a slot, I guess it isn't necessarily a
PCIe *endpoint*; it could also be a switch upstream port.

> situation: although the regulator is "owned" by the EP and would be best
> handled on its driver, the EP cannot be discovered and probed unless its
> regulator is already turned on.
> 
> Signed-off-by: Jim Quinlan <jim2101024@gmail.com>
> ---
>  drivers/pci/controller/pcie-brcmstb.c | 90 ++++++++++++++++++++++++++-
>  1 file changed, 87 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
> index e330e6811f0b..b76ec7d9af32 100644
> --- a/drivers/pci/controller/pcie-brcmstb.c
> +++ b/drivers/pci/controller/pcie-brcmstb.c
> @@ -24,6 +24,7 @@
>  #include <linux/pci.h>
>  #include <linux/pci-ecam.h>
>  #include <linux/printk.h>
> +#include <linux/regulator/consumer.h>
>  #include <linux/reset.h>
>  #include <linux/sizes.h>
>  #include <linux/slab.h>
> @@ -169,6 +170,7 @@
>  #define SSC_STATUS_SSC_MASK		0x400
>  #define SSC_STATUS_PLL_LOCK_MASK	0x800
>  #define PCIE_BRCM_MAX_MEMC		3
> +#define PCIE_BRCM_MAX_EP_REGULATORS	4
>  
>  #define IDX_ADDR(pcie)			(pcie->reg_offsets[EXT_CFG_INDEX])
>  #define DATA_ADDR(pcie)			(pcie->reg_offsets[EXT_CFG_DATA])
> @@ -295,8 +297,27 @@ struct brcm_pcie {
>  	u32			hw_rev;
>  	void			(*perst_set)(struct brcm_pcie *pcie, u32 val);
>  	void			(*bridge_sw_init_set)(struct brcm_pcie *pcie, u32 val);
> +	struct regulator_bulk_data supplies[PCIE_BRCM_MAX_EP_REGULATORS];
> +	unsigned int		num_supplies;
>  };
>  
> +static int brcm_set_regulators(struct brcm_pcie *pcie, bool on)
> +{
> +	struct device *dev = pcie->dev;
> +	int ret;
> +
> +	if (!pcie->num_supplies)
> +		return 0;
> +	if (on)
> +		ret = regulator_bulk_enable(pcie->num_supplies, pcie->supplies);
> +	else
> +		ret = regulator_bulk_disable(pcie->num_supplies, pcie->supplies);
> +	if (ret)
> +		dev_err(dev, "failed to %s EP regulators\n",
> +			on ? "enable" : "disable");
> +	return ret;
> +}
> +
>  /*
>   * This is to convert the size of the inbound "BAR" region to the
>   * non-linear values of PCIE_X_MISC_RC_BAR[123]_CONFIG_LO.SIZE
> @@ -1141,16 +1162,63 @@ static void brcm_pcie_turn_off(struct brcm_pcie *pcie)
>  	pcie->bridge_sw_init_set(pcie, 1);
>  }
>  
> +static int brcm_pcie_get_regulators(struct brcm_pcie *pcie)
> +{
> +	struct device_node *child, *parent = pcie->np;
> +	const unsigned int max_name_len = 64 + 4;
> +	struct property *pp;
> +
> +	/* Look for regulator supply property in the EP device subnodes */
> +	for_each_available_child_of_node(parent, child) {
> +		/*
> +		 * Do a santiy test to ensure that this is an EP node

s/santiy/sanity/

> +		 * (e.g. node name: "pci-ep@0,0").  The slot number
> +		 * should always be 0 as our controller only has a single
> +		 * port.
> +		 */
> +		const char *p = strstr(child->full_name, "@0");
> +
> +		if (!p || (p[2] && p[2] != ','))
> +			continue;
> +
> +		/* Now look for regulator supply properties */
> +		for_each_property_of_node(child, pp) {
> +			int i, n = strnlen(pp->name, max_name_len);
> +
> +			if (n <= 7 || strncmp("-supply", &pp->name[n - 7], 7))
> +				continue;
> +
> +			/* Make sure this is not a duplicate */
> +			for (i = 0; i < pcie->num_supplies; i++)
> +				if (strncmp(pcie->supplies[i].supply,
> +					    pp->name, max_name_len) == 0)
> +					continue;
> +
> +			if (pcie->num_supplies < PCIE_BRCM_MAX_EP_REGULATORS)
> +				pcie->supplies[pcie->num_supplies++].supply = pp->name;
> +			else
> +				dev_warn(pcie->dev, "No room for EP supply %s\n",
> +					 pp->name);
> +		}
> +	}
> +	/*
> +	 * Get the regulators that the EP devices require.  We cannot use
> +	 * pcie->dev as the device argument in regulator_bulk_get() since
> +	 * it will not find the regulators.  Instead, use NULL and the
> +	 * regulators are looked up by their name.

The comment doesn't explain the interesting part of why you need NULL
instead of "pcie->dev".  I assume it has something to do with the
platform topology and its DT description.

This appears to be the only instance in the whole kernel of a use of
regulator_bulk_get() or devm_regulator_bulk_get() with NULL.  That
definitely warrants a comment, so I'm glad you've got something here.

The regulator_bulk_get() function comment doesn't mention the
possibility of "dev == NULL", although regulator_dev_lookup(),
create_regulator(), device_link_add() do check for it being NULL, so I
guess it's not a surprise.  We may call dev_err(NULL), which I think
will *work* without crashing even though it will look like a mistake
on the output.

> +	 */
> +	return regulator_bulk_get(NULL, pcie->num_supplies, pcie->supplies);

devm_regulator_bulk_get()?

> +}
> +
>  static int brcm_pcie_suspend(struct device *dev)
>  {
>  	struct brcm_pcie *pcie = dev_get_drvdata(dev);
> -	int ret;
>  
>  	brcm_pcie_turn_off(pcie);
> -	ret = brcm_phy_stop(pcie);
> +	brcm_phy_stop(pcie);

If we no longer care whether brcm_phy_stop() returns an error, nobody
looks at the return value and it could be void.

>  	clk_disable_unprepare(pcie->clk);
>  
> -	return ret;
> +	return brcm_set_regulators(pcie, false);
>  }
>  
>  static int brcm_pcie_resume(struct device *dev)
> @@ -1163,6 +1231,10 @@ static int brcm_pcie_resume(struct device *dev)
>  	base = pcie->base;
>  	clk_prepare_enable(pcie->clk);
>  
> +	ret = brcm_set_regulators(pcie, true);
> +	if (ret)
> +		return ret;
> +
>  	ret = brcm_phy_start(pcie);
>  	if (ret)
>  		goto err;
> @@ -1199,6 +1271,8 @@ static void __brcm_pcie_remove(struct brcm_pcie *pcie)
>  	brcm_phy_stop(pcie);
>  	reset_control_assert(pcie->rescal);
>  	clk_disable_unprepare(pcie->clk);
> +	brcm_set_regulators(pcie, false);
> +	regulator_bulk_free(pcie->num_supplies, pcie->supplies);
>  }
>  
>  static int brcm_pcie_remove(struct platform_device *pdev)
> @@ -1289,6 +1363,16 @@ static int brcm_pcie_probe(struct platform_device *pdev)
>  		return ret;
>  	}
>  
> +	ret = brcm_pcie_get_regulators(pcie);
> +	if (ret) {
> +		dev_err(pcie->dev, "failed to get regulators (err=%d)\n", ret);
> +		goto fail;
> +	}
> +
> +	ret = brcm_set_regulators(pcie, true);
> +	if (ret)
> +		goto fail;
> +
>  	ret = brcm_pcie_setup(pcie);
>  	if (ret)
>  		goto fail;
> -- 
> 2.17.1
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 2/6] PCI: brcmstb: Add control of EP voltage regulators

[Jim Quinlan]

On Fri, Mar 26, 2021 at 4:11 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
>
> On Fri, Mar 26, 2021 at 03:19:00PM -0400, Jim Quinlan wrote:
> > Control of EP regulators by the RC is needed because of the chicken-and-egg
>
> Can you expand "EP"?  Not sure if this refers to "endpoint" or
> something else.
Yes I meant "endpoint" -- I will expand it.
>
> If this refers to a device in a slot, I guess it isn't necessarily aWe only support this feature for endpoint devices; it they hav
> PCIe *endpoint*; it could also be a switch upstream port.
True; to be precise I mean the device directly connected to the single RC port.
>
> > situation: although the regulator is "owned" by the EP and would be best
> > handled on its driver, the EP cannot be discovered and probed unless its
> > regulator is already turned on.
> >
> > Signed-off-by: Jim Quinlan <jim2101024@gmail.com>
> > ---
> >  drivers/pci/controller/pcie-brcmstb.c | 90 ++++++++++++++++++++++++++-
> >  1 file changed, 87 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
> > index e330e6811f0b..b76ec7d9af32 100644
> > --- a/drivers/pci/controller/pcie-brcmstb.c
> > +++ b/drivers/pci/controller/pcie-brcmstb.c
> > @@ -24,6 +24,7 @@
> >  #include <linux/pci.h>
> >  #include <linux/pci-ecam.h>
> >  #include <linux/printk.h>
> > +#include <linux/regulator/consumer.h>
> >  #include <linux/reset.h>
> >  #include <linux/sizes.h>
> >  #include <linux/slab.h>
> > @@ -169,6 +170,7 @@
> >  #define SSC_STATUS_SSC_MASK          0x400
> >  #define SSC_STATUS_PLL_LOCK_MASK     0x800
> >  #define PCIE_BRCM_MAX_MEMC           3
> > +#define PCIE_BRCM_MAX_EP_REGULATORS  4
> >
> >  #define IDX_ADDR(pcie)                       (pcie->reg_offsets[EXT_CFG_INDEX])
> >  #define DATA_ADDR(pcie)                      (pcie->reg_offsets[EXT_CFG_DATA])
> > @@ -295,8 +297,27 @@ struct brcm_pcie {
> >       u32                     hw_rev;
> >       void                    (*perst_set)(struct brcm_pcie *pcie, u32 val);
> >       void                    (*bridge_sw_init_set)(struct brcm_pcie *pcie, u32 val);
> > +     struct regulator_bulk_data supplies[PCIE_BRCM_MAX_EP_REGULATORS];
> > +     unsigned int            num_supplies;
> >  };
> >
> > +static int brcm_set_regulators(struct brcm_pcie *pcie, bool on)
> > +{
> > +     struct device *dev = pcie->dev;
> > +     int ret;
> > +
> > +     if (!pcie->num_supplies)
> > +             return 0;
> > +     if (on)
> > +             ret = regulator_bulk_enable(pcie->num_supplies, pcie->supplies);
> > +     else
> > +             ret = regulator_bulk_disable(pcie->num_supplies, pcie->supplies);
> > +     if (ret)
> > +             dev_err(dev, "failed to %s EP regulators\n",
> > +                     on ? "enable" : "disable");
> > +     return ret;
> > +}
> > +
> >  /*
> >   * This is to convert the size of the inbound "BAR" region to the
> >   * non-linear values of PCIE_X_MISC_RC_BAR[123]_CONFIG_LO.SIZE
> > @@ -1141,16 +1162,63 @@ static void brcm_pcie_turn_off(struct brcm_pcie *pcie)
> >       pcie->bridge_sw_init_set(pcie, 1);
> >  }
> >
> > +static int brcm_pcie_get_regulators(struct brcm_pcie *pcie)
> > +{
> > +     struct device_node *child, *parent = pcie->np;
> > +     const unsigned int max_name_len = 64 + 4;
> > +     struct property *pp;
> > +
> > +     /* Look for regulator supply property in the EP device subnodes */
> > +     for_each_available_child_of_node(parent, child) {
> > +             /*
> > +              * Do a santiy test to ensure that this is an EP node
>
> s/santiy/sanity/
>
> > +              * (e.g. node name: "pci-ep@0,0").  The slot number
> > +              * should always be 0 as our controller only has a single
> > +              * port.
> > +              */
> > +             const char *p = strstr(child->full_name, "@0");
> > +
> > +             if (!p || (p[2] && p[2] != ','))
> > +                     continue;
> > +
> > +             /* Now look for regulator supply properties */
> > +             for_each_property_of_node(child, pp) {
> > +                     int i, n = strnlen(pp->name, max_name_len);
> > +
> > +                     if (n <= 7 || strncmp("-supply", &pp->name[n - 7], 7))
> > +                             continue;
> > +
> > +                     /* Make sure this is not a duplicate */
> > +                     for (i = 0; i < pcie->num_supplies; i++)
> > +                             if (strncmp(pcie->supplies[i].supply,
> > +                                         pp->name, max_name_len) == 0)
> > +                                     continue;
> > +
> > +                     if (pcie->num_supplies < PCIE_BRCM_MAX_EP_REGULATORS)
> > +                             pcie->supplies[pcie->num_supplies++].supply = pp->name;
> > +                     else
> > +                             dev_warn(pcie->dev, "No room for EP supply %s\n",
> > +                                      pp->name);
> > +             }
> > +     }
> > +     /*
> > +      * Get the regulators that the EP devices require.  We cannot use
> > +      * pcie->dev as the device argument in regulator_bulk_get() since
> > +      * it will not find the regulators.  Instead, use NULL and the
> > +      * regulators are looked up by their name.
>
> The comment doesn't explain the interesting part of why you need NULL
> instead of "pcie->dev".  I assume it has something to do with the
> platform topology and its DT description.
>
> This appears to be the only instance in the whole kernel of a use of
> regulator_bulk_get() or devm_regulator_bulk_get() with NULL.  That
> definitely warrants a comment, so I'm glad you've got something here.
>
> The regulator_bulk_get() function comment doesn't mention the
> possibility of "dev == NULL", although regulator_dev_lookup(),
> create_regulator(), device_link_add() do check for it being NULL, so I
> guess it's not a surprise.  We may call dev_err(NULL), which I thinkWe only support this feature for endpoint devices; it they hav
> will *work* without crashing even though it will look like a mistake
> on the output.
Folks wanted me to put the "supply" in the endpoint subnode.  After
looking at the regulator code I assumed that using the pcie->dev in
this call would not work as the supply property is not in its DT node.
Turns out it works fine; I will fix it.
>
> > +      */
> > +     return regulator_bulk_get(NULL, pcie->num_supplies, pcie->supplies);
>
> devm_regulator_bulk_get()?
Yep.
>
> > +}
> > +
> >  static int brcm_pcie_suspend(struct device *dev)
> >  {
> >       struct brcm_pcie *pcie = dev_get_drvdata(dev);
> > -     int ret;
> >
> >       brcm_pcie_turn_off(pcie);
> > -     ret = brcm_phy_stop(pcie);
> > +     brcm_phy_stop(pcie);We only support this feature for endpoint devices; it they hav
>
> If we no longer care whether brcm_phy_stop() returns an error, nobody
> looks at the return value and it could be void.
Will fix.

Thanks,
Jim Quinlan
Broadcom STB
>
> >       clk_disable_unprepare(pcie->clk);
> >
> > -     return ret;
> > +     return brcm_set_regulators(pcie, false);
> >  }
> >
> >  static int brcm_pcie_resume(struct device *dev)
> > @@ -1163,6 +1231,10 @@ static int brcm_pcie_resume(struct device *dev)
> >       base = pcie->base;
> >       clk_prepare_enable(pcie->clk);
> >
> > +     ret = brcm_set_regulators(pcie, true);
> > +     if (ret)
> > +             return ret;
> > +
> >       ret = brcm_phy_start(pcie);
> >       if (ret)
> >               goto err;
> > @@ -1199,6 +1271,8 @@ static void __brcm_pcie_remove(struct brcm_pcie *pcie)
> >       brcm_phy_stop(pcie);
> >       reset_control_assert(pcie->rescal);
> >       clk_disable_unprepare(pcie->clk);
> > +     brcm_set_regulators(pcie, false);
> > +     regulator_bulk_free(pcie->num_supplies, pcie->supplies);
> >  }
> >
> >  static int brcm_pcie_remove(struct platform_device *pdev)
> > @@ -1289,6 +1363,16 @@ static int brcm_pcie_probe(struct platform_device *pdev)
> >               return ret;
> >       }
> >
> > +     ret = brcm_pcie_get_regulators(pcie);
> > +     if (ret) {
> > +             dev_err(pcie->dev, "failed to get regulators (err=%d)\n", ret);
> > +             goto fail;
> > +     }
> > +
> > +     ret = brcm_set_regulators(pcie, true);
> > +     if (ret)
> > +             goto fail;
> > +
> >       ret = brcm_pcie_setup(pcie);
> >       if (ret)
> >               goto fail;
> > --
> > 2.17.1
> >

Re: [PATCH v3 2/6] PCI: brcmstb: Add control of EP voltage regulators

[Jim Quinlan]

On Fri, Mar 26, 2021 at 4:11 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
>
> On Fri, Mar 26, 2021 at 03:19:00PM -0400, Jim Quinlan wrote:
> > Control of EP regulators by the RC is needed because of the chicken-and-egg
>
> Can you expand "EP"?  Not sure if this refers to "endpoint" or
> something else.
Yes I meant "endpoint" -- I will expand it.
>
> If this refers to a device in a slot, I guess it isn't necessarily aWe only support this feature for endpoint devices; it they hav
> PCIe *endpoint*; it could also be a switch upstream port.
True; to be precise I mean the device directly connected to the single RC port.
>
> > situation: although the regulator is "owned" by the EP and would be best
> > handled on its driver, the EP cannot be discovered and probed unless its
> > regulator is already turned on.
> >
> > Signed-off-by: Jim Quinlan <jim2101024@gmail.com>
> > ---
> >  drivers/pci/controller/pcie-brcmstb.c | 90 ++++++++++++++++++++++++++-
> >  1 file changed, 87 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
> > index e330e6811f0b..b76ec7d9af32 100644
> > --- a/drivers/pci/controller/pcie-brcmstb.c
> > +++ b/drivers/pci/controller/pcie-brcmstb.c
> > @@ -24,6 +24,7 @@
> >  #include <linux/pci.h>
> >  #include <linux/pci-ecam.h>
> >  #include <linux/printk.h>
> > +#include <linux/regulator/consumer.h>
> >  #include <linux/reset.h>
> >  #include <linux/sizes.h>
> >  #include <linux/slab.h>
> > @@ -169,6 +170,7 @@
> >  #define SSC_STATUS_SSC_MASK          0x400
> >  #define SSC_STATUS_PLL_LOCK_MASK     0x800
> >  #define PCIE_BRCM_MAX_MEMC           3
> > +#define PCIE_BRCM_MAX_EP_REGULATORS  4
> >
> >  #define IDX_ADDR(pcie)                       (pcie->reg_offsets[EXT_CFG_INDEX])
> >  #define DATA_ADDR(pcie)                      (pcie->reg_offsets[EXT_CFG_DATA])
> > @@ -295,8 +297,27 @@ struct brcm_pcie {
> >       u32                     hw_rev;
> >       void                    (*perst_set)(struct brcm_pcie *pcie, u32 val);
> >       void                    (*bridge_sw_init_set)(struct brcm_pcie *pcie, u32 val);
> > +     struct regulator_bulk_data supplies[PCIE_BRCM_MAX_EP_REGULATORS];
> > +     unsigned int            num_supplies;
> >  };
> >
> > +static int brcm_set_regulators(struct brcm_pcie *pcie, bool on)
> > +{
> > +     struct device *dev = pcie->dev;
> > +     int ret;
> > +
> > +     if (!pcie->num_supplies)
> > +             return 0;
> > +     if (on)
> > +             ret = regulator_bulk_enable(pcie->num_supplies, pcie->supplies);
> > +     else
> > +             ret = regulator_bulk_disable(pcie->num_supplies, pcie->supplies);
> > +     if (ret)
> > +             dev_err(dev, "failed to %s EP regulators\n",
> > +                     on ? "enable" : "disable");
> > +     return ret;
> > +}
> > +
> >  /*
> >   * This is to convert the size of the inbound "BAR" region to the
> >   * non-linear values of PCIE_X_MISC_RC_BAR[123]_CONFIG_LO.SIZE
> > @@ -1141,16 +1162,63 @@ static void brcm_pcie_turn_off(struct brcm_pcie *pcie)
> >       pcie->bridge_sw_init_set(pcie, 1);
> >  }
> >
> > +static int brcm_pcie_get_regulators(struct brcm_pcie *pcie)
> > +{
> > +     struct device_node *child, *parent = pcie->np;
> > +     const unsigned int max_name_len = 64 + 4;
> > +     struct property *pp;
> > +
> > +     /* Look for regulator supply property in the EP device subnodes */
> > +     for_each_available_child_of_node(parent, child) {
> > +             /*
> > +              * Do a santiy test to ensure that this is an EP node
>
> s/santiy/sanity/
>
> > +              * (e.g. node name: "pci-ep@0,0").  The slot number
> > +              * should always be 0 as our controller only has a single
> > +              * port.
> > +              */
> > +             const char *p = strstr(child->full_name, "@0");
> > +
> > +             if (!p || (p[2] && p[2] != ','))
> > +                     continue;
> > +
> > +             /* Now look for regulator supply properties */
> > +             for_each_property_of_node(child, pp) {
> > +                     int i, n = strnlen(pp->name, max_name_len);
> > +
> > +                     if (n <= 7 || strncmp("-supply", &pp->name[n - 7], 7))
> > +                             continue;
> > +
> > +                     /* Make sure this is not a duplicate */
> > +                     for (i = 0; i < pcie->num_supplies; i++)
> > +                             if (strncmp(pcie->supplies[i].supply,
> > +                                         pp->name, max_name_len) == 0)
> > +                                     continue;
> > +
> > +                     if (pcie->num_supplies < PCIE_BRCM_MAX_EP_REGULATORS)
> > +                             pcie->supplies[pcie->num_supplies++].supply = pp->name;
> > +                     else
> > +                             dev_warn(pcie->dev, "No room for EP supply %s\n",
> > +                                      pp->name);
> > +             }
> > +     }
> > +     /*
> > +      * Get the regulators that the EP devices require.  We cannot use
> > +      * pcie->dev as the device argument in regulator_bulk_get() since
> > +      * it will not find the regulators.  Instead, use NULL and the
> > +      * regulators are looked up by their name.
>
> The comment doesn't explain the interesting part of why you need NULL
> instead of "pcie->dev".  I assume it has something to do with the
> platform topology and its DT description.
>
> This appears to be the only instance in the whole kernel of a use of
> regulator_bulk_get() or devm_regulator_bulk_get() with NULL.  That
> definitely warrants a comment, so I'm glad you've got something here.
>
> The regulator_bulk_get() function comment doesn't mention the
> possibility of "dev == NULL", although regulator_dev_lookup(),
> create_regulator(), device_link_add() do check for it being NULL, so I
> guess it's not a surprise.  We may call dev_err(NULL), which I thinkWe only support this feature for endpoint devices; it they hav
> will *work* without crashing even though it will look like a mistake
> on the output.
Folks wanted me to put the "supply" in the endpoint subnode.  After
looking at the regulator code I assumed that using the pcie->dev in
this call would not work as the supply property is not in its DT node.
Turns out it works fine; I will fix it.
>
> > +      */
> > +     return regulator_bulk_get(NULL, pcie->num_supplies, pcie->supplies);
>
> devm_regulator_bulk_get()?
Yep.
>
> > +}
> > +
> >  static int brcm_pcie_suspend(struct device *dev)
> >  {
> >       struct brcm_pcie *pcie = dev_get_drvdata(dev);
> > -     int ret;
> >
> >       brcm_pcie_turn_off(pcie);
> > -     ret = brcm_phy_stop(pcie);
> > +     brcm_phy_stop(pcie);We only support this feature for endpoint devices; it they hav
>
> If we no longer care whether brcm_phy_stop() returns an error, nobody
> looks at the return value and it could be void.
Will fix.

Thanks,
Jim Quinlan
Broadcom STB
>
> >       clk_disable_unprepare(pcie->clk);
> >
> > -     return ret;
> > +     return brcm_set_regulators(pcie, false);
> >  }
> >
> >  static int brcm_pcie_resume(struct device *dev)
> > @@ -1163,6 +1231,10 @@ static int brcm_pcie_resume(struct device *dev)
> >       base = pcie->base;
> >       clk_prepare_enable(pcie->clk);
> >
> > +     ret = brcm_set_regulators(pcie, true);
> > +     if (ret)
> > +             return ret;
> > +
> >       ret = brcm_phy_start(pcie);
> >       if (ret)
> >               goto err;
> > @@ -1199,6 +1271,8 @@ static void __brcm_pcie_remove(struct brcm_pcie *pcie)
> >       brcm_phy_stop(pcie);
> >       reset_control_assert(pcie->rescal);
> >       clk_disable_unprepare(pcie->clk);
> > +     brcm_set_regulators(pcie, false);
> > +     regulator_bulk_free(pcie->num_supplies, pcie->supplies);
> >  }
> >
> >  static int brcm_pcie_remove(struct platform_device *pdev)
> > @@ -1289,6 +1363,16 @@ static int brcm_pcie_probe(struct platform_device *pdev)
> >               return ret;
> >       }
> >
> > +     ret = brcm_pcie_get_regulators(pcie);
> > +     if (ret) {
> > +             dev_err(pcie->dev, "failed to get regulators (err=%d)\n", ret);
> > +             goto fail;
> > +     }
> > +
> > +     ret = brcm_set_regulators(pcie, true);
> > +     if (ret)
> > +             goto fail;
> > +
> >       ret = brcm_pcie_setup(pcie);
> >       if (ret)
> >               goto fail;
> > --
> > 2.17.1
> >

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 2/6] PCI: brcmstb: Add control of EP voltage regulators

[Mark Brown]

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

On Fri, Mar 26, 2021 at 03:19:00PM -0400, Jim Quinlan wrote:

> Control of EP regulators by the RC is needed because of the chicken-and-egg
> situation: although the regulator is "owned" by the EP and would be best
> handled on its driver, the EP cannot be discovered and probed unless its
> regulator is already turned on.

Ideally the driver core would have a way for buses to register devices
pre physical enumeration and give drivers a callback that could be used
to do whatever is needed to trigger enumeration, letting the bus then
match newly physically enumerated devices with the software enumerated
struct devices.  Soundwire has something in this area for a bus level
solution.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v3 2/6] PCI: brcmstb: Add control of EP voltage regulators

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 689 bytes --]

On Fri, Mar 26, 2021 at 03:19:00PM -0400, Jim Quinlan wrote:

> Control of EP regulators by the RC is needed because of the chicken-and-egg
> situation: although the regulator is "owned" by the EP and would be best
> handled on its driver, the EP cannot be discovered and probed unless its
> regulator is already turned on.

Ideally the driver core would have a way for buses to register devices
pre physical enumeration and give drivers a callback that could be used
to do whatever is needed to trigger enumeration, letting the bus then
match newly physically enumerated devices with the software enumerated
struct devices.  Soundwire has something in this area for a bus level
solution.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 2/6] PCI: brcmstb: Add control of EP voltage regulators

[Mark Brown]

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

On Fri, Mar 26, 2021 at 03:19:00PM -0400, Jim Quinlan wrote:

> +		/* Now look for regulator supply properties */
> +		for_each_property_of_node(child, pp) {
> +			int i, n = strnlen(pp->name, max_name_len);
> +
> +			if (n <= 7 || strncmp("-supply", &pp->name[n - 7], 7))
> +				continue;

Here you are figuring out a device local supply name...

> +	/*
> +	 * Get the regulators that the EP devices require.  We cannot use
> +	 * pcie->dev as the device argument in regulator_bulk_get() since
> +	 * it will not find the regulators.  Instead, use NULL and the
> +	 * regulators are looked up by their name.
> +	 */
> +	return regulator_bulk_get(NULL, pcie->num_supplies, pcie->supplies);

...and here you are trying to look up that device local name in the
global namespace.  That's not going to work well, the global names that
supplies are labelled with may be completely different to what the chip
designer called them and there could easily be naming collisions between
different chips.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v3 2/6] PCI: brcmstb: Add control of EP voltage regulators

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 993 bytes --]

On Fri, Mar 26, 2021 at 03:19:00PM -0400, Jim Quinlan wrote:

> +		/* Now look for regulator supply properties */
> +		for_each_property_of_node(child, pp) {
> +			int i, n = strnlen(pp->name, max_name_len);
> +
> +			if (n <= 7 || strncmp("-supply", &pp->name[n - 7], 7))
> +				continue;

Here you are figuring out a device local supply name...

> +	/*
> +	 * Get the regulators that the EP devices require.  We cannot use
> +	 * pcie->dev as the device argument in regulator_bulk_get() since
> +	 * it will not find the regulators.  Instead, use NULL and the
> +	 * regulators are looked up by their name.
> +	 */
> +	return regulator_bulk_get(NULL, pcie->num_supplies, pcie->supplies);

...and here you are trying to look up that device local name in the
global namespace.  That's not going to work well, the global names that
supplies are labelled with may be completely different to what the chip
designer called them and there could easily be naming collisions between
different chips.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 2/6] PCI: brcmstb: Add control of EP voltage regulators

[Jim Quinlan]

On Mon, Mar 29, 2021 at 12:25 PM Mark Brown <broonie@kernel.org>
w./lib/python3.6/site-packages/dtschema/schemasrote:
>
> On Fri, Mar 26, 2021 at 03:19:00PM -0400, Jim Quinlan wrote:
>
> > +             /* Now look for regulator supply properties */
> > +             for_each_property_of_node(child, pp) {
> > +                     int i, n = strnlen(pp->name, max_name_len);
> > +
> > +                     if (n <= 7 || strncmp("-supply", &pp->name[n - 7], 7))
> > +                             continue;
>
> Here you are figuring out a device local supply name...
>
> > +     /*
> > +      * Get the regulators that the EP devianswerces require.  We cannot use
> > +      * pcie->dev as the device argument in regulator_bulk_get() since
> > +      * it will not find the regulators.  Instead, use NULL and the
> > +      * regulators are looked up by their name.
> > +      */
> > +     return regulator_bulk_get(NULL, pcie->num_supplies, pcie->supplies);
>
> ...and here you are trying to look up that device local name in the
> global namespace.  That's not going to work well, the global names that
> supplies are labelled with may be completely different to what the chip
> designer called them and there could easily be naming collisions between
> different chips.

Hello Mark,
I am re-submitting this pullreq using
"devm_regulator_bulk_get(pcie->dev, ...)"; is your concern about the
NULL for the device and if so does this fix it?  If not, what do you
suggest that I do?
Thanks,
Jim

Re: [PATCH v3 2/6] PCI: brcmstb: Add control of EP voltage regulators

[Jim Quinlan]

On Mon, Mar 29, 2021 at 12:25 PM Mark Brown <broonie@kernel.org>
w./lib/python3.6/site-packages/dtschema/schemasrote:
>
> On Fri, Mar 26, 2021 at 03:19:00PM -0400, Jim Quinlan wrote:
>
> > +             /* Now look for regulator supply properties */
> > +             for_each_property_of_node(child, pp) {
> > +                     int i, n = strnlen(pp->name, max_name_len);
> > +
> > +                     if (n <= 7 || strncmp("-supply", &pp->name[n - 7], 7))
> > +                             continue;
>
> Here you are figuring out a device local supply name...
>
> > +     /*
> > +      * Get the regulators that the EP devianswerces require.  We cannot use
> > +      * pcie->dev as the device argument in regulator_bulk_get() since
> > +      * it will not find the regulators.  Instead, use NULL and the
> > +      * regulators are looked up by their name.
> > +      */
> > +     return regulator_bulk_get(NULL, pcie->num_supplies, pcie->supplies);
>
> ...and here you are trying to look up that device local name in the
> global namespace.  That's not going to work well, the global names that
> supplies are labelled with may be completely different to what the chip
> designer called them and there could easily be naming collisions between
> different chips.

Hello Mark,
I am re-submitting this pullreq using
"devm_regulator_bulk_get(pcie->dev, ...)"; is your concern about the
NULL for the device and if so does this fix it?  If not, what do you
suggest that I do?
Thanks,
Jim

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 2/6] PCI: brcmstb: Add control of EP voltage regulators

[Mark Brown]

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

On Mon, Mar 29, 2021 at 12:39:50PM -0400, Jim Quinlan wrote:
> On Mon, Mar 29, 2021 at 12:25 PM Mark Brown <broonie@kernel.org>

> > Here you are figuring out a device local supply name...

> > > +     /*
> > > +      * Get the regulators that the EP devianswerces require.  We cannot use
> > > +      * pcie->dev as the device argument in regulator_bulk_get() since
> > > +      * it will not find the regulators.  Instead, use NULL and the
> > > +      * regulators are looked up by their name.
> > > +      */
> > > +     return regulator_bulk_get(NULL, pcie->num_supplies, pcie->supplies);

> > ...and here you are trying to look up that device local name in the
> > global namespace.  That's not going to work well, the global names that
> > supplies are labelled with may be completely different to what the chip
> > designer called them and there could easily be naming collisions between
> > different chips.

> "devm_regulator_bulk_get(pcie->dev, ...)"; is your concern about the
> NULL for the device and if so does this fix it?  If not, what do you
> suggest that I do?

If you use the struct device for the PCIe controller then that's going
to first try the PCIe controller then the global namespace so you still
have all the same problems.  You really need to use the struct device
for the device that is being supplied not some random other struct
device you happened to find in the system.

As I said in my earlier reply I think either the driver core or PCI
needs something like Soundwire has which lets it create struct devices
for things that have been enumerated via software but not enumerated by
hardware and a callback or something which lets those devices take
whatever steps are needed to trigger probe.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v3 2/6] PCI: brcmstb: Add control of EP voltage regulators

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 1728 bytes --]

On Mon, Mar 29, 2021 at 12:39:50PM -0400, Jim Quinlan wrote:
> On Mon, Mar 29, 2021 at 12:25 PM Mark Brown <broonie@kernel.org>

> > Here you are figuring out a device local supply name...

> > > +     /*
> > > +      * Get the regulators that the EP devianswerces require.  We cannot use
> > > +      * pcie->dev as the device argument in regulator_bulk_get() since
> > > +      * it will not find the regulators.  Instead, use NULL and the
> > > +      * regulators are looked up by their name.
> > > +      */
> > > +     return regulator_bulk_get(NULL, pcie->num_supplies, pcie->supplies);

> > ...and here you are trying to look up that device local name in the
> > global namespace.  That's not going to work well, the global names that
> > supplies are labelled with may be completely different to what the chip
> > designer called them and there could easily be naming collisions between
> > different chips.

> "devm_regulator_bulk_get(pcie->dev, ...)"; is your concern about the
> NULL for the device and if so does this fix it?  If not, what do you
> suggest that I do?

If you use the struct device for the PCIe controller then that's going
to first try the PCIe controller then the global namespace so you still
have all the same problems.  You really need to use the struct device
for the device that is being supplied not some random other struct
device you happened to find in the system.

As I said in my earlier reply I think either the driver core or PCI
needs something like Soundwire has which lets it create struct devices
for things that have been enumerated via software but not enumerated by
hardware and a callback or something which lets those devices take
whatever steps are needed to trigger probe.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 2/6] PCI: brcmstb: Add control of EP voltage regulators

[Jim Quinlan]

    /* Pmap_idx to avs pmap number */
    const uint8_t pmap_idx_to_avs_id[20];

On Mon, Mar 29, 2021 at 1:16 PM Mark Brown <broonie@kernel.org> wrote:
>
> On Mon, Mar 29, 2021 at 12:39:50PM -0400, Jim Quinlan wrote:
> > On Mon, Mar 29, 2021 at 12:25 PM Mark Brown <broonie@kernel.org>
>
> > > Here you are figuring out a device local supply name...
>
> > > > +     /*
> > > > +      * Get the regulators that the EP devianswerces require.  We cannot use
> > > > +      * pcie->dev as the device argument in regulator_bulk_get() since
> > > > +      * it will not find the regulators.  Instead, use NULL and the
> > > > +      * regulators are looked up by their name.
> > > > +      */
> > > > +     return regulator_bulk_get(NULL, pcie->num_supplies, pcie->supplies);
>
> > > ...and here you are trying to look up that device local name in the
> > > global namespace.  That's not going to work well, the global names that
> > > supplies are labelled with may be completely different to what the chip
> > > designer called them and there could easily be naming collisions between
> > > different chips.
>
> > "devm_regulator_bulk_get(pcie->dev, ...)"; is your concern about the
> > NULL for the device and if so does this fix it?  If not, what do you
> > suggest that I do?
>
> If you use the struct device for the PCIe controller then that's going
> to first try the PCIe controller then the global namespace so you still
> have all the same problems.  You really need to use the struct device
> for the device that is being supplied not some random other struct
> device you happened to find in the system.
Hello Mark,
I'm not concerned about a namespace collision and I don't think you
should be concerned either.  First, this driver is for Broadcom STB
PCIe chips and boards, and we also deliver the DT to the customers.
We typically do not have any other regulators in the DT besides the
ones I am proposing.  For example, the 7216 SOC DT has 0 other
regulators -- no namespace collision possible.  Our DT-generating
scripts also flag namespace issues.  I admit that this driver is also
used by RPi chips, but I can easily exclude this feature from the RPI
if Nicolas has any objection.

Further, if you want, I can restrict the search to the two regulators
I am proposing to add to pci-bus.yaml:  "vpcie12v-supply" and
"vpcie3v3-supply".

Is the above enough to alleviate your concerns about global namespace collision?

>
> As I said in my earlier reply I think either the driver core or PCI
> needs something like Soundwire has which lets it create struct devices
> for things that have been enumerated via software but not enumerated by
> hardware and a callback or something which lets those devices take
> whatever steps are needed to trigger probe.

Can you please elaborate this further and in detail?  This sounds like
a decent-size undertaking, and the last time I followed a review
suggestion that was similar in spirit to this, the pullreq was
ironically NAKed by the person who suggested it.  Do you really think
it is necessary to construct such a subsystem just to avoid the  very
remote possibility of a namespace collision which is our (Broadcom
STB) responsibility and consequence to avoid?

Regards,
Jim Quinlan
Broadcom STB

Re: [PATCH v3 2/6] PCI: brcmstb: Add control of EP voltage regulators

[Jim Quinlan]

    /* Pmap_idx to avs pmap number */
    const uint8_t pmap_idx_to_avs_id[20];

On Mon, Mar 29, 2021 at 1:16 PM Mark Brown <broonie@kernel.org> wrote:
>
> On Mon, Mar 29, 2021 at 12:39:50PM -0400, Jim Quinlan wrote:
> > On Mon, Mar 29, 2021 at 12:25 PM Mark Brown <broonie@kernel.org>
>
> > > Here you are figuring out a device local supply name...
>
> > > > +     /*
> > > > +      * Get the regulators that the EP devianswerces require.  We cannot use
> > > > +      * pcie->dev as the device argument in regulator_bulk_get() since
> > > > +      * it will not find the regulators.  Instead, use NULL and the
> > > > +      * regulators are looked up by their name.
> > > > +      */
> > > > +     return regulator_bulk_get(NULL, pcie->num_supplies, pcie->supplies);
>
> > > ...and here you are trying to look up that device local name in the
> > > global namespace.  That's not going to work well, the global names that
> > > supplies are labelled with may be completely different to what the chip
> > > designer called them and there could easily be naming collisions between
> > > different chips.
>
> > "devm_regulator_bulk_get(pcie->dev, ...)"; is your concern about the
> > NULL for the device and if so does this fix it?  If not, what do you
> > suggest that I do?
>
> If you use the struct device for the PCIe controller then that's going
> to first try the PCIe controller then the global namespace so you still
> have all the same problems.  You really need to use the struct device
> for the device that is being supplied not some random other struct
> device you happened to find in the system.
Hello Mark,
I'm not concerned about a namespace collision and I don't think you
should be concerned either.  First, this driver is for Broadcom STB
PCIe chips and boards, and we also deliver the DT to the customers.
We typically do not have any other regulators in the DT besides the
ones I am proposing.  For example, the 7216 SOC DT has 0 other
regulators -- no namespace collision possible.  Our DT-generating
scripts also flag namespace issues.  I admit that this driver is also
used by RPi chips, but I can easily exclude this feature from the RPI
if Nicolas has any objection.

Further, if you want, I can restrict the search to the two regulators
I am proposing to add to pci-bus.yaml:  "vpcie12v-supply" and
"vpcie3v3-supply".

Is the above enough to alleviate your concerns about global namespace collision?

>
> As I said in my earlier reply I think either the driver core or PCI
> needs something like Soundwire has which lets it create struct devices
> for things that have been enumerated via software but not enumerated by
> hardware and a callback or something which lets those devices take
> whatever steps are needed to trigger probe.

Can you please elaborate this further and in detail?  This sounds like
a decent-size undertaking, and the last time I followed a review
suggestion that was similar in spirit to this, the pullreq was
ironically NAKed by the person who suggested it.  Do you really think
it is necessary to construct such a subsystem just to avoid the  very
remote possibility of a namespace collision which is our (Broadcom
STB) responsibility and consequence to avoid?

Regards,
Jim Quinlan
Broadcom STB

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 2/6] PCI: brcmstb: Add control of EP voltage regulators

[Mark Brown]

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

On Mon, Mar 29, 2021 at 03:48:46PM -0400, Jim Quinlan wrote:

> I'm not concerned about a namespace collision and I don't think you
> should be concerned either.  First, this driver is for Broadcom STB
> PCIe chips and boards, and we also deliver the DT to the customers.
> We typically do not have any other regulators in the DT besides the
> ones I am proposing.  For example, the 7216 SOC DT has 0 other

You may not describe these regulators in the DT but you must have other
regulators in your system, most devices need power to operate.  In any
case "this works for me with my DT on my system and nobody will ever
change our reference design" isn't really a great approach, frankly it's
not a claim I entirely believe and even if it turns out to be true for
your systems if we establish this as being how regulators work for
soldered down PCI devices everyone else is going to want to do the same
thing, most likely making the same claims for how much control they have
over the systems things will run on.

> regulators -- no namespace collision possible.  Our DT-generating
> scripts also flag namespace issues.  I admit that this driver is also
> used by RPi chips, but I can easily exclude this feature from the RPI
> if Nicolas has any objection.

That's certainly an issue, obviously the RPI is the sort of system where
I'd imagine this would be particularly useful.

> Further, if you want, I can restrict the search to the two regulators
> I am proposing to add to pci-bus.yaml:  "vpcie12v-supply" and
> "vpcie3v3-supply".

No, that doesn't help - what happens if someone uses separate regulators
for different PCI devices?  The reason the supplies are device namespaced
is that each device can look up it's own supplies and label them how it
wants without reference to anything else on the board.  Alternatively
what happens if some device has another supply it needs to power on (eg,
something that wants a clean LDO output for analogue use)?

> Is the above enough to alleviate your concerns about global namespace collision?

Not really.  TBH it looks like this driver is keeping the regulators
enabled all the time except for suspend and resume anyway, if that's all
that's going on I'd have thought that you wouldn't need any explicit
management in the driver anyway?  Just mark the regualtors as always on
and set up an appropriate suspend mode configuration and everything
should work without the drivers doing anything.  Unless your PMIC isn't
able to provide separate suspend mode configuration for the regulators?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v3 2/6] PCI: brcmstb: Add control of EP voltage regulators

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 2539 bytes --]

On Mon, Mar 29, 2021 at 03:48:46PM -0400, Jim Quinlan wrote:

> I'm not concerned about a namespace collision and I don't think you
> should be concerned either.  First, this driver is for Broadcom STB
> PCIe chips and boards, and we also deliver the DT to the customers.
> We typically do not have any other regulators in the DT besides the
> ones I am proposing.  For example, the 7216 SOC DT has 0 other

You may not describe these regulators in the DT but you must have other
regulators in your system, most devices need power to operate.  In any
case "this works for me with my DT on my system and nobody will ever
change our reference design" isn't really a great approach, frankly it's
not a claim I entirely believe and even if it turns out to be true for
your systems if we establish this as being how regulators work for
soldered down PCI devices everyone else is going to want to do the same
thing, most likely making the same claims for how much control they have
over the systems things will run on.

> regulators -- no namespace collision possible.  Our DT-generating
> scripts also flag namespace issues.  I admit that this driver is also
> used by RPi chips, but I can easily exclude this feature from the RPI
> if Nicolas has any objection.

That's certainly an issue, obviously the RPI is the sort of system where
I'd imagine this would be particularly useful.

> Further, if you want, I can restrict the search to the two regulators
> I am proposing to add to pci-bus.yaml:  "vpcie12v-supply" and
> "vpcie3v3-supply".

No, that doesn't help - what happens if someone uses separate regulators
for different PCI devices?  The reason the supplies are device namespaced
is that each device can look up it's own supplies and label them how it
wants without reference to anything else on the board.  Alternatively
what happens if some device has another supply it needs to power on (eg,
something that wants a clean LDO output for analogue use)?

> Is the above enough to alleviate your concerns about global namespace collision?

Not really.  TBH it looks like this driver is keeping the regulators
enabled all the time except for suspend and resume anyway, if that's all
that's going on I'd have thought that you wouldn't need any explicit
management in the driver anyway?  Just mark the regualtors as always on
and set up an appropriate suspend mode configuration and everything
should work without the drivers doing anything.  Unless your PMIC isn't
able to provide separate suspend mode configuration for the regulators?

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 2/6] PCI: brcmstb: Add control of EP voltage regulators

[Florian Fainelli]

On 3/29/21 1:45 PM, Mark Brown wrote:
> On Mon, Mar 29, 2021 at 03:48:46PM -0400, Jim Quinlan wrote:
> 
>> I'm not concerned about a namespace collision and I don't think you
>> should be concerned either.  First, this driver is for Broadcom STB
>> PCIe chips and boards, and we also deliver the DT to the customers.
>> We typically do not have any other regulators in the DT besides the
>> ones I am proposing.  For example, the 7216 SOC DT has 0 other
> 
> You may not describe these regulators in the DT but you must have other
> regulators in your system, most devices need power to operate.  In any
> case "this works for me with my DT on my system and nobody will ever
> change our reference design" isn't really a great approach, frankly it's
> not a claim I entirely believe and even if it turns out to be true for
> your systems if we establish this as being how regulators work for
> soldered down PCI devices everyone else is going to want to do the same
> thing, most likely making the same claims for how much control they have
> over the systems things will run on.
> 
>> regulators -- no namespace collision possible.  Our DT-generating
>> scripts also flag namespace issues.  I admit that this driver is also
>> used by RPi chips, but I can easily exclude this feature from the RPI
>> if Nicolas has any objection.
> 
> That's certainly an issue, obviously the RPI is the sort of system where
> I'd imagine this would be particularly useful.
> 
>> Further, if you want, I can restrict the search to the two regulators
>> I am proposing to add to pci-bus.yaml:  "vpcie12v-supply" and
>> "vpcie3v3-supply".
> 
> No, that doesn't help - what happens if someone uses separate regulators
> for different PCI devices?  The reason the supplies are device namespaced
> is that each device can look up it's own supplies and label them how it
> wants without reference to anything else on the board.  Alternatively
> what happens if some device has another supply it needs to power on (eg,
> something that wants a clean LDO output for analogue use)?
> 
>> Is the above enough to alleviate your concerns about global namespace collision?
> 
> Not really.  TBH it looks like this driver is keeping the regulators
> enabled all the time except for suspend and resume anyway, if that's all
> that's going on I'd have thought that you wouldn't need any explicit
> management in the driver anyway?  Just mark the regualtors as always on
> and set up an appropriate suspend mode configuration and everything
> should work without the drivers doing anything.  Unless your PMIC isn't
> able to provide separate suspend mode configuration for the regulators?
> 

We have typically GPIO-controlled and PMIC (via SCMI) controlled
regulators. During PCIe enumeration we need these regulators turned on
to be successful in training the PCIe link and discover the end-point
attached, so there an always on regulator would work.

When we enter a system suspend state however there are really two cases:

- the end-point supports Wake-on (typically wake-on-WLAN) and we need
its power supplied kept on to support that

- the end-point does not support or participate in any wake-up, there we
want to turn its supplies off to save power

How would we go about supporting such an use case with an always on
regulator?
-- 
Florian

Re: [PATCH v3 2/6] PCI: brcmstb: Add control of EP voltage regulators

[Florian Fainelli]

On 3/29/21 1:45 PM, Mark Brown wrote:
> On Mon, Mar 29, 2021 at 03:48:46PM -0400, Jim Quinlan wrote:
> 
>> I'm not concerned about a namespace collision and I don't think you
>> should be concerned either.  First, this driver is for Broadcom STB
>> PCIe chips and boards, and we also deliver the DT to the customers.
>> We typically do not have any other regulators in the DT besides the
>> ones I am proposing.  For example, the 7216 SOC DT has 0 other
> 
> You may not describe these regulators in the DT but you must have other
> regulators in your system, most devices need power to operate.  In any
> case "this works for me with my DT on my system and nobody will ever
> change our reference design" isn't really a great approach, frankly it's
> not a claim I entirely believe and even if it turns out to be true for
> your systems if we establish this as being how regulators work for
> soldered down PCI devices everyone else is going to want to do the same
> thing, most likely making the same claims for how much control they have
> over the systems things will run on.
> 
>> regulators -- no namespace collision possible.  Our DT-generating
>> scripts also flag namespace issues.  I admit that this driver is also
>> used by RPi chips, but I can easily exclude this feature from the RPI
>> if Nicolas has any objection.
> 
> That's certainly an issue, obviously the RPI is the sort of system where
> I'd imagine this would be particularly useful.
> 
>> Further, if you want, I can restrict the search to the two regulators
>> I am proposing to add to pci-bus.yaml:  "vpcie12v-supply" and
>> "vpcie3v3-supply".
> 
> No, that doesn't help - what happens if someone uses separate regulators
> for different PCI devices?  The reason the supplies are device namespaced
> is that each device can look up it's own supplies and label them how it
> wants without reference to anything else on the board.  Alternatively
> what happens if some device has another supply it needs to power on (eg,
> something that wants a clean LDO output for analogue use)?
> 
>> Is the above enough to alleviate your concerns about global namespace collision?
> 
> Not really.  TBH it looks like this driver is keeping the regulators
> enabled all the time except for suspend and resume anyway, if that's all
> that's going on I'd have thought that you wouldn't need any explicit
> management in the driver anyway?  Just mark the regualtors as always on
> and set up an appropriate suspend mode configuration and everything
> should work without the drivers doing anything.  Unless your PMIC isn't
> able to provide separate suspend mode configuration for the regulators?
> 

We have typically GPIO-controlled and PMIC (via SCMI) controlled
regulators. During PCIe enumeration we need these regulators turned on
to be successful in training the PCIe link and discover the end-point
attached, so there an always on regulator would work.

When we enter a system suspend state however there are really two cases:

- the end-point supports Wake-on (typically wake-on-WLAN) and we need
its power supplied kept on to support that

- the end-point does not support or participate in any wake-up, there we
want to turn its supplies off to save power

How would we go about supporting such an use case with an always on
regulator?
-- 
Florian

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 2/6] PCI: brcmstb: Add control of EP voltage regulators

[Mark Brown]

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

On Mon, Mar 29, 2021 at 02:09:58PM -0700, Florian Fainelli wrote:
> On 3/29/21 1:45 PM, Mark Brown wrote:

> > management in the driver anyway?  Just mark the regualtors as always on
> > and set up an appropriate suspend mode configuration and everything
> > should work without the drivers doing anything.  Unless your PMIC isn't
> > able to provide separate suspend mode configuration for the regulators?

> We have typically GPIO-controlled and PMIC (via SCMI) controlled
> regulators. During PCIe enumeration we need these regulators turned on
> to be successful in training the PCIe link and discover the end-point
> attached, so there an always on regulator would work.

> When we enter a system suspend state however there are really two cases:

> - the end-point supports Wake-on (typically wake-on-WLAN) and we need
> its power supplied kept on to support that

> - the end-point does not support or participate in any wake-up, there we
> want to turn its supplies off to save power

> How would we go about supporting such an use case with an always on
> regulator?

With a PMIC most PMICs have a system suspend mode with separate
regulator configuration for that and there's seprate regulator API
control for those, including DT bindings.  If that needs runtime
configuration for something hidden by SCMI I'd hope the SCMI regulator
stuff has facilities for that, if not then I guess a spec extension is
needed.  If you want to dynamically select if something is on during
suspend there's not really a way around regulator API support.

For a GPIO regulator you probably need something that does a disable on
the way down, assuming that the GPIO/pin controller doesn't end up
having it's own suspend mode control that ends up powering things off
anyway.  With GPIOs pinctrl on the pins rather than exposing as a
regulator might be enough.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v3 2/6] PCI: brcmstb: Add control of EP voltage regulators

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 1850 bytes --]

On Mon, Mar 29, 2021 at 02:09:58PM -0700, Florian Fainelli wrote:
> On 3/29/21 1:45 PM, Mark Brown wrote:

> > management in the driver anyway?  Just mark the regualtors as always on
> > and set up an appropriate suspend mode configuration and everything
> > should work without the drivers doing anything.  Unless your PMIC isn't
> > able to provide separate suspend mode configuration for the regulators?

> We have typically GPIO-controlled and PMIC (via SCMI) controlled
> regulators. During PCIe enumeration we need these regulators turned on
> to be successful in training the PCIe link and discover the end-point
> attached, so there an always on regulator would work.

> When we enter a system suspend state however there are really two cases:

> - the end-point supports Wake-on (typically wake-on-WLAN) and we need
> its power supplied kept on to support that

> - the end-point does not support or participate in any wake-up, there we
> want to turn its supplies off to save power

> How would we go about supporting such an use case with an always on
> regulator?

With a PMIC most PMICs have a system suspend mode with separate
regulator configuration for that and there's seprate regulator API
control for those, including DT bindings.  If that needs runtime
configuration for something hidden by SCMI I'd hope the SCMI regulator
stuff has facilities for that, if not then I guess a spec extension is
needed.  If you want to dynamically select if something is on during
suspend there's not really a way around regulator API support.

For a GPIO regulator you probably need something that does a disable on
the way down, assuming that the GPIO/pin controller doesn't end up
having it's own suspend mode control that ends up powering things off
anyway.  With GPIOs pinctrl on the pins rather than exposing as a
regulator might be enough.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v3 3/6] PCI: brcmstb: Do not turn off regulators if EP can wake up

[Jim Quinlan]

If any downstream device may wake up during S2/S3 suspend, we do not want
to turn off its power when suspending.

Signed-off-by: Jim Quinlan <jim2101024@gmail.com>
---
 drivers/pci/controller/pcie-brcmstb.c | 58 +++++++++++++++++++++++----
 1 file changed, 51 insertions(+), 7 deletions(-)

diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
index b76ec7d9af32..89745bb6ada6 100644
--- a/drivers/pci/controller/pcie-brcmstb.c
+++ b/drivers/pci/controller/pcie-brcmstb.c
@@ -193,6 +193,7 @@ static inline void brcm_pcie_bridge_sw_init_set_generic(struct brcm_pcie *pcie,
 static inline void brcm_pcie_perst_set_4908(struct brcm_pcie *pcie, u32 val);
 static inline void brcm_pcie_perst_set_7278(struct brcm_pcie *pcie, u32 val);
 static inline void brcm_pcie_perst_set_generic(struct brcm_pcie *pcie, u32 val);
+static bool brcm_pcie_link_up(struct brcm_pcie *pcie);
 
 enum {
 	RGR1_SW_INIT_1,
@@ -299,22 +300,65 @@ struct brcm_pcie {
 	void			(*bridge_sw_init_set)(struct brcm_pcie *pcie, u32 val);
 	struct regulator_bulk_data supplies[PCIE_BRCM_MAX_EP_REGULATORS];
 	unsigned int		num_supplies;
+	bool			ep_wakeup_capable;
 };
 
-static int brcm_set_regulators(struct brcm_pcie *pcie, bool on)
+static int pci_dev_may_wakeup(struct pci_dev *dev, void *data)
 {
+	bool *ret = data;
+
+	if (device_may_wakeup(&dev->dev)) {
+		*ret = true;
+		dev_dbg(&dev->dev, "disable cancelled for wake-up device\n");
+	}
+	return (int) *ret;
+}
+
+enum {
+	TURN_OFF,		/* Turn egulators off, unless an EP is wakeup-capable */
+	TURN_OFF_ALWAYS,	/* Turn Regulators off, no exceptions */
+	TURN_ON,		/* Turn regulators on, unless pcie->ep_wakeup_capable */
+};
+
+static int brcm_set_regulators(struct brcm_pcie *pcie, int how)
+{
+	struct pci_host_bridge *bridge = pci_host_bridge_from_priv(pcie);
 	struct device *dev = pcie->dev;
 	int ret;
 
 	if (!pcie->num_supplies)
 		return 0;
-	if (on)
+	if (how == TURN_ON) {
+		if (pcie->ep_wakeup_capable) {
+			/*
+			 * We are resuming from a suspend.  In the
+			 * previous suspend we did not disable the power
+			 * supplies, so there is no need to enable them
+			 * (and falsely increase their usage count).
+			 */
+			pcie->ep_wakeup_capable = false;
+			return 0;
+		}
+	} else if (how == TURN_OFF) {
+		/*
+		 * If at least one device on this bus is enabled as a
+		 * wake-up source, do not turn off regulators.
+		 */
+		pcie->ep_wakeup_capable = false;
+		if (bridge->bus && brcm_pcie_link_up(pcie)) {
+			pci_walk_bus(bridge->bus, pci_dev_may_wakeup, &pcie->ep_wakeup_capable);
+			if (pcie->ep_wakeup_capable)
+				return 0;
+		}
+	}
+
+	if (how == TURN_ON)
 		ret = regulator_bulk_enable(pcie->num_supplies, pcie->supplies);
 	else
 		ret = regulator_bulk_disable(pcie->num_supplies, pcie->supplies);
 	if (ret)
 		dev_err(dev, "failed to %s EP regulators\n",
-			on ? "enable" : "disable");
+			how == TURN_ON ? "enable" : "disable");
 	return ret;
 }
 
@@ -1218,7 +1262,7 @@ static int brcm_pcie_suspend(struct device *dev)
 	brcm_phy_stop(pcie);
 	clk_disable_unprepare(pcie->clk);
 
-	return brcm_set_regulators(pcie, false);
+	return brcm_set_regulators(pcie, TURN_OFF);
 }
 
 static int brcm_pcie_resume(struct device *dev)
@@ -1231,7 +1275,7 @@ static int brcm_pcie_resume(struct device *dev)
 	base = pcie->base;
 	clk_prepare_enable(pcie->clk);
 
-	ret = brcm_set_regulators(pcie, true);
+	ret = brcm_set_regulators(pcie, TURN_ON);
 	if (ret)
 		return ret;
 
@@ -1271,7 +1315,7 @@ static void __brcm_pcie_remove(struct brcm_pcie *pcie)
 	brcm_phy_stop(pcie);
 	reset_control_assert(pcie->rescal);
 	clk_disable_unprepare(pcie->clk);
-	brcm_set_regulators(pcie, false);
+	brcm_set_regulators(pcie, TURN_OFF_ALWAYS);
 	regulator_bulk_free(pcie->num_supplies, pcie->supplies);
 }
 
@@ -1369,7 +1413,7 @@ static int brcm_pcie_probe(struct platform_device *pdev)
 		goto fail;
 	}
 
-	ret = brcm_set_regulators(pcie, true);
+	ret = brcm_set_regulators(pcie, TURN_ON);
 	if (ret)
 		goto fail;
 
-- 
2.17.1

[PATCH v3 3/6] PCI: brcmstb: Do not turn off regulators if EP can wake up

[Jim Quinlan]

If any downstream device may wake up during S2/S3 suspend, we do not want
to turn off its power when suspending.

Signed-off-by: Jim Quinlan <jim2101024@gmail.com>
---
 drivers/pci/controller/pcie-brcmstb.c | 58 +++++++++++++++++++++++----
 1 file changed, 51 insertions(+), 7 deletions(-)

diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
index b76ec7d9af32..89745bb6ada6 100644
--- a/drivers/pci/controller/pcie-brcmstb.c
+++ b/drivers/pci/controller/pcie-brcmstb.c
@@ -193,6 +193,7 @@ static inline void brcm_pcie_bridge_sw_init_set_generic(struct brcm_pcie *pcie,
 static inline void brcm_pcie_perst_set_4908(struct brcm_pcie *pcie, u32 val);
 static inline void brcm_pcie_perst_set_7278(struct brcm_pcie *pcie, u32 val);
 static inline void brcm_pcie_perst_set_generic(struct brcm_pcie *pcie, u32 val);
+static bool brcm_pcie_link_up(struct brcm_pcie *pcie);
 
 enum {
 	RGR1_SW_INIT_1,
@@ -299,22 +300,65 @@ struct brcm_pcie {
 	void			(*bridge_sw_init_set)(struct brcm_pcie *pcie, u32 val);
 	struct regulator_bulk_data supplies[PCIE_BRCM_MAX_EP_REGULATORS];
 	unsigned int		num_supplies;
+	bool			ep_wakeup_capable;
 };
 
-static int brcm_set_regulators(struct brcm_pcie *pcie, bool on)
+static int pci_dev_may_wakeup(struct pci_dev *dev, void *data)
 {
+	bool *ret = data;
+
+	if (device_may_wakeup(&dev->dev)) {
+		*ret = true;
+		dev_dbg(&dev->dev, "disable cancelled for wake-up device\n");
+	}
+	return (int) *ret;
+}
+
+enum {
+	TURN_OFF,		/* Turn egulators off, unless an EP is wakeup-capable */
+	TURN_OFF_ALWAYS,	/* Turn Regulators off, no exceptions */
+	TURN_ON,		/* Turn regulators on, unless pcie->ep_wakeup_capable */
+};
+
+static int brcm_set_regulators(struct brcm_pcie *pcie, int how)
+{
+	struct pci_host_bridge *bridge = pci_host_bridge_from_priv(pcie);
 	struct device *dev = pcie->dev;
 	int ret;
 
 	if (!pcie->num_supplies)
 		return 0;
-	if (on)
+	if (how == TURN_ON) {
+		if (pcie->ep_wakeup_capable) {
+			/*
+			 * We are resuming from a suspend.  In the
+			 * previous suspend we did not disable the power
+			 * supplies, so there is no need to enable them
+			 * (and falsely increase their usage count).
+			 */
+			pcie->ep_wakeup_capable = false;
+			return 0;
+		}
+	} else if (how == TURN_OFF) {
+		/*
+		 * If at least one device on this bus is enabled as a
+		 * wake-up source, do not turn off regulators.
+		 */
+		pcie->ep_wakeup_capable = false;
+		if (bridge->bus && brcm_pcie_link_up(pcie)) {
+			pci_walk_bus(bridge->bus, pci_dev_may_wakeup, &pcie->ep_wakeup_capable);
+			if (pcie->ep_wakeup_capable)
+				return 0;
+		}
+	}
+
+	if (how == TURN_ON)
 		ret = regulator_bulk_enable(pcie->num_supplies, pcie->supplies);
 	else
 		ret = regulator_bulk_disable(pcie->num_supplies, pcie->supplies);
 	if (ret)
 		dev_err(dev, "failed to %s EP regulators\n",
-			on ? "enable" : "disable");
+			how == TURN_ON ? "enable" : "disable");
 	return ret;
 }
 
@@ -1218,7 +1262,7 @@ static int brcm_pcie_suspend(struct device *dev)
 	brcm_phy_stop(pcie);
 	clk_disable_unprepare(pcie->clk);
 
-	return brcm_set_regulators(pcie, false);
+	return brcm_set_regulators(pcie, TURN_OFF);
 }
 
 static int brcm_pcie_resume(struct device *dev)
@@ -1231,7 +1275,7 @@ static int brcm_pcie_resume(struct device *dev)
 	base = pcie->base;
 	clk_prepare_enable(pcie->clk);
 
-	ret = brcm_set_regulators(pcie, true);
+	ret = brcm_set_regulators(pcie, TURN_ON);
 	if (ret)
 		return ret;
 
@@ -1271,7 +1315,7 @@ static void __brcm_pcie_remove(struct brcm_pcie *pcie)
 	brcm_phy_stop(pcie);
 	reset_control_assert(pcie->rescal);
 	clk_disable_unprepare(pcie->clk);
-	brcm_set_regulators(pcie, false);
+	brcm_set_regulators(pcie, TURN_OFF_ALWAYS);
 	regulator_bulk_free(pcie->num_supplies, pcie->supplies);
 }
 
@@ -1369,7 +1413,7 @@ static int brcm_pcie_probe(struct platform_device *pdev)
 		goto fail;
 	}
 
-	ret = brcm_set_regulators(pcie, true);
+	ret = brcm_set_regulators(pcie, TURN_ON);
 	if (ret)
 		goto fail;
 
-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 3/6] PCI: brcmstb: Do not turn off regulators if EP can wake up

[Bjorn Helgaas]

On Fri, Mar 26, 2021 at 03:19:01PM -0400, Jim Quinlan wrote:
> If any downstream device may wake up during S2/S3 suspend, we do not want
> to turn off its power when suspending.
> 
> Signed-off-by: Jim Quinlan <jim2101024@gmail.com>
> ---
>  drivers/pci/controller/pcie-brcmstb.c | 58 +++++++++++++++++++++++----
>  1 file changed, 51 insertions(+), 7 deletions(-)

> +enum {
> +	TURN_OFF,		/* Turn egulators off, unless an EP is wakeup-capable */
> +	TURN_OFF_ALWAYS,	/* Turn Regulators off, no exceptions */
> +	TURN_ON,		/* Turn regulators on, unless pcie->ep_wakeup_capable */

s/egulators/regulators/
s/Regulators/regulators/

Re: [PATCH v3 3/6] PCI: brcmstb: Do not turn off regulators if EP can wake up

[Bjorn Helgaas]

On Fri, Mar 26, 2021 at 03:19:01PM -0400, Jim Quinlan wrote:
> If any downstream device may wake up during S2/S3 suspend, we do not want
> to turn off its power when suspending.
> 
> Signed-off-by: Jim Quinlan <jim2101024@gmail.com>
> ---
>  drivers/pci/controller/pcie-brcmstb.c | 58 +++++++++++++++++++++++----
>  1 file changed, 51 insertions(+), 7 deletions(-)

> +enum {
> +	TURN_OFF,		/* Turn egulators off, unless an EP is wakeup-capable */
> +	TURN_OFF_ALWAYS,	/* Turn Regulators off, no exceptions */
> +	TURN_ON,		/* Turn regulators on, unless pcie->ep_wakeup_capable */

s/egulators/regulators/
s/Regulators/regulators/

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v3 4/6] PCI: brcmstb: Give 7216 SOCs their own config type

[Jim Quinlan]

This distinction is required for an imminent commit.

Signed-off-by: Jim Quinlan <jim2101024@gmail.com>
Acked-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/pci/controller/pcie-brcmstb.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
index 89745bb6ada6..9c8b922a9c19 100644
--- a/drivers/pci/controller/pcie-brcmstb.c
+++ b/drivers/pci/controller/pcie-brcmstb.c
@@ -260,6 +260,13 @@ 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,
+};
+
 struct brcm_msi {
 	struct device		*dev;
 	void __iomem		*base;
@@ -1336,7 +1343,7 @@ static const struct of_device_id brcm_pcie_match[] = {
 	{ .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 },
 	{},
 };
-- 
2.17.1

[PATCH v3 4/6] PCI: brcmstb: Give 7216 SOCs their own config type

[Jim Quinlan]

This distinction is required for an imminent commit.

Signed-off-by: Jim Quinlan <jim2101024@gmail.com>
Acked-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/pci/controller/pcie-brcmstb.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
index 89745bb6ada6..9c8b922a9c19 100644
--- a/drivers/pci/controller/pcie-brcmstb.c
+++ b/drivers/pci/controller/pcie-brcmstb.c
@@ -260,6 +260,13 @@ 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,
+};
+
 struct brcm_msi {
 	struct device		*dev;
 	void __iomem		*base;
@@ -1336,7 +1343,7 @@ static const struct of_device_id brcm_pcie_match[] = {
 	{ .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 },
 	{},
 };
-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v3 5/6] PCI: brcmstb: Add panic/die handler to RC driver

[Jim Quinlan]

Whereas most PCIe HW returns 0xffffffff on illegal accesses and the like,
by default Broadcom's STB PCIe controller effects an abort.  This simple
handler determines if the PCIe controller was the cause of the abort and if
so, prints out diagnostic info.

Example output:
  brcm-pcie 8b20000.pcie: Error: Mem Acc: 32bit, Read, @0x38000000
  brcm-pcie 8b20000.pcie:  Type: TO=0 Abt=0 UnspReq=1 AccDsble=0 BadAddr=0

Signed-off-by: Jim Quinlan <jim2101024@gmail.com>
Acked-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/pci/controller/pcie-brcmstb.c | 122 ++++++++++++++++++++++++++
 1 file changed, 122 insertions(+)

diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
index 9c8b922a9c19..2d9288399014 100644
--- a/drivers/pci/controller/pcie-brcmstb.c
+++ b/drivers/pci/controller/pcie-brcmstb.c
@@ -12,11 +12,13 @@
 #include <linux/ioport.h>
 #include <linux/irqchip/chained_irq.h>
 #include <linux/irqdomain.h>
+#include <linux/kdebug.h>
 #include <linux/kernel.h>
 #include <linux/list.h>
 #include <linux/log2.h>
 #include <linux/module.h>
 #include <linux/msi.h>
+#include <linux/notifier.h>
 #include <linux/of_address.h>
 #include <linux/of_irq.h>
 #include <linux/of_pci.h>
@@ -186,6 +188,39 @@
 #define  PCIE_DVT_PMU_PCIE_PHY_CTRL_DAST_PWRDN_MASK		0x1
 #define  PCIE_DVT_PMU_PCIE_PHY_CTRL_DAST_PWRDN_SHIFT		0x0
 
+/* Error report regiseters */
+#define PCIE_OUTB_ERR_TREAT				0x6000
+#define  PCIE_OUTB_ERR_TREAT_CONFIG_MASK		0x1
+#define  PCIE_OUTB_ERR_TREAT_MEM_MASK			0x2
+#define PCIE_OUTB_ERR_VALID				0x6004
+#define PCIE_OUTB_ERR_CLEAR				0x6008
+#define PCIE_OUTB_ERR_ACC_INFO				0x600c
+#define  PCIE_OUTB_ERR_ACC_INFO_CFG_ERR_MASK		0x01
+#define  PCIE_OUTB_ERR_ACC_INFO_MEM_ERR_MASK		0x02
+#define  PCIE_OUTB_ERR_ACC_INFO_TYPE_64_MASK		0x04
+#define  PCIE_OUTB_ERR_ACC_INFO_DIR_WRITE_MASK		0x10
+#define  PCIE_OUTB_ERR_ACC_INFO_BYTE_LANES_MASK		0xff00
+#define PCIE_OUTB_ERR_ACC_ADDR				0x6010
+#define PCIE_OUTB_ERR_ACC_ADDR_BUS_MASK			0xff00000
+#define PCIE_OUTB_ERR_ACC_ADDR_DEV_MASK			0xf8000
+#define PCIE_OUTB_ERR_ACC_ADDR_FUNC_MASK		0x7000
+#define PCIE_OUTB_ERR_ACC_ADDR_REG_MASK			0xfff
+#define PCIE_OUTB_ERR_CFG_CAUSE				0x6014
+#define  PCIE_OUTB_ERR_CFG_CAUSE_TIMEOUT_MASK		0x40
+#define  PCIE_OUTB_ERR_CFG_CAUSE_ABORT_MASK		0x20
+#define  PCIE_OUTB_ERR_CFG_CAUSE_UNSUPP_REQ_MASK	0x10
+#define  PCIE_OUTB_ERR_CFG_CAUSE_ACC_TIMEOUT_MASK	0x4
+#define  PCIE_OUTB_ERR_CFG_CAUSE_ACC_DISABLED_MASK	0x2
+#define  PCIE_OUTB_ERR_CFG_CAUSE_ACC_64BIT__MASK	0x1
+#define PCIE_OUTB_ERR_MEM_ADDR_LO			0x6018
+#define PCIE_OUTB_ERR_MEM_ADDR_HI			0x601c
+#define PCIE_OUTB_ERR_MEM_CAUSE				0x6020
+#define  PCIE_OUTB_ERR_MEM_CAUSE_TIMEOUT_MASK		0x40
+#define  PCIE_OUTB_ERR_MEM_CAUSE_ABORT_MASK		0x20
+#define  PCIE_OUTB_ERR_MEM_CAUSE_UNSUPP_REQ_MASK	0x10
+#define  PCIE_OUTB_ERR_MEM_CAUSE_ACC_DISABLED_MASK	0x2
+#define  PCIE_OUTB_ERR_MEM_CAUSE_BAD_ADDR_MASK		0x1
+
 /* Forward declarations */
 struct brcm_pcie;
 static inline void brcm_pcie_bridge_sw_init_set_7278(struct brcm_pcie *pcie, u32 val);
@@ -218,6 +253,7 @@ struct pcie_cfg_data {
 	const enum pcie_type type;
 	void (*perst_set)(struct brcm_pcie *pcie, u32 val);
 	void (*bridge_sw_init_set)(struct brcm_pcie *pcie, u32 val);
+	const bool has_err_report;
 };
 
 static const int pcie_offsets[] = {
@@ -265,6 +301,7 @@ static const struct pcie_cfg_data bcm7216_cfg = {
 	.type		= BCM7278,
 	.perst_set	= brcm_pcie_perst_set_7278,
 	.bridge_sw_init_set = brcm_pcie_bridge_sw_init_set_7278,
+	.has_err_report = true,
 };
 
 struct brcm_msi {
@@ -308,8 +345,87 @@ struct brcm_pcie {
 	struct regulator_bulk_data supplies[PCIE_BRCM_MAX_EP_REGULATORS];
 	unsigned int		num_supplies;
 	bool			ep_wakeup_capable;
+	bool			has_err_report;
+	struct notifier_block	die_notifier;
 };
 
+/* Dump out PCIe errors on die or panic */
+static int dump_pcie_error(struct notifier_block *self, unsigned long v, void *p)
+{
+	const struct brcm_pcie *pcie = container_of(self, struct brcm_pcie, die_notifier);
+	void __iomem *base = pcie->base;
+	int i, is_cfg_err, is_mem_err, lanes;
+	char *width_str, *direction_str, lanes_str[9];
+	u32 info;
+
+	if (readl(base + PCIE_OUTB_ERR_VALID) == 0)
+		return NOTIFY_DONE;
+	info = readl(base + PCIE_OUTB_ERR_ACC_INFO);
+
+
+	is_cfg_err = !!(info & PCIE_OUTB_ERR_ACC_INFO_CFG_ERR_MASK);
+	is_mem_err = !!(info & PCIE_OUTB_ERR_ACC_INFO_MEM_ERR_MASK);
+	width_str = (info & PCIE_OUTB_ERR_ACC_INFO_TYPE_64_MASK) ? "64bit" : "32bit";
+	direction_str = (info & PCIE_OUTB_ERR_ACC_INFO_DIR_WRITE_MASK) ? "Write" : "Read";
+	lanes = FIELD_GET(PCIE_OUTB_ERR_ACC_INFO_BYTE_LANES_MASK, info);
+	for (i = 0, lanes_str[8] = 0; i < 8; i++)
+		lanes_str[i] = (lanes & (1 << i)) ? '1' : '0';
+
+	if (is_cfg_err) {
+		u32 cfg_addr = readl(base + PCIE_OUTB_ERR_ACC_ADDR);
+		u32 cause = readl(base + PCIE_OUTB_ERR_CFG_CAUSE);
+		int bus = FIELD_GET(PCIE_OUTB_ERR_ACC_ADDR_BUS_MASK, cfg_addr);
+		int dev = FIELD_GET(PCIE_OUTB_ERR_ACC_ADDR_DEV_MASK, cfg_addr);
+		int func = FIELD_GET(PCIE_OUTB_ERR_ACC_ADDR_FUNC_MASK, cfg_addr);
+		int reg = FIELD_GET(PCIE_OUTB_ERR_ACC_ADDR_REG_MASK, cfg_addr);
+
+		dev_err(pcie->dev, "Error: CFG Acc, %s, %s, Bus=%d, Dev=%d, Fun=%d, Reg=0x%x, lanes=%s\n",
+			width_str, direction_str, bus, dev, func, reg, lanes_str);
+		dev_err(pcie->dev, " Type: TO=%d Abt=%d UnsupReq=%d AccTO=%d AccDsbld=%d Acc64bit=%d\n",
+			!!(cause & PCIE_OUTB_ERR_CFG_CAUSE_TIMEOUT_MASK),
+			!!(cause & PCIE_OUTB_ERR_CFG_CAUSE_ABORT_MASK),
+			!!(cause & PCIE_OUTB_ERR_CFG_CAUSE_UNSUPP_REQ_MASK),
+			!!(cause & PCIE_OUTB_ERR_CFG_CAUSE_ACC_TIMEOUT_MASK),
+			!!(cause & PCIE_OUTB_ERR_CFG_CAUSE_ACC_DISABLED_MASK),
+			!!(cause & PCIE_OUTB_ERR_CFG_CAUSE_ACC_64BIT__MASK));
+	}
+
+	if (is_mem_err) {
+		u32 cause = readl(base + PCIE_OUTB_ERR_MEM_CAUSE);
+		u32 lo = readl(base + PCIE_OUTB_ERR_MEM_ADDR_LO);
+		u32 hi = readl(base + PCIE_OUTB_ERR_MEM_ADDR_HI);
+		u64 addr = ((u64)hi << 32) | (u64)lo;
+
+		dev_err(pcie->dev, "Error: Mem Acc, %s, %s, @0x%llx, lanes=%s\n",
+			width_str, direction_str, addr, lanes_str);
+		dev_err(pcie->dev, " Type: TO=%d Abt=%d UnsupReq=%d AccDsble=%d BadAddr=%d\n",
+			!!(cause & PCIE_OUTB_ERR_MEM_CAUSE_TIMEOUT_MASK),
+			!!(cause & PCIE_OUTB_ERR_MEM_CAUSE_ABORT_MASK),
+			!!(cause & PCIE_OUTB_ERR_MEM_CAUSE_UNSUPP_REQ_MASK),
+			!!(cause & PCIE_OUTB_ERR_MEM_CAUSE_ACC_DISABLED_MASK),
+			!!(cause & PCIE_OUTB_ERR_MEM_CAUSE_BAD_ADDR_MASK));
+	}
+
+	/* Clear the error */
+	writel(1, base + PCIE_OUTB_ERR_CLEAR);
+
+	return NOTIFY_DONE;
+}
+
+static void brcm_register_die_notifiers(struct brcm_pcie *pcie)
+{
+	pcie->die_notifier.notifier_call = dump_pcie_error;
+	register_die_notifier(&pcie->die_notifier);
+	atomic_notifier_chain_register(&panic_notifier_list, &pcie->die_notifier);
+}
+
+static void brcm_unregister_die_notifiers(struct brcm_pcie *pcie)
+{
+	unregister_die_notifier(&pcie->die_notifier);
+	atomic_notifier_chain_unregister(&panic_notifier_list, &pcie->die_notifier);
+	pcie->die_notifier.notifier_call = NULL;
+}
+
 static int pci_dev_may_wakeup(struct pci_dev *dev, void *data)
 {
 	bool *ret = data;
@@ -1332,6 +1448,8 @@ static int brcm_pcie_remove(struct platform_device *pdev)
 	struct pci_host_bridge *bridge = pci_host_bridge_from_priv(pcie);
 
 	pci_stop_root_bus(bridge->bus);
+	if (pcie->has_err_report)
+		brcm_unregister_die_notifiers(pcie);
 	pci_remove_root_bus(bridge->bus);
 	__brcm_pcie_remove(pcie);
 
@@ -1371,6 +1489,7 @@ static int brcm_pcie_probe(struct platform_device *pdev)
 	pcie->np = np;
 	pcie->reg_offsets = data->offsets;
 	pcie->type = data->type;
+	pcie->has_err_report = data->has_err_report;
 	pcie->perst_set = data->perst_set;
 	pcie->bridge_sw_init_set = data->bridge_sw_init_set;
 
@@ -1448,6 +1567,9 @@ static int brcm_pcie_probe(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, pcie);
 
+	if (pcie->has_err_report)
+		brcm_register_die_notifiers(pcie);
+
 	return pci_host_probe(bridge);
 fail:
 	__brcm_pcie_remove(pcie);
-- 
2.17.1

[PATCH v3 5/6] PCI: brcmstb: Add panic/die handler to RC driver

[Jim Quinlan]

Whereas most PCIe HW returns 0xffffffff on illegal accesses and the like,
by default Broadcom's STB PCIe controller effects an abort.  This simple
handler determines if the PCIe controller was the cause of the abort and if
so, prints out diagnostic info.

Example output:
  brcm-pcie 8b20000.pcie: Error: Mem Acc: 32bit, Read, @0x38000000
  brcm-pcie 8b20000.pcie:  Type: TO=0 Abt=0 UnspReq=1 AccDsble=0 BadAddr=0

Signed-off-by: Jim Quinlan <jim2101024@gmail.com>
Acked-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/pci/controller/pcie-brcmstb.c | 122 ++++++++++++++++++++++++++
 1 file changed, 122 insertions(+)

diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
index 9c8b922a9c19..2d9288399014 100644
--- a/drivers/pci/controller/pcie-brcmstb.c
+++ b/drivers/pci/controller/pcie-brcmstb.c
@@ -12,11 +12,13 @@
 #include <linux/ioport.h>
 #include <linux/irqchip/chained_irq.h>
 #include <linux/irqdomain.h>
+#include <linux/kdebug.h>
 #include <linux/kernel.h>
 #include <linux/list.h>
 #include <linux/log2.h>
 #include <linux/module.h>
 #include <linux/msi.h>
+#include <linux/notifier.h>
 #include <linux/of_address.h>
 #include <linux/of_irq.h>
 #include <linux/of_pci.h>
@@ -186,6 +188,39 @@
 #define  PCIE_DVT_PMU_PCIE_PHY_CTRL_DAST_PWRDN_MASK		0x1
 #define  PCIE_DVT_PMU_PCIE_PHY_CTRL_DAST_PWRDN_SHIFT		0x0
 
+/* Error report regiseters */
+#define PCIE_OUTB_ERR_TREAT				0x6000
+#define  PCIE_OUTB_ERR_TREAT_CONFIG_MASK		0x1
+#define  PCIE_OUTB_ERR_TREAT_MEM_MASK			0x2
+#define PCIE_OUTB_ERR_VALID				0x6004
+#define PCIE_OUTB_ERR_CLEAR				0x6008
+#define PCIE_OUTB_ERR_ACC_INFO				0x600c
+#define  PCIE_OUTB_ERR_ACC_INFO_CFG_ERR_MASK		0x01
+#define  PCIE_OUTB_ERR_ACC_INFO_MEM_ERR_MASK		0x02
+#define  PCIE_OUTB_ERR_ACC_INFO_TYPE_64_MASK		0x04
+#define  PCIE_OUTB_ERR_ACC_INFO_DIR_WRITE_MASK		0x10
+#define  PCIE_OUTB_ERR_ACC_INFO_BYTE_LANES_MASK		0xff00
+#define PCIE_OUTB_ERR_ACC_ADDR				0x6010
+#define PCIE_OUTB_ERR_ACC_ADDR_BUS_MASK			0xff00000
+#define PCIE_OUTB_ERR_ACC_ADDR_DEV_MASK			0xf8000
+#define PCIE_OUTB_ERR_ACC_ADDR_FUNC_MASK		0x7000
+#define PCIE_OUTB_ERR_ACC_ADDR_REG_MASK			0xfff
+#define PCIE_OUTB_ERR_CFG_CAUSE				0x6014
+#define  PCIE_OUTB_ERR_CFG_CAUSE_TIMEOUT_MASK		0x40
+#define  PCIE_OUTB_ERR_CFG_CAUSE_ABORT_MASK		0x20
+#define  PCIE_OUTB_ERR_CFG_CAUSE_UNSUPP_REQ_MASK	0x10
+#define  PCIE_OUTB_ERR_CFG_CAUSE_ACC_TIMEOUT_MASK	0x4
+#define  PCIE_OUTB_ERR_CFG_CAUSE_ACC_DISABLED_MASK	0x2
+#define  PCIE_OUTB_ERR_CFG_CAUSE_ACC_64BIT__MASK	0x1
+#define PCIE_OUTB_ERR_MEM_ADDR_LO			0x6018
+#define PCIE_OUTB_ERR_MEM_ADDR_HI			0x601c
+#define PCIE_OUTB_ERR_MEM_CAUSE				0x6020
+#define  PCIE_OUTB_ERR_MEM_CAUSE_TIMEOUT_MASK		0x40
+#define  PCIE_OUTB_ERR_MEM_CAUSE_ABORT_MASK		0x20
+#define  PCIE_OUTB_ERR_MEM_CAUSE_UNSUPP_REQ_MASK	0x10
+#define  PCIE_OUTB_ERR_MEM_CAUSE_ACC_DISABLED_MASK	0x2
+#define  PCIE_OUTB_ERR_MEM_CAUSE_BAD_ADDR_MASK		0x1
+
 /* Forward declarations */
 struct brcm_pcie;
 static inline void brcm_pcie_bridge_sw_init_set_7278(struct brcm_pcie *pcie, u32 val);
@@ -218,6 +253,7 @@ struct pcie_cfg_data {
 	const enum pcie_type type;
 	void (*perst_set)(struct brcm_pcie *pcie, u32 val);
 	void (*bridge_sw_init_set)(struct brcm_pcie *pcie, u32 val);
+	const bool has_err_report;
 };
 
 static const int pcie_offsets[] = {
@@ -265,6 +301,7 @@ static const struct pcie_cfg_data bcm7216_cfg = {
 	.type		= BCM7278,
 	.perst_set	= brcm_pcie_perst_set_7278,
 	.bridge_sw_init_set = brcm_pcie_bridge_sw_init_set_7278,
+	.has_err_report = true,
 };
 
 struct brcm_msi {
@@ -308,8 +345,87 @@ struct brcm_pcie {
 	struct regulator_bulk_data supplies[PCIE_BRCM_MAX_EP_REGULATORS];
 	unsigned int		num_supplies;
 	bool			ep_wakeup_capable;
+	bool			has_err_report;
+	struct notifier_block	die_notifier;
 };
 
+/* Dump out PCIe errors on die or panic */
+static int dump_pcie_error(struct notifier_block *self, unsigned long v, void *p)
+{
+	const struct brcm_pcie *pcie = container_of(self, struct brcm_pcie, die_notifier);
+	void __iomem *base = pcie->base;
+	int i, is_cfg_err, is_mem_err, lanes;
+	char *width_str, *direction_str, lanes_str[9];
+	u32 info;
+
+	if (readl(base + PCIE_OUTB_ERR_VALID) == 0)
+		return NOTIFY_DONE;
+	info = readl(base + PCIE_OUTB_ERR_ACC_INFO);
+
+
+	is_cfg_err = !!(info & PCIE_OUTB_ERR_ACC_INFO_CFG_ERR_MASK);
+	is_mem_err = !!(info & PCIE_OUTB_ERR_ACC_INFO_MEM_ERR_MASK);
+	width_str = (info & PCIE_OUTB_ERR_ACC_INFO_TYPE_64_MASK) ? "64bit" : "32bit";
+	direction_str = (info & PCIE_OUTB_ERR_ACC_INFO_DIR_WRITE_MASK) ? "Write" : "Read";
+	lanes = FIELD_GET(PCIE_OUTB_ERR_ACC_INFO_BYTE_LANES_MASK, info);
+	for (i = 0, lanes_str[8] = 0; i < 8; i++)
+		lanes_str[i] = (lanes & (1 << i)) ? '1' : '0';
+
+	if (is_cfg_err) {
+		u32 cfg_addr = readl(base + PCIE_OUTB_ERR_ACC_ADDR);
+		u32 cause = readl(base + PCIE_OUTB_ERR_CFG_CAUSE);
+		int bus = FIELD_GET(PCIE_OUTB_ERR_ACC_ADDR_BUS_MASK, cfg_addr);
+		int dev = FIELD_GET(PCIE_OUTB_ERR_ACC_ADDR_DEV_MASK, cfg_addr);
+		int func = FIELD_GET(PCIE_OUTB_ERR_ACC_ADDR_FUNC_MASK, cfg_addr);
+		int reg = FIELD_GET(PCIE_OUTB_ERR_ACC_ADDR_REG_MASK, cfg_addr);
+
+		dev_err(pcie->dev, "Error: CFG Acc, %s, %s, Bus=%d, Dev=%d, Fun=%d, Reg=0x%x, lanes=%s\n",
+			width_str, direction_str, bus, dev, func, reg, lanes_str);
+		dev_err(pcie->dev, " Type: TO=%d Abt=%d UnsupReq=%d AccTO=%d AccDsbld=%d Acc64bit=%d\n",
+			!!(cause & PCIE_OUTB_ERR_CFG_CAUSE_TIMEOUT_MASK),
+			!!(cause & PCIE_OUTB_ERR_CFG_CAUSE_ABORT_MASK),
+			!!(cause & PCIE_OUTB_ERR_CFG_CAUSE_UNSUPP_REQ_MASK),
+			!!(cause & PCIE_OUTB_ERR_CFG_CAUSE_ACC_TIMEOUT_MASK),
+			!!(cause & PCIE_OUTB_ERR_CFG_CAUSE_ACC_DISABLED_MASK),
+			!!(cause & PCIE_OUTB_ERR_CFG_CAUSE_ACC_64BIT__MASK));
+	}
+
+	if (is_mem_err) {
+		u32 cause = readl(base + PCIE_OUTB_ERR_MEM_CAUSE);
+		u32 lo = readl(base + PCIE_OUTB_ERR_MEM_ADDR_LO);
+		u32 hi = readl(base + PCIE_OUTB_ERR_MEM_ADDR_HI);
+		u64 addr = ((u64)hi << 32) | (u64)lo;
+
+		dev_err(pcie->dev, "Error: Mem Acc, %s, %s, @0x%llx, lanes=%s\n",
+			width_str, direction_str, addr, lanes_str);
+		dev_err(pcie->dev, " Type: TO=%d Abt=%d UnsupReq=%d AccDsble=%d BadAddr=%d\n",
+			!!(cause & PCIE_OUTB_ERR_MEM_CAUSE_TIMEOUT_MASK),
+			!!(cause & PCIE_OUTB_ERR_MEM_CAUSE_ABORT_MASK),
+			!!(cause & PCIE_OUTB_ERR_MEM_CAUSE_UNSUPP_REQ_MASK),
+			!!(cause & PCIE_OUTB_ERR_MEM_CAUSE_ACC_DISABLED_MASK),
+			!!(cause & PCIE_OUTB_ERR_MEM_CAUSE_BAD_ADDR_MASK));
+	}
+
+	/* Clear the error */
+	writel(1, base + PCIE_OUTB_ERR_CLEAR);
+
+	return NOTIFY_DONE;
+}
+
+static void brcm_register_die_notifiers(struct brcm_pcie *pcie)
+{
+	pcie->die_notifier.notifier_call = dump_pcie_error;
+	register_die_notifier(&pcie->die_notifier);
+	atomic_notifier_chain_register(&panic_notifier_list, &pcie->die_notifier);
+}
+
+static void brcm_unregister_die_notifiers(struct brcm_pcie *pcie)
+{
+	unregister_die_notifier(&pcie->die_notifier);
+	atomic_notifier_chain_unregister(&panic_notifier_list, &pcie->die_notifier);
+	pcie->die_notifier.notifier_call = NULL;
+}
+
 static int pci_dev_may_wakeup(struct pci_dev *dev, void *data)
 {
 	bool *ret = data;
@@ -1332,6 +1448,8 @@ static int brcm_pcie_remove(struct platform_device *pdev)
 	struct pci_host_bridge *bridge = pci_host_bridge_from_priv(pcie);
 
 	pci_stop_root_bus(bridge->bus);
+	if (pcie->has_err_report)
+		brcm_unregister_die_notifiers(pcie);
 	pci_remove_root_bus(bridge->bus);
 	__brcm_pcie_remove(pcie);
 
@@ -1371,6 +1489,7 @@ static int brcm_pcie_probe(struct platform_device *pdev)
 	pcie->np = np;
 	pcie->reg_offsets = data->offsets;
 	pcie->type = data->type;
+	pcie->has_err_report = data->has_err_report;
 	pcie->perst_set = data->perst_set;
 	pcie->bridge_sw_init_set = data->bridge_sw_init_set;
 
@@ -1448,6 +1567,9 @@ static int brcm_pcie_probe(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, pcie);
 
+	if (pcie->has_err_report)
+		brcm_register_die_notifiers(pcie);
+
 	return pci_host_probe(bridge);
 fail:
 	__brcm_pcie_remove(pcie);
-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v3 6/6] PCI: brcmstb: Check return value of clk_prepare_enable()

[Jim Quinlan]

The check was missing on PCIe resume.

Signed-off-by: Jim Quinlan <jim2101024@gmail.com>
Acked-by: Florian Fainelli <f.fainelli@gmail.com>
Fixes: 8195b7417018 ("PCI: brcmstb: Add suspend and resume pm_ops")
---
 drivers/pci/controller/pcie-brcmstb.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
index 2d9288399014..f6d9d785b301 100644
--- a/drivers/pci/controller/pcie-brcmstb.c
+++ b/drivers/pci/controller/pcie-brcmstb.c
@@ -1396,7 +1396,9 @@ static int brcm_pcie_resume(struct device *dev)
 	int ret;
 
 	base = pcie->base;
-	clk_prepare_enable(pcie->clk);
+	ret = clk_prepare_enable(pcie->clk);
+	if (ret)
+		return ret;
 
 	ret = brcm_set_regulators(pcie, TURN_ON);
 	if (ret)
@@ -1535,7 +1537,9 @@ static int brcm_pcie_probe(struct platform_device *pdev)
 
 	ret = brcm_pcie_get_regulators(pcie);
 	if (ret) {
-		dev_err(pcie->dev, "failed to get regulators (err=%d)\n", ret);
+		pcie->num_supplies = 0;
+		if (ret != -EPROBE_DEFER)
+			dev_err(pcie->dev, "failed to get regulators (err=%d)\n", ret);
 		goto fail;
 	}
 
-- 
2.17.1

[PATCH v3 6/6] PCI: brcmstb: Check return value of clk_prepare_enable()

[Jim Quinlan]

The check was missing on PCIe resume.

Signed-off-by: Jim Quinlan <jim2101024@gmail.com>
Acked-by: Florian Fainelli <f.fainelli@gmail.com>
Fixes: 8195b7417018 ("PCI: brcmstb: Add suspend and resume pm_ops")
---
 drivers/pci/controller/pcie-brcmstb.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
index 2d9288399014..f6d9d785b301 100644
--- a/drivers/pci/controller/pcie-brcmstb.c
+++ b/drivers/pci/controller/pcie-brcmstb.c
@@ -1396,7 +1396,9 @@ static int brcm_pcie_resume(struct device *dev)
 	int ret;
 
 	base = pcie->base;
-	clk_prepare_enable(pcie->clk);
+	ret = clk_prepare_enable(pcie->clk);
+	if (ret)
+		return ret;
 
 	ret = brcm_set_regulators(pcie, TURN_ON);
 	if (ret)
@@ -1535,7 +1537,9 @@ static int brcm_pcie_probe(struct platform_device *pdev)
 
 	ret = brcm_pcie_get_regulators(pcie);
 	if (ret) {
-		dev_err(pcie->dev, "failed to get regulators (err=%d)\n", ret);
+		pcie->num_supplies = 0;
+		if (ret != -EPROBE_DEFER)
+			dev_err(pcie->dev, "failed to get regulators (err=%d)\n", ret);
 		goto fail;
 	}
 
-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 6/6] PCI: brcmstb: Check return value of clk_prepare_enable()

[Bjorn Helgaas]

On Fri, Mar 26, 2021 at 03:19:04PM -0400, Jim Quinlan wrote:
> The check was missing on PCIe resume.

"PCIe resume" isn't really a thing, per se.  PCI/PCIe gives us device
power states (D0, D3hot, etc), and Linux power management builds
suspend/resume on top of those.  Maybe:

  Check for failure of clk_prepare_enable() on device resume.

> Signed-off-by: Jim Quinlan <jim2101024@gmail.com>
> Acked-by: Florian Fainelli <f.fainelli@gmail.com>
> Fixes: 8195b7417018 ("PCI: brcmstb: Add suspend and resume pm_ops")
> ---
>  drivers/pci/controller/pcie-brcmstb.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
> index 2d9288399014..f6d9d785b301 100644
> --- a/drivers/pci/controller/pcie-brcmstb.c
> +++ b/drivers/pci/controller/pcie-brcmstb.c
> @@ -1396,7 +1396,9 @@ static int brcm_pcie_resume(struct device *dev)
>  	int ret;
>  
>  	base = pcie->base;
> -	clk_prepare_enable(pcie->clk);
> +	ret = clk_prepare_enable(pcie->clk);
> +	if (ret)
> +		return ret;

This fix doesn't look like it depends on the EP regulator support.
Maybe it should be a preparatory patch before patch 1/6?  It could
then easily be backported to kernels that contain 8195b7417018 but not
EP regulator support.

>  	ret = brcm_set_regulators(pcie, TURN_ON);
>  	if (ret)
> @@ -1535,7 +1537,9 @@ static int brcm_pcie_probe(struct platform_device *pdev)
>  
>  	ret = brcm_pcie_get_regulators(pcie);
>  	if (ret) {
> -		dev_err(pcie->dev, "failed to get regulators (err=%d)\n", ret);
> +		pcie->num_supplies = 0;
> +		if (ret != -EPROBE_DEFER)
> +			dev_err(pcie->dev, "failed to get regulators (err=%d)\n", ret);

Looks like this hunk might belong somewhere else, e.g., in patch 2/6?
The "Fixes:" line suggests that this patch could/should be backported to
every kernel that contains 8195b7417018, but 8195b7417018 doesn't have
pcie->num_supplies.

>  		goto fail;
>  	}
>  
> -- 
> 2.17.1
> 

Re: [PATCH v3 6/6] PCI: brcmstb: Check return value of clk_prepare_enable()

[Bjorn Helgaas]

On Fri, Mar 26, 2021 at 03:19:04PM -0400, Jim Quinlan wrote:
> The check was missing on PCIe resume.

"PCIe resume" isn't really a thing, per se.  PCI/PCIe gives us device
power states (D0, D3hot, etc), and Linux power management builds
suspend/resume on top of those.  Maybe:

  Check for failure of clk_prepare_enable() on device resume.

> Signed-off-by: Jim Quinlan <jim2101024@gmail.com>
> Acked-by: Florian Fainelli <f.fainelli@gmail.com>
> Fixes: 8195b7417018 ("PCI: brcmstb: Add suspend and resume pm_ops")
> ---
>  drivers/pci/controller/pcie-brcmstb.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
> index 2d9288399014..f6d9d785b301 100644
> --- a/drivers/pci/controller/pcie-brcmstb.c
> +++ b/drivers/pci/controller/pcie-brcmstb.c
> @@ -1396,7 +1396,9 @@ static int brcm_pcie_resume(struct device *dev)
>  	int ret;
>  
>  	base = pcie->base;
> -	clk_prepare_enable(pcie->clk);
> +	ret = clk_prepare_enable(pcie->clk);
> +	if (ret)
> +		return ret;

This fix doesn't look like it depends on the EP regulator support.
Maybe it should be a preparatory patch before patch 1/6?  It could
then easily be backported to kernels that contain 8195b7417018 but not
EP regulator support.

>  	ret = brcm_set_regulators(pcie, TURN_ON);
>  	if (ret)
> @@ -1535,7 +1537,9 @@ static int brcm_pcie_probe(struct platform_device *pdev)
>  
>  	ret = brcm_pcie_get_regulators(pcie);
>  	if (ret) {
> -		dev_err(pcie->dev, "failed to get regulators (err=%d)\n", ret);
> +		pcie->num_supplies = 0;
> +		if (ret != -EPROBE_DEFER)
> +			dev_err(pcie->dev, "failed to get regulators (err=%d)\n", ret);

Looks like this hunk might belong somewhere else, e.g., in patch 2/6?
The "Fixes:" line suggests that this patch could/should be backported to
every kernel that contains 8195b7417018, but 8195b7417018 doesn't have
pcie->num_supplies.

>  		goto fail;
>  	}
>  
> -- 
> 2.17.1
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel