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.

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

bsp: cvitek: fix cvitek/c906_little build warning #9865

Closed
wants to merge 1 commit into from
Closed

bsp: cvitek: fix cvitek/c906_little build warning #9865

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
2 changes: 2 additions & 0 deletions bsp/cvitek/c906_little/board/board.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,8 @@
#define __BOARD_H__

#include <rtconfig.h>
#include "tick.h"
Copy link
Contributor

Choose a reason for hiding this comment

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

我觉得 bsp/cvitek/c906_little/board/tick.h 实际上是不需要的。

tick_isr 和 rt_hw_tick_init 是 定义在 libcpu/risc-v/common64/tick.c 而且是不可重载的。
同时这两个函数已经通过 libcpu/risc-v/common64/tick.h 导出，所以 bsp 应该直接 include libcpu/risc-v/common64/tick.h 这个文件，而不是自己再定义一份 bsp/cvitek/c906_little/board/tick.h 。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

小核现在用的并不是common64中的代码，用的是common+rv64中的代码。这个代码使用与RV64,里面没有对tick做适配，都是每个bsp自己实现
https://github.com/RT-Thread/rt-thread/blob/master/libcpu/risc-v/SConscript

# add common code files
if rtconfig.CPU in common64_arch :
    group += SConscript(os.path.join('common64', 'SConscript'))
else :
    group += SConscript(os.path.join('common', 'SConscript'))

Copy link
Contributor

Choose a reason for hiding this comment

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

小核现在用的并不是common64中的代码

嗯，是我看错了。

Copy link
Contributor

Choose a reason for hiding this comment

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

请将 bsp/cvitek/c906_little/board/tick.h 中的 tick_isr 删掉，这个函数对于 rv64 根本不存在的。其实 bsp/k210 也有类似的问题。

#include "drv_uart.h"

extern rt_uint8_t HeapBase;
extern rt_uint8_t HeapLimit;
Expand Down
3 changes: 1 addition & 2 deletions libcpu/risc-v/common/trap_common.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,6 @@ rt_weak rt_isr_handler_t rt_hw_interrupt_install(int vector, rt_isr_handler_t ha
void *param, const char *name)
{
rt_isr_handler_t old_handler = RT_NULL;
void *user_param = param;

if(vector < ISR_NUMBER)
{
Expand All @@ -74,7 +73,7 @@ rt_weak void rt_rv32_system_irq_handler(rt_uint32_t mcause)
rt_uint32_t exception = !(mcause & 0x80000000);
if(exception)
{
s_stack_frame = (rt_hw_stack_frame_t *)mscratch;
s_stack_frame = (volatile rt_hw_stack_frame_t *)(uintptr_t)mscratch;
rt_show_stack_frame();
}
else
Expand Down
2 changes: 2 additions & 0 deletions libcpu/risc-v/rv64/trap.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,8 @@

#include <encoding.h>

extern int rt_hw_tick_isr(void);
Copy link
Member

Choose a reason for hiding this comment

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

这个可以直接引用头文件嘛？extern声明方式比较偷懒，之前遇到过因为函数参数发生变化，导致每个extern位置都需要改一遍的情况，不利于维护

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  1. 首先这个不是偷懒不偷懒的问题！！！！
  2. 我的理解：libcpu比bsp更底层，底层调用上层实现的.h是不是更不合理。
  3. 如果大家觉得需要调用.h来实现，我没有意见，可以来做

extern void rt_hw_irq_isr(void);
Copy link
Contributor

Choose a reason for hiding this comment

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

针对 rt_hw_tick_isr & rt_hw_irq_isr， 我认为比较好的做法是参考 rt_hw_soft_irq_isr 的做法，即在 libcpu/risc-v/rv64/trap.c 中为它们定义 weak 函数。

定义 weak 函数的目的实际上就是允许不同的 bsp 可以根据自己的需要在自己的 bsp 中重载其实现，如果没有重载则使用 weak 函数。

而使用 extern 方式则会强制要求 bsp 必须实现重载，不够灵活。

Copy link
Contributor Author

@flyingcys flyingcys Jan 10, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

这2个函数，是强制需要bsp实现的。
因为大部分bsp是不需要 rt_hw_soft_irq_isr 这个实现的，SMP下就需要实现，但是rt_hw_tick_isr和rt_hw_irq_isr是必须要的。

Copy link
Contributor

Choose a reason for hiding this comment

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

好吧，那先不搞 weak

Copy link
Member

Choose a reason for hiding this comment

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

但是rt_hw_tick_isr和rt_hw_irq_isr是必须要的。

那么就需要在rthw.h头文件中进行声明，纳入到整体进行管理。可以有多个层级，

image

Copy link
Contributor

@unicornx unicornx Jan 26, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

首先我搜索发现使用 'rv64'(libcpu/risc-v/rv64) 的 bsp 总共有三个：

  • bsp/cvitek/c906_little/rtconfig.py
  • bsp/juicevm/rtconfig.py
  • bsp/k210/rtconfig.py

libcpu/risc-v/rv64 中 handle_trap 采用 weak 方式定义，bsp 可以直接用这个函数，也可以自己定义重载它。
如果是直接用这个函数，需要注意这个函数中又会调用三个函数：

  • rt_hw_soft_irq_isr
  • rt_hw_tick_isr
  • rt_hw_irq_isr

其中在 libcpu/risc-v/rv64 中，rt_hw_soft_irq_isr 有 weak 定义，而其他两个没有，这意味着如果 bsp 没有重载 handle_trap，则自己必须定义 rt_hw_tick_isr 和 rt_hw_irq_isr，这也是 bsp/cvitek/c906_little 的做法，类似的 bsp 还有 bsp/k210，而形如 bsp/juicevm 则是自己实现的 handle_trap。

如此看来，rt_hw_tick_isr 和 rt_hw_irq_isr 并不是所有使用 libcpu/risc-v/rv64 的 bsp 都要必须实现的，bsp 要实现它们的前提是自己没有重载实现 handle_trap。那么我们在 libcpu/risc-v/rv64 中用 extern 修饰 rt_hw_tick_isr 和 rt_hw_irq_isr 也是不对的了。

另外 rt_hw_tick_isr/rt_hw_irq_isr callback 函数看上去也就是 libcpu/risc-v/rv64 在用，不值得将声明放到 ./include/rthw.h 中。

综上所述，我觉得最好的解决方案，还是将 rt_hw_tick_isr/rt_hw_irq_isr 参考 rt_hw_soft_irq_isr 的方式增加 weak 定义。

btw, 我目前觉得 rv64 这个 libcpu 应该标记为 NOT RECOMMEND 了，当然这是后话。


struct exception_stack_frame
{
Expand Down
Loading