| Message ID | bd027379713cbaafa21ffe9e848ebb7f475ca0e7.1710930542.git.geert+renesas@glider.be (mailing list archive) |
|---|---|
| State | Under Review |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
| Series | clocksource/drivers/renesas-ostm: Avoid reprobe after successful early probe | expand |
On Wed, Mar 20, 2024 at 10:40 AM Geert Uytterhoeven <geert+renesas@glider.be> 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@glider.be> > --- > 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(+) > Reviwed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> Cheers, Prabhakar > 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; > > err_cleanup: > -- > 2.34.1 > >
On Wed, Mar 20, 2024 at 3:30 AM Geert Uytterhoeven <geert+renesas@glider.be> 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@glider.be> > --- > 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()? -Saravana > > err_cleanup: > -- > 2.34.1 >
Hi Saravana, On Wed, Mar 20, 2024 at 9:18 PM Saravana Kannan <saravanak@google.com> wrote: > On Wed, Mar 20, 2024 at 3:30 AM Geert Uytterhoeven > <geert+renesas@glider.be> 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@glider.be> > > --- > > 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
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; err_cleanup:
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@glider.be> --- 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(+)