gpio-pxa initcall level change and machine init breakage

DISCUSSION Mike Dunn 4 participants 13 messages Apr 20, 2013 – Apr 26, 2013

Subsystems (3)

Arm (32-bit) Arm sub-architectures PXA2xx/PXA3xx support

lore

Share & tools

lore

Mike Dunn

author

2013-04-20 15:26 UTC #0 lore raw

Hi,

After a few months of neglect, I rebased my palm treo 680 kernel with the latest from Linus' official tree, and now all the calls to gpio lib functions (gpio_request(), etc) made from the init_machine() method in struct machine_desc fail. I tracked this back to commit 6c7e660a27da7494c670bfba21cfeba30457656c dated Jan 23 2013

gpio: pxa: set initcall level to module init

gpio & pinctrl driver are used together. The pinctrl driver is already launched before gpio driver in Makefile. So set gpio driver to module init level. Otherwise, the sequence will be inverted.

Signed-off-by Haojian Zhuang <[email protected]>

Signed-off-by Linus Walleij <[email protected]>

At the time, Haojian addressed a question about this patch regarding module dependencies, advising use of the deferred probe infrastructure. But unless I'm mistaken, mine is an initcall level problem, not a dependency issue among driver probes. Now that gpio-pxa initialization is at the device level (initcall6), calls to gpiolib are no longer possible from init_machine(), which remains at the initcall3 level.

I feel like I must be missing something, because a lot of pxa boards call gpiolib from init machine(), so my breakage should not be an isolated case.

If I'm not missing anything and the patch is necessary, it looks like one solution would be to move all the initializations that use gpiolib to the init_late method in struct machine_desc, which runs as initcall7.

Grateful for any advice!

Thanks,
Mike

Haojian Zhuang

2013-04-21 06:02 UTC #1 lore raw

Quoted 27 lines · Mike Dunn

Hi,
After a few months of neglect, I rebased my palm treo 680 kernel with the latest from Linus' official tree, and now all the calls to gpio lib functions (gpio_request(), etc) made from the init_machine() method in struct machine_desc fail. I tracked this back to commit 6c7e660a27da7494c670bfba21cfeba30457656c dated Jan 23 2013
gpio: pxa: set initcall level to module init
gpio & pinctrl driver are used together. The pinctrl driver is already launched before gpio driver in Makefile. So set gpio driver to module init level. Otherwise, the sequence will be inverted.
Signed-off-by Haojian Zhuang <[email protected]>
Signed-off-by Linus Walleij <[email protected]>
At the time, Haojian addressed a question about this patch regarding module dependencies, advising use of the deferred probe infrastructure. But unless I'm mistaken, mine is an initcall level problem, not a dependency issue among driver probes. Now that gpio-pxa initialization is at the device level (initcall6), calls to gpiolib are no longer possible from init_machine(), which remains at the initcall3 level.
I feel like I must be missing something, because a lot of pxa boards call gpiolib from init machine(), so my breakage should not be an isolated case.

Quoted 8 lines

If I'm not missing anything and the patch is necessary, it looks like one solution would be to move all the initializations that use gpiolib to the init_late method in struct machine_desc, which runs as initcall7.
Grateful for any advice!
Thanks,
Mike

Actually gpio request should be handled in the driver, not machine platform driver. For example, lcd init may request gpio. pxafb.c provides the callback to handle it. We need to move code of requesting gpio into the callback.

Since this change breaks current platform, we have to revert it. We still need a code cleanup in arch-pxa directory.

Linus,
Could you help to revert this commit? Or do I need to send a revert commit to you?

Regards
Haojian

Mike Dunn

author

2013-04-21 22:23 UTC #2 lore raw

Quoted 32 lines · Haojian Zhuang

Quoted 31 lines · Mike Dunn
Hi,
After a few months of neglect, I rebased my palm treo 680 kernel with the latest from Linus' official tree, and now all the calls to gpio lib functions (gpio_request(), etc) made from the init_machine() method in struct machine_desc fail. I tracked this back to commit 6c7e660a27da7494c670bfba21cfeba30457656c dated Jan 23 2013
gpio: pxa: set initcall level to module init
gpio & pinctrl driver are used together. The pinctrl driver is already launched before gpio driver in Makefile. So set gpio driver to module init level. Otherwise, the sequence will be inverted.
Signed-off-by Haojian Zhuang <[email protected]>
Signed-off-by Linus Walleij <[email protected]>
At the time, Haojian addressed a question about this patch regarding module dependencies, advising use of the deferred probe infrastructure. But unless I'm mistaken, mine is an initcall level problem, not a dependency issue among driver probes. Now that gpio-pxa initialization is at the device level (initcall6), calls to gpiolib are no longer possible from init_machine(), which remains at the initcall3 level.
I feel like I must be missing something, because a lot of pxa boards call gpiolib from init machine(), so my breakage should not be an isolated case.
If I'm not missing anything and the patch is necessary, it looks like one solution would be to move all the initializations that use gpiolib to the init_late method in struct machine_desc, which runs as initcall7.

Quoted 8 lines

Quoted 4 lines
Grateful for any advice!
Thanks,
Mike
Actually gpio request should be handled in the driver, not machine platform driver. For example, lcd init may request gpio. pxafb.c provides the callback to handle it. We need to move code of requesting gpio into the callback.

Yes, I see. Thanks for the example.

Quoted 3 lines

Since this change breaks current platform, we have to revert it. We still need a code cleanup in arch-pxa directory.

I knew some cleanup was needed to the palm stuff, but didn't realize the extent.
Should not be too difficult; I hope to have patches soon. But there are other
pxa boards that grab various gpios in init_machine() and may not be as straightforward.

Quoted 4 lines

Linus,
Could you help to revert this commit? Or do I need to send a revert commit to you?

Will the big guy see this? Only posted to linux-arm-kernel ML.

Thanks Haojian,
Mike

Haojian Zhuang

2013-04-22 00:58 UTC #3 lore raw

Quoted 63 lines · Mike Dunn

Quoted 32 lines · Haojian Zhuang
Quoted 31 lines · Mike Dunn
Hi,
After a few months of neglect, I rebased my palm treo 680 kernel with the latest from Linus' official tree, and now all the calls to gpio lib functions (gpio_request(), etc) made from the init_machine() method in struct machine_desc fail. I tracked this back to commit 6c7e660a27da7494c670bfba21cfeba30457656c dated Jan 23 2013
gpio: pxa: set initcall level to module init
gpio & pinctrl driver are used together. The pinctrl driver is already launched before gpio driver in Makefile. So set gpio driver to module init level. Otherwise, the sequence will be inverted.
Signed-off-by Haojian Zhuang <[email protected]>
Signed-off-by Linus Walleij <[email protected]>
At the time, Haojian addressed a question about this patch regarding module dependencies, advising use of the deferred probe infrastructure. But unless I'm mistaken, mine is an initcall level problem, not a dependency issue among driver probes. Now that gpio-pxa initialization is at the device level (initcall6), calls to gpiolib are no longer possible from init_machine(), which remains at the initcall3 level.
I feel like I must be missing something, because a lot of pxa boards call gpiolib from init machine(), so my breakage should not be an isolated case.
If I'm not missing anything and the patch is necessary, it looks like one solution would be to move all the initializations that use gpiolib to the init_late method in struct machine_desc, which runs as initcall7.
Quoted 8 lines
Quoted 4 lines
Grateful for any advice!
Thanks,
Mike
Actually gpio request should be handled in the driver, not machine platform driver. For example, lcd init may request gpio. pxafb.c provides the callback to handle it. We need to move code of requesting gpio into the callback.
Yes, I see. Thanks for the example.
Quoted 3 lines
Since this change breaks current platform, we have to revert it. We still need a code cleanup in arch-pxa directory.
I knew some cleanup was needed to the palm stuff, but didn't realize the extent.
Should not be too difficult; I hope to have patches soon. But there are other
pxa boards that grab various gpios in init_machine() and may not be as straightforward.
Quoted 4 lines
Linus,
Could you help to revert this commit? Or do I need to send a revert commit to you?

Quoted 4 lines

Will the big guy see this? Only posted to linux-arm-kernel ML.
Thanks Haojian,
Mike

Sorry, I forgot to loop him in.

Linus,
Should I send the revert commit to you? Or will you revert it directly?

Regards
Haojian

Linus Walleij

2013-04-23 07:26 UTC #4 lore raw

Quoted 5 lines · Haojian Zhuang

