| Message ID | 20220206144343.2194-1-crawford.benjamin15@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] add pmu to amlogic meson sm1 | expand |
Hi, On 06/02/2022 15:43, Benjamin Mordaunt wrote: > --- > The dts for meson sm1 appears to omit the SoC's PMU, > which is essential for accessing perf events regarding > e.g. cache on e.g. the Odroid C4 platform. Add it. > > arch/arm64/boot/dts/amlogic/meson-sm1.dtsi | 13 ++++++++++++- > 1 file changed, 12 insertions(+), 1 deletion(-) > > diff --git a/arch/arm64/boot/dts/amlogic/meson-sm1.dtsi b/arch/arm64/boot/dts/amlogic/meson-sm1.dtsi > index 3d8b1f4f2..4147eecd2 100644 > --- a/arch/arm64/boot/dts/amlogic/meson-sm1.dtsi > +++ b/arch/arm64/boot/dts/amlogic/meson-sm1.dtsi > @@ -6,6 +6,8 @@ > > #include "meson-g12-common.dtsi" > #include <dt-bindings/clock/axg-audio-clkc.h> > +#include <dt-bindings/interrupt-controller/irq.h> > +#include <dt-bindings/interrupt-controller/arm-gic.h> > #include <dt-bindings/power/meson-sm1-power.h> > #include <dt-bindings/reset/amlogic,meson-axg-audio-arb.h> > #include <dt-bindings/reset/amlogic,meson-g12a-audio-reset.h> > @@ -90,7 +92,16 @@ l2: l2-cache0 { > compatible = "cache"; > }; > }; > - > + > + arm-pmu { > + compatible = "arm,cortex-a55-pmu"; > + interrupts = <GIC_SPI 137 IRQ_TYPE_LEVEL_HIGH>, > + <GIC_SPI 138 IRQ_TYPE_LEVEL_HIGH>, > + <GIC_SPI 153 IRQ_TYPE_LEVEL_HIGH>, > + <GIC_SPI 154 IRQ_TYPE_LEVEL_HIGH>; > + interrupt-affinity = <&cpu0>, <&cpu1>, <&cpu2>, <&cpu3>; > + }; > + > cpu_opp_table: opp-table { > compatible = "operating-points-v2"; > opp-shared; Please see Marc's comments about PMU support: http://lore.kernel.org/r/8735pcq63o.wl-maz@kernel.org Neil
On 2022-02-07 08:14, Neil Armstrong wrote: > Hi, > > On 06/02/2022 15:43, Benjamin Mordaunt wrote: >> --- >> The dts for meson sm1 appears to omit the SoC's PMU, >> which is essential for accessing perf events regarding >> e.g. cache on e.g. the Odroid C4 platform. Add it. >> >> arch/arm64/boot/dts/amlogic/meson-sm1.dtsi | 13 ++++++++++++- >> 1 file changed, 12 insertions(+), 1 deletion(-) >> >> diff --git a/arch/arm64/boot/dts/amlogic/meson-sm1.dtsi >> b/arch/arm64/boot/dts/amlogic/meson-sm1.dtsi >> index 3d8b1f4f2..4147eecd2 100644 >> --- a/arch/arm64/boot/dts/amlogic/meson-sm1.dtsi >> +++ b/arch/arm64/boot/dts/amlogic/meson-sm1.dtsi >> @@ -6,6 +6,8 @@ >> #include "meson-g12-common.dtsi" >> #include <dt-bindings/clock/axg-audio-clkc.h> >> +#include <dt-bindings/interrupt-controller/irq.h> >> +#include <dt-bindings/interrupt-controller/arm-gic.h> >> #include <dt-bindings/power/meson-sm1-power.h> >> #include <dt-bindings/reset/amlogic,meson-axg-audio-arb.h> >> #include <dt-bindings/reset/amlogic,meson-g12a-audio-reset.h> >> @@ -90,7 +92,16 @@ l2: l2-cache0 { >> compatible = "cache"; >> }; >> }; >> - >> + >> + arm-pmu { >> + compatible = "arm,cortex-a55-pmu"; >> + interrupts = <GIC_SPI 137 IRQ_TYPE_LEVEL_HIGH>, >> + <GIC_SPI 138 IRQ_TYPE_LEVEL_HIGH>, >> + <GIC_SPI 153 IRQ_TYPE_LEVEL_HIGH>, >> + <GIC_SPI 154 IRQ_TYPE_LEVEL_HIGH>; >> + interrupt-affinity = <&cpu0>, <&cpu1>, <&cpu2>, <&cpu3>; >> + }; >> + >> cpu_opp_table: opp-table { >> compatible = "operating-points-v2"; >> opp-shared; > > Please see Marc's comments about PMU support: > http://lore.kernel.org/r/8735pcq63o.wl-maz@kernel.org If SM1 actually has distinct per-core interrupts as the patch implies then it's fine - it's only G12B and anything else that combines multiple PMU IRQs into a single SPI which are unsupportable. Robin.
On 2022-02-07 13:33, Benjamin Mordaunt wrote: > The S905X3’s (Odroid C4) datasheet appears to report 4 individual PMUIRQs, > much in the same way as GX. If that is indeed the case, I think this patch > applies. Otherwise, is the datasheet wrong if Marc’s comments apply? A fairly solid test would be to run a sampling event (e.g. `perf stat`) taskset to a single CPU and observe the corresponding IRQ count increase in /proc/interrupts, for each core in turn. If that behaves as expected then chances are everything is indeed sane. Couple of nitpicks for the patch itself - you're almost there, but you've got spurious tabs on the blank lines, plus you need a proper commit message and your sign-off above the "---" line - anything you add below there is treated as additional commentary for reviewers' benefit and will be discarded by `git am`. Cheers, Robin. > > / Ben > > On Mon, 7 Feb 2022 at 13:20, Robin Murphy <robin.murphy@arm.com> wrote: > >> On 2022-02-07 08:14, Neil Armstrong wrote: >>> Hi, >>> >>> On 06/02/2022 15:43, Benjamin Mordaunt wrote: >>>> --- >>>> The dts for meson sm1 appears to omit the SoC's PMU, >>>> which is essential for accessing perf events regarding >>>> e.g. cache on e.g. the Odroid C4 platform. Add it. >>>> >>>> arch/arm64/boot/dts/amlogic/meson-sm1.dtsi | 13 ++++++++++++- >>>> 1 file changed, 12 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/arch/arm64/boot/dts/amlogic/meson-sm1.dtsi >>>> b/arch/arm64/boot/dts/amlogic/meson-sm1.dtsi >>>> index 3d8b1f4f2..4147eecd2 100644 >>>> --- a/arch/arm64/boot/dts/amlogic/meson-sm1.dtsi >>>> +++ b/arch/arm64/boot/dts/amlogic/meson-sm1.dtsi >>>> @@ -6,6 +6,8 @@ >>>> #include "meson-g12-common.dtsi" >>>> #include <dt-bindings/clock/axg-audio-clkc.h> >>>> +#include <dt-bindings/interrupt-controller/irq.h> >>>> +#include <dt-bindings/interrupt-controller/arm-gic.h> >>>> #include <dt-bindings/power/meson-sm1-power.h> >>>> #include <dt-bindings/reset/amlogic,meson-axg-audio-arb.h> >>>> #include <dt-bindings/reset/amlogic,meson-g12a-audio-reset.h> >>>> @@ -90,7 +92,16 @@ l2: l2-cache0 { >>>> compatible = "cache"; >>>> }; >>>> }; >>>> - >>>> + >>>> + arm-pmu { >>>> + compatible = "arm,cortex-a55-pmu"; >>>> + interrupts = <GIC_SPI 137 IRQ_TYPE_LEVEL_HIGH>, >>>> + <GIC_SPI 138 IRQ_TYPE_LEVEL_HIGH>, >>>> + <GIC_SPI 153 IRQ_TYPE_LEVEL_HIGH>, >>>> + <GIC_SPI 154 IRQ_TYPE_LEVEL_HIGH>; >>>> + interrupt-affinity = <&cpu0>, <&cpu1>, <&cpu2>, <&cpu3>; >>>> + }; >>>> + >>>> cpu_opp_table: opp-table { >>>> compatible = "operating-points-v2"; >>>> opp-shared; >>> >>> Please see Marc's comments about PMU support: >>> http://lore.kernel.org/r/8735pcq63o.wl-maz@kernel.org >> >> If SM1 actually has distinct per-core interrupts as the patch implies >> then it's fine - it's only G12B and anything else that combines multiple >> PMU IRQs into a single SPI which are unsupportable. >> >> Robin. >> >
Okay so this does appear to be behaving with some issues (unless I've just got 2 of the interrupt numbers wrong, but it seems unlikely at this stage). CPUs 0 and 1 seem to be behaving somewhat sensibly, while `perf stat` reports `<not supported>` for caches and branches with CPUs 2 and 3. Would this suggest this implementation is indeed broken? Thanks, Ben On Mon, Feb 7, 2022 at 1:47 PM Robin Murphy <robin.murphy@arm.com> wrote: > > On 2022-02-07 13:33, Benjamin Mordaunt wrote: > > The S905X3’s (Odroid C4) datasheet appears to report 4 individual PMUIRQs, > > much in the same way as GX. If that is indeed the case, I think this patch > > applies. Otherwise, is the datasheet wrong if Marc’s comments apply? > > A fairly solid test would be to run a sampling event (e.g. `perf stat`) > taskset to a single CPU and observe the corresponding IRQ count increase > in /proc/interrupts, for each core in turn. If that behaves as expected > then chances are everything is indeed sane. > > Couple of nitpicks for the patch itself - you're almost there, but > you've got spurious tabs on the blank lines, plus you need a proper > commit message and your sign-off above the "---" line - anything you add > below there is treated as additional commentary for reviewers' benefit > and will be discarded by `git am`. > > Cheers, > Robin. > > > > > / Ben > > > > On Mon, 7 Feb 2022 at 13:20, Robin Murphy <robin.murphy@arm.com> wrote: > > > >> On 2022-02-07 08:14, Neil Armstrong wrote: > >>> Hi, > >>> > >>> On 06/02/2022 15:43, Benjamin Mordaunt wrote: > >>>> --- > >>>> The dts for meson sm1 appears to omit the SoC's PMU, > >>>> which is essential for accessing perf events regarding > >>>> e.g. cache on e.g. the Odroid C4 platform. Add it. > >>>> > >>>> arch/arm64/boot/dts/amlogic/meson-sm1.dtsi | 13 ++++++++++++- > >>>> 1 file changed, 12 insertions(+), 1 deletion(-) > >>>> > >>>> diff --git a/arch/arm64/boot/dts/amlogic/meson-sm1.dtsi > >>>> b/arch/arm64/boot/dts/amlogic/meson-sm1.dtsi > >>>> index 3d8b1f4f2..4147eecd2 100644 > >>>> --- a/arch/arm64/boot/dts/amlogic/meson-sm1.dtsi > >>>> +++ b/arch/arm64/boot/dts/amlogic/meson-sm1.dtsi > >>>> @@ -6,6 +6,8 @@ > >>>> #include "meson-g12-common.dtsi" > >>>> #include <dt-bindings/clock/axg-audio-clkc.h> > >>>> +#include <dt-bindings/interrupt-controller/irq.h> > >>>> +#include <dt-bindings/interrupt-controller/arm-gic.h> > >>>> #include <dt-bindings/power/meson-sm1-power.h> > >>>> #include <dt-bindings/reset/amlogic,meson-axg-audio-arb.h> > >>>> #include <dt-bindings/reset/amlogic,meson-g12a-audio-reset.h> > >>>> @@ -90,7 +92,16 @@ l2: l2-cache0 { > >>>> compatible = "cache"; > >>>> }; > >>>> }; > >>>> - > >>>> + > >>>> + arm-pmu { > >>>> + compatible = "arm,cortex-a55-pmu"; > >>>> + interrupts = <GIC_SPI 137 IRQ_TYPE_LEVEL_HIGH>, > >>>> + <GIC_SPI 138 IRQ_TYPE_LEVEL_HIGH>, > >>>> + <GIC_SPI 153 IRQ_TYPE_LEVEL_HIGH>, > >>>> + <GIC_SPI 154 IRQ_TYPE_LEVEL_HIGH>; > >>>> + interrupt-affinity = <&cpu0>, <&cpu1>, <&cpu2>, <&cpu3>; > >>>> + }; > >>>> + > >>>> cpu_opp_table: opp-table { > >>>> compatible = "operating-points-v2"; > >>>> opp-shared; > >>> > >>> Please see Marc's comments about PMU support: > >>> http://lore.kernel.org/r/8735pcq63o.wl-maz@kernel.org > >> > >> If SM1 actually has distinct per-core interrupts as the patch implies > >> then it's fine - it's only G12B and anything else that combines multiple > >> PMU IRQs into a single SPI which are unsupportable. > >> > >> Robin. > >> > >
On 2022-02-08 16:02, Benjamin Mordaunt wrote: > Okay so this does appear to be behaving with some issues (unless I've > just got 2 of the interrupt numbers wrong, but it seems unlikely at > this stage). > > CPUs 0 and 1 seem to be behaving somewhat sensibly, while `perf stat` > reports `<not supported>` for caches and branches with CPUs 2 and 3. > Would this suggest this implementation is indeed broken? Taking a closer look, the interrupts for CPUs 2 and 3 here are clearly wrong since they already belong to some audio controllers in the DTSI. I looked up documentation and found an S905D3 manual from Khadas which documents SPI 137 (GIC interrupt 169) as "|PMUIRQ_a[3:0]", which I can't read as anything other than "logical OR of all 4 PMU IRQs". Not sure about SPI 138 but I'd guess it's something that just happens to fire in a manner that appears to sort of work for the PMU. Sadly it looks like Neil's instinct was right and this one is also in fact dead, sorry :( Robin. > > Thanks, > Ben > > > On Mon, Feb 7, 2022 at 1:47 PM Robin Murphy <robin.murphy@arm.com> wrote: >> >> On 2022-02-07 13:33, Benjamin Mordaunt wrote: >>> The S905X3’s (Odroid C4) datasheet appears to report 4 individual PMUIRQs, >>> much in the same way as GX. If that is indeed the case, I think this patch >>> applies. Otherwise, is the datasheet wrong if Marc’s comments apply? >> >> A fairly solid test would be to run a sampling event (e.g. `perf stat`) >> taskset to a single CPU and observe the corresponding IRQ count increase >> in /proc/interrupts, for each core in turn. If that behaves as expected >> then chances are everything is indeed sane. >> >> Couple of nitpicks for the patch itself - you're almost there, but >> you've got spurious tabs on the blank lines, plus you need a proper >> commit message and your sign-off above the "---" line - anything you add >> below there is treated as additional commentary for reviewers' benefit >> and will be discarded by `git am`. >> >> Cheers, >> Robin. >> >>> >>> / Ben >>> >>> On Mon, 7 Feb 2022 at 13:20, Robin Murphy <robin.murphy@arm.com> wrote: >>> >>>> On 2022-02-07 08:14, Neil Armstrong wrote: >>>>> Hi, >>>>> >>>>> On 06/02/2022 15:43, Benjamin Mordaunt wrote: >>>>>> --- >>>>>> The dts for meson sm1 appears to omit the SoC's PMU, >>>>>> which is essential for accessing perf events regarding >>>>>> e.g. cache on e.g. the Odroid C4 platform. Add it. >>>>>> >>>>>> arch/arm64/boot/dts/amlogic/meson-sm1.dtsi | 13 ++++++++++++- >>>>>> 1 file changed, 12 insertions(+), 1 deletion(-) >>>>>> >>>>>> diff --git a/arch/arm64/boot/dts/amlogic/meson-sm1.dtsi >>>>>> b/arch/arm64/boot/dts/amlogic/meson-sm1.dtsi >>>>>> index 3d8b1f4f2..4147eecd2 100644 >>>>>> --- a/arch/arm64/boot/dts/amlogic/meson-sm1.dtsi >>>>>> +++ b/arch/arm64/boot/dts/amlogic/meson-sm1.dtsi >>>>>> @@ -6,6 +6,8 @@ >>>>>> #include "meson-g12-common.dtsi" >>>>>> #include <dt-bindings/clock/axg-audio-clkc.h> >>>>>> +#include <dt-bindings/interrupt-controller/irq.h> >>>>>> +#include <dt-bindings/interrupt-controller/arm-gic.h> >>>>>> #include <dt-bindings/power/meson-sm1-power.h> >>>>>> #include <dt-bindings/reset/amlogic,meson-axg-audio-arb.h> >>>>>> #include <dt-bindings/reset/amlogic,meson-g12a-audio-reset.h> >>>>>> @@ -90,7 +92,16 @@ l2: l2-cache0 { >>>>>> compatible = "cache"; >>>>>> }; >>>>>> }; >>>>>> - >>>>>> + >>>>>> + arm-pmu { >>>>>> + compatible = "arm,cortex-a55-pmu"; >>>>>> + interrupts = <GIC_SPI 137 IRQ_TYPE_LEVEL_HIGH>, >>>>>> + <GIC_SPI 138 IRQ_TYPE_LEVEL_HIGH>, >>>>>> + <GIC_SPI 153 IRQ_TYPE_LEVEL_HIGH>, >>>>>> + <GIC_SPI 154 IRQ_TYPE_LEVEL_HIGH>; >>>>>> + interrupt-affinity = <&cpu0>, <&cpu1>, <&cpu2>, <&cpu3>; >>>>>> + }; >>>>>> + >>>>>> cpu_opp_table: opp-table { >>>>>> compatible = "operating-points-v2"; >>>>>> opp-shared; >>>>> >>>>> Please see Marc's comments about PMU support: >>>>> http://lore.kernel.org/r/8735pcq63o.wl-maz@kernel.org >>>> >>>> If SM1 actually has distinct per-core interrupts as the patch implies >>>> then it's fine - it's only G12B and anything else that combines multiple >>>> PMU IRQs into a single SPI which are unsupportable. >>>> >>>> Robin. >>>> >>>
diff --git a/arch/arm64/boot/dts/amlogic/meson-sm1.dtsi b/arch/arm64/boot/dts/amlogic/meson-sm1.dtsi index 3d8b1f4f2..4147eecd2 100644 --- a/arch/arm64/boot/dts/amlogic/meson-sm1.dtsi +++ b/arch/arm64/boot/dts/amlogic/meson-sm1.dtsi @@ -6,6 +6,8 @@ #include "meson-g12-common.dtsi" #include <dt-bindings/clock/axg-audio-clkc.h> +#include <dt-bindings/interrupt-controller/irq.h> +#include <dt-bindings/interrupt-controller/arm-gic.h> #include <dt-bindings/power/meson-sm1-power.h> #include <dt-bindings/reset/amlogic,meson-axg-audio-arb.h> #include <dt-bindings/reset/amlogic,meson-g12a-audio-reset.h> @@ -90,7 +92,16 @@ l2: l2-cache0 { compatible = "cache"; }; }; - + + arm-pmu { + compatible = "arm,cortex-a55-pmu"; + interrupts = <GIC_SPI 137 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 138 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 153 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 154 IRQ_TYPE_LEVEL_HIGH>; + interrupt-affinity = <&cpu0>, <&cpu1>, <&cpu2>, <&cpu3>; + }; + cpu_opp_table: opp-table { compatible = "operating-points-v2"; opp-shared;