Re: [Buildroot] [PATCH 1/1] arch/Config.in.x86: add MMX instruction set capability for Geode CPU

[Thomas Petazzoni via buildroot <buildroot@buildroot.org>]

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
-- 
Thomas Petazzoni, co-owner and CEO, Bootlin
Embedded Linux and Kernel engineering and training
https://bootlin.com
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot