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

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

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.

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.

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.

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.

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

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.

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

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