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 packaging method #42

Merged
merged 1 commit into from
Jul 5, 2024
Merged

Conversation

flyingcys
Copy link
Owner

@flyingcys flyingcys commented Jun 30, 2024

Use pre-build files to reduce compilation time

More detailed requirements, see RT-Thread#9060.

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

[

  • milkv-duo256m
  • milkv-duo

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

修改打包方式 为预编译文件方式,提高编译速度

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

请提供验证的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

@flyingcys flyingcys changed the title Update packaging method [bsp/cvitek] Update packaging method Jun 30, 2024
@@ -16,6 +16,27 @@ echo ${ROOT_PATH}

get_board_type

echo $BOARD_TYPE
Copy link
Collaborator

@unicornx unicornx Jul 1, 2024

Choose a reason for hiding this comment

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

有个代码的优化改进建议,可能并不适合在这个 pr 里做,只是在 review 代码的时候觉得需要提出来,或许后面可以另外提 pr 改进:

目前 bsp/cvitek/board_env.shget_board_type 的处理感觉有点冗余:
如果 BOARD_CONFIG / BOARD_VALUE / STORAGE_VAUE 可以统一起来代码可以写得简洁些
目前 "milkv-duo" 和 "milkv-duo256m" 这种命名规则和其他的不统一

如果能统一成 ”milkv_duo_sd" 和 "milkv_duo256m_sd" 就和其他的命令规则统一起来了。

Copy link
Owner Author

Choose a reason for hiding this comment

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

这些是沿用的milkv板厂的定义,没有做特殊处理。

Copy link
Collaborator

Choose a reason for hiding this comment

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

这些是沿用的milkv板厂的定义,没有做特殊处理。

ok, 那就后续看看还有机会改进,我去先提个 pr https://github.com/flyingcys/rt-thread/issues/44。

echo $BOARD_TYPE
if [ "${BOARD_TYPE}" == "milkv-duo" ]; then
MV_BOARD_LINK="cv1800b_milkv_duo_sd"
CHIP_ARCH="cv180x"
Copy link
Collaborator

Choose a reason for hiding this comment

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

为何 MV_BOARD_LINK 不用 cv180x ,这样一旦解决了 BOARD_VALUE 的格式统一问题(见上一个代码优化改进建议),我们就可以将 MV_BOARD_LINK 统一为 $CHIP_ARCH_$BOARD_VALUE

Copy link
Owner Author

Choose a reason for hiding this comment

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

MV_BOARD_LINK 这个uboot/fsbl等下面编译的文件对应的,这个是从milk-v仓库里面编译出来的文件夹对应的,文件也只编译后copy过来。为了减少维护成本,所以没有做特殊处理

Copy link
Collaborator

Choose a reason for hiding this comment

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

这个也作为后续的改进处理吧,和 #44 一起。

CHIP_ARCH="cv180x"
elif [ "${BOARD_TYPE}" == "milkv-duo256m" ]; then
MV_BOARD_LINK="cv1812cp_milkv_duo256m_sd"
CHIP_ARCH="cv181x"
Copy link
Collaborator

Choose a reason for hiding this comment

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

同上,为何 MV_BOARD_LINK 不用 cv181x

Copy link
Owner Author

Choose a reason for hiding this comment

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

因为现在使用的是milkv-duo系列板子上编译copy过来的文件夹,沿用了这个名字

Copy link
Collaborator

Choose a reason for hiding this comment

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

同上,这个也作为后续的改进处理吧,和 #44 一起。

Copy link
Collaborator

Choose a reason for hiding this comment

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

我印象里 chip_conf.bin 是通过 chip_conf.py 生成的,我建议不要作为 prebuild 的 bin 提供,而且我们已经在 bsp/cvitek/pre-build/fsbl/plat/cv18xxx 目录下提供了 chip_conf.py

Copy link
Owner Author

Choose a reason for hiding this comment

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

目前小核打包没有用到其他脚本,只用到了小核打包一个命令做文件合成,没有做其他文件的生成,所以没有用到chip_conf.py。
如果觉得这个文件多余可以将chip_conf.py文件删除

Copy link
Collaborator

Choose a reason for hiding this comment

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

那就直接删除 py 文件吧

Copy link
Collaborator

@unicornx unicornx Jul 1, 2024

Choose a reason for hiding this comment

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

test 下是不是只用到了 ddr_param.binempty.bin
其他未用到的文件建议清理掉。

Copy link
Owner Author

Choose a reason for hiding this comment

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

可以删除

Copy link
Collaborator

Choose a reason for hiding this comment

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

以下文件应该没有用到吧。没用到的文件请清理

  • fipsign.py
  • platform_device.c
  • platform.c
  • platform.mk

Copy link
Owner Author

Choose a reason for hiding this comment

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

可以删除

Copy link
Collaborator

@unicornx unicornx Jul 1, 2024

Choose a reason for hiding this comment

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

这个 cache 目录应该不需要提交

Copy link
Owner Author

Choose a reason for hiding this comment

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

可以删除

Copy link
Collaborator

Choose a reason for hiding this comment

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

alios 目录下的文件会用到吗?我觉得应该不需要吧?

Copy link
Owner Author

Choose a reason for hiding this comment

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

可以删除

Copy link
Collaborator

Choose a reason for hiding this comment

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

bsp/cvitek/pre-build/tools/common/image_tool/
这个目录下貌似很多文件都用不到,需要清理。

Copy link
Owner Author

Choose a reason for hiding this comment

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

这个文件在spinorflash和spinandflash打包的时候需要用到,不能删除

Copy link
Collaborator

Choose a reason for hiding this comment

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

这个文件在spinorflash和spinandflash打包的时候需要用到,不能删除

你说的是哪个文件?

我指的是 bsp/cvitek/pre-build/tools/common/image_tool/ 下的好多文件,不是指某个文件呢

ls bsp/cvitek/pre-build/tools/common/image_tool/ -l
total 828
drwxrwxr-x 2 u u   4096  7月  1 10:43 alios
-rwxrwxr-x 1 u u   2349  7月  1 10:43 cimg2raw.py
-rwxrwxr-x 1 u u   5431  7月  1 10:43 create_automount.py
-rw-rw-r-- 1 u u   8807  7月  1 10:43 mkcvipart.py
-rw-rw-r-- 1 u u    892  7月  1 10:43 mk_imgHeader.py
-rw-rw-r-- 1 u u   3814  7月  1 10:43 mk_package.py
-rw-rw-r-- 1 u u   7666  7月  1 10:43 pack_images.py
drwxrwxr-x 2 u u   4096  7月  1 10:43 __pycache__
-rwxrwxr-x 1 u u   5779  7月  1 10:43 raw2cimg.py
-rw-rw-r-- 1 u u 524288  7月  1 10:43 sv_cv182x_2k_pg.bin
-rw-rw-r-- 1 u u 262144  7月  1 10:43 sv_cv182x_4k_pg.bin
-rw-rw-r-- 1 u u   4012  7月  1 10:43 XmlParser.py

这些文件都会用到?

Copy link
Owner Author

Choose a reason for hiding this comment

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

这些文件需要专门去分析,哪些需要,哪些不需要吗?原厂是这么一个文件包的,如果需要针对性的删除,会增加后续管理成本

Copy link
Collaborator

Choose a reason for hiding this comment

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

这些文件需要专门去分析,哪些需要,哪些不需要吗?原厂是这么一个文件包的,如果需要针对性的删除,会增加后续管理成本

ok, 你先尽量清理吧,如果实在搞不清楚的,可以先留着后面再清理。

@unicornx
Copy link
Collaborator

unicornx commented Jul 1, 2024

采用 pre-build 方案后,应该不需要下载 bootloadr 目录了。

bsp/cvitek/board_env.sh 中的 check_bootloader 函数需要去掉。

@unicornx
Copy link
Collaborator

unicornx commented Jul 1, 2024

目前有了 prebuild 目录后,所以我建议将 bsp/cvitek/mkimage 移动到 bsp/cvitek/pre-build/tools 下去。因为这个明显也是一个 prebuild 的可执行文件。

@unicornx
Copy link
Collaborator

unicornx commented Jul 1, 2024

bsp/cvitek/mksdimg.shbsp/cvitek/combine-fip.sh 中有重复的代码, 是否可以合并到 board_env.sh 中去?

get_board_type

echo $BOARD_TYPE
if [ "${BOARD_TYPE}" == "milkv-duo" ]; then
	MV_BOARD_LINK="cv1800b_milkv_duo_sd"
	CHIP_ARCH="cv180x"
elif [ "${BOARD_TYPE}" == "milkv-duo-spinand" ]; then
	MV_BOARD_LINK="cv1800b_milkv_duo_spinand"
	CHIP_ARCH="cv180x"
elif [ "${BOARD_TYPE}" == "milkv-duo-spinor" ]; then
	MV_BOARD_LINK="cv1800b_milkv_duo_spinor"
	CHIP_ARCH="cv180x"
elif [ "${BOARD_TYPE}" == "milkv-duo256m" ]; then
	MV_BOARD_LINK="cv1812cp_milkv_duo256m_sd"
	CHIP_ARCH="cv181x"
elif [ "${BOARD_TYPE}" == "milkv-duo256m-spinand" ]; then
	MV_BOARD_LINK="cv1812cp_milkv_duo256m_spinand"
	CHIP_ARCH="cv181x"
elif [ "${BOARD_TYPE}" == "milkv-duo256m-spinor" ]; then
	MV_BOARD_LINK="cv1812cp_milkv_duo256m_spinor"
	CHIP_ARCH="cv181x"
fi

@unicornx
Copy link
Collaborator

unicornx commented Jul 1, 2024

走读代码时发现两个以前就存在的代码问题,都不大,建议在这个 pr 中一起修正掉:

一个是 bsp/cvitek/c906_little/rtconfig.py, bsp/cvitek/cv18xx_aarch64/rtconfig.pybsp/cvitek/cv18xx_risc-v/rtconfig.py 中都定义了 DUMP_ACTION。但实际上不会执行,建议删掉。

还有一个是 bsp/cvitek/board_env.sh 中的 get_board_type 函数中最后两行 export 前缩进格式有问题,需要对齐。

@unicornx
Copy link
Collaborator

unicornx commented Jul 1, 2024

请确认一下,本次修改对 bsp/cvitek/cv18xx_aarch64 还有影响?

我看 bsp/cvitek/cv18xx_aarch64/rtconfig.py 中也会有调用 mksdimg.sh 的逻辑。

BTW,想问一下现在 arm 核的状态,这个现在是否在积极维护中,我这边其实一直没有怎么关注过这个,但是 build
这边似乎 arm 和 riscv 有耦合,所以想问问。

@flyingcys
Copy link
Owner Author

请确认一下,本次修改对 bsp/cvitek/cv18xx_aarch64 还有影响?

我看 bsp/cvitek/cv18xx_aarch64/rtconfig.py 中也会有调用 mksdimg.sh 的逻辑。

BTW,想问一下现在 arm 核的状态,这个现在是否在积极维护中,我这边其实一直没有怎么关注过这个,但是 build 这边似乎 arm 和 riscv 有耦合,所以想问问。

arm和我这边没有做具体测试和运行,这个bsp一开始也不是我弄的,我这边不是很清楚。

@unicornx
Copy link
Collaborator

unicornx commented Jul 1, 2024

请确认一下,本次修改对 bsp/cvitek/cv18xx_aarch64 还有影响?
我看 bsp/cvitek/cv18xx_aarch64/rtconfig.py 中也会有调用 mksdimg.sh 的逻辑。
BTW,想问一下现在 arm 核的状态,这个现在是否在积极维护中,我这边其实一直没有怎么关注过这个,但是 build 这边似乎 arm 和 riscv 有耦合,所以想问问。

arm和我这边没有做具体测试和运行,这个bsp一开始也不是我弄的,我这边不是很清楚。

就是说 arm 这边还有谁在维护?如果没有人维护,这是一个问题啊,那还不如删除了事 :)

