RFC: Change PCI DTS scheme for port/link specific properties

RFC: Change PCI DTS scheme for port/link specific properties

[Pali Rohár]

Hello Rob!

I think that the current DT scheme for PCI buses and devices defined in
Linux kernel tree has wrong definitions of port/link specific properties:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/pci/pci.txt

Port or link specific properties are at least: max-link-speed,
reset-gpios and supports-clkreq. And are currently defined as properties
of host bridges.

Host Bridge contains one or more PCIe Root Ports (each represented as
PCI Bridge device) and to each PCIe Root Port can be connected exactly
one PCIe Upstream device.

PCIe Upstream device can be either endpoint PCIe card or it can be also
PCIe switch is consists of exactly one Upstream Port (represented as PCI
Bridge device) and then one or more Downstream Port devices (each
represent as PCI Bridge device). To each Downstream Port can be
connected again exactly one PCIe Upstream device.

Port or link specific properties (e.g. max-link-speed, reset-gpios and
supports-clkreq) define "the PCIe link" between the Root/Downstream
device and Endpoint/Upstream device. And it is basically Root/Downstream
device which configures the port or link. So I think that these
properties should not be in Host Bridge DTS node, but rather in DTS node
which represents Root Port (or Downstream Port in case of PCIe switches).

Mauro wrote in another email, that he has PCIe topology with
single-root-port host bridge to which is connected multi-port PCIe
switch and he needs to control reset-gpios of devices connected to
downstream ports of PCIe switch.

Current pci.txt DT scheme is fully unsuitable for this kind of setup as
basically PCIe switch is completely independent device of host bridge
and so host bridge really should not define in its node properties which
do not belong to host bridge itself.

Rob, what do you think, how to solve this issue?

I would suggest to define that max-link-speed, reset-gpios and
supports-clkreq properties should be in node for Root Port and
Downstream Port devices and not in host bridge nodes.

So DTS for PCIe controller which has 2 root ports where to first root
port is connected PCIe switch with 2 cards and to second root port is
connected just endpoint card:

pcie-host-bridge {
	compatible = "vendor-controller-string"; /* e.g. designware, etc... */

	pcie-root-port-1@0,0 {
		reg = <0x00000000 0 0 0 0>; /* root port at device 0 */
		reset-gpios = ...; /* resets card connected to root-port-1 which is pcie-switch-1-upstream-port */
		max-link-speed = <3> /* link between root-port-1 and switch is GEN3 */

		pcie-switch-1-upstream-port@0,0 {
			reg = ...; /* upstream port at device 0 */

			pcie-switch-1-downstream-port-1@X,0 {
				reg = ...; /* downstream port 1 at switch specific address */
				reset-gpios = ...; /* resets card connected to switch's port 1 */
				max-link-speed = <1> /* link between this port and card is GEN1 */

				/* optional node for endpoint card */
				/* pcie-card@0,0 { ... }; */
			};

			pcie-switch-1-downstream-port-2@Y,0 {
				reg = ...; /* downstream port 2 at switch specific address */
				reset-gpios = ...; /* resets card connected to switch's port 2 */
				max-link-speed = <1> /* link between this port and card is GEN1 */

				/* optional node for endpoint card */
				/* pcie-card@0,0 { ... }; */
			};
		};
	};

	pcie-root-port-2@1,0 {
		reg = <0x00000800 0 0 0 0>; /* root port at device 1 */
		reset-gpios = ...; /* resets card connected to root-port-2 */
		max-link-speed = <2> /* link between root-port-2 and card below is just GEN2 */

		/* optional node for endpoint card */
		/* pcie-card@0,0 { ... }; */
	};
};

Any opinion?

Re: RFC: Change PCI DTS scheme for port/link specific properties

[Rob Herring]

On Sat, Oct 23, 2021 at 9:43 AM Pali Rohár <pali@kernel.org> wrote:
>
> Hello Rob!
>
> I think that the current DT scheme for PCI buses and devices defined in
> Linux kernel tree has wrong definitions of port/link specific properties:
>
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/pci/pci.txt
>
> Port or link specific properties are at least: max-link-speed,
> reset-gpios and supports-clkreq. And are currently defined as properties
> of host bridges.

pci-bus.yaml in dtschema is what matters now and it's a bit more flexible.

