From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ian Campbell Subject: Re: [PATCH] xen/arm: copy cpu clock-frequency to CPU DT node. Date: Wed, 6 Nov 2013 10:28:49 +0000 Message-ID: <1383733729.26213.50.camel@kazak.uk.xensource.com> References: <1383339515-10472-1-git-send-email-jfraser@broadcom.com> <1383585129.8826.132.camel@kazak.uk.xensource.com> <1383590120.2465.121.camel@chaos.and.broadcom.com> <1383645387.13961.23.camel@kazak.uk.xensource.com> <1383645884.13961.28.camel@kazak.uk.xensource.com> <1383681272.2465.184.camel@chaos.and.broadcom.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1383681272.2465.184.camel@chaos.and.broadcom.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: jfraser@broadcom.com Cc: Julien Grall , xen-devel@lists.xen.org List-Id: xen-devel@lists.xenproject.org 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.