arm64: dts: Add PMU node for APM X-Gene Storm SOC

arm64: dts: Add PMU node for APM X-Gene Storm SOC

[Vinayak Kale]

This patch adds the PMU device tree node for APM X-Gene Storm SOC.

Please note that this patch has dependancy on a GIC driver patch [1] which is
yet to be approved by maintainers.

[1]- https://lkml.org/lkml/2014/2/27/605 
(irqchip:gic: change access of gicc_ctrl register to read modify write)

Signed-off-by: Vinayak Kale <vkale@apm.com>
---
 arch/arm64/boot/dts/apm-storm.dtsi | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/arch/arm64/boot/dts/apm-storm.dtsi b/arch/arm64/boot/dts/apm-storm.dtsi
index d37d736..587348d 100644
--- a/arch/arm64/boot/dts/apm-storm.dtsi
+++ b/arch/arm64/boot/dts/apm-storm.dtsi
@@ -97,6 +97,11 @@
 		clock-frequency = <50000000>;
 	};
 
+	pmu {
+		compatible = "arm,armv8-pmuv3";
+		interrupts = <1 12 0xff04>;
+	};
+
 	soc {
 		compatible = "simple-bus";
 		#address-cells = <2>;
-- 
1.8.2.1

arm64: dts: Add PMU node for APM X-Gene Storm SOC

[Mark Rutland]

On Mon, Mar 17, 2014 at 09:21:35AM +0000, Vinayak Kale wrote:
> This patch adds the PMU device tree node for APM X-Gene Storm SOC.
> 
> Please note that this patch has dependancy on a GIC driver patch [1] which is
> yet to be approved by maintainers.
> 
> [1]- https://lkml.org/lkml/2014/2/27/605 
> (irqchip:gic: change access of gicc_ctrl register to read modify write)
> 
> Signed-off-by: Vinayak Kale <vkale@apm.com>
> ---
>  arch/arm64/boot/dts/apm-storm.dtsi | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/apm-storm.dtsi b/arch/arm64/boot/dts/apm-storm.dtsi
> index d37d736..587348d 100644
> --- a/arch/arm64/boot/dts/apm-storm.dtsi
> +++ b/arch/arm64/boot/dts/apm-storm.dtsi
> @@ -97,6 +97,11 @@
>  		clock-frequency = <50000000>;
>  	};
>  
> +	pmu {
> +		compatible = "arm,armv8-pmuv3";
> +		interrupts = <1 12 0xff04>;
> +	};

Do Potenza CPUs only have the PMUv3 events, and nothing
Potenza-specific?

I would have expected something like:

compatible = "apm,potenza-pmu", "arm,armv8-pmuv3";

Cheers,
Mark.

arm64: dts: Add PMU node for APM X-Gene Storm SOC

[Vinayak Kale]

On Mon, Mar 17, 2014 at 4:03 PM, Mark Rutland <mark.rutland@arm.com> wrote:
> On Mon, Mar 17, 2014 at 09:21:35AM +0000, Vinayak Kale wrote:
>> This patch adds the PMU device tree node for APM X-Gene Storm SOC.
>>
>> Please note that this patch has dependancy on a GIC driver patch [1] which is
>> yet to be approved by maintainers.
>>
>> [1]- https://lkml.org/lkml/2014/2/27/605
>> (irqchip:gic: change access of gicc_ctrl register to read modify write)
>>
>> Signed-off-by: Vinayak Kale <vkale@apm.com>
>> ---
>>  arch/arm64/boot/dts/apm-storm.dtsi | 5 +++++
>>  1 file changed, 5 insertions(+)
>>
>> diff --git a/arch/arm64/boot/dts/apm-storm.dtsi b/arch/arm64/boot/dts/apm-storm.dtsi
>> index d37d736..587348d 100644
>> --- a/arch/arm64/boot/dts/apm-storm.dtsi
>> +++ b/arch/arm64/boot/dts/apm-storm.dtsi
>> @@ -97,6 +97,11 @@
>>               clock-frequency = <50000000>;
>>       };
>>
>> +     pmu {
>> +             compatible = "arm,armv8-pmuv3";
>> +             interrupts = <1 12 0xff04>;
>> +     };
>
> Do Potenza CPUs only have the PMUv3 events, and nothing
> Potenza-specific?
On top of PMUv3 events, Potenza CPU do have few implementation defined events.
>
> I would have expected something like:
>
> compatible = "apm,potenza-pmu", "arm,armv8-pmuv3";
PMUv3 allows to have implementation defined events. And as such the
driver doesn't have any dependency on such events. Do you still think
we need additional compatible string for Potenza CPU?
>
> Cheers,
> Mark.

