From mboxrd@z Thu Jan 1 00:00:00 1970 From: Felipe Balbi Subject: Re: [PATCHv6 01/11] omap: prcm: switch to a chained IRQ handler mechanism Date: Mon, 25 Jul 2011 20:03:01 +0300 Message-ID: <20110725170300.GG18062@legolas.emea.dhcp.ti.com> References: <1311611771-15093-1-git-send-email-t-kristo@ti.com> <1311611771-15093-2-git-send-email-t-kristo@ti.com> Reply-To: balbi@ti.com Mime-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="uWAbeGC4mMoqIxdF" Return-path: Received: from na3sys009aob106.obsmtp.com ([74.125.149.76]:48738 "EHLO na3sys009aog106.obsmtp.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1751113Ab1GYRDF (ORCPT ); Mon, 25 Jul 2011 13:03:05 -0400 Received: by mail-fx0-f43.google.com with SMTP id 17so7413076fxg.16 for ; Mon, 25 Jul 2011 10:03:03 -0700 (PDT) Content-Disposition: inline In-Reply-To: <1311611771-15093-2-git-send-email-t-kristo@ti.com> Sender: linux-omap-owner@vger.kernel.org List-Id: linux-omap@vger.kernel.org To: Tero Kristo Cc: linux-omap@vger.kernel.org, Thomas Petazzoni , "Avinash.H.M" , Kevin Hilman , "Cousson, Benoit" , Tony Lindgren , "Govindraj.R" , Felipe Balbi , Paul Walmsley , Thomas Gleixner --uWAbeGC4mMoqIxdF Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable Hi, On Mon, Jul 25, 2011 at 07:36:01PM +0300, Tero Kristo wrote: > Introduce a chained interrupt handler mechanism for the PRCM > interrupt, so that individual PRCM event can cleanly be handled by > handlers in separate drivers. We do this by introducing PRCM event which drivers ? Are those somehow "children" of the "PRCM device" ?? If that's the case, you shouldn't need to match against names as you could allocate a platform_device for your children and pass in your resources with correct IRQ numbers. > names, which are then matched to the particular PRCM interrupt bit > depending on the specific OMAP SoC being used. >=20 > arch/arm/mach-omap2/prcm.c implements the chained interrupt mechanism > itself, with SoC specific support / init structure defined in > arch/arm/mach-omap2/prm2xxx_3xxx.c and arch/arm/mach-omap2/prm4xxx.c > respectively. At initialization time, the set of PRCM events is filtered > against the SoC on which we are running, keeping only the ones that are > actually useful. All the logic is written to be generic with regard to > OMAP3/OMAP4, even though OMAP3 has single PRCM event registers and OMAP4 > has two PRCM event registers. Then if OMAP5 has 3, OMAP6 4 and OMAP7 5, OMAP3 will also have an array of 5 PRCM events even though it only needs one, another argument for dynamic allocation ? > --- [snip] > @@ -246,64 +249,7 @@ static int _prcm_int_handle_wakeup(void) > c +=3D prcm_clear_mod_irqs(OMAP3430ES2_USBHOST_MOD, 1); > } > =20 > - return c; > -} > - > -/* > - * 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 irqreturn_t prcm_interrupt_handler (int irq, void *dev_id) > -{ > - u32 irqenable_mpu, irqstatus_mpu; > - int c =3D 0; > - > - irqenable_mpu =3D omap2_prm_read_mod_reg(OCP_MOD, > - OMAP3_PRM_IRQENABLE_MPU_OFFSET); > - irqstatus_mpu =3D omap2_prm_read_mod_reg(OCP_MOD, > - OMAP3_PRM_IRQSTATUS_MPU_OFFSET); > - irqstatus_mpu &=3D irqenable_mpu; > - > - do { > - if (irqstatus_mpu & (OMAP3430_WKUP_ST_MASK | > - OMAP3430_IO_ST_MASK)) { > - c =3D _prcm_int_handle_wakeup(); > - > - /* > - * Is the MPU PRCM interrupt handler racing with the > - * IVA2 PRCM interrupt handler ? > - */ > - WARN(c =3D=3D 0, "prcm: WARNING: PRCM indicated MPU wakeup " > - "but no wakeup sources are marked\n"); > - } else { > - /* XXX we need to expand our PRCM interrupt handler */ > - WARN(1, "prcm: WARNING: PRCM interrupt received, but " > - "no code to handle it (%08x)\n", irqstatus_mpu); > - } > - > - omap2_prm_write_mod_reg(irqstatus_mpu, OCP_MOD, > - OMAP3_PRM_IRQSTATUS_MPU_OFFSET); > - > - irqstatus_mpu =3D omap2_prm_read_mod_reg(OCP_MOD, > - OMAP3_PRM_IRQSTATUS_MPU_OFFSET); > - irqstatus_mpu &=3D irqenable_mpu; > - > - } while (irqstatus_mpu); > - > - return IRQ_HANDLED; > + return c ? IRQ_HANDLED : IRQ_NONE; > } > =20 > static void omap34xx_save_context(u32 *save) > @@ -875,20 +821,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 =3D omap3_prcm_irq_init(); > + if (ret) { > + pr_err("omap_prcm_irq_init() failed with %d\n", ret); > + goto err_prcm_irq_init; > + } > + > + prcm_wkup_irq =3D omap_prcm_event_to_irq("wkup"); > + prcm_io_irq =3D omap_prcm_event_to_irq("io"); > + > + ret =3D request_irq(prcm_wkup_irq, _prcm_int_handle_wakeup, > + IRQF_NO_SUSPEND, "prcm_wkup", NULL); > =20 > - ret =3D request_irq(INT_34XX_PRCM_MPU_IRQ, > - (irq_handler_t)prcm_interrupt_handler, > - IRQF_DISABLED, "prcm", NULL); > if (ret) { > - printk(KERN_ERR "request_irq failed to register for 0x%x\n", > - INT_34XX_PRCM_MPU_IRQ); > - goto err1; > + printk(KERN_ERR "Failed to request prcm_wkup irq\n"); > + goto err_prcm_wkup; > + } > + > + ret =3D request_irq(prcm_io_irq, _prcm_int_handle_wakeup, > + IRQF_NO_SUSPEND, "prcm_io", NULL); > + > + if (ret) { > + printk(KERN_ERR "Failed to request prcm_io irq\n"); > + goto err_prcm_io; > } > =20 > ret =3D pwrdm_for_each(pwrdms_setup, NULL); > if (ret) { > printk(KERN_ERR "Failed to setup powerdomains\n"); > - goto err2; > + goto err_pwrdms_setup; > } > =20 > (void) clkdm_for_each(clkdms_setup, NULL); > @@ -896,7 +857,7 @@ static int __init omap3_pm_init(void) > mpu_pwrdm =3D pwrdm_lookup("mpu_pwrdm"); > if (mpu_pwrdm =3D=3D NULL) { > printk(KERN_ERR "Failed to get mpu_pwrdm\n"); > - goto err2; > + goto err_pwrdms_setup; > } > =20 > neon_pwrdm =3D pwrdm_lookup("neon_pwrdm"); > @@ -944,14 +905,19 @@ static int __init omap3_pm_init(void) > } > =20 > omap3_save_scratchpad_contents(); > -err1: > + > return ret; > -err2: > - free_irq(INT_34XX_PRCM_MPU_IRQ, NULL); > + > + err_pwrdms_setup: > list_for_each_entry_safe(pwrst, tmp, &pwrst_list, node) { > list_del(&pwrst->node); > kfree(pwrst); > } > + err_prcm_io: > + free_irq(prcm_wkup_irq, NULL); > + err_prcm_wkup: > + omap_prcm_irq_cleanup(); > + err_prcm_irq_init: > return ret; > } > =20 > diff --git a/arch/arm/mach-omap2/prcm.c b/arch/arm/mach-omap2/prcm.c > index 2e40a5c..83cf8ae 100644 > --- a/arch/arm/mach-omap2/prcm.c > +++ b/arch/arm/mach-omap2/prcm.c > @@ -45,6 +47,209 @@ void __iomem *cm2_base; > =20 > #define MAX_MODULE_ENABLE_WAIT 100000 > =20 > +/* Maximum number of PRCM interrupt status registers */ > +#define OMAP_PRCM_MAX_NR_PENDING_REG 2 > + > +/* 64 interrupts needed on OMAP4, 32 on OMAP3 */ > +#define OMAP_PRCM_NR_IRQS 64 > + > +/* Setup for the interrupt handling based on used platform */ > +static struct omap_prcm_irq_setup *irq_setup; > + > +static struct irq_chip_generic *prcm_irq_chips[OMAP_PRCM_MAX_NR_PENDING_= REG]; I still think this would be better dynamically allocated. If this happens to increase in OMAP6/7/8... noone will convert to dynamic allocation, rather will only increase the macro above. > +/* > + * 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 =3D irq_desc_get_chip(desc); > + > + /* > + * Loop until all pending irqs are handled, since > + * generic_handle_irq() 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) > + >=3D 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(irq_setup->base_irq + virtirq); > + > + chip->irq_unmask(&desc->irq_data); can't the IRQ subsystem handle this for you ? I was expecting it would call irq_ack() and irq_unmask() automatically and you wouldn't have to do it yourself. Maybe Thomas can clear this out ? Thomas, should we call ->irq_ack() ->irq_mask ourselves here ? > +/* > + * Given a PRCM event name, returns the corresponding IRQ on which the > + * handler should be registered. > + */ > +int omap_prcm_event_to_irq(const char *name) > +{ > + int i; > + > + for (i =3D 0; i < ARRAY_SIZE(omap_prcm_irqs); i++) > + if (!strcmp(omap_prcm_irqs[i].name, name)) > + return irq_setup->base_irq + omap_prcm_irqs[i].offset; > + > + return -ENOENT; > +} > + > +/* > + * Reverses memory allocated and other setups done by > + * omap_prcm_irq_init(). > + */ > +void omap_prcm_irq_cleanup(void) > +{ > + int i; > + > + for (i =3D 0; i < OMAP_PRCM_MAX_NR_PENDING_REG; i++) { > + if (prcm_irq_chips[i]) > + irq_remove_generic_chip(prcm_irq_chips[i], 0xffffffff, > + 0, 0); > + prcm_irq_chips[i] =3D NULL; > + } > + > + irq_set_chained_handler(irq_setup->irq, NULL); > + > + if (irq_setup->base_irq > 0) > + irq_free_descs(irq_setup->base_irq, OMAP_PRCM_NR_IRQS); > + irq_setup->base_irq =3D 0; > +} > + > +/* > + * Prepare the array of PRCM events corresponding to the current SoC, > + * and set-up the chained interrupt handler mechanism. > + */ > +static int __init omap_prcm_irq_init(void) > +{ > + int i; > + struct irq_chip_generic *gc; > + struct irq_chip_type *ct; > + u32 mask[2] =3D { 0, 0 }; > + int offset; > + int max_irq =3D 0; > + > + for (i =3D 0; i < ARRAY_SIZE(omap_prcm_irqs); i++) > + if (omap_chip_is(omap_prcm_irqs[i].omap_chip)) { > + offset =3D omap_prcm_irqs[i].offset; > + if (offset < 32) > + mask[0] |=3D 1 << offset; > + else > + mask[1] |=3D 1 << (offset - 32); > + if (offset > max_irq) > + max_irq =3D offset; > + } > + > + irq_set_chained_handler(irq_setup->irq, prcm_irq_handler); > + > + irq_setup->base_irq =3D irq_alloc_descs(-1, 0, OMAP_PRCM_NR_IRQS, 0); > + > + if (irq_setup->base_irq < 0) { > + pr_err("PRCM: failed to allocate irq descs\n"); > + goto err; > + } > + > + for (i =3D 0; i <=3D max_irq / 32; i++) { > + gc =3D irq_alloc_generic_chip("PRCM", 1, > + irq_setup->base_irq + i * 32, NULL, handle_level_irq); > + > + if (!gc) { > + pr_err("PRCM: failed to allocate generic chip\n"); > + goto err; > + } > + ct =3D gc->chip_types; > + ct->chip.irq_ack =3D irq_gc_ack; > + ct->chip.irq_mask =3D irq_gc_mask_clr_bit; > + ct->chip.irq_unmask =3D irq_gc_mask_set_bit; > + > + ct->regs.ack =3D irq_setup->ack + (i << 2); > + ct->regs.mask =3D irq_setup->mask + (i << 2); > + > + irq_setup_generic_chip(gc, mask[i], 0, IRQ_NOREQUEST, 0); > + prcm_irq_chips[i] =3D gc; > + } > + return 0; > + > +err: > + omap_prcm_irq_cleanup(); > + return -ENOMEM; > +} > + > +int __init omap3_prcm_irq_init(void) > +{ > + irq_setup =3D &omap3_prcm_irq_setup; if you make this a platform_driver, there would be no need for this trickery. You could pass this as driver data. Something like: struct omap_prcm_irq_setup omap3_prcm_irq_setup =3D { .ack =3D (u32)OMAP3430_PRM_IRQSTATUS_MPU, .mask =3D (u32)OMAP3430_PRM_IRQENABLE_MPU, .pending_events =3D omap3_prm_pending_events, .irq =3D INT_34XX_PRCM_MPU_IRQ, }; struct const struct platform_device_id prcm_id_table[] __devinitconst =3D { { .name =3D "omap3-prcm", .driver_data =3D &omap3_prcm_irq_setup, }, { .name =3D "omap4-prcm", .driver_data =3D &omap4_prcm_irq_setup, }, }; MODULE_DEVICE_TABLE(platform, prcm_id_table); static struct platform_driver prcm_driver =3D { .probe =3D prcm_probe, .remove =3D __devexit_p(prcm_remove), .driver =3D { .name =3D "prcm", .pm =3D DEV_PM_OPS, }, .id_table =3D &prcm_id_table, }; or something similar. Then on probe you can make a copy of irq_setup to your driver's context structure, or only use temporarily to initialize some fields and so on... --=20 balbi --uWAbeGC4mMoqIxdF Content-Type: application/pgp-signature; name="signature.asc" Content-Description: Digital signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.11 (GNU/Linux) iQEcBAEBAgAGBQJOLaHEAAoJEAv8Txj19kN1RFAIAI94VVpXN8i3UjXvtWyarBvo d3ac5dBuBYTTgzptKNfjU5xJLB8lw012HIxlMp82BnGBg3fIP5/JNLAZHqNIKM1J 69JtjGN9d2t+uRpOp00P2O1zvSQDQ6tvyM9qs1QrMbEH6jfO/J61ZE99/5jkmTmb ITzPpW0bB9mpmkbTDpF2WU253oXg2aMi78wT5JA0LAKfwtzoNbJoPZrw6BDV8lf1 lsG+PnWU1DcrEeVnPR0SrgPWan/pyE25B1uCABBQN8RZk78ccESjrIpmfxSRfNYz SFx1ZKAGKY2RiDVngWOmWZnOwEKO5iPtbreMF/xcgZ2Jijs352+mvUauD8I0/nk= =8+XV -----END PGP SIGNATURE----- --uWAbeGC4mMoqIxdF--