diff mbox series

[net-next,3/6] net: stmmac: start phylink before setting up hardware

Message ID 20230116103926.276869-4-clement.leger@bootlin.com (mailing list archive)
State Changes Requested
Delegated to: Netdev Maintainers
Headers show
Series net: stmmac: add renesas,rzn1-gmac support | expand

Checks

Context Check Description
netdev/tree_selection success Clearly marked for net-next, async
netdev/fixes_present success Fixes tag not required for -next series
netdev/subject_prefix success Link
netdev/cover_letter success Series has a cover letter
netdev/patch_count success Link
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 0 this patch: 0
netdev/cc_maintainers success CCed 12 of 12 maintainers
netdev/build_clang success Errors and warnings before: 0 this patch: 0
netdev/module_param success Was 0 now: 0
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 0 this patch: 0
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 19 lines checked
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Clément Léger Jan. 16, 2023, 10:39 a.m. UTC
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(-)

Comments

Russell King (Oracle) Jan. 16, 2023, 10:53 a.m. UTC | #1
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.
Clément Léger Feb. 7, 2023, 2:41 p.m. UTC | #2
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,
Russell King (Oracle) Feb. 10, 2023, 11:03 a.m. UTC | #3
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.
Clément Léger Feb. 10, 2023, 11:20 a.m. UTC | #4
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
diff mbox series

Patch

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);