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

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

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