* [RFC PATCH 1/1] of/irq: create interrupts-extended-2 property
@ 2014-01-15 11:47 Jean-Christophe PLAGNIOL-VILLARD
[not found] ` < 20140115161224.GH25824@e106331-lin.cambridge.arm.com>
` (2 more replies)
0 siblings, 3 replies; 16+ messages in thread
From: Jean-Christophe PLAGNIOL-VILLARD @ 2014-01-15 11:47 UTC (permalink / raw)
To: linux-arm-kernel
The new interrupts-extended property, which reuses the phandle+arguments
pattern used by GPIOs and other core bindings, still have some issue.
If an SoC have already specifiy interrupt and a board want to add specific
interrupt such as GPIO (which can be optionnal) be need to re-define
interrupts-extended. So allow to have an optionnale interrupts-extended-2
property.
Today the problem is ofen solve by defining a gpio-xxx property and then do a gpio_to_irq
in the C code. *Which is wrong!!*. We need to describe the IRQ.
Signed-off-by: Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>
Cc: Grant Likely <grant.likely@linaro.org>
Cc: Rob Herring <rob.herring@calxeda.com>
---
We have the same issue on pinctrl
.../devicetree/bindings/interrupt-controller/interrupts.txt | 4 ++++
drivers/of/irq.c | 10 ++++++++--
2 files changed, 12 insertions(+), 2 deletions(-)
diff --git a/Documentation/devicetree/bindings/interrupt-controller/interrupts.txt b/Documentation/devicetree/bindings/interrupt-controller/interrupts.txt
index 1486497..5d559fd 100644
--- a/Documentation/devicetree/bindings/interrupt-controller/interrupts.txt
+++ b/Documentation/devicetree/bindings/interrupt-controller/interrupts.txt
@@ -25,8 +25,12 @@ to reference multiple interrupt parents. Each entry in this property contains
both the parent phandle and the interrupt specifier. "interrupts-extended"
should only be used when a device has multiple interrupt parents.
+The "interrupts-extended-2" allow to extend at board level node interrupt without
+having to re-define the SoC interrupts.
+
Example:
interrupts-extended = <&intc1 5 1>, <&intc2 1 0>;
+ interrupts-extended-2 = <&intc1 6 1>
A device node may contain either "interrupts" or "interrupts-extended", but not
both. If both properties are present, then the operating system should log an
diff --git a/drivers/of/irq.c b/drivers/of/irq.c
index 786b0b4..bc36710 100644
--- a/drivers/of/irq.c
+++ b/drivers/of/irq.c
@@ -307,8 +307,14 @@ int of_irq_parse_one(struct device_node *device, int index, struct of_phandle_ar
/* Try the new-style interrupts-extended */
res = of_parse_phandle_with_args(device, "interrupts-extended",
"#interrupt-cells", index, out_irq);
- if (res)
- return -EINVAL;
+ if (res) {
+ /* Try the new-style interrupts-extended-2 */
+ res = of_parse_phandle_with_args(device, "interrupts-extended-2",
+ "#interrupt-cells", index, out_irq);
+ if (res)
+ return -EINVAL;
+ }
+
return of_irq_parse_raw(addr, out_irq);
}
intlen /= sizeof(*intspec);
--
1.8.4.3
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [RFC PATCH 1/1] of/irq: create interrupts-extended-2 property
2014-01-15 11:47 [RFC PATCH 1/1] of/irq: create interrupts-extended-2 property Jean-Christophe PLAGNIOL-VILLARD
[not found] ` < 20140115161224.GH25824@e106331-lin.cambridge.arm.com>
[not found] ` < 20140115121750.GE25824@e106331-lin.cambridge.arm.com>
@ 2014-01-15 12:17 ` Mark Rutland
2014-01-15 12:40 ` Jean-Christophe PLAGNIOL-VILLARD
2014-01-15 14:56 ` Grant Likely
2 siblings, 2 replies; 16+ messages in thread
From: Mark Rutland @ 2014-01-15 12:17 UTC (permalink / raw)
To: linux-arm-kernel
On Wed, Jan 15, 2014 at 11:47:25AM +0000, Jean-Christophe PLAGNIOL-VILLARD wrote:
> The new interrupts-extended property, which reuses the phandle+arguments
> pattern used by GPIOs and other core bindings, still have some issue.
>
> If an SoC have already specifiy interrupt and a board want to add specific
> interrupt such as GPIO (which can be optionnal) be need to re-define
> interrupts-extended. So allow to have an optionnale interrupts-extended-2
> property.
>
NAK.
This is a hack that works around a dts organisation issue. This is _not_
a binding or parsing issue.
Properties can be overridden - just describe all of the interrupts in
the final dts file.
> Today the problem is ofen solve by defining a gpio-xxx property and then do a gpio_to_irq
> in the C code. *Which is wrong!!*. We need to describe the IRQ.
>
> Signed-off-by: Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>
> Cc: Grant Likely <grant.likely@linaro.org>
> Cc: Rob Herring <rob.herring@calxeda.com>
> ---
>
> We have the same issue on pinctrl
> .../devicetree/bindings/interrupt-controller/interrupts.txt | 4 ++++
> drivers/of/irq.c | 10 ++++++++--
> 2 files changed, 12 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/interrupt-controller/interrupts.txt b/Documentation/devicetree/bindings/interrupt-controller/interrupts.txt
> index 1486497..5d559fd 100644
> --- a/Documentation/devicetree/bindings/interrupt-controller/interrupts.txt
> +++ b/Documentation/devicetree/bindings/interrupt-controller/interrupts.txt
> @@ -25,8 +25,12 @@ to reference multiple interrupt parents. Each entry in this property contains
> both the parent phandle and the interrupt specifier. "interrupts-extended"
> should only be used when a device has multiple interrupt parents.
>
> +The "interrupts-extended-2" allow to extend at board level node interrupt without
> +having to re-define the SoC interrupts.
> +
> Example:
> interrupts-extended = <&intc1 5 1>, <&intc2 1 0>;
> + interrupts-extended-2 = <&intc1 6 1>
>
> A device node may contain either "interrupts" or "interrupts-extended", but not
> both. If both properties are present, then the operating system should log an
> diff --git a/drivers/of/irq.c b/drivers/of/irq.c
> index 786b0b4..bc36710 100644
> --- a/drivers/of/irq.c
> +++ b/drivers/of/irq.c
> @@ -307,8 +307,14 @@ int of_irq_parse_one(struct device_node *device, int index, struct of_phandle_ar
> /* Try the new-style interrupts-extended */
> res = of_parse_phandle_with_args(device, "interrupts-extended",
> "#interrupt-cells", index, out_irq);
> - if (res)
> - return -EINVAL;
> + if (res) {
> + /* Try the new-style interrupts-extended-2 */
> + res = of_parse_phandle_with_args(device, "interrupts-extended-2",
> + "#interrupt-cells", index, out_irq);
This also breaks error reporting.
What if you have an interrupt in the middle of interrupts-extended which
fails to parse? This will jump to the interrupts-extended-2 property and
grab the wrong interrupt rather than propagating the error.
Thanks,
Mark.
^ permalink raw reply [flat|nested] 16+ messages in thread
* [RFC PATCH 1/1] of/irq: create interrupts-extended-2 property
2014-01-15 12:17 ` Mark Rutland
@ 2014-01-15 12:40 ` Jean-Christophe PLAGNIOL-VILLARD
2014-01-15 13:46 ` Mark Rutland
2014-01-15 14:56 ` Grant Likely
1 sibling, 1 reply; 16+ messages in thread
From: Jean-Christophe PLAGNIOL-VILLARD @ 2014-01-15 12:40 UTC (permalink / raw)
To: linux-arm-kernel
On 12:17 Wed 15 Jan , Mark Rutland wrote:
> On Wed, Jan 15, 2014 at 11:47:25AM +0000, Jean-Christophe PLAGNIOL-VILLARD wrote:
> > The new interrupts-extended property, which reuses the phandle+arguments
> > pattern used by GPIOs and other core bindings, still have some issue.
> >
> > If an SoC have already specifiy interrupt and a board want to add specific
> > interrupt such as GPIO (which can be optionnal) be need to re-define
> > interrupts-extended. So allow to have an optionnale interrupts-extended-2
> > property.
> >
>
> NAK.
>
> This is a hack that works around a dts organisation issue. This is _not_
> a binding or parsing issue.
So the DT is stupid. Yes w have board and SoC information
so we do not want to duplicate them. Having a way to descript SoC vs board
specific information need to be provided.
>
> Properties can be overridden - just describe all of the interrupts in
> the final dts file.
this is wrong, which mean you duplicate informaation and duplicate bug and
fixes
>
> > Today the problem is ofen solve by defining a gpio-xxx property and then do a gpio_to_irq
> > in the C code. *Which is wrong!!*. We need to describe the IRQ.
> >
> > Signed-off-by: Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>
> > Cc: Grant Likely <grant.likely@linaro.org>
> > Cc: Rob Herring <rob.herring@calxeda.com>
> > ---
> >
> > We have the same issue on pinctrl
> > .../devicetree/bindings/interrupt-controller/interrupts.txt | 4 ++++
> > drivers/of/irq.c | 10 ++++++++--
> > 2 files changed, 12 insertions(+), 2 deletions(-)
> >
> > diff --git a/Documentation/devicetree/bindings/interrupt-controller/interrupts.txt b/Documentation/devicetree/bindings/interrupt-controller/interrupts.txt
> > index 1486497..5d559fd 100644
> > --- a/Documentation/devicetree/bindings/interrupt-controller/interrupts.txt
> > +++ b/Documentation/devicetree/bindings/interrupt-controller/interrupts.txt
> > @@ -25,8 +25,12 @@ to reference multiple interrupt parents. Each entry in this property contains
> > both the parent phandle and the interrupt specifier. "interrupts-extended"
> > should only be used when a device has multiple interrupt parents.
> >
> > +The "interrupts-extended-2" allow to extend at board level node interrupt without
> > +having to re-define the SoC interrupts.
> > +
> > Example:
> > interrupts-extended = <&intc1 5 1>, <&intc2 1 0>;
> > + interrupts-extended-2 = <&intc1 6 1>
> >
> > A device node may contain either "interrupts" or "interrupts-extended", but not
> > both. If both properties are present, then the operating system should log an
> > diff --git a/drivers/of/irq.c b/drivers/of/irq.c
> > index 786b0b4..bc36710 100644
> > --- a/drivers/of/irq.c
> > +++ b/drivers/of/irq.c
> > @@ -307,8 +307,14 @@ int of_irq_parse_one(struct device_node *device, int index, struct of_phandle_ar
> > /* Try the new-style interrupts-extended */
> > res = of_parse_phandle_with_args(device, "interrupts-extended",
> > "#interrupt-cells", index, out_irq);
> > - if (res)
> > - return -EINVAL;
> > + if (res) {
> > + /* Try the new-style interrupts-extended-2 */
> > + res = of_parse_phandle_with_args(device, "interrupts-extended-2",
> > + "#interrupt-cells", index, out_irq);
>
> This also breaks error reporting.
>
> What if you have an interrupt in the middle of interrupts-extended which
> fails to parse? This will jump to the interrupts-extended-2 property and
> grab the wrong interrupt rather than propagating the error.
>
> Thanks,
> Mark.
^ permalink raw reply [flat|nested] 16+ messages in thread
* [RFC PATCH 1/1] of/irq: create interrupts-extended-2 property
2014-01-15 12:40 ` Jean-Christophe PLAGNIOL-VILLARD
@ 2014-01-15 13:46 ` Mark Rutland
2014-01-15 14:01 ` Jean-Christophe PLAGNIOL-VILLARD
` (2 more replies)
0 siblings, 3 replies; 16+ messages in thread
From: Mark Rutland @ 2014-01-15 13:46 UTC (permalink / raw)
To: linux-arm-kernel
On Wed, Jan 15, 2014 at 12:40:41PM +0000, Jean-Christophe PLAGNIOL-VILLARD wrote:
> On 12:17 Wed 15 Jan , Mark Rutland wrote:
> > On Wed, Jan 15, 2014 at 11:47:25AM +0000, Jean-Christophe PLAGNIOL-VILLARD wrote:
> > > The new interrupts-extended property, which reuses the phandle+arguments
> > > pattern used by GPIOs and other core bindings, still have some issue.
> > >
> > > If an SoC have already specifiy interrupt and a board want to add specific
> > > interrupt such as GPIO (which can be optionnal) be need to re-define
> > > interrupts-extended. So allow to have an optionnale interrupts-extended-2
> > > property.
> > >
> >
> > NAK.
> >
> > This is a hack that works around a dts organisation issue. This is _not_
> > a binding or parsing issue.
>
> So the DT is stupid. Yes w have board and SoC information
> so we do not want to duplicate them. Having a way to descript SoC vs board
> specific information need to be provided.
I agree that the current situation isn't fantastic. That doesn't make
the DT stupid. There are other solutions to this problem.
>
> >
> > Properties can be overridden - just describe all of the interrupts in
> > the final dts file.
> this is wrong, which mean you duplicate informaation and duplicate bug and
> fixes
It's certainly not nice, but it doesn't make it wrong.
The same problem can be found with any other list property that varies
per-board. Hacking the parsing of each property is not a solution.
Today the only way to implement what you want is to have the list in the
final file.
One way of working with that would be to have the common interrupts
described in a shared macro in the soc file:
#define SOC_COMMON_INTERRUPTS <&intc1 5 1>, <&intc2 1 0>
Then the soc file can have:
interrupts = SOC_COMMON_INTERRUPTS;
And the board can have:
interrupts = SOC_COMMON_INTERRUPTS, <&intc1 6 1>;
That gains functionality equivalent to your proposal, without
introducing new code or new edge cases in interrupt parsing. However, it
does make it unclear how many interrupts are described, and it won't
interact well with interrupt-names.
Another, more invasive option would be extend the dts syntax and teach
dtc to handle property appending. Then the soc dts could stay as it is,
and the board dts could have something like:
/append-property/ interrupts = <&intc1 6 1>;
/append-property/ interrupt-names = "board-specific-irq";
Both these options solve the issue at the source, are general to other
properties, and allow more than one level of hierarchy (the proposed
interrupts-extended-2 only allows one level).
Thanks,
Mark.
^ permalink raw reply [flat|nested] 16+ messages in thread
* [RFC PATCH 1/1] of/irq: create interrupts-extended-2 property
2014-01-15 13:46 ` Mark Rutland
@ 2014-01-15 14:01 ` Jean-Christophe PLAGNIOL-VILLARD
2014-01-15 15:16 ` Rob Herring
2014-01-15 16:12 ` Mark Rutland
2 siblings, 0 replies; 16+ messages in thread
From: Jean-Christophe PLAGNIOL-VILLARD @ 2014-01-15 14:01 UTC (permalink / raw)
To: linux-arm-kernel
On 13:46 Wed 15 Jan , Mark Rutland wrote:
> On Wed, Jan 15, 2014 at 12:40:41PM +0000, Jean-Christophe PLAGNIOL-VILLARD wrote:
> > On 12:17 Wed 15 Jan , Mark Rutland wrote:
> > > On Wed, Jan 15, 2014 at 11:47:25AM +0000, Jean-Christophe PLAGNIOL-VILLARD wrote:
> > > > The new interrupts-extended property, which reuses the phandle+arguments
> > > > pattern used by GPIOs and other core bindings, still have some issue.
> > > >
> > > > If an SoC have already specifiy interrupt and a board want to add specific
> > > > interrupt such as GPIO (which can be optionnal) be need to re-define
> > > > interrupts-extended. So allow to have an optionnale interrupts-extended-2
> > > > property.
> > > >
> > >
> > > NAK.
> > >
> > > This is a hack that works around a dts organisation issue. This is _not_
> > > a binding or parsing issue.
> >
> > So the DT is stupid. Yes w have board and SoC information
> > so we do not want to duplicate them. Having a way to descript SoC vs board
> > specific information need to be provided.
>
> I agree that the current situation isn't fantastic. That doesn't make
> the DT stupid. There are other solutions to this problem.
>
> >
> > >
> > > Properties can be overridden - just describe all of the interrupts in
> > > the final dts file.
> > this is wrong, which mean you duplicate informaation and duplicate bug and
> > fixes
>
> It's certainly not nice, but it doesn't make it wrong.
>
> The same problem can be found with any other list property that varies
> per-board. Hacking the parsing of each property is not a solution.
agreed we have the same issue on pinctrl as example and it's become a
nightmare
>
> Today the only way to implement what you want is to have the list in the
> final file.
>
> One way of working with that would be to have the common interrupts
> described in a shared macro in the soc file:
>
> #define SOC_COMMON_INTERRUPTS <&intc1 5 1>, <&intc2 1 0>
>
> Then the soc file can have:
>
> interrupts = SOC_COMMON_INTERRUPTS;
>
> And the board can have:
>
> interrupts = SOC_COMMON_INTERRUPTS, <&intc1 6 1>;
hugly for me as a board could work on x SoC so the final dt could be compile
for more than 1 SoC if you one alias
>
> That gains functionality equivalent to your proposal, without
> introducing new code or new edge cases in interrupt parsing. However, it
> does make it unclear how many interrupts are described, and it won't
> interact well with interrupt-names.
>
> Another, more invasive option would be extend the dts syntax and teach
> dtc to handle property appending. Then the soc dts could stay as it is,
> and the board dts could have something like:
>
> /append-property/ interrupts = <&intc1 6 1>;
> /append-property/ interrupt-names = "board-specific-irq";
>
> Both these options solve the issue at the source, are general to other
> properties, and allow more than one level of hierarchy (the proposed
> interrupts-extended-2 only allows one level).
agreed here but this touch dtc
Best Regards,
J.
>
> Thanks,
> Mark.
^ permalink raw reply [flat|nested] 16+ messages in thread
* [RFC PATCH 1/1] of/irq: create interrupts-extended-2 property
2014-01-15 12:17 ` Mark Rutland
2014-01-15 12:40 ` Jean-Christophe PLAGNIOL-VILLARD
@ 2014-01-15 14:56 ` Grant Likely
1 sibling, 0 replies; 16+ messages in thread
From: Grant Likely @ 2014-01-15 14:56 UTC (permalink / raw)
To: linux-arm-kernel
On Wed, Jan 15, 2014 at 12:17 PM, Mark Rutland <mark.rutland@arm.com> wrote:
> On Wed, Jan 15, 2014 at 11:47:25AM +0000, Jean-Christophe PLAGNIOL-VILLARD wrote:
>> The new interrupts-extended property, which reuses the phandle+arguments
>> pattern used by GPIOs and other core bindings, still have some issue.
>>
>> If an SoC have already specifiy interrupt and a board want to add specific
>> interrupt such as GPIO (which can be optionnal) be need to re-define
>> interrupts-extended. So allow to have an optionnale interrupts-extended-2
>> property.
>>
>
> NAK.
>
> This is a hack that works around a dts organisation issue. This is _not_
> a binding or parsing issue.
>
> Properties can be overridden - just describe all of the interrupts in
> the final dts file.
Agreed. The current binding handles the case of multiple interrupts just fine.
g.
^ permalink raw reply [flat|nested] 16+ messages in thread
* [RFC PATCH 1/1] of/irq: create interrupts-extended-2 property
2014-01-15 13:46 ` Mark Rutland
2014-01-15 14:01 ` Jean-Christophe PLAGNIOL-VILLARD
@ 2014-01-15 15:16 ` Rob Herring
2014-01-15 15:28 ` Rob Herring
2014-01-15 16:12 ` Mark Rutland
2 siblings, 1 reply; 16+ messages in thread
From: Rob Herring @ 2014-01-15 15:16 UTC (permalink / raw)
To: linux-arm-kernel
On Wed, Jan 15, 2014 at 7:46 AM, Mark Rutland <mark.rutland@arm.com> wrote:
> On Wed, Jan 15, 2014 at 12:40:41PM +0000, Jean-Christophe PLAGNIOL-VILLARD wrote:
>> On 12:17 Wed 15 Jan , Mark Rutland wrote:
>> > On Wed, Jan 15, 2014 at 11:47:25AM +0000, Jean-Christophe PLAGNIOL-VILLARD wrote:
>> > > The new interrupts-extended property, which reuses the phandle+arguments
>> > > pattern used by GPIOs and other core bindings, still have some issue.
>> > >
>> > > If an SoC have already specifiy interrupt and a board want to add specific
>> > > interrupt such as GPIO (which can be optionnal) be need to re-define
>> > > interrupts-extended. So allow to have an optionnale interrupts-extended-2
>> > > property.
>> > >
>> >
>> > NAK.
>> >
>> > This is a hack that works around a dts organisation issue. This is _not_
>> > a binding or parsing issue.
>>
>> So the DT is stupid. Yes w have board and SoC information
>> so we do not want to duplicate them. Having a way to descript SoC vs board
>> specific information need to be provided.
>
> I agree that the current situation isn't fantastic. That doesn't make
> the DT stupid. There are other solutions to this problem.
>
>>
>> >
>> > Properties can be overridden - just describe all of the interrupts in
>> > the final dts file.
>> this is wrong, which mean you duplicate informaation and duplicate bug and
>> fixes
>
> It's certainly not nice, but it doesn't make it wrong.
>
> The same problem can be found with any other list property that varies
> per-board. Hacking the parsing of each property is not a solution.
>
> Today the only way to implement what you want is to have the list in the
> final file.
>
> One way of working with that would be to have the common interrupts
> described in a shared macro in the soc file:
>
> #define SOC_COMMON_INTERRUPTS <&intc1 5 1>, <&intc2 1 0>
Yuck. Please, let's not encourage this pattern.
> Then the soc file can have:
>
> interrupts = SOC_COMMON_INTERRUPTS;
>
> And the board can have:
>
> interrupts = SOC_COMMON_INTERRUPTS, <&intc1 6 1>;
Unless this is solved in dtc include mechanism, just duplicate the
interrupts in each board file. I'd rather have the duplication than
continued churn of refactoring dtsi files.
Rob
^ permalink raw reply [flat|nested] 16+ messages in thread
* [RFC PATCH 1/1] of/irq: create interrupts-extended-2 property
2014-01-15 15:16 ` Rob Herring
@ 2014-01-15 15:28 ` Rob Herring
0 siblings, 0 replies; 16+ messages in thread
From: Rob Herring @ 2014-01-15 15:28 UTC (permalink / raw)
To: linux-arm-kernel
Fixing DT list address...
On Wed, Jan 15, 2014 at 9:16 AM, Rob Herring <robherring2@gmail.com> wrote:
> On Wed, Jan 15, 2014 at 7:46 AM, Mark Rutland <mark.rutland@arm.com> wrote:
>> On Wed, Jan 15, 2014 at 12:40:41PM +0000, Jean-Christophe PLAGNIOL-VILLARD wrote:
>>> On 12:17 Wed 15 Jan , Mark Rutland wrote:
>>> > On Wed, Jan 15, 2014 at 11:47:25AM +0000, Jean-Christophe PLAGNIOL-VILLARD wrote:
>>> > > The new interrupts-extended property, which reuses the phandle+arguments
>>> > > pattern used by GPIOs and other core bindings, still have some issue.
>>> > >
>>> > > If an SoC have already specifiy interrupt and a board want to add specific
>>> > > interrupt such as GPIO (which can be optionnal) be need to re-define
>>> > > interrupts-extended. So allow to have an optionnale interrupts-extended-2
>>> > > property.
>>> > >
>>> >
>>> > NAK.
>>> >
>>> > This is a hack that works around a dts organisation issue. This is _not_
>>> > a binding or parsing issue.
>>>
>>> So the DT is stupid. Yes w have board and SoC information
>>> so we do not want to duplicate them. Having a way to descript SoC vs board
>>> specific information need to be provided.
>>
>> I agree that the current situation isn't fantastic. That doesn't make
>> the DT stupid. There are other solutions to this problem.
>>
>>>
>>> >
>>> > Properties can be overridden - just describe all of the interrupts in
>>> > the final dts file.
>>> this is wrong, which mean you duplicate informaation and duplicate bug and
>>> fixes
>>
>> It's certainly not nice, but it doesn't make it wrong.
>>
>> The same problem can be found with any other list property that varies
>> per-board. Hacking the parsing of each property is not a solution.
>>
>> Today the only way to implement what you want is to have the list in the
>> final file.
>>
>> One way of working with that would be to have the common interrupts
>> described in a shared macro in the soc file:
>>
>> #define SOC_COMMON_INTERRUPTS <&intc1 5 1>, <&intc2 1 0>
>
> Yuck. Please, let's not encourage this pattern.
>
>> Then the soc file can have:
>>
>> interrupts = SOC_COMMON_INTERRUPTS;
>>
>> And the board can have:
>>
>> interrupts = SOC_COMMON_INTERRUPTS, <&intc1 6 1>;
>
> Unless this is solved in dtc include mechanism, just duplicate the
> interrupts in each board file. I'd rather have the duplication than
> continued churn of refactoring dtsi files.
>
> Rob
^ permalink raw reply [flat|nested] 16+ messages in thread
* [RFC PATCH 1/1] of/irq: create interrupts-extended-2 property
2014-01-15 13:46 ` Mark Rutland
2014-01-15 14:01 ` Jean-Christophe PLAGNIOL-VILLARD
2014-01-15 15:16 ` Rob Herring
@ 2014-01-15 16:12 ` Mark Rutland
2014-01-15 18:08 ` Jean-Christophe PLAGNIOL-VILLARD
2014-01-20 22:47 ` Grant Likely
2 siblings, 2 replies; 16+ messages in thread
From: Mark Rutland @ 2014-01-15 16:12 UTC (permalink / raw)
To: linux-arm-kernel
>
> Another, more invasive option would be extend the dts syntax and teach
> dtc to handle property appending. Then the soc dts could stay as it is,
> and the board dts could have something like:
>
> /append-property/ interrupts = <&intc1 6 1>;
> /append-property/ interrupt-names = "board-specific-irq";
>
> Both these options solve the issue at the source, are general to other
> properties, and allow more than one level of hierarchy (the proposed
> interrupts-extended-2 only allows one level).
I've just had a go at implementing the append-property mechanism above
in dtc, and it was far easier than I expected (patch below).
Does anyone have any issues with the /append-property/ idea?
Thanks,
Mark.
---->8----
>From 88be0036b6a966bd7506f58e3cb9ce9ea4c5ac48 Mon Sep 17 00:00:00 2001
From: Mark Rutland <mark.rutland@arm.com>
Date: Wed, 15 Jan 2014 15:43:51 +0000
Subject: [PATCH] dtc: add ability to append properties
When dealing with hierarchies of dtsi files, handling minute differences
between individual boards can become very painful. Adding a single
board-specific interrupt requires duplicating the entire interrupts
property, which requires duplication of common values. This makes bug
fixing painful and if not handled very carefully files diverge rapdily.
To ameliorate this, this patch adds the ability to append properties,
allowing board files to describe only the additional values required in
a property. This functionality also works with strings, so parallel
properties like interupts and interrupt-names stay in sync. Properties
may be appended multiple times, and deleting properties clears all
previous appended values.
To append a property, secondary definitions must be prefixed with
/append-property/. This is longer than a possible '+=' syntax, but makes
it far easier to spot when appending behaviour is requested, and so
hopefully will lead to fewer buggy dts files.
For example, if the following dts fragements are compiled together:
/ {
interrupts = <0>, <1>;
interrupt-names = "zero", "one";
};
/ {
/append-property/ interrupts = <2>, <3>;
/append-property/ interrupt-names = "two", "three";
};
They will result in a dtb equivalent to the following dts fragment:
/ {
interrupts = <0>, <1>, <2>, <3>;
interrupt-names = "zero", "one", "two", "three";
};
Signed-off-by: Mark Rutland <mark.rutland@arm.com>
---
dtc-lexer.l | 7 +++++++
dtc-parser.y | 5 +++++
dtc.h | 2 ++
livetree.c | 15 ++++++++++++++-
4 files changed, 28 insertions(+), 1 deletion(-)
diff --git a/dtc-lexer.l b/dtc-lexer.l
index 0cd7e67..7989abe 100644
--- a/dtc-lexer.l
+++ b/dtc-lexer.l
@@ -131,6 +131,13 @@ static bool pop_input_file(void);
return DT_DEL_PROP;
}
+<*>"/append-property/" {
+ DPRINT("Keyword: /append-property/\n");
+ DPRINT("<PROPNODENAME>\n");
+ BEGIN(PROPNODENAME);
+ return DT_APPEND_PROP;
+ }
+
<*>"/delete-node/" {
DPRINT("Keyword: /delete-node/\n");
DPRINT("<PROPNODENAME>\n");
diff --git a/dtc-parser.y b/dtc-parser.y
index 4864631..a8409ed 100644
--- a/dtc-parser.y
+++ b/dtc-parser.y
@@ -63,6 +63,7 @@ static unsigned char eval_char_literal(const char *s);
%token DT_LSHIFT DT_RSHIFT DT_LE DT_GE DT_EQ DT_NE DT_AND DT_OR
%token DT_BITS
%token DT_DEL_PROP
+%token DT_APPEND_PROP
%token DT_DEL_NODE
%token <propnodename> DT_PROPNODENAME
%token <literal> DT_LITERAL
@@ -195,6 +196,10 @@ propdef:
{
$$ = build_property($1, empty_data);
}
+ | DT_APPEND_PROP DT_PROPNODENAME '=' propdata ';'
+ {
+ $$ = build_property_append($2, $4);
+ }
| DT_DEL_PROP DT_PROPNODENAME ';'
{
$$ = build_property_delete($2);
diff --git a/dtc.h b/dtc.h
index 20e4d56..8687530 100644
--- a/dtc.h
+++ b/dtc.h
@@ -133,6 +133,7 @@ struct label {
};
struct property {
+ bool appended;
bool deleted;
char *name;
struct data val;
@@ -186,6 +187,7 @@ void delete_labels(struct label **labels);
struct property *build_property(char *name, struct data val);
struct property *build_property_delete(char *name);
+struct property *build_property_append(char *name, struct data val);
struct property *chain_property(struct property *first, struct property *list);
struct property *reverse_properties(struct property *first);
diff --git a/livetree.c b/livetree.c
index b61465f..894e42b 100644
--- a/livetree.c
+++ b/livetree.c
@@ -74,6 +74,15 @@ struct property *build_property_delete(char *name)
return new;
}
+struct property *build_property_append(char *name, struct data val)
+{
+ struct property *new = build_property(name, val);
+
+ new->appended = 1;
+
+ return new;
+}
+
struct property *chain_property(struct property *first, struct property *list)
{
assert(first->next == NULL);
@@ -167,7 +176,11 @@ struct node *merge_nodes(struct node *old_node, struct node *new_node)
for_each_label_withdel(new_prop->labels, l)
add_label(&old_prop->labels, l->label);
- old_prop->val = new_prop->val;
+ if (new_prop->appended)
+ old_prop->val = data_merge(old_prop->val, new_prop->val);
+ else
+ old_prop->val = new_prop->val;
+
old_prop->deleted = 0;
free(new_prop);
new_prop = NULL;
--
1.8.1.1
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [RFC PATCH 1/1] of/irq: create interrupts-extended-2 property
2014-01-15 16:12 ` Mark Rutland
@ 2014-01-15 18:08 ` Jean-Christophe PLAGNIOL-VILLARD
2014-01-20 22:47 ` Grant Likely
1 sibling, 0 replies; 16+ messages in thread
From: Jean-Christophe PLAGNIOL-VILLARD @ 2014-01-15 18:08 UTC (permalink / raw)
To: linux-arm-kernel
On 16:12 Wed 15 Jan , Mark Rutland wrote:
> >
> > Another, more invasive option would be extend the dts syntax and teach
> > dtc to handle property appending. Then the soc dts could stay as it is,
> > and the board dts could have something like:
> >
> > /append-property/ interrupts = <&intc1 6 1>;
> > /append-property/ interrupt-names = "board-specific-irq";
> >
> > Both these options solve the issue at the source, are general to other
> > properties, and allow more than one level of hierarchy (the proposed
> > interrupts-extended-2 only allows one level).
>
> I've just had a go at implementing the append-property mechanism above
> in dtc, and it was far easier than I expected (patch below).
>
> Does anyone have any issues with the /append-property/ idea?
Acked-by: Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>
you finish it before me :(
Best Regards,
J.
>
> Thanks,
> Mark.
>
> ---->8----
>
> From 88be0036b6a966bd7506f58e3cb9ce9ea4c5ac48 Mon Sep 17 00:00:00 2001
> From: Mark Rutland <mark.rutland@arm.com>
> Date: Wed, 15 Jan 2014 15:43:51 +0000
> Subject: [PATCH] dtc: add ability to append properties
>
> When dealing with hierarchies of dtsi files, handling minute differences
> between individual boards can become very painful. Adding a single
> board-specific interrupt requires duplicating the entire interrupts
> property, which requires duplication of common values. This makes bug
> fixing painful and if not handled very carefully files diverge rapdily.
>
> To ameliorate this, this patch adds the ability to append properties,
> allowing board files to describe only the additional values required in
> a property. This functionality also works with strings, so parallel
> properties like interupts and interrupt-names stay in sync. Properties
> may be appended multiple times, and deleting properties clears all
> previous appended values.
>
> To append a property, secondary definitions must be prefixed with
> /append-property/. This is longer than a possible '+=' syntax, but makes
> it far easier to spot when appending behaviour is requested, and so
> hopefully will lead to fewer buggy dts files.
>
> For example, if the following dts fragements are compiled together:
>
> / {
> interrupts = <0>, <1>;
> interrupt-names = "zero", "one";
> };
>
> / {
> /append-property/ interrupts = <2>, <3>;
> /append-property/ interrupt-names = "two", "three";
> };
>
> They will result in a dtb equivalent to the following dts fragment:
>
> / {
> interrupts = <0>, <1>, <2>, <3>;
> interrupt-names = "zero", "one", "two", "three";
> };
>
> Signed-off-by: Mark Rutland <mark.rutland@arm.com>
> ---
> dtc-lexer.l | 7 +++++++
> dtc-parser.y | 5 +++++
> dtc.h | 2 ++
> livetree.c | 15 ++++++++++++++-
> 4 files changed, 28 insertions(+), 1 deletion(-)
>
> diff --git a/dtc-lexer.l b/dtc-lexer.l
> index 0cd7e67..7989abe 100644
> --- a/dtc-lexer.l
> +++ b/dtc-lexer.l
> @@ -131,6 +131,13 @@ static bool pop_input_file(void);
> return DT_DEL_PROP;
> }
>
> +<*>"/append-property/" {
> + DPRINT("Keyword: /append-property/\n");
> + DPRINT("<PROPNODENAME>\n");
> + BEGIN(PROPNODENAME);
> + return DT_APPEND_PROP;
> + }
> +
> <*>"/delete-node/" {
> DPRINT("Keyword: /delete-node/\n");
> DPRINT("<PROPNODENAME>\n");
> diff --git a/dtc-parser.y b/dtc-parser.y
> index 4864631..a8409ed 100644
> --- a/dtc-parser.y
> +++ b/dtc-parser.y
> @@ -63,6 +63,7 @@ static unsigned char eval_char_literal(const char *s);
> %token DT_LSHIFT DT_RSHIFT DT_LE DT_GE DT_EQ DT_NE DT_AND DT_OR
> %token DT_BITS
> %token DT_DEL_PROP
> +%token DT_APPEND_PROP
> %token DT_DEL_NODE
> %token <propnodename> DT_PROPNODENAME
> %token <literal> DT_LITERAL
> @@ -195,6 +196,10 @@ propdef:
> {
> $$ = build_property($1, empty_data);
> }
> + | DT_APPEND_PROP DT_PROPNODENAME '=' propdata ';'
> + {
> + $$ = build_property_append($2, $4);
> + }
> | DT_DEL_PROP DT_PROPNODENAME ';'
> {
> $$ = build_property_delete($2);
> diff --git a/dtc.h b/dtc.h
> index 20e4d56..8687530 100644
> --- a/dtc.h
> +++ b/dtc.h
> @@ -133,6 +133,7 @@ struct label {
> };
>
> struct property {
> + bool appended;
> bool deleted;
> char *name;
> struct data val;
> @@ -186,6 +187,7 @@ void delete_labels(struct label **labels);
>
> struct property *build_property(char *name, struct data val);
> struct property *build_property_delete(char *name);
> +struct property *build_property_append(char *name, struct data val);
> struct property *chain_property(struct property *first, struct property *list);
> struct property *reverse_properties(struct property *first);
>
> diff --git a/livetree.c b/livetree.c
> index b61465f..894e42b 100644
> --- a/livetree.c
> +++ b/livetree.c
> @@ -74,6 +74,15 @@ struct property *build_property_delete(char *name)
> return new;
> }
>
> +struct property *build_property_append(char *name, struct data val)
> +{
> + struct property *new = build_property(name, val);
> +
> + new->appended = 1;
> +
> + return new;
> +}
> +
> struct property *chain_property(struct property *first, struct property *list)
> {
> assert(first->next == NULL);
> @@ -167,7 +176,11 @@ struct node *merge_nodes(struct node *old_node, struct node *new_node)
> for_each_label_withdel(new_prop->labels, l)
> add_label(&old_prop->labels, l->label);
>
> - old_prop->val = new_prop->val;
> + if (new_prop->appended)
> + old_prop->val = data_merge(old_prop->val, new_prop->val);
> + else
> + old_prop->val = new_prop->val;
> +
> old_prop->deleted = 0;
> free(new_prop);
> new_prop = NULL;
> --
> 1.8.1.1
>
^ permalink raw reply [flat|nested] 16+ messages in thread
* [RFC PATCH 1/1] of/irq: create interrupts-extended-2 property
2014-01-15 16:12 ` Mark Rutland
2014-01-15 18:08 ` Jean-Christophe PLAGNIOL-VILLARD
@ 2014-01-20 22:47 ` Grant Likely
2014-01-21 1:03 ` Olof Johansson
1 sibling, 1 reply; 16+ messages in thread
From: Grant Likely @ 2014-01-20 22:47 UTC (permalink / raw)
To: linux-arm-kernel
On Wed, 15 Jan 2014 16:12:24 +0000, Mark Rutland <mark.rutland@arm.com> wrote:
> >
> > Another, more invasive option would be extend the dts syntax and teach
> > dtc to handle property appending. Then the soc dts could stay as it is,
> > and the board dts could have something like:
> >
> > /append-property/ interrupts = <&intc1 6 1>;
> > /append-property/ interrupt-names = "board-specific-irq";
> >
> > Both these options solve the issue at the source, are general to other
> > properties, and allow more than one level of hierarchy (the proposed
> > interrupts-extended-2 only allows one level).
>
> I've just had a go at implementing the append-property mechanism above
> in dtc, and it was far easier than I expected (patch below).
>
> Does anyone have any issues with the /append-property/ idea?
I think that is reasonable.
g.
>
> Thanks,
> Mark.
>
> ---->8----
>
> From 88be0036b6a966bd7506f58e3cb9ce9ea4c5ac48 Mon Sep 17 00:00:00 2001
> From: Mark Rutland <mark.rutland@arm.com>
> Date: Wed, 15 Jan 2014 15:43:51 +0000
> Subject: [PATCH] dtc: add ability to append properties
>
> When dealing with hierarchies of dtsi files, handling minute differences
> between individual boards can become very painful. Adding a single
> board-specific interrupt requires duplicating the entire interrupts
> property, which requires duplication of common values. This makes bug
> fixing painful and if not handled very carefully files diverge rapdily.
>
> To ameliorate this, this patch adds the ability to append properties,
> allowing board files to describe only the additional values required in
> a property. This functionality also works with strings, so parallel
> properties like interupts and interrupt-names stay in sync. Properties
> may be appended multiple times, and deleting properties clears all
> previous appended values.
>
> To append a property, secondary definitions must be prefixed with
> /append-property/. This is longer than a possible '+=' syntax, but makes
> it far easier to spot when appending behaviour is requested, and so
> hopefully will lead to fewer buggy dts files.
>
> For example, if the following dts fragements are compiled together:
>
> / {
> interrupts = <0>, <1>;
> interrupt-names = "zero", "one";
> };
>
> / {
> /append-property/ interrupts = <2>, <3>;
> /append-property/ interrupt-names = "two", "three";
> };
>
> They will result in a dtb equivalent to the following dts fragment:
>
> / {
> interrupts = <0>, <1>, <2>, <3>;
> interrupt-names = "zero", "one", "two", "three";
> };
>
> Signed-off-by: Mark Rutland <mark.rutland@arm.com>
> ---
> dtc-lexer.l | 7 +++++++
> dtc-parser.y | 5 +++++
> dtc.h | 2 ++
> livetree.c | 15 ++++++++++++++-
> 4 files changed, 28 insertions(+), 1 deletion(-)
>
> diff --git a/dtc-lexer.l b/dtc-lexer.l
> index 0cd7e67..7989abe 100644
> --- a/dtc-lexer.l
> +++ b/dtc-lexer.l
> @@ -131,6 +131,13 @@ static bool pop_input_file(void);
> return DT_DEL_PROP;
> }
>
> +<*>"/append-property/" {
> + DPRINT("Keyword: /append-property/\n");
> + DPRINT("<PROPNODENAME>\n");
> + BEGIN(PROPNODENAME);
> + return DT_APPEND_PROP;
> + }
> +
> <*>"/delete-node/" {
> DPRINT("Keyword: /delete-node/\n");
> DPRINT("<PROPNODENAME>\n");
> diff --git a/dtc-parser.y b/dtc-parser.y
> index 4864631..a8409ed 100644
> --- a/dtc-parser.y
> +++ b/dtc-parser.y
> @@ -63,6 +63,7 @@ static unsigned char eval_char_literal(const char *s);
> %token DT_LSHIFT DT_RSHIFT DT_LE DT_GE DT_EQ DT_NE DT_AND DT_OR
> %token DT_BITS
> %token DT_DEL_PROP
> +%token DT_APPEND_PROP
> %token DT_DEL_NODE
> %token <propnodename> DT_PROPNODENAME
> %token <literal> DT_LITERAL
> @@ -195,6 +196,10 @@ propdef:
> {
> $$ = build_property($1, empty_data);
> }
> + | DT_APPEND_PROP DT_PROPNODENAME '=' propdata ';'
> + {
> + $$ = build_property_append($2, $4);
> + }
> | DT_DEL_PROP DT_PROPNODENAME ';'
> {
> $$ = build_property_delete($2);
> diff --git a/dtc.h b/dtc.h
> index 20e4d56..8687530 100644
> --- a/dtc.h
> +++ b/dtc.h
> @@ -133,6 +133,7 @@ struct label {
> };
>
> struct property {
> + bool appended;
> bool deleted;
> char *name;
> struct data val;
> @@ -186,6 +187,7 @@ void delete_labels(struct label **labels);
>
> struct property *build_property(char *name, struct data val);
> struct property *build_property_delete(char *name);
> +struct property *build_property_append(char *name, struct data val);
> struct property *chain_property(struct property *first, struct property *list);
> struct property *reverse_properties(struct property *first);
>
> diff --git a/livetree.c b/livetree.c
> index b61465f..894e42b 100644
> --- a/livetree.c
> +++ b/livetree.c
> @@ -74,6 +74,15 @@ struct property *build_property_delete(char *name)
> return new;
> }
>
> +struct property *build_property_append(char *name, struct data val)
> +{
> + struct property *new = build_property(name, val);
> +
> + new->appended = 1;
> +
> + return new;
> +}
> +
> struct property *chain_property(struct property *first, struct property *list)
> {
> assert(first->next == NULL);
> @@ -167,7 +176,11 @@ struct node *merge_nodes(struct node *old_node, struct node *new_node)
> for_each_label_withdel(new_prop->labels, l)
> add_label(&old_prop->labels, l->label);
>
> - old_prop->val = new_prop->val;
> + if (new_prop->appended)
> + old_prop->val = data_merge(old_prop->val, new_prop->val);
> + else
> + old_prop->val = new_prop->val;
> +
> old_prop->deleted = 0;
> free(new_prop);
> new_prop = NULL;
> --
> 1.8.1.1
>
^ permalink raw reply [flat|nested] 16+ messages in thread
* [RFC PATCH 1/1] of/irq: create interrupts-extended-2 property
2014-01-20 22:47 ` Grant Likely
@ 2014-01-21 1:03 ` Olof Johansson
2014-01-21 8:57 ` David Gibson
` (2 more replies)
0 siblings, 3 replies; 16+ messages in thread
From: Olof Johansson @ 2014-01-21 1:03 UTC (permalink / raw)
To: linux-arm-kernel
On Mon, Jan 20, 2014 at 2:47 PM, Grant Likely <grant.likely@linaro.org> wrote:
> On Wed, 15 Jan 2014 16:12:24 +0000, Mark Rutland <mark.rutland@arm.com> wrote:
>> >
>> > Another, more invasive option would be extend the dts syntax and teach
>> > dtc to handle property appending. Then the soc dts could stay as it is,
>> > and the board dts could have something like:
>> >
>> > /append-property/ interrupts = <&intc1 6 1>;
>> > /append-property/ interrupt-names = "board-specific-irq";
>> >
>> > Both these options solve the issue at the source, are general to other
>> > properties, and allow more than one level of hierarchy (the proposed
>> > interrupts-extended-2 only allows one level).
>>
>> I've just had a go at implementing the append-property mechanism above
>> in dtc, and it was far easier than I expected (patch below).
>>
>> Does anyone have any issues with the /append-property/ idea?
>
> I think that is reasonable.
The main problem with this (same for clocks) is if you need to append
something with a name when the original didn't have any.
Reordering entries might not work for interrupts, since the bindings
might have requirements on order.
I'm not aware of a good solution for this. Suggestions welcome.
-Olof
^ permalink raw reply [flat|nested] 16+ messages in thread
* [RFC PATCH 1/1] of/irq: create interrupts-extended-2 property
2014-01-21 1:03 ` Olof Johansson
@ 2014-01-21 8:57 ` David Gibson
2014-01-21 12:02 ` Mark Rutland
2014-01-24 17:39 ` Grant Likely
2 siblings, 0 replies; 16+ messages in thread
From: David Gibson @ 2014-01-21 8:57 UTC (permalink / raw)
To: linux-arm-kernel
On Mon, Jan 20, 2014 at 05:03:23PM -0800, Olof Johansson wrote:
> On Mon, Jan 20, 2014 at 2:47 PM, Grant Likely <grant.likely@linaro.org> wrote:
> > On Wed, 15 Jan 2014 16:12:24 +0000, Mark Rutland <mark.rutland@arm.com> wrote:
> >> >
> >> > Another, more invasive option would be extend the dts syntax and teach
> >> > dtc to handle property appending. Then the soc dts could stay as it is,
> >> > and the board dts could have something like:
> >> >
> >> > /append-property/ interrupts = <&intc1 6 1>;
> >> > /append-property/ interrupt-names = "board-specific-irq";
> >> >
> >> > Both these options solve the issue at the source, are general to other
> >> > properties, and allow more than one level of hierarchy (the proposed
> >> > interrupts-extended-2 only allows one level).
> >>
> >> I've just had a go at implementing the append-property mechanism above
> >> in dtc, and it was far easier than I expected (patch below).
> >>
> >> Does anyone have any issues with the /append-property/ idea?
> >
> > I think that is reasonable.
>
>
> The main problem with this (same for clocks) is if you need to append
> something with a name when the original didn't have any.
>
> Reordering entries might not work for interrupts, since the bindings
> might have requirements on order.
>
> I'm not aware of a good solution for this. Suggestions welcome.
So, in principle my preferred way to handle things like this would be
to add richer expression support, including a token for "previous
value" when overlaying.
But although I've recently had another look at it, that's still a ways
off, so I wouldn't object to another approach as a stopgap, as long as
its not too hideous.
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140121/a8e3ce63/attachment.sig>
^ permalink raw reply [flat|nested] 16+ messages in thread
* [RFC PATCH 1/1] of/irq: create interrupts-extended-2 property
2014-01-21 1:03 ` Olof Johansson
2014-01-21 8:57 ` David Gibson
@ 2014-01-21 12:02 ` Mark Rutland
2014-01-21 12:46 ` Jean-Christophe PLAGNIOL-VILLARD
2014-01-24 17:39 ` Grant Likely
2 siblings, 1 reply; 16+ messages in thread
From: Mark Rutland @ 2014-01-21 12:02 UTC (permalink / raw)
To: linux-arm-kernel
On Tue, Jan 21, 2014 at 01:03:23AM +0000, Olof Johansson wrote:
> On Mon, Jan 20, 2014 at 2:47 PM, Grant Likely <grant.likely@linaro.org> wrote:
> > On Wed, 15 Jan 2014 16:12:24 +0000, Mark Rutland <mark.rutland@arm.com> wrote:
> >> >
> >> > Another, more invasive option would be extend the dts syntax and teach
> >> > dtc to handle property appending. Then the soc dts could stay as it is,
> >> > and the board dts could have something like:
> >> >
> >> > /append-property/ interrupts = <&intc1 6 1>;
> >> > /append-property/ interrupt-names = "board-specific-irq";
> >> >
> >> > Both these options solve the issue at the source, are general to other
> >> > properties, and allow more than one level of hierarchy (the proposed
> >> > interrupts-extended-2 only allows one level).
> >>
> >> I've just had a go at implementing the append-property mechanism above
> >> in dtc, and it was far easier than I expected (patch below).
> >>
> >> Does anyone have any issues with the /append-property/ idea?
> >
> > I think that is reasonable.
>
>
> The main problem with this (same for clocks) is if you need to append
> something with a name when the original didn't have any.
Can you not just add a name in the original file? I assume we're not
going to use this to adjust dts files we're not already in full control
of.
>
> Reordering entries might not work for interrupts, since the bindings
> might have requirements on order.
That's a fair point.
Do we currently have any optional/board-specific interrupts which must
appear at the start or middle of the list?
For those, could we add names? The kernel should be abel to fall back to
ordering if names aren't present, and we can recommend a particular
ordering for compatiblity with older kernels.
As a general preventative measure it would be nice to have named
elements whenever elements can be optional.
>
> I'm not aware of a good solution for this. Suggestions welcome.
Me neither. Prepending and appending is easy.
Inserting and/or modifying the list requires knowledge of the size of
each element (and for variable-sized entries requires knowledge of the
particular binding, which we cannot embed in dtc).
I suspect adding richer syntax for modifying properties in arbitrary
ways will devolve into a turing tarpit.
Thanks,
Mark.
^ permalink raw reply [flat|nested] 16+ messages in thread
* [RFC PATCH 1/1] of/irq: create interrupts-extended-2 property
2014-01-21 12:02 ` Mark Rutland
@ 2014-01-21 12:46 ` Jean-Christophe PLAGNIOL-VILLARD
0 siblings, 0 replies; 16+ messages in thread
From: Jean-Christophe PLAGNIOL-VILLARD @ 2014-01-21 12:46 UTC (permalink / raw)
To: linux-arm-kernel
On 12:02 Tue 21 Jan , Mark Rutland wrote:
> On Tue, Jan 21, 2014 at 01:03:23AM +0000, Olof Johansson wrote:
> > On Mon, Jan 20, 2014 at 2:47 PM, Grant Likely <grant.likely@linaro.org> wrote:
> > > On Wed, 15 Jan 2014 16:12:24 +0000, Mark Rutland <mark.rutland@arm.com> wrote:
> > >> >
> > >> > Another, more invasive option would be extend the dts syntax and teach
> > >> > dtc to handle property appending. Then the soc dts could stay as it is,
> > >> > and the board dts could have something like:
> > >> >
> > >> > /append-property/ interrupts = <&intc1 6 1>;
> > >> > /append-property/ interrupt-names = "board-specific-irq";
> > >> >
> > >> > Both these options solve the issue at the source, are general to other
> > >> > properties, and allow more than one level of hierarchy (the proposed
> > >> > interrupts-extended-2 only allows one level).
> > >>
> > >> I've just had a go at implementing the append-property mechanism above
> > >> in dtc, and it was far easier than I expected (patch below).
> > >>
> > >> Does anyone have any issues with the /append-property/ idea?
> > >
> > > I think that is reasonable.
> >
> >
> > The main problem with this (same for clocks) is if you need to append
> > something with a name when the original didn't have any.
>
> Can you not just add a name in the original file? I assume we're not
> going to use this to adjust dts files we're not already in full control
> of.
>
> >
> > Reordering entries might not work for interrupts, since the bindings
> > might have requirements on order.
>
> That's a fair point.
>
> Do we currently have any optional/board-specific interrupts which must
> appear at the start or middle of the list?
>
> For those, could we add names? The kernel should be abel to fall back to
> ordering if names aren't present, and we can recommend a particular
> ordering for compatiblity with older kernels.
>
> As a general preventative measure it would be nice to have named
> elements whenever elements can be optional.
I never was a fanof index search I do agree the names irq is the best way
>
> >
> > I'm not aware of a good solution for this. Suggestions welcome.
>
> Me neither. Prepending and appending is easy.
>
> Inserting and/or modifying the list requires knowledge of the size of
> each element (and for variable-sized entries requires knowledge of the
> particular binding, which we cannot embed in dtc).
>
> I suspect adding richer syntax for modifying properties in arbitrary
> ways will devolve into a turing tarpit.
no this need to stay simple if too much complexe => mess up, unmaintainable
if you really have complex stuff duplicate the info
Best Regards,
J.
>
> Thanks,
> Mark.
^ permalink raw reply [flat|nested] 16+ messages in thread
* [RFC PATCH 1/1] of/irq: create interrupts-extended-2 property
2014-01-21 1:03 ` Olof Johansson
2014-01-21 8:57 ` David Gibson
2014-01-21 12:02 ` Mark Rutland
@ 2014-01-24 17:39 ` Grant Likely
2 siblings, 0 replies; 16+ messages in thread
From: Grant Likely @ 2014-01-24 17:39 UTC (permalink / raw)
To: linux-arm-kernel
On Mon, 20 Jan 2014 17:03:23 -0800, Olof Johansson <olof@lixom.net> wrote:
> On Mon, Jan 20, 2014 at 2:47 PM, Grant Likely <grant.likely@linaro.org> wrote:
> > On Wed, 15 Jan 2014 16:12:24 +0000, Mark Rutland <mark.rutland@arm.com> wrote:
> >> >
> >> > Another, more invasive option would be extend the dts syntax and teach
> >> > dtc to handle property appending. Then the soc dts could stay as it is,
> >> > and the board dts could have something like:
> >> >
> >> > /append-property/ interrupts = <&intc1 6 1>;
> >> > /append-property/ interrupt-names = "board-specific-irq";
> >> >
> >> > Both these options solve the issue at the source, are general to other
> >> > properties, and allow more than one level of hierarchy (the proposed
> >> > interrupts-extended-2 only allows one level).
> >>
> >> I've just had a go at implementing the append-property mechanism above
> >> in dtc, and it was far easier than I expected (patch below).
> >>
> >> Does anyone have any issues with the /append-property/ idea?
> >
> > I think that is reasonable.
>
>
> The main problem with this (same for clocks) is if you need to append
> something with a name when the original didn't have any.
>
> Reordering entries might not work for interrupts, since the bindings
> might have requirements on order.
Hmmm, yes. to handle that case would need a higher level construct that
keeps the name and interrupt value together. Maybe something like this?
device {
add-interrupt(27);
add-interrupt(29, 5, controller=&intc1);
add-interrupt(2, 4, name="error");
};
I'm using python-like syntax here where controller and name are optional
arguments.
We could get DTC to generate the appropriate interrupt-parent,
interrupts, interrupts-extended and interrupt-names properties as
needed. Of course the shared context would be lost when the tree is
compiled, but it would be useful for building up parsing data. It would
also work for gpio, clocks and similar bindings. Kind of like an
intelligent macro.
g.
^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2014-01-24 17:39 UTC | newest]
Thread overview: 16+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-01-15 11:47 [RFC PATCH 1/1] of/irq: create interrupts-extended-2 property Jean-Christophe PLAGNIOL-VILLARD
[not found] ` < 20140115161224.GH25824@e106331-lin.cambridge.arm.com>
[not found] ` <20140120224742. EC905C40B12@trevor.secretlab.ca>
[not found] ` < 20140115121750.GE25824@e106331-lin.cambridge.arm.com>
[not found] ` <20140115124041. GD9558@ns203013.ovh.net>
2014-01-15 12:17 ` Mark Rutland
2014-01-15 12:40 ` Jean-Christophe PLAGNIOL-VILLARD
2014-01-15 13:46 ` Mark Rutland
2014-01-15 14:01 ` Jean-Christophe PLAGNIOL-VILLARD
2014-01-15 15:16 ` Rob Herring
2014-01-15 15:28 ` Rob Herring
2014-01-15 16:12 ` Mark Rutland
2014-01-15 18:08 ` Jean-Christophe PLAGNIOL-VILLARD
2014-01-20 22:47 ` Grant Likely
2014-01-21 1:03 ` Olof Johansson
2014-01-21 8:57 ` David Gibson
2014-01-21 12:02 ` Mark Rutland
2014-01-21 12:46 ` Jean-Christophe PLAGNIOL-VILLARD
2014-01-24 17:39 ` Grant Likely
2014-01-15 14:56 ` Grant Likely
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).