arm64: dts: Add PMU node for APM X-Gene Storm SOC

[Vinayak Kale]

Somehow the "[PATCH]" string in the subject is missing, I'll see
what's wrong and correct the same in next version.

Thanks
-Vinayak

On Mon, Mar 17, 2014 at 2:51 PM, Vinayak Kale <vkale@apm.com> wrote:
> This patch adds the PMU device tree node for APM X-Gene Storm SOC.
>
> Please note that this patch has dependancy on a GIC driver patch [1] which is
> yet to be approved by maintainers.
>
> [1]- https://lkml.org/lkml/2014/2/27/605
> (irqchip:gic: change access of gicc_ctrl register to read modify write)
>
> Signed-off-by: Vinayak Kale <vkale@apm.com>
> ---
>  arch/arm64/boot/dts/apm-storm.dtsi | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/apm-storm.dtsi b/arch/arm64/boot/dts/apm-storm.dtsi
> index d37d736..587348d 100644
> --- a/arch/arm64/boot/dts/apm-storm.dtsi
> +++ b/arch/arm64/boot/dts/apm-storm.dtsi
> @@ -97,6 +97,11 @@
>                 clock-frequency = <50000000>;
>         };
>
> +       pmu {
> +               compatible = "arm,armv8-pmuv3";
> +               interrupts = <1 12 0xff04>;
> +       };
> +
>         soc {
>                 compatible = "simple-bus";
>                 #address-cells = <2>;
> --
> 1.8.2.1
>

arm64: dts: Add PMU node for APM X-Gene Storm SOC

[Mark Rutland]

On Mon, Mar 17, 2014 at 11:22:55AM +0000, Vinayak Kale wrote:
> On Mon, Mar 17, 2014 at 4:03 PM, Mark Rutland <mark.rutland@arm.com> wrote:
> > On Mon, Mar 17, 2014 at 09:21:35AM +0000, Vinayak Kale wrote:
> >> This patch adds the PMU device tree node for APM X-Gene Storm SOC.
> >>
> >> Please note that this patch has dependancy on a GIC driver patch [1] which is
> >> yet to be approved by maintainers.
> >>
> >> [1]- https://lkml.org/lkml/2014/2/27/605
> >> (irqchip:gic: change access of gicc_ctrl register to read modify write)
> >>
> >> Signed-off-by: Vinayak Kale <vkale@apm.com>
> >> ---
> >>  arch/arm64/boot/dts/apm-storm.dtsi | 5 +++++
> >>  1 file changed, 5 insertions(+)
> >>
> >> diff --git a/arch/arm64/boot/dts/apm-storm.dtsi b/arch/arm64/boot/dts/apm-storm.dtsi
> >> index d37d736..587348d 100644
> >> --- a/arch/arm64/boot/dts/apm-storm.dtsi
> >> +++ b/arch/arm64/boot/dts/apm-storm.dtsi
> >> @@ -97,6 +97,11 @@
> >>               clock-frequency = <50000000>;
> >>       };
> >>
> >> +     pmu {
> >> +             compatible = "arm,armv8-pmuv3";
> >> +             interrupts = <1 12 0xff04>;
> >> +     };
> >
> > Do Potenza CPUs only have the PMUv3 events, and nothing
> > Potenza-specific?
> On top of PMUv3 events, Potenza CPU do have few implementation defined events.

Ok. So Potenza is compatible with PMUv3 (and can have it in the list),
but also has additional features for which there should be a string.

> >
> > I would have expected something like:
> >
> > compatible = "apm,potenza-pmu", "arm,armv8-pmuv3";
> PMUv3 allows to have implementation defined events. And as such the
> driver doesn't have any dependency on such events. Do you still think
> we need additional compatible string for Potenza CPU?

I do. This is precisely the reason for the compatible list. Being as
explicit as possible with the compatible list entries and having a
series of fallbacks is the sensible thing to do.

