[Buildroot] [PATCH] ci20_defconfig: disable madd instructions to avoid FPU bug

[Buildroot] [PATCH] ci20_defconfig: disable madd instructions to avoid FPU bug

[Vicente Olivert Riera]

MIPS Creator CI20 has an Ingenic JZ4780 SoC which features an XBurst
CPU. The FPU on that CPU has a bug that can generate incorrect results
in certain cases. The problem shows up when you have several of these
instructions in sequence with dependant operands.

Using the -mno-fused-madd option prevents gcc from emitting these
instructions.

More details here:
 - https://groups.google.com/forum/#!topic/mips-creator-ci20/spDB2jjbizM
 - https://android.googlesource.com/platform/build/+/90ce45347064210585a3a1f59a0514c22c753c8a

Signed-off-by: Vicente Olivert Riera <Vincent.Riera@imgtec.com>
---
 configs/ci20_defconfig | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/configs/ci20_defconfig b/configs/ci20_defconfig
index 0455170..1894554 100644
--- a/configs/ci20_defconfig
+++ b/configs/ci20_defconfig
@@ -8,6 +8,8 @@ BR2_PACKAGE_HOST_LINUX_HEADERS_CUSTOM_3_18=y
 
 # system
 BR2_TARGET_GENERIC_GETTY_PORT="ttyS4"
+# Avoid FPU bug
+BR2_TARGET_OPTIMIZATION="-mno-fused-madd"
 
 # kernel
 BR2_LINUX_KERNEL=y
-- 
2.10.1

[Buildroot] [PATCH] ci20_defconfig: disable madd instructions to avoid FPU bug

[Thomas Petazzoni]

Hello,

On Tue, 18 Oct 2016 13:24:21 +0100, Vicente Olivert Riera wrote:
> MIPS Creator CI20 has an Ingenic JZ4780 SoC which features an XBurst
> CPU. The FPU on that CPU has a bug that can generate incorrect results
> in certain cases. The problem shows up when you have several of these
> instructions in sequence with dependant operands.
> 
> Using the -mno-fused-madd option prevents gcc from emitting these
> instructions.
> 
> More details here:
>  - https://groups.google.com/forum/#!topic/mips-creator-ci20/spDB2jjbizM
>  - https://android.googlesource.com/platform/build/+/90ce45347064210585a3a1f59a0514c22c753c8a
> 
> Signed-off-by: Vicente Olivert Riera <Vincent.Riera@imgtec.com>
> ---
>  configs/ci20_defconfig | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/configs/ci20_defconfig b/configs/ci20_defconfig
> index 0455170..1894554 100644
> --- a/configs/ci20_defconfig
> +++ b/configs/ci20_defconfig
> @@ -8,6 +8,8 @@ BR2_PACKAGE_HOST_LINUX_HEADERS_CUSTOM_3_18=y
>  
>  # system
>  BR2_TARGET_GENERIC_GETTY_PORT="ttyS4"
> +# Avoid FPU bug
> +BR2_TARGET_OPTIMIZATION="-mno-fused-madd"

Unfortunately, this means that other people using the same MIPS CPU,
but not using your defconfig will fall into this issue.

For the Intel X1000, which also had a bug, we've added a separate
entry in arch/Config.in.x86. Should we be doing the same here for this
MIPS CPU ?

Note: this is *really* a question. I am not sure we want to make such a
change to the MIPS CPU selection.

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[Buildroot] [PATCH] ci20_defconfig: disable madd instructions to avoid FPU bug

[Vicente Olivert Riera]

Hello Thomas,

On 18/10/16 13:54, Thomas Petazzoni wrote:
> Hello,
> 
> On Tue, 18 Oct 2016 13:24:21 +0100, Vicente Olivert Riera wrote:
>> MIPS Creator CI20 has an Ingenic JZ4780 SoC which features an XBurst
>> CPU. The FPU on that CPU has a bug that can generate incorrect results
>> in certain cases. The problem shows up when you have several of these
>> instructions in sequence with dependant operands.
>>
>> Using the -mno-fused-madd option prevents gcc from emitting these
>> instructions.
>>
>> More details here:
>>  - https://groups.google.com/forum/#!topic/mips-creator-ci20/spDB2jjbizM
>>  - https://android.googlesource.com/platform/build/+/90ce45347064210585a3a1f59a0514c22c753c8a
>>
>> Signed-off-by: Vicente Olivert Riera <Vincent.Riera@imgtec.com>
>> ---
>>  configs/ci20_defconfig | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/configs/ci20_defconfig b/configs/ci20_defconfig
>> index 0455170..1894554 100644
>> --- a/configs/ci20_defconfig
>> +++ b/configs/ci20_defconfig
>> @@ -8,6 +8,8 @@ BR2_PACKAGE_HOST_LINUX_HEADERS_CUSTOM_3_18=y
>>  
>>  # system
>>  BR2_TARGET_GENERIC_GETTY_PORT="ttyS4"
>> +# Avoid FPU bug
>> +BR2_TARGET_OPTIMIZATION="-mno-fused-madd"
> 
> Unfortunately, this means that other people using the same MIPS CPU,
> but not using your defconfig will fall into this issue.

True.

> For the Intel X1000, which also had a bug, we've added a separate
> entry in arch/Config.in.x86. Should we be doing the same here for this
> MIPS CPU ?
> 
> Note: this is *really* a question. I am not sure we want to make such a
> change to the MIPS CPU selection.

We could add an Ingenic XBurst entry in the MIPS CPU selection, and then
add that -mno-fused-madd option to the wrapper based on that selection.

If you are OK with that, please let me know and I'll cook the patch.

Vincent.

> Thomas
> 

[Buildroot] [PATCH] ci20_defconfig: disable madd instructions to avoid FPU bug

[Peter Korsgaard]

>>>>> "Vicente" == Vicente Olivert Riera <Vincent.Riera@imgtec.com> writes:

Hi,

 >> Note: this is *really* a question. I am not sure we want to make such a
 >> change to the MIPS CPU selection.

 > We could add an Ingenic XBurst entry in the MIPS CPU selection, and then
 > add that -mno-fused-madd option to the wrapper based on that selection.

 > If you are OK with that, please let me know and I'll cook the patch.

Yes, I believe this is the way we want to move forward.

-- 
Bye, Peter Korsgaard

[Buildroot] [PATCH] ci20_defconfig: disable madd instructions to avoid FPU bug

[Thomas Petazzoni]

Hello,

On Wed, 19 Oct 2016 09:47:03 +0100, Vicente Olivert Riera wrote:

> We could add an Ingenic XBurst entry in the MIPS CPU selection, and then
> add that -mno-fused-madd option to the wrapper based on that selection.
> 
> If you are OK with that, please let me know and I'll cook the patch.

If I understand correctly:

 - JZ4780 is the SoC
 - XBurst is the core
 - MIPS32r2 is the ISA

So indeed, it makes sense to add an option for XBurst, in order to
mimic what we do on ARM.

Ideally, we should mimic what we do on ARM, and only list in
"Target architecture variants" the cores and not the ISA. How many
vendors are doing MIPS cores, and how many cores are they doing?

If you look at ARM, we have the following situation:

 - Freescale, TI, Atmel, Marvell, Qualcomm, Nvidia, etc. are doing SoCs
   They are way too many for Buildroot to have a list of them.

 - Very few vendors are doing cores. Most of the SoC vendors are using
   the cores from ARM. For example, we have ARM926, Cortex-A7, Cortex-A8,
   Cortex-A53, etc.

   Those are the ones that are listed in "Target Architecture Variant",
   and we use the selected option to know the ISA.

 - ISAs are ARMv4, ARMv5, ARMv6, ARMv7, ARMv8, with a few variants.

So having a similar model for MIPS would be ideal.

Best regards,

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[Buildroot] [PATCH] ci20_defconfig: disable madd instructions to avoid FPU bug

[Vicente Olivert Riera]

Hello Peter and Thomas,

thanks both of you for your comments. I have marked this patch as
changes requested and I've already sent a 2-patch series to add support
for XBurst cores and select that kind of core in the ci20_defconfig.

Regards,

Vincent

On 19/10/16 10:32, Thomas Petazzoni wrote:
> Hello,
> 
> On Wed, 19 Oct 2016 09:47:03 +0100, Vicente Olivert Riera wrote:
> 
>> We could add an Ingenic XBurst entry in the MIPS CPU selection, and then
>> add that -mno-fused-madd option to the wrapper based on that selection.
>>
>> If you are OK with that, please let me know and I'll cook the patch.
> 
> If I understand correctly:
> 
>  - JZ4780 is the SoC
>  - XBurst is the core
>  - MIPS32r2 is the ISA
> 
> So indeed, it makes sense to add an option for XBurst, in order to
> mimic what we do on ARM.
> 
> Ideally, we should mimic what we do on ARM, and only list in
> "Target architecture variants" the cores and not the ISA. How many
> vendors are doing MIPS cores, and how many cores are they doing?
> 
> If you look at ARM, we have the following situation:
> 
>  - Freescale, TI, Atmel, Marvell, Qualcomm, Nvidia, etc. are doing SoCs
>    They are way too many for Buildroot to have a list of them.
> 
>  - Very few vendors are doing cores. Most of the SoC vendors are using
>    the cores from ARM. For example, we have ARM926, Cortex-A7, Cortex-A8,
>    Cortex-A53, etc.
> 
>    Those are the ones that are listed in "Target Architecture Variant",
>    and we use the selected option to know the ISA.
> 
>  - ISAs are ARMv4, ARMv5, ARMv6, ARMv7, ARMv8, with a few variants.
> 
> So having a similar model for MIPS would be ideal.
> 
> Best regards,
> 
> Thomas
> 

