Skip to content

[bsp/cvitek]为后续使用ioremap做准备#9129

Closed
heyuanjie87 wants to merge 1 commit intoRT-Thread:masterfrom
heyuanjie87:hyj0702
Closed

[bsp/cvitek]为后续使用ioremap做准备#9129
heyuanjie87 wants to merge 1 commit intoRT-Thread:masterfrom
heyuanjie87:hyj0702

Conversation

@heyuanjie87
Copy link
Copy Markdown
Contributor

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

[

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

为了让rv平台也使用ioremap

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

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

]

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

@Rbb666 Rbb666 added the BSP: Cvitek BSP related with cvitek label Jul 2, 2024
@unicornx
Copy link
Copy Markdown
Contributor

unicornx commented Jul 3, 2024

我猜测你提交这些 pr 的主要目的是为了基于 KERNEL_REMAP #9123 的基础上对 cvitek 的驱动做一些适配,所以我建议你的相关 pr 等 #9123 合入后在此基础上再提交你的改动。

@heyuanjie87
Copy link
Copy Markdown
Contributor Author

我猜测你提交这些 pr 的主要目的是为了基于 KERNEL_REMAP #9123 的基础上对 cvitek 的驱动做一些适配,所以我建议你的相关 pr 等 #9123 合入后在此基础上再提交你的改动。

ioremap和kernel remap互不影响的

@polarvid polarvid added the +1 Agree +1 label Jul 4, 2024
@mysterywolf mysterywolf requested a review from BernardXiong July 4, 2024 10:35
@BernardXiong BernardXiong requested a review from unicornx July 5, 2024 01:22
@unicornx
Copy link
Copy Markdown
Contributor

unicornx commented Jul 5, 2024

我猜测你提交这些 pr 的主要目的是为了基于 KERNEL_REMAP #9123 的基础上对 cvitek 的驱动做一些适配,所以我建议你的相关 pr 等 #9123 合入后在此基础上再提交你的改动。

ioremap和kernel remap互不影响的

@heyuanjie87 能否介绍一下,你对 “后续使用ioremap” 有些什么规划,了解一下或许对我 review 你的改动有帮助,谢谢。

@heyuanjie87
Copy link
Copy Markdown
Contributor Author

我猜测你提交这些 pr 的主要目的是为了基于 KERNEL_REMAP #9123 的基础上对 cvitek 的驱动做一些适配,所以我建议你的相关 pr 等 #9123 合入后在此基础上再提交你的改动。

ioremap和kernel remap互不影响的

@heyuanjie87 能否介绍一下,你对 “后续使用ioremap” 有些什么规划,了解一下或许对我 review 你的改动有帮助,谢谢。

因为要让smart运行起来需要对地址空间进行重新划分,ioremap就是为了让驱动的地址空间服从调配

@heyuanjie87
Copy link
Copy Markdown
Contributor Author

我猜测你提交这些 pr 的主要目的是为了基于 KERNEL_REMAP #9123 的基础上对 cvitek 的驱动做一些适配,所以我建议你的相关 pr 等 #9123 合入后在此基础上再提交你的改动。

ioremap和kernel remap互不影响的

@heyuanjie87 能否介绍一下,你对 “后续使用ioremap” 有些什么规划,了解一下或许对我 review 你的改动有帮助,谢谢。

RTT没有为no-mmu的情况定义兼容的ioremap宏,所以这里增加一个头文件来应对不使用mmu的情况

@unicornx
Copy link
Copy Markdown
Contributor

unicornx commented Jul 6, 2024

我猜测你提交这些 pr 的主要目的是为了基于 KERNEL_REMAP #9123 的基础上对 cvitek 的驱动做一些适配,所以我建议你的相关 pr 等 #9123 合入后在此基础上再提交你的改动。

ioremap和kernel remap互不影响的

@heyuanjie87 能否介绍一下,你对 “后续使用ioremap” 有些什么规划,了解一下或许对我 review 你的改动有帮助,谢谢。

