| Message ID | 1365148088-11175-1-git-send-email-javier.martinez@collabora.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 04/05/2013 02:48 AM, Javier Martinez Canillas wrote: > According to Documentation/devicetree/bindings/interrupt-controller/interrupts.txt > the "#interrupt-cells" property of an "interrupt-controller" is used > to define the number of cells needed to specify a single interrupt. > > A commonly used variant is two cell on which #interrupt-cells = <2> > and the first cell defines the index of the interrupt in the controller > and the second cell is used to specify any of the following flags: > > - bits[3:0] trigger type and level flags > 1 = low-to-high edge triggered > 2 = high-to-low edge triggered > 4 = active high level-sensitive > 8 = active low level-sensitive > > An example of an interrupt controller which use the two cell format is > the OMAP GPIO controller that allows GPIO lines to be used as IRQ > (Documentation/devicetree/bindings/gpio/gpio-omap.txt) > > But setting #interrupt-cells = <2> on the OMAP GPIO device node and > specifying the GPIO-IRQ type and level flags on the second cell does not > store this value on the populated IORESOURCE_IRQ struct resource. > > This is because when using an IRQ from an interrupt controller and > setting both cells (e.g:) > > interrupt-parent = <&gpio6>; > interrupts = <16 8>; > > A call to of_irq_to_resource() is made and this function calls to > irq_of_parse_and_map_type() to get the virtual IRQ mapped to the real > index for this interrupt controller. This IRQ number is populated on > the struct resource: > > int of_irq_to_resource(struct device_node *dev, int index, struct resource *r) > { > int irq = irq_of_parse_and_map(dev, index); > .. > r->start = r->end = irq; > } > > irq_of_parse_and_map() calls to irq_create_of_mapping() which calls to > the correct xlate function handler according to "#interrupt-cells" > (irq_domain_xlate_onecell or irq_domain_xlate_twocell) and to > irq_set_irq_type() to set the IRQ type. > > But the type is never returned so it can't be saved on the IRQ struct > resource flags member. > > This means that drivers that need the IRQ type/level flags defined in > the DT won't be able to get it. But the interrupt controllers that need the information should be able to get to it via irqd_get_trigger_type. What problem exactly are you trying to fix? What driver would use this? My understanding of the IORESOURCE_IRQ_xxx (and DMA) bits are they are ISA specific and therefore should not be used on non-ISA buses. Rob
On 04/08/2013 04:05 PM, Rob Herring wrote: > On 04/05/2013 02:48 AM, Javier Martinez Canillas wrote: >> According to Documentation/devicetree/bindings/interrupt-controller/interrupts.txt >> the "#interrupt-cells" property of an "interrupt-controller" is used >> to define the number of cells needed to specify a single interrupt. ... >> But the type is never returned so it can't be saved on the IRQ struct >> resource flags member. >> >> This means that drivers that need the IRQ type/level flags defined in >> the DT won't be able to get it. > > But the interrupt controllers that need the information should be able > to get to it via irqd_get_trigger_type. What problem exactly are you > trying to fix? What driver would use this? FYI, that is indeed what I did in sound/soc/codecs/wm8903.c. Thinking back, I'm not sure if that was the right thing or whether I should have sent this same patch:-)
On 04/09/2013 12:05 AM, Rob Herring wrote: > On 04/05/2013 02:48 AM, Javier Martinez Canillas wrote: >> According to Documentation/devicetree/bindings/interrupt-controller/interrupts.txt >> the "#interrupt-cells" property of an "interrupt-controller" is used >> to define the number of cells needed to specify a single interrupt. >> >> A commonly used variant is two cell on which #interrupt-cells = <2> >> and the first cell defines the index of the interrupt in the controller >> and the second cell is used to specify any of the following flags: >> >> - bits[3:0] trigger type and level flags >> 1 = low-to-high edge triggered >> 2 = high-to-low edge triggered >> 4 = active high level-sensitive >> 8 = active low level-sensitive >> >> An example of an interrupt controller which use the two cell format is >> the OMAP GPIO controller that allows GPIO lines to be used as IRQ >> (Documentation/devicetree/bindings/gpio/gpio-omap.txt) >> >> But setting #interrupt-cells = <2> on the OMAP GPIO device node and >> specifying the GPIO-IRQ type and level flags on the second cell does not >> store this value on the populated IORESOURCE_IRQ struct resource. >> >> This is because when using an IRQ from an interrupt controller and >> setting both cells (e.g:) >> >> interrupt-parent = <&gpio6>; >> interrupts = <16 8>; >> >> A call to of_irq_to_resource() is made and this function calls to >> irq_of_parse_and_map_type() to get the virtual IRQ mapped to the real >> index for this interrupt controller. This IRQ number is populated on >> the struct resource: >> >> int of_irq_to_resource(struct device_node *dev, int index, struct resource *r) >> { >> int irq = irq_of_parse_and_map(dev, index); >> .. >> r->start = r->end = irq; >> } >> >> irq_of_parse_and_map() calls to irq_create_of_mapping() which calls to >> the correct xlate function handler according to "#interrupt-cells" >> (irq_domain_xlate_onecell or irq_domain_xlate_twocell) and to >> irq_set_irq_type() to set the IRQ type. >> >> But the type is never returned so it can't be saved on the IRQ struct >> resource flags member. >> >> This means that drivers that need the IRQ type/level flags defined in >> the DT won't be able to get it. > > But the interrupt controllers that need the information should be able > to get to it via irqd_get_trigger_type. What problem exactly are you > trying to fix? What driver would use this? > Yes but this is not about the interrupt controller wanting this information but a device driver that is using the IORESOURCE_IRQ struct resource that has the information about the virtual IRQ associated with a GPIO-IRQ. The driver doesn't know neither care if its IRQ line is connected to a line of an real IRQ controller or to a GPIO controller that allows a GPIO line to be used as an IRQ. > My understanding of the IORESOURCE_IRQ_xxx (and DMA) bits are they are > ISA specific and therefore should not be used on non-ISA buses. > Many TI OMAP2+ SoC based boards have an SMSC LAN911x/912x controller (drivers/net/ethernet/smsc/smsc911x.c) that is connected to the OMAP processor through its General-Purpose Memory Controller (GPMC) and this LAN driver obtain its IRQ and I/O address space from a struct resource IORESOURCE_IRQ and IORESOURCE_MEM respectively, that is filled by the DeviceTree core. It does this: irq_res = platform_get_resource(pdev, IORESOURCE_IRQ, 0); irq_flags = irq_res->flags & IRQF_TRIGGER_MASK; Since of_irq_to_resource() doesn't fill the trigger/level flags on the IORESOURCE_IRQ struct resource, irq_flags will always be 0 regarding the value specified on the second cell of the "interrupts" DT property. A previous discussion about this can be found here [1]. > Rob > > Thanks a lot and best regards, Javier [1]: https://patchwork.kernel.org/patch/2194911/
On 04/09/2013 12:16 AM, Stephen Warren wrote: > On 04/08/2013 04:05 PM, Rob Herring wrote: >> On 04/05/2013 02:48 AM, Javier Martinez Canillas wrote: >>> According to Documentation/devicetree/bindings/interrupt-controller/interrupts.txt >>> the "#interrupt-cells" property of an "interrupt-controller" is used >>> to define the number of cells needed to specify a single interrupt. > ... >>> But the type is never returned so it can't be saved on the IRQ struct >>> resource flags member. >>> >>> This means that drivers that need the IRQ type/level flags defined in >>> the DT won't be able to get it. >> >> But the interrupt controllers that need the information should be able >> to get to it via irqd_get_trigger_type. What problem exactly are you >> trying to fix? What driver would use this? > > FYI, that is indeed what I did in sound/soc/codecs/wm8903.c. Thinking > back, I'm not sure if that was the right thing or whether I should have > sent this same patch:-) > Hi Stephen, I'm glad you agree :-) I could change drivers/net/ethernet/smsc/smsc911x.c to get the type flags for the IRQ with irqd_get_trigger_type() but I prefer $subject because: a) This works in the non-DT case with board files and filling the resources from platform data in arch/arm/mach-omap2/gpmc-smsc911x.c. So this is definitely a bug on the DT core. b) I don't see why of_irq_to_resource() should discard the type/level flags when filling the IORESOURCE_IRQ if it was specified on the DT. c) We will have to change all drivers that expect to get the IRQ type flags from a IORESOURCE_IRQ struct resource. Thanks a lot and best regards, Javier
On 04/08/2013 05:56 PM, Javier Martinez Canillas wrote: > On 04/09/2013 12:16 AM, Stephen Warren wrote: >> On 04/08/2013 04:05 PM, Rob Herring wrote: >>> On 04/05/2013 02:48 AM, Javier Martinez Canillas wrote: >>>> According to Documentation/devicetree/bindings/interrupt-controller/interrupts.txt >>>> the "#interrupt-cells" property of an "interrupt-controller" is used >>>> to define the number of cells needed to specify a single interrupt. >> ... >>>> But the type is never returned so it can't be saved on the IRQ struct >>>> resource flags member. >>>> >>>> This means that drivers that need the IRQ type/level flags defined in >>>> the DT won't be able to get it. >>> >>> But the interrupt controllers that need the information should be able >>> to get to it via irqd_get_trigger_type. What problem exactly are you >>> trying to fix? What driver would use this? >> >> FYI, that is indeed what I did in sound/soc/codecs/wm8903.c. Thinking >> back, I'm not sure if that was the right thing or whether I should have >> sent this same patch:-) >> > > Hi Stephen, > > I'm glad you agree :-) > > I could change drivers/net/ethernet/smsc/smsc911x.c to get the type flags for > the IRQ with irqd_get_trigger_type() but I prefer $subject because: irqd_get_trigger_type probably is not meant for outside of irqchips. Creating an irq_get_irq_type function which takes an irq number would be the right function as that does not expose struct irq_data. > a) This works in the non-DT case with board files and filling the resources from > platform data in arch/arm/mach-omap2/gpmc-smsc911x.c. So this is definitely a > bug on the DT core. And hackery/abuse like this: arch/arm/mach-omap2/board-3630sdp.c:32: .flags = GPMC_MUX_ADD_DATA | IORESOURCE_IRQ_LOWLEVEL, > b) I don't see why of_irq_to_resource() should discard the type/level flags when > filling the IORESOURCE_IRQ if it was specified on the DT. > > c) We will have to change all drivers that expect to get the IRQ type flags from > a IORESOURCE_IRQ struct resource. I'm not convinced that is a high number of drivers. Nearly all the occurrences of IORESOURCE_IRQ_ in drivers/ are for ISA (acpi/pnp) and drivers for ISA devices. Rob
On Tue, Apr 9, 2013 at 4:45 AM, Rob Herring <robherring2@gmail.com> wrote: > On 04/08/2013 05:56 PM, Javier Martinez Canillas wrote: >> On 04/09/2013 12:16 AM, Stephen Warren wrote: >>> On 04/08/2013 04:05 PM, Rob Herring wrote: >>>> On 04/05/2013 02:48 AM, Javier Martinez Canillas wrote: >>>>> According to Documentation/devicetree/bindings/interrupt-controller/interrupts.txt >>>>> the "#interrupt-cells" property of an "interrupt-controller" is used >>>>> to define the number of cells needed to specify a single interrupt. >>> ... >>>>> But the type is never returned so it can't be saved on the IRQ struct >>>>> resource flags member. >>>>> >>>>> This means that drivers that need the IRQ type/level flags defined in >>>>> the DT won't be able to get it. >>>> >>>> But the interrupt controllers that need the information should be able >>>> to get to it via irqd_get_trigger_type. What problem exactly are you >>>> trying to fix? What driver would use this? >>> >>> FYI, that is indeed what I did in sound/soc/codecs/wm8903.c. Thinking >>> back, I'm not sure if that was the right thing or whether I should have >>> sent this same patch:-) >>> >> >> Hi Stephen, >> >> I'm glad you agree :-) >> >> I could change drivers/net/ethernet/smsc/smsc911x.c to get the type flags for >> the IRQ with irqd_get_trigger_type() but I prefer $subject because: > > irqd_get_trigger_type probably is not meant for outside of irqchips. > Creating an irq_get_irq_type function which takes an irq number would be > the right function as that does not expose struct irq_data. > Ok, I can add an irqd_get_trigger_type() that just return the flags to the caller without exposing the struct irq_data and using it on the SMSC 911x driver instead using struct resource *irq_res->flags I hope networking folks understand why this change is needed in this driver to get the type/level flags for an IRQ defined on a DT... >> a) This works in the non-DT case with board files and filling the resources from >> platform data in arch/arm/mach-omap2/gpmc-smsc911x.c. So this is definitely a >> bug on the DT core. > > And hackery/abuse like this: > > arch/arm/mach-omap2/board-3630sdp.c:32: .flags = > GPMC_MUX_ADD_DATA | IORESOURCE_IRQ_LOWLEVEL, > >> b) I don't see why of_irq_to_resource() should discard the type/level flags when >> filling the IORESOURCE_IRQ if it was specified on the DT. >> >> c) We will have to change all drivers that expect to get the IRQ type flags from >> a IORESOURCE_IRQ struct resource. > > I'm not convinced that is a high number of drivers. Nearly all the > occurrences of IORESOURCE_IRQ_ in drivers/ are for ISA (acpi/pnp) and > drivers for ISA devices. > If IORESOURCE_IRQ is just supposed to be used for ISA devices drivers that use ACPI/PnP instead DT, then of_irq_to_resource() callers should just use irq_of_parse_and_map() that returns the virtual IRQ number for an index within a controller instead of a struct resource. In fact I wonder what's the point to have an of_irq_to_resource() function at all if IORESOURCE_IRQ is not supposed to be used for devices connected through dumb buses that need a DT and the struct resource will only hold the mapped virtual IRQ number and no the IRQ flags. > Rob > Best regards, Javier
On Tue, Apr 9, 2013 at 10:26 AM, Javier Martinez Canillas <javier@dowhile0.org> wrote: > On Tue, Apr 9, 2013 at 4:45 AM, Rob Herring <robherring2@gmail.com> wrote: >> On 04/08/2013 05:56 PM, Javier Martinez Canillas wrote: >>> On 04/09/2013 12:16 AM, Stephen Warren wrote: >>>> On 04/08/2013 04:05 PM, Rob Herring wrote: >>>>> On 04/05/2013 02:48 AM, Javier Martinez Canillas wrote: >>>>>> According to Documentation/devicetree/bindings/interrupt-controller/interrupts.txt >>>>>> the "#interrupt-cells" property of an "interrupt-controller" is used >>>>>> to define the number of cells needed to specify a single interrupt. >>>> ... >>>>>> But the type is never returned so it can't be saved on the IRQ struct >>>>>> resource flags member. >>>>>> >>>>>> This means that drivers that need the IRQ type/level flags defined in >>>>>> the DT won't be able to get it. >>>>> >>>>> But the interrupt controllers that need the information should be able >>>>> to get to it via irqd_get_trigger_type. What problem exactly are you >>>>> trying to fix? What driver would use this? >>>> >>>> FYI, that is indeed what I did in sound/soc/codecs/wm8903.c. Thinking >>>> back, I'm not sure if that was the right thing or whether I should have >>>> sent this same patch:-) >>>> >>> >>> Hi Stephen, >>> >>> I'm glad you agree :-) >>> >>> I could change drivers/net/ethernet/smsc/smsc911x.c to get the type flags for >>> the IRQ with irqd_get_trigger_type() but I prefer $subject because: >> >> irqd_get_trigger_type probably is not meant for outside of irqchips. >> Creating an irq_get_irq_type function which takes an irq number would be >> the right function as that does not expose struct irq_data. >> > > Ok, I can add an irqd_get_trigger_type() that just return the flags to I meant irq_get_irq_type() of course. Best regards, Javier
On Tue, Apr 9, 2013 at 4:45 AM, Rob Herring <robherring2@gmail.com> wrote: > On 04/08/2013 05:56 PM, Javier Martinez Canillas wrote: >> On 04/09/2013 12:16 AM, Stephen Warren wrote: >>> On 04/08/2013 04:05 PM, Rob Herring wrote: >>>> On 04/05/2013 02:48 AM, Javier Martinez Canillas wrote: >>>>> According to Documentation/devicetree/bindings/interrupt-controller/interrupts.txt >>>>> the "#interrupt-cells" property of an "interrupt-controller" is used >>>>> to define the number of cells needed to specify a single interrupt. >>> ... >>>>> But the type is never returned so it can't be saved on the IRQ struct >>>>> resource flags member. >>>>> >>>>> This means that drivers that need the IRQ type/level flags defined in >>>>> the DT won't be able to get it. >>>> >>>> But the interrupt controllers that need the information should be able >>>> to get to it via irqd_get_trigger_type. What problem exactly are you >>>> trying to fix? What driver would use this? >>> >>> FYI, that is indeed what I did in sound/soc/codecs/wm8903.c. Thinking >>> back, I'm not sure if that was the right thing or whether I should have >>> sent this same patch:-) >>> >> >> Hi Stephen, >> >> I'm glad you agree :-) >> >> I could change drivers/net/ethernet/smsc/smsc911x.c to get the type flags for >> the IRQ with irqd_get_trigger_type() but I prefer $subject because: > > irqd_get_trigger_type probably is not meant for outside of irqchips. > Creating an irq_get_irq_type function which takes an irq number would be > the right function as that does not expose struct irq_data. > Hi Rob, I sent a patch-set a few days ago that adds an irq_get_irq_type() function [1] as you suggested and this function is used on the smsc911x driver probe function to get the edge/level flags [2]. It would be great if I can get your feedback on this. Thanks a lot and best regards, Javier [1]: http://permalink.gmane.org/gmane.linux.ports.arm.omap/97117 [2]: http://permalink.gmane.org/gmane.linux.network/265587
On Tue, 09 Apr 2013 00:44:05 +0200, Javier Martinez Canillas <javier.martinez@collabora.co.uk> wrote: > On 04/09/2013 12:05 AM, Rob Herring wrote: > > On 04/05/2013 02:48 AM, Javier Martinez Canillas wrote: > >> This means that drivers that need the IRQ type/level flags defined in > >> the DT won't be able to get it. > > > > But the interrupt controllers that need the information should be able > > to get to it via irqd_get_trigger_type. What problem exactly are you > > trying to fix? What driver would use this? > > > > Yes but this is not about the interrupt controller wanting this information but > a device driver that is using the IORESOURCE_IRQ struct resource that has the > information about the virtual IRQ associated with a GPIO-IRQ. > > The driver doesn't know neither care if its IRQ line is connected to a line of > an real IRQ controller or to a GPIO controller that allows a GPIO line to be > used as an IRQ. > > > My understanding of the IORESOURCE_IRQ_xxx (and DMA) bits are they are > > ISA specific and therefore should not be used on non-ISA buses. > > > > Many TI OMAP2+ SoC based boards have an SMSC LAN911x/912x controller > (drivers/net/ethernet/smsc/smsc911x.c) that is connected to the OMAP processor > through its General-Purpose Memory Controller (GPMC) and this LAN driver obtain > its IRQ and I/O address space from a struct resource IORESOURCE_IRQ and > IORESOURCE_MEM respectively, that is filled by the DeviceTree core. > > It does this: > > irq_res = platform_get_resource(pdev, IORESOURCE_IRQ, 0); > irq_flags = irq_res->flags & IRQF_TRIGGER_MASK; > > Since of_irq_to_resource() doesn't fill the trigger/level flags on the > IORESOURCE_IRQ struct resource, irq_flags will always be 0 regarding the value > specified on the second cell of the "interrupts" DT property. > > A previous discussion about this can be found here [1]. I can't remember if there was ever a reason for not returning the IRQ flags, but I don't have any major objection to doing so if drivers find them useful. The one concern I do have however is if it will cause any problems with drivers that expect flags == IORESOURCE_IRQ without any additional flags. Any users doing that are buggy anyway, but I do want to be careful about breakage. I'll go over your patch and reply with comments. g.
On Fri, 5 Apr 2013 09:48:08 +0200, Javier Martinez Canillas <javier.martinez@collabora.co.uk> wrote: [...] > irq_of_parse_and_map() calls to irq_create_of_mapping() which calls to > the correct xlate function handler according to "#interrupt-cells" > (irq_domain_xlate_onecell or irq_domain_xlate_twocell) and to > irq_set_irq_type() to set the IRQ type. > > But the type is never returned so it can't be saved on the IRQ struct > resource flags member. > > This means that drivers that need the IRQ type/level flags defined in > the DT won't be able to get it. > > Signed-off-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk> [...] > diff --git a/include/linux/of_irq.h b/include/linux/of_irq.h > index 535cecf..98aec57 100644 > --- a/include/linux/of_irq.h > +++ b/include/linux/of_irq.h > @@ -66,6 +66,10 @@ extern int of_irq_map_one(struct device_node *device, int index, > extern unsigned int irq_create_of_mapping(struct device_node *controller, > const u32 *intspec, > unsigned int intsize); > +extern unsigned int irq_create_of_mapping_type(struct device_node *controller, > + const u32 *intspec, > + unsigned int intsize, > + unsigned int *otype); I count 11 users of irq_create_of_mapping(). That's a managable number to update. Instead of creating a new function, please modify the existing one and split it off into a separate patch. Otherwise the patch looks fine to me. g.
On 06/06/2013 01:26 AM, Grant Likely wrote: > On Tue, 09 Apr 2013 00:44:05 +0200, Javier Martinez Canillas <javier.martinez@collabora.co.uk> wrote: >> On 04/09/2013 12:05 AM, Rob Herring wrote: >> > On 04/05/2013 02:48 AM, Javier Martinez Canillas wrote: >> >> This means that drivers that need the IRQ type/level flags defined in >> >> the DT won't be able to get it. >> > >> > But the interrupt controllers that need the information should be able >> > to get to it via irqd_get_trigger_type. What problem exactly are you >> > trying to fix? What driver would use this? >> > >> >> Yes but this is not about the interrupt controller wanting this information but >> a device driver that is using the IORESOURCE_IRQ struct resource that has the >> information about the virtual IRQ associated with a GPIO-IRQ. >> >> The driver doesn't know neither care if its IRQ line is connected to a line of >> an real IRQ controller or to a GPIO controller that allows a GPIO line to be >> used as an IRQ. >> >> > My understanding of the IORESOURCE_IRQ_xxx (and DMA) bits are they are >> > ISA specific and therefore should not be used on non-ISA buses. >> > >> >> Many TI OMAP2+ SoC based boards have an SMSC LAN911x/912x controller >> (drivers/net/ethernet/smsc/smsc911x.c) that is connected to the OMAP processor >> through its General-Purpose Memory Controller (GPMC) and this LAN driver obtain >> its IRQ and I/O address space from a struct resource IORESOURCE_IRQ and >> IORESOURCE_MEM respectively, that is filled by the DeviceTree core. >> >> It does this: >> >> irq_res = platform_get_resource(pdev, IORESOURCE_IRQ, 0); >> irq_flags = irq_res->flags & IRQF_TRIGGER_MASK; >> >> Since of_irq_to_resource() doesn't fill the trigger/level flags on the >> IORESOURCE_IRQ struct resource, irq_flags will always be 0 regarding the value >> specified on the second cell of the "interrupts" DT property. >> >> A previous discussion about this can be found here [1]. > > I can't remember if there was ever a reason for not returning the IRQ > flags, but I don't have any major objection to doing so if drivers find > them useful. The one concern I do have however is if it will cause any > problems with drivers that expect flags == IORESOURCE_IRQ without any > additional flags. Any users doing that are buggy anyway, but I do want > to be careful about breakage. > > I'll go over your patch and reply with comments. > > g. > It turns out that if you don't pass a trigger type to the request_irq() call, it will use the trigger type set by the irq chip earlier with .xlate For some reasons it was not getting the right trigger type when I sent this patch but now it is working correctly so probably there was a bug on the irq_chip driver I was using (drivers/gpio/gpio-omap.c) and got fixed in the meantime. So I don't need this patch anymore to make the LAN chip work on my board and I don't know if the fact that an empty edge/level flags defaults to what was set using the .xlate function handler and this function is called in irq_create_of_mapping() is a reason enough to not return the IRQ flags on the struct resource. Having said that, if you still think this patch is useful then I can send a v2 that take into account your comments on the patch. Best regards, Javier
On 06/06/2013 01:34 AM, Grant Likely wrote: > On Fri, 5 Apr 2013 09:48:08 +0200, Javier Martinez Canillas <javier.martinez@collabora.co.uk> wrote: > [...] >> irq_of_parse_and_map() calls to irq_create_of_mapping() which calls to >> the correct xlate function handler according to "#interrupt-cells" >> (irq_domain_xlate_onecell or irq_domain_xlate_twocell) and to >> irq_set_irq_type() to set the IRQ type. >> >> But the type is never returned so it can't be saved on the IRQ struct >> resource flags member. >> >> This means that drivers that need the IRQ type/level flags defined in >> the DT won't be able to get it. >> >> Signed-off-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk> > [...] >> diff --git a/include/linux/of_irq.h b/include/linux/of_irq.h >> index 535cecf..98aec57 100644 >> --- a/include/linux/of_irq.h >> +++ b/include/linux/of_irq.h >> @@ -66,6 +66,10 @@ extern int of_irq_map_one(struct device_node *device, int index, >> extern unsigned int irq_create_of_mapping(struct device_node *controller, >> const u32 *intspec, >> unsigned int intsize); >> +extern unsigned int irq_create_of_mapping_type(struct device_node *controller, >> + const u32 *intspec, >> + unsigned int intsize, >> + unsigned int *otype); Hi Grant, thanks a lot for your feedback. > > I count 11 users of irq_create_of_mapping(). That's a managable number > to update. Yes, but since of_irq_to_resource() doesn't call irq_create_of_mapping() directly but it does though irq_of_parse_and_map(), then this function signature has to be modified too. I'm counting 223 users of irq_of_parse_and_map() so the change is not that small anymore. Another approach is to call of_irq_map_one() and irq_create_of_mapping() directly from of_irq_to_resource() instead of using irq_of_parse_and_map() but I don't like that... > Instead of creating a new function, please modify the > existing one and split it off into a separate patch. But that won't break git bisect-ability? or do you mean to first just change the functions signatures by adding an argument and update its users and then in a separate patch do the actual change to the functions to store the IRQ? > Otherwise the patch looks fine to me. > > g. > Best regards, Javier
On 00:26 Thu 06 Jun , Grant Likely wrote: > On Tue, 09 Apr 2013 00:44:05 +0200, Javier Martinez Canillas <javier.martinez@collabora.co.uk> wrote: > > On 04/09/2013 12:05 AM, Rob Herring wrote: > > > On 04/05/2013 02:48 AM, Javier Martinez Canillas wrote: > > >> This means that drivers that need the IRQ type/level flags defined in > > >> the DT won't be able to get it. > > > > > > But the interrupt controllers that need the information should be able > > > to get to it via irqd_get_trigger_type. What problem exactly are you > > > trying to fix? What driver would use this? > > > > > > > Yes but this is not about the interrupt controller wanting this information but > > a device driver that is using the IORESOURCE_IRQ struct resource that has the > > information about the virtual IRQ associated with a GPIO-IRQ. > > > > The driver doesn't know neither care if its IRQ line is connected to a line of > > an real IRQ controller or to a GPIO controller that allows a GPIO line to be > > used as an IRQ. > > > > > My understanding of the IORESOURCE_IRQ_xxx (and DMA) bits are they are > > > ISA specific and therefore should not be used on non-ISA buses. > > > > > > > Many TI OMAP2+ SoC based boards have an SMSC LAN911x/912x controller > > (drivers/net/ethernet/smsc/smsc911x.c) that is connected to the OMAP processor > > through its General-Purpose Memory Controller (GPMC) and this LAN driver obtain > > its IRQ and I/O address space from a struct resource IORESOURCE_IRQ and > > IORESOURCE_MEM respectively, that is filled by the DeviceTree core. > > > > It does this: > > > > irq_res = platform_get_resource(pdev, IORESOURCE_IRQ, 0); > > irq_flags = irq_res->flags & IRQF_TRIGGER_MASK; > > > > Since of_irq_to_resource() doesn't fill the trigger/level flags on the > > IORESOURCE_IRQ struct resource, irq_flags will always be 0 regarding the value > > specified on the second cell of the "interrupts" DT property. why do you need to known this in you driver this need to be handle in the irq chip I does use irq gpio without at all Best Regards, J.
On Thu, Jun 6, 2013 at 10:50 AM, Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com> wrote: > On 00:26 Thu 06 Jun , Grant Likely wrote: >> On Tue, 09 Apr 2013 00:44:05 +0200, Javier Martinez Canillas <javier.martinez@collabora.co.uk> wrote: >> > On 04/09/2013 12:05 AM, Rob Herring wrote: >> > > On 04/05/2013 02:48 AM, Javier Martinez Canillas wrote: >> > >> This means that drivers that need the IRQ type/level flags defined in >> > >> the DT won't be able to get it. >> > > >> > > But the interrupt controllers that need the information should be able >> > > to get to it via irqd_get_trigger_type. What problem exactly are you >> > > trying to fix? What driver would use this? >> > > >> > >> > Yes but this is not about the interrupt controller wanting this information but >> > a device driver that is using the IORESOURCE_IRQ struct resource that has the >> > information about the virtual IRQ associated with a GPIO-IRQ. >> > >> > The driver doesn't know neither care if its IRQ line is connected to a line of >> > an real IRQ controller or to a GPIO controller that allows a GPIO line to be >> > used as an IRQ. >> > >> > > My understanding of the IORESOURCE_IRQ_xxx (and DMA) bits are they are >> > > ISA specific and therefore should not be used on non-ISA buses. >> > > >> > >> > Many TI OMAP2+ SoC based boards have an SMSC LAN911x/912x controller >> > (drivers/net/ethernet/smsc/smsc911x.c) that is connected to the OMAP processor >> > through its General-Purpose Memory Controller (GPMC) and this LAN driver obtain >> > its IRQ and I/O address space from a struct resource IORESOURCE_IRQ and >> > IORESOURCE_MEM respectively, that is filled by the DeviceTree core. >> > >> > It does this: >> > >> > irq_res = platform_get_resource(pdev, IORESOURCE_IRQ, 0); >> > irq_flags = irq_res->flags & IRQF_TRIGGER_MASK; >> > >> > Since of_irq_to_resource() doesn't fill the trigger/level flags on the >> > IORESOURCE_IRQ struct resource, irq_flags will always be 0 regarding the value >> > specified on the second cell of the "interrupts" DT property. > > why do you need to known this in you driver this need to be handle in the irq > chip > > I does use irq gpio without at all > > Best Regards, > J. > Yes, in fact as I said on an earlier email on this thread if you don't pass a trigger type to the request_irq() call, it will use the trigger type set by the irq chip earlier with .xlate. A few months ago when I was testing DT on my OMAP3 board, for some reasons it was not getting the right trigger type. That's why I sent this patch but it seems it was a bug on the GPIO omap irq_chip driver since now is working correctly without this. It is taking the trigger type that was set on the .xlate function handler. So, I don't need this patch anymore to make the LAN chip work on my board and I don't know if the fact that an empty edge/level flags means that is going to be used whatever was set on the .xlate function handler is a reason enough to not return the IRQ flags on the struct resource or if someone else is still interested on this patch and filling this information on the struct resource. Best regards, Javier
On Thursday 06 of June 2013 10:50:39 Jean-Christophe PLAGNIOL-VILLARD wrote: > On 00:26 Thu 06 Jun , Grant Likely wrote: > > On Tue, 09 Apr 2013 00:44:05 +0200, Javier Martinez Canillas <javier.martinez@collabora.co.uk> wrote: > > > On 04/09/2013 12:05 AM, Rob Herring wrote: > > > > On 04/05/2013 02:48 AM, Javier Martinez Canillas wrote: > > > >> This means that drivers that need the IRQ type/level flags > > > >> defined in > > > >> the DT won't be able to get it. > > > > > > > > But the interrupt controllers that need the information should be > > > > able > > > > to get to it via irqd_get_trigger_type. What problem exactly are > > > > you > > > > trying to fix? What driver would use this? > > > > > > Yes but this is not about the interrupt controller wanting this > > > information but a device driver that is using the IORESOURCE_IRQ > > > struct resource that has the information about the virtual IRQ > > > associated with a GPIO-IRQ. > > > > > > The driver doesn't know neither care if its IRQ line is connected to > > > a line of an real IRQ controller or to a GPIO controller that > > > allows a GPIO line to be used as an IRQ. > > > > > > > My understanding of the IORESOURCE_IRQ_xxx (and DMA) bits are they > > > > are > > > > ISA specific and therefore should not be used on non-ISA buses. > > > > > > Many TI OMAP2+ SoC based boards have an SMSC LAN911x/912x controller > > > (drivers/net/ethernet/smsc/smsc911x.c) that is connected to the OMAP > > > processor through its General-Purpose Memory Controller (GPMC) and > > > this LAN driver obtain its IRQ and I/O address space from a struct > > > resource IORESOURCE_IRQ and IORESOURCE_MEM respectively, that is > > > filled by the DeviceTree core. > > > > > > It does this: > > > > > > irq_res = platform_get_resource(pdev, IORESOURCE_IRQ, 0); > > > irq_flags = irq_res->flags & IRQF_TRIGGER_MASK; > > > > > > Since of_irq_to_resource() doesn't fill the trigger/level flags on > > > the > > > IORESOURCE_IRQ struct resource, irq_flags will always be 0 regarding > > > the value specified on the second cell of the "interrupts" DT > > > property. > > why do you need to known this in you driver this need to be handle in > the irq chip There are devices that support multiple interrupt trigger types, like Broadcom WLAN chips, and drivers must configure them properly for requested trigger type, obtaining the information from flags field of the IRQ resource. Best regards, Tomasz
diff --git a/drivers/of/irq.c b/drivers/of/irq.c index a3c1c5a..6f6aa75 100644 --- a/drivers/of/irq.c +++ b/drivers/of/irq.c @@ -27,22 +27,39 @@ #include <linux/slab.h> /** - * irq_of_parse_and_map - Parse and map an interrupt into linux virq space + * irq_of_parse_and_map_type - Parse and map an interrupt into linux virq space * @device: Device node of the device whose interrupt is to be mapped * @index: Index of the interrupt to map + * @type: Interrupt trigger type and level flags filled by this function * * This function is a wrapper that chains of_irq_map_one() and * irq_create_of_mapping() to make things easier to callers */ -unsigned int irq_of_parse_and_map(struct device_node *dev, int index) +static unsigned int irq_of_parse_and_map_type(struct device_node *dev, + int index, unsigned int *type) { struct of_irq oirq; + unsigned int virq; if (of_irq_map_one(dev, index, &oirq)) return 0; - return irq_create_of_mapping(oirq.controller, oirq.specifier, - oirq.size); + virq = irq_create_of_mapping_type(oirq.controller, oirq.specifier, + oirq.size, type); + return virq; +} + +/** + * irq_of_parse_and_map - Parse and map an interrupt into linux virq space + * @device: Device node of the device whose interrupt is to be mapped + * @index: Index of the interrupt to map + * + * This function is a wrapper of irq_of_parse_and_map_type() when the IRQ + * type and level flags are not needed + */ +unsigned int irq_of_parse_and_map(struct device_node *dev, int index) +{ + return irq_of_parse_and_map_type(dev, index, NULL); } EXPORT_SYMBOL_GPL(irq_of_parse_and_map); @@ -338,7 +355,8 @@ EXPORT_SYMBOL_GPL(of_irq_map_one); */ int of_irq_to_resource(struct device_node *dev, int index, struct resource *r) { - int irq = irq_of_parse_and_map(dev, index); + int type = IRQ_TYPE_NONE; + int irq = irq_of_parse_and_map_type(dev, index, &type); /* Only dereference the resource if both the * resource and the irq are valid. */ @@ -353,7 +371,7 @@ int of_irq_to_resource(struct device_node *dev, int index, struct resource *r) &name); r->start = r->end = irq; - r->flags = IORESOURCE_IRQ; + r->flags = (IORESOURCE_IRQ | type); r->name = name ? name : dev->full_name; } diff --git a/include/linux/of_irq.h b/include/linux/of_irq.h index 535cecf..98aec57 100644 --- a/include/linux/of_irq.h +++ b/include/linux/of_irq.h @@ -66,6 +66,10 @@ extern int of_irq_map_one(struct device_node *device, int index, extern unsigned int irq_create_of_mapping(struct device_node *controller, const u32 *intspec, unsigned int intsize); +extern unsigned int irq_create_of_mapping_type(struct device_node *controller, + const u32 *intspec, + unsigned int intsize, + unsigned int *otype); extern int of_irq_to_resource(struct device_node *dev, int index, struct resource *r); extern int of_irq_count(struct device_node *dev); diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c index 96f3a1d..4a2f222 100644 --- a/kernel/irq/irqdomain.c +++ b/kernel/irq/irqdomain.c @@ -636,8 +636,9 @@ int irq_create_strict_mappings(struct irq_domain *domain, unsigned int irq_base, } EXPORT_SYMBOL_GPL(irq_create_strict_mappings); -unsigned int irq_create_of_mapping(struct device_node *controller, - const u32 *intspec, unsigned int intsize) +unsigned int irq_create_of_mapping_type(struct device_node *controller, + const u32 *intspec, unsigned int intsize, + unsigned int *otype) { struct irq_domain *domain; irq_hw_number_t hwirq; @@ -681,8 +682,17 @@ unsigned int irq_create_of_mapping(struct device_node *controller, if (type != IRQ_TYPE_NONE && type != (irqd_get_trigger_type(irq_get_irq_data(virq)))) irq_set_irq_type(virq, type); + if (otype) + *otype = type; return virq; } +EXPORT_SYMBOL_GPL(irq_create_of_mapping_type); + +unsigned int irq_create_of_mapping(struct device_node *controller, + const u32 *intspec, unsigned int intsize) +{ + return irq_create_of_mapping_type(controller, intspec, intsize, NULL); +} EXPORT_SYMBOL_GPL(irq_create_of_mapping); /**
According to Documentation/devicetree/bindings/interrupt-controller/interrupts.txt the "#interrupt-cells" property of an "interrupt-controller" is used to define the number of cells needed to specify a single interrupt. A commonly used variant is two cell on which #interrupt-cells = <2> and the first cell defines the index of the interrupt in the controller and the second cell is used to specify any of the following flags: - bits[3:0] trigger type and level flags 1 = low-to-high edge triggered 2 = high-to-low edge triggered 4 = active high level-sensitive 8 = active low level-sensitive An example of an interrupt controller which use the two cell format is the OMAP GPIO controller that allows GPIO lines to be used as IRQ (Documentation/devicetree/bindings/gpio/gpio-omap.txt) But setting #interrupt-cells = <2> on the OMAP GPIO device node and specifying the GPIO-IRQ type and level flags on the second cell does not store this value on the populated IORESOURCE_IRQ struct resource. This is because when using an IRQ from an interrupt controller and setting both cells (e.g:) interrupt-parent = <&gpio6>; interrupts = <16 8>; A call to of_irq_to_resource() is made and this function calls to irq_of_parse_and_map_type() to get the virtual IRQ mapped to the real index for this interrupt controller. This IRQ number is populated on the struct resource: int of_irq_to_resource(struct device_node *dev, int index, struct resource *r) { int irq = irq_of_parse_and_map(dev, index); .. r->start = r->end = irq; } irq_of_parse_and_map() calls to irq_create_of_mapping() which calls to the correct xlate function handler according to "#interrupt-cells" (irq_domain_xlate_onecell or irq_domain_xlate_twocell) and to irq_set_irq_type() to set the IRQ type. But the type is never returned so it can't be saved on the IRQ struct resource flags member. This means that drivers that need the IRQ type/level flags defined in the DT won't be able to get it. Signed-off-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk> --- drivers/of/irq.c | 30 ++++++++++++++++++++++++------ include/linux/of_irq.h | 4 ++++ kernel/irq/irqdomain.c | 14 ++++++++++++-- 3 files changed, 40 insertions(+), 8 deletions(-)