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

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

[Andrew Lunn]

Hi Adam

> If kernel has to bring up two Ethernet interfaces, the processor has two
> peripherals with functionality of MACs (in i.MX6ULL these are Fast Ethernet
> Controllers, FECs), but uses a shared MDIO bus, then the kernel first probes
> one MAC, enables clock for its PHY, probes MDIO bus tryng to discover _all_
> PHYs, and then probes the second MAC, and enables clock for its PHY. The
> result is that the second PHY is still inactive during PHY discovery. Thus,
> one Ethernet interface is not functional.

What clock are you talking about? Do you have the FEC feeding a 50MHz
clock to the PHY? Each FEC providing its own clock to its own PHY? And
are you saying a PHY without its reference clock does not respond to
MDIO reads and hence the second PHY does not probe because it has no
reference clock?

	  Andrew

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

[Adam Rudziński]

Hi Andrew.

W dniu 2020-08-29 o 00:28, Andrew Lunn pisze:
> Hi Adam
>
>> If kernel has to bring up two Ethernet interfaces, the processor has two
>> peripherals with functionality of MACs (in i.MX6ULL these are Fast Ethernet
>> Controllers, FECs), but uses a shared MDIO bus, then the kernel first probes
>> one MAC, enables clock for its PHY, probes MDIO bus tryng to discover _all_
>> PHYs, and then probes the second MAC, and enables clock for its PHY. The
>> result is that the second PHY is still inactive during PHY discovery. Thus,
>> one Ethernet interface is not functional.
> What clock are you talking about? Do you have the FEC feeding a 50MHz
> clock to the PHY? Each FEC providing its own clock to its own PHY? And
> are you saying a PHY without its reference clock does not respond to
> MDIO reads and hence the second PHY does not probe because it has no
> reference clock?
>
> 	  Andrew

Yes, exactly. In my case the PHYs are LAN8720A, and it works this way.
Maybe this problem is PHY-related.
I also think that my proposition allows to make device tree look better 
and be less confusing, as a side-effect.

Best regards,
Adam

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

[Andrew Lunn]

On Sat, Aug 29, 2020 at 12:34:05AM +0200, Adam Rudziński wrote:
> Hi Andrew.
> 
> W dniu 2020-08-29 o 00:28, Andrew Lunn pisze:
> > Hi Adam
> > 
> > > If kernel has to bring up two Ethernet interfaces, the processor has two
> > > peripherals with functionality of MACs (in i.MX6ULL these are Fast Ethernet
> > > Controllers, FECs), but uses a shared MDIO bus, then the kernel first probes
> > > one MAC, enables clock for its PHY, probes MDIO bus tryng to discover _all_
> > > PHYs, and then probes the second MAC, and enables clock for its PHY. The
> > > result is that the second PHY is still inactive during PHY discovery. Thus,
> > > one Ethernet interface is not functional.
> > What clock are you talking about? Do you have the FEC feeding a 50MHz
> > clock to the PHY? Each FEC providing its own clock to its own PHY? And
> > are you saying a PHY without its reference clock does not respond to
> > MDIO reads and hence the second PHY does not probe because it has no
> > reference clock?
> > 
> > 	  Andrew
> 
> Yes, exactly. In my case the PHYs are LAN8720A, and it works this way.

O.K. Boards i've seen like this have both PHYs driver from the first
MAC. Or the clock goes the other way, the PHY has a crystal and it
feeds the FEC.

I would say the correct way to solve this is to make the FEC a clock
provider. It should register its clocks with the common clock
framework. The MDIO bus can then request the clock from the second FEC
before it scans the bus. Or we add the clock to the PHY node so it
enables the clock before probing it. There are people who want this
sort of framework code, to be able to support a GPIO reset, which
needs releasing before probing the bus for the PHY.

Anyway, post your patch, so we get a better idea what you are
proposing.

	Andrew

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

[Adam Rudziński]

W dniu 2020-08-29 o 00:53, Andrew Lunn pisze:
> On Sat, Aug 29, 2020 at 12:34:05AM +0200, Adam Rudziński wrote:
>> Hi Andrew.
>>
>> W dniu 2020-08-29 o 00:28, Andrew Lunn pisze:
>>> Hi Adam
>>>
>>>> If kernel has to bring up two Ethernet interfaces, the processor has two
>>>> peripherals with functionality of MACs (in i.MX6ULL these are Fast Ethernet
>>>> Controllers, FECs), but uses a shared MDIO bus, then the kernel first probes
>>>> one MAC, enables clock for its PHY, probes MDIO bus tryng to discover _all_
>>>> PHYs, and then probes the second MAC, and enables clock for its PHY. The
>>>> result is that the second PHY is still inactive during PHY discovery. Thus,
>>>> one Ethernet interface is not functional.
>>> What clock are you talking about? Do you have the FEC feeding a 50MHz
>>> clock to the PHY? Each FEC providing its own clock to its own PHY? And
>>> are you saying a PHY without its reference clock does not respond to
>>> MDIO reads and hence the second PHY does not probe because it has no
>>> reference clock?
>>>
>>> 	  Andrew
>> Yes, exactly. In my case the PHYs are LAN8720A, and it works this way.
> O.K. Boards i've seen like this have both PHYs driver from the first
> MAC. Or the clock goes the other way, the PHY has a crystal and it
> feeds the FEC.
>
> I would say the correct way to solve this is to make the FEC a clock
> provider. It should register its clocks with the common clock
> framework. The MDIO bus can then request the clock from the second FEC
> before it scans the bus. Or we add the clock to the PHY node so it
> enables the clock before probing it. There are people who want this
> sort of framework code, to be able to support a GPIO reset, which
> needs releasing before probing the bus for the PHY.
>
> Anyway, post your patch, so we get a better idea what you are
> proposing.
>
> 	Andrew

Hm, this sounds reasonable, but complicated at the same time. I have 
spent some time searching for possible solution and never found anything 
teaching something similar, so I'd also speculate that it's kind of not 
very well documented. That doesn't mean I'm against these solutions, 
just that seems to be beyond capabilities of many mortals who even try 
to read.

OK, so a patch it is. Please, let me know how to make the patch so that 
it was useful and as convenient as possible for you. Would you like me 
to use some specific code/repo/branch/... as its base?

Best regards,
Adam

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

[Florian Fainelli]

On 8/28/2020 4:14 PM, Adam Rudziński wrote:
> W dniu 2020-08-29 o 00:53, Andrew Lunn pisze:
>> On Sat, Aug 29, 2020 at 12:34:05AM +0200, Adam Rudziński wrote:
>>> Hi Andrew.
>>>
>>> W dniu 2020-08-29 o 00:28, Andrew Lunn pisze:
>>>> Hi Adam
>>>>
>>>>> If kernel has to bring up two Ethernet interfaces, the processor 
>>>>> has two
>>>>> peripherals with functionality of MACs (in i.MX6ULL these are Fast 
>>>>> Ethernet
>>>>> Controllers, FECs), but uses a shared MDIO bus, then the kernel 
>>>>> first probes
>>>>> one MAC, enables clock for its PHY, probes MDIO bus tryng to 
>>>>> discover _all_
>>>>> PHYs, and then probes the second MAC, and enables clock for its 
>>>>> PHY. The
>>>>> result is that the second PHY is still inactive during PHY 
>>>>> discovery. Thus,
>>>>> one Ethernet interface is not functional.
>>>> What clock are you talking about? Do you have the FEC feeding a 50MHz
>>>> clock to the PHY? Each FEC providing its own clock to its own PHY? And
>>>> are you saying a PHY without its reference clock does not respond to
>>>> MDIO reads and hence the second PHY does not probe because it has no
>>>> reference clock?
>>>>
>>>>       Andrew
>>> Yes, exactly. In my case the PHYs are LAN8720A, and it works this way.
>> O.K. Boards i've seen like this have both PHYs driver from the first
>> MAC. Or the clock goes the other way, the PHY has a crystal and it
>> feeds the FEC.
>>
>> I would say the correct way to solve this is to make the FEC a clock
>> provider. It should register its clocks with the common clock
>> framework. The MDIO bus can then request the clock from the second FEC
>> before it scans the bus. Or we add the clock to the PHY node so it
>> enables the clock before probing it. There are people who want this
>> sort of framework code, to be able to support a GPIO reset, which
>> needs releasing before probing the bus for the PHY.
>>
>> Anyway, post your patch, so we get a better idea what you are
>> proposing.
>>
>>     Andrew
> 
> Hm, this sounds reasonable, but complicated at the same time. I have 
> spent some time searching for possible solution and never found anything 
> teaching something similar, so I'd also speculate that it's kind of not 
> very well documented. That doesn't mean I'm against these solutions, 
> just that seems to be beyond capabilities of many mortals who even try 
> to read.
> 
> OK, so a patch it is. Please, let me know how to make the patch so that 
> it was useful and as convenient as possible for you. Would you like me 
> to use some specific code/repo/branch/... as its base?

