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

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

[Jim Quinlan]

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

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

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


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

 .../bindings/pci/brcm,stb-pcie.yaml           |  12 +
 drivers/pci/controller/pcie-brcmstb.c         | 219 +++++++++++++++++-
 2 files changed, 228 insertions(+), 3 deletions(-)

-- 
2.17.1


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

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

[Jim Quinlan]

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

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

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

diff --git a/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml b/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
index 807694b4f41f..baacc3d7ec87 100644
--- a/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
+++ b/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
@@ -85,6 +85,18 @@ properties:
       minItems: 1
       maxItems: 3
 
+  vpcie12v-supply:
+    description: 12v regulator phandle for the endpoint device
+
+  vpcie3v3-supply:
+    description: 3.3v regulator phandle for the endpoint device
+
+  vpcie1v8-supply:
+    description: 1.8v regulator phandle for the endpoint device
+
+  vpcie0v9-supply:
+    description: 0.9v regulator phandle for the endpoint device
+
 required:
   - reg
   - ranges
-- 
2.17.1


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

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

[Rob Herring]

On Mon, Nov 30, 2020 at 04:11:38PM -0500, Jim Quinlan wrote:
> Quite 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.
> 
> Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com>
> ---
>  .../devicetree/bindings/pci/brcm,stb-pcie.yaml       | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml b/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
> index 807694b4f41f..baacc3d7ec87 100644
> --- a/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
> +++ b/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
> @@ -85,6 +85,18 @@ properties:
>        minItems: 1
>        maxItems: 3
>  
> +  vpcie12v-supply:
> +    description: 12v regulator phandle for the endpoint device
> +
> +  vpcie3v3-supply:
> +    description: 3.3v regulator phandle for the endpoint device

12V and 3.3V are standard slot supplies, can you add them to 
pci-bus.yaml. Then some day maybe we can have common slot handling code.
 
With that, here you just need:

vpcie3v3-supply: true

> +
> +  vpcie1v8-supply:
> +    description: 1.8v regulator phandle for the endpoint device
> +
> +  vpcie0v9-supply:
> +    description: 0.9v regulator phandle for the endpoint device

These are not standard. They go to a soldered down device or 
non-standard connector? For the former, the device should really be 
described in DT and the supplies added there.

Mini PCIe connector also has 1.5V supply.

Rob

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

[Jim Quinlan]

On Wed, Dec 9, 2020 at 10:07 AM Rob Herring <robh@kernel.org> wrote:
>
> On Mon, Nov 30, 2020 at 04:11:38PM -0500, Jim Quinlan wrote:
> > Quite 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.
> >
> > Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com>
> > ---
> >  .../devicetree/bindings/pci/brcm,stb-pcie.yaml       | 12 ++++++++++++
> >  1 file changed, 12 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml b/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
> > index 807694b4f41f..baacc3d7ec87 100644
> > --- a/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
> > +++ b/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
> > @@ -85,6 +85,18 @@ properties:
> >        minItems: 1
> >        maxItems: 3
> >
> > +  vpcie12v-supply:
> > +    description: 12v regulator phandle for the endpoint device
> > +
> > +  vpcie3v3-supply:
> > +    description: 3.3v regulator phandle for the endpoint device
>
> 12V and 3.3V are standard slot supplies, can you add them to
> pci-bus.yaml. Then some day maybe we can have common slot handling code.
>
> With that, here you just need:
>
> vpcie3v3-supply: true

Hi Rob,

Sorry for the delay in responding -- I just came back from vacation.

The problem we have is that these regulators are not "slot" supplies
-- our HW does not support PCI slots, so if and when general slot
power-handling code came along it would probably screw us up.   If you
don't think there is a problem then I will submit the two supply-names
you OKed, even though they may not match the voltages we are using for
the EPs.

For us, the supplies are for the EP chip's power.  We have the PCIe
controller turning them "on" for power-on/resume and "off" for
power-off/suspend.  We need the "xxx-supply" property in the
controller's DT node because of the chicken-and-egg situation: if the
property was in the EP's DT node, the RC  will never discover the EP
to see that there is a regulator to turn on.   We would be happy with
a single supply name, something like "ep-power".  We would be ecstatic
to have two (ep0-power, ep1-power).

I'm not sure if you remember but FlorianF talked to you about this
situation and concluded that something like the above was the way to
go forward.  For the latest pullreq I  just copied Rockchip's bindings
since you reviewed their bindings commit but it looks like you've
changed your mind.   Given the constraints I have described, what is
the best path forward?

Thanks,
Jim Quinlan
Broadcom STB
>
> > +
> > +  vpcie1v8-supply:
> > +    description: 1.8v regulator phandle for the endpoint device
> > +
> > +  vpcie0v9-supply:
> > +    description: 0.9v regulator phandle for the endpoint device
>
> These are not standard. They go to a soldered down device or
> non-standard connector? For the former, the device should really be
> described in DT and the supplies added there.
>
> Mini PCIe connector also has 1.5V supply.
>
> Rob

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