RTT 这边有 maintainer 的概念吗?否则出现无人认领的情况,以后怎么办啊,现在这个 arm 就是一个问题。

Copy link
Collaborator

Choose a reason for hiding this comment

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

fip.bin 文件是不需要的,请删除

Copy link
Owner Author

Choose a reason for hiding this comment

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

可以删除

@unicornx
Copy link
Collaborator

unicornx commented Jul 2, 2024

请确认一下,本次修改对 bsp/cvitek/cv18xx_aarch64 还有影响?
我看 bsp/cvitek/cv18xx_aarch64/rtconfig.py 中也会有调用 mksdimg.sh 的逻辑。
BTW,想问一下现在 arm 核的状态,这个现在是否在积极维护中,我这边其实一直没有怎么关注过这个,但是 build 这边似乎 arm 和 riscv 有耦合,所以想问问。

arm和我这边没有做具体测试和运行,这个bsp一开始也不是我弄的,我这边不是很清楚。

好吧, arm 的事情先放放,这个 pr 先不考虑 arm 的事情。但请在 commit 的 message 中注明。

因为我考虑这个 feature 也是需要回主线的,所以 arm 的事情最终还是要确认下来,我会另外去找熊大他们确认,包括 maintainer 的事情。已经提单跟踪:#45

@flyingcys
Copy link
Owner Author

