Re: [PATCH] clocksource/drivers/renesas-ostm: Avoid reprobe after successful early probe

[Geert Uytterhoeven]

Hi Saravana,

On Wed, Mar 20, 2024 at 9:18 PM Saravana Kannan <saravanak@xxxxxxxxxx> wrote:
> On Wed, Mar 20, 2024 at 3:30 AM Geert Uytterhoeven
> <geert+renesas@xxxxxxxxx> wrote:
> > The Renesas OS Timer (OSTM) driver contains two probe points, of which
> > only one should complete:
> >   1. Early probe, using TIMER_OF_DECLARE(), to provide the sole
> >      clocksource on (arm32) RZ/A1 and RZ/A2 SoCs,
> >   2. Normal probe, using a platform driver, to provide additional timers
> >      on (arm64 + riscv) RZ/G2L and similar SoCs.
> >
> > The latter is needed because using OSTM on RZ/G2L requires manipulation
> > of its reset signal, which is not yet available at the time of early
> > probe, causing early probe to fail with -EPROBE_DEFER.  It is only
> > enabled when building a kernel with support for the RZ/G2L family, so it
> > does not impact RZ/A1 and RZ/A2.  Hence only one probe method can
> > complete on all affected systems.
> >
> > As relying on the order of initialization of subsystems inside the
> > kernel is fragile, set the DT node's OF_POPULATED flag after a succesful
> > early probe.  This makes sure the platform driver's probe is never
> > called after a successful early probe.
> >
> > Signed-off-by: Geert Uytterhoeven <geert+renesas@xxxxxxxxx>
> > ---
> > Tested on RZ/A2 (after force-enabling the platform driver probe).
> > Regression-tested on RZ/Five (member of the RZ/G2L family).
> >
> > In between commit 4f41fe386a94639c ("clocksource/drivers/timer-probe:
> > Avoid creating dead devices") and its revert 4479730e9263befb (both in
> > v5.7), the clocksource core took care of this.  Other subsystems[1]
> > still handle this, either minimally (by just setting OF_POPULATED), or
> > fully (by also clearing OF_POPULATED again in case of probe failure).
> >
> > Note that despite the revert in the clocksource core, several
> > clocksource drivers[2] still clear the OF_POPULATED flag manually, to
> > force probing the same device using both TIMER_OF_DECLARE() and standard
> > platform device probing (the latter may be done in a different driver).
> >
> > [1] See of_clk_init(), of_gpiochip_scan_gpios(), of_irq_init().
> > [2] drivers/clocksource/ingenic-sysost.c
> >     drivers/clocksource/ingenic-timer.c
> >     drivers/clocksource/timer-versatile.c
> > ---
> >  drivers/clocksource/renesas-ostm.c | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/drivers/clocksource/renesas-ostm.c b/drivers/clocksource/renesas-ostm.c
> > index 8da972dc171365bc..37db7e23a4d29135 100644
> > --- a/drivers/clocksource/renesas-ostm.c
> > +++ b/drivers/clocksource/renesas-ostm.c
> > @@ -210,6 +210,7 @@ static int __init ostm_init(struct device_node *np)
> >                 pr_info("%pOF: used for clock events\n", np);
> >         }
> >
> > +       of_node_set_flag(np, OF_POPULATED);
> >         return 0;
>
> Couldn't you also solve this by using the more specific compatible
> strings for the driver and TIMER_OF_DECLARE()?

That's another option, but would considerably grow the number of
compatible values to match against.

Note that the actual OSTM module is (assumed to be) identical. The
differences lie in the integration into the SoC (wiring of the
module's reset). Hence using the compatible value to differentiate
looks wrong to me.  It would be nice if this could be handled
automatically, which is what the reverted commit 4f41fe386a94639c
("clocksource/drivers/timer-probe: Avoid creating dead devices") did...

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68korg

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