New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 687622 link

Starred by 1 user

Issue metadata

Status: Assigned
Owner:
Last visit > 30 days ago
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 3
Type: Bug



Sign in to add a comment

ec: When should we use CONFIG_FPU?

Project Member Reported by sha...@chromium.org, Feb 1 2017

Issue description

Out of our four standard / shipping EC families, three support FPU instructions / regs (lm4, mec1322, npcx) and one does not (stm32 - maybe some variants support FPU, but our typical stm32f0 does not).

We have CONFIG_FPU in our codebase (undef'd by default) for floating point support. Let's see where it's used:

(1) cortex-m reset / context switch code, to init / backup FP regs.
(2) Some calibration code in common (required, no alternate fixed point implementation exists yet) - mag_cal.c, mat33.c.
(3) math_util.h - use floats vs fixed point for math_util.c, implements rotate() which is commonly used in motion sense code / drivers.

Currently our decision to use CONFIG_FPU is usually whether we compile in code from (2). Eg. undef CONFIG_FPU by default until things stop compiling, then add it.

Is this the right decision always? Let's look at reef vs kevin on TOT, reef defines CONFIG_FPU and kevin does not. Both heavily use motion sense code. By adding CONFIG_FPU on kevin, I can immediately save 192 bytes of RAM. Of course there are other important differences I have not measured (precision, performance, power?).

Does it make sense to always define CONFIG_FPU if we will be calling rotate()? Let's discuss.
 
Project Member

Comment 1 by bugdroid1@chromium.org, Feb 9 2017

Labels: merge-merged-firmware-gru-8785.B
The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform/ec/+/279bf12776599febbb0cb11dd7a830f654103d8c

commit 279bf12776599febbb0cb11dd7a830f654103d8c
Author: Shawn Nematbakhsh <shawnn@chromium.org>
Date: Thu Feb 09 19:25:34 2017

kevin & friends: Define CONFIG_FPU

There are net SRAM and performance savings to be had by defining
CONFIG_FPU, so define it for kevin and followers.

BUG=chromium:687622
BRANCH=gru
TEST=Verify compilation + screen rotation on kevin.

Change-Id: Ia759ab2cd493c7aaae4389ce51022446c3c83953
Signed-off-by: Shawn Nematbakhsh <shawnn@chromium.org>
Reviewed-on: https://chromium-review.googlesource.com/439915
Tested-by: Aseda Aboagye <aaboagye@chromium.org>
Reviewed-by: Aseda Aboagye <aaboagye@chromium.org>

[modify] https://crrev.com/279bf12776599febbb0cb11dd7a830f654103d8c/board/scarlet/board.h
[modify] https://crrev.com/279bf12776599febbb0cb11dd7a830f654103d8c/board/bob/board.h
[modify] https://crrev.com/279bf12776599febbb0cb11dd7a830f654103d8c/board/kevin/board.h

Comment 2 by gkihumba@google.com, Mar 31 2017

Owner: sha...@chromium.org
Status: Assigned (was: Untriaged)

Comment 3 by sha...@chromium.org, Jun 22 2017

There's some more work to be done here, and it requires discussion.

Prior to a recent nds32 CL (https://chromium-review.googlesource.com/427640), we only supported CONFIG_FPU on hardware with an actual hardware floating point unit (eg. we didn't bother with software floating point on Cortex-M0). Now, software support has been added to nds32. Re-reading my original post on this bug, it has the following implications for nds32:

- We can define CONFIG_FPU and now run some driver calibration routines for which no fixed point implementation exists. Great! But...

- Our motion sense routines will now use SW floating point rather than fixed point. I haven't done any actual testing, but I have a feeling this is undesired.

My thought is that all cros-ec code (outside of board/, chip/ or core/) should be functional in fixed or floating point, which will be selected based on CONFIG_FPU (this is how motion sense already works). And, we should only allow CONFIG_FPU for parts with HW FPU (eg. parts where we a floating point implementation is probably preferred).

Thoughts?

Comment 4 by sha...@chromium.org, Jun 22 2017

Cc: -aaboagye@chromium.org jbruneau@chromium.org

Comment 5 by sha...@chromium.org, Jun 22 2017

Cc: aaboagye@chromium.org

Comment 6 by vpalatin@google.com, Jun 23 2017

> Prior to a recent nds32 CL (https://chromium-review.googlesource.com/427640), 
> we only supported CONFIG_FPU on hardware with an actual
> hardware floating point unit

the nds32 has (limited) floating point hardware, it is just badly integrated with the CPU and requires weird quirks (eg it shares registers with integers)
but the implementation doesn't qualify as 'software'
#3: I did not convert the magnetometer library to fixed point: looking at the code and seeing run I was afraid losing the exponent would be a problem.

The rest of the motion sensor code works fine with floating point, sqrt as defined in math_util.c will be replaced with the implementation in nds32/math.c.
Will the register shifting increase the code size compared to the fixed point code?

Sign in to add a comment