RTT没有为no-mmu的情况定义兼容的ioremap宏,所以这里增加一个头文件来应对不使用mmu的情况

所以,这个是不是还是应该作为一个内核级别,针对 ioremap 的 general 的处理,而不是放在 bsp/cvitek 这个层次?否则以后每个 bsp 都要重复写一遍。要不要再考虑一下?如果不是很急着只是要让 bsp/cvitek 跑起来?

另外,还是那句老话,我建议以后 commit 信息多写些修改原因,思路等方便大家 review,也方便以后合入 master 后的后来人学习。其实如果你在 commit 中能写上 “RTT没有为no-mmu的情况定义兼容的ioremap宏,所以这里增加一个头文件来应对不使用mmu的情况” 这句话就好了,具体见 https://github.com/RT-Thread/rt-thread/issues/9136。

@unicornx
Copy link
Copy Markdown
Contributor

unicornx commented Jul 6, 2024

我想起来,正对这个改动,我们在 #9120 中曾经讨论过,但没有形成最终结论。见 #9120 (comment)

我看 @polarvid 曾经建议加一个 Kconfig,这个大家觉得如何?

@heyuanjie87
Copy link
Copy Markdown
Contributor Author

我猜测你提交这些 pr 的主要目的是为了基于 KERNEL_REMAP #9123 的基础上对 cvitek 的驱动做一些适配,所以我建议你的相关 pr 等 #9123 合入后在此基础上再提交你的改动。

ioremap和kernel remap互不影响的

@heyuanjie87 能否介绍一下,你对 “后续使用ioremap” 有些什么规划,了解一下或许对我 review 你的改动有帮助,谢谢。

RTT没有为no-mmu的情况定义兼容的ioremap宏,所以这里增加一个头文件来应对不使用mmu的情况

所以,这个是不是还是应该作为一个内核级别,针对 ioremap 的 general 的处理,而不是放在 bsp/cvitek 这个层次?否则以后每个 bsp 都要重复写一遍。要不要再考虑一下?如果不是很急着只是要让 bsp/cvitek 跑起来?

另外,还是那句老话,我建议以后 commit 信息多写些修改原因,思路等方便大家 review,也方便以后合入 master 后的后来人学习。其实如果你在 commit 中能写上 “RTT没有为no-mmu的情况定义兼容的ioremap宏,所以这里增加一个头文件来应对不使用mmu的情况” 这句话就好了,具体见 https://github.com/RT-Thread/rt-thread/issues/9136。

在libcpu层处理这个问题是最好的了一劳永逸

@heyuanjie87
Copy link
Copy Markdown
Contributor Author

我想起来,正对这个改动,我们在 #9120 中曾经讨论过,但没有形成最终结论。见 #9120 (comment)

我看 @polarvid 曾经建议加一个 Kconfig,这个大家觉得如何?

不赞同再加kconfig,那样配置太多了

@unicornx
Copy link
Copy Markdown
Contributor

unicornx commented Jul 6, 2024

我猜测你提交这些 pr 的主要目的是为了基于 KERNEL_REMAP #9123 的基础上对 cvitek 的驱动做一些适配,所以我建议你的相关 pr 等 #9123 合入后在此基础上再提交你的改动。

ioremap和kernel remap互不影响的

@heyuanjie87 能否介绍一下,你对 “后续使用ioremap” 有些什么规划,了解一下或许对我 review 你的改动有帮助,谢谢。

RTT没有为no-mmu的情况定义兼容的ioremap宏,所以这里增加一个头文件来应对不使用mmu的情况

所以,这个是不是还是应该作为一个内核级别,针对 ioremap 的 general 的处理,而不是放在 bsp/cvitek 这个层次?否则以后每个 bsp 都要重复写一遍。要不要再考虑一下?如果不是很急着只是要让 bsp/cvitek 跑起来?
另外,还是那句老话,我建议以后 commit 信息多写些修改原因,思路等方便大家 review,也方便以后合入 master 后的后来人学习。其实如果你在 commit 中能写上 “RTT没有为no-mmu的情况定义兼容的ioremap宏,所以这里增加一个头文件来应对不使用mmu的情况” 这句话就好了,具体见 https://github.com/RT-Thread/rt-thread/issues/9136。

在libcpu层处理这个问题是最好的了一劳永逸

我对 RTT 理解还不够深入,哪位大佬给个建议? 😊

@polarvid
Copy link
Copy Markdown
Contributor

polarvid commented Jul 6, 2024

不赞同再加kconfig,那样配置太多了

Kconfig 不一定是用户可选的呀。实际上内核很多配置 menuconfig 都看不到,是通过规则使能的。这个只是说有个宏,比如 MM_USING_DUMMY_IOREMAP,然后 BSP 根据规则主动 select 。

@heyuanjie87
Copy link
Copy Markdown
Contributor Author

不赞同再加kconfig,那样配置太多了

Kconfig 不一定是用户可选的呀。实际上内核很多配置 menuconfig 都看不到,是通过规则使能的。这个只是说有个宏,比如 MM_USING_DUMMY_IOREMAP,然后 BSP 根据规则主动 select 。

那加个RT_UING_IOREMAP?,只有当ARCH_MM_MMU也存在的时候才执行remap否则使用兼容的宏

@polarvid
Copy link
Copy Markdown
Contributor

polarvid commented Jul 8, 2024

那加个RT_UING_IOREMAP?,只有当ARCH_MM_MMU也存在的时候才执行remap否则使用兼容的宏

ARCH_MM_MMU 是指硬件存在 mmu,然后会保证 mmu 驱动引入,并且工作。ioremap 是个拓展的内存管理模块,是否使能是软件决定的。

换句话说,

  • 板子上有 mmu -> 需要内存管理组件 -> 选中 ARCH_MM_MMU
  • 驱动实现时只需要建立静态内存映射 -> 由 MM 模块(ARCH_MM_MMU)完成支持。因此可以不使用拓展的 ioremap 支持,或者做一个假 ioremap 保证同一套代码可用
  • 驱动实现时需要建立动态内存映射 -> 选中 MM_USING_IOREMAP

所以,select MM_USING_IOREMAP 应该是在 bsp 目录下操作的。

config BSP_USING_CV18XX
    bool
    select ARCH_RISCV64
    select RT_USING_SYSTEM_WORKQUEUE
    select RT_USING_COMPONENTS_INIT
    select RT_USING_USER_MAIN
    select RT_USING_CACHE
    select ARCH_MM_MMU
    select RT_USING_DEVICE_OPS
    select ARCH_REMAP_KERNEL if RT_USING_SMART
+   select MM_USING_IOREMAP
    default y

@polarvid polarvid mentioned this pull request Jul 8, 2024
4 tasks
@unicornx
Copy link
Copy Markdown
Contributor

unicornx commented Jul 8, 2024

那加个RT_UING_IOREMAP?,只有当ARCH_MM_MMU也存在的时候才执行remap否则使用兼容的宏

ARCH_MM_MMU 是指硬件存在 mmu,然后会保证 mmu 驱动引入,并且工作。ioremap 是个拓展的内存管理模块,是否使能是软件决定的。

换句话说,

  • 板子上有 mmu -> 需要内存管理组件 -> 选中 ARCH_MM_MMU
  • 驱动实现时只需要建立静态内存映射 -> 由 MM 模块(ARCH_MM_MMU)完成支持。因此可以不使用拓展的 ioremap 支持,或者做一个假 ioremap 保证同一套代码可用
  • 驱动实现时需要建立动态内存映射 -> 选中 MM_USING_IOREMAP

所以,select MM_USING_IOREMAP 应该是在 bsp 目录下操作的。

config BSP_USING_CV18XX
    bool
    select ARCH_RISCV64
    select RT_USING_SYSTEM_WORKQUEUE
    select RT_USING_COMPONENTS_INIT
    select RT_USING_USER_MAIN
    select RT_USING_CACHE
    select ARCH_MM_MMU
    select RT_USING_DEVICE_OPS
    select ARCH_REMAP_KERNEL if RT_USING_SMART
+   select MM_USING_IOREMAP
    default y

如上面 @polarvid 的描述,我觉得在 BSP 下加个选项是可行的。
这里 MM_USING_IOREMAP 应该是一个新建的 BSP 级别的配置选项吧?RTT 中配置选项的命名有什么规则么,可以区分是内核级别还是 BSP 级别?

但对这个 patch 中的代码改动,我有个补充改进建议,就是最好在 BSP 级别新定义一个宏或者函数封装 rt_iounmap, 而不是反过来,我们不能改变读代码的人对 rt_iounmap 的含义的理解,上面文字中所谓的 "做一个假 ioremap 保证同一套代码可用" 应该就是我这里说的意思吧?@polarvid

@polarvid
Copy link
Copy Markdown
Contributor

polarvid commented Jul 9, 2024

我们不能改变读代码的人对 rt_iounmap 的含义的理解

赞同。在不带 MMU 的平台看到 ioremap 调用很怪异,而且也容易忽略这个平台确实有个 nommu 的 c906 little。按理说应该是有个类似 get_mmio_region() ,然后根据 mmu、是否采纳动态 mmio 映射等等配置去选择。从逻辑上就顺畅很多。

@heyuanjie87
Copy link
Copy Markdown
Contributor Author

我们不能改变读代码的人对 rt_iounmap 的含义的理解

赞同。在不带 MMU 的平台看到 ioremap 调用很怪异,而且也容易忽略这个平台确实有个 nommu 的 c906 little。按理说应该是有个类似 get_mmio_region() ,然后根据 mmu、是否采纳动态 mmio 映射等等配置去选择。从逻辑上就顺畅很多。

不赞同,空即是色,无既是有,nommu是mmu的特例

@polarvid
Copy link
Copy Markdown
Contributor

polarvid commented Jul 9, 2024

不必把设计的问题脱离实用的角度来讨论。

名称混淆

ioremap() 从名称上理解会执行一次内存映射,在不做映射的场景下,这个名称会引发歧义,使人迷惑。

难以应对变化的需求

“获取外设的访存地址” 这个需求仅仅是在当前内核下,就会被 ARCH_MM_MMU, RT_USING_DM, 使用 ioremap 等多种变量影响。如果简单的通过几个宏,实际上根本无法满足这些需求。

对于 “获取外设的访存地址” 这个需求,nommu/mmu-disable(后面这两种我都概括为 nommu),线性静态映射,非线性动态映射,dm2.0 处理方式会有极大不同。我以最容易被误会的两个,nommu 和线性映射,举例子。表面上看,好像驱动实现在这两种场景下都是拿到外设地址返回就够了。但是对于线性映射来说,这种所谓的外设地址中隐含了非常重要的问题。因为 nommu 中,外设地址不需要软件填写页表就能访问,但是线性映射是依赖于初始化阶段的静态映射的。那问题来了,驱动凭什么知道这个地址被映射过呢?进一步,映射对应的 cache 属性,buffer 属性,shareable 属性,读写执行权限,这些都是什么呢?这些配置在现在的 bsp 全部是通过暗藏的语义决定的。而显然这并不是一种良好的编程实践。所以,对于线性映射的场景,“获取外设地址”并不是简单把物理地址返回给用户这么天真无邪。而是需要查显式的配置,确认这段地址存在明确符合要求的映射。

上面两种情况本身就足够混乱了,而在 dd 2.0 引入后这个问题只会更加复杂。此时再来看,rt_ioremap 这个名字多搞个宏,一把梭哈满足所有场景难道还有合理性吗?

