risc-v: add comments about not needing 16-byte stack frame alignment

Since we are not actually executing on a stack frame that is not 16-byte
aligned, we are following the guidance (linked below). Add comments for
future developers to explain why.

Also, saving system stack pointer in the switch to function since the
isr function takes special care to not over write the stack pointer when
we are already using the system stack.

According to documentation, the stack frame should be 128-bit aligned
upon entering function boundaries.

"In the standard RISC-V calling convention, the stack pointer sp is
always 16-byte aligned" from
https://riscv.org/specifications/isa-spec-pdf/

"The stack grows downwards (towards lower addresses) and the stack
pointer shall be aligned to a 128-bit boundary upon procedure entry"
from
https://github.com/riscv/riscv-elf-psabi-doc/blob/master/riscv-elf.md

See also documentation issues discussing this
https://github.com/riscv/riscv-elf-psabi-doc/issues/21

BRANCH=none
BUG=none
TEST=ITE RISC-V FPU implementation still works

Signed-off-by: Jett Rink <jettrink@chromium.org>
Change-Id: I3460e6ee2b68c7793c72517e7d2d9bc645aaea65
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/ec/+/2173119
Tested-by: Dino Li <Dino.Li@ite.com.tw>
Reviewed-by: Aseda Aboagye <aaboagye@chromium.org>

3 files changed, 48 insertions, 7 deletions

diff --git a/core/riscv-rv32i/cpu.h b/core/riscv-rv32i/cpu.h
index 2cb31a6faa..e46b893ad6 100644
--- a/core/riscv-rv32i/cpu.h
+++ b/core/riscv-rv32i/cpu.h
@@ -8,6 +8,10 @@
 #ifndef __CROS_EC_CPU_H
 #define __CROS_EC_CPU_H
 
+/*
+ * This is the space required by both __irq_isr and __switch_task to store all
+ * of the caller and callee registers for each task context before switching.
+ */
 #ifdef CONFIG_FPU
 /* additional space to save FP registers (fcsr, ft0-11, fa0-7, fs0-11) */
 #define TASK_SCRATCHPAD_SIZE (62)
diff --git a/core/riscv-rv32i/init.S b/core/riscv-rv32i/init.S
index de98846686..dedd7a644f 100644
--- a/core/riscv-rv32i/init.S
+++ b/core/riscv-rv32i/init.S
@@ -147,18 +147,35 @@ __irq_isr:
         fsw ft2,  -19*4(sp)
         fsw ft1,  -18*4(sp)
         fsw ft0,  -17*4(sp)
+        /*
+         * Note: we never execute on this stack frame, so it does not need to
+         * be 16-byte aligned.
+         */
         addi sp, sp, -37*4
 #else
+        /*
+         * Note: we never execute on this stack frame, so it does not need to
+         * be 16-byte aligned.
+         */
         addi sp, sp, -16*4
 #endif
-        /* save sp to scratch register */
+        /* Save sp to scratch register */
         csrw mscratch, sp
-        /* switch to system stack if we are called from process stack */
+        /* Load top of system stack address into t0 for comparison */
         la t0, stack_end
-        /* no chagne sp if sp < end of system stack */
-        bltu sp, t0, __no_adjust_sp
+        /*
+         * Switch to system stack (which is in lower memory than task stack)
+         * if we are not already operating with the system stack
+         */
+        bltu sp, t0, __sp_16byte_aligned
         mv sp, t0
-__no_adjust_sp:
+__sp_16byte_aligned:
+        /*
+         * This ensures sp is 16-byte aligned. This only applies to when there
+         * is an interrupt before tasks start. Otherwise stack_end is already
+         * 16-byte aligned.
+         */
+        andi sp, sp, -16
         /* read exception cause */
         csrr t0, mcause
         /* isolate exception cause */
@@ -388,7 +405,16 @@ _data_end:
 _data_lma_start:
 .long __data_lma_start
 
-/* Reserve space for system stack */
+/*
+ * Reserve space for system stack.
+ *
+ * Main routine and ISR will share this space before tasks start.
+ * This space is then dedicated to ISRs after tasks start.
+ *
+ * NOTE: Location of system stack (.bss.system_stack) must be less than
+ * tasks stacks (task_stacks@.bss) and scratchpad for first context switch
+ * (scratchpad[]@.bss.task_scratchpad).
+ */
 .section .bss.system_stack
 stack_start:
 .space CONFIG_STACK_SIZE, 0
diff --git a/core/riscv-rv32i/switch.S b/core/riscv-rv32i/switch.S
index cfff3e8bc3..b31e75076a 100644
--- a/core/riscv-rv32i/switch.S
+++ b/core/riscv-rv32i/switch.S
@@ -43,6 +43,8 @@ __switch_task:
 	beq a0, t0, __irq_exit
 	/* save our new scheduled task  */
 	sw a0, 0(t1)
+        /* save our current location in system stack so we can restore at end */
+        add t3, sp, zero
         /* restore current process stack pointer */
         csrr sp, mscratch
         /* get the task program counter saved at exception entry */
@@ -76,10 +78,18 @@ __switch_task:
         fsw fs0,  -13*4(sp)
         /* save program counter on the current process stack */
         sw t5,       -25*4(sp)
+        /*
+         * Note: we never execute on this stack frame, so it does not need to
+         * be 16-byte aligned.
+         */
         addi sp, sp, -25*4
 #else
 	/* save program counter on the current process stack */
         sw t5,       -13*4(sp)
+        /*
+         * Note: we never execute on this stack frame, so it does not need to
+         * be 16-byte aligned.
+         */
         addi sp, sp, -13*4
 #endif
         /* save the task stack pointer in its context */
@@ -128,7 +138,8 @@ __switch_task:
          * __irq_exit will restore sp from scratch register again before mret.
          */
         csrw mscratch, sp
-        la sp, stack_end
+        /* restore system stack */
+        add sp, t3, zero
         j __irq_exit
 
 /**