The stmmac on the Renesas RZ/N1 platform is connected to the PCS which
must be configured to provide a correct RGMII RX clock to the stmmac IP.
Without the RX clock, the driver will fail to initialize the hardware
(more specifically, the driver will report it fails to reset DMA). In
order to fix that, start phylink mecanism before setting up hardware.

Signed-off-by: Clément Léger <clement.leger@bootlin.com>
---
 drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index f2247b8cf0a3..88c941003855 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -3818,6 +3818,12 @@ static int __stmmac_open(struct net_device *dev,
 		}
 	}
 
+	/* We need to setup the phy & PCS before accessing the stmmac registers
+	 * because in some cases (RZ/N1), if the stmmac IP is not clocked by the
+	 * PCS, hardware init will fail because it lacks a RGMII RX clock.
+	 */
+	phylink_start(priv->phylink);
+
 	ret = stmmac_hw_setup(dev, true);
 	if (ret < 0) {
 		netdev_err(priv->dev, "%s: Hw setup failed\n", __func__);
@@ -3826,7 +3832,6 @@ static int __stmmac_open(struct net_device *dev,
 
 	stmmac_init_coalesce(priv);
 
-	phylink_start(priv->phylink);
 	/* We may have called phylink_speed_down before */
 	phylink_speed_up(priv->phylink);
 
-- 
2.39.0

On Mon, Jan 16, 2023 at 11:39:23AM +0100, Clément Léger wrote:
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index f2247b8cf0a3..88c941003855 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> @@ -3818,6 +3818,12 @@ static int __stmmac_open(struct net_device *dev,
>  		}
>  	}
>  
> +	/* We need to setup the phy & PCS before accessing the stmmac registers
> +	 * because in some cases (RZ/N1), if the stmmac IP is not clocked by the
> +	 * PCS, hardware init will fail because it lacks a RGMII RX clock.
> +	 */
> +	phylink_start(priv->phylink);

So what happens if you end up with the mac_link_up method being called
at this point in the driver, before the hardware has been setup ?

If you use a fixed-link, that's a real possibility.

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

Le Mon, 16 Jan 2023 10:53:49 +0000,
"Russell King (Oracle)" <linux@armlinux.org.uk> a écrit :

> On Mon, Jan 16, 2023 at 11:39:23AM +0100, Clément Léger wrote:
> > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > index f2247b8cf0a3..88c941003855 100644
> > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > @@ -3818,6 +3818,12 @@ static int __stmmac_open(struct net_device *dev,
> >  		}
> >  	}
> >  
> > +	/* We need to setup the phy & PCS before accessing the stmmac registers
> > +	 * because in some cases (RZ/N1), if the stmmac IP is not clocked by the
> > +	 * PCS, hardware init will fail because it lacks a RGMII RX clock.
> > +	 */
> > +	phylink_start(priv->phylink);  
> 
> So what happens if you end up with the mac_link_up method being called
> at this point in the driver, before the hardware has been setup ?
> 
> If you use a fixed-link, that's a real possibility.

I actually have this setup. On the board, one GMAC is connected to a
DSA switch using a fixed-link and the other using the PCS such as added
by this series.

From what I see, indeed, the mac_link_up() function is called before
stmmac_hw_setup(). This does not seems to have any effect on my setup
(except making it working of course) but I agree this is clearly not
ideal.

What I could do is adding a function in the miic pcs driver that could
be called from my rzn1 stmmac probe function to actually configure the
PCS at probe time based on the detected "phy-mode". Does that seems
better to you ?

Thanks,

-- 
Clément Léger,
Embedded Linux and Kernel engineer at Bootlin
https://bootlin.com

