* [PATCH] xen/arm: copy cpu clock-frequency to CPU DT node. @ 2013-11-01 20:58 Jon Fraser 2013-11-04 17:12 ` Ian Campbell 0 siblings, 1 reply; 9+ messages in thread From: Jon Fraser @ 2013-11-01 20:58 UTC (permalink / raw) To: xen-devel; +Cc: 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 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] xen/arm: copy cpu clock-frequency to CPU DT node. 2013-11-01 20:58 [PATCH] xen/arm: copy cpu clock-frequency to CPU DT node Jon Fraser @ 2013-11-04 17:12 ` Ian Campbell 2013-11-04 18:35 ` Jon Fraser 0 siblings, 1 reply; 9+ messages in thread From: Ian Campbell @ 2013-11-04 17:12 UTC (permalink / raw) To: Jon Fraser; +Cc: Julien Grall, xen-devel 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. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] xen/arm: copy cpu clock-frequency to CPU DT node. 2013-11-04 17:12 ` Ian Campbell @ 2013-11-04 18:35 ` Jon Fraser 2013-11-05 9:56 ` Ian Campbell 0 siblings, 1 reply; 9+ messages in thread From: Jon Fraser @ 2013-11-04 18:35 UTC (permalink / raw) To: Ian Campbell; +Cc: Julien Grall, jfraser, xen-devel 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. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] xen/arm: copy cpu clock-frequency to CPU DT node. 2013-11-04 18:35 ` Jon Fraser @ 2013-11-05 9:56 ` Ian Campbell 2013-11-05 10:04 ` Ian Campbell 0 siblings, 1 reply; 9+ messages in thread From: Ian Campbell @ 2013-11-05 9:56 UTC (permalink / raw) To: jfraser; +Cc: Julien Grall, xen-devel 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. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] xen/arm: copy cpu clock-frequency to CPU DT node. 2013-11-05 9:56 ` Ian Campbell @ 2013-11-05 10:04 ` Ian Campbell 2013-11-05 19:54 ` Jon Fraser 0 siblings, 1 reply; 9+ messages in thread From: Ian Campbell @ 2013-11-05 10:04 UTC (permalink / raw) To: jfraser; +Cc: Julien Grall, xen-devel 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. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] xen/arm: copy cpu clock-frequency to CPU DT node. 2013-11-05 10:04 ` Ian Campbell @ 2013-11-05 19:54 ` Jon Fraser 2013-11-06 10:28 ` Ian Campbell 0 siblings, 1 reply; 9+ messages in thread From: Jon Fraser @ 2013-11-05 19:54 UTC (permalink / raw) To: Ian Campbell; +Cc: Julien Grall, jfraser, xen-devel 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] xen/arm: copy cpu clock-frequency to CPU DT node. 2013-11-05 19:54 ` Jon Fraser @ 2013-11-06 10:28 ` Ian Campbell 2013-11-07 23:50 ` [PATCH V2] xen/arm: Device Tree cpu clock-frequency Jon Fraser 0 siblings, 1 reply; 9+ messages in thread From: Ian Campbell @ 2013-11-06 10:28 UTC (permalink / raw) To: jfraser; +Cc: Julien Grall, xen-devel 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. ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH V2] xen/arm: Device Tree cpu clock-frequency 2013-11-06 10:28 ` Ian Campbell @ 2013-11-07 23:50 ` Jon Fraser 2013-11-11 16:26 ` Ian Campbell 0 siblings, 1 reply; 9+ messages in thread From: Jon Fraser @ 2013-11-07 23:50 UTC (permalink / raw) To: xen-devel; +Cc: julien.grall, ian.campbell, 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 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH V2] xen/arm: Device Tree cpu clock-frequency 2013-11-07 23:50 ` [PATCH V2] xen/arm: Device Tree cpu clock-frequency Jon Fraser @ 2013-11-11 16:26 ` Ian Campbell 0 siblings, 0 replies; 9+ messages in thread From: Ian Campbell @ 2013-11-11 16:26 UTC (permalink / raw) To: Jon Fraser; +Cc: julien.grall, xen-devel 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2013-11-11 16:26 UTC | newest] Thread overview: 9+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2013-11-01 20:58 [PATCH] xen/arm: copy cpu clock-frequency to CPU DT node Jon Fraser 2013-11-04 17:12 ` Ian Campbell 2013-11-04 18:35 ` Jon Fraser 2013-11-05 9:56 ` Ian Campbell 2013-11-05 10:04 ` Ian Campbell 2013-11-05 19:54 ` Jon Fraser 2013-11-06 10:28 ` Ian Campbell 2013-11-07 23:50 ` [PATCH V2] xen/arm: Device Tree cpu clock-frequency Jon Fraser 2013-11-11 16:26 ` Ian Campbell
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).