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

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

[Marek Vasut]

Add support for ethernet on the imx8mn-ddr4-evk.

Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Fabio Estevam <festevam@gmail.com>
Cc: Peng Fan <peng.fan@nxp.com>
Cc: Stefano Babic <sbabic@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>;
 	fsl,magic-packet;
 	status = "okay";
 
diff --git a/board/freescale/imx8mn_evk/imx8mn_evk.c b/board/freescale/imx8mn_evk/imx8mn_evk.c
index 9a0a0488bf4..8c4bb6c5b8e 100644
--- a/board/freescale/imx8mn_evk/imx8mn_evk.c
+++ b/board/freescale/imx8mn_evk/imx8mn_evk.c
@@ -6,18 +6,53 @@
 #include <common.h>
 #include <env.h>
 #include <init.h>
+#include <miiphy.h>
+#include <netdev.h>
 #include <asm/global_data.h>
+#include <asm/io.h>
 
 DECLARE_GLOBAL_DATA_PTR;
 
-int board_init(void)
+int board_mmc_get_env_dev(int devno)
+{
+	return devno;
+}
+
+#if IS_ENABLED(CONFIG_FEC_MXC)
+static int setup_fec(void)
 {
+	struct iomuxc_gpr_base_regs *gpr =
+		(struct iomuxc_gpr_base_regs *)IOMUXC_GPR_BASE_ADDR;
+
+	/* Use 125M anatop REF_CLK1 for ENET1, not from external */
+	clrsetbits_le32(&gpr->gpr[1], 0x2000, 0);
+
 	return 0;
 }
 
-int board_mmc_get_env_dev(int devno)
+int board_phy_config(struct phy_device *phydev)
 {
-	return devno;
+	/* enable rgmii rxc skew and phy mode select to RGMII copper */
+	phy_write(phydev, MDIO_DEVAD_NONE, 0x1d, 0x1f);
+	phy_write(phydev, MDIO_DEVAD_NONE, 0x1e, 0x8);
+
+	phy_write(phydev, MDIO_DEVAD_NONE, 0x1d, 0x00);
+	phy_write(phydev, MDIO_DEVAD_NONE, 0x1e, 0x82ee);
+	phy_write(phydev, MDIO_DEVAD_NONE, 0x1d, 0x05);
+	phy_write(phydev, MDIO_DEVAD_NONE, 0x1e, 0x100);
+
+	if (phydev->drv->config)
+		phydev->drv->config(phydev);
+	return 0;
+}
+#endif
+
+int board_init(void)
+{
+	if (IS_ENABLED(CONFIG_FEC_MXC))
+		setup_fec();
+
+	return 0;
 }
 
 int board_late_init(void)
diff --git a/configs/imx8mn_ddr4_evk_defconfig b/configs/imx8mn_ddr4_evk_defconfig
index f7ea65886d5..c9a00bf2c2f 100644
--- a/configs/imx8mn_ddr4_evk_defconfig
+++ b/configs/imx8mn_ddr4_evk_defconfig
@@ -43,6 +43,9 @@ CONFIG_CMD_FUSE=y
 CONFIG_CMD_GPIO=y
 CONFIG_CMD_I2C=y
 CONFIG_CMD_MMC=y
+CONFIG_CMD_DHCP=y
+CONFIG_CMD_MII=y
+CONFIG_CMD_PING=y
 CONFIG_CMD_CACHE=y
 CONFIG_CMD_REGULATOR=y
 CONFIG_CMD_EXT2=y
@@ -71,7 +74,12 @@ CONFIG_MMC_HS400_ES_SUPPORT=y
 CONFIG_MMC_HS400_SUPPORT=y
 CONFIG_FSL_ESDHC_IMX=y
 CONFIG_PHYLIB=y
+CONFIG_PHY_ATHEROS=y
 CONFIG_DM_ETH=y
+CONFIG_DM_ETH_PHY=y
+CONFIG_PHY_GIGE=y
+CONFIG_FEC_MXC=y
+CONFIG_MII=y
 CONFIG_PINCTRL=y
 CONFIG_SPL_PINCTRL=y
 CONFIG_PINCTRL_IMX8M=y
-- 
2.30.0

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

[Jagan Teki]

Hi Marek,

On Sat, Feb 27, 2021 at 3:44 AM Marek Vasut <marex@denx.de> wrote:
>
> Add support for ethernet on the imx8mn-ddr4-evk.
>
> Signed-off-by: Marek Vasut <marex@denx.de>
> Cc: Fabio Estevam <festevam@gmail.com>
> Cc: Peng Fan <peng.fan@nxp.com>
> Cc: Stefano Babic <sbabic@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?

Jagan.

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

[Marek Vasut]

