[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

[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

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.