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

[Suman Anna <s-anna-l0cyMroinI0@public.gmane.org>]

Hi Laurent,

>
> On Tuesday 09 September 2014 17:31:44 Suman Anna wrote:
>>> 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 ?
>>
>> Can you give the subsys_initcall solution a try to see if that resolves
>> the problem at hand? That would be a much smaller change, if that
>> doesn't work we can go with this patch.
> 
> It seems to work.
> 
>> I will work on my cleanup list for 3.19.
> 
> Does that schedule still hold ? If so I'll submit a simple subsys_initcall() 
> patch for v3.18.

Yes, that would be great. I am working on the patches and will post them
in a couple of days.

regards
Suman

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