Re: [PATCHv3 1/6] omap: prcm: switch to a chained IRQ handler mechanism

[Tero Kristo <t-kristo@ti.com>]

On Thu, 2011-06-23 at 10:19 +0200, Tony Lindgren wrote:
> * Tero Kristo <t-kristo@ti.com> [110622 09:38]:
> > 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
> > 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 individual PRCM events for OMAP3 and OMAP4 being
> > described in arch/arm/mach-omap2/prcm3xxx.c and
> > arch/arm/mach-omap2/prcm4xxx.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.
> 
> Nice, this makes things more generic. Some comments below.
>  
> > +int omap_prcm_irq_init(void)
> > +{
> > +	int i, j;
> > +	struct omap_prcm_irq *unfiltered_irqs;
> > +	unsigned unfiltered_irqs_nr;
> > +
> > +	if (cpu_is_omap34xx() || cpu_is_omap3630()) {
> > +		unfiltered_irqs          = omap_prcm_3xxx_irqs;
> > +		unfiltered_irqs_nr       = omap_prcm_3xxx_irqs_nr;
> > +		omap_prcm_mask_event     = omap3_prcm_mask_event;
> > +		omap_prcm_unmask_event   = omap3_prcm_unmask_event;
> > +		omap_prcm_ack_event      = omap3_prcm_ack_event;
> > +		omap_prcm_pending_events = omap3_prcm_pending_events;
> > +		irq_set_chained_handler(INT_34XX_PRCM_MPU_IRQ,
> > +					prcm_irq_handler);
> > +	} else if (cpu_is_omap44xx()) {
> > +		unfiltered_irqs          = omap_prcm_4xxx_irqs;
> > +		unfiltered_irqs_nr       = omap_prcm_4xxx_irqs_nr;
> > +		omap_prcm_mask_event     = omap4_prcm_mask_event;
> > +		omap_prcm_unmask_event   = omap4_prcm_unmask_event;
> > +		omap_prcm_ack_event      = omap4_prcm_ack_event;
> > +		omap_prcm_pending_events = omap4_prcm_pending_events;
> > +		irq_set_chained_handler(OMAP44XX_IRQ_PRCM, prcm_irq_handler);
> > +	} else {
> > +		return -ENODEV;
> > +	}
> > +
> > +	for (i = 0; i < unfiltered_irqs_nr; i++)
> > +		if (omap_chip_is(unfiltered_irqs[i].omap_chip))
> > +			omap_prcm_irqs_nr++;
> > +
> > +	omap_prcm_irqs = kmalloc(omap_prcm_irqs_nr *
> > +				 sizeof(struct omap_prcm_irq),
> > +				 GFP_KERNEL);
> > +	if (!omap_prcm_irqs)
> > +		return -ENOMEM;
> > +
> > +	for (i = 0, j = 0; i < unfiltered_irqs_nr; i++)
> > +		if (omap_chip_is(unfiltered_irqs[i].omap_chip)) {
> > +			memcpy(&omap_prcm_irqs[j], &unfiltered_irqs[i],
> > +			       sizeof(struct omap_prcm_irq));
> > +			j++;
> > +		}
> > +
> > +	for (i = OMAP_PRCM_IRQ_BASE; i < OMAP_PRCM_IRQ_END; i++) {
> > +		irq_set_chip(i, &prcm_irq_chip);
> > +		irq_set_handler(i, handle_level_irq);
> > +		set_irq_flags(i, IRQF_VALID);
> > +	}
> > +
> > +	return 0;
> > +}
> 
> Please make omap_prcm_irq_init generic so you pass it the configuration.
> Otherwise you have to add more else if cpu_is_omap code for each new omap.
> Then you can add just an arch_initcall for each new omap to call
> omap_prcm_irq_init. This will also make it easier to add support for
> initializing things from device tree for omap_prcm_irq_init.

Yea, can do this.

> 
> > +/*
> > + * Reverses memory allocated and other setups done by
> > + * omap_prcm_irq_init().
> > + */
> > +void omap_prcm_irq_cleanup(void)
> > +{
> > +	int i;
> > +
> > +	for (i = OMAP_PRCM_IRQ_BASE; i < OMAP_PRCM_IRQ_END; i++) {
> > +		irq_set_chip(i, NULL);
> > +		irq_set_handler(i, NULL);
> > +		set_irq_flags(i, 0);
> > +	}
> > +
> > +	kfree(omap_prcm_irqs);
> > +
> > +	if (cpu_is_omap34xx() || cpu_is_omap3630()) {
> > +		irq_set_chained_handler(INT_34XX_PRCM_MPU_IRQ, NULL);
> > +	} else {
> > +		irq_set_chained_handler(OMAP44XX_IRQ_PRCM, NULL);
> > +	}
> > +}
> 
> Please get rid of the cpu_is_omap tests here too so prcm.c is
> generic for the new code added.

Same.

> 
> > +struct omap_prcm_irq  __initdata omap_prcm_3xxx_irqs[] = {
> > +	OMAP_PRCM_IRQ("wkup",                  0,
> > +		      CHIP_IS_OMAP3430 | CHIP_GE_OMAP3630ES1_1),
> > +	OMAP_PRCM_IRQ("evgenon",               2,
> > +		      CHIP_IS_OMAP3430 | CHIP_GE_OMAP3630ES1_1),
> > +	OMAP_PRCM_IRQ("evgenoff",              3,
> > +		      CHIP_IS_OMAP3430 | CHIP_GE_OMAP3630ES1_1),
> ...
> 
> Please note consider that this data will be coming from device
> tree and will disappear from here. We won't be merging any new
> data after v3.1 unless it comes from device tree. So this too
> will need to be converted because we won't be able to add support
> for new omaps otherwise.

This part I am not too sure what you mean with this. Do you have some
info / examples about the device tree somewhere and how this data should
be converted?

> 
> Also, please Cc linux-arm-kernel too.
> 
> Regards,
> 
> Tony



Texas Instruments Oy, Tekniikantie 12, 02150 Espoo. Y-tunnus: 0115040-6. Kotipaikka: Helsinki