| Message ID | 20241119-upstream_s32cc_gmac-v5-16-7dcc90fcffef@oss.nxp.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Add support for Synopsis DWMAC IP on NXP Automotive SoCs S32G2xx/S32G3xx/S32R45 | expand |
On Tue, Nov 19, 2024 at 04:00:22PM +0100, Jan Petrous via B4 Relay wrote: > From: "Jan Petrous (OSS)" <jan.petrous@oss.nxp.com> > > The stmmac driver supports many vendors SoCs using Synopsys-licensed > Ethernet controller IP. Most of these vendors reuse the stmmac_platform > codebase, which has a potential PTP clock initialization issue. > The PTP clock rate reading might require ungating what is not provided. > > Fix the PTP clock initialization by enabling it immediately. > > Signed-off-by: Jan Petrous (OSS) <jan.petrous@oss.nxp.com> > --- > drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c > index b1e4df1a86a0..db3e8ef4fc3a 100644 > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c > @@ -632,7 +632,7 @@ stmmac_probe_config_dt(struct platform_device *pdev, u8 *mac) > clk_prepare_enable(plat->pclk); > > /* Fall-back to main clock in case of no PTP ref is passed */ > - plat->clk_ptp_ref = devm_clk_get(&pdev->dev, "ptp_ref"); > + plat->clk_ptp_ref = devm_clk_get_enabled(&pdev->dev, "ptp_ref"); > if (IS_ERR(plat->clk_ptp_ref)) { > plat->clk_ptp_rate = clk_get_rate(plat->stmmac_clk); > plat->clk_ptp_ref = NULL; Looking at where the driver makes use of clk_ptp_ref, it currently prepares and enables this clock via stmmac_open(), disables and unprepares via stmmac_release(). There could be a platform where this is being used as a power saving measure, and replacing devm_clk_get() with devm_clk_get_enabled() will defeat that. I would suggest that if you need the clock to be enabled in order to get its rate, then the call to clk_get_rate() should have the enable/disable around it to allow these other sites to work as they have done. Alternatively, we may take the view that the power saving is not necessary, or stopping the clock is not a good idea (loss of time in the 1588 block?) so the above changed would be sensible but only if the clk_prepare_enable() and clk_disable_unprepare() calls on this particular clock are also removed. I can't say which is the correct way forward.
On Tue, Nov 19, 2024 at 05:09:15PM +0000, Russell King (Oracle) wrote: > On Tue, Nov 19, 2024 at 04:00:22PM +0100, Jan Petrous via B4 Relay wrote: > > From: "Jan Petrous (OSS)" <jan.petrous@oss.nxp.com> > > > > The stmmac driver supports many vendors SoCs using Synopsys-licensed > > Ethernet controller IP. Most of these vendors reuse the stmmac_platform > > codebase, which has a potential PTP clock initialization issue. > > The PTP clock rate reading might require ungating what is not provided. > > > > Fix the PTP clock initialization by enabling it immediately. > > > > Signed-off-by: Jan Petrous (OSS) <jan.petrous@oss.nxp.com> > > --- > > drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c > > index b1e4df1a86a0..db3e8ef4fc3a 100644 > > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c > > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c > > @@ -632,7 +632,7 @@ stmmac_probe_config_dt(struct platform_device *pdev, u8 *mac) > > clk_prepare_enable(plat->pclk); > > > > /* Fall-back to main clock in case of no PTP ref is passed */ > > - plat->clk_ptp_ref = devm_clk_get(&pdev->dev, "ptp_ref"); > > + plat->clk_ptp_ref = devm_clk_get_enabled(&pdev->dev, "ptp_ref"); > > if (IS_ERR(plat->clk_ptp_ref)) { > > plat->clk_ptp_rate = clk_get_rate(plat->stmmac_clk); > > plat->clk_ptp_ref = NULL; > > Looking at where the driver makes use of clk_ptp_ref, it currently > prepares and enables this clock via stmmac_open(), disables and > unprepares via stmmac_release(). > > There could be a platform where this is being used as a power saving > measure, and replacing devm_clk_get() with devm_clk_get_enabled() will > defeat that. > > I would suggest that if you need the clock to be enabled in order to > get its rate, then the call to clk_get_rate() should have the > enable/disable around it to allow these other sites to work as they > have done. > > Alternatively, we may take the view that the power saving is not > necessary, or stopping the clock is not a good idea (loss of time > in the 1588 block?) so the above changed would be sensible but only > if the clk_prepare_enable() and clk_disable_unprepare() calls on > this particular clock are also removed. > > I can't say which is the correct way forward. > For me it looks more conservative way to use first option = enclose the clk_get_rate() with clk_prepare_enable() and clk_disable_unprepare() as this don't change the PTP clock status for other glue drivers. BR. /Jan
On 11/19/24 16:00, Jan Petrous (OSS) wrote: > From: "Jan Petrous (OSS)" <jan.petrous@oss.nxp.com> > > The stmmac driver supports many vendors SoCs using Synopsys-licensed > Ethernet controller IP. Most of these vendors reuse the stmmac_platform > codebase, which has a potential PTP clock initialization issue. > The PTP clock rate reading might require ungating what is not provided. > > Fix the PTP clock initialization by enabling it immediately. > > Signed-off-by: Jan Petrous (OSS) <jan.petrous@oss.nxp.com> Side, process-related note: it would be great if you could trim the patch series below 16 (currently off-by-one): https://elixir.bootlin.com/linux/v6.11.8/source/Documentation/process/maintainer-netdev.rst#L14 This patch looks like an independent fix, possibly worth the 'net' tree. If so, please submit this patch separately, including a suitable fixes tag and including the 'net' keyword in the patch subj prefix. Thanks, Paolo
On Thu, Nov 21, 2024 at 08:45:20AM +0100, Paolo Abeni wrote: > On 11/19/24 16:00, Jan Petrous (OSS) wrote: > > From: "Jan Petrous (OSS)" <jan.petrous@oss.nxp.com> > > > > The stmmac driver supports many vendors SoCs using Synopsys-licensed > > Ethernet controller IP. Most of these vendors reuse the stmmac_platform > > codebase, which has a potential PTP clock initialization issue. > > The PTP clock rate reading might require ungating what is not provided. > > > > Fix the PTP clock initialization by enabling it immediately. > > > > Signed-off-by: Jan Petrous (OSS) <jan.petrous@oss.nxp.com> > > Side, process-related note: it would be great if you could trim the > patch series below 16 (currently off-by-one): > > https://elixir.bootlin.com/linux/v6.11.8/source/Documentation/process/maintainer-netdev.rst#L14 > > This patch looks like an independent fix, possibly worth the 'net' tree. > If so, please submit this patch separately, including a suitable fixes > tag and including the 'net' keyword in the patch subj prefix. > Yeh, I also was a bit worried about patchwork fail check on patch series size. Thanks for your hint, I will remove this PTP clock rate patch from the series and resend it to the 'net' tree with fixes tag. BR. /Jan
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c index b1e4df1a86a0..db3e8ef4fc3a 100644 --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c @@ -632,7 +632,7 @@ stmmac_probe_config_dt(struct platform_device *pdev, u8 *mac) clk_prepare_enable(plat->pclk); /* Fall-back to main clock in case of no PTP ref is passed */ - plat->clk_ptp_ref = devm_clk_get(&pdev->dev, "ptp_ref"); + plat->clk_ptp_ref = devm_clk_get_enabled(&pdev->dev, "ptp_ref"); if (IS_ERR(plat->clk_ptp_ref)) { plat->clk_ptp_rate = clk_get_rate(plat->stmmac_clk); plat->clk_ptp_ref = NULL;