[Buildroot] [PATCH] ci20_defconfig: disable madd instructions to avoid FPU bug

[Arnout Vandecappelle]

On 19-10-16 11:32, Thomas Petazzoni wrote:
> Hello,
> 
> On Wed, 19 Oct 2016 09:47:03 +0100, Vicente Olivert Riera wrote:
> 
>> We could add an Ingenic XBurst entry in the MIPS CPU selection, and then
>> add that -mno-fused-madd option to the wrapper based on that selection.
>>
>> If you are OK with that, please let me know and I'll cook the patch.
> 
> If I understand correctly:
> 
>  - JZ4780 is the SoC
>  - XBurst is the core
>  - MIPS32r2 is the ISA
> 
> So indeed, it makes sense to add an option for XBurst, in order to
> mimic what we do on ARM.
> 
> Ideally, we should mimic what we do on ARM, and only list in
> "Target architecture variants" the cores and not the ISA. How many
> vendors are doing MIPS cores, and how many cores are they doing?

 I believe that for MIPS, every vendor uses their own core, and that there is an
almost one-to-one mapping between SoC family and core. AFAIU it's only the
instruction set that is standardized. My gcc supports 74 cores... Do we really
want to add all of these?

 That said, now we already have a bit a mixed situation: we have a few
processors and a few generic options in Config.in.mips. But I expect that
Buildroot will often be used on SoCs where either Buildroot or GCC doesn't have
a processor-specific option yet, so the generic ones will still be needed.

> If you look at ARM, we have the following situation:
> 
>  - Freescale, TI, Atmel, Marvell, Qualcomm, Nvidia, etc. are doing SoCs
>    They are way too many for Buildroot to have a list of them.
> 
>  - Very few vendors are doing cores. Most of the SoC vendors are using
>    the cores from ARM. For example, we have ARM926, Cortex-A7, Cortex-A8,
>    Cortex-A53, etc.

 That's the difference of course. ARM owns the core, while MIPS is a more or
less open architecture. Not open enough, of course, which is why RISC-V was started.

 Regards,
 Arnout


>    Those are the ones that are listed in "Target Architecture Variant",
>    and we use the selected option to know the ISA.
> 
>  - ISAs are ARMv4, ARMv5, ARMv6, ARMv7, ARMv8, with a few variants.
> 
> So having a similar model for MIPS would be ideal.
> 
> Best regards,
> 
> Thomas
> 

-- 
Arnout Vandecappelle                          arnout at mind be
Senior Embedded Software Architect            +32-16-286500
Essensium/Mind                                http://www.mind.be
G.Geenslaan 9, 3001 Leuven, Belgium           BE 872 984 063 RPR Leuven
LinkedIn profile: http://www.linkedin.com/in/arnoutvandecappelle
GPG fingerprint:  7493 020B C7E3 8618 8DEC 222C 82EB F404 F9AC 0DDF

[Buildroot] [PATCH] ci20_defconfig: disable madd instructions to avoid FPU bug

[Thomas Petazzoni]

Hello,

On Fri, 21 Oct 2016 21:45:52 +0200, Arnout Vandecappelle wrote:

> > Ideally, we should mimic what we do on ARM, and only list in
> > "Target architecture variants" the cores and not the ISA. How many
> > vendors are doing MIPS cores, and how many cores are they doing?  
> 
>  I believe that for MIPS, every vendor uses their own core, and that there is an
> almost one-to-one mapping between SoC family and core. AFAIU it's only the
> instruction set that is standardized. My gcc supports 74 cores... Do we really
> want to add all of these?

Well, did you count for ARM? There are also many many cores supported
in gcc. Admittedly probably not 74 cores, indeed.

>  That said, now we already have a bit a mixed situation: we have a few
> processors and a few generic options in Config.in.mips. But I expect that
> Buildroot will often be used on SoCs where either Buildroot or GCC doesn't have
> a processor-specific option yet, so the generic ones will still be needed.

OK, fair enough. Then maybe they should be clearly separated by a
comment within the choice...endchoice block.

Best regards,

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com