[PATCH] ARM: imx: imx8mn-ddr4-evk: Add ethernet support

[Stefano Babic]

On 02.03.21 17:11, Marek Vasut wrote:
> On 3/2/21 4:24 PM, Tom Rini wrote:
>> On Tue, Mar 02, 2021 at 03:52:39PM +0100, Marek Vasut wrote:
>>> On 3/2/21 10:16 AM, Michael Nazzareno Trimarchi wrote:
>>>> Hi Marek
>>>>
>>>> On Tue, Mar 2, 2021 at 9:40 AM Marek Vasut <marex at denx.de> wrote:
>>>>>
>>>>> On 3/2/21 9:21 AM, Michael Nazzareno Trimarchi wrote:
>>>>>> Hi Marek
>>>>>>
>>>>>> On Tue, Mar 2, 2021 at 9:13 AM Marek Vasut <marex at denx.de> wrote:
>>>>>>>
>>>>>>> On 3/2/21 8:53 AM, Jagan Teki wrote:
>>>>>>>> On Sat, Feb 27, 2021 at 6:21 PM Marek Vasut <marex at denx.de> wrote:
>>>>>>>>>
>>>>>>>>> On 2/27/21 8:54 AM, Jagan Teki wrote:
>>>>>>>>>> Hi Marek,
>>>>>>>>>>
>>>>>>>>>> On Sat, Feb 27, 2021 at 3:44 AM Marek Vasut <marex at denx.de> 
>>>>>>>>>> wrote:
>>>>>>>>>>>
>>>>>>>>>>> Add support for ethernet on the imx8mn-ddr4-evk.
>>>>>>>>>>>
>>>>>>>>>>> Signed-off-by: Marek Vasut <marex at denx.de>
>>>>>>>>>>> Cc: Fabio Estevam <festevam at gmail.com>
>>>>>>>>>>> Cc: Peng Fan <peng.fan at nxp.com>
>>>>>>>>>>> Cc: Stefano Babic <sbabic at denx.de>
>>>>>>>>>>> ---
>>>>>>>>>>>       arch/arm/dts/imx8mn-evk.dtsi            |  1 +
>>>>>>>>>>>       board/freescale/imx8mn_evk/imx8mn_evk.c | 41 
>>>>>>>>>>> +++++++++++++++++++++++--
>>>>>>>>>>>       configs/imx8mn_ddr4_evk_defconfig       |  8 +++++
>>>>>>>>>>>       3 files changed, 47 insertions(+), 3 deletions(-)
>>>>>>>>>>>
>>>>>>>>>>> diff --git a/arch/arm/dts/imx8mn-evk.dtsi 
>>>>>>>>>>> b/arch/arm/dts/imx8mn-evk.dtsi
>>>>>>>>>>> index 76d042a4cf0..416fadb22b1 100644
>>>>>>>>>>> --- a/arch/arm/dts/imx8mn-evk.dtsi
>>>>>>>>>>> +++ b/arch/arm/dts/imx8mn-evk.dtsi
>>>>>>>>>>> @@ -53,6 +53,7 @@
>>>>>>>>>>>              pinctrl-0 = <&pinctrl_fec1>;
>>>>>>>>>>>              phy-mode = "rgmii-id";
>>>>>>>>>>>              phy-handle = <&ethphy0>;
>>>>>>>>>>> +       phy-reset-gpios = <&gpio4 22 GPIO_ACTIVE_LOW>;
>>>>>>>>>>
>>>>>>>>>> I believe this is phy reset-gpio to be part of mdio child 
>>>>>>>>>> node. any
>>>>>>>>>> idea why it added in fec1 node?
>>>>>>>>>
>>>>>>>>> Try $ git grep phy-reset drivers/net/ , that should clarify it 
>>>>>>>>> all.
>>>>>>>>> In short, FEC in U-Boot still uses legacy bindings.
>>>>>>>>
>>>>>>>> True. I think it's better to add this property on -u-boot.dtsi 
>>>>>>>> which
>>>>>>>> helps to maintain Linux bindings.
>>>>>>>
>>>>>>> The reset GPIO is not specific to U-Boot in any way, so no.
>>>>>>> Instead, the Linux DT bindings should be updated too, but that's a
>>>>>>> separate topic.
>>>>>>
>>>>>> Deprecated optional properties:
>>>>>>            To avoid these, create a phy node according to phy.txt 
>>>>>> in the same
>>>>>>            directory, and point the fec's "phy-handle" property to 
>>>>>> it. Then use
>>>>>>            the phy's reset binding, again described by phy.txt.
>>>>>> - phy-reset-gpios : Should specify the gpio for phy reset
>>>>>> - phy-reset-duration : Reset duration in milliseconds.  Should 
>>>>>> present
>>>>>>      only if property "phy-reset-gpios" is available.  Missing the 
>>>>>> property
>>>>>>      will have the duration be 1 millisecond.  Numbers greater 
>>>>>> than 1000 are
>>>>>>
>>>>>> Jagan refer maybe that property is deprecated
>>>>>
>>>>> See above, U-Boot does not support the per-PHY reset bindings with 
>>>>> FEC.
>>>>> However, the PHY reset GPIO binding is not U-Boot specific, so it 
>>>>> should
>>>>> not be hidden in -u-boot.dtsi .
>>>>
>>>> Yes, I understand. What Jagan is proposing is the following (that you
>>>> can disagree) it's
>>>> just move deprecated part in uboot.dtsi so we can keep align dtsi from
>>>> linux without
>>>> having this problem.
>>>
>>> What should happen is that this same thing should be fixed in Linux, 
>>> i.e.
>>> the missing bindings should be added there, and then once a DT sync 
>>> happens,
>>> whoever does the sync should also then update the FEC driver to handle
>>> per-PHY resets. But that is out of scope of this patch.
>>>
>>> What is for certain is that if the DT sync removes the reset GPIO 
>>> from DT,
>>> ethernet will no longer work in U-Boot and it will show up during 
>>> testing.
>>> If there is some extra property hidden in -u-boot.dtsi, this problem 
>>> will
>>> not trigger test failure then.
>>
>> This seems like a case where the "deprecated" property needs to be
>> populated in the upstream dts file, even if the Linux Kernel doesn't
>> need it strictly because we do and we might not need / want to redesign
>> our driver to meet what Linux expects yet again.
> 
> We really should fix the FEC driver, except that its way out of scope of 
> this patch. And before someone points out that should be simple to do, 
> there is a real unpleasant bonus with FEC and PHY reset bindings, in the 
> ordering in which FEC generates clock for the PHY during reset, for 
> which Linux grew a rather convoluted logic in the FEC driver and the PHY 
> subsystem, see the LAN8710 PHY discussions in Linux which seem to pop up 
> every now and then. So implementing the whole PHY reset handling in FEC 
> driver, to behave just like Linux, isn't as trivial as it seems at first.

The simple rule of thumb here is to fix the driver in u-boot, and then 
think about if such as big effort makes sense in U-Boot. I vote for 
applying this.

Regards,
Stefano


-- 
=====================================================================
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sbabic at denx.de
=====================================================================