Re: [PATCH] ARM: smp: Allow real broadcast device selection instead of always dummy
On Wed, 13 Mar 2013, Santosh Shilimkar wrote: On Wednesday 13 March 2013 07:49 PM, Thomas Gleixner wrote: Though making the rating of the dummy lower is definitely a good thing, so a real hardware device which is detected later can replace the dummy device. So yes, the rating should be low for the dummy timer. Exactly. As Mark pointed out, you have already applied Mark's patch. I was just wondering whether you can take the $subject patch as well with ack from Russell, Mark. It can go through the ARM tree as well, no dependencies on my stuff. Thanks, tglx -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] ARM: smp: Allow real broadcast device selection instead of always dummy
On Thu, 14 Mar 2013, Santosh Shilimkar wrote: On Wednesday 13 March 2013 09:48 PM, Mark Rutland wrote: On Wed, Mar 13, 2013 at 03:44:03PM +, Santosh Shilimkar wrote: On Wednesday 13 March 2013 05:55 PM, Mark Rutland wrote: On Wed, Mar 13, 2013 at 11:24:01AM +, Santosh Shilimkar wrote: On Wednesday 13 March 2013 03:46 PM, Mark Rutland wrote: Hi Santosh, [..] Is the problem that the dummy timer is being registered as the broadcast source, or that it is selected as a local timer in preference of real timers? Dummy timer is preferred over real broadcast timer. Ok. [...] I do agree it'd be worth lowering the dummy timer's rating to ensure it doesn't override a real timer elsewhere. Yep. Can I add you acked-by tag then for $subject patch ? Would be good to get this one merged as well. Sure. Though could you reword the commit message? The patch solves the more general issue of a dummy being preferred over real hardware even outside of choosing the broadcast device. Acked-by: Mark Rutland mark.rutl...@arm.com Thanks. For record, patch is in end of the email which I plan to put into patch system. Regards, Santosh From 57c501bcdc88c7ff26a5c63956be07e94a5083c5 Mon Sep 17 00:00:00 2001 From: Santosh Shilimkar santosh.shilim...@ti.com Date: Wed, 13 Mar 2013 12:33:16 +0530 Subject: [PATCH 1/2] ARM: smp: Avoid dummy clockevent being preferred over real hardware clock-event With recent arm broadcast time clean-up from Mark Rutland, the dummy broadcast device is always registered with timer subsystem. And since the rating of the dummy clock event is very high, it may be preferred over a real clock event. This is a change in behavior from past and not an intended one. So reduce the rating of the dummy clockevent so that real clockevent device is selected when available. Acked-by: Mark Rutland mark.rutl...@arm.com Acked-by: Thomas Gleixner t...@linutronix.de Signed-off-by: Santosh Shilimkar santosh.shilim...@ti.com --- arch/arm/kernel/smp.c |2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/arch/arm/kernel/smp.c b/arch/arm/kernel/smp.c index 31644f1..79078ed 100644 --- a/arch/arm/kernel/smp.c +++ b/arch/arm/kernel/smp.c @@ -480,7 +480,7 @@ static void __cpuinit broadcast_timer_setup(struct clock_event_device *evt) evt-features = CLOCK_EVT_FEAT_ONESHOT | CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_DUMMY; - evt-rating = 400; + evt-rating = 100; evt-mult = 1; evt-set_mode = broadcast_timer_set_mode; -- 1.7.9.5 -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH 1/4] DRIVERS: IRQCHIP: Add crossbar irqchip driver
On Thu, 12 Sep 2013, Sricharan R wrote: Signed-off-by: Sricharan R r.sricha...@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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH 1/4] DRIVERS: IRQCHIP: Add crossbar irqchip driver
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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH 1/4] DRIVERS: IRQCHIP: Add crossbar irqchip driver
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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH 1/4] DRIVERS: IRQCHIP: Add crossbar irqchip driver
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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH 1/4] DRIVERS: IRQCHIP: Add crossbar irqchip driver
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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH 1/4] DRIVERS: IRQCHIP: Add crossbar irqchip driver
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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH 1/4] DRIVERS: IRQCHIP: Add crossbar irqchip driver
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 t...@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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH 1/4] DRIVERS: IRQCHIP: Add crossbar irqchip driver
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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH 0/6] DRIVERS: IRQCHIP: Add support for crossbar IP
On Tue, 1 Oct 2013, Rob Herring wrote: On 10/01/2013 06:13 AM, Sricharan R wrote: Is there an actual usecase on a single h/w design that you run out of interrupts and it is a user decision which interrupts to use? I don't think that matters. What matters is that you have a single DT entry which tells the driver which crossbar irq to use for that particular device. And that is the only information which is relevant because the IP block is connected to a crossbar input and not to a GIC line. The crossbar provides the mapping and this is really best done at runtime w/o having hardcoded information in the DT or at some other place. You could fill in the interrupt-map at run-time. It would have to be early (bootloader or early kernel init) and can't be at request_irq time. How is that supposed to work when you have no idea at early boot time which particular IP blocks are going to be used later on? http://www.spinics.net/lists/linux-omap/msg97085.html This has nothing to do with the GIC, so it does not belong there. Errm. crossbar is a mapping device which happens to sit between the GIC and the IP blocks. So how is this NOT related to GIC ? Thanks, tglx -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH 1/6] DRIVERS: IRQCHIP: IRQ-GIC: Add support for routable irqs
On Mon, 30 Sep 2013, Sricharan R wrote:
diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
index 1760ceb..c5778ab 100644
--- a/drivers/irqchip/irq-gic.c
+++ b/drivers/irqchip/irq-gic.c
@@ -72,6 +72,8 @@ struct gic_chip_data {
static DEFINE_RAW_SPINLOCK(irq_controller_lock);
+const struct irq_domain_ops *gic_routable_irq_domain_ops;
+
/*
* The GIC mapping of CPU interfaces does not necessarily match
* the logical CPU numbering. Let's use a mapping as returned
@@ -675,11 +677,26 @@ static int gic_irq_domain_map(struct irq_domain *d,
unsigned int irq,
irq_set_chip_and_handler(irq, gic_chip,
handle_fasteoi_irq);
set_irq_flags(irq, IRQF_VALID | IRQF_PROBE);
+
+ if (gic_routable_irq_domain_ops
+ gic_routable_irq_domain_ops-map)
+ gic_routable_irq_domain_ops-map(d, irq, hw);
Shudder. Why are you sprinkling these if (ops ops-fun)
conditionals all over the place instead of having a default ops
implementation which handles the non crossbar case by proper empty
functions. That code is not on a hot path so it does not matter at
all.
}
irq_set_chip_data(irq, d-host_data);
return 0;
}
+static void gic_irq_domain_unmap(struct irq_domain *d, unsigned int irq)
+{
+ irq_hw_number_t hw = irq_get_irq_data(irq)-hwirq;
+
+ if (hw 32) {
Groan. This wants to be in the ops-unmap function. It's not related
to the GIC core code.
+ if (gic_routable_irq_domain_ops
+ gic_routable_irq_domain_ops-unmap)
+ gic_routable_irq_domain_ops-unmap(d, irq);
+ }
+}
+
static int gic_irq_domain_xlate(struct irq_domain *d,
struct device_node *controller,
const u32 *intspec, unsigned int intsize,
@@ -694,8 +711,15 @@ static int gic_irq_domain_xlate(struct irq_domain *d,
*out_hwirq = intspec[1] + 16;
/* For SPIs, we need to add 16 more to get the GIC irq ID number */
- if (!intspec[0])
- *out_hwirq += 16;
+ if (!intspec[0]) {
+ if (gic_routable_irq_domain_ops
+ gic_routable_irq_domain_ops-xlate)
+ *out_hwirq = gic_routable_irq_domain_ops-xlate(d,
+ controller, intspec, intsize,
+ out_hwirq, out_type);
+ else
+ *out_hwirq += 16;
+ }
So if you have a default xlate ops implementation then this boils down to
if (!intspec[0])
*out_hwirq = routing_ops-xlate()
And the default (non crossbar) implementation would be:
return *out_hwirq + 16;
Thanks,
tglx
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH 2/6] DRIVERS: IRQCHIP: CROSSBAR: Add support for Crossbar IP
On Mon, 30 Sep 2013, Sricharan R wrote:
+/*
+ * @int_max: maximum number of supported interrupts
+ * @irq_map: array of interrupts to crossbar number mapping
+ * @crossbar_base: crossbar base address
+ * @register_offsets: offsets for each irq number
+ */
+struct crossbar_device {
+ uint int_max;
+ uint *irq_map;
Why do you need another map here?
Isn't the linear_revmap of the irqdomain sufficient?
+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;
+ return i;
+ }
+ }
+
+ return -ENODEV;
+}
+
+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)
+{
+ return allocate_free_irq(intspec[1]) + GIC_IRQ_START;
Mooo. In the error case you return:
-ENODEV + GIC_IRQ_START == -19 + 32 == 13
Yikes.
+
+ /*
+ * 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;
I'm amazed by such a brilliant hardware design.
Thanks,
tglx
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V2 1/7] DRIVERS: IRQCHIP: IRQ-GIC: Add support for routable irqs
On Wed, 30 Oct 2013, Sricharan R wrote:
@@ -700,11 +709,22 @@ static int gic_irq_domain_xlate(struct irq_domain *d,
*out_hwirq = intspec[1] + 16;
/* For SPIs, we need to add 16 more to get the GIC irq ID number */
- if (!intspec[0])
+ if (!intspec[0]) {
*out_hwirq += 16;
Minor nit. This should be in the default implementation. The crossbar
implementation will fill out_hwirq in its own way and is not
interested in the +16 operation at all.
+ ret = gic_routable_irq_domain_ops-xlate(d, controller,
+ intspec,
+ intsize,
+ out_hwirq,
+ out_type);
+
+ gic-domain = irq_domain_add_legacy(node, gic_irqs, irq_base,
+ hwirq_base, gic_irq_domain_ops, gic);
+ } else {
+ if (WARN_ON(!gic_routable_irq_domain_ops))
+ return;
This warning is pointless, because you have default ops now.
+
+ gic-domain = irq_domain_add_linear(node, nr_routable_irqs,
+ gic_irq_domain_ops,
+ gic);
}
Thanks,
tglx
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V2 2/7] DRIVERS: IRQCHIP: CROSSBAR: Add support for Crossbar IP
On Wed, 30 Oct 2013, Sricharan R wrote:
+static inline const u32 allocate_free_irq(int cb_no)
I understand the static inline part, but const u32 is more than
fishy. What's wrong with static inline int ?
+{
+ int i;
+
+ for (i = 0; i cb-int_max; i++) {
+ if (cb-irq_map[i] == IRQ_FREE) {
+ cb-irq_map[i] = cb_no;
+ return i;
+ }
+ }
+
+ return -ENODEV;
+}
+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)
+{
+ unsigned long ret = 0;
Why do you need to initialize ret when you assign a value to it in the
next line?
Thanks,
tglx
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V4 1/4] DRIVERS: IRQCHIP: IRQ-GIC: Add support for routable irqs
On Thu, 14 Nov 2013, Sricharan R wrote: [V3] Addressed unnecessary warn-on and updated default xlate function as per Thomas Gleixner comments Reviewed-by: Thomas Gleixner t...@linutronix.de -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V5 2/4] DRIVERS: IRQCHIP: CROSSBAR: Add support for Crossbar IP
On Mon, 3 Feb 2014, Sricharan R wrote: I already have your reviewed-by tag for the first patch in this series. Kevin was pointing out that irqchip maintainer tag is needed for this patch as well to be merged. We are planning to take this series through arm-soc tree. Can i please have your tag for this patch as well ? Acked-by-me -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[patch 08/26] arm: Replace various irq_desc accesses
Use the proper functions. There is no need to fiddle with irq_desc.
Signed-off-by: Thomas Gleixner t...@linutronix.de
Cc: Shawn Guo shawn@linaro.org
Cc: arm linux-arm-ker...@lists.infradead.org
Cc: omap linux-omap@vger.kernel.org
Cc: Tony Lindgren t...@atomide.com
Cc: Russell King rmk+ker...@arm.linux.org.uk
---
arch/arm/mach-imx/pm-imx6q.c|7 +++
arch/arm/mach-omap1/ams-delta-fiq.c |7 ++-
2 files changed, 5 insertions(+), 9 deletions(-)
Index: tip/arch/arm/mach-imx/pm-imx6q.c
===
--- tip.orig/arch/arm/mach-imx/pm-imx6q.c
+++ tip/arch/arm/mach-imx/pm-imx6q.c
@@ -120,7 +120,7 @@ static void imx6q_enable_wb(bool enable)
int imx6q_set_lpm(enum mxc_cpu_pwr_mode mode)
{
- struct irq_desc *iomuxc_irq_desc;
+ struct irq_data *iomuxc_irq_data = irq_get_irq_data(32);
u32 val = readl_relaxed(ccm_base + CLPCR);
val = ~BM_CLPCR_LPM;
@@ -167,10 +167,9 @@ int imx6q_set_lpm(enum mxc_cpu_pwr_mode
* 3) Software should mask IRQ #32 right after CCM Low-Power mode
*is set (set bits 0-1 of CCM_CLPCR).
*/
- iomuxc_irq_desc = irq_to_desc(32);
- imx_gpc_irq_unmask(iomuxc_irq_desc-irq_data);
+ imx_gpc_irq_unmask(iomuxc_irq_data);
writel_relaxed(val, ccm_base + CLPCR);
- imx_gpc_irq_mask(iomuxc_irq_desc-irq_data);
+ imx_gpc_irq_mask(iomuxc_irq_data);
return 0;
}
Index: tip/arch/arm/mach-omap1/ams-delta-fiq.c
===
--- tip.orig/arch/arm/mach-omap1/ams-delta-fiq.c
+++ tip/arch/arm/mach-omap1/ams-delta-fiq.c
@@ -44,13 +44,10 @@ static unsigned int irq_counter[16];
static irqreturn_t deferred_fiq(int irq, void *dev_id)
{
- struct irq_desc *irq_desc;
- struct irq_chip *irq_chip = NULL;
int gpio, irq_num, fiq_count;
+ struct irq_chip *irq_chip;
- irq_desc = irq_to_desc(gpio_to_irq(AMS_DELTA_GPIO_PIN_KEYBRD_CLK));
- if (irq_desc)
- irq_chip = irq_desc-irq_data.chip;
+ irq_chip = irq_get_irq_chip(gpio_to_irq(AMS_DELTA_GPIO_PIN_KEYBRD_CLK));
/*
* For each handled GPIO interrupt, keep calling its interrupt handler
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 00/14] irqchip: crossbar: driver fixes
On Tue, 3 Jun 2014, Sricharan R wrote: Please Cc all maintainers on such changes plus the relevant mailinglist. See MAINTAINERS. Thanks, tglx -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC] irqchip: gic: always mask interrupts during suspend
On Tue, 10 Jun 2014, Brian Norris wrote: Other random thought: it seems like any irqchip driver which does lazy IRQ masking ought to use IRQCHIP_MASK_ON_SUSPEND. So maybe the IRQ core should just do something like: if (!chip-irq_disable) chip-flags |= IRQCHIP_MASK_ON_SUSPEND; No. Lazy irq disable and the suspend logic are different beasts. That's up to the platform to decide this. Just for the record: there is a world outside of ARM... But that brings me to a different question: Why are you not putting that customization into the device tree instead of trying to add this to some random arch/arm/mach-foo files? Thanks, tglx -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V2 00/19] irqchip: crossbar: driver fixes
On Fri, 13 Jun 2014, Jason Cooper wrote: On Fri, Jun 13, 2014 at 09:48:24AM -0700, Joe Perches wrote: On Fri, 2014-06-13 at 12:37 -0400, Jason Cooper wrote: On Fri, Jun 13, 2014 at 09:14:34AM -0700, Joe Perches wrote: On Fri, 2014-06-13 at 11:01 -0400, Jason Cooper wrote: Please format the subject lines like so: irqchip: crossbar: Set cb pointer ... ^ | \-- note the capitalization I suggest you don't make this a rule and focus on more important stuff instead. Says the one, who pesters people about typos in changelogs WTF? Jason is doing a great job in reviewing and he knows what to concentrate on. It's not my rule. ;-) Who's rule is it then? Thomas' Sentences start with an upper case letter. Our brain is trained on that rule when parsing a line. So for people who actually review patches by reading them instead of running a spell checker, consistent formatting more important than avoiding the random typo, which our brain just blends out in most of the cases. Unfortunately also when the typo is in actual code :( Thanks, tglx -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 00/26] genirq: fix use of irq_find_mapping outside of legal RCU context
On Tue, 26 Aug 2014, Marc Zyngier wrote:
A number of irqchip drivers are directly calling irq_find_mapping,
which may use a rcu_read_lock call when walking the radix tree.
Turns out that if you hit that point with CONFIG_PROVE_RCU enabled,
the kernel will shout at you, as using RCU in this context may be
illegal (specially if coming from the idle state, where RCU would be
in a quiescent state).
A possible fix would be to wrap calls to irq_find_mapping into a
RCU_NONIDLE macro, but that really looks ugly.
This patch series introduce another generic IRQ entry point
(handle_domain_irq), which has the exact same behaviour as handle_IRQ
(as defined on arm, arm64 and openrisc), except that it also takes a
irq_domain pointer. This allows the logical IRQ lookup to be done
inside the irq_{enter,exit} section, which contains a
rcu_irq_{enter,exit}, making it safe.
Looks good. Should this be routed to the genirq tree?
Thanks,
tglx
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 00/26] genirq: fix use of irq_find_mapping outside of legal RCU context
On Wed, 3 Sep 2014, Jason Cooper wrote:
On Wed, Aug 27, 2014 at 10:33:44AM +0100, Marc Zyngier wrote:
Hi Thomas,
On Tue, Aug 26 2014 at 10:34:51 pm BST, Thomas Gleixner
t...@linutronix.de wrote:
On Tue, 26 Aug 2014, Marc Zyngier wrote:
A number of irqchip drivers are directly calling irq_find_mapping,
which may use a rcu_read_lock call when walking the radix tree.
Turns out that if you hit that point with CONFIG_PROVE_RCU enabled,
the kernel will shout at you, as using RCU in this context may be
illegal (specially if coming from the idle state, where RCU would be
in a quiescent state).
A possible fix would be to wrap calls to irq_find_mapping into a
RCU_NONIDLE macro, but that really looks ugly.
This patch series introduce another generic IRQ entry point
(handle_domain_irq), which has the exact same behaviour as handle_IRQ
(as defined on arm, arm64 and openrisc), except that it also takes a
irq_domain pointer. This allows the logical IRQ lookup to be done
inside the irq_{enter,exit} section, which contains a
rcu_irq_{enter,exit}, making it safe.
Looks good. Should this be routed to the genirq tree?
I'm happy for you to take this series, provided the architecture
maintainers agree on it (I'm still to hear from the openrisc guys, and
their mailing-list seems to positively hate my guts).
I think everyone's had a chance to look over it by now. Thomas, shall I
take the series?
Yes please.
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 00/26] genirq: fix use of irq_find_mapping outside of legal RCU context
On Wed, 3 Sep 2014, Marc Zyngier wrote:
[Dropping li...@openrisc.net from the CC list]
On 03/09/14 13:09, Thomas Gleixner wrote:
On Wed, 3 Sep 2014, Jason Cooper wrote:
On Wed, Aug 27, 2014 at 10:33:44AM +0100, Marc Zyngier wrote:
Hi Thomas,
On Tue, Aug 26 2014 at 10:34:51 pm BST, Thomas Gleixner
t...@linutronix.de wrote:
On Tue, 26 Aug 2014, Marc Zyngier wrote:
A number of irqchip drivers are directly calling irq_find_mapping,
which may use a rcu_read_lock call when walking the radix tree.
Turns out that if you hit that point with CONFIG_PROVE_RCU enabled,
the kernel will shout at you, as using RCU in this context may be
illegal (specially if coming from the idle state, where RCU would be
in a quiescent state).
A possible fix would be to wrap calls to irq_find_mapping into a
RCU_NONIDLE macro, but that really looks ugly.
This patch series introduce another generic IRQ entry point
(handle_domain_irq), which has the exact same behaviour as handle_IRQ
(as defined on arm, arm64 and openrisc), except that it also takes a
irq_domain pointer. This allows the logical IRQ lookup to be done
inside the irq_{enter,exit} section, which contains a
rcu_irq_{enter,exit}, making it safe.
Looks good. Should this be routed to the genirq tree?
I'm happy for you to take this series, provided the architecture
maintainers agree on it (I'm still to hear from the openrisc guys, and
their mailing-list seems to positively hate my guts).
I think everyone's had a chance to look over it by now. Thomas, shall I
take the series?
Yes please.
Do you want a pull request? Or are you picking up the patches from the ML?
Did you pick up the Acked/Reviewed/Ignored-by tags?
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V2 3/3] mfd: palmas: Add support for optional wakeup
On Fri, 5 Sep 2014, Nishanth Menon wrote: + if (!palmas-wakeirq) + goto no_wake_irq; + + ret = devm_request_irq(palmas-dev, palmas-wakeirq, +palmas_wake_irq, +IRQF_ONESHOT | pdata-irq_flags, Why is this marked IRQF_ONESHOT? +dev_name(palmas-dev), +palmas); + if (ret 0) + goto err_i2c; Why err and not doing the obvious clearing of palmas-wakeirq and keep at least the i2c functional? Thanks, tglx -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V3 3/3] mfd: palmas: Add support for optional wakeup
On Thu, 18 Sep 2014, Nishanth Menon wrote:
+static irqreturn_t palmas_wake_irq(int irq, void *_palmas)
+{
+ /*
+ * Return Not handled so that interrupt is disabled.
And how is that interrupt disabled by returning IRQ_NONE? You mean it
gets disabled after it got reraised 10 times and the spurious
detector kills it?
+ * Level event ensures that the event is eventually handled
+ * by the appropriate chip handler already registered
Eventually handled? So eventually it's not handled?
+ */
+ return IRQ_NONE;
+}
+
int palmas_ext_control_req_config(struct palmas *palmas,
enum palmas_external_requestor_id id, int ext_ctrl, bool enable)
{
@@ -409,6 +420,7 @@ static void palmas_dt_to_pdata(struct i2c_client *i2c,
pdata-mux_from_pdata = 1;
pdata-pad2 = prop;
}
+ pdata-wakeirq = irq_of_parse_and_map(node, 1);
/* The default for this register is all masked */
ret = of_property_read_u32(node, ti,power-ctrl, prop);
@@ -521,6 +533,7 @@ static int palmas_i2c_probe(struct i2c_client *i2c,
i2c_set_clientdata(i2c, palmas);
palmas-dev = i2c-dev;
palmas-irq = i2c-irq;
+ palmas-wakeirq = pdata-wakeirq;
match = of_match_device(of_palmas_match_tbl, i2c-dev);
@@ -587,6 +600,25 @@ static int palmas_i2c_probe(struct i2c_client *i2c,
if (ret 0)
goto err_i2c;
+ if (!palmas-wakeirq)
+ goto no_wake_irq;
+
+ ret = devm_request_irq(palmas-dev, palmas-wakeirq,
+palmas_wake_irq,
+pdata-irq_flags,
+dev_name(palmas-dev),
+palmas);
+ if (ret 0) {
+ dev_err(palmas-dev, Invalid wakeirq(%d) (res: %d), skiping\n,
+ palmas-wakeirq, ret);
+ palmas-wakeirq = 0;
+ } else {
+ /* We use wakeirq only during suspend-resume path */
+ device_set_wakeup_capable(palmas-dev, true);
+ disable_irq_nosync(palmas-wakeirq);
Urgh. Why nosysnc? And why do you want to do that at all?
irq_set_status_flags(irq, IRQ_NOAUTOEN);
Is what you want to set before requesting the irq.
+ }
+
+no_wake_irq:
no_irq:
slave = PALMAS_BASE_TO_SLAVE(PALMAS_PU_PD_OD_BASE);
addr = PALMAS_BASE_TO_REG(PALMAS_PU_PD_OD_BASE,
@@ -706,6 +738,34 @@ static int palmas_i2c_remove(struct i2c_client *i2c)
return 0;
}
+static int palmas_i2c_suspend(struct i2c_client *i2c, pm_message_t mesg)
+{
+ struct palmas *palmas = i2c_get_clientdata(i2c);
+ struct device *dev = i2c-dev;
+
+ if (!palmas-wakeirq)
+ return 0;
+
+ if (device_may_wakeup(dev))
+ enable_irq(palmas-wakeirq);
+
+ return 0;
+}
+
+static int palmas_i2c_resume(struct i2c_client *i2c)
+{
+ struct palmas *palmas = i2c_get_clientdata(i2c);
+ struct device *dev = i2c-dev;
+
+ if (!palmas-wakeirq)
+ return 0;
+
+ if (device_may_wakeup(dev))
+ disable_irq_nosync(palmas-wakeirq);
Again, why nosync?
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V3 3/3] mfd: palmas: Add support for optional wakeup
On Thu, 18 Sep 2014, Nishanth Menon wrote:
On 17:57-20140918, Thomas Gleixner wrote:
I suppose I can improve the commit message to elaborate this better?
Will that help?
You also want to improve the comment in the empty handler.
+ */
+ return IRQ_NONE;
And it still does not explain WHY you think that returning IRQ_NONE is
the right thing to do here. You actually handle the interrupt, right?
Just because the handler is an NOP does not mean you did not handle
it.
+static int palmas_i2c_suspend(struct i2c_client *i2c, pm_message_t mesg)
+{
+ struct palmas *palmas = i2c_get_clientdata(i2c);
+ struct device *dev = i2c-dev;
+
+ if (!palmas-wakeirq)
+ return 0;
+
+ if (device_may_wakeup(dev))
+ enable_irq(palmas-wakeirq);
+
+ return 0;
+}
+
+static int palmas_i2c_resume(struct i2c_client *i2c)
+{
+ struct palmas *palmas = i2c_get_clientdata(i2c);
+ struct device *dev = i2c-dev;
+
+ if (!palmas-wakeirq)
+ return 0;
+
+ if (device_may_wakeup(dev))
+ disable_irq_nosync(palmas-wakeirq);
Again, why nosync?
true - nosync is not necessary at here. disable_irq is however necessary
as we are not interested in wakeup events for level changes.
We just use the enable/disable to control when we'd want to arm the pin
for waking up from suspend state.
And what is issuing the call to enable/disable_irq_wake()?
So if that interrupt is not marked proper then you can bring your
device into a wont resume state easily
start suspend
enable wakeirq
disable_device_irqs()
if (!iswakeup_irq())
disable_irq() // does not mask due to lazy masking
wakeirq fires
if (irq_is_disabled())
mask_irq();
transition into suspend
Now your pinctrl irq is masked at the HW level and wont wake the
machine up ever again.
So now looking at that pinctrl irq chip thing, which seems to be
designed to handle these kind of wakeups. That thing looks massivly
wrong as well, simply because it enforces to use
enable_irq/disable_irq().
So because the sole purpose of this chip is to handle the separate
wakeup style interrupt, it should actually NOT enable the interrupt in
the irq_unmask callback.
Simply because during normal operation nothing is interested in the
interrupt and any operation which might enable it (including request
irq) is just making the system handle completely pointless interrupts
and hoops and loops juggling with enable/disable irq.
So the right thing here is to have an empty unmask function and do the
actual unmask only in the irq_set_wake() callback. mask of course
needs to do what it says. The point is, that the following sequence of
code will just work w/o generating an interrupt on the wakeirq line
outside of the wake enabled context.
dev_init()
request_wakeirq();
suspend()
if (may_wake())
enable_irq_wake();
resume()
if (may_wake())
disable_irq_wake();
The other omap drivers using this have the same issue ... And of
course they are subtly different.
The uart one handles the actual device interrupt, which is violating
the general rule of possible interrupt reentrancy in the pm-runtime
case if the two interrupts are affine to two different cores. Yes,
it's protected by a lock and works by chance
The mmc one issues a disable_irq_nosync() in the wakeup irq handler
itself.
WHY does one driver need that and the other does not? You are not even
able to come up with a common scheme for OMAP. I don't want to see the
mess others are going to create when this stuff becomes more used.
Thanks,
tglx
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V3 3/3] mfd: palmas: Add support for optional wakeup
On Fri, 19 Sep 2014, Nishanth Menon wrote: On 08:37-20140919, Thomas Gleixner wrote: The other omap drivers using this have the same issue ... And of course they are subtly different. The uart one handles the actual device interrupt, which is violating the general rule of possible interrupt reentrancy in the pm-runtime case if the two interrupts are affine to two different cores. Yes, it's protected by a lock and works by chance The mmc one issues a disable_irq_nosync() in the wakeup irq handler itself. WHY does one driver need that and the other does not? You are not even able to come up with a common scheme for OMAP. I don't want to see the mess others are going to create when this stuff becomes more used. Thanks, tglx I think I understand your concern - I request Tony to comment about this. I mean, I can try and hook things like uart in other drivers (like https://patchwork.kernel.org/patch/4759171/ ), but w.r.t overall generic usage guideline wise, I would prefer Tony to comment. No, the uart and that i2c thing are just wrong. Assume the following device irq affine to cpu0 wakeup irq affine to cpu1 CPU 0 CPU 1 runtime suspend enable_wake(wakeup irq); wakeup interrupt is raised device interrupt is raised dev_handler(device) dev_handler(device) It might work due to locking, but it is nevertheless wrong. Interrupt handlers for devices are guaranteed not to be reentrant. And this brilliant stuff simply violates that guarantee. So, no. It's wrong even if it happens to work by chance. Thanks, tglx -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V3 3/3] mfd: palmas: Add support for optional wakeup
On Fri, 19 Sep 2014, Tony Lindgren wrote:
* Thomas Gleixner t...@linutronix.de [140919 10:37]:
From hardware point of view the wake-up events behave like interrupts
and could also be used as the only interrupt in some messed up cases.
That avoids all kinds of custom APIs from driver point.
The re-entrancy problem we've most likely had ever since we enabled
the PRCM interrupts, and maybe that's why I did not even consider
that part. I think before that we were calling the driver interrupt
after waking up from the PM code..
Anyways, how about the following to deal with the re-entrancy problem:
1. The wake-up interrupt handler must have a separate interrupt
handler that just calls tasklet_schedule()
2. The device interrupt handler also just calls tasklet_schedule()
3. The tasklet then does pm_runtime_get, handles the registers, and
so on.
Or would we still have a re-entrancy problem somewhere else with
that?
Why on earth are you wanting tasklets in there? That's just silly,
really.
The wakeup handler is supposed to bring the thing out of deep sleep
and nothing else. All you want it to do is to mask itself and save the
information that the real device irq is pending.
A stub handler for the wakeup irq is enough. We can have that in the
irq/pm core and all it would do is simply:
irqreturn_t handle_jinxed_wakeup_irq(unsigned irq, void *dev_id)
{
unsigned device_irq = get_dev_irq(dev_id);
force_mask(irq);
set_irq_pending(device_irq);
return HANDLED;
}
So on resume_device_irqs() the real device interrupt gets reenabled
and unmasked (if it was masked) and the interrupt gets resent either
in hardware (level or retrigger) or by the software resend mechanism.
That completely avoids tasklets, reentrant irq handlers and all other
crap which might be required.
Thanks,
tglx
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V3 3/3] mfd: palmas: Add support for optional wakeup
On Fri, 19 Sep 2014, Tony Lindgren wrote: * Thomas Gleixner t...@linutronix.de [140919 12:47]: Why on earth are you wanting tasklets in there? That's just silly, really. Lack of a framework on driver side to cope with this in a generic way? :p So instead of creating such a thing we rather have a completely ass backward workaround which spreads itself all over the tree? You SoC folks really need a quarterly sanity check. Thanks, tglx -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V3 3/3] mfd: palmas: Add support for optional wakeup
Tony,
On Thu, 6 Nov 2014, Tony Lindgren wrote:
Any comments on the patch below? Let me know if you want to keep the
devm stuff out of kernel/irq/manage.c.
Sorry, this slipped through the cracks.
+static int setup_wakeirq(struct device *dev, unsigned int wakeirq,
+unsigned long wakeflags, bool devm)
+{
+ int ret;
+
+ if (!(dev wakeirq)) {
+ pr_err(Missing device or wakeirq for %s irq %d\n,
+ dev_name(dev), wakeirq);
+ return -EINVAL;
+ }
+
+ if (!(wakeflags
+ (IRQF_TRIGGER_LOW | IRQF_TRIGGER_HIGH | IRQF_ONESHOT))) {
+ pr_err(Invalid wakeirq for %s irq %d, must be level oneshot\n,
+ dev_name(dev), wakeirq);
This looks odd.
Why do you want to enforce LEVEL and ONESHOT? I can see the point for
ONESHOT, but I'm wondering about the requirement for level.
Now if you really want to enforce level AND oneshot, your check is
wrong as it will not trigger on
wakeflags = IRQF_TRIGGER_LOW;
wakeflags = IRQF_TRIGGER_HIGH;
wakeflags = IRQF_ONESHOT;
Not what you really want, right?
+int request_wake_irq(struct device *dev, unsigned int wakeirq,
+unsigned long wakeflags)
+{
+ return setup_wakeirq(dev, wakeirq, wakeflags, false);
+}
+EXPORT_SYMBOL(request_wake_irq);
_GPL please
+
+int devm_request_wake_irq(struct device *dev, unsigned int wakeirq,
+ unsigned long wakeflags)
+{
+ return setup_wakeirq(dev, wakeirq, wakeflags, false);
Shouldnt that have devm = true?
Thanks,
tglx
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V3 3/3] mfd: palmas: Add support for optional wakeup
On Thu, 13 Nov 2014, Tony Lindgren wrote:
Oops thanks for catching that. As the devres stuff is separate, I've
updated the patch to keep it that way by adding a minimal manage.h.
This avoids including internals.h in devres.c. Does that seem usable
for you?
What's wrong with internals.h? devres.c is core code, so it is not
affected of the ban to include internals.h :)
+/**
+ * init_disabled_wakeirq - initialize a wake-up interrupt for a device
+ * @dev: device to wake up on the wake-up interrupt
+ * @wakeirq: wake-up interrupt for the device
+ * @wakeflags: wake-up interrupt flags
+ *
+ * Note that the wake-up interrupt starts disabled. The wake-up interrupt
+ * is typically enabled from the device pm_runtime_suspend() and disabled
+ * again in the device pm_runtime_resume(). For runtime PM, the wake-up
+ * interrupt should be always enabled, and for device suspend and resume,
+ * the wake-up interrupt should be enabled depending on the device specific
+ * configuration for device_can_wakeup().
+ *
+ * Note also that we are not resending the lost device interrupts.
+ * We assume that the wake-up interrupt just needs to wake-up the device,
+ * and then device pm_runtime_resume() can deal with the situation.
+ *
+ * There are at least the following reasons to not resend the lost device
+ * interrupts automatically based on the wake-up interrupt:
+ *
+ * 1. There can be interrupt reentry issues calling the device interrupt
+ * based on the wake-up interrupt if done in the device driver. It
+ * could be done with check_irq_resend() after checking the device
+ * interrupt mask if we really wanted to though.
+ *
+ * 2. The device interrupt handler would need to be set up properly with
+ * pm_runtime_irq_safe(). Ideally you don't want to call pm_runtime
+ * calls from the device interrupt handler at all.
+ *
+ * 3. The IRQ subsystem may not know if it's safe to call the device
+ * interrupt unless the driver updates the interrupt status with
+ * disable_irq() and enable_irq() in addition to just disabling the
+ * interrupt at the hardware level in the device registers.
+ *
+ * So if replaying the lost device interrupts is absolutely needed from the
+ * hardware point of view, it's probably best to set up a completely
+ * separate wake-up interrupt handler for the wake-up interrupt in the
+ * device driver because of the reasons above.
Can we please kill this last paragraph? I'm already seeing the
gazillion of I think it is required to do so for my s special
chip implementations in random drivers which all get it wrong again.
So I'd rather provide a mechanism upfront which lets the driver know
that the wakeup interrupt originated from that device, i.e. let the
wake up handler call
pm_wakeup_irq(dev);
which calls:
pm_runtime_mark_last_busy(dev);
pm_request_resume(dev);
and aside of that tells the device via a flag or preferrably a
sequence counter that the wakeup irq has been triggered. So affected
devices can handle it based on that information w/o implementing the
next broken variant of wakeup irq handlers.
That also allows to remove the wakeflags check for level/edge.
+ */
+int init_disabled_wakeirq(struct device *dev, unsigned int wakeirq,
+ unsigned long wakeflags)
+{
+ if (!(dev wakeirq)) {
This is the second time I stumbled over this. While it is correct it
would be simpler to parse
if (!dev || !wakeirq) {
At least for my review damaged brain :)
+ pr_err(Missing device or wakeirq for %s irq %d\n,
+dev_name(dev), wakeirq);
+ return -EINVAL;
+ }
+
+ if (!(wakeflags IRQF_ONESHOT)) {
+ pr_err(Invalid wakeirq for %s irq %d, must be oneshot\n,
+dev_name(dev), wakeirq);
+ return -EINVAL;
+ }
Is there a reason why we force the wakeirq into a threaded handler?
Thanks,
tglx
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] treewide: Convert clockevents_notify to use int cpu
On Wed, 10 Dec 2014, Joe Perches wrote: As far as I can tell, there's no value indirecting the cpu passed to this function via a void *. Update all the callers and called functions from within clockevents_notify. Aside of that there is no value for this 'notification' function at all. This should be seperate explicit calls. The notify function is a leftover from the original implementation which used actual notifier chains. I'll send out a cleanup series later today. Thanks, tglx -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC v1 15/25] genirq: Kill the first parameter 'irq' of irq_flow_handler_t
On Wed, 20 May 2015, Jiang Liu wrote:
On 2015/5/20 23:28, Thomas Gleixner wrote:
On Wed, 20 May 2015, Jiang Liu wrote:
-static void locomo_handler(unsigned int irq, struct irq_desc *desc)
+static void locomo_handler(struct irq_desc *desc)
{
struct locomo *lchip = irq_desc_get_chip_data(desc);
+ unsigned int irq;
int req, i;
That leaves irq unitialized
That should be OK, 'irq' here is just a local variable.
Actually it may be changed as:
if (req) {
/* generate the next interrupt(s) */
-irq = lchip-irq_base;
+unsigned int irq = lchip-irq_base;
Indeed.
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC v1 15/25] genirq: Kill the first parameter 'irq' of irq_flow_handler_t
On Wed, 20 May 2015, Jiang Liu wrote:
-static void locomo_handler(unsigned int irq, struct irq_desc *desc)
+static void locomo_handler(struct irq_desc *desc)
{
struct locomo *lchip = irq_desc_get_chip_data(desc);
+ unsigned int irq;
int req, i;
That leaves irq unitialized
Thanks,
tglx
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] futex: lower the lock contention on the HB lock during wake up
On Fri, 19 Jun 2015, Kevin Hilman wrote:
On Wed, Jun 17, 2015 at 1:33 AM, Sebastian Andrzej Siewior
A handful of boot test failures on ARM/OMAP were found by kernelci.org
in next-20150619[1] and were bisected down to this patch, which hit
next-20150619 in the form of commit 881bd58d6e9e (futex: Lower the
lock contention on the HB lock during wake up). I confirmed that
reverting that patch on top of next-20150619 gets things booting again
for the affected platforms.
I haven't debugged this any further, but full boot logs are available
for the boot failures[2][3] and the linux-omap list and maintainer are
Cc'd here to help investigate further if needed.
Found it. Dunno, how I missed that one. Fix below.
Thanks,
tglx
---
diff --git a/kernel/locking/rtmutex.c b/kernel/locking/rtmutex.c
index 10dbeb6fe96f..5674b073473c 100644
--- a/kernel/locking/rtmutex.c
+++ b/kernel/locking/rtmutex.c
@@ -1365,9 +1365,14 @@ rt_mutex_fastunlock(struct rt_mutex *lock,
if (likely(rt_mutex_cmpxchg(lock, current, NULL))) {
rt_mutex_deadlock_account_unlock(current);
- } else if (slowfn(lock, wake_q)) {
+ } else {
+ bool deboost = slowfn(lock, wake_q);
+
+ wake_up_q(wake_q);
+
/* Undo pi boosting if necessary: */
- rt_mutex_adjust_prio(current);
+ if (deboost)
+ rt_mutex_adjust_prio(current);
}
}
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
Re: [PATCH] clocksource: Allow toggling between runtime and persistent clocksource for idle
On Mon, 6 Jul 2015, Tony Lindgren wrote:
* Thomas Gleixner t...@linutronix.de [150706 07:20]:
On Mon, 6 Jul 2015, Tony Lindgren wrote:
The timekeeping accuracy issue certainly needs some thinking, and
also the resolution between the clocksources can be different.. In the
test case I have the slow timer is always on and of a lower resolution
than the ARM global timer being used during runtime.
Got some handy timer test in mind you want me to run to provide data
on the accuracy?
John Stultz might have something.
+/**
+ * clocksource_pm_enter - change to a persistent clocksource before idle
+ *
+ * Changes system to use a persistent clocksource for idle. Intended to
+ * be called from CPUidle from the last active CPU.
+ */
+int clocksource_pm_enter(void)
+{
+ bool oneshot = tick_oneshot_mode_active();
+ struct clocksource *best;
+
+ if (WARN_ONCE(!mutex_trylock(clocksource_mutex),
+ Unable to get clocksource_mutex))
+ return -EINTR;
This trylock serves which purpose?
Well we don't want to start changing clocksource if something is
running like you pointed out.
Well yes, but
I really cannot see how this is proper serialized.
We need to allow this only from the last cpu before hitting idle.
And I cannot see anything which does so.
cpu0cpu1
is_idle
go_idle()
clocksource_pm_enter()
lock(cs_mutex);
wakeup()
clocksource_pm_exit()
trylock fails
...
unlock(cs_mutex);
-- Crap!
@@ -1086,7 +1086,18 @@ int timekeeping_notify(struct clocksource *clock)
if (tk-tkr_mono.clock == clock)
return 0;
- stop_machine(change_clocksource, clock, NULL);
+
+ /*
+ * We may want to toggle between a fast and a persistent
+ * clocksource from CPUidle on the last active CPU and can't
+ * use stop_machine at that point.
+ */
+ if (cpumask_test_cpu(smp_processor_id(), cpu_online_mask)
Can you please explain how this code gets called from an offline cpu?
Last cpu getting idled..
That does not make any sense at all. How is idle related to the online
mask? An idle cpu is still online.
+ !rcu_is_watching())
So pick some random combination of conditions and define that it is
correct, right? How on earth does !rcu_watching() tell that this is
the last running cpu.
We have called rcu_idle_enter() from cpuidle_idle_call(). Do you have
some better test in mind when the last cpu is about hit idle?
The cpuidle code should know that. And if it does not know, it better
should keep track of that information and based on it provide the
proper serialization, so the call into the timekeeping code is not a
subject to guess work and race conditions.
Thanks,
tglx
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] clocksource: Allow toggling between runtime and persistent clocksource for idle
On Mon, 6 Jul 2015, John Stultz wrote: On Mon, Jul 6, 2015 at 12:12 AM, Tony Lindgren t...@atomide.com wrote: Some persistent clocksources can be on a slow external bus. For shorter latencies for RT use, let's allow toggling the clocksource during idle between a faster non-persistent runtime clocksource and a slower persistent clocksource. So yea, as Thomas mentioned, this will cause problems for timekeeping accuracy, since we end up resetting the ntp state when we change clocksource (additionally you gain a bit of error each time you switch clocksources). So you'll most likely see trouble w/ ntpd steering the clock. I'm not sure its quite clear in the description as to the need here. Could you expand a bit as to the rational for why? I assume it has to do with you have a high-res clocksource that is good for fine-grained clock_gettime() calls, but wraps quickly, making it poor for long idle times. So you're trying to swap to a less fine grained clocksource that won't wrap so fast? The persistent-clock-like name muddies things further, since the persistent-clock is used for suspend, while it looks like this is just for idle times. Though that are idle states where the cpu is powered off so the fast clock source is powered off as well We all know how well that works from the x86/TSC horror. It's just the same thing again with a different arch prefix. Thanks, tglx -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] clocksource: Allow toggling between runtime and persistent clocksource for idle
On Mon, 6 Jul 2015, Tony Lindgren wrote:
Some persistent clocksources can be on a slow external bus. For shorter
latencies for RT use, let's allow toggling the clocksource during idle
between a faster non-persistent runtime clocksource and a slower persistent
clocksource.
I really cannot follow that RT argument here. The whole switchover
causes latencies itself and whats worse is, that this breaks
timekeeping accuracy because there is no way to switch clocksources
atomically without loss.
---
include/linuxt-email-lkml-omap/clocksource.h | 2 ++
Interesting file name.
extern int timekeeping_notify(struct clocksource *clock);
+extern int clocksource_pm_enter(void);
+extern void clocksource_pm_exit(void);
Unfortunately you are not providing the call site, so I cannot see
from which context this is going to be called.
I fear its from the guts of the idle code probably with interrupts
disabled etc , right?
+/**
+ * clocksource_pm_enter - change to a persistent clocksource before idle
+ *
+ * Changes system to use a persistent clocksource for idle. Intended to
+ * be called from CPUidle from the last active CPU.
+ */
+int clocksource_pm_enter(void)
+{
+ bool oneshot = tick_oneshot_mode_active();
+ struct clocksource *best;
+
+ if (WARN_ONCE(!mutex_trylock(clocksource_mutex),
+ Unable to get clocksource_mutex))
+ return -EINTR;
This trylock serves which purpose?
+
+ best = clocksource_find_best(oneshot, true, false);
+ if (best) {
+ if (curr_clocksource != best
+ !timekeeping_notify(best)) {
+ runtime_clocksource = curr_clocksource;
+ curr_clocksource = best;
+ }
+ }
+ mutex_unlock(clocksource_mutex);
+
+ return !!best;
+}
+
+/**
+ * clocksource_pm_exit - change to a runtime clocksrouce after idle
+ *
+ * Changes system to use the best clocksource for runtime. Intended to
+ * be called after waking up from CPUidle on the first active CPU.
+ */
+void clocksource_pm_exit(void)
+{
+ if (WARN_ONCE(!mutex_trylock(clocksource_mutex),
+ Unable to get clocksource_mutex))
+ return;
+
+ if (runtime_clocksource) {
+ if (curr_clocksource != runtime_clocksource
+ !timekeeping_notify(runtime_clocksource)) {
+ curr_clocksource = runtime_clocksource;
+ runtime_clocksource = NULL;
+ }
+ }
+ mutex_unlock(clocksource_mutex);
+}
I really cannot see how this is proper serialized.
#ifdef CONFIG_SYSFS
/**
* sysfs_show_current_clocksources - sysfs interface for current clocksource
diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
index bca3667..0379260 100644
--- a/kernel/time/timekeeping.c
+++ b/kernel/time/timekeeping.c
@@ -1086,7 +1086,18 @@ int timekeeping_notify(struct clocksource *clock)
if (tk-tkr_mono.clock == clock)
return 0;
- stop_machine(change_clocksource, clock, NULL);
+
+ /*
+ * We may want to toggle between a fast and a persistent
+ * clocksource from CPUidle on the last active CPU and can't
+ * use stop_machine at that point.
+ */
+ if (cpumask_test_cpu(smp_processor_id(), cpu_online_mask)
Can you please explain how this code gets called from an offline cpu?
+ !rcu_is_watching())
So pick some random combination of conditions and define that it is
correct, right? How on earth does !rcu_watching() tell that this is
the last running cpu.
+ change_clocksource(clock);
+ else
+ stop_machine(change_clocksource, clock, NULL);
This patch definitely earns a place in my ugly code museum under the
category 'Does not explode in my face, so it must be correct'.
Thanks,
tglx
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] gpio: omap: use raw locks for locking
On Mon, 27 Jul 2015, Sebastian Andrzej Siewior wrote: On 07/27/2015 02:50 PM, Linus Walleij wrote: Patch applied. thanks. Now this question appear in my head: Is drivers/gpio full of stuff that will not work with the -RT kernel, and is this a change that should be done mutatis mutandis on all the GPIO drivers? I described two call paths where you need a rawlock_t. If your gpio driver uses irq_chip_generic then you a rawlock here and things should be fine. In general: If your gpio controller acts as an interrupts controller (that is via chained handler) then you need the raw-locks if you need any locking (if you have a write 1 to mask/unmask/enable/disable register then you probably don't need any locking here at all). If the gpio controller does not act as an interrupt controller than the spinlock_t type should be enough. If your gpio-interrupt controller requests its interrupt via requested_threaded_irq() then it should do handle_nested_irq() and a mutex is probably used for locking. Using request_irq() with 0 flags is kind of broken. It works in IRQ-context and delivers the interrupts with generic_handle_irq() and this one passes it the handler (like handle_edge_irq() / handle_level_irq()) which takes a raw_lock. Now, if you boot the vanilla kernel with threadedirq then the irq-handler runs in threaded context and you can't take a spinlock here anymore. So I think you should use here IRQF_NO_THREAD here (and the raw lock type). I added tglx on Cc: to back up because it is Monday. Indeed it was monday. It's an RT only problem. On mainline spinlock resovles to raw_spinlock. Now there is the story what breaks in RT: 1) irq controller specific locks which are taken in the irq chip callbacks need to be raw_spinlocks because these functions are called with irq_desc-lock helds and interrupts disabled. If that irq chip lock is not raw you rightfully get a backtrace. raw_spinlock_irq(desc-lock); chip-callback() spin_lock(chip_private_lock); might_sleep() triggers 2) locks which are taken in chained interrupt handlers need to be raw on RT. These handlers run in hard interrupt context so again might_sleep() rightfully complains. Thanks, tglx -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH RESEND] irqchip: omap-intc: improve IRQ handler
On Mon, 20 Jul 2015, Felipe Balbi wrote: + irqnr = intc_readl(INTC_SIR); + irqnr = ACTIVEIRQ_MASK; + WARN(!irqnr, Spurious IRQ ?\n); Shouldn't that be WARN_ONCE? Thanks, tglx -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] irqchip: omap-intc: improve IRQ handler
On Wed, 15 Jul 2015, Tony Lindgren wrote: Felipe, * Tony Lindgren t...@atomide.com [150119 13:41]: * Felipe Balbi ba...@ti.com [150102 10:50]: as it turns out the current IRQ number will *always* be available from SIR register which renders the reads of PENDING registers as plain unnecessary overhead. In order to catch any situation where SIR reads as zero, we're adding a WARN() to turn it into a very verbose error and users actually report it. With this patch average running time of omap_intc_handle_irq() reduced from about 28.5us to 19.8us as measured by the kernel function profiler. Tested with BeagleBoneBlack Rev A5C. Signed-off-by: Felipe Balbi ba...@ti.com Jason, looks like this is not showing up in Linux next. The same for the changes I did for dm81xx. Can you please resend this to Jason? Looks like this is still not merged. Please send it to me asap and please cc lkml on irqchip patches. Thanks, tglx -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] irqchip: omap-intc: fix spurious irq handling
On Mon, 19 Oct 2015, Sekhar Nori wrote:
> + /*
> + * A spurious IRQ can result if interrupt that triggered the
> + * sorting is no longer active during the sorting (10 INTC
> + * functional clock cycles after interrupt assertion). Or a
> + * change in interrupt mask affected the result during sorting
> + * time. There is no special handling required except ignoring
> + * the SIR register value just read and retrying.
> + * See section 6.2.5 of AM335x TRM Literature Number: SPRUH73K
> + */
> + if ((irqnr & SPURIOUSIRQ_MASK) == SPURIOUSIRQ_MASK) {
> + pr_debug_ratelimited("%s: spurious irq!\n", __func__);
I'd prefer that this is a pr_once() and the spurious interrupt counter
is incremented. That's far more useful as it gives you real
information about the frequency of the issue.
Thanks,
tglx
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] sched_clock: add data pointer argument to read callback
Mans, On Fri, 9 Oct 2015, Mans Rullgard wrote: > This passes a data pointer specified in the sched_clock_register() > call to the read callback allowing simpler implementations thereof. > > In this patch, existing uses of this interface are simply updated > with a null pointer. I can't see any simplifaction of any driver. This is called in hot pathes and that extra unused argument just adds register pressure for no value. What are you actually trying to solve? Thanks, tglx -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] gpio: omap: convert to use generic irq handler
Grygorii, can you please provide a patch set against 4.1-RT? That stuff rejects left and right. Thanks, tglx -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] gpio: omap: convert to use generic irq handler
Grygorii, On Tue, 13 Oct 2015, Grygorii Strashko wrote: > I'd very appreciate for any advice of how to better proceed with your request. > - I can try to apply and re-send only patches marked by '*' > - I can prepare branch with all above patches Please provide a branch on top of 4.1.10 which contains the whole lot. I have a look at it and decide then how to go from there. Thanks, tglx -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: CONFIG_DEBUG_SHIRQ and PM
On Tue, 25 Aug 2015, Felipe Balbi wrote:
Hi Ingo,
Thanks for not cc'ing the irq maintainer
I'm facing an issue with CONFIG_DEBUG_SHIRQ and pm_runtime when using
devm_request_*irq().
If we using devm_request_*irq(), that irq will be freed after device
drivers' -remove() gets called. If on -remove(), we're calling
pm_runtime_put_sync(); pm_runtime_disable(), device's clocks might get
gated and, because we do an extra call to the device's IRQ handler when
CONFIG_DEBUG_SHIRQ=y, we might trigger an abort exception if, inside the
IRQ handler, we try to read a register which is clocked by the device's
clock.
This is, of course, really old code which has been in tree for many,
many years. I guess nobody has been running their tests in the setup
mentioned above (CONFIG_DEBUG_SHIRQ=y, pm_runtime_put_sync() on
-remove(), a register read on IRQ handler, and a shared IRQ handler),
so that's why we never caught this before.
Disabling CONFIG_DEBUG_SHIRQ, of course, makes the problem go away, but
if driver *must* be ready to receive, and handle, an IRQ even during
module removal, I wonder what the IRQ handler should do. We can't, in
most cases, call pm_runtime_put_sync() from IRQ handler.
Well, a shared interrupt handler must handle this situation, no matter
what. Assume the following:
irqreturn_t dev_irq(int irq, void *data)
{
struct devdata *dd = data;
u32 state;
state = readl(dd-base);
...
}
void module_exit(void)
{
/* Write to the device interrupt register */
disable_device_irq(dd-base);
/*
* After this point the device does not longer
* raise an interrupt
*/
iounmap(dd-base);
free_irq();
If the other device which shares the interrupt line raises an
interrupt after the unmap and before free_irq() removed the device
handler from the irq, the machine is toast, because the dev_irq
handler is still called.
If the handler is shut down after critical parts of the driver/device
are shut down, then you can
- either can change the setup/teardown ordering
disable_device_irq(dd-base);
free_irq();
iounmap(dd-base);
- or have a proper flag in the private data which tells the interrupt
handler to sod off.
irqreturn_t dev_irq(int irq, void *data)
{
struct devdata *dd = data;
if (dd-shutdown)
return IRQ_NONE;
...
void module_exit(void)
{
disable_device_irq(dd-base);
dd-shutdown = 1;
/* On an SMP machine you also need: */
synchronize_irq(dd-irq);
So for the problem at hand, the devm magic needs to make sure that the
crucial parts are still alive when the devm allocated irq is released.
I have no idea how that runtime PM stuff is integrated into devm (I
fear not at all), so it's hard to give you a proper advise on that.
Thanks,
tglx
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Routable IRQs
Felipe, On Tue, 29 Dec 2015, Felipe Balbi wrote: > Anyway, the interesting part is that PRUSS has 64 events (on current > incarnations at least) and PRUSS has 10 physical IRQ lines to the ARM > land. Each of these 64 events can be routed to any of these 10 IRQ > lines. This might not be very useful on UP (AM335x & AM437x) other than > the fact that soft-IP drivers running on Linux would need to guarantee > they are the ones who should handle the IRQ. However, on SMP (AM57xx) we > could have real tangible benefits by means of IRQ affinity, etc. > > So, the question is, what is there in IRQ subsystem today for routable > IRQ support ? > > If a Diagram helps here's a simple one. Note that I'm not showing > details on the PRUSS side, but that side can also map events pretty much > any way it wants. > > .. .. > | HOST CPU | | PRUSS| > || || > || || > | irq0 |<-.--|evt0| > || | || > | irq1 | | .---|evt1| > || | | || > | irq2 | '--|evt2| > || | || > | irq3 | | || > || | || > | irq4 | | | . | > || | || > | irq5 | | | . | > || | || > | irq6 | | | . | > || | || > | irq7 |<' || > || || > | irq8 | || > || || > | irq9 |<|evtN| > '' '' > > Given this setup, what I want to do, is let soft-IP drivers running on > linux rely on standard *request_*irq() calls and DTS descrition. But I'm > still considering how/if we should describe the routing itself or just > go round-robin (i.o.w. irq0 -> evt0, irq1 -> evt1, ..., irq9 -> evt9, > irq0 -> evt10, ...). > > Thoughts ? I have a few questions: - Is there a "mapping" block between PRUSS and the host interrupt controller or is this "mapping" block part of PRUSS? - We all know how well shared interrupts work. Is there a point of supporting 64 interrupts when you only have 10 irq lines available? - I assume that the PRUSS interrupt mapping is more or less a question of the firmware implementation. So you either have a fixed association in the firmware which is reflected in the DT description of the IP block or you need an interface to tell the PRUSS firmware which event it should map to which irq line. Is there actually a value in doing the latter? Thanks, tglx -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Routable IRQs
Felipe, On Wed, 30 Dec 2015, Felipe Balbi wrote: > Thomas Gleixner <t...@linutronix.de> writes: > > - Is there a "mapping" block between PRUSS and the host interrupt > > controller > >or is this "mapping" block part of PRUSS? > > The description in TRM is a bit "poor", but from what I can gather, the > mapping is done on an interrupt controller inside the PRUSS. However, > Linux is the one who's got the driver for that INTC (well, Linux will be > the one with the soft ethernet/uart/whatever IP to talk to). All of its > (INTC's) registers are memory mapped to the ARM side. Ok. And the INTC registers include the "mapping" configuration, right? > > - We all know how well shared interrupts work. Is there a point of > > supporting > >64 interrupts when you only have 10 irq lines available? > > I'm looking at these 64 events more like MSI kind of events. It's just Well, that's fine to look at them this way, but they will end up shared no matter what. > that the events themselves can be routed to any of the 10 available HW > IRQ lines. > > > - I assume that the PRUSS interrupt mapping is more or less a question of > > the > >firmware implementation. So you either have a fixed association in the > >firmware which is reflected in the DT description of the IP block or you > >need an interface to tell the PRUSS firmware which event it should map to > >which irq line. Is there actually a value in doing the latter? > > right, I'd say the mapping is pretty static. Unless Suman has some extra > information which I don't. I guess the question was really to see if > there was an easy way for doing this so we don't have to mess with DTS > for every other FW and their neighbor. Well, you will need information about every other firmware simply because you need to know which events the firmware is actually using and what the purpose of the particular event is. Assume you have a simple uart with 3 events (RX, TX, status). So how will the firmware tell you which event is which? You have a few options: 1) DT + fixed mapping scheme: Describe the PRUSS event number in DT and have a fixed mapping scheme like the one you mentioned evt0 -> irq0 . 2) DT + DT mapping scheme Describe the PRUSS event number in DT and describe the mapping scheme in DT as well 3) DT + dynamic mapping scheme Describe the PRUSS event number in DT and let your interrupt controller associate the irq number dynamically. That's kind of similar to MSI with the exception that it needs to support shared interrupts. 4) Fully dynamic association Have a query interface to the firmware which tells you which event it uses for which particular purpose (RX, TX ...) and then establish a dynamic mapping to one of the interrupts. Not sure which level of complexity you want :) Thanks, tglx -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] clocksource: arm_global_timer: fix suspend resume
On Fri, 20 Nov 2015, John Stultz wrote: > So its unlikely that the hardware both stays running through suspend, > but also might halt in idle. That would be "unique". The amount of creativity put into the next variants of differently broken timers is amazing. So I wouldn't be too surprised if such a thing actually surfaces. Thanks, tglx -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
