Re: [PATCH v4 2/2] powerpc: make MPIC honor the "pic-no-reset" device tree property

[Meador Inge <meador_inge@mentor.com>]

On 03/01/2011 09:22 PM, Benjamin Herrenschmidt wrote:
>
>> diff --git a/arch/powerpc/include/asm/mpic.h b/arch/powerpc/include/asm/mpic.h
>> index e000cce..7e1be12 100644
>> --- a/arch/powerpc/include/asm/mpic.h
>> +++ b/arch/powerpc/include/asm/mpic.h
>> @@ -325,6 +325,8 @@ struct mpic
>>   #ifdef CONFIG_PM
>>   	struct mpic_irq_save	*save_data;
>>   #endif
>> +
>> +	int cpu;
>>   };
>
> Why ? The MPIC isn't specifically associated with a CPU and whatever we
> pick as default during boot isn't relevant later on, so I don't see why
> we should have global permanent state here.

I agree.  I shouldn't have cached that.  We should probably introduce a 
helper function to get the cpuid, though.  The:

	unsigned int cpu = 0;

	if (mpic->flags & MPIC_PRIMARY)
		cpu = hard_smp_processor_id();

code is scattered in three places: '_mpic_cpu_write', '_mpic_cpu_read', 
and 'mpic_init'.

>>   /* Check if we have one of those nice broken MPICs with a flipped endian on
>> @@ -622,6 +631,14 @@ static unsigned int mpic_is_ipi(struct mpic *mpic, unsigned int irq)
>>   	return (src>= mpic->ipi_vecs[0]&&  src<= mpic->ipi_vecs[3]);
>>   }
>>
>> +/* Determine if the linux irq is a timer interrupt */
>> +static unsigned int mpic_is_timer_interrupt(struct mpic *mpic, unsigned int irq)
>> +{
>> +	unsigned int src = mpic_irq_to_hw(irq);
>> +
>> +	return (src>= mpic->timer_vecs[0]&&  src<= mpic->timer_vecs[3]);
>> +}
>> +
>>
>>   /* Convert a cpu mask from logical to physical cpu numbers. */
>>   static inline u32 mpic_physmask(u32 cpumask)
>> @@ -967,6 +984,15 @@ static int mpic_host_map(struct irq_host *h, unsigned int virq,
>>   	if (hw>= mpic->irq_count)
>>   		return -EINVAL;
>>
>> +	/* If the MPIC was reset, then all vectors have already been
>> +	 * initialized.  Otherwise, the appropriate vector needs to be
>> +	 * initialized here to ensure that only used sources are setup with
>> +	 * a vector.
>> +	 */
>> +	if (mpic->flags&  MPIC_NO_RESET)
>> +		if (!(mpic_is_ipi(mpic, hw) || mpic_is_timer_interrupt(mpic, hw)))
>> +			mpic_init_vector(mpic, hw);
>> +
>
> The above isn't useful. Of those two registers you want to initialize,
> afaik, one (the destination) will be initialized by the core calling
> into set_affinity when the interrupt is requested, and the other one is

AFAIK, we can't rely on 'set_affinity' always being called.  I don't 
think it is called at all when !defined(CONFIG_SMP) and if it was, then 
that would be an error:

	/* include/linux/irq.h */

	#else /* CONFIG_SMP */

	static inline int irq_set_affinity(unsigned int irq,
		const struct cpumask *m)
	{
		return -EINVAL;
	}

> partially initialized in set_type, I'd say just modify set_type to
> initialize the source as well, and problem solved, no ?

The priority has to be initialized as well.  They could both be done in 
'mpic_set_irq_type', but that seems like a weird place since it has 
nothing to do with actually setting the type.

Since we already have 'mpic_irq_set_priority' and 'mpic_set_vector', 
perhaps a better option is to add 'mpic_set_destination' and put the 
following in 'mpic_host_map' (using the cpuid helper function suggested 
above):

	/* Lazy source init when MPIC_NO_RESET */
	if (!mpic_is_ipi(mpic, hw) && (mpic->flags & MPIC_NO_RESET)) {
		mpic_set_vector(virq, hw);
		mpic_set_destination(virq, mpic_cpuid(mpic));
		mpic_irq_set_priority(virq, 8);
	}

It is more overhead, but it reads well.  Thoughts?

> Or is there a chance that the interrupt was left unmasked ?

There shouldn't be.  The original idea was that either the boot program 
would leave it masked or one of the AMP OSes would boot without 
'pic-no-reset', which would mask everything.  Most likely the boot program.

> I think it would be kosher to mask it in set_type unconditionally, I don't think it's ever supposed
> to be called for an enabled interrupt.

I will look into that.

Thanks,

-- 
Meador Inge     | meador_inge AT mentor.com
Mentor Embedded | http://www.mentor.com/embedded-software