| Message ID | 20241119-upstream_s32cc_gmac-v5-16-7dcc90fcffef@oss.nxp.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v5,01/16] net: driver: stmmac: Fix CSR divider comment | expand |
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
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;