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

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

[Shenwei Wang]

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@nxp.com>
Reviewed-by: Frank Li <frank.li@nxp.com>
---
 .../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;
+}
+
+static void imx_dwmac_fix_speed_mx93(void *priv, unsigned int speed)
+{
+	struct plat_stmmacenet_data *plat_dat;
+	struct imx_priv_data *dwmac = priv;
+	int val, ctrl, old_ctrl;
+
+	imx_dwmac_fix_speed(priv, speed);
+
+	old_ctrl = readl(dwmac->base_addr + MAC_CTRL_REG);
+	plat_dat = dwmac->plat_dat;
+	ctrl = old_ctrl & ~CTRL_SPEED_MASK;
+
+	/* by default ctrl will be SPEED_1000 */
+	if (speed == SPEED_100)
+		ctrl |= RMII_RESET_SPEED;
+	if (speed == SPEED_10)
+		ctrl |= TEN_BASET_RESET_SPEED;
+
+	if (imx_dwmac_is_fixed_link(dwmac)) {
+		writel(ctrl, dwmac->base_addr + MAC_CTRL_REG);
+
+		/* Ensure the settings for CTRL are applied */
+		wmb();
+
+		val = MX93_GPR_ENET_QOS_INTF_SEL_RGMII;
+		regmap_update_bits(dwmac->intf_regmap, dwmac->intf_reg_off,
+				   MX93_GPR_ENET_QOS_INTF_MODE_MASK, val);
+		usleep_range(50, 100);
+		val = MX93_GPR_ENET_QOS_INTF_SEL_RGMII | MX93_GPR_ENET_QOS_CLK_GEN_EN;
+		regmap_update_bits(dwmac->intf_regmap, dwmac->intf_reg_off,
+				   MX93_GPR_ENET_QOS_INTF_MODE_MASK, val);
+
+		writel(old_ctrl, dwmac->base_addr + MAC_CTRL_REG);
+	}
+}
+
 static int imx_dwmac_mx93_reset(void *priv, void __iomem *ioaddr)
 {
 	struct plat_stmmacenet_data *plat_dat = priv;
@@ -317,8 +376,11 @@ static int imx_dwmac_probe(struct platform_device *pdev)
 	plat_dat->exit = imx_dwmac_exit;
 	plat_dat->clks_config = imx_dwmac_clks_config;
 	plat_dat->fix_mac_speed = imx_dwmac_fix_speed;
+	if (of_machine_is_compatible("fsl,imx93"))
+		plat_dat->fix_mac_speed = imx_dwmac_fix_speed_mx93;
 	plat_dat->bsp_priv = dwmac;
 	dwmac->plat_dat = plat_dat;
+	dwmac->base_addr = stmmac_res.addr;
 
 	ret = imx_dwmac_clks_config(dwmac, true);
 	if (ret)
-- 
2.34.1

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

[Russell King (Oracle)]

On Tue, Jul 25, 2023 at 02:49:31PM -0500, Shenwei Wang wrote:
> +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;

Why would the phy_node or the phylink_node ever be pointing at the
fixed-link node?

For one, phylink expects the fwnode being passed to it to be pointing
at the _parent_ node of the fixed-link node, since it looks up from
the parent for "fixed-link" node.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

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

[kernel test robot]

Hi Shenwei,

kernel test robot noticed the following build warnings:

[auto build test WARNING on shawnguo/for-next]
[also build test WARNING on net-next/main net/main linus/master v6.5-rc3 next-20230725]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Shenwei-Wang/net-stmmac-dwmac-imx-pause-the-TXC-clock-in-fixed-link/20230726-035218
base:   https://git.kernel.org/pub/scm/linux/kernel/git/shawnguo/linux.git for-next
patch link:    https://lore.kernel.org/r/20230725194931.1989102-1-shenwei.wang%40nxp.com
patch subject: [PATCH] net: stmmac: dwmac-imx: pause the TXC clock in fixed-link
config: alpha-allyesconfig (https://download.01.org/0day-ci/archive/20230726/202307260736.EE13gy3b-lkp@intel.com/config)
compiler: alpha-linux-gcc (GCC) 12.3.0
reproduce: (https://download.01.org/0day-ci/archive/20230726/202307260736.EE13gy3b-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202307260736.EE13gy3b-lkp@intel.com/

All warnings (new ones prefixed by >>):

   drivers/net/ethernet/stmicro/stmmac/dwmac-imx.c: In function 'imx_dwmac_fix_speed_mx93':
>> drivers/net/ethernet/stmicro/stmmac/dwmac-imx.c:240:38: warning: variable 'plat_dat' set but not used [-Wunused-but-set-variable]
     240 |         struct plat_stmmacenet_data *plat_dat;
         |                                      ^~~~~~~~


vim +/plat_dat +240 drivers/net/ethernet/stmicro/stmmac/dwmac-imx.c

   237	
   238	static void imx_dwmac_fix_speed_mx93(void *priv, unsigned int speed)
   239	{
 > 240		struct plat_stmmacenet_data *plat_dat;
   241		struct imx_priv_data *dwmac = priv;
   242		int val, ctrl, old_ctrl;
   243	
   244		imx_dwmac_fix_speed(priv, speed);
   245	
   246		old_ctrl = readl(dwmac->base_addr + MAC_CTRL_REG);
   247		plat_dat = dwmac->plat_dat;
   248		ctrl = old_ctrl & ~CTRL_SPEED_MASK;
   249	
   250		/* by default ctrl will be SPEED_1000 */
   251		if (speed == SPEED_100)
   252			ctrl |= RMII_RESET_SPEED;
   253		if (speed == SPEED_10)
   254			ctrl |= TEN_BASET_RESET_SPEED;
   255	
   256		if (imx_dwmac_is_fixed_link(dwmac)) {
   257			writel(ctrl, dwmac->base_addr + MAC_CTRL_REG);
   258	
   259			/* Ensure the settings for CTRL are applied */
   260			wmb();
   261	
   262			val = MX93_GPR_ENET_QOS_INTF_SEL_RGMII;
   263			regmap_update_bits(dwmac->intf_regmap, dwmac->intf_reg_off,
   264					   MX93_GPR_ENET_QOS_INTF_MODE_MASK, val);
   265			usleep_range(50, 100);
   266			val = MX93_GPR_ENET_QOS_INTF_SEL_RGMII | MX93_GPR_ENET_QOS_CLK_GEN_EN;
   267			regmap_update_bits(dwmac->intf_regmap, dwmac->intf_reg_off,
   268					   MX93_GPR_ENET_QOS_INTF_MODE_MASK, val);
   269	
   270			writel(old_ctrl, dwmac->base_addr + MAC_CTRL_REG);
   271		}
   272	}
   273	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

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

[Vladimir Oltean]

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@nxp.com>
> Reviewed-by: Frank Li <frank.li@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.

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

>  .../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)) {

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.

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

[Andrew Lunn]

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

The SJA1105 has the problem, so i would expect it to be involved in
the solution. Otherwise, how is this going to work for other MAC
drivers?

Maybe you need to expose a common clock framework clock for the TXC
clock line, which the SJA1105 can disable/enable? That then makes it
clear what other MAC drivers need to do.

      Andrew

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

[Vladimir Oltean]

Hi Andrew,

On Wed, Jul 26, 2023 at 10:32:00AM +0200, Andrew Lunn wrote:
> 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).
> 
> The SJA1105 has the problem, so i would expect it to be involved in
> the solution. Otherwise, how is this going to work for other MAC
> drivers?
> 
> Maybe you need to expose a common clock framework clock for the TXC
> clock line, which the SJA1105 can disable/enable? That then makes it
> clear what other MAC drivers need to do.
> 
>       Andrew
> 

The delicate nature of the SJA1105 bug is that as far as I understand,
the switch is not aware of the fact that its RGMII delay line went out
of whack. Its port MII status registers say that they're okay.

Also, if I understand Shenwei's workaround procedure, it deals more with
"prevention" than with "recovery". I'm not sure that (reliable) recovery
is possible. I'm trying to gather more data from NXP colleagues.

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

[Shenwei Wang]

> -----Original Message-----
> From: Russell King <linux@armlinux.org.uk>
> Sent: Tuesday, July 25, 2023 4:05 PM
> To: Shenwei Wang <shenwei.wang@nxp.com>
> Cc: David S. Miller <davem@davemloft.net>; Eric Dumazet
> <edumazet@google.com>; Jakub Kicinski <kuba@kernel.org>; Paolo Abeni
> <pabeni@redhat.com>; Maxime Coquelin <mcoquelin.stm32@gmail.com>;
> Shawn Guo <shawnguo@kernel.org>; dl-linux-imx <linux-imx@nxp.com>;
> Giuseppe Cavallaro <peppe.cavallaro@st.com>; Alexandre Torgue
> <alexandre.torgue@foss.st.com>; Jose Abreu <joabreu@synopsys.com>; Sascha
> Hauer <s.hauer@pengutronix.de>; Pengutronix Kernel Team
> <kernel@pengutronix.de>; Fabio Estevam <festevam@gmail.com>;
> netdev@vger.kernel.org; linux-stm32@st-md-mailman.stormreply.com; linux-
> arm-kernel@lists.infradead.org; imx@lists.linux.dev; Frank Li <frank.li@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
>
>
> On Tue, Jul 25, 2023 at 02:49:31PM -0500, Shenwei Wang wrote:
> > +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;
>
> Why would the phy_node or the phylink_node ever be pointing at the fixed-link
> node?
>

The logic was learned from the function of stmmac_probe_config_dt, and it normally
save the phy handle to those two members: phy_node and phylink_node. But seems
checking phy_node is enough here, right?

        plat->phy_node = of_parse_phandle(np, "phy-handle", 0);

        /* PHYLINK automatically parses the phy-handle property */
        plat->phylink_node = np;

> For one, phylink expects the fwnode being passed to it to be pointing at the
> _parent_ node of the fixed-link node, since it looks up from the parent for
> "fixed-link" node.
>

Yes,  the above line of code passes the parent node to phylink_node.

Thanks,
Shenwei

> --
> RMK's Patch system:
> https://www.ar/
> mlinux.org.uk%2Fdeveloper%2Fpatches%2F&data=05%7C01%7Cshenwei.wang
> %40nxp.com%7Cd5a4b8372a4a4e5092b008db8d52c6ce%7C686ea1d3bc2b4c6f
> a92cd99c5c301635%7C0%7C0%7C638259158876867949%7CUnknown%7CTWF
> pbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6
> Mn0%3D%7C3000%7C%7C%7C&sdata=XPNpTlv7jbmOfeiQL5w0A6M2c3p5AOiT
> UOGg73ijFb8%3D&reserved=0
> FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

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

[Russell King (Oracle)]

On Wed, Jul 26, 2023 at 03:00:49PM +0000, Shenwei Wang wrote:
> 
> 
> > -----Original Message-----
> > From: Russell King <linux@armlinux.org.uk>
> > Sent: Tuesday, July 25, 2023 4:05 PM
> > To: Shenwei Wang <shenwei.wang@nxp.com>
> > Cc: David S. Miller <davem@davemloft.net>; Eric Dumazet
> > <edumazet@google.com>; Jakub Kicinski <kuba@kernel.org>; Paolo Abeni
> > <pabeni@redhat.com>; Maxime Coquelin <mcoquelin.stm32@gmail.com>;
> > Shawn Guo <shawnguo@kernel.org>; dl-linux-imx <linux-imx@nxp.com>;
> > Giuseppe Cavallaro <peppe.cavallaro@st.com>; Alexandre Torgue
> > <alexandre.torgue@foss.st.com>; Jose Abreu <joabreu@synopsys.com>; Sascha
> > Hauer <s.hauer@pengutronix.de>; Pengutronix Kernel Team
> > <kernel@pengutronix.de>; Fabio Estevam <festevam@gmail.com>;
> > netdev@vger.kernel.org; linux-stm32@st-md-mailman.stormreply.com; linux-
> > arm-kernel@lists.infradead.org; imx@lists.linux.dev; Frank Li <frank.li@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
> >
> >
> > On Tue, Jul 25, 2023 at 02:49:31PM -0500, Shenwei Wang wrote:
> > > +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;
> >
> > Why would the phy_node or the phylink_node ever be pointing at the fixed-link
> > node?
> >
> 
> The logic was learned from the function of stmmac_probe_config_dt, and it normally
> save the phy handle to those two members: phy_node and phylink_node. But seems
> checking phy_node is enough here, right?
> 
>         plat->phy_node = of_parse_phandle(np, "phy-handle", 0);
> 
>         /* PHYLINK automatically parses the phy-handle property */
>         plat->phylink_node = np;

So, plat->phy_node will never ever be equal to your "dn" above.
plat->phylink_node is the same as dwmac->dev->of_node above, and
so plat->phylink_node will never be your "dn" above either.

Those two together means that imx_dwmac_is_fixed_link() will _always_
return false, and thus most of the code you're adding is rather
useless.

It also means the code you're submitting probably hasn't been properly
tested.

Have you confirmed that imx_dwmac_is_fixed_link() will actually return
true in your testing? Under what conditions did your testing reveal a
true return value from this function?

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

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

[Shenwei Wang]

> -----Original Message-----
> From: Vladimir Oltean <olteanv@gmail.com>
> Sent: Tuesday, July 25, 2023 7:44 PM
> To: Shenwei Wang <shenwei.wang@nxp.com>
> Cc: David S. Miller <davem@davemloft.net>; Eric Dumazet
> <edumazet@google.com>; Jakub Kicinski <kuba@kernel.org>; Paolo Abeni
> <pabeni@redhat.com>; Maxime Coquelin <mcoquelin.stm32@gmail.com>;
> Shawn Guo <shawnguo@kernel.org>; dl-linux-imx <linux-imx@nxp.com>;
> Russell King <linux@armlinux.org.uk>; Giuseppe Cavallaro
> <peppe.cavallaro@st.com>; Alexandre Torgue <alexandre.torgue@foss.st.com>;
> Jose Abreu <joabreu@synopsys.com>; Sascha Hauer <s.hauer@pengutronix.de>;
> Pengutronix Kernel Team <kernel@pengutronix.de>; Fabio Estevam
> <festevam@gmail.com>; netdev@vger.kernel.org; linux-stm32@st-md-
> mailman.stormreply.com; linux-arm-kernel@lists.infradead.org;
> imx@lists.linux.dev; Frank Li <frank.li@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@nxp.com>
> > Reviewed-by: Frank Li <frank.li@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.

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

[Russell King (Oracle)]

On Wed, Jul 26, 2023 at 03:10:19PM +0000, Shenwei Wang wrote:
> > 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.

... and this makes me wonder what DT node structure you think would
describe a fixed-link.

A valid ethernet device node would be:

	dwmac-node {
		phy-handle = <&phy1>;
	};

In this case:
	dwmac->dev->of_node points at "dwmac-node"
	plat->phylink_node points at "dwmac-node"
	plat->phy_node points at "phy1"
	Your "dn" is NULL.
	Therefore, your imx_dwmac_is_fixed_link() returns false.

	dwmac-node {
		fixed-link {
			speed = <...>;
			full-duplex;
		};
	};

In this case:
	dwmac->dev->of_node points at "dwmac-node"
	plat->phylink_node points at "dwmac-node"
	plat->phy_node is NULL
	Your "dn" points at the "fixed-link" node.
	Therefore, your imx_dwmac_is_fixed_link() also returns false.

Now, as far as your comment "What I need to determine is if the PHY
currently in use is a fixed-link." I'm just going "Eh? What?" at that,
because it makes zero sense to me.

stmmac uses phylink. phylink doesn't use a PHY for fixed-links, unlike
the old phylib-based fixed-link implementation that software-emulated
a clause-22 PHY. With phylink, when fixed-link is specified, there is
_no_ PHY.

There is no need to do any of this poking about to determine if the
link that is being brought up is a fixed-link or not, because phylink's
callbacks into the MAC driver already contain this information in the
"mode" argument. However, that is not passed to the driver's internal
priv->plat->fix_mac_speed() method - but this is the information you
need.

There is no need to write code to try and second-guess this, phylink
tells drivers what mode it is operating under.

stmmac really needs to be cleaned up - and really doesn't need more
complexity when the information is already being provided to the
driver.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

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

[Shenwei Wang]

> -----Original Message-----
> From: Russell King <linux@armlinux.org.uk>
> Sent: Wednesday, July 26, 2023 10:29 AM
> To: Shenwei Wang <shenwei.wang@nxp.com>
> Cc: Vladimir Oltean <olteanv@gmail.com>; David S. Miller
> <davem@davemloft.net>; Eric Dumazet <edumazet@google.com>; Jakub
> Kicinski <kuba@kernel.org>; Paolo Abeni <pabeni@redhat.com>; Maxime
> Coquelin <mcoquelin.stm32@gmail.com>; Shawn Guo <shawnguo@kernel.org>;
> dl-linux-imx <linux-imx@nxp.com>; Giuseppe Cavallaro
> <peppe.cavallaro@st.com>; Alexandre Torgue <alexandre.torgue@foss.st.com>;
> Jose Abreu <joabreu@synopsys.com>; Sascha Hauer <s.hauer@pengutronix.de>;
> Pengutronix Kernel Team <kernel@pengutronix.de>; Fabio Estevam
> <festevam@gmail.com>; netdev@vger.kernel.org; linux-stm32@st-md-
> mailman.stormreply.com; linux-arm-kernel@lists.infradead.org;
> imx@lists.linux.dev; Frank Li <frank.li@nxp.com>
> Subject: Re: [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
>
>
> On Wed, Jul 26, 2023 at 03:10:19PM +0000, Shenwei Wang wrote:
> > > 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.
>
> ... and this makes me wonder what DT node structure you think would describe a
> fixed-link.
>
> A valid ethernet device node would be:
>
>         dwmac-node {
>                 phy-handle = <&phy1>;
>         };
>
> In this case:
>         dwmac->dev->of_node points at "dwmac-node"
>         plat->phylink_node points at "dwmac-node"
>         plat->phy_node points at "phy1"
>         Your "dn" is NULL.
>         Therefore, your imx_dwmac_is_fixed_link() returns false.
>
>         dwmac-node {
>                 fixed-link {
>                         speed = <...>;
>                         full-duplex;
>                 };
>         };
>
> In this case:
>         dwmac->dev->of_node points at "dwmac-node"
>         plat->phylink_node points at "dwmac-node"
>         plat->phy_node is NULL
>         Your "dn" points at the "fixed-link" node.
>         Therefore, your imx_dwmac_is_fixed_link() also returns false.
>
> Now, as far as your comment "What I need to determine is if the PHY currently
> in use is a fixed-link." I'm just going "Eh? What?" at that, because it makes zero
> sense to me.
>
> stmmac uses phylink. phylink doesn't use a PHY for fixed-links, unlike the old
> phylib-based fixed-link implementation that software-emulated a clause-22 PHY.
> With phylink, when fixed-link is specified, there is _no_ PHY.

So you mean the fixed-link node will always be the highest priority to be used in the phylink
use case?  If so, I just need to check if there is a fixed-link node as Vladimir pointed out, right?

>
> There is no need to do any of this poking about to determine if the link that is
> being brought up is a fixed-link or not, because phylink's callbacks into the MAC
> driver already contain this information in the "mode" argument. However, that
> is not passed to the driver's internal
> priv->plat->fix_mac_speed() method - but this is the information you
> need.
>

Yes, you are right. The best way is to change the fix_mac_speed prototype but it
will change several other platforms. That's why I didn't go that way.

Thanks,
Shenwei

> There is no need to write code to try and second-guess this, phylink tells drivers
> what mode it is operating under.
>
> stmmac really needs to be cleaned up - and really doesn't need more complexity
> when the information is already being provided to the driver.
>
> --
> RMK's Patch system:
> https://www.ar/
> mlinux.org.uk%2Fdeveloper%2Fpatches%2F&data=05%7C01%7Cshenwei.wang
> %40nxp.com%7C3d20eec67cbe49ecdbf008db8ded1aab%7C686ea1d3bc2b4c6fa
> 92cd99c5c301635%7C0%7C0%7C638259821712485867%7CUnknown%7CTWFp
> bGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6
> Mn0%3D%7C3000%7C%7C%7C&sdata=pMle7Mu4ed57Qjf7DR2PPQ67F5oKq9qr
> GRG%2BNMpSMDM%3D&reserved=0
> FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

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

[Shenwei Wang]

> -----Original Message-----
> From: Russell King <linux@armlinux.org.uk>
> Sent: Wednesday, July 26, 2023 10:09 AM
> To: Shenwei Wang <shenwei.wang@nxp.com>
> Cc: David S. Miller <davem@davemloft.net>; Eric Dumazet
> <edumazet@google.com>; Jakub Kicinski <kuba@kernel.org>; Paolo Abeni
> <pabeni@redhat.com>; Maxime Coquelin <mcoquelin.stm32@gmail.com>;
> Shawn Guo <shawnguo@kernel.org>; dl-linux-imx <linux-imx@nxp.com>;
> Giuseppe Cavallaro <peppe.cavallaro@st.com>; Alexandre Torgue
> <alexandre.torgue@foss.st.com>; Jose Abreu <joabreu@synopsys.com>; Sascha
> Hauer <s.hauer@pengutronix.de>; Pengutronix Kernel Team
> <kernel@pengutronix.de>; Fabio Estevam <festevam@gmail.com>;
> netdev@vger.kernel.org; linux-stm32@st-md-mailman.stormreply.com; linux-
> arm-kernel@lists.infradead.org; imx@lists.linux.dev; Frank Li <frank.li@nxp.com>
> Subject: Re: [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
>
>
> On Wed, Jul 26, 2023 at 03:00:49PM +0000, Shenwei Wang wrote:
> >
> >
> > > -----Original Message-----
> > > From: Russell King <linux@armlinux.org.uk>
> > > Sent: Tuesday, July 25, 2023 4:05 PM
> > > To: Shenwei Wang <shenwei.wang@nxp.com>
> > > Cc: David S. Miller <davem@davemloft.net>; Eric Dumazet
> > > <edumazet@google.com>; Jakub Kicinski <kuba@kernel.org>; Paolo Abeni
> > > <pabeni@redhat.com>; Maxime Coquelin <mcoquelin.stm32@gmail.com>;
> > > Shawn Guo <shawnguo@kernel.org>; dl-linux-imx <linux-imx@nxp.com>;
> > > Giuseppe Cavallaro <peppe.cavallaro@st.com>; Alexandre Torgue
> > > <alexandre.torgue@foss.st.com>; Jose Abreu <joabreu@synopsys.com>;
> > > Sascha Hauer <s.hauer@pengutronix.de>; Pengutronix Kernel Team
> > > <kernel@pengutronix.de>; Fabio Estevam <festevam@gmail.com>;
> > > netdev@vger.kernel.org; linux-stm32@st-md-mailman.stormreply.com;
> > > linux- arm-kernel@lists.infradead.org; imx@lists.linux.dev; Frank Li
> > > <frank.li@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
> > >
> > >
> > > On Tue, Jul 25, 2023 at 02:49:31PM -0500, Shenwei Wang wrote:
> > > > +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;
> > >
> > > Why would the phy_node or the phylink_node ever be pointing at the
> > > fixed-link node?
> > >
> >
> > The logic was learned from the function of stmmac_probe_config_dt, and
> > it normally save the phy handle to those two members: phy_node and
> > phylink_node. But seems checking phy_node is enough here, right?
> >
> >         plat->phy_node = of_parse_phandle(np, "phy-handle", 0);
> >
> >         /* PHYLINK automatically parses the phy-handle property */
> >         plat->phylink_node = np;
>
> So, plat->phy_node will never ever be equal to your "dn" above.
> plat->phylink_node is the same as dwmac->dev->of_node above, and
> so plat->phylink_node will never be your "dn" above either.
>
> Those two together means that imx_dwmac_is_fixed_link() will _always_ return
> false, and thus most of the code you're adding is rather useless.
>
> It also means the code you're submitting probably hasn't been properly tested.
>
> Have you confirmed that imx_dwmac_is_fixed_link() will actually return true in
> your testing? Under what conditions did your testing reveal a true return value
> from this function?
>

We can't make that assumption. In my testing code, I had trace statements in that
section to indicate the code was actually executed. You may get some clues from the following DTS:

+&eqos {
+       pinctrl-names = "default";
+       pinctrl-0 = <&pinctrl_eqos_rgmii>;
+       phy-mode = "rgmii-rxid";
+       phy-handle = <&fixed0>;
+       status = "okay";
+
+       fixed0: fixed-link {
+               speed = <1000>;
+               full-duplex;
+       };
+
+       mdio {
+               compatible = "snps,dwmac-mdio";
+               #address-cells = <1>;
+               #size-cells = <0>;
+               clock-frequency = <2500000>;
+
+               phy0: ethernet-phy@8 {
+                       reg = <0x8>;
+                       max-speed = <100>;
+                       #address-cells = <1>;
+                       #size-cells = <0>;
+
+                       phy1: ethernet-phy@9 {
+                               reg = <0x9>;
+                               max-speed = <100>;
+                       };
+               };
...


Thanks,
Shenwei



> --
> RMK's Patch system:
> https://www.ar/
> mlinux.org.uk%2Fdeveloper%2Fpatches%2F&data=05%7C01%7Cshenwei.wang
> %40nxp.com%7C2793017863ea494fd07808db8dea50c3%7C686ea1d3bc2b4c6f
> a92cd99c5c301635%7C0%7C0%7C638259809734982456%7CUnknown%7CTWF
> pbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6
> Mn0%3D%7C3000%7C%7C%7C&sdata=1HZj6eDR1nNvep6S9OXlg%2BbhbekGKjS
> AWvw2LYEa9Ig%3D&reserved=0
> FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

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

[Russell King (Oracle)]

On Wed, Jul 26, 2023 at 04:10:11PM +0000, Shenwei Wang wrote:
> > So, plat->phy_node will never ever be equal to your "dn" above.
> > plat->phylink_node is the same as dwmac->dev->of_node above, and
> > so plat->phylink_node will never be your "dn" above either.
> >
> > Those two together means that imx_dwmac_is_fixed_link() will _always_ return
> > false, and thus most of the code you're adding is rather useless.
> >
> > It also means the code you're submitting probably hasn't been properly tested.
> >
> > Have you confirmed that imx_dwmac_is_fixed_link() will actually return true in
> > your testing? Under what conditions did your testing reveal a true return value
> > from this function?
> >
> 
> We can't make that assumption. In my testing code, I had trace statements in that
> section to indicate the code was actually executed. You may get some clues from the following DTS:
> 
> +&eqos {
> +       pinctrl-names = "default";
> +       pinctrl-0 = <&pinctrl_eqos_rgmii>;
> +       phy-mode = "rgmii-rxid";
> +       phy-handle = <&fixed0>;
> +       status = "okay";
> +
> +       fixed0: fixed-link {
> +               speed = <1000>;
> +               full-duplex;
> +       };

This is just totally botched up.

"fixed0" is _not_ a PHY, therefore you should NOT be referencing it
in phy-handle. Please see the DT binding document:

  phy-handle:
    $ref: /schemas/types.yaml#/definitions/phandle
    description:
      Specifies a reference to a node representing a PHY device.

  fixed-link:
    oneOf:
      - $ref: /schemas/types.yaml#/definitions/uint32-array
        deprecated: true
...
      - type: object
        additionalProperties: false
        properties:
          speed:
...

As I said, fixed-link is _not_ a PHY, and thus phy-handle must *not*
be used to point at it.

The mere presence of a node called "fixed-link" will make this "eqos"
device use that fixed-link node, and the phy-handle will be ignored.

So sorry, but as far as your patch goes, it's a hard NAK from me
right now until the DT description is actually correct.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

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

[Russell King (Oracle)]

On Wed, Jul 26, 2023 at 03:59:38PM +0000, Shenwei Wang wrote:
> > -----Original Message-----
> > From: Russell King <linux@armlinux.org.uk>
> > Sent: Wednesday, July 26, 2023 10:29 AM
> > To: Shenwei Wang <shenwei.wang@nxp.com>
> > Cc: Vladimir Oltean <olteanv@gmail.com>; David S. Miller
> > <davem@davemloft.net>; Eric Dumazet <edumazet@google.com>; Jakub
> > Kicinski <kuba@kernel.org>; Paolo Abeni <pabeni@redhat.com>; Maxime
> > Coquelin <mcoquelin.stm32@gmail.com>; Shawn Guo <shawnguo@kernel.org>;
> > dl-linux-imx <linux-imx@nxp.com>; Giuseppe Cavallaro
> > <peppe.cavallaro@st.com>; Alexandre Torgue <alexandre.torgue@foss.st.com>;
> > Jose Abreu <joabreu@synopsys.com>; Sascha Hauer <s.hauer@pengutronix.de>;
> > Pengutronix Kernel Team <kernel@pengutronix.de>; Fabio Estevam
> > <festevam@gmail.com>; netdev@vger.kernel.org; linux-stm32@st-md-
> > mailman.stormreply.com; linux-arm-kernel@lists.infradead.org;
> > imx@lists.linux.dev; Frank Li <frank.li@nxp.com>
> > Subject: Re: [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
> >
> >
> > On Wed, Jul 26, 2023 at 03:10:19PM +0000, Shenwei Wang wrote:
> > > > 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.
> >
> > ... and this makes me wonder what DT node structure you think would describe a
> > fixed-link.
> >
> > A valid ethernet device node would be:
> >
> >         dwmac-node {
> >                 phy-handle = <&phy1>;
> >         };
> >
> > In this case:
> >         dwmac->dev->of_node points at "dwmac-node"
> >         plat->phylink_node points at "dwmac-node"
> >         plat->phy_node points at "phy1"
> >         Your "dn" is NULL.
> >         Therefore, your imx_dwmac_is_fixed_link() returns false.
> >
> >         dwmac-node {
> >                 fixed-link {
> >                         speed = <...>;
> >                         full-duplex;
> >                 };
> >         };
> >
> > In this case:
> >         dwmac->dev->of_node points at "dwmac-node"
> >         plat->phylink_node points at "dwmac-node"
> >         plat->phy_node is NULL
> >         Your "dn" points at the "fixed-link" node.
> >         Therefore, your imx_dwmac_is_fixed_link() also returns false.
> >
> > Now, as far as your comment "What I need to determine is if the PHY currently
> > in use is a fixed-link." I'm just going "Eh? What?" at that, because it makes zero
> > sense to me.
> >
> > stmmac uses phylink. phylink doesn't use a PHY for fixed-links, unlike the old
> > phylib-based fixed-link implementation that software-emulated a clause-22 PHY.
> > With phylink, when fixed-link is specified, there is _no_ PHY.
> 
> So you mean the fixed-link node will always be the highest priority to
> be used in the phylink use case?

Yes, because that is how all network drivers have behaved. If you look
at the function that Vladimir pointed out, then you will notice that
the mere presence of a fixed-link node makes it a "fixed link".

> If so, I just need to check if there is a fixed-link node as Vladimir pointed out, right?

You could, but that is grossly inefficient, and I will NAK it because
by doing so, it makes this messy driver even worse.

> > There is no need to do any of this poking about to determine if the link that is
> > being brought up is a fixed-link or not, because phylink's callbacks into the MAC
> > driver already contain this information in the "mode" argument. However, that
> > is not passed to the driver's internal
> > priv->plat->fix_mac_speed() method - but this is the information you
> > need.
> >
> 
> Yes, you are right. The best way is to change the fix_mac_speed prototype
> but it will change several other platforms. That's why I didn't go that way.

Why is that a problem?

I really don't get this "I can't get at information I need without
changing a driver internal interface, so I'll write some really
inefficient code to work around the problem and make the driver
even more messy" attitude.

It's not like you're changing a publicly visible API - it's a
driver private API and all the users of it are in the kernel tree.

A standard part of open source development is not to bodge around
existing code, but to implement efficient solutions to problems.

As phylink *already* tells stmmac_mac_link_up() whether it is
operating with a PHY, fixed-link, or in-band mode, the stmmac
layer has the information you need, but doesn't pass this into
the fix_mac_speed() function.

The best solution to this is *not* to bodge around it by trying
to second-guess what's going on and thus creating messy code.

Given that we have the full source available which we can modify,
then changing things like this function pointer prototype is
absolutely acceptable, and in this case is the correct way to
address the issue you have.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

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

[Vladimir Oltean]

On Wed, Jul 26, 2023 at 05:29:30PM +0100, Russell King (Oracle) wrote:
> On Wed, Jul 26, 2023 at 04:10:11PM +0000, Shenwei Wang wrote:
> > > So, plat->phy_node will never ever be equal to your "dn" above.
> > > plat->phylink_node is the same as dwmac->dev->of_node above, and
> > > so plat->phylink_node will never be your "dn" above either.
> > >
> > > Those two together means that imx_dwmac_is_fixed_link() will _always_ return
> > > false, and thus most of the code you're adding is rather useless.
> > >
> > > It also means the code you're submitting probably hasn't been properly tested.
> > >
> > > Have you confirmed that imx_dwmac_is_fixed_link() will actually return true in
> > > your testing? Under what conditions did your testing reveal a true return value
> > > from this function?
> > >
> > 
> > We can't make that assumption. In my testing code, I had trace statements in that
> > section to indicate the code was actually executed. You may get some clues from the following DTS:
> > 
> > +&eqos {
> > +       pinctrl-names = "default";
> > +       pinctrl-0 = <&pinctrl_eqos_rgmii>;
> > +       phy-mode = "rgmii-rxid";
> > +       phy-handle = <&fixed0>;
> > +       status = "okay";
> > +
> > +       fixed0: fixed-link {
> > +               speed = <1000>;
> > +               full-duplex;
> > +       };
> 
> This is just totally botched up.
> 
> "fixed0" is _not_ a PHY, therefore you should NOT be referencing it
> in phy-handle. Please see the DT binding document:
> 
>   phy-handle:
>     $ref: /schemas/types.yaml#/definitions/phandle
>     description:
>       Specifies a reference to a node representing a PHY device.
> 
>   fixed-link:
>     oneOf:
>       - $ref: /schemas/types.yaml#/definitions/uint32-array
>         deprecated: true
> ...
>       - type: object
>         additionalProperties: false
>         properties:
>           speed:
> ...
> 
> As I said, fixed-link is _not_ a PHY, and thus phy-handle must *not*
> be used to point at it.
> 
> The mere presence of a node called "fixed-link" will make this "eqos"
> device use that fixed-link node, and the phy-handle will be ignored.
> 
> So sorry, but as far as your patch goes, it's a hard NAK from me
> right now until the DT description is actually correct.
> 
> -- 
> RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
> 

Shenwei, the correct way to describe the link between the eQOS and the
SJA1105 port in imx93-11x11-evk-sja1105.dts is:

#include "imx93-11x11-evk.dts"

&eqos {
	/delete-property/ phy-handle;
	clk_csr = <5>;

	fixed-link {
		speed = <1000>;
		full-duplex;
	};

	mdio {
		/delete-property/ ethernet-phy@1;

		/* TJA1102 */
		phy0: ethernet-phy@8 {
			#address-cells = <1>;
			#size-cells = <0>;
			reg = <0x8>;

			phy1: ethernet-phy@9 {
				reg = <0x9>;
			};
		};

		/* TJA1102 */
		phy2: ethernet-phy@e {
			#address-cells = <1>;
			#size-cells = <0>;
			reg = <0xe>;

			phy3: ethernet-phy@f {
				reg = <0xf>;
			};
		};
	};
};

&iomuxc {
	pinctrl_lpspi8: lpspi8grp {
		fsl,pins = <
			MX93_PAD_GPIO_IO12__GPIO2_IO12  0x3fe
			MX93_PAD_GPIO_IO13__LPSPI8_SIN  0x3fe
			MX93_PAD_GPIO_IO14__LPSPI8_SOUT 0x3fe
			MX93_PAD_GPIO_IO15__LPSPI8_SCK  0x3fe
		>;
	};
};

&lpspi8 {
	fsl,spi-num-chipselects = <1>;
	pinctrl-names = "default";
	pinctrl-0 = <&pinctrl_lpspi8>;
	cs-gpios = <&gpio2 12 GPIO_ACTIVE_LOW>;
	pinctrl-assert-gpios = <&pcal6524_b 3 GPIO_ACTIVE_LOW>;
	status = "okay";

	ethernet-switch@0 {
		reg = <0x0>;
		compatible = "nxp,sja1105q";
		#address-cells = <1>;
		#size-cells = <0>;
		spi-max-frequency = <4000000>;
		spi-cpha;
		status = "okay";

		ethernet-ports {
			#address-cells = <1>;
			#size-cells = <0>;

			port@0 {
				ethernet = <&eqos>;
				phy-mode = "rgmii-id";
				tx-internal-delay-ps = <2000>;
				rx-internal-delay-ps = <2000>;
				reg = <0>;

				fixed-link {
					speed = <1000>;
					full-duplex;
				};
			};

			port@1 {
				phy-handle = <&phy0>;
				label = "swp1";
				phy-mode = "mii";
				reg = <1>;
			};

			port@2 {
				label = "swp2";
				phy-handle = <&phy1>;
				phy-mode = "mii";
				reg = <2>;
			};

			port@3 {
				label = "swp3";
				phy-handle = <&phy2>;
				phy-mode = "rmii";
				reg = <3>;
			};

			port@4 {
				phy-handle = <&phy3>;
				label = "swp4";
				phy-mode = "rmii";
				reg = <4>;
			};
		};
	};
};

The "mdio" node under "eqos" is just the OF node of the MDIO bus.
It doesn't necessarily mean that the net_device corresponding to the
stmmac uses all those PHYs. It uses just the PHY that it has a
phy-handle to. It may not even have a phy-handle at all, just a
fixed-link, like above.

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

[Shenwei Wang]

> -----Original Message-----
> From: Russell King <linux@armlinux.org.uk>
> Sent: Wednesday, July 26, 2023 11:30 AM
> To: Shenwei Wang <shenwei.wang@nxp.com>
> Cc: David S. Miller <davem@davemloft.net>; Eric Dumazet
> <edumazet@google.com>; Jakub Kicinski <kuba@kernel.org>; Paolo Abeni
> <pabeni@redhat.com>; Maxime Coquelin <mcoquelin.stm32@gmail.com>;
> Shawn Guo <shawnguo@kernel.org>; dl-linux-imx <linux-imx@nxp.com>;
> Giuseppe Cavallaro <peppe.cavallaro@st.com>; Alexandre Torgue
> <alexandre.torgue@foss.st.com>; Jose Abreu <joabreu@synopsys.com>; Sascha
> Hauer <s.hauer@pengutronix.de>; Pengutronix Kernel Team
> <kernel@pengutronix.de>; Fabio Estevam <festevam@gmail.com>;
> netdev@vger.kernel.org; linux-stm32@st-md-mailman.stormreply.com; linux-
> arm-kernel@lists.infradead.org; imx@lists.linux.dev; Frank Li <frank.li@nxp.com>
> Subject: Re: [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
>
>
> On Wed, Jul 26, 2023 at 04:10:11PM +0000, Shenwei Wang wrote:
> > > So, plat->phy_node will never ever be equal to your "dn" above.
> > > plat->phylink_node is the same as dwmac->dev->of_node above, and
> > > so plat->phylink_node will never be your "dn" above either.
> > >
> > > Those two together means that imx_dwmac_is_fixed_link() will
> > > _always_ return false, and thus most of the code you're adding is rather
> useless.
> > >
> > > It also means the code you're submitting probably hasn't been properly
> tested.
> > >
> > > Have you confirmed that imx_dwmac_is_fixed_link() will actually
> > > return true in your testing? Under what conditions did your testing
> > > reveal a true return value from this function?
> > >
> >
> > We can't make that assumption. In my testing code, I had trace
> > statements in that section to indicate the code was actually executed. You
> may get some clues from the following DTS:
> >
> > +&eqos {
> > +       pinctrl-names = "default";
> > +       pinctrl-0 = <&pinctrl_eqos_rgmii>;
> > +       phy-mode = "rgmii-rxid";
> > +       phy-handle = <&fixed0>;
> > +       status = "okay";
> > +
> > +       fixed0: fixed-link {
> > +               speed = <1000>;
> > +               full-duplex;
> > +       };
>
> This is just totally botched up.
>
> "fixed0" is _not_ a PHY, therefore you should NOT be referencing it in phy-
> handle. Please see the DT binding document:
>

If there is a hidden rule here, it should be added to the CHECK_DTBS schema target.
That way, users would get a warning or syntax error when running the tools, reminding
them to follow the undisclosed rule.

Thanks,
Shenwei

>   phy-handle:
>     $ref: /schemas/types.yaml#/definitions/phandle
>     description:
>       Specifies a reference to a node representing a PHY device.
>
>   fixed-link:
>     oneOf:
>       - $ref: /schemas/types.yaml#/definitions/uint32-array
>         deprecated: true
> ...
>       - type: object
>         additionalProperties: false
>         properties:
>           speed:
> ...
>
> As I said, fixed-link is _not_ a PHY, and thus phy-handle must *not* be used to
> point at it.
>
> The mere presence of a node called "fixed-link" will make this "eqos"
> device use that fixed-link node, and the phy-handle will be ignored.
>
> So sorry, but as far as your patch goes, it's a hard NAK from me right now until
> the DT description is actually correct.
>
> --
> RMK's Patch system:
> https://www.ar/
> mlinux.org.uk%2Fdeveloper%2Fpatches%2F&data=05%7C01%7Cshenwei.wang
> %40nxp.com%7C2396bc12c0524d7e006e08db8df58103%7C686ea1d3bc2b4c6f
> a92cd99c5c301635%7C0%7C0%7C638259857794101296%7CUnknown%7CTWF
> pbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6
> Mn0%3D%7C3000%7C%7C%7C&sdata=6VRey8tkgXhSaXSrf%2B0JVhwUivzVFPK
> QDzte0oKrIck%3D&reserved=0
> FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

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

[Andrew Lunn]

> > > +&eqos {
> > > +       pinctrl-names = "default";
> > > +       pinctrl-0 = <&pinctrl_eqos_rgmii>;
> > > +       phy-mode = "rgmii-rxid";
> > > +       phy-handle = <&fixed0>;
> > > +       status = "okay";
> > > +
> > > +       fixed0: fixed-link {
> > > +               speed = <1000>;
> > > +               full-duplex;
> > > +       };
> >
> > This is just totally botched up.
> >
> > "fixed0" is _not_ a PHY, therefore you should NOT be referencing it in phy-
> > handle. Please see the DT binding document:
> >
> 
> If there is a hidden rule here, it should be added to the CHECK_DTBS schema target.
> That way, users would get a warning or syntax error when running the tools, reminding
> them to follow the undisclosed rule.

I've no idea how to actually express that in yaml. phy-handle is just
a pointer to another node. There is no type associated to it, so i
don't see how we can say it needs to point to a node within an MDIO
bus. I wounder if it is possible to do a pattern match on the name of
the reference? It probably should match "*phy*".

    Andrew

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

[Shenwei Wang]

> -----Original Message-----
> From: Russell King <linux@armlinux.org.uk>
> Sent: Wednesday, July 26, 2023 12:01 PM
> To: Shenwei Wang <shenwei.wang@nxp.com>
> Cc: Vladimir Oltean <olteanv@gmail.com>; David S. Miller
> <davem@davemloft.net>; Eric Dumazet <edumazet@google.com>; Jakub
> Kicinski <kuba@kernel.org>; Paolo Abeni <pabeni@redhat.com>; Maxime
> Coquelin <mcoquelin.stm32@gmail.com>; Shawn Guo <shawnguo@kernel.org>;
> dl-linux-imx <linux-imx@nxp.com>; Giuseppe Cavallaro
> <peppe.cavallaro@st.com>; Alexandre Torgue <alexandre.torgue@foss.st.com>;
> Jose Abreu <joabreu@synopsys.com>; Sascha Hauer <s.hauer@pengutronix.de>;
> Pengutronix Kernel Team <kernel@pengutronix.de>; Fabio Estevam
> <festevam@gmail.com>; netdev@vger.kernel.org; linux-stm32@st-md-
> mailman.stormreply.com; linux-arm-kernel@lists.infradead.org;
> imx@lists.linux.dev; Frank Li <frank.li@nxp.com>
> Subject: Re: [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
>
>
> On Wed, Jul 26, 2023 at 03:59:38PM +0000, Shenwei Wang wrote:
> > > -----Original Message-----
> > > From: Russell King <linux@armlinux.org.uk>
> > > Sent: Wednesday, July 26, 2023 10:29 AM
> > > To: Shenwei Wang <shenwei.wang@nxp.com>
> > > Cc: Vladimir Oltean <olteanv@gmail.com>; David S. Miller
> > > <davem@davemloft.net>; Eric Dumazet <edumazet@google.com>; Jakub
> > > Kicinski <kuba@kernel.org>; Paolo Abeni <pabeni@redhat.com>; Maxime
> > > Coquelin <mcoquelin.stm32@gmail.com>; Shawn Guo
> > > <shawnguo@kernel.org>; dl-linux-imx <linux-imx@nxp.com>; Giuseppe
> > > Cavallaro <peppe.cavallaro@st.com>; Alexandre Torgue
> > > <alexandre.torgue@foss.st.com>; Jose Abreu <joabreu@synopsys.com>;
> > > Sascha Hauer <s.hauer@pengutronix.de>; Pengutronix Kernel Team
> > > <kernel@pengutronix.de>; Fabio Estevam <festevam@gmail.com>;
> > > netdev@vger.kernel.org; linux-stm32@st-md- mailman.stormreply.com;
> > > linux-arm-kernel@lists.infradead.org;
> > > imx@lists.linux.dev; Frank Li <frank.li@nxp.com>
> > > Subject: Re: [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
> > >
> > >
> > > On Wed, Jul 26, 2023 at 03:10:19PM +0000, Shenwei Wang wrote:
> > > > > 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.
> > >
> > > ... and this makes me wonder what DT node structure you think would
> > > describe a fixed-link.
> > >
> > > A valid ethernet device node would be:
> > >
> > >         dwmac-node {
> > >                 phy-handle = <&phy1>;
> > >         };
> > >
> > > In this case:
> > >         dwmac->dev->of_node points at "dwmac-node"
> > >         plat->phylink_node points at "dwmac-node"
> > >         plat->phy_node points at "phy1"
> > >         Your "dn" is NULL.
> > >         Therefore, your imx_dwmac_is_fixed_link() returns false.
> > >
> > >         dwmac-node {
> > >                 fixed-link {
> > >                         speed = <...>;
> > >                         full-duplex;
> > >                 };
> > >         };
> > >
> > > In this case:
> > >         dwmac->dev->of_node points at "dwmac-node"
> > >         plat->phylink_node points at "dwmac-node"
> > >         plat->phy_node is NULL
> > >         Your "dn" points at the "fixed-link" node.
> > >         Therefore, your imx_dwmac_is_fixed_link() also returns false.
> > >
> > > Now, as far as your comment "What I need to determine is if the PHY
> > > currently in use is a fixed-link." I'm just going "Eh? What?" at
> > > that, because it makes zero sense to me.
> > >
> > > stmmac uses phylink. phylink doesn't use a PHY for fixed-links,
> > > unlike the old phylib-based fixed-link implementation that software-
> emulated a clause-22 PHY.
> > > With phylink, when fixed-link is specified, there is _no_ PHY.
> >
> > So you mean the fixed-link node will always be the highest priority to
> > be used in the phylink use case?
>
> Yes, because that is how all network drivers have behaved. If you look at the
> function that Vladimir pointed out, then you will notice that the mere presence
> of a fixed-link node makes it a "fixed link".
>

Then, the way this phylink driver behaves makes the rest of the discussion kind of pointless
for now, because I don't actually need fix_mac_speed to give me any interface info now.
The basic of_phy_is_fixed_link check does the job for me.

Not sure why you think it's inefficient - could you explain that part?

Thanks,
Shenwei

> > If so, I just need to check if there is a fixed-link node as Vladimir pointed out,
> right?
>
> You could, but that is grossly inefficient, and I will NAK it because by doing so, it
> makes this messy driver even worse.
>
> > > There is no need to do any of this poking about to determine if the
> > > link that is being brought up is a fixed-link or not, because
> > > phylink's callbacks into the MAC driver already contain this
> > > information in the "mode" argument. However, that is not passed to
> > > the driver's internal
> > > priv->plat->fix_mac_speed() method - but this is the information you
> > > need.
> > >
> >
> > Yes, you are right. The best way is to change the fix_mac_speed
> > prototype but it will change several other platforms. That's why I didn't go that
> way.
>
> Why is that a problem?
>
> I really don't get this "I can't get at information I need without changing a driver
> internal interface, so I'll write some really inefficient code to work around the
> problem and make the driver even more messy" attitude.
>
> It's not like you're changing a publicly visible API - it's a driver private API and all
> the users of it are in the kernel tree.
>
> A standard part of open source development is not to bodge around existing
> code, but to implement efficient solutions to problems.
>
> As phylink *already* tells stmmac_mac_link_up() whether it is operating with a
> PHY, fixed-link, or in-band mode, the stmmac layer has the information you
> need, but doesn't pass this into the fix_mac_speed() function.
>
> The best solution to this is *not* to bodge around it by trying to second-guess
> what's going on and thus creating messy code.
>
> Given that we have the full source available which we can modify, then changing
> things like this function pointer prototype is absolutely acceptable, and in this
> case is the correct way to address the issue you have.
>
> --
> RMK's Patch system:
> https://www.ar/
> mlinux.org.uk%2Fdeveloper%2Fpatches%2F&data=05%7C01%7Cshenwei.wang
> %40nxp.com%7C4a805e10cd20434538f608db8df9edf5%7C686ea1d3bc2b4c6fa
> 92cd99c5c301635%7C0%7C0%7C638259876795921310%7CUnknown%7CTWFp
> bGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6
> Mn0%3D%7C3000%7C%7C%7C&sdata=9A30fzTXCfc6fYU4Ttijm0c7Piy18tFUWW
> yxtyQoJLo%3D&reserved=0
> FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

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

[Russell King (Oracle)]

On Wed, Jul 26, 2023 at 06:47:15PM +0000, Shenwei Wang wrote:
> 
> 
> > -----Original Message-----
> > From: Russell King <linux@armlinux.org.uk>
> > Sent: Wednesday, July 26, 2023 12:01 PM
> > To: Shenwei Wang <shenwei.wang@nxp.com>
> > Cc: Vladimir Oltean <olteanv@gmail.com>; David S. Miller
> > <davem@davemloft.net>; Eric Dumazet <edumazet@google.com>; Jakub
> > Kicinski <kuba@kernel.org>; Paolo Abeni <pabeni@redhat.com>; Maxime
> > Coquelin <mcoquelin.stm32@gmail.com>; Shawn Guo <shawnguo@kernel.org>;
> > dl-linux-imx <linux-imx@nxp.com>; Giuseppe Cavallaro
> > <peppe.cavallaro@st.com>; Alexandre Torgue <alexandre.torgue@foss.st.com>;
> > Jose Abreu <joabreu@synopsys.com>; Sascha Hauer <s.hauer@pengutronix.de>;
> > Pengutronix Kernel Team <kernel@pengutronix.de>; Fabio Estevam
> > <festevam@gmail.com>; netdev@vger.kernel.org; linux-stm32@st-md-
> > mailman.stormreply.com; linux-arm-kernel@lists.infradead.org;
> > imx@lists.linux.dev; Frank Li <frank.li@nxp.com>
> > Subject: Re: [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
> >
> >
> > On Wed, Jul 26, 2023 at 03:59:38PM +0000, Shenwei Wang wrote:
> > > > -----Original Message-----
> > > > From: Russell King <linux@armlinux.org.uk>
> > > > Sent: Wednesday, July 26, 2023 10:29 AM
> > > > To: Shenwei Wang <shenwei.wang@nxp.com>
> > > > Cc: Vladimir Oltean <olteanv@gmail.com>; David S. Miller
> > > > <davem@davemloft.net>; Eric Dumazet <edumazet@google.com>; Jakub
> > > > Kicinski <kuba@kernel.org>; Paolo Abeni <pabeni@redhat.com>; Maxime
> > > > Coquelin <mcoquelin.stm32@gmail.com>; Shawn Guo
> > > > <shawnguo@kernel.org>; dl-linux-imx <linux-imx@nxp.com>; Giuseppe
> > > > Cavallaro <peppe.cavallaro@st.com>; Alexandre Torgue
> > > > <alexandre.torgue@foss.st.com>; Jose Abreu <joabreu@synopsys.com>;
> > > > Sascha Hauer <s.hauer@pengutronix.de>; Pengutronix Kernel Team
> > > > <kernel@pengutronix.de>; Fabio Estevam <festevam@gmail.com>;
> > > > netdev@vger.kernel.org; linux-stm32@st-md- mailman.stormreply.com;
> > > > linux-arm-kernel@lists.infradead.org;
> > > > imx@lists.linux.dev; Frank Li <frank.li@nxp.com>
> > > > Subject: Re: [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
> > > >
> > > >
> > > > On Wed, Jul 26, 2023 at 03:10:19PM +0000, Shenwei Wang wrote:
> > > > > > 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.
> > > >
> > > > ... and this makes me wonder what DT node structure you think would
> > > > describe a fixed-link.
> > > >
> > > > A valid ethernet device node would be:
> > > >
> > > >         dwmac-node {
> > > >                 phy-handle = <&phy1>;
> > > >         };
> > > >
> > > > In this case:
> > > >         dwmac->dev->of_node points at "dwmac-node"
> > > >         plat->phylink_node points at "dwmac-node"
> > > >         plat->phy_node points at "phy1"
> > > >         Your "dn" is NULL.
> > > >         Therefore, your imx_dwmac_is_fixed_link() returns false.
> > > >
> > > >         dwmac-node {
> > > >                 fixed-link {
> > > >                         speed = <...>;
> > > >                         full-duplex;
> > > >                 };
> > > >         };
> > > >
> > > > In this case:
> > > >         dwmac->dev->of_node points at "dwmac-node"
> > > >         plat->phylink_node points at "dwmac-node"
> > > >         plat->phy_node is NULL
> > > >         Your "dn" points at the "fixed-link" node.
> > > >         Therefore, your imx_dwmac_is_fixed_link() also returns false.
> > > >
> > > > Now, as far as your comment "What I need to determine is if the PHY
> > > > currently in use is a fixed-link." I'm just going "Eh? What?" at
> > > > that, because it makes zero sense to me.
> > > >
> > > > stmmac uses phylink. phylink doesn't use a PHY for fixed-links,
> > > > unlike the old phylib-based fixed-link implementation that software-
> > emulated a clause-22 PHY.
> > > > With phylink, when fixed-link is specified, there is _no_ PHY.
> > >
> > > So you mean the fixed-link node will always be the highest priority to
> > > be used in the phylink use case?
> >
> > Yes, because that is how all network drivers have behaved. If you look at the
> > function that Vladimir pointed out, then you will notice that the mere presence
> > of a fixed-link node makes it a "fixed link".
> >
> 
> Then, the way this phylink driver behaves makes the rest of the discussion kind of pointless
> for now, because I don't actually need fix_mac_speed to give me any interface info now.
> The basic of_phy_is_fixed_link check does the job for me.
> 
> Not sure why you think it's inefficient - could you explain that part?

Because of_phy_is_fixed_link() has to chase various pointers, walk
the child nodes and do a string compare on each, whereas you could
just be testing an integer!

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

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

[Shenwei Wang]

> -----Original Message-----
> From: Russell King <linux@armlinux.org.uk>
> Sent: Wednesday, July 26, 2023 2:02 PM
> To: Shenwei Wang <shenwei.wang@nxp.com>
> Cc: Vladimir Oltean <olteanv@gmail.com>; David S. Miller
> <davem@davemloft.net>; Eric Dumazet <edumazet@google.com>; Jakub
> Kicinski <kuba@kernel.org>; Paolo Abeni <pabeni@redhat.com>; Maxime
> Coquelin <mcoquelin.stm32@gmail.com>; Shawn Guo <shawnguo@kernel.org>;
> dl-linux-imx <linux-imx@nxp.com>; Giuseppe Cavallaro
> <peppe.cavallaro@st.com>; Alexandre Torgue <alexandre.torgue@foss.st.com>;
> Jose Abreu <joabreu@synopsys.com>; Sascha Hauer <s.hauer@pengutronix.de>;
> Pengutronix Kernel Team <kernel@pengutronix.de>; Fabio Estevam
> <festevam@gmail.com>; netdev@vger.kernel.org; linux-stm32@st-md-
> mailman.stormreply.com; linux-arm-kernel@lists.infradead.org;
> imx@lists.linux.dev; Frank Li <frank.li@nxp.com>
> Subject: Re: [EXT] Re: [PATCH] net: stmmac: dwmac-imx: pause the TXC clock in
> fixed-link
> >
> > Then, the way this phylink driver behaves makes the rest of the
> > discussion kind of pointless for now, because I don't actually need
> fix_mac_speed to give me any interface info now.
> > The basic of_phy_is_fixed_link check does the job for me.
> >
> > Not sure why you think it's inefficient - could you explain that part?
>
> Because of_phy_is_fixed_link() has to chase various pointers, walk the child
> nodes and do a string compare on each, whereas you could just be testing an
> integer!
>

I don't think It's worth the effort to change the definition of fix_mac_speed across all platforms,
because the function is only called once when the interface is up.

Thanks,
Shenwei

> --
> RMK's Patch system:
> https://www.ar/
> mlinux.org.uk%2Fdeveloper%2Fpatches%2F&data=05%7C01%7Cshenwei.wang
> %40nxp.com%7C682cfc4392cc4e4b539508db8e0ae172%7C686ea1d3bc2b4c6fa
> 92cd99c5c301635%7C0%7C0%7C638259949620016392%7CUnknown%7CTWFp
> bGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6
> Mn0%3D%7C3000%7C%7C%7C&sdata=sJy6gzd1LBe%2Bikz1lL3Pq4fK30ehMY%2
> BJKQMJbkOFp4I%3D&reserved=0
> FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

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

[Russell King (Oracle)]

On Wed, Jul 26, 2023 at 07:17:59PM +0000, Shenwei Wang wrote:
> > Because of_phy_is_fixed_link() has to chase various pointers, walk the child
> > nodes and do a string compare on each, whereas you could just be testing an
> > integer!
> >
> 
> I don't think It's worth the effort to change the definition of fix_mac_speed across all platforms,
> because the function is only called once when the interface is up.

If you look at Feiyang Chen's patch set, then his first patch of his
set adds a pointer to struct stmmac_priv to a whole bunch of callbacks
used between the stmmac core and the various implementations.

If you're not willing to do it, then I will send a patch instead.

I don't see what the problem is.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

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

[Russell King (Oracle)]

On Wed, Jul 26, 2023 at 07:17:59PM +0000, Shenwei Wang wrote:
> > Because of_phy_is_fixed_link() has to chase various pointers, walk the child
> > nodes and do a string compare on each, whereas you could just be testing an
> > integer!
> >
> 
> I don't think It's worth the effort to change the definition of fix_mac_speed across all platforms,
> because the function is only called once when the interface is up.

If you look at Feiyang Chen's patch set, then his first patch of his
set adds a pointer to struct stmmac_priv to a whole bunch of callbacks
used between the stmmac core and the various implementations.

If you're not willing to do it, then I will send a patch instead.

I don't see what the problem is.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Shenwei Wang]

> -----Original Message-----
> From: Russell King <linux@armlinux.org.uk>
> Sent: Thursday, July 27, 2023 3:59 AM
> To: Shenwei Wang <shenwei.wang@nxp.com>
> Cc: Vladimir Oltean <olteanv@gmail.com>; David S. Miller
> <davem@davemloft.net>; Eric Dumazet <edumazet@google.com>; Jakub
> Kicinski <kuba@kernel.org>; Paolo Abeni <pabeni@redhat.com>; Maxime
> Coquelin <mcoquelin.stm32@gmail.com>; Shawn Guo <shawnguo@kernel.org>;
> dl-linux-imx <linux-imx@nxp.com>; Giuseppe Cavallaro
> <peppe.cavallaro@st.com>; Alexandre Torgue <alexandre.torgue@foss.st.com>;
> Jose Abreu <joabreu@synopsys.com>; Sascha Hauer <s.hauer@pengutronix.de>;
> Pengutronix Kernel Team <kernel@pengutronix.de>; Fabio Estevam
> <festevam@gmail.com>; netdev@vger.kernel.org; linux-stm32@st-md-
> mailman.stormreply.com; linux-arm-kernel@lists.infradead.org;
> imx@lists.linux.dev; Frank Li <frank.li@nxp.com>
> Subject: Re: [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
>
>
> On Wed, Jul 26, 2023 at 07:17:59PM +0000, Shenwei Wang wrote:
> > > Because of_phy_is_fixed_link() has to chase various pointers, walk
> > > the child nodes and do a string compare on each, whereas you could
> > > just be testing an integer!
> > >
> >
> > I don't think It's worth the effort to change the definition of
> > fix_mac_speed across all platforms, because the function is only called once
> when the interface is up.
>
> If you look at Feiyang Chen's patch set, then his first patch of his set adds a
> pointer to struct stmmac_priv to a whole bunch of callbacks used between the
> stmmac core and the various implementations.
>
> If you're not willing to do it, then I will send a patch instead.
>
> I don't see what the problem is.
>

Never mind. I will pull this off.

Thanks,
Shenwei

> --
> RMK's Patch system:
> https://www.ar/
> mlinux.org.uk%2Fdeveloper%2Fpatches%2F&data=05%7C01%7Cshenwei.wang
> %40nxp.com%7C70e2358c209e47c8612608db8e7fb5cc%7C686ea1d3bc2b4c6fa
> 92cd99c5c301635%7C0%7C0%7C638260451379427007%7CUnknown%7CTWFp
> bGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6
> Mn0%3D%7C3000%7C%7C%7C&sdata=KQCQ%2B6t%2BMz0EQsCAYOJ%2BY3Of
> OG68KqJB0%2FCLiGnULRo%3D&reserved=0
> FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

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

[Shenwei Wang]

> -----Original Message-----
> From: Russell King <linux@armlinux.org.uk>
> Sent: Thursday, July 27, 2023 3:59 AM
> To: Shenwei Wang <shenwei.wang@nxp.com>
> Cc: Vladimir Oltean <olteanv@gmail.com>; David S. Miller
> <davem@davemloft.net>; Eric Dumazet <edumazet@google.com>; Jakub
> Kicinski <kuba@kernel.org>; Paolo Abeni <pabeni@redhat.com>; Maxime
> Coquelin <mcoquelin.stm32@gmail.com>; Shawn Guo <shawnguo@kernel.org>;
> dl-linux-imx <linux-imx@nxp.com>; Giuseppe Cavallaro
> <peppe.cavallaro@st.com>; Alexandre Torgue <alexandre.torgue@foss.st.com>;
> Jose Abreu <joabreu@synopsys.com>; Sascha Hauer <s.hauer@pengutronix.de>;
> Pengutronix Kernel Team <kernel@pengutronix.de>; Fabio Estevam
> <festevam@gmail.com>; netdev@vger.kernel.org; linux-stm32@st-md-
> mailman.stormreply.com; linux-arm-kernel@lists.infradead.org;
> imx@lists.linux.dev; Frank Li <frank.li@nxp.com>
> Subject: Re: [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
>
>
> On Wed, Jul 26, 2023 at 07:17:59PM +0000, Shenwei Wang wrote:
> > > Because of_phy_is_fixed_link() has to chase various pointers, walk
> > > the child nodes and do a string compare on each, whereas you could
> > > just be testing an integer!
> > >
> >
> > I don't think It's worth the effort to change the definition of
> > fix_mac_speed across all platforms, because the function is only called once
> when the interface is up.
>
> If you look at Feiyang Chen's patch set, then his first patch of his set adds a
> pointer to struct stmmac_priv to a whole bunch of callbacks used between the
> stmmac core and the various implementations.
>
> If you're not willing to do it, then I will send a patch instead.
>
> I don't see what the problem is.
>

Never mind. I will pull this off.

Thanks,
Shenwei

> --
> RMK's Patch system:
> https://www.ar/
> mlinux.org.uk%2Fdeveloper%2Fpatches%2F&data=05%7C01%7Cshenwei.wang
> %40nxp.com%7C70e2358c209e47c8612608db8e7fb5cc%7C686ea1d3bc2b4c6fa
> 92cd99c5c301635%7C0%7C0%7C638260451379427007%7CUnknown%7CTWFp
> bGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6
> Mn0%3D%7C3000%7C%7C%7C&sdata=KQCQ%2B6t%2BMz0EQsCAYOJ%2BY3Of
> OG68KqJB0%2FCLiGnULRo%3D&reserved=0
> FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel