Message-ID: <532AC902.6050909@gmail.com>
Date:	Thu, 20 Mar 2014 11:54:58 +0100
From:	Tomasz Figa <tomasz.figa@...il.com>
To:	Cho KyongHo <pullip.cho@...sung.com>,
	Tomasz Figa <t.figa@...sung.com>
CC:	Linux DeviceTree <devicetree@...r.kernel.org>,
	Linux Samsung SOC <linux-samsung-soc@...r.kernel.org>,
	Prathyush <prathyush.k@...sung.com>,
	Grant Grundler <grundler@...omium.org>,
	Joerg Roedel <joro@...tes.org>,
	Linux Kernel <linux-kernel@...r.kernel.org>,
	Sachin Kamat <sachin.kamat@...aro.org>,
	Linux IOMMU <iommu@...ts.linux-foundation.org>,
	Kukjin Kim <kgene.kim@...sung.com>,
	Sylwester Nawrocki <s.nawrocki@...sung.com>,
	Varun Sethi <Varun.Sethi@...escale.com>,
	Antonios Motakis <a.motakis@...tualopensystems.com>,
	Linux ARM Kernel <linux-arm-kernel@...ts.infradead.org>,
	Rahul Sharma <rahul.sharma@...sung.com>
Subject: Re: [PATCH v11 20/27] iommu/exynos: allow having multiple System
 MMUs for a master H/W

On 20.03.2014 11:22, Cho KyongHo wrote:
> On Wed, 19 Mar 2014 16:14:57 +0100, Tomasz Figa wrote:
>> 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.
>>
>
> Ok. Understood why you have mentioned such.
>
>>> 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@...40000 {
>> 		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@...80000 {
>> 		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.
>>
>
> If it is required to map frame buffers for the System MMU of a specific window,
> you can specify different phandles to mmu-masters of sysmmu_fimd0w04 and
> sysmmu_0w123.
>
> However, I think it is more convenient that all windows of a FIMD share the
> same virtual address space because
>   - Exynos5250: FIMD has one System MMU
>   - Exynos5420: FIMD has 2 System MMUs for Window 0,4 and 1,2,3
>   - Another SoC which is not ready for upstreaming: FIMD has 2 System MMUs for window 0,1 and 2,3,4
> (I also discouraged when I found a new Soc has different H/W bus topology :)
>
> For this reason, I prefer allowing a single master to have multiple System MMU.
>

Well, it sure can be more convenient from programming point of view, but 
there might be certain security-related aspects that would prefer more 
fine-granular control over memory accesses of IP blocks.

>>> 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.
>>
>
> As I mentioned above, drivers can control every System MMU separately.

You are missing one point here - device tree stability. Once you specify 
the same phandle for both masters of FIMD (or any other multi-master IP 
block) and distribute such device tree, FIMD (or any other) driver will 
be able to support only shared address space mode on such board.

Probably the preferred solution would be to reuse stream ID mechanism of 
ARM System MMU bindings and make such multi master devices specify 
#stream-id-cells = <1> and have IDs properly assigned for each of their 
masters. This would be the best choice for consistency reasons, as 
existing bindings would be reused, without reinventing the wheel.

Best regards,
Tomasz
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists