[PATCH v4 0/6] PCI: brcmstb: add slot0 device regulators and panic handler

[PATCH v4 0/6] PCI: brcmstb: add slot0 device regulators and panic handler

[Jim Quinlan]

v4 [NOTE: I'm not sure this fixes RobH and MarkB constraints but I'd
          like to use this pullreq as a basis for future discussion.]
   [Commit: Add bindings for ...]
     -- Fix syntax error in YAML bindings example (RobH)
     -- {vpcie12v,vpcie3v3}-supply props are back in root complex DT node
        (I believe RobH said this was okay)
   [Commit: Add control of ..]
     -- Do not do global search for regulator; now we look specifically
        for the property {vpcie12v,vpcie3v3}-supply in the root complex
	DT node and then call devm_regulator_bulk_get() (MarkB)
     -- Use devm_regulator_bulk_get() (Bjorn)
     -- s/EP/slot0 device/ (Bjorn)
     -- Spelling, capitalization (Bjorn)
     -- Have brcm_phy_stop() return a void (Bjorn)
   [Commit: Do not turn off ...]
     -- Capitalization (Bjorn)
   [Commit: Check return value ...]
     -- Commit message content (Bjorn)
     -- Move 6/6 hunk to 2/6 where it belongs (Bjorn)
     -- Move the rest of 6/6 before all other commits (Bjorn)

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):
  PCI: brcmstb: Check return value of clk_prepare_enable()
  dt-bindings: PCI: Add bindings for Brcmstb endpoint device voltage
    regulators
  PCI: brcmstb: Add control of slot0 device 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

 .../bindings/pci/brcm,stb-pcie.yaml           |   4 +
 drivers/pci/controller/pcie-brcmstb.c         | 262 +++++++++++++++++-
 2 files changed, 259 insertions(+), 7 deletions(-)

-- 
2.17.1

[PATCH v4 0/6] PCI: brcmstb: add slot0 device regulators and panic handler

[Jim Quinlan]

v4 [NOTE: I'm not sure this fixes RobH and MarkB constraints but I'd
          like to use this pullreq as a basis for future discussion.]
   [Commit: Add bindings for ...]
     -- Fix syntax error in YAML bindings example (RobH)
     -- {vpcie12v,vpcie3v3}-supply props are back in root complex DT node
        (I believe RobH said this was okay)
   [Commit: Add control of ..]
     -- Do not do global search for regulator; now we look specifically
        for the property {vpcie12v,vpcie3v3}-supply in the root complex
	DT node and then call devm_regulator_bulk_get() (MarkB)
     -- Use devm_regulator_bulk_get() (Bjorn)
     -- s/EP/slot0 device/ (Bjorn)
     -- Spelling, capitalization (Bjorn)
     -- Have brcm_phy_stop() return a void (Bjorn)
   [Commit: Do not turn off ...]
     -- Capitalization (Bjorn)
   [Commit: Check return value ...]
     -- Commit message content (Bjorn)
     -- Move 6/6 hunk to 2/6 where it belongs (Bjorn)
     -- Move the rest of 6/6 before all other commits (Bjorn)

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):
  PCI: brcmstb: Check return value of clk_prepare_enable()
  dt-bindings: PCI: Add bindings for Brcmstb endpoint device voltage
    regulators
  PCI: brcmstb: Add control of slot0 device 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

 .../bindings/pci/brcm,stb-pcie.yaml           |   4 +
 drivers/pci/controller/pcie-brcmstb.c         | 262 +++++++++++++++++-
 2 files changed, 259 insertions(+), 7 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 v4 1/6] PCI: brcmstb: Check return value of clk_prepare_enable()

[Jim Quinlan]

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 | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
index e330e6811f0b..4ce1f3a60574 100644
--- a/drivers/pci/controller/pcie-brcmstb.c
+++ b/drivers/pci/controller/pcie-brcmstb.c
@@ -1161,7 +1161,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_phy_start(pcie);
 	if (ret)
-- 
2.17.1

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

[Jim Quinlan]

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 | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
index e330e6811f0b..4ce1f3a60574 100644
--- a/drivers/pci/controller/pcie-brcmstb.c
+++ b/drivers/pci/controller/pcie-brcmstb.c
@@ -1161,7 +1161,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_phy_start(pcie);
 	if (ret)
-- 
2.17.1


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

[PATCH v4 2/6] dt-bindings: PCI: Add bindings for Brcmstb endpoint device 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 | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml b/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
index f90557f6deb8..f2caa5b3b281 100644
--- a/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
+++ b/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
@@ -64,6 +64,9 @@ properties:
 
   aspm-no-l0s: true
 
+  vpcie12v-supply: true
+  vpcie3v3-supply: true
+
   brcm,scb-sizes:
     description: u64 giving the 64bit PCIe memory
       viewport size of a memory controller.  There may be up to
@@ -156,5 +159,6 @@ examples:
                                  <0x42000000 0x1 0x80000000 0x3 0x00000000 0x0 0x80000000>;
                     brcm,enable-ssc;
                     brcm,scb-sizes =  <0x0000000080000000 0x0000000080000000>;
+                    vpcie12v-supply = <&vreg12>;
             };
     };
-- 
2.17.1

[PATCH v4 2/6] dt-bindings: PCI: Add bindings for Brcmstb endpoint device 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 | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml b/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
index f90557f6deb8..f2caa5b3b281 100644
--- a/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
+++ b/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
@@ -64,6 +64,9 @@ properties:
 
   aspm-no-l0s: true
 
+  vpcie12v-supply: true
+  vpcie3v3-supply: true
+
   brcm,scb-sizes:
     description: u64 giving the 64bit PCIe memory
       viewport size of a memory controller.  There may be up to
@@ -156,5 +159,6 @@ examples:
                                  <0x42000000 0x1 0x80000000 0x3 0x00000000 0x0 0x80000000>;
                     brcm,enable-ssc;
                     brcm,scb-sizes =  <0x0000000080000000 0x0000000080000000>;
+                    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 v4 2/6] dt-bindings: PCI: Add bindings for Brcmstb endpoint device voltage regulators

[Mark Brown]

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

On Thu, Apr 01, 2021 at 05:21:42PM -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 | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml b/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
> index f90557f6deb8..f2caa5b3b281 100644
> --- a/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
> +++ b/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
> @@ -64,6 +64,9 @@ properties:
>  
>    aspm-no-l0s: true
>  
> +  vpcie12v-supply: true
> +  vpcie3v3-supply: true
> +

No great problem with having these in the controller node (assming it
accurately describes the hardware) but I do think we ought to also be
able to describe these per slot.

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

Re: [PATCH v4 2/6] dt-bindings: PCI: Add bindings for Brcmstb endpoint device voltage regulators

[Mark Brown]

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

On Thu, Apr 01, 2021 at 05:21:42PM -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 | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml b/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
> index f90557f6deb8..f2caa5b3b281 100644
> --- a/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
> +++ b/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
> @@ -64,6 +64,9 @@ properties:
>  
>    aspm-no-l0s: true
>  
> +  vpcie12v-supply: true
> +  vpcie3v3-supply: true
> +

No great problem with having these in the controller node (assming it
accurately describes the hardware) but I do think we ought to also be
able to describe these per slot.

[-- 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 v4 2/6] dt-bindings: PCI: Add bindings for Brcmstb endpoint device voltage regulators

[Jim Quinlan]

On Tue, Apr 6, 2021 at 12:47 PM Mark Brown <broonie@kernel.org> wrote:
>
> On Thu, Apr 01, 2021 at 05:21:42PM -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 | 4 ++++
> >  1 file changed, 4 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml b/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
> > index f90557f6deb8..f2caa5b3b281 100644
> > --- a/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
> > +++ b/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
> > @@ -64,6 +64,9 @@ properties:
> >
> >    aspm-no-l0s: true
> >
> > +  vpcie12v-supply: true
> > +  vpcie3v3-supply: true
> > +
>
> No great problem with having these in the controller node (assming it
> accurately describes the hardware) but I do think we ought to also be
> able to describe these per slot.

Hi Mark,
Can you explain what you think that would look like in the DT?
Thanks,
Jim Quinlan
Broadcom STB

Re: [PATCH v4 2/6] dt-bindings: PCI: Add bindings for Brcmstb endpoint device voltage regulators

[Jim Quinlan]

On Tue, Apr 6, 2021 at 12:47 PM Mark Brown <broonie@kernel.org> wrote:
>
> On Thu, Apr 01, 2021 at 05:21:42PM -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 | 4 ++++
> >  1 file changed, 4 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml b/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
> > index f90557f6deb8..f2caa5b3b281 100644
> > --- a/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
> > +++ b/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
> > @@ -64,6 +64,9 @@ properties:
> >
> >    aspm-no-l0s: true
> >
> > +  vpcie12v-supply: true
> > +  vpcie3v3-supply: true
> > +
>
> No great problem with having these in the controller node (assming it
> accurately describes the hardware) but I do think we ought to also be
> able to describe these per slot.

Hi Mark,
Can you explain what you think that would look like in the DT?
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 v4 2/6] dt-bindings: PCI: Add bindings for Brcmstb endpoint device voltage regulators

[Mark Brown]

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

On Tue, Apr 06, 2021 at 01:26:51PM -0400, Jim Quinlan wrote:
> On Tue, Apr 6, 2021 at 12:47 PM Mark Brown <broonie@kernel.org> wrote:

> > No great problem with having these in the controller node (assming it
> > accurately describes the hardware) but I do think we ought to also be
> > able to describe these per slot.

> Can you explain what you think that would look like in the DT?

I *think* that's just some properties on the nodes for the endpoints,
note that the driver could just ignore them for now.  Not sure where or
if we document any extensions but child nodes are in section 4 of the
v2.1 PCI bus binding.

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

Re: [PATCH v4 2/6] dt-bindings: PCI: Add bindings for Brcmstb endpoint device voltage regulators

[Mark Brown]

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

On Tue, Apr 06, 2021 at 01:26:51PM -0400, Jim Quinlan wrote:
> On Tue, Apr 6, 2021 at 12:47 PM Mark Brown <broonie@kernel.org> wrote:

> > No great problem with having these in the controller node (assming it
> > accurately describes the hardware) but I do think we ought to also be
> > able to describe these per slot.

> Can you explain what you think that would look like in the DT?

I *think* that's just some properties on the nodes for the endpoints,
note that the driver could just ignore them for now.  Not sure where or
if we document any extensions but child nodes are in section 4 of the
v2.1 PCI bus binding.

[-- 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 v4 2/6] dt-bindings: PCI: Add bindings for Brcmstb endpoint device voltage regulators

[Jim Quinlan]

On Tue, Apr 6, 2021 at 1:32 PM Mark Brown <broonie@kernel.org> wrote:
>
> On Tue, Apr 06, 2021 at 01:26:51PM -0400, Jim Quinlan wrote:
> > On Tue, Apr 6, 2021 at 12:47 PM Mark Brown <broonie@kernel.org> wrote:
>
> > > No great problem with having these in the controller node (assming it
> > > accurately describes the hardware) but I do think we ought to also be
> > > able to describe these per slot.
>
> > Can you explain what you think that would look like in the DT?
>
> I *think* that's just some properties on the nodes for the endpoints,
> note that the driver could just ignore them for now.  Not sure where or
> if we document any extensions but child nodes are in section 4 of the
> v2.1 PCI bus binding.

Hi Mark,

I'm a little confused -- here is how I remember the chronology of the
"DT bindings" commit reviews, please correct me if I'm wrong:

o JimQ submitted a pullreq for using voltage regulators in the same
style as the existing "rockport" PCIe driver.
o After some deliberation, RobH preferred that the voltage regulators
should go into the PCIe subnode device's DT node.
o JimQ put the voltage regulators in the subnode device's DT node.
o MarkB didn't like the fact that the code did a global search for the
regulator since it could not provide the owning struct device* handle.
o RobH relented, and said that if it is just two specific and standard
voltage regulators, perhaps they can go in the parent DT node after
all.
o JimQ put the regulators back in the PCIe node.
o MarkB now wants the regulators to go back into the child node again?

Folks, please advise.

Regards,
Jim Quinlan
Broadcom STB

Re: [PATCH v4 2/6] dt-bindings: PCI: Add bindings for Brcmstb endpoint device voltage regulators

[Jim Quinlan]

On Tue, Apr 6, 2021 at 1:32 PM Mark Brown <broonie@kernel.org> wrote:
>
> On Tue, Apr 06, 2021 at 01:26:51PM -0400, Jim Quinlan wrote:
> > On Tue, Apr 6, 2021 at 12:47 PM Mark Brown <broonie@kernel.org> wrote:
>
> > > No great problem with having these in the controller node (assming it
> > > accurately describes the hardware) but I do think we ought to also be
> > > able to describe these per slot.
>
> > Can you explain what you think that would look like in the DT?
>
> I *think* that's just some properties on the nodes for the endpoints,
> note that the driver could just ignore them for now.  Not sure where or
> if we document any extensions but child nodes are in section 4 of the
> v2.1 PCI bus binding.

Hi Mark,

I'm a little confused -- here is how I remember the chronology of the
"DT bindings" commit reviews, please correct me if I'm wrong:

o JimQ submitted a pullreq for using voltage regulators in the same
style as the existing "rockport" PCIe driver.
o After some deliberation, RobH preferred that the voltage regulators
should go into the PCIe subnode device's DT node.
o JimQ put the voltage regulators in the subnode device's DT node.
o MarkB didn't like the fact that the code did a global search for the
regulator since it could not provide the owning struct device* handle.
o RobH relented, and said that if it is just two specific and standard
voltage regulators, perhaps they can go in the parent DT node after
all.
o JimQ put the regulators back in the PCIe node.
o MarkB now wants the regulators to go back into the child node again?

Folks, please advise.

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 v4 2/6] dt-bindings: PCI: Add bindings for Brcmstb endpoint device voltage regulators

[Mark Brown]

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

On Tue, Apr 06, 2021 at 02:25:49PM -0400, Jim Quinlan wrote:

> I'm a little confused -- here is how I remember the chronology of the
> "DT bindings" commit reviews, please correct me if I'm wrong:

> o JimQ submitted a pullreq for using voltage regulators in the same
> style as the existing "rockport" PCIe driver.
> o After some deliberation, RobH preferred that the voltage regulators
> should go into the PCIe subnode device's DT node.
> o JimQ put the voltage regulators in the subnode device's DT node.
> o MarkB didn't like the fact that the code did a global search for the
> regulator since it could not provide the owning struct device* handle.
> o RobH relented, and said that if it is just two specific and standard
> voltage regulators, perhaps they can go in the parent DT node after
> all.
> o JimQ put the regulators back in the PCIe node.
> o MarkB now wants the regulators to go back into the child node again?

...having pointed out a couple of times now that there's no physical
requirement that the supplies be shared between slots never mind with
the controller.  Also note that as I've said depending on what the
actual requirements of the controller node are you might want to have
the regulators in both places, and further note that the driver does not
have to actively use everything in the binding document (although if
it's not using something that turns out to be a requirement it's likely
to run into hardware where that causes bugs at some point).

Frankly I'm not clear why you're trying to handle powering on PCI slots
in a specific driver, surely PCI devices are PCI devices regardless of
the controller?

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

Re: [PATCH v4 2/6] dt-bindings: PCI: Add bindings for Brcmstb endpoint device voltage regulators

[Mark Brown]

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

On Tue, Apr 06, 2021 at 02:25:49PM -0400, Jim Quinlan wrote:

> I'm a little confused -- here is how I remember the chronology of the
> "DT bindings" commit reviews, please correct me if I'm wrong:

> o JimQ submitted a pullreq for using voltage regulators in the same
> style as the existing "rockport" PCIe driver.
> o After some deliberation, RobH preferred that the voltage regulators
> should go into the PCIe subnode device's DT node.
> o JimQ put the voltage regulators in the subnode device's DT node.
> o MarkB didn't like the fact that the code did a global search for the
> regulator since it could not provide the owning struct device* handle.
> o RobH relented, and said that if it is just two specific and standard
> voltage regulators, perhaps they can go in the parent DT node after
> all.
> o JimQ put the regulators back in the PCIe node.
> o MarkB now wants the regulators to go back into the child node again?

...having pointed out a couple of times now that there's no physical
requirement that the supplies be shared between slots never mind with
the controller.  Also note that as I've said depending on what the
actual requirements of the controller node are you might want to have
the regulators in both places, and further note that the driver does not
have to actively use everything in the binding document (although if
it's not using something that turns out to be a requirement it's likely
to run into hardware where that causes bugs at some point).

Frankly I'm not clear why you're trying to handle powering on PCI slots
in a specific driver, surely PCI devices are PCI devices regardless of
the controller?

[-- 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 v4 2/6] dt-bindings: PCI: Add bindings for Brcmstb endpoint device voltage regulators

[Florian Fainelli]

On 4/7/2021 4:27 AM, Mark Brown wrote:
> On Tue, Apr 06, 2021 at 02:25:49PM -0400, Jim Quinlan wrote:
> 
>> I'm a little confused -- here is how I remember the chronology of the
>> "DT bindings" commit reviews, please correct me if I'm wrong:
> 
>> o JimQ submitted a pullreq for using voltage regulators in the same
>> style as the existing "rockport" PCIe driver.
>> o After some deliberation, RobH preferred that the voltage regulators
>> should go into the PCIe subnode device's DT node.
>> o JimQ put the voltage regulators in the subnode device's DT node.
>> o MarkB didn't like the fact that the code did a global search for the
>> regulator since it could not provide the owning struct device* handle.
>> o RobH relented, and said that if it is just two specific and standard
>> voltage regulators, perhaps they can go in the parent DT node after
>> all.
>> o JimQ put the regulators back in the PCIe node.
>> o MarkB now wants the regulators to go back into the child node again?
> 
> ...having pointed out a couple of times now that there's no physical
> requirement that the supplies be shared between slots never mind with
> the controller.  Also note that as I've said depending on what the
> actual requirements of the controller node are you might want to have
> the regulators in both places, and further note that the driver does not
> have to actively use everything in the binding document (although if
> it's not using something that turns out to be a requirement it's likely
> to run into hardware where that causes bugs at some point).
> 
> Frankly I'm not clear why you're trying to handle powering on PCI slots
> in a specific driver, surely PCI devices are PCI devices regardless of
> the controller?

