| Message ID | 1363173980-11428-1-git-send-email-vikas.sajjan@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 -- 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
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 -- 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
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 >
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 -- 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
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. -- 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
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 -- 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
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 -- 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
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. -- 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
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 -- 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
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. -- 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
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 -- 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
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>; }; };
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(-)