Having the additional string allows things to work now with the standard
events, and possibly later with the full set of events, without
requiring every single DTB to be updated. It also allows us to target
workarounds more precisely, for any issues we might not currently be
aware of.

Cheers,
Mark.

arm64: dts: Add PMU node for APM X-Gene Storm SOC

[Vinayak Kale]

On Mon, Mar 17, 2014 at 5:14 PM, Mark Rutland <mark.rutland@arm.com> wrote:
> On Mon, Mar 17, 2014 at 11:22:55AM +0000, Vinayak Kale wrote:
>> On Mon, Mar 17, 2014 at 4:03 PM, Mark Rutland <mark.rutland@arm.com> wrote:
>> > On Mon, Mar 17, 2014 at 09:21:35AM +0000, Vinayak Kale wrote:
>> >> This patch adds the PMU device tree node for APM X-Gene Storm SOC.
>> >>
>> >> Please note that this patch has dependancy on a GIC driver patch [1] which is
>> >> yet to be approved by maintainers.
>> >>
>> >> [1]- https://lkml.org/lkml/2014/2/27/605
>> >> (irqchip:gic: change access of gicc_ctrl register to read modify write)
>> >>
>> >> Signed-off-by: Vinayak Kale <vkale@apm.com>
>> >> ---
>> >>  arch/arm64/boot/dts/apm-storm.dtsi | 5 +++++
>> >>  1 file changed, 5 insertions(+)
>> >>
>> >> diff --git a/arch/arm64/boot/dts/apm-storm.dtsi b/arch/arm64/boot/dts/apm-storm.dtsi
>> >> index d37d736..587348d 100644
>> >> --- a/arch/arm64/boot/dts/apm-storm.dtsi
>> >> +++ b/arch/arm64/boot/dts/apm-storm.dtsi
>> >> @@ -97,6 +97,11 @@
>> >>               clock-frequency = <50000000>;
>> >>       };
>> >>
>> >> +     pmu {
>> >> +             compatible = "arm,armv8-pmuv3";
>> >> +             interrupts = <1 12 0xff04>;
>> >> +     };
>> >
>> > Do Potenza CPUs only have the PMUv3 events, and nothing
>> > Potenza-specific?
>> On top of PMUv3 events, Potenza CPU do have few implementation defined events.
>
> Ok. So Potenza is compatible with PMUv3 (and can have it in the list),
> but also has additional features for which there should be a string.
>
>> >
>> > I would have expected something like:
>> >
>> > compatible = "apm,potenza-pmu", "arm,armv8-pmuv3";
>> PMUv3 allows to have implementation defined events. And as such the
>> driver doesn't have any dependency on such events. Do you still think
>> we need additional compatible string for Potenza CPU?
>
> I do. This is precisely the reason for the compatible list. Being as
> explicit as possible with the compatible list entries and having a
> series of fallbacks is the sensible thing to do.
>
> Having the additional string allows things to work now with the standard
> events, and possibly later with the full set of events, without
> requiring every single DTB to be updated. It also allows us to target
> workarounds more precisely, for any issues we might not currently be
> aware of.

Okay, will add the new string in next version.

For documentation of bindings for "apm,potenza-pmu", can I insert it
in existing arm-pmu bindings file
(i.e.Documentation/devicetree/bindings/arm/pmu.txt) or shall I create
new file?

>
> Cheers,
> Mark.

arm64: dts: Add PMU node for APM X-Gene Storm SOC

[Mark Rutland]

