Re: [PATCH] net: phy: at803x: Improve hibernation support on start up

Re: [PATCH] net: phy: at803x: Improve hibernation support on start up

[Oleksij Rempel]

CCing clk folks in the loop.

On Wed, Aug 09, 2023 at 05:37:45AM +0000, Wei Fang wrote:
...
> > Hm.. how about officially defining this PHY as the clock provider and disable
> > PHY automatic hibernation as long as clock is acquired?
> > 
> Sorry, I don't know much about the clock provider/consumer, but I think there
> will be more changes if we use clock provider/consume mechanism.

Yes, more changes will be needed.

> Furthermore,
> we would expect the hibernation mode is enabled when the ethernet interface
> is brought up but the cable is not plugged, that is to say, we only need the PHY to
> provide the clock for a while to make the MAC reset successfully. 

Means, if external clock is not provided, MAC is not fully functional.
Correct?

What kind of MAC operation will fail in this case?
For example, if stmmac_open() fails without external clock, will
stmmac_release() work properly? Will we be able to do any configuration
on an interface which is opened, but without active link and hibernated
clock? How about self tests?

> Therefore, I think
> the current approach is more simple and effective, and it takes full advantage of the
> characteristics of the hardware (The PHY will continue to provide the clock about
> 10 seconds after hibernation mode is enabled when the cable is not plugged and
> automatically disable the clock after 10 seconds, so the 10 seconds is enough for
> the MAC to reset successfully).

If multiple independent operations are synchronized based on the
assumption that 10 seconds should be enough, bad thing happens.

If fully functional external clock provider is need to initialize the
MAC, just disabling this clock on already initialized HW without doing
proper re-initialization sequence is usually bad idea. HW may get some
glitch which will make troubleshooting a pain.

Regards,
Oleksij
-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH] net: phy: at803x: Improve hibernation support on start up

[Russell King (Oracle)]

On Wed, Aug 09, 2023 at 08:08:36AM +0200, Oleksij Rempel wrote:
> If fully functional external clock provider is need to initialize the
> MAC, just disabling this clock on already initialized HW without doing
> proper re-initialization sequence is usually bad idea. HW may get some
> glitch which will make troubleshooting a pain.

There are cases where the PHY sits on a MDIO bus that is created
by the ethernet MAC driver, which means the PHY only exists during
the ethernet MAC driver probe.

I think that provided the clock is only obtained after we know the
PHY is present, that would probably be fine - but doing it before
the MDIO bus has been created will of course cause problems.

We've had these issues before with stmmac, so this "stmmac needs the
PHY receive clock" is nothing new - it's had problems with system
suspend/resume in the past, and I've made suggestions... and when
there's been two people trying to work on it, I've attempted to get
them to talk to each other which resulted in nothing further
happening.

Another solution could possibly be that we reserve bit 30 on the
PHY dev_flags to indicate that the receive clock must always be
provided. I suspect that would have an advantage in another
situation - for EEE, there's a control bit which allows the
receive clock to be stopped while the link is in low-power state.
If the MAC always needs the receive clock, then obviously that
should be blocked.

-- 
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: phy: at803x: Improve hibernation support on start up

[Wei Fang]

> > Furthermore,
> > we would expect the hibernation mode is enabled when the ethernet
> > interface is brought up but the cable is not plugged, that is to say,
> > we only need the PHY to provide the clock for a while to make the MAC
> reset successfully.
> 
> Means, if external clock is not provided, MAC is not fully functional.
> Correct?
> 
The MAC will failed to do software reset if the PHY input clocks are not
present. According to the datasheet from Synopsys, the software reset
is for the MAC and the DMA controller reset the logic and all internal
registers of the DMA, MTL, and MAC.

One obvious error log can be seen from the console shows as follows.
[   26.670276] imx-dwmac 30bf0000.ethernet: Failed to reset the dma
[   26.676322] imx-dwmac 30bf0000.ethernet eth1: stmmac_hw_setup: DMA engine initialization failed
[   26.685103] imx-dwmac 30bf0000.ethernet eth1: __stmmac_open: Hw setup failed

I believe other manufacturers who use the dwmac IP also have encountered
the similar issue and use other methods to resolve it, such as intel,
49725ffc15fc ("net: stmmac: power up/down serdes in stmmac_open/release ")

> What kind of MAC operation will fail in this case?
Below are the steps from Marek to reproduce the issue.
- Make sure "qca,disable-hibernation-mode" is NOT present in PHY DT node
- Boot the machine with NO ethernet cable plugged into the affected port
   (i.e. the EQoS port), this is important
