[EXT] Re: [PATCH] net: stmmac: dwmac-imx: pause the TXC clock in fixed-link

[Shenwei Wang]

> -----Original Message-----
> From: Vladimir Oltean <olteanv at gmail.com>
> Sent: Tuesday, July 25, 2023 7:44 PM
> To: Shenwei Wang <shenwei.wang at nxp.com>
> Cc: David S. Miller <davem at davemloft.net>; Eric Dumazet
> <edumazet at google.com>; Jakub Kicinski <kuba at kernel.org>; Paolo Abeni
> <pabeni at redhat.com>; Maxime Coquelin <mcoquelin.stm32 at gmail.com>;
> Shawn Guo <shawnguo at kernel.org>; dl-linux-imx <linux-imx at nxp.com>;
> Russell King <linux at armlinux.org.uk>; Giuseppe Cavallaro
> <peppe.cavallaro at st.com>; Alexandre Torgue <alexandre.torgue at foss.st.com>;
> Jose Abreu <joabreu at synopsys.com>; Sascha Hauer <s.hauer at pengutronix.de>;
> Pengutronix Kernel Team <kernel at pengutronix.de>; Fabio Estevam
> <festevam at gmail.com>; netdev at vger.kernel.org; linux-stm32 at st-md-
> mailman.stormreply.com; linux-arm-kernel at lists.infradead.org;
> imx at lists.linux.dev; Frank Li <frank.li at nxp.com>
> Subject: [EXT] Re: [PATCH] net: stmmac: dwmac-imx: pause the TXC clock in
> fixed-link
> 
> Caution: This is an external email. Please take care when clicking links or
> opening attachments. When in doubt, report the message using the 'Report this
> email' button
> 
> 
> Hi Shenwei,
> 
> On Tue, Jul 25, 2023 at 02:49:31PM -0500, Shenwei Wang wrote:
> > When using a fixed-link setup, certain devices like the SJA1105
> > require a small pause in the TXC clock line to enable their internal
> > tunable delay line (TDL).
> >
> > To satisfy this requirement, this patch temporarily disables the TX
> > clock, and restarts it after a required period. This provides the
> > required silent interval on the clock line for SJA1105 to complete the
> > frequency transition and enable the internal TDLs.
> >
> > So far we have only enabled this feature on the i.MX93 platform.
> >
> > Signed-off-by: Shenwei Wang <shenwei.wang at nxp.com>
> > Reviewed-by: Frank Li <frank.li at nxp.com>
> > ---
> 
> Sorry for not responding off-list. Super busy.
> 
> I've tested both this patch on top of net-next as well as the lf-6.1.y version
> you've sent separately - on a cold boot in both cases. Both the
> i.MX93 base board and the SJA1105 EVB (powered by an external power supply)
> were cold booted.
> 

As you can access the internal repo, you don't need this patch and can just test the function directly on the
latest branch of lf-6.1.y. 

> Unfortunately, the patch does not appear to work as intended, and ethtool -S
> eth1 still shows no RX counter incrementing on the SJA1105 CPU port when used
> in RGMII mode (where the problem is).
> 

I have two SJA1105 evaluation boards available, and both are functioning as expected.

> >  .../net/ethernet/stmicro/stmmac/dwmac-imx.c   | 62 +++++++++++++++++++
> >  1 file changed, 62 insertions(+)
> >
> > diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-imx.c
> > b/drivers/net/ethernet/stmicro/stmmac/dwmac-imx.c
> > index b9378a63f0e8..799aedeec094 100644
> > --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-imx.c
> > +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-imx.c
> > @@ -40,6 +40,9 @@
> >  #define DMA_BUS_MODE                 0x00001000
> >  #define DMA_BUS_MODE_SFT_RESET               (0x1 << 0)
> >  #define RMII_RESET_SPEED             (0x3 << 14)
> > +#define TEN_BASET_RESET_SPEED                (0x2 << 14)
> > +#define RGMII_RESET_SPEED            (0x0 << 14)
> > +#define CTRL_SPEED_MASK                      (0x3 << 14)
> >
> >  struct imx_dwmac_ops {
> >       u32 addr_width;
> > @@ -56,6 +59,7 @@ struct imx_priv_data {
> >       struct regmap *intf_regmap;
> >       u32 intf_reg_off;
> >       bool rmii_refclk_ext;
> > +     void __iomem *base_addr;
> >
> >       const struct imx_dwmac_ops *ops;
> >       struct plat_stmmacenet_data *plat_dat; @@ -212,6 +216,61 @@
> > static void imx_dwmac_fix_speed(void *priv, unsigned int speed)
> >               dev_err(dwmac->dev, "failed to set tx rate %lu\n",
> > rate);  }
> >
> > +static bool imx_dwmac_is_fixed_link(struct imx_priv_data *dwmac) {
> > +     struct plat_stmmacenet_data *plat_dat;
> > +     struct device_node *dn;
> > +
> > +     if (!dwmac || !dwmac->plat_dat)
> > +             return false;
> > +
> > +     plat_dat = dwmac->plat_dat;
> > +     dn = of_get_child_by_name(dwmac->dev->of_node, "fixed-link");
> > +     if (!dn)
> > +             return false;
> > +
> > +     if (plat_dat->phy_node == dn || plat_dat->phylink_node == dn)
> > +             return true;
> > +
> > +     return false;
> > +}
> 
> I'm really not sure what prompted the complication here, since instead of:
> 
> if (imx_dwmac_is_fixed_link(dwmac)) {
> 
> you can do:
> 
> #include <linux/of_mdio.h>
> 
> if (of_phy_is_fixed_link(dwmac->dev->of_node)) {
> 

This does not help in this case. What I need to determine is if the PHY currently in use is a fixed-link.
The dwmac DTS node may have multiple PHY nodes defined, including both fixed-link and real PHYs.

Thanks,
Shenwei

> and the latter has the advantage that it also matches (tested on imx93-11x11-
> evk-sja1105.dts). I've had to make this change for testing, because otherwise,
> the workaround wasn't even executing. Other than that, I've done no other
> debugging.
> 
> Considering the fact that you need to resend a functional version even in
> principle anyway, let's continue the discussion and debugging off-list.
> 
> Ah, please be aware of the message from the kernel test robot which said that
> you're setting but not using the plat_dat variable in
> imx_dwmac_fix_speed_mx93().
> It's probably a remnant of what later became imx_dwmac_is_fixed_link(), but it
> still needs to be removed.