Quoted 4 lines · Mike Dunn
Will the big guy see this? Only posted to linux-arm-kernel ML.
Thanks Haojian,
Mike

I guess Haojian is referring to me, not Torvalds ...

Quoted 4 lines

Sorry, I forgot to loop him in.
Linus,
Should I send the revert commit to you? Or will you revert it directly?

How hard is it to fix the real problem?

I'm looking at arch/arm/mach-pxa/palmtreo.c, would something like this solve the problem:

Quoted 1 line

From 5acf99a576f563d48ca5d25b9e5a1bcdd09331eb Mon Sep 17 00:00:00 2001

From: Linus Walleij <[email protected]>
Date: Tue, 23 Apr 2013 09:24:43 +0200
Subject: [PATCH] ARM: pxa: set up Treo GPIOs using device initcall

This augments the Treo setup to grab LCD GPIOs at device_initcall() level instead of at init_machine() time, and propagates any returned errors so that deferred probe will work.

Signed-off-by Linus Walleij <[email protected]>

---
 arch/arm/mach-pxa/palmtreo.c | 36 +++++++++++++++++++++++-------------
 1 file changed, 23 insertions(+), 13 deletions(-)

arch/arm/mach-pxa/palmtreo.c +23 ~~-13~~

index d82a50b..669b609 100644
@@ -455,16 +455,20 @@ static void __init palmphone_common_init(void)
 }
 
 #ifdef CONFIG_MACH_TREO680
-void __init treo680_gpio_init(void)
+static int __init treo680_gpio_init(void)
 {
     unsigned int gpio;
+    int ret;
 
     /* drive all three lcd gpios high initially */
     const unsigned long lcd_flags = GPIOF_INIT_HIGH | GPIOF_DIR_OUT;
 
     /*
      * LCD GPIO initialization...
+     * only run this on the Treo680
      */
+    if (!machine_is_treo680())
+        return 0;
 
     /*
      * This is likely the power to the lcd.  Toggling it low/high appears to

@@ -474,8 +478,9 @@ void __init treo680_gpio_init(void)
      * treo680 configures it as gpio.
      */
     gpio = GPIO_NR_TREO680_LCD_POWER;
-    if (gpio_request_one(gpio, lcd_flags, "LCD power") < 0)
-        goto fail;
+    ret = gpio_request_one(gpio, lcd_flags, "LCD power");
+    if (ret < 0)
+        goto fail_lcd_power;
 
     /*
      * These two are called "enables", for lack of a better understanding.

@@ -486,28 +491,33 @@ void __init treo680_gpio_init(void)
      * revisited.
      */
     gpio = GPIO_NR_TREO680_LCD_EN;
-    if (gpio_request_one(gpio, lcd_flags, "LCD enable") < 0)
-        goto fail;
+    ret = gpio_request_one(gpio, lcd_flags, "LCD enable");
+    if (ret < 0)
+        goto fail_lcd_en;
     gpio = GPIO_NR_TREO680_LCD_EN_N;
-    if (gpio_request_one(gpio, lcd_flags, "LCD enable_n") < 0)
-        goto fail;
+    ret = gpio_request_one(gpio, lcd_flags, "LCD enable_n");
+    if (ret < 0)
+        goto fail_lcd_en_n;
 
     /* driving this low turns LCD on */
     gpio_set_value(GPIO_NR_TREO680_LCD_EN_N, 0);
 
-    return;
- fail:
-    pr_err("gpio %d initialization failed\n", gpio);
-    gpio_free(GPIO_NR_TREO680_LCD_POWER);
+    return 0;
+
+fail_lcd_en_n:
     gpio_free(GPIO_NR_TREO680_LCD_EN);
-    gpio_free(GPIO_NR_TREO680_LCD_EN_N);
+fail_lcd_en:
+    gpio_free(GPIO_NR_TREO680_LCD_POWER);
+fail_lcd_power:
+    pr_err("gpio %d initialization failed\n", gpio);
+    return ret;
 }
+device_initcall(treo680_gpio_init);
 
 static void __init treo680_init(void)
 {
     pxa2xx_mfp_config(ARRAY_AND_SIZE(treo680_pin_config));
     palmphone_common_init();
-    treo680_gpio_init();
     palm27x_mmc_init(GPIO_NR_TREO_SD_DETECT_N, GPIO_NR_TREO680_SD_READONLY,
             GPIO_NR_TREO680_SD_POWER, 0);
     treo680_docg4_flash_init();

--
1.7.11.3

(Tell me if gmail screws up the whitespace and I'll send it with git-send-email ...)

Yours,
Linus Walleij

Haojian Zhuang

2013-04-23 07:49 UTC #5 lore raw

Quoted 12 lines · Linus Walleij

Quoted 5 lines · Haojian Zhuang
Quoted 4 lines · Mike Dunn
Will the big guy see this? Only posted to linux-arm-kernel ML.
Thanks Haojian,
Mike
I guess Haojian is referring to me, not Torvalds ...
Quoted 2 lines
Sorry, I forgot to loop him in.

Quoted 8 lines

Quoted 2 lines
Linus,
Should I send the revert commit to you? Or will you revert it directly?
How hard is it to fix the real problem?
I'm looking at arch/arm/mach-pxa/palmtreo.c, would something like this solve the problem:

Your fix could resolve the issue on palmtreo. After grep gpio_request() in arch-pxa,
I found that there're a lot of files using gpio_request() in .init_machine().

Regards
Haojian

Mike Dunn

author

2013-04-23 19:42 UTC #6 lore raw

In reply to Linus Walleij

Quoted 9 lines · Linus Walleij

Quoted 5 lines · Haojian Zhuang
Quoted 4 lines · Mike Dunn
Will the big guy see this? Only posted to linux-arm-kernel ML.
Thanks Haojian,
Mike
I guess Haojian is referring to me, not Torvalds ...

Oops, sorry Linus. Note that the other Linus has the patch in his official tree.

[...]

Quoted 6 lines

How hard is it to fix the real problem?
I'm looking at arch/arm/mach-pxa/palmtreo.c, would something like this solve the problem:

Quoted 8 lines

Quoted 1 line
From 5acf99a576f563d48ca5d25b9e5a1bcdd09331eb Mon Sep 17 00:00:00 2001
From: Linus Walleij <[email protected]>
Date: Tue, 23 Apr 2013 09:24:43 +0200
Subject: [PATCH] ARM: pxa: set up Treo GPIOs using device initcall
This augments the Treo setup to grab LCD GPIOs at device_initcall() level instead of at init_machine() time, and propagates any returned errors so that deferred probe will work.

Thanks for the patch. I am not having initial success, but will troubleshoot tomorrow morning.

On a more general note... I'm surprised that gpiolib can not run earlier, for something so low-level. The kernel documentation on gpio talks about running early. I'm not involved in nor knowledgeable of the pinctrl/gpio work so I won't question the need. I'll probably be able to educate myself some more tomorrow.

Thanks again,
Mike

Mike Dunn

author

2013-04-24 16:07 UTC #12 lore raw

In reply to Haojian Zhuang

Linus,

The patch you offered yesterday does not fix things on palmtreo, because there are calls to gpio_request() in another file (palm27x.c) that are made from init_machine(). I'd be grateful for any insight into how to resolve this. Please see below...

Quoted 32 lines · Haojian Zhuang

Quoted 31 lines · Mike Dunn
Hi,
After a few months of neglect, I rebased my palm treo 680 kernel with the latest from Linus' official tree, and now all the calls to gpio lib functions (gpio_request(), etc) made from the init_machine() method in struct machine_desc fail. I tracked this back to commit 6c7e660a27da7494c670bfba21cfeba30457656c dated Jan 23 2013
gpio: pxa: set initcall level to module init
gpio & pinctrl driver are used together. The pinctrl driver is already launched before gpio driver in Makefile. So set gpio driver to module init level. Otherwise, the sequence will be inverted.
Signed-off-by Haojian Zhuang <[email protected]>
Signed-off-by Linus Walleij <[email protected]>
At the time, Haojian addressed a question about this patch regarding module dependencies, advising use of the deferred probe infrastructure. But unless I'm mistaken, mine is an initcall level problem, not a dependency issue among driver probes. Now that gpio-pxa initialization is at the device level (initcall6), calls to gpiolib are no longer possible from init_machine(), which remains at the initcall3 level.
I feel like I must be missing something, because a lot of pxa boards call gpiolib from init machine(), so my breakage should not be an isolated case.
If I'm not missing anything and the patch is necessary, it looks like one solution would be to move all the initializations that use gpiolib to the init_late method in struct machine_desc, which runs as initcall7.

Quoted 8 lines

Quoted 4 lines
Grateful for any advice!
Thanks,
Mike
Actually gpio request should be handled in the driver, not machine platform driver. For example, lcd init may request gpio. pxafb.c provides the callback to handle it. We need to move code of requesting gpio into the callback.

It appears that some pxa27x drivers will need to be reworked to accomplish this.
For example, in the case of the pxa27x lcd driver (pxafb), there are callbacks
for turning the lcd and its backlight on and off, but it seems that the gpios should have already been requested when these run. The board-specific gpios *could* be passed to the pxafb driver via platform_data, and the driver could request them in its probe function, but currently this is not done.

Moving gpiolib calls to init_late() in struct machine_desc, which runs as initcall7, may also be a solution.

Quoted 3 lines

Since this change breaks current platform, we have to revert it. We still need a code cleanup in arch-pxa directory.

Indeed, if I'm not mistaken, at least these boards are now broken:
- tosa
- baloon3
- gumstix
- hx4700
- magician
- mioa701
- palmtreo/palmcentro
- palmld
- palmtc
- palmz72

Quoted 5 lines

Linus,
Could you help to revert this commit? Or do I need to send a revert commit to you?

Thanks,
Mike

Linus Walleij

2013-04-24 19:37 UTC #7 lore raw

In reply to Mike Dunn

Quoted 2 lines · Mike Dunn

On a more general note... I'm surprised that gpiolib can not run earlier, for something so low-level.

Well I think the patch we're discussing is making it run less early, and it can actually run as arch_initcall() or similar if you wish (it's up to the driver).

However that is beside the point, Haojian is still doing the right thing trying to push this to driver init (==module_init) level. The reason is that basically all hardware drivers should be initialized at this level because the other init levels are basically for other things and driver deps are just shoehorned into them.

Instead of relying on things to be set up early one shall rely on deferred probe.

Alas, it is not an easy change, and not expected to happen quickly or painlessly.

Yours,
Linus Walleij

Robert Jarzmik

maintainer

2013-04-25 19:36 UTC #8 lore raw

Quoted 9 lines · Linus Walleij

Quoted 2 lines · Mike Dunn
On a more general note... I'm surprised that gpiolib can not run earlier, for something so low-level.
Well I think the patch we're discussing is making it run less early, and it can actually run as arch_initcall() or similar if you wish (it's up to the driver).

