pkarashchenko commented on a change in pull request #5192:
URL: https://github.com/apache/incubator-nuttx/pull/5192#discussion_r780389520
##########
File path: arch/risc-v/src/common/riscv_fault.c
##########
@@ -54,61 +66,61 @@
*
****************************************************************************/
-void up_fault(int irq, uint64_t *regs)
+void up_fault(int irq, uintptr_t *regs)
{
CURRENT_REGS = regs;
- _alert("EPC:%016" PRIx64 "\n",
+ _alert("EPC:%0" PRIxREG "\n",
Review comment:
Maybe move `"0"` to `PRIxREG`?
##########
File path: arch/risc-v/src/fe310/fe310_irq_dispatch.c
##########
@@ -52,10 +52,10 @@ volatile uint32_t * g_current_regs;
* fe310_dispatch_irq
****************************************************************************/
-void *fe310_dispatch_irq(uint32_t vector, uint32_t *regs)
+void *fe310_dispatch_irq(uint32_t vector, uintptr_t *regs)
{
uint32_t irq = (vector >> 27) | (vector & 0xf);
- uint32_t *mepc = regs;
+ uint32_t *mepc = (uint32_t *)regs;
Review comment:
Maybe can switch to `uintptr_t` here at well? Or it is not possible?
##########
File path: arch/risc-v/src/k210/k210_irq_dispatch.c
##########
@@ -51,10 +51,10 @@ extern void up_fault(int irq, uint64_t *regs);
* k210_dispatch_irq
****************************************************************************/
-void *k210_dispatch_irq(uint64_t vector, uint64_t *regs)
+void *k210_dispatch_irq(uint64_t vector, uintptr_t *regs)
Review comment:
Can we go with `uintptr_t vector`?
##########
File path: arch/risc-v/src/common/riscv_schedulesigaction.c
##########
@@ -37,6 +37,18 @@
#include "riscv_internal.h"
#include "riscv_arch.h"
+/****************************************************************************
+ * Pre-processor Definitions
+ ****************************************************************************/
+
+/* Format output with register width and hex */
+
+#ifdef CONFIG_ARCH_RV32
+# define PRIxREG "%08"PRIxPTR
Review comment:
I recommend to leave `"%"` in format string and move here to
```suggestion
# define PRIxREG "08" PRIxPTR
```
Same in other places
##########
File path: arch/risc-v/src/litex/litex_irq_dispatch.c
##########
@@ -51,10 +51,10 @@ volatile uint32_t * g_current_regs;
* litex_dispatch_irq
****************************************************************************/
-void *litex_dispatch_irq(uint32_t vector, uint32_t *regs)
+void *litex_dispatch_irq(uint32_t vector, uintptr_t *regs)
{
uint32_t irq = (vector >> 27) | (vector & 0xf);
- uint32_t *mepc = regs;
+ uint32_t *mepc = (uint32_t *)regs;
Review comment:
Can we go with `uintprt_t *mepc`?
##########
File path: arch/risc-v/include/common/irq.h
##########
@@ -134,6 +140,14 @@
# error not supported !!!
Review comment:
In 32 bit variant of include we had different vales for FPU registers.
##########
File path: arch/risc-v/include/common/irq.h
##########
@@ -134,6 +140,14 @@
# error not supported !!!
Review comment:
32-bit variant had
```
#if defined(CONFIG_ARCH_DPFPU)
# define FPU_REG_SIZE 2
#elif defined(CONFIG_ARCH_QPFPU)
# define FPU_REG_SIZE 4
#else
# define FPU_REG_SIZE 1
#endif
```
##########
File path: arch/risc-v/include/common/irq.h
##########
@@ -134,6 +140,14 @@
# error not supported !!!
#endif
+/* REG size */
+
+#ifdef CONFIG_ARCH_RV32
+#define REG_SIZE 1
+#else
+#define REG_SIZE 2
+#endif
Review comment:
Where this is used? Probably the idea was to define this before `#ifdef
CONFIG_ARCH_FPU` and have a next code:
```
#if defined(CONFIG_ARCH_DPFPU)
# define FPU_REG_SIZE (2 / REG_SIZE)
#elif defined(CONFIG_ARCH_QPFPU)
# define FPU_REG_SIZE (4 / REG_SIZE)
#else
# if REG_SIZE > 1
# error not supported !!!
# else
# define FPU_REG_SIZE 1
# endif
#endif
```
??
##########
File path: arch/risc-v/include/common/irq.h
##########
@@ -134,6 +140,14 @@
# error not supported !!!
#endif
+/* REG size */
+
+#ifdef CONFIG_ARCH_RV32
+#define REG_SIZE 1
+#else
+#define REG_SIZE 2
+#endif
Review comment:
Where this is used? Probably the idea was to define this before `#ifdef
CONFIG_ARCH_FPU` and have a next code:
```
#if defined(CONFIG_ARCH_DPFPU)
# define FPU_REG_SIZE (2 / REG_SIZE)
#elif defined(CONFIG_ARCH_QPFPU)
# define FPU_REG_SIZE (4 / REG_SIZE)
#else
# if REG_SIZE > 1
# error not supported !!!
# else
# define FPU_REG_SIZE 1
# endif
#endif
```
??
##########
File path: arch/risc-v/include/common/irq.h
##########
@@ -134,6 +140,14 @@
# error not supported !!!
#endif
+/* REG size */
+
+#ifdef CONFIG_ARCH_RV32
+#define REG_SIZE 1
+#else
+#define REG_SIZE 2
+#endif
Review comment:
Where this is used? Probably the idea was to define this before `#ifdef
CONFIG_ARCH_FPU` and have a next code:
```
#if defined(CONFIG_ARCH_DPFPU)
# define FPU_REG_SIZE (2 / REG_SIZE)
#elif defined(CONFIG_ARCH_QPFPU)
# define FPU_REG_SIZE (4 / REG_SIZE)
#else
# if REG_SIZE > 1
# error not supported !!!
# else
# define FPU_REG_SIZE 1
# endif
#endif
```
??
or
```
#if defined(CONFIG_ARCH_DPFPU)
# define FPU_REG_SIZE (2 / REG_SIZE)
#elif defined(CONFIG_ARCH_QPFPU)
# define FPU_REG_SIZE (4 / REG_SIZE)
#else
# define FPU_REG_SIZE (1 / REG_SIZE)
#endif
#if FPU_REG_SIZE == 0
# error not supported !!!
#endif
```
##########
File path: arch/risc-v/src/common/riscv_internal.h
##########
@@ -171,8 +160,8 @@ void riscv_ack_irq(int irq);
void riscv_copystate(uint64_t *dest, uint64_t *src);
void riscv_copyfullstate(uint64_t *dest, uint64_t *src);
#else
-void riscv_copystate(uint32_t *dest, uint32_t *src);
-void riscv_copyfullstate(uint32_t *dest, uint32_t *src);
+void riscv_copystate(uintptr_t *dest, uintptr_t *src);
+void riscv_copyfullstate(uintptr_t *dest, uintptr_t *src);
#endif
Review comment:
```suggestion
void riscv_copystate(uintptr_t *dest, uintptr_t *src);
void riscv_copyfullstate(uintptr_t *dest, uintptr_t *src);
```
eliminate `#ifdef CONFIG_ARCH_RV64`
##########
File path: arch/risc-v/src/common/riscv_internal.h
##########
@@ -184,8 +173,8 @@ uint32_t riscv_get_newintctx(void);
void riscv_savefpu(uint64_t *regs);
void riscv_restorefpu(const uint64_t *regs);
#else /* !CONFIG_ARCH_RV64 */
-void riscv_savefpu(uint32_t *regs);
-void riscv_restorefpu(const uint32_t *regs);
+void riscv_savefpu(uintptr_t *regs);
+void riscv_restorefpu(const uintptr_t *regs);
Review comment:
Eliminate `CONFIG_ARCH_RV64` and use only
```
void riscv_savefpu(uintptr_t *regs);
void riscv_restorefpu(const uintptr_t *regs);
```
##########
File path: arch/risc-v/src/common/riscv_swint.c
##########
@@ -455,7 +455,7 @@ int riscv_swint(int irq, void *context, void *arg)
rtcb->flags |= TCB_FLAG_SYSCALL;
#else
- svcerr("ERROR: Bad SYS call: %" PRId64 "\n", regs[REG_A0]);
+ svcerr("ERROR: Bad SYS call: %" PRIdPTR "\n", regs[REG_A0]);
Review comment:
Maybe can switch to `PRIxREG` here.
Also rework `static void riscv_registerdump(const uint64_t *regs)` to use
`uintptr_t`.
##########
File path: arch/risc-v/src/fe310/fe310_irq_dispatch.c
##########
@@ -52,10 +52,10 @@ volatile uint32_t * g_current_regs;
* fe310_dispatch_irq
****************************************************************************/
-void *fe310_dispatch_irq(uint32_t vector, uint32_t *regs)
+void *fe310_dispatch_irq(uint32_t vector, uintptr_t *regs)
{
uint32_t irq = (vector >> 27) | (vector & 0xf);
- uint32_t *mepc = regs;
+ uint32_t *mepc = (uint32_t *)regs;
Review comment:
The same for `vector` and `irq`
##########
File path: arch/risc-v/src/qemu-rv32/qemu_rv32_head.S
##########
@@ -154,21 +139,6 @@ exception_common:
lw s0, 32*4(sp) /* restore mstatus */
csrw mstatus, s0
-#if defined(INT_XCPT_REGS) && INT_XCPT_REGS >= 39
- lw x28, 36*4(sp)
- lw x29, 37*4(sp)
- lw x30, 38*4(sp)
- csrrw x0, 0x7b4, x28
- csrrw x0, 0x7b5, x29
- csrrw x0, 0x7b6, x30
- lw x28, 33*4(sp)
- lw x29, 34*4(sp)
- lw x30, 35*4(sp)
- csrrw x0, 0x7b0, x28
- csrrw x0, 0x7b1, x29
- csrrw x0, 0x7b2, x30
-#endif
Review comment:
Why this is removed?
##########
File path: arch/risc-v/src/qemu-rv32/qemu_rv32_head.S
##########
@@ -105,21 +105,6 @@ exception_common:
sw x30, 30*4(sp) /* t5 */
sw x31, 31*4(sp) /* t6 */
-#if defined(INT_XCPT_REGS) && INT_XCPT_REGS >= 39
- csrr x28, 0x7b0
- csrr x29, 0x7b1
- csrr x30, 0x7b2
- sw x28, 33*4(sp)
- sw x29, 34*4(sp)
- sw x30, 35*4(sp)
- csrr x28, 0x7b4
- csrr x29, 0x7b5
- csrr x30, 0x7b6
- sw x28, 36*4(sp)
- sw x29, 37*4(sp)
- sw x30, 38*4(sp)
-#endif
-
Review comment:
Why this is removed?
##########
File path: arch/risc-v/src/rv32m1/rv32m1_irq_dispatch.c
##########
@@ -53,11 +53,11 @@ volatile uint32_t * g_current_regs;
****************************************************************************/
LOCATE_ITCM
-void *rv32m1_dispatch_irq(uint32_t vector, uint32_t *regs)
+void *rv32m1_dispatch_irq(uintptr_t vector, uintptr_t *regs)
{
int vec = vector & 0x1f;
int irq = (vector >> 27) + vec;
Review comment:
```suggestion
uintptr_t vec = vector & 0x1f;
uintptr_t irq = (vector >> 27) + vec;
```
##########
File path: arch/risc-v/src/mpfs/mpfs_head.S
##########
@@ -24,7 +24,7 @@
#include <nuttx/config.h>
#include <arch/csr.h>
-#include <arch/rv64gc/irq.h>
+#include <arch/common/irq.h>
Review comment:
```suggestion
#include <arch/irq.h>
```
##########
File path: arch/risc-v/src/rv32m1/rv32m1_head.S
##########
@@ -23,7 +23,7 @@
****************************************************************************/
#include <nuttx/config.h>
-#include <arch/rv32im/irq.h>
+#include <arch/common/irq.h>
Review comment:
```suggestion
#include <arch/irq.h>
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscr...@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
