net: fec: Defer probe if other FEC has deferred MDIO

Commit Message

Sverdlin, Alexander Feb. 8, 2023, 10:18 a.m. UTC

From: Alexander Sverdlin <[e-mail]>

In FEC_QUIRK_SINGLE_MDIO case, if fec1 has mdio subnode which is being
probe-deferred because of, for instance, reset-gpio, defer any consequtive
fec2+ probe, we don't want them to register DT-less MDIO bus, but to share
DT-aware MDIO bus from fec1.

Signed-off-by: Alexander Sverdlin <[e-mail]>
---
 drivers/net/ethernet/freescale/fec_main.c | 34 +++++++++++++++++++++--
 1 file changed, 32 insertions(+), 2 deletions(-)

Comments

Andrew Lunn Feb. 9, 2023, 12:55 a.m. UTC | #1

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

Could you not add an else clause here? return -EPROBE_DEFFER?

Basically, if fec0 has not probed, deffer the probing of fec1?

	   Andrew

Sverdlin, Alexander Feb. 9, 2023, 7:27 a.m. UTC | #2

Hello Andrew,

On Thu, 2023-02-09 at 01:55 +0100, Andrew Lunn wrote:
> > -       if ((fep->quirks & FEC_QUIRK_SINGLE_MDIO) && fep->dev_id >
> > 0) {
> > +       if (fep->quirks & FEC_QUIRK_SINGLE_MDIO) {
> >                 /* fec1 uses fec0 mii_bus */
> >                 if (mii_cnt && fec0_mii_bus) {
> >                         fep->mii_bus = fec0_mii_bus;
> >                         mii_cnt++;
> >                         return 0;
> >                 }
> > -               return -ENOENT;
> 
> Could you not add an else clause here? return -EPROBE_DEFFER?
> 
> Basically, if fec0 has not probed, deffer the probing of fec1?

we do have a configuration with i.MX8 where we have only fec2 enabled
(and has mdio node).
I'm not sure if it was thought of by fec driver developers (it makes
a lot of non-obvious assumtions), but that's how it works now.

Wei Fang Feb. 9, 2023, 9:17 a.m. UTC | #3

> -----Original Message-----
> From: Sverdlin, Alexander <[e-mail]>
> Sent: 2023年2月9日 15:28
> To: [e-mail]
> Cc: Wei Fang <[e-mail]>; Clark Wang <[e-mail]>;
> Shenwei Wang <[e-mail]>; [e-mail];
> dl-linux-imx <[e-mail]>; [e-mail]
> Subject: Re: [PATCH] net: fec: Defer probe if other FEC has deferred MDIO
> 
> Hello Andrew,
> 
> On Thu, 2023-02-09 at 01:55 +0100, Andrew Lunn wrote:
> > > -       if ((fep->quirks & FEC_QUIRK_SINGLE_MDIO) && fep->dev_id >
> > > 0) {
> > > +       if (fep->quirks & FEC_QUIRK_SINGLE_MDIO) {
> > >                 /* fec1 uses fec0 mii_bus */
> > >                 if (mii_cnt && fec0_mii_bus) {
> > >                         fep->mii_bus = fec0_mii_bus;
> > >                         mii_cnt++;
> > >                         return 0;
> > >                 }
> > > -               return -ENOENT;
> >
> > Could you not add an else clause here? return -EPROBE_DEFFER?
> >
> > Basically, if fec0 has not probed, deffer the probing of fec1?
> 
> we do have a configuration with i.MX8 where we have only fec2 enabled (and
> has mdio node).
> I'm not sure if it was thought of by fec driver developers (it makes a lot of
> non-obvious assumtions), but that's how it works now.
> 

Hi Alexander,

This issue seems that the fec2 (without mdio subnode) registers mii_bus first, then
the fec1 (has mdio subnode) use the fec2's mii_bus when fec1 probes again, finally
both fec1 and fec2 can not connect to PHY. Am I right?

If so, I think this issue can't be reproduced on the upstream tree, because the quirks of
i.MX8 on the upstream tree do not set the FEC_QUIRK_SINGLE_MDIO bit. So, the fec1
will registers a mii_bus in your case rather than using the fec2's mii_bus. I'm a bit curious
that have you tried to reproduce this issue base on the upstream tree?

Sverdlin, Alexander Feb. 9, 2023, 10:10 a.m. UTC | #4

Hello Wei,

On Thu, 2023-02-09 at 09:17 +0000, Wei Fang wrote:
> > > > -       if ((fep->quirks & FEC_QUIRK_SINGLE_MDIO) && fep-
> > > > >dev_id >
> > > > 0) {
> > > > +       if (fep->quirks & FEC_QUIRK_SINGLE_MDIO) {
> > > >                 /* fec1 uses fec0 mii_bus */
> > > >                 if (mii_cnt && fec0_mii_bus) {
> > > >                         fep->mii_bus = fec0_mii_bus;
> > > >                         mii_cnt++;
> > > >                         return 0;
> > > >                 }
> > > > -               return -ENOENT;
> > > 
> > > Could you not add an else clause here? return -EPROBE_DEFFER?
> > > 
> > > Basically, if fec0 has not probed, deffer the probing of fec1?
> > 
> > we do have a configuration with i.MX8 where we have only fec2
> > enabled (and
> > has mdio node).
> > I'm not sure if it was thought of by fec driver developers (it
> > makes a lot of
> > non-obvious assumtions), but that's how it works now.
> > 
> 
> Hi Alexander,
> 
> This issue seems that the fec2 (without mdio subnode) registers
> mii_bus first, then
> the fec1 (has mdio subnode) use the fec2's mii_bus when fec1 probes
> again, finally
> both fec1 and fec2 can not connect to PHY. Am I right?

yes, this is exactly what happens (except that we have more than one
PHY in this mdio node, which is then not registered/parsed).

> If so, I think this issue can't be reproduced on the upstream tree,
> because the quirks of
> i.MX8 on the upstream tree do not set the FEC_QUIRK_SINGLE_MDIO bit.
> So, the fec1
> will registers a mii_bus in your case rather than using the fec2's
> mii_bus. I'm a bit curious
> that have you tried to reproduce this issue base on the upstream
> tree?

You are right, there is unfortunately no i.MX8 support in the upstream
tree, so it's not possible to reproduce anything. Just wanted to
discuss the probe concept of this driver, which is rather fragile
with all there static local variables, probe call counters and relying
on the probe order. All of this falls together like a house of cards
if something gets deferred.

Andrew Lunn Feb. 9, 2023, 1:35 p.m. UTC | #5

> You are right, there is unfortunately no i.MX8 support in the upstream
> tree, so it's not possible to reproduce anything.

commit 947240ebcc635ab063f17ba027352c3a474d2438
Author: Fugang Duan <[e-mail]>
Date:   Wed Jul 28 19:51:59 2021 +0800

    net: fec: add imx8mq and imx8qm new versions support
    
    The ENET of imx8mq and imx8qm are basically the same as imx6sx,
    but they have new features support based on imx6sx, like:
    - imx8mq: supports IEEE 802.3az EEE standard.
    - imx8qm: supports RGMII mode delayed clock.

Are you using some other imx8 SoC?

> Just wanted to discuss the probe concept of this driver, which is
> rather fragile with all there static local variables, probe call
> counters and relying on the probe order. All of this falls together
> like a house of cards if something gets deferred.

I agree with the comments about it being fragile. It would be good to
get all the naming from OF nodes/addresses. But it needs doing by
somebody with access to a test farm of lots of different boards with
IMX2/5, IMX6 through to 8 and Vybrid.

    Andrew

Sverdlin, Alexander Feb. 9, 2023, 2:14 p.m. UTC | #6

Hello Andrew,

On Thu, 2023-02-09 at 14:35 +0100, Andrew Lunn wrote:
> > You are right, there is unfortunately no i.MX8 support in the
> > upstream
> > tree, so it's not possible to reproduce anything.
> 
> commit 947240ebcc635ab063f17ba027352c3a474d2438
> Author: Fugang Duan <[e-mail]>
> Date:   Wed Jul 28 19:51:59 2021 +0800
> 
>     net: fec: add imx8mq and imx8qm new versions support
>     
>     The ENET of imx8mq and imx8qm are basically the same as imx6sx,
>     but they have new features support based on imx6sx, like:
>     - imx8mq: supports IEEE 802.3az EEE standard.
>     - imx8qm: supports RGMII mode delayed clock.
> 
> Are you using some other imx8 SoC?

I'm referring to imx8qxp/imx8dxp and my "git diff --stat" shows
that upstream has only 9% of LoCs used to boot this SoC.
There is a bit more than just Ethernet in it...

I however believe that my patch preserved the legacy behavior, in
DT-less cases and cases with only one of the two FEC ports enabled
in DT. But I can maintain the patch locally as well if there
is no interest to this fix.

Patch

diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
index 2341597408d12..d4d6dc10dba71 100644
--- a/drivers/net/ethernet/freescale/fec_main.c
+++ b/drivers/net/ethernet/freescale/fec_main.c
@@ -2301,14 +2301,13 @@  static int fec_enet_mii_init(struct platform_device *pdev)
 	 * mdio interface in board design, and need to be configured by
 	 * fec0 mii_bus.
 	 */
-	if ((fep->quirks & FEC_QUIRK_SINGLE_MDIO) && fep->dev_id > 0) {
+	if (fep->quirks & FEC_QUIRK_SINGLE_MDIO) {
 		/* fec1 uses fec0 mii_bus */
 		if (mii_cnt && fec0_mii_bus) {
 			fep->mii_bus = fec0_mii_bus;
 			mii_cnt++;
 			return 0;
 		}
-		return -ENOENT;
 	}
 
 	bus_freq = 2500000; /* 2.5MHz by default */
@@ -2319,6 +2318,37 @@  static int fec_enet_mii_init(struct platform_device *pdev)
 							  "suppress-preamble");
 	}
 
+	while (!node) {
+		/* If we've got so far there is either no FEC node with mdio
+		 * subnode at all (in this case original behavior was to go on
+		 * and create an MDIO bus not related to any DT node), or there
+		 * is another FEC node with mdio subnode out there (in this case
+		 * we defer the probe until MDIO bus will be instantiated in the
+		 * context of its parent node.
+		 */
+		struct device_node *np, *cp;
+		const struct of_device_id *of_id = of_match_device(fec_dt_ids, &pdev->dev);
+
+		if (!of_id)
+			break;
+
+		/* Loop over nodes with same "compatible" as pdev has */
+		for_each_compatible_node(np, NULL, of_id->compatible) {
+			if (!of_device_is_available(np))
+				continue;
+
+			cp = of_get_child_by_name(np, "mdio");
+			if (cp) {
+				of_node_put(cp);
+				of_node_put(np);
+
+				return -EPROBE_DEFER;
+			}
+		}
+
+		break;
+	}
+
 	/*
 	 * Set MII speed (= clk_get_rate() / 2 * phy_speed)
 	 *