| Message ID | 20180517131727.29263-1-eric@anholt.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, 17 May 2018, Eric Anholt wrote: > diff --git a/arch/arm/boot/dts/bcm2837.dtsi b/arch/arm/boot/dts/bcm2837.dtsi > index 7704bb029605..1f5e5c782835 100644 > --- a/arch/arm/boot/dts/bcm2837.dtsi > +++ b/arch/arm/boot/dts/bcm2837.dtsi > @@ -17,6 +17,12 @@ > }; > }; > > + arm-pmu { > + compatible = "arm,cortex-a53-pmu", "arm,cortex-a7-pmu"; > + interrupt-parent = <&local_intc>; > + interrupts = <9 IRQ_TYPE_LEVEL_HIGH>; > + }; > + why this and not arm-pmu { compatible = "arm,armv8-pmuv3"; interrupt-parent = <&local_intc>; interrupts = <9>; }; which works, though when I didn't get very far when I submitted the patch to add this last August. Vince
Vince Weaver <vincent.weaver@maine.edu> writes: > On Thu, 17 May 2018, Eric Anholt wrote: > >> diff --git a/arch/arm/boot/dts/bcm2837.dtsi b/arch/arm/boot/dts/bcm2837.dtsi >> index 7704bb029605..1f5e5c782835 100644 >> --- a/arch/arm/boot/dts/bcm2837.dtsi >> +++ b/arch/arm/boot/dts/bcm2837.dtsi >> @@ -17,6 +17,12 @@ >> }; >> }; >> >> + arm-pmu { >> + compatible = "arm,cortex-a53-pmu", "arm,cortex-a7-pmu"; >> + interrupt-parent = <&local_intc>; >> + interrupts = <9 IRQ_TYPE_LEVEL_HIGH>; >> + }; >> + > > why this and not > > arm-pmu { > compatible = "arm,armv8-pmuv3"; > interrupt-parent = <&local_intc>; > interrupts = <9>; > }; > > which works, though when I didn't get very far when I submitted the patch > to add this last August. Is that better than a53? I'm happy to switch to that. The important part to me is the a7, since basically everyone with this hw is running arm32.
> Eric Anholt <eric@anholt.net> hat am 17. Mai 2018 um 15:17 geschrieben: > > > The a53 and a7 counters seem to match up, so we advertise a7 so that > arm32 can probe. > > Signed-off-by: Eric Anholt <eric@anholt.net> Acked-by: Stefan Wahren <stefan.wahren@i2se.com>
On Thu, 17 May 2018, Eric Anholt wrote: > > Is that better than a53? I'm happy to switch to that. The important > part to me is the a7, since basically everyone with this hw is running > arm32. no, on further investigation it looks like a53 is more proper to use than the generic armv8. Is the armv8 pmu on the cortex-a53 backwards compatible with armv7? I'm dreading having to pull up the various ARM ARMs to look for myself so if it works for you I'm fine with that part too. The biggest pushback I had with my original patch was no one believing irq 9 was the right one to use. Vince
> Vince Weaver <vincent.weaver@maine.edu> hat am 17. Mai 2018 um 16:11 geschrieben: > > > On Thu, 17 May 2018, Eric Anholt wrote: > > > diff --git a/arch/arm/boot/dts/bcm2837.dtsi b/arch/arm/boot/dts/bcm2837.dtsi > > index 7704bb029605..1f5e5c782835 100644 > > --- a/arch/arm/boot/dts/bcm2837.dtsi > > +++ b/arch/arm/boot/dts/bcm2837.dtsi > > @@ -17,6 +17,12 @@ > > }; > > }; > > > > + arm-pmu { > > + compatible = "arm,cortex-a53-pmu", "arm,cortex-a7-pmu"; > > + interrupt-parent = <&local_intc>; > > + interrupts = <9 IRQ_TYPE_LEVEL_HIGH>; > > + }; > > + > > why this and not > > arm-pmu { > compatible = "arm,armv8-pmuv3"; > interrupt-parent = <&local_intc>; > interrupts = <9>; > }; > > which works, though when I didn't get very far when I submitted the patch > to add this last August. Eric's variant is more specific so we better chose this. The init code diverse between the general arm,armv8-pmuv3 and arm,cortex-a53-pmu. > > Vince > > _______________________________________________ > linux-rpi-kernel mailing list > linux-rpi-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-rpi-kernel
On Thu, 17 May 2018, Stefan Wahren wrote: > > > Eric Anholt <eric@anholt.net> hat am 17. Mai 2018 um 15:17 geschrieben: > > > > > > The a53 and a7 counters seem to match up, so we advertise a7 so that > > arm32 can probe. so how closely did you look at the a53/a7 differences? I see some major differences, especially with the CPU_CYCLES event (0xff vs 0x11). The proper fix here might be to add a cortex-a53 PMU entry to the armv7 code rather than trying to treat it as a cortex-a7. Vince
On Thu, May 17, 2018 at 2:17 PM, Eric Anholt <eric@anholt.net> wrote: > The a53 and a7 counters seem to match up, so we advertise a7 so that > arm32 can probe. > > Signed-off-by: Eric Anholt <eric@anholt.net> Tested-by: Peter Robinson <pbrobinson@gmail.com> We've carried the same/equiv patch in Fedora for a while with no issues. Peter > --- > arch/arm/boot/dts/bcm2837.dtsi | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/arch/arm/boot/dts/bcm2837.dtsi b/arch/arm/boot/dts/bcm2837.dtsi > index 7704bb029605..1f5e5c782835 100644 > --- a/arch/arm/boot/dts/bcm2837.dtsi > +++ b/arch/arm/boot/dts/bcm2837.dtsi > @@ -17,6 +17,12 @@ > }; > }; > > + arm-pmu { > + compatible = "arm,cortex-a53-pmu", "arm,cortex-a7-pmu"; > + interrupt-parent = <&local_intc>; > + interrupts = <9 IRQ_TYPE_LEVEL_HIGH>; > + }; > + > timer { > compatible = "arm,armv7-timer"; > interrupt-parent = <&local_intc>; > -- > 2.17.0 > > > _______________________________________________ > linux-rpi-kernel mailing list > linux-rpi-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-rpi-kernel
Hi, [added Peter, Ingo and Arnaldo] > Vince Weaver <vincent.weaver@maine.edu> hat am 17. Mai 2018 um 18:34 geschrieben: > > > On Thu, 17 May 2018, Stefan Wahren wrote: > > > > > > Eric Anholt <eric@anholt.net> hat am 17. Mai 2018 um 15:17 geschrieben: > > > > > > > > > The a53 and a7 counters seem to match up, so we advertise a7 so that > > > arm32 can probe. > > so how closely did you look at the a53/a7 differences? I see some major > differences, especially with the CPU_CYCLES event (0xff vs 0x11). > > The proper fix here might be to add a cortex-a53 PMU entry to the armv7 > code rather than trying to treat it as a cortex-a7. we like to use the PMU of BCM2837 SoC (4x A53 cores) under arm32 and arm64. What is the right way (tm) to the define the DT compatibles? Does the arm32 PMU driver need patching for proper A53 support? Stefan > > Vince
Vince Weaver <vincent.weaver@maine.edu> writes: > On Thu, 17 May 2018, Stefan Wahren wrote: > >> >> > Eric Anholt <eric@anholt.net> hat am 17. Mai 2018 um 15:17 geschrieben: >> > >> > >> > The a53 and a7 counters seem to match up, so we advertise a7 so that >> > arm32 can probe. > > so how closely did you look at the a53/a7 differences? I see some major > differences, especially with the CPU_CYCLES event (0xff vs 0x11). I'm a bit lost in the code, but it seemed like the 0xff was a placeholder for a bit of special behavior, but that the cpu_cycles -> ARMV7_PERFCTR_CLOCK_CYCLES mapping got you that same value in the end.
On Thu, May 17, 2018 at 06:55:26PM +0200, Stefan Wahren wrote: > > Vince Weaver <vincent.weaver@maine.edu> hat am 17. Mai 2018 um 18:34 geschrieben: > > On Thu, 17 May 2018, Stefan Wahren wrote: > > > > Eric Anholt <eric@anholt.net> hat am 17. Mai 2018 um 15:17 geschrieben: > > > > The a53 and a7 counters seem to match up, so we advertise a7 so that > > > > arm32 can probe. > > > > so how closely did you look at the a53/a7 differences? I see some major > > differences, especially with the CPU_CYCLES event (0xff vs 0x11). > > > > The proper fix here might be to add a cortex-a53 PMU entry to the armv7 > > code rather than trying to treat it as a cortex-a7. > > we like to use the PMU of BCM2837 SoC (4x A53 cores) under arm32 and arm64. > > What is the right way (tm) to the define the DT compatibles? > Does the arm32 PMU driver need patching for proper A53 support? I'm completely clueless on all of this; Mark might have ideas.
On Thu, 17 May 2018, Peter Zijlstra wrote: > On Thu, May 17, 2018 at 06:55:26PM +0200, Stefan Wahren wrote: > > > Vince Weaver <vincent.weaver@maine.edu> hat am 17. Mai 2018 um 18:34 geschrieben: > > > On Thu, 17 May 2018, Stefan Wahren wrote: > > > > > Eric Anholt <eric@anholt.net> hat am 17. Mai 2018 um 15:17 geschrieben: > > > > > > The a53 and a7 counters seem to match up, so we advertise a7 so that > > > > > arm32 can probe. > > > > > > so how closely did you look at the a53/a7 differences? I see some major > > > differences, especially with the CPU_CYCLES event (0xff vs 0x11). > > > > > > The proper fix here might be to add a cortex-a53 PMU entry to the armv7 > > > code rather than trying to treat it as a cortex-a7. > > > > we like to use the PMU of BCM2837 SoC (4x A53 cores) under arm32 and arm64. > > > > What is the right way (tm) to the define the DT compatibles? > > Does the arm32 PMU driver need patching for proper A53 support? > > I'm completely clueless on all of this; Mark might have ideas. Spending more time looking at it the only obvious differences are the previously mentioned CYCLES difference, as well as the cortex-a7 has 18 events in the perf_cache_map but cortex-a53 only has 3. Plus probably support for the various other features of the armv8v3 pmu that the a7 knows nothing about. Is it hard to get lines in the DT changed once they are there? If we go with cortex-a7 now, would it be possible to later drop that if proper cortex-a53 support is added to the armv7 pmu driver? Or would that lead to all kinds of back-compatability mess? Vince
On Thu, 17 May 2018, Vince Weaver wrote: > On Thu, 17 May 2018, Peter Zijlstra wrote: > with cortex-a7 now, would it be possible to later drop that if proper > cortex-a53 support is added to the armv7 pmu driver? Or would that lead > to all kinds of back-compatability mess? For what it's worth, the pi-foundation kernel bcm2710 device tree file does: arm-pmu { #ifdef RPI364 compatible = "arm,armv8-pmuv3", "arm,cortex-a7-pmu"; #else compatible = "arm,cortex-a7-pmu"; #endif interrupt-parent = <&local_intc>; interrupts = <9>; }; Which is probably where I was getting the arm,armv8-pmuv3 from in my original patch. Vince
On 05/17/2018 12:31 PM, Vince Weaver wrote: > On Thu, 17 May 2018, Vince Weaver wrote: > >> On Thu, 17 May 2018, Peter Zijlstra wrote: >> with cortex-a7 now, would it be possible to later drop that if proper >> cortex-a53 support is added to the armv7 pmu driver? Or would that lead >> to all kinds of back-compatability mess? > > For what it's worth, the pi-foundation kernel bcm2710 device tree file > does: > > arm-pmu { > #ifdef RPI364 > compatible = "arm,armv8-pmuv3", "arm,cortex-a7-pmu"; > #else > compatible = "arm,cortex-a7-pmu"; > #endif > interrupt-parent = <&local_intc>; > interrupts = <9>; > }; > > > Which is probably where I was getting the arm,armv8-pmuv3 from in my > original patch. I thought somehow that Marc Z. had unified arch/arm/kernel/perf_event_v7.c and arch/arm64/kernel/perf_event.c into a common driver entry point under drivers/perf/arm_pmu.c but I don't see it and after about 15 minutes looking at it, it does not look as trivial as I though to separate out those files so the ARMv8 PMU description can be moved into a generic location for instance. FWIW, Broadcom STB chips, even when 64-bit capable or often used with an 32-bit ARM kernel, so having the ARMv8 PMUs work under a 32-bit ARM kernel would be great. The downstream solution we have sued thus far is to find the closest compatible string to represent those, which is not great...
On 17/05/18 20:59, Florian Fainelli wrote: > On 05/17/2018 12:31 PM, Vince Weaver wrote: >> On Thu, 17 May 2018, Vince Weaver wrote: >> >>> On Thu, 17 May 2018, Peter Zijlstra wrote: >>> with cortex-a7 now, would it be possible to later drop that if proper >>> cortex-a53 support is added to the armv7 pmu driver? Or would that lead >>> to all kinds of back-compatability mess? >> >> For what it's worth, the pi-foundation kernel bcm2710 device tree file >> does: >> >> arm-pmu { >> #ifdef RPI364 >> compatible = "arm,armv8-pmuv3", "arm,cortex-a7-pmu"; Hahaha. Funny that. Not. That's really silly. The DT *must* describe the HW, and having contradictory information is not helping. This is going to lead to all kind of miscounted events (to take the above example) A7 and A53 are significantly different, and thus will count events differently.... >> #else >> compatible = "arm,cortex-a7-pmu"; >> #endif >> interrupt-parent = <&local_intc>; >> interrupts = <9>; >> }; >> >> >> Which is probably where I was getting the arm,armv8-pmuv3 from in my >> original patch. > > I thought somehow that Marc Z. had unified > arch/arm/kernel/perf_event_v7.c and arch/arm64/kernel/perf_event.c into > a common driver entry point under drivers/perf/arm_pmu.c but I don't see > it and after about 15 minutes looking at it, it does not look as trivial > as I though to separate out those files so the ARMv8 PMU description can > be moved into a generic location for instance. I have a pretty simple series[1] which I used to profile 32bit guests on an arm64 KVM host. Nobody really cared about it because running a 32bit kernel on 64bit HW is a bit odd, to say the least, and I'm probably the only one actually running 32bit VMs. > FWIW, Broadcom STB chips, even when 64-bit capable or often used with an > 32-bit ARM kernel, so having the ARMv8 PMUs work under a 32-bit ARM > kernel would be great. The downstream solution we have sued thus far is > to find the closest compatible string to represent those, which is not > great... Ah, so you're *really* doing that? I'm not going to ask why, I'm scared of the answer... ;-) Anyway, I can repost that series if that will prevent people from having that kind of silly hacks. Thanks, M. [1] https://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms.git/log/?h=kvm-arm/pmuv3-32bit
Hi Marc, > Marc Zyngier <marc.zyngier@arm.com> hat am 18. Mai 2018 um 10:07 geschrieben: > > I have a pretty simple series[1] which I used to profile 32bit guests on > an arm64 KVM host. Nobody really cared about it because running a 32bit > kernel on 64bit HW is a bit odd, to say the least, and I'm probably the > only one actually running 32bit VMs. > > > FWIW, Broadcom STB chips, even when 64-bit capable or often used with an > > 32-bit ARM kernel, so having the ARMv8 PMUs work under a 32-bit ARM > > kernel would be great. The downstream solution we have sued thus far is > > to find the closest compatible string to represent those, which is not > > great... > Ah, so you're *really* doing that? I'm not going to ask why, I'm scared > of the answer... ;-) > > Anyway, I can repost that series if that will prevent people from having > that kind of silly hacks. yes please. But there is a minor nit: some patches introduce new files without SPDX tags. Thanks Stefan > > Thanks, > > M. > > [1] > https://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms.git/log/?h=kvm-arm/pmuv3-32bit > -- > Jazz is not dead. It just smells funny...
On 18/05/18 10:37, Stefan Wahren wrote: > Hi Marc, > >> Marc Zyngier <marc.zyngier@arm.com> hat am 18. Mai 2018 um 10:07 geschrieben: >> >> I have a pretty simple series[1] which I used to profile 32bit guests on >> an arm64 KVM host. Nobody really cared about it because running a 32bit >> kernel on 64bit HW is a bit odd, to say the least, and I'm probably the >> only one actually running 32bit VMs. >> >>> FWIW, Broadcom STB chips, even when 64-bit capable or often used with an >>> 32-bit ARM kernel, so having the ARMv8 PMUs work under a 32-bit ARM >>> kernel would be great. The downstream solution we have sued thus far is >>> to find the closest compatible string to represent those, which is not >>> great... >> Ah, so you're *really* doing that? I'm not going to ask why, I'm scared >> of the answer... ;-) >> >> Anyway, I can repost that series if that will prevent people from having >> that kind of silly hacks. > > yes please. But there is a minor nit: some patches introduce new files without SPDX tags. I'm shocked! ;-) M.
diff --git a/arch/arm/boot/dts/bcm2837.dtsi b/arch/arm/boot/dts/bcm2837.dtsi index 7704bb029605..1f5e5c782835 100644 --- a/arch/arm/boot/dts/bcm2837.dtsi +++ b/arch/arm/boot/dts/bcm2837.dtsi @@ -17,6 +17,12 @@ }; }; + arm-pmu { + compatible = "arm,cortex-a53-pmu", "arm,cortex-a7-pmu"; + interrupt-parent = <&local_intc>; + interrupts = <9 IRQ_TYPE_LEVEL_HIGH>; + }; + timer { compatible = "arm,armv7-timer"; interrupt-parent = <&local_intc>;
The a53 and a7 counters seem to match up, so we advertise a7 so that arm32 can probe. Signed-off-by: Eric Anholt <eric@anholt.net> --- arch/arm/boot/dts/bcm2837.dtsi | 6 ++++++ 1 file changed, 6 insertions(+)