Quoted 8 lines

However that is beside the point, Haojian is still doing the right thing trying to push this to driver init (==module_init) level. The reason is that basically all hardware drivers should be initialized at this level because the other init levels are basically for other things and driver deps are just shoehorned into them.
Instead of relying on things to be set up early one shall rely on deferred probe.

Hi Linus,

Even if that is the long term goal, and I agree with that goal, let me quote Documentation/gpio.txt : "However, for spinlock-safe GPIOs it's OK to use them before tasking is enabled, as part of early board setup.".

When gpio abstraction was written, it was assumed GPIO usage was usable in early platform setup code. As this was granted, we (board maintainers) coded accordingly.

Now this patch breaks boards. I disagree in having my board code broken without notice. What I would find less intrusive would be to :
(a) revert the patch
(b) inform board maintainers that they must fix their board code (for example give them 1 window)
(c) put back the patch. Board maintainers who did not amend their board code cannot say anymore they didn't know it. Board maintainers will also have time to rise objections if they think they cannot change their board code (which I do not believe is possible, but who knows ...)

Ah and change the Documentation too I think, if you want GPIO to be only usable from device initcall level.

Quoted 2 lines

Alas, it is not an easy change, and not expected to happen quickly or painlessly.

Sure, so let's do it in right order, shall we ?

Cheers.

Signature · 4 lines

Robert

PS: When I say "broken code", in the mioa701 case, my phone doesn't wakeup on
phone calls. Disturbing, isn't it ?

Linus Walleij

2013-04-25 21:22 UTC #9 lore raw

Quoted 3 lines · Robert Jarzmik

Now this patch breaks boards. I disagree in having my board code broken without notice. What I would find less intrusive would be to :
(a) revert the patch

This was done by pull request to Torvalds yesterday ...

