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...
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
