Skip to content

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
@unicornx
Copy link
Contributor

unicornx commented Jan 8, 2025

I have taken over this work and submitted another PR #9887.

This PR can be dropped now.

@unicornx unicornx closed this Jan 8, 2025
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