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@xxxxxxxxxxx>
> >
> > 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@xxxxxxxxxxx>
> > ---
> > 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
