Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

boards:khadas: adding support for the edge2 #79559

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

Conversation

EstAK
Copy link

@EstAK EstAK commented Oct 8, 2024

Hello, this PR adds support for the rk3588s soc and khadas edge2 board

The board requires to download a tool and a SPL should I add them directly or it is fine as is with links to them ?

@zephyrbot zephyrbot added the area: ARM64 ARM (64-bit) Architecture label Oct 8, 2024
boards/khadas/edge2/CMakeLists.txt Outdated Show resolved Hide resolved
boards/khadas/edge2/Kconfig.khadas_edge2 Outdated Show resolved Hide resolved
boards/khadas/edge2/board.cmake Outdated Show resolved Hide resolved
boards/khadas/edge2/doc/index.rst Outdated Show resolved Hide resolved
soc/rockchip/rk3588s/Kconfig.defconfig.rk3588s Outdated Show resolved Hide resolved
soc/rockchip/soc.yml Outdated Show resolved Hide resolved
@EstAK
Copy link
Author

EstAK commented Oct 11, 2024

@nordicjm hey I did the the changes is it fine now ?

Copy link
Collaborator

@nordicjm nordicjm left a comment

Choose a reason for hiding this comment

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

Make the SoC changes first then add the new soc/board in the second commit

soc/rockchip/rk3588s/Kconfig.defconfig.rk3588s Outdated Show resolved Hide resolved
boards/khadas/edge2/doc/index.rst Outdated Show resolved Hide resolved
@EstAK EstAK force-pushed the edge2 branch 2 times, most recently from 03f76e1 to 008ba4c Compare October 11, 2024 09:06
@EstAK
Copy link
Author

EstAK commented Oct 11, 2024

@nordicjm hey I have reordered the commits (sorry for making a mess with the git)

@EstAK EstAK force-pushed the edge2 branch 3 times, most recently from 172942c to b7cf5ef Compare October 16, 2024 15:38
@EstAK
Copy link
Author

EstAK commented Oct 18, 2024

@nordicjm hello, I have re-ordered the commits and fixed the issues following your inputs,

  • the first commits add the rk35 series and moves the rk3568 under it
  • the second commit adds the khadas edge2 board an its SoC rk3568s

nordicjm
nordicjm previously approved these changes Oct 25, 2024
@EstAK
Copy link
Author

EstAK commented Oct 25, 2024

hey @carlocaione @npitre @povergoing @SgrrZhf can you take a look at this PR ? thank you in advance :)

@kartben
Copy link
Collaborator

kartben commented Dec 10, 2024

@EstAK unfortunately this needs a rebase due to merge conflict. Thanks!

Copy link
Collaborator

@kartben kartben left a comment

Choose a reason for hiding this comment

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

some comments related to documentation (btw, please consider adding an image for this board), and a question re: board name to @nordicjm

@@ -0,0 +1,5 @@
board:
name: khadas_edge2
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we typically wouldn't want the vendor name to prefix the board name, as it's redundant, is it not @nordicjm?
There are some boards that do it, but it's a minority AFAICS. That being said, "edge2" definitely is a very generic name so it doesn't sound like a bad idea to anticipate and namespace it, it's just that I am not sure what the rule/guideline is here.

@@ -0,0 +1,5 @@
board:
name: khadas_edge2
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
name: khadas_edge2
name: khadas_edge2
full_name: Edge2

Comment on lines 1 to 4
.. _khadas_edge2:

Khadas Edge2
#################################
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
.. _khadas_edge2:
Khadas Edge2
#################################
.. zephyr:board:: khadas_edge2


.. code-block:: console

mkimage -C none -A arm64 -O linux -a 0x10000000 -e 0x10000000 -d build/zephyr/zephyr.bin build/zephyr/zephyr.img
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit

Suggested change
mkimage -C none -A arm64 -O linux -a 0x10000000 -e 0x10000000 -d build/zephyr/zephyr.bin build/zephyr/zephyr.img
mkimage -C none -A arm64 -O linux -a 0x10000000 -e 0x10000000 -d build/zephyr/zephyr.bin build/zephyr/zephyr.img


.. code-block:: console

mmc read ${pxefile_addr_r} 0x100000 0x1000; bootm start ${pxefile_addr_r}; bootm loados; bootm go
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
mmc read ${pxefile_addr_r} 0x100000 0x1000; bootm start ${pxefile_addr_r}; bootm loados; bootm go
mmc read ${pxefile_addr_r} 0x100000 0x1000; bootm start ${pxefile_addr_r}; bootm loados; bootm go

Comment on lines 82 to 83
*** Booting Zephyr OS build XXXXXXXXXXXX ***
Hello World! khadas_edge2
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
*** Booting Zephyr OS build XXXXXXXXXXXX ***
Hello World! khadas_edge2
*** Booting Zephyr OS build XXXXXXXXXXXX ***
Hello World! khadas_edge2

Comment on lines 6 to 12
Overview
********

See <https://www.khadas.com/edge2>

Hardware
********

See <https://docs.khadas.com/products/sbc/edge2/hardware/start>
Copy link
Collaborator

Choose a reason for hiding this comment

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

please check rendered output https://builds.zephyrproject.io/zephyr/pr/79559/docs/boards/khadas/edge2/doc/index.html and use the correct ReStructuredText way of adding links

@kartben kartben assigned kartben and carlocaione and unassigned carlocaione Dec 12, 2024
@EstAK
Copy link
Author

EstAK commented Dec 13, 2024

@kartben thank you for your feedback I will try to make the changes as soon as possible :)

added the rk35 series and moved the rk3568 under it

Signed-off-by: Esteban Aguililla Klein <esteban.aguililla.klein.pro@outlook.com>
@kartben
Copy link
Collaborator

kartben commented Jan 6, 2025

@kartben thank you for your feedback I will try to make the changes as soon as possible :)

it looks like you might have addressed the feedback -- can you please re-request a review when you feel this is ready (and it might already be the case), otherwise it's hard for reviewers to know (and be notified!) when it's time to have another look :) Thanks!

@kartben
Copy link
Collaborator

kartben commented Jan 6, 2025

Also, a reminder to check the rendered output for your docs as there might still be some issues, it would seem
https://builds.zephyrproject.io/zephyr/pr/79559/docs/boards/khadas/edge2/doc/index.html
You may also build the docs locally, if that helps

@EstAK EstAK force-pushed the edge2 branch 2 times, most recently from a436716 to 2bd4655 Compare January 8, 2025 16:56
added the rk35 series and moved the rk3568 under it

Signed-off-by: Esteban Aguililla Klein <esteban.aguililla.klein.pro@outlook.com>
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
area: ARM64 ARM (64-bit) Architecture
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants