Skip to content
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

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

WwWangGuan
Copy link
Contributor

拉取/合并请求描述:(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)

  • BSP:bsp/qemu-virt64-riscv生
  • .config:
  • action:

]

当前拉取/合并请求的状态 Intent for your PR

必须选择一项 Choose one (Mandatory):

  • 本拉取/合并请求是一个草稿版本 This PR is for a code-review and is intended to get feedback
  • 本拉取/合并请求是一个成熟版本 This PR is mature, and ready to be integrated into the repo

代码质量 Code Quality:

我在这个拉取/合并请求中已经考虑了 As part of this pull request, I've considered the following:

  • 已经仔细查看过代码改动的对比 Already check the difference between PR and old code
  • 代码风格正确,包括缩进空格,命名及其他风格 Style guide is adhered to, including spacing, naming and other styles
  • 没有垃圾代码,代码尽量精简,不包含#if 0代码,不包含已经被注释了的代码 All redundant code is removed and cleaned up
  • 所有变更均有原因及合理的,并且不会影响到其他软件组件代码或BSP All modifications are justified and not affect other components or BSP
  • 对难懂代码均提供对应的注释 I've commented appropriately where code is tricky
  • 代码是高质量的 Code in this PR is of high quality
  • 已经使用formatting 等源码格式化工具确保格式符合RT-Thread代码规范 This PR complies with RT-Thread code specification
  • 如果是新增bsp, 已经添加ci检查到.github/workflows/bsp_buildings.yml 详细请参考链接BSP自查

WwWangGuan and others added 3 commits November 13, 2024 22:58
在bsp/qemu-virt64-riscv中,RISCV_S_MODE的配置在driver中,应该修改RISCV_S_MODE到libc中
@WwWangGuan
Copy link
Contributor Author

@unicornx 请求review

@unicornx unicornx self-requested a review November 27, 2024 23:46

config RISCV_S_MODE
bool "RT-Thread run in RISC-V S-Mode(supervisor mode)"
depends on BOARD_QEMU_VIRT_RV64
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

libcpu 依赖 bsp? 这个关系反过来了吧?

Copy link
Contributor Author

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中显示配置选项

Copy link
Contributor

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 可见了,这本身没有问题啊

Copy link
Contributor

@unicornx unicornx left a 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/KconfigBOARD_QEMU_VIRT_RV64 下的那些 select。虽然目前似乎也就只有 bsp/qemu-virt64-riscv 一个 bsp 会用到 libcpu/risc-v/virt64, 但为了后面扩展以及保留现在的逻辑,还是让 bsp 来开启这个功能为好。

Q4: RISCV_S_MODE 移到 libcpu 下后建议都统一加上 ARCH 前缀

@unicornx
Copy link
Contributor

Adding @BernardXiong, 我看 readme 的 maintainer 是您,所以加上您来一起 review。看看这个 RISCV_S_MODE 是不是需要改一下,以及移到 libcpu 中是否合适?

@BernardXiong
Copy link
Member

这个可以移到libcpu中,但是不应该设置depends on某个BSP。可以做为一个MODE而存在,但应该移除掉信息显示,然后由某个BSP自行来打开(默认select),或BSP提供额外的选项来做选择。另外,名称上请保持RTT的风格,例如有 ARCH_ARM_BOOTWITH_FLUSH_CACHE

那么这个名称可以定义为 ARCH_RISCV_USING_S_MODEARCH_RISCV_USING_M_MODE等。

@BernardXiong BernardXiong added the discussion This PR/issue needs to be discussed later label Nov 29, 2024
@BernardXiong BernardXiong changed the title fix : libc : change RISCV_S_MODE to libc fix : libc : change RISCV_S_MODE to libcpu Nov 29, 2024
@BernardXiong BernardXiong changed the title fix : libc : change RISCV_S_MODE to libcpu fix : libcpu : change RISCV_S_MODE to libcpu Nov 29, 2024
@unicornx
Copy link
Contributor

commit 的 title 中我建议不要加 fix 这样的前缀。其实 title 里的前缀只要标注涉及的模块就好了。fix 这些词直接作为后面的文字,或者写到 commit 的 message body 中去。例如:

libcpu: move RISCV_S_MODE from bsp to libcpu

Fix the issue ,,,,,,

Signed-off-by:  ......

@unicornx unicornx added the Arch: RISC-V BSP related with risc-v label Dec 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Arch: RISC-V BSP related with risc-v BSP discussion This PR/issue needs to be discussed later libcpu
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug] it's not proper to define RISCV_S_MODE in bsp level
3 participants