Quoted 6 lines

(b) inform board maintainers that they must fix their board code (for example give them 1 window)
(c) put back the patch. Board maintainers who did not amend their board code cannot say anymore they didn't know it. Board maintainers will also have time to rise objections if they think they cannot change their board code (which I do not believe is possible, but who knows ...)

So when it comes to the PXA there is maybe this list of things that would be nice to change, and this is likely on top of it ... moving the PXA machines over to device tree would be more important if people who like them want them so stay around, because non-device tree platforms will at some point become deprecated, and fixing the platforms to support device tree will inevitably lead to this problem being fixed as well.

Yours,
Linus Walleij

Mike Dunn

author

2013-04-26 12:38 UTC #11 lore raw

In reply to Robert Jarzmik

Quoted 29 lines · Robert Jarzmik

Quoted 9 lines · Linus Walleij
Quoted 2 lines · Mike Dunn
On a more general note... I'm surprised that gpiolib can not run earlier, for something so low-level.
Well I think the patch we're discussing is making it run less early, and it can actually run as arch_initcall() or similar if you wish (it's up to the driver).
Quoted 8 lines
However that is beside the point, Haojian is still doing the right thing trying to push this to driver init (==module_init) level. The reason is that basically all hardware drivers should be initialized at this level because the other init levels are basically for other things and driver deps are just shoehorned into them.
Instead of relying on things to be set up early one shall rely on deferred probe.
Hi Linus,
Even if that is the long term goal, and I agree with that goal, let me quote Documentation/gpio.txt : "However, for spinlock-safe GPIOs it's OK to use them before tasking is enabled, as part of early board setup.".
When gpio abstraction was written, it was assumed GPIO usage was usable in early platform setup code. As this was granted, we (board maintainers) coded accordingly.

Quoted 9 lines

Now this patch breaks boards. I disagree in having my board code broken without notice. What I would find less intrusive would be to :
(a) revert the patch
(b) inform board maintainers that they must fix their board code (for example give them 1 window)
(c) put back the patch. Board maintainers who did not amend their board code cannot say anymore they didn't know it. Board maintainers will also have time to rise objections if they think they cannot change their board code (which I do not believe is possible, but who knows ...)

Yes, thank you Robert. More than a few boards were broken. Old architectures get no respect ;)

Some drivers may need rework, not just board support code. For the pxa27x lcd driver (pxafb), I am thinking that the board will have to pass a callback function to the driver by way of platform_data, which the driver will call from its probe function. Advice gratefully received!

Quoted 2 lines

Ah and change the Documentation too I think, if you want GPIO to be only usable from device initcall level.

As Linus mentioned, it depends on the driver. There is no standard initcall level for them. Do 'grep initcall drivers/gpio/*.c' and you'll see the variety.
This is a problem with the gpio abstraction that should be addressed, at least
in the documentation. I wouldn't mind helping, but I'm pretty clueless at this point.

Thanks,
Mike

Robert Jarzmik

maintainer

2013-04-26 17:48 UTC #10 lore raw

In reply to Linus Walleij

Quoted 2 lines · Linus Walleij

On Thu, Apr 25, 2013 at 9:36 PM, Robert Jarzmik <[email protected]> wrote:
This was done by pull request to Torvalds yesterday ...

Thanks, that's a relief.

Quoted 8 lines

Quoted 7 lines
(b) inform board maintainers that they must fix their board code (for example give them 1 window)
(c) put back the patch. Board maintainers who did not amend their board code cannot say anymore they didn't know it. Board maintainers will also have time to rise objections if they think they cannot change their board code (which I do not believe is possible, but who knows ...)

Quoted 7 lines

So when it comes to the PXA there is maybe this list of things that would be nice to change, and this is likely on top of it ... moving the PXA machines over to device tree would be more important if people who like them want them so stay around, because non-device tree platforms will at some point become deprecated, and fixing the platforms to support device tree will inevitably lead to this problem being fixed as well.

Yeah, probably. That's a good long term goal too.
Now you reverted, I'm under pressure to fix mioa701 to get rid of gpio calls in its board setup code :)

As for device tree conversion, well that's another story.
For my case for example I'll have to redevelop the bootloader (IPL+SPL) in order for suspend to RAM to work ... no world is perfecgt.

Cheers.

Signature · 1 line

Robert