From: Marc Zyngier <marc.zyngier@arm.com> To: Thomas Petazzoni <thomas.petazzoni@free-electrons.com> Cc: Thomas Gleixner <tglx@linutronix.de>, Jason Cooper <jason@lakedaemon.net>, linux-kernel@vger.kernel.org, devicetree@vger.kernel.org, Rob Herring <robh+dt@kernel.org>, Ian Campbell <ijc+devicetree@hellion.org.uk>, Pawel Moll <pawel.moll@arm.com>, Mark Rutland <mark.rutland@arm.com>, Kumar Gala <galak@codeaurora.org>, Andrew Lunn <andrew@lunn.ch>, Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>, Gregory Clement <gregory.clement@free-electrons.com>, linux-arm-kernel@lists.infradead.org, Nadav Haklai <nadavh@marvell.com>, Hanna Hawa <hannah@marvell.com>, Yehuda Yitschak <yehuday@marvell.com>, Antoine Tenart <antoine.tenart@free-electrons.com> Subject: Re: [PATCH 4/6] irqchip: irq-mvebu-icu: new driver for Marvell ICU Date: Tue, 30 May 2017 14:40:51 +0100 [thread overview] Message-ID: <d9616800-83c4-7ae2-19fa-0fb90adbcaae@arm.com> (raw) In-Reply-To: <20170530151755.12d0d5e3@free-electrons.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...
WARNING: multiple messages have this Message-ID (diff)
From: marc.zyngier@arm.com (Marc Zyngier) To: linux-arm-kernel@lists.infradead.org Subject: [PATCH 4/6] irqchip: irq-mvebu-icu: new driver for Marvell ICU Date: Tue, 30 May 2017 14:40:51 +0100 [thread overview] Message-ID: <d9616800-83c4-7ae2-19fa-0fb90adbcaae@arm.com> (raw) In-Reply-To: <20170530151755.12d0d5e3@free-electrons.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 at 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...
next prev parent reply other threads:[~2017-05-30 13:40 UTC|newest] Thread overview: 107+ messages / expand[flat|nested] mbox.gz Atom feed top 2017-05-30 9:16 [PATCH 0/6] Add support for the ICU unit in Marvell Armada 7K/8K Thomas Petazzoni 2017-05-30 9:16 ` Thomas Petazzoni 2017-05-30 9:16 ` Thomas Petazzoni 2017-05-30 9:16 ` [PATCH 1/6] dt-bindings: interrupt-controller: add DT binding for the Marvell GICP Thomas Petazzoni 2017-05-30 9:16 ` Thomas Petazzoni 2017-05-30 9:16 ` [PATCH 2/6] dt-bindings: interrupt-controller: add DT binding for the Marvell ICU Thomas Petazzoni 2017-05-30 9:16 ` Thomas Petazzoni 2017-05-30 10:37 ` Marc Zyngier 2017-05-30 10:37 ` Marc Zyngier 2017-05-30 10:37 ` Marc Zyngier 2017-05-30 11:41 ` Thomas Petazzoni 2017-05-30 11:41 ` Thomas Petazzoni 2017-05-30 11:41 ` Thomas Petazzoni 2017-05-30 9:16 ` [PATCH 3/6] irqchip: irq-mvebu-gicp: new driver for Marvell GICP Thomas Petazzoni 2017-05-30 9:16 ` Thomas Petazzoni 2017-05-30 13:55 ` Marc Zyngier 2017-05-30 13:55 ` Marc Zyngier 2017-05-30 13:55 ` Marc Zyngier 2017-05-30 14:54 ` Thomas Petazzoni 2017-05-30 14:54 ` Thomas Petazzoni 2017-05-30 14:54 ` Thomas Petazzoni 2017-05-30 15:17 ` Marc Zyngier 2017-05-30 15:17 ` Marc Zyngier 2017-05-30 15:17 ` Marc Zyngier 2017-05-30 15:25 ` Thomas Petazzoni 2017-05-30 15:25 ` Thomas Petazzoni 2017-05-30 15:33 ` Marc Zyngier 2017-05-30 15:33 ` Marc Zyngier 2017-05-30 15:33 ` Marc Zyngier 2017-05-30 9:16 ` [PATCH 4/6] irqchip: irq-mvebu-icu: new driver for Marvell ICU Thomas Petazzoni 2017-05-30 9:16 ` Thomas Petazzoni 2017-05-30 11:10 ` Marc Zyngier 2017-05-30 11:10 ` Marc Zyngier 2017-05-30 12:05 ` Thomas Petazzoni 2017-05-30 12:05 ` Thomas Petazzoni 2017-05-30 12:05 ` Thomas Petazzoni 2017-05-30 13:06 ` Marc Zyngier 2017-05-30 13:06 ` Marc Zyngier 2017-05-30 13:06 ` Marc Zyngier 2017-05-30 13:17 ` Thomas Petazzoni 2017-05-30 13:17 ` Thomas Petazzoni 2017-05-30 13:17 ` Thomas Petazzoni 2017-05-30 13:40 ` Marc Zyngier [this message] 2017-05-30 13:40 ` Marc Zyngier 2017-06-25 6:47 ` [EXT] " Yehuda Yitschak 2017-06-25 6:47 ` Yehuda Yitschak 2017-06-25 6:47 ` Yehuda Yitschak 2017-05-30 12:04 ` Antoine Tenart 2017-05-30 12:04 ` Antoine Tenart 2017-05-30 12:04 ` Antoine Tenart 2017-05-30 12:19 ` Russell King - ARM Linux 2017-05-30 12:19 ` Russell King - ARM Linux 2017-05-30 12:19 ` Russell King - ARM Linux 2017-05-30 12:33 ` Thomas Petazzoni 2017-05-30 12:33 ` Thomas Petazzoni 2017-05-30 12:33 ` Thomas Petazzoni 2017-05-30 12:56 ` Russell King - ARM Linux 2017-05-30 12:56 ` Russell King - ARM Linux 2017-05-30 12:56 ` Russell King - ARM Linux 2017-05-30 13:27 ` Andrew Lunn 2017-05-30 13:27 ` Andrew Lunn 2017-05-30 13:27 ` Andrew Lunn 2017-05-30 13:34 ` Thomas Petazzoni 2017-05-30 13:34 ` Thomas Petazzoni 2017-05-30 13:34 ` Thomas Petazzoni 2017-05-30 13:42 ` Russell King - ARM Linux 2017-05-30 13:42 ` Russell King - ARM Linux 2017-05-30 13:42 ` Russell King - ARM Linux 2017-05-30 14:03 ` Andrew Lunn 2017-05-30 14:03 ` Andrew Lunn 2017-05-30 14:03 ` Andrew Lunn 2017-05-30 14:36 ` Russell King - ARM Linux 2017-05-30 14:36 ` Russell King - ARM Linux 2017-05-30 14:36 ` Russell King - ARM Linux 2017-05-30 14:26 ` Thomas Petazzoni 2017-05-30 14:26 ` Thomas Petazzoni 2017-05-30 14:26 ` Thomas Petazzoni 2017-05-30 14:39 ` Russell King - ARM Linux 2017-05-30 14:39 ` Russell King - ARM Linux 2017-05-30 14:39 ` Russell King - ARM Linux 2017-05-30 14:49 ` Andrew Lunn 2017-05-30 14:49 ` Andrew Lunn 2017-05-30 14:49 ` Andrew Lunn 2017-05-30 15:08 ` Russell King - ARM Linux 2017-05-30 15:08 ` Russell King - ARM Linux 2017-05-30 13:28 ` Thomas Petazzoni 2017-05-30 13:28 ` Thomas Petazzoni 2017-05-30 13:28 ` Thomas Petazzoni 2017-05-30 9:16 ` [PATCH 5/6] arm64: marvell: enable ICU and GICP drivers Thomas Petazzoni 2017-05-30 9:16 ` Thomas Petazzoni 2017-05-30 9:16 ` Thomas Petazzoni 2017-05-30 9:16 ` [PATCH 6/6] arm64: dts: marvell: enable GICP and ICU on Armada 7K/8K Thomas Petazzoni 2017-05-30 9:16 ` Thomas Petazzoni 2017-05-30 14:02 ` Marc Zyngier 2017-05-30 14:02 ` Marc Zyngier 2017-05-30 14:02 ` Marc Zyngier 2017-05-30 14:28 ` Thomas Petazzoni 2017-05-30 14:28 ` Thomas Petazzoni 2017-05-30 14:28 ` Thomas Petazzoni 2017-05-30 9:16 ` [PATCH 6/6] arm64: dts: marvell: enable GICP and ICU Thomas Petazzoni 2017-05-30 9:16 ` Thomas Petazzoni 2017-05-30 9:22 ` Thomas Petazzoni 2017-05-30 9:22 ` Thomas Petazzoni 2017-05-30 9:22 ` Thomas Petazzoni 2017-05-30 14:15 ` [PATCH 0/6] Add support for the ICU unit in Marvell Armada 7K/8K Marc Zyngier 2017-05-30 14:15 ` Marc Zyngier 2017-05-30 14:15 ` Marc Zyngier
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=d9616800-83c4-7ae2-19fa-0fb90adbcaae@arm.com \ --to=marc.zyngier@arm.com \ --cc=andrew@lunn.ch \ --cc=antoine.tenart@free-electrons.com \ --cc=devicetree@vger.kernel.org \ --cc=galak@codeaurora.org \ --cc=gregory.clement@free-electrons.com \ --cc=hannah@marvell.com \ --cc=ijc+devicetree@hellion.org.uk \ --cc=jason@lakedaemon.net \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-kernel@vger.kernel.org \ --cc=mark.rutland@arm.com \ --cc=nadavh@marvell.com \ --cc=pawel.moll@arm.com \ --cc=robh+dt@kernel.org \ --cc=sebastian.hesselbarth@gmail.com \ --cc=tglx@linutronix.de \ --cc=thomas.petazzoni@free-electrons.com \ --cc=yehuday@marvell.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.