| Message ID | 1389786445-10598-1-git-send-email-plagnioj@jcrosoft.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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.
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.
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.
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.
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.
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
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
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);
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(-)