Hi,
On Thu, Jun 30, 2011 at 08:51:39AM +0300, Tero Kristo wrote:
> On Wed, 2011-06-29 at 18:53 +0200, Balbi, Felipe wrote:
> > Hi,
> >
> > On Wed, Jun 29, 2011 at 12:04:55PM +0300, Tero Kristo wrote:
> > > diff --git a/arch/arm/mach-omap2/pm34xx.c b/arch/arm/mach-omap2/pm34xx.c
> > > index 96a7624..89cf027 100644
> > > --- a/arch/arm/mach-omap2/pm34xx.c
> > > +++ b/arch/arm/mach-omap2/pm34xx.c
> > > @@ -880,20 +824,35 @@ static int __init omap3_pm_init(void)
> > > /* XXX prcm_setup_regs needs to be before enabling hw
> > > * supervised mode for powerdomains */
> > > prcm_setup_regs();
> > > + ret = omap_prcm_irq_init();
> > > + if (ret) {
> > > + pr_err("omap_prcm_irq_init() failed with %d\n", ret);
> > > + goto err_prcm_irq_init;
> > > + }
> > > +
> > > + prcm_wkup_irq = omap_prcm_event_to_irq("wkup");
> > > + prcm_io_irq = omap_prcm_event_to_irq("io");
> > > +
> > > + ret = request_irq(prcm_wkup_irq, _prcm_int_handle_wakeup,
> > > + IRQF_NO_SUSPEND | IRQF_DISABLED, "prcm_wkup", NULL);
> >
> > does this _have_ to be all in hardirq context ?
>
> Not really, imo this does not need to be done in an interrupt at all.
> The wakeup event ack can be done just before entering next idle, as I
> did in a previous version of this set, but it did not receive that
> positive comments yet. I will probably try to push this idea forward
> once this set is pulled.
I see... BTW, IRQF_DISABLED is a nop now, you don't need it. I missed
that before ;-)
> > > diff --git a/arch/arm/mach-omap2/prcm.c b/arch/arm/mach-omap2/prcm.c
> > > index 6be1438..794e451 100644
> > > --- a/arch/arm/mach-omap2/prcm.c
> > > +++ b/arch/arm/mach-omap2/prcm.c
> > > @@ -23,6 +23,8 @@
> > > #include <linux/clk.h>
> > > #include <linux/io.h>
> > > #include <linux/delay.h>
> > > +#include <linux/irq.h>
> > > +#include <linux/slab.h>
> > >
> > > #include <mach/system.h>
> > > #include <plat/common.h>
> > > @@ -45,6 +47,167 @@ void __iomem *cm2_base;
> > >
> > > #define MAX_MODULE_ENABLE_WAIT 100000
> > >
> > > +/* Setup for the interrupt handling based on used platform */
> > > +static struct omap_prcm_irq_setup *irq_setup;
> >
> > you can set this is irq_chip data, then you can:
> >
> > > +static void prcm_irq_ack(struct irq_data *data)
> > > +{
> > struct omap_prcm_irq_setup *irq_setup = irq_data_get_irq_chip_data(data)
> > unsigned int prcm_irq = data->irq - irq_setup->base;
> >
> > irq_setup->ack_event(prcm_irq);
> > > +}
> >
> > ditto to all other operations.
>
> This is related to the dynamic allocation of irq numbers you speak of
> later I think... anyway, I'll take a look at this.
not really, you already set something as data to that function. But I
guess it's the generic irq chip ? Even in that case, you should be able
to access struct omap_prcm_irq_set from struct irq_data.
> > > +static struct irq_chip_generic *prcm_irq_chips[OMAP_PRCM_MAX_NR_PENDING_REG];
> >
> > can't you allocate this dynamically ???
>
> Well yea, but it is only 1 or 2 pointers. The code for dynamic
> allocation will eat more than 4 bytes easily.
fair enough...
> > > +/*
> > > + * PRCM Interrupt Handler
> > > + *
> > > + * The PRM_IRQSTATUS_MPU register indicates if there are any pending
> > > + * interrupts from the PRCM for the MPU. These bits must be cleared in
> > > + * order to clear the PRCM interrupt. The PRCM interrupt handler is
> > > + * implemented to simply clear the PRM_IRQSTATUS_MPU in order to clear
> > > + * the PRCM interrupt. Please note that bit 0 of the PRM_IRQSTATUS_MPU
> > > + * register indicates that a wake-up event is pending for the MPU and
> > > + * this bit can only be cleared if the all the wake-up events latched
> > > + * in the various PM_WKST_x registers have been cleared. The interrupt
> > > + * handler is implemented using a do-while loop so that if a wake-up
> > > + * event occurred during the processing of the prcm interrupt handler
> > > + * (setting a bit in the corresponding PM_WKST_x register and thus
> > > + * preventing us from clearing bit 0 of the PRM_IRQSTATUS_MPU register)
> > > + * this would be handled.
> > > + */
> > > +static void prcm_irq_handler(unsigned int irq, struct irq_desc *desc)
> > > +{
> > > + unsigned long pending[OMAP_PRCM_MAX_NR_PENDING_REG];
> > > + struct irq_chip *chip = irq_desc_get_chip(desc);
> > > +
> > > + /*
> > > + * Loop until all pending irqs are handled, since
> > > + * generic_handle_irq(), called by prcm_irq_handle_virtirqs()
> > > + * can cause new irqs to come
> > > + */
> > > + while (1) {
> > > + unsigned int virtirq;
> > > +
> > > + chip->irq_ack(&desc->irq_data);
> > > +
> > > + memset(pending, 0, sizeof(pending));
> > > + irq_setup->pending_events(pending);
> > > +
> > > + /* No bit set, then all IRQs are handled */
> > > + if (find_first_bit(pending, OMAP_PRCM_NR_IRQS)
> > > + >= OMAP_PRCM_NR_IRQS) {
> > > + chip->irq_unmask(&desc->irq_data);
> > > + break;
> > > + }
> > > +
> > > + /*
> > > + * Loop on all currently pending irqs so that new irqs
> > > + * cannot starve previously pending irqs
> > > + */
> > > + for_each_set_bit(virtirq, pending, OMAP_PRCM_NR_IRQS)
> > > + generic_handle_irq(OMAP_PRCM_IRQ_BASE + virtirq);
> >
> > if you use nested IRQ threads, you could be using
> > handle_nested_irq(irq);
>
> 1st level PRCM interrupt can't be a thread I think. I don't think
technically they can, now if it's fair to do that, I doubt. But I tend
to agree with tglx when he says top half should only check if the IRQ
comes from this device, then we spend very little time on hardirq
context and since IRQ threads are the next big thing after hardirqs,
there shouldn't be too much delay... hopefully
> chained interrupt handlers even support that, and even if they did,
no, chained can't fire another threads, but if they are already a
thread, the chained handler should run on the same thread, at least
that's how I understood the implementation.
> there might be reasons where in some cases we want to execute some of
> the PRCM events as hard irqs.
I don't see the point, after you know which interrupts you must handle,
there shouldn't be many things preventing you from doing the handling
itself in a thread. The IRQ line is even re-enabled so no delays should
be experienced.
--
balbi
Attachment:
signature.asc
Description: Digital signature
