-
Notifications
You must be signed in to change notification settings - Fork 5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix : libcpu : change RISCV_S_MODE to libcpu #9718
base: master
Are you sure you want to change the base?
Conversation
在bsp/qemu-virt64-riscv中,RISCV_S_MODE的配置在driver中,应该修改RISCV_S_MODE到libc中
@unicornx 请求review |
|
||
config RISCV_S_MODE | ||
bool "RT-Thread run in RISC-V S-Mode(supervisor mode)" | ||
depends on BOARD_QEMU_VIRT_RV64 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
libcpu 依赖 bsp? 这个关系反过来了吧?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
libcpu 依赖 bsp? 这个关系反过来了吧?
不是依赖,不写这个depend on 会导致RISCV_S_MODE宏定义在所有调用了ARCH_RISCV_64的menuconfig中显示配置选项
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
RISCV_S_MODE
现在移到 libcpu 中就是意味着这个配置对所有 riscv64 可见了,这本身没有问题啊
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
存在以下问题:
Q1:PR 时的 commits 不应该保留开发中的历史信息,除非为了方便 review 或者的确需要分成多个 commit 提交(譬如按照主题提交),其他一般情况下,只保留一个 commit。
Q2: commit message 不符合要求:具体见 https://github.com/plctlab/plct-rt-thread/tree/notes/0.notes#readme
Q3: 正确的依赖关系和层级关系应该是 bsp 依赖内核的 common 模块(譬如libcpu)我建议 RISCV_S_MODE
应该默认为 n,然后在 bsp/qemu-virt64-riscv
中打开,具体可以参考 bsp/qemu-virt64-riscv/Kconfig
中 BOARD_QEMU_VIRT_RV64
下的那些 select。虽然目前似乎也就只有 bsp/qemu-virt64-riscv
一个 bsp 会用到 libcpu/risc-v/virt64
, 但为了后面扩展以及保留现在的逻辑,还是让 bsp 来开启这个功能为好。
Q4: RISCV_S_MODE
移到 libcpu 下后建议都统一加上 ARCH 前缀
Adding @BernardXiong, 我看 readme 的 maintainer 是您,所以加上您来一起 review。看看这个 RISCV_S_MODE 是不是需要改一下,以及移到 libcpu 中是否合适? |
这个可以移到libcpu中,但是不应该设置depends on某个BSP。可以做为一个MODE而存在,但应该移除掉信息显示,然后由某个BSP自行来打开(默认select),或BSP提供额外的选项来做选择。另外,名称上请保持RTT的风格,例如有 那么这个名称可以定义为 |
commit 的 title 中我建议不要加 fix 这样的前缀。其实 title 里的前缀只要标注涉及的模块就好了。fix 这些词直接作为后面的文字,或者写到 commit 的 message body 中去。例如:
|
拉取/合并请求描述:(PR description)
[
为什么提交这份PR (why to submit this PR)
fix #9670
你的解决方案是什么 (what is your solution)
修改libc使之仅对bsp/qemu-virt64-riscv生效
请提供验证的bsp和config (provide the config and bsp)
]
当前拉取/合并请求的状态 Intent for your PR
必须选择一项 Choose one (Mandatory):
代码质量 Code Quality:
我在这个拉取/合并请求中已经考虑了 As part of this pull request, I've considered the following:
#if 0
代码,不包含已经被注释了的代码 All redundant code is removed and cleaned up