- Make sure the EQoS MAC is not brought up e.g. by systemd-networkd or
   whatever other tool, I use busybox initramfs for testing with plain
   script as init (it mounts the various filesystems and runs /bin/sh)
- Wait longer than 10 seconds
- If possible, measure AR8031 PHY pin 33 RX_CLK, wait for the RX_CLK to
   be turned OFF by the PHY (means PHY entered hibernation)
- ifconfig ethN up -- try to bring up the EQoS MAC <observe failure>

But the situation I encountered on i.MX8DXL-EVK platform was not as
complicated as above. At that time, it only took 3 steps to reproduce
this issue. But now, the latest upstream tree cannot reproduce this
issue based on the following three steps. Something has been changed,
I think it should be feafeb53140a (arm64: dts: imx8dxl-evk: Fix eqos phy reset gpio).
- unplug the cable
- ifconfig eth0 down and wait for about 10 seconds
- ifconfig eth0 up

> For example, if stmmac_open() fails without external clock, will
> stmmac_release() work properly?
Actually, I don't know much about stmmac driver and the dwmac IP,
Because I'm not responsible for this IP on NXP i.MX platform. But I
have a look at the code of stmmac_release(), I think stmmac_release()
will work properly, because it invokes phylink_stop() and phylink_disconnect_phy()
first which will disable the clock from PHY.

>Will we be able to do any configuration on
> an interface which is opened, but without active link and hibernated clock?
I don't know, but as far as I know, we can not set the VLAN filter successfully
due to lack of clock. So there may be other configurations that depend on the
clock.

> How about self tests?
> 
> > Therefore, I think
> > the current approach is more simple and effective, and it takes full
> > advantage of the characteristics of the hardware (The PHY will
> > continue to provide the clock about
> > 10 seconds after hibernation mode is enabled when the cable is not
> > plugged and automatically disable the clock after 10 seconds, so the
> > 10 seconds is enough for the MAC to reset successfully).
> 
> If multiple independent operations are synchronized based on the
> assumption that 10 seconds should be enough, bad thing happens.
> 
> If fully functional external clock provider is need to initialize the MAC, just
> disabling this clock on already initialized HW without doing proper
> re-initialization sequence is usually bad idea. HW may get some glitch which
> will make troubleshooting a pain.
> 

Re: [PATCH] net: phy: at803x: Improve hibernation support on start up

[Russell King (Oracle)]

On Thu, Aug 10, 2023 at 03:28:13AM +0000, Wei Fang wrote:
> > > Furthermore,
> > > we would expect the hibernation mode is enabled when the ethernet
> > > interface is brought up but the cable is not plugged, that is to say,
> > > we only need the PHY to provide the clock for a while to make the MAC
> > reset successfully.
> > 
> > Means, if external clock is not provided, MAC is not fully functional.
> > Correct?
> > 
> The MAC will failed to do software reset if the PHY input clocks are not
> present.

Yes, that's well known to me - it's been brought up before with stmmac
and phylink. I said earlier in this thread about this.

> > For example, if stmmac_open() fails without external clock, will
> > stmmac_release() work properly?
> Actually, I don't know much about stmmac driver and the dwmac IP,
> Because I'm not responsible for this IP on NXP i.MX platform. But I
> have a look at the code of stmmac_release(), I think stmmac_release()
> will work properly, because it invokes phylink_stop() and phylink_disconnect_phy()
> first which will disable the clock from PHY.

When tearing down a network device, there should be no reason to call
phylink_stop() - unregistering the netdev will take the network device
down, and thus call the .ndo_stop method if the device was up. .ndo_stop
should already be calling phylink_stop().

I just don't get why driver authors don't check things like this...

-- 
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: phy: at803x: Improve hibernation support on start up

[Russell King (Oracle)]

