[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 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.

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>