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

feat: support ARCH_REMAP_KERNEL on libcpu/c906 #9123

Merged
merged 1 commit into from
Jul 11, 2024

Conversation

polarvid
Copy link
Contributor

@polarvid polarvid commented Jun 30, 2024

拉取/合并请求描述:(PR description)

[

为什么提交这份PR (why to submit this PR)

This change was necessary to enable the remapping of the kernel image to
a high virtual address region on the c906 platform.

你的解决方案是什么 (what is your solution)

Just a trival duplication work from #9067.

Changes:

  • Introduced new configuration options ARCH_REMAP_KERNEL, and
    ARCH_USING_ASID under the ARCH_RISCV64 section.
  • Updated MMU initialization and switching functions to incorporate
    remapping handling.
  • Modified page table setup for proper memory attribute settings.
  • Added support for early memory setup, kernel remapping
  • Added conditional compilation for ASID support in the rt_aspace struct,
    since this is not enable currently for most architecture.

请提供验证的bsp和config (provide the config and bsp)

  • BSP: milkv duo 256, persimUI m7
  • .config:
  • action:

Standard

image

Smart

image

]

当前拉取/合并请求的状态 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

@polarvid polarvid marked this pull request as ready for review June 30, 2024 11:13
@polarvid polarvid requested a review from BernardXiong as a code owner June 30, 2024 11:13
@BernardXiong BernardXiong requested a review from unicornx July 1, 2024 01:50
@polarvid polarvid marked this pull request as draft July 2, 2024 02:22
@@ -20,13 +20,15 @@
#include "page.h"
#include "lwp_arch.h"

/* must defined as 0xFFFFFFC000200000 */
Copy link
Contributor

Choose a reason for hiding this comment

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

这个注释看上去好像对程序员没有什么帮助,建议给出这么改的原因,而不是仅仅说必须要这么改

Copy link
Contributor Author

Choose a reason for hiding this comment

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

这个不是应用程序可以配置的选项,基本来说是 bootloader 决定的。所以实际也没有什么可以解释的。

Copy link
Contributor

Choose a reason for hiding this comment

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

这个不是应用程序可以配置的选项,基本来说是 bootloader 决定的。所以实际也没有什么可以解释的。

so, 看起来更好的解释是:“0xFFFFFFC000200000 is defined by bootloader", 不是吗?:)

另外我看 link_smart.lds 中定义了这个值是 load address

Copy link
Contributor Author

Choose a reason for hiding this comment

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

是的~ 这个会补充一下。

Copy link
Contributor

Choose a reason for hiding this comment

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

建议把这个文件重命名为 drv_pinmix.c。因为根据我的理解(主要是参考 linux 那边),pinctrl 和 pinmux 是两个不同的概念,pinctrl 即 pin control 的缩写,主要指的是对管脚设置譬如上拉,下拉等控制行为,而 pinmux 是 pin multiplex 的缩写,即管脚功能复用。而我们这里应该是指管脚复用,不是吗?

