Re: [PATCH 4/6] irqchip: irq-mvebu-icu: new driver for Marvell ICU

[Marc Zyngier <marc.zyngier@arm.com>]

On 30/05/17 14:17, Thomas Petazzoni wrote:
> Hello,
> 
> On Tue, 30 May 2017 14:06:52 +0100, Marc Zyngier wrote:
> 
>>> Would drivers/irqchip/irq-mvebu-gicp.h, included by both
>>> irq-mvebu-gicp.c and irq-mvebu-icu.c be fine for you?  
>>
>> Sure, that'd be fine, assuming that it is necessary (see below).
> 
> Right, if we merge everything into one file, then it's simpler :)
> 
>>>> What's the relationship between ICU_MAX_IRQS and
>>>> IRQS_PER_ICU/ICU_MAX_SPI_IRQ_IN_GIC, if any? Is there only a single ICU?
>>>> Or can you have multiple ones?  
>>>
>>> There is one ICU per CP. The Armada 7K SoC has one CP, the Armada 8K
>>> SoC has two CPs. Therefore on Armada 8K, you have two ICUs, one per CP.  
>>
>> OK. Is there any restriction on which SPI an ICU can generate?
> 
> Not that I'm aware of. Even though the fact that there are two ranges
> of SPI interrupts, which might make one think there is one range per
> ICU, this is not the case: any ICU can trigger any SPI within those
> ranges. Which is why the ICU driver handles the 128 available SPIs
> through a single global bitmap rather than a per-ICU bitmap.

OK.

> 
>>> But I see your point: there is in fact no direct relation between the
>>> number of GICP SPI interrupts reserved and the number of ICUs and
>>> interrupts per ICU.  
>>
>> Indeed. And maybe we should have an instance of the ICU device per CP.
> 
> Not sure what you mean here: we already have one instance of the ICU
> device per CP.
> 
> armada-cp110-master.dtsi describes the ICU in the master CP,
> armada-cp110-slave.dtsi describes the ICU in the slave CP. So in the
> patch series I have posted, on an Armada 8K that has two CPs, the
> ->probe() of irq-mvebu-icu.c gets called twice, once per ICU, and we
> have one instance of the ICU per CP, as expected.
> 
> Am I missing something?

No, I'm just being remarkably thick today. Sorry about the noise.

> 
>>>>> +#define ICU_GIC_SPI_BASE0	64
>>>>> +#define ICU_GIC_SPI_BASE1	288    
>>>>
>>>> My own gut feeling is that there will be another version of this IP one
>>>> of these days, with different bases. Should we bite the bullet right
>>>> away and put those in DT?  
>>>
>>> Where should these properties go? Under the gicp DT node, or under the
>>> ICU DT node?  
>>
>> If the ICU has no knowledge of the SPI it can generate, I'd rather put
>> that in the GICP node.
> 
> Something like:
> 
> 	marvell,spi-ranges = <64 64>, <288 64>;
> 
> And then the ICU ->probe() routine walks the marvell,gicp phandle, find
> the gicp node and parses this information?

Either that, or you keep a separate GICP probing. Up to you, really.

> 
>>> We in fact don't really care about how many ICUs we have here. We have
>>> 128 GICP SPI interrupts available, in ranges:
>>>
>>>  - ICU_GIC_SPI_BASE0 ; ICU_GIC_SPI_BASE0 + 64
>>>
>>>  - ICU_GIC_SPI_BASE1 ; ICU_GIC_SPI_BASE1 + 64
>>>
>>> The icu_irq_alloc bitmap is a global one, which allows to allocate one
>>> GICP SPI interrupts amongst the available 128 interrupts, and this
>>> function simply allows to map the index in this bitmap (from 0 to 127)
>>> to the corresponding GICP SPI interrupt.  
>>
>> That makes a lot more sense now, thanks.
> 
> I should probably add a comment explaining this in the driver.

Yeah, that'd help.

>>>>> +	 */
>>>>> +	if (hwirq == ICU_SATA0_IRQ_INT || hwirq == ICU_SATA1_IRQ_INT) {
>>>>> +		writel(icu_int, icu->base + ICU_INT_CFG(ICU_SATA0_IRQ_INT));
>>>>> +		writel(icu_int, icu->base + ICU_INT_CFG(ICU_SATA1_IRQ_INT));
>>>>> +	}    
>>>>
>>>> Aren't you wasting an SPI here?  
>>>
>>> No: we allocate a single SPI, icu_int. What we're doing here is that we
>>> have two different wired interrupts in the CP that are "connected" to
>>> the same GICP SPI interrupt.  
>>
>> But if both ports are enabled, you're going to allocate one SPI per call
>> to this function, and the last one wins (you never "remember" that you
>> have configured one port already, and always allocate a new interrupt).
> 
> Yes, but no, because the DT only declares one of the two interrupts
> currently:
> 
>  			cpm_sata0: sata@540000 {
>  				compatible = "marvell,armada-8k-ahci",
>  					     "generic-ahci";
>  				reg = <0x540000 0x30000>;
> -				interrupts = <GIC_SPI 63 IRQ_TYPE_LEVEL_HIGH>;
> +				interrupts = <ICU_GRP_NSR 107 IRQ_TYPE_LEVEL_HIGH>;
> 
> Yes, needs to be fixed, with proper changes to the AHCI driver, but
> that's a separate matter.

OK. Can you expose both interrupts in the DT already, assuming this
doesn't break anything? Wasting an SPI is not that big a deal, and I
want to make sure we'll have a smooth upgrade path when transitioning
from the irqchip hack to the ahci solution.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...