On Mon, Mar 17, 2014 at 12:01:56PM +0000, Vinayak Kale wrote:
> On Mon, Mar 17, 2014 at 5:14 PM, Mark Rutland <mark.rutland@arm.com> wrote:
> > On Mon, Mar 17, 2014 at 11:22:55AM +0000, Vinayak Kale wrote:
> >> On Mon, Mar 17, 2014 at 4:03 PM, Mark Rutland <mark.rutland@arm.com> wrote:
> >> > On Mon, Mar 17, 2014 at 09:21:35AM +0000, Vinayak Kale wrote:
> >> >> This patch adds the PMU device tree node for APM X-Gene Storm SOC.
> >> >>
> >> >> Please note that this patch has dependancy on a GIC driver patch [1] which is
> >> >> yet to be approved by maintainers.
> >> >>
> >> >> [1]- https://lkml.org/lkml/2014/2/27/605
> >> >> (irqchip:gic: change access of gicc_ctrl register to read modify write)
> >> >>
> >> >> Signed-off-by: Vinayak Kale <vkale@apm.com>
> >> >> ---
> >> >>  arch/arm64/boot/dts/apm-storm.dtsi | 5 +++++
> >> >>  1 file changed, 5 insertions(+)
> >> >>
> >> >> diff --git a/arch/arm64/boot/dts/apm-storm.dtsi b/arch/arm64/boot/dts/apm-storm.dtsi
> >> >> index d37d736..587348d 100644
> >> >> --- a/arch/arm64/boot/dts/apm-storm.dtsi
> >> >> +++ b/arch/arm64/boot/dts/apm-storm.dtsi
> >> >> @@ -97,6 +97,11 @@
> >> >>               clock-frequency = <50000000>;
> >> >>       };
> >> >>
> >> >> +     pmu {
> >> >> +             compatible = "arm,armv8-pmuv3";
> >> >> +             interrupts = <1 12 0xff04>;
> >> >> +     };
> >> >
> >> > Do Potenza CPUs only have the PMUv3 events, and nothing
> >> > Potenza-specific?
> >> On top of PMUv3 events, Potenza CPU do have few implementation defined events.
> >
> > Ok. So Potenza is compatible with PMUv3 (and can have it in the list),
> > but also has additional features for which there should be a string.
> >
> >> >
> >> > I would have expected something like:
> >> >
> >> > compatible = "apm,potenza-pmu", "arm,armv8-pmuv3";
> >> PMUv3 allows to have implementation defined events. And as such the
> >> driver doesn't have any dependency on such events. Do you still think
> >> we need additional compatible string for Potenza CPU?
> >
> > I do. This is precisely the reason for the compatible list. Being as
> > explicit as possible with the compatible list entries and having a
> > series of fallbacks is the sensible thing to do.
> >
> > Having the additional string allows things to work now with the standard
> > events, and possibly later with the full set of events, without
> > requiring every single DTB to be updated. It also allows us to target
> > workarounds more precisely, for any issues we might not currently be
> > aware of.
> 
> Okay, will add the new string in next version.
> 
> For documentation of bindings for "apm,potenza-pmu", can I insert it
> in existing arm-pmu bindings file
> (i.e.Documentation/devicetree/bindings/arm/pmu.txt) or shall I create
> new file?

Drop it in the existing file.

Mark.

arm64: dts: Add PMU node for APM X-Gene Storm SOC

[Mark Rutland]

On Tue, Mar 18, 2014 at 01:17:46PM +0000, Ashwin Chaugle wrote:
>    Hi Vinayak, Mark,
> 
>    On 17 March 2014 07:22, Vinayak Kale <[1]vkale@apm.com> wrote:
> 
>      > Do Potenza CPUs only have the PMUv3 events, and nothing
>      > Potenza-specific?
>      On top of PMUv3 events, Potenza CPU do have few implementation defined
>      events.
>      >
>      > I would have expected something like:
>      >
>      > compatible = "apm,potenza-pmu", "arm,armv8-pmuv3";
>      PMUv3 allows to have implementation defined events. And as such the
>      driver doesn't have any dependency on such events. Do you still think
>      we need additional compatible string for Potenza CPU?
> 
>    Can't these implementation specific events be used as raw perf events, or
>    extending that event space? (Or is that interface gone?) I may be missing
>    the details here, but I'm wondering if the method to program and count
>    these extra events is similar to ARM architected events, then why add
>    another DT binding?

It would be possible to use raw events, however that's a software level
detail, and not something that matters to the hardware description.

The hardware description should be as explicit as possible to allow the
software stack to change in future. For example, we might want to expose
implementation-specific events under sysfs in future, for which having
the implementation-specific compatible string in addition to the more
generic fallback is helpful.

This is why the compatible string exists. Making good use of it today
means a platform is more likely to have good support in future.

Having the string now is cheap, and makes it easier to tailor the
software stack more to the hardware in future. Not having the string
makes it harder. There is no reason not to have the more specific
compatible string.

Cheers,
Mark.