On Wed, Aug 09, 2023 at 09:43:49AM +0100, Russell King (Oracle) wrote:
> On Wed, Aug 09, 2023 at 08:08:36AM +0200, Oleksij Rempel wrote:
> > If fully functional external clock provider is need to initialize the
> > MAC, just disabling this clock on already initialized HW without doing
> > proper re-initialization sequence is usually bad idea. HW may get some
> > glitch which will make troubleshooting a pain.
> 
> There are cases where the PHY sits on a MDIO bus that is created
> by the ethernet MAC driver, which means the PHY only exists during
> the ethernet MAC driver probe.
> 
> I think that provided the clock is only obtained after we know the
> PHY is present, that would probably be fine - but doing it before
> the MDIO bus has been created will of course cause problems.
> 
> We've had these issues before with stmmac, so this "stmmac needs the
> PHY receive clock" is nothing new - it's had problems with system
> suspend/resume in the past, and I've made suggestions... and when
> there's been two people trying to work on it, I've attempted to get
> them to talk to each other which resulted in nothing further
> happening.
> 
> Another solution could possibly be that we reserve bit 30 on the
> PHY dev_flags to indicate that the receive clock must always be
> provided. I suspect that would have an advantage in another
> situation - for EEE, there's a control bit which allows the
> receive clock to be stopped while the link is in low-power state.
> If the MAC always needs the receive clock, then obviously that
> should be blocked.

Something like this for starters:

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index fcab363d8dfa..a954f1d61709 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -1254,6 +1254,11 @@ static int stmmac_phy_setup(struct stmmac_priv *priv)
 			~(MAC_10HD | MAC_100HD | MAC_1000HD);
 	priv->phylink_config.mac_managed_pm = true;
 
+	/* stmmac always requires a receive clock in order for things like
+	 * hardware reset to work.
+	 */
+	priv->phylink_config.mac_requires_rxc = true;
+
 	phylink = phylink_create(&priv->phylink_config, fwnode,
 				 mode, &stmmac_phylink_mac_ops);
 	if (IS_ERR(phylink))
diff --git a/drivers/net/phy/at803x.c b/drivers/net/phy/at803x.c
index 13c4121fa309..619a63a0d14f 100644
--- a/drivers/net/phy/at803x.c
+++ b/drivers/net/phy/at803x.c
@@ -990,7 +990,8 @@ static int at803x_hibernation_mode_config(struct phy_device *phydev)
 	/* The default after hardware reset is hibernation mode enabled. After
 	 * software reset, the value is retained.
 	 */
-	if (!(priv->flags & AT803X_DISABLE_HIBERNATION_MODE))
+	if (!(priv->flags & AT803X_DISABLE_HIBERNATION_MODE) &&
+	    !(phydev->dev_flags & PHY_F_RXC_ALWAYS_ON))
 		return 0;
 
 	return at803x_debug_reg_mask(phydev, AT803X_DEBUG_REG_HIB_CTRL,
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 3e9909b30938..4d1a37487923 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -3216,6 +3216,8 @@ static int phy_probe(struct device *dev)
 			goto out;
 	}
 
+        phy_disable_interrupts(phydev);
+
 	/* Start out supporting everything. Eventually,
 	 * a controller will attach, and may modify one
 	 * or both of these values
@@ -3333,16 +3335,6 @@ static int phy_remove(struct device *dev)
 	return 0;
 }
 
-static void phy_shutdown(struct device *dev)
-{
-	struct phy_device *phydev = to_phy_device(dev);
-
-	if (phydev->state == PHY_READY || !phydev->attached_dev)
-		return;
-
-	phy_disable_interrupts(phydev);
-}
-
 /**
  * phy_driver_register - register a phy_driver with the PHY layer
  * @new_driver: new phy_driver to register
@@ -3376,7 +3368,6 @@ int phy_driver_register(struct phy_driver *new_driver, struct module *owner)
 	new_driver->mdiodrv.driver.bus = &mdio_bus_type;
 	new_driver->mdiodrv.driver.probe = phy_probe;
 	new_driver->mdiodrv.driver.remove = phy_remove;
-	new_driver->mdiodrv.driver.shutdown = phy_shutdown;
 	new_driver->mdiodrv.driver.owner = owner;
 	new_driver->mdiodrv.driver.probe_type = PROBE_FORCE_SYNCHRONOUS;
 
diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
index 4f1c8bb199e9..6568a2759101 100644
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c
@@ -1830,6 +1830,8 @@ static int phylink_bringup_phy(struct phylink *pl, struct phy_device *phy,
 static int phylink_attach_phy(struct phylink *pl, struct phy_device *phy,
 			      phy_interface_t interface)
 {
+	u32 flags = 0;
+
 	if (WARN_ON(pl->cfg_link_an_mode == MLO_AN_FIXED ||
 		    (pl->cfg_link_an_mode == MLO_AN_INBAND &&
 		     phy_interface_mode_is_8023z(interface) && !pl->sfp_bus)))
@@ -1838,7 +1840,10 @@ static int phylink_attach_phy(struct phylink *pl, struct phy_device *phy,
 	if (pl->phydev)
 		return -EBUSY;
 
-	return phy_attach_direct(pl->netdev, phy, 0, interface);
+	if (pl->config.mac_requires_rxc)
+		flags |= PHY_F_RXC_ALWAYS_ON;
+
+	return phy_attach_direct(pl->netdev, phy, flags, interface);
 }
 
 /**
@@ -1941,6 +1946,9 @@ int phylink_fwnode_phy_connect(struct phylink *pl,
 		pl->link_config.interface = pl->link_interface;
 	}
 
+	if (pl->config.mac_requires_rxc)
+		flags |= PHY_F_RXC_ALWAYS_ON;
+
 	ret = phy_attach_direct(pl->netdev, phy_dev, flags,
 				pl->link_interface);
 	phy_device_free(phy_dev);
diff --git a/include/linux/phy.h b/include/linux/phy.h
index ba08b0e60279..79df5e01707d 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -761,6 +761,7 @@ struct phy_device {
 
 /* Generic phy_device::dev_flags */
 #define PHY_F_NO_IRQ		0x80000000
+#define PHY_F_RXC_ALWAYS_ON	BIT(30)
 
 static inline struct phy_device *to_phy_device(const struct device *dev)
 {
diff --git a/include/linux/phylink.h b/include/linux/phylink.h
index 789c516c6b4a..a83c1a77338f 100644
--- a/include/linux/phylink.h
+++ b/include/linux/phylink.h
@@ -204,6 +204,7 @@ enum phylink_op_type {
  * @poll_fixed_state: if true, starts link_poll,
  *		      if MAC link is at %MLO_AN_FIXED mode.
  * @mac_managed_pm: if true, indicate the MAC driver is responsible for PHY PM.
+ * @mac_requires_rxc: if true, the MAC always requires a receive clock from PHY.
  * @ovr_an_inband: if true, override PCS to MLO_AN_INBAND
  * @get_fixed_state: callback to execute to determine the fixed link state,
  *		     if MAC link is at %MLO_AN_FIXED mode.
@@ -216,6 +217,7 @@ struct phylink_config {
 	enum phylink_op_type type;
 	bool poll_fixed_state;
 	bool mac_managed_pm;
+	bool mac_requires_rxc;
 	bool ovr_an_inband;
 	void (*get_fixed_state)(struct phylink_config *config,
 				struct phylink_link_state *state);

-- 
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: phy: at803x: Improve hibernation support on start up

[Romain Gantois]

Hello Russell,

On Thu, 10 Aug 2023, Russell King (Oracle) wrote:

> > We've had these issues before with stmmac, so this "stmmac needs the
> > PHY receive clock" is nothing new - it's had problems with system
> > suspend/resume in the past, and I've made suggestions... and when
> > there's been two people trying to work on it, I've attempted to get
> > them to talk to each other which resulted in nothing further
> > happening.
> > 
> > Another solution could possibly be that we reserve bit 30 on the
> > PHY dev_flags to indicate that the receive clock must always be
> > provided. I suspect that would have an advantage in another
> ...
> 
> Something like this for starters:
> 
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> ...

I've implemented and tested the general-case solution you proposed to this 
receive clock issue with stmmac drivers. The core of your suggestion is pretty 
much unchanged, I just added a phylink_pcs flag for standalone PCS drivers that 
also need to provide the receive clock.

I'd like to send a series for this upstream, which would allow solving this 
issue for both the DWMAC RZN1 case and the AT803x PHY suspend/hibernate case 
(and also potentially other cases with a similar bug).

I wanted to ask you how you would prefer to be credited in my patch series. I 
was considering putting you as author and first signer of the initial patch 
adding the phy_dev flag. Would that be okay or would you prefer something else?

Best Regards,

-- 
Romain Gantois, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH] net: phy: at803x: Improve hibernation support on start up

[Marek Vasut]

On 12/14/23 09:13, Romain Gantois wrote:
> 
> Hello Russell,
> 
> On Thu, 10 Aug 2023, Russell King (Oracle) wrote:
> 
>>> We've had these issues before with stmmac, so this "stmmac needs the
>>> PHY receive clock" is nothing new - it's had problems with system
>>> suspend/resume in the past, and I've made suggestions... and when
>>> there's been two people trying to work on it, I've attempted to get
>>> them to talk to each other which resulted in nothing further
>>> happening.
>>>
>>> Another solution could possibly be that we reserve bit 30 on the
>>> PHY dev_flags to indicate that the receive clock must always be
>>> provided. I suspect that would have an advantage in another
>> ...
>>
>> Something like this for starters:
>>
>> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
>> ...
> 
> I've implemented and tested the general-case solution you proposed to this
> receive clock issue with stmmac drivers. The core of your suggestion is pretty
> much unchanged, I just added a phylink_pcs flag for standalone PCS drivers that
> also need to provide the receive clock.
> 
> I'd like to send a series for this upstream, which would allow solving this
> issue for both the DWMAC RZN1 case and the AT803x PHY suspend/hibernate case
> (and also potentially other cases with a similar bug).
> 
> I wanted to ask you how you would prefer to be credited in my patch series. I
> was considering putting you as author and first signer of the initial patch
> adding the phy_dev flag. Would that be okay or would you prefer something else?

Credit it whichever way you see fit, don't worry, better focus on the 
fix. I can test the result on MX8MM/MX8MP, so feel free to CC me on that.

Re: [PATCH] net: phy: at803x: Improve hibernation support on start up

[Russell King (Oracle)]

On Thu, Dec 14, 2023 at 09:13:58AM +0100, Romain Gantois wrote:
> Hello Russell,
> 
> I've implemented and tested the general-case solution you proposed to this 
> receive clock issue with stmmac drivers. The core of your suggestion is pretty 
> much unchanged, I just added a phylink_pcs flag for standalone PCS drivers that 
> also need to provide the receive clock.

So this affects the ability of PCS to operate correctly as well as MACs?
Would you enlighten which PCS are affected, and what PCS <--> PHY link
modes this is required for?

> I'd like to send a series for this upstream, which would allow solving this 
> issue for both the DWMAC RZN1 case and the AT803x PHY suspend/hibernate case 
> (and also potentially other cases with a similar bug).
> 
> I wanted to ask you how you would prefer to be credited in my patch series. I 
> was considering putting you as author and first signer of the initial patch 
> adding the phy_dev flag. Would that be okay or would you prefer something else?

It depends how big the changes are from my patches. If more than 50% of
the patch remains my work, please retain my authorship. If you wish to
also indicate your authorship, then there is a mechanism to do that -
Co-developed-by: from submitting-patches.rst:

Co-developed-by: states that the patch was co-created by multiple
developers; it is used to give attribution to co-authors (in addition
to the author attributed by the From: tag) when several people work on
a single patch. Since Co-developed-by: denotes authorship, every
Co-developed-by: must be immediately followed by a Signed-off-by: of
the associated co-author.

See submitting-patches.rst for examples of the ordering of the
attributations.

Thanks for asking.

-- 
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: phy: at803x: Improve hibernation support on start up

[Romain Gantois]

On Thu, 14 Dec 2023, Russell King (Oracle) wrote:

> On Thu, Dec 14, 2023 at 09:13:58AM +0100, Romain Gantois wrote:
> > Hello Russell,
> > 
> > I've implemented and tested the general-case solution you proposed to this 
> > receive clock issue with stmmac drivers. The core of your suggestion is pretty 
> > much unchanged, I just added a phylink_pcs flag for standalone PCS drivers that 
> > also need to provide the receive clock.
> 
> So this affects the ability of PCS to operate correctly as well as MACs?
> Would you enlighten which PCS are affected, and what PCS <--> PHY link
> modes this is required for?

The affected hardware is the RZN1 GMAC1 that is found in the r9a06g032 SoC from 
Renesas. This MAC controller is internally connected to a custom PCS that
functions as a RGMII converter. This converter is handled by a standalone PCS 
driver that is already upstream, unlike the GMAC1 driver. So in hardware, the 
MAC/PHY links are organised this way:

    RZN1 GMAC1 <--[GMII]--> MII_CONV1 (internal) <--[RGMII]--> external PHY

The issue is that the RX clock from the external PHY has to be transmitted by 
the MII converter before it reaches GMAC1. Therefore, if the RZN1 PCS driver 
isn't configured to let the clock through before the stmmac GMAC1 driver 
initializes its hardware, then said initialization will fail with a vague DMA 
error.

To solve this, I added a flag to phylink_pcs and made the phylink core set it in 
phylink_validate_mac_and_pcs(). This gives the PCS driver a chance to check the 
flag in pcs_validate() and allow the clock through before the GMAC1 net device 
is brought up.

Regards,

-- 
Romain Gantois, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com