[v5,16/16] net: stmmac: platform: Fix PTP clock rate reading

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

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