On 2/27/21 8:54 AM, Jagan Teki wrote:
> Hi Marek,
> 
> On Sat, Feb 27, 2021 at 3:44 AM Marek Vasut <marex@denx.de> wrote:
>>
>> Add support for ethernet on the imx8mn-ddr4-evk.
>>
>> Signed-off-by: Marek Vasut <marex@denx.de>
>> Cc: Fabio Estevam <festevam@gmail.com>
>> Cc: Peng Fan <peng.fan@nxp.com>
>> Cc: Stefano Babic <sbabic@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.

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

[Jagan Teki]

On Sat, Feb 27, 2021 at 6:21 PM Marek Vasut <marex@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@denx.de> wrote:
> >>
> >> Add support for ethernet on the imx8mn-ddr4-evk.
> >>
> >> Signed-off-by: Marek Vasut <marex@denx.de>
> >> Cc: Fabio Estevam <festevam@gmail.com>
> >> Cc: Peng Fan <peng.fan@nxp.com>
> >> Cc: Stefano Babic <sbabic@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.

Jagan.

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

[Marek Vasut]

On 3/2/21 8:53 AM, Jagan Teki wrote:
> On Sat, Feb 27, 2021 at 6:21 PM Marek Vasut <marex@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@denx.de> wrote:
>>>>
>>>> Add support for ethernet on the imx8mn-ddr4-evk.
>>>>
>>>> Signed-off-by: Marek Vasut <marex@denx.de>
>>>> Cc: Fabio Estevam <festevam@gmail.com>
>>>> Cc: Peng Fan <peng.fan@nxp.com>
>>>> Cc: Stefano Babic <sbabic@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.

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

[Michael Nazzareno Trimarchi]

Hi Marek

On Tue, Mar 2, 2021 at 9:13 AM Marek Vasut <marex@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@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@denx.de> wrote:
> >>>>
> >>>> Add support for ethernet on the imx8mn-ddr4-evk.
> >>>>
> >>>> Signed-off-by: Marek Vasut <marex@denx.de>
> >>>> Cc: Fabio Estevam <festevam@gmail.com>
> >>>> Cc: Peng Fan <peng.fan@nxp.com>
> >>>> Cc: Stefano Babic <sbabic@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

Michael

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

[Marek Vasut]

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@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@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@denx.de> wrote:
>>>>>>
>>>>>> Add support for ethernet on the imx8mn-ddr4-evk.
>>>>>>
>>>>>> Signed-off-by: Marek Vasut <marex@denx.de>
>>>>>> Cc: Fabio Estevam <festevam@gmail.com>
>>>>>> Cc: Peng Fan <peng.fan@nxp.com>
>>>>>> Cc: Stefano Babic <sbabic@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 .

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

[Michael Nazzareno Trimarchi]

Hi Marek

On Tue, Mar 2, 2021 at 9:40 AM Marek Vasut <marex@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@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@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@denx.de> wrote:
> >>>>>>
> >>>>>> Add support for ethernet on the imx8mn-ddr4-evk.
> >>>>>>
> >>>>>> Signed-off-by: Marek Vasut <marex@denx.de>
> >>>>>> Cc: Fabio Estevam <festevam@gmail.com>
> >>>>>> Cc: Peng Fan <peng.fan@nxp.com>
> >>>>>> Cc: Stefano Babic <sbabic@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.

Michael

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

[Marek Vasut]

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@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@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@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@denx.de> wrote:
>>>>>>>>
>>>>>>>> Add support for ethernet on the imx8mn-ddr4-evk.
>>>>>>>>
>>>>>>>> Signed-off-by: Marek Vasut <marex@denx.de>
>>>>>>>> Cc: Fabio Estevam <festevam@gmail.com>
>>>>>>>> Cc: Peng Fan <peng.fan@nxp.com>
>>>>>>>> Cc: Stefano Babic <sbabic@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.

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

[Tom Rini]

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@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@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@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@denx.de> wrote:
> > > > > > > > > 
> > > > > > > > > Add support for ethernet on the imx8mn-ddr4-evk.
> > > > > > > > > 
> > > > > > > > > Signed-off-by: Marek Vasut <marex@denx.de>
> > > > > > > > > Cc: Fabio Estevam <festevam@gmail.com>
> > > > > > > > > Cc: Peng Fan <peng.fan@nxp.com>
> > > > > > > > > Cc: Stefano Babic <sbabic@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.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20210302/3f59caa5/attachment.sig>

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