There is no currently a way to deal with that situation since you have a
chicken and egg problem to solve: there is no pci_device created until
you enumerate the PCI bus, and you cannot enumerate the bus and create
those pci_devices unless you power on the slots/PCIe end-points attached
to the root complex. There are precedents like the rockchip PCIe RC
driver, and yes, we should not be cargo culting this, which is why we
are trying to understand what is it that should be done here and there
has been conflicting advice, or rather our interpretation has led to
perceiving it as a conflicting.

If the regulator had a variation where it supported passing a
device_node reference to look up regulator properties, we could solve
this generically for all devices, that does not exist, and you stated
you will not accept changes like those, fair enough.

When you suggested to look at soundwire for an example of "software
devices", we did that but it was not clear where the sdw_slave would be
created prior to sdw_slave_add(), but this does not matter too much.

Let us say we try to solve this generically using the same idea that we
would pre-populate pci_device prior to calling pci_scan_child_bus(). We
could do something along these lines:

- pci_scan_child_bus() would attempt to walk the list of device_node
children from the PCIe root complex's device_node and call
pci_alloc_dev() for each of these devices that it finds, along with
calling device_initialize() and ensuring that pci_dev::device::of_node
is set correctly by calling pci_set_of_node()/set_dev_node(). Finally we
call list_add_tail() with the pci_bus_sem semaphore held to add that
pci_device to bus->devices such that we can later find them

- from there on we try to get the regulators of those pci_device objects
we just allocated and we try to enable their regulators, either based on
a specific list of supplies or just trying to enable all supplied declared.

- now pci_scan_child_bus() will attempt to scan the bus for real by
reading the pci_device objects ID but we already have such objects, so
we need to update pci_scan_device() to search bus::devices and if
nothing is found we allocate one

Is that roughly what you have in mind as to what should be done?
-- 
Florian

Re: [PATCH v4 2/6] dt-bindings: PCI: Add bindings for Brcmstb endpoint device voltage regulators

[Florian Fainelli]

On 4/7/2021 4:27 AM, Mark Brown wrote:
> On Tue, Apr 06, 2021 at 02:25:49PM -0400, Jim Quinlan wrote:
> 
>> I'm a little confused -- here is how I remember the chronology of the
>> "DT bindings" commit reviews, please correct me if I'm wrong:
> 
>> o JimQ submitted a pullreq for using voltage regulators in the same
>> style as the existing "rockport" PCIe driver.
>> o After some deliberation, RobH preferred that the voltage regulators
>> should go into the PCIe subnode device's DT node.
>> o JimQ put the voltage regulators in the subnode device's DT node.
>> o MarkB didn't like the fact that the code did a global search for the
>> regulator since it could not provide the owning struct device* handle.
>> o RobH relented, and said that if it is just two specific and standard
>> voltage regulators, perhaps they can go in the parent DT node after
>> all.
>> o JimQ put the regulators back in the PCIe node.
>> o MarkB now wants the regulators to go back into the child node again?
> 
> ...having pointed out a couple of times now that there's no physical
> requirement that the supplies be shared between slots never mind with
> the controller.  Also note that as I've said depending on what the
> actual requirements of the controller node are you might want to have
> the regulators in both places, and further note that the driver does not
> have to actively use everything in the binding document (although if
> it's not using something that turns out to be a requirement it's likely
> to run into hardware where that causes bugs at some point).
> 
> Frankly I'm not clear why you're trying to handle powering on PCI slots
> in a specific driver, surely PCI devices are PCI devices regardless of
> the controller?

There is no currently a way to deal with that situation since you have a
chicken and egg problem to solve: there is no pci_device created until
you enumerate the PCI bus, and you cannot enumerate the bus and create
those pci_devices unless you power on the slots/PCIe end-points attached
to the root complex. There are precedents like the rockchip PCIe RC
driver, and yes, we should not be cargo culting this, which is why we
are trying to understand what is it that should be done here and there
has been conflicting advice, or rather our interpretation has led to
perceiving it as a conflicting.

If the regulator had a variation where it supported passing a
device_node reference to look up regulator properties, we could solve
this generically for all devices, that does not exist, and you stated
you will not accept changes like those, fair enough.

When you suggested to look at soundwire for an example of "software
devices", we did that but it was not clear where the sdw_slave would be
created prior to sdw_slave_add(), but this does not matter too much.

Let us say we try to solve this generically using the same idea that we
would pre-populate pci_device prior to calling pci_scan_child_bus(). We
could do something along these lines:

- pci_scan_child_bus() would attempt to walk the list of device_node
children from the PCIe root complex's device_node and call
pci_alloc_dev() for each of these devices that it finds, along with
calling device_initialize() and ensuring that pci_dev::device::of_node
is set correctly by calling pci_set_of_node()/set_dev_node(). Finally we
call list_add_tail() with the pci_bus_sem semaphore held to add that
pci_device to bus->devices such that we can later find them

- from there on we try to get the regulators of those pci_device objects
we just allocated and we try to enable their regulators, either based on
a specific list of supplies or just trying to enable all supplied declared.

- now pci_scan_child_bus() will attempt to scan the bus for real by
reading the pci_device objects ID but we already have such objects, so
we need to update pci_scan_device() to search bus::devices and if
nothing is found we allocate one

Is that roughly what you have in mind as to what should be done?
-- 
Florian

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

Re: [PATCH v4 2/6] dt-bindings: PCI: Add bindings for Brcmstb endpoint device voltage regulators

[Mark Brown]

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

On Wed, Apr 07, 2021 at 11:35:57AM -0700, Florian Fainelli wrote:
> On 4/7/2021 4:27 AM, Mark Brown wrote:

> > Frankly I'm not clear why you're trying to handle powering on PCI slots
> > in a specific driver, surely PCI devices are PCI devices regardless of
> > the controller?

> There is no currently a way to deal with that situation since you have a
> chicken and egg problem to solve: there is no pci_device created until
> you enumerate the PCI bus, and you cannot enumerate the bus and create
> those pci_devices unless you power on the slots/PCIe end-points attached
> to the root complex. There are precedents like the rockchip PCIe RC
> driver, and yes, we should not be cargo culting this, which is why we
> are trying to understand what is it that should be done here and there
> has been conflicting advice, or rather our interpretation has led to
> perceiving it as a conflicting.

As you note below I've pointed you at Slimbus which has a similar
problem and solves it at a bus level although it thought of this from
day one which makes life easier; I do think it'd be good to get this
stuff in the driver core since it's an issue that affects every
enumerable bus in the end but nobody's summoned up the enthusiasm for
that (including me).

> If the regulator had a variation where it supported passing a
> device_node reference to look up regulator properties, we could solve
> this generically for all devices, that does not exist, and you stated
> you will not accept changes like those, fair enough.

I'm certainly not enthusiastic about the idea and the likely abuse isn't
inspiring, and of course regulators aren't the only resource that might
be needed to get something up and running and would need to be extended
in the end.  That said I've not seen any concrete proposals either.

> When you suggested to look at soundwire for an example of "software
> devices", we did that but it was not clear where the sdw_slave would be
> created prior to sdw_slave_add(), but this does not matter too much.

Looks like sdw_acpi_find_slaves() and sdw_of_find_slaves().

> Let us say we try to solve this generically using the same idea that we
> would pre-populate pci_device prior to calling pci_scan_child_bus(). We
> could do something along these lines:

...

> - from there on we try to get the regulators of those pci_device objects
> we just allocated and we try to enable their regulators, either based on
> a specific list of supplies or just trying to enable all supplied declared.

I'd suggest specfying the supplies that PCI provides to slots in the
spec with standard names and just using that list, at least as a start.
That'd probably cover most cases and allow the binding to be written at
the generic PCI level rather than having individual devices need to name
their supplies for the binding documentation and validation stuff which
seems easier.  Devices with extra stuff can always extend the binding.

> - now pci_scan_child_bus() will attempt to scan the bus for real by
> reading the pci_device objects ID but we already have such objects, so
> we need to update pci_scan_device() to search bus::devices and if
> nothing is found we allocate one

> Is that roughly what you have in mind as to what should be done?

Yes, pretty much.  Ideally there'd be some way for drivers to get a
callback prior to enumeration to handle any custom stuff for embedded
cases but unless someone actually has a use case for that you could just
punt.

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

