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@xxxxxxxxxx> 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?