[Mark Brown]

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

On Mon, Jan 04, 2021 at 05:12:11PM -0500, Jim Quinlan wrote:

> For us, the supplies are for the EP chip's power.  We have the PCIe
> controller turning them "on" for power-on/resume and "off" for
> power-off/suspend.  We need the "xxx-supply" property in the
> controller's DT node because of the chicken-and-egg situation: if the
> property was in the EP's DT node, the RC  will never discover the EP
> to see that there is a regulator to turn on.   We would be happy with
> a single supply name, something like "ep-power".  We would be ecstatic
> to have two (ep0-power, ep1-power).

Why can't the controller look at the nodes describing devices for
standard properties?

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

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

[Jim Quinlan]

On Tue, Jan 5, 2021 at 9:01 AM Mark Brown <broonie@kernel.org> wrote:
>
> On Mon, Jan 04, 2021 at 05:12:11PM -0500, Jim Quinlan wrote:
>
> > For us, the supplies are for the EP chip's power.  We have the PCIe
> > controller turning them "on" for power-on/resume and "off" for
> > power-off/suspend.  We need the "xxx-supply" property in the
> > controller's DT node because of the chicken-and-egg situation: if the
> > property was in the EP's DT node, the RC  will never discover the EP
> > to see that there is a regulator to turn on.   We would be happy with
> > a single supply name, something like "ep-power".  We would be ecstatic
> > to have two (ep0-power, ep1-power).
>
> Why can't the controller look at the nodes describing devices for
> standard properties?
Hi Mark,

It just feels wrong for the driver (RC) of one DT node to be acting on
a property of another driver's (EP) node, even though it is a subnode.
There is also the possibility of the EP driver acting upon the
property simultaneously; we don't really have control of what EP
device and drivers are paired with our SOCs.
In addition, this just pushes the binding name issue down a level --
what should these power supplies be called?  They are not slot power
supplies.  Can the  Broadcom STB PCIe RC driver's binding document
specify and define the properties of EP sub-nodes?

Regards,
Jim Quinlan
Broadcom STB

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

[Mark Brown]

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

On Tue, Jan 05, 2021 at 10:09:21AM -0500, Jim Quinlan wrote:
> On Tue, Jan 5, 2021 at 9:01 AM Mark Brown <broonie@kernel.org> wrote:

> > > For us, the supplies are for the EP chip's power.  We have the PCIe
> > > controller turning them "on" for power-on/resume and "off" for
> > > power-off/suspend.  We need the "xxx-supply" property in the
> > > controller's DT node because of the chicken-and-egg situation: if the
> > > property was in the EP's DT node, the RC  will never discover the EP
> > > to see that there is a regulator to turn on.   We would be happy with

> > Why can't the controller look at the nodes describing devices for
> > standard properties?

> It just feels wrong for the driver (RC) of one DT node to be acting on
> a property of another driver's (EP) node, even though it is a subnode.

This is something we do for other buses, for example where there's
device specific tuning that is actually implemented in the controller
hardware.

> There is also the possibility of the EP driver acting upon the
> property simultaneously; we don't really have control of what EP
> device and drivers are paired with our SOCs.

If the device is trying to do something with a supply that's a standard
part of the bus outside of the bus it seems like that's going to lead to
problems no matter what, due to the discovery issues the device must be
coordinating with the bus somehow.

> In addition, this just pushes the binding name issue down a level --
> what should these power supplies be called?  They are not slot power
> supplies.  Can the  Broadcom STB PCIe RC driver's binding document
> specify and define the properties of EP sub-nodes?

I assume the supplies have some name in the PCI specs, whatever names
are used there would probably be appropriate.

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

Re: [PATCH v2 0/6] brcmstb: add EP regulators and panic handler

[Bjorn Helgaas]

On Mon, Nov 30, 2020 at 04:11:37PM -0500, Jim Quinlan wrote:
> v2 -- Use regulator bulk API rather than multiple calls (MarkB).
> 
> v1 -- Bindings are added for fixed regulators that may power the EP device.
>    -- The brcmstb RC driver is modified to control these regulators
>       during probe, suspend, and resume.
>    -- 7216 type SOCs have additional error reporting HW and a
>       panic handler is added to dump its info.
>    -- A missing return value check is added.
> 
> 
> Jim Quinlan (6):
>   dt-bindings: PCI: Add bindings for Brcmstb EP voltage regulators
>   PCI: brcmstb: Add control of EP voltage regulator(s)
>   PCI: brcmstb: Do not turn off regulators if EP can wake up
>   PCI: brcmstb: Give 7216 SOCs their own config type
>   PCI: brcmstb: Add panic/die handler to RC driver
>   PCI: brcmstb: check return value of clk_prepare_enable()

