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

[Vladimir Oltean <olteanv@gmail.com>]

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.