Skip to content

boards: m5stack: cores3: Add CoreS3 SE variant #79765

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

Jump to bottom
Merged
merged 2 commits into from
Dec 3, 2024
Merged

boards: m5stack: cores3: Add CoreS3 SE variant #79765

merged 2 commits into from
Dec 3, 2024

Conversation

@soburi
Copy link
Member

@soburi soburi commented Oct 14, 2024 •
edited
Loading

Add se variant to support the low-cost CoreS3 SE.

Also add grove and mbus definitions and enable the peripherals.

@soburi soburi added the DNM This PR should not be merged (Do Not Merge) label Oct 14, 2024
@zephyrbot zephyrbot added area: MFD area: LED Label to identify LED subsystem labels Oct 14, 2024
This was referenced Oct 14, 2024
Merged
Closed
@soburi soburi force-pushed the m5stack_cores3_rtc_tf_led branch 7 times, most recently from 83b4d2b to a3d11e3 Compare October 17, 2024 06:43
@soburi soburi force-pushed the m5stack_cores3_rtc_tf_led branch from a3d11e3 to 26c02e3 Compare November 14, 2024 03:33
@soburi soburi added platform: ESP32 Espressif ESP32 and removed area: LED Label to identify LED subsystem area: Shields Shields (add-on boards) area: MFD area: Devicetree labels Nov 21, 2024
@soburi
Copy link
Member Author

soburi commented Nov 22, 2024

