diff mbox

[v8,1/7] irqchip: vic: Parse interrupt and resume masks from device tree

Message ID 1500724.07LRpFtgcx@flatron (mailing list archive)
State New, archived
Headers show

Commit Message

Tomasz Figa Aug. 22, 2013, 11:22 p.m. UTC
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.

Comments

Linus Walleij Aug. 23, 2013, 6:12 p.m. UTC | #1
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
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Stephen Warren Aug. 23, 2013, 10:11 p.m. UTC | #2
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?
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Tomasz Figa Aug. 23, 2013, 11:04 p.m. UTC | #3
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

--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Stephen Warren Aug. 23, 2013, 11:19 p.m. UTC | #4
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).
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Tomasz Figa Aug. 23, 2013, 11:33 p.m. UTC | #5
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

--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Rob Herring Aug. 24, 2013, 3:25 p.m. UTC | #6
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
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Tomasz Figa Aug. 24, 2013, 3:31 p.m. UTC | #7
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

--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Rob Herring Aug. 24, 2013, 4:35 p.m. UTC | #8
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
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Tomasz Figa Aug. 24, 2013, 4:57 p.m. UTC | #9
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

--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

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;
 }