[PATCH] xen/arm: copy cpu clock-frequency to CPU DT node.

[PATCH] xen/arm: copy cpu clock-frequency to CPU DT node.

[Jon Fraser]

When creating the CPU DT node, copy the clock-frequency if present.

Signed-off-by: Jon Fraser <jfraser@broadcom.com>
---
 xen/arch/arm/domain_build.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index c644be2..b212627 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -391,6 +391,7 @@ static int make_cpus_node(const struct domain *d, void *fdt,
     u32 len;
     /* Placeholder for cpu@ + a 32-bit number + \0 */
     char buf[15];
+    u32 *clock_frequency;
 
     DPRINT("Create cpus node\n");
 
@@ -411,6 +412,7 @@ static int make_cpus_node(const struct domain *d, void *fdt,
         if ( dt_device_type_is_equal(npcpu, "cpu") )
         {
             compatible = dt_get_property(npcpu, "compatible", &len);
+            clock_frequency = (u32 *)dt_get_property(npcpu, "clock-frequency", NULL);
             break;
         }
     }
@@ -457,6 +459,12 @@ static int make_cpus_node(const struct domain *d, void *fdt,
         if ( res )
             return res;
 
+        if (clock_frequency) {
+            res = fdt_property_cell(fdt, "clock-frequency", *(u32 *)clock_frequency);
+            if ( res )
+                return res;
+        }
+
         res = fdt_end_node(fdt);
         if ( res )
             return res;
-- 
1.7.11.3

Re: [PATCH] xen/arm: copy cpu clock-frequency to CPU DT node.

[Ian Campbell]

On Fri, 2013-11-01 at 16:58 -0400, Jon Fraser wrote:
> When creating the CPU DT node, copy the clock-frequency if present.
> 
> Signed-off-by: Jon Fraser <jfraser@broadcom.com>
> ---
>  xen/arch/arm/domain_build.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index c644be2..b212627 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -391,6 +391,7 @@ static int make_cpus_node(const struct domain *d, void *fdt,
>      u32 len;
>      /* Placeholder for cpu@ + a 32-bit number + \0 */
>      char buf[15];
> +    u32 *clock_frequency;
>  
>      DPRINT("Create cpus node\n");
>  
> @@ -411,6 +412,7 @@ static int make_cpus_node(const struct domain *d, void *fdt,
>          if ( dt_device_type_is_equal(npcpu, "cpu") )
>          {
>              compatible = dt_get_property(npcpu, "compatible", &len);
> +            clock_frequency = (u32 *)dt_get_property(npcpu, "clock-frequency", NULL);

Julien's the expert but I think you need to use dt_property_read_u32
here, to get the correct endianness conversion (as well as for pure
forms sake of using the correct API for the job).

>              break;
>          }
>      }
> @@ -457,6 +459,12 @@ static int make_cpus_node(const struct domain *d, void *fdt,
>          if ( res )
>              return res;
>  
> +        if (clock_frequency) {
> +            res = fdt_property_cell(fdt, "clock-frequency", *(u32 *)clock_frequency);

I suppose there ought to be some API for this side of things too, but I
can't see it right now...

Note that fdt_property_cell contains a cpu_to_fdt32 so it is converting
while the read of the property not, so I think the code is broken as is?

Ian.

Re: [PATCH] xen/arm: copy cpu clock-frequency to CPU DT node.

[Jon Fraser]

On Mon, 2013-11-04 at 17:12 +0000, Ian Campbell wrote:
> On Fri, 2013-11-01 at 16:58 -0400, Jon Fraser wrote:
> > When creating the CPU DT node, copy the clock-frequency if present.
> > 
...
> Julien's the expert but I think you need to use dt_property_read_u32
> here, to get the correct endianness conversion (as well as for pure
> forms sake of using the correct API for the job).
> 
I'll fix that.

> >              break;
> >          }
> >      }
> > @@ -457,6 +459,12 @@ static int make_cpus_node(const struct domain *d, void *fdt,
> >          if ( res )
> >              return res;
> >  
> > +        if (clock_frequency) {
> > +            res = fdt_property_cell(fdt, "clock-frequency", *(u32 *)clock_frequency);
> 
> I suppose there ought to be some API for this side of things too, but I
> can't see it right now...
> 
> Note that fdt_property_cell contains a cpu_to_fdt32 so it is converting
> while the read of the property not, so I think the code is broken as is?

Yesss, it is broken.  When I checked the property in /proc/device-tree,
I failed to realize it was endian swapped.

Re: [PATCH] xen/arm: copy cpu clock-frequency to CPU DT node.

[Ian Campbell]

On Mon, 2013-11-04 at 13:35 -0500, Jon Fraser wrote:
> On Mon, 2013-11-04 at 17:12 +0000, Ian Campbell wrote:
> > On Fri, 2013-11-01 at 16:58 -0400, Jon Fraser wrote:
> > > When creating the CPU DT node, copy the clock-frequency if present.
> > > 
> ...
> > Julien's the expert but I think you need to use dt_property_read_u32
> > here, to get the correct endianness conversion (as well as for pure
> > forms sake of using the correct API for the job).
> > 
> I'll fix that.
> 
> > >              break;
> > >          }
> > >      }
> > > @@ -457,6 +459,12 @@ static int make_cpus_node(const struct domain *d, void *fdt,
> > >          if ( res )
> > >              return res;
> > >  
> > > +        if (clock_frequency) {
> > > +            res = fdt_property_cell(fdt, "clock-frequency", *(u32 *)clock_frequency);
> > 
> > I suppose there ought to be some API for this side of things too, but I
> > can't see it right now...
> > 
> > Note that fdt_property_cell contains a cpu_to_fdt32 so it is converting
> > while the read of the property not, so I think the code is broken as is?
> 
> Yesss, it is broken.  When I checked the property in /proc/device-tree,
> I failed to realize it was endian swapped.

I guess nothing much critical is relying on this value. What is it
supposed to be used for?
linux/Documentation/devicetree/booting-without-of.txt seems to imply it
is mostly optional for non-PPC.

Ian.

Re: [PATCH] xen/arm: copy cpu clock-frequency to CPU DT node.

[Ian Campbell]

On Tue, 2013-11-05 at 09:56 +0000, Ian Campbell wrote:
> On Mon, 2013-11-04 at 13:35 -0500, Jon Fraser wrote:
> > On Mon, 2013-11-04 at 17:12 +0000, Ian Campbell wrote:
> > > On Fri, 2013-11-01 at 16:58 -0400, Jon Fraser wrote:
> > > > When creating the CPU DT node, copy the clock-frequency if present.
> > > > 
> > ...
> > > Julien's the expert but I think you need to use dt_property_read_u32
> > > here, to get the correct endianness conversion (as well as for pure
> > > forms sake of using the correct API for the job).
> > > 
> > I'll fix that.
> > 
> > > >              break;
> > > >          }
> > > >      }
> > > > @@ -457,6 +459,12 @@ static int make_cpus_node(const struct domain *d, void *fdt,
> > > >          if ( res )
> > > >              return res;
> > > >  
> > > > +        if (clock_frequency) {
> > > > +            res = fdt_property_cell(fdt, "clock-frequency", *(u32 *)clock_frequency);
> > > 
> > > I suppose there ought to be some API for this side of things too, but I
> > > can't see it right now...
> > > 
> > > Note that fdt_property_cell contains a cpu_to_fdt32 so it is converting
> > > while the read of the property not, so I think the code is broken as is?
> > 
> > Yesss, it is broken.  When I checked the property in /proc/device-tree,
> > I failed to realize it was endian swapped.
> 
> I guess nothing much critical is relying on this value. What is it
> supposed to be used for?
> linux/Documentation/devicetree/booting-without-of.txt seems to imply it
> is mostly optional for non-PPC.

BTW, I'm asking because I'm not sure if exposing the underlying clock
speed to the guest VCPU is the right thing to do. It can vary across
pCPUs (e.g. big.LITTLE) and with power control etc.

Ian.

Re: [PATCH] xen/arm: copy cpu clock-frequency to CPU DT node.

[Jon Fraser]

On Tue, 2013-11-05 at 10:04 +0000, Ian Campbell wrote:
> On Tue, 2013-11-05 at 09:56 +0000, Ian Campbell wrote:
> > On Mon, 2013-11-04 at 13:35 -0500, Jon Fraser wrote:
> > > On Mon, 2013-11-04 at 17:12 +0000, Ian Campbell wrote:
> > > > On Fri, 2013-11-01 at 16:58 -0400, Jon Fraser wrote:
> > > > > When creating the CPU DT node, copy the clock-frequency if present.
> > > > > 
> > > ...
> > > > Julien's the expert but I think you need to use dt_property_read_u32
> > > > here, to get the correct endianness conversion (as well as for pure
> > > > forms sake of using the correct API for the job).
> > > > 
> > > I'll fix that.
> > > 
> > > > >              break;
> > > > >          }
> > > > >      }
> > > > > @@ -457,6 +459,12 @@ static int make_cpus_node(const struct domain *d, void *fdt,
> > > > >          if ( res )
> > > > >              return res;
> > > > >  
> > > > > +        if (clock_frequency) {
> > > > > +            res = fdt_property_cell(fdt, "clock-frequency", *(u32 *)clock_frequency);
> > > > 
> > > > I suppose there ought to be some API for this side of things too, but I
> > > > can't see it right now...
> > > > 
> > > > Note that fdt_property_cell contains a cpu_to_fdt32 so it is converting
> > > > while the read of the property not, so I think the code is broken as is?
> > > 
> > > Yesss, it is broken.  When I checked the property in /proc/device-tree,
> > > I failed to realize it was endian swapped.
> > 
> > I guess nothing much critical is relying on this value. What is it
> > supposed to be used for?
> > linux/Documentation/devicetree/booting-without-of.txt seems to imply it
> > is mostly optional for non-PPC.
> 
> BTW, I'm asking because I'm not sure if exposing the underlying clock
> speed to the guest VCPU is the right thing to do. It can vary across
> pCPUs (e.g. big.LITTLE) and with power control etc.
> 
> Ian.

As far as I can tell, it is intended to be used for load balancing
for the case of differing cpu capabilities.   Looking down the road,
if I was doing Power Management in Dom0, I would want to know
the cpu topology and cpu speeds, but that info is probably available by
other means.

I just wanted to squelch the annoying linux kernel error messages.

/cpus/cpu@0 missing clock-frequency property
/cpus/cpu@1 missing clock-frequency property
/cpus/cpu@2 missing clock-frequency property
/cpus/cpu@3 missing clock-frequency property

Do you foresee, at this point, any problem with giving all the guest
VCPUs the same clock frequency setting?

Jon

Re: [PATCH] xen/arm: copy cpu clock-frequency to CPU DT node.

[Ian Campbell]

On Tue, 2013-11-05 at 14:54 -0500, Jon Fraser wrote:
> On Tue, 2013-11-05 at 10:04 +0000, Ian Campbell wrote:
> > On Tue, 2013-11-05 at 09:56 +0000, Ian Campbell wrote:
> > > On Mon, 2013-11-04 at 13:35 -0500, Jon Fraser wrote:
> > > > On Mon, 2013-11-04 at 17:12 +0000, Ian Campbell wrote:
> > > > > On Fri, 2013-11-01 at 16:58 -0400, Jon Fraser wrote:
> > > > > > When creating the CPU DT node, copy the clock-frequency if present.
> > > > > > 
> > > > ...
> > > > > Julien's the expert but I think you need to use dt_property_read_u32
> > > > > here, to get the correct endianness conversion (as well as for pure
> > > > > forms sake of using the correct API for the job).
> > > > > 
> > > > I'll fix that.
> > > > 
> > > > > >              break;
> > > > > >          }
> > > > > >      }
> > > > > > @@ -457,6 +459,12 @@ static int make_cpus_node(const struct domain *d, void *fdt,
> > > > > >          if ( res )
> > > > > >              return res;
> > > > > >  
> > > > > > +        if (clock_frequency) {
> > > > > > +            res = fdt_property_cell(fdt, "clock-frequency", *(u32 *)clock_frequency);
> > > > > 
> > > > > I suppose there ought to be some API for this side of things too, but I
> > > > > can't see it right now...
> > > > > 
> > > > > Note that fdt_property_cell contains a cpu_to_fdt32 so it is converting
> > > > > while the read of the property not, so I think the code is broken as is?
> > > > 
> > > > Yesss, it is broken.  When I checked the property in /proc/device-tree,
> > > > I failed to realize it was endian swapped.
> > > 
> > > I guess nothing much critical is relying on this value. What is it
> > > supposed to be used for?
> > > linux/Documentation/devicetree/booting-without-of.txt seems to imply it
> > > is mostly optional for non-PPC.
> > 
> > BTW, I'm asking because I'm not sure if exposing the underlying clock
> > speed to the guest VCPU is the right thing to do. It can vary across
> > pCPUs (e.g. big.LITTLE) and with power control etc.
> > 
> > Ian.
> 
> As far as I can tell, it is intended to be used for load balancing
> for the case of differing cpu capabilities.   Looking down the road,
> if I was doing Power Management in Dom0, I would want to know
> the cpu topology and cpu speeds, but that info is probably available by
> other means.
> 
> I just wanted to squelch the annoying linux kernel error messages.
> 
> /cpus/cpu@0 missing clock-frequency property
> /cpus/cpu@1 missing clock-frequency property
> /cpus/cpu@2 missing clock-frequency property
> /cpus/cpu@3 missing clock-frequency property

Ah, I started noticing them too recently but didn't connect them with
this patch.

> Do you foresee, at this point, any problem with giving all the guest
> VCPUs the same clock frequency setting?

Do you mean same as in give all VCPUs a static hardcoded frequency or
what you did here passing through something real?

I think in general I would prefer to passthrough something real as you
have done. I'm always a little bit wary of accidentally creating an ABI
which we end up forced to support. Adding a hardcoded value could be
seen to do that, whereas passing through is going to naturally vary. One
would hope noone would ever rely on a hardcoded clock-frequency anyway.

I'm not sure what to do about domU, since I don't think the physical CPU
frequency is necessarily easily available to the toolstack. We have a
similar issue with the CPU compatiblity string. I guess that all needs
to be plumbed up. :-( (not your problem here though, so don't worry!)

Ian.

[PATCH V2] xen/arm: Device Tree cpu clock-frequency

[Jon Fraser]

When creating CPU device tree properties, copy the
clock-frequency if present.

Quiets annoying messages from linux kernel:
"/cpus/cpu@0 missing clock-frequency property"

Signed-off-by: Jon Fraser <jfraser@broadcom.com>
---
 xen/arch/arm/domain_build.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index c644be2..186746c 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -391,6 +391,8 @@ static int make_cpus_node(const struct domain *d, void *fdt,
     u32 len;
     /* Placeholder for cpu@ + a 32-bit number + \0 */
     char buf[15];
+    u32 clock_frequency;
+    bool_t clock_valid;
 
     DPRINT("Create cpus node\n");
 
@@ -411,6 +413,8 @@ static int make_cpus_node(const struct domain *d, void *fdt,
         if ( dt_device_type_is_equal(npcpu, "cpu") )
         {
             compatible = dt_get_property(npcpu, "compatible", &len);
+            clock_valid = dt_property_read_u32(npcpu, "clock-frequency",
+                                            &clock_frequency);
             break;
         }
     }
@@ -457,6 +461,12 @@ static int make_cpus_node(const struct domain *d, void *fdt,
         if ( res )
             return res;
 
+        if (clock_valid) {
+            res = fdt_property_cell(fdt, "clock-frequency", clock_frequency);
+            if ( res )
+                return res;
+        }
+
         res = fdt_end_node(fdt);
         if ( res )
             return res;
-- 
1.7.11.3

Re: [PATCH V2] xen/arm: Device Tree cpu clock-frequency

[Ian Campbell]

On Thu, 2013-11-07 at 18:50 -0500, Jon Fraser wrote:
> When creating CPU device tree properties, copy the
> clock-frequency if present.
> 
> Quiets annoying messages from linux kernel:
> "/cpus/cpu@0 missing clock-frequency property"
> 
> Signed-off-by: Jon Fraser <jfraser@broadcom.com>

Acked-by: Ian Campbell <ian.campbell@citrix.com>

+ applied, thanks