[PATCH] ARM: dts: add interrupt-names property to get interrupt resource by name

[PATCH] ARM: dts: add interrupt-names property to get interrupt resource by name

[Vikas Sajjan]

The FIMD driver expects the "vsync" interrupt to be mentioned as the 1st
parameter in the FIMD DT node. So to meet this expectation of the driver,
the FIMD DT node was forced to be made by keeping "vsync" as the 1st
parameter.

this resolves the above mentioned "hack" by introducing
"interrupt-names", so that FIMD driver can get the interrupt resource by
name as discussed at
http://www.mail-archive.com/linux-samsung-soc@vger.kernel.org/msg16211.html

patch is dependent on https://patchwork.kernel.org/patch/2184981/

Signed-off-by: Vikas Sajjan <vikas.sajjan@linaro.org>
---
 arch/arm/boot/dts/exynos5250.dtsi |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/arm/boot/dts/exynos5250.dtsi b/arch/arm/boot/dts/exynos5250.dtsi
index 0ee4706..76c8911 100644
--- a/arch/arm/boot/dts/exynos5250.dtsi
+++ b/arch/arm/boot/dts/exynos5250.dtsi
@@ -588,6 +588,7 @@
 		compatible = "samsung,exynos5-fimd";
 		interrupt-parent = <&combiner>;
 		reg = <0x14400000 0x40000>;
-		interrupts = <18 5>, <18 4>, <18 6>;
+		interrupt-names = "fifo", "vsync", "lcd_sys";
+		interrupts = <18 4>, <18 5>, <18 6>;
 	};
 };
-- 
1.7.9.5

Re: [PATCH] ARM: dts: add interrupt-names property to get interrupt resource by name

[Rob Herring]

The subject is completely misleading. Make it clear what the scope of
this patch is.

On 03/13/2013 06:26 AM, Vikas Sajjan wrote:
> The FIMD driver expects the "vsync" interrupt to be mentioned as the 1st
> parameter in the FIMD DT node. So to meet this expectation of the driver,
> the FIMD DT node was forced to be made by keeping "vsync" as the 1st
> parameter.
> 
> this resolves the above mentioned "hack" by introducing
> "interrupt-names", so that FIMD driver can get the interrupt resource by
> name as discussed at
> http://www.mail-archive.com/linux-samsung-soc@vger.kernel.org/msg16211.html

I fail to see what the hack is. The order of interrupt properties must
be defined by the binding. interrupt-names is auxiliary data and must
not be required by an OS.

> patch is dependent on https://patchwork.kernel.org/patch/2184981/

Why the split? These should be combined.

> 
> Signed-off-by: Vikas Sajjan <vikas.sajjan@linaro.org>
> ---
>  arch/arm/boot/dts/exynos5250.dtsi |    3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm/boot/dts/exynos5250.dtsi b/arch/arm/boot/dts/exynos5250.dtsi
> index 0ee4706..76c8911 100644
> --- a/arch/arm/boot/dts/exynos5250.dtsi
> +++ b/arch/arm/boot/dts/exynos5250.dtsi
> @@ -588,6 +588,7 @@
>  		compatible = "samsung,exynos5-fimd";
>  		interrupt-parent = <&combiner>;
>  		reg = <0x14400000 0x40000>;
> -		interrupts = <18 5>, <18 4>, <18 6>;
> +		interrupt-names = "fifo", "vsync", "lcd_sys";
> +		interrupts = <18 4>, <18 5>, <18 6>;

There should be some documentation describing the order of the interrupts.

Rob

Re: [PATCH] ARM: dts: add interrupt-names property to get interrupt resource by name

[Sylwester Nawrocki]

Rob,

On 03/13/2013 03:39 PM, Rob Herring wrote:
> I fail to see what the hack is. The order of interrupt properties must
> be defined by the binding. interrupt-names is auxiliary data and must
> not be required by an OS.

It is clear that the order of the interrupts must be defined by the
bindings. But how useful <resource>-names properties are when we
cannot define them as required ? If an OS cannot rely on them then
it must use some other, reliable, method to identify the resources,
e.g. by hard coding the indexes. If we have to do it then why even
bother with the <resource>-names properties ? I can see interrupt-names
property specified as required in at least 2 bindings' documentation
and all bindings having reg-names property define it as required.
Are they wrong them ?

Sorry to bother about perhaps obvious things, but I'm really confused
now.

--

Thanks,
Sylwester

Re: [PATCH] ARM: dts: add interrupt-names property to get interrupt resource by name

[Rob Herring]

