| Message ID | 1500724.07LRpFtgcx@flatron (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Aug 23, 2013 at 1:22 AM, Tomasz Figa <tomasz.figa@gmail.com> wrote: > This patch extends vic_of_init to parse valid interrupt sources > and resume sources masks from device tree. > > If mask values are not specified in device tree, all sources > are assumed to be valid, as before this patch. > > Signed-off-by: Tomasz Figa <tomasz.figa@gmail.com> > --- > Documentation/devicetree/bindings/arm/vic.txt | 6 ++++++ > drivers/irqchip/irq-vic.c | 7 ++++++- > 2 files changed, 12 insertions(+), 1 deletion(-) > > Changes since v7: > - Renamed interrupt-mask property to valid-mask, for consistency with > bindings of other interrupt controllers. Reviewed-by: Linus Walleij <linus.walleij@linaro.org> Yours, Linus Walleij
On 08/22/2013 05:22 PM, Tomasz Figa wrote: > This patch extends vic_of_init to parse valid interrupt sources > and resume sources masks from device tree. > > If mask values are not specified in device tree, all sources > are assumed to be valid, as before this patch. Can you explain further why the VIC needs this information up-front? Presumably it can accumulate it as devices request interrupts. > diff --git a/Documentation/devicetree/bindings/arm/vic.txt b/Documentation/devicetree/bindings/arm/vic.txt > +- valid-mask : Bit mask of valid interrupt sources (defaults to all valid) Until a device requests an interrupt, it can be left disabled. Once something does request it, it gets enabled. If nothing requests it, it never gets enabled. Doesn't the result of that logic end up being the same thing as valid-mask represents? The only difference would be if some device requests an invalid interrupt source, but then why not just fix the interrupt client instead of adding this property to reject the request? > +- wakeup-mask : Bit mask of interrupt sources that can wake up the system > + (defaults to all allowed) Shouldn't drivers for end-devices request interrupts, and then set wake enable on those interrupts, which would then trickle up the IRQ chain to tell the VIC which interrupts to enable wakeup on?
On Friday 23 of August 2013 16:11:18 Stephen Warren wrote: > On 08/22/2013 05:22 PM, Tomasz Figa wrote: > > This patch extends vic_of_init to parse valid interrupt sources > > and resume sources masks from device tree. > > > > If mask values are not specified in device tree, all sources > > are assumed to be valid, as before this patch. > > Can you explain further why the VIC needs this information up-front? > Presumably it can accumulate it as devices request interrupts. It does not need this information just for operation, but this makes the hardware description more detailed and allows better sanity checking of interrupts being requested. To clarify, this is a mask of valid interrupt sources of the VIC, where set bit indicates that given signal is wired and clear bit that it is not. > > diff --git a/Documentation/devicetree/bindings/arm/vic.txt > > b/Documentation/devicetree/bindings/arm/vic.txt > > > > +- valid-mask : Bit mask of valid interrupt sources (defaults to all > > valid) > Until a device requests an interrupt, it can be left disabled. Once > something does request it, it gets enabled. If nothing requests it, it > never gets enabled. Doesn't the result of that logic end up being the > same thing as valid-mask represents? The only difference would be if > some device requests an invalid interrupt source, but then why not just > fix the interrupt client instead of adding this property to reject the > request? This property does not have anything to do with enabling or disabling (aka unmasking/masking) of interrupts. It just lists valid interrupt signals of given VIC. > > +- wakeup-mask : Bit mask of interrupt sources that can wake up the > > system + (defaults to all allowed) > > Shouldn't drivers for end-devices request interrupts, and then set wake > enable on those interrupts, which would then trickle up the IRQ chain to > tell the VIC which interrupts to enable wakeup on? This is the same as previous property. It does not have anything to do with requesting particular signal to wake up the system, but rather indicating that particular signal _can_ be requested to do so. Again, this is not strictly needed for correct operation, but this way you wouldn't get any sanity check over wake up signals being requested. Best regards, Tomasz
On 08/23/2013 05:04 PM, Tomasz Figa wrote: > On Friday 23 of August 2013 16:11:18 Stephen Warren wrote: >> On 08/22/2013 05:22 PM, Tomasz Figa wrote: >>> This patch extends vic_of_init to parse valid interrupt sources >>> and resume sources masks from device tree. >>> >>> If mask values are not specified in device tree, all sources >>> are assumed to be valid, as before this patch. >> >> Can you explain further why the VIC needs this information up-front? >> Presumably it can accumulate it as devices request interrupts. > > It does not need this information just for operation, but this makes the > hardware description more detailed and allows better sanity checking of > interrupts being requested. Ah, OK. It may be worth mentioning the intent of the properties. I suppose this is purely a representation of HW then, so it's reasonable to include it in the binding. I'm not sure /quite/ how useful it is; after all the error-checking that it enables will never trigger assuming the rest of the DT is written to "request" the correct interrupts. However, I guess there is little harm in allowing these properties. Bikeshedding a bit, but perhaps rename wakeup-mask to valid-wakeup-mask (and perhaps valid-mask to valid-source-mask for consistency, although I see you already renamed that to be consistent with other bindings...)? To me, wakeup-mask sounds like a configuration of which sources should be configured to wakeup the system; something to do with configuration/policy rather than HW capabilities. Either way, the binding, Acked-by: Stephen Warren <swarren@nvidia.com> (I assume those 2 new properties always have 1 cell since the controller can only support up to 32 IRQ sources? If not, then perhaps add wording to describe how long the properties should be).
On Friday 23 of August 2013 17:19:50 Stephen Warren wrote: > On 08/23/2013 05:04 PM, Tomasz Figa wrote: > > On Friday 23 of August 2013 16:11:18 Stephen Warren wrote: > >> On 08/22/2013 05:22 PM, Tomasz Figa wrote: > >>> This patch extends vic_of_init to parse valid interrupt sources > >>> and resume sources masks from device tree. > >>> > >>> If mask values are not specified in device tree, all sources > >>> are assumed to be valid, as before this patch. > >> > >> Can you explain further why the VIC needs this information up-front? > >> Presumably it can accumulate it as devices request interrupts. > > > > It does not need this information just for operation, but this makes > > the hardware description more detailed and allows better sanity > > checking of interrupts being requested. > > Ah, OK. It may be worth mentioning the intent of the properties. Right, a bit more detailed description will be nice indeed. I didn't think such thing like this is so uncommon to need such. > I > suppose this is purely a representation of HW then, so it's reasonable > to include it in the binding. I'm not sure /quite/ how useful it is; > after all the error-checking that it enables will never trigger assuming > the rest of the DT is written to "request" the correct interrupts. > However, I guess there is little harm in allowing these properties. Well, the valid-mask is indeed a bit redundant (although might let you spot errors in interrupt specification in your device tree faster), but wakeup-mask is something that prevents drivers from incorrectly thinking that an interrupt can wake the system up, while it can't. Otherwise enable_irq_wake() wouldn't know when to return an error. > Bikeshedding a bit, but perhaps rename wakeup-mask to valid-wakeup-mask > (and perhaps valid-mask to valid-source-mask for consistency, although I > see you already renamed that to be consistent with other bindings...)? > To me, wakeup-mask sounds like a configuration of which sources should > be configured to wakeup the system; something to do with > configuration/policy rather than HW capabilities. Yes, valid-wakeup-mask sounds reasonably to me. The valid-mask property has been taken from bindings/arm/versatile-fpga-irq.txt, as suggested by Linus Walleij. > Either way, the binding, > Acked-by: Stephen Warren <swarren@nvidia.com> Thanks. > (I assume those 2 new properties always have 1 cell since the controller > can only support up to 32 IRQ sources? If not, then perhaps add wording > to describe how long the properties should be). Correct, one VIC can support up to 32 interrupt sources. A word on this in binding description will be nice indeed. Best regards, Tomasz
On Fri, Aug 23, 2013 at 6:04 PM, Tomasz Figa <tomasz.figa@gmail.com> wrote: > On Friday 23 of August 2013 16:11:18 Stephen Warren wrote: >> On 08/22/2013 05:22 PM, Tomasz Figa wrote: >> > This patch extends vic_of_init to parse valid interrupt sources >> > and resume sources masks from device tree. >> > >> > If mask values are not specified in device tree, all sources >> > are assumed to be valid, as before this patch. >> >> Can you explain further why the VIC needs this information up-front? >> Presumably it can accumulate it as devices request interrupts. > > It does not need this information just for operation, but this makes the > hardware description more detailed and allows better sanity checking of > interrupts being requested. > > To clarify, this is a mask of valid interrupt sources of the VIC, where > set bit indicates that given signal is wired and clear bit that it is not. I agree with Stephen here. The valid interrupts are the ones in the DT. The reserved ones are the ones not present. If it is not needed for the operation of the VIC, then remove it. The argument of sanity checking could apply to all interrupt controllers. Rob
On Saturday 24 of August 2013 10:25:26 Rob Herring wrote: > On Fri, Aug 23, 2013 at 6:04 PM, Tomasz Figa <tomasz.figa@gmail.com> wrote: > > On Friday 23 of August 2013 16:11:18 Stephen Warren wrote: > >> On 08/22/2013 05:22 PM, Tomasz Figa wrote: > >> > This patch extends vic_of_init to parse valid interrupt sources > >> > and resume sources masks from device tree. > >> > > >> > If mask values are not specified in device tree, all sources > >> > are assumed to be valid, as before this patch. > >> > >> Can you explain further why the VIC needs this information up-front? > >> Presumably it can accumulate it as devices request interrupts. > > > > It does not need this information just for operation, but this makes > > the hardware description more detailed and allows better sanity > > checking of interrupts being requested. > > > > To clarify, this is a mask of valid interrupt sources of the VIC, > > where > > set bit indicates that given signal is wired and clear bit that it is > > not. > I agree with Stephen here. The valid interrupts are the ones in the > DT. The reserved ones are the ones not present. If it is not needed > for the operation of the VIC, then remove it. The argument of sanity > checking could apply to all interrupt controllers. Sorry, but I don't get what's wrong in having a more detailed description than required just for operation of the hardware. The feature of sanity checks based on interrupt_mask (here now called valid-mask) has been present in the VIC driver since a long time already (if not from the beginning of existence of this driver) and before we started using DT, the mask was being passed from platform code as VIC init function argument. I'd prefer this feature to be available when using DT as well, unless we really want to move things backwards, just because we want to use DT... Best regards, Tomasz
On 08/24/2013 10:31 AM, Tomasz Figa wrote: > On Saturday 24 of August 2013 10:25:26 Rob Herring wrote: >> On Fri, Aug 23, 2013 at 6:04 PM, Tomasz Figa <tomasz.figa@gmail.com> > wrote: >>> On Friday 23 of August 2013 16:11:18 Stephen Warren wrote: >>>> On 08/22/2013 05:22 PM, Tomasz Figa wrote: >>>>> This patch extends vic_of_init to parse valid interrupt sources >>>>> and resume sources masks from device tree. >>>>> >>>>> If mask values are not specified in device tree, all sources >>>>> are assumed to be valid, as before this patch. >>>> >>>> Can you explain further why the VIC needs this information up-front? >>>> Presumably it can accumulate it as devices request interrupts. >>> >>> It does not need this information just for operation, but this makes >>> the hardware description more detailed and allows better sanity >>> checking of interrupts being requested. >>> >>> To clarify, this is a mask of valid interrupt sources of the VIC, >>> where >>> set bit indicates that given signal is wired and clear bit that it is >>> not. >> I agree with Stephen here. The valid interrupts are the ones in the >> DT. The reserved ones are the ones not present. If it is not needed >> for the operation of the VIC, then remove it. The argument of sanity >> checking could apply to all interrupt controllers. > > Sorry, but I don't get what's wrong in having a more detailed description > than required just for operation of the hardware. > > The feature of sanity checks based on interrupt_mask (here now called > valid-mask) has been present in the VIC driver since a long time already > (if not from the beginning of existence of this driver) and before we > started using DT, the mask was being passed from platform code as VIC init > function argument. So we should base the binding on the Linux software design? > I'd prefer this feature to be available when using DT as well, unless we > really want to move things backwards, just because we want to use DT... As I mentioned all these arguments apply to ALL interrupt controllers except ones which a mask does not work. So IF this makes sense, then this should be a generic property and generic code to support. You simply have the same information twice. One is distributed and one is centralized. While it adds a way to validate things it also adds a way to introduce errors. Suppose someone writes a dts such that valid-mask matches the irq lines present in that dts (simply because they were lazy or don't have documentation of all interrupt lines). Then you go add a node with a new interrupt (because the initial dts was not complete). Updating the valid-mask could very easily be forgotten. Yes, this should all be found by testing, but people don't always have access to all the h/w. This issue would also not likely be obvious in a review. Rob
On Saturday 24 of August 2013 11:35:03 Rob Herring wrote: > On 08/24/2013 10:31 AM, Tomasz Figa wrote: > > On Saturday 24 of August 2013 10:25:26 Rob Herring wrote: > >> On Fri, Aug 23, 2013 at 6:04 PM, Tomasz Figa <tomasz.figa@gmail.com> > > > > wrote: > >>> On Friday 23 of August 2013 16:11:18 Stephen Warren wrote: > >>>> On 08/22/2013 05:22 PM, Tomasz Figa wrote: > >>>>> This patch extends vic_of_init to parse valid interrupt sources > >>>>> and resume sources masks from device tree. > >>>>> > >>>>> If mask values are not specified in device tree, all sources > >>>>> are assumed to be valid, as before this patch. > >>>> > >>>> Can you explain further why the VIC needs this information > >>>> up-front? > >>>> Presumably it can accumulate it as devices request interrupts. > >>> > >>> It does not need this information just for operation, but this makes > >>> the hardware description more detailed and allows better sanity > >>> checking of interrupts being requested. > >>> > >>> To clarify, this is a mask of valid interrupt sources of the VIC, > >>> where > >>> set bit indicates that given signal is wired and clear bit that it > >>> is > >>> not. > >> > >> I agree with Stephen here. The valid interrupts are the ones in the > >> DT. The reserved ones are the ones not present. If it is not needed > >> for the operation of the VIC, then remove it. The argument of sanity > >> checking could apply to all interrupt controllers. > > > > Sorry, but I don't get what's wrong in having a more detailed > > description than required just for operation of the hardware. > > > > The feature of sanity checks based on interrupt_mask (here now called > > valid-mask) has been present in the VIC driver since a long time > > already (if not from the beginning of existence of this driver) and > > before we started using DT, the mask was being passed from platform > > code as VIC init function argument. > > So we should base the binding on the Linux software design? I would put it different way: The binding should provide information detailed enough to allow any features allowed by Linux software design. Well, anyway, I don't believe that existing, pre-ARM, bindings wasn't influenced by how things worked on PowerPC platforms and software running on them (including OpenFirmware and OS'es like Linux). > > I'd prefer this feature to be available when using DT as well, unless > > we really want to move things backwards, just because we want to use > > DT... > As I mentioned all these arguments apply to ALL interrupt controllers > except ones which a mask does not work. So IF this makes sense, then > this should be a generic property and generic code to support. Isn't a binding of particular device free to define a private property? Anyway, I wouldn't be opposed to making it generic. > You simply have the same information twice. One is distributed Assuming you have it specified correctly under client nodes. > and one > is centralized. While it adds a way to validate things it also adds a > way to introduce errors. Suppose someone writes a dts such that > valid-mask matches the irq lines present in that dts (simply because > they were lazy or don't have documentation of all interrupt lines). That's why this property is optional. If you don't know how this is set up on your hardware then you don't fill it with a random value, but just skip it instead. > Then > you go add a node with a new interrupt (because the initial dts was not > complete). Updating the valid-mask could very easily be forgotten. Yes, > this should all be found by testing, but people don't always have > access to all the h/w. Adding a node with a new interrupt usually means adding support for new hardware. This is something I wouldn't really want to see submitted untested... Anyway, since I'd like this series to be merged ASAP and this patch is not strictly necessary, especially this optional sanity check, let me just drop this property and move on with other things... Best regards, Tomasz
diff --git a/Documentation/devicetree/bindings/arm/vic.txt b/Documentation/devicetree/bindings/arm/vic.txt index 266716b..3379666 100644 --- a/Documentation/devicetree/bindings/arm/vic.txt +++ b/Documentation/devicetree/bindings/arm/vic.txt @@ -18,6 +18,9 @@ Required properties: Optional properties: - interrupts : Interrupt source for parent controllers if the VIC is nested. +- valid-mask : Bit mask of valid interrupt sources (defaults to all valid) +- wakeup-mask : Bit mask of interrupt sources that can wake up the system + (defaults to all allowed) Example: @@ -26,4 +29,7 @@ Example: interrupt-controller; #interrupt-cells = <1>; reg = <0x60000 0x1000>; + + valid-mask = <0xffffff7f>; + wakeup-mask = <0x0000ff7f>; }; diff --git a/drivers/irqchip/irq-vic.c b/drivers/irqchip/irq-vic.c index 2bbb004..6437b18 100644 --- a/drivers/irqchip/irq-vic.c +++ b/drivers/irqchip/irq-vic.c @@ -469,6 +469,8 @@ void __init vic_init(void __iomem *base, unsigned int irq_start, int __init vic_of_init(struct device_node *node, struct device_node *parent) { void __iomem *regs; + u32 interrupt_mask = ~0; + u32 wakeup_mask = ~0; if (WARN(parent, "non-root VICs are not supported")) return -EINVAL; @@ -477,10 +479,13 @@ int __init vic_of_init(struct device_node *node, struct device_node *parent) if (WARN_ON(!regs)) return -EIO; + of_property_read_u32(node, "valid-mask", &interrupt_mask); + of_property_read_u32(node, "wakeup-mask", &wakeup_mask); + /* * Passing 0 as first IRQ makes the simple domain allocate descriptors */ - __vic_init(regs, 0, ~0, ~0, node); + __vic_init(regs, 0, interrupt_mask, wakeup_mask, node); return 0; }
This patch extends vic_of_init to parse valid interrupt sources and resume sources masks from device tree. If mask values are not specified in device tree, all sources are assumed to be valid, as before this patch. Signed-off-by: Tomasz Figa <tomasz.figa@gmail.com> --- Documentation/devicetree/bindings/arm/vic.txt | 6 ++++++ drivers/irqchip/irq-vic.c | 7 ++++++- 2 files changed, 12 insertions(+), 1 deletion(-) Changes since v7: - Renamed interrupt-mask property to valid-mask, for consistency with bindings of other interrupt controllers.