Re: [PATCH v4 2/6] dt-bindings: PCI: Add bindings for Brcmstb endpoint device voltage regulators

[Mark Brown]

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

On Wed, Apr 07, 2021 at 11:35:57AM -0700, Florian Fainelli wrote:
> On 4/7/2021 4:27 AM, Mark Brown wrote:

> > Frankly I'm not clear why you're trying to handle powering on PCI slots
> > in a specific driver, surely PCI devices are PCI devices regardless of
> > the controller?

> There is no currently a way to deal with that situation since you have a
> chicken and egg problem to solve: there is no pci_device created until
> you enumerate the PCI bus, and you cannot enumerate the bus and create
> those pci_devices unless you power on the slots/PCIe end-points attached
> to the root complex. There are precedents like the rockchip PCIe RC
> driver, and yes, we should not be cargo culting this, which is why we
> are trying to understand what is it that should be done here and there
> has been conflicting advice, or rather our interpretation has led to
> perceiving it as a conflicting.

As you note below I've pointed you at Slimbus which has a similar
problem and solves it at a bus level although it thought of this from
day one which makes life easier; I do think it'd be good to get this
stuff in the driver core since it's an issue that affects every
enumerable bus in the end but nobody's summoned up the enthusiasm for
that (including me).

> If the regulator had a variation where it supported passing a
> device_node reference to look up regulator properties, we could solve
> this generically for all devices, that does not exist, and you stated
> you will not accept changes like those, fair enough.

I'm certainly not enthusiastic about the idea and the likely abuse isn't
inspiring, and of course regulators aren't the only resource that might
be needed to get something up and running and would need to be extended
in the end.  That said I've not seen any concrete proposals either.

> When you suggested to look at soundwire for an example of "software
> devices", we did that but it was not clear where the sdw_slave would be
> created prior to sdw_slave_add(), but this does not matter too much.

Looks like sdw_acpi_find_slaves() and sdw_of_find_slaves().

> Let us say we try to solve this generically using the same idea that we
> would pre-populate pci_device prior to calling pci_scan_child_bus(). We
> could do something along these lines:

...

> - from there on we try to get the regulators of those pci_device objects
> we just allocated and we try to enable their regulators, either based on
> a specific list of supplies or just trying to enable all supplied declared.

I'd suggest specfying the supplies that PCI provides to slots in the
spec with standard names and just using that list, at least as a start.
That'd probably cover most cases and allow the binding to be written at
the generic PCI level rather than having individual devices need to name
their supplies for the binding documentation and validation stuff which
seems easier.  Devices with extra stuff can always extend the binding.

> - now pci_scan_child_bus() will attempt to scan the bus for real by
> reading the pci_device objects ID but we already have such objects, so
> we need to update pci_scan_device() to search bus::devices and if
> nothing is found we allocate one

> Is that roughly what you have in mind as to what should be done?

Yes, pretty much.  Ideally there'd be some way for drivers to get a
callback prior to enumeration to handle any custom stuff for embedded
cases but unless someone actually has a use case for that you could just
punt.

[-- 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 v4 2/6] dt-bindings: PCI: Add bindings for Brcmstb endpoint device voltage regulators

[Rob Herring]

On Tue, Apr 06, 2021 at 02:25:49PM -0400, Jim Quinlan wrote:
> On Tue, Apr 6, 2021 at 1:32 PM Mark Brown <broonie@kernel.org> wrote:
> >
> > On Tue, Apr 06, 2021 at 01:26:51PM -0400, Jim Quinlan wrote:
> > > On Tue, Apr 6, 2021 at 12:47 PM Mark Brown <broonie@kernel.org> wrote:
> >
> > > > No great problem with having these in the controller node (assming it
> > > > accurately describes the hardware) but I do think we ought to also be
> > > > able to describe these per slot.

PCIe is effectively point to point, so there's only 1 slot unless 
there's a PCIe switch in the middle. If that's the case, then it's all 
more complicated.

> > > Can you explain what you think that would look like in the DT?
> >
> > I *think* that's just some properties on the nodes for the endpoints,
> > note that the driver could just ignore them for now.  Not sure where or
> > if we document any extensions but child nodes are in section 4 of the
> > v2.1 PCI bus binding.
> 
> Hi Mark,
> 
> I'm a little confused -- here is how I remember the chronology of the
> "DT bindings" commit reviews, please correct me if I'm wrong:
> 
> o JimQ submitted a pullreq for using voltage regulators in the same
> style as the existing "rockport" PCIe driver.
> o After some deliberation, RobH preferred that the voltage regulators
> should go into the PCIe subnode device's DT node.

IIRC, that's because you said there isn't a standard slot.

> o JimQ put the voltage regulators in the subnode device's DT node.
> o MarkB didn't like the fact that the code did a global search for the
> regulator since it could not provide the owning struct device* handle.
> o RobH relented, and said that if it is just two specific and standard
> voltage regulators, perhaps they can go in the parent DT node after
> all.
> o JimQ put the regulators back in the PCIe node.
> o MarkB now wants the regulators to go back into the child node again?
> 
> Folks, please advise.
> 
> Regards,
> Jim Quinlan
> Broadcom STB

Re: [PATCH v4 2/6] dt-bindings: PCI: Add bindings for Brcmstb endpoint device voltage regulators

[Rob Herring]

On Tue, Apr 06, 2021 at 02:25:49PM -0400, Jim Quinlan wrote:
> On Tue, Apr 6, 2021 at 1:32 PM Mark Brown <broonie@kernel.org> wrote:
> >
> > On Tue, Apr 06, 2021 at 01:26:51PM -0400, Jim Quinlan wrote:
> > > On Tue, Apr 6, 2021 at 12:47 PM Mark Brown <broonie@kernel.org> wrote:
> >
> > > > No great problem with having these in the controller node (assming it
> > > > accurately describes the hardware) but I do think we ought to also be
> > > > able to describe these per slot.

PCIe is effectively point to point, so there's only 1 slot unless 
there's a PCIe switch in the middle. If that's the case, then it's all 
more complicated.

> > > Can you explain what you think that would look like in the DT?
> >
> > I *think* that's just some properties on the nodes for the endpoints,
> > note that the driver could just ignore them for now.  Not sure where or
> > if we document any extensions but child nodes are in section 4 of the
> > v2.1 PCI bus binding.
> 
> Hi Mark,
> 
> I'm a little confused -- here is how I remember the chronology of the
> "DT bindings" commit reviews, please correct me if I'm wrong:
> 
> o JimQ submitted a pullreq for using voltage regulators in the same
> style as the existing "rockport" PCIe driver.
> o After some deliberation, RobH preferred that the voltage regulators
> should go into the PCIe subnode device's DT node.

IIRC, that's because you said there isn't a standard slot.

> o JimQ put the voltage regulators in the subnode device's DT node.
> o MarkB didn't like the fact that the code did a global search for the
> regulator since it could not provide the owning struct device* handle.
> o RobH relented, and said that if it is just two specific and standard
> voltage regulators, perhaps they can go in the parent DT node after
> all.
> o JimQ put the regulators back in the PCIe node.
> o MarkB now wants the regulators to go back into the child node again?
> 
> Folks, please advise.
> 
> 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 v4 2/6] dt-bindings: PCI: Add bindings for Brcmstb endpoint device voltage regulators

[Mark Brown]

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

On Thu, Apr 08, 2021 at 11:20:16AM -0500, Rob Herring wrote:
> On Tue, Apr 06, 2021 at 02:25:49PM -0400, Jim Quinlan wrote:
> > On Tue, Apr 6, 2021 at 1:32 PM Mark Brown <broonie@kernel.org> wrote:

> > > > On Tue, Apr 6, 2021 at 12:47 PM Mark Brown <broonie@kernel.org> wrote:

> > > > > No great problem with having these in the controller node (assming it
> > > > > accurately describes the hardware) but I do think we ought to also be
> > > > > able to describe these per slot.

> PCIe is effectively point to point, so there's only 1 slot unless 
> there's a PCIe switch in the middle. If that's the case, then it's all 
> more complicated.

So even for PCIe that case exists, and it's not entirely clear to me
that this is all definitively PCIe specific.

> > o After some deliberation, RobH preferred that the voltage regulators
> > should go into the PCIe subnode device's DT node.

> IIRC, that's because you said there isn't a standard slot.

I recall some message in the thread that said both cases exist even with
this specific system.

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

Re: [PATCH v4 2/6] dt-bindings: PCI: Add bindings for Brcmstb endpoint device voltage regulators

[Mark Brown]

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

On Thu, Apr 08, 2021 at 11:20:16AM -0500, Rob Herring wrote:
> On Tue, Apr 06, 2021 at 02:25:49PM -0400, Jim Quinlan wrote:
> > On Tue, Apr 6, 2021 at 1:32 PM Mark Brown <broonie@kernel.org> wrote:

> > > > On Tue, Apr 6, 2021 at 12:47 PM Mark Brown <broonie@kernel.org> wrote:

> > > > > No great problem with having these in the controller node (assming it
> > > > > accurately describes the hardware) but I do think we ought to also be
> > > > > able to describe these per slot.

> PCIe is effectively point to point, so there's only 1 slot unless 
> there's a PCIe switch in the middle. If that's the case, then it's all 
> more complicated.

So even for PCIe that case exists, and it's not entirely clear to me
that this is all definitively PCIe specific.

> > o After some deliberation, RobH preferred that the voltage regulators
> > should go into the PCIe subnode device's DT node.

> IIRC, that's because you said there isn't a standard slot.

I recall some message in the thread that said both cases exist even with
this specific system.

[-- 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 v4 2/6] dt-bindings: PCI: Add bindings for Brcmstb endpoint device voltage regulators

[Jim Quinlan]

On Thu, Apr 8, 2021 at 12:20 PM Rob Herring <robh@kernel.org> wrote:
>
> On Tue, Apr 06, 2021 at 02:25:49PM -0400, Jim Quinlan wrote:
> > On Tue, Apr 6, 2021 at 1:32 PM Mark Brown <broonie@kernel.org> wrote:
> > >
> > > On Tue, Apr 06, 2021 at 01:26:51PM -0400, Jim Quinlan wrote:
> > > > On Tue, Apr 6, 2021 at 12:47 PM Mark Brown <broonie@kernel.org> wrote:
> > >
> > > > > No great problem with having these in the controller node (assming it
> > > > > accurately describes the hardware) but I do think we ought to also be
> > > > > able to describe these per slot.
>
> PCIe is effectively point to point, so there's only 1 slot unless
> there's a PCIe switch in the middle. If that's the case, then it's all
> more complicated.
>
> > > > Can you explain what you think that would look like in the DT?
> > >
> > > I *think* that's just some properties on the nodes for the endpoints,
> > > note that the driver could just ignore them for now.  Not sure where or
> > > if we document any extensions but child nodes are in section 4 of the
> > > v2.1 PCI bus binding.
> >
> > Hi Mark,
> >
> > I'm a little confused -- here is how I remember the chronology of the
> > "DT bindings" commit reviews, please correct me if I'm wrong:
> >
> > o JimQ submitted a pullreq for using voltage regulators in the same
> > style as the existing "rockport" PCIe driver.
> > o After some deliberation, RobH preferred that the voltage regulators
> > should go into the PCIe subnode device's DT node.
>
> IIRC, that's because you said there isn't a standard slot.
Admittedly, I'm not exactly sure what you mean by a "standard slot".
Our PCIIe HW does not support  hotplug or have a presence bit or
anything like that.  Our root complex has one port; it can only
directly connect to a single switch or endpoint. This connection shows
up as slot0.  The voltage regulator(s) involved depend on a GPIO that
turns the power  on/off for the connected device/chip.  The gpio pin
can vary from board to board but this is easily handled in our DT.
Some boards have regulators that are always on and not associated with
a GPIO pin -- these have no representation in our DT.

Regards,
Jim


>
> > o JimQ put the voltage regulators in the subnode device's DT node.
> > o MarkB didn't like the fact that the code did a global search for the
> > regulator since it could not provide the owning struct device* handle.
> > o RobH relented, and said that if it is just two specific and standard
> > voltage regulators, perhaps they can go in the parent DT node after
> > all.
> > o JimQ put the regulators back in the PCIe node.
> > o MarkB now wants the regulators to go back into the child node again?
> >
> > Folks, please advise.
> >
> > Regards,
> > Jim Quinlan
> > Broadcom STB

Re: [PATCH v4 2/6] dt-bindings: PCI: Add bindings for Brcmstb endpoint device voltage regulators

[Jim Quinlan]

On Thu, Apr 8, 2021 at 12:20 PM Rob Herring <robh@kernel.org> wrote:
>
> On Tue, Apr 06, 2021 at 02:25:49PM -0400, Jim Quinlan wrote:
> > On Tue, Apr 6, 2021 at 1:32 PM Mark Brown <broonie@kernel.org> wrote:
> > >
> > > On Tue, Apr 06, 2021 at 01:26:51PM -0400, Jim Quinlan wrote:
> > > > On Tue, Apr 6, 2021 at 12:47 PM Mark Brown <broonie@kernel.org> wrote:
> > >
> > > > > No great problem with having these in the controller node (assming it
> > > > > accurately describes the hardware) but I do think we ought to also be
> > > > > able to describe these per slot.
>
> PCIe is effectively point to point, so there's only 1 slot unless
> there's a PCIe switch in the middle. If that's the case, then it's all
> more complicated.
>
> > > > Can you explain what you think that would look like in the DT?
> > >
> > > I *think* that's just some properties on the nodes for the endpoints,
> > > note that the driver could just ignore them for now.  Not sure where or
> > > if we document any extensions but child nodes are in section 4 of the
> > > v2.1 PCI bus binding.
> >
> > Hi Mark,
> >
> > I'm a little confused -- here is how I remember the chronology of the
> > "DT bindings" commit reviews, please correct me if I'm wrong:
> >
> > o JimQ submitted a pullreq for using voltage regulators in the same
> > style as the existing "rockport" PCIe driver.
> > o After some deliberation, RobH preferred that the voltage regulators
> > should go into the PCIe subnode device's DT node.
>
> IIRC, that's because you said there isn't a standard slot.
Admittedly, I'm not exactly sure what you mean by a "standard slot".
Our PCIIe HW does not support  hotplug or have a presence bit or
anything like that.  Our root complex has one port; it can only
directly connect to a single switch or endpoint. This connection shows
up as slot0.  The voltage regulator(s) involved depend on a GPIO that
turns the power  on/off for the connected device/chip.  The gpio pin
can vary from board to board but this is easily handled in our DT.
Some boards have regulators that are always on and not associated with
a GPIO pin -- these have no representation in our DT.

Regards,
Jim


>
> > o JimQ put the voltage regulators in the subnode device's DT node.
> > o MarkB didn't like the fact that the code did a global search for the
> > regulator since it could not provide the owning struct device* handle.
> > o RobH relented, and said that if it is just two specific and standard
> > voltage regulators, perhaps they can go in the parent DT node after
> > all.
> > o JimQ put the regulators back in the PCIe node.
> > o MarkB now wants the regulators to go back into the child node again?
> >
> > Folks, please advise.
> >
> > 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 v4 2/6] dt-bindings: PCI: Add bindings for Brcmstb endpoint device voltage regulators

[Rob Herring]

On Thu, Apr 08, 2021 at 12:58:05PM -0400, Jim Quinlan wrote:
> On Thu, Apr 8, 2021 at 12:20 PM Rob Herring <robh@kernel.org> wrote:
> >
> > On Tue, Apr 06, 2021 at 02:25:49PM -0400, Jim Quinlan wrote:
> > > On Tue, Apr 6, 2021 at 1:32 PM Mark Brown <broonie@kernel.org> wrote:
> > > >
> > > > On Tue, Apr 06, 2021 at 01:26:51PM -0400, Jim Quinlan wrote:
> > > > > On Tue, Apr 6, 2021 at 12:47 PM Mark Brown <broonie@kernel.org> wrote:
> > > >
> > > > > > No great problem with having these in the controller node (assming it
> > > > > > accurately describes the hardware) but I do think we ought to also be
> > > > > > able to describe these per slot.
> >
> > PCIe is effectively point to point, so there's only 1 slot unless
> > there's a PCIe switch in the middle. If that's the case, then it's all
> > more complicated.
> >
> > > > > Can you explain what you think that would look like in the DT?
> > > >
> > > > I *think* that's just some properties on the nodes for the endpoints,
> > > > note that the driver could just ignore them for now.  Not sure where or
> > > > if we document any extensions but child nodes are in section 4 of the
> > > > v2.1 PCI bus binding.
> > >
> > > Hi Mark,
> > >
> > > I'm a little confused -- here is how I remember the chronology of the
> > > "DT bindings" commit reviews, please correct me if I'm wrong:
> > >
> > > o JimQ submitted a pullreq for using voltage regulators in the same
> > > style as the existing "rockport" PCIe driver.
> > > o After some deliberation, RobH preferred that the voltage regulators
> > > should go into the PCIe subnode device's DT node.
> >
> > IIRC, that's because you said there isn't a standard slot.
> Admittedly, I'm not exactly sure what you mean by a "standard slot".
> Our PCIIe HW does not support  hotplug or have a presence bit or
> anything like that.  Our root complex has one port; it can only
> directly connect to a single switch or endpoint. This connection shows
> up as slot0.  The voltage regulator(s) involved depend on a GPIO that
> turns the power  on/off for the connected device/chip.  The gpio pin
> can vary from board to board but this is easily handled in our DT.
> Some boards have regulators that are always on and not associated with
> a GPIO pin -- these have no representation in our DT.

By standard slot, I mean you have standard voltage rails 12V and 3.3V 
(or 1.5 and 3.3 for mini PCIe) and PERST# signal, no other extra 
things to make a device discoverable, and the timing for 
those rails and PERST# follow what the spec defines.

