ARM: OMAP: make iommu subsys_initcall to fix builtin omap3isp

PATCH New Ohad Ben-Cohen 3 participants 9 messages Feb 26, 2012 – Mar 1, 2012

Subsystems (4)

Arm (32-bit) Arm sub-architectures OMAP2+ support IOMMU

lore

Share & tools

lore

Ohad Ben-Cohen

author

2012-02-26 10:14 UTC #0 lore raw

PATCH 1/1 ARM: OMAP: make iommu subsys_initcall to fix builtin omap3isp

omap3isp depends on omap's iommu and will fail to probe if initialized before it (which always happen if they are builtin).

Make omap's iommu subsys_initcall as an interim solution until the probe deferral mechanism is merged.

Reported-by James <[email protected]>

Debugged-by Laurent Pinchart <[email protected]>

Signed-off-by Ohad Ben-Cohen <[email protected]>

Cc stable <[email protected]>

Cc Tony Lindgren <[email protected]>

Cc Hiroshi Doyu <[email protected]>

Cc Joerg Roedel <[email protected]>

---
 arch/arm/mach-omap2/mailbox.c |    3 ++-
 drivers/iommu/omap-iommu.c    |    3 ++-
 2 files changed, 4 insertions(+), 2 deletions(-)

arch/arm/mach-omap2/mailbox.c +2 -1

index 609ea2d..a6db1e4 100644
@@ -412,7 +412,8 @@ static void __exit omap2_mbox_exit(void)
 	platform_driver_unregister(&omap2_mbox_driver);
 }
 
-module_init(omap2_mbox_init);
+/* must be ready before omap3isp is probed */
+subsys_initcall(omap2_mbox_init);
 module_exit(omap2_mbox_exit);
 
 MODULE_LICENSE("GPL v2");

drivers/iommu/omap-iommu.c +2 -1

index 4e661f0..821062a 100644
@@ -1222,7 +1222,8 @@ static int __init omap_iommu_init(void)
 
 	return platform_driver_register(&omap_iommu_driver);
 }
-module_init(omap_iommu_init);
+/* must be ready before omap3isp is probed */
+subsys_initcall(omap_iommu_init);
 
 static void __exit omap_iommu_exit(void)
 {

Signature · 1 line

1.7.5.4

Laurent Pinchart

2012-02-26 17:34 UTC #1 lore raw

PATCH 1/1 ARM: OMAP: make iommu subsys_initcall to fix builtin omap3isp

Hi Ohad,

Quoted 5 lines · On Sunday 26 February 2012 12:14:14 Ohad Ben-Cohen wrote:

omap3isp depends on omap's iommu and will fail to probe if initialized before it (which always happen if they are builtin).
Make omap's iommu subsys_initcall as an interim solution until the probe deferral mechanism is merged.

How will that fix the problem ?

I'm fine with this patch, as it fixes the problem as well, although I still believe modifying the link order would be a better fix in this case.

Quoted 17 lines

Reported-by James <[email protected]>
Debugged-by Laurent Pinchart <[email protected]>
Signed-off-by Ohad Ben-Cohen <[email protected]>
Cc stable <[email protected]>
Cc Tony Lindgren <[email protected]>
Cc Hiroshi Doyu <[email protected]>
Cc Joerg Roedel <[email protected]>
---
 arch/arm/mach-omap2/mailbox.c |    3 ++-
 drivers/iommu/omap-iommu.c    |    3 ++-
 2 files changed, 4 insertions(+), 2 deletions(-)
arch/arm/mach-omap2/mailbox.c +0 -0
index 609ea2d..a6db1e4 100644

arch/arm/mach-omap2/mailbox.c static void __exit omap2_mbox_exit(void) · line 415

Quoted 5 lines

arch/arm/mach-omap2/mailbox.c +1 -1

@@ -412,7 +412,8 @@ static void __exit omap2_mbox_exit(void)
 	platform_driver_unregister(&omap2_mbox_driver);
 }
 
-module_init(omap2_mbox_init);
+/* must be ready before omap3isp is probed */

The problem is not limited to the omap3isp driver, the DSP driver could be affected as well.

Quoted 18 lines

arch/arm/mach-omap2/mailbox.c +1 -0

+subsys_initcall(omap2_mbox_init);
 module_exit(omap2_mbox_exit);
 
 MODULE_LICENSE("GPL v2");

drivers/iommu/omap-iommu.c +2 -1

index 4e661f0..821062a 100644
@@ -1222,7 +1222,8 @@ static int __init omap_iommu_init(void)
 
 	return platform_driver_register(&omap_iommu_driver);
 }
