1. Patchew
  2. linux
  3. View series

[PATCH] arm64: dts: renesas: r9a07g044: Add Cortex-A55 PMU node

Prabhakar posted 1 patch 2 years, 7 months ago
There is a newer version of this series
arch/arm64/boot/dts/renesas/r9a07g044.dtsi | 5 +++++
1 file changed, 5 insertions(+)
[PATCH] arm64: dts: renesas: r9a07g044: Add Cortex-A55 PMU node
Posted by Prabhakar 2 years, 7 months ago 
From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>

Enable the performance monitor unit for the Cortex-A55 cores on the
RZ/G2L (r9a07g044) SoC.

Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
---
 arch/arm64/boot/dts/renesas/r9a07g044.dtsi | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/arch/arm64/boot/dts/renesas/r9a07g044.dtsi b/arch/arm64/boot/dts/renesas/r9a07g044.dtsi
index 80b2332798d9..ff9bdc03a3ed 100644
--- a/arch/arm64/boot/dts/renesas/r9a07g044.dtsi
+++ b/arch/arm64/boot/dts/renesas/r9a07g044.dtsi
@@ -161,6 +161,11 @@ opp-50000000 {
 		};
 	};
 
+	pmu_a55 {
+		compatible = "arm,cortex-a55-pmu";
+		interrupts-extended = <&gic GIC_PPI 7 IRQ_TYPE_LEVEL_HIGH>;
+	};
+
 	psci {
 		compatible = "arm,psci-1.0", "arm,psci-0.2";
 		method = "smc";
-- 
2.25.1
RE: [PATCH] arm64: dts: renesas: r9a07g044: Add Cortex-A55 PMU node
Posted by Biju Das 2 years, 7 months ago 
Hi Prabhakar,

Thanks for the patch.

> Subject: [PATCH] arm64: dts: renesas: r9a07g044: Add Cortex-A55 PMU node
> 
> From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> 
> Enable the performance monitor unit for the Cortex-A55 cores on the RZ/G2L
> (r9a07g044) SoC.
> 
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> ---
>  arch/arm64/boot/dts/renesas/r9a07g044.dtsi | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/renesas/r9a07g044.dtsi
> b/arch/arm64/boot/dts/renesas/r9a07g044.dtsi
> index 80b2332798d9..ff9bdc03a3ed 100644
> --- a/arch/arm64/boot/dts/renesas/r9a07g044.dtsi
> +++ b/arch/arm64/boot/dts/renesas/r9a07g044.dtsi
> @@ -161,6 +161,11 @@ opp-50000000 {
>  		};
>  	};
> 
> +	pmu_a55 {
> +		compatible = "arm,cortex-a55-pmu";
> +		interrupts-extended = <&gic GIC_PPI 7 IRQ_TYPE_LEVEL_HIGH>;

Just a question, Is it tested? timer node[1] defines irq type as LOW, here it is high.
Also do we need to define (GIC_CPU_MASK_SIMPLE(2) | IRQ_TYPE_LEVEL_LOW) as it has 2 cores??

[1]
timer {
		compatible = "arm,armv8-timer";
		interrupts-extended = <&gic GIC_PPI 13 (GIC_CPU_MASK_SIMPLE(2) | IRQ_TYPE_LEVEL_LOW)>,
				      <&gic GIC_PPI 14 (GIC_CPU_MASK_SIMPLE(2) | IRQ_TYPE_LEVEL_LOW)>,
				      <&gic GIC_PPI 11 (GIC_CPU_MASK_SIMPLE(2) | IRQ_TYPE_LEVEL_LOW)>,
				      <&gic GIC_PPI 10 (GIC_CPU_MASK_SIMPLE(2) | IRQ_TYPE_LEVEL_LOW)>;
	};

Cheers,
Biju


> +	};
> +
>  	psci {
>  		compatible = "arm,psci-1.0", "arm,psci-0.2";
>  		method = "smc";
> --
> 2.25.1
Re: [PATCH] arm64: dts: renesas: r9a07g044: Add Cortex-A55 PMU node
Posted by Lad, Prabhakar 2 years, 7 months ago 
Hi Biju,

Thank you for the review.

On Fri, Jan 27, 2023 at 6:38 PM Biju Das <biju.das.jz@bp.renesas.com> wrote:
>
> Hi Prabhakar,
>
> Thanks for the patch.
>
> > Subject: [PATCH] arm64: dts: renesas: r9a07g044: Add Cortex-A55 PMU node
> >
> > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> >
> > Enable the performance monitor unit for the Cortex-A55 cores on the RZ/G2L
> > (r9a07g044) SoC.
> >
> > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > ---
> >  arch/arm64/boot/dts/renesas/r9a07g044.dtsi | 5 +++++
> >  1 file changed, 5 insertions(+)
> >
> > diff --git a/arch/arm64/boot/dts/renesas/r9a07g044.dtsi
> > b/arch/arm64/boot/dts/renesas/r9a07g044.dtsi
> > index 80b2332798d9..ff9bdc03a3ed 100644
> > --- a/arch/arm64/boot/dts/renesas/r9a07g044.dtsi
> > +++ b/arch/arm64/boot/dts/renesas/r9a07g044.dtsi
> > @@ -161,6 +161,11 @@ opp-50000000 {
> >               };
> >       };
> >
> > +     pmu_a55 {
> > +             compatible = "arm,cortex-a55-pmu";
> > +             interrupts-extended = <&gic GIC_PPI 7 IRQ_TYPE_LEVEL_HIGH>;
>
> Just a question, Is it tested?
Yes this was tested with perf test (https://pastebin.com/dkckcYHr)

> timer node[1] defines irq type as LOW, here it is high.
You are right looking at the RZG2L_InterruptMapping_rev01.xlsx this
should be LOW. (I followed the SPI IRQS where all the LEVEL interrupts
are HIGH)

> Also do we need to define (GIC_CPU_MASK_SIMPLE(2) | IRQ_TYPE_LEVEL_LOW) as it has 2 cores??
>
No this is not required for example here [0] where it has 6 cores.

[0] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/boot/dts/renesas/r8a779f0.dtsi?h=v6.2-rc5#n203

Cheers,
Prabhakar
RE: [PATCH] arm64: dts: renesas: r9a07g044: Add Cortex-A55 PMU node
Posted by Biju Das 2 years, 7 months ago 
Hi Prabhakar,


> Subject: Re: [PATCH] arm64: dts: renesas: r9a07g044: Add Cortex-A55 PMU node
> 
> Hi Biju,
> 
> Thank you for the review.
> 
> On Fri, Jan 27, 2023 at 6:38 PM Biju Das <biju.das.jz@bp.renesas.com> wrote:
> >
> > Hi Prabhakar,
> >
> > Thanks for the patch.
> >
> > > Subject: [PATCH] arm64: dts: renesas: r9a07g044: Add Cortex-A55 PMU
> > > node
> > >
> > > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > >
> > > Enable the performance monitor unit for the Cortex-A55 cores on the
> > > RZ/G2L
> > > (r9a07g044) SoC.
> > >
> > > Signed-off-by: Lad Prabhakar
> > > <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > > ---
> > >  arch/arm64/boot/dts/renesas/r9a07g044.dtsi | 5 +++++
> > >  1 file changed, 5 insertions(+)
> > >
> > > diff --git a/arch/arm64/boot/dts/renesas/r9a07g044.dtsi
> > > b/arch/arm64/boot/dts/renesas/r9a07g044.dtsi
> > > index 80b2332798d9..ff9bdc03a3ed 100644
> > > --- a/arch/arm64/boot/dts/renesas/r9a07g044.dtsi
> > > +++ b/arch/arm64/boot/dts/renesas/r9a07g044.dtsi
> > > @@ -161,6 +161,11 @@ opp-50000000 {
> > >               };
> > >       };
> > >
> > > +     pmu_a55 {
> > > +             compatible = "arm,cortex-a55-pmu";
> > > +             interrupts-extended = <&gic GIC_PPI 7
> > > + IRQ_TYPE_LEVEL_HIGH>;
> >
> > Just a question, Is it tested?
> Yes this was tested with perf test
> 
> > timer node[1] defines irq type as LOW, here it is high.
> You are right looking at the RZG2L_InterruptMapping_rev01.xlsx this should
> be LOW. (I followed the SPI IRQS where all the LEVEL interrupts are HIGH)
> 
> > Also do we need to define (GIC_CPU_MASK_SIMPLE(2) | IRQ_TYPE_LEVEL_LOW) as
> it has 2 cores??
> >
> No this is not required for example here [0] where it has 6 cores.

I may be wrong, That is the only example[1], where the A55 PMU per cpu interrupts and number of a55 cores in the DT 
are not matching.

[1]
https://elixir.bootlin.com/linux/latest/B/ident/arm%2Ccortex-a55-pmu

Cheers,
Biju
Re: [PATCH] arm64: dts: renesas: r9a07g044: Add Cortex-A55 PMU node
Posted by Lad, Prabhakar 2 years, 7 months ago 
Hi Mark and Will,

On Fri, Jan 27, 2023 at 9:48 PM Biju Das <biju.das.jz@bp.renesas.com> wrote:
>
> Hi Prabhakar,
>
>
> > Subject: Re: [PATCH] arm64: dts: renesas: r9a07g044: Add Cortex-A55 PMU node
> >
> > Hi Biju,
> >
> > Thank you for the review.
> >
> > On Fri, Jan 27, 2023 at 6:38 PM Biju Das <biju.das.jz@bp.renesas.com> wrote:
> > >
> > > Hi Prabhakar,
> > >
> > > Thanks for the patch.
> > >
> > > > Subject: [PATCH] arm64: dts: renesas: r9a07g044: Add Cortex-A55 PMU
> > > > node
> > > >
> > > > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > > >
> > > > Enable the performance monitor unit for the Cortex-A55 cores on the
> > > > RZ/G2L
> > > > (r9a07g044) SoC.
> > > >
> > > > Signed-off-by: Lad Prabhakar
> > > > <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > > > ---
> > > >  arch/arm64/boot/dts/renesas/r9a07g044.dtsi | 5 +++++
> > > >  1 file changed, 5 insertions(+)
> > > >
> > > > diff --git a/arch/arm64/boot/dts/renesas/r9a07g044.dtsi
> > > > b/arch/arm64/boot/dts/renesas/r9a07g044.dtsi
> > > > index 80b2332798d9..ff9bdc03a3ed 100644
> > > > --- a/arch/arm64/boot/dts/renesas/r9a07g044.dtsi
> > > > +++ b/arch/arm64/boot/dts/renesas/r9a07g044.dtsi
> > > > @@ -161,6 +161,11 @@ opp-50000000 {
> > > >               };
> > > >       };
> > > >
> > > > +     pmu_a55 {
> > > > +             compatible = "arm,cortex-a55-pmu";
> > > > +             interrupts-extended = <&gic GIC_PPI 7
> > > > + IRQ_TYPE_LEVEL_HIGH>;
> > >
> > > Just a question, Is it tested?
> > Yes this was tested with perf test
> >
> > > timer node[1] defines irq type as LOW, here it is high.
> > You are right looking at the RZG2L_InterruptMapping_rev01.xlsx this should
> > be LOW. (I followed the SPI IRQS where all the LEVEL interrupts are HIGH)
> >
> > > Also do we need to define (GIC_CPU_MASK_SIMPLE(2) | IRQ_TYPE_LEVEL_LOW) as
> > it has 2 cores??
> > >
> > No this is not required for example here [0] where it has 6 cores.
>
> I may be wrong, That is the only example[1], where the A55 PMU per cpu interrupts and number of a55 cores in the DT
> are not matching.
>
Some SoCs specify the GIC_CPU_MASK_SIMPLE(x) while describing the PPI
interrupt for the PMU and some dont [1]. What should be the correct
usage when specifying the PPI interrupts for the PMU with multiple CPU
cores (we are using
arm,cortex-a55-pmu)?

 [1] https://elixir.bootlin.com/linux/latest/B/ident/arm%2Ccortex-a55-pmu

Cheers,
Prabhakar
Re: [PATCH] arm64: dts: renesas: r9a07g044: Add Cortex-A55 PMU node
Posted by Mark Rutland 2 years, 7 months ago 
On Mon, Jan 30, 2023 at 02:04:44PM +0000, Lad, Prabhakar wrote:
> Hi Mark and Will,
> 
> On Fri, Jan 27, 2023 at 9:48 PM Biju Das <biju.das.jz@bp.renesas.com> wrote:
> >
> > Hi Prabhakar,
> >
> >
> > > Subject: Re: [PATCH] arm64: dts: renesas: r9a07g044: Add Cortex-A55 PMU node
> > >
> > > Hi Biju,
> > >
> > > Thank you for the review.
> > >
> > > On Fri, Jan 27, 2023 at 6:38 PM Biju Das <biju.das.jz@bp.renesas.com> wrote:
> > > >
> > > > Hi Prabhakar,
> > > >
> > > > Thanks for the patch.
> > > >
> > > > > Subject: [PATCH] arm64: dts: renesas: r9a07g044: Add Cortex-A55 PMU
> > > > > node
> > > > >
> > > > > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > > > >
> > > > > Enable the performance monitor unit for the Cortex-A55 cores on the
> > > > > RZ/G2L
> > > > > (r9a07g044) SoC.
> > > > >
> > > > > Signed-off-by: Lad Prabhakar
> > > > > <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > > > > ---
> > > > >  arch/arm64/boot/dts/renesas/r9a07g044.dtsi | 5 +++++
> > > > >  1 file changed, 5 insertions(+)
> > > > >
> > > > > diff --git a/arch/arm64/boot/dts/renesas/r9a07g044.dtsi
> > > > > b/arch/arm64/boot/dts/renesas/r9a07g044.dtsi
> > > > > index 80b2332798d9..ff9bdc03a3ed 100644
> > > > > --- a/arch/arm64/boot/dts/renesas/r9a07g044.dtsi
> > > > > +++ b/arch/arm64/boot/dts/renesas/r9a07g044.dtsi
> > > > > @@ -161,6 +161,11 @@ opp-50000000 {
> > > > >               };
> > > > >       };
> > > > >
> > > > > +     pmu_a55 {
> > > > > +             compatible = "arm,cortex-a55-pmu";
> > > > > +             interrupts-extended = <&gic GIC_PPI 7
> > > > > + IRQ_TYPE_LEVEL_HIGH>;
> > > >
> > > > Just a question, Is it tested?
> > > Yes this was tested with perf test
> > >
> > > > timer node[1] defines irq type as LOW, here it is high.
> > > You are right looking at the RZG2L_InterruptMapping_rev01.xlsx this should
> > > be LOW. (I followed the SPI IRQS where all the LEVEL interrupts are HIGH)
> > >
> > > > Also do we need to define (GIC_CPU_MASK_SIMPLE(2) | IRQ_TYPE_LEVEL_LOW) as
> > > it has 2 cores??
> > > >
> > > No this is not required for example here [0] where it has 6 cores.
> >
> > I may be wrong, That is the only example[1], where the A55 PMU per cpu interrupts and number of a55 cores in the DT
> > are not matching.
> >
> Some SoCs specify the GIC_CPU_MASK_SIMPLE(x) while describing the PPI
> interrupt for the PMU and some dont [1]. What should be the correct
> usage when specifying the PPI interrupts for the PMU with multiple CPU
> cores (we are using
> arm,cortex-a55-pmu)?
> 
>  [1] https://elixir.bootlin.com/linux/latest/B/ident/arm%2Ccortex-a55-pmu

This is a GICv3 system. the GICv3 interrupts binding *does not* have a cpumask,
and it's always wrong to use GIC_CPU_MASK_SIMPLE() (or any mask, for that
matter) for GICv3

The GICv2 binding has the mask, but even there it's arguably pointless.

Please do not add the mask here, since it would violate the GICv3 binding.

Thanks,
Mark.
Re: [PATCH] arm64: dts: renesas: r9a07g044: Add Cortex-A55 PMU node
Posted by Lad, Prabhakar 2 years, 7 months ago 
Hi Mark,

On Mon, Jan 30, 2023 at 2:10 PM Mark Rutland <mark.rutland@arm.com> wrote:
>
> On Mon, Jan 30, 2023 at 02:04:44PM +0000, Lad, Prabhakar wrote:
> > Hi Mark and Will,
> >
> > On Fri, Jan 27, 2023 at 9:48 PM Biju Das <biju.das.jz@bp.renesas.com> wrote:
> > >
> > > Hi Prabhakar,
> > >
> > >
> > > > Subject: Re: [PATCH] arm64: dts: renesas: r9a07g044: Add Cortex-A55 PMU node
> > > >
> > > > Hi Biju,
> > > >
> > > > Thank you for the review.
> > > >
> > > > On Fri, Jan 27, 2023 at 6:38 PM Biju Das <biju.das.jz@bp.renesas.com> wrote:
> > > > >
> > > > > Hi Prabhakar,
> > > > >
> > > > > Thanks for the patch.
> > > > >
> > > > > > Subject: [PATCH] arm64: dts: renesas: r9a07g044: Add Cortex-A55 PMU
> > > > > > node
> > > > > >
> > > > > > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > > > > >
> > > > > > Enable the performance monitor unit for the Cortex-A55 cores on the
> > > > > > RZ/G2L
> > > > > > (r9a07g044) SoC.
> > > > > >
> > > > > > Signed-off-by: Lad Prabhakar
> > > > > > <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > > > > > ---
> > > > > >  arch/arm64/boot/dts/renesas/r9a07g044.dtsi | 5 +++++
> > > > > >  1 file changed, 5 insertions(+)
> > > > > >
> > > > > > diff --git a/arch/arm64/boot/dts/renesas/r9a07g044.dtsi
> > > > > > b/arch/arm64/boot/dts/renesas/r9a07g044.dtsi
> > > > > > index 80b2332798d9..ff9bdc03a3ed 100644
> > > > > > --- a/arch/arm64/boot/dts/renesas/r9a07g044.dtsi
> > > > > > +++ b/arch/arm64/boot/dts/renesas/r9a07g044.dtsi
> > > > > > @@ -161,6 +161,11 @@ opp-50000000 {
> > > > > >               };
> > > > > >       };
> > > > > >
> > > > > > +     pmu_a55 {
> > > > > > +             compatible = "arm,cortex-a55-pmu";
> > > > > > +             interrupts-extended = <&gic GIC_PPI 7
> > > > > > + IRQ_TYPE_LEVEL_HIGH>;
> > > > >
> > > > > Just a question, Is it tested?
> > > > Yes this was tested with perf test
> > > >
> > > > > timer node[1] defines irq type as LOW, here it is high.
> > > > You are right looking at the RZG2L_InterruptMapping_rev01.xlsx this should
> > > > be LOW. (I followed the SPI IRQS where all the LEVEL interrupts are HIGH)
> > > >
> > > > > Also do we need to define (GIC_CPU_MASK_SIMPLE(2) | IRQ_TYPE_LEVEL_LOW) as
> > > > it has 2 cores??
> > > > >
> > > > No this is not required for example here [0] where it has 6 cores.
> > >
> > > I may be wrong, That is the only example[1], where the A55 PMU per cpu interrupts and number of a55 cores in the DT
> > > are not matching.
> > >
> > Some SoCs specify the GIC_CPU_MASK_SIMPLE(x) while describing the PPI
> > interrupt for the PMU and some dont [1]. What should be the correct
> > usage when specifying the PPI interrupts for the PMU with multiple CPU
> > cores (we are using
> > arm,cortex-a55-pmu)?
> >
> >  [1] https://elixir.bootlin.com/linux/latest/B/ident/arm%2Ccortex-a55-pmu
>
> This is a GICv3 system. the GICv3 interrupts binding *does not* have a cpumask,
> and it's always wrong to use GIC_CPU_MASK_SIMPLE() (or any mask, for that
> matter) for GICv3
>
> The GICv2 binding has the mask, but even there it's arguably pointless.
>
> Please do not add the mask here, since it would violate the GICv3 binding.
>
Thank you for the clarification.

