You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@nuttx.apache.org by "yf13 (via GitHub)" <gi...@apache.org> on 2023/12/13 04:59:33 UTC
[PR] arch/risc-v: Initial support for CanMV-k230 board [nuttx]
yf13 opened a new pull request, #11379:
URL: https://github.com/apache/nuttx/pull/11379
The code is mainly derived from the NuttX qemu-rv/rv-virt codebase.
Major changes:
- boards/Kconfig: add new BOARD_K230_CANMV
- arch/risc-v/Kconfig: add new CHIP_K230
- arch/risc-v/src/common/riscv_mtimer.c: use 32bit clint access for K230 chip
New additions:
- arch/risc-v/include/k230/: k230 SoC definitions
- arch/risc-v/src/k230/: k230 SoC sources
- boards/risc-v/k230/canmv230/: CanMV-K230 board sources and configs
- Documentation/platforms/risc-v/k230/canmv230/boards/canmv230.md: simple doc
## Summary
Only FLAT build works now.
## Impact
No impact to existing risc-v targets.
## Testing
oestest pass on CanMV-k230.
--
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
Re: [PR] risc-v: Initial support for CanMV-k230 board [nuttx]
Posted by "yf13 (via GitHub)" <gi...@apache.org>.
yf13 commented on code in PR #11379:
URL: https://github.com/apache/nuttx/pull/11379#discussion_r1427535349
##########
arch/risc-v/src/common/riscv_mtimer.c:
##########
@@ -83,7 +83,7 @@ static const struct oneshot_operations_s g_riscv_mtimer_ops =
#ifndef CONFIG_ARCH_USE_S_MODE
static uint64_t riscv_mtimer_get_mtime(struct riscv_mtimer_lowerhalf_s *priv)
{
-#ifdef CONFIG_ARCH_RV64
+#if defined(CONFIG_ARCH_RV64) && !defined(CONFIG_ARCH_CHIP_K230)
Review Comment:
I don't know NuttX well enough, and I need time to learn Kconfig tweaking if I must do it. In general it is a little rushy to include such change with this PR as it will impact other ports that I may not have chance to test. That is why my original suggestion is to take a debt for now and notice common design group to figure out a final solution.
I strongly believe that change deserves its own PR so that to make changes to common part more visible.
--
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
Re: [PR] risc-v: Initial support for CanMV-k230 board [nuttx]
Posted by "yf13 (via GitHub)" <gi...@apache.org>.
yf13 commented on PR #11379:
URL: https://github.com/apache/nuttx/pull/11379#issuecomment-1857294870
@xiaoxiang781216 they've been squashed.
--
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
Re: [PR] risc-v: Initial support for CanMV-k230 board [nuttx]
Posted by "yf13 (via GitHub)" <gi...@apache.org>.
yf13 commented on code in PR #11379:
URL: https://github.com/apache/nuttx/pull/11379#discussion_r1427535349
##########
arch/risc-v/src/common/riscv_mtimer.c:
##########
@@ -83,7 +83,7 @@ static const struct oneshot_operations_s g_riscv_mtimer_ops =
#ifndef CONFIG_ARCH_USE_S_MODE
static uint64_t riscv_mtimer_get_mtime(struct riscv_mtimer_lowerhalf_s *priv)
{
-#ifdef CONFIG_ARCH_RV64
+#if defined(CONFIG_ARCH_RV64) && !defined(CONFIG_ARCH_CHIP_K230)
Review Comment:
I am still new to NuttX thus I need time to learn Kconfig if it must be within this PR.
Meanwhile, I still feel that we are too rushy to include that within the scope of this PR as it may impact other ports that I can't test. That is why I suggested to take a debt and book a ticket like "add explicit MMIO width option for RiscV". I sure can participate that later if needed.
So please reconsider if we can do it later.
--
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
Re: [PR] risc-v: Initial support for CanMV-k230 board [nuttx]
Posted by "yf13 (via GitHub)" <gi...@apache.org>.
yf13 commented on code in PR #11379:
URL: https://github.com/apache/nuttx/pull/11379#discussion_r1426091218
##########
arch/risc-v/src/common/riscv_mtimer.c:
##########
@@ -83,7 +83,7 @@ static const struct oneshot_operations_s g_riscv_mtimer_ops =
#ifndef CONFIG_ARCH_USE_S_MODE
static uint64_t riscv_mtimer_get_mtime(struct riscv_mtimer_lowerhalf_s *priv)
{
-#ifdef CONFIG_ARCH_RV64
+#if defined(CONFIG_ARCH_RV64) && !defined(CONFIG_ARCH_CHIP_K230)
Review Comment:
@xiaoxiang781216 Good catch! This is quick-n-dirty solution as I mentioned in maillist.
The current common design assumes MMIO width is alway same as CPU width. This is not true for chips like K230 (64-bit CPU with 32-bit MMIO only).
Other solutions I can think of are:
a) See if current RiscV layer design already allows overriding this common layer behavior at chip specific layer.
b) Refactoring the common layer so that to use explicit config options for MMIO width choice. However, doing this will need broader consensus as it impacts all Risc-V ports, if MMIO_WIDTH != ARCH_WIDTH also holds for other architecture, then the new option may go beyond RiscV scope.
As I am new with NuttX, I don't know if option [a] holds or not at this time. If it doesn't hold, then I would suggest to take current solution first. Then solve option [b], then clean up this dirty one.
Please teach your suggestions.
--
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
Re: [PR] risc-v: Initial support for CanMV-k230 board [nuttx]
Posted by "yf13 (via GitHub)" <gi...@apache.org>.
yf13 commented on code in PR #11379:
URL: https://github.com/apache/nuttx/pull/11379#discussion_r1427483175
##########
arch/risc-v/src/common/riscv_mtimer.c:
##########
@@ -83,7 +83,7 @@ static const struct oneshot_operations_s g_riscv_mtimer_ops =
#ifndef CONFIG_ARCH_USE_S_MODE
static uint64_t riscv_mtimer_get_mtime(struct riscv_mtimer_lowerhalf_s *priv)
{
-#ifdef CONFIG_ARCH_RV64
+#if defined(CONFIG_ARCH_RV64) && !defined(CONFIG_ARCH_CHIP_K230)
Review Comment:
@xiaoxiang781216
if you are sure that non-RISCV architectures won't have this different MMIO width issue, I am fine to contain the issue within RISCV scope for now. Otherwise later you will see that ARCH_MMIO_NO_64BIT is more proper.
On the other side I am concerned with this NO_64BIT style design as when 128 bit RiscV chip arrives later you will need add MMIO_NO_128BIT.
Maybe we can consider explicit options like::
- two booleans like `ARCH_RV_MMIO_64BIT` and `ARCH_RV_MMIO_32BIT`?
- one enum like `ARCH_RV_MMIO_BITS`?
--
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
Re: [PR] arch/risc-v: Initial support for CanMV-k230 board [nuttx]
Posted by "yf13 (via GitHub)" <gi...@apache.org>.
yf13 commented on PR #11379:
URL: https://github.com/apache/nuttx/pull/11379#issuecomment-1854728747
@xiaoxiang781216 they are sqaushed, please check again. Thanks.
--
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
Re: [PR] risc-v: Initial support for CanMV-k230 board [nuttx]
Posted by "yf13 (via GitHub)" <gi...@apache.org>.
yf13 commented on code in PR #11379:
URL: https://github.com/apache/nuttx/pull/11379#discussion_r1427844470
##########
arch/risc-v/src/common/riscv_mtimer.c:
##########
@@ -83,7 +83,7 @@ static const struct oneshot_operations_s g_riscv_mtimer_ops =
#ifndef CONFIG_ARCH_USE_S_MODE
static uint64_t riscv_mtimer_get_mtime(struct riscv_mtimer_lowerhalf_s *priv)
{
-#ifdef CONFIG_ARCH_RV64
+#if defined(CONFIG_ARCH_RV64) && !defined(CONFIG_ARCH_CHIP_K230)
Review Comment:
@xiaoxiang781216
Suppose we introduce a new int type `ARCH_RV_MMIO_BITS` which defaults to 32 or 64 depending on ARCH_RV32 or ARCH_RV64 respectively. How we set its value to 32 for ARCH_CHIP_K230 in Kconfig?
After some test, it seems that this might work:
```
config ARCH_RV_MMIO_BITS
int "MMIO bit width"
default 32 if ARCH_CHIP_K230 || ARCH_RV32
default 64 if ARCH_RV64
```
Is this ok?
--
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
Re: [PR] risc-v: Initial support for CanMV-k230 board [nuttx]
Posted by "yf13 (via GitHub)" <gi...@apache.org>.
yf13 commented on code in PR #11379:
URL: https://github.com/apache/nuttx/pull/11379#discussion_r1427844470
##########
arch/risc-v/src/common/riscv_mtimer.c:
##########
@@ -83,7 +83,7 @@ static const struct oneshot_operations_s g_riscv_mtimer_ops =
#ifndef CONFIG_ARCH_USE_S_MODE
static uint64_t riscv_mtimer_get_mtime(struct riscv_mtimer_lowerhalf_s *priv)
{
-#ifdef CONFIG_ARCH_RV64
+#if defined(CONFIG_ARCH_RV64) && !defined(CONFIG_ARCH_CHIP_K230)
Review Comment:
@xiaoxiang781216
Suppose we introduce a new int type `ARCH_RV_MMIO_BITS` which defaults to 32 or 64 depending on ARCH_RV32 or ARCH_RV64 respectively. How we override its value to 32 for ARCH_CHIP_K230?
--
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
Re: [PR] risc-v: Initial support for CanMV-k230 board [nuttx]
Posted by "xiaoxiang781216 (via GitHub)" <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #11379:
URL: https://github.com/apache/nuttx/pull/11379#discussion_r1428998617
##########
arch/risc-v/Kconfig:
##########
@@ -260,6 +281,13 @@ config ARCH_RV_ISA_C
bool
default n
+config ARCH_RV_MMIO_BITS
+ int "MMIO bit width"
Review Comment:
```suggestion
int
```
--
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
Re: [PR] risc-v: Initial support for CanMV-k230 board [nuttx]
Posted by "xiaoxiang781216 (via GitHub)" <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #11379:
URL: https://github.com/apache/nuttx/pull/11379#discussion_r1426988713
##########
arch/risc-v/src/common/riscv_mtimer.c:
##########
@@ -83,7 +83,7 @@ static const struct oneshot_operations_s g_riscv_mtimer_ops =
#ifndef CONFIG_ARCH_USE_S_MODE
static uint64_t riscv_mtimer_get_mtime(struct riscv_mtimer_lowerhalf_s *priv)
{
-#ifdef CONFIG_ARCH_RV64
+#if defined(CONFIG_ARCH_RV64) && !defined(CONFIG_ARCH_CHIP_K230)
Review Comment:
I think we can add an option(e.g. ARCH_RV_MMIO_NO_64BIT)
```
config ARCH_RV_MMIO_NO_64BIT
bool
depnds on ARCH_RV64
```
and select ARCH_RV_MMIO_NO_64BIT under ARCH_RV64.
--
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
Re: [PR] risc-v: Initial support for CanMV-k230 board [nuttx]
Posted by "yf13 (via GitHub)" <gi...@apache.org>.
yf13 commented on code in PR #11379:
URL: https://github.com/apache/nuttx/pull/11379#discussion_r1427844470
##########
arch/risc-v/src/common/riscv_mtimer.c:
##########
@@ -83,7 +83,7 @@ static const struct oneshot_operations_s g_riscv_mtimer_ops =
#ifndef CONFIG_ARCH_USE_S_MODE
static uint64_t riscv_mtimer_get_mtime(struct riscv_mtimer_lowerhalf_s *priv)
{
-#ifdef CONFIG_ARCH_RV64
+#if defined(CONFIG_ARCH_RV64) && !defined(CONFIG_ARCH_CHIP_K230)
Review Comment:
@xiaoxiang781216
Suppose we introduce a new int type `ARCH_RV_MMIO_BITS` which defaults to 32 or 64 depending on ARCH_RV32 or ARCH_RV64 respectively. How we override its value to 32 for ARCH_CHIP_K230? I
--
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
Re: [PR] arch/risc-v: Initial support for CanMV-k230 board [nuttx]
Posted by "yf13 (via GitHub)" <gi...@apache.org>.
yf13 commented on PR #11379:
URL: https://github.com/apache/nuttx/pull/11379#issuecomment-1853801896
@xiaoxiang781216 Thanks for the comments!
For documentation issue, I sent an update following `JH7110/Star64` pattern.
For the CI error, I am unsure how to use the `tools/refereh.sh`, as it asked to many questions which I am unsure how to handle correctly. Since I am too new with NuttX, please help me to get through.
--
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
Re: [PR] risc-v: Initial support for CanMV-k230 board [nuttx]
Posted by "yf13 (via GitHub)" <gi...@apache.org>.
yf13 commented on code in PR #11379:
URL: https://github.com/apache/nuttx/pull/11379#discussion_r1426091218
##########
arch/risc-v/src/common/riscv_mtimer.c:
##########
@@ -83,7 +83,7 @@ static const struct oneshot_operations_s g_riscv_mtimer_ops =
#ifndef CONFIG_ARCH_USE_S_MODE
static uint64_t riscv_mtimer_get_mtime(struct riscv_mtimer_lowerhalf_s *priv)
{
-#ifdef CONFIG_ARCH_RV64
+#if defined(CONFIG_ARCH_RV64) && !defined(CONFIG_ARCH_CHIP_K230)
Review Comment:
Good catch! This is quick-n-dirty solution as I mentioned in maillist.
Other solutions I can think of are:
a) See if current RiscV layer design already allows overriding this common layer behavior at chip specific layer.
b) Refactoring the common layer so that to use explicit config options for this CLINT MMIO bus width choice. Current design assumption of "64-bit chips will do 64-bit CLINT MMIO" doesn't hold true for chips like k230. However, doing this will need broader consensus as it impacts all Risc-V ports.
As I am new with NuttX, I don't know if option a holds or not at this time. If it doesn't hold, then I would suggest to take current solution first. Then the common layaer have option b done, then we can clean up the dirty one.
Please teach your suggestions.
##########
arch/risc-v/src/common/riscv_mtimer.c:
##########
@@ -83,7 +83,7 @@ static const struct oneshot_operations_s g_riscv_mtimer_ops =
#ifndef CONFIG_ARCH_USE_S_MODE
static uint64_t riscv_mtimer_get_mtime(struct riscv_mtimer_lowerhalf_s *priv)
{
-#ifdef CONFIG_ARCH_RV64
+#if defined(CONFIG_ARCH_RV64) && !defined(CONFIG_ARCH_CHIP_K230)
Review Comment:
Good catch! This is quick-n-dirty solution as I mentioned in maillist.
Other solutions I can think of are:
a) See if current RiscV layer design already allows overriding this common layer behavior at chip specific layer.
b) Refactoring the common layer so that to use explicit config options for this CLINT MMIO bus width choice. Current design assumption of "64-bit chips will do 64-bit CLINT MMIO" doesn't hold true for chips like k230. However, doing this will need broader consensus as it impacts all Risc-V ports.
As I am new with NuttX, I don't know if option a holds or not at this time. If it doesn't hold, then I would suggest to take current solution first. Then the common layaer have option b done, then we can clean up the dirty one.
Please teach your suggestions.
##########
arch/risc-v/src/common/riscv_mtimer.c:
##########
@@ -83,7 +83,7 @@ static const struct oneshot_operations_s g_riscv_mtimer_ops =
#ifndef CONFIG_ARCH_USE_S_MODE
static uint64_t riscv_mtimer_get_mtime(struct riscv_mtimer_lowerhalf_s *priv)
{
-#ifdef CONFIG_ARCH_RV64
+#if defined(CONFIG_ARCH_RV64) && !defined(CONFIG_ARCH_CHIP_K230)
Review Comment:
Good catch! This is quick-n-dirty solution as I mentioned in maillist.
Other solutions I can think of are:
a) See if current RiscV layer design already allows overriding this common layer behavior at chip specific layer.
b) Refactoring the common layer so that to use explicit config options for this CLINT MMIO bus width choice. Current design assumption of "64-bit chips will do 64-bit CLINT MMIO" doesn't hold true for chips like k230. However, doing this will need broader consensus as it impacts all Risc-V ports.
As I am new with NuttX, I don't know if option a holds or not at this time. If it doesn't hold, then I would suggest to take current solution first. Then the common layaer have option b done, then we can clean up the dirty one.
Please teach your suggestions.
##########
arch/risc-v/src/common/riscv_mtimer.c:
##########
@@ -83,7 +83,7 @@ static const struct oneshot_operations_s g_riscv_mtimer_ops =
#ifndef CONFIG_ARCH_USE_S_MODE
static uint64_t riscv_mtimer_get_mtime(struct riscv_mtimer_lowerhalf_s *priv)
{
-#ifdef CONFIG_ARCH_RV64
+#if defined(CONFIG_ARCH_RV64) && !defined(CONFIG_ARCH_CHIP_K230)
Review Comment:
Good catch! This is quick-n-dirty solution as I mentioned in maillist.
Other solutions I can think of are:
a) See if current RiscV layer design already allows overriding this common layer behavior at chip specific layer.
b) Refactoring the common layer so that to use explicit config options for this CLINT MMIO bus width choice. Current design assumption of "64-bit chips will do 64-bit CLINT MMIO" doesn't hold true for chips like k230. However, doing this will need broader consensus as it impacts all Risc-V ports.
As I am new with NuttX, I don't know if option a holds or not at this time. If it doesn't hold, then I would suggest to take current solution first. Then the common layaer have option b done, then we can clean up the dirty one.
Please teach your suggestions.
##########
arch/risc-v/src/common/riscv_mtimer.c:
##########
@@ -83,7 +83,7 @@ static const struct oneshot_operations_s g_riscv_mtimer_ops =
#ifndef CONFIG_ARCH_USE_S_MODE
static uint64_t riscv_mtimer_get_mtime(struct riscv_mtimer_lowerhalf_s *priv)
{
-#ifdef CONFIG_ARCH_RV64
+#if defined(CONFIG_ARCH_RV64) && !defined(CONFIG_ARCH_CHIP_K230)
Review Comment:
Good catch! This is quick-n-dirty solution as I mentioned in maillist.
Other solutions I can think of are:
a) See if current RiscV layer design already allows overriding this common layer behavior at chip specific layer.
b) Refactoring the common layer so that to use explicit config options for this CLINT MMIO bus width choice. Current design assumption of "64-bit chips will do 64-bit CLINT MMIO" doesn't hold true for chips like k230. However, doing this will need broader consensus as it impacts all Risc-V ports.
As I am new with NuttX, I don't know if option a holds or not at this time. If it doesn't hold, then I would suggest to take current solution first. Then the common layaer have option b done, then we can clean up the dirty one.
Please teach your suggestions.
--
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
Re: [PR] risc-v: Initial support for CanMV-k230 board [nuttx]
Posted by "yf13 (via GitHub)" <gi...@apache.org>.
yf13 commented on code in PR #11379:
URL: https://github.com/apache/nuttx/pull/11379#discussion_r1429039401
##########
arch/risc-v/Kconfig:
##########
@@ -260,6 +281,13 @@ config ARCH_RV_ISA_C
bool
default n
+config ARCH_RV_MMIO_BITS
+ int "MMIO bit width"
Review Comment:
fixed
--
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
Re: [PR] risc-v: Initial support for CanMV-k230 board [nuttx]
Posted by "yf13 (via GitHub)" <gi...@apache.org>.
yf13 commented on code in PR #11379:
URL: https://github.com/apache/nuttx/pull/11379#discussion_r1426091218
##########
arch/risc-v/src/common/riscv_mtimer.c:
##########
@@ -83,7 +83,7 @@ static const struct oneshot_operations_s g_riscv_mtimer_ops =
#ifndef CONFIG_ARCH_USE_S_MODE
static uint64_t riscv_mtimer_get_mtime(struct riscv_mtimer_lowerhalf_s *priv)
{
-#ifdef CONFIG_ARCH_RV64
+#if defined(CONFIG_ARCH_RV64) && !defined(CONFIG_ARCH_CHIP_K230)
Review Comment:
Good catch! This is quick-n-dirty solution as I mentioned in maillist.
Other solutions I can think of are:
a) See if current RiscV layer design already allows overriding this common layer behavior at chip specific layer.
b) Refactoring the common layer so that to use explicit config options for this CLINT MMIO bus width choice. Current design assumption of "64-bit chips will do 64-bit CLINT MMIO" doesn't hold true for chips like k230. However, doing this will need broader consensus as it impacts all Risc-V ports.
As I am new with NuttX, I don't know if option [a] holds or not at this time. If it doesn't hold, then I would suggest to take current solution first. Then solve option [b], then clean up this dirty one.
Please teach your suggestions.
--
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
Re: [PR] risc-v: Initial support for CanMV-k230 board [nuttx]
Posted by "yf13 (via GitHub)" <gi...@apache.org>.
yf13 commented on code in PR #11379:
URL: https://github.com/apache/nuttx/pull/11379#discussion_r1426091218
##########
arch/risc-v/src/common/riscv_mtimer.c:
##########
@@ -83,7 +83,7 @@ static const struct oneshot_operations_s g_riscv_mtimer_ops =
#ifndef CONFIG_ARCH_USE_S_MODE
static uint64_t riscv_mtimer_get_mtime(struct riscv_mtimer_lowerhalf_s *priv)
{
-#ifdef CONFIG_ARCH_RV64
+#if defined(CONFIG_ARCH_RV64) && !defined(CONFIG_ARCH_CHIP_K230)
Review Comment:
Good catch! This is quick-n-dirty solution as I mentioned in maillist.
The current common design assumes MMIO width is alway same as CPU width. This is not true for chips like K230 (64-bit CPU with 32-bit MMIO only).
Other solutions I can think of are:
a) See if current RiscV layer design already allows overriding this common layer behavior at chip specific layer.
b) Refactoring the common layer so that to use explicit config options for MMIO width choice. However, doing this will need broader consensus as it impacts all Risc-V ports, if MMIO_WIDTH != ARCH_WIDTH also holds for other architecture, then the new option may go beyond RiscV scope.
As I am new with NuttX, I don't know if option [a] holds or not at this time. If it doesn't hold, then I would suggest to take current solution first. Then solve option [b], then clean up this dirty one.
Please teach your suggestions.
--
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
Re: [PR] risc-v: Initial support for CanMV-k230 board [nuttx]
Posted by "yf13 (via GitHub)" <gi...@apache.org>.
yf13 commented on code in PR #11379:
URL: https://github.com/apache/nuttx/pull/11379#discussion_r1427535349
##########
arch/risc-v/src/common/riscv_mtimer.c:
##########
@@ -83,7 +83,7 @@ static const struct oneshot_operations_s g_riscv_mtimer_ops =
#ifndef CONFIG_ARCH_USE_S_MODE
static uint64_t riscv_mtimer_get_mtime(struct riscv_mtimer_lowerhalf_s *priv)
{
-#ifdef CONFIG_ARCH_RV64
+#if defined(CONFIG_ARCH_RV64) && !defined(CONFIG_ARCH_CHIP_K230)
Review Comment:
I don't know NuttX well enough, and I need time to learn Kconfig tweaking if I must do it. In general it is a little rushy to include such change with this PR as it will impact other ports that I may not have chance to test. That is why my original suggestion is to take a debt for now and notice common design group to figure out a final solution.
I strongly believe that change deserves its own PR so that to make changes to common part more visible. I still feel its better to accept my current treatment, then create a new refactoring ticket like "add explicit MMIO width option for riscv".
--
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
Re: [PR] risc-v: Initial support for CanMV-k230 board [nuttx]
Posted by "yf13 (via GitHub)" <gi...@apache.org>.
yf13 commented on code in PR #11379:
URL: https://github.com/apache/nuttx/pull/11379#discussion_r1428610566
##########
arch/risc-v/src/common/riscv_mtimer.c:
##########
@@ -83,7 +83,7 @@ static const struct oneshot_operations_s g_riscv_mtimer_ops =
#ifndef CONFIG_ARCH_USE_S_MODE
static uint64_t riscv_mtimer_get_mtime(struct riscv_mtimer_lowerhalf_s *priv)
{
-#ifdef CONFIG_ARCH_RV64
+#if defined(CONFIG_ARCH_RV64) && !defined(CONFIG_ARCH_CHIP_K230)
Review Comment:
@xiaoxiang781216 I've update the PR using newly added `ARCH_RV_MMIO_BITS` config above. I did `ostest` checks with `rv-virt` and `canmv230`, plus manual config checks for a few RiscV ports. Hope there is no unwanted side effects.
Please review and let me know if there is anything else.
--
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
Re: [PR] risc-v: Initial support for CanMV-k230 board [nuttx]
Posted by "yf13 (via GitHub)" <gi...@apache.org>.
yf13 commented on code in PR #11379:
URL: https://github.com/apache/nuttx/pull/11379#discussion_r1427844470
##########
arch/risc-v/src/common/riscv_mtimer.c:
##########
@@ -83,7 +83,7 @@ static const struct oneshot_operations_s g_riscv_mtimer_ops =
#ifndef CONFIG_ARCH_USE_S_MODE
static uint64_t riscv_mtimer_get_mtime(struct riscv_mtimer_lowerhalf_s *priv)
{
-#ifdef CONFIG_ARCH_RV64
+#if defined(CONFIG_ARCH_RV64) && !defined(CONFIG_ARCH_CHIP_K230)
Review Comment:
@xiaoxiang781216
Suppose we introduce a new int type `ARCH_RV_MMIO_BITS` which defaults to 32 or 64 depending on ARCH_RV32 or ARCH_RV64 respectively. How we set its value to 32 for ARCH_CHIP_K230 in Kconfig?
If there is no way to derve in Kconfig, then we have to explicitly define it in `defconfig` file, is this the right way?
--
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
Re: [PR] risc-v: Initial support for CanMV-k230 board [nuttx]
Posted by "yf13 (via GitHub)" <gi...@apache.org>.
yf13 commented on code in PR #11379:
URL: https://github.com/apache/nuttx/pull/11379#discussion_r1427483175
##########
arch/risc-v/src/common/riscv_mtimer.c:
##########
@@ -83,7 +83,7 @@ static const struct oneshot_operations_s g_riscv_mtimer_ops =
#ifndef CONFIG_ARCH_USE_S_MODE
static uint64_t riscv_mtimer_get_mtime(struct riscv_mtimer_lowerhalf_s *priv)
{
-#ifdef CONFIG_ARCH_RV64
+#if defined(CONFIG_ARCH_RV64) && !defined(CONFIG_ARCH_CHIP_K230)
Review Comment:
@xiaoxiang781216
if you are sure that non-RISCV architectures won't have this different MMIO width issue, I am fine to contain the issue within RISCV scope for now. Otherwise later you will see that ARCH_MMIO_NO_64BIT is more proper.
On the other side I am concerned with this NO_64BIT style design as when 128 bit RiscV chip arrives later you will need add MMIO_NO_128BIT.
Maybe we can consider explicit options like::
- two booleans like `ARCH_RV_MMIO_64BIT` and `ARCH_RV_MMIO_32BIT`?
- or one enum like `ARCH_RV_MMIO_BITS`?
--
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
Re: [PR] risc-v: Initial support for CanMV-k230 board [nuttx]
Posted by "xiaoxiang781216 (via GitHub)" <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #11379:
URL: https://github.com/apache/nuttx/pull/11379#discussion_r1427524746
##########
arch/risc-v/src/common/riscv_mtimer.c:
##########
@@ -83,7 +83,7 @@ static const struct oneshot_operations_s g_riscv_mtimer_ops =
#ifndef CONFIG_ARCH_USE_S_MODE
static uint64_t riscv_mtimer_get_mtime(struct riscv_mtimer_lowerhalf_s *priv)
{
-#ifdef CONFIG_ARCH_RV64
+#if defined(CONFIG_ARCH_RV64) && !defined(CONFIG_ARCH_CHIP_K230)
Review Comment:
It's fine to put either the common place or risc-v specific, just provide the change you think it's best.
--
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
Re: [PR] arch/risc-v: Initial support for CanMV-k230 board [nuttx]
Posted by "xiaoxiang781216 (via GitHub)" <gi...@apache.org>.
xiaoxiang781216 commented on PR #11379:
URL: https://github.com/apache/nuttx/pull/11379#issuecomment-1853757164
@yf13 please run `./tools/refresh.sh canmv230/nsh` to fix the ci error:
```
Normalize canmv230/nsh
31d30
< CONFIG_DEBUG_ASSERTIONS=y
44d42
< CONFIG_LIBM_TOOLCHAIN=n
Saving the new configuration file
```
--
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
Re: [PR] risc-v: Initial support for CanMV-k230 board [nuttx]
Posted by "yf13 (via GitHub)" <gi...@apache.org>.
yf13 commented on code in PR #11379:
URL: https://github.com/apache/nuttx/pull/11379#discussion_r1426091218
##########
arch/risc-v/src/common/riscv_mtimer.c:
##########
@@ -83,7 +83,7 @@ static const struct oneshot_operations_s g_riscv_mtimer_ops =
#ifndef CONFIG_ARCH_USE_S_MODE
static uint64_t riscv_mtimer_get_mtime(struct riscv_mtimer_lowerhalf_s *priv)
{
-#ifdef CONFIG_ARCH_RV64
+#if defined(CONFIG_ARCH_RV64) && !defined(CONFIG_ARCH_CHIP_K230)
Review Comment:
@xiaoxiang781216 Good catch! This is quick-n-dirty solution as I mentioned in maillist.
The current common design assumes that MMIO width is alway same as CPU width. This is not true for chips like K230 (64-bit CPU with 32-bit MMIO only).
Other solutions I can think of are:
a) See if current RiscV layer design already allows overriding this common layer behavior at chip specific layer.
b) Refactoring the common layer so that to use explicit config options for MMIO width choice. However, doing this will need broader consensus as it impacts all Risc-V ports. If other architecture also have different MMIO and CPU width, then this option goes beyond RiscV scope, depending on the semantics of "common".
As a NuttX newbie, I don't know if option [a] holds at this time. If not then I would suggest to take a debt first, then have common layer resolve option [b], then we clean up and payback the debt.
Please teach your suggestions.
--
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
Re: [PR] arch/risc-v: Initial support for CanMV-k230 board [nuttx]
Posted by "yf13 (via GitHub)" <gi...@apache.org>.
yf13 commented on PR #11379:
URL: https://github.com/apache/nuttx/pull/11379#issuecomment-1853814597
@xiaoxiang781216 after using the `tools/refresh.sh --silent canmv230/nsh`, I noticed the defconfig was normalized. so I sent another update. Hope this is what you expected.
--
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
Re: [PR] risc-v: Initial support for CanMV-k230 board [nuttx]
Posted by "yf13 (via GitHub)" <gi...@apache.org>.
yf13 commented on code in PR #11379:
URL: https://github.com/apache/nuttx/pull/11379#discussion_r1427844470
##########
arch/risc-v/src/common/riscv_mtimer.c:
##########
@@ -83,7 +83,7 @@ static const struct oneshot_operations_s g_riscv_mtimer_ops =
#ifndef CONFIG_ARCH_USE_S_MODE
static uint64_t riscv_mtimer_get_mtime(struct riscv_mtimer_lowerhalf_s *priv)
{
-#ifdef CONFIG_ARCH_RV64
+#if defined(CONFIG_ARCH_RV64) && !defined(CONFIG_ARCH_CHIP_K230)
Review Comment:
@xiaoxiang781216
Suppose we add a `ARCH_RV_MMIO_BITS` option which defaults to 32 or 64 depending on ARCH_RV32 or ARCH_RV64 respectively. How we set its value to 32 for ARCH_CHIP_K230? It seems that we can't do `select ARCH_RV_MMIO_BITS 32` in Kconfig...
After some test, it seems that this might work:
```
config ARCH_RV_MMIO_BITS
int "MMIO bit width"
default 32 if ARCH_CHIP_K230 || ARCH_RV32
default 64 if ARCH_RV64
```
Is this ok? do you know better ways?
If this Kconfig thing is safe enough, using it in `riscv_mtimer.c` should be straight forward.
--
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
Re: [PR] risc-v: Initial support for CanMV-k230 board [nuttx]
Posted by "yf13 (via GitHub)" <gi...@apache.org>.
yf13 commented on code in PR #11379:
URL: https://github.com/apache/nuttx/pull/11379#discussion_r1426091218
##########
arch/risc-v/src/common/riscv_mtimer.c:
##########
@@ -83,7 +83,7 @@ static const struct oneshot_operations_s g_riscv_mtimer_ops =
#ifndef CONFIG_ARCH_USE_S_MODE
static uint64_t riscv_mtimer_get_mtime(struct riscv_mtimer_lowerhalf_s *priv)
{
-#ifdef CONFIG_ARCH_RV64
+#if defined(CONFIG_ARCH_RV64) && !defined(CONFIG_ARCH_CHIP_K230)
Review Comment:
@xiaoxiang781216 Good catch! This is quick-n-dirty solution as I mentioned in maillist.
The current common design assumes that MMIO width is alway same as CPU width. This is not true for chips like K230 (64-bit CPU with 32-bit MMIO only).
Other solutions I can think of are:
a) See if current RiscV layer design already allows overriding this common layer behavior at chip specific layer.
b) Refactoring the common layer so that to use explicit config options for MMIO width choice. However, doing this will need broader consensus as it impacts all Risc-V ports, if MMIO_WIDTH != ARCH_WIDTH also holds for other architecture, then the new option may go beyond RiscV scope.
As I am new with NuttX, I don't know if option [a] holds or not at this time. If it doesn't hold, then I would suggest to take current solution first. Then solve option [b], then clean up this dirty one.
Please teach your suggestions.
--
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
Re: [PR] risc-v: Initial support for CanMV-k230 board [nuttx]
Posted by "yf13 (via GitHub)" <gi...@apache.org>.
yf13 commented on code in PR #11379:
URL: https://github.com/apache/nuttx/pull/11379#discussion_r1426091218
##########
arch/risc-v/src/common/riscv_mtimer.c:
##########
@@ -83,7 +83,7 @@ static const struct oneshot_operations_s g_riscv_mtimer_ops =
#ifndef CONFIG_ARCH_USE_S_MODE
static uint64_t riscv_mtimer_get_mtime(struct riscv_mtimer_lowerhalf_s *priv)
{
-#ifdef CONFIG_ARCH_RV64
+#if defined(CONFIG_ARCH_RV64) && !defined(CONFIG_ARCH_CHIP_K230)
Review Comment:
Good catch! This is quick-n-dirty solution as I mentioned in maillist.
K230 chip is 64-bit but only supports 32-bit MMIO for CLINT, but current common design assumes MMIO width is alway same as CPU width. This is not always true.
Other solutions I can think of are:
a) See if current RiscV layer design already allows overriding this common layer behavior at chip specific layer.
b) Refactoring the common layer so that to use explicit config options for this CLINT MMIO bus width choice. However, doing this will need broader consensus as it impacts all Risc-V ports.
As I am new with NuttX, I don't know if option [a] holds or not at this time. If it doesn't hold, then I would suggest to take current solution first. Then solve option [b], then clean up this dirty one.
Please teach your suggestions.
--
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
Re: [PR] risc-v: Initial support for CanMV-k230 board [nuttx]
Posted by "xiaoxiang781216 (via GitHub)" <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #11379:
URL: https://github.com/apache/nuttx/pull/11379#discussion_r1426048197
##########
arch/risc-v/Kconfig:
##########
@@ -230,6 +230,27 @@ config ARCH_CHIP_BL808
---help---
Bouffalo Lab BL808 SoC.
+config ARCH_CHIP_K230
+ bool "Kendryte K230"
Review Comment:
all space to tab
--
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
Re: [PR] risc-v: Initial support for CanMV-k230 board [nuttx]
Posted by "yf13 (via GitHub)" <gi...@apache.org>.
yf13 commented on code in PR #11379:
URL: https://github.com/apache/nuttx/pull/11379#discussion_r1427844470
##########
arch/risc-v/src/common/riscv_mtimer.c:
##########
@@ -83,7 +83,7 @@ static const struct oneshot_operations_s g_riscv_mtimer_ops =
#ifndef CONFIG_ARCH_USE_S_MODE
static uint64_t riscv_mtimer_get_mtime(struct riscv_mtimer_lowerhalf_s *priv)
{
-#ifdef CONFIG_ARCH_RV64
+#if defined(CONFIG_ARCH_RV64) && !defined(CONFIG_ARCH_CHIP_K230)
Review Comment:
@xiaoxiang781216
Suppose we introduce a new int type `ARCH_RV_MMIO_BITS` which defaults to 32 or 64 depending on ARCH_RV32 or ARCH_RV64 respectively. How we set its value to 32 for ARCH_CHIP_K230 in Kconfig?
If there is no way to derive in Kconfig, then we have to explicitly set it in `defconfig` file, is this the right way?
--
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
Re: [PR] risc-v: Initial support for CanMV-k230 board [nuttx]
Posted by "yf13 (via GitHub)" <gi...@apache.org>.
yf13 commented on code in PR #11379:
URL: https://github.com/apache/nuttx/pull/11379#discussion_r1426091218
##########
arch/risc-v/src/common/riscv_mtimer.c:
##########
@@ -83,7 +83,7 @@ static const struct oneshot_operations_s g_riscv_mtimer_ops =
#ifndef CONFIG_ARCH_USE_S_MODE
static uint64_t riscv_mtimer_get_mtime(struct riscv_mtimer_lowerhalf_s *priv)
{
-#ifdef CONFIG_ARCH_RV64
+#if defined(CONFIG_ARCH_RV64) && !defined(CONFIG_ARCH_CHIP_K230)
Review Comment:
Good catch! This is quick-n-dirty solution as I mentioned in maillist.
The current common design assumes MMIO width is alway same as CPU width. This is not true for chips like K230 (64-bit CPU with 32-bit MMIO only).
Other solutions I can think of are:
a) See if current RiscV layer design already allows overriding this common layer behavior at chip specific layer.
b) Refactoring the common layer so that to use explicit config options for MMIO width choice. However, doing this will need broader consensus as it impacts all Risc-V ports, if MMIO_WIDTH != ARCH_WIDTH also holds for other architecture, then the new option may affect non-RiscV arch as well.
As I am new with NuttX, I don't know if option [a] holds or not at this time. If it doesn't hold, then I would suggest to take current solution first. Then solve option [b], then clean up this dirty one.
Please teach your suggestions.
--
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
Re: [PR] risc-v: Initial support for CanMV-k230 board [nuttx]
Posted by "yf13 (via GitHub)" <gi...@apache.org>.
yf13 commented on code in PR #11379:
URL: https://github.com/apache/nuttx/pull/11379#discussion_r1427844470
##########
arch/risc-v/src/common/riscv_mtimer.c:
##########
@@ -83,7 +83,7 @@ static const struct oneshot_operations_s g_riscv_mtimer_ops =
#ifndef CONFIG_ARCH_USE_S_MODE
static uint64_t riscv_mtimer_get_mtime(struct riscv_mtimer_lowerhalf_s *priv)
{
-#ifdef CONFIG_ARCH_RV64
+#if defined(CONFIG_ARCH_RV64) && !defined(CONFIG_ARCH_CHIP_K230)
Review Comment:
@xiaoxiang781216
Suppose we add a `ARCH_RV_MMIO_BITS` option which defaults to 32 or 64 depending on ARCH_RV32 or ARCH_RV64 respectively. How we set its value to 32 for ARCH_CHIP_K230? I found `select ARCH_RV_MMIO_BITS 32` is wrong syntax.
After some test, it seems that this might work:
```
config ARCH_RV_MMIO_BITS
int "MMIO bit width"
default 32 if ARCH_CHIP_K230 || ARCH_RV32
default 64 if ARCH_RV64
```
Is this ok? do you know better ways?
If this Kconfig thing is safe enough, using it in `riscv_mtimer.c` should be straight forward.
--
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
Re: [PR] risc-v: Initial support for CanMV-k230 board [nuttx]
Posted by "yf13 (via GitHub)" <gi...@apache.org>.
yf13 commented on code in PR #11379:
URL: https://github.com/apache/nuttx/pull/11379#discussion_r1427535349
##########
arch/risc-v/src/common/riscv_mtimer.c:
##########
@@ -83,7 +83,7 @@ static const struct oneshot_operations_s g_riscv_mtimer_ops =
#ifndef CONFIG_ARCH_USE_S_MODE
static uint64_t riscv_mtimer_get_mtime(struct riscv_mtimer_lowerhalf_s *priv)
{
-#ifdef CONFIG_ARCH_RV64
+#if defined(CONFIG_ARCH_RV64) && !defined(CONFIG_ARCH_CHIP_K230)
Review Comment:
I don't know NuttX well enough, and I need time to learn Kconfig tweaking if I must do it within this PR.
I still feel that we are too rushy to include such change with this PR as it may impact other ports that I can't test. That is why I suggested to take a debt and book a dediacted ticket like "add explicit MMIO width option for RiscV" and notice common design group to figure out a final solution.
So please reconsider.
--
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
Re: [PR] risc-v: Initial support for CanMV-k230 board [nuttx]
Posted by "yf13 (via GitHub)" <gi...@apache.org>.
yf13 commented on code in PR #11379:
URL: https://github.com/apache/nuttx/pull/11379#discussion_r1428610566
##########
arch/risc-v/src/common/riscv_mtimer.c:
##########
@@ -83,7 +83,7 @@ static const struct oneshot_operations_s g_riscv_mtimer_ops =
#ifndef CONFIG_ARCH_USE_S_MODE
static uint64_t riscv_mtimer_get_mtime(struct riscv_mtimer_lowerhalf_s *priv)
{
-#ifdef CONFIG_ARCH_RV64
+#if defined(CONFIG_ARCH_RV64) && !defined(CONFIG_ARCH_CHIP_K230)
Review Comment:
@xiaoxiang781216 I've update the PR using newly added `ARCH_RV_MMIO_BITS` config above.
I did `ostest` checks with `rv-virt` and `canmv230`, plus manual config checks for a few RiscV ports.
Please review and let me know if there is anything else.
--
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
Re: [PR] risc-v: Initial support for CanMV-k230 board [nuttx]
Posted by "xiaoxiang781216 (via GitHub)" <gi...@apache.org>.
xiaoxiang781216 commented on PR #11379:
URL: https://github.com/apache/nuttx/pull/11379#issuecomment-1857254755
please squash your patch into one.
--
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
Re: [PR] risc-v: Initial support for CanMV-k230 board [nuttx]
Posted by "yf13 (via GitHub)" <gi...@apache.org>.
yf13 commented on code in PR #11379:
URL: https://github.com/apache/nuttx/pull/11379#discussion_r1427535349
##########
arch/risc-v/src/common/riscv_mtimer.c:
##########
@@ -83,7 +83,7 @@ static const struct oneshot_operations_s g_riscv_mtimer_ops =
#ifndef CONFIG_ARCH_USE_S_MODE
static uint64_t riscv_mtimer_get_mtime(struct riscv_mtimer_lowerhalf_s *priv)
{
-#ifdef CONFIG_ARCH_RV64
+#if defined(CONFIG_ARCH_RV64) && !defined(CONFIG_ARCH_CHIP_K230)
Review Comment:
I don't know NuttX well enough, and I need time to learn Kconfig tweaking if I must do it within this PR.
I still feel that we are too rushy to include such change with this PR as it may impact other ports that I can't test. That is why I suggested to take a debt now, then use a dediacted ticket like "add explicit MMIO width option for RiscV" and notice common design group to figure out a final solution.
So please reconsider.
--
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
Re: [PR] arch/risc-v: Initial support for CanMV-k230 board [nuttx]
Posted by "xiaoxiang781216 (via GitHub)" <gi...@apache.org>.
xiaoxiang781216 commented on PR #11379:
URL: https://github.com/apache/nuttx/pull/11379#issuecomment-1854378017
@yf13 please squash your fix into the first patch
--
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
Re: [PR] risc-v: Initial support for CanMV-k230 board [nuttx]
Posted by "yf13 (via GitHub)" <gi...@apache.org>.
yf13 commented on code in PR #11379:
URL: https://github.com/apache/nuttx/pull/11379#discussion_r1426091218
##########
arch/risc-v/src/common/riscv_mtimer.c:
##########
@@ -83,7 +83,7 @@ static const struct oneshot_operations_s g_riscv_mtimer_ops =
#ifndef CONFIG_ARCH_USE_S_MODE
static uint64_t riscv_mtimer_get_mtime(struct riscv_mtimer_lowerhalf_s *priv)
{
-#ifdef CONFIG_ARCH_RV64
+#if defined(CONFIG_ARCH_RV64) && !defined(CONFIG_ARCH_CHIP_K230)
Review Comment:
@xiaoxiang781216 Good catch! This is quick-n-dirty solution as I mentioned in maillist.
The current common design assumes that MMIO width is alway same as CPU width. This is not true for chips like K230 (64-bit CPU with 32-bit MMIO only).
Other solutions I can think of are:
a) See if current RiscV layer design already allows overriding this common layer behavior at chip specific layer.
b) Refactoring the common layer so that to use explicit config options for MMIO width choice. However, doing this will need broader consensus as it impacts all Risc-V ports. If other architecture also have different MMIO and CPU width, then this option goes beyond RiscV scope, depending on the semantics of "common".
As I am new with NuttX, I don't know if option [a] holds or not at this time. If not then I would suggest to take the debt with current solution first, then have common layer owener resolve option [b], then we clean up this one to pay the debt.
Please teach your suggestions.
--
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
Re: [PR] risc-v: Initial support for CanMV-k230 board [nuttx]
Posted by "xiaoxiang781216 (via GitHub)" <gi...@apache.org>.
xiaoxiang781216 merged PR #11379:
URL: https://github.com/apache/nuttx/pull/11379
--
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
Re: [PR] risc-v: Initial support for CanMV-k230 board [nuttx]
Posted by "yf13 (via GitHub)" <gi...@apache.org>.
yf13 commented on PR #11379:
URL: https://github.com/apache/nuttx/pull/11379#issuecomment-1858730276
@xiaoxiang781216 `ARCH_RV_MMIO_BITS` option has been used in `riscv_mtimer.c`, please let me know if there is anything else.
--
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
Re: [PR] risc-v: Initial support for CanMV-k230 board [nuttx]
Posted by "yf13 (via GitHub)" <gi...@apache.org>.
yf13 commented on code in PR #11379:
URL: https://github.com/apache/nuttx/pull/11379#discussion_r1427844470
##########
arch/risc-v/src/common/riscv_mtimer.c:
##########
@@ -83,7 +83,7 @@ static const struct oneshot_operations_s g_riscv_mtimer_ops =
#ifndef CONFIG_ARCH_USE_S_MODE
static uint64_t riscv_mtimer_get_mtime(struct riscv_mtimer_lowerhalf_s *priv)
{
-#ifdef CONFIG_ARCH_RV64
+#if defined(CONFIG_ARCH_RV64) && !defined(CONFIG_ARCH_CHIP_K230)
Review Comment:
@xiaoxiang781216
Suppose we introduce a new int type `ARCH_RV_MMIO_BITS` which defaults to 32 or 63 depending on ARCH_RV32 or ARCH_RV64 respectively. How we override its value to 32 for ARCH_CHIP_K230? I
--
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
Re: [PR] risc-v: Initial support for CanMV-k230 board [nuttx]
Posted by "yf13 (via GitHub)" <gi...@apache.org>.
yf13 commented on code in PR #11379:
URL: https://github.com/apache/nuttx/pull/11379#discussion_r1427844470
##########
arch/risc-v/src/common/riscv_mtimer.c:
##########
@@ -83,7 +83,7 @@ static const struct oneshot_operations_s g_riscv_mtimer_ops =
#ifndef CONFIG_ARCH_USE_S_MODE
static uint64_t riscv_mtimer_get_mtime(struct riscv_mtimer_lowerhalf_s *priv)
{
-#ifdef CONFIG_ARCH_RV64
+#if defined(CONFIG_ARCH_RV64) && !defined(CONFIG_ARCH_CHIP_K230)
Review Comment:
@xiaoxiang781216
Suppose we add a `ARCH_RV_MMIO_BITS` option which defaults to 32 or 64 depending on ARCH_RV32 or ARCH_RV64 respectively. How we set its value to 32 for ARCH_CHIP_K230? I found `select ARCH_RV_MMIO_BITS 32` is wrong syntax.
After some test, it seems that this might work:
```
config ARCH_RV_MMIO_BITS
int "MMIO bit width"
default 32 if ARCH_CHIP_K230 || ARCH_RV32
default 64 if ARCH_RV64
```
Is this ok? do you know better ways?
If this Kconfig thing is safe enough, using it in `mtimer_isr.c` should be straight forward.
--
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
Re: [PR] risc-v: Initial support for CanMV-k230 board [nuttx]
Posted by "yf13 (via GitHub)" <gi...@apache.org>.
yf13 commented on code in PR #11379:
URL: https://github.com/apache/nuttx/pull/11379#discussion_r1426091218
##########
arch/risc-v/src/common/riscv_mtimer.c:
##########
@@ -83,7 +83,7 @@ static const struct oneshot_operations_s g_riscv_mtimer_ops =
#ifndef CONFIG_ARCH_USE_S_MODE
static uint64_t riscv_mtimer_get_mtime(struct riscv_mtimer_lowerhalf_s *priv)
{
-#ifdef CONFIG_ARCH_RV64
+#if defined(CONFIG_ARCH_RV64) && !defined(CONFIG_ARCH_CHIP_K230)
Review Comment:
Good catch! This is quick-n-dirty solution as I mentioned in maillist.
The current common design assumes MMIO width is alway same as CPU width. This is not true for chips like K230 (64-bit CPU with 32-bit MMIO only).
Other solutions I can think of are:
a) See if current RiscV layer design already allows overriding this common layer behavior at chip specific layer.
b) Refactoring the common layer so that to use explicit config options for MMIO width choice. However, doing this will need broader consensus as it impacts all Risc-V ports.
As I am new with NuttX, I don't know if option [a] holds or not at this time. If it doesn't hold, then I would suggest to take current solution first. Then solve option [b], then clean up this dirty one.
Please teach your suggestions.
--
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
Re: [PR] risc-v: Initial support for CanMV-k230 board [nuttx]
Posted by "yf13 (via GitHub)" <gi...@apache.org>.
yf13 commented on code in PR #11379:
URL: https://github.com/apache/nuttx/pull/11379#discussion_r1426419958
##########
boards/Kconfig:
##########
@@ -1947,6 +1947,12 @@ config ARCH_BOARD_JH7110_STAR64
This options selects support for NuttX on PINE64 Star64 based
on StarFive JH7110 SoC.
+config ARCH_BOARD_K230_CANMV
Review Comment:
@xiaoxiang781216 I pushed updates for about `space-vs-tab` formating issues. please let me know if you have more feedbacks!
--
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
Re: [PR] risc-v: Initial support for CanMV-k230 board [nuttx]
Posted by "xiaoxiang781216 (via GitHub)" <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #11379:
URL: https://github.com/apache/nuttx/pull/11379#discussion_r1426048326
##########
arch/risc-v/Kconfig:
##########
@@ -281,6 +302,7 @@ config ARCH_CHIP
default "hpm6750" if ARCH_CHIP_HPM6750
default "jh7110" if ARCH_CHIP_JH7110
default "bl808" if ARCH_CHIP_BL808
+ default "k230" if ARCH_CHIP_K230
Review Comment:
ditto
##########
boards/Kconfig:
##########
@@ -1947,6 +1947,12 @@ config ARCH_BOARD_JH7110_STAR64
This options selects support for NuttX on PINE64 Star64 based
on StarFive JH7110 SoC.
+config ARCH_BOARD_K230_CANMV
Review Comment:
ditto
##########
arch/risc-v/src/common/riscv_mtimer.c:
##########
@@ -83,7 +83,7 @@ static const struct oneshot_operations_s g_riscv_mtimer_ops =
#ifndef CONFIG_ARCH_USE_S_MODE
static uint64_t riscv_mtimer_get_mtime(struct riscv_mtimer_lowerhalf_s *priv)
{
-#ifdef CONFIG_ARCH_RV64
+#if defined(CONFIG_ARCH_RV64) && !defined(CONFIG_ARCH_CHIP_K230)
Review Comment:
why add the chip specific check in common code?
--
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
Re: [PR] risc-v: Initial support for CanMV-k230 board [nuttx]
Posted by "yf13 (via GitHub)" <gi...@apache.org>.
yf13 commented on code in PR #11379:
URL: https://github.com/apache/nuttx/pull/11379#discussion_r1427535349
##########
arch/risc-v/src/common/riscv_mtimer.c:
##########
@@ -83,7 +83,7 @@ static const struct oneshot_operations_s g_riscv_mtimer_ops =
#ifndef CONFIG_ARCH_USE_S_MODE
static uint64_t riscv_mtimer_get_mtime(struct riscv_mtimer_lowerhalf_s *priv)
{
-#ifdef CONFIG_ARCH_RV64
+#if defined(CONFIG_ARCH_RV64) && !defined(CONFIG_ARCH_CHIP_K230)
Review Comment:
I don't know NuttX well enough, and I need time to learn Kconfig tweaking if I must do it. In general it is a little rushy to include such change with this PR as it will impact other ports that I may not have chance to test. That is why my original suggestion is to take a debt for now and notice common design group to figure out a final solution.
--
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