Re: [PATCH 1/2] iommu/omap: Reverse dependency between omap-iommu and omap-iommu2

[Laurent Pinchart <laurent.pinchart-ryLnwIuWjnjg/C1BVhZhaw@public.gmane.org>]

Hi Suman,

On Tuesday 09 September 2014 16:33:11 Suman Anna wrote:
> On 09/09/2014 10:45 AM, Laurent Pinchart wrote:
> > The OMAP IOMMU driver supports both the OMAP1 and OMAP2+ IOMMU variants
> > by splitting the driver into a core module and a thin arch-specific
> > operations module.
> > 
> > (In practice only the OMAP2+ module omap-iommu2 is implemented, but
> > let's not denigrate the effort.)
> 
> Thank you for the patch. I had something similar in my list of cleanup
> TODO items on the OMAP IOMMU driver, but I was intending to cut down
> more and consolidate the omap-iommu.c and omap-iommu2.c files into a
> single one.
> 
> This had been the case from the introduction of the driver going back to
> v2.6.30, and OMAP1 was never added and I doubt it would be added anytime
> in the foreseeable future.
> 
> > The arch-specific operations module registers itself with the OMAP IOMMU
> > core module at initialization time. This initializes a module global
> > arch-specific operations pointer, used at runtime by the IOMMU
> > instances.
> > 
> > This scheme causes several issues. In addition to making it impossible
> > to support different OMAP IOMMU types in a single system (which in all
> > fairness is quite unlikely to happen),
> 
> Yep, except for a few enhancements (like reporting Fault PC address on
> OMAP4 DSPs, and dropping both endianness support), the core IOMMU
> functionality hasn't changed much between OMAP2 and the latest OMAP4+
> SoCs. So, my plan was to completely get rid of the iommu_functions (it
> also eliminates the need for exporting most of the OMAP IOMMU API). So
> while I am ok with the current patch, I prefer consolidation than
> keeping the scalability alive, it can always be added if a need for that
> arises. What do you think?

I agree with your approach, but in the meantime we have a problem to solve. 
How about applying this patch now (it goes in the right direction anyway), and 
then removing the iommu functions when you will have time ?

>  it also causes initialization
> 
> > ordering issues by requiring the arch-specific operations module to be
> > loaded before any IOMMU user. This results in a probe breakage with the
> > OMAP3 ISP driver when not compiled as a module.
> 
> This can be fixed if we make the current omap-iommu2.c as a
> subsys_initcall as well, right?
> 
> regards
> Suman
> 
> > Fix the problem by inverting the dependency. Instead of having the
> > omap-iommu2 module register itself to iommu-omap, make the iommu-omap
> > retrieve the omap-iommu2 operations structure directly when probing the
> > IOMMU device. This ensures that a probed IOMMU will always have valid
> > arch-specific operations.
> > 
> > As the arch-specific operations pointer is now initialized at probe
> > time, this change requires turning it from a global variable into a
> > per-device variable.
> > 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart-ryLnwIuWjnjg/C1BVhZhaw@public.gmane.org>
> > ---
> > 
> >  drivers/iommu/omap-iommu-debug.c |  6 ++-
> >  drivers/iommu/omap-iommu.c       | 94 +++++++++++++----------------------
> >  drivers/iommu/omap-iommu.h       | 10 ++++-
> >  drivers/iommu/omap-iommu2.c      | 18 +-------
> >  4 files changed, 45 insertions(+), 83 deletions(-)

-- 
Regards,

Laurent Pinchart