> Host Bridge contains one or more PCIe Root Ports (each represented as
> PCI Bridge device) and to each PCIe Root Port can be connected exactly
> one PCIe Upstream device.
>
> PCIe Upstream device can be either endpoint PCIe card or it can be also
> PCIe switch is consists of exactly one Upstream Port (represented as PCI
> Bridge device) and then one or more Downstream Port devices (each
> represent as PCI Bridge device). To each Downstream Port can be
> connected again exactly one PCIe Upstream device.
>
> Port or link specific properties (e.g. max-link-speed, reset-gpios and
> supports-clkreq) define "the PCIe link" between the Root/Downstream
> device and Endpoint/Upstream device. And it is basically Root/Downstream
> device which configures the port or link. So I think that these
> properties should not be in Host Bridge DTS node, but rather in DTS node
> which represents Root Port (or Downstream Port in case of PCIe switches).

I tend to agree, but that ship has sailed because we don't tend to
have a RP node in DT. Most host bridges also tend to be a single RP.
In those cases, the properties come from whatever node we have.

Certainly if there are multiple RPs on the host bridge bus (bus 0),
then we need multiple child nodes for the RPs. IIRC, some host
bindings do this already.

> Mauro wrote in another email, that he has PCIe topology with
> single-root-port host bridge to which is connected multi-port PCIe
> switch and he needs to control reset-gpios of devices connected to
> downstream ports of PCIe switch.

I did a lot of work on that to get it right in terms of having the
right nodes matching the bus hierarchy and resets distributed in the
nodes.

>
> Current pci.txt DT scheme is fully unsuitable for this kind of setup as
> basically PCIe switch is completely independent device of host bridge
> and so host bridge really should not define in its node properties which
> do not belong to host bridge itself.
>
> Rob, what do you think, how to solve this issue?
>
> I would suggest to define that max-link-speed, reset-gpios and
> supports-clkreq properties should be in node for Root Port and
> Downstream Port devices and not in host bridge nodes.
>
> So DTS for PCIe controller which has 2 root ports where to first root
> port is connected PCIe switch with 2 cards and to second root port is
> connected just endpoint card:
>
> pcie-host-bridge {
>         compatible = "vendor-controller-string"; /* e.g. designware, etc... */
>
>         pcie-root-port-1@0,0 {

pcie@0,0 and 'device_type = "pci"' are needed to indicate this is a
bridge node and apply the schema.

>                 reg = <0x00000000 0 0 0 0>; /* root port at device 0 */
>                 reset-gpios = ...; /* resets card connected to root-port-1 which is pcie-switch-1-upstream-port */
>                 max-link-speed = <3> /* link between root-port-1 and switch is GEN3 */
>
>                 pcie-switch-1-upstream-port@0,0 {
>                         reg = ...; /* upstream port at device 0 */
>
>                         pcie-switch-1-downstream-port-1@X,0 {
>                                 reg = ...; /* downstream port 1 at switch specific address */
>                                 reset-gpios = ...; /* resets card connected to switch's port 1 */
>                                 max-link-speed = <1> /* link between this port and card is GEN1 */
>
>                                 /* optional node for endpoint card */
>                                 /* pcie-card@0,0 { ... }; */
>                         };
>
>                         pcie-switch-1-downstream-port-2@Y,0 {
>                                 reg = ...; /* downstream port 2 at switch specific address */
>                                 reset-gpios = ...; /* resets card connected to switch's port 2 */
>                                 max-link-speed = <1> /* link between this port and card is GEN1 */
>
>                                 /* optional node for endpoint card */
>                                 /* pcie-card@0,0 { ... }; */
>                         };
>                 };
>         };
>
>         pcie-root-port-2@1,0 {
>                 reg = <0x00000800 0 0 0 0>; /* root port at device 1 */
>                 reset-gpios = ...; /* resets card connected to root-port-2 */
>                 max-link-speed = <2> /* link between root-port-2 and card below is just GEN2 */
>
>                 /* optional node for endpoint card */
>                 /* pcie-card@0,0 { ... }; */
>         };
> };
>
> Any opinion?

Re: RFC: Change PCI DTS scheme for port/link specific properties

[Pali Rohár]

On Monday 25 October 2021 10:39:42 Rob Herring wrote:
> On Sat, Oct 23, 2021 at 9:43 AM Pali Rohár <pali@kernel.org> wrote:
> >
> > Hello Rob!
> >
> > I think that the current DT scheme for PCI buses and devices defined in
> > Linux kernel tree has wrong definitions of port/link specific properties:
> >
> > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/pci/pci.txt
> >
> > Port or link specific properties are at least: max-link-speed,
> > reset-gpios and supports-clkreq. And are currently defined as properties
> > of host bridges.
> 
> pci-bus.yaml in dtschema is what matters now and it's a bit more flexible.

I do not see any pci-bus.yaml file in linux kernel tree... It is missing
or it is external?

