Skip to content

[bsp/milkv 仅用于讨论-后续拆分提交]为后续使用ioremap做准备 #9120

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.

Sign up for GitHub

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

Closed
wants to merge 8 commits into from
Closed

[bsp/milkv 仅用于讨论-后续拆分提交]为后续使用ioremap做准备 #9120

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
9871819
[bsp/cvitek]rtos模式下定义ioremap宏
heyuanjie87 Jun 28, 2024
0dd4ceb
Create pinctrl.c
heyuanjie87 Jun 28, 2024
e66b670
移动函数到pinctrl.c
heyuanjie87 Jun 28, 2024
1035b88
是否remap由pinmux_base_ioremap()实现
heyuanjie87 Jun 28, 2024
9cc9e56
使用ioremap
heyuanjie87 Jun 28, 2024
36fb2eb
使用ioremap
heyuanjie87 Jun 28, 2024
c40294c
Update drv_gpio.c
heyuanjie87 Jul 1, 2024
1b6ec69
Update drv_ioremap.h
heyuanjie87 Jul 1, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 0 additions & 12 deletions bsp/cvitek/cv18xx_aarch64/board/board.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,15 +89,3 @@ void rt_hw_board_init(void)
rt_hw_common_setup();
}
#endif /* RT_USING_OFW */

static rt_ubase_t pinmux_base = RT_NULL;

rt_ubase_t pinmux_base_ioremap(void)
{
if (pinmux_base == RT_NULL)
{
pinmux_base = (rt_size_t)rt_ioremap((void*)0x03001000, 0x1000);
}

return pinmux_base;
}
7 changes: 4 additions & 3 deletions bsp/cvitek/drivers/drv_gpio.c
View file
Open in desktop
Copy link
Contributor

Choose a reason for hiding this comment

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

写法建议修改为如下方式。理由，没有必要赋值两次，定义时直接放 BSS，可以有助于减小 bin 文件 size

diff --git a/bsp/cvitek/drivers/drv_gpio.c b/bsp/cvitek/drivers/drv_gpio.c
index aef6b291d7..be24f5dcc4 100644
--- a/bsp/cvitek/drivers/drv_gpio.c
+++ b/bsp/cvitek/drivers/drv_gpio.c
@@ -53,8 +53,8 @@ rt_inline void dwapb_write32(rt_ubase_t addr, rt_uint32_t value)
     HWREG32(addr) = value;
 }
 