Please split this PR into separate PRs for each of the added features (see https://docs.zephyrproject.org/latest/contribute/contributor_expectations.html#defining-smaller-prs).

I split and simplified this PR.
I will address the rest of the parts after #80112 is merged.

henrikbrixandersen
henrikbrixandersen reviewed Nov 22, 2024
View reviewed changes
Copy link
Member

@henrikbrixandersen henrikbrixandersen left a comment

Choose a reason for hiding this comment

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

Is this really a board "revision" and not a board "variant"?

@soburi soburi force-pushed the m5stack_cores3_rtc_tf_led branch 2 times, most recently from fcb28b5 to e816e64 Compare November 24, 2024 02:24
@soburi soburi changed the title boards: m5stack: cores3: Add revision for supporting CoreS3 SE boards: m5stack: cores3: Add CoreS3 SE variant Nov 24, 2024
@soburi soburi force-pushed the m5stack_cores3_rtc_tf_led branch from e816e64 to 4c278b4 Compare November 24, 2024 02:28
@soburi
Copy link
Member Author

soburi commented Nov 24, 2024

Is this really a board "revision" and not a board "variant"?

That's right. I fixed it.

@soburi soburi force-pushed the m5stack_cores3_rtc_tf_led branch from 4c278b4 to b9c7a58 Compare November 24, 2024 03:04
sylvioalves
sylvioalves reviewed Nov 24, 2024
View reviewed changes
boards/m5stack/m5stack_cores3/m5stack_cores3_esp32s3_procpu_se.yaml Outdated
Copy link
Contributor

@sylvioalves sylvioalves Nov 24, 2024

Choose a reason for hiding this comment

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

Any reason to let gpio and pinmux in ignore_tags?

Copy link
Member Author

@soburi soburi Nov 24, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@sylvioalves
I added a commit to add a connector definition and enable the required peripherals.
I also updated the yaml definition accordingly. Could you confirm it?

Copy link

@marekmatej marekmatej Dec 2, 2024

Choose a reason for hiding this comment

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

I think it would be much better to amend the commit by moving this part and squash it with the last commit where you do the other changes.

Copy link
Member Author

@soburi soburi Dec 2, 2024

Choose a reason for hiding this comment

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

I am done squashing commits.
And according to this #78718 (comment)
discussion, I removed the ignore_tags: section.

@soburi soburi requested a review from sylvioalves November 25, 2024 02:25
@soburi
Copy link
Member Author

soburi commented Nov 30, 2024

@sylvioalves @marekmatej
Could you review it if you can take a while?

marekmatej
marekmatej requested changes Dec 2, 2024
View reviewed changes
Copy link

@marekmatej marekmatej left a comment

Choose a reason for hiding this comment

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

The file rename part I leave up to you to decide. The yaml changes should be unified.

boards/m5stack/m5stack_cores3/m5stack_cores3_esp32s3_appcpu.dts Outdated
Copy link

@marekmatej marekmatej Dec 2, 2024

Choose a reason for hiding this comment

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

any reason for renaming this file? IMO it does not need to follow the SoC structure as when addressing the build target. The same applies to all renamed files.

Copy link
Member Author

@soburi soburi Dec 2, 2024

Choose a reason for hiding this comment

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

The "Board Porting Guide"
https://docs.zephyrproject.org/latest/hardware/porting/board_porting.html
says, the filename should be,

As described in the Write your devicetree and Write Kconfig files sections the board default configuration is created from the files <board>.dts / <board>_<qualifiers>.dts and <board>defconfig / <board><qualifiers>_defconfig. When building for a specific board revision, the above files are used as a starting point and the following board files will be used in addition:

and here, the <qualifiers> is,

Board qualifiers of the form <soc>/<cpucluster>/<variant> are normalized so that / is replaced with _ when used for filenames, for example: soc1/foo becomes soc1_foo when used in filenames.

So, my understanding is that the original file name is no problem for it is worked but does not follow the specs.
If following the spec requires the SoC name, "esp32s3" in this case, it is needed.

I am fixing it to comply with the specifications, but I know this is not a convention in the esp32 domain, so I will revert it if that is better.

Copy link
Member Author

@soburi soburi Dec 2, 2024

Choose a reason for hiding this comment

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

I changed the file name back to the original.
Complying with the spec may be necessary in the long term, but it would be better to do this together with the other boards at that time.

Copy link

@marekmatej marekmatej Dec 3, 2024

Choose a reason for hiding this comment

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

I got your point! I too think that it would be better to address the file name format for a group of boards. Thanks for clarifying your intention and bringing up the documentation bits.

boards/m5stack/m5stack_cores3/m5stack_cores3_esp32s3_procpu_se.yaml Outdated
Copy link

@marekmatej marekmatej Dec 2, 2024

Choose a reason for hiding this comment

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

I think it would be much better to amend the commit by moving this part and squash it with the last commit where you do the other changes.

soburi added 2 commits December 3, 2024 08:40
@soburi
boards: m5stack: cores3: Add CoreS3 SE variant
0bbc1b2 
Add `se` variant to support the low-cost CoreS3 SE.

- Add configuration files
  Add `m5stack_cores3_procpu_se(.dts|.yaml|defconfig)` files.
  Reorganize dts files to split common parts.

- Update .yaml file
  Add gpio, can, counter, entropy, pwm, and pinmux to the supported
  feature group. Remove the `ignore_tags:` section.

- Update documents
  Add and modify information about CoreS3 SE.
  Add more description about sysbuild.

Signed-off-by: TOKITA Hiroshi <[email protected]>
@soburi
boards: m5stack: cores3: Add mbus and grove connector
7011fa3 
Adding M-Bus and Grove connector and also add configuration
for related peripherals.

Signed-off-by: TOKITA Hiroshi <[email protected]>
@soburi soburi force-pushed the m5stack_cores3_rtc_tf_led branch from 62c62d5 to 7011fa3 Compare December 2, 2024 23:41
@soburi soburi requested a review from marekmatej December 3, 2024 01:47
sylvioalves
sylvioalves approved these changes Dec 3, 2024
View reviewed changes
@fabiobaltieri fabiobaltieri merged commit 405efb3 into zephyrproject-rtos:main Dec 3, 2024
19 checks passed
@soburi soburi deleted the m5stack_cores3_rtc_tf_led branch December 3, 2024 15:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@sylvioalves sylvioalves sylvioalves approved these changes

@marekmatej marekmatej marekmatej approved these changes

@aasinclair aasinclair Awaiting requested review from aasinclair

@bbilas bbilas Awaiting requested review from bbilas

@gmarull gmarull Awaiting requested review from gmarull

@Mani-Sadhasivam Mani-Sadhasivam Awaiting requested review from Mani-Sadhasivam

@simonguinot simonguinot Awaiting requested review from simonguinot

@avisconti avisconti Awaiting requested review from avisconti

@erwango erwango Awaiting requested review from erwango

@galak galak Awaiting requested review from galak

@jfischer-no jfischer-no Awaiting requested review from jfischer-no

@kartben kartben Awaiting requested review from kartben

@mnkp mnkp Awaiting requested review from mnkp

@henrikbrixandersen henrikbrixandersen Awaiting requested review from henrikbrixandersen

Assignees

@marekmatej marekmatej

@sylvioalves sylvioalves

Labels

platform: ESP32 Espressif ESP32

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

9 participants

@soburi @henrikbrixandersen @kartben @marekmatej @sylvioalves @fabiobaltieri @Mani-Sadhasivam @simonguinot @zephyrbot