If/when you repost this, capitalize the subjects consistently.

>  .../bindings/pci/brcm,stb-pcie.yaml           |  12 +
>  drivers/pci/controller/pcie-brcmstb.c         | 219 +++++++++++++++++-
>  2 files changed, 228 insertions(+), 3 deletions(-)
> 
> -- 
> 2.17.1
> 

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

[Rob Herring]

On Mon, Jan 4, 2021 at 3:12 PM Jim Quinlan <jim2101024@gmail.com> wrote:
>
> On Wed, Dec 9, 2020 at 10:07 AM Rob Herring <robh@kernel.org> wrote:
> >
> > On Mon, Nov 30, 2020 at 04:11:38PM -0500, Jim Quinlan wrote:
> > > Quite 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.
> > >
> > > Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com>
> > > ---
> > >  .../devicetree/bindings/pci/brcm,stb-pcie.yaml       | 12 ++++++++++++
> > >  1 file changed, 12 insertions(+)
> > >
> > > diff --git a/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml b/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
> > > index 807694b4f41f..baacc3d7ec87 100644
> > > --- a/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
> > > +++ b/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
> > > @@ -85,6 +85,18 @@ properties:
> > >        minItems: 1
> > >        maxItems: 3
> > >
> > > +  vpcie12v-supply:
> > > +    description: 12v regulator phandle for the endpoint device
> > > +
> > > +  vpcie3v3-supply:
> > > +    description: 3.3v regulator phandle for the endpoint device
> >
> > 12V and 3.3V are standard slot supplies, can you add them to
> > pci-bus.yaml. Then some day maybe we can have common slot handling code.
> >
> > With that, here you just need:
> >
> > vpcie3v3-supply: true
>
> Hi Rob,
>
> Sorry for the delay in responding -- I just came back from vacation.

NP, me too.

> The problem we have is that these regulators are not "slot" supplies
> -- our HW does not support PCI slots, so if and when general slot
> power-handling code came along it would probably screw us up.   If you
> don't think there is a problem then I will submit the two supply-names
> you OKed, even though they may not match the voltages we are using for
> the EPs.

Maybe no slots, but you defined the voltages here and they look like
standard voltages. Given this is at least the 2nd usage of these
properties, it seemed like they should be common. Slot or no physical
slot.

> For us, the supplies are for the EP chip's power.  We have the PCIe
> controller turning them "on" for power-on/resume and "off" for
> power-off/suspend.  We need the "xxx-supply" property in the
> controller's DT node because of the chicken-and-egg situation: if the
> property was in the EP's DT node, the RC  will never discover the EP
> to see that there is a regulator to turn on.   We would be happy with
> a single supply name, something like "ep-power".  We would be ecstatic
> to have two (ep0-power, ep1-power).

The chicken-and-egg problem is nothing new. The same thing has come up
for USB, MDIO, MMC/SD to name a few. If devices on a discoverable bus
are not discoverable, then they need to be described in DT. I've given
suggestions many times how to fix the kernel side.

As Mark said, there's no reason you can't look at other nodes for your
data. The data a driver needs isn't always nicely packaged up into a
single node. The DT structure should match the h/w. The EP is a
different device from the PCI host and its supplies belong in its
node.

Not that if we really wanted to have complete slot support, we'd
probably end up having slot nodes in DT. That's generally where we've
ended up at for other cases.

Now there's a second problem here. If this is not standard PCIe rails
which have a defined power sequencing, then you really need to
describe the EP device in DT. Otherwise, we don't know what the power
sequencing is. I will reject any properties such as delays which try
to poorly describe power sequencing in DT.

>
> I'm not sure if you remember but FlorianF talked to you about this
> situation and concluded that something like the above was the way to
> go forward.

Unless it was last week, assume I don't remember.

>  For the latest pullreq I  just copied Rockchip's bindings
> since you reviewed their bindings commit but it looks like you've
> changed your mind.

Well, no. First, it takes more than one to see a pattern. So yes, how
we describe something might evolve. Second, I didn't ask for anything
different from Rockchip here. Just move what Rockchip had to a common
location to reuse. But your reply has convinced me you need an EP
node.

>   Given the constraints I have described, what is
> the best path forward?
>
> Thanks,
> Jim Quinlan
> Broadcom STB
> >
> > > +
> > > +  vpcie1v8-supply:
> > > +    description: 1.8v regulator phandle for the endpoint device
> > > +
> > > +  vpcie0v9-supply:
> > > +    description: 0.9v regulator phandle for the endpoint device
> >
> > These are not standard. They go to a soldered down device or
> > non-standard connector? For the former, the device should really be
> > described in DT and the supplies added there.
> >
> > Mini PCIe connector also has 1.5V supply.
> >
> > Rob