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

[bsp][cvitek] update the arm core project #9749

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

liYony
Copy link
Contributor

@liYony liYony commented Dec 7, 2024

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

[

Fixed: #9694

更新 cvitek bsp 的ARM核相关工程文件。

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

  • 解决编译报错问题。
  • 解决固件无法成功运行的问题。

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

  • 编译报错主要是因为对于cvitek单核包含#include <smp_call.h>导致找不到rt_hw_spinlock_t数据类型。
  • 固件无法运行主要是因为,aarch64相关的bsp无论是在运行标准版核Smart对于外设的访问都需要做ioremap。

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

  • BSP:

  • .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自查

@github-actions github-actions bot added BSP BSP: Cvitek BSP related with cvitek Arch: ARM/AArch64 BSP related with arm libcpu labels Dec 7, 2024
@mysterywolf mysterywolf requested a review from unicornx December 8, 2024 02:48
#define DRV_IOUNMAP(addr) rt_iounmap(addr)
#else
#ifdef ARCH_ARM
#include <ioremap.h>
Copy link
Member

Choose a reason for hiding this comment

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

前面 #ifdef RT_USING_SMART 的时候已经包含了这个头文件

Copy link
Member

Choose a reason for hiding this comment

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

这里感觉似乎不那么对的样子。是说, 非smart环境下,aarch64下也需要做ioremap吗?如果是这样,似乎riscv64下也应该做ioremap啊

Copy link
Contributor Author

Choose a reason for hiding this comment

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

image
rv 非smart没开mmu 但是 arm 是否是smart都开mmu了

Copy link
Contributor Author

Choose a reason for hiding this comment

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

所以没开smart rv这边可以不用做ioremap

Copy link
Contributor Author

Choose a reason for hiding this comment

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

image

@unicornx
Copy link
Contributor

unicornx commented Dec 9, 2024

@liYony 请问你这个 pr 是不是为了修改 #9694 这个 issue?
如果是的,请将 PR 和 issue 关联起来,方法很简单,就是在 PR 的描述中加上 Fixed #9696, 具体参考 #9705 的写法如下:
image

BTW: 如果你这个 pr 就是 fix 了 #9694,我就 reassign 给你吧,原来是 assign 给 @Z8MAN8, 但是一直没动静。

@liYony
Copy link
Contributor Author

liYony commented Dec 9, 2024

@liYony 请问你这个 pr 是不是为了修改 #9694 这个 issue? 如果是的,请将 PR 和 issue 关联起来,方法很简单,就是在 PR 的描述中加上 Fixed #9696, 具体参考 #9705 的写法如下: image

BTW: 如果你这个 pr 就是 fix 了 #9694,我就 reassign 给你吧,原来是 assign 给 @Z8MAN8, 但是一直没动静。

ok

@unicornx
Copy link
Contributor

Copy link
Contributor

Choose a reason for hiding this comment

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

这个修改影响了 common 的逻辑,不适合和这个 duo 特定的 pr 一起合入,请另外提一个 pr 并详细说明修改的理由。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

#9771 Accept and Done!

#define DRV_IOREMAP(addr, size) rt_ioremap(addr, size)
#define DRV_IOUNMAP(addr) rt_iounmap(addr)
#else
#define DRV_IOREMAP(addr, size) (addr)
#define DRV_IOUNMAP(addr)
#endif
#endif /* ARCH_ARM */
Copy link
Contributor

Choose a reason for hiding this comment

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

你是想表达对于 ARCH_ARM, 无论是否 smart 都要 ioremap 么?是否可以写成下面这个样子更清楚点:

#if defined RT_USING_SMART || defined ARCH_ARM
#include <ioremap.h>

#define DRV_IOREMAP(addr, size) rt_ioremap(addr, size)
#define DRV_IOUNMAP(addr) rt_iounmap(addr)
#else
#define DRV_IOREMAP(addr, size) (addr)
#define DRV_IOUNMAP(addr)
#endif

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Accept

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.

见上面的 comments

@liYony
Copy link
Contributor Author

liYony commented Dec 10, 2024

此 pr 合并前须先合并 #9771 ,否则还是会存在编译报错

@unicornx
Copy link
Contributor

3 个问题:
1)PR 中的 commit 不要保留开发历史,如果要 review,除非特别情况下需要将本次 PR 分多个 commit 提交,否则请 squash

2) 请补全 commit 信息。具体要求参考:https://github.com/plctlab/plct-rt-thread/tree/notes/0.notes#%E5%A6%82%E4%BD%95%E6%8F%90%E4%BA%A4-git-commit

3) 修改完后如果需要提请第二次 review,请点击 re-request review
image

@unicornx unicornx removed the libcpu label Dec 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Arch: ARM/AArch64 BSP related with arm BSP: Cvitek BSP related with cvitek BSP
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug] bsp/cvitek/cv18xx_aarch64 需要维护
3 participants