[PATCH] arm: bcm2835: Add the PMU to the devicetree.

[PATCH] arm: bcm2835: Add the PMU to the devicetree.

[Eric Anholt]

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(+)

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

[PATCH] arm: bcm2835: Add the PMU to the devicetree.

[Vince Weaver]

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

[PATCH] arm: bcm2835: Add the PMU to the devicetree.

[Eric Anholt]

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.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 832 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20180517/78df2543/attachment.sig>

[PATCH] arm: bcm2835: Add the PMU to the devicetree.

[Vince Weaver]

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

[PATCH] arm: bcm2835: Add the PMU to the devicetree.

[Stefan Wahren]

> 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 at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-rpi-kernel

[PATCH] arm: bcm2835: Add the PMU to the devicetree.

[Stefan Wahren]

> 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>

[PATCH] arm: bcm2835: Add the PMU to the devicetree.

[Vince Weaver]

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

[PATCH] arm: bcm2835: Add the PMU to the devicetree.

[Stefan Wahren]

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

[PATCH] arm: bcm2835: Add the PMU to the devicetree.

[Peter Zijlstra]

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.

[PATCH] arm: bcm2835: Add the PMU to the devicetree.

[Vince Weaver]

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

[PATCH] arm: bcm2835: Add the PMU to the devicetree.

[Vince Weaver]

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

[PATCH] arm: bcm2835: Add the PMU to the devicetree.

[Florian Fainelli]

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...
-- 
Florian

[PATCH] arm: bcm2835: Add the PMU to the devicetree.

[Marc Zyngier]

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
-- 
Jazz is not dead. It just smells funny...

[PATCH] arm: bcm2835: Add the PMU to the devicetree.

[Stefan Wahren]

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...

[PATCH] arm: bcm2835: Add the PMU to the devicetree.

[Marc Zyngier]

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.
-- 
Jazz is not dead. It just smells funny...

[PATCH] arm: bcm2835: Add the PMU to the devicetree.

[Eric Anholt]

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.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 832 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20180517/24fefc2e/attachment-0001.sig>

[PATCH] arm: bcm2835: Add the PMU to the devicetree.

[Peter Robinson]

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 at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-rpi-kernel