采用 pre-build 方案后,应该不需要下载 bootloadr 目录了。

bsp/cvitek/board_env.sh 中的 check_bootloader 函数需要去掉。

可以删除

@flyingcys
Copy link
Owner Author

bsp/cvitek/mksdimg.shbsp/cvitek/combine-fip.sh 中有重复的代码, 是否可以合并到 board_env.sh 中去?

get_board_type

echo $BOARD_TYPE
if [ "${BOARD_TYPE}" == "milkv-duo" ]; then
	MV_BOARD_LINK="cv1800b_milkv_duo_sd"
	CHIP_ARCH="cv180x"
elif [ "${BOARD_TYPE}" == "milkv-duo-spinand" ]; then
	MV_BOARD_LINK="cv1800b_milkv_duo_spinand"
	CHIP_ARCH="cv180x"
elif [ "${BOARD_TYPE}" == "milkv-duo-spinor" ]; then
	MV_BOARD_LINK="cv1800b_milkv_duo_spinor"
	CHIP_ARCH="cv180x"
elif [ "${BOARD_TYPE}" == "milkv-duo256m" ]; then
	MV_BOARD_LINK="cv1812cp_milkv_duo256m_sd"
	CHIP_ARCH="cv181x"
elif [ "${BOARD_TYPE}" == "milkv-duo256m-spinand" ]; then
	MV_BOARD_LINK="cv1812cp_milkv_duo256m_spinand"
	CHIP_ARCH="cv181x"
elif [ "${BOARD_TYPE}" == "milkv-duo256m-spinor" ]; then
	MV_BOARD_LINK="cv1812cp_milkv_duo256m_spinor"
	CHIP_ARCH="cv181x"
fi

可以合并

@flyingcys
Copy link
Owner Author

DUMP_ACTION

DUMP_ACTION 在rt-thread中基本上都有,这个不建议做删除了

@flyingcys flyingcys force-pushed the duo-v5.1.0-fip branch 2 times, most recently from 834d681 to 7e72d29 Compare July 4, 2024 14:17
@unicornx
Copy link
Collaborator

unicornx commented Jul 5, 2024

DUMP_ACTION

DUMP_ACTION 在rt-thread中基本上都有,这个不建议做删除了

好吧,先留着,观察一下

Use pre-build files to reduce compilation time

FIXME: this change does not cover arm.

Signed-off-by: flyingcys <flyingcys@163.com>
Reviewed-by: Chen Wang <unicorn_wang@outlook.com>
Tested-by:  Chen Wang <unicorn_wang@outlook.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants