| Message ID | 1393534281-30759-1-git-send-email-swarren@wwwdotorg.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Feb 27, 2014 at 01:51:19PM -0700, Stephen Warren wrote: > From: Stephen Warren <swarren@nvidia.com> > > Some boards or SoCs have an inverter between the PMIC IRQ output pin and > the IRQ controller input signal. > > The IRQ specifier in DT is meant to represent the IRQ flags at the input > to the IRQ controller. > > The Palmas HW's IRQ output has configurable polarity. Software needs to > know which polarity to choose for the IRQ output. Software may be tempted > to extract the IRQ polarity from the IRQ specifier in order to make this > choice. > > That approach works fine if the IRQ signal is routed directly from the > PMIC to the IRQ controller with no intervening logic. However, if the > signal is inverted between the two, this approach gets the wrong answer. > > Add an additional optional DT property which indicates that such an > inversion occurs. This allows DT to give complete information about the > desired IRQ output polarity to software. > > An alternative would have been to add a new non-optional DT parameter to > indicate the exact desired output polarity. However, this would have been > an incompatible change to the DT binding. > > Signed-off-by: Stephen Warren <swarren@nvidia.com> > Acked-by: Laxman Dewangan <ldewangan@nvidia.com> > Acked-by: Lee Jones <lee.jones@linaro.org> > --- > v2: Split V1's patch 1/2 into separate patches 1/3 and 2/3. > --- > Documentation/devicetree/bindings/mfd/palmas.txt | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/Documentation/devicetree/bindings/mfd/palmas.txt b/Documentation/devicetree/bindings/mfd/palmas.txt > index e5f0f8303461..76ec509d5f87 100644 > --- a/Documentation/devicetree/bindings/mfd/palmas.txt > +++ b/Documentation/devicetree/bindings/mfd/palmas.txt > @@ -18,6 +18,12 @@ Required properties: > ti,tps659038 > and also the generic series names > ti,palmas > +- interrupts : Should contain a single entry for the IRQ output. > +- ti,irq-externally-inverted : If missing, the polarity of the Palmas IRQ > + output should be set to the opposite of the polarity indicated by the IRQ > + specifier in the interrupts property. If absent, the polarity should be > + configured to match. This allows the representation of an inverter between > + the Palmas IRQ output and the interrupt parent's IRQ input. This has got to be the wrong way to do things, all this leads to is every device doing this property in its own way and having totally inconsistent properties all meaning the same thing. If there is some other hardware inverting lines then there should be a generic binding for this in DT. This is not describing the palmas hardware but some external object to the palmas. Graeme > - interrupt-controller : palmas has its own internal IRQs > - #interrupt-cells : should be set to 2 for IRQ number and flags > The first cell is the IRQ number. > -- > 1.8.1.5 > > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On 02/27/2014 02:02 PM, Graeme Gregory wrote: > On Thu, Feb 27, 2014 at 01:51:19PM -0700, Stephen Warren wrote: >> Some boards or SoCs have an inverter between the PMIC IRQ output pin and >> the IRQ controller input signal. >> >> The IRQ specifier in DT is meant to represent the IRQ flags at the input >> to the IRQ controller. >> >> The Palmas HW's IRQ output has configurable polarity. Software needs to >> know which polarity to choose for the IRQ output. Software may be tempted >> to extract the IRQ polarity from the IRQ specifier in order to make this >> choice. >> >> That approach works fine if the IRQ signal is routed directly from the >> PMIC to the IRQ controller with no intervening logic. However, if the >> signal is inverted between the two, this approach gets the wrong answer. >> >> Add an additional optional DT property which indicates that such an >> inversion occurs. This allows DT to give complete information about the >> desired IRQ output polarity to software. >> >> An alternative would have been to add a new non-optional DT parameter to >> indicate the exact desired output polarity. However, this would have been >> an incompatible change to the DT binding. >> diff --git a/Documentation/devicetree/bindings/mfd/palmas.txt b/Documentation/devicetree/bindings/mfd/palmas.txt >> +- ti,irq-externally-inverted : If missing, the polarity of the Palmas IRQ >> + output should be set to the opposite of the polarity indicated by the IRQ >> + specifier in the interrupts property. If absent, the polarity should be >> + configured to match. This allows the representation of an inverter between >> + the Palmas IRQ output and the interrupt parent's IRQ input. > > This has got to be the wrong way to do things, all this leads to is every > device doing this property in its own way and having totally inconsistent > properties all meaning the same thing. > > If there is some other hardware inverting lines then there should be > a generic binding for this in DT. This is not describing the palmas hardware > but some external object to the palmas. I'd be fine with removing the "ti," vendor prefix from the property name, and promoting it to be a cross-device standard. I'm not sure that many devices will need this though; most don't have configurable output polarity. Still, I guess that shouldn't stop us from creating standards for the cases where it is needed. If the DT reviewers can ack the concept, I'm happy to respin the patch with the more generic property name.
On Thu, Feb 27, 2014 at 02:35:42PM -0700, Stephen Warren wrote: > On 02/27/2014 02:02 PM, Graeme Gregory wrote: > >> +- ti,irq-externally-inverted : If missing, the polarity of the Palmas IRQ > >> + output should be set to the opposite of the polarity indicated by the IRQ > >> + specifier in the interrupts property. If absent, the polarity should be > >> + configured to match. This allows the representation of an inverter between > >> + the Palmas IRQ output and the interrupt parent's IRQ input. > > This has got to be the wrong way to do things, all this leads to is every > > device doing this property in its own way and having totally inconsistent > > properties all meaning the same thing. > > If there is some other hardware inverting lines then there should be > > a generic binding for this in DT. This is not describing the palmas hardware > > but some external object to the palmas. > I'd be fine with removing the "ti," vendor prefix from the property > name, and promoting it to be a cross-device standard. > I'm not sure that many devices will need this though; most don't have > configurable output polarity. Still, I guess that shouldn't stop us from > creating standards for the cases where it is needed. It's really common for PMICs and CODECs to be able to set any interrupt polarity at least. > If the DT reviewers can ack the concept, I'm happy to respin the patch > with the more generic property name. I'm not sure that renaming the property really deals with the concerns though since drivers still all need to manually add support for this, shouldn't there be an interrupt controller described in the DT which just chains on to the parent with the polarity inverted to do the impedence match?
On 02/27/2014 10:58 PM, Mark Brown wrote: > On Thu, Feb 27, 2014 at 02:35:42PM -0700, Stephen Warren wrote: >> On 02/27/2014 02:02 PM, Graeme Gregory wrote: > >>>> +- ti,irq-externally-inverted : If missing, the polarity of the Palmas IRQ >>>> + output should be set to the opposite of the polarity indicated by the IRQ >>>> + specifier in the interrupts property. If absent, the polarity should be >>>> + configured to match. This allows the representation of an inverter between >>>> + the Palmas IRQ output and the interrupt parent's IRQ input. > >>> This has got to be the wrong way to do things, all this leads to is every >>> device doing this property in its own way and having totally inconsistent >>> properties all meaning the same thing. > >>> If there is some other hardware inverting lines then there should be >>> a generic binding for this in DT. This is not describing the palmas hardware >>> but some external object to the palmas. > >> I'd be fine with removing the "ti," vendor prefix from the property >> name, and promoting it to be a cross-device standard. > >> I'm not sure that many devices will need this though; most don't have >> configurable output polarity. Still, I guess that shouldn't stop us from >> creating standards for the cases where it is needed. > > It's really common for PMICs and CODECs to be able to set any interrupt > polarity at least. > >> If the DT reviewers can ack the concept, I'm happy to respin the patch >> with the more generic property name. > > I'm not sure that renaming the property really deals with the concerns > though since drivers still all need to manually add support for this, > shouldn't there be an interrupt controller described in the DT which > just chains on to the parent with the polarity inverted to do the > impedence match? I had thought of that when first dealing with this a couple years ago, but Olof suggested that was too complicated. Certainly, adding an "inverting" interrupt controller into the path would solve the problem without inventing any new concept in DT. However, the inverter really isn't an interrupt controller in any meaningful fashion. It has no status/mask/enable/... registers at all; it's nothing more than an inverter gate, at least in my case. Hence, I'm actually not convinced that adding an extra interrupt controller into the path is correct here. Another alternative might be to add an extra IRQ bit in the IRQ specifier (and something similar would be needed for GPIO specifiers) that indicates "inversion between source and destination". This could be queried by drivers in exactly the same way as the existing polarity/type IRQ flags. We'd need to update each individual IRQ controller binding to enable that flag, since each binding defines its own definition of such flags. (although in practice since most use the same centrally suggested flags, this wouldn't be any more than just saying yes, this binding allows that new flag to be used).
On Fri, Feb 28, 2014 at 09:34:33AM -0700, Stephen Warren wrote: > On 02/27/2014 10:58 PM, Mark Brown wrote: > > I'm not sure that renaming the property really deals with the concerns > > though since drivers still all need to manually add support for this, > > shouldn't there be an interrupt controller described in the DT which > > just chains on to the parent with the polarity inverted to do the > > impedence match? > I had thought of that when first dealing with this a couple years ago, > but Olof suggested that was too complicated. It's not obvious to me that it should be especially hard but I've not thought about it too deeply. > Another alternative might be to add an extra IRQ bit in the IRQ > specifier (and something similar would be needed for GPIO specifiers) > that indicates "inversion between source and destination". This could be > queried by drivers in exactly the same way as the existing polarity/type > IRQ flags. We'd need to update each individual IRQ controller binding to > enable that flag, since each binding defines its own definition of such > flags. (although in practice since most use the same centrally suggested > flags, this wouldn't be any more than just saying yes, this binding > allows that new flag to be used). Yes, doing something on the controller side seems more obvious here.
On Fri, Feb 28, 2014 at 09:34:33AM -0700, Stephen Warren wrote: > On 02/27/2014 10:58 PM, Mark Brown wrote: > > On Thu, Feb 27, 2014 at 02:35:42PM -0700, Stephen Warren wrote: > >> On 02/27/2014 02:02 PM, Graeme Gregory wrote: > > > >>>> +- ti,irq-externally-inverted : If missing, the polarity of the Palmas IRQ > >>>> + output should be set to the opposite of the polarity indicated by the IRQ > >>>> + specifier in the interrupts property. If absent, the polarity should be > >>>> + configured to match. This allows the representation of an inverter between > >>>> + the Palmas IRQ output and the interrupt parent's IRQ input. > > > >>> This has got to be the wrong way to do things, all this leads to is every > >>> device doing this property in its own way and having totally inconsistent > >>> properties all meaning the same thing. > > > >>> If there is some other hardware inverting lines then there should be > >>> a generic binding for this in DT. This is not describing the palmas hardware > >>> but some external object to the palmas. > > > >> I'd be fine with removing the "ti," vendor prefix from the property > >> name, and promoting it to be a cross-device standard. > > > >> I'm not sure that many devices will need this though; most don't have > >> configurable output polarity. Still, I guess that shouldn't stop us from > >> creating standards for the cases where it is needed. > > > > It's really common for PMICs and CODECs to be able to set any interrupt > > polarity at least. > > > >> If the DT reviewers can ack the concept, I'm happy to respin the patch > >> with the more generic property name. > > > > I'm not sure that renaming the property really deals with the concerns > > though since drivers still all need to manually add support for this, > > shouldn't there be an interrupt controller described in the DT which > > just chains on to the parent with the polarity inverted to do the > > impedence match? > > I had thought of that when first dealing with this a couple years ago, > but Olof suggested that was too complicated. > > Certainly, adding an "inverting" interrupt controller into the path > would solve the problem without inventing any new concept in DT. > However, the inverter really isn't an interrupt controller in any > meaningful fashion. It has no status/mask/enable/... registers at all; > it's nothing more than an inverter gate, at least in my case. Hence, I'm > actually not convinced that adding an extra interrupt controller into > the path is correct here. > A interupt controller inline that does the inversion was my first thought but I think that is probably overkill unless there is a whole range of different interupt filtering operations. > Another alternative might be to add an extra IRQ bit in the IRQ > specifier (and something similar would be needed for GPIO specifiers) > that indicates "inversion between source and destination". This could be > queried by drivers in exactly the same way as the existing polarity/type > IRQ flags. We'd need to update each individual IRQ controller binding to > enable that flag, since each binding defines its own definition of such > flags. (although in practice since most use the same centrally suggested > flags, this wouldn't be any more than just saying yes, this binding > allows that new flag to be used). > A new flag would meet my concerns of every chip doing basic inversion in different ways. Graeme
On 03/02/2014 05:52 PM, Graeme Gregory wrote: > On Fri, Feb 28, 2014 at 09:34:33AM -0700, Stephen Warren wrote: >> On 02/27/2014 10:58 PM, Mark Brown wrote: >>> On Thu, Feb 27, 2014 at 02:35:42PM -0700, Stephen Warren wrote: >>>> On 02/27/2014 02:02 PM, Graeme Gregory wrote: >>> >>>>>> +- ti,irq-externally-inverted : If missing, the polarity of the Palmas IRQ >>>>>> + output should be set to the opposite of the polarity indicated by the IRQ >>>>>> + specifier in the interrupts property. If absent, the polarity should be >>>>>> + configured to match. This allows the representation of an inverter between >>>>>> + the Palmas IRQ output and the interrupt parent's IRQ input. >>> >>>>> This has got to be the wrong way to do things, all this leads to is every >>>>> device doing this property in its own way and having totally inconsistent >>>>> properties all meaning the same thing. ... >> Another alternative might be to add an extra IRQ bit in the IRQ >> specifier (and something similar would be needed for GPIO specifiers) >> that indicates "inversion between source and destination". This could be >> queried by drivers in exactly the same way as the existing polarity/type >> IRQ flags. We'd need to update each individual IRQ controller binding to >> enable that flag, since each binding defines its own definition of such >> flags. (although in practice since most use the same centrally suggested >> flags, this wouldn't be any more than just saying yes, this binding >> allows that new flag to be used). >> > A new flag would meet my concerns of every chip doing basic inversion > in different ways. OK, I'll look into a new flag. The one thing I worry about is that we can either: 1) Have every IC driver with a configurable output polarity read a DT flag (and we can fully standardize it's name), which is 1 line of code per IC driver. or: 2) We can go through every single interrupt controller's DT binding and driver, and implement (document and parse) the new IRQ specifier flag there, and pass it throgh the Linux IRQ stack, yet we still need code in every IC driver to actually read the flag, so we haven't removed any code. It seems /much/ simpler, and no more of a maintenance or consistency burden, to just have the IC driver read the flag directly from their own DT. Still, as I said, I'll go try and code it up and see how bad it is in practice.
On Mon, Mar 03, 2014 at 09:41:36AM -0700, Stephen Warren wrote: > 2) We can go through every single interrupt controller's DT binding and > driver, and implement (document and parse) the new IRQ specifier flag > there, and pass it throgh the Linux IRQ stack, yet we still need code in > every IC driver to actually read the flag, so we haven't removed any > code. It seems /much/ simpler, and no more of a maintenance or > consistency burden, to just have the IC driver read the flag directly > from their own DT. Why does this have to be handled by the chip drivers? Can't the interrupt controller configured in a way that the chip drivers never need to see it at all, for example by adding a "override the mode for this input" flag inside the device?
diff --git a/Documentation/devicetree/bindings/mfd/palmas.txt b/Documentation/devicetree/bindings/mfd/palmas.txt index e5f0f8303461..76ec509d5f87 100644 --- a/Documentation/devicetree/bindings/mfd/palmas.txt +++ b/Documentation/devicetree/bindings/mfd/palmas.txt @@ -18,6 +18,12 @@ Required properties: ti,tps659038 and also the generic series names ti,palmas +- interrupts : Should contain a single entry for the IRQ output. +- ti,irq-externally-inverted : If missing, the polarity of the Palmas IRQ + output should be set to the opposite of the polarity indicated by the IRQ + specifier in the interrupts property. If absent, the polarity should be + configured to match. This allows the representation of an inverter between + the Palmas IRQ output and the interrupt parent's IRQ input. - interrupt-controller : palmas has its own internal IRQs - #interrupt-cells : should be set to 2 for IRQ number and flags The first cell is the IRQ number.