(Note to myself, to drop GIC_CPU_MASK_SIMPLE() from timer nodes from
rzg2l family)

Cheers,
Prabhakar
Re: [PATCH] arm64: dts: renesas: r9a07g044: Add Cortex-A55 PMU node
Posted by Geert Uytterhoeven 2 years, 7 months ago 
On Fri, Jan 27, 2023 at 10:48 PM Biju Das <biju.das.jz@bp.renesas.com> wrote:
> > Subject: Re: [PATCH] arm64: dts: renesas: r9a07g044: Add Cortex-A55 PMU node
> > On Fri, Jan 27, 2023 at 6:38 PM Biju Das <biju.das.jz@bp.renesas.com> wrote:
> > > > Subject: [PATCH] arm64: dts: renesas: r9a07g044: Add Cortex-A55 PMU
> > > > node
> > > >
> > > > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > > >
> > > > Enable the performance monitor unit for the Cortex-A55 cores on the
> > > > RZ/G2L
> > > > (r9a07g044) SoC.
> > > >
> > > > Signed-off-by: Lad Prabhakar
> > > > <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > > > ---
> > > >  arch/arm64/boot/dts/renesas/r9a07g044.dtsi | 5 +++++
> > > >  1 file changed, 5 insertions(+)
> > > >
> > > > diff --git a/arch/arm64/boot/dts/renesas/r9a07g044.dtsi
> > > > b/arch/arm64/boot/dts/renesas/r9a07g044.dtsi
> > > > index 80b2332798d9..ff9bdc03a3ed 100644
> > > > --- a/arch/arm64/boot/dts/renesas/r9a07g044.dtsi
> > > > +++ b/arch/arm64/boot/dts/renesas/r9a07g044.dtsi
> > > > @@ -161,6 +161,11 @@ opp-50000000 {
> > > >               };
> > > >       };
> > > >
> > > > +     pmu_a55 {
> > > > +             compatible = "arm,cortex-a55-pmu";
> > > > +             interrupts-extended = <&gic GIC_PPI 7
> > > > + IRQ_TYPE_LEVEL_HIGH>;
> > >
> > > Just a question, Is it tested?
> > Yes this was tested with perf test
> >
> > > timer node[1] defines irq type as LOW, here it is high.
> > You are right looking at the RZG2L_InterruptMapping_rev01.xlsx this should
> > be LOW. (I followed the SPI IRQS where all the LEVEL interrupts are HIGH)
> >
> > > Also do we need to define (GIC_CPU_MASK_SIMPLE(2) | IRQ_TYPE_LEVEL_LOW) as
> > it has 2 cores??
> > >
> > No this is not required for example here [0] where it has 6 cores.
>
> I may be wrong, That is the only example[1], where the A55 PMU per cpu interrupts and number of a55 cores in the DT
> are not matching.
>
> [1]
> https://elixir.bootlin.com/linux/latest/B/ident/arm%2Ccortex-a55-pmu

Indeed, this looks like an omission, propagated through
arch/arm64/boot/dts/renesas/r8a779[afg]0.dtsi.

And doesn't this apply to all PPI interrupts, i.e. shouldn't the GIC
in arch/arm64/boot/dts/renesas/r9a07g0{43u,44u,54}.dtsi specify the
mask in their interrupts properties, too?

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds
Re: [PATCH] arm64: dts: renesas: r9a07g044: Add Cortex-A55 PMU node
Posted by Lad, Prabhakar 2 years, 7 months ago 
Hi Geert,

On Mon, Jan 30, 2023 at 10:05 AM Geert Uytterhoeven
<geert@linux-m68k.org> wrote:
>
> On Fri, Jan 27, 2023 at 10:48 PM Biju Das <biju.das.jz@bp.renesas.com> wrote:
> > > Subject: Re: [PATCH] arm64: dts: renesas: r9a07g044: Add Cortex-A55 PMU node
> > > On Fri, Jan 27, 2023 at 6:38 PM Biju Das <biju.das.jz@bp.renesas.com> wrote:
> > > > > Subject: [PATCH] arm64: dts: renesas: r9a07g044: Add Cortex-A55 PMU
> > > > > node
> > > > >
> > > > > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > > > >
> > > > > Enable the performance monitor unit for the Cortex-A55 cores on the
> > > > > RZ/G2L
> > > > > (r9a07g044) SoC.
> > > > >
> > > > > Signed-off-by: Lad Prabhakar
> > > > > <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > > > > ---
> > > > >  arch/arm64/boot/dts/renesas/r9a07g044.dtsi | 5 +++++
> > > > >  1 file changed, 5 insertions(+)
> > > > >
> > > > > diff --git a/arch/arm64/boot/dts/renesas/r9a07g044.dtsi
> > > > > b/arch/arm64/boot/dts/renesas/r9a07g044.dtsi
> > > > > index 80b2332798d9..ff9bdc03a3ed 100644
> > > > > --- a/arch/arm64/boot/dts/renesas/r9a07g044.dtsi
> > > > > +++ b/arch/arm64/boot/dts/renesas/r9a07g044.dtsi
> > > > > @@ -161,6 +161,11 @@ opp-50000000 {
> > > > >               };
> > > > >       };
> > > > >
> > > > > +     pmu_a55 {
> > > > > +             compatible = "arm,cortex-a55-pmu";
> > > > > +             interrupts-extended = <&gic GIC_PPI 7
> > > > > + IRQ_TYPE_LEVEL_HIGH>;
> > > >
> > > > Just a question, Is it tested?
> > > Yes this was tested with perf test
> > >
> > > > timer node[1] defines irq type as LOW, here it is high.
> > > You are right looking at the RZG2L_InterruptMapping_rev01.xlsx this should
> > > be LOW. (I followed the SPI IRQS where all the LEVEL interrupts are HIGH)
> > >
> > > > Also do we need to define (GIC_CPU_MASK_SIMPLE(2) | IRQ_TYPE_LEVEL_LOW) as
> > > it has 2 cores??
> > > >
> > > No this is not required for example here [0] where it has 6 cores.
> >
> > I may be wrong, That is the only example[1], where the A55 PMU per cpu interrupts and number of a55 cores in the DT
> > are not matching.
> >
> > [1]
> > https://elixir.bootlin.com/linux/latest/B/ident/arm%2Ccortex-a55-pmu
>
> Indeed, this looks like an omission, propagated through
> arch/arm64/boot/dts/renesas/r8a779[afg]0.dtsi.
>
> And doesn't this apply to all PPI interrupts, i.e. shouldn't the GIC
> in arch/arm64/boot/dts/renesas/r9a07g0{43u,44u,54}.dtsi specify the
> mask in their interrupts properties, too?
>
I was under the impression that the GIC_CPU_MASK_SIMPLE(x) was only
needed if the driver handled per-cpu stuff.

Marc, what should be the correct usage?

Cheers,
Prabhakar
Re: [PATCH] arm64: dts: renesas: r9a07g044: Add Cortex-A55 PMU node
Posted by Marc Zyngier 2 years, 7 months ago 
On Mon, 30 Jan 2023 13:13:26 +0000,
"Lad, Prabhakar" <prabhakar.csengg@gmail.com> wrote:
> 
> Hi Geert,
> 
> On Mon, Jan 30, 2023 at 10:05 AM Geert Uytterhoeven
> <geert@linux-m68k.org> wrote:
> >
> > On Fri, Jan 27, 2023 at 10:48 PM Biju Das <biju.das.jz@bp.renesas.com> wrote:
> > > > Subject: Re: [PATCH] arm64: dts: renesas: r9a07g044: Add Cortex-A55 PMU node
> > > > On Fri, Jan 27, 2023 at 6:38 PM Biju Das <biju.das.jz@bp.renesas.com> wrote:
> > > > > > Subject: [PATCH] arm64: dts: renesas: r9a07g044: Add Cortex-A55 PMU
> > > > > > node
> > > > > >
> > > > > > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > > > > >
> > > > > > Enable the performance monitor unit for the Cortex-A55 cores on the
> > > > > > RZ/G2L
> > > > > > (r9a07g044) SoC.
> > > > > >
> > > > > > Signed-off-by: Lad Prabhakar
> > > > > > <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > > > > > ---
> > > > > >  arch/arm64/boot/dts/renesas/r9a07g044.dtsi | 5 +++++
> > > > > >  1 file changed, 5 insertions(+)
> > > > > >
> > > > > > diff --git a/arch/arm64/boot/dts/renesas/r9a07g044.dtsi
> > > > > > b/arch/arm64/boot/dts/renesas/r9a07g044.dtsi
> > > > > > index 80b2332798d9..ff9bdc03a3ed 100644
> > > > > > --- a/arch/arm64/boot/dts/renesas/r9a07g044.dtsi
> > > > > > +++ b/arch/arm64/boot/dts/renesas/r9a07g044.dtsi
> > > > > > @@ -161,6 +161,11 @@ opp-50000000 {
> > > > > >               };
> > > > > >       };
> > > > > >
> > > > > > +     pmu_a55 {
> > > > > > +             compatible = "arm,cortex-a55-pmu";
> > > > > > +             interrupts-extended = <&gic GIC_PPI 7
> > > > > > + IRQ_TYPE_LEVEL_HIGH>;
> > > > >
> > > > > Just a question, Is it tested?
> > > > Yes this was tested with perf test
> > > >
> > > > > timer node[1] defines irq type as LOW, here it is high.
> > > > You are right looking at the RZG2L_InterruptMapping_rev01.xlsx this should
> > > > be LOW. (I followed the SPI IRQS where all the LEVEL interrupts are HIGH)
> > > >
> > > > > Also do we need to define (GIC_CPU_MASK_SIMPLE(2) | IRQ_TYPE_LEVEL_LOW) as
> > > > it has 2 cores??
> > > > >
> > > > No this is not required for example here [0] where it has 6 cores.
> > >
> > > I may be wrong, That is the only example[1], where the A55 PMU per cpu interrupts and number of a55 cores in the DT
> > > are not matching.
> > >
> > > [1]
> > > https://elixir.bootlin.com/linux/latest/B/ident/arm%2Ccortex-a55-pmu
> >
> > Indeed, this looks like an omission, propagated through
> > arch/arm64/boot/dts/renesas/r8a779[afg]0.dtsi.
> >
> > And doesn't this apply to all PPI interrupts, i.e. shouldn't the GIC
> > in arch/arm64/boot/dts/renesas/r9a07g0{43u,44u,54}.dtsi specify the
> > mask in their interrupts properties, too?
> >
> I was under the impression that the GIC_CPU_MASK_SIMPLE(x) was only
> needed if the driver handled per-cpu stuff.
> 
> Marc, what should be the correct usage?

I'm reading the DT correctly, this system has a GICv3, which is quite
natural for an A55-based system. For this configuration, no mask is
required.

The CPU mask stuff only applies to pre-GICv3. With GICv3+, you simply
cannot express such a mask, as there is no practical limit to the
number of CPUs.

	M.

-- 
Without deviation from the norm, progress is not possible.
Re: [PATCH] arm64: dts: renesas: r9a07g044: Add Cortex-A55 PMU node
Posted by Lad, Prabhakar 2 years, 7 months ago 
Hi Marc,

On Mon, Jan 30, 2023 at 1:26 PM Marc Zyngier <maz@kernel.org> wrote:
>
> On Mon, 30 Jan 2023 13:13:26 +0000,
> "Lad, Prabhakar" <prabhakar.csengg@gmail.com> wrote:
> >
> > Hi Geert,
> >
> > On Mon, Jan 30, 2023 at 10:05 AM Geert Uytterhoeven
> > <geert@linux-m68k.org> wrote:
> > >
> > > On Fri, Jan 27, 2023 at 10:48 PM Biju Das <biju.das.jz@bp.renesas.com> wrote:
> > > > > Subject: Re: [PATCH] arm64: dts: renesas: r9a07g044: Add Cortex-A55 PMU node
> > > > > On Fri, Jan 27, 2023 at 6:38 PM Biju Das <biju.das.jz@bp.renesas.com> wrote:
> > > > > > > Subject: [PATCH] arm64: dts: renesas: r9a07g044: Add Cortex-A55 PMU
> > > > > > > node
> > > > > > >
> > > > > > > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > > > > > >
> > > > > > > Enable the performance monitor unit for the Cortex-A55 cores on the
> > > > > > > RZ/G2L
> > > > > > > (r9a07g044) SoC.
> > > > > > >
> > > > > > > Signed-off-by: Lad Prabhakar
> > > > > > > <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > > > > > > ---
> > > > > > >  arch/arm64/boot/dts/renesas/r9a07g044.dtsi | 5 +++++
> > > > > > >  1 file changed, 5 insertions(+)
> > > > > > >
> > > > > > > diff --git a/arch/arm64/boot/dts/renesas/r9a07g044.dtsi
> > > > > > > b/arch/arm64/boot/dts/renesas/r9a07g044.dtsi
> > > > > > > index 80b2332798d9..ff9bdc03a3ed 100644
> > > > > > > --- a/arch/arm64/boot/dts/renesas/r9a07g044.dtsi
> > > > > > > +++ b/arch/arm64/boot/dts/renesas/r9a07g044.dtsi
> > > > > > > @@ -161,6 +161,11 @@ opp-50000000 {
> > > > > > >               };
> > > > > > >       };
> > > > > > >
> > > > > > > +     pmu_a55 {
> > > > > > > +             compatible = "arm,cortex-a55-pmu";
> > > > > > > +             interrupts-extended = <&gic GIC_PPI 7
> > > > > > > + IRQ_TYPE_LEVEL_HIGH>;
> > > > > >
> > > > > > Just a question, Is it tested?
> > > > > Yes this was tested with perf test
> > > > >
> > > > > > timer node[1] defines irq type as LOW, here it is high.
> > > > > You are right looking at the RZG2L_InterruptMapping_rev01.xlsx this should
> > > > > be LOW. (I followed the SPI IRQS where all the LEVEL interrupts are HIGH)
> > > > >
> > > > > > Also do we need to define (GIC_CPU_MASK_SIMPLE(2) | IRQ_TYPE_LEVEL_LOW) as
> > > > > it has 2 cores??
> > > > > >
> > > > > No this is not required for example here [0] where it has 6 cores.
> > > >
> > > > I may be wrong, That is the only example[1], where the A55 PMU per cpu interrupts and number of a55 cores in the DT
> > > > are not matching.
> > > >
> > > > [1]
> > > > https://elixir.bootlin.com/linux/latest/B/ident/arm%2Ccortex-a55-pmu
> > >
> > > Indeed, this looks like an omission, propagated through
> > > arch/arm64/boot/dts/renesas/r8a779[afg]0.dtsi.
> > >
> > > And doesn't this apply to all PPI interrupts, i.e. shouldn't the GIC
> > > in arch/arm64/boot/dts/renesas/r9a07g0{43u,44u,54}.dtsi specify the
> > > mask in their interrupts properties, too?
> > >
> > I was under the impression that the GIC_CPU_MASK_SIMPLE(x) was only
> > needed if the driver handled per-cpu stuff.
> >
> > Marc, what should be the correct usage?
>
> I'm reading the DT correctly, this system has a GICv3, which is quite
> natural for an A55-based system. For this configuration, no mask is
> required.
>
> The CPU mask stuff only applies to pre-GICv3. With GICv3+, you simply
> cannot express such a mask, as there is no practical limit to the
> number of CPUs.
>
Thank you for the clarification.

Cheers,
Prabhakar
Re: [PATCH] arm64: dts: renesas: r9a07g044: Add Cortex-A55 PMU node
Posted by Krzysztof Kozlowski 2 years, 7 months ago 
On 27/01/2023 18:40, Prabhakar wrote:
> From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> 
> Enable the performance monitor unit for the Cortex-A55 cores on the
> RZ/G2L (r9a07g044) SoC.
> 
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> ---
>  arch/arm64/boot/dts/renesas/r9a07g044.dtsi | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/renesas/r9a07g044.dtsi b/arch/arm64/boot/dts/renesas/r9a07g044.dtsi
> index 80b2332798d9..ff9bdc03a3ed 100644
> --- a/arch/arm64/boot/dts/renesas/r9a07g044.dtsi
> +++ b/arch/arm64/boot/dts/renesas/r9a07g044.dtsi
> @@ -161,6 +161,11 @@ opp-50000000 {
>  		};
>  	};
>  
> +	pmu_a55 {

No underscores in node names. This is usually called just 'pmu'.

Best regards,
Krzysztof
Re: [PATCH] arm64: dts: renesas: r9a07g044: Add Cortex-A55 PMU node
Posted by Lad, Prabhakar 2 years, 7 months ago 
Hi Krzysztof,

Thank you for the review.

On Fri, Jan 27, 2023 at 5:54 PM Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:
>
> On 27/01/2023 18:40, Prabhakar wrote:
> > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> >
> > Enable the performance monitor unit for the Cortex-A55 cores on the
> > RZ/G2L (r9a07g044) SoC.
> >
> > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > ---
> >  arch/arm64/boot/dts/renesas/r9a07g044.dtsi | 5 +++++
> >  1 file changed, 5 insertions(+)
> >
> > diff --git a/arch/arm64/boot/dts/renesas/r9a07g044.dtsi b/arch/arm64/boot/dts/renesas/r9a07g044.dtsi
> > index 80b2332798d9..ff9bdc03a3ed 100644
> > --- a/arch/arm64/boot/dts/renesas/r9a07g044.dtsi
> > +++ b/arch/arm64/boot/dts/renesas/r9a07g044.dtsi
> > @@ -161,6 +161,11 @@ opp-50000000 {
> >               };
> >       };
> >
> > +     pmu_a55 {
>
> No underscores in node names. This is usually called just 'pmu'.
>
Ok, I will update it in the next version.

Cheers,
Prabhakar

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.