diff mbox

[01/16] gpio: rcar: use postcore_init()

Message ID 87iovvz3nd.wl%kuninori.morimoto.gx@renesas.com (mailing list archive)
State Awaiting Upstream
Headers show

Commit Message

Kuninori Morimoto Nov. 14, 2013, 10:19 a.m. UTC
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(-)

Comments

Laurent Pinchart Nov. 14, 2013, 1:42 p.m. UTC | #1
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");
Kuninori Morimoto Nov. 15, 2013, 12:26 a.m. UTC | #2
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
Laurent Pinchart Nov. 18, 2013, 2 p.m. UTC | #3
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
Linus Walleij Nov. 19, 2013, 9:57 a.m. UTC | #4
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
Laurent Pinchart Nov. 19, 2013, 12:36 p.m. UTC | #5
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.
Kuninori Morimoto Nov. 21, 2013, 2:02 a.m. UTC | #6
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 mbox

Patch

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");