lkml.org 
[lkml]   [2017]   [May]   [30]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: [PATCH 4/6] irqchip: irq-mvebu-icu: new driver for Marvell ICU
    From
    Date
    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...

    \
     
     \ /
      Last update: 2017-05-30 15:42    [W:3.111 / U:1.100 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site