| 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 |
| 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 |
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.
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,
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.
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 --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);
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(-)