[PATCH v5 0/6] PCI: brcmstb: have host-bridge turn on sub-device power

[PATCH v5 0/6] PCI: brcmstb: have host-bridge turn on sub-device power

[Jim Quinlan]

v5 [NOTE: It has been a while since v4.  Sorry]
     -- See "PCI: allow for callback to prepare nascent subdev"
        commit message for the cornerstone of this patchset
        and the reasons behind it.  This is a new commit.
     -- The RC driver now looks into its DT children and
        turns on regulators for a sub-device, and this occurs
	prior to PCIe link as it must.
     -- Dropped commits not related to the focus of this patchset.

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):
  dt-bindings: PCI: Add bindings for Brcmstb EP voltage regulators
  PCI: allow for callback to prepare nascent subdev
  PCI: brcmstb: split brcm_pcie_setup() into two funcs
  PCI: brcmstb: Add control of subdevice voltage regulators
  PCI: brcmstb: Do not turn off regulators if EP can wake up
  PCI: brcmstb: change brcm_phy_stop() to return void

 .../bindings/pci/brcm,stb-pcie.yaml           |  23 ++
 drivers/base/core.c                           |   5 +
 drivers/pci/controller/pcie-brcmstb.c         | 231 ++++++++++++++++--
 drivers/pci/probe.c                           |  47 +++-
 include/linux/device.h                        |   3 +
 include/linux/pci.h                           |   3 +
 6 files changed, 286 insertions(+), 26 deletions(-)

-- 
2.17.1

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

[Jim Quinlan]

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

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

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

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

diff --git a/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml b/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
index b9589a0daa5c..fec13e4f6eda 100644
--- a/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
+++ b/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
@@ -154,5 +154,28 @@ examples:
                                  <0x42000000 0x1 0x80000000 0x3 0x00000000 0x0 0x80000000>;
                     brcm,enable-ssc;
                     brcm,scb-sizes =  <0x0000000080000000 0x0000000080000000>;
+
+                    /* PCIe bridge */
+                    pci@0,0 {
+                            #address-cells = <3>;
+                            #size-cells = <2>;
+                            reg = <0x0 0x0 0x0 0x0 0x0>;
+                            device_type = "pci";
+                            ranges;
+
+                            /* PCIe endpoint */
+                            pci@0,0 {
+                                    device_type = "pci";
+                                    assigned-addresses = <0x82010000 0x0 0xf8000000 0x6 0x00000000 0x0 0x2000>;
+                                    reg = <0x0 0x0 0x0 0x0 0x0>;
+                                    compatible = "pci14e4,1688";
+                                    vpcie3v3-supply = <&vreg7>;
+
+                                    #address-cells = <3>;
+                                    #size-cells = <2>;
+
+                                    ranges;
+                            };
+                    };
             };
     };
-- 
2.17.1

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

[Mark Brown]

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

On Fri, Oct 22, 2021 at 10:06:54AM -0400, Jim Quinlan wrote:

> 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

This contains updates to add the generic PCIe supply rails, not the
brcm-ep-a and brcm-ep-b supplies (which as I said on the other patch
look like they ought to be renamed).  That's fine since they're
obviously not generic PCIe things but this means that those bindings
need to be added to the device specific bindings here.  Currently
there's only an update to the examples.

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

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

[Jim Quinlan]

On Fri, Oct 22, 2021 at 10:49 AM Mark Brown <broonie@kernel.org> wrote:
>
> On Fri, Oct 22, 2021 at 10:06:54AM -0400, Jim Quinlan wrote:
>
> > 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
>
> This contains updates to add the generic PCIe supply rails, not the
> brcm-ep-a and brcm-ep-b supplies (which as I said on the other patch
> look like they ought to be renamed).  That's fine since they're
> obviously not generic PCIe things but this means that those bindings
> need to be added to the device specific bindings here.  Currently
> there's only an update to the examples.

Just to be clear, and assuming that the brcm-ep-[ab] supply names are
green-lighted by you and Rob, are you saying
I have to update the github site or our YAML file?  If the latter, it
seems odd to be describing
an EP-device property in the YAML for an RC driver since the github
site already describes the EP-device.

Jim

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

[Mark Brown]

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

On Fri, Oct 22, 2021 at 03:24:50PM -0400, Jim Quinlan wrote:

> Just to be clear, and assuming that the brcm-ep-[ab] supply names are
> green-lighted by you and Rob, are you saying
> I have to update the github site or our YAML file?  If the latter, it
> seems odd to be describing
> an EP-device property in the YAML for an RC driver since the github
> site already describes the EP-device.

If you're extending the binding to have additional features beyond what
the generic binding has then I'd expect something in the device specific
binding.  This doesn't seem different to how controllers and devices for
other buses frequently add properties on top of the generic properties
for the bus.

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

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

[Rob Herring]

On Fri, 22 Oct 2021 10:06:54 -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>
> ---
>  .../bindings/pci/brcm,stb-pcie.yaml           | 23 +++++++++++++++++++
>  1 file changed, 23 insertions(+)
> 

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:
./Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml:169:111: [warning] line too long (111 > 110 characters) (line-length)

dtschema/dtc warnings/errors:

doc reference errors (make refcheckdocs):

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

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

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

pip3 install dtschema --upgrade

Please check and re-submit.

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

[Rob Herring]

On Fri, Oct 22, 2021 at 10:06:54AM -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>
> ---
>  .../bindings/pci/brcm,stb-pcie.yaml           | 23 +++++++++++++++++++
>  1 file changed, 23 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml b/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
> index b9589a0daa5c..fec13e4f6eda 100644
> --- a/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
> +++ b/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
> @@ -154,5 +154,28 @@ examples:
>                                   <0x42000000 0x1 0x80000000 0x3 0x00000000 0x0 0x80000000>;
>                      brcm,enable-ssc;
>                      brcm,scb-sizes =  <0x0000000080000000 0x0000000080000000>;
> +
> +                    /* PCIe bridge */

More specifically, the root port.

> +                    pci@0,0 {
> +                            #address-cells = <3>;
> +                            #size-cells = <2>;
> +                            reg = <0x0 0x0 0x0 0x0 0x0>;
> +                            device_type = "pci";
> +                            ranges;
> +
> +                            /* PCIe endpoint */
> +                            pci@0,0 {
> +                                    device_type = "pci";

This means this device is a PCI bridge which wouldn't typically be the 
endpoint. Is that intended? 

> +                                    assigned-addresses = <0x82010000 0x0 0xf8000000 0x6 0x00000000 0x0 0x2000>;
> +                                    reg = <0x0 0x0 0x0 0x0 0x0>;
> +                                    compatible = "pci14e4,1688";
> +                                    vpcie3v3-supply = <&vreg7>;
> +
> +                                    #address-cells = <3>;
> +                                    #size-cells = <2>;
> +
> +                                    ranges;
> +                            };
> +                    };
>              };
>      };
> -- 
> 2.17.1
> 
> 

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

[Jim Quinlan]

On Mon, Oct 25, 2021 at 6:24 PM Rob Herring <robh@kernel.org> wrote:
>
> On Fri, Oct 22, 2021 at 10:06:54AM -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>
> > ---
> >  .../bindings/pci/brcm,stb-pcie.yaml           | 23 +++++++++++++++++++
> >  1 file changed, 23 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml b/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
> > index b9589a0daa5c..fec13e4f6eda 100644
> > --- a/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
> > +++ b/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
> > @@ -154,5 +154,28 @@ examples:
> >                                   <0x42000000 0x1 0x80000000 0x3 0x00000000 0x0 0x80000000>;
> >                      brcm,enable-ssc;
> >                      brcm,scb-sizes =  <0x0000000080000000 0x0000000080000000>;
> > +
> > +                    /* PCIe bridge */
>
> More specifically, the root port.
>
> > +                    pci@0,0 {
> > +                            #address-cells = <3>;
> > +                            #size-cells = <2>;
> > +                            reg = <0x0 0x0 0x0 0x0 0x0>;
> > +                            device_type = "pci";
> > +                            ranges;
> > +
> > +                            /* PCIe endpoint */
> > +                            pci@0,0 {
> > +                                    device_type = "pci";
>
> This means this device is a PCI bridge which wouldn't typically be the
> endpoint. Is that intended?
Hi Rob,

I'm not sure I understand what you are saying --  do you want the
innermost node to be named something like ep-pci@0,0, and its
containing node pci-bridge@0,0?   Or, more likely, I'm missing the
point.  If my DT subtree is this

pcie@8b10000 {
    compatible = "brcm,bcm7278-pcie";
    ....
    pci-bridge@0,0 {
        reg = <0x0 0x0 0x0 0x0 0x0>; /* bus 0 */
        .....
        pci-ep@0,0,0 {
            reg = <0x10000 0x0 0x0 0x0 0x0>;  /* bus 1 */
            vpcie3v3-supply = <&vreg8>;
            ...
        }
    }
}

then the of_nodes appear to align correctly with the devices:

$ cd /sys/devices/platform/
$ cat 8b10000.pcie/of_node/name
pcie
$ cat 8b10000.pcie/pci0000:00/0000:00:00.0/of_node
pci-bridge
$ cat 8b10000.pcie/pci0000:00/0000:00:00.0/0000:01:00.0/of_node/name
pci-ep

and the EP device works of course.  I've even printed out the
device_node structure in the EP driver's probe and it is as expected.
I've noticed that examples such as
"arch/arm64/boot/dts/nvidia/tegra186.dtsi" have the EP node (eg
pci@1,0) directly under the
host bridge DT node (pcie@10003000).  I did try doing that, but the EP
device's probe is given a NUL device_node pointer.

I don't think it matters but our PCIe controllers only have a single root port.

Please advise,
Jim

>
> > +                                    assigned-addresses = <0x82010000 0x0 0xf8000000 0x6 0x00000000 0x0 0x2000>;
> > +                                    reg = <0x0 0x0 0x0 0x0 0x0>;
> > +                                    compatible = "pci14e4,1688";
> > +                                    vpcie3v3-supply = <&vreg7>;
> > +
> > +                                    #address-cells = <3>;
> > +                                    #size-cells = <2>;
> > +
> > +                                    ranges;
> > +                            };
> > +                    };
> >              };
> >      };
> > --
> > 2.17.1
> >
> >

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

