[PATCH] ARM: OMAP: make iommu subsys_initcall to fix builtin omap3isp

[PATCH] ARM: OMAP: make iommu subsys_initcall to fix builtin omap3isp

[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.

Reported-by: James <angweiyang@gmail.com>
Debugged-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Signed-off-by: Ohad Ben-Cohen <ohad@wizery.com>
Cc: stable <stable@vger.kernel.org>
Cc: Tony Lindgren <tony@atomide.com>
Cc: Hiroshi Doyu <hdoyu@nvidia.com>
Cc: Joerg Roedel <Joerg.Roedel@amd.com>
---
 arch/arm/mach-omap2/mailbox.c |    3 ++-
 drivers/iommu/omap-iommu.c    |    3 ++-
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/arch/arm/mach-omap2/mailbox.c b/arch/arm/mach-omap2/mailbox.c
index 609ea2d..a6db1e4 100644
--- a/arch/arm/mach-omap2/mailbox.c
+++ b/arch/arm/mach-omap2/mailbox.c
@@ -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");
diff --git a/drivers/iommu/omap-iommu.c b/drivers/iommu/omap-iommu.c
index 4e661f0..821062a 100644
--- a/drivers/iommu/omap-iommu.c
+++ b/drivers/iommu/omap-iommu.c
@@ -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)
 {
-- 
1.7.5.4

[PATCH] ARM: OMAP: make iommu subsys_initcall to fix builtin omap3isp

[Laurent Pinchart]

Hi Ohad,

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.

> Reported-by: James <angweiyang@gmail.com>
> Debugged-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Signed-off-by: Ohad Ben-Cohen <ohad@wizery.com>
> Cc: stable <stable@vger.kernel.org>
> Cc: Tony Lindgren <tony@atomide.com>
> Cc: Hiroshi Doyu <hdoyu@nvidia.com>
> Cc: Joerg Roedel <Joerg.Roedel@amd.com>
> ---
>  arch/arm/mach-omap2/mailbox.c |    3 ++-
>  drivers/iommu/omap-iommu.c    |    3 ++-
>  2 files changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm/mach-omap2/mailbox.c b/arch/arm/mach-omap2/mailbox.c
> index 609ea2d..a6db1e4 100644
> --- a/arch/arm/mach-omap2/mailbox.c
> +++ b/arch/arm/mach-omap2/mailbox.c
> @@ -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.

> +subsys_initcall(omap2_mbox_init);
>  module_exit(omap2_mbox_exit);
> 
>  MODULE_LICENSE("GPL v2");
> diff --git a/drivers/iommu/omap-iommu.c b/drivers/iommu/omap-iommu.c
> index 4e661f0..821062a 100644
> --- a/drivers/iommu/omap-iommu.c
> +++ b/drivers/iommu/omap-iommu.c
> @@ -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)
>  {
-- 
Regards,

Laurent Pinchart

[PATCH] ARM: OMAP: make iommu subsys_initcall to fix builtin omap3isp

[Ohad Ben-Cohen]

Hi Laurent,

On Sun, Feb 26, 2012 at 7:34 PM, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
> 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 ?

> 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.

>> +/* 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.

[PATCH] ARM: OMAP: make iommu subsys_initcall to fix builtin omap3isp

[Laurent Pinchart]

Hi Ohad,

On Sunday 26 February 2012 20:30:17 Ohad Ben-Cohen wrote:
> On Sun, Feb 26, 2012 at 7:34 PM, Laurent Pinchart wrote:
> > 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 ?

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 ?

> > 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.

> >> +/* 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.

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

-- 
Regards,

Laurent Pinchart

[PATCH] ARM: OMAP: make iommu subsys_initcall to fix builtin omap3isp

[Ohad Ben-Cohen]

Hi Laurent,

On Mon, Feb 27, 2012 at 12:47 AM, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
> 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.

> 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.

> 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.

[PATCH] ARM: OMAP: make iommu subsys_initcall to fix builtin omap3isp

[Joerg Roedel]

On Mon, Feb 27, 2012 at 09:00:51AM +0200, Ohad Ben-Cohen wrote:
> On Mon, Feb 27, 2012 at 12:47 AM, Laurent Pinchart
> > 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.

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


	Joerg

-- 
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

[PATCH] ARM: OMAP: make iommu subsys_initcall to fix builtin omap3isp

[Laurent Pinchart]

Hi Ohad,

On Monday 27 February 2012 09:00:51 Ohad Ben-Cohen wrote:
> On Mon, Feb 27, 2012 at 12:47 AM, Laurent Pinchart wrote:
> > 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.
> 
> > 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.
> 
> > 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 ?

-- 
Regards,

Laurent Pinchart

[PATCH] ARM: OMAP: make iommu subsys_initcall to fix builtin omap3isp

[Ohad Ben-Cohen]

Hi Laurent,

On Thu, Mar 1, 2012 at 6:37 PM, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
> 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.

[PATCH] ARM: OMAP: make iommu subsys_initcall to fix builtin omap3isp

[Joerg Roedel]

On Sun, Feb 26, 2012 at 12:14:14PM +0200, 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.
> 
> Reported-by: James <angweiyang@gmail.com>
> Debugged-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Signed-off-by: Ohad Ben-Cohen <ohad@wizery.com>
> Cc: stable <stable@vger.kernel.org>
> Cc: Tony Lindgren <tony@atomide.com>
> Cc: Hiroshi Doyu <hdoyu@nvidia.com>
> Cc: Joerg Roedel <Joerg.Roedel@amd.com>

Applied, thanks.

-- 
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