Re: [PATCH v7 06/10] dt-bindings: phy: Add bindings for HiKey 970 PCIe PHY

[Mauro Carvalho Chehab <mchehab+huawei@kernel.org>]

Em Tue, 27 Jul 2021 01:50:20 +0200
Mauro Carvalho Chehab <mchehab+huawei@kernel.org> escreveu:

> Em Mon, 26 Jul 2021 15:37:28 -0600
> Rob Herring <robh@kernel.org> escreveu:
> 

> > > > > +  reset-gpios:
> > > > > +    description: PCI PERST reset GPIOs
> > > > > +    maxItems: 4
> > > > > +
> > > > > +  clkreq-gpios:
> > > > > +    description: Clock request GPIOs
> > > > > +    maxItems: 3    
> > > >
> > > > Again, this will not work.    
> > >
> > > Just to be sure: you're talking about the PERST# gpios (e. g. reset-gpios)
> > > here, right?    
> > 
> > Both that and CLKREQ.

The original DT from the downstream version (found at Linaro's tree)
has:

	pcie@f4000000 {
		compatible = "hisilicon,hikey970";
...
		switch,reset-gpios = <&gpio7 0 0 >;
		eth,reset-gpios = <&gpio25 2 0 >;
		m_2,reset-gpios = <&gpio3 1 0 >;
		mini1,reset-gpios = <&gpio27 4 0 >;

		eth,clkreq-gpios = <&gpio20 6 0 >;
		m_2,clkreq-gpios = <&gpio27 3 0 >;
		mini1,clkreq-gpios = <&gpio17 0 0 >;
	};

So, if we're willing to have a single reset-gpios for the PCIe
interface, in order to follow the current pci-bus.yaml schema,
this would probably be:

	reset-gpios = <&gpio7 0 0 >;

which maps to the PEX8606 PCIe bridge chip.

With that, DT still need to point a per-slot clkreq and
reset-gpio.

One alternative would be to map it as either 3 PCI or PHY
child nodes. E. g. something like this:

	pcie@f4000000 {
		compatible = "hisilicon,kirin970-pcie";
...
		reset-gpios = <&gpio7 0 0 >;

		slot {
			eth {
				reset-gpios = <&gpio25 2 0>;
				clkreq-gpios = <&gpio20 6 0>;
			};
			m2 {
				reset-gpios = <&gpio3 1 0>;
				clkreq-gpios = <&gpio27 3 0>;
			};
			mini1 {
				reset-gpios = <&gpio27 4 0>;
				clkreq-gpios = <&gpio17 0 0>;
			};
		};
	};


Placing the child nodes ("slot"?) at the pci bus properties makes more
sense to me, but placing them at the PHY node has the advantage of 
only affecting Kirin 970.

In either case, if each child would need a different power supply,
it won't be hard to add a "slot-supply" property later on. 

Would something like that be acceptable for you?

> > > If you have a better idea, I'm all ears.    
> > 
> > There's already a spec for populating PCI devices in DT. It's existed
> > for over 20 years with OpenFirmware[1]. It's not widely used on FDT
> > systems because most cases to date are just a single device attached
> > and they don't have extra things needing to be described in DT. There
> > are a few, but not many examples in the tree of PCI devices with DT
> > nodes. That's the only way to generically describe the topology you
> > have.  
> >
> > [1] https://www.devicetree.org/open-firmware/home.html#OFDbussupps  

I was unable to find anything useful there at the two PCI documents.

This one:
	https://www.devicetree.org/open-firmware/bindings/pci/pci-express.txt

has just one property that might be useful:

	physical-slot#

The main one:
	https://www.devicetree.org/open-firmware/bindings/pci/pci2_1.pdf

mentions a few child properties, but it doesn't show how those were
supposed to be mapped, and none of the properties mentioned there
specify clocks, gpios, or reset pins.

Thanks,
Mauro