| Message ID | 20220206144343.2194-1-crawford.benjamin15@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] add pmu to amlogic meson sm1 | expand |
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;