[Marek Vasut]

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@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@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@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@denx.de> wrote:
>>>>>>>>>>
>>>>>>>>>> Add support for ethernet on the imx8mn-ddr4-evk.
>>>>>>>>>>
>>>>>>>>>> Signed-off-by: Marek Vasut <marex@denx.de>
>>>>>>>>>> Cc: Fabio Estevam <festevam@gmail.com>
>>>>>>>>>> Cc: Peng Fan <peng.fan@nxp.com>
>>>>>>>>>> Cc: Stefano Babic <sbabic@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.

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

[Tom Rini]

On Tue, Mar 02, 2021 at 05:11:48PM +0100, 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@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@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@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@denx.de> wrote:
> > > > > > > > > > > 
> > > > > > > > > > > Add support for ethernet on the imx8mn-ddr4-evk.
> > > > > > > > > > > 
> > > > > > > > > > > Signed-off-by: Marek Vasut <marex@denx.de>
> > > > > > > > > > > Cc: Fabio Estevam <festevam@gmail.com>
> > > > > > > > > > > Cc: Peng Fan <peng.fan@nxp.com>
> > > > > > > > > > > Cc: Stefano Babic <sbabic@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.

It's out of scope for this patch, yes.  And given what you note here I'm
not even sure it's really required / a great idea for U-Boot.  I added
Rob to the list here because this is another case of "Is DT ABI or Linux
driver description?".

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20210302/a8390de2/attachment.sig>

[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@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@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@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@denx.de> 
>>>>>>>>>> wrote:
>>>>>>>>>>>
>>>>>>>>>>> Add support for ethernet on the imx8mn-ddr4-evk.
>>>>>>>>>>>
>>>>>>>>>>> Signed-off-by: Marek Vasut <marex@denx.de>
>>>>>>>>>>> Cc: Fabio Estevam <festevam@gmail.com>
>>>>>>>>>>> Cc: Peng Fan <peng.fan@nxp.com>
>>>>>>>>>>> Cc: Stefano Babic <sbabic@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
=====================================================================

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

[Tom Rini]

On Tue, Mar 02, 2021 at 05:48:12PM +0100, Stefano Babic wrote:
> 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@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@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@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@denx.de> wrote:
> > > > > > > > > > > > 
> > > > > > > > > > > > Add support for ethernet on the imx8mn-ddr4-evk.
> > > > > > > > > > > > 
> > > > > > > > > > > > Signed-off-by: Marek Vasut <marex@denx.de>
> > > > > > > > > > > > Cc: Fabio Estevam <festevam@gmail.com>
> > > > > > > > > > > > Cc: Peng Fan <peng.fan@nxp.com>
> > > > > > > > > > > > Cc: Stefano Babic <sbabic@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.

To be extra clear, I too believe this patch should come in.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20210302/397bb650/attachment.sig>

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

[sbabic at denx.de]

> Add support for ethernet on the imx8mn-ddr4-evk.
> Signed-off-by: Marek Vasut <marex@denx.de>
> Cc: Fabio Estevam <festevam@gmail.com>
> Cc: Peng Fan <peng.fan@nxp.com>
> Cc: Stefano Babic <sbabic@denx.de>
Applied to u-boot-imx, master, thanks !

Best regards,
Stefano Babic

-- 
=====================================================================
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
=====================================================================

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

[Stefano Babic]

Hi Marek,

sorry, false positive. This patch creates warnings for the imx8mn_evk board:

    aarch64:  +   imx8mn_evk
+(imx8mn_evk) board/freescale/imx8mn_evk/imx8mn_evk.c: In function 
'board_init':
+(imx8mn_evk) board/freescale/imx8mn_evk/imx8mn_evk.c:53:3: error: 
implicit declaration of function 'setup_fec' 
[-Werror=implicit-function-declaration]
+(imx8mn_evk)    53 |   setup_fec();
+(imx8mn_evk)       |   ^~~~~~~~~
+(imx8mn_evk) cc1: all warnings being treated as errors
+(imx8mn_evk) make[2]: *** [scripts/Makefile.build:266: 
board/freescale/imx8mn_evk/imx8mn_evk.o] Error 1
+(imx8mn_evk) make[1]: *** [Makefile:1746: board/freescale/imx8mn_evk] 
Error 2
+(imx8mn_evk) make: *** [Makefile:171: sub-make] Error 2

Can you check this ? Thanks !

Best regards,
Stefano


On 08.04.21 22:58, sbabic at denx.de wrote:
>> Add support for ethernet on the imx8mn-ddr4-evk.
>> Signed-off-by: Marek Vasut <marex@denx.de>
>> Cc: Fabio Estevam <festevam@gmail.com>
>> Cc: Peng Fan <peng.fan@nxp.com>
>> Cc: Stefano Babic <sbabic@denx.de>
> Applied to u-boot-imx, master, thanks !
> 
> Best regards,
> Stefano Babic
>