合理的 get_mmio_region 应该是怎样的?

个人认为合理的方式是走类似设备树的模式,完全走配置(哪怕是静态也需要编译期常量的静态配置)。每种驱动首先在编译期注册识别名,需求属性。如果是无设备树模式,还需要提供外设地址范围。如果是使能 mmu 且无设备树,还需要额外提供映射地址。

然后 get_mmio_region(enum device_type type) 直接根据这份配置,返回地址。至于具体实现到底是查表,动态 ioremap,还是走 dm 查 fdt 动态映射,各回各家各找各妈就好。

@heyuanjie87
Copy link
Copy Markdown
Contributor Author

get_mmio_region

get_mmio_region 可以认为是需要调用ioremap的上层接口,在nommu或线性映射的情况下ioremap这个接口又该如何?如果完全禁止这给写驱动代码带来一定麻烦。MM_USING_IOREMAP这个宏完全足够了

@BernardXiong
Copy link
Copy Markdown
Member

不必把设计的问题脱离实用的角度来讨论。

名称混淆

ioremap() 从名称上理解会执行一次内存映射,在不做映射的场景下,这个名称会引发歧义,使人迷惑。

难以应对变化的需求

“获取外设的访存地址” 这个需求仅仅是在当前内核下,就会被 ARCH_MM_MMU, RT_USING_DM, 使用 ioremap 等多种变量影响。如果简单的通过几个宏,实际上根本无法满足这些需求。

对于 “获取外设的访存地址” 这个需求,nommu/mmu-disable(后面这两种我都概括为 nommu),线性静态映射,非线性动态映射,dm2.0 处理方式会有极大不同。我以最容易被误会的两个,nommu 和线性映射,举例子。表面上看,好像驱动实现在这两种场景下都是拿到外设地址返回就够了。但是对于线性映射来说,这种所谓的外设地址中隐含了非常重要的问题。因为 nommu 中,外设地址不需要软件填写页表就能访问,但是线性映射是依赖于初始化阶段的静态映射的。那问题来了,驱动凭什么知道这个地址被映射过呢?进一步,映射对应的 cache 属性,buffer 属性,shareable 属性,读写执行权限,这些都是什么呢?这些配置在现在的 bsp 全部是通过暗藏的语义决定的。而显然这并不是一种良好的编程实践。所以,对于线性映射的场景,“获取外设地址”并不是简单把物理地址返回给用户这么天真无邪。而是需要查显式的配置,确认这段地址存在明确符合要求的映射。

上面两种情况本身就足够混乱了,而在 dd 2.0 引入后这个问题只会更加复杂。此时再来看,rt_ioremap 这个名字多搞个宏,一把梭哈满足所有场景难道还有合理性吗?

合理的 get_mmio_region 应该是怎样的?

个人认为合理的方式是走类似设备树的模式,完全走配置(哪怕是静态也需要编译期常量的静态配置)。每种驱动首先在编译期注册识别名,需求属性。如果是无设备树模式,还需要提供外设地址范围。如果是使能 mmu 且无设备树,还需要额外提供映射地址。

然后 get_mmio_region(enum device_type type) 直接根据这份配置,返回地址。至于具体实现到底是查表,动态 ioremap,还是走 dm 查 fdt 动态映射,各回各家各找各妈就好。

get_mmio_region(enum device_type type) 如果这样的更合理,是否可以统一做这部分的处理?而rt_ioremap退出历史的舞台?

@BernardXiong BernardXiong added discussion This PR/issue needs to be discussed later 🎯 Focus Should focus on this issue/discussion/pr labels Jul 11, 2024
@heyuanjie87 heyuanjie87 deleted the hyj0702 branch July 11, 2024 03:12
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 discussion This PR/issue needs to be discussed later 🎯 Focus Should focus on this issue/discussion/pr +1 Agree +1

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants