From mboxrd@z Thu Jan  1 00:00:00 1970
From: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Subject: Re: [PATCH] ACPI: Add fixups for AMD P-state figures.
Date: Wed, 6 Mar 2013 10:53:07 -0500
Message-ID: <20130306155307.GB13118@phenom.dumpdata.com>
References: <1362512728-28770-1-git-send-email-konrad.wilk@oracle.com>
	<51365401.4050205@oracle.com>
	<20130305213319.GA8235@phenom.dumpdata.com>
	<5137150502000078000C375D@nat28.tlf.novell.com>
Mime-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
Return-path: <xen-devel-bounces@lists.xen.org>
Content-Disposition: inline
In-Reply-To: <5137150502000078000C375D@nat28.tlf.novell.com>
List-Unsubscribe: <http://lists.xen.org/cgi-bin/mailman/options/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=unsubscribe>
List-Post: <mailto:xen-devel@lists.xen.org>
List-Help: <mailto:xen-devel-request@lists.xen.org?subject=help>
List-Subscribe: <http://lists.xen.org/cgi-bin/mailman/listinfo/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=subscribe>
Sender: xen-devel-bounces@lists.xen.org
Errors-To: xen-devel-bounces@lists.xen.org
To: Jan Beulich <JBeulich@suse.com>
Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com>, xen-devel@lists.xensource.com, bp@suse.de, stefan.bader@canonical.com
List-Id: xen-devel@lists.xenproject.org

On Wed, Mar 06, 2013 at 09:05:57AM +0000, Jan Beulich wrote:
> >>> On 05.03.13 at 22:33, Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> wrote:
> > +static void amd_fixup_frequency(struct xen_processor_px *px)
> > +{
> > +	u32 hi, lo, fid, did;
> > +	int index = px->control & 0x00000007;
> > +
> > +	if (boot_cpu_data.x86_vendor != X86_VENDOR_AMD)
> > +		return;
> 
> This is pointless, the driver as a whole is already AMD specific.
> 
> > +
> > +	if ((boot_cpu_data.x86 == 0x10 && boot_cpu_data.x86_model < 10)
> > +	    || boot_cpu_data.x86 == 0x11) {
> 
> Instead I wonder whether this could (properly inverted) serve as
> an early return condition, reducing indentation on the subsequent
> block.
> 
> > +		rdmsr(MSR_PSTATE_DEF_BASE + index, lo, hi);
> > +        /*
> > +         * MSR C001_0064+:
> > +         * Bit 63: PstateEn. Read-write. If set, the P-state is valid.
> > +         */
> > +        if (!(hi & (1UL << 31)))
> > +            return;
> > +
> > +        fid = lo & 0x3f;
> > +        did = (lo >> 6) & 7;
> > +        if (boot_cpu_data.x86 == 0x10)
> > +            px->core_frequency = (100 * (fid + 0x10)) >> did;
> > +        else
> > +            px->core_frequency = (100 * (fid + 8)) >> did;
> 
> 0x10 vs 8? Please settle on decimal (preferred) or hex numbers in
> a calculation like this.
> 
> > +	}
> > +}
> 
> And as Boris already pointed out - indentation here should be
> consistent in itself _and_ with the rest of the file.

I not sure if it is my editor - but under vim it looks fine. It is just
when I send it and look under 'mutt' then I see it.

Either way, let me make the changes you suggested and send out
a revised patch shortly.

> 
> > +
> > +static void amd_fixup_freq(struct processor_performance *perf)
> > +{
> > +
> > +    int i;
> 
> unsigned int
> 
> Jan
> 
> > +
> > +    for (i = 0; i < perf->state_count; i++)
> > +        amd_fixup_frequency(&perf->states[i]);
> > +
> > +}
> >  static int powernow_cpufreq_verify(struct cpufreq_policy *policy)
> >  {
> >      struct acpi_cpufreq_data *data;
> 
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
>