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

[Rob Herring]

On Tue, Jul 27, 2021 at 12:52 AM Mauro Carvalho Chehab
<mchehab+huawei at kernel.org> wrote:
>
> Em Tue, 27 Jul 2021 01:50:20 +0200
> Mauro Carvalho Chehab <mchehab+huawei at kernel.org> escreveu:
>
> > Em Mon, 26 Jul 2021 15:37:28 -0600
> > Rob Herring <robh at 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 at 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 at 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?

On the right track, but there's already a definition for what child
devices look like in pci2_1.pdf. I think you want something like this:

pcie at f4000000 { // RP: Bus 0, Device 0
    compatible = "hisilicon,kirin970-pcie";
    ...
    reset-gpios = <&gpio7 0 0>;  // PERST to switch

    pcie at 0 { // PCIe switch: Bus 1, Device 0
        reg = <0 0 0 0 0>;
        compatible = "pciclass,0604";
        device_type = "pci";

        pcie at 1 { // NC (Can omit this node)
            reg = <0x80 0 0 0 0>;
            compatible = "pciclass,0604";
            device_type = "pci";
        };

        pcie at 4 { // M.2
            reg = <0x200 0 0 0 0>;
            compatible = "pciclass,0604";
            device_type = "pci";
            reset-gpios = <&gpio7 1 0>; // PERST to M.2 slot
       };

        pcie at 5 { // Mini
            reg = <0x280 0 0 0 0>;
            compatible = "pciclass,0604";
            device_type = "pci";
            reset-gpios = <&gpio7 2 0>; // PERST to Mini slot
        };

        pcie at 7 { // Ethernet
            reg = <0x380 0 0 0 0>;
            compatible = "pciclass,0604";
            device_type = "pci";
            reset-gpios = <&gpio7 3 0>; // PERST to Ethernet

            ethernet at 0 {
                reg = <0 0 0 0 0>;
                local-mac-address = [ 00 01 02 03 04 05 06 ];
            };
        };

        pcie at 9 { // NC
            reg = <0x480 0 0 0 0>;
            compatible = "pciclass,0604";
            device_type = "pci";
       };
};

This is based on what you previously sent:
00:00.0 PCI bridge: Huawei Technologies Co., Ltd. Device 3670 (rev 01)
01:00.0 PCI bridge: PLX Technology, Inc. PEX 8606 6 Lane, 6 Port PCI
Express Gen 2 (5.0 GT/s) Switch (rev ba)
02:01.0 PCI bridge: PLX Technology, Inc. PEX 8606 6 Lane, 6 Port PCI
Express Gen 2 (5.0 GT/s) Switch (rev ba)
02:04.0 PCI bridge: PLX Technology, Inc. PEX 8606 6 Lane, 6 Port PCI
Express Gen 2 (5.0 GT/s) Switch (rev ba)
02:05.0 PCI bridge: PLX Technology, Inc. PEX 8606 6 Lane, 6 Port PCI
Express Gen 2 (5.0 GT/s) Switch (rev ba)
02:07.0 PCI bridge: PLX Technology, Inc. PEX 8606 6 Lane, 6 Port PCI
Express Gen 2 (5.0 GT/s) Switch (rev ba)
02:09.0 PCI bridge: PLX Technology, Inc. PEX 8606 6 Lane, 6 Port PCI
Express Gen 2 (5.0 GT/s) Switch (rev ba)
06:00.0 Ethernet controller: Realtek Semiconductor Co., Ltd.
RTL8111/8168/8411 PCI Express Gigabit Ethernet Controller (rev 07)

A few notes:
I left out #size-cells, #address-cells, and ranges for brevity.

I'm not completely sure I've got the bridges mapped to the right
functions on Hikey970. That's my best guess looking at the schematics.
You should be able to confirm which bridge is the parent bridge for
ethernet at least.

The compatible strings aren't strictly needed. Linux doesn't look at them.

There's a pretty complete example in:
arch/mips/boot/dts/loongson/loongson64-2k1000.dtsi

The simplest Linux implementation to handle the above is just walk the
child nodes and get all the 'reset-gpios' properties. That's not the
implementation I think we should have though. We should handle the
GPIO as each bridge is probed and children scanned.

Rob