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 <[email protected]>
---
.../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
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.
On Fri, Oct 22, 2021 at 10:49 AM Mark Brown <[email protected]> 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
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.
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 <[email protected]>
> ---
> .../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.
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 <[email protected]>
> ---
> .../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
>
>
On Mon, Oct 25, 2021 at 6:24 PM Rob Herring <[email protected]> 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 <[email protected]>
> > ---
> > .../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
> >
> >
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
On Tue, Oct 26, 2021 at 4:27 PM Jim Quinlan <[email protected]> wrote:
>
> On Mon, Oct 25, 2021 at 6:24 PM Rob Herring <[email protected]> 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 <[email protected]>
> > > ---
> > > .../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
On Wed, Oct 27, 2021 at 4:54 PM Rob Herring <[email protected]> wrote:
>
> On Tue, Oct 26, 2021 at 4:27 PM Jim Quinlan <[email protected]> wrote:
> >
> > On Mon, Oct 25, 2021 at 6:24 PM Rob Herring <[email protected]> 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 <[email protected]>
> > > > ---
> > > > .../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
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.