Le 08/07/2022 à 19:14, Pali Rohár a écrit :
> On Monday 04 July 2022 15:13:58 Pali Rohár wrote:
>> On Monday 04 July 2022 14:07:10 Arnd Bergmann wrote:
>>> On Mon, Jul 4, 2022 at 12:39 PM Pali Rohár <p...@kernel.org> wrote:
>>>> On Monday 04 July 2022 20:23:29 Michael Ellerman wrote:
>>>>> On 2 July 2022 7:44:05 pm AEST, "Pali Rohár" <p...@kernel.org> wrote:
>>>>>> On Tuesday 24 May 2022 11:39:39 Pali Rohár wrote:
>>>>>>> gcc e500 compiler does not support -mcpu=powerpc option. When it is
>>>>>>> specified then gcc throws compile error:
>>>>>>>
>>>>>>> gcc: error: unrecognized argument in option ‘-mcpu=powerpc’
>>>>>>> gcc: note: valid arguments to ‘-mcpu=’ are: 8540 8548 native
>>>>>>>
>>>>>>> So do not set -mcpu=powerpc option when CONFIG_E500 is set. Correct
>>>>>>> option
>>>>>>> -mcpu=8540 for CONFIG_E500 is set few lines below in that Makefile.
>>>>>>>
>>>>>>> Signed-off-by: Pali Rohár <p...@kernel.org>
>>>>>>> Cc: sta...@vger.kernel.org
>>>>>>
>>>>>> Michael, do you have any objections about this patch?
>>>>>
>>>>> I don't particularly like it :)
>>>>>
>>>>> From the discussion with Segher, it sounds like this is a problem with a
>>>>> specific build of gcc that you're using, not a general problem with gcc
>>>>> built with e500 support.
>>>>
>>>> Well, the "full" build of gcc for e500 cores with SPE does not support
>>>> -mcpu=powerpc option. So I think this is a general problem. I do not
>>>> think that this is "specific build" as this is the correct build of gcc
>>>> for these processors with e500 cores.
>>>>
>>>> "stripped". build of gcc without SPE support for e500 cores does not
>>>> have this problem...
>>>
>>> I can see a couple of problems with the CPU selection, but I don't think
>>> this is a major one, as nobody should be using those SPE compilers for
>>> building the kernel. Just use a modern powerpc-gcc build.
>>
>> The point is to use same compiler for building kernel as for the all
>> other parts of the system.
>>
>> I just do not see reason why for kernel it is needed to build completely
>> different toolchain and compiler.
>>
>>>>> Keying it off CONFIG_E500 means it will fix your problem, but not anyone
>>>>> else who has a different non-e500 compiler that also doesn't support
>>>>> -mcpu=powerpc (for whatever reason).
>>>>>
>>>>> So I wonder if a better fix is to use cc-option when setting
>>>>> -mcpu=powerpc.
>>>>>
>>>>
>>>> Comment for that code which adds -mpcu=powerpc says:
>>>>
>>>> they are needed to set a sane 32-bit cpu target for the 64-bit cross
>>>> compiler which may default to the wrong ISA.
>>>>
>>>> So I'm not sure how to handle this in other way. GCC uses -mpcu=8540
>>>> option for specifying to compile code for e500 cores and seems that
>>>> -mcpu=8540 is supported by all e500 compilers...
>>>>
>>>> Few lines below is code
>>>>
>>>> CFLAGS-$(CONFIG_E500) += $(call cc-option,-mcpu=8540
>>>> -msoft-float,-mcpu=powerpc)
>>>>
>>>> which for e500 kernel builds user either -mcpu=8540 or -mcpu=powerpc
>>>> (probably as a fallback if -mcpu=8540 is not supported).
>>>
>>> The -mcpu=powerpc fallback can probably be skipped here, that must have been
>>> for compilers predating the addition of -mcpu=8540, and even the oldest ones
>>> support that now.
>>
>> Ok, makes sense.
>>
>>>> So for me it looks like that problematic code
>>>>
>>>> KBUILD_CFLAGS += -mcpu=powerpc
>>>> KBUILD_AFLAGS += -mcpu=powerpc
>>>>
>>>> needs to be somehow skipped when compiling for CONFIG_E500.
>>>>> My change which skips that code base on ifndef CONFIG_E500 should be
>>>> fine as when CONFIG_E500 is disabled it does nothing and when it is
>>>> enabled then code
>>>>
>>>> CFLAGS-$(CONFIG_E500) += $(call cc-option,-mcpu=8540
>>>> -msoft-float,-mcpu=powerpc)
>>>>
>>>> is called which sets -mcpu option suitable for e500.
>>>
>>> I think this part is indeed fishy, but adding another special case for E500
>>> seems to take it in the wrong direction.
>>>
>>> Nick added this in 4bf4f42a2feb ("powerpc/kbuild: Set default generic
>>> machine type
>>> for 32-bit compile") as a compile-time fix to prevent the default target
>>> from
>>> getting used when the compiler supports both 64-bit and 32-bit. This is the
>>> right idea, but it's inconsistent to pass different flags depending on the
>>> type
>>> of toolchain, and it loses the more specific options.
>>>
>>> Another problem I see is that a kernel that is built for both E500 and
>>> E500MC
>>> uses -mcpu=e500mc and may not actually work on the older ones either
>>> (even with your patch).
>>
>> That is probably truth, -mcpu=8540 should have been chosen. (Anyway it
>> should have been called -mcpu=e500, no idea why gcc still name it 8540.)
>>
>>> I think what you actually want is to set one option for each of the
>>> possible CPU types:
>>>
>>> CFLAGS_CPU-$(CONFIG_PPC_BOOK3S_32) := -mcpu=powerpc
>>> CFLAGS_CPU-$(CONFIG_PPC_85xx) := -mcpu=8540
>>> CFLAGS_CPU-$(CONFIG_PPC8xx) := -mcpu=860
>>> CFLAGS_CPU-$(CONFIG_PPC44x) := -mcpu=440
>>> CFLAGS_CPU-$(CONFIG_PPC40x) := -mcpu=405
>>> ifdef CONFIG_CPU_LITTLE_ENDIAN
>>> CFLAGS_CPU-$(CONFIG_BOOK3S_64) := -mcpu=power8
>>> else
>>> CFLAGS_CPU-$(CONFIG_BOOK3S_64) := -mcpu=power5
>>> endif
>>> CFLAGS_CPU-$(CONFIG_BOOK3E_64) := -mcpu=powerpc64
>>
>> Yes, this is something I would expect that in Makefile should be.
>
> So what about this change?
>
> diff --git a/arch/powerpc/Makefile b/arch/powerpc/Makefile
> index a0cd70712061..74a608b5796a 100644
> --- a/arch/powerpc/Makefile
> +++ b/arch/powerpc/Makefile
> @@ -15,22 +15,7 @@ HAS_BIARCH := $(call cc-option-yn, -m32)
> # Set default 32 bits cross compilers for vdso and boot wrapper
> CROSS32_COMPILE ?=
>
> -ifeq ($(HAS_BIARCH),y)
> -ifeq ($(CROSS32_COMPILE),)
> -ifdef CONFIG_PPC32
> -# These options will be overridden by any -mcpu option that the CPU
> -# or platform code sets later on the command line, but they are needed
> -# to set a sane 32-bit cpu target for the 64-bit cross compiler which
> -# may default to the wrong ISA.
> -KBUILD_CFLAGS += -mcpu=powerpc
> -KBUILD_AFLAGS += -mcpu=powerpc
> -endif
> -endif
> -endif
> -
> -ifdef CONFIG_PPC_BOOK3S_32
> -KBUILD_CFLAGS += -mcpu=powerpc
> -endif
> +CFLAGS-$(CONFIG_PPC_BOOK3S_32) += -mcpu=powerpc
This comes too early, it is overriden by later CFLAGS-$(CONFIG_PPC32) :=
something
>
> # If we're on a ppc/ppc64/ppc64le machine use that defconfig, otherwise
> just use
> # ppc64_defconfig because we have nothing better to go on.
> @@ -163,17 +148,14 @@ CFLAGS-$(CONFIG_PPC32) += $(call cc-option,
> $(MULTIPLEWORD))
>
> CFLAGS-$(CONFIG_PPC32) += $(call cc-option,-mno-readonly-in-sdata)
>
> -ifdef CONFIG_PPC_BOOK3S_64
> ifdef CONFIG_CPU_LITTLE_ENDIAN
> -CFLAGS-$(CONFIG_GENERIC_CPU) += -mcpu=power8
> -CFLAGS-$(CONFIG_GENERIC_CPU) += $(call cc-option,-mtune=power9,-mtune=power8)
> +CFLAGS-$(CONFIG_PPC_BOOK3S_64) += -mcpu=power8
> +CFLAGS-$(CONFIG_PPC_BOOK3S_64) += $(call
> cc-option,-mtune=power9,-mtune=power8)
> else
> -CFLAGS-$(CONFIG_GENERIC_CPU) += $(call cc-option,-mtune=power7,$(call
> cc-option,-mtune=power5))
> -CFLAGS-$(CONFIG_GENERIC_CPU) += $(call cc-option,-mcpu=power5,-mcpu=power4)
> -endif
> -else ifdef CONFIG_PPC_BOOK3E_64
> -CFLAGS-$(CONFIG_GENERIC_CPU) += -mcpu=powerpc64
> +CFLAGS-$(CONFIG_PPC_BOOK3S_64) += $(call cc-option,-mtune=power7,$(call
> cc-option,-mtune=power5))
> +CFLAGS-$(CONFIG_PPC_BOOK3S_64) += $(call cc-option,-mcpu=power5,-mcpu=power4)
So before that change I got -mcpu=power9
Now I get -mtune=power7 -mcpu=power5 -mcpu=power9
> endif
> +CFLAGS-$(CONFIG_PPC_BOOK3E_64) += -mcpu=powerpc64
>
> ifdef CONFIG_FUNCTION_TRACER
> CC_FLAGS_FTRACE := -pg
> @@ -193,13 +175,8 @@ endif
> CFLAGS-$(CONFIG_E5500_CPU) += $(E5500_CPU)
> CFLAGS-$(CONFIG_E6500_CPU) += $(call cc-option,-mcpu=e6500,$(E5500_CPU))
>
> -ifdef CONFIG_PPC32
> -ifdef CONFIG_PPC_E500MC
> -CFLAGS-y += $(call cc-option,-mcpu=e500mc,-mcpu=powerpc)
> -else
> +CFLAGS-$(CONFIG_PPC_E500MC) += $(call cc-option,-mcpu=e500mc,-mcpu=powerpc)
Before I got -mcpu=e6500
Now I get -mcpu=powerpc64 -mcpu=e6500 -mcpu=e500mc -mcpu=8540
> CFLAGS-$(CONFIG_E500) += $(call cc-option,-mcpu=8540
> -msoft-float,-mcpu=powerpc)
> -endif
> -endif
>
> asinstr := $(call as-instr,lis 9$(comma)foo@high,-DHAVE_AS_ATHIGH=1)
>
>
>
>> But what to do with fallback value?
>>
>>> For the non-generic CPU types, there is also CONFIG_TARGET_CPU,
>>> and the list above could just get folded into that instead.
>>>
>>> Arnd
Christophe
