Re: [PATCH v2 05/12] irqchip/gic: Prepare for more than 16 PPIs

[Marc Zyngier]

Hi Julien,

On 22/08/2019 17:11, Julien wrote:
> Hi Marc,
> 
> On 06/08/19 11:01, Marc Zyngier wrote:
>> GICv3.1 allows up to 80 PPIs (16 legaci PPIs and 64 Extended PPIs),
>> meaning we can't just leave the old 16 hardcoded everywhere.
>>
>> We also need to add the infrastructure to discover the number of PPIs
>> on a per redistributor basis, although we still pretend there is only
>> 16 of them for now.
>>
>> No functional change.
>>
>> Signed-off-by: Marc Zyngier <maz@xxxxxxxxxx>
>> ---
>>   drivers/irqchip/irq-gic-common.c | 19 ++++++++++++-------
>>   drivers/irqchip/irq-gic-common.h |  2 +-
>>   drivers/irqchip/irq-gic-v3.c     | 22 +++++++++++++++-------
>>   drivers/irqchip/irq-gic.c        |  2 +-
>>   drivers/irqchip/irq-hip04.c      |  2 +-
>>   5 files changed, 30 insertions(+), 17 deletions(-)
>>
>> diff --git a/drivers/irqchip/irq-gic-common.c b/drivers/irqchip/irq-gic-common.c
>> index 6900b6f0921c..14110db01c05 100644
>> --- a/drivers/irqchip/irq-gic-common.c
>> +++ b/drivers/irqchip/irq-gic-common.c
>> @@ -128,26 +128,31 @@ void gic_dist_config(void __iomem *base, int gic_irqs,
>>   		sync_access();
>>   }
>>   
>> -void gic_cpu_config(void __iomem *base, void (*sync_access)(void))
>> +void gic_cpu_config(void __iomem *base, int nr, void (*sync_access)(void))
>>   {
>>   	int i;
>>   
>>   	/*
>>   	 * Deal with the banked PPI and SGI interrupts - disable all
>> -	 * PPI interrupts, ensure all SGI interrupts are enabled.
>> -	 * Make sure everything is deactivated.
>> +	 * private interrupts. Make sure everything is deactivated.
>>   	 */
>> -	writel_relaxed(GICD_INT_EN_CLR_X32, base + GIC_DIST_ACTIVE_CLEAR);
>> -	writel_relaxed(GICD_INT_EN_CLR_PPI, base + GIC_DIST_ENABLE_CLEAR);
>> -	writel_relaxed(GICD_INT_EN_SET_SGI, base + GIC_DIST_ENABLE_SET);
>> +	for (i = 0; i < nr; i += 32) {
> 
> You added "nr" as argument but if "nr" isn't a multiple of 32 weird 
> things might happen, no?
>
> It would be worth specifying that somewhere, and checking it with a WARN().

TBH, I'm unsure whether that's worth it. The architecture is completely
built around having the private interrupts in blocks of 32, and you can
only get something wrong if you misdecode the number of interrupts from
the registers.

> Maybe it might be worth reducing the granularity to manipulating 16 irqs 
> since there are 16 SGI + 16 PPI + 64 EPPI, but that might not be very 
> useful right now.

I don't see what this brings us at this point. The architecture doesn't
seem to go in the direction of adding more SGIs, so we're pretty safe on
that front...

Thanks,

	M.
-- 
Jazz is not dead, it just smells funny...