Re: [PATCH net-next 9/9] net: stmmac: convert to phylink managed EEE support

[Jon Hunter <jonathanh@nvidia.com>]

On 14/02/2025 11:21, Russell King (Oracle) wrote:
> On Fri, Feb 14, 2025 at 10:58:55AM +0000, Jon Hunter wrote:
>> Thanks for the feedback. So ...
>>
>> 1. I can confirm that suspend works if I disable EEE via ethtool
>> 2. Prior to this change I do see phy_eee_rx_clock_stop being called
>>     to enable the clock resuming from suspend, but after this change
>>     it is not.
>>
>> Prior to this change I see (note the prints around 389-392 are when
>> we resume from suspend) ...
>>
>> [    4.654454] Broadcom BCM89610 stmmac-0:00: phy_eee_rx_clock_stop: clk_stop_enable 0
> 
> This is a bug in phylink - it shouldn't have been calling
> phy_eee_rx_clock_stop() where a MAC doesn't support phylink managed EEE.
> 
>> [    4.723123] dwc-eth-dwmac 2490000.ethernet eth0: configuring for phy/rgmii link mode
>> [    7.629652] Broadcom BCM89610 stmmac-0:00: phy_eee_rx_clock_stop: clk_stop_enable 1
> 
> Presumably, this is when the link comes up before suspend, so the PHY
> has been configured to allow the RX clock to be stopped prior to suspend
> 
>> [  389.086185] dwc-eth-dwmac 2490000.ethernet eth0: configuring for phy/rgmii link mode
>> [  392.863744] Broadcom BCM89610 stmmac-0:00: phy_eee_rx_clock_stop: clk_stop_enable 1
> 
> Presumably, as this is after resume, this is again when the link comes
> up (that's the only time that stmmac calls phy_eee_rx_clock_stop().)
> 
>> After this change I see ...
>>
>> [    4.644614] Broadcom BCM89610 stmmac-0:00: phy_eee_rx_clock_stop: clk_stop_enable 1
>> [    4.679224] dwc-eth-dwmac 2490000.ethernet eth0: configuring for phy/rgmii link mode
>> [  191.219828] dwc-eth-dwmac 2490000.ethernet eth0: configuring for phy/rgmii link mode
> 
> To me, this looks no different - the PHY was configured for clock stop
> before suspending in both cases.
> 
> However, something else to verify with the old code - after boot and the
> link comes up (so you get the second phy_eee_rx_clock_stop() at 7s),
> try unplugging the link and re-plugging it. Then try suspending.

OK will do. I will have to try that when I am back in the office next week.

> The point of this test is to verify whether the PHY ignores changes to
> the RX clock stop configuration while the link is up.
> 
> 
> 
> The next stage is to instrument dwmac4_set_eee_mode(),
> dwmac4_reset_eee_mode() and dwmac4_set_eee_lpi_entry_timer() to print
> the final register values in each function vs dwmac4_set_lpi_mode() in
> the new code. Also, I think instrumenting stmmac_common_interrupt() to
> print a message when we get either CORE_IRQ_TX_PATH_IN_LPI_MODE or
> CORE_IRQ_TX_PATH_EXIT_LPI_MODE indicating a change in LPI state would
> be a good idea.
> 
> I'd like to see how this all ties up with suspend, resume, link up
> and down events, so please don't trim the log so much.

OK thanks. I will instrument these too and get some more logs.

Cheers!
Jon

-- 
nvpublic