Re: drivers/of/of_mdio.c needs a small modification

["Adam Rudziński" <adam.rudzinski@arf.net.pl>]

W dniu 2020-08-30 o 01:16, Andrew Lunn pisze:
>> I meant that with the split description of the mdio node the mdio bus for
>> use in the system would be selected almost automatically. Suppose that I can
>> do the device tree "my way":
>> &fec2 {
>> ...
>>      mdio { phy2 ... };
>> ...
>> };
>> &fec1 {
>> ...
>>      mdio { phy1 ... };
>> ...
>> };
>> This emphasizes which PHY is intended for use by which FEC, that's why it
>> looks more natural for me.
> And it looks really wrong to me. It suggests there are two busses, and
> each PHY is on its own bus. When in fact there is one MDIO bus with
> two PHYs. Device tree should represents the real hardware, not some
> pseudo description.
>
>       Andrew

Sure, the "split" variant may cause a misleading first impression for a 
human reader. Similarly, one might argue that having both PHYs under one 
mdio node suggests that both PHYs are connected to the same FEC. One way 
or another, the device tree is a pretty complex thing and requires some 
effort to read it correctly. The important thing is if the kernel is 
getting the correct information.

The discussion got a bit off-topic, though. I'm not advocating any 
particular structure of the device tree, nor I'm saying any should be 
supported, as long the existing standard(s) make(s) it possible to do 
their job(s).

Getting back to the original problem, I have tried the solution with 
clocks defined under phy nodes, and it didn't work. eth0 was up, but 
eth1 again faced "fec 2188000.ethernet eth1: Unable to connect to phy". 
Details below, maybe it was my fault.

imx6ull.dtsi defines:

                         fec1: ethernet@2188000 {
                                 compatible = "fsl,imx6ul-fec", 
"fsl,imx6q-fec";
                                 reg = <0x2188000 0x4000>;
                                 interrupts = <GIC_SPI 118 
IRQ_TYPE_LEVEL_HIGH>,
                                              <GIC_SPI 119 
IRQ_TYPE_LEVEL_HIGH>;
                                 clocks = <&clks IMX6UL_CLK_ENET>,
                                          <&clks IMX6UL_CLK_ENET_AHB>,
                                          <&clks IMX6UL_CLK_ENET_PTP>,
                                          <&clks IMX6UL_CLK_ENET_REF>,
                                          <&clks IMX6UL_CLK_ENET_REF>;
                                 clock-names = "ipg", "ahb", "ptp",
                                               "enet_clk_ref", "enet_out";
                                 stop-mode = <&gpr 0x10 3>;
                                 fsl,num-tx-queues=<1>;
                                 fsl,num-rx-queues=<1>;
                                 fsl,magic-packet;
                                 fsl,wakeup_irq = <0>;
                                 status = "disabled";
                         };

                        fec2: ethernet@20b4000 {
                                 compatible = "fsl,imx6ul-fec", 
"fsl,imx6q-fec";
                                 reg = <0x20b4000 0x4000>;
                                 interrupts = <GIC_SPI 120 
IRQ_TYPE_LEVEL_HIGH>,
                                              <GIC_SPI 121 
IRQ_TYPE_LEVEL_HIGH>;
                                 clocks = <&clks IMX6UL_CLK_ENET>,
                                          <&clks IMX6UL_CLK_ENET_AHB>,
                                          <&clks IMX6UL_CLK_ENET_PTP>,
                                          <&clks IMX6UL_CLK_ENET2_REF_125M>,
                                          <&clks IMX6UL_CLK_ENET2_REF_125M>;
                                 clock-names = "ipg", "ahb", "ptp",
                                               "enet_clk_ref", "enet_out";
                                 stop-mode = <&gpr 0x10 4>;
                                 fsl,num-tx-queues=<1>;
                                 fsl,num-rx-queues=<1>;
                                 fsl,magic-packet;
                                 fsl,wakeup_irq = <0>;
                                 status = "disabled";
                         };

so in my top-level dts file (which includes imx6ull.dtsi) I've tried:

&fec1 {
         pinctrl-names = "default";
         pinctrl-0 = <&pinctrl_enet1>;
         phy-mode = "rmii";
         phy-handle = <&ethphy0>;
         status = "okay";
};

&fec2 {
         pinctrl-names = "default";
         pinctrl-0 = <&pinctrl_enet2>, <&pinctrl_enet2_mdio>;
         phy-mode = "rmii";
         phy-handle = <&ethphy1>;
         status = "okay";

         mdio {
                 #address-cells = <1>;
                 #size-cells = <0>;

                 ethphy0: ethernet-phy@0 {
                         reg = <0>;
                         clocks = <&clks IMX6UL_CLK_ENET_REF>;
                 };

                 ethphy1: ethernet-phy@1 {
                         reg = <1>;
                         clocks = <&clks IMX6UL_CLK_ENET2_REF_125M>;
                 };
         };
};

Adding compatible = "..." and max-speed = "..." didn't change anything.

Please, let me know if I omitted something important in the test, and if 
I should repeat it with amended device tree.

Best regards,
Adam