| Message ID | 20230208101821.871269-1-alexander.sverdlin@siemens.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: fec: Defer probe if other FEC has deferred MDIO | expand |
> - 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
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.
> -----Original Message----- > From: Sverdlin, Alexander <alexander.sverdlin@siemens.com> > Sent: 2023年2月9日 15:28 > To: andrew@lunn.ch > Cc: Wei Fang <wei.fang@nxp.com>; Clark Wang <xiaoning.wang@nxp.com>; > Shenwei Wang <shenwei.wang@nxp.com>; netdev@vger.kernel.org; > dl-linux-imx <linux-imx@nxp.com>; linux-kernel@vger.kernel.org > 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?
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.
> 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 <fugang.duan@nxp.com> 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
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 <fugang.duan@nxp.com> > 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.
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) *