Re: [PATCH 3/3] net: stmmac: Add DWMAC glue layer for Renesas GBETH

[Russell King (Oracle)]

On Wed, Mar 05, 2025 at 09:26:43PM +0000, Lad, Prabhakar wrote:
> I did investigate on these lines:
> 
> 1] With my current patch series I have the below in remove callback
> 
> +static void renesas_gbeth_remove(struct platform_device *pdev)
> +{
> +       struct renesas_gbeth *gbeth = get_stmmac_bsp_priv(&pdev->dev);
> +
> +       stmmac_dvr_remove(&pdev->dev);
> +
> +       clk_bulk_disable_unprepare(gbeth->num_clks, gbeth->clks);
> +}
> 
> After dumping the CLK registers I found out that the Rx and Rx-180 CLK
> never got turned OFF after unbind.

I think that's where further investigation needs to happen. This
suggests there's more enables than disables for these clocks, but
there's nothing that I can see in your submitted driver that would
account for that behaviour.

> 2] I replaced the remove callback with below ie first turn OFF
> Tx-180/Rx/Rx-180 clocks
> 
> +static void renesas_gbeth_remove(struct platform_device *pdev)
> +{
> +       struct renesas_gbeth *gbeth = get_stmmac_bsp_priv(&pdev->dev);
> +
> +       clk_bulk_disable_unprepare(gbeth->num_clks, gbeth->clks);
> +
> +      stmmac_dvr_remove(&pdev->dev);
> +}
> 
> After dumping the CLK registers I confirmed all the clocks were OFF
> (CSR/PCLK/Tx/Tx-180/Rx/Rx-180) after unbind operation. Now when I do a
> bind operation Rx clock fails to enable, which is probably because the
> PHY is not providing any clock.

However, disabling clocks _before_ unregistering the net device is a
bad thing to do! The netdev could still be in use.

You can add:

	if (ndev->phydev)
		phy_eee_rx_clock_stop(ndev->phydev, false);

just before unregister_netdev() in stmmac_dvr_remove() only as a way
to test out that idea.

Do the clock registers you refer to only update when the relevant
clocks are actually running?

> > However, PHYs that have negotiated EEE are permitted to stop their
> > receive clock, which can be enabled by an appropriate control bit.
> > phy_eee_rx_clock_stop() manipulates that bit. stmmac has in most
> > cases permitted the PHY to stop its receive clock.
> >
> You mean phy_eee_rx_clock_stop() is the one which tells the PHY to
> disable the Rx clocks? Actually I tried the below hunk with this as
> well the Rx clock fails to be turned ON after unbind/bind operation.

phy_eee_rx_clock_stop() doesn't turn the clock on/off per-se, it
controls the bit which gives the PHY permission to disable the clock
when the media is in EEE low-power mode. Note that 802.3 does not
give a default setting for this bit, so:

> diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
> index 0ba434104f5b..e16f4a6f5715 100644
> --- a/drivers/net/phy/phy.c
> +++ b/drivers/net/phy/phy.c
> @@ -1756,6 +1756,7 @@ EXPORT_SYMBOL_GPL(phy_eee_tx_clock_stop_capable);
>   */
>  int phy_eee_rx_clock_stop(struct phy_device *phydev, bool clk_stop_enable)
>  {
> +       return 0;
>         int ret;
> 
>         /* Configure the PHY to stop receiving xMII

May not be wise, and if you want to ensure that the PHY does not stop
the clock, then forcing clk_stop_enable to zero would be better.

> > NVidia have been a recent victim of this - it is desirable to allow
> > receive clock stop, but there hasn't been the APIs in the kernel
> > to allow MAC drivers to re-enable the clock when they need it.
> >
> > Up until now, I had thought this was just a suspend/resume issue
> > (which is NVidia's reported case). Your testing suggests that it is
> > more widespread than that.
> >
> > While I've been waiting to hear from you, I've prepared some patches
> > that change the solution that I proposed for NVidia (currently on top
> > of that patch set).
>
> I tried your latest patches [0], this didnt resolve the issue.

I assume without the modification to phy_eee_rx_clock_stop() above -
thanks. If so, then your issue is not EEE related.

> [0] https://lore.kernel.org/all/Z8bbnSG67rqTj0pH@xxxxxxxxxxxxxxxxxxxxx/

Wasn't quite the latest, but still had the same build bug (thanks for
reporting, now fixed.) Latest is equivalent so no need to re-test.

> > However, before I proceed with them, I need you to get to the bottom
> > of why:
> >
> > # ip li set dev $if down
> > # ip li set dev $if up
> >
> > doesn't trigger it, but removing and re-inserting the module does.
> >
> Doing the above does not turn OFF/ON all the clocks. Looking at the
> dump from the CLK driver on my platform only stmmaceth and pclk are
> the clocks which get toggled and rest remain ON. Note Im not sure if
> the PHY is disabling the Rx clocks when I run ip li set dev $if down I
> cannot scope that pin on the board.
> 
> Please let me know if you have any pointers for me to look further
> into this issue.

Given the behaviour that you're reporting from your clock layer, I'm
wondering if the problem is actually there... it seems weird that clocks
aren't being turned off and on when they should.

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