| Message ID | 20230716110038.16200-1-dungeonlords789@naver.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [1/1] arch/Config.in.x86: add MMX instruction set capability for Geode CPU | expand |
Hello Andrei, Thanks for your patch! See some comments below. On Sun, 16 Jul 2023 20:00:38 +0900 Cherniaev Andrei <dungeonlords789@naver.com> wrote: > From: AndreiCherniaev <dungeonlords789@naver.com> > > Currently, there is no MMX support for modern GEODE processors in Buildroot. > Yet, some packages like ffmpeg can leverage it when the package is built. > > Signed-off-by: Cherniaev Andrei <dungeonlords789@naver.com> Could you please adjust your git configuration so that the From: field value and the Signed-off-by: field value are matching? > --- > arch/Config.in.x86 | 20 +++++++++++++++----- > 1 file changed, 15 insertions(+), 5 deletions(-) > > diff --git a/arch/Config.in.x86 b/arch/Config.in.x86 > index c770ae3ffe..595db48d91 100644 > --- a/arch/Config.in.x86 > +++ b/arch/Config.in.x86 > @@ -521,11 +521,10 @@ config BR2_x86_steamroller > select BR2_X86_CPU_HAS_SSE4 > select BR2_X86_CPU_HAS_SSE42 > config BR2_x86_geode > - bool "geode" > - # Don't include MMX support because there several variant of geode > - # processor, some with MMX support, some without. > - # See: http://en.wikipedia.org/wiki/Geode_%28processor%29 > - depends on !BR2_x86_64 > + bool "geode" > + depends on !BR2_x86_64 > + help > + see also BR2_x86_geode_with_MMX Please don't change the indentation. Also, please keep the comment, but adapt it to the new reality. The help text that you have added is not very useful, just don't add a help text in this case. > config BR2_x86_c3 > bool "Via/Cyrix C3 (Samuel/Ezra cores)" > depends on !BR2_x86_64 > @@ -633,5 +632,16 @@ config BR2_READELF_ARCH_NAME > default "Intel 80386" if BR2_i386 > default "Advanced Micro Devices X86-64" if BR2_x86_64 > > +config BR2_x86_geode_with_MMX This should go inside the choice...endchoice to select the CPU variant, i.e it should be treated as another CPU variant. Please name the option BR2_x86_geode_mmx > + bool "GEODE with MMX" bool "geode with MMX" > + depends on BR2_x86_geode Drop this, not relevant once it is part of the choice...endchoice of CPU variants. > + default n Not needed, a boolean is already disabled by default. > + select BR2_X86_CPU_HAS_MMX > + help > + there several variant of geode processor, > + some with MMX support, some without. > + See: http://en.wikipedia.org/wiki/Geode_%28processor%29 > + And decide can you use MMX or not Either convert that into a comment, or if it's a help text, there should also be a help text for the Geode without MMX option, and it should probably be something like this: There are several variants of the Geode processors, some supporting MMX instructions, some not. See https://en.wikipedia.org/wiki/Geode_%28processor%29 to determine if your particular Geode processor supports MMX or not. Could you adjust your patch according to those suggestions and send a new iteration? Thanks a lot! Thomas
diff --git a/arch/Config.in.x86 b/arch/Config.in.x86 index c770ae3ffe..595db48d91 100644 --- a/arch/Config.in.x86 +++ b/arch/Config.in.x86 @@ -521,11 +521,10 @@ config BR2_x86_steamroller select BR2_X86_CPU_HAS_SSE4 select BR2_X86_CPU_HAS_SSE42 config BR2_x86_geode - bool "geode" - # Don't include MMX support because there several variant of geode - # processor, some with MMX support, some without. - # See: http://en.wikipedia.org/wiki/Geode_%28processor%29 - depends on !BR2_x86_64 + bool "geode" + depends on !BR2_x86_64 + help + see also BR2_x86_geode_with_MMX config BR2_x86_c3 bool "Via/Cyrix C3 (Samuel/Ezra cores)" depends on !BR2_x86_64 @@ -633,5 +632,16 @@ config BR2_READELF_ARCH_NAME default "Intel 80386" if BR2_i386 default "Advanced Micro Devices X86-64" if BR2_x86_64 +config BR2_x86_geode_with_MMX + bool "GEODE with MMX" + depends on BR2_x86_geode + default n + select BR2_X86_CPU_HAS_MMX + help + there several variant of geode processor, + some with MMX support, some without. + See: http://en.wikipedia.org/wiki/Geode_%28processor%29 + And decide can you use MMX or not + # vim: ft=kconfig # -*- mode:kconfig; -*-