There's also CLKREQ, WAKE, and hotplug detect signals, but I think those 
are all optional and could be tied off. I think most PCI h/w is not 
hotplug capable.

Rob

Re: [PATCH v4 2/6] dt-bindings: PCI: Add bindings for Brcmstb endpoint device voltage regulators

[Rob Herring]

On Thu, Apr 08, 2021 at 12:58:05PM -0400, Jim Quinlan wrote:
> On Thu, Apr 8, 2021 at 12:20 PM Rob Herring <robh@kernel.org> wrote:
> >
> > On Tue, Apr 06, 2021 at 02:25:49PM -0400, Jim Quinlan wrote:
> > > On Tue, Apr 6, 2021 at 1:32 PM Mark Brown <broonie@kernel.org> wrote:
> > > >
> > > > On Tue, Apr 06, 2021 at 01:26:51PM -0400, Jim Quinlan wrote:
> > > > > On Tue, Apr 6, 2021 at 12:47 PM Mark Brown <broonie@kernel.org> wrote:
> > > >
> > > > > > No great problem with having these in the controller node (assming it
> > > > > > accurately describes the hardware) but I do think we ought to also be
> > > > > > able to describe these per slot.
> >
> > PCIe is effectively point to point, so there's only 1 slot unless
> > there's a PCIe switch in the middle. If that's the case, then it's all
> > more complicated.
> >
> > > > > Can you explain what you think that would look like in the DT?
> > > >
> > > > I *think* that's just some properties on the nodes for the endpoints,
> > > > note that the driver could just ignore them for now.  Not sure where or
> > > > if we document any extensions but child nodes are in section 4 of the
> > > > v2.1 PCI bus binding.
> > >
> > > Hi Mark,
> > >
> > > I'm a little confused -- here is how I remember the chronology of the
> > > "DT bindings" commit reviews, please correct me if I'm wrong:
> > >
> > > o JimQ submitted a pullreq for using voltage regulators in the same
> > > style as the existing "rockport" PCIe driver.
> > > o After some deliberation, RobH preferred that the voltage regulators
> > > should go into the PCIe subnode device's DT node.
> >
> > IIRC, that's because you said there isn't a standard slot.
> Admittedly, I'm not exactly sure what you mean by a "standard slot".
> Our PCIIe HW does not support  hotplug or have a presence bit or
> anything like that.  Our root complex has one port; it can only
> directly connect to a single switch or endpoint. This connection shows
> up as slot0.  The voltage regulator(s) involved depend on a GPIO that
> turns the power  on/off for the connected device/chip.  The gpio pin
> can vary from board to board but this is easily handled in our DT.
> Some boards have regulators that are always on and not associated with
> a GPIO pin -- these have no representation in our DT.

By standard slot, I mean you have standard voltage rails 12V and 3.3V 
(or 1.5 and 3.3 for mini PCIe) and PERST# signal, no other extra 
things to make a device discoverable, and the timing for 
those rails and PERST# follow what the spec defines.

There's also CLKREQ, WAKE, and hotplug detect signals, but I think those 
are all optional and could be tied off. I think most PCI h/w is not 
hotplug capable.

Rob

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

Re: [PATCH v4 2/6] dt-bindings: PCI: Add bindings for Brcmstb endpoint device voltage regulators

[Rob Herring]

On Tue, Apr 06, 2021 at 05:47:08PM +0100, Mark Brown wrote:
> On Thu, Apr 01, 2021 at 05:21:42PM -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 | 4 ++++
> >  1 file changed, 4 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml b/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
> > index f90557f6deb8..f2caa5b3b281 100644
> > --- a/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
> > +++ b/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
> > @@ -64,6 +64,9 @@ properties:
> >  
> >    aspm-no-l0s: true
> >  
> > +  vpcie12v-supply: true
> > +  vpcie3v3-supply: true
> > +
> 
> No great problem with having these in the controller node (assming it
> accurately describes the hardware) but I do think we ought to also be
> able to describe these per slot.

My hesistancy here is child nodes are already defined to be devices, not 
slots. That's generally the same thing though. However, 'reg' is a bit 
problematic as it includes the bus number which is dynamically 
assigned. (This is a mismatch between true OpenFirmware and FDT as OF 
was designed to populate the DT with what was discovered and that 
includes some runtime config such as bus number assignments.) Maybe we 
just say for FDT, the bus number is always 0 and ignored. In any case, 
there needs to be some thought on this as well as the more complicated 
case of devices needing device specific setup to be enumerated.

Rob

Re: [PATCH v4 2/6] dt-bindings: PCI: Add bindings for Brcmstb endpoint device voltage regulators

[Rob Herring]

On Tue, Apr 06, 2021 at 05:47:08PM +0100, Mark Brown wrote:
> On Thu, Apr 01, 2021 at 05:21:42PM -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 | 4 ++++
> >  1 file changed, 4 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml b/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
> > index f90557f6deb8..f2caa5b3b281 100644
> > --- a/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
> > +++ b/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
> > @@ -64,6 +64,9 @@ properties:
> >  
> >    aspm-no-l0s: true
> >  
> > +  vpcie12v-supply: true
> > +  vpcie3v3-supply: true
> > +
> 
> No great problem with having these in the controller node (assming it
> accurately describes the hardware) but I do think we ought to also be
> able to describe these per slot.

My hesistancy here is child nodes are already defined to be devices, not 
slots. That's generally the same thing though. However, 'reg' is a bit 
problematic as it includes the bus number which is dynamically 
assigned. (This is a mismatch between true OpenFirmware and FDT as OF 
was designed to populate the DT with what was discovered and that 
includes some runtime config such as bus number assignments.) Maybe we 
just say for FDT, the bus number is always 0 and ignored. In any case, 
there needs to be some thought on this as well as the more complicated 
case of devices needing device specific setup to be enumerated.

Rob

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

[PATCH v4 3/6] PCI: brcmstb: Add control of slot0 device voltage regulators

[Jim Quinlan]

This Broadcom STB has one port and connects directly to one device, be it a
switch or an endpoint.  We want to be able to turn on/off any regulators
for that device.  Control of regulators is needed because of the
chicken-and-egg situation: although the regulator is "owned" by the device
and would be best handled by its driver, the device 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 | 83 +++++++++++++++++++++++++--
 1 file changed, 78 insertions(+), 5 deletions(-)

diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
index 4ce1f3a60574..1b0de0c7da60 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])
@@ -192,6 +194,11 @@ 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 const char * const supplies[] = {
+	"vpcie12v-supply",
+	"vpcie3v3-supply",
+};
+
 enum {
 	RGR1_SW_INIT_1,
 	EXT_CFG_INDEX,
@@ -295,8 +302,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
@@ -1112,9 +1138,10 @@ static inline int brcm_phy_start(struct brcm_pcie *pcie)
 	return pcie->rescal ? brcm_phy_cntl(pcie, 1) : 0;
 }
 
-static inline int brcm_phy_stop(struct brcm_pcie *pcie)
+static inline void brcm_phy_stop(struct brcm_pcie *pcie)
 {
-	return pcie->rescal ? brcm_phy_cntl(pcie, 0) : 0;
+	if (pcie->rescal)
+		brcm_phy_cntl(pcie, 0);
 }
 
 static void brcm_pcie_turn_off(struct brcm_pcie *pcie)
@@ -1141,16 +1168,45 @@ 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 *np = pcie->np;
+	struct property *pp;
+	const unsigned int ns = ARRAY_SIZE(supplies);
+	unsigned int i;
+	int ret = 0;
+
+	/* Look for specific pcie regulators in the RC DT node. */
+	for_each_property_of_node(np, pp) {
+		for (i = 0; i < ns; i++)
+			if (strcmp(supplies[i], pp->name) == 0)
+				break;
+		if (i >= ns)
+			continue;
+
+		if (pcie->num_supplies < PCIE_BRCM_MAX_EP_REGULATORS)
+			pcie->supplies[pcie->num_supplies++].supply
+				= supplies[i];
+		else
+			dev_warn(pcie->dev, "No room for supply %s\n",
+				 supplies[i]);
+	}
+
+	if (pcie->num_supplies)
+		ret = devm_regulator_bulk_get(pcie->dev, pcie->num_supplies,
+					      pcie->supplies);
+	return ret;
+}
+
 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)
@@ -1165,6 +1221,10 @@ static int brcm_pcie_resume(struct device *dev)
 	if (ret)
 		return ret;
 
+	ret = brcm_set_regulators(pcie, true);
+	if (ret)
+		return ret;
+
 	ret = brcm_phy_start(pcie);
 	if (ret)
 		goto err;
@@ -1201,6 +1261,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);
 }
 
 static int brcm_pcie_remove(struct platform_device *pdev)
@@ -1291,6 +1352,18 @@ static int brcm_pcie_probe(struct platform_device *pdev)
 		return ret;
 	}
 
+	ret = brcm_pcie_get_regulators(pcie);
+	if (ret) {
+		pcie->num_supplies = 0;
+		if (ret != -EPROBE_DEFER)
+			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 v4 3/6] PCI: brcmstb: Add control of slot0 device voltage regulators

[Jim Quinlan]

This Broadcom STB has one port and connects directly to one device, be it a
switch or an endpoint.  We want to be able to turn on/off any regulators
for that device.  Control of regulators is needed because of the
chicken-and-egg situation: although the regulator is "owned" by the device
and would be best handled by its driver, the device 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 | 83 +++++++++++++++++++++++++--
 1 file changed, 78 insertions(+), 5 deletions(-)

diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
index 4ce1f3a60574..1b0de0c7da60 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])
@@ -192,6 +194,11 @@ 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 const char * const supplies[] = {
+	"vpcie12v-supply",
+	"vpcie3v3-supply",
+};
+
 enum {
 	RGR1_SW_INIT_1,
 	EXT_CFG_INDEX,
@@ -295,8 +302,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
@@ -1112,9 +1138,10 @@ static inline int brcm_phy_start(struct brcm_pcie *pcie)
 	return pcie->rescal ? brcm_phy_cntl(pcie, 1) : 0;
 }
 
-static inline int brcm_phy_stop(struct brcm_pcie *pcie)
+static inline void brcm_phy_stop(struct brcm_pcie *pcie)
 {
-	return pcie->rescal ? brcm_phy_cntl(pcie, 0) : 0;
+	if (pcie->rescal)
+		brcm_phy_cntl(pcie, 0);
 }
 
 static void brcm_pcie_turn_off(struct brcm_pcie *pcie)
@@ -1141,16 +1168,45 @@ 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 *np = pcie->np;
+	struct property *pp;
+	const unsigned int ns = ARRAY_SIZE(supplies);
+	unsigned int i;
+	int ret = 0;
+
+	/* Look for specific pcie regulators in the RC DT node. */
+	for_each_property_of_node(np, pp) {
+		for (i = 0; i < ns; i++)
+			if (strcmp(supplies[i], pp->name) == 0)
+				break;
+		if (i >= ns)
+			continue;
+
+		if (pcie->num_supplies < PCIE_BRCM_MAX_EP_REGULATORS)
+			pcie->supplies[pcie->num_supplies++].supply
+				= supplies[i];
+		else
+			dev_warn(pcie->dev, "No room for supply %s\n",
+				 supplies[i]);
+	}
+
+	if (pcie->num_supplies)
+		ret = devm_regulator_bulk_get(pcie->dev, pcie->num_supplies,
+					      pcie->supplies);
+	return ret;
+}
+
 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)
@@ -1165,6 +1221,10 @@ static int brcm_pcie_resume(struct device *dev)
 	if (ret)
 		return ret;
 
+	ret = brcm_set_regulators(pcie, true);
+	if (ret)
+		return ret;
+
 	ret = brcm_phy_start(pcie);
 	if (ret)
 		goto err;
@@ -1201,6 +1261,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);
 }
 
 static int brcm_pcie_remove(struct platform_device *pdev)
@@ -1291,6 +1352,18 @@ static int brcm_pcie_probe(struct platform_device *pdev)
 		return ret;
 	}
 
+	ret = brcm_pcie_get_regulators(pcie);
+	if (ret) {
+		pcie->num_supplies = 0;
+		if (ret != -EPROBE_DEFER)
+			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 v4 3/6] PCI: brcmstb: Add control of slot0 device voltage regulators

[Mark Brown]

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

On Thu, Apr 01, 2021 at 05:21:43PM -0400, Jim Quinlan wrote:

> +	/* Look for specific pcie regulators in the RC DT node. */
> +	for_each_property_of_node(np, pp) {
> +		for (i = 0; i < ns; i++)
> +			if (strcmp(supplies[i], pp->name) == 0)

This is broken, the driver knows which supplies are expected, the device
can't function without these supplies so the driver should just
unconditionally request them like any other supply.

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

Re: [PATCH v4 3/6] PCI: brcmstb: Add control of slot0 device voltage regulators

[Mark Brown]

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

On Thu, Apr 01, 2021 at 05:21:43PM -0400, Jim Quinlan wrote:

> +	/* Look for specific pcie regulators in the RC DT node. */
> +	for_each_property_of_node(np, pp) {
> +		for (i = 0; i < ns; i++)
> +			if (strcmp(supplies[i], pp->name) == 0)

This is broken, the driver knows which supplies are expected, the device
can't function without these supplies so the driver should just
unconditionally request them like any other supply.

[-- 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 v4 3/6] PCI: brcmstb: Add control of slot0 device voltage regulators

[Jim Quinlan]

On Tue, Apr 6, 2021 at 12:34 PM Mark Brown <broonie@kernel.org> wrote:
>
> On Thu, Apr 01, 2021 at 05:21:43PM -0400, Jim Quinlan wrote:
>
> > +     /* Look for specific pcie regulators in the RC DT node. */
> > +     for_each_property_of_node(np, pp) {
> > +             for (i = 0; i < ns; i++)
> > +                     if (strcmp(supplies[i], pp->name) == 0)
>
> This is broken, the driver knows which supplies are expected, the device
> can't function without these supplies so the driver should just
> unconditionally request them like any other supply.

Hi  Mark,
Some boards require the regulators, some do not.  So the driver is
only  sure what the names may be if they are present.  If  I put these
names in my struct regulator_bulk_data array and do a
devm_regulator_bulk_get(), I will get the following for the boards
that do not need the regulators (e.g. the RPi SOC):

[    6.823820] brcm-pcie xxx.pcie: supply vpcie12v-supply not found,
using dummy regulator
[    6.832265] brcm-pcie xxx.pcie: supply vpcie3v3-supply not found,
using dummy regulator

IIRC you consider this a debug feature?  Be that as it may, these
lines will confuse our customers and I'd like that they not be printed
if possible.

So I ask you to allow the code as is.  If you still insist, I will
change and resubmit.

Regards,
Jim Quinlan
Broadcom STB

Re: [PATCH v4 3/6] PCI: brcmstb: Add control of slot0 device voltage regulators

[Jim Quinlan]

On Tue, Apr 6, 2021 at 12:34 PM Mark Brown <broonie@kernel.org> wrote:
>
> On Thu, Apr 01, 2021 at 05:21:43PM -0400, Jim Quinlan wrote:
>
> > +     /* Look for specific pcie regulators in the RC DT node. */
> > +     for_each_property_of_node(np, pp) {
> > +             for (i = 0; i < ns; i++)
> > +                     if (strcmp(supplies[i], pp->name) == 0)
>
> This is broken, the driver knows which supplies are expected, the device
> can't function without these supplies so the driver should just
> unconditionally request them like any other supply.

Hi  Mark,
Some boards require the regulators, some do not.  So the driver is
only  sure what the names may be if they are present.  If  I put these
names in my struct regulator_bulk_data array and do a
devm_regulator_bulk_get(), I will get the following for the boards
that do not need the regulators (e.g. the RPi SOC):

[    6.823820] brcm-pcie xxx.pcie: supply vpcie12v-supply not found,
using dummy regulator
[    6.832265] brcm-pcie xxx.pcie: supply vpcie3v3-supply not found,
using dummy regulator

IIRC you consider this a debug feature?  Be that as it may, these
lines will confuse our customers and I'd like that they not be printed
if possible.

So I ask you to allow the code as is.  If you still insist, I will
change and resubmit.

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 v4 3/6] PCI: brcmstb: Add control of slot0 device voltage regulators

[Mark Brown]

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

On Tue, Apr 06, 2021 at 12:59:16PM -0400, Jim Quinlan wrote:
> On Tue, Apr 6, 2021 at 12:34 PM Mark Brown <broonie@kernel.org> wrote:
> > On Thu, Apr 01, 2021 at 05:21:43PM -0400, Jim Quinlan wrote:

> > This is broken, the driver knows which supplies are expected, the device
> > can't function without these supplies so the driver should just
> > unconditionally request them like any other supply.

> Some boards require the regulators, some do not.  So the driver is

No, some boards have the supplies described in firmware and some do not.

> only  sure what the names may be if they are present.  If  I put these
> names in my struct regulator_bulk_data array and do a
> devm_regulator_bulk_get(), I will get the following for the boards
> that do not need the regulators (e.g. the RPi SOC):
> 
> [    6.823820] brcm-pcie xxx.pcie: supply vpcie12v-supply not found,
> using dummy regulator
> [    6.832265] brcm-pcie xxx.pcie: supply vpcie3v3-supply not found,
> using dummy regulator

Sure, those are just warnings.

> IIRC you consider this a debug feature?  Be that as it may, these
> lines will confuse our customers and I'd like that they not be printed
> if possible.

You can stop the warnings by updating your firmware to more completely
describe the system - ideally all the supplies in the system would be
described for future proofing.  Or if this is a custom software stack
just delete whatever error checking and warnings you like.  The warnings
are there in case we've not got something mapped properly (eg, if there
were a typo in a property name) and things stop working, it's not great
to just ignore errors.

> So I ask you to allow the code as is.  If you still insist, I will
> change and resubmit.

Remove it, conditional code like this is just as bad in this driver as
it is in every other one.

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

Re: [PATCH v4 3/6] PCI: brcmstb: Add control of slot0 device voltage regulators

[Mark Brown]

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

On Tue, Apr 06, 2021 at 12:59:16PM -0400, Jim Quinlan wrote:
> On Tue, Apr 6, 2021 at 12:34 PM Mark Brown <broonie@kernel.org> wrote:
> > On Thu, Apr 01, 2021 at 05:21:43PM -0400, Jim Quinlan wrote:

> > This is broken, the driver knows which supplies are expected, the device
> > can't function without these supplies so the driver should just
> > unconditionally request them like any other supply.

> Some boards require the regulators, some do not.  So the driver is

No, some boards have the supplies described in firmware and some do not.

> only  sure what the names may be if they are present.  If  I put these
> names in my struct regulator_bulk_data array and do a
> devm_regulator_bulk_get(), I will get the following for the boards
> that do not need the regulators (e.g. the RPi SOC):
> 
> [    6.823820] brcm-pcie xxx.pcie: supply vpcie12v-supply not found,
> using dummy regulator
> [    6.832265] brcm-pcie xxx.pcie: supply vpcie3v3-supply not found,
> using dummy regulator

Sure, those are just warnings.

> IIRC you consider this a debug feature?  Be that as it may, these
> lines will confuse our customers and I'd like that they not be printed
> if possible.

You can stop the warnings by updating your firmware to more completely
describe the system - ideally all the supplies in the system would be
described for future proofing.  Or if this is a custom software stack
just delete whatever error checking and warnings you like.  The warnings
are there in case we've not got something mapped properly (eg, if there
were a typo in a property name) and things stop working, it's not great
to just ignore errors.

> So I ask you to allow the code as is.  If you still insist, I will
> change and resubmit.

Remove it, conditional code like this is just as bad in this driver as
it is in every other one.

[-- 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 v4 3/6] PCI: brcmstb: Add control of slot0 device voltage regulators

[Jim Quinlan]

On Tue, Apr 6, 2021 at 1:23 PM Mark Brown <broonie@kernel.org> wrote:
>
> On Tue, Apr 06, 2021 at 12:59:16PM -0400, Jim Quinlan wrote:
> > On Tue, Apr 6, 2021 at 12:34 PM Mark Brown <broonie@kernel.org> wrote:
> > > On Thu, Apr 01, 2021 at 05:21:43PM -0400, Jim Quinlan wrote:
>
> > > This is broken, the driver knows which supplies are expected, the device
> > > can't function without these supplies so the driver should just
> > > unconditionally request them like any other supply.
>
> > Some boards require the regulators, some do not.  So the driver is
>
> No, some boards have the supplies described in firmware and some do not.
True.
>
> > only  sure what the names may be if they are present.  If  I put these
> > names in my struct regulator_bulk_data array and do a
> > devm_regulator_bulk_get(), I will get the following for the boards
> > that do not need the regulators (e.g. the RPi SOC):
> >
> > [    6.823820] brcm-pcie xxx.pcie: supply vpcie12v-supply not found,
> > using dummy regulator
> > [    6.832265] brcm-pcie xxx.pcie: supply vpcie3v3-supply not found,
> > using dummy regulator
>
> Sure, those are just warnings.
>
> > IIRC you consider this a debug feature?  Be that as it may, these
> > lines will confuse our customers and I'd like that they not be printed
> > if possible.
>
> You can stop the warnings by updating your firmware to more completely
> describe the system - ideally all the supplies in the system would be
> described for future proofing.  Or if this is a custom software stack
> just delete whatever error checking and warnings you like.  The warnings
> are there in case we've not got something mapped properly (eg, if there
> were a typo in a property name) and things stop working, it's not great
> to just ignore errors.
A lot of this is really not under our control.
>
> > So I ask you to allow the code as is.  If you still insist, I will
> > change and resubmit.
>
> Remove it, conditional code like this is just as bad in this driver as
> it is in every other one.
I will remove this and resubmit.
Thanks,
Jim Quinlan
Broadcom STB

