| Message ID | 87iovvz3nd.wl%kuninori.morimoto.gx@renesas.com (mailing list archive) |
|---|---|
| State | Awaiting Upstream |
| Headers | show |
Hi Morimoto-san, Thank you for the patch. On Thursday 14 November 2013 02:19:54 Kuninori Morimoto wrote: > Renesas GPIO is being interlocked with PFC, and GPIO is > very basic system for R-Car. > GPIO should be initialised in same timing as PFC. > The GPIO based system doesn't work correctly without this patch. Could you please describe the failure in a bit more details ? > > Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com> > --- > drivers/gpio/gpio-rcar.c | 12 +++++++++++- > 1 file changed, 11 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpio/gpio-rcar.c b/drivers/gpio/gpio-rcar.c > index 6038966..0a10325 100644 > --- a/drivers/gpio/gpio-rcar.c > +++ b/drivers/gpio/gpio-rcar.c > @@ -454,7 +454,17 @@ static struct platform_driver gpio_rcar_device_driver = > { } > }; > > -module_platform_driver(gpio_rcar_device_driver); > +static int __init gpio_rcar_init(void) > +{ > + return platform_driver_register(&gpio_rcar_device_driver); > +} > +postcore_initcall(gpio_rcar_init); > + > +static void __exit gpio_rcar_exit(void) > +{ > + platform_driver_unregister(&gpio_rcar_device_driver); > +} > +module_exit(gpio_rcar_exit); > > MODULE_AUTHOR("Magnus Damm"); > MODULE_DESCRIPTION("Renesas R-Car GPIO Driver");
Hi Laurent > > Renesas GPIO is being interlocked with PFC, and GPIO is > > very basic system for R-Car. > > GPIO should be initialised in same timing as PFC. > > The GPIO based system doesn't work correctly without this patch. > > Could you please describe the failure in a bit more details ? Real kernel log is better than my explain :P These series needs gpio-regulator. *before* case, sh_mobile_sdhi can't use gpio-regulator because of timing issue. ----------------- before --------------------- ... sh-pfc pfc-r8a7790: r8a77900_pfc support registered ... SDHI0Vcc: Failed to request enable GPIO184: -517 reg-fixed-voltage reg-fixed-voltage.1: Failed to register regulator: -517 platform reg-fixed-voltage.1: Driver reg-fixed-voltage requests probe deferral SDHI2Vcc: Failed to request enable GPIO185: -517 reg-fixed-voltage reg-fixed-voltage.2: Failed to register regulator: -517 platform reg-fixed-voltage.2: Driver reg-fixed-voltage requests probe deferral gpio-regulator gpio-regulator.0: Could not obtain regulator setting GPIOs: -517 platform gpio-regulator.0: Driver gpio-regulator requests probe deferral gpio-regulator gpio-regulator.1: Could not obtain regulator setting GPIOs: -517 platform gpio-regulator.1: Driver gpio-regulator requests probe deferral ... gpio_rcar gpio_rcar.0: driving 32 GPIOs gpio_rcar gpio_rcar.1: driving 32 GPIOs gpio_rcar gpio_rcar.2: driving 32 GPIOs gpio_rcar gpio_rcar.3: driving 32 GPIOs gpio_rcar gpio_rcar.4: driving 32 GPIOs gpio_rcar gpio_rcar.5: driving 32 GPIOs ... sh_mobile_sdhi sh_mobile_sdhi.0: mmc0 base at 0xee100000 clock rate 97 MHz sh_mobile_sdhi sh_mobile_sdhi.2: mmc1 base at 0xee140000 clock rate 48 MHz ... SDHI0Vcc: 3300 mV SDHI2Vcc: 3300 mV vqmmc: 1800 <--> 3300 mV at 3300 mV vqmmc: 1800 <--> 3300 mV at 3300 mV ... ---------------- after ------------------ ... sh-pfc pfc-r8a7790: r8a77900_pfc support registered gpio_rcar gpio_rcar.0: driving 32 GPIOs gpio_rcar gpio_rcar.1: driving 32 GPIOs gpio_rcar gpio_rcar.2: driving 32 GPIOs gpio_rcar gpio_rcar.3: driving 32 GPIOs gpio_rcar gpio_rcar.4: driving 32 GPIOs gpio_rcar gpio_rcar.5: driving 32 GPIOs ... SDHI0Vcc: 3300 mV SDHI2Vcc: 3300 mV vqmmc: 1800 <--> 3300 mV at 3300 mV vqmmc: 1800 <--> 3300 mV at 3300 mV ... sh_mobile_sdhi sh_mobile_sdhi.0: mmc0 base at 0xee100000 clock rate 97 MHz sh_mobile_sdhi sh_mobile_sdhi.2: mmc1 base at 0xee140000 clock rate 48 MHz Best regards --- Kuninori Morimoto -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Morimoto-san, (CC'ing Linus Walleij) On Thursday 14 November 2013 16:26:53 Kuninori Morimoto wrote: > Hi Laurent > > > > Renesas GPIO is being interlocked with PFC, and GPIO is > > > very basic system for R-Car. > > > GPIO should be initialised in same timing as PFC. > > > The GPIO based system doesn't work correctly without this patch. > > > > Could you please describe the failure in a bit more details ? > > Real kernel log is better than my explain :P > These series needs gpio-regulator. > *before* case, sh_mobile_sdhi can't use gpio-regulator because of timing > issue. > > ----------------- before --------------------- > ... > sh-pfc pfc-r8a7790: r8a77900_pfc support registered > ... > SDHI0Vcc: Failed to request enable GPIO184: -517 > reg-fixed-voltage reg-fixed-voltage.1: Failed to register regulator: -517 > platform reg-fixed-voltage.1: Driver reg-fixed-voltage requests probe > deferral SDHI2Vcc: Failed to request enable GPIO185: -517 > reg-fixed-voltage reg-fixed-voltage.2: Failed to register regulator: -517 > platform reg-fixed-voltage.2: Driver reg-fixed-voltage requests probe > deferral gpio-regulator gpio-regulator.0: Could not obtain regulator > setting GPIOs: -517 platform gpio-regulator.0: Driver gpio-regulator > requests probe deferral gpio-regulator gpio-regulator.1: Could not obtain > regulator setting GPIOs: -517 platform gpio-regulator.1: Driver > gpio-regulator requests probe deferral ... Except for the verbosity of the error messages, this looks pretty sane to me. Regulator probe gets deferred because the required GPIO isn't accessible yet, and the device gets reprobed later on. I'm not against moving the gpio-rcar initialization to postcore or subsys initcall, but in that case I believe we should standardize (or at least try to) this across the GPIO drivers. We currently have $ cat drivers/gpio/gpio-*.c | grep _initcall | grep '^[a-z]' | sed 's/(.*//' | sort | uniq -c 2 arch_initcall 1 core_initcall 1 device_initcall 1 late_initcall 11 postcore_initcall 2 pure_initcall 31 subsys_initcall $ cat drivers/gpio/gpio-*.c | grep 'module_.*_driver' | sed 's/(.*//' | sort | uniq -c 3 module_i2c_driver 4 module_pci_driver 23 module_platform_driver 1 module_spi_driver Linus, do you have any guidelines on this ? > gpio_rcar gpio_rcar.0: driving 32 GPIOs > gpio_rcar gpio_rcar.1: driving 32 GPIOs > gpio_rcar gpio_rcar.2: driving 32 GPIOs > gpio_rcar gpio_rcar.3: driving 32 GPIOs > gpio_rcar gpio_rcar.4: driving 32 GPIOs > gpio_rcar gpio_rcar.5: driving 32 GPIOs > ... > sh_mobile_sdhi sh_mobile_sdhi.0: mmc0 base at 0xee100000 clock rate 97 MHz > sh_mobile_sdhi sh_mobile_sdhi.2: mmc1 base at 0xee140000 clock rate 48 MHz > ... > SDHI0Vcc: 3300 mV > SDHI2Vcc: 3300 mV > vqmmc: 1800 <--> 3300 mV at 3300 mV > vqmmc: 1800 <--> 3300 mV at 3300 mV > ... > > ---------------- after ------------------ > ... > sh-pfc pfc-r8a7790: r8a77900_pfc support registered > gpio_rcar gpio_rcar.0: driving 32 GPIOs > gpio_rcar gpio_rcar.1: driving 32 GPIOs > gpio_rcar gpio_rcar.2: driving 32 GPIOs > gpio_rcar gpio_rcar.3: driving 32 GPIOs > gpio_rcar gpio_rcar.4: driving 32 GPIOs > gpio_rcar gpio_rcar.5: driving 32 GPIOs > ... > SDHI0Vcc: 3300 mV > SDHI2Vcc: 3300 mV > vqmmc: 1800 <--> 3300 mV at 3300 mV > vqmmc: 1800 <--> 3300 mV at 3300 mV > ... > sh_mobile_sdhi sh_mobile_sdhi.0: mmc0 base at 0xee100000 clock rate 97 MHz > sh_mobile_sdhi sh_mobile_sdhi.2: mmc1 base at 0xee140000 clock rate 48 MHz
On Mon, Nov 18, 2013 at 3:00 PM, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > I'm not against moving the gpio-rcar initialization to postcore or subsys > initcall, but in that case I believe we should standardize (or at least try > to) this across the GPIO drivers. We currently have > > $ cat drivers/gpio/gpio-*.c | grep _initcall | grep '^[a-z]' | sed 's/(.*//' | > sort | uniq -c > 2 arch_initcall > 1 core_initcall > 1 device_initcall > 1 late_initcall > 11 postcore_initcall > 2 pure_initcall > 31 subsys_initcall > > $ cat drivers/gpio/gpio-*.c | grep 'module_.*_driver' | sed 's/(.*//' | sort | > uniq -c > 3 module_i2c_driver > 4 module_pci_driver > 23 module_platform_driver > 1 module_spi_driver > > Linus, do you have any guidelines on this ? The general guideline, as everybody should be aware ;-) is that we should always use module_init(), i.e. device_initcall() and let deferred probe handle any dependencies. The only exception would be things like timers and interrupt controllers... Usually not relying on deferred probe is a sign of bugs in the deferral probe path. I know "my" drivers have this problem too, I would prefer that we try to fix the root issue instead of trying to shovel initcalls around. Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Linus, On Tuesday 19 November 2013 10:57:43 Linus Walleij wrote: > On Mon, Nov 18, 2013 at 3:00 PM, Laurent Pinchart wrote: > > I'm not against moving the gpio-rcar initialization to postcore or subsys > > initcall, but in that case I believe we should standardize (or at least > > try > > to) this across the GPIO drivers. We currently have > > > > $ cat drivers/gpio/gpio-*.c | grep _initcall | grep '^[a-z]' | sed > > 's/(.*//' | sort | uniq -c > > 2 arch_initcall > > 1 core_initcall > > 1 device_initcall > > 1 late_initcall > > 11 postcore_initcall > > 2 pure_initcall > > 31 subsys_initcall > > > > $ cat drivers/gpio/gpio-*.c | grep 'module_.*_driver' | sed 's/(.*//' | > > sort | uniq -c > > 3 module_i2c_driver > > 4 module_pci_driver > > 23 module_platform_driver > > 1 module_spi_driver > > > > Linus, do you have any guidelines on this ? > > The general guideline, as everybody should be aware ;-) is that we > should always use module_init(), i.e. device_initcall() and let deferred > probe handle any dependencies. Thought so, good :-) > The only exception would be things like timers and interrupt controllers... I wouldn't be against moving regulators and gpios one level up in the initcall order, given that they provide resources used by a very large number of drivers. That would be an optimization only though, and not a work around broken probe deferral paths. > Usually not relying on deferred probe is a sign of bugs in the deferral > probe path. > > I know "my" drivers have this problem too, I would prefer that we try to fix > the root issue instead of trying to shovel initcalls around. Sounds good to me.
Hi Laurent, Linus, > > The general guideline, as everybody should be aware ;-) is that we > > should always use module_init(), i.e. device_initcall() and let deferred > > probe handle any dependencies. > > Thought so, good :-) > > > The only exception would be things like timers and interrupt controllers... > > I wouldn't be against moving regulators and gpios one level up in the initcall > order, given that they provide resources used by a very large number of > drivers. That would be an optimization only though, and not a work around > broken probe deferral paths. > > > Usually not relying on deferred probe is a sign of bugs in the deferral > > probe path. > > > > I know "my" drivers have this problem too, I would prefer that we try to fix > > the root issue instead of trying to shovel initcalls around. I see. I will fixup driver side. Thank you Best regards --- Kuninori Morimoto -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/gpio/gpio-rcar.c b/drivers/gpio/gpio-rcar.c index 6038966..0a10325 100644 --- a/drivers/gpio/gpio-rcar.c +++ b/drivers/gpio/gpio-rcar.c @@ -454,7 +454,17 @@ static struct platform_driver gpio_rcar_device_driver = { } }; -module_platform_driver(gpio_rcar_device_driver); +static int __init gpio_rcar_init(void) +{ + return platform_driver_register(&gpio_rcar_device_driver); +} +postcore_initcall(gpio_rcar_init); + +static void __exit gpio_rcar_exit(void) +{ + platform_driver_unregister(&gpio_rcar_device_driver); +} +module_exit(gpio_rcar_exit); MODULE_AUTHOR("Magnus Damm"); MODULE_DESCRIPTION("Renesas R-Car GPIO Driver");
Renesas GPIO is being interlocked with PFC, and GPIO is very basic system for R-Car. GPIO should be initialised in same timing as PFC. The GPIO based system doesn't work correctly without this patch. Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com> --- drivers/gpio/gpio-rcar.c | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-)