target/ppc: Add ibm, purr and ibm, spurr device-tree properties

[Qemu-devel] [QEMU-PPC] [PATCH] target/ppc: Add ibm, purr and ibm, spurr device-tree properties

Posted by Suraj Jitindar Singh 4 years, 11 months ago

The ibm,purr and ibm,spurr device tree properties are used to indicate
that the processor implements the Processor Utilisation of Resources
Register (PURR) and Scaled Processor Utilisation of Resources Registers
(SPURR), respectively. Each property has a single value which represents
the level of architecture supported. A value of 1 for ibm,purr means
support for the version of the PURR defined in book 3 in version 2.02 of
the architecture. A value of 1 for ibm,spurr means support for the
version of the SPURR defined in version 2.05 of the architecture.

Add these properties for all processors for which the PURR and SPURR
registers are generated.

Signed-off-by: Suraj Jitindar Singh <sjitindarsingh@gmail.com>
---
 hw/ppc/spapr.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 2ef3ce4362..8580a8dc67 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -500,7 +500,10 @@ static void spapr_populate_cpu_dt(CPUState *cs, void *fdt, int offset,
     _FDT((fdt_setprop(fdt, offset, "64-bit", NULL, 0)));
 
     if (env->spr_cb[SPR_PURR].oea_read) {
-        _FDT((fdt_setprop(fdt, offset, "ibm,purr", NULL, 0)));
+        _FDT((fdt_setprop_cell(fdt, offset, "ibm,purr", 1)));
+    }
+    if (env->spr_cb[SPR_SPURR].oea_read) {
+        _FDT((fdt_setprop_cell(fdt, offset, "ibm,spurr", 1)));
     }
 
     if (ppc_hash64_has(cpu, PPC_HASH64_1TSEG)) {
-- 
2.13.6

Re: [Qemu-devel] [QEMU-PPC] [PATCH] target/ppc: Add ibm, purr and ibm, spurr device-tree properties

Posted by David Gibson 4 years, 11 months ago

On Mon, May 06, 2019 at 11:48:03AM +1000, Suraj Jitindar Singh wrote:
> The ibm,purr and ibm,spurr device tree properties are used to indicate
> that the processor implements the Processor Utilisation of Resources
> Register (PURR) and Scaled Processor Utilisation of Resources Registers
> (SPURR), respectively. Each property has a single value which represents
> the level of architecture supported. A value of 1 for ibm,purr means
> support for the version of the PURR defined in book 3 in version 2.02 of
> the architecture. A value of 1 for ibm,spurr means support for the
> version of the SPURR defined in version 2.05 of the architecture.
> 
> Add these properties for all processors for which the PURR and SPURR
> registers are generated.

So.. what does the current empty property mean?  Is it just wrong by
spec, or does it actually mean something incorrect?

> 
> Signed-off-by: Suraj Jitindar Singh <sjitindarsingh@gmail.com>
> ---
>  hw/ppc/spapr.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 2ef3ce4362..8580a8dc67 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -500,7 +500,10 @@ static void spapr_populate_cpu_dt(CPUState *cs, void *fdt, int offset,
>      _FDT((fdt_setprop(fdt, offset, "64-bit", NULL, 0)));
>  
>      if (env->spr_cb[SPR_PURR].oea_read) {
> -        _FDT((fdt_setprop(fdt, offset, "ibm,purr", NULL, 0)));
> +        _FDT((fdt_setprop_cell(fdt, offset, "ibm,purr", 1)));
> +    }
> +    if (env->spr_cb[SPR_SPURR].oea_read) {
> +        _FDT((fdt_setprop_cell(fdt, offset, "ibm,spurr", 1)));
>      }
>  
>      if (ppc_hash64_has(cpu, PPC_HASH64_1TSEG)) {

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

Re: [Qemu-devel] [QEMU-PPC] [PATCH] target/ppc: Add ibm, purr and ibm, spurr device-tree properties

Posted by Suraj Jitindar Singh 4 years, 11 months ago

On Mon, 2019-05-06 at 13:23 +1000, David Gibson wrote:
> On Mon, May 06, 2019 at 11:48:03AM +1000, Suraj Jitindar Singh wrote:
> > The ibm,purr and ibm,spurr device tree properties are used to
> > indicate
> > that the processor implements the Processor Utilisation of
> > Resources
> > Register (PURR) and Scaled Processor Utilisation of Resources
> > Registers
> > (SPURR), respectively. Each property has a single value which
> > represents
> > the level of architecture supported. A value of 1 for ibm,purr
> > means
> > support for the version of the PURR defined in book 3 in version
> > 2.02 of
> > the architecture. A value of 1 for ibm,spurr means support for the
> > version of the SPURR defined in version 2.05 of the architecture.
> > 
> > Add these properties for all processors for which the PURR and
> > SPURR
> > registers are generated.
> 
> So.. what does the current empty property mean?  Is it just wrong by
> spec, or does it actually mean something incorrect?

Af far as I can tell, an empty property is invalid according to PAPR.
A level value is required to communicate the level of purr implemented.

Should probably have:

Fixes: 0da6f3fef9a "spapr: Reorganize CPU dt generation code"

> 
> > 
> > Signed-off-by: Suraj Jitindar Singh <sjitindarsingh@gmail.com>
> > ---
> >  hw/ppc/spapr.c | 5 ++++-
> >  1 file changed, 4 insertions(+), 1 deletion(-)
> > 
> > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > index 2ef3ce4362..8580a8dc67 100644
> > --- a/hw/ppc/spapr.c
> > +++ b/hw/ppc/spapr.c
> > @@ -500,7 +500,10 @@ static void spapr_populate_cpu_dt(CPUState
> > *cs, void *fdt, int offset,
> >      _FDT((fdt_setprop(fdt, offset, "64-bit", NULL, 0)));
> >  
> >      if (env->spr_cb[SPR_PURR].oea_read) {
> > -        _FDT((fdt_setprop(fdt, offset, "ibm,purr", NULL, 0)));
> > +        _FDT((fdt_setprop_cell(fdt, offset, "ibm,purr", 1)));
> > +    }
> > +    if (env->spr_cb[SPR_SPURR].oea_read) {
> > +        _FDT((fdt_setprop_cell(fdt, offset, "ibm,spurr", 1)));
> >      }
> >  
> >      if (ppc_hash64_has(cpu, PPC_HASH64_1TSEG)) {
> 
>

Re: [Qemu-devel] [QEMU-PPC] [PATCH] target/ppc: Add ibm, purr and ibm, spurr device-tree properties

Posted by David Gibson 4 years, 11 months ago

On Tue, May 07, 2019 at 09:43:34AM +1000, Suraj Jitindar Singh wrote:
> On Mon, 2019-05-06 at 13:23 +1000, David Gibson wrote:
> > On Mon, May 06, 2019 at 11:48:03AM +1000, Suraj Jitindar Singh wrote:
> > > The ibm,purr and ibm,spurr device tree properties are used to
> > > indicate
> > > that the processor implements the Processor Utilisation of
> > > Resources
> > > Register (PURR) and Scaled Processor Utilisation of Resources
> > > Registers
> > > (SPURR), respectively. Each property has a single value which
> > > represents
> > > the level of architecture supported. A value of 1 for ibm,purr
> > > means
> > > support for the version of the PURR defined in book 3 in version
> > > 2.02 of
> > > the architecture. A value of 1 for ibm,spurr means support for the
> > > version of the SPURR defined in version 2.05 of the architecture.
> > > 
> > > Add these properties for all processors for which the PURR and
> > > SPURR
> > > registers are generated.
> > 
> > So.. what does the current empty property mean?  Is it just wrong by
> > spec, or does it actually mean something incorrect?
> 
> Af far as I can tell, an empty property is invalid according to PAPR.
> A level value is required to communicate the level of purr
> implemented.

Ok, makes sense.  Applied.

> 
> Should probably have:
> 
> Fixes: 0da6f3fef9a "spapr: Reorganize CPU dt generation code"

I've folded that in.

> 
> > 
> > > 
> > > Signed-off-by: Suraj Jitindar Singh <sjitindarsingh@gmail.com>
> > > ---
> > >  hw/ppc/spapr.c | 5 ++++-
> > >  1 file changed, 4 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > > index 2ef3ce4362..8580a8dc67 100644
> > > --- a/hw/ppc/spapr.c
> > > +++ b/hw/ppc/spapr.c
> > > @@ -500,7 +500,10 @@ static void spapr_populate_cpu_dt(CPUState
> > > *cs, void *fdt, int offset,
> > >      _FDT((fdt_setprop(fdt, offset, "64-bit", NULL, 0)));
> > >  
> > >      if (env->spr_cb[SPR_PURR].oea_read) {
> > > -        _FDT((fdt_setprop(fdt, offset, "ibm,purr", NULL, 0)));
> > > +        _FDT((fdt_setprop_cell(fdt, offset, "ibm,purr", 1)));
> > > +    }
> > > +    if (env->spr_cb[SPR_SPURR].oea_read) {
> > > +        _FDT((fdt_setprop_cell(fdt, offset, "ibm,spurr", 1)));
> > >      }
> > >  
> > >      if (ppc_hash64_has(cpu, PPC_HASH64_1TSEG)) {
> > 
> > 
> 

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson