Re: [PATCHv6 01/11] omap: prcm: switch to a chained IRQ handler mechanism

[Felipe Balbi <balbi@ti.com>]

[-- Attachment #1: Type: text/plain, Size: 12896 bytes --]

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.
> 
> 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 += prcm_clear_mod_irqs(OMAP3430ES2_USBHOST_MOD, 1);
>  	}
>  
> -	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 = 0;
> -
> -	irqenable_mpu = omap2_prm_read_mod_reg(OCP_MOD,
> -					 OMAP3_PRM_IRQENABLE_MPU_OFFSET);
> -	irqstatus_mpu = omap2_prm_read_mod_reg(OCP_MOD,
> -					 OMAP3_PRM_IRQSTATUS_MPU_OFFSET);
> -	irqstatus_mpu &= irqenable_mpu;
> -
> -	do {
> -		if (irqstatus_mpu & (OMAP3430_WKUP_ST_MASK |
> -				     OMAP3430_IO_ST_MASK)) {
> -			c = _prcm_int_handle_wakeup();
> -
> -			/*
> -			 * Is the MPU PRCM interrupt handler racing with the
> -			 * IVA2 PRCM interrupt handler ?
> -			 */
> -			WARN(c == 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 = omap2_prm_read_mod_reg(OCP_MOD,
> -					OMAP3_PRM_IRQSTATUS_MPU_OFFSET);
> -		irqstatus_mpu &= irqenable_mpu;
> -
> -	} while (irqstatus_mpu);
> -
> -	return IRQ_HANDLED;
> +	return c ? IRQ_HANDLED : IRQ_NONE;
>  }
>  
>  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 = 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 = 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, "prcm_wkup", NULL);
>  
> -	ret = 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 = 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;
>  	}
>  
>  	ret = pwrdm_for_each(pwrdms_setup, NULL);
>  	if (ret) {
>  		printk(KERN_ERR "Failed to setup powerdomains\n");
> -		goto err2;
> +		goto err_pwrdms_setup;
>  	}
>  
>  	(void) clkdm_for_each(clkdms_setup, NULL);
> @@ -896,7 +857,7 @@ static int __init omap3_pm_init(void)
>  	mpu_pwrdm = pwrdm_lookup("mpu_pwrdm");
>  	if (mpu_pwrdm == NULL) {
>  		printk(KERN_ERR "Failed to get mpu_pwrdm\n");
> -		goto err2;
> +		goto err_pwrdms_setup;
>  	}
>  
>  	neon_pwrdm = pwrdm_lookup("neon_pwrdm");
> @@ -944,14 +905,19 @@ static int __init omap3_pm_init(void)
>  	}
>  
>  	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;
>  }
>  
> 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;
>  
>  #define MAX_MODULE_ENABLE_WAIT		100000
>  
> +/* 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 = 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)
> +		    >= 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 = 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 = 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] = 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 = 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] = { 0, 0 };
> +	int offset;
> +	int max_irq = 0;
> +
> +	for (i = 0; i < ARRAY_SIZE(omap_prcm_irqs); i++)
> +		if (omap_chip_is(omap_prcm_irqs[i].omap_chip)) {
> +			offset = omap_prcm_irqs[i].offset;
> +			if (offset < 32)
> +				mask[0] |= 1 << offset;
> +			else
> +				mask[1] |= 1 << (offset - 32);
> +			if (offset > max_irq)
> +				max_irq = offset;
> +		}
> +
> +	irq_set_chained_handler(irq_setup->irq, prcm_irq_handler);
> +
> +	irq_setup->base_irq = 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 = 0; i <= max_irq / 32; i++) {
> +		gc = 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 = gc->chip_types;
> +		ct->chip.irq_ack = irq_gc_ack;
> +		ct->chip.irq_mask = irq_gc_mask_clr_bit;
> +		ct->chip.irq_unmask = irq_gc_mask_set_bit;
> +
> +		ct->regs.ack = irq_setup->ack + (i << 2);
> +		ct->regs.mask = irq_setup->mask + (i << 2);
> +
> +		irq_setup_generic_chip(gc, mask[i], 0, IRQ_NOREQUEST, 0);
> +		prcm_irq_chips[i] = gc;
> +	}
> +	return 0;
> +
> +err:
> +	omap_prcm_irq_cleanup();
> +	return -ENOMEM;
> +}
> +
> +int __init omap3_prcm_irq_init(void)
> +{
> +	irq_setup = &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 = {
	.ack		= (u32)OMAP3430_PRM_IRQSTATUS_MPU,
	.mask		= (u32)OMAP3430_PRM_IRQENABLE_MPU,
	.pending_events	= omap3_prm_pending_events,
	.irq		= INT_34XX_PRCM_MPU_IRQ,
};

struct const struct platform_device_id prcm_id_table[] __devinitconst =
{
	{
		.name		= "omap3-prcm",
		.driver_data	= &omap3_prcm_irq_setup,
	},
	{
		.name		= "omap4-prcm",
		.driver_data	= &omap4_prcm_irq_setup,
	},
};
MODULE_DEVICE_TABLE(platform, prcm_id_table);

static struct platform_driver prcm_driver = {
	.probe		= prcm_probe,
	.remove		= __devexit_p(prcm_remove),
	.driver		= {
		.name	= "prcm",
		.pm	= DEV_PM_OPS,
	},
	.id_table	= &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...

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 490 bytes --]