Re: [RFC PATCH v1 17/40] metag: IRQ handling

[James Hogan <james.hogan@imgtec.com>]

On 09/11/12 14:12, Arnd Bergmann wrote:
> On Wednesday 31 October 2012, James Hogan wrote:
> 
>> diff --git a/arch/metag/include/asm/irq.h b/arch/metag/include/asm/irq.h
>> new file mode 100644
>> index 0000000..da24886
>> --- /dev/null
>> +++ b/arch/metag/include/asm/irq.h
>> +#ifndef HW_IRQS
>> +#define HW_IRQS  (HWSTATMETA_OFFSET_MAX + HWSTATEXT_OFFSET_MAX + \
>> +		HWSTATEXT2_OFFSET_MAX + HWSTATEXT4_OFFSET_MAX + \
>> +		HWSTATEXT6_OFFSET_MAX)
>> +#endif
>> +
>> +#define META_IRQS 32
>> +
>> +#define NR_IRQS (META_IRQS + HW_IRQS)
> 
> I think you should use sparse IRQs right from the start and avoid
> hardcoding interrupt numbers from the start.

I presume core irq numbers need hard coding (e.g. those required by core
code like cross-hardware-thread irqs and perf counters)? Or would the
recommended way be to have a function which is given the core hw irq
number and converts/maps a virtual irq for it using the irq domain?

>> +/**
>> + * meta_intc_stat_addr() - Get the address of a HWSTATEXT register
>> + * @hw:		Hardware IRQ number (within external trigger block)
>> + *
>> + * Returns:	Address of a HWSTATEXT register containing the status bit for
>> + *		the specified hardware IRQ number
>> + */
>> +static void __iomem *meta_intc_stat_addr(irq_hw_number_t hw)
>> +{
>> +	return (void __iomem *)(HWSTATEXT +
>> +				HWSTAT_STRIDE * meta_intc_bank(hw));
>> +}
> 
> Hardcoding register addresses like this is considered bad style, even for
> an interrupt controller. Better use ioremap to initialize a pointer
> dynamically at boot time.

All these registers are part of the core Meta registers in the non-MMU
region and so are always fixed. I have altered this file to use
metag_in32/metag_out32 instead of readl/writel to make that clearer.

>> +struct irq_chip meta_intc_edge_chip = {
>> +	.irq_startup		= meta_intc_startup_irq,
>> +	.irq_shutdown		= meta_intc_shutdown_irq,
>> +	.irq_ack		= meta_intc_ack_irq,
>> +	.irq_mask		= meta_intc_mask_irq,
>> +	.irq_unmask		= meta_intc_unmask_irq,
>> +	.irq_set_type		= meta_intc_irq_set_type,
>> +	.irq_set_affinity	= meta_intc_set_affinity,
>> +	.flags			= META_INTC_CHIP_FLAGS,
>> +};
>> +
>> +struct irq_chip meta_intc_level_chip = {
>> +	.irq_startup		= meta_intc_startup_irq,
>> +	.irq_shutdown		= meta_intc_shutdown_irq,
>> +	.irq_set_type		= meta_intc_irq_set_type,
>> +	.irq_mask		= meta_intc_mask_irq,
>> +	.irq_unmask		= meta_intc_unmask_irq,
>> +	.irq_set_affinity	= meta_intc_set_affinity,
>> +	.flags			= META_INTC_CHIP_FLAGS,
>> +};
> 
> I would recommend trying to put the external irq_chip code into
> drivers/irqchip, which we are currently establishing. Right now,
> that directory is still a bit dependent on the ARM architecture,
> but IMHO that is an even stronger reason to use it for a new
> architecture, so we can make it arch independent in the process,
> without the fear of breaking existing users. After it works
> on multiple architectures, we can start migrating the others.

I'll look into this.

Thanks
James