[GitHub] [incubator-nuttx] pkarashchenko commented on a change in pull request #5192: arch/risc-v: Merge rv32im and rv64gc into common

[GitBox <gi...@apache.org>]

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.




-- 
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-unsubscribe@nuttx.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org