On 03/13/2013 05:42 PM, Sylwester Nawrocki wrote:
> Rob,
> 
> On 03/13/2013 03:39 PM, Rob Herring wrote:
>> I fail to see what the hack is. The order of interrupt properties must
>> be defined by the binding. interrupt-names is auxiliary data and must
>> not be required by an OS.
> 
> It is clear that the order of the interrupts must be defined by the
> bindings. But how useful <resource>-names properties are when we
> cannot define them as required ? If an OS cannot rely on them then
> it must use some other, reliable, method to identify the resources,
> e.g. by hard coding the indexes. If we have to do it then why even
> bother with the <resource>-names properties ? I can see interrupt-names
> property specified as required in at least 2 bindings' documentation
> and all bindings having reg-names property define it as required.
> Are they wrong them ?

You can require the name for a binding definition, but that does not
remove the requirement that the order and index of interrupts also be
defined by the binding. Then it is up to the OS to use names or
hard-coded indexes. Hard-coded indexes are not a hack. This is how FDT
and OF are defined to work.

I'm still not clear how changing the order of the interrupts removes a hack.

Rob

Re: [PATCH] ARM: dts: add interrupt-names property to get interrupt resource by name

[Stephen Warren]

On 03/18/2013 09:50 AM, Rob Herring wrote:
> On 03/13/2013 05:42 PM, Sylwester Nawrocki wrote:
>> Rob,
>>
>> On 03/13/2013 03:39 PM, Rob Herring wrote:
>>> I fail to see what the hack is. The order of interrupt properties must
>>> be defined by the binding. interrupt-names is auxiliary data and must
>>> not be required by an OS.

Is that true for all foo-names properties, or only for interrupt-names?
I was under the impression that foo-names was specifically invented so
that the order of the entries didn't matter, and instead they could be
requested by name.

Re: [PATCH] ARM: dts: add interrupt-names property to get interrupt resource by name

[Rob Herring]

On 03/18/2013 01:11 PM, Stephen Warren wrote:
> On 03/18/2013 09:50 AM, Rob Herring wrote:
>> On 03/13/2013 05:42 PM, Sylwester Nawrocki wrote:
>>> Rob,
>>>
>>> On 03/13/2013 03:39 PM, Rob Herring wrote:
>>>> I fail to see what the hack is. The order of interrupt properties must
>>>> be defined by the binding. interrupt-names is auxiliary data and must
>>>> not be required by an OS.
> 
> Is that true for all foo-names properties, or only for interrupt-names?
> I was under the impression that foo-names was specifically invented so
> that the order of the entries didn't matter, and instead they could be
> requested by name.

I think it depends on the specific name the property is tied too. For
interrupt and reg properties which have a long history and convention,
the order should be defined. IIRC, this was Grant's position too. For
new bindings, perhaps we can be more lenient.

Rob

Re: [PATCH] ARM: dts: add interrupt-names property to get interrupt resource by name

[Arnd Bergmann]

On Monday 18 March 2013 17:27:35 Rob Herring wrote:
> 
> I think it depends on the specific name the property is tied too. For
> interrupt and reg properties which have a long history and convention,
> the order should be defined. IIRC, this was Grant's position too. For
> new bindings, perhaps we can be more lenient.

At least in case of the dmaengine binding, order is not significant
and cannot be, because you may have multiple DMA specifiers referring
to the a output of the slave device connected to multiple dma engines,
and we require that they all use the same name in that case.

	Arnd

Re: [PATCH] ARM: dts: add interrupt-names property to get interrupt resource by name

[Stephen Warren]

On 03/18/2013 04:27 PM, Rob Herring wrote:
> On 03/18/2013 01:11 PM, Stephen Warren wrote:
>> On 03/18/2013 09:50 AM, Rob Herring wrote:
>>> On 03/13/2013 05:42 PM, Sylwester Nawrocki wrote:
>>>> Rob,
>>>>
>>>> On 03/13/2013 03:39 PM, Rob Herring wrote:
>>>>> I fail to see what the hack is. The order of interrupt properties must
>>>>> be defined by the binding. interrupt-names is auxiliary data and must
>>>>> not be required by an OS.
>>
>> Is that true for all foo-names properties, or only for interrupt-names?
>> I was under the impression that foo-names was specifically invented so
>> that the order of the entries didn't matter, and instead they could be
>> requested by name.
> 
> I think it depends on the specific name the property is tied too. For
> interrupt and reg properties which have a long history and convention,
> the order should be defined. IIRC, this was Grant's position too. For
> new bindings, perhaps we can be more lenient.

OK, that makes sense for interrupts/reg. Can we decide that clock-namess
are new-style and that order is not significant? I guess gpio-names too?

I guess this should be documented in whatever binding describes the core
interrupts/reg-names/gpio-names/clock-names/dma-names properties.

Re: [PATCH] ARM: dts: add interrupt-names property to get interrupt resource by name

[Sylwester Nawrocki]

On 03/19/2013 12:05 AM, Stephen Warren wrote:
> On 03/18/2013 04:27 PM, Rob Herring wrote:
>> On 03/18/2013 01:11 PM, Stephen Warren wrote:
>>> On 03/18/2013 09:50 AM, Rob Herring wrote:
>>>> On 03/13/2013 05:42 PM, Sylwester Nawrocki wrote:
>>>>> Rob,
>>>>>
>>>>> On 03/13/2013 03:39 PM, Rob Herring wrote:
>>>>>> I fail to see what the hack is. The order of interrupt properties must
>>>>>> be defined by the binding. interrupt-names is auxiliary data and must
>>>>>> not be required by an OS.
>>>
>>> Is that true for all foo-names properties, or only for interrupt-names?
>>> I was under the impression that foo-names was specifically invented so
>>> that the order of the entries didn't matter, and instead they could be
>>> requested by name.
>>
>> I think it depends on the specific name the property is tied too. For
>> interrupt and reg properties which have a long history and convention,
>> the order should be defined. IIRC, this was Grant's position too. For
>> new bindings, perhaps we can be more lenient.
>
> OK, that makes sense for interrupts/reg. Can we decide that clock-namess
> are new-style and that order is not significant? I guess gpio-names too?
>
> I guess this should be documented in whatever binding describes the core
> interrupts/reg-names/gpio-names/clock-names/dma-names properties.

It certainly would be useful to have it documented somewhere. Not sure if
resource-names.txt would be a good place to have more information about the
order for each property.

Re: [PATCH] ARM: dts: add interrupt-names property to get interrupt resource by name

[Sylwester Nawrocki]

On 03/18/2013 04:50 PM, Rob Herring wrote:
> On 03/13/2013 05:42 PM, Sylwester Nawrocki wrote:
>> On 03/13/2013 03:39 PM, Rob Herring wrote:
>>> I fail to see what the hack is. The order of interrupt properties must
>>> be defined by the binding. interrupt-names is auxiliary data and must
>>> not be required by an OS.
>>
>> It is clear that the order of the interrupts must be defined by the
>> bindings. But how useful<resource>-names properties are when we
>> cannot define them as required ? If an OS cannot rely on them then
>> it must use some other, reliable, method to identify the resources,
>> e.g. by hard coding the indexes. If we have to do it then why even
>> bother with the<resource>-names properties ? I can see interrupt-names
>> property specified as required in at least 2 bindings' documentation
>> and all bindings having reg-names property define it as required.
>> Are they wrong them ?
>
> You can require the name for a binding definition, but that does not
> remove the requirement that the order and index of interrupts also be
> defined by the binding. Then it is up to the OS to use names or
> hard-coded indexes. Hard-coded indexes are not a hack. This is how FDT
> and OF are defined to work.

OK, that makes it all crystal clear for me, thanks.

> I'm still not clear how changing the order of the interrupts removes a hack.

Originally the binding in question was not specifying the interrupt
order at all. And the drivers required order of the interrupt resources
different than in what they were normally defined in the SoC interrupt
combiner. So I suggested to use the interrupt-names property to make it
all more explicit and less error prone. I once had to fix the order of
the FIMD interrupts in the device tree to make the display work, since
I used a patch where the interrupts were listed in the IRQ combiner order,
and not the order expected by the driver.

I wasn't really clear then whether the order of interrupts needs to be
still fixed by the binding when the interrupt-names property was used.

That said I don't think there was previously any hack there. Just an
undocumented expected order of the interrupts in DT, different than the
normal order in the IRQ combiner. So it is really hard to agree with
what's written in the $subject patch description as it is now.

Thanks,
Sylwester

Re: [PATCH] ARM: dts: add interrupt-names property to get interrupt resource by name

[Vikas Sajjan]

HI,

On Wed, Mar 20, 2013 at 3:10 AM, Sylwester Nawrocki
<sylvester.nawrocki@gmail.com> wrote:
> On 03/18/2013 04:50 PM, Rob Herring wrote:
>>
>> On 03/13/2013 05:42 PM, Sylwester Nawrocki wrote:
>>>
>>> On 03/13/2013 03:39 PM, Rob Herring wrote:
>>>>
>>>> I fail to see what the hack is. The order of interrupt properties must
>>>> be defined by the binding. interrupt-names is auxiliary data and must
>>>> not be required by an OS.
>>>
>>>
>>> It is clear that the order of the interrupts must be defined by the
>>> bindings. But how useful<resource>-names properties are when we
>>> cannot define them as required ? If an OS cannot rely on them then
>>> it must use some other, reliable, method to identify the resources,
>>> e.g. by hard coding the indexes. If we have to do it then why even
>>> bother with the<resource>-names properties ? I can see interrupt-names
>>> property specified as required in at least 2 bindings' documentation
>>> and all bindings having reg-names property define it as required.
>>> Are they wrong them ?
>>
>>
>> You can require the name for a binding definition, but that does not
>> remove the requirement that the order and index of interrupts also be
>> defined by the binding. Then it is up to the OS to use names or
>> hard-coded indexes. Hard-coded indexes are not a hack. This is how FDT
>> and OF are defined to work.
>
>
> OK, that makes it all crystal clear for me, thanks.
>
>
>> I'm still not clear how changing the order of the interrupts removes a
>> hack.
>
>
> Originally the binding in question was not specifying the interrupt
> order at all. And the drivers required order of the interrupt resources
> different than in what they were normally defined in the SoC interrupt
> combiner. So I suggested to use the interrupt-names property to make it
> all more explicit and less error prone. I once had to fix the order of
> the FIMD interrupts in the device tree to make the display work, since
> I used a patch where the interrupts were listed in the IRQ combiner order,
> and not the order expected by the driver.
>
> I wasn't really clear then whether the order of interrupts needs to be
> still fixed by the binding when the interrupt-names property was used.
>
> That said I don't think there was previously any hack there. Just an
> undocumented expected order of the interrupts in DT, different than the
> normal order in the IRQ combiner. So it is really hard to agree with
> what's written in the $subject patch description as it is now.
>

Yes, there was NO hack as such earlier, just the documentation was not clear.
But IMO, as suggested by Sylwester using  "interrupt-names" property
makes it more explicit and less error prone.

Regarding this patch, it will be abandoned, Leela will post a single
patch by squash this patch and
https://patchwork.kernel.org/patch/2184981/

> Thanks,
> Sylwester
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc"
> in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] ARM: dts: add interrupt-names property to get interrupt resource by name

[Vikas Sajjan]

Hi,

On 13 March 2013 20:09, Rob Herring <robherring2@gmail.com> wrote:
> The subject is completely misleading. Make it clear what the scope of
> this patch is.
>
> On 03/13/2013 06:26 AM, Vikas Sajjan wrote:
>> The FIMD driver expects the "vsync" interrupt to be mentioned as the 1st
>> parameter in the FIMD DT node. So to meet this expectation of the driver,
>> the FIMD DT node was forced to be made by keeping "vsync" as the 1st
>> parameter.
>>
>> this resolves the above mentioned "hack" by introducing
>> "interrupt-names", so that FIMD driver can get the interrupt resource by
>> name as discussed at
>> http://www.mail-archive.com/linux-samsung-soc@vger.kernel.org/msg16211.html
>
> I fail to see what the hack is. The order of interrupt properties must
> be defined by the binding. interrupt-names is auxiliary data and must
> not be required by an OS.
>
>> patch is dependent on https://patchwork.kernel.org/patch/2184981/
>
> Why the split? These should be combined.
Sure, I shall ask Leela to merge this patch with
https://patchwork.kernel.org/patch/2184981/
and resend.

>
>>
>> Signed-off-by: Vikas Sajjan <vikas.sajjan@linaro.org>
>> ---
>>  arch/arm/boot/dts/exynos5250.dtsi |    3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/arm/boot/dts/exynos5250.dtsi b/arch/arm/boot/dts/exynos5250.dtsi
>> index 0ee4706..76c8911 100644
>> --- a/arch/arm/boot/dts/exynos5250.dtsi
>> +++ b/arch/arm/boot/dts/exynos5250.dtsi
>> @@ -588,6 +588,7 @@
>>               compatible = "samsung,exynos5-fimd";
>>               interrupt-parent = <&combiner>;
>>               reg = <0x14400000 0x40000>;
>> -             interrupts = <18 5>, <18 4>, <18 6>;
>> +             interrupt-names = "fifo", "vsync", "lcd_sys";
>> +             interrupts = <18 4>, <18 5>, <18 6>;
>
> There should be some documentation describing the order of the interrupts.
>
> Rob
>



-- 
Thanks and Regards
 Vikas Sajjan