[Bjorn Helgaas]

On Tue, Oct 26, 2021 at 05:27:32PM -0400, Jim Quinlan wrote:

> I don't think it matters but our PCIe controllers only have a single
> root port.

Just to kibitz, and I don't know anything about the DT binding under
discussion here, but I would prefer if DTs and drivers did not have
the "single root port" assumption baked deeply in them.

I expect some controllers to support multiple root ports, and it would
be really nice if the DTs and drivers all had similar structures with
the single-root-port controllers just being the N=1 case.

For example, some drivers put their per-root port stuff in
*_add_pcie_port() functions, which I think is a nice way to do it
because it leaves the door open for calling *_add_pcie_port() in a
loop.

Ironically, the only driver I see that looks like it currently
supports multiple root ports is pci-mvebu.c, and it doesn't have an
_add_pcie_port() function.

Having this sort of consistent structure and naming across drivers is
a huge help for ongoing maintenance.

Bjorn

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

[Pali Rohár]

On Wednesday 27 October 2021 11:58:57 Bjorn Helgaas wrote:
> On Tue, Oct 26, 2021 at 05:27:32PM -0400, Jim Quinlan wrote:
> 
> > I don't think it matters but our PCIe controllers only have a single
> > root port.
> 
> Just to kibitz, and I don't know anything about the DT binding under
> discussion here, but I would prefer if DTs and drivers did not have
> the "single root port" assumption baked deeply in them.

+1

Please look also at my proposal for similar/same issue:
https://lore.kernel.org/linux-pci/20211023144252.z7ou2l2tvm6cvtf7@pali/t/#u

> I expect some controllers to support multiple root ports, and it would
> be really nice if the DTs and drivers all had similar structures with
> the single-root-port controllers just being the N=1 case.
> 
> For example, some drivers put their per-root port stuff in
> *_add_pcie_port() functions, which I think is a nice way to do it
> because it leaves the door open for calling *_add_pcie_port() in a
> loop.
> 
> Ironically, the only driver I see that looks like it currently
> supports multiple root ports is pci-mvebu.c, and it doesn't have an
> _add_pcie_port() function.

Ironically, pci-mvebu.c is doing it wrong because HW has basically N
fully independent HW host bridges and pci-mvebu.c driver is registering
one kernel virtual host bridge device and is merging root ports of all
host bridges into this one "virtual" bus which belongs to that kernel
virtual host bridge... Plus root ports are also "virtual" because they
are broken in HW. So correctly pci-mvebu.c should have been split into
separate host bridge DTS nodes, but due to backward compatibility it is
not possible.

> Having this sort of consistent structure and naming across drivers is
> a huge help for ongoing maintenance.
> 
> Bjorn

+1

That is why I sent that my proposal. Defining this as a common way for
(new) drivers would really helps a lot all maintenance.

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

[Rob Herring]

