Hi Niklas,
On Wed, May 8, 2024 at 5:11 PM Niklas Söderlund
<niklas.soderlund+renesas@xxxxxxxxxxxx> wrote:
> On 2024-05-08 16:00:21 +0200, Andrew Lunn wrote:
> > > I agree it's odd and I will try to find out.
> > >
> > > If I remove all pm_ calls and the include of pm_runtime.h register reads
> > > from the device do no longer works, so operating the device fails. Even
> > > if I dig out the root cause for this, is there any harm in keeping the
> > > pm_ operations in the initial entablement?
> >
> > It suggests something is broken. Do we want to merge broken code?
>
> Of course I do not want broken code merged. I was curious if you knew of
> any harmful side effect of of using pm_ functions I was unaware of.
>
> > Once we understand the root cause maybe then we can decide it is O.K.
>
> The root cause is that the module clock is not enabled without some
> action. If I remove all pm_ calls as well as the inclusion of
> linux/pm_runtime.h. The tsn module clock is left disabled after probe
> completes.
> As the clock is disabled trying to operate the device is not possible.
Which is perfectly normal...
> The clock can either be enabled by the pm_ calls as show or be replaced
> by an explicit clk_enable(), like this (the other pm_ related
> calls/includes are of course also removed).
>
> - pm_runtime_enable(&pdev->dev);
> - pm_runtime_get_sync(&pdev->dev);
> + clk_enable(priv->clk);
>
> Either of the two methods leaves the module clock running and the driver
> can operate the device.
> I would prefer to keep the pm_ operations, but if you prefer I can
> switch to using clk_enable().
While explicit clock management works, please use pm_runtime_*()
instead, as the device is part of a Clock Domain. Also, the TSN
block may be reused on a different SoC, where it is part of a real
power domain, and manual clock management won't be sufficient.
BTW, if you need to debug more fine-grained Runtime PM handling, you
can add a volatile variable that tracks the module clock state
(update it in cpg_mstp_clock_endisable()), and WARN() in rtsn_read()
and friends if the clock is disabled.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
