| Message ID | 1413985427-20918-6-git-send-email-ezequiel.garcia@free-electrons.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Oct 22, 2014 at 02:43:45PM +0100, Ezequiel Garcia wrote: > The Armada 375 SoC has a Cortex-A9 CPU, and so the PMU is available > to be used. This commit enables it in the devicetree. > > Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com> > --- > arch/arm/boot/dts/armada-375.dtsi | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/arch/arm/boot/dts/armada-375.dtsi b/arch/arm/boot/dts/armada-375.dtsi > index de65714..f131cd2 100644 > --- a/arch/arm/boot/dts/armada-375.dtsi > +++ b/arch/arm/boot/dts/armada-375.dtsi > @@ -55,6 +55,11 @@ > }; > }; > > + pmu { > + compatible = "arm,cortex-a9-pmu"; > + interrupts-extended = <&mpic 3>; > + }; Just to check - the interrupts from both CPUs are muxed into a single line into the interrupt controller? This isn't gonig to work at the moment -- the perf code will associate this interrupt with CPU0 and you'll lose events on CPU1. Hopefully there's a separate interrupt for CPU1? Mark. > + > soc { > compatible = "marvell,armada375-mbus", "marvell,armada370-mbus", "simple-bus"; > #address-cells = <2>; > -- > 2.1.0 > > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel >
Hi Mark, Thanks for the reply. You made me research and test this in depth :) On 10/22/2014 11:04 AM, Mark Rutland wrote: > On Wed, Oct 22, 2014 at 02:43:45PM +0100, Ezequiel Garcia wrote: >> The Armada 375 SoC has a Cortex-A9 CPU, and so the PMU is available >> to be used. This commit enables it in the devicetree. >> >> Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com> >> --- >> arch/arm/boot/dts/armada-375.dtsi | 5 +++++ >> 1 file changed, 5 insertions(+) >> >> diff --git a/arch/arm/boot/dts/armada-375.dtsi b/arch/arm/boot/dts/armada-375.dtsi >> index de65714..f131cd2 100644 >> --- a/arch/arm/boot/dts/armada-375.dtsi >> +++ b/arch/arm/boot/dts/armada-375.dtsi >> @@ -55,6 +55,11 @@ >> }; >> }; >> >> + pmu { >> + compatible = "arm,cortex-a9-pmu"; >> + interrupts-extended = <&mpic 3>; >> + }; > > Just to check - the interrupts from both CPUs are muxed into a single > line into the interrupt controller? > > This isn't gonig to work at the moment -- the perf code will associate > this interrupt with CPU0 and you'll lose events on CPU1. > > Hopefully there's a separate interrupt for CPU1? > The <mpic 3> is a per CPU interrupt. Actually, the interrupt contains more than just PMU events, it contains a summary of several CPU events: Perf counters for each CPU, Power management interrupts for each CPU, L2 cache interrupt, among others. The interrupt cause can be read from a banked register called "CPU subsystem local cause". Conversely, each of these must be enabled from another (banked) register. As far as I understand, this works (or should work) because the irqchip driver enables *just* the perf counter interrupt for the running CPU in the armada_xp_mpic_secondary_init(). Then, the ARM perf code requests the interrupt as per cpu and things just work, right? However: 1) In the Armada 375 SoC case, the MPIC is chained and the code does not register the cpu notifier, so armada_xp_mpic_secondary_init is not called on CPU1. 2) Even when ensuring armada_xp_mpic_secondary_init is called on each CPU, and thus each perf counter interrupt is enabled, I can't see the PMU interrupt for CPU1, but just the one for the boot CPU. I'll check with the hardware designer about this.
Dear Ezequiel Garcia, On Wed, 22 Oct 2014 19:16:42 -0300, Ezequiel Garcia wrote: > The <mpic 3> is a per CPU interrupt. > > Actually, the interrupt contains more than just PMU events, it contains > a summary of several CPU events: Perf counters for each CPU, Power > management interrupts for each CPU, L2 cache interrupt, among others. This is kind of a side discussion but if this <mpic 3> interrupts does much more than PMU events, then we should implement a separate irqchip driver for this, to "demultiplex" the events notified by this interrupt. Yes, today we are only using the PMU events from <mpic 3>, but what if tomorrow we need to be notified of other events in other drivers? We would be screwed, because we would have to change the DT representation: the PMU would no longer take <&mpic 3> as the interrupt, but <&some_other_irq_controller XYZ> as the interrupt. Best regards, Thomas
On Wed, Oct 22, 2014 at 11:16:42PM +0100, Ezequiel Garcia wrote: > Hi Mark, > > Thanks for the reply. You made me research and test this in depth :) > > On 10/22/2014 11:04 AM, Mark Rutland wrote: > > On Wed, Oct 22, 2014 at 02:43:45PM +0100, Ezequiel Garcia wrote: > >> The Armada 375 SoC has a Cortex-A9 CPU, and so the PMU is available > >> to be used. This commit enables it in the devicetree. > >> > >> Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com> > >> --- > >> arch/arm/boot/dts/armada-375.dtsi | 5 +++++ > >> 1 file changed, 5 insertions(+) > >> > >> diff --git a/arch/arm/boot/dts/armada-375.dtsi b/arch/arm/boot/dts/armada-375.dtsi > >> index de65714..f131cd2 100644 > >> --- a/arch/arm/boot/dts/armada-375.dtsi > >> +++ b/arch/arm/boot/dts/armada-375.dtsi > >> @@ -55,6 +55,11 @@ > >> }; > >> }; > >> > >> + pmu { > >> + compatible = "arm,cortex-a9-pmu"; > >> + interrupts-extended = <&mpic 3>; > >> + }; > > > > Just to check - the interrupts from both CPUs are muxed into a single > > line into the interrupt controller? > > > > This isn't gonig to work at the moment -- the perf code will associate > > this interrupt with CPU0 and you'll lose events on CPU1. > > > > Hopefully there's a separate interrupt for CPU1? > > > > The <mpic 3> is a per CPU interrupt. Ah, ok. I hadn't realised that was the case. That should be fine, then. > Actually, the interrupt contains more than just PMU events, it contains > a summary of several CPU events: Perf counters for each CPU, Power > management interrupts for each CPU, L2 cache interrupt, among others. > > The interrupt cause can be read from a banked register called "CPU > subsystem local cause". Conversely, each of these must be enabled from > another (banked) register. I think that as Thomas said in his reply that it would make sense to expose these as separate interrupts out of the mpic, given we have the capability to distinguish them at the mpic. That will prevent a lot of pain (e.g. percpu interrupts can't currently be shared), and would get rid of the need to hack armada_xp_mpic_secondary_init to enable a particular interrupt. > As far as I understand, this works (or should work) because the irqchip > driver enables *just* the perf counter interrupt for the running CPU in > the armada_xp_mpic_secondary_init(). Then, the ARM perf code requests > the interrupt as per cpu and things just work, right? That would probably work currently, but I haven't thought about it in great detail. It would be nicer if we enabled the perf counter interrupt only in response to an enable_percpu_irq though. > However: > > 1) In the Armada 375 SoC case, the MPIC is chained and the code does not > register the cpu notifier, so armada_xp_mpic_secondary_init is not > called on CPU1. > > 2) Even when ensuring armada_xp_mpic_secondary_init is called on each > CPU, and thus each perf counter interrupt is enabled, I can't see the > PMU interrupt for CPU1, but just the one for the boot CPU. > > I'll check with the hardware designer about this. Ok. It would be nice to get to the bottom of that. It doesn't seem like a great idea to add the PMU node with a potential dodgy interrupt -- you could get misleading output from perf in the case of overflows. Thanks, Mark.
On 10/23/2014 06:14 AM, Thomas Petazzoni wrote: > Dear Ezequiel Garcia, > > On Wed, 22 Oct 2014 19:16:42 -0300, Ezequiel Garcia wrote: > >> The <mpic 3> is a per CPU interrupt. >> >> Actually, the interrupt contains more than just PMU events, it contains >> a summary of several CPU events: Perf counters for each CPU, Power >> management interrupts for each CPU, L2 cache interrupt, among others. > > This is kind of a side discussion but if this <mpic 3> interrupts does > much more than PMU events, then we should implement a separate irqchip > driver for this, to "demultiplex" the events notified by this interrupt. > Oh, I didn't realize this was possible. > Yes, today we are only using the PMU events from <mpic 3>, but what if > tomorrow we need to be notified of other events in other drivers? We > would be screwed, because we would have to change the DT > representation: the PMU would no longer take <&mpic 3> as the > interrupt, but <&some_other_irq_controller XYZ> as the interrupt. > Yes, that sounds like a much better approach. I'll take a look and try to prepare something.
Dear Ezequiel Garcia, On Thu, 23 Oct 2014 08:51:27 -0300, Ezequiel Garcia wrote: > > On Wed, 22 Oct 2014 19:16:42 -0300, Ezequiel Garcia wrote: > > > >> The <mpic 3> is a per CPU interrupt. > >> > >> Actually, the interrupt contains more than just PMU events, it contains > >> a summary of several CPU events: Perf counters for each CPU, Power > >> management interrupts for each CPU, L2 cache interrupt, among others. > > > > This is kind of a side discussion but if this <mpic 3> interrupts does > > much more than PMU events, then we should implement a separate irqchip > > driver for this, to "demultiplex" the events notified by this interrupt. > > Oh, I didn't realize this was possible. The only trick is that it can't be a separate DT node, because the registers that contains the mask/cause informations for the events notified by <mpic 3> belongs to the MPIC registers area. Not sure how to handle that, maybe Mark will have some suggestions. Thomas
On 10/23/2014 09:07 AM, Thomas Petazzoni wrote: > Dear Ezequiel Garcia, > > On Thu, 23 Oct 2014 08:51:27 -0300, Ezequiel Garcia wrote: > >>> On Wed, 22 Oct 2014 19:16:42 -0300, Ezequiel Garcia wrote: >>> >>>> The <mpic 3> is a per CPU interrupt. >>>> >>>> Actually, the interrupt contains more than just PMU events, it contains >>>> a summary of several CPU events: Perf counters for each CPU, Power >>>> management interrupts for each CPU, L2 cache interrupt, among others. >>> >>> This is kind of a side discussion but if this <mpic 3> interrupts does >>> much more than PMU events, then we should implement a separate irqchip >>> driver for this, to "demultiplex" the events notified by this interrupt. >> >> Oh, I didn't realize this was possible. > > The only trick is that it can't be a separate DT node, because the > registers that contains the mask/cause informations for the events > notified by <mpic 3> belongs to the MPIC registers area. > Technically speaking, we can stop requesting the region in the mpic irqchip driver and share the registers, just as we do in other drivers. Not the most elegant thing to do, though.
On Thu, Oct 23, 2014 at 01:07:31PM +0100, Thomas Petazzoni wrote: > Dear Ezequiel Garcia, > > On Thu, 23 Oct 2014 08:51:27 -0300, Ezequiel Garcia wrote: > > > > On Wed, 22 Oct 2014 19:16:42 -0300, Ezequiel Garcia wrote: > > > > > >> The <mpic 3> is a per CPU interrupt. > > >> > > >> Actually, the interrupt contains more than just PMU events, it contains > > >> a summary of several CPU events: Perf counters for each CPU, Power > > >> management interrupts for each CPU, L2 cache interrupt, among others. > > > > > > This is kind of a side discussion but if this <mpic 3> interrupts does > > > much more than PMU events, then we should implement a separate irqchip > > > driver for this, to "demultiplex" the events notified by this interrupt. > > > > Oh, I didn't realize this was possible. > > The only trick is that it can't be a separate DT node, because the > registers that contains the mask/cause informations for the events > notified by <mpic 3> belongs to the MPIC registers area. > > Not sure how to handle that, maybe Mark will have some suggestions. I'm not sure I follow why you would need a separate irqchip driver. Why can't this live in the existing mpic driver? Thanks, Mark.
On 10/23/2014 10:18 AM, Mark Rutland wrote: > On Thu, Oct 23, 2014 at 01:07:31PM +0100, Thomas Petazzoni wrote: >> Dear Ezequiel Garcia, >> >> On Thu, 23 Oct 2014 08:51:27 -0300, Ezequiel Garcia wrote: >> >>>> On Wed, 22 Oct 2014 19:16:42 -0300, Ezequiel Garcia wrote: >>>> >>>>> The <mpic 3> is a per CPU interrupt. >>>>> >>>>> Actually, the interrupt contains more than just PMU events, it contains >>>>> a summary of several CPU events: Perf counters for each CPU, Power >>>>> management interrupts for each CPU, L2 cache interrupt, among others. >>>> >>>> This is kind of a side discussion but if this <mpic 3> interrupts does >>>> much more than PMU events, then we should implement a separate irqchip >>>> driver for this, to "demultiplex" the events notified by this interrupt. >>> >>> Oh, I didn't realize this was possible. >> >> The only trick is that it can't be a separate DT node, because the >> registers that contains the mask/cause informations for the events >> notified by <mpic 3> belongs to the MPIC registers area. >> >> Not sure how to handle that, maybe Mark will have some suggestions. > > I'm not sure I follow why you would need a separate irqchip driver. Why > can't this live in the existing mpic driver? > I've tried to add a demux interrupt controller for the CPU summary interrupts to be able to hook to the proper interrupt. This was doable without much pain [1]. However, due to the way the Performance counter overflow IRQ is exposed, it doesn't seem to meet perf's PMU irq handling requirement. For per CPU interrupts, perf requests the interrupt and expects to get an interrupt on each CPU, with the counter overflow event for that CPU. This is not the case for the CPU Summary interrupt. This *is* a per CPU interrupt, but there is a separate interrupt line for each CPU: CPU summary per CPU interrupt 0, for the Perf counter on CPU0 CPU summary per CPU interrupt 1, for the Perf counter on CPU1 So, I thought about exposing the interrupt as a shared one and use interrupts 0 and 1, triggering interrupts on CPU0 with the counter overflow for CPU0 and CPU1. This doesn't work either, as the perf code expects to set the interrupt affinity to route each shared interrupt to the appropriate CPU. I'm not sure how can I set the interrupt affinity for the demux controller, being a chained interrupt controller. At this point, due to the SoC weirdness in exposing the PMU IRQ, I'm starting to think we will have to live with software events for this SoC, but I'd love to be proved wrong. [1] http://sprunge.us/MfVN
diff --git a/arch/arm/boot/dts/armada-375.dtsi b/arch/arm/boot/dts/armada-375.dtsi index de65714..f131cd2 100644 --- a/arch/arm/boot/dts/armada-375.dtsi +++ b/arch/arm/boot/dts/armada-375.dtsi @@ -55,6 +55,11 @@ }; }; + pmu { + compatible = "arm,cortex-a9-pmu"; + interrupts-extended = <&mpic 3>; + }; + soc { compatible = "marvell,armada375-mbus", "marvell,armada370-mbus", "simple-bus"; #address-cells = <2>;
The Armada 375 SoC has a Cortex-A9 CPU, and so the PMU is available to be used. This commit enables it in the devicetree. Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com> --- arch/arm/boot/dts/armada-375.dtsi | 5 +++++ 1 file changed, 5 insertions(+)