Message-ID: <50E5F96E.2040703@gmail.com>
Date:	Thu, 03 Jan 2013 22:34:38 +0100
From:	Sylwester Nawrocki <sylvester.nawrocki@...il.com>
To:	KyongHo Cho <pullip.cho@...sung.com>
CC:	Linux ARM Kernel <linux-arm-kernel@...ts.infradead.org>,
	Linux IOMMU <iommu@...ts.linux-foundation.org>,
	Linux Kernel <linux-kernel@...r.kernel.org>,
	Linux Samsung SOC <linux-samsung-soc@...r.kernel.org>,
	Hyunwoong Kim <khw0178.kim@...sung.com>,
	Joerg Roedel <joro@...tes.org>,
	Kukjin Kim <kgene.kim@...sung.com>,
	Prathyush <prathyush.k@...sung.com>,
	Rahul Sharma <rahul.sharma@...sung.com>,
	Subash Patel <supash.ramaswamy@...aro.org>,
	devicetree-discuss <devicetree-discuss@...ts.ozlabs.org>,
	Thomas Abraham <thomas.abraham@...aro.org>,
	Tomasz Figa <t.figa@...sung.com>
Subject: Re: [PATCH v6 05/12] iommu/exynos: support for device tree

On 01/02/2013 06:53 AM, KyongHo Cho wrote:
>> > +/*
>> > + * Descriptions of Device Tree node for System MMU
>> > + *
>> > + * A System MMU should be described by a single tree node.
>> > + *
>> > + * A System MMU node should have the following properties:
>> > + * - reg: tuples of the base address and the size of the IO region
> of System MMU
>> > + * - compatible: it must be "samsung,exynos-sysmmu".
>>
>>  I think this compatible property is too generic. It should include
>>  specific SoC
>>  name in it, e.g. samsung,exynos4210-sysmmu. Please refer to this thread
>>  http://www.spinics.net/lists/linux-omap/msg83512.html, which discusses
>>  similar issue.
>
> Ok, I got the point. BTW , I think this will cause adding a
> compatibility property whenever a SoC is released even though it has
> completely sameSystem MMU. Is it inevitable?

No, there is no need for separate 'compatible' property for each SoC.
If the iommu is same across several SoCs same 'compatible' string should be
used for it. Please refer to the below table.

  SoC       | sysmmu type |    compatible
-------------------------------------------------------------
exynos4210 |  A          | samsung,exynos4210-sysmmu
exynos4212 |  A          | samsung,exynos4210-sysmmu
exynos4412 |  B          | samsung,exynos4412-sysmmu
exynos5250 |  C          | samsung,exynos5250-sysmmu

In fact, the sysmmu device compatible property could contain more than one
string, e.g.

   compatible = "samsung,exynos4412-sysmmu", "samsung,exynos4210-sysmmu";

if type A is subset of type B in terms of functionality and the register
maps are compatible.

It might be useful to put in the documentation which compatible applies
to which SoC. Then the driver can distinguish between different versions
of the hardware by looking at the 'compatible' property.

In patch "[PATCH v6 10/12] iommu/exynos: pass version information from DT"
you are doing something that looks strange to me. Couldn't you use 
compatible
property to derive the SYSMMU major/minor version information ? Or do these
numbers differ even within single SoC type ?

>> > + * - interrupt-parent = specify if the interrupt of System MMU is
> generated by
>> > + *   interrupt combiner or interrupt controller.
>> > + * - interrupts: tuples of interrupt numbers. a tuple has 2 elements if
>> > + *   @interrupt-parent is '<&combiner>', 3 elements otherwise.
>>
>>  It's probably enough to say that format of the interrupts property is
>>  dependant on
>>  the interrupt controller used.
>
> I agree that :)
>
>> > + *
>> > + * 'mmuname', 'reg' and 'interrupts' properties can be an array if
> the System
>> > + * MMU driver controls several number of System MMUs at the same
> time. Note that
>> > + * the number of elements in those three properties must be the same.
>>
>>  It might be useful to provide some example here.
>
> examples are there in Documentations directory.

Oops, sorry. I missed these are just comments in the code. AFAIU, there is
no need to keep 2 copies of the binding's description. I would put it only
under the Documentation directory.

>> > + * The following properties are optional:
>> > + * - mmuname: name of the System MMU for debugging purpose
>>
>>  Not sure if it is something that is supposed to be included in FDT. You
>>  could
>>  probably derive it from the 'compatible' property, if it is really
>>  needed. The device
>>  tree bindings should not be treated as direct replacement for platform
>>  data structures.
>
> actually it is just for debugging purpose.
> I understand that it should not be there.
> Thank you.
>
>> > + * - mmu-master: reference to the node of the master device.
>> > + * - mmu-master-compat: 'compatible' proberty of the node of the
> master device
>> > + *    of System MMU. This is ignored if @mmu-master is currectly
> specified.
>> > + * - mmu-master-no: instance number of the master device of System
> MMU. This is
>> > + *    ignored if @mmu-master is correctly specified. This is '0' by
> default.
>>
>>  Maybe device node aliases would be better alternative here ? But what
>>  would be
>>  a use case where you can't set 'mmu-master' and 'mmu-master-no' is
> needed ?
>
> It is for platform devices that are not specified in FDT.
> Don't we need consider that?

I don't think there is a need for that. But I might be missing something.
In any case it's probably better to use aliases, e.g. like it's done with
the exynos5 gscaler device.
--
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