On Tue, Feb 07, 2023 at 03:41:35PM +0100, Clément Léger wrote:
> Le Mon, 16 Jan 2023 10:53:49 +0000,
> "Russell King (Oracle)" <linux@armlinux.org.uk> a écrit :
> 
> > On Mon, Jan 16, 2023 at 11:39:23AM +0100, Clément Léger wrote:
> > > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > > index f2247b8cf0a3..88c941003855 100644
> > > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > > @@ -3818,6 +3818,12 @@ static int __stmmac_open(struct net_device *dev,
> > >  		}
> > >  	}
> > >  
> > > +	/* We need to setup the phy & PCS before accessing the stmmac registers
> > > +	 * because in some cases (RZ/N1), if the stmmac IP is not clocked by the
> > > +	 * PCS, hardware init will fail because it lacks a RGMII RX clock.
> > > +	 */
> > > +	phylink_start(priv->phylink);  
> > 
> > So what happens if you end up with the mac_link_up method being called
> > at this point in the driver, before the hardware has been setup ?
> > 
> > If you use a fixed-link, that's a real possibility.
> 
> I actually have this setup. On the board, one GMAC is connected to a
> DSA switch using a fixed-link and the other using the PCS such as added
> by this series.
> 
> From what I see, indeed, the mac_link_up() function is called before
> stmmac_hw_setup(). This does not seems to have any effect on my setup
> (except making it working of course) but I agree this is clearly not
> ideal.
> 
> What I could do is adding a function in the miic pcs driver that could
> be called from my rzn1 stmmac probe function to actually configure the
> PCS at probe time based on the detected "phy-mode". Does that seems
> better to you ?

I think Clark Wang is also working on addressing a very similar problem
with stmmac. Please can you check out his work first, he's adding a new
function to phylink to bring the PHY up early in the resume path.

I would like you both to work together to address what seems to be the
same issue.

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

Le Fri, 10 Feb 2023 11:03:34 +0000,
"Russell King (Oracle)" <linux@armlinux.org.uk> a écrit :

> On Tue, Feb 07, 2023 at 03:41:35PM +0100, Clément Léger wrote:
> > Le Mon, 16 Jan 2023 10:53:49 +0000,
> > "Russell King (Oracle)" <linux@armlinux.org.uk> a écrit :
> >   
> > > On Mon, Jan 16, 2023 at 11:39:23AM +0100, Clément Léger wrote:  
> > > > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > > > index f2247b8cf0a3..88c941003855 100644
> > > > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > > > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > > > @@ -3818,6 +3818,12 @@ static int __stmmac_open(struct net_device *dev,
> > > >  		}
> > > >  	}
> > > >  
> > > > +	/* We need to setup the phy & PCS before accessing the stmmac registers
> > > > +	 * because in some cases (RZ/N1), if the stmmac IP is not clocked by the
> > > > +	 * PCS, hardware init will fail because it lacks a RGMII RX clock.
> > > > +	 */
> > > > +	phylink_start(priv->phylink);    
> > > 
> > > So what happens if you end up with the mac_link_up method being called
> > > at this point in the driver, before the hardware has been setup ?
> > > 
> > > If you use a fixed-link, that's a real possibility.  
> > 
> > I actually have this setup. On the board, one GMAC is connected to a
> > DSA switch using a fixed-link and the other using the PCS such as added
> > by this series.
> > 
> > From what I see, indeed, the mac_link_up() function is called before
> > stmmac_hw_setup(). This does not seems to have any effect on my setup
> > (except making it working of course) but I agree this is clearly not
> > ideal.
> > 
> > What I could do is adding a function in the miic pcs driver that could
> > be called from my rzn1 stmmac probe function to actually configure the
> > PCS at probe time based on the detected "phy-mode". Does that seems
> > better to you ?  
> 
> I think Clark Wang is also working on addressing a very similar problem
> with stmmac. Please can you check out his work first, he's adding a new
> function to phylink to bring the PHY up early in the resume path.
> 
> I would like you both to work together to address what seems to be the
> same issue.
> 

Acked

-- 
Clément Léger,
Embedded Linux and Kernel engineer at Bootlin
https://bootlin.com