> > Host Bridge contains one or more PCIe Root Ports (each represented as
> > PCI Bridge device) and to each PCIe Root Port can be connected exactly
> > one PCIe Upstream device.
> >
> > PCIe Upstream device can be either endpoint PCIe card or it can be also
> > PCIe switch is consists of exactly one Upstream Port (represented as PCI
> > Bridge device) and then one or more Downstream Port devices (each
> > represent as PCI Bridge device). To each Downstream Port can be
> > connected again exactly one PCIe Upstream device.
> >
> > Port or link specific properties (e.g. max-link-speed, reset-gpios and
> > supports-clkreq) define "the PCIe link" between the Root/Downstream
> > device and Endpoint/Upstream device. And it is basically Root/Downstream
> > device which configures the port or link. So I think that these
> > properties should not be in Host Bridge DTS node, but rather in DTS node
> > which represents Root Port (or Downstream Port in case of PCIe switches).
> 
> I tend to agree, but that ship has sailed because we don't tend to
> have a RP node in DT. Most host bridges also tend to be a single RP.
> In those cases, the properties come from whatever node we have.

For, for cases with single root port on host bridge bus, it could be
possible to have these port/link properties directly in host bridge
node. As it is unambiguous what they describe and to which hw part they
belong.

> Certainly if there are multiple RPs on the host bridge bus (bus 0),
> then we need multiple child nodes for the RPs. IIRC, some host
> bindings do this already.

Yes, some of them are doing it. And it would be a good if it is a
requirement for all new multi-port bindings and pcie controller drivers.

As it is a mess if every driver define these properties by its own.
Having one common / standard driver agnostic way for defining
complicated topology is really better.

> > Mauro wrote in another email, that he has PCIe topology with
> > single-root-port host bridge to which is connected multi-port PCIe
> > switch and he needs to control reset-gpios of devices connected to
> > downstream ports of PCIe switch.
> 
> I did a lot of work on that to get it right in terms of having the
> right nodes matching the bus hierarchy and resets distributed in the
> nodes.
> 
> >
> > Current pci.txt DT scheme is fully unsuitable for this kind of setup as
> > basically PCIe switch is completely independent device of host bridge
> > and so host bridge really should not define in its node properties which
> > do not belong to host bridge itself.
> >
> > Rob, what do you think, how to solve this issue?
> >
> > I would suggest to define that max-link-speed, reset-gpios and
> > supports-clkreq properties should be in node for Root Port and
> > Downstream Port devices and not in host bridge nodes.
> >
> > So DTS for PCIe controller which has 2 root ports where to first root
> > port is connected PCIe switch with 2 cards and to second root port is
> > connected just endpoint card:
> >
> > pcie-host-bridge {
> >         compatible = "vendor-controller-string"; /* e.g. designware, etc... */
> >
> >         pcie-root-port-1@0,0 {
> 
> pcie@0,0 and 'device_type = "pci"' are needed to indicate this is a
> bridge node and apply the schema.

Ok. I probably omitted more properties here. I just wanted to show
example how link speeds and PERST# pins are defined here.

> >                 reg = <0x00000000 0 0 0 0>; /* root port at device 0 */
> >                 reset-gpios = ...; /* resets card connected to root-port-1 which is pcie-switch-1-upstream-port */
> >                 max-link-speed = <3> /* link between root-port-1 and switch is GEN3 */
> >
> >                 pcie-switch-1-upstream-port@0,0 {
> >                         reg = ...; /* upstream port at device 0 */
> >
> >                         pcie-switch-1-downstream-port-1@X,0 {
> >                                 reg = ...; /* downstream port 1 at switch specific address */
> >                                 reset-gpios = ...; /* resets card connected to switch's port 1 */
> >                                 max-link-speed = <1> /* link between this port and card is GEN1 */
> >
> >                                 /* optional node for endpoint card */
> >                                 /* pcie-card@0,0 { ... }; */
> >                         };
> >
> >                         pcie-switch-1-downstream-port-2@Y,0 {
> >                                 reg = ...; /* downstream port 2 at switch specific address */
> >                                 reset-gpios = ...; /* resets card connected to switch's port 2 */
> >                                 max-link-speed = <1> /* link between this port and card is GEN1 */
> >
> >                                 /* optional node for endpoint card */
> >                                 /* pcie-card@0,0 { ... }; */
> >                         };
> >                 };
> >         };
> >
> >         pcie-root-port-2@1,0 {
> >                 reg = <0x00000800 0 0 0 0>; /* root port at device 1 */
> >                 reset-gpios = ...; /* resets card connected to root-port-2 */
> >                 max-link-speed = <2> /* link between root-port-2 and card below is just GEN2 */
> >
> >                 /* optional node for endpoint card */
> >                 /* pcie-card@0,0 { ... }; */
> >         };
> > };
> >
> > Any opinion?