| Message ID | 20180410080119.18407-1-christian.gmeiner@gmail.com |
|---|---|
| State | Superseded |
| Delegated to: | Bin Meng |
| Headers | show |
| Series | [U-Boot] x86: queensbay: switche P state to the highest freq | expand |
Hi Christian, On Tue, Apr 10, 2018 at 4:01 PM, Christian Gmeiner <christian.gmeiner@gmail.com> wrote: > Fixes performance issue when running vx5/vx7 images. Could you elaborate more on what performance issue you are seeing? nits: better to spell out "VxWorks" > > Signed-off-by: Christian Gmeiner <christian.gmeiner@gmail.com> > --- > 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? > + * > + * SPDX-License-Identifier: GPL-2.0+ > + */ > + > +#include <common.h> > +#include <cpu.h> > +#include <dm.h> > +#include <asm/cpu.h> > +#include <asm/cpu_x86.h> > +#include <asm/msr.h> > + > +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 > + msr_write(MSR_IA32_MISC_ENABLES, msr); > + > + msr = msr_read(MSR_IA32_PERF_CTL); > + msr.lo = 0x101f; ditto > + 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" > + > + /* 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. > + > +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" > + { } > +}; > + > +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
Hi Bin Thanks for you quick review! > > On Tue, Apr 10, 2018 at 4:01 PM, Christian Gmeiner > <christian.gmeiner@gmail.com> 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 > nits: better to spell out "VxWorks" ok > >> >> Signed-off-by: Christian Gmeiner <christian.gmeiner@gmail.com> >> --- >> 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 <common.h> >> +#include <cpu.h> >> +#include <dm.h> >> +#include <asm/cpu.h> >> +#include <asm/cpu_x86.h> >> +#include <asm/msr.h> >> + >> +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. >> + >> +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, >> +}; >> --
Hi Christian, On Tue, Apr 10, 2018 at 4:47 PM, Christian Gmeiner <christian.gmeiner@gmail.com> wrote: > Hi Bin > > Thanks for you quick review! > >> >> On Tue, Apr 10, 2018 at 4:01 PM, Christian Gmeiner >> <christian.gmeiner@gmail.com> 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 <christian.gmeiner@gmail.com> >>> --- >>> 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 <common.h> >>> +#include <cpu.h> >>> +#include <dm.h> >>> +#include <asm/cpu.h> >>> +#include <asm/cpu_x86.h> >>> +#include <asm/msr.h> >>> + >>> +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
Hi Bin, 2018-04-10 11:07 GMT+02:00 Bin Meng <bmeng.cn@gmail.com>: > Hi Christian, > > On Tue, Apr 10, 2018 at 4:47 PM, Christian Gmeiner > <christian.gmeiner@gmail.com> wrote: >> Hi Bin >> >> Thanks for you quick review! >> >>> >>> On Tue, Apr 10, 2018 at 4:01 PM, Christian Gmeiner >>> <christian.gmeiner@gmail.com> 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. > Will do. >> >>> nits: better to spell out "VxWorks" >> >> ok >> >>> >>>> >>>> Signed-off-by: Christian Gmeiner <christian.gmeiner@gmail.com> >>>> --- >>>> 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 <common.h> >>>> +#include <cpu.h> >>>> +#include <dm.h> >>>> +#include <asm/cpu.h> >>>> +#include <asm/cpu_x86.h> >>>> +#include <asm/msr.h> >>>> + >>>> +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. > The dts is not in its ideal state I would say but that is fixable. I hope to send out patches for this board soon. >>>> + >>>> +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, >>>> +}; >>>> --
Hi Christian, On Tue, Apr 10, 2018 at 5:09 PM, Christian Gmeiner <christian.gmeiner@gmail.com> wrote: > Hi Bin, > > 2018-04-10 11:07 GMT+02:00 Bin Meng <bmeng.cn@gmail.com>: >> Hi Christian, >> >> On Tue, Apr 10, 2018 at 4:47 PM, Christian Gmeiner >> <christian.gmeiner@gmail.com> wrote: >>> Hi Bin >>> >>> Thanks for you quick review! >>> >>>> >>>> On Tue, Apr 10, 2018 at 4:01 PM, Christian Gmeiner >>>> <christian.gmeiner@gmail.com> 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. >> > > Will do. > >>> >>>> nits: better to spell out "VxWorks" >>> >>> ok >>> >>>> >>>>> >>>>> Signed-off-by: Christian Gmeiner <christian.gmeiner@gmail.com> >>>>> --- >>>>> 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 <common.h> >>>>> +#include <cpu.h> >>>>> +#include <dm.h> >>>>> +#include <asm/cpu.h> >>>>> +#include <asm/cpu_x86.h> >>>>> +#include <asm/msr.h> >>>>> + >>>>> +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. >> > > The dts is not in its ideal state I would say but that is fixable. I > hope to send > out patches for this board soon. > Great. Looking forward to your board support patch. Regards, Bin
2018-04-10 11:11 GMT+02:00 Bin Meng <bmeng.cn@gmail.com>: > Hi Christian, > > On Tue, Apr 10, 2018 at 5:09 PM, Christian Gmeiner > <christian.gmeiner@gmail.com> wrote: >> Hi Bin, >> >> 2018-04-10 11:07 GMT+02:00 Bin Meng <bmeng.cn@gmail.com>: >>> Hi Christian, >>> >>> On Tue, Apr 10, 2018 at 4:47 PM, Christian Gmeiner >>> <christian.gmeiner@gmail.com> wrote: >>>> Hi Bin >>>> >>>> Thanks for you quick review! >>>> >>>>> >>>>> On Tue, Apr 10, 2018 at 4:01 PM, Christian Gmeiner >>>>> <christian.gmeiner@gmail.com> 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. >>> >> >> Will do. >> >>>> >>>>> nits: better to spell out "VxWorks" >>>> >>>> ok >>>> >>>>> >>>>>> >>>>>> Signed-off-by: Christian Gmeiner <christian.gmeiner@gmail.com> >>>>>> --- >>>>>> 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 <common.h> >>>>>> +#include <cpu.h> >>>>>> +#include <dm.h> >>>>>> +#include <asm/cpu.h> >>>>>> +#include <asm/cpu_x86.h> >>>>>> +#include <asm/msr.h> >>>>>> + >>>>>> +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 I *think* I have read this msr running under a 'good' bios so I am not really sure what the meaning of the bits are. I tried a quick google and did not found any E6xx datasheet containing the description. Is it okay to just use the magic value here?
Hi Christian, On Tue, Apr 10, 2018 at 5:48 PM, Christian Gmeiner <christian.gmeiner@gmail.com> wrote: > 2018-04-10 11:11 GMT+02:00 Bin Meng <bmeng.cn@gmail.com>: >> Hi Christian, >> >> On Tue, Apr 10, 2018 at 5:09 PM, Christian Gmeiner >> <christian.gmeiner@gmail.com> wrote: >>> Hi Bin, >>> >>> 2018-04-10 11:07 GMT+02:00 Bin Meng <bmeng.cn@gmail.com>: >>>> Hi Christian, >>>> >>>> On Tue, Apr 10, 2018 at 4:47 PM, Christian Gmeiner >>>> <christian.gmeiner@gmail.com> wrote: >>>>> Hi Bin >>>>> >>>>> Thanks for you quick review! >>>>> >>>>>> >>>>>> On Tue, Apr 10, 2018 at 4:01 PM, Christian Gmeiner >>>>>> <christian.gmeiner@gmail.com> 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. >>>> >>> >>> Will do. >>> >>>>> >>>>>> nits: better to spell out "VxWorks" >>>>> >>>>> ok >>>>> >>>>>> >>>>>>> >>>>>>> Signed-off-by: Christian Gmeiner <christian.gmeiner@gmail.com> >>>>>>> --- >>>>>>> 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 <common.h> >>>>>>> +#include <cpu.h> >>>>>>> +#include <dm.h> >>>>>>> +#include <asm/cpu.h> >>>>>>> +#include <asm/cpu_x86.h> >>>>>>> +#include <asm/msr.h> >>>>>>> + >>>>>>> +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 > > I *think* I have read this msr running under a 'good' bios so I am not > really sure > what the meaning of the bits are. I tried a quick google and did not found any > E6xx datasheet containing the description. Is it okay to just use the > magic value here? > I will see if I can find anything. Regards, Bin
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 + * + * SPDX-License-Identifier: GPL-2.0+ + */ + +#include <common.h> +#include <cpu.h> +#include <dm.h> +#include <asm/cpu.h> +#include <asm/cpu_x86.h> +#include <asm/msr.h> + +static void set_max_freq(void) +{ + msr_t msr; + + msr = msr_read(MSR_IA32_MISC_ENABLES); + msr.lo |= (1 << 16); + msr_write(MSR_IA32_MISC_ENABLES, msr); + + msr = msr_read(MSR_IA32_PERF_CTL); + msr.lo = 0x101f; + msr_write(MSR_IA32_PERF_CTL, msr); +} + +static int cpu_x86_queensbay_probe(struct udevice *dev) +{ + if (!ll_boot_init()) + return 0; + debug("Init Queensbay core\n"); + + /* Set core to max frequency ratio */ + set_max_freq(); + + return 0; +} + +static int queensbay_get_count(struct udevice *dev) +{ + return 2; +} + +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" }, + { } +}; + +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, +};
Fixes performance issue when running vx5/vx7 images. Signed-off-by: Christian Gmeiner <christian.gmeiner@gmail.com> --- 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