Re: [PATCH v4 3/6] PCI: brcmstb: Add control of slot0 device voltage regulators

[Jim Quinlan]

On Tue, Apr 6, 2021 at 1:23 PM Mark Brown <broonie@kernel.org> wrote:
>
> On Tue, Apr 06, 2021 at 12:59:16PM -0400, Jim Quinlan wrote:
> > On Tue, Apr 6, 2021 at 12:34 PM Mark Brown <broonie@kernel.org> wrote:
> > > On Thu, Apr 01, 2021 at 05:21:43PM -0400, Jim Quinlan wrote:
>
> > > This is broken, the driver knows which supplies are expected, the device
> > > can't function without these supplies so the driver should just
> > > unconditionally request them like any other supply.
>
> > Some boards require the regulators, some do not.  So the driver is
>
> No, some boards have the supplies described in firmware and some do not.
True.
>
> > only  sure what the names may be if they are present.  If  I put these
> > names in my struct regulator_bulk_data array and do a
> > devm_regulator_bulk_get(), I will get the following for the boards
> > that do not need the regulators (e.g. the RPi SOC):
> >
> > [    6.823820] brcm-pcie xxx.pcie: supply vpcie12v-supply not found,
> > using dummy regulator
> > [    6.832265] brcm-pcie xxx.pcie: supply vpcie3v3-supply not found,
> > using dummy regulator
>
> Sure, those are just warnings.
>
> > IIRC you consider this a debug feature?  Be that as it may, these
> > lines will confuse our customers and I'd like that they not be printed
> > if possible.
>
> You can stop the warnings by updating your firmware to more completely
> describe the system - ideally all the supplies in the system would be
> described for future proofing.  Or if this is a custom software stack
> just delete whatever error checking and warnings you like.  The warnings
> are there in case we've not got something mapped properly (eg, if there
> were a typo in a property name) and things stop working, it's not great
> to just ignore errors.
A lot of this is really not under our control.
>
> > So I ask you to allow the code as is.  If you still insist, I will
> > change and resubmit.
>
> Remove it, conditional code like this is just as bad in this driver as
> it is in every other one.
I will remove this and resubmit.
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

[PATCH v4 4/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 1b0de0c7da60..4c79aff66de7 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);
 
 static const char * const supplies[] = {
 	"vpcie12v-supply",
@@ -304,22 +305,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 regulators 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;
 }
 
@@ -1206,7 +1250,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)
@@ -1221,7 +1265,7 @@ static int brcm_pcie_resume(struct device *dev)
 	if (ret)
 		return ret;
 
-	ret = brcm_set_regulators(pcie, true);
+	ret = brcm_set_regulators(pcie, TURN_ON);
 	if (ret)
 		return ret;
 
@@ -1261,7 +1305,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);
 }
 
 static int brcm_pcie_remove(struct platform_device *pdev)
@@ -1360,7 +1404,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 v4 4/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 1b0de0c7da60..4c79aff66de7 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);
 
 static const char * const supplies[] = {
 	"vpcie12v-supply",
@@ -304,22 +305,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 regulators 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;
 }
 
@@ -1206,7 +1250,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)
@@ -1221,7 +1265,7 @@ static int brcm_pcie_resume(struct device *dev)
 	if (ret)
 		return ret;
 
-	ret = brcm_set_regulators(pcie, true);
+	ret = brcm_set_regulators(pcie, TURN_ON);
 	if (ret)
 		return ret;
 
@@ -1261,7 +1305,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);
 }
 
 static int brcm_pcie_remove(struct platform_device *pdev)
@@ -1360,7 +1404,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

[PATCH v4 5/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 4c79aff66de7..44128df33785 100644
--- a/drivers/pci/controller/pcie-brcmstb.c
+++ b/drivers/pci/controller/pcie-brcmstb.c
@@ -265,6 +265,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;
@@ -1325,7 +1332,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 v4 5/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 4c79aff66de7..44128df33785 100644
--- a/drivers/pci/controller/pcie-brcmstb.c
+++ b/drivers/pci/controller/pcie-brcmstb.c
@@ -265,6 +265,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;
@@ -1325,7 +1332,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 v4 6/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 44128df33785..73a12c62b94e 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);
@@ -223,6 +258,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[] = {
@@ -270,6 +306,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 {
@@ -313,8 +350,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;
@@ -1321,6 +1437,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);
 
@@ -1360,6 +1478,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;
 
@@ -1439,6 +1558,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 v4 6/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 44128df33785..73a12c62b94e 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);
@@ -223,6 +258,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[] = {
@@ -270,6 +306,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 {
@@ -313,8 +350,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;
@@ -1321,6 +1437,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);
 
@@ -1360,6 +1478,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;
 
@@ -1439,6 +1558,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