From mboxrd@z Thu Jan 1 00:00:00 1970 From: Bin Meng Date: Tue, 10 Apr 2018 17:07:33 +0800 Subject: [U-Boot] [PATCH] x86: queensbay: switche P state to the highest freq In-Reply-To: References: <20180410080119.18407-1-christian.gmeiner@gmail.com> Message-ID: List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de Hi Christian, On Tue, Apr 10, 2018 at 4:47 PM, Christian Gmeiner wrote: > Hi Bin > > Thanks for you quick review! > >> >> On Tue, Apr 10, 2018 at 4:01 PM, Christian Gmeiner >> wrote: >>> Fixes performance issue when running vx5/vx7 images. >> >> Could you elaborate more on what performance issue you are seeing? >> > > The cache performance was bad without this change. We found this problem > when switching from bldk to u-boot. > > old: https://imagebin.ca/v/3xssw7stbY6A > new: https://imagebin.ca/v/3xstHMCC9fmJ Thank you for the benchmark data. Then please consider describing your benchmark test suite and on what configuration the data is worse without this patch (eg: sequential 128-bit read) in the commit message. This will help other people understand the patch and the commit in the future. > >> nits: better to spell out "VxWorks" > > ok > >> >>> >>> Signed-off-by: Christian Gmeiner >>> --- >>> arch/x86/cpu/queensbay/Makefile | 2 +- >>> arch/x86/cpu/queensbay/cpu.c | 61 +++++++++++++++++++++++++++++++++++++++++ >>> 2 files changed, 62 insertions(+), 1 deletion(-) >>> create mode 100644 arch/x86/cpu/queensbay/cpu.c >>> >>> diff --git a/arch/x86/cpu/queensbay/Makefile b/arch/x86/cpu/queensbay/Makefile >>> index c0681995bd..3dd23465d4 100644 >>> --- a/arch/x86/cpu/queensbay/Makefile >>> +++ b/arch/x86/cpu/queensbay/Makefile >>> @@ -5,4 +5,4 @@ >>> # >>> >>> obj-y += fsp_configs.o irq.o >>> -obj-y += tnc.o >>> +obj-y += tnc.o cpu.o >>> diff --git a/arch/x86/cpu/queensbay/cpu.c b/arch/x86/cpu/queensbay/cpu.c >>> new file mode 100644 >>> index 0000000000..e98e4ac8ef >>> --- /dev/null >>> +++ b/arch/x86/cpu/queensbay/cpu.c >>> @@ -0,0 +1,61 @@ >>> +/* >>> + * Copyright (C) 2016, Bachmann electronic GmbH >> >> nits: 2018? >> > > This change was made some months ago :) But yeah.. can change it. > >>> + * >>> + * SPDX-License-Identifier: GPL-2.0+ >>> + */ >>> + >>> +#include >>> +#include >>> +#include >>> +#include >>> +#include >>> +#include >>> + >>> +static void set_max_freq(void) >>> +{ >>> + msr_t msr; >>> + >>> + msr = msr_read(MSR_IA32_MISC_ENABLES); >>> + msr.lo |= (1 << 16); >> >> Please use macro instead of magic numbers > > ok > >> >>> + msr_write(MSR_IA32_MISC_ENABLES, msr); >>> + >>> + msr = msr_read(MSR_IA32_PERF_CTL); >>> + msr.lo = 0x101f; >> >> ditto > > ok > >> >>> + msr_write(MSR_IA32_PERF_CTL, msr); >>> +} >>> + >>> +static int cpu_x86_queensbay_probe(struct udevice *dev) >> >> Queensbay is the platform codename, not the core codename. Queensbay = >> TunnelCreek (the processor) + Topcliff (the bridge chipset). >> >>> +{ >>> + if (!ll_boot_init()) >>> + return 0; >>> + debug("Init Queensbay core\n"); >> >> Should be "TunnelCreek" >> > > ok > >>> + >>> + /* Set core to max frequency ratio */ >>> + set_max_freq(); >>> + >>> + return 0; >>> +} >>> + >>> +static int queensbay_get_count(struct udevice *dev) >>> +{ >>> + return 2; >>> +} >> >> Please use cpu_x86_get_count() instead of hard code. >> > > That one uses the dts as basis for this information and I need to > add two cpu nodes. I also thought about using cpuid for this. Not > sure what is better. > I don't understand. Isn't your board U-Boot ported from Intel Crown Bay? There is arch/x86/dts/crownbay.dts already. You don't need do any mods in the dts. >>> + >>> +static const struct cpu_ops cpu_x86_queensbay_ops = { >>> + .get_desc = cpu_x86_get_desc, >>> + .get_count = queensbay_get_count, >>> +}; >>> + >>> +static const struct udevice_id cpu_x86_queensbay_ids[] = { >>> + { .compatible = "intel,queensbay-cpu" }, >> >> This should be "intel,tunnelcreek-cpu" > > ok > >> >>> + { } >>> +}; >>> + >>> +U_BOOT_DRIVER(cpu_x86_queensbay_drv) = { >>> + .name = "cpu_x86_queensbay", >>> + .id = UCLASS_CPU, >>> + .of_match = cpu_x86_queensbay_ids, >>> + .bind = cpu_x86_bind, >>> + .probe = cpu_x86_queensbay_probe, >>> + .ops = &cpu_x86_queensbay_ops, >>> +}; >>> -- Regards, Bin