This is targeting the net-next tree, see the netdev-FAQ here for details:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/networking/netdev-FAQ.rst

I will be posting some patches for our ARCH_BRCMSTB platforms which 
require that we turn on the internal PHY's digital clock otherwise it 
does not respond on the MDIO bus and we cannot discover its ID and we 
cannot bind to a PHY driver. I will make sure to copy you so you can see 
if this would work for you.
-- 
Florian

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

[Adam Rudziński]

[-- Attachment #1: Type: text/plain, Size: 11011 bytes --]

W dniu 2020-08-29 o 05:29, Florian Fainelli pisze:
>
> On 8/28/2020 4:14 PM, Adam Rudziński wrote:
>> W dniu 2020-08-29 o 00:53, Andrew Lunn pisze:
>>> On Sat, Aug 29, 2020 at 12:34:05AM +0200, Adam Rudziński wrote:
>>>> Hi Andrew.
>>>>
>>>> W dniu 2020-08-29 o 00:28, Andrew Lunn pisze:
>>>>> Hi Adam
>>>>>
>>>>>> If kernel has to bring up two Ethernet interfaces, the processor 
>>>>>> has two
>>>>>> peripherals with functionality of MACs (in i.MX6ULL these are 
>>>>>> Fast Ethernet
>>>>>> Controllers, FECs), but uses a shared MDIO bus, then the kernel 
>>>>>> first probes
>>>>>> one MAC, enables clock for its PHY, probes MDIO bus tryng to 
>>>>>> discover _all_
>>>>>> PHYs, and then probes the second MAC, and enables clock for its 
>>>>>> PHY. The
>>>>>> result is that the second PHY is still inactive during PHY 
>>>>>> discovery. Thus,
>>>>>> one Ethernet interface is not functional.
>>>>> What clock are you talking about? Do you have the FEC feeding a 50MHz
>>>>> clock to the PHY? Each FEC providing its own clock to its own PHY? 
>>>>> And
>>>>> are you saying a PHY without its reference clock does not respond to
>>>>> MDIO reads and hence the second PHY does not probe because it has no
>>>>> reference clock?
>>>>>
>>>>>       Andrew
>>>> Yes, exactly. In my case the PHYs are LAN8720A, and it works this way.
>>> O.K. Boards i've seen like this have both PHYs driver from the first
>>> MAC. Or the clock goes the other way, the PHY has a crystal and it
>>> feeds the FEC.
>>>
>>> I would say the correct way to solve this is to make the FEC a clock
>>> provider. It should register its clocks with the common clock
>>> framework. The MDIO bus can then request the clock from the second FEC
>>> before it scans the bus. Or we add the clock to the PHY node so it
>>> enables the clock before probing it. There are people who want this
>>> sort of framework code, to be able to support a GPIO reset, which
>>> needs releasing before probing the bus for the PHY.
>>>
>>> Anyway, post your patch, so we get a better idea what you are
>>> proposing.
>>>
>>>     Andrew
>>
>> Hm, this sounds reasonable, but complicated at the same time. I have 
>> spent some time searching for possible solution and never found 
>> anything teaching something similar, so I'd also speculate that it's 
>> kind of not very well documented. That doesn't mean I'm against these 
>> solutions, just that seems to be beyond capabilities of many mortals 
>> who even try to read.
>>
>> OK, so a patch it is. Please, let me know how to make the patch so 
>> that it was useful and as convenient as possible for you. Would you 
>> like me to use some specific code/repo/branch/... as its base?
>
> This is targeting the net-next tree, see the netdev-FAQ here for details:
>
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/networking/netdev-FAQ.rst 
>
>
> I will be posting some patches for our ARCH_BRCMSTB platforms which 
> require that we turn on the internal PHY's digital clock otherwise it 
> does not respond on the MDIO bus and we cannot discover its ID and we 
> cannot bind to a PHY driver. I will make sure to copy you so you can 
> see if this would work for you.

Actually, I came to conclusion, that sending a patch for discussion is 
not the best idea at this time, because the original code for my kernel 
was from here:

https://github.com/SoMLabs/somlabs-linux-imx/tree/imx_4.19.35_1.1.0
commit ef35d67fb

and although it still looks similar in the kernel, I've assumed that now 
the safest way is to just attach the original and modified files. So 
here they go, with some discussion below. I hope this doesn't make a 
mess and is still good.


The essential general modification is in

drivers/of/of_mdio.c
include/linux/of_mdio.h

and to make the driver profit on that, target-specific (well, the driver 
is target-specific) modification in

drivers/net/ethernet/freescale/fec_main.c


Modification in of_mdio.c

"Original" function of_mdiobus_register contains a loop over all child 
nodes.

/**
  * of_mdiobus_register - Register mii_bus and create PHYs from the 
device tree
  * @mdio: pointer to mii_bus structure
  * @np: pointer to device_node of MDIO bus.
  *
  * This function registers the mii_bus structure and registers a phy_device
  * for each child node of @np.
  */
int of_mdiobus_register(struct mii_bus *mdio, struct device_node *np)
{
(...)
         /* Register the MDIO bus */
         rc = mdiobus_register(mdio);
         if (rc)
                 return rc;

         /* Loop over the child nodes and register a phy_device for each 
phy */
         for_each_available_child_of_node(np, child) {
                 addr = of_mdio_parse_addr(&mdio->dev, child);
                 if (addr < 0) {
                         scanphys = true;
                         continue;
                 }

                 if (of_mdiobus_child_is_phy(child))
                         rc = of_mdiobus_register_phy(mdio, child, addr);
                 else
                         rc = of_mdiobus_register_device(mdio, child, addr);

                 if (rc == -ENODEV)
                         dev_err(&mdio->dev,
                                 "MDIO device at address %d is missing.\n",
                                 addr);
                 else if (rc)
                         goto unregister;
         }

         if (!scanphys)
                 return 0;
(...)
}

The loop is preceeded by mdiobus_register, so this provides service only 
for a new MDIO bus. It is not possible to later add more child nodes. 
Therefore the driver has to know all the child nodes before it registers 
the bus. The device tree looks more or less like this:

&fec2 {
(...)
     mdio {
         (all PHYs here)
     };
(...)
};

&fec1 {
    (some stuff, but no mdio here)
};

The driver (fec_main.c) for the second FEC has to only set the pointer 
to the shared MDIO bus, but also cannot do anything more:

         if ((fep->quirks & FEC_QUIRK_SINGLE_MDIO) && fep->dev_id > 0) {
                 /* fec1 uses fec0 mii_bus */
                 if (mii_cnt && fec0_mii_bus) {
                         fep->mii_bus = fec0_mii_bus;
                         *fec_mii_bus_share = FEC0_MII_BUS_SHARE_TRUE;
                         mii_cnt++;
                         return 0;
                 }
                 return -ENOENT;
         }

I propose to get the loop out of of_mdiobus_register and make it a new 
public function (with prototype in of_mdio.h; most likely it makes sense 
to add checking if pointers are not NULL).

/**
  * of_mdiobus_register - Register mii_bus and create PHYs from the 
device tree
  * @mdio: pointer to mii_bus structure
  * @np: pointer to device_node of MDIO bus.
  *
  * This function registers the mii_bus structure and registers a phy_device
  * for each child node of @np.
  */
int of_mdiobus_register(struct mii_bus *mdio, struct device_node *np)
{
(...)
         rc = mdiobus_register(mdio);
         if (rc)
                 return rc;

         rc = of_mdiobus_register_children(mdio, np, &scanphys);
         if (rc)
                 goto unregister;

         if (!scanphys)
                 return 0;
(...)
}

/**
  * of_mdiobus_register_children - Create PHYs from the device tree
  * @mdio: pointer to mii_bus structure
  * @np: pointer to device_node of MDIO bus.
  * @scanphys: pointer to boolean variable telling if PHYs with
  *            empty reg property should be scanned by the calling function
  *            or NULL if this is information is not needed
  *
  * This function registers a phy_device for each child node of @np.
  */
int of_mdiobus_register_children(struct mii_bus *mdio, struct 
device_node *np, bool *scanphys)
{
         struct device_node *child;
         int addr, rc;

         /* Loop over the child nodes and register a phy_device for each 
phy */
         for_each_available_child_of_node(np, child) {
                 addr = of_mdio_parse_addr(&mdio->dev, child);
                 if (addr < 0) {
                         if (scanphys) { *scanphys = true; }
                         continue;
                 }

                 if (of_mdiobus_child_is_phy(child))
                         rc = of_mdiobus_register_phy(mdio, child, addr);
                 else
                         rc = of_mdiobus_register_device(mdio, child, addr);
                 if (rc == -ENODEV)
                         dev_err(&mdio->dev,
                                 "MDIO device at address %d is missing.\n",
                                 addr);
                 else if (rc)
                         return rc;
         }

         return 0;
}

The driver would be able to add the new PHYs to the shared MDIO bus by 
calling of_mdiobus_register_children. Then the device tree looks like 
this, which is more reasonable in my opinion:

&fec2 {
(...)
     mdio {
         (phy for fec2 here)
     };
(...)
};

&fec1 {
(...)
     mdio {
         (phy for fec1 here)
     };
(...)
};

The driver would be able to add the new PHYs to the shared MDIO bus by 
calling of_mdiobus_register_children.

         if ((fep->quirks & FEC_QUIRK_SINGLE_MDIO) && fep->dev_id > 0) {
                 /* fec1 uses fec0 mii_bus */
                 if (mii_cnt && fec0_mii_bus) {
                         fep->mii_bus = fec0_mii_bus;
                         *fec_mii_bus_share = FEC0_MII_BUS_SHARE_TRUE;
                         mii_cnt++;
                         node = of_get_child_by_name(pdev->dev.of_node, 
"mdio");
                         return 
of_mdiobus_register_children(fep->mii_bus, node, NULL);
                 }
                 return -ENOENT;
         }

This could be also done later "dynamically" on runtime.

Best regards,
Adam


[-- Attachment #2: original.tar.bz2 --]
[-- Type: application/octet-stream, Size: 27747 bytes --]

[-- Attachment #3: proposed.tar.bz2 --]
[-- Type: application/octet-stream, Size: 27900 bytes --]

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

[Andrew Lunn]

> The driver would be able to add the new PHYs to the shared MDIO bus by
> calling of_mdiobus_register_children. Then the device tree looks like this,
> which is more reasonable in my opinion:
> 
> &fec2 {
> (...)
>     mdio {
>         (phy for fec2 here)
>     };
> (...)
> };
> 
> &fec1 {
> (...)
>     mdio {
>         (phy for fec1 here)
>     };
> (...)
> };

DT describes hardware, and the topology of the hardware. The hardware really is:

ethernet1@83fec000 {
	compatible = "fsl,imx51-fec", "fsl,imx27-fec";
	reg = <0x83fec000 0x4000>;
	interrupts = <87>;
	phy-mode = "mii";
	phy-reset-gpios = <&gpio2 14 GPIO_ACTIVE_LOW>; /* GPIO2_14 */
	local-mac-address = [00 04 9F 01 1B B9];
	phy-supply = <&reg_fec_supply>;
	phy-handle = <&ethphy1>;
	mdio {
	        clock-frequency = <5000000>;
		ethphy1: ethernet-phy@1 {
			compatible = "ethernet-phy-ieee802.3-c22";
			reg = <1>;
			max-speed = <100>;
		};
		ethphy2: ethernet-phy@2 {
			compatible = "ethernet-phy-ieee802.3-c22";
			reg = <2>;
			max-speed = <100>;
		};
	};
};

ethernet2@84fec000 {
	compatible = "fsl,imx51-fec", "fsl,imx27-fec";
	reg = <0x83fec000 0x4000>;
	interrupts = <87>;
	phy-mode = "mii";
	phy-reset-gpios = <&gpio2 15 GPIO_ACTIVE_LOW>; /* GPIO2_15 */
	local-mac-address = [00 04 9F 01 1B BA];
	phy-supply = <&reg_fec_supply>;
	phy-handle = <&ethphy2>;
};

What is missing from this is clocks. The IMX has a central clock
provider:

                        clks: clock-controller@20c4000 {
                                compatible = "fsl,imx6ul-ccm";
                                reg = <0x020c4000 0x4000>;
                                interrupts = <GIC_SPI 87 IRQ_TYPE_LEVEL_HIGH>,
                                             <GIC_SPI 88 IRQ_TYPE_LEVEL_HIGH>;
                                #clock-cells = <1>;
                                clocks = <&ckil>, <&osc>, <&ipp_di0>, <&ipp_di1>;
                                clock-names = "ckil", "osc", "ipp_di0", "ipp_di1";
                        };

and it exports two clocks, MX6UL_CLK_ENET1_REF, MX6UL_CLK_ENET2_REF

So adding the clock properties:

ethernet1@83fec000 {
	compatible = "fsl,imx51-fec", "fsl,imx27-fec";
	reg = <0x83fec000 0x4000>;
	interrupts = <87>;
	phy-mode = "mii";
	phy-reset-gpios = <&gpio2 14 GPIO_ACTIVE_LOW>; /* GPIO2_14 */
	local-mac-address = [00 04 9F 01 1B B9];
	phy-supply = <&reg_fec_supply>;
	phy-handle = <&ethphy1>;
	mdio {
	        clock-frequency = <5000000>;
		ethphy1: ethernet-phy@1 {
			compatible = "ethernet-phy-ieee802.3-c22";
			reg = <1>;
			max-speed = <100>;
			clocks = <&clks MX6UL_CLK_ENET1_REF>;
		};
		ethphy2: ethernet-phy@2 {
			compatible = "ethernet-phy-ieee802.3-c22";
			reg = <2>;
			max-speed = <100>;
			clocks = <&clks MX6UL_CLK_ENET2_REF>;
		};
	};
};

ethernet2@84fec000 {
	compatible = "fsl,imx51-fec", "fsl,imx27-fec";
	reg = <0x83fec000 0x4000>;
	interrupts = <87>;
	phy-mode = "mii";
	phy-reset-gpios = <&gpio2 15 GPIO_ACTIVE_LOW>; /* GPIO2_15 */
	local-mac-address = [00 04 9F 01 1B BA];
	phy-supply = <&reg_fec_supply>;
	phy-handle = <&ethphy2>;
};

Also look at drivers/net/phy/micrel.c. It has code to look up a FEC
clock and use it. But that code assumes the PHY responds to MDIO reads
when the clock is not ticking. It sounds like your PHY does not?
Please double check that. If it does not, you need to add clock code
to the PHY core. Florians patchset will help with that.

	Andrew

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

[Adam Rudziński]

W dniu 2020-08-29 o 17:15, Andrew Lunn pisze:
>> The driver would be able to add the new PHYs to the shared MDIO bus by
>> calling of_mdiobus_register_children. Then the device tree looks like this,
>> which is more reasonable in my opinion:
>>
>> &fec2 {
>> (...)
>>      mdio {
>>          (phy for fec2 here)
>>      };
>> (...)
>> };
>>
>> &fec1 {
>> (...)
>>      mdio {
>>          (phy for fec1 here)
>>      };
>> (...)
>> };
> DT describes hardware, and the topology of the hardware. The hardware really is:
>
> ethernet1@83fec000 {
> 	compatible = "fsl,imx51-fec", "fsl,imx27-fec";
> 	reg = <0x83fec000 0x4000>;
> 	interrupts = <87>;
> 	phy-mode = "mii";
> 	phy-reset-gpios = <&gpio2 14 GPIO_ACTIVE_LOW>; /* GPIO2_14 */
> 	local-mac-address = [00 04 9F 01 1B B9];
> 	phy-supply = <&reg_fec_supply>;
> 	phy-handle = <&ethphy1>;
> 	mdio {
> 	        clock-frequency = <5000000>;
> 		ethphy1: ethernet-phy@1 {
> 			compatible = "ethernet-phy-ieee802.3-c22";
> 			reg = <1>;
> 			max-speed = <100>;
> 		};
> 		ethphy2: ethernet-phy@2 {
> 			compatible = "ethernet-phy-ieee802.3-c22";
> 			reg = <2>;
> 			max-speed = <100>;
> 		};
> 	};
> };
>
> ethernet2@84fec000 {
> 	compatible = "fsl,imx51-fec", "fsl,imx27-fec";
> 	reg = <0x83fec000 0x4000>;
> 	interrupts = <87>;
> 	phy-mode = "mii";
> 	phy-reset-gpios = <&gpio2 15 GPIO_ACTIVE_LOW>; /* GPIO2_15 */
> 	local-mac-address = [00 04 9F 01 1B BA];
> 	phy-supply = <&reg_fec_supply>;
> 	phy-handle = <&ethphy2>;
> };

This is true assuming that the PHYs are always and forever connected to 
one specific MDIO bus. This is probably reasonable. Although, in i.MX 
the MDIO bus of FEC1 and FEC2 shares the pins. So with this "split" 
description one can just comment out FEC2 and still enjoy operational 
ethernet (FEC1 + its PHY). This may be simpler for hardware guys, like 
me, who don't have that much experience with device trees. But yes, that 
doesn't necessarily mean it's the good way to go.

> Also look at drivers/net/phy/micrel.c. It has code to look up a FEC
> clock and use it. But that code assumes the PHY responds to MDIO reads
> when the clock is not ticking. It sounds like your PHY does not?
> Please double check that. If it does not, you need to add clock code
> to the PHY core. Florians patchset will help with that.
>
I'm sure of that - LAN8720A needs to have the clock from FEC or external 
generator to be able to talk over MDIO.

I can see that one way or another, it's still some modification of the 
source code. You know the ropes, you decide if and which one makes sense.

Best regards,
Adam

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

[Andrew Lunn]

> This is true assuming that the PHYs are always and forever connected to one
> specific MDIO bus. This is probably reasonable. Although, in i.MX the MDIO
> bus of FEC1 and FEC2 shares the pins.

In general, they do not. In fact, i don't see how that can work. The
FEC drive provides no mutual exclusion between MDIO operations on
different MDIO controllers. So one controller could be performing a
read while the other a write. If they are sharing the same pins, how
do you drive the clock pin both high and low at the same time? How do
you have the data pin both high impedance so the PHY can drive it, and
also drive out a 0 or a 1 to perform a right?

What is suspect you can do is use pinmux to connect the pins to either
ethernet1 MDIO controller, or ethernet2 mdio controller. But never
both. You have to decide which gets to control the bus, and the other
controller is isolated.

> I'm sure of that - LAN8720A needs to have the clock from FEC or external
> generator to be able to talk over MDIO.

O.K. Then you need to core to enable the clock before scanning the
bus.

	Andrew

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

[Adam Rudziński]

W dniu 2020-08-29 o 18:00, Andrew Lunn pisze:
>> This is true assuming that the PHYs are always and forever connected to one
>> specific MDIO bus. This is probably reasonable. Although, in i.MX the MDIO
>> bus of FEC1 and FEC2 shares the pins.
> In general, they do not. In fact, i don't see how that can work. The
> FEC drive provides no mutual exclusion between MDIO operations on
> different MDIO controllers.
> (...)
> You have to decide which gets to control the bus, and the other
> controller is isolated.

Yes, true, but that was not what I meant. In i.MX6ULL MDIO buses of both 
FECs share the same pins, but of course only one can be active. It makes 
no sense to try to use both MDIO buses if the PHYs are connected to the 
same wires. I agree, the current state is good from this point of view.

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.
Here, at least the FEC driver will automatically use MDIO bus from FEC2 
for all the PHYs.

Now suppose that I want to have a cheaper version, with only one 
Ethernet interface on board:
//&fec2 {
//...
//    mdio { phy2 ... };
//...
//};
&fec1 {
...
     mdio { phy1 ... };
...
};
Now, the system will use MDIO bus from FEC1.

This is just less playing with the device tree. But I agree, that you 
don't do that frequently, so that doesn't save much effort.
And, what this example doesn't show, is that the pin assignment has to 
be modfied - mdio pins have to get to fec1 now.

This way of building the device tree results from my proposition, but it 
was not my motivation.

Maybe it would be more useful if the device tree would be preprocessed 
and then compiled based on some conditions:

#if defined(CONFIG_USE_FEC2)
&fec2 {
...
     mdio { phy2 ... };
...
};
#endif
#if defined(CONFIG_USE_FEC1)
&fec1 {
...
     mdio { phy1 ... };
...
};
#endif

Best regards,
Adam

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

[Andrew Lunn]

> 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

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

[Adam Rudziński]

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