[lkml]   [2014]   [Mar]   [19]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
Messages in this thread
SubjectRe: [PATCH v11 20/27] iommu/exynos: allow having multiple System MMUs for a master H/W
On 19.03.2014 14:20, Tomasz Figa wrote:
> On 19.03.2014 01:39, Cho KyongHo wrote:
>> On Tue, 18 Mar 2014 15:26:48 +0100, Tomasz Figa wrote:
>>> On 18.03.2014 14:01, Cho KyongHo wrote:
>>>> On Fri, 14 Mar 2014 17:12:03 +0100, Tomasz Figa wrote:
>>>>> Hi KyongHo,
>>>>> On 14.03.2014 06:10, Cho KyongHo wrote:
>>>>>> Some master device descriptor like fimc-is which is an abstraction
>>>>>> of very complex H/W may have multiple System MMUs. For those devices,
>>>>>> the design of the link between System MMU and its master H/W is
>>>>>> needed
>>>>>> to be reconsidered.
>>>>>> A link structure, sysmmu_list_data is introduced that provides a link
>>>>>> to master H/W and that has a pointer to the device descriptor of a
>>>>>> System MMU. Given a device descriptor of a master H/W, it is possible
>>>>>> to traverse all System MMUs that must be controlled along with the
>>>>>> master H/W.
>>>>> NAK.
>>>>> A device driver should handle particular hardware instances
>>>>> separately,
>>>>> without abstracting a virtual hardware instance consisting of multiple
>>>>> physical ones.
>>>>> If such abstraction is needed, it should be done above the
>>>>> exynos-iommu
>>>>> driver, e.g. by something like iommu-composite driver that would
>>>>> aggregate several IOMMUs. Keep in mind that such IOMMUs in a group
>>>>> could
>>>>> be different, e.g. different Exynos SysMMU versions or even completely
>>>>> different IPs handled by different drivers.
>>>>> Still, I don't think there is a real need for such abstraction.
>>>>> Instead,
>>>>> related drivers shall be fixed to properly handle multiple memory
>>>>> masters and their IOMMUs.
>>>> G2D, Scalers and FIMD of Exynos5420 has 2 System MMUs while aother
>>>> SoC like
>>>> Exynos5250 does not.
>>>> I don't understand why you are negative to this approach.
>>>> This is the simplest than the others.
>>>> Let me show you an example.
>>>> FIMC-IS driver just controls MCU in FIMC-IS subsystem and the
>>>> firmware of
>>>> the MCU controls all other peripherals in the subsystem. Each
>>>> peripherals
>>>> have their own System MMU. Moreover, the configuration of the
>>>> peripherals
>>>> varies according to the SoCs.
>>>> If System MMU driver accepts multiple masters, everything is done in
>>>> DT.
>>>> But I worry that it is not easy if System MMU driver does not support
>>>> multiple masters.
>>> I believe I have stated enough reasons why this kind of implementation
>>> is bad. I'm not going to waste time repeating myself.
>>> Your concerns presented above are valid, however they are not related to
>>> what is wrong with this patch. I have given you two proper ways to
>>> handle this, none should be forced upon particular IOMMU master drivers
>>> - their authors should have the chance to select the method that works
>>> best for them.
>> I don't still understand why you think this patch is wrong.
>> I think this is the best way not to think for all the driver developers
>> about other things than their business logic.
> I agree, but one of the ways I proposed (an iommu-composite layer above
> the IOMMU low level drivers) doesn't add any extra responsibility of
> driver developers.
> Moreover, it's this kind of business logic in low level drivers that is
> adding more responsibility, because it introduces additional complexity
> and makes the driver harder to read, maintain and extend in future.
>> This does not hurt anyone and I think this is good enough.
> Well, it is barely good enough. It is a good practice to make a low
> level driver handle a single device instance and this is how Linux
> driver model is designed.
> Moreover, a single device tree node _must_ represent a single hardware
> block, so you can't group multiple SysMMUs into a single device tree node.

OK, you add nodes for single SysMMUs devices which is fine, sorry. I was
under impression that one kernel device (struct device) corresponds to
multiple SysMMUs, but this was before your patches, sorry. So one issue
less, but it's still not good.

> Furthermore, if you force grouping of SysMMUs into a single virtual one,
> you enforce using the same address space for all masters of some
> particular hardware blocks, while potentially driver developers would
> like to separate them.

Probably some clarification is needed. Your other patch adds:

sysmmu_fimd0w04: sysmmu@14640000 {
compatible = "samsung,sysmmu-v3.3";
reg = <0x14640000 0x1000>;
interrupt-parent = <&combiner>;
interrupts = <3 2>;
clock-names = "sysmmu", "master";
clocks = <&clock 422>, <&clock 421>;
samsung,power-domain = <&disp_pd>;
mmu-masters = <&fimd>;

sysmmu_fimd0w123: sysmmu@14680000 {
compatible = "samsung,sysmmu-v3.3";
reg = <0x14680000 0x1000>;
interrupt-parent = <&combiner>;
interrupts = <3 0>;
clock-names = "sysmmu", "master";
clocks = <&clock 423>, <&clock 421>;
samsung,power-domain = <&disp_pd>;
mmu-masters = <&fimd>;

From such description, in future FIMD driver won't be able to determine
which SysMMU is used for windows 0 and 4 and which for windows 1, 2 and
3. However it would be desirable to handle both SysMMUs separately,
allowing each SysMMU to have only mappings for frame buffers needed by
respective FIMD windows.

> A good interface design should not enforce any particular implementation
> and this is what we should stick to in this case as well.
>> If you want to provide another layer between master device and system mmu
>> as you mentioned, you do that. This patch does not restrict it.
> It does, as mentioned above. With a device tree listing multiple SysMMUs
> as one, you can't separate them.

What I mean is that based on DT description, drivers should be able to
control SysMMUs separately if they want to.

Best regards,

 \ /
  Last update: 2014-03-19 16:41    [W:0.083 / U:27.636 seconds]
©2003-2018 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site