| Message ID | 20211022140714.28767-2-jim2101024@gmail.com |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | PCI: brcmstb: have host-bridge turn on sub-device power | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | success | |
| robh/dt-meta-schema | fail | build log |
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 <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
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 <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.
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 > >
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 > > > >
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 <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
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
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.
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; + }; + }; }; };
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(+)