-static rt_ubase_t dwapb_gpio_base = DWAPB_GPIOA_BASE;
-static rt_ubase_t dwapb_gpio_base_e = DWAPB_GPIOE_BASE;
+static rt_ubase_t dwapb_gpio_base;
+static rt_ubase_t dwapb_gpio_base_e;
 
 static struct dwapb_event
 {
@@ -303,6 +303,9 @@ static void rt_hw_gpio_isr(int irqno, void *param)
 
 int rt_hw_gpio_init(void)
 {
+    dwapb_gpio_base = (rt_size_t)rt_ioremap(DWAPB_GPIOA_BASE, DWAPB_GPIO_SIZE);
+    dwapb_gpio_base_e = (rt_size_t)rt_ioremap(DWAPB_GPIOE_BASE, DWAPB_GPIO_SIZE);
+
     rt_device_pin_register("gpio", &_dwapb_ops, RT_NULL);
 
 #define INT_INSTALL_GPIO_DEVICE(no)     \

Original file line number Diff line number Diff line change
Expand Up @@ -11,9 +11,7 @@
#include <rtthread.h>
#include <rtdevice.h>
#include <board.h>
#ifdef RT_USING_SMART
#include <ioremap.h>
#endif
#include "drv_ioremap.h"

#ifdef RT_USING_PIN
#include "drv_gpio.h"
Expand Down Expand Up @@ -303,6 +301,9 @@ static void rt_hw_gpio_isr(int irqno, void *param)

int rt_hw_gpio_init(void)
{
dwapb_gpio_base = (rt_size_t)rt_ioremap((void*)dwapb_gpio_base, 0x10000);
dwapb_gpio_base_e = (rt_size_t)rt_ioremap((void*)dwapb_gpio_base_e, 0x10000);

rt_device_pin_register("gpio", &_dwapb_ops, RT_NULL);

#define INT_INSTALL_GPIO_DEVICE(no) \
Expand Down
9 changes: 9 additions & 0 deletions bsp/cvitek/drivers/drv_ioremap.h
View file
Open in desktop
Copy link
Contributor

Choose a reason for hiding this comment

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

drv_ioremap.h 中的逻辑是否可以合并到 ioremap.h 中，即是否存在通用的逻辑，譬如修改 ioremap.h 如下。

不过我这个修改建议影响面可能会比较大，需要 @BernardXiong 熊大评估一下是否可以。

#ifdef RT_USING_SMART

void *rt_ioremap_early(void *paddr, size_t size);
void *rt_ioremap(void *paddr, size_t size);
void *rt_ioremap_nocache(void *paddr, size_t size);
void *rt_ioremap_cached(void *paddr, size_t size);
void *rt_ioremap_wt(void *paddr, size_t size);
void rt_iounmap(volatile void *addr);

extern void *rt_ioremap_start;
extern size_t rt_ioremap_size;

#else

#define rt_ioremap_early
#define rt_ioremap(paddr, size) (paddr)
#define rt_ioremap_nocache(paddr, size) (paddr)
#define rt_ioremap_cached(paddr, size) (paddr)
#define rt_ioremap_wt(paddr, size) (paddr)
#define rt_iounmap(addr)

#endif // RT_USING_SMART

Copy link
Contributor

Choose a reason for hiding this comment

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

arm64 那边非 smart 也会使用 ioremap API。详细参考 DD2.0 的实现。

Copy link
Contributor

Choose a reason for hiding this comment

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

arm64 那边非 smart 也会使用 ioremap API。详细参考 DD2.0 的实现。

哦，是不是说 ioremap 的 API 是否使用取决于是否开启了 mmu，即 ARCH_MM_MMU?

Copy link
Contributor

@polarvid polarvid Jul 2, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

ioremap 确实依赖于 mmu，但是是否启用应该是偏软件层面的选择。如果整体驱动设计上就是基于运行时配置，动态且灵活的，比如 DD2.0 的实现，那么选择 ioremap 就是必然的（因为编译期是不能确定 MMIO 空间的）。否则，就像大多数 rtos 的实现，io 地址访问就是 1：1 线性映射的空间。那确实没必要使用这个功能。

所以是否使用真正的 ioremap 可以单独做一个 Kconfig，由软件来配置。

Copy link
Contributor

Choose a reason for hiding this comment

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

arm64 那边非 smart 也会使用 ioremap API。详细参考 DD2.0 的实现。

我目前对 ioremap 的理解可能有些局限，基于这个 pr 要做的事情来看我的理解就是在开启 mmu 后，我们需要将 io 的物理地址映射到虚拟地址才能访问。那么我的疑问是，难道这个不是开启 mmu 后必选的吗？难道还有什么情况下开了 mmu 后我们依然可以直接用物理地址来访问 io？

此外，我对 @polarvid 上面所说的 ”如果整体驱动设计上就是基于运行时配置，动态且灵活的，比如 DD2.0 的实现，那么选择 ioremap 就是必然的“ 这段描述还不太理解，以及 dd2.0 是什么我还不了解，能否再详细说明一下？这里的 ”运行时配置“ 和 io 地址 remap 是什么关系？

Thanks，

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的nommu都补充一份ioremap.h是最方便的了

Copy link
Contributor

Choose a reason for hiding this comment

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

给libcpu的nommu都补充一份ioremap.h是最方便的了

没看懂什么意思啊 :(

Copy link
Contributor

@polarvid polarvid Jul 2, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

ioremap 不是创建虚拟地址映射唯一的方法。具体来说内核支持线性映射和非线性映射，ioremap 从属于非线性映射。因为有两种算法，所以才说选择 ioremap 是软件决定的。更具体地说，选择什么方式是设备模型的设计决定的，所以才提到 DD2.0 框架。

至于为什么、是什么，说来话长了……

Copy link
Contributor Author

Choose a reason for hiding this comment

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

可以在支持mmu这类芯片的驱动里都用上ioremap接口，至于咋映射就交给接口决定，情况复杂了驱动里处理起来麻烦

Copy link
Member

Choose a reason for hiding this comment

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

dd 2.0，指的是device driver v2.0，相比较原device driver而言。主要来说，它尝试使用设备树了。

至于ioremap怎么怎么着的，我也都弄得不太清楚了。对于ioremap，我的建议是能理顺，逻辑上通顺。反逻辑的就很别扭。

"在开启 mmu 后，我们需要将 io 的物理地址映射到虚拟地址才能访问。" 从这个角度来说，是建议当使用了mmu时，就需要有一份ioremap的，即使这份地址映射是 1:1 的。不知道大家对这份语义是否认同

Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
#pragma once

#include <rtconfig.h>

#ifdef RT_USING_SMART
#include <ioremap.h>
#else
#define rt_ioremap(a, s) (a)
#endif
12 changes: 2 additions & 10 deletions bsp/cvitek/drivers/drv_uart.c
View file
Open in desktop
Copy link
Contributor

@unicornx unicornx Jul 2, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

和上面对 drv_gpio.c 的思路一样，实际上我觉得目前 drv_uart.c 代码中定义全局变量用宏技巧并没有太大优势，建议统一改成类似如下, 不仅可以优化 bin 的 size，可读性也会好点：

#ifdef BSP_USING_UART0
static struct hw_uart_device _uart0_device;
#endif
......

int rt_hw_uart_init(void)
{
    ....

#ifdef BSP_USING_UART0
    pinmux_config(BSP_UART0_RX_PINNAME, UART0_RX, pinname_whitelist_uart0_rx);
    pinmux_config(BSP_UART0_TX_PINNAME, UART0_TX, pinname_whitelist_uart0_tx);
    BSP_INSTALL_UART_DEVICE(0);
    uart->hw_base = (rt_size_t)rt_ioremap(UART0_BASE, 0x10000);
    uart->irqno = UART0_IRQ;
#endif
	......
}

Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,8 @@
#define DBG_LVL DBG_WARNING
#include <rtdbg.h>

#include "drv_ioremap.h"

/*
* Divide positive or negative dividend by positive divisor and round
* to closest integer. Result is undefined for negative divisors and
Expand Down Expand Up @@ -252,45 +254,35 @@ int rt_hw_uart_init(void)
PINMUX_CONFIG(UART0_RX, UART0_RX);
PINMUX_CONFIG(UART0_TX, UART0_TX);
BSP_INSTALL_UART_DEVICE(0);
#if defined(ARCH_ARM)
uart->hw_base = (rt_size_t)rt_ioremap((void*)uart->hw_base, 0x10000);
#endif /* defined(ARCH_ARM) */
#endif

#ifdef RT_USING_UART1
PINMUX_CONFIG(IIC0_SDA, UART1_RX);
PINMUX_CONFIG(IIC0_SCL, UART1_TX);
BSP_INSTALL_UART_DEVICE(1);
#if defined(ARCH_ARM)
uart->hw_base = (rt_size_t)rt_ioremap((void*)uart->hw_base, 0x10000);
#endif /* defined(ARCH_ARM) */
#endif

#ifdef RT_USING_UART2
PINMUX_CONFIG(SD1_D1, UART2_RX);
PINMUX_CONFIG(SD1_D2, UART2_TX);
BSP_INSTALL_UART_DEVICE(2);
#if defined(ARCH_ARM)
uart->hw_base = (rt_size_t)rt_ioremap((void*)uart->hw_base, 0x10000);
#endif /* defined(ARCH_ARM) */
#endif

#ifdef RT_USING_UART3
PINMUX_CONFIG(SD1_D1, UART3_RX);
PINMUX_CONFIG(SD1_D2, UART3_TX);
BSP_INSTALL_UART_DEVICE(3);
#if defined(ARCH_ARM)
uart->hw_base = (rt_size_t)rt_ioremap((void*)uart->hw_base, 0x10000);
#endif /* defined(ARCH_ARM) */
#endif

#ifdef RT_USING_UART4
PINMUX_CONFIG(SD1_GP0, UART4_RX);
PINMUX_CONFIG(SD1_GP1, UART4_TX);
BSP_INSTALL_UART_DEVICE(4);
#if defined(ARCH_ARM)
uart->hw_base = (rt_size_t)rt_ioremap((void*)uart->hw_base, 0x10000);
#endif /* defined(ARCH_ARM) */
#endif

return 0;
Expand Down
5 changes: 1 addition & 4 deletions bsp/cvitek/drivers/libraries/cv181x/pinctrl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,12 +23,9 @@
#define PAD_MIPI_TXM0__MIPI_TXM0 0
#define PAD_MIPI_TXP0__MIPI_TXP0 0

#if defined(ARCH_ARM)
extern rt_ubase_t pinmux_base_ioremap(void);
#define PINMUX_BASE pinmux_base_ioremap()
Copy link
Contributor

Choose a reason for hiding this comment

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

对 PINMUX_BASE 的访问会很多，可以搜一下 bsp/cvitek/ 下，我发现除了涉及 PINMUX_CONFIG 外，在 sdmmc 驱动中，很多寄存器的定义都是相对于 PINMUX_BASE 的，见 bsp/cvitek/drivers/libraries/sdif/dw_sdmmc.h。如果将 PINMUX_BASE 定义为调用 pinmux_base_ioremap()，效率会降低很多。

所以我这里建议：是否可以将 PINMUX_BASE 定义为 pinmux_base 全局变量, 然后在 board 初始化阶段，确保在早于 uart 初始化之前，就把 pinmux 给初始化了。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

PINMUX_BASE这个暂时保留，这次提交主要是想不太多修改原来代码基础上使用ioremap，效率问题可以后面修改

#else
#define PINMUX_BASE 0x03001000
#endif /* defined(ARCH_ARM) */

#define PINMUX_MASK(PIN_NAME) FMUX_GPIO_FUNCSEL_##PIN_NAME##_MASK
#define PINMUX_OFFSET(PIN_NAME) FMUX_GPIO_FUNCSEL_##PIN_NAME##_OFFSET
#define PINMUX_VALUE(PIN_NAME, FUNC_NAME) PIN_NAME##__##FUNC_NAME
Expand Down
14 changes: 14 additions & 0 deletions bsp/cvitek/drivers/pinctrl.c
View file
Open in desktop
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, 所以既然这里已经要加这个文件，建议统一。当然主要的理由还是上面那个。

Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
#include <rtdef.h>
#include "drv_ioremap.h"

static rt_ubase_t pinmux_base = RT_NULL;

rt_ubase_t pinmux_base_ioremap(void)
{
if (pinmux_base == RT_NULL)
{
pinmux_base = (rt_size_t)rt_ioremap((void*)0x03001000, 0x1000);
}

return pinmux_base;
}
Loading