Re: [PATCH net-next 12/12] arm64: dts: qcom: qcs6490-rb3gen2: enable TC9564 with a single QCS8081 phy

[Andrew Lunn <andrew@lunn.ch>]

M> I’m afraid this must be necessarily long but to help navigate the
> general structure is:

Thanks for the long email.

> Let’s turn our attention to MDIO.
> 
> Following the pattern above where all the links related to power come
> from the main device node, then the phy node for the qca8081 in an
> rb3gen2 would look like something like this:
> 
>     tc956x_emac1_phy: ethernet-phy@1c {
>         compatible = "ethernet-phy-id004d.d101";
>         reg = <0x1c>;
> 
>         reset-gpios = <&tc956x_emac0 1 GPIO_ACTIVE_LOW>;
>         # On RB3gen2 all supplies are controlled by a single GPIO
>         # so we link all supplies to that single regulator
>         avdd-supply = <&qep_1p8>;
>         avdd18-supply = <&qep_1p8>;
>         vdd-supply = <&qep_1p8>;
>         vddldo-supply = <&qep_1p8>;
>         vdd18-supply = <&qep_1p8>;
>         vdd125-supply = <&qep_1p8>;
> 
>         pinctrl-names = "default";
>         pinctrl-0 = <&qep_irq_pin>;
>         interrupts-extended = <&tlmm 101 IRQ_TYPE_LEVEL_LOW>;
>     };
> 
> Once we have established what the DT looks like then the question
> becomes where to put "something, written in C [or Rust], that contains
> burned in knowledge of how to turn on a wcn6855^H^H^H^H^H^H^Hqca8081"?
> 
> The qca8081 on rb3gen2 just gets a bunch of individual regulators and a
> reset. As in the pci-pwrctrl-tc9563 example above, this means there is
> nothing in the DT for a pwrseq driver to bind to.

I don't understand that. We have a node in device tree for the PHY. We
can put the needed properties there somehow. Maybe similar to how
pinctrl works. Before probing the device, the driver core looks for
the pinctrl- properties, and activates them. We could have similar
properties for power sequencing, before enumerating an address, do
whatever the pwrseq-names requests? It might need help for the driver
actually enumerating the bus, to point to core at the node.

> Even if we could find
> a way to do that, it is not obviously useful to decouple how to turn on
> an MDIO device from how to drive it. Thus I think the right answer to
> that is to put the code to fire up the regulators into the qca808x.c
> driver and it looks to me like the existing probe/remove methods would
> already work perfectly well as the place to put it.

But you have a chicken/egg. Same as with PCI, with Ethernet PHYs, you
ask the device what it is, and then find a driver to drive it, and
then call the probe function. However, if its clock is off, its GPIO
reset is held in reset, its power regulators are switch off, it never
answers when you ask it, what are you? If you have no idea what it is,
you cannot probe it. And if you cannot probe it, the probe function
cannot turn all these things on.

The standard reply we have is, ignore the fact the device can be
enumerated, hard code in DT what its ID is. And that is what you have
above in your DT fragment. But you also need to live with the
consequences of it being wrong, since that driver is going to be
loaded, no questions asked. And don't think about having a rev A board
and a rev B board which have different pin compatible PHYs, and want
to use the enumeration to get the correct driver loaded.

DT developers continually want to make use of the enumeration feature,
since it is there, part of 802.3, it is how it was done for the first
20 years of networking, etc. And they keep trying to extend the
current code to get the PHY running to allow it to be enumerated.  And
i keep rejecting it.

Ideally, i would like some generic core code which can be given a list
of resources, probably phandles, and timing information. And it will
walk the list and turn them on, inserting the requested delays. And
then we can enumerate the device.

Until such code exists, i agree, forget the device is enumerable, hard
code the ID, and get the probe function to enable the regulators, take
the device out of reset, enable the clocks, etc. And repeat that in
every PHY driver.

       Andrew