On Tue, Oct 26, 2021 at 4:27 PM Jim Quinlan <james.quinlan@broadcom.com> wrote:
>
> On Mon, Oct 25, 2021 at 6:24 PM Rob Herring <robh@kernel.org> wrote:
> >
> > On Fri, Oct 22, 2021 at 10:06:54AM -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>
> > > ---
> > >  .../bindings/pci/brcm,stb-pcie.yaml           | 23 +++++++++++++++++++
> > >  1 file changed, 23 insertions(+)
> > >
> > > diff --git a/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml b/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
> > > index b9589a0daa5c..fec13e4f6eda 100644
> > > --- a/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
> > > +++ b/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
> > > @@ -154,5 +154,28 @@ examples:
> > >                                   <0x42000000 0x1 0x80000000 0x3 0x00000000 0x0 0x80000000>;
> > >                      brcm,enable-ssc;
> > >                      brcm,scb-sizes =  <0x0000000080000000 0x0000000080000000>;
> > > +
> > > +                    /* PCIe bridge */
> >
> > More specifically, the root port.
> >
> > > +                    pci@0,0 {
> > > +                            #address-cells = <3>;
> > > +                            #size-cells = <2>;
> > > +                            reg = <0x0 0x0 0x0 0x0 0x0>;
> > > +                            device_type = "pci";
> > > +                            ranges;
> > > +
> > > +                            /* PCIe endpoint */
> > > +                            pci@0,0 {
> > > +                                    device_type = "pci";
> >
> > This means this device is a PCI bridge which wouldn't typically be the
> > endpoint. Is that intended?
> Hi Rob,
>
> I'm not sure I understand what you are saying --  do you want the
> innermost node to be named something like ep-pci@0,0, and its
> containing node pci-bridge@0,0?   Or, more likely, I'm missing the
> point.  If my DT subtree is this

I'm confused as to how a bridge is the endpoint. If it is a bridge
(which 'device_type = "pci"' means it is), then there should be
another PCI device under it. That may or may not have a DT node.

> pcie@8b10000 {
>     compatible = "brcm,bcm7278-pcie";
>     ....
>     pci-bridge@0,0 {
>         reg = <0x0 0x0 0x0 0x0 0x0>; /* bus 0 */
>         .....
>         pci-ep@0,0,0 {
>             reg = <0x10000 0x0 0x0 0x0 0x0>;  /* bus 1 */
>             vpcie3v3-supply = <&vreg8>;
>             ...
>         }
>     }
> }
>
> then the of_nodes appear to align correctly with the devices:
>
> $ cd /sys/devices/platform/
> $ cat 8b10000.pcie/of_node/name
> pcie
> $ cat 8b10000.pcie/pci0000:00/0000:00:00.0/of_node
> pci-bridge
> $ cat 8b10000.pcie/pci0000:00/0000:00:00.0/0000:01:00.0/of_node/name
> pci-ep

What does 'lspci -tv' show?

>
> and the EP device works of course.  I've even printed out the
> device_node structure in the EP driver's probe and it is as expected.
> I've noticed that examples such as
> "arch/arm64/boot/dts/nvidia/tegra186.dtsi" have the EP node (eg
> pci@1,0) directly under the
> host bridge DT node (pcie@10003000).  I did try doing that, but the EP
> device's probe is given a NUL device_node pointer.

If you want a complex example I know that's right, then see hikey970.

Rob

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

[Jim Quinlan]

On Wed, Oct 27, 2021 at 4:54 PM Rob Herring <robh@kernel.org> wrote:
>
> On Tue, Oct 26, 2021 at 4:27 PM Jim Quinlan <james.quinlan@broadcom.com> wrote:
> >
> > On Mon, Oct 25, 2021 at 6:24 PM Rob Herring <robh@kernel.org> wrote:
> > >
> > > On Fri, Oct 22, 2021 at 10:06:54AM -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>
> > > > ---
> > > >  .../bindings/pci/brcm,stb-pcie.yaml           | 23 +++++++++++++++++++
> > > >  1 file changed, 23 insertions(+)
> > > >
> > > > diff --git a/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml b/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
> > > > index b9589a0daa5c..fec13e4f6eda 100644
> > > > --- a/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
> > > > +++ b/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
> > > > @@ -154,5 +154,28 @@ examples:
> > > >                                   <0x42000000 0x1 0x80000000 0x3 0x00000000 0x0 0x80000000>;
> > > >                      brcm,enable-ssc;
> > > >                      brcm,scb-sizes =  <0x0000000080000000 0x0000000080000000>;
> > > > +
> > > > +                    /* PCIe bridge */
> > >
> > > More specifically, the root port.
> > >
> > > > +                    pci@0,0 {
> > > > +                            #address-cells = <3>;
> > > > +                            #size-cells = <2>;
> > > > +                            reg = <0x0 0x0 0x0 0x0 0x0>;
> > > > +                            device_type = "pci";
> > > > +                            ranges;
> > > > +
> > > > +                            /* PCIe endpoint */
> > > > +                            pci@0,0 {
> > > > +                                    device_type = "pci";
> > >
> > > This means this device is a PCI bridge which wouldn't typically be the
> > > endpoint. Is that intended?
> > Hi Rob,
> >
> > I'm not sure I understand what you are saying --  do you want the
> > innermost node to be named something like ep-pci@0,0, and its
> > containing node pci-bridge@0,0?   Or, more likely, I'm missing the
> > point.  If my DT subtree is this
>
> I'm confused as to how a bridge is the endpoint. If it is a bridge
> (which 'device_type = "pci"' means it is), then there should be
> another PCI device under it. That may or may not have a DT node.

I did not know that device_type="pci" implies that it must be a
bridge;  [1] says "device_type" is deprecated  for PCI and [2] defers
to Open Firmware EEE 1275, which is not free AFAICT. Do you have
better URLs that describe this?  At any rate, I will remove the
device_type="pci" from the innermost DT node, and resubmit.

>
> > pcie@8b10000 {
> >     compatible = "brcm,bcm7278-pcie";
> >     ....
> >     pci-bridge@0,0 {
> >         reg = <0x0 0x0 0x0 0x0 0x0>; /* bus 0 */
> >         .....
> >         pci-ep@0,0,0 {
> >             reg = <0x10000 0x0 0x0 0x0 0x0>;  /* bus 1 */
> >             vpcie3v3-supply = <&vreg8>;
> >             ...
> >         }
> >     }
> > }
> >
> > then the of_nodes appear to align correctly with the devices:
> >
> > $ cd /sys/devices/platform/
> > $ cat 8b10000.pcie/of_node/name
> > pcie
> > $ cat 8b10000.pcie/pci0000:00/0000:00:00.0/of_node
> > pci-bridge
> > $ cat 8b10000.pcie/pci0000:00/0000:00:00.0/0000:01:00.0/of_node/name
> > pci-ep
>
> What does 'lspci -tv' show?

$ lspci -tv
-+-[0000:01]---00.0  Intel Corporation Wireless 7260
  \-[0000:00]---00.0  Broadcom Inc. and subsidiaries Device 7278


>
> >
> > and the EP device works of course.  I've even printed out the
> > device_node structure in the EP driver's probe and it is as expected.
> > I've noticed that examples such as
> > "arch/arm64/boot/dts/nvidia/tegra186.dtsi" have the EP node (eg
> > pci@1,0) directly under the
> > host bridge DT node (pcie@10003000).  I did try doing that, but the EP
> > device's probe is given a NUL device_node pointer.
>
> If you want a complex example I know that's right, then see hikey970.

Thanks, will look.

Jim

[1] https://buildmedia.readthedocs.org/media/pdf/devicetree-specification/latest/devicetree-specification.pdf
[2] https://www.openfirmware.info/data/docs/bus.pci.pdf
>
> Rob

[PATCH v5 2/6] PCI: allow for callback to prepare nascent subdev

[Jim Quinlan]

We would like the Broadcom STB PCIe root complex driver to be able to turn
off/on regulators[1] that provide power to endpoint[2] devices.  Typically,
the drivers of these endpoint devices are stock Linux drivers that are not
aware that these regulator(s) exist and must be turned on for the driver to
be probed.  The simple solution of course is to turn these regulators on at
boot and keep them on.  However, this solution does not satisfy at least
three of our usage modes:

1. For example, one customer uses multiple PCIe controllers, but wants the
ability to, by script, turn any or all of them by and their subdevices off
to save power, e.g. when in battery mode.

2. Another example is when a watchdog script discovers that an endpoint
device is in an unresponsive state and would like to unbind, power toggle,
and re-bind just the PCIe endpoint and controller.

3. Of course we also want power turned off during suspend mode.  However,
some endpoint devices may be able to "wake" during suspend and we need to
recognise this case and veto the nominal act of turning off its regulator.
Such is the case with Wake-on-LAN and Wake-on-WLAN support where PCIe
end-point device needs to be kept powered on in order to receive network
packets and wake-up the system.

In all of these cases it is advantageous for the PCIe controller to govern
the turning off/on the regulators needed by the endpoint device.  The first
two cases can be done by simply unbinding and binding the PCIe controller,
if the controller has control of these regulators.

This commit solves the "chicken-and-egg" problem by providing a callback to
the RC driver when a downstream device is "discovered" by inspecting its
DT, and allowing said driver to allocate the device object prematurely in
order to get the regulator(s) and turn them on before the device's ID is
read.

[1] These regulators typically govern the actual power supply to the
    endpoint chip.  Sometimes they may be a the official PCIe socket
    power -- such as 3.3v or aux-3.3v.  Sometimes they are truly
    the regulator(s) that supply power to the EP chip.

[2] The 99% configuration of our boards is a single endpoint device
    attached to the PCIe controller.  I use the term endpoint but it could
    possible mean a switch as well.

Signed-off-by: Jim Quinlan <jim2101024@gmail.com>
---
 drivers/base/core.c    |  5 +++++
 drivers/pci/probe.c    | 47 ++++++++++++++++++++++++++++++++----------
 include/linux/device.h |  3 +++
 include/linux/pci.h    |  3 +++
 4 files changed, 47 insertions(+), 11 deletions(-)

diff --git a/drivers/base/core.c b/drivers/base/core.c
index 249da496581a..62d9ac123ae5 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -2864,6 +2864,10 @@ static void klist_children_put(struct klist_node *n)
  */
 void device_initialize(struct device *dev)
 {
+	/* Return if this has been called already. */
+	if (dev->device_initialized)
+		return;
+
 	dev->kobj.kset = devices_kset;
 	kobject_init(&dev->kobj, &device_ktype);
 	INIT_LIST_HEAD(&dev->dma_pools);
@@ -2892,6 +2896,7 @@ void device_initialize(struct device *dev)
 #ifdef CONFIG_SWIOTLB
 	dev->dma_io_tlb_mem = &io_tlb_default_mem;
 #endif
+	dev->device_initialized = true;
 }
 EXPORT_SYMBOL_GPL(device_initialize);
 
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index d9fc02a71baa..12947e972b7b 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -2372,27 +2372,52 @@ EXPORT_SYMBOL(pci_bus_read_dev_vendor_id);
  */
 static struct pci_dev *pci_scan_device(struct pci_bus *bus, int devfn)
 {
-	struct pci_dev *dev;
+	struct pci_host_bridge *hb = pci_find_host_bridge(bus);
+	struct pci_dev *dev = NULL;
 	u32 l;
 
-	if (!pci_bus_read_dev_vendor_id(bus, devfn, &l, 60*1000))
-		return NULL;
+	/*
+	 * If the host bridge has a pci_subdev_prepare() function, first
+	 * call it with true as the first argument to see if it "cares"
+	 * about this device.  A non-zero return value indicates it cares,
+	 * so in that case partially allocate some of the device and call
+	 * pci_subdev_prepare() again, with false as the first argument.
+	 * This tells it to allow the host bridge driver to pre-allocate
+	 * some resources such as voltage regulators.
+	 */
+	if (hb->pci_subdev_prepare
+	    && hb->pci_subdev_prepare(true, bus, devfn, NULL, NULL)) {
+		dev = pci_alloc_dev(bus);
+		if (!dev)
+			return NULL;
 
-	dev = pci_alloc_dev(bus);
-	if (!dev)
-		return NULL;
+		dev->devfn = devfn;
+		device_initialize(&dev->dev);
 
+		/* Call again, this time for actual prep work */
+		if (hb->pci_subdev_prepare(false, bus, devfn, hb, dev)
+		    || !pci_bus_read_dev_vendor_id(bus, devfn, &l, 60*1000))
+			goto err_out;
+	} else {
+		if (!pci_bus_read_dev_vendor_id(bus, devfn, &l, 60*1000))
+			return NULL;
+		dev = pci_alloc_dev(bus);
+		if (!dev)
+			return NULL;
+	}
 	dev->devfn = devfn;
 	dev->vendor = l & 0xffff;
 	dev->device = (l >> 16) & 0xffff;
 
-	if (pci_setup_device(dev)) {
-		pci_bus_put(dev->bus);
-		kfree(dev);
-		return NULL;
-	}
+	if (pci_setup_device(dev))
+		goto err_out;
 
 	return dev;
+
+err_out:
+	pci_bus_put(dev->bus);
+	kfree(dev);
+	return NULL;
 }
 
 void pcie_report_downtraining(struct pci_dev *dev)
diff --git a/include/linux/device.h b/include/linux/device.h
index e270cb740b9e..cf175684a270 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -461,6 +461,8 @@ struct dev_links_info {
  *		and optionall (if the coherent mask is large enough) also
  *		for dma allocations.  This flag is managed by the dma ops
  *		instance from ->dma_supported.
+ * @device_initialized: true if device_initialize(dev) has already been
+ *		invoked, false otherwise.
  *
  * At the lowest level, every device in a Linux system is represented by an
  * instance of struct device. The device structure contains the information
@@ -575,6 +577,7 @@ struct device {
 #ifdef CONFIG_DMA_OPS_BYPASS
 	bool			dma_ops_bypass : 1;
 #endif
+	bool			device_initialized : 1;
 };
 
 /**
diff --git a/include/linux/pci.h b/include/linux/pci.h
index cd8aa6fce204..7a72b3af1e33 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -552,6 +552,9 @@ struct pci_host_bridge {
 	u8 (*swizzle_irq)(struct pci_dev *, u8 *); /* Platform IRQ swizzler */
 	int (*map_irq)(const struct pci_dev *, u8, u8);
 	void (*release_fn)(struct pci_host_bridge *);
+	int (*pci_subdev_prepare)(bool query, struct pci_bus *bus, int devfn,
+				  struct pci_host_bridge *hb,
+				  struct pci_dev *pdev);
 	void		*release_data;
 	unsigned int	ignore_reset_delay:1;	/* For entire hierarchy */
 	unsigned int	no_ext_tags:1;		/* No Extended Tags */
-- 
2.17.1

Re: [PATCH v5 2/6] PCI: allow for callback to prepare nascent subdev

[Greg Kroah-Hartman]

On Fri, Oct 22, 2021 at 10:06:55AM -0400, Jim Quinlan wrote:
> We would like the Broadcom STB PCIe root complex driver to be able to turn
> off/on regulators[1] that provide power to endpoint[2] devices.  Typically,
> the drivers of these endpoint devices are stock Linux drivers that are not
> aware that these regulator(s) exist and must be turned on for the driver to
> be probed.  The simple solution of course is to turn these regulators on at
> boot and keep them on.  However, this solution does not satisfy at least
> three of our usage modes:
> 
> 1. For example, one customer uses multiple PCIe controllers, but wants the
> ability to, by script, turn any or all of them by and their subdevices off
> to save power, e.g. when in battery mode.
> 
> 2. Another example is when a watchdog script discovers that an endpoint
> device is in an unresponsive state and would like to unbind, power toggle,
> and re-bind just the PCIe endpoint and controller.
> 
> 3. Of course we also want power turned off during suspend mode.  However,
> some endpoint devices may be able to "wake" during suspend and we need to
> recognise this case and veto the nominal act of turning off its regulator.
> Such is the case with Wake-on-LAN and Wake-on-WLAN support where PCIe
> end-point device needs to be kept powered on in order to receive network
> packets and wake-up the system.
> 
> In all of these cases it is advantageous for the PCIe controller to govern
> the turning off/on the regulators needed by the endpoint device.  The first
> two cases can be done by simply unbinding and binding the PCIe controller,
> if the controller has control of these regulators.
> 
> This commit solves the "chicken-and-egg" problem by providing a callback to
> the RC driver when a downstream device is "discovered" by inspecting its
> DT, and allowing said driver to allocate the device object prematurely in
> order to get the regulator(s) and turn them on before the device's ID is
> read.
> 
> [1] These regulators typically govern the actual power supply to the
>     endpoint chip.  Sometimes they may be a the official PCIe socket
>     power -- such as 3.3v or aux-3.3v.  Sometimes they are truly
>     the regulator(s) that supply power to the EP chip.
> 
> [2] The 99% configuration of our boards is a single endpoint device
>     attached to the PCIe controller.  I use the term endpoint but it could
>     possible mean a switch as well.
> 
> Signed-off-by: Jim Quinlan <jim2101024@gmail.com>
> ---
>  drivers/base/core.c    |  5 +++++
>  drivers/pci/probe.c    | 47 ++++++++++++++++++++++++++++++++----------
>  include/linux/device.h |  3 +++
>  include/linux/pci.h    |  3 +++
>  4 files changed, 47 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/base/core.c b/drivers/base/core.c
> index 249da496581a..62d9ac123ae5 100644
> --- a/drivers/base/core.c
> +++ b/drivers/base/core.c
> @@ -2864,6 +2864,10 @@ static void klist_children_put(struct klist_node *n)
>   */
>  void device_initialize(struct device *dev)
>  {
> +	/* Return if this has been called already. */
> +	if (dev->device_initialized)
> +		return;
> +

Ick, no!  Who would ever be calling this function more than once?  That
"should" be impossible.

This function should only be called by a bus when it first creates the
structure and before anything is done with it.  As the bus itself
controls the creation, it already "knows" when the structure was
initialzed so it should not have to be called again.

Please fix the bus logic that requires this, it is broken.

Consider this a NACK for this patch, sorry.

greg k-h

Re: [PATCH v5 2/6] PCI: allow for callback to prepare nascent subdev

[Jim Quinlan]

On Fri, Oct 22, 2021 at 10:34 AM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> On Fri, Oct 22, 2021 at 10:06:55AM -0400, Jim Quinlan wrote:
> > We would like the Broadcom STB PCIe root complex driver to be able to turn
> > off/on regulators[1] that provide power to endpoint[2] devices.  Typically,
> > the drivers of these endpoint devices are stock Linux drivers that are not
> > aware that these regulator(s) exist and must be turned on for the driver to
> > be probed.  The simple solution of course is to turn these regulators on at
> > boot and keep them on.  However, this solution does not satisfy at least
> > three of our usage modes:
> >
> > 1. For example, one customer uses multiple PCIe controllers, but wants the
> > ability to, by script, turn any or all of them by and their subdevices off
> > to save power, e.g. when in battery mode.
> >
> > 2. Another example is when a watchdog script discovers that an endpoint
> > device is in an unresponsive state and would like to unbind, power toggle,
> > and re-bind just the PCIe endpoint and controller.
> >
> > 3. Of course we also want power turned off during suspend mode.  However,
> > some endpoint devices may be able to "wake" during suspend and we need to
> > recognise this case and veto the nominal act of turning off its regulator.
> > Such is the case with Wake-on-LAN and Wake-on-WLAN support where PCIe
> > end-point device needs to be kept powered on in order to receive network
> > packets and wake-up the system.
> >
> > In all of these cases it is advantageous for the PCIe controller to govern
> > the turning off/on the regulators needed by the endpoint device.  The first
> > two cases can be done by simply unbinding and binding the PCIe controller,
> > if the controller has control of these regulators.
> >
> > This commit solves the "chicken-and-egg" problem by providing a callback to
> > the RC driver when a downstream device is "discovered" by inspecting its
> > DT, and allowing said driver to allocate the device object prematurely in
> > order to get the regulator(s) and turn them on before the device's ID is
> > read.
> >
> > [1] These regulators typically govern the actual power supply to the
> >     endpoint chip.  Sometimes they may be a the official PCIe socket
> >     power -- such as 3.3v or aux-3.3v.  Sometimes they are truly
> >     the regulator(s) that supply power to the EP chip.
> >
> > [2] The 99% configuration of our boards is a single endpoint device
> >     attached to the PCIe controller.  I use the term endpoint but it could
> >     possible mean a switch as well.
> >
> > Signed-off-by: Jim Quinlan <jim2101024@gmail.com>
> > ---
> >  drivers/base/core.c    |  5 +++++
> >  drivers/pci/probe.c    | 47 ++++++++++++++++++++++++++++++++----------
> >  include/linux/device.h |  3 +++
> >  include/linux/pci.h    |  3 +++
> >  4 files changed, 47 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/base/core.c b/drivers/base/core.c
> > index 249da496581a..62d9ac123ae5 100644
> > --- a/drivers/base/core.c
> > +++ b/drivers/base/core.c
> > @@ -2864,6 +2864,10 @@ static void klist_children_put(struct klist_node *n)
> >   */
> >  void device_initialize(struct device *dev)
> >  {
> > +     /* Return if this has been called already. */
> > +     if (dev->device_initialized)
> > +             return;
> > +
>
> Ick, no!  Who would ever be calling this function more than once?  That
> "should" be impossible.
>
> This function should only be called by a bus when it first creates the
> structure and before anything is done with it.  As the bus itself
> controls the creation, it already "knows" when the structure was
> initialzed so it should not have to be called again.



>
> Please fix the bus logic that requires this, it is broken.
Got it, thanks for the prompt reply.

JimQ
>
> Consider this a NACK for this patch, sorry.
>
> greg k-h

[PATCH v5 3/6] PCI: brcmstb: split brcm_pcie_setup() into two funcs

[Jim Quinlan]

We need to split a function in two so that the driver can take advantage of
the recently added function pci_subdev_prepare() which is a member
of struct pci_host_bridge.

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

diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
index cc30215f5a43..ba4d6daf312c 100644
--- a/drivers/pci/controller/pcie-brcmstb.c
+++ b/drivers/pci/controller/pcie-brcmstb.c
@@ -863,17 +863,10 @@ static inline int brcm_pcie_get_rc_bar2_size_and_offset(struct brcm_pcie *pcie,
 
 static int brcm_pcie_setup(struct brcm_pcie *pcie)
 {
-	struct pci_host_bridge *bridge = pci_host_bridge_from_priv(pcie);
 	u64 rc_bar2_offset, rc_bar2_size;
 	void __iomem *base = pcie->base;
-	struct device *dev = pcie->dev;
-	struct resource_entry *entry;
-	bool ssc_good = false;
-	struct resource *res;
-	int num_out_wins = 0;
-	u16 nlw, cls, lnksta;
-	int i, ret, memc;
-	u32 tmp, burst, aspm_support;
+	int ret, memc;
+	u32 tmp, burst;
 
 	/* Reset the bridge */
 	pcie->bridge_sw_init_set(pcie, 1);
@@ -956,6 +949,21 @@ static int brcm_pcie_setup(struct brcm_pcie *pcie)
 
 	if (pcie->gen)
 		brcm_pcie_set_gen(pcie, pcie->gen);
+	return 0;
+}
+
+static int brcm_pcie_linkup(struct brcm_pcie *pcie)
+{
+	struct pci_host_bridge *bridge = pci_host_bridge_from_priv(pcie);
+	struct device *dev = pcie->dev;
+	void __iomem *base = pcie->base;
+	struct resource_entry *entry;
+	struct resource *res;
+	int num_out_wins = 0;
+	u16 nlw, cls, lnksta;
+	bool ssc_good = false;
+	u32 aspm_support, tmp;
+	int ret, i;
 
 	/* Unassert the fundamental reset */
 	pcie->perst_set(pcie, 0);
@@ -1186,6 +1194,10 @@ static int brcm_pcie_resume(struct device *dev)
 	if (ret)
 		goto err_reset;
 
+	ret = brcm_pcie_linkup(pcie);
+	if (ret)
+		goto err_reset;
+
 	if (pcie->msi)
 		brcm_msi_set_regs(pcie->msi);
 
-- 
2.17.1

[PATCH v5 4/6] PCI: brcmstb: Add control of subdevice voltage regulators

[Jim Quinlan]

This Broadcom STB PCIe RC driver 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 | 148 +++++++++++++++++++++++++-
 1 file changed, 146 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
index ba4d6daf312c..35924af1c3aa 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>
@@ -192,6 +193,13 @@ 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[] = {
+	"vpcie3v3-supply",
+	"vpcie3v3aux-supply",
+	"brcm-ep-a-supply",
+	"brcm-ep-b-supply",
+};
+
 enum {
 	RGR1_SW_INIT_1,
 	EXT_CFG_INDEX,
@@ -295,8 +303,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[ARRAY_SIZE(supplies)];
+	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
@@ -1148,6 +1175,55 @@ 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 pci_dev *dev)
+{
+	const unsigned int ns = ARRAY_SIZE(supplies);
+	struct device_node *dn;
+	struct property *pp;
+	unsigned int i;
+	int ret;
+
+	/* This is for Broadcom STB/CM chips only */
+	if (pcie->type == BCM2711)
+		return 0;
+
+	pci_set_of_node(dev);
+	dn = dev->dev.of_node;
+	if (!dn)
+		return 0;
+
+	for_each_property_of_node(dn, pp) {
+		for (i = 0; i < ns; i++)
+			if (strcmp(supplies[i], pp->name) == 0)
+				break;
+		if (i >= ns || pcie->num_supplies >= ARRAY_SIZE(supplies))
+			continue;
+
+		pcie->supplies[pcie->num_supplies++].supply = supplies[i];
+	}
+
+	if (pcie->num_supplies == 0)
+		return 0;
+
+	/*
+	 * We set the name ahead of time as the regulator core expects
+	 * it to exist when regulator_bulk_get() is called.
+	 */
+	dev_set_name(&dev->dev, "%04x:%02x:%02x.%d",
+		     pci_domain_nr(dev->bus),
+		     dev->bus->number, PCI_SLOT(dev->devfn),
+		     PCI_FUNC(dev->devfn));
+	/*
+	 * We cannot use devm_regulator_bulk_get() because the
+	 * downstream device may be removed w/o the regulator
+	 * first being disabled by the host bridge.
+	 */
+	ret = regulator_bulk_get(&dev->dev, pcie->num_supplies,
+				 pcie->supplies);
+
+	return ret;
+}
+
 static int brcm_pcie_suspend(struct device *dev)
 {
 	struct brcm_pcie *pcie = dev_get_drvdata(dev);
@@ -1158,7 +1234,7 @@ static int brcm_pcie_suspend(struct device *dev)
 	reset_control_rearm(pcie->rescal);
 	clk_disable_unprepare(pcie->clk);
 
-	return ret;
+	return brcm_set_regulators(pcie, false);
 }
 
 static int brcm_pcie_resume(struct device *dev)
@@ -1174,6 +1250,9 @@ static int brcm_pcie_resume(struct device *dev)
 	ret = reset_control_reset(pcie->rescal);
 	if (ret)
 		goto err_disable_clk;
+	ret = brcm_set_regulators(pcie, true);
+	if (ret)
+		goto err_reset;
 
 	ret = brcm_phy_start(pcie);
 	if (ret)
@@ -1217,6 +1296,10 @@ static void __brcm_pcie_remove(struct brcm_pcie *pcie)
 	brcm_phy_stop(pcie);
 	reset_control_rearm(pcie->rescal);
 	clk_disable_unprepare(pcie->clk);
+	if (pcie->num_supplies) {
+		(void)brcm_set_regulators(pcie, false);
+		regulator_bulk_free(pcie->num_supplies, pcie->supplies);
+	}
 }
 
 static int brcm_pcie_remove(struct platform_device *pdev)
@@ -1241,6 +1324,56 @@ static const struct of_device_id brcm_pcie_match[] = {
 	{},
 };
 
+static int brcm_pcie_pci_subdev_prepare(bool query, struct pci_bus *bus, int devfn,
+					struct pci_host_bridge *bridge,
+					struct pci_dev *pdev)
+{
+	struct brcm_pcie *pcie;
+	int ret = 0;
+
+	/*
+	 * We only care about a device that is directly connected
+	 * to the root complex, ie bus == 1 and slot == 0.
+	 */
+	if (query)
+		return (bus->number == 1 && PCI_SLOT(devfn) == 0);
+
+
+	pcie = (struct brcm_pcie *) bridge->sysdata;
+	ret = brcm_pcie_get_regulators(pcie, pdev);
+	if (ret) {
+		pcie->num_supplies = 0;
+		if (ret != -EPROBE_DEFER)
+			dev_err(pcie->dev, "failed to get regulators (err=%d)\n", ret);
+		return ret;
+	}
+
+	ret = brcm_set_regulators(pcie, true);
+	if (ret)
+		goto err_out0;
+
+	ret = brcm_pcie_linkup(pcie);
+	if (ret)
+		goto err_out1;
+
+	/*
+	 * dev_set_name() was called in brcm_set_regulators().  Free the
+	 * string it allocated as it will be called again when
+	 * pci_setup_device() is invoked.
+	 */
+	kfree_const(pdev->dev.kobj.name);
+	pdev->dev.kobj.name = NULL;
+
+	return 0;
+
+err_out1:
+	brcm_set_regulators(pcie, false);
+err_out0:
+	regulator_bulk_free(pcie->num_supplies, pcie->supplies);
+	pcie->num_supplies = 0;
+	return ret;
+}
+
 static int brcm_pcie_probe(struct platform_device *pdev)
 {
 	struct device_node *np = pdev->dev.of_node, *msi_np;
@@ -1327,12 +1460,23 @@ static int brcm_pcie_probe(struct platform_device *pdev)
 		}
 	}
 
+	bridge->pci_subdev_prepare = brcm_pcie_pci_subdev_prepare;
 	bridge->ops = &brcm_pcie_ops;
 	bridge->sysdata = pcie;
 
 	platform_set_drvdata(pdev, pcie);
 
-	return pci_host_probe(bridge);
+	ret = pci_host_probe(bridge);
+	if (ret)
+		goto fail;
+
+	if (!brcm_pcie_link_up(pcie)) {
+		brcm_pcie_remove(pdev);
+		return -ENODEV;
+	}
+
+	return 0;
+
 fail:
 	__brcm_pcie_remove(pcie);
 	return ret;
-- 
2.17.1

Re: [PATCH v5 4/6] PCI: brcmstb: Add control of subdevice voltage regulators

[Mark Brown]

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

On Fri, Oct 22, 2021 at 10:06:57AM -0400, Jim Quinlan wrote:

> +static const char * const supplies[] = {
> +	"vpcie3v3-supply",
> +	"vpcie3v3aux-supply",
> +	"brcm-ep-a-supply",
> +	"brcm-ep-b-supply",
> +};

Why are you including "-supply" in the names here?  That will lead to
a double -supply when we look in the DT which probably isn't what you're
looking for.

Also are you *sure* that the device has supplies with names like
"brcm-ep-a"?  That seems rather unidiomatic for electrical engineering,
the names here are supposed to correspond to the names used in the
datasheet for the part.

> +	/* This is for Broadcom STB/CM chips only */
> +	if (pcie->type == BCM2711)
> +		return 0;

It is a relief that other chips have managed to work out how to avoid
requiring power.

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

Re: [PATCH v5 4/6] PCI: brcmstb: Add control of subdevice voltage regulators

[Jim Quinlan]

On Fri, Oct 22, 2021 at 10:31 AM Mark Brown <broonie@kernel.org> wrote:
>
> On Fri, Oct 22, 2021 at 10:06:57AM -0400, Jim Quinlan wrote:
>
> > +static const char * const supplies[] = {
> > +     "vpcie3v3-supply",
> > +     "vpcie3v3aux-supply",
> > +     "brcm-ep-a-supply",
> > +     "brcm-ep-b-supply",
> > +};
>
> Why are you including "-supply" in the names here?  That will lead to
> a double -supply when we look in the DT which probably isn't what you're
> looking for.
I'm not sure how this got past testing; will fix.

>
> Also are you *sure* that the device has supplies with names like
> "brcm-ep-a"?  That seems rather unidiomatic for electrical engineering,
> the names here are supposed to correspond to the names used in the
> datasheet for the part.
I try to explain this in the commit message of"PCI: allow for callback
to prepare nascent subdev".  Wrt to the names,

       "These regulators typically govern the actual power supply to the
        endpoint chip.  Sometimes they may be a the official PCIe socket
        power -- such as 3.3v or aux-3.3v.  Sometimes they are truly
        the regulator(s) that supply power to the EP chip."

Each different SOC./board we deal with may present different ways of
making the EP device power on.  We are using
an abstraction name "brcm-ep-a"  to represent some required regulator
to make the EP  work for a specific board.  The RC
driver cannot hard code a descriptive name as it must work for all
boards designed by us, others, and third parties.
The EP driver also doesn't know  or care about the regulator name, and
this driver is often closed source and often immutable.  The EP
device itself may come from Brcm, a third party,  or sometimes a competitor.

Basically, we find using a generic name such as "brcm-ep-a-supply"
quite handy and many of our customers embrace this feature.
I know that Rob was initially against such a generic name, but I
vaguely remember him seeing some merit to this, perhaps a tiny bit :-)
Or my memory is shot, which could very well be the case.

>
> > +     /* This is for Broadcom STB/CM chips only */
> > +     if (pcie->type == BCM2711)
> > +             return 0;
>
> It is a relief that other chips have managed to work out how to avoid
> requiring power.
I'm not sure that the other Broadcom groups have our customers, our
customers' requirements, and the amount and variation of boards that
run our PCIe driver on the SOC.

Jim

Re: [PATCH v5 4/6] PCI: brcmstb: Add control of subdevice voltage regulators

[Mark Brown]

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

On Fri, Oct 22, 2021 at 03:15:59PM -0400, Jim Quinlan wrote:

> Each different SOC./board we deal with may present different ways of
> making the EP device power on.  We are using
> an abstraction name "brcm-ep-a"  to represent some required regulator
> to make the EP  work for a specific board.  The RC
> driver cannot hard code a descriptive name as it must work for all
> boards designed by us, others, and third parties.
> The EP driver also doesn't know  or care about the regulator name, and
> this driver is often closed source and often immutable.  The EP
> device itself may come from Brcm, a third party,  or sometimes a competitor.

> Basically, we find using a generic name such as "brcm-ep-a-supply"
> quite handy and many of our customers embrace this feature.
> I know that Rob was initially against such a generic name, but I
> vaguely remember him seeing some merit to this, perhaps a tiny bit :-)
> Or my memory is shot, which could very well be the case.

That sounds like it just shouldn't be a regulator at all, perhaps the
board happens to need a regulator there but perhaps it needs a clock,
GPIO or some specific sequence of actions.  It sounds like you need some
sort of quirking mechanism to cope with individual boards with board
specific bindings.

I'd suggest as a first pass omitting this and then looking at some
actual systems later when working out how to support them, no sense in
getting the main thing held up by difficult edge cases.

> > > +     /* This is for Broadcom STB/CM chips only */
> > > +     if (pcie->type == BCM2711)
> > > +             return 0;

> > It is a relief that other chips have managed to work out how to avoid
> > requiring power.

> I'm not sure that the other Broadcom groups have our customers, our
> customers' requirements, and the amount and variation of boards that
> run our PCIe driver on the SOC.

Sure, but equally they might (even if they didn't spot it yet) and in
general it's safer to err on the side of describing the hardware so we
can use that information later.

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

Re: [PATCH v5 4/6] PCI: brcmstb: Add control of subdevice voltage regulators

[Jim Quinlan]

On Fri, Oct 22, 2021 at 3:47 PM Mark Brown <broonie@kernel.org> wrote:
>
> On Fri, Oct 22, 2021 at 03:15:59PM -0400, Jim Quinlan wrote:
>
> > Each different SOC./board we deal with may present different ways of
> > making the EP device power on.  We are using
> > an abstraction name "brcm-ep-a"  to represent some required regulator
> > to make the EP  work for a specific board.  The RC
> > driver cannot hard code a descriptive name as it must work for all
> > boards designed by us, others, and third parties.
> > The EP driver also doesn't know  or care about the regulator name, and
> > this driver is often closed source and often immutable.  The EP
> > device itself may come from Brcm, a third party,  or sometimes a competitor.
>
> > Basically, we find using a generic name such as "brcm-ep-a-supply"
> > quite handy and many of our customers embrace this feature.
> > I know that Rob was initially against such a generic name, but I
> > vaguely remember him seeing some merit to this, perhaps a tiny bit :-)
> > Or my memory is shot, which could very well be the case.
>
> That sounds like it just shouldn't be a regulator at all, perhaps the
> board happens to need a regulator there but perhaps it needs a clock,
> GPIO or some specific sequence of actions.  It sounds like you need some
> sort of quirking mechanism to cope with individual boards with board
> specific bindings.
The boards involved may have no PCIe sockets, or run the gamut of the different
PCIe sockets.  They all offer gpio(s) to turn off/on their power supply(s) to
make their PCIe device endpoint functional.  It is not viable to add
new Linux quirk or DT
code for each board.  First is the volume and variety of the boards
that use our SOCs.. Second, is
our lack of information/control:  often, the board is designed by one
company (not us), and
given to another company as the middleman, and then they want the
features outlined
in my aforementioned commit message.

>
> I'd suggest as a first pass omitting this and then looking at some
> actual systems later when working out how to support them, no sense in
> getting the main thing held up by difficult edge cases.

These are not edge cases -- some of these are major customers.

Regards,
Jim

>
> > > > +     /* This is for Broadcom STB/CM chips only */
> > > > +     if (pcie->type == BCM2711)
> > > > +             return 0;
>
> > > It is a relief that other chips have managed to work out how to avoid
> > > requiring power.
>
> > I'm not sure that the other Broadcom groups have our customers, our
> > customers' requirements, and the amount and variation of boards that
> > run our PCIe driver on the SOC.
>
> Sure, but equally they might (even if they didn't spot it yet) and in
> general it's safer to err on the side of describing the hardware so we
> can use that information later.

Re: [PATCH v5 4/6] PCI: brcmstb: Add control of subdevice voltage regulators

[Mark Brown]

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

On Mon, Oct 25, 2021 at 09:50:09AM -0400, Jim Quinlan wrote:
> On Fri, Oct 22, 2021 at 3:47 PM Mark Brown <broonie@kernel.org> wrote:
> > On Fri, Oct 22, 2021 at 03:15:59PM -0400, Jim Quinlan wrote:

> > That sounds like it just shouldn't be a regulator at all, perhaps the
> > board happens to need a regulator there but perhaps it needs a clock,
> > GPIO or some specific sequence of actions.  It sounds like you need some
> > sort of quirking mechanism to cope with individual boards with board
> > specific bindings.

> The boards involved may have no PCIe sockets, or run the gamut of the different
> PCIe sockets.  They all offer gpio(s) to turn off/on their power supply(s) to
> make their PCIe device endpoint functional.  It is not viable to add
> new Linux quirk or DT
> code for each board.  First is the volume and variety of the boards
> that use our SOCs.. Second, is
> our lack of information/control:  often, the board is designed by one
> company (not us), and
> given to another company as the middleman, and then they want the
> features outlined
> in my aforementioned commit message.

Other vendors have plenty of variation in board design yet we still have
device trees that describe the hardware, I can't see why these systems
should be so different.  It is entirely normal for system integrators to
collaborate on this and even upstream their own code, this happens all
the time, there is no need for everything to be implemented directly the
SoC vendor.  

If there are generic quirks that match a common pattern seen in a lot of
board then properties can be defined for those, this is in fact the
common case.  This is no reason to just shove in some random thing that
doesn't describe the hardware, that's a great way to end up stuck with
an ABI that is fragile and difficult to understand or improve.
Potentially some of these things should be being handled in the bindings
and drivers drivers for the relevant PCI devices rather than in the PCI
controller.

> > I'd suggest as a first pass omitting this and then looking at some
> > actual systems later when working out how to support them, no sense in
> > getting the main thing held up by difficult edge cases.

> These are not edge cases -- some of these are major customers.

I'm trying to help you progress the driver by decoupling things which
are causing difficulty from the simple parts so that we don't need to
keep looking at the simple bits over and over again.  If these systems
are very common or familiar then it should be fairly easy to describe
the common patterns in what they're doing.

In any case whatever gets done needs to be documented in the bindings
documents.

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

Re: [PATCH v5 4/6] PCI: brcmstb: Add control of subdevice voltage regulators

[Florian Fainelli]

On 10/25/21 7:58 AM, Mark Brown wrote:
> On Mon, Oct 25, 2021 at 09:50:09AM -0400, Jim Quinlan wrote:
>> On Fri, Oct 22, 2021 at 3:47 PM Mark Brown <broonie@kernel.org> wrote:
>>> On Fri, Oct 22, 2021 at 03:15:59PM -0400, Jim Quinlan wrote:
> 
>>> That sounds like it just shouldn't be a regulator at all, perhaps the
>>> board happens to need a regulator there but perhaps it needs a clock,
>>> GPIO or some specific sequence of actions.  It sounds like you need some
>>> sort of quirking mechanism to cope with individual boards with board
>>> specific bindings.
> 
>> The boards involved may have no PCIe sockets, or run the gamut of the different
>> PCIe sockets.  They all offer gpio(s) to turn off/on their power supply(s) to
>> make their PCIe device endpoint functional.  It is not viable to add
>> new Linux quirk or DT
>> code for each board.  First is the volume and variety of the boards
>> that use our SOCs.. Second, is
>> our lack of information/control:  often, the board is designed by one
>> company (not us), and
>> given to another company as the middleman, and then they want the
>> features outlined
>> in my aforementioned commit message.
> 
> Other vendors have plenty of variation in board design yet we still have
> device trees that describe the hardware, I can't see why these systems
> should be so different.  It is entirely normal for system integrators to
> collaborate on this and even upstream their own code, this happens all
> the time, there is no need for everything to be implemented directly the
> SoC vendor.

This is all well and good and there is no disagreement here that it
should just be that way but it does not reflect what Jim and I are
confronted with on a daily basis. We work in a tightly controlled
environment using a waterfall approach, whatever we come up with as a
SoC vendor gets used usually without further modification by the OEMs,
when OEMs do change things we have no visibility into anyway.

We have a boot loader that goes into great lengths to tailor the FDT
blob passed to the kernel to account for board-specific variations, PCIe
voltage regulators being just one of those variations. This is usually
how we quirk and deal with any board specific details, so I fail to
understand what you mean by "quirks that match a common pattern".

Also, I don't believe other vendors are quite as concerned with
conserving power as we are, it could be that they are just better at it
through different ways, or we have a particular sensitivity to the subject.

> 
> If there are generic quirks that match a common pattern seen in a lot of
> board then properties can be defined for those, this is in fact the
> common case.  This is no reason to just shove in some random thing that
> doesn't describe the hardware, that's a great way to end up stuck with
> an ABI that is fragile and difficult to understand or improve.

I would argue that at least 2 out of the 4 supplies proposed do describe
hardware signals. The latter two are more abstract to say the least,
however I believe it is done that way because they are composite
supplies controlling both the 12V and 3.3V supplies coming into a PCIe
device (Jim is that right?). So how do we call the latter supply then?
vpcie12v3v...-supply?

> Potentially some of these things should be being handled in the bindings
> and drivers drivers for the relevant PCI devices rather than in the PCI
> controller.

The description and device tree binding can be and should be in a PCI
device binding rather than pci-bus.yaml.

The handling however goes back to the chicken and egg situation that we
talked about multiple times before: no supply -> no link UP -> no
enumeration -> no PCI device, therefore no driver can have a chance to
control the regulator. These are not hotplug capable systems by the way,
but even if they were, we would still run into the same problem. Given
that most reference boards do have mechanical connectors that people can
plug random devices into, we cannot provide a compatible string
containing the PCI vendor/device ID ahead of time because we don't know
what will be plugged in. In the case of a MCM, we would, but then we
only solved about 15% of the boards we need to support, so we have not
really progressed much.

> 
>>> I'd suggest as a first pass omitting this and then looking at some
>>> actual systems later when working out how to support them, no sense in
>>> getting the main thing held up by difficult edge cases.
> 
>> These are not edge cases -- some of these are major customers.
> 
> I'm trying to help you progress the driver by decoupling things which
> are causing difficulty from the simple parts so that we don't need to
> keep looking at the simple bits over and over again.  If these systems
> are very common or familiar then it should be fairly easy to describe
> the common patterns in what they're doing.

We are appreciative of your feedback, and Rob's, and everyone else
looking at the patches really.

> 
> In any case whatever gets done needs to be documented in the bindings
> documents.

Is not that what patch #1 does?
-- 
Florian

Re: [PATCH v5 4/6] PCI: brcmstb: Add control of subdevice voltage regulators

[Rob Herring]

On Mon, Oct 25, 2021 at 03:04:34PM -0700, Florian Fainelli wrote:
> On 10/25/21 7:58 AM, Mark Brown wrote:
> > On Mon, Oct 25, 2021 at 09:50:09AM -0400, Jim Quinlan wrote:
> >> On Fri, Oct 22, 2021 at 3:47 PM Mark Brown <broonie@kernel.org> wrote:
> >>> On Fri, Oct 22, 2021 at 03:15:59PM -0400, Jim Quinlan wrote:
> > 
> >>> That sounds like it just shouldn't be a regulator at all, perhaps the
> >>> board happens to need a regulator there but perhaps it needs a clock,
> >>> GPIO or some specific sequence of actions.  It sounds like you need some
> >>> sort of quirking mechanism to cope with individual boards with board
> >>> specific bindings.
> > 
> >> The boards involved may have no PCIe sockets, or run the gamut of the different
> >> PCIe sockets.  They all offer gpio(s) to turn off/on their power supply(s) to
> >> make their PCIe device endpoint functional.  It is not viable to add
> >> new Linux quirk or DT
> >> code for each board.  First is the volume and variety of the boards
> >> that use our SOCs.. Second, is
> >> our lack of information/control:  often, the board is designed by one
> >> company (not us), and
> >> given to another company as the middleman, and then they want the
> >> features outlined
> >> in my aforementioned commit message.
> > 
> > Other vendors have plenty of variation in board design yet we still have
> > device trees that describe the hardware, I can't see why these systems
> > should be so different.  It is entirely normal for system integrators to
> > collaborate on this and even upstream their own code, this happens all
> > the time, there is no need for everything to be implemented directly the
> > SoC vendor.
> 
> This is all well and good and there is no disagreement here that it
> should just be that way but it does not reflect what Jim and I are
> confronted with on a daily basis. We work in a tightly controlled
> environment using a waterfall approach, whatever we come up with as a
> SoC vendor gets used usually without further modification by the OEMs,
> when OEMs do change things we have no visibility into anyway.
> 
> We have a boot loader that goes into great lengths to tailor the FDT
> blob passed to the kernel to account for board-specific variations, PCIe
> voltage regulators being just one of those variations. This is usually
> how we quirk and deal with any board specific details, so I fail to
> understand what you mean by "quirks that match a common pattern".
> 
> Also, I don't believe other vendors are quite as concerned with
> conserving power as we are, it could be that they are just better at it
> through different ways, or we have a particular sensitivity to the subject.
> 
> > 
> > If there are generic quirks that match a common pattern seen in a lot of
> > board then properties can be defined for those, this is in fact the
> > common case.  This is no reason to just shove in some random thing that
> > doesn't describe the hardware, that's a great way to end up stuck with
> > an ABI that is fragile and difficult to understand or improve.
> 
> I would argue that at least 2 out of the 4 supplies proposed do describe
> hardware signals. The latter two are more abstract to say the least,
> however I believe it is done that way because they are composite
> supplies controlling both the 12V and 3.3V supplies coming into a PCIe
> device (Jim is that right?). So how do we call the latter supply then?
> vpcie12v3v...-supply?
> 
> > Potentially some of these things should be being handled in the bindings
> > and drivers drivers for the relevant PCI devices rather than in the PCI
> > controller.
> 
> The description and device tree binding can be and should be in a PCI
> device binding rather than pci-bus.yaml.
> 
> The handling however goes back to the chicken and egg situation that we
> talked about multiple times before: no supply -> no link UP -> no
> enumeration -> no PCI device, therefore no driver can have a chance to
> control the regulator. These are not hotplug capable systems by the way,
> but even if they were, we would still run into the same problem. Given
> that most reference boards do have mechanical connectors that people can
> plug random devices into, we cannot provide a compatible string
> containing the PCI vendor/device ID ahead of time because we don't know
> what will be plugged in. 

I thought you didn't have connectors or was it just they are 
non-standard? If the latter case, what are the supply rails for the 
connector?

I'd be okay if there's no compatible as long as there's not a continual 
stream of DT properties trying to describe power sequencing 
requirements.

> In the case of a MCM, we would, but then we
> only solved about 15% of the boards we need to support, so we have not
> really progressed much.

MCM is multi-chip module?

Rob

Re: [PATCH v5 4/6] PCI: brcmstb: Add control of subdevice voltage regulators

[Florian Fainelli]

On 10/25/21 3:43 PM, Rob Herring wrote:
> On Mon, Oct 25, 2021 at 03:04:34PM -0700, Florian Fainelli wrote:
>> On 10/25/21 7:58 AM, Mark Brown wrote:
>>> On Mon, Oct 25, 2021 at 09:50:09AM -0400, Jim Quinlan wrote:
>>>> On Fri, Oct 22, 2021 at 3:47 PM Mark Brown <broonie@kernel.org> wrote:
>>>>> On Fri, Oct 22, 2021 at 03:15:59PM -0400, Jim Quinlan wrote:
>>>
>>>>> That sounds like it just shouldn't be a regulator at all, perhaps the
>>>>> board happens to need a regulator there but perhaps it needs a clock,
>>>>> GPIO or some specific sequence of actions.  It sounds like you need some
>>>>> sort of quirking mechanism to cope with individual boards with board
>>>>> specific bindings.
>>>
>>>> The boards involved may have no PCIe sockets, or run the gamut of the different
>>>> PCIe sockets.  They all offer gpio(s) to turn off/on their power supply(s) to
>>>> make their PCIe device endpoint functional.  It is not viable to add
>>>> new Linux quirk or DT
>>>> code for each board.  First is the volume and variety of the boards
>>>> that use our SOCs.. Second, is
>>>> our lack of information/control:  often, the board is designed by one
>>>> company (not us), and
>>>> given to another company as the middleman, and then they want the
>>>> features outlined
>>>> in my aforementioned commit message.
>>>
>>> Other vendors have plenty of variation in board design yet we still have
>>> device trees that describe the hardware, I can't see why these systems
>>> should be so different.  It is entirely normal for system integrators to
>>> collaborate on this and even upstream their own code, this happens all
>>> the time, there is no need for everything to be implemented directly the
>>> SoC vendor.
>>
>> This is all well and good and there is no disagreement here that it
>> should just be that way but it does not reflect what Jim and I are
>> confronted with on a daily basis. We work in a tightly controlled
>> environment using a waterfall approach, whatever we come up with as a
>> SoC vendor gets used usually without further modification by the OEMs,
>> when OEMs do change things we have no visibility into anyway.
>>
>> We have a boot loader that goes into great lengths to tailor the FDT
>> blob passed to the kernel to account for board-specific variations, PCIe
>> voltage regulators being just one of those variations. This is usually
>> how we quirk and deal with any board specific details, so I fail to
>> understand what you mean by "quirks that match a common pattern".
>>
>> Also, I don't believe other vendors are quite as concerned with
>> conserving power as we are, it could be that they are just better at it
>> through different ways, or we have a particular sensitivity to the subject.
>>
>>>
>>> If there are generic quirks that match a common pattern seen in a lot of
>>> board then properties can be defined for those, this is in fact the
>>> common case.  This is no reason to just shove in some random thing that
>>> doesn't describe the hardware, that's a great way to end up stuck with
>>> an ABI that is fragile and difficult to understand or improve.
>>
>> I would argue that at least 2 out of the 4 supplies proposed do describe
>> hardware signals. The latter two are more abstract to say the least,
>> however I believe it is done that way because they are composite
>> supplies controlling both the 12V and 3.3V supplies coming into a PCIe
>> device (Jim is that right?). So how do we call the latter supply then?
>> vpcie12v3v...-supply?
>>
>>> Potentially some of these things should be being handled in the bindings
>>> and drivers drivers for the relevant PCI devices rather than in the PCI
>>> controller.
>>
>> The description and device tree binding can be and should be in a PCI
>> device binding rather than pci-bus.yaml.
>>
>> The handling however goes back to the chicken and egg situation that we
>> talked about multiple times before: no supply -> no link UP -> no
>> enumeration -> no PCI device, therefore no driver can have a chance to
>> control the regulator. These are not hotplug capable systems by the way,
>> but even if they were, we would still run into the same problem. Given
>> that most reference boards do have mechanical connectors that people can
>> plug random devices into, we cannot provide a compatible string
>> containing the PCI vendor/device ID ahead of time because we don't know
>> what will be plugged in. 
> 
> I thought you didn't have connectors or was it just they are 
> non-standard? If the latter case, what are the supply rails for the 
> connector?

We now have reference boards with full-sized x1 and x4 connectors in
addition to half sized and full-sized mini-PCIe connectors and the
soldered down Wi-Fi on board (WOMBO) and the Multi-chip Module packages
(MCM).

When using connectors we would use the standard PCIe pinout nomenclature
however for the latter two, there appears to be a variety of
non-standard stuff being done there. We reviewed some schematics with
Jim and it looks like some of the usages for the regulators are just
laziness on the Wi-Fi driver side, like asking the kernel to keep the
radio on (PA, LNA etc.) as if it was as critical and necessary as the
12V and 3.3V supplies that actually power on the PCIe end-point... We
will get those fixed hopefully.

> 
> I'd be okay if there's no compatible as long as there's not a continual 
> stream of DT properties trying to describe power sequencing 
> requirements.

Have not looked whether Dmitry's power sequencing generalizing is
helping us here:

https://www.spinics.net/lists/linux-bluetooth/msg93564.html

it still looks like the PCIe host controller is involved in requesting
the power sequence.

> 
>> In the case of a MCM, we would, but then we
>> only solved about 15% of the boards we need to support, so we have not
>> really progressed much.
> 
> MCM is multi-chip module?

Correct, see above.
-- 
Florian

Re: [PATCH v5 4/6] PCI: brcmstb: Add control of subdevice voltage regulators

[Mark Brown]

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

On Mon, Oct 25, 2021 at 03:04:34PM -0700, Florian Fainelli wrote:
> On 10/25/21 7:58 AM, Mark Brown wrote:

> > Other vendors have plenty of variation in board design yet we still have
> > device trees that describe the hardware, I can't see why these systems
> > should be so different.  It is entirely normal for system integrators to
> > collaborate on this and even upstream their own code, this happens all
> > the time, there is no need for everything to be implemented directly the
> > SoC vendor.

> This is all well and good and there is no disagreement here that it
> should just be that way but it does not reflect what Jim and I are
> confronted with on a daily basis. We work in a tightly controlled
> environment using a waterfall approach, whatever we come up with as a
> SoC vendor gets used usually without further modification by the OEMs,
> when OEMs do change things we have no visibility into anyway.

This doesn't really sound terribly unusual frankly, which means it
shouldn't be insurmountable.  It also doesn't sound like a great
approach to base ABIs around.

> We have a boot loader that goes into great lengths to tailor the FDT
> blob passed to the kernel to account for board-specific variations, PCIe
> voltage regulators being just one of those variations. This is usually
> how we quirk and deal with any board specific details, so I fail to
> understand what you mean by "quirks that match a common pattern".

If more than one board needs the same accomodation, for example if it's
common for a reset line to be GPIO controlled, then a common property
could be used by many different boards rather than requiring each
individual board to have board specific code.  This is on some level
what most DT properties boil down to.

> Also, I don't believe other vendors are quite as concerned with
> conserving power as we are, it could be that they are just better at it
> through different ways, or we have a particular sensitivity to the subject.

I'm fairly sure that for example phone vendors pay a bit of attention to
power consumption.

> > If there are generic quirks that match a common pattern seen in a lot of
> > board then properties can be defined for those, this is in fact the
> > common case.  This is no reason to just shove in some random thing that
> > doesn't describe the hardware, that's a great way to end up stuck with
> > an ABI that is fragile and difficult to understand or improve.

> I would argue that at least 2 out of the 4 supplies proposed do describe
> hardware signals. The latter two are more abstract to say the least,
> however I believe it is done that way because they are composite
> supplies controlling both the 12V and 3.3V supplies coming into a PCIe
> device (Jim is that right?). So how do we call the latter supply then?
> vpcie12v3v...-supply?

The binding for a consumer should reflect what's going into that
consumer, this means that if you have 12V and 3.3V supplies then the
device should have two distinct supplies for that.  The device binding
should not change based on how those supplies are provided or any
relationship between the supplies outside the device, there should
definitely be no reason to define any new supplies for this purpose -
that would reflect a fundamental misunderstanding of the abstractions in
the API.

If (as it sounds) you've got systems with two supplies with GPIO enables
controlled by a single GPIO then you should just describe that directly
in device tree, this is quite common and there is support in there
already for identifying and reference counting the shared GPIO in that
case.

> > Potentially some of these things should be being handled in the bindings
> > and drivers drivers for the relevant PCI devices rather than in the PCI
> > controller.

> The description and device tree binding can be and should be in a PCI
> device binding rather than pci-bus.yaml.

Well, it's a bit of a shared responsibility where the thing being
provided is a standards conforming connector rather than there being an
embedded device with much more potential for variation.

> The handling however goes back to the chicken and egg situation that we
> talked about multiple times before: no supply -> no link UP -> no
> enumeration -> no PCI device, therefore no driver can have a chance to
> control the regulator. These are not hotplug capable systems by the way,
> but even if they were, we would still run into the same problem. Given
> that most reference boards do have mechanical connectors that people can
> plug random devices into, we cannot provide a compatible string
> containing the PCI vendor/device ID ahead of time because we don't know
> what will be plugged in. In the case of a MCM, we would, but then we
> only solved about 15% of the boards we need to support, so we have not
> really progressed much.

I would expect it's possible to make a PCI binding for a standard
physical layer bus connection as part of the generic PCI bindings, for
example by using some standard invalid ID for the device ID that can't
exist in a physical system or just omitting the device ID.  That seems
like a fairly clear case of being able to describe actual hardware that
physically exists - you can see the physical socket on the board.

> > In any case whatever gets done needs to be documented in the bindings
> > documents.

> Is not that what patch #1 does?

It just updated the example, it didn't document any new properties.  The
standard supplies are documented in the patch to the core standard that
was referenced so they're fine but not the extra Broadcom specific ones
that I've raised concerns with.

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

Re: [PATCH v5 4/6] PCI: brcmstb: Add control of subdevice voltage regulators

[Rob Herring]

On Fri, Oct 22, 2021 at 03:15:59PM -0400, Jim Quinlan wrote:
> On Fri, Oct 22, 2021 at 10:31 AM Mark Brown <broonie@kernel.org> wrote:
> >
> > On Fri, Oct 22, 2021 at 10:06:57AM -0400, Jim Quinlan wrote:
> >
> > > +static const char * const supplies[] = {
> > > +     "vpcie3v3-supply",
> > > +     "vpcie3v3aux-supply",
> > > +     "brcm-ep-a-supply",
> > > +     "brcm-ep-b-supply",
> > > +};
> >
> > Why are you including "-supply" in the names here?  That will lead to
> > a double -supply when we look in the DT which probably isn't what you're
> > looking for.
> I'm not sure how this got past testing; will fix.
> 
> >
> > Also are you *sure* that the device has supplies with names like
> > "brcm-ep-a"?  That seems rather unidiomatic for electrical engineering,
> > the names here are supposed to correspond to the names used in the
> > datasheet for the part.
> I try to explain this in the commit message of"PCI: allow for callback
> to prepare nascent subdev".  Wrt to the names,
> 
>        "These regulators typically govern the actual power supply to the
>         endpoint chip.  Sometimes they may be a the official PCIe socket
>         power -- such as 3.3v or aux-3.3v.  Sometimes they are truly
>         the regulator(s) that supply power to the EP chip."
> 
> Each different SOC./board we deal with may present different ways of
> making the EP device power on.  We are using
> an abstraction name "brcm-ep-a"  to represent some required regulator
> to make the EP  work for a specific board.  The RC
> driver cannot hard code a descriptive name as it must work for all
> boards designed by us, others, and third parties.
> The EP driver also doesn't know  or care about the regulator name, and
> this driver is often closed source and often immutable.  The EP
> device itself may come from Brcm, a third party,  or sometimes a competitor.
> 
> Basically, we find using a generic name such as "brcm-ep-a-supply"
> quite handy and many of our customers embrace this feature.
> I know that Rob was initially against such a generic name, but I
> vaguely remember him seeing some merit to this, perhaps a tiny bit :-)
> Or my memory is shot, which could very well be the case.

I don't recall being in favor of this. If you've got standard rails 
(3.3V and 12V), then I'm fine with standard properties for them with or 
without a slot.

Rob

[PATCH v5 5/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 | 59 +++++++++++++++++++++++----
 1 file changed, 52 insertions(+), 7 deletions(-)

diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
index 35924af1c3aa..505f74bd1a51 100644
--- a/drivers/pci/controller/pcie-brcmstb.c
+++ b/drivers/pci/controller/pcie-brcmstb.c
@@ -192,6 +192,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[] = {
 	"vpcie3v3-supply",
@@ -305,22 +306,65 @@ struct brcm_pcie {
 	void			(*bridge_sw_init_set)(struct brcm_pcie *pcie, u32 val);
 	struct regulator_bulk_data supplies[ARRAY_SIZE(supplies)];
 	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;
 }
 
@@ -1234,7 +1278,7 @@ static int brcm_pcie_suspend(struct device *dev)
 	reset_control_rearm(pcie->rescal);
 	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)
@@ -1250,7 +1294,8 @@ static int brcm_pcie_resume(struct device *dev)
 	ret = reset_control_reset(pcie->rescal);
 	if (ret)
 		goto err_disable_clk;
-	ret = brcm_set_regulators(pcie, true);
+
+	ret = brcm_set_regulators(pcie, TURN_ON);
 	if (ret)
 		goto err_reset;
 
@@ -1297,7 +1342,7 @@ static void __brcm_pcie_remove(struct brcm_pcie *pcie)
 	reset_control_rearm(pcie->rescal);
 	clk_disable_unprepare(pcie->clk);
 	if (pcie->num_supplies) {
-		(void)brcm_set_regulators(pcie, false);
+		(void)brcm_set_regulators(pcie, TURN_OFF_ALWAYS);
 		regulator_bulk_free(pcie->num_supplies, pcie->supplies);
 	}
 }
@@ -1348,7 +1393,7 @@ static int brcm_pcie_pci_subdev_prepare(bool query, struct pci_bus *bus, int dev
 		return ret;
 	}
 
-	ret = brcm_set_regulators(pcie, true);
+	ret = brcm_set_regulators(pcie, TURN_ON);
 	if (ret)
 		goto err_out0;
 
-- 
2.17.1

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

[Mark Brown]

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

On Fri, Oct 22, 2021 at 10:06:58AM -0400, Jim Quinlan wrote:

> +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);

I can't help but think this would be easier to follow as multiple
functions, there is very little code sharing between the different
paths especially the on and off paths.

>  	if (pcie->num_supplies) {
> -		(void)brcm_set_regulators(pcie, false);
> +		(void)brcm_set_regulators(pcie, TURN_OFF_ALWAYS);

I should've mentioned this on the earlier path but it's not normal Linux
style to cast return values to void and looks worrying.

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

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

[Jim Quinlan]

On Fri, Oct 22, 2021 at 10:39 AM Mark Brown <broonie@kernel.org> wrote:
>
> On Fri, Oct 22, 2021 at 10:06:58AM -0400, Jim Quinlan wrote:
>
> > +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);
>
> I can't help but think this would be easier to follow as multiple
> functions, there is very little code sharing between the different
> paths especially the on and off paths.
Agree; I just wanted to make less changes to struct pci_host_bridge.  Will fix.

>
> >       if (pcie->num_supplies) {
> > -             (void)brcm_set_regulators(pcie, false);
> > +             (void)brcm_set_regulators(pcie, TURN_OFF_ALWAYS);
>
> I should've mentioned this on the earlier path but it's not normal Linux
> style to cast return values to void and looks worrying.
Got it.

Thanks,
JimQ

[PATCH v5 6/6] PCI: brcmstb: change brcm_phy_stop() to return void

[Jim Quinlan]

We do not use the result of this function so make it void.

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

diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
index 505f74bd1a51..d3e6d9df67b5 100644
--- a/drivers/pci/controller/pcie-brcmstb.c
+++ b/drivers/pci/controller/pcie-brcmstb.c
@@ -1190,9 +1190,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)
@@ -1271,10 +1272,9 @@ static int brcm_pcie_get_regulators(struct brcm_pcie *pcie, struct pci_dev *dev)
 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);
 	reset_control_rearm(pcie->rescal);
 	clk_disable_unprepare(pcie->clk);
 
-- 
2.17.1