另外,在内核主线树外,我和 @flyingcys 在做一些 pinmux 的开发(感兴趣可以参考 https://github.com/flyingcys/rt-thread/pull/21),只是还没有提交主线上来,相关文件名也是取的 drv_pinmux.c, 所以既然这里已经要加这个文件,建议统一。当然主要的理由还是上面那个。

@unicornx
Copy link
Contributor

unicornx commented Jul 3, 2024

从 PR 的title来看,修改应该尽量局限在 bsp/cvitek 内部,但是我发现这个 pr 中修改了很多 common 的逻辑:

  • components
  • libcpu

这些是对原来 KERNEK_REMAP 修改 #9067 的补充吗?是不是应该以一个单独的 PR 的形式先行提交,然后再提交 bsp/cvitek 的部分?

@polarvid
Copy link
Contributor Author

polarvid commented Jul 3, 2024

是不是应该以一个单独的 PR 的形式先行提交,然后再提交 bsp/cvitek 的部分?

commit 可以拆成多个。但是部分 commits 是需要联合 PR 的。

因为 libcpu 的修改和 bsp 有些东西是完全关联的。而不是解耦的。否则会导致使用异常。

@unicornx
Copy link
Contributor

unicornx commented Jul 3, 2024

@polarvid @BernardXiong

btw, 目前针对 rtt 的 riscv 的修改是否有工作组在推进?我刚刚参与 RTT 开发,对 rtt 的 riscv 方面的整体状态并不是很清楚,是否有渠道,SIG 等类似的组织在讨论这方面的工作,很想参与一下。这样对我理解 RTT 以及 review 这些 pr 或许会有帮助.

Thanks

@unicornx
Copy link
Contributor

unicornx commented Jul 3, 2024

是不是应该以一个单独的 PR 的形式先行提交,然后再提交 bsp/cvitek 的部分?

commit 可以拆成多个。但是部分 commits 是需要联合 PR 的。

因为 libcpu 的修改和 bsp 有些东西是完全关联的。而不是解耦的。否则会导致使用异常。

同意,必要的话,在 一个 pr 中 拆分多个 commit 会更好。

另外建议 commit message 中多写点,帮助大家理解,谢谢。

@polarvid
Copy link
Contributor Author

polarvid commented Jul 3, 2024

目前针对 rtt 的 riscv 的修改是否有工作组在推进?

官方渠道的报名方式在这个文章末尾有 https://mp.weixin.qq.com/s/CVbEQ4pHKVN2wVa_Uuk-bQ 😉

1719974054672

@polarvid polarvid changed the title bsp: enable KERNEL_REMAP for cvitek platform feat: support ARCH_REMAP_KERNEL on libcpu/c906 Jul 4, 2024
@polarvid polarvid added the BSP: Cvitek BSP related with cvitek label Jul 4, 2024
@polarvid polarvid marked this pull request as ready for review July 4, 2024 04:01
@BernardXiong
Copy link
Member

@polarvid @BernardXiong

btw, 目前针对 rtt 的 riscv 的修改是否有工作组在推进?我刚刚参与 RTT 开发,对 rtt 的 riscv 方面的整体状态并不是很清楚,是否有渠道,SIG 等类似的组织在讨论这方面的工作,很想参与一下。这样对我理解 RTT 以及 review 这些 pr 或许会有帮助.

Thanks

唔,看来shell是提供了二维码,我刚看到,还想说,是否拉个群呢~~

@polarvid polarvid mentioned this pull request Jul 5, 2024
4 tasks
@polarvid polarvid requested a review from unicornx July 6, 2024 11:31
@polarvid
Copy link
Contributor Author

polarvid commented Jul 8, 2024

works with persim M7 on commit ec2f44e

This change was necessary to enable the remapping of the kernel image to
a high virtual address region on the c906 platform.

Changes:
- Introduced new configuration options `ARCH_REMAP_KERNEL`, and
  `ARCH_USING_ASID` under the `ARCH_RISCV64` section.
- Updated MMU initialization and switching functions to incorporate
  remapping handling.
- Modified page table setup for proper memory attribute settings.
- Added support for early memory setup, kernel remapping
- Added conditional compilation for ASID support in the `rt_aspace` struct,
  since this is not enable currently for most architecture.

Signed-off-by: Shell <[email protected]>
@polarvid
Copy link
Contributor Author

polarvid commented Jul 8, 2024

Rebased to upstream

@BernardXiong BernardXiong added the 🎯 Focus Should focus on this issue/discussion/pr label Jul 11, 2024
@BernardXiong BernardXiong merged commit beee77f into RT-Thread:master Jul 11, 2024
44 checks passed
BernardXiong pushed a commit that referenced this pull request Jul 11, 2024
feat: [libcpu/c906] support ARCH_REMAP_KERNEL

This change was necessary to enable the remapping of the kernel image to
a high virtual address region on the c906 platform.

Changes:
- Introduced new configuration options `ARCH_REMAP_KERNEL`, and
  `ARCH_USING_ASID` under the `ARCH_RISCV64` section.
- Updated MMU initialization and switching functions to incorporate
  remapping handling.
- Modified page table setup for proper memory attribute settings.
- Added support for early memory setup, kernel remapping
- Added conditional compilation for ASID support in the `rt_aspace` struct,
  since this is not enable currently for most architecture.

Signed-off-by: Shell <[email protected]>
Co-authored-by: Shell <[email protected]>
@unicornx unicornx mentioned this pull request Jul 19, 2024
messigogogo pushed a commit to messigogogo/rt-thread that referenced this pull request Jul 22, 2024
feat: [libcpu/c906] support ARCH_REMAP_KERNEL

This change was necessary to enable the remapping of the kernel image to
a high virtual address region on the c906 platform.

Changes:
- Introduced new configuration options `ARCH_REMAP_KERNEL`, and
  `ARCH_USING_ASID` under the `ARCH_RISCV64` section.
- Updated MMU initialization and switching functions to incorporate
  remapping handling.
- Modified page table setup for proper memory attribute settings.
- Added support for early memory setup, kernel remapping
- Added conditional compilation for ASID support in the `rt_aspace` struct,
  since this is not enable currently for most architecture.

Signed-off-by: Shell <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
BSP: Cvitek BSP related with cvitek 🎯 Focus Should focus on this issue/discussion/pr
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants