| Message ID | 1531924476-23261-1-git-send-email-phil.edworthy@renesas.com (mailing list archive) |
|---|---|
| State | Rejected |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
Hi Phil, On Wed, Jul 18, 2018 at 4:34 PM Phil Edworthy <phil.edworthy@renesas.com> wrote: > To avoid all SoC peripheral drivers deferring their probes, both clock and > pinctrl drivers should already be probed. Since the pinctrl driver requires > a clock to access the registers, the clock driver should be probed before > the pinctrl driver. > > Therefore, move the clock driver from subsys_initcall to core_initcall. > > Signed-off-by: Phil Edworthy <phil.edworthy@renesas.com> Thanks for your patch! The (not yet upstreamed) pinctrl driver uses postcore_initcall(), right? > --- a/drivers/clk/renesas/r9a06g032-clocks.c > +++ b/drivers/clk/renesas/r9a06g032-clocks.c > @@ -877,17 +877,18 @@ static const struct of_device_id r9a06g032_match[] = { > { } > }; > > -static struct platform_driver r9a06g032_clock_driver = { > +static struct platform_driver r9a06g032_clock_driver __refdata = { > .driver = { > .name = "renesas,r9a06g032-sysctrl", > .of_match_table = r9a06g032_match, > }, > + .probe = r9a06g032_clocks_probe, > }; > > static int __init r9a06g032_clocks_init(void) > { > - return platform_driver_probe(&r9a06g032_clock_driver, > - r9a06g032_clocks_probe); > + platform_driver_register(&r9a06g032_clock_driver); > + return 0; > } Why are all of the above changes needed? Shouldn't the platform_driver_probe() keep on working? If it does not, it means the clock driver has some other dependency, and cannot be bound immediately. This is potentially a dangerous situation, as r9a06g032_clocks_probe() is __init, but can still be called at any time later. Hence using platform_driver_probe() is the safe thing to do, possibly with a different reshuffling of the clock and pinctrl initcall priorities. > -subsys_initcall(r9a06g032_clocks_init); > +core_initcall(r9a06g032_clocks_init); Gr{oetje,eeting}s, Geert
Hi Geert, On 20 July 2018 12:21, Geert Uytterhoeven wrote: > On Wed, Jul 18, 2018 at 4:34 PM Phil Edworthy wrote: > > To avoid all SoC peripheral drivers deferring their probes, both clock > > and pinctrl drivers should already be probed. Since the pinctrl driver > > requires a clock to access the registers, the clock driver should be > > probed before the pinctrl driver. > > > > Therefore, move the clock driver from subsys_initcall to core_initcall. > > > > Signed-off-by: Phil Edworthy <phil.edworthy@renesas.com> > > Thanks for your patch! Thanks for your review! > The (not yet upstreamed) pinctrl driver uses postcore_initcall(), right? No, the pinctrl driver uses subsys_initcall, but postcore_initcall or arch_initcall may be better to make it clear about the dependencies. > > --- a/drivers/clk/renesas/r9a06g032-clocks.c > > +++ b/drivers/clk/renesas/r9a06g032-clocks.c > > @@ -877,17 +877,18 @@ static const struct of_device_id > r9a06g032_match[] = { > > { } > > }; > > > > -static struct platform_driver r9a06g032_clock_driver = { > > +static struct platform_driver r9a06g032_clock_driver __refdata = { > > .driver = { > > .name = "renesas,r9a06g032-sysctrl", > > .of_match_table = r9a06g032_match, > > }, > > + .probe = r9a06g032_clocks_probe, > > }; > > > > static int __init r9a06g032_clocks_init(void) { > > - return platform_driver_probe(&r9a06g032_clock_driver, > > - r9a06g032_clocks_probe); > > + platform_driver_register(&r9a06g032_clock_driver); > > + return 0; That should be: + return platform_driver_register(&r9a06g032_clock_driver); > > } > > Why are all of the above changes needed? > Shouldn't the platform_driver_probe() keep on working? > If it does not, it means the clock driver has some other dependency, and > cannot be bound immediately. This is potentially a dangerous situation, as > r9a06g032_clocks_probe() is __init, but can still be called at any time later. > Hence using platform_driver_probe() is the safe thing to do, possibly with a > different reshuffling of the clock and pinctrl initcall priorities. No, you cannot call platform_driver_probe() from core_initcall. All drivers that are in core_initcall call platform_driver_register(). Thanks Phil > > -subsys_initcall(r9a06g032_clocks_init); > > +core_initcall(r9a06g032_clocks_init); > > Gr{oetje,eeting}s, > > Geert > > -- > Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux- > m68k.org > > 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
Hi Phil, On Fri, Jul 20, 2018 at 2:06 PM Phil Edworthy <phil.edworthy@renesas.com> wrote: > On 20 July 2018 12:21, Geert Uytterhoeven wrote: > > On Wed, Jul 18, 2018 at 4:34 PM Phil Edworthy wrote: > > > To avoid all SoC peripheral drivers deferring their probes, both clock > > > and pinctrl drivers should already be probed. Since the pinctrl driver > > > requires a clock to access the registers, the clock driver should be > > > probed before the pinctrl driver. > > > > > > Therefore, move the clock driver from subsys_initcall to core_initcall. > > > > > > Signed-off-by: Phil Edworthy <phil.edworthy@renesas.com> > > > > Thanks for your patch! > Thanks for your review! > > > The (not yet upstreamed) pinctrl driver uses postcore_initcall(), right? > No, the pinctrl driver uses subsys_initcall, but postcore_initcall or > arch_initcall may be better to make it clear about the dependencies. if the pinctrl driver uses subsys_initcall(), ... > > > --- a/drivers/clk/renesas/r9a06g032-clocks.c > > > +++ b/drivers/clk/renesas/r9a06g032-clocks.c > > > @@ -877,17 +877,18 @@ static const struct of_device_id > > r9a06g032_match[] = { > > > { } > > > }; > > > > > > -static struct platform_driver r9a06g032_clock_driver = { > > > +static struct platform_driver r9a06g032_clock_driver __refdata = { > > > .driver = { > > > .name = "renesas,r9a06g032-sysctrl", > > > .of_match_table = r9a06g032_match, > > > }, > > > + .probe = r9a06g032_clocks_probe, > > > }; > > > > > > static int __init r9a06g032_clocks_init(void) { > > > - return platform_driver_probe(&r9a06g032_clock_driver, > > > - r9a06g032_clocks_probe); > > > + platform_driver_register(&r9a06g032_clock_driver); > > > + return 0; > That should be: > + return platform_driver_register(&r9a06g032_clock_driver); > > > > } > > > > Why are all of the above changes needed? > > Shouldn't the platform_driver_probe() keep on working? > > If it does not, it means the clock driver has some other dependency, and > > cannot be bound immediately. This is potentially a dangerous situation, as > > r9a06g032_clocks_probe() is __init, but can still be called at any time later. > > Hence using platform_driver_probe() is the safe thing to do, possibly with a > > different reshuffling of the clock and pinctrl initcall priorities. > No, you cannot call platform_driver_probe() from core_initcall. > All drivers that are in core_initcall call platform_driver_register(). Hence they cannot have their probe function __init. > > Thanks > Phil > > > > -subsys_initcall(r9a06g032_clocks_init); > > > +core_initcall(r9a06g032_clocks_init); ... using postcore_initcall() or arch_initcall() here, should work with platform_driver_probe()? Gr{oetje,eeting}s, Geert
Hi Geert, On 20 July 2018 13:12, Geert Uytterhoeven wrote: > On Fri, Jul 20, 2018 at 2:06 PM Phil Edworthy wrote: > > On 20 July 2018 12:21, Geert Uytterhoeven wrote: > > > On Wed, Jul 18, 2018 at 4:34 PM Phil Edworthy wrote: > > > > To avoid all SoC peripheral drivers deferring their probes, both > > > > clock and pinctrl drivers should already be probed. Since the > > > > pinctrl driver requires a clock to access the registers, the clock > > > > driver should be probed before the pinctrl driver. > > > > > > > > Therefore, move the clock driver from subsys_initcall to core_initcall. > > > > > > > > Signed-off-by: Phil Edworthy <phil.edworthy@renesas.com> > > > > > > Thanks for your patch! > > Thanks for your review! > > > > > The (not yet upstreamed) pinctrl driver uses postcore_initcall(), right? > > No, the pinctrl driver uses subsys_initcall, but postcore_initcall or > > arch_initcall may be better to make it clear about the dependencies. > > if the pinctrl driver uses subsys_initcall(), ... > > > > > --- a/drivers/clk/renesas/r9a06g032-clocks.c > > > > +++ b/drivers/clk/renesas/r9a06g032-clocks.c > > > > @@ -877,17 +877,18 @@ static const struct of_device_id > > > r9a06g032_match[] = { > > > > { } > > > > }; > > > > > > > > -static struct platform_driver r9a06g032_clock_driver = { > > > > +static struct platform_driver r9a06g032_clock_driver __refdata = > > > > +{ > > > > .driver = { > > > > .name = "renesas,r9a06g032-sysctrl", > > > > .of_match_table = r9a06g032_match, > > > > }, > > > > + .probe = r9a06g032_clocks_probe, > > > > }; > > > > > > > > static int __init r9a06g032_clocks_init(void) { > > > > - return platform_driver_probe(&r9a06g032_clock_driver, > > > > - r9a06g032_clocks_probe); > > > > + platform_driver_register(&r9a06g032_clock_driver); > > > > + return 0; > > That should be: > > + return platform_driver_register(&r9a06g032_clock_driver); > > > > > > } > > > > > > Why are all of the above changes needed? > > > Shouldn't the platform_driver_probe() keep on working? > > > If it does not, it means the clock driver has some other dependency, > > > and cannot be bound immediately. This is potentially a dangerous > > > situation, as > > > r9a06g032_clocks_probe() is __init, but can still be called at any time later. > > > Hence using platform_driver_probe() is the safe thing to do, > > > possibly with a different reshuffling of the clock and pinctrl initcall > priorities. > > No, you cannot call platform_driver_probe() from core_initcall. > > All drivers that are in core_initcall call platform_driver_register(). > > Hence they cannot have their probe function __init. > > > > > Thanks > > Phil > > > > > > -subsys_initcall(r9a06g032_clocks_init); > > > > +core_initcall(r9a06g032_clocks_init); > > ... using postcore_initcall() or arch_initcall() here, should work with > platform_driver_probe()? Nope, you have to use platform_driver_register() for DT based drivers. subsys_initcall is the earliest you can use platform_driver_probe(). Thanks Phil > Gr{oetje,eeting}s, > > Geert > > -- > Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux- > m68k.org > > 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
Hi Phil, On Fri, Jul 20, 2018 at 2:26 PM Phil Edworthy <phil.edworthy@renesas.com> wrote: > On 20 July 2018 13:12, Geert Uytterhoeven wrote: > > On Fri, Jul 20, 2018 at 2:06 PM Phil Edworthy wrote: > > > On 20 July 2018 12:21, Geert Uytterhoeven wrote: > > > > On Wed, Jul 18, 2018 at 4:34 PM Phil Edworthy wrote: > > > > > To avoid all SoC peripheral drivers deferring their probes, both > > > > > clock and pinctrl drivers should already be probed. Since the > > > > > pinctrl driver requires a clock to access the registers, the clock > > > > > driver should be probed before the pinctrl driver. > > > > > > > > > > Therefore, move the clock driver from subsys_initcall to core_initcall. > > > > > > > > > > Signed-off-by: Phil Edworthy <phil.edworthy@renesas.com> > > > > > > > > Thanks for your patch! > > > Thanks for your review! > > > > > > > The (not yet upstreamed) pinctrl driver uses postcore_initcall(), right? > > > No, the pinctrl driver uses subsys_initcall, but postcore_initcall or > > > arch_initcall may be better to make it clear about the dependencies. > > > > if the pinctrl driver uses subsys_initcall(), ... > > > > > -subsys_initcall(r9a06g032_clocks_init); > > > > > +core_initcall(r9a06g032_clocks_init); > > > > ... using postcore_initcall() or arch_initcall() here, should work with > > platform_driver_probe()? > Nope, you have to use platform_driver_register() for DT based drivers. > subsys_initcall is the earliest you can use platform_driver_probe(). So drivers/misc/atmel_tclib.c and drivers/pinctrl/pinctrl-coh901.c, which use arch_initcall(), cannot work? If that is really true, you can still use subsys_initcall() in the clock driver, and subsys_initcall_sync() in the pinctrl driver. Gr{oetje,eeting}s, Geert
Hi Geert, On 20 July 2018 13:41, Geert Uytterhoeven wrote: > On Fri, Jul 20, 2018 at 2:26 PM Phil Edworthy wrote: > > On 20 July 2018 13:12, Geert Uytterhoeven wrote: > > > On Fri, Jul 20, 2018 at 2:06 PM Phil Edworthy wrote: > > > > On 20 July 2018 12:21, Geert Uytterhoeven wrote: > > > > > On Wed, Jul 18, 2018 at 4:34 PM Phil Edworthy wrote: > > > > > > To avoid all SoC peripheral drivers deferring their probes, > > > > > > both clock and pinctrl drivers should already be probed. Since > > > > > > the pinctrl driver requires a clock to access the registers, > > > > > > the clock driver should be probed before the pinctrl driver. > > > > > > > > > > > > Therefore, move the clock driver from subsys_initcall to > core_initcall. > > > > > > > > > > > > Signed-off-by: Phil Edworthy <phil.edworthy@renesas.com> > > > > > > > > > > Thanks for your patch! > > > > Thanks for your review! > > > > > > > > > The (not yet upstreamed) pinctrl driver uses postcore_initcall(), right? > > > > No, the pinctrl driver uses subsys_initcall, but postcore_initcall > > > > or arch_initcall may be better to make it clear about the dependencies. > > > > > > if the pinctrl driver uses subsys_initcall(), ... > > > > > > > -subsys_initcall(r9a06g032_clocks_init); > > > > > > +core_initcall(r9a06g032_clocks_init); > > > > > > ... using postcore_initcall() or arch_initcall() here, should work > > > with platform_driver_probe()? > > Nope, you have to use platform_driver_register() for DT based drivers. > > subsys_initcall is the earliest you can use platform_driver_probe(). > > So drivers/misc/atmel_tclib.c and drivers/pinctrl/pinctrl-coh901.c, which use > arch_initcall(), cannot work? How those drivers work I do not know. However, I tried with the rzn1 clock driver and it does not work. > If that is really true, you can still use subsys_initcall() in the clock driver, and > subsys_initcall_sync() in the pinctrl driver. True, I'm not sure how people decide which initcall to use and whether the _sync variants of the initcalls have a special meaning or intention. As far as I know they were introduced for threaded probes, so are we supposed to use them for driver dependencies like this? Do you know why the rza1 pinctrl driver is running from core_initcall()? Thanks Phil
Hi Phil, CC Jacopo On Fri, Jul 20, 2018 at 3:57 PM Phil Edworthy <phil.edworthy@renesas.com> wrote: > On 20 July 2018 13:41, Geert Uytterhoeven wrote: > > On Fri, Jul 20, 2018 at 2:26 PM Phil Edworthy wrote: > > > On 20 July 2018 13:12, Geert Uytterhoeven wrote: > > > > On Fri, Jul 20, 2018 at 2:06 PM Phil Edworthy wrote: > > > > > On 20 July 2018 12:21, Geert Uytterhoeven wrote: > > > > > > On Wed, Jul 18, 2018 at 4:34 PM Phil Edworthy wrote: > > > > > > > To avoid all SoC peripheral drivers deferring their probes, > > > > > > > both clock and pinctrl drivers should already be probed. Since > > > > > > > the pinctrl driver requires a clock to access the registers, > > > > > > > the clock driver should be probed before the pinctrl driver. > > > > > > > > > > > > > > Therefore, move the clock driver from subsys_initcall to > > core_initcall. > > > > > > > > > > > > > > Signed-off-by: Phil Edworthy <phil.edworthy@renesas.com> > > > > > > > > > > > > Thanks for your patch! > > > > > Thanks for your review! > > > > > > > > > > > The (not yet upstreamed) pinctrl driver uses postcore_initcall(), right? > > > > > No, the pinctrl driver uses subsys_initcall, but postcore_initcall > > > > > or arch_initcall may be better to make it clear about the dependencies. > > > > > > > > if the pinctrl driver uses subsys_initcall(), ... > > > > > > > > > -subsys_initcall(r9a06g032_clocks_init); > > > > > > > +core_initcall(r9a06g032_clocks_init); > > > > > > > > ... using postcore_initcall() or arch_initcall() here, should work > > > > with platform_driver_probe()? > > > Nope, you have to use platform_driver_register() for DT based drivers. > > > subsys_initcall is the earliest you can use platform_driver_probe(). > > > > So drivers/misc/atmel_tclib.c and drivers/pinctrl/pinctrl-coh901.c, which use > > arch_initcall(), cannot work? > How those drivers work I do not know. However, I tried with the rzn1 clock > driver and it does not work. > > > If that is really true, you can still use subsys_initcall() in the clock driver, and > > subsys_initcall_sync() in the pinctrl driver. > True, I'm not sure how people decide which initcall to use and whether the > _sync variants of the initcalls have a special meaning or intention. As far as > I know they were introduced for threaded probes, so are we supposed to > use them for driver dependencies like this? > > Do you know why the rza1 pinctrl driver is running from core_initcall()? Probably because it can? Note that drivers/clk/renesas/clk-rz.c uses CLK_OF_DECLARE(). Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org 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
diff --git a/drivers/clk/renesas/r9a06g032-clocks.c b/drivers/clk/renesas/r9a06g032-clocks.c index a0b6ecd..b03d616 100644 --- a/drivers/clk/renesas/r9a06g032-clocks.c +++ b/drivers/clk/renesas/r9a06g032-clocks.c @@ -877,17 +877,18 @@ static const struct of_device_id r9a06g032_match[] = { { } }; -static struct platform_driver r9a06g032_clock_driver = { +static struct platform_driver r9a06g032_clock_driver __refdata = { .driver = { .name = "renesas,r9a06g032-sysctrl", .of_match_table = r9a06g032_match, }, + .probe = r9a06g032_clocks_probe, }; static int __init r9a06g032_clocks_init(void) { - return platform_driver_probe(&r9a06g032_clock_driver, - r9a06g032_clocks_probe); + platform_driver_register(&r9a06g032_clock_driver); + return 0; } -subsys_initcall(r9a06g032_clocks_init); +core_initcall(r9a06g032_clocks_init);
To avoid all SoC peripheral drivers deferring their probes, both clock and pinctrl drivers should already be probed. Since the pinctrl driver requires a clock to access the registers, the clock driver should be probed before the pinctrl driver. Therefore, move the clock driver from subsys_initcall to core_initcall. Signed-off-by: Phil Edworthy <phil.edworthy@renesas.com> --- drivers/clk/renesas/r9a06g032-clocks.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-)