| Message ID | 1379000351-15672-2-git-send-email-r.sricharan@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, 12 Sep 2013, Sricharan R wrote: > Signed-off-by: Sricharan R <r.sricharan@ti.com> > --- > There is lockdep warning during the boot. This is because we try to > do one request_irq with in another and that results in kmalloc being > called from an atomic context, which generates the warning. > Any suggestions to overcome this will help. You can't be serious about that. You post a patch series with a serious bug in it instead of asking in the first place how to solve the problem at hand. So why do you actually need to call request_irq() from inside request_irq() and how do you actually manage to do that at all? I have a hard time to figure out how request_irq() might call itself recursive. And I have no intention to look at your patch to figure out that you abuse an irq chip callback to do so, simply because that would force me to use abusive language which is frowned upon nowadays. Can you please explain what you are trying to solve without referring to your existing broken implementation. Thanks, tglx -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Thomas, On Thursday 12 September 2013 04:18 PM, Thomas Gleixner wrote: > On Thu, 12 Sep 2013, Sricharan R wrote: >> Signed-off-by: Sricharan R <r.sricharan@ti.com> >> --- >> There is lockdep warning during the boot. This is because we try to >> do one request_irq with in another and that results in kmalloc being >> called from an atomic context, which generates the warning. >> Any suggestions to overcome this will help. > > You can't be serious about that. You post a patch series with a > serious bug in it instead of asking in the first place how to solve > the problem at hand. > > So why do you actually need to call request_irq() from inside > request_irq() and how do you actually manage to do that at all? > > I have a hard time to figure out how request_irq() might call itself > recursive. And I have no intention to look at your patch to figure out > that you abuse an irq chip callback to do so, simply because that > would force me to use abusive language which is frowned upon nowadays. > This is an outcome of the discussion on earlier patch set [1] which tried to add IRQ event router functionality. From beginning I was against the idea because I also felt that we are abusing the irqchip infrastructure and force fitting the cross-bar to be behave like an irqchip. But Linus W and few others strongly felt it to make it an irqchip implementation. > Can you please explain what you are trying to solve without referring > to your existing broken implementation. > I will try to summaries the IP and its need here. On TI SOCs, we have an IP cross-bar which is essentially an even router(hardware). It can route the IRQ and DMA in existing implementation. Specifically for the IRQ case addressed here, the cross-bar IP sits between the interrupt controller and peripheral interrupts. CPU <-- GIC <----- CROSSBAR <----- PERIPHERAL IRQs Just to expand it better, cross-bar input IRQ lines are more than what a GIC IRQ controller can support. e.q Total 250 peripheral IRQ lines out of which GIC support only 160 IRQ lines. So the idea here is to dynamically map the IRQ lines at cross-bar level to pick based on request_irq() so that one can optimize the use of limited IRQ lines at the GIC level. Strictly speaking the need is just establish the IRQ connection from peripheral to GIC and thats achieved at the request_irq() level. Earlier approach was to statically build this connections using the DT information in a separate driver probe but it had limitations of fixing the IRQ map and taking away flexibility what this IP provide. Hope this gives better picture to you behind the patch series. Just for others to know, use of IRQCHIP also forced to have all the irqchip handler redirection which is pure redirection including the irq-handler. And since it is *fast path* I asked Sricharan to measure the latency which is around ~2 uS( 1GHz CPU) overhead for every interrupt just because of redirections. Regards, Santosh [1] https://lkml.org/lkml/2013/7/18/362 -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Sep 12, 2013 at 09:09:08PM +0530, Sricharan R wrote: > +unsigned int crossbar_request_irq(struct irq_data *d) > +{ > + int cb_no = d->hwirq; > + int virq = allocate_free_irq(cb_no); > + void *irq = &cb->crossbar_map[cb_no].hwirq; > + int err; > + > + err = request_threaded_irq(virq, crossbar_irq, NULL, > + 0, "CROSSBAR", irq); this is wrong, why don't you just set crossbar up as a chained handler.
On Thu, 12 Sep 2013, Felipe Balbi wrote: > On Thu, Sep 12, 2013 at 09:09:08PM +0530, Sricharan R wrote: > > +unsigned int crossbar_request_irq(struct irq_data *d) > > +{ > > + int cb_no = d->hwirq; > > + int virq = allocate_free_irq(cb_no); > > + void *irq = &cb->crossbar_map[cb_no].hwirq; > > + int err; > > + > > + err = request_threaded_irq(virq, crossbar_irq, NULL, > > + 0, "CROSSBAR", irq); > > this is wrong, why don't you just set crossbar up as a chained handler. That's just a detail, which does not solve the underlying problem of the crossbar -> GIC mapping. Thanks, tglx -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 12 Sep 2013, Thomas Gleixner wrote: > On Thu, 12 Sep 2013, Felipe Balbi wrote: > > > On Thu, Sep 12, 2013 at 09:09:08PM +0530, Sricharan R wrote: > > > +unsigned int crossbar_request_irq(struct irq_data *d) > > > +{ > > > + int cb_no = d->hwirq; > > > + int virq = allocate_free_irq(cb_no); > > > + void *irq = &cb->crossbar_map[cb_no].hwirq; > > > + int err; > > > + > > > + err = request_threaded_irq(virq, crossbar_irq, NULL, > > > + 0, "CROSSBAR", irq); > > > > this is wrong, why don't you just set crossbar up as a chained handler. > > That's just a detail, which does not solve the underlying problem of > the crossbar -> GIC mapping. And actually the thing is the other way round: GIC distributes to crossbar, so the underlying GIC irq would be the chained handler. Still does not solve the setup/request_irq issue. Thanks, tglx -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 12 Sep 2013, Santosh Shilimkar wrote: > Specifically for the IRQ case addressed here, the cross-bar IP > sits between the interrupt controller and peripheral interrupts. > > CPU <-- GIC <----- CROSSBAR <----- PERIPHERAL IRQs > > Just to expand it better, cross-bar input IRQ lines are more than > what a GIC IRQ controller can support. > e.q Total 250 peripheral IRQ lines out of which GIC support > only 160 IRQ lines. > > So the idea here is to dynamically map the IRQ lines at > cross-bar level to pick based on request_irq() so that one > can optimize the use of limited IRQ lines at the GIC level. > Strictly speaking the need is just establish the IRQ > connection from peripheral to GIC and thats achieved > at the request_irq() level. > > Earlier approach was to statically build this connections > using the DT information in a separate driver probe but > it had limitations of fixing the IRQ map and taking away > flexibility what this IP provide. > > Hope this gives better picture to you behind the patch > series. Yes. I halfways understand what you are trying to achieve. So CROSSBAR is a routing block between the peripheral and the GIC in order to expand the number of possible interrupts. Now the real question is, how that expansion mechanism is supposed to work. There are two possible scenarios: 1) Expand the number of handled interrupts beyond the GIC capacity: That requires a mechanism in CROSSBAR to map several CROSSBAR interrupts to a particular GIC interrupt and provide a demux mechanism to invoke the shared handlers. 2) Provide a mapping mechanism between possibly 250 interrupt numbers and a limitation of a total 160 active interrupts by the underlying GIC. What are you trying to achieve? Thanks, tglx -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thursday 12 September 2013 06:22 PM, Thomas Gleixner wrote: > On Thu, 12 Sep 2013, Santosh Shilimkar wrote: >> Specifically for the IRQ case addressed here, the cross-bar IP >> sits between the interrupt controller and peripheral interrupts. >> >> CPU <-- GIC <----- CROSSBAR <----- PERIPHERAL IRQs >> >> Just to expand it better, cross-bar input IRQ lines are more than >> what a GIC IRQ controller can support. >> e.q Total 250 peripheral IRQ lines out of which GIC support >> only 160 IRQ lines. >> >> So the idea here is to dynamically map the IRQ lines at >> cross-bar level to pick based on request_irq() so that one >> can optimize the use of limited IRQ lines at the GIC level. >> Strictly speaking the need is just establish the IRQ >> connection from peripheral to GIC and thats achieved >> at the request_irq() level. >> >> Earlier approach was to statically build this connections >> using the DT information in a separate driver probe but >> it had limitations of fixing the IRQ map and taking away >> flexibility what this IP provide. >> >> Hope this gives better picture to you behind the patch >> series. > > Yes. I halfways understand what you are trying to achieve. > > So CROSSBAR is a routing block between the peripheral and the GIC in > order to expand the number of possible interrupts. > > Now the real question is, how that expansion mechanism is supposed to > work. There are two possible scenarios: > > 1) Expand the number of handled interrupts beyond the GIC capacity: > > That requires a mechanism in CROSSBAR to map several CROSSBAR > interrupts to a particular GIC interrupt and provide a demux > mechanism to invoke the shared handlers. > This is not possible in hardware and not supported. Hardware has no notion of muxing multiple IRQ's to generate 1 IRQ or ack etc functionality. Its a simple MUX to tie knots between input and output wires. > 2) Provide a mapping mechanism between possibly 250 interrupt numbers > and a limitation of a total 160 active interrupts by the underlying > GIC. > This is the need and problem we are trying to solve. Regards, Santosh -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 12 Sep 2013, Santosh Shilimkar wrote: > On Thursday 12 September 2013 06:22 PM, Thomas Gleixner wrote: > > Now the real question is, how that expansion mechanism is supposed to > > work. There are two possible scenarios: > > > > 1) Expand the number of handled interrupts beyond the GIC capacity: > > > > That requires a mechanism in CROSSBAR to map several CROSSBAR > > interrupts to a particular GIC interrupt and provide a demux > > mechanism to invoke the shared handlers. > > > This is not possible in hardware and not supported. Hardware has > no notion of muxing multiple IRQ's to generate 1 IRQ or ack etc > functionality. Its a simple MUX to tie knots between input and output > wires. It's not a MUX. It's a ROUTING mechanism. That's similar to the mechanisms which are used by MSI[X]. We assign arbitrary interrupt numbers to a device and route them to some underlying limited hardware interrupt controller. > > 2) Provide a mapping mechanism between possibly 250 interrupt numbers > > and a limitation of a total 160 active interrupts by the underlying > > GIC. > > > This is the need and problem we are trying to solve. Let me summarize: - GIC supports up to 160 interrupts - CROSSBAR supports up to 250 interrupts - CROSSBAR routes up to 160 out of 250 interrupts to the GIC ones - Drivers request a CROSSBAR interrupt number which must be mapped to some arbitrary available GIC irq number So basically the CROSSBAR mechanism is pretty much the same as MSI[X] just in a different flavour and with a different set of semantics and limitations, i.e. poor mans MSI[X] with a new level of bogosity. So if CROSSBAR is going to be the new fangled SoC MSI[X] long term equivalent then you better provide some infrastructure for that and make the drivers ready to use it. Maybe check with the PCI/MSI folks to share some of the interfaces. If that whole thing is another onetime HW designers wet dream, then please go back to the limited but completely functional (Who is going to use more than 160 peripheral interrupts????) device tree model. I really have no interest to support hardware designer brain farts. Thanks, tglx -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thursday 12 September 2013 08:26 PM, Thomas Gleixner wrote: > On Thu, 12 Sep 2013, Santosh Shilimkar wrote: >> On Thursday 12 September 2013 06:22 PM, Thomas Gleixner wrote: >>> Now the real question is, how that expansion mechanism is supposed to >>> work. There are two possible scenarios: >>> >>> 1) Expand the number of handled interrupts beyond the GIC capacity: >>> >>> That requires a mechanism in CROSSBAR to map several CROSSBAR >>> interrupts to a particular GIC interrupt and provide a demux >>> mechanism to invoke the shared handlers. >>> >> This is not possible in hardware and not supported. Hardware has >> no notion of muxing multiple IRQ's to generate 1 IRQ or ack etc >> functionality. Its a simple MUX to tie knots between input and output >> wires. > > It's not a MUX. It's a ROUTING mechanism. That's similar to the > mechanisms which are used by MSI[X]. We assign arbitrary interrupt > numbers to a device and route them to some underlying limited hardware > interrupt controller. > >>> 2) Provide a mapping mechanism between possibly 250 interrupt numbers >>> and a limitation of a total 160 active interrupts by the underlying >>> GIC. >>> >> This is the need and problem we are trying to solve. > > Let me summarize: > > - GIC supports up to 160 interrupts > > - CROSSBAR supports up to 250 interrupts > > - CROSSBAR routes up to 160 out of 250 interrupts to the GIC ones > > - Drivers request a CROSSBAR interrupt number which must be mapped > to some arbitrary available GIC irq number > Correct. > So basically the CROSSBAR mechanism is pretty much the same as MSI[X] > just in a different flavour and with a different set of semantics and > limitations, i.e. poor mans MSI[X] with a new level of bogosity. > > So if CROSSBAR is going to be the new fangled SoC MSI[X] long term > equivalent then you better provide some infrastructure for that and > make the drivers ready to use it. Maybe check with the PCI/MSI folks > to share some of the interfaces. > > If that whole thing is another onetime HW designers wet dream, then > please go back to the limited but completely functional (Who is going > to use more than 160 peripheral interrupts????) device tree model. I > really have no interest to support hardware designer brain farts. > Thanks for clear NAK for irqchip approach. I should have looped you in the discussion where I was also suggesting against the irqchip approach. We will try to look at MSI stuff but if its get too complicated am going to fall-back to the initial probe based approach to achieve the functionality. Thanks again for clear direction and useful discussion. Regards, Santosh -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Friday 13 September 2013 07:12 AM, Santosh Shilimkar wrote: > On Thursday 12 September 2013 08:26 PM, Thomas Gleixner wrote: >> On Thu, 12 Sep 2013, Santosh Shilimkar wrote: >>> On Thursday 12 September 2013 06:22 PM, Thomas Gleixner wrote: >>>> Now the real question is, how that expansion mechanism is supposed to >>>> work. There are two possible scenarios: >>>> >>>> 1) Expand the number of handled interrupts beyond the GIC capacity: >>>> >>>> That requires a mechanism in CROSSBAR to map several CROSSBAR >>>> interrupts to a particular GIC interrupt and provide a demux >>>> mechanism to invoke the shared handlers. >>>> >>> This is not possible in hardware and not supported. Hardware has >>> no notion of muxing multiple IRQ's to generate 1 IRQ or ack etc >>> functionality. Its a simple MUX to tie knots between input and output >>> wires. >> It's not a MUX. It's a ROUTING mechanism. That's similar to the >> mechanisms which are used by MSI[X]. We assign arbitrary interrupt >> numbers to a device and route them to some underlying limited hardware >> interrupt controller. >> >>>> 2) Provide a mapping mechanism between possibly 250 interrupt numbers >>>> and a limitation of a total 160 active interrupts by the underlying >>>> GIC. >>>> >>> This is the need and problem we are trying to solve. >> Let me summarize: >> >> - GIC supports up to 160 interrupts >> >> - CROSSBAR supports up to 250 interrupts >> >> - CROSSBAR routes up to 160 out of 250 interrupts to the GIC ones >> >> - Drivers request a CROSSBAR interrupt number which must be mapped >> to some arbitrary available GIC irq number >> > Correct. > >> So basically the CROSSBAR mechanism is pretty much the same as MSI[X] >> just in a different flavour and with a different set of semantics and >> limitations, i.e. poor mans MSI[X] with a new level of bogosity. >> >> So if CROSSBAR is going to be the new fangled SoC MSI[X] long term >> equivalent then you better provide some infrastructure for that and >> make the drivers ready to use it. Maybe check with the PCI/MSI folks >> to share some of the interfaces. >> >> If that whole thing is another onetime HW designers wet dream, then >> please go back to the limited but completely functional (Who is going >> to use more than 160 peripheral interrupts????) device tree model. I >> really have no interest to support hardware designer brain farts. >> > Thanks for clear NAK for irqchip approach. I should have looped you > in the discussion where I was also suggesting against the irqchip > approach. We will try to look at MSI stuff but if its get too > complicated am going to fall-back to the initial probe based > approach to achieve the functionality. > > Thanks again for clear direction and useful discussion. Thanks for the feedback. I will look in to the MSI driver and see if how that would work. Regards, Sricharan -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 12 Sep 2013, Santosh Shilimkar wrote: > On Thursday 12 September 2013 08:26 PM, Thomas Gleixner wrote: > > Let me summarize: > > > > - GIC supports up to 160 interrupts > > > > - CROSSBAR supports up to 250 interrupts > > > > - CROSSBAR routes up to 160 out of 250 interrupts to the GIC ones > > > > - Drivers request a CROSSBAR interrupt number which must be mapped > > to some arbitrary available GIC irq number > > > Correct. > > > So basically the CROSSBAR mechanism is pretty much the same as MSI[X] > > just in a different flavour and with a different set of semantics and > > limitations, i.e. poor mans MSI[X] with a new level of bogosity. > > > > So if CROSSBAR is going to be the new fangled SoC MSI[X] long term > > equivalent then you better provide some infrastructure for that and > > make the drivers ready to use it. Maybe check with the PCI/MSI folks > > to share some of the interfaces. > > > > If that whole thing is another onetime HW designers wet dream, then > > please go back to the limited but completely functional (Who is going > > to use more than 160 peripheral interrupts????) device tree model. I > > really have no interest to support hardware designer brain farts. > > > Thanks for clear NAK for irqchip approach. I should have looped you > in the discussion where I was also suggesting against the irqchip > approach. We will try to look at MSI stuff but if its get too > complicated am going to fall-back to the initial probe based > approach to achieve the functionality. Before you dig into MSI, lets talk about irq domains first. GIC implements a legacy irq domain, i.e. a linear domain of all possible GIC interrupts with a 1:1 mapping. So why can't you make use of irq domains and have the whole routing business implemented sanely? What's needed is in gic_init_bases(): if (of_property_read(node, "routable_irqs", &nr_routable_irqs) { irq_domain_add_legacy(nr_gic_irqs); } else { irq_domain_add_legacy(nr_per_cpu_irqs); irq_domain_add_linear(nr_routable_irqs); } Now that separate domain has an xlate function which grabs a free GIC irq from a bitmap and returns the hardware irq number in the gic space. The map/unmap callbacks take care of setting up / tearing down the route in the crossbar. Thoughts? Thanks, tglx -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Friday 13 September 2013 10:24 AM, Thomas Gleixner wrote: > On Thu, 12 Sep 2013, Santosh Shilimkar wrote: >> On Thursday 12 September 2013 08:26 PM, Thomas Gleixner wrote: >>> Let me summarize: >>> >>> - GIC supports up to 160 interrupts >>> >>> - CROSSBAR supports up to 250 interrupts >>> >>> - CROSSBAR routes up to 160 out of 250 interrupts to the GIC ones >>> >>> - Drivers request a CROSSBAR interrupt number which must be mapped >>> to some arbitrary available GIC irq number >>> >> Correct. >> >>> So basically the CROSSBAR mechanism is pretty much the same as MSI[X] >>> just in a different flavour and with a different set of semantics and >>> limitations, i.e. poor mans MSI[X] with a new level of bogosity. >>> >>> So if CROSSBAR is going to be the new fangled SoC MSI[X] long term >>> equivalent then you better provide some infrastructure for that and >>> make the drivers ready to use it. Maybe check with the PCI/MSI folks >>> to share some of the interfaces. >>> >>> If that whole thing is another onetime HW designers wet dream, then >>> please go back to the limited but completely functional (Who is going >>> to use more than 160 peripheral interrupts????) device tree model. I >>> really have no interest to support hardware designer brain farts. >>> >> Thanks for clear NAK for irqchip approach. I should have looped you >> in the discussion where I was also suggesting against the irqchip >> approach. We will try to look at MSI stuff but if its get too >> complicated am going to fall-back to the initial probe based >> approach to achieve the functionality. > > Before you dig into MSI, lets talk about irq domains first. > > GIC implements a legacy irq domain, i.e. a linear domain of all > possible GIC interrupts with a 1:1 mapping. > > So why can't you make use of irq domains and have the whole routing > business implemented sanely? > > What's needed is in gic_init_bases(): > > if (of_property_read(node, "routable_irqs", &nr_routable_irqs) { > irq_domain_add_legacy(nr_gic_irqs); > } else { > irq_domain_add_legacy(nr_per_cpu_irqs); > irq_domain_add_linear(nr_routable_irqs); > } > > Now that separate domain has an xlate function which grabs a free GIC > irq from a bitmap and returns the hardware irq number in the gic > space. The map/unmap callbacks take care of setting up / tearing down > the route in the crossbar. > > Thoughts? > This sounds pretty good idea. We will explore above option. Thanks Thomas. Regards, Santosh -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Sep 13, 2013 at 4:24 PM, Thomas Gleixner <tglx@linutronix.de> wrote: > So why can't you make use of irq domains and have the whole routing > business implemented sanely? > > What's needed is in gic_init_bases(): > > if (of_property_read(node, "routable_irqs", &nr_routable_irqs) { > irq_domain_add_legacy(nr_gic_irqs); > } else { > irq_domain_add_legacy(nr_per_cpu_irqs); > irq_domain_add_linear(nr_routable_irqs); > } > > Now that separate domain has an xlate function which grabs a free GIC > irq from a bitmap and returns the hardware irq number in the gic > space. The map/unmap callbacks take care of setting up / tearing down > the route in the crossbar. This is obviously the right approach, it's exactly what .map should do the only special thing here being that we have hardware to perform the mapping ... bah why didn't I realize this :-( Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Thomas, On Tuesday 17 September 2013 05:56 PM, Linus Walleij wrote: > On Fri, Sep 13, 2013 at 4:24 PM, Thomas Gleixner <tglx@linutronix.de> wrote: > >> So why can't you make use of irq domains and have the whole routing >> business implemented sanely? >> >> What's needed is in gic_init_bases(): >> irq >> if (of_property_read(node, "routable_irqs", &nr_routable_irqs) { >> irq_domain_add_legacy(nr_gic_irqs); >> } else { >> irq_domain_add_legacy(nr_per_cpu_irqs); >> irq_domain_add_linear(nr_routable_irqs); >> } >> >> Now that separate domain has an xlate function which grabs a free GIC >> irq from a bitmap and returns the hardware irq number in the gic >> space. The map/unmap callbacks take care of setting up / tearing down >> the route in the crossbar. > This is obviously the right approach, it's exactly what .map should do > the only special thing here being that we have hardware to perform > the mapping ... bah why didn't I realize this :-( > > Yours, > Linus Walleij Thanks for the suggestion. So as i understand this, this implies using the GIC domain itself and add the support for dynamically routable irqs (like crossbar) with in the GIC driver itself right ? Regards, Sricharan -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Thomas, On Friday 13 September 2013 10:55 AM, Santosh Shilimkar wrote: > On Friday 13 September 2013 10:24 AM, Thomas Gleixner wrote: [...] >> Before you dig into MSI, lets talk about irq domains first. >> >> GIC implements a legacy irq domain, i.e. a linear domain of all >> possible GIC interrupts with a 1:1 mapping. >> >> So why can't you make use of irq domains and have the whole routing >> business implemented sanely? >> >> What's needed is in gic_init_bases(): >> >> if (of_property_read(node, "routable_irqs", &nr_routable_irqs) { >> irq_domain_add_legacy(nr_gic_irqs); >> } else { >> irq_domain_add_legacy(nr_per_cpu_irqs); >> irq_domain_add_linear(nr_routable_irqs); >> } >> >> Now that separate domain has an xlate function which grabs a free GIC >> irq from a bitmap and returns the hardware irq number in the gic >> space. The map/unmap callbacks take care of setting up / tearing down >> the route in the crossbar. >> >> Thoughts? >> > This sounds pretty good idea. We will explore above option. > Thanks Thomas. > After further looking into this, the irqdomain approach lets us setup the map only once during the init. This is similar to the earlier approach of cross-bar driver where at probe time the router was setup. The whole debate started with the fact that we shouldn't fix the irq mapping at probe and should dynamically change the mapping based on [request/free]_irq() to be able to maximize the use of the IP. Since we have agreed now to move ahead with irdomain, i thought of mentioning it here. Regards, Santosh -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wednesday 18 September 2013 07:22 PM, Sricharan R wrote: > Hi Thomas, > > On Tuesday 17 September 2013 05:56 PM, Linus Walleij wrote: >> On Fri, Sep 13, 2013 at 4:24 PM, Thomas Gleixner <tglx@linutronix.de> wrote: >> >>> So why can't you make use of irq domains and have the whole routing >>> business implemented sanely? >>> >>> What's needed is in gic_init_bases(): >>> irq >>> if (of_property_read(node, "routable_irqs", &nr_routable_irqs) { >>> irq_domain_add_legacy(nr_gic_irqs); >>> } else { >>> irq_domain_add_legacy(nr_per_cpu_irqs); >>> irq_domain_add_linear(nr_routable_irqs); >>> } >>> >>> Now that separate domain has an xlate function which grabs a free GIC >>> irq from a bitmap and returns the hardware irq number in the gic >>> space. The map/unmap callbacks take care of setting up / tearing down >>> the route in the crossbar. >> This is obviously the right approach, it's exactly what .map should do >> the only special thing here being that we have hardware to perform >> the mapping ... bah why didn't I realize this :-( >> >> Yours, >> Linus Walleij > Thanks for the suggestion. > > So as i understand this, this implies using the GIC domain itself and > add the support for dynamically routable irqs (like crossbar) with in the > GIC driver itself right ? Please ignore this. So the question was more of how to implement the call outs in the case of routable irqs from map/ unmap callbacks. I will look more here and come back. Regards, Sricharan -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, 18 Sep 2013, Sricharan R wrote: > On Wednesday 18 September 2013 07:22 PM, Sricharan R wrote: > > Hi Thomas, > > > > On Tuesday 17 September 2013 05:56 PM, Linus Walleij wrote: > >> On Fri, Sep 13, 2013 at 4:24 PM, Thomas Gleixner <tglx@linutronix.de> wrote: > >> > >>> So why can't you make use of irq domains and have the whole routing > >>> business implemented sanely? > >>> > >>> What's needed is in gic_init_bases(): > >>> irq > >>> if (of_property_read(node, "routable_irqs", &nr_routable_irqs) { > >>> irq_domain_add_legacy(nr_gic_irqs); > >>> } else { > >>> irq_domain_add_legacy(nr_per_cpu_irqs); > >>> irq_domain_add_linear(nr_routable_irqs); > >>> } > >>> > >>> Now that separate domain has an xlate function which grabs a free GIC > >>> irq from a bitmap and returns the hardware irq number in the gic > >>> space. The map/unmap callbacks take care of setting up / tearing down > >>> the route in the crossbar. > >> This is obviously the right approach, it's exactly what .map should do > >> the only special thing here being that we have hardware to perform > >> the mapping ... bah why didn't I realize this :-( > >> > >> Yours, > >> Linus Walleij > > Thanks for the suggestion. > > > > So as i understand this, this implies using the GIC domain itself and > > add the support for dynamically routable irqs (like crossbar) with in the > > GIC driver itself right ? > Please ignore this. So the question was more of how to implement the > call outs in the case of routable irqs from map/ unmap callbacks. If you look closely at what I suggested, you'll notice that I added a separate domain in the routed case. Go figure ... Thanks, tglx -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, 18 Sep 2013, Santosh Shilimkar wrote: > On Friday 13 September 2013 10:55 AM, Santosh Shilimkar wrote: > > On Friday 13 September 2013 10:24 AM, Thomas Gleixner wrote: > > [...] > > >> Before you dig into MSI, lets talk about irq domains first. > >> > >> GIC implements a legacy irq domain, i.e. a linear domain of all > >> possible GIC interrupts with a 1:1 mapping. > >> > >> So why can't you make use of irq domains and have the whole routing > >> business implemented sanely? > >> > >> What's needed is in gic_init_bases(): > >> > >> if (of_property_read(node, "routable_irqs", &nr_routable_irqs) { > >> irq_domain_add_legacy(nr_gic_irqs); > >> } else { > >> irq_domain_add_legacy(nr_per_cpu_irqs); > >> irq_domain_add_linear(nr_routable_irqs); > >> } > >> > >> Now that separate domain has an xlate function which grabs a free GIC > >> irq from a bitmap and returns the hardware irq number in the gic > >> space. The map/unmap callbacks take care of setting up / tearing down > >> the route in the crossbar. > >> > >> Thoughts? > >> > > This sounds pretty good idea. We will explore above option. > > Thanks Thomas. > > > After further looking into this, the irqdomain approach lets us > setup the map only once during the init. This is similar to > the earlier approach of cross-bar driver where at probe time > the router was setup. The whole debate started with the fact > that we shouldn't fix the irq mapping at probe and should > dynamically change the mapping based on [request/free]_irq() > to be able to maximize the use of the IP. Well, that's what irq_of_parse_and_map() resp. irq_create_mapping and irq_dispose_mapping() are for. It requires changes to drivers, but you can't get that thing for free. Thanks, tglx -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, I have a few comments, mostly on the DT binding and parsing. > diff --git a/Documentation/devicetree/bindings/arm/omap/irq-crossbar.txt b/Documentation/devicetree/bindings/arm/omap/irq-crossbar.txt > new file mode 100644 > index 0000000..5d465cf > --- /dev/null > +++ b/Documentation/devicetree/bindings/arm/omap/irq-crossbar.txt > @@ -0,0 +1,39 @@ > +* IRQ CROSSBAR > + > +Some socs have a large number of interrupts requests to service > +the needs of its many peripherals and subsystems. All of the > +interrupt lines from the subsystems are not needed at the same > +time, so they have to be muxed to the irq-controller appropriately. > +In such places a interrupt controllers are preceded by an CROSSBAR > +that provides flexibility in muxing the device requests to the controller > +inputs. > + > +Required properties: > +- compatible : Should be "irq-crossbar" Missing vendor prefix, this should be something like "ti,irq-crossbar". Does this have a more specific name than CROSSBAR that can be used to qualify it? > +- interrupt-parent: phandle to crossbar's interrupt parent. > +- interrupt-controller: Identifies the node as an interrupt controller. > +- interrupt-cells: Should be the same value as the interrupt parent. That doesn't make sense. The crossbar driver is necessarily interpreting these cells in a way the parent won't (as it supports more interrupts). What are the meaning of these cells? > +- reg: Base address and the size of the crossbar registers. > +- max-crossbar-lines: Total number of input lines of the crossbar. > +- max-irqs: Total number of irqs available at the interrupt controller. Is this the maximum number of interrupts targeting the parent interrupt controller? Starting at what number, ending at what number? Can this have gaps? Is this a shortcut so in the GIC case you don't have to describe up to 160 interrupts? I can see why you don't want to, but there's a big loss of generality here... > +- reg-size: size of the crossbar registers. As in the size of all the registers (the size component of reg)? Or is this the size of each individual register? Does that apply to all registers or only a subset of them? What units are these in, bytes? What are valid sizes? Is this really that configurable? > +- irqs-reserved: List of the reserved irq lines that are not muxed using > + crossbar. These interrupt lines are reserved in the soc, > + so crossbar bar driver should not consider them as free > + lines. Are these reserved inputs lines, or outputs to the parent interrupt controller? What is the format of each entry in this list? The example seems to be a different format to the parent interrupt controller (which per your binding also defined the crossbar's interrupt format). While <0 1 2> is a valid interrupt per the GIC binding (SPI 0 edge-triggered both ways), <3 5 6>, <131 132 139>, and <140 . .> are not. > + > +Examples: > + crossbar_mpu: @4a020000 { > + compatible = "irq-crossbar"; > + interrupt-parent = <&gic>; > + interrupt-controller; > + #interrupt-cells = <3>; > + reg = <0x4a002a48 0x130>; > + max-crossbar-lines = <512>; > + max-irqs = <160>; > + reg-size = <2>; > + irqs-reserved = <0 1 2 3 5 6 131 132 139 140>; > + #address-cells = <1>; > + #size-cells = <1>; Why are there #address-cells and #size cells? This has no children, and this affects any interrupt-map property (as the parent unit address now must be a single cell, that isn't going to be used). [...] > +static int crossbar_set_affinity(struct irq_data *d, > + const struct cpumask *mask_val, > + bool force) > +{ > + struct irq_chip *chip; > + struct irq_data *data; > + int ret = 0; > + > + crossbar_to_irq_chip_data(d->hwirq, &chip, &data); > + > + if (chip->irq_set_affinity) > + ret = chip->irq_set_affinity(data, mask_val, force); > + > + return ret; So if our parent chip can't set affinity, we pretend it can? irq_set_affinity in kernel/irq/manage.c returns -EINVAL if an irqchip doesn't have irq_set_affinity. > +/* > + * Request and free are already called in atomic contexts > + */ > +unsigned int crossbar_request_irq(struct irq_data *d) > +{ > + int cb_no = d->hwirq; > + int virq = allocate_free_irq(cb_no); > + void *irq = &cb->crossbar_map[cb_no].hwirq; > + int err; > + > + err = request_threaded_irq(virq, crossbar_irq, NULL, > + 0, "CROSSBAR", irq); > + if (err) > + pr_err("\n request_irq failed for crossbar %d", cb_no); Why does the print begin with a newline rather than ending with one? > + > + return 0; > +} [...] > +static int crossbar_domain_xlate(struct irq_domain *d, > + struct device_node *controller, > + const u32 *intspec, unsigned int intsize, > + unsigned long *out_hwirq, > + unsigned int *out_type) > +{ > + int i, cb_no; > + u32 *cb_intspec = kzalloc(intsize * sizeof(int), GFP_KERNEL); > + > + if (!cb_intspec) > + return -ENOMEM; > + > + cb_no = intspec[1]; So #interrupt-cells must be at least <2>. You should sanity check intsize >= 2 before this line or you'll perform an illegal array access. > + > + if (WARN_ON(intsize < 1)) > + return -EINVAL; This sanity check is both wrong and too late, as mentioned above. > + > + cb->crossbar_map[cb_no].intspec = cb_intspec; > + > + /* > + * Free irq is allocated and mapped during request_irq > + * So just save the interrupt properties here > + */ > + for (i = 0; i < intsize; i++) > + cb->crossbar_map[cb_no].intspec[i] = intspec[i]; > + > + cb->crossbar_map[cb_no].intspec_size = intsize; > + *out_hwirq = intspec[1]; > + *out_type = IRQ_TYPE_NONE; > + > + return 0; > +} > + > +const struct irq_domain_ops crossbar_domain_ops = { > + .map = crossbar_domain_map, > + .xlate = crossbar_domain_xlate > +}; > + > +static int __init crossbar_of_init(struct device_node *node, > + struct device_node *parent) > +{ > + int i, size, max, reserved = 0; > + const __be32 *irqsr; > + > + if (!parent) { > + pr_err("\n interrupt-parent is missing"); Another odd newline. > + return -ENODEV; > + } > + > + cb = kzalloc(sizeof(struct cb_device *), GFP_KERNEL); > + > + if (!cb) > + return -ENOMEM; > + > + cb->irqp = parent; > + > + cb->crossbar_base = of_iomap(node, 0); > + if (!cb->crossbar_base) > + return -ENOMEM; Leaking allocated cb here. > + > + of_property_read_u32(node, "max-crossbar-lines", &max); > + cb->crossbar_map = kzalloc(max * sizeof(struct pirqs *), GFP_KERNEL); > + > + if (!cb->crossbar_map) > + return -ENOMEM; Leaking cb and cb->crossbar_base mapping. > + > + cb->domain = irq_domain_add_linear(node, max, > + &crossbar_domain_ops, NULL); > + > + if (!cb->domain) { > + pr_err("Couldn't register an IRQ domain\n"); > + return -ENODEV; > + } Leaking cb, cb->crossbar_base, and cb->crossbar_map here. > + > + of_property_read_u32(node, "max-irqs", &max); > + cb->irq_map = kzalloc(max * sizeof(int), GFP_KERNEL); > + if (!cb->irq_map) > + return -ENOMEM; Leaking cb, cb->crossbar_base, cb->crossbar_map, and cb->domain. > + > + cb->int_max = max; > + > + for (i = 0; i < max; i++) > + cb->irq_map[i] = IRQ_FREE; > + > + /* Get and mark reserved irqs */ > + irqsr = of_get_property(node, "irqs-reserved", &size); > + size /= sizeof(int); The entries will be __be32, not int. > + > + for (i = 0; i < size; i++) > + cb->irq_map[be32_to_cpup(irqsr + i)] = 0; No sanity check on array bounds? What about a dt that has something like: irqs-reserved = <0x0 0xcccccccc 0xffffffff>; It's clearly wrong, and we can detect that rather than bringing down the kernel... > + > + cb->register_offsets = kzalloc(max * sizeof(int), GFP_KERNEL); > + if (!cb->register_offsets) > + return -ENOMEM; Leaking cb, cb->crossbar_base, cb->crossbar_map, cb->domain, and cb->irq_map here. > + > + of_property_read_u32(node, "reg-size", &size); Sanity check? > + > + /* > + * Register offsets are not linear because of the > + * reserved irqs. so find and store the offsets once. > + */ > + for (i = 0; i < max; i++) { > + if (!cb->irq_map[i]) > + continue; > + > + cb->register_offsets[i] = reserved; > + reserved += size; > + } > + > + switch (size) { > + case 1: > + cb->write = crossbar_writeb; > + break; > + case 2: > + cb->write = crossbar_writew; > + break; > + case 4: > + cb->write = crossbar_writel; > + break; > + default: > + break; Perform cleanup and return -EINVAL here? > + } > + > + return 0; > +} > +IRQCHIP_DECLARE(crossbar, "crossbar-irqchip", crossbar_of_init); > -- > 1.7.9.5 Thanks, Mark. -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Mark, On Friday 20 September 2013 02:28 PM, Mark Rutland wrote: > Hi, > > I have a few comments, mostly on the DT binding and parsing. > Thanks for the review. The idea of seeing the crossbar as a new IRQCHIP itself did not go and the latest direction on this was to handle it inside the GIC. http://www.spinics.net/lists/linux-omap/msg97085.html I am working on that now. I would have agreed with most of the comments below, otherwise. >> diff --git a/Documentation/devicetree/bindings/arm/omap/irq-crossbar.txt b/Documentation/devicetree/bindings/arm/omap/irq-crossbar.txt >> new file mode 100644 >> index 0000000..5d465cf >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/arm/omap/irq-crossbar.txt >> @@ -0,0 +1,39 @@ >> +* IRQ CROSSBAR >> + >> +Some socs have a large number of interrupts requests to service >> +the needs of its many peripherals and subsystems. All of the >> +interrupt lines from the subsystems are not needed at the same >> +time, so they have to be muxed to the irq-controller appropriately. >> +In such places a interrupt controllers are preceded by an CROSSBAR >> +that provides flexibility in muxing the device requests to the controller >> +inputs. >> + >> +Required properties: >> +- compatible : Should be "irq-crossbar" > Missing vendor prefix, this should be something like "ti,irq-crossbar". > Does this have a more specific name than CROSSBAR that can be used to > qualify it? yes, ti,irq-crossbar. Not sure if it can be called as anything generically apart from crossbar . >> +- interrupt-parent: phandle to crossbar's interrupt parent. >> +- interrupt-controller: Identifies the node as an interrupt controller. >> +- interrupt-cells: Should be the same value as the interrupt parent. > That doesn't make sense. The crossbar driver is necessarily interpreting > these cells in a way the parent won't (as it supports more interrupts). > What are the meaning of these cells? These properties were added so that the DT code identifies this as a interrupt controller and map the children's irq in to this domain and to map the free irqs allocated in this driver to its parent. >> +- reg: Base address and the size of the crossbar registers. >> +- max-crossbar-lines: Total number of input lines of the crossbar. >> +- max-irqs: Total number of irqs available at the interrupt controller. > Is this the maximum number of interrupts targeting the parent interrupt > controller? Starting at what number, ending at what number? Can this > have gaps? > > Is this a shortcut so in the GIC case you don't have to describe up to > 160 interrupts? I can see why you don't want to, but there's a big loss > of generality here... > Yes, this was the maximum irqs available at the parent. The gaps was not considered here because it was mentioned used the below property irqs-reserved. >> +- reg-size: size of the crossbar registers. > As in the size of all the registers (the size component of reg)? > > Or is this the size of each individual register? Does that apply to all > registers or only a subset of them? > > What units are these in, bytes? > > What are valid sizes? > > Is this really that configurable? This was meant to describe the size a individual register and applied to all. This was used to choose the API's to write. But yes some more description could be made here. >> +- irqs-reserved: List of the reserved irq lines that are not muxed using >> + crossbar. These interrupt lines are reserved in the soc, >> + so crossbar bar driver should not consider them as free >> + lines. > Are these reserved inputs lines, or outputs to the parent interrupt > controller? > > What is the format of each entry in this list? > > The example seems to be a different format to the parent interrupt > controller (which per your binding also defined the crossbar's interrupt > format). While <0 1 2> is a valid interrupt per the GIC binding (SPI 0 > edge-triggered both ways), <3 5 6>, <131 132 139>, and <140 . .> are > not. These were parent's input lines that were not muxed from crossbar but directly connected from peripherals, so the driver should not consider it as a free line while allocating a irq. This property was meant to interpreted only in this driver. >> + >> +Examples: >> + crossbar_mpu: @4a020000 { >> + compatible = "irq-crossbar"; >> + interrupt-parent = <&gic>; >> + interrupt-controller; >> + #interrupt-cells = <3>; >> + reg = <0x4a002a48 0x130>; >> + max-crossbar-lines = <512>; >> + max-irqs = <160>; >> + reg-size = <2>; >> + irqs-reserved = <0 1 2 3 5 6 131 132 139 140>; >> + #address-cells = <1>; >> + #size-cells = <1>; > Why are there #address-cells and #size cells? This has no children, and > this affects any interrupt-map property (as the parent unit address now > must be a single cell, that isn't going to be used). > > [...] yes, they could have been dropped and simply inherit from parent. >> +static int crossbar_set_affinity(struct irq_data *d, >> + const struct cpumask *mask_val, >> + bool force) >> +{ >> + struct irq_chip *chip; >> + struct irq_data *data; >> + int ret = 0; >> + >> + crossbar_to_irq_chip_data(d->hwirq, &chip, &data); >> + >> + if (chip->irq_set_affinity) >> + ret = chip->irq_set_affinity(data, mask_val, force); >> + >> + return ret; > So if our parent chip can't set affinity, we pretend it can? > > irq_set_affinity in kernel/irq/manage.c returns -EINVAL if an irqchip > doesn't have irq_set_affinity. > Yes the return value should be corrected for the other case. >> +/* >> + * Request and free are already called in atomic contexts >> + */ >> +unsigned int crossbar_request_irq(struct irq_data *d) >> +{ >> + int cb_no = d->hwirq; >> + int virq = allocate_free_irq(cb_no); >> + void *irq = &cb->crossbar_map[cb_no].hwirq; >> + int err; >> + >> + err = request_threaded_irq(virq, crossbar_irq, NULL, >> + 0, "CROSSBAR", irq); >> + if (err) >> + pr_err("\n request_irq failed for crossbar %d", cb_no); > Why does the print begin with a newline rather than ending with one? > yes, should be in the end. >> + >> + return 0; >> +} > [...] > >> +static int crossbar_domain_xlate(struct irq_domain *d, >> + struct device_node *controller, >> + const u32 *intspec, unsigned int intsize, >> + unsigned long *out_hwirq, >> + unsigned int *out_type) >> +{ >> + int i, cb_no; >> + u32 *cb_intspec = kzalloc(intsize * sizeof(int), GFP_KERNEL); >> + >> + if (!cb_intspec) >> + return -ENOMEM; >> + >> + cb_no = intspec[1]; > So #interrupt-cells must be at least <2>. You should sanity check > intsize >= 2 before this line or you'll perform an illegal array access. > yes, that was missing. >> + >> + if (WARN_ON(intsize < 1)) >> + return -EINVAL; > This sanity check is both wrong and too late, as mentioned above. > >> + >> + cb->crossbar_map[cb_no].intspec = cb_intspec; >> + >> + /* >> + * Free irq is allocated and mapped during request_irq >> + * So just save the interrupt properties here >> + */ >> + for (i = 0; i < intsize; i++) >> + cb->crossbar_map[cb_no].intspec[i] = intspec[i]; >> + >> + cb->crossbar_map[cb_no].intspec_size = intsize; >> + *out_hwirq = intspec[1]; >> + *out_type = IRQ_TYPE_NONE; >> + >> + return 0; >> +} >> + >> +const struct irq_domain_ops crossbar_domain_ops = { >> + .map = crossbar_domain_map, >> + .xlate = crossbar_domain_xlate >> +}; >> + >> +static int __init crossbar_of_init(struct device_node *node, >> + struct device_node *parent) >> +{ >> + int i, size, max, reserved = 0; >> + const __be32 *irqsr; >> + >> + if (!parent) { >> + pr_err("\n interrupt-parent is missing"); > Another odd newline. correct. >> + return -ENODEV; >> + } >> + >> + cb = kzalloc(sizeof(struct cb_device *), GFP_KERNEL); >> + >> + if (!cb) >> + return -ENOMEM; >> + >> + cb->irqp = parent; >> + >> + cb->crossbar_base = of_iomap(node, 0); >> + if (!cb->crossbar_base) >> + return -ENOMEM; > Leaking allocated cb here. Agree here and other leaks mentioned below, free is missing. >> + >> + of_property_read_u32(node, "max-crossbar-lines", &max); >> + cb->crossbar_map = kzalloc(max * sizeof(struct pirqs *), GFP_KERNEL); >> + >> + if (!cb->crossbar_map) >> + return -ENOMEM; > Leaking cb and cb->crossbar_base mapping. > >> + >> + cb->domain = irq_domain_add_linear(node, max, >> + &crossbar_domain_ops, NULL); >> + >> + if (!cb->domain) { >> + pr_err("Couldn't register an IRQ domain\n"); >> + return -ENODEV; >> + } > Leaking cb, cb->crossbar_base, and cb->crossbar_map here. > >> + >> + of_property_read_u32(node, "max-irqs", &max); >> + cb->irq_map = kzalloc(max * sizeof(int), GFP_KERNEL); >> + if (!cb->irq_map) >> + return -ENOMEM; > Leaking cb, cb->crossbar_base, cb->crossbar_map, and cb->domain. > >> + >> + cb->int_max = max; >> + >> + for (i = 0; i < max; i++) >> + cb->irq_map[i] = IRQ_FREE; >> + >> + /* Get and mark reserved irqs */ >> + irqsr = of_get_property(node, "irqs-reserved", &size); >> + size /= sizeof(int); > The entries will be __be32, not int. > >> + >> + for (i = 0; i < size; i++) >> + cb->irq_map[be32_to_cpup(irqsr + i)] = 0; > No sanity check on array bounds? > > What about a dt that has something like: > > irqs-reserved = <0x0 0xcccccccc 0xffffffff>; > > It's clearly wrong, and we can detect that rather than bringing down the > kernel... yes, sanity check was required here. >> + >> + cb->register_offsets = kzalloc(max * sizeof(int), GFP_KERNEL); >> + if (!cb->register_offsets) >> + return -ENOMEM; > Leaking cb, cb->crossbar_base, cb->crossbar_map, cb->domain, and > cb->irq_map here. > >> + >> + of_property_read_u32(node, "reg-size", &size); > Sanity check? > >> + >> + /* >> + * Register offsets are not linear because of the >> + * reserved irqs. so find and store the offsets once. >> + */ >> + for (i = 0; i < max; i++) { >> + if (!cb->irq_map[i]) >> + continue; >> + >> + cb->register_offsets[i] = reserved; >> + reserved += size; >> + } >> + >> + switch (size) { >> + case 1: >> + cb->write = crossbar_writeb; >> + break; >> + case 2: >> + cb->write = crossbar_writew; >> + break; >> + case 4: >> + cb->write = crossbar_writel; >> + break; >> + default: >> + break; > Perform cleanup and return -EINVAL here? correct. >> + } >> + >> + return 0; >> +} >> +IRQCHIP_DECLARE(crossbar, "crossbar-irqchip", crossbar_of_init); >> -- >> 1.7.9.5 Regards, Sricharan -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/Documentation/devicetree/bindings/arm/omap/irq-crossbar.txt b/Documentation/devicetree/bindings/arm/omap/irq-crossbar.txt new file mode 100644 index 0000000..5d465cf --- /dev/null +++ b/Documentation/devicetree/bindings/arm/omap/irq-crossbar.txt @@ -0,0 +1,39 @@ +* IRQ CROSSBAR + +Some socs have a large number of interrupts requests to service +the needs of its many peripherals and subsystems. All of the +interrupt lines from the subsystems are not needed at the same +time, so they have to be muxed to the irq-controller appropriately. +In such places a interrupt controllers are preceded by an CROSSBAR +that provides flexibility in muxing the device requests to the controller +inputs. + +Required properties: +- compatible : Should be "irq-crossbar" +- interrupt-parent: phandle to crossbar's interrupt parent. +- interrupt-controller: Identifies the node as an interrupt controller. +- interrupt-cells: Should be the same value as the interrupt parent. +- reg: Base address and the size of the crossbar registers. +- max-crossbar-lines: Total number of input lines of the crossbar. +- max-irqs: Total number of irqs available at the interrupt controller. +- reg-size: size of the crossbar registers. +- irqs-reserved: List of the reserved irq lines that are not muxed using + crossbar. These interrupt lines are reserved in the soc, + so crossbar bar driver should not consider them as free + lines. + +Examples: + crossbar_mpu: @4a020000 { + compatible = "irq-crossbar"; + interrupt-parent = <&gic>; + interrupt-controller; + #interrupt-cells = <3>; + reg = <0x4a002a48 0x130>; + max-crossbar-lines = <512>; + max-irqs = <160>; + reg-size = <2>; + irqs-reserved = <0 1 2 3 5 6 131 132 139 140>; + #address-cells = <1>; + #size-cells = <1>; + }; + diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig index 4a33351..5152777 100644 --- a/drivers/irqchip/Kconfig +++ b/drivers/irqchip/Kconfig @@ -41,3 +41,12 @@ config VERSATILE_FPGA_IRQ_NR int default 4 depends on VERSATILE_FPGA_IRQ + +config IRQCHIP_CROSSBAR + bool + help + Those socs which has a crossbar IP to mux the irqs from peripherals + to the interrupt-controllers can use this. This driver would + dynamically allocate a free interrupt line and map the peripheral + crossbar input to the parent interrupt-controller. + diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile index cda4cb5..0033d93 100644 --- a/drivers/irqchip/Makefile +++ b/drivers/irqchip/Makefile @@ -16,3 +16,4 @@ obj-$(CONFIG_RENESAS_INTC_IRQPIN) += irq-renesas-intc-irqpin.o obj-$(CONFIG_RENESAS_IRQC) += irq-renesas-irqc.o obj-$(CONFIG_VERSATILE_FPGA_IRQ) += irq-versatile-fpga.o obj-$(CONFIG_ARCH_VT8500) += irq-vt8500.o +obj-$(CONFIG_IRQCHIP_CROSSBAR) += irq-crossbar.o diff --git a/drivers/irqchip/irq-crossbar.c b/drivers/irqchip/irq-crossbar.c new file mode 100644 index 0000000..3980502 --- /dev/null +++ b/drivers/irqchip/irq-crossbar.c @@ -0,0 +1,407 @@ +/* + * drivers/irqchip/irq-crossbar.c + * + * Copyright (C) 2013 Texas Instruments Incorporated - http://www.ti.com + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 as + * published by the Free Software Foundation. + * + */ +#include <linux/err.h> +#include <linux/init.h> +#include <linux/io.h> +#include <linux/irqdomain.h> +#include <linux/of_address.h> +#include <linux/of_irq.h> +#include <linux/platform_device.h> +#include <linux/interrupt.h> +#include <linux/irq.h> +#include <linux/slab.h> +#include "irqchip.h" + +#define IRQ_FREE -1 + +/* + * @hwirq: hardware irq line number + * @virq: linux irq corresponding to hwirq + * @intspec: interrupt specifier passed from DT + * @intspec_size: intspec size + */ +struct pirqs { + int hwirq; + int virq; + u32 *intspec; + int intspec_size; +}; + +/* + * @irq_map: array of interrupts to crossbar number mapping + * @crossbar_map: array of interrupt parent's virtual irqs + * @crossbar_base: crossbar base address + * @domain: crossbar domain ptr + * @irqp: pointer to crossbar irq parent + * @register_offsets: offsets for each irq number + * @int_max: maximum number of supported interrupts + */ +struct crossbar_device { + int *irq_map; + struct pirqs *crossbar_map; + void __iomem *crossbar_base; + struct irq_domain *domain; + struct device_node *irqp; + int *register_offsets; + int int_max; + void (*write) (int, int); +}; + +static struct crossbar_device *cb; +static struct lock_class_key crossbar_lock_class; + +static inline void crossbar_writel(int irq_no, int cb_no) +{ + writel(cb_no, cb->crossbar_base + cb->register_offsets[irq_no]); +} + +static inline void crossbar_writew(int irq_no, int cb_no) +{ + writew(cb_no, cb->crossbar_base + cb->register_offsets[irq_no]); +} + +static inline void crossbar_writeb(int irq_no, int cb_no) +{ + writeb(cb_no, cb->crossbar_base + cb->register_offsets[irq_no]); +} + +static inline int map_free_irq(int cb_no, int irq) +{ + int virq; + + cb->crossbar_map[cb_no].intspec[1] = irq; + + /* This maps the free_irq to the parent domain */ + virq = irq_create_of_mapping(cb->irqp, + (const u32 *)cb->crossbar_map[cb_no].intspec, + cb->crossbar_map[cb_no].intspec_size); + + cb->crossbar_map[cb_no].virq = virq; + cb->crossbar_map[cb_no].hwirq = irq; + + return virq; +} + +static inline const u32 allocate_free_irq(int cb_no) +{ + int i; + + for (i = 0; i < cb->int_max; i++) { + if (cb->irq_map[i] == IRQ_FREE) { + cb->irq_map[i] = cb_no; + cb->write(i, cb_no); + return map_free_irq(cb_no, i); + } + } + + return -ENODEV; +} + +static irqreturn_t crossbar_irq(int unused, + void *hwirq) +{ + int *irq = hwirq; + int cb_no = cb->irq_map[*irq]; + + generic_handle_irq(irq_find_mapping(cb->domain, cb_no)); + + return IRQ_HANDLED; +} + +static inline int crossbar_to_virq(int irq) +{ + return cb->crossbar_map[irq].virq; +} + +static inline int crossbar_to_irq(int irq) +{ + return cb->crossbar_map[irq].hwirq; +} + +static inline void crossbar_to_irq_chip_data(int irq, + struct irq_chip **chip, + struct irq_data **data) +{ + int virq; + + virq = crossbar_to_virq(irq); + *chip = irq_get_chip(virq); + *data = irq_get_irq_data(virq); +} + +/* + * Drivers can call these callbacks directly. + * So re-direct it to the parent chip. + */ +static void crossbar_eoi_irq(struct irq_data *d) +{ + struct irq_chip *chip; + struct irq_data *data; + + crossbar_to_irq_chip_data(d->hwirq, &chip, &data); + + if (chip->irq_eoi) + return chip->irq_eoi(data); +} + +static int crossbar_set_type(struct irq_data *d, unsigned int type) +{ + struct irq_chip *chip; + struct irq_data *data; + int ret = 0; + + crossbar_to_irq_chip_data(d->hwirq, &chip, &data); + + if (chip->irq_set_type) + ret = chip->irq_set_type(data, type); + + return ret; +} + +static void crossbar_unmask_irq(struct irq_data *d) +{ + struct irq_chip *chip; + struct irq_data *data; + + crossbar_to_irq_chip_data(d->hwirq, &chip, &data); + + if (chip->irq_unmask) + return chip->irq_unmask(data); +} + +static void crossbar_mask_irq(struct irq_data *d) +{ + struct irq_chip *chip; + struct irq_data *data; + + crossbar_to_irq_chip_data(d->hwirq, &chip, &data); + + if (chip->irq_mask) + return chip->irq_mask(data); +} + +#ifdef CONFIG_SMP +static int crossbar_set_affinity(struct irq_data *d, + const struct cpumask *mask_val, + bool force) +{ + struct irq_chip *chip; + struct irq_data *data; + int ret = 0; + + crossbar_to_irq_chip_data(d->hwirq, &chip, &data); + + if (chip->irq_set_affinity) + ret = chip->irq_set_affinity(data, mask_val, force); + + return ret; +} +#endif + +/* + * Request and free are already called in atomic contexts + */ +unsigned int crossbar_request_irq(struct irq_data *d) +{ + int cb_no = d->hwirq; + int virq = allocate_free_irq(cb_no); + void *irq = &cb->crossbar_map[cb_no].hwirq; + int err; + + err = request_threaded_irq(virq, crossbar_irq, NULL, + 0, "CROSSBAR", irq); + if (err) + pr_err("\n request_irq failed for crossbar %d", cb_no); + + return 0; +} + +void crossbar_free_irq(struct irq_data *d) +{ + void *irq = &cb->crossbar_map[d->hwirq].hwirq; + int hwirq = crossbar_to_irq(d->hwirq); + int virq = crossbar_to_virq(d->hwirq); + + free_irq(virq, irq); + cb->irq_map[hwirq] = IRQ_FREE; + irq_dispose_mapping(virq); + cb->crossbar_map[d->hwirq].virq = 0; +} + +static void crossbar_print_name(struct irq_data *d, struct seq_file *p) +{ + struct irq_chip *chip; + struct irq_data *data; + struct irqaction *action; + + crossbar_to_irq_chip_data(d->hwirq, &chip, &data); + action = irq_to_desc(data->irq)->action; + + seq_printf(p, " %8s", d->chip->name); + + /* interrupt proc entries look better this way */ + while (action != NULL) { + action->name = irq_to_desc(d->irq)->action->name; + action = action->next; + } +} + +static struct irq_chip crossbar_chip = { + .name = "CROSSBAR", + .irq_startup = crossbar_request_irq, + .irq_shutdown = crossbar_free_irq, + .irq_unmask = crossbar_unmask_irq, + .irq_mask = crossbar_mask_irq, + .irq_eoi = crossbar_eoi_irq, + .irq_set_type = crossbar_set_type, +#ifdef CONFIG_SMP + .irq_set_affinity = crossbar_set_affinity, +#endif + .irq_print_chip = crossbar_print_name +}; + +static int crossbar_domain_map(struct irq_domain *d, unsigned int irq, + irq_hw_number_t hw) +{ + irq_set_lockdep_class(irq, &crossbar_lock_class); + irq_set_chip_and_handler(irq, &crossbar_chip, + handle_simple_irq); + + set_irq_flags(irq, IRQF_VALID | IRQF_PROBE); + return 0; +} + +static int crossbar_domain_xlate(struct irq_domain *d, + struct device_node *controller, + const u32 *intspec, unsigned int intsize, + unsigned long *out_hwirq, + unsigned int *out_type) +{ + int i, cb_no; + u32 *cb_intspec = kzalloc(intsize * sizeof(int), GFP_KERNEL); + + if (!cb_intspec) + return -ENOMEM; + + cb_no = intspec[1]; + + if (WARN_ON(intsize < 1)) + return -EINVAL; + + cb->crossbar_map[cb_no].intspec = cb_intspec; + + /* + * Free irq is allocated and mapped during request_irq + * So just save the interrupt properties here + */ + for (i = 0; i < intsize; i++) + cb->crossbar_map[cb_no].intspec[i] = intspec[i]; + + cb->crossbar_map[cb_no].intspec_size = intsize; + *out_hwirq = intspec[1]; + *out_type = IRQ_TYPE_NONE; + + return 0; +} + +const struct irq_domain_ops crossbar_domain_ops = { + .map = crossbar_domain_map, + .xlate = crossbar_domain_xlate +}; + +static int __init crossbar_of_init(struct device_node *node, + struct device_node *parent) +{ + int i, size, max, reserved = 0; + const __be32 *irqsr; + + if (!parent) { + pr_err("\n interrupt-parent is missing"); + return -ENODEV; + } + + cb = kzalloc(sizeof(struct cb_device *), GFP_KERNEL); + + if (!cb) + return -ENOMEM; + + cb->irqp = parent; + + cb->crossbar_base = of_iomap(node, 0); + if (!cb->crossbar_base) + return -ENOMEM; + + of_property_read_u32(node, "max-crossbar-lines", &max); + cb->crossbar_map = kzalloc(max * sizeof(struct pirqs *), GFP_KERNEL); + + if (!cb->crossbar_map) + return -ENOMEM; + + cb->domain = irq_domain_add_linear(node, max, + &crossbar_domain_ops, NULL); + + if (!cb->domain) { + pr_err("Couldn't register an IRQ domain\n"); + return -ENODEV; + } + + of_property_read_u32(node, "max-irqs", &max); + cb->irq_map = kzalloc(max * sizeof(int), GFP_KERNEL); + if (!cb->irq_map) + return -ENOMEM; + + cb->int_max = max; + + for (i = 0; i < max; i++) + cb->irq_map[i] = IRQ_FREE; + + /* Get and mark reserved irqs */ + irqsr = of_get_property(node, "irqs-reserved", &size); + size /= sizeof(int); + + for (i = 0; i < size; i++) + cb->irq_map[be32_to_cpup(irqsr + i)] = 0; + + cb->register_offsets = kzalloc(max * sizeof(int), GFP_KERNEL); + if (!cb->register_offsets) + return -ENOMEM; + + of_property_read_u32(node, "reg-size", &size); + + /* + * Register offsets are not linear because of the + * reserved irqs. so find and store the offsets once. + */ + for (i = 0; i < max; i++) { + if (!cb->irq_map[i]) + continue; + + cb->register_offsets[i] = reserved; + reserved += size; + } + + switch (size) { + case 1: + cb->write = crossbar_writeb; + break; + case 2: + cb->write = crossbar_writew; + break; + case 4: + cb->write = crossbar_writel; + break; + default: + break; + } + + return 0; +} +IRQCHIP_DECLARE(crossbar, "crossbar-irqchip", crossbar_of_init);
Some socs have a large number of interrupts requests to service the needs of its many peripherals and subsystems. All of the interrupt lines from the subsystems are not needed at the same time, so they have to be muxed to the irq-controller appropriately. In such places a interrupt controllers are preceded by an CROSSBAR that provides flexibility in muxing the device requests to the controller inputs. This models the crossbar IP as a cascaded irqchip controller. The peripheral crossbar inputs are mapped on to the crossbar irq-domain. The driver then allocates a 'free' irq line and maps that to the actual interrupt controller's domain. So every external peripheral interrupt is routed through the crossbar handler. GIC <----- CROSSBAR <----- PERIPHERAL INTERRUPT LINES peripheral's irq_of_parse_and_map() | | crossbar_xlate() | | saves the interrupt properties passed peripheral's request_irq(crossbar_number) | | crossbar_request_irq | | allocates free irq and maps it to parent domain | | request_irq(mapped interrupt number) gic_interrupt_hanadler | | crossbar_irq(interrupt number) | | get crossbar number from interrupt number | | handle_irq(crossbar_domain(crossbar number)) The irqchip callback hooks added here are just a redirection to the parent irqchip. This adds a extra translation in the fast path. The maximum increase in the average interrupt latency due to the same was measured as around 1.63us on a cpu running at 1GHZ. cat /proc/interrupts looks like this, with both crossbar and interrupt number CPU0 CPU1 45: 267 0 GIC OMAP UART0 205: 267 0 CROSSBAR OMAP UART0 Cc: Thomas Gleixner <tglx@linutronix.de> Cc: Linus Walleij <linus.walleij@linaro.org> Cc: Santosh Shilimkar <santosh.shilimkar@ti.com> Cc: Russell King <linux@arm.linux.org.uk> Cc: Tony Lindgren <tony@atomide.com> Cc: Rajendra Nayak <rnayak@ti.com> Signed-off-by: Sricharan R <r.sricharan@ti.com> --- There is lockdep warning during the boot. This is because we try to do one request_irq with in another and that results in kmalloc being called from an atomic context, which generates the warning. Any suggestions to overcome this will help. WARNING: at kernel/lockdep.c:2740 lockdep_trace_alloc+0xe8/0x108() DEBUG_LOCKS_WARN_ON(irqs_disabled_flags(flags)) .../devicetree/bindings/arm/omap/irq-crossbar.txt | 39 ++ drivers/irqchip/Kconfig | 9 + drivers/irqchip/Makefile | 1 + drivers/irqchip/irq-crossbar.c | 407 ++++++++++++++++++++ 4 files changed, 456 insertions(+) create mode 100644 Documentation/devicetree/bindings/arm/omap/irq-crossbar.txt create mode 100644 drivers/irqchip/irq-crossbar.c