-module_init(omap_iommu_init);
+/* must be ready before omap3isp is probed */
+subsys_initcall(omap_iommu_init);
 
 static void __exit omap_iommu_exit(void)
 {

Signature · 3 lines

Regards,

Laurent Pinchart

Ohad Ben-Cohen

author

2012-02-26 18:30 UTC #2 lore raw

PATCH 1/1 ARM: OMAP: make iommu subsys_initcall to fix builtin omap3isp

Hi Laurent,

Quoted 8 lines · Laurent Pinchart

Quoted 5 lines · On Sunday 26 February 2012 12:14:14 Ohad Ben-Cohen wrote:
omap3isp depends on omap's iommu and will fail to probe if initialized before it (which always happen if they are builtin).
Make omap's iommu subsys_initcall as an interim solution until the probe deferral mechanism is merged.
How will that fix the problem ?

Sorry, I'm not entirely sure I understand the question: are you asking about this patch or about the probe deferral thingy ?

Quoted 2 lines

I'm fine with this patch, as it fixes the problem as well, although I still believe modifying the link order would be a better fix in this case.

I'm personally not a huge fan of implicit link order dependencies, as someone may change the order in the future (for whatever benign reason) without even realizing he's breaking drivers.

Quoted 4 lines

Quoted 1 line
+/* must be ready before omap3isp is probed */
The problem is not limited to the omap3isp driver, the DSP driver could be affected as well.

If you refer to tidspbridge, then I'm not sure it has the same issue:
IIUC it doesn't enable the iommu hw on probe; only in response to an ioctl command (btw, tidspbridge doesn't even use this omap iommu driver - it manipulates the iommu registers directly. iicks.)

The omap4 solutions (remoteproc et al.) will only enable the iommu after userspace is alive, so no risk there as well.

Thanks,
Ohad.

Laurent Pinchart

2012-02-26 22:47 UTC #3 lore raw

PATCH 1/1 ARM: OMAP: make iommu subsys_initcall to fix builtin omap3isp

Hi Ohad,

Quoted 5 lines · On Sunday 26 February 2012 20:30:17 Ohad Ben-Cohen wrote:

Quoted 4 lines · Laurent Pinchart
Quoted 3 lines · On Sunday 26 February 2012 12:14:14 Ohad Ben-Cohen wrote:
omap3isp depends on omap's iommu and will fail to probe if initialized before it (which always happen if they are builtin).

Quoted 7 lines

Quoted 4 lines
Quoted 2 lines
Make omap's iommu subsys_initcall as an interim solution until the probe deferral mechanism is merged.
How will that fix the problem ?
Sorry, I'm not entirely sure I understand the question: are you asking about this patch or about the probe deferral thingy ?

I'm asking about the probe deferral mechanism. The omap3-isp driver will still call iommu_attach_device() in its probe function. What will happen then ? Will it return an error ? On what basis will it do so ?

Quoted 6 lines

Quoted 2 lines
I'm fine with this patch, as it fixes the problem as well, although I still believe modifying the link order would be a better fix in this case.
I'm personally not a huge fan of implicit link order dependencies, as someone may change the order in the future (for whatever benign reason) without even realizing he's breaking drivers.

That's what the comment in the Makefile is for ;-) I don't think it's a perfect solution either, but it avoids playing with the various initcalls. The OMAP3 IOMMU isn't a subsystem, subsys_initcall() looks a bit like an API abuse to me. But as I mentioned in my previous mail, I'm fine with the patch if you think it's a better solution.

Quoted 9 lines

Quoted 4 lines
Quoted 1 line
+/* must be ready before omap3isp is probed */
The problem is not limited to the omap3isp driver, the DSP driver could be affected as well.
If you refer to tidspbridge, then I'm not sure it has the same issue:
IIUC it doesn't enable the iommu hw on probe; only in response to an ioctl command (btw, tidspbridge doesn't even use this omap iommu driver - it manipulates the iommu registers directly. iicks.)

Looking at the tidspbridge driver, it seems to handle its IOMMU itself. That's an item for someone's to-do list :-)

Are drivers supposed to call iommu_attach_device() in their probe() function or later on ? The API doesn't seem to be documented.

Quoted 2 lines

The omap4 solutions (remoteproc et al.) will only enable the iommu after userspace is alive, so no risk there as well.

Signature · 3 lines

Regards,

Laurent Pinchart

Ohad Ben-Cohen

author

2012-02-27 07:00 UTC #4 lore raw

PATCH 1/1 ARM: OMAP: make iommu subsys_initcall to fix builtin omap3isp

Hi Laurent,

Quoted 3 lines · Laurent Pinchart

I'm asking about the probe deferral mechanism. The omap3-isp driver will still call iommu_attach_device() in its probe function. What will happen then ? Will it return an error ? On what basis will it do so ?

Yes, iommu_attach_device() will fail, and omap3isp's probe function will then need to return -EAGAIN to request that its probe function will be invoked again later on.

Quoted 4 lines

That's what the comment in the Makefile is for ;-) I don't think it's a perfect solution either, but it avoids playing with the various initcalls. The OMAP3 IOMMU isn't a subsystem, subsys_initcall() looks a bit like an API abuse to me.

Yes, it's dirty.

But it's explicit and consistent across build system changes (without imposing anything on the build system). We do it all the time with other subsystems. We don't like it, but luckily Grant came up with the deferred probing mechanism, which will fix this all very nicely.

Quoted 2 lines

Are drivers supposed to call iommu_attach_device() in their probe() function or later on ?

If you can defer attaching to a later point, most commonly to a point where the device is opened by the user, it's better - less power will be consumed.

Thanks,
Ohad.

Joerg Roedel

2012-02-27 12:02 UTC #5 lore raw

PATCH 1/1 ARM: OMAP: make iommu subsys_initcall to fix builtin omap3isp

Quoted 6 lines · Ohad Ben-Cohen

On Mon, Feb 27, 2012 at 12:47 AM, Laurent Pinchart
Quoted 4 lines
That's what the comment in the Makefile is for ;-) I don't think it's a perfect solution either, but it avoids playing with the various initcalls. The OMAP3 IOMMU isn't a subsystem, subsys_initcall() looks a bit like an API abuse to me.

Quoted 6 lines

Yes, it's dirty.
But it's explicit and consistent across build system changes (without imposing anything on the build system). We do it all the time with other subsystems. We don't like it, but luckily Grant came up with the deferred probing mechanism, which will fix this all very nicely.

Agreed. It is not the best fix, but at least more reliable then the Makefile change.

Joerg

Signature · 5 lines

AMD Operating System Research Center

Advanced Micro Devices GmbH Einsteinring 24 85609 Dornach
General Managers: Alberto Bozzo
Registration: Dornach, Landkr. Muenchen; Registerger. Muenchen, HRB Nr. 43632

Laurent Pinchart

2012-03-01 16:37 UTC #6 lore raw

PATCH 1/1 ARM: OMAP: make iommu subsys_initcall to fix builtin omap3isp

In reply to Ohad Ben-Cohen

Hi Ohad,

Quoted 21 lines · On Monday 27 February 2012 09:00:51 Ohad Ben-Cohen wrote:

Quoted 3 lines · Laurent Pinchart
I'm asking about the probe deferral mechanism. The omap3-isp driver will still call iommu_attach_device() in its probe function. What will happen then ? Will it return an error ? On what basis will it do so ?
Yes, iommu_attach_device() will fail, and omap3isp's probe function will then need to return -EAGAIN to request that its probe function will be invoked again later on.
Quoted 4 lines
That's what the comment in the Makefile is for ;-) I don't think it's a perfect solution either, but it avoids playing with the various initcalls. The OMAP3 IOMMU isn't a subsystem, subsys_initcall() looks a bit like an API abuse to me.
Yes, it's dirty.
But it's explicit and consistent across build system changes (without imposing anything on the build system). We do it all the time with other subsystems. We don't like it, but luckily Grant came up with the deferred probing mechanism, which will fix this all very nicely.

Quoted 6 lines

Quoted 2 lines
Are drivers supposed to call iommu_attach_device() in their probe() function or later on ?
If you can defer attaching to a later point, most commonly to a point where the device is opened by the user, it's better - less power will be consumed.

I'll try that then. How expensive is the iommu_attach_device() (and detach) operation in terms of CPU time ?

Signature · 3 lines

Regards,

Laurent Pinchart

Ohad Ben-Cohen

author

2012-03-01 17:12 UTC #7 lore raw

PATCH 1/1 ARM: OMAP: make iommu subsys_initcall to fix builtin omap3isp

Hi Laurent,

Quoted 2 lines · Laurent Pinchart

I'll try that then. How expensive is the iommu_attach_device() (and detach) operation in terms of CPU time ?

omap_iommu_attach() basically enables the iommu clock and configures that IP block.

I suspect it's negligible but I didn't really measure it...

Thanks,
Ohad.

Joerg Roedel

Applied

2012-02-27 13:23 UTC #8 lore raw

PATCH 1/1 ARM: OMAP: make iommu subsys_initcall to fix builtin omap3isp

In reply to Ohad Ben-Cohen

Quoted 6 lines · Ohad Ben-Cohen

omap3isp depends on omap's iommu and will fail to probe if initialized before it (which always happen if they are builtin).
Make omap's iommu subsys_initcall as an interim solution until the probe deferral mechanism is merged.

Quoted 7 lines

Reported-by James <[email protected]>
Debugged-by Laurent Pinchart <[email protected]>
Signed-off-by Ohad Ben-Cohen <[email protected]>
Cc stable <[email protected]>
Cc Tony Lindgren <[email protected]>
Cc Hiroshi Doyu <[email protected]>
Cc Joerg Roedel <[email protected]>

Applied, thanks.

Signature · 5 lines

AMD Operating System Research Center

Advanced Micro Devices GmbH Einsteinring 24 85609 Dornach
General Managers: Alberto Bozzo
Registration: Dornach, Landkr. Muenchen; Registerger. Muenchen, HRB Nr. 43632