Skip to content

Conversation

@JasonLin-RealTek
Copy link
Contributor

This PR adds support for the RTS5912 EC Chip from Realtek.

The initial support for the board includes TIMER, GPIO, CLOCK_CONTROL, PINCTRL.

please help us review the pull request.

Thanks!

@zephyrbot zephyrbot added area: UART Universal Asynchronous Receiver-Transmitter area: Timer Timer area: Pinctrl area: GPIO area: Devicetree Binding PR modifies or adds a Device Tree binding area: Clock Control labels Jul 1, 2024
@github-actions
Copy link

github-actions bot commented Jul 1, 2024

Hello @JasonLin-RealTek, and thank you very much for your first pull request to the Zephyr project!
Our Continuous Integration pipeline will execute a series of checks on your Pull Request commit messages and code, and you are expected to address any failures by updating the PR. Please take a look at our commit message guidelines to find out how to format your commit messages, and at our contribution workflow to understand how to update your Pull Request. If you haven't already, please make sure to review the project's Contributor Expectations and update (by amending and force-pushing the commits) your pull request if necessary.
If you are stuck or need help please join us on Discord and ask your question there. Additionally, you can escalate the review when applicable. 😊

Copy link
Member

@gmarull gmarull left a comment

Choose a reason for hiding this comment

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

please, organize commit content properly, it seems that first commit contains e.g. pinctrl stuff which should not be there.

Comment on lines 8 to 9
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
config BOARD
default "rts5912_evb"

this is hwmv1

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
bool "Realtek RTS5912 Evaluation Board"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

Copy link
Contributor

Choose a reason for hiding this comment

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

convert to webp, then put it through https://tinypng.com/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Copy link
Contributor

Choose a reason for hiding this comment

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

non-blocking comment: should there be flash partitions here?

Copy link
Contributor

Choose a reason for hiding this comment

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

unaddressed

Copy link
Member

Choose a reason for hiding this comment

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

power states must not be at board level

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We have put those power state to rts5912.dtsi.

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
# CONSOLE
# Console

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

Comment on lines 6 to 10
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if SOC_FAMILY_REALTEK_EC
rsource "*/Kconfig.defconfig.series"
endif # SOC_FAMILY_REALTEK_EC
if SOC_FAMILY_REALTEK_EC
rsource "*/Kconfig.defconfig.series"
endif # SOC_FAMILY_REALTEK_EC

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

Comment on lines 6 to 9
Copy link
Contributor

Choose a reason for hiding this comment

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

add newline

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
select SOC_FAMILY_REALTEK_EC

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Comment on lines 12 to 20
Copy link
Contributor

Choose a reason for hiding this comment

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

non-blocking comment: do you really need a soc series when the soc and soc series name as the same? Is this really a series?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There may be series and plenty of SoCs with different names in the future.

In order to maintain the consistency of the folder structure, we will use the same name for the series and SoCs for now.

@JasonLin-RealTek JasonLin-RealTek force-pushed the add_realtek_rts5912_soc branch 2 times, most recently from e16ff4a to 7a10790 Compare July 3, 2024 11:08
@JasonLin-RealTek
Copy link
Contributor Author

JasonLin-RealTek commented Jul 4, 2024

please, organize commit content properly, it seems that first commit contains e.g. pinctrl stuff which should not be there.

We followed the suggestions to fix the code and passed the twister testing.
Are there any further suggestions? Could you please approve the PR?

Copy link
Member

@gmarull gmarull left a comment

Choose a reason for hiding this comment

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

I can't see any changes regarding my previous request

@JasonLin-RealTek JasonLin-RealTek force-pushed the add_realtek_rts5912_soc branch from 7a10790 to 7b8a3da Compare July 5, 2024 10:51
@JasonLin-RealTek
Copy link
Contributor Author

I can't see any changes regarding my previous request

We misunderstood the request and moved the [pinctrl+register] files to their own commit.
Now we have committed the device tree file into a single commit.
Is that correct?

@JasonLin-RealTek JasonLin-RealTek requested a review from gmarull July 8, 2024 02:15
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
bool "The output binary with RTS5912 image header"
bool "Add RTS5912 image header to output binary"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

Copy link
Contributor

Choose a reason for hiding this comment

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

*The

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

Comment on lines 40 to 22
Copy link
Contributor

Choose a reason for hiding this comment

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

But they should come from dts, not Kconfig

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
#source "soc/realtek/*/Kconfig.soc"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

@JasonLin-RealTek JasonLin-RealTek force-pushed the add_realtek_rts5912_soc branch from 7b8a3da to 2a3274a Compare July 8, 2024 11:37
keith-zephyr
keith-zephyr previously approved these changes Jan 7, 2025
rettichschnidi
rettichschnidi previously approved these changes Jan 7, 2025
Copy link
Contributor

@rettichschnidi rettichschnidi left a comment

Choose a reason for hiding this comment

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

Very superficial review, but all my nits got fixed -> approved

@fabiobaltieri
Copy link
Member

@gmarull could you take a look?

Comment on lines 19 to 20
Copy link
Member

@gmarull gmarull Jan 8, 2025

Choose a reason for hiding this comment

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

this should be captured in DT, see other examples by inspecting clocks {} node.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We added the clocks {} node and captured these in DT, thanks.

Copy link
Member

Choose a reason for hiding this comment

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

LOG_ERR

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We fixed it, thanks.

Copy link
Member

Choose a reason for hiding this comment

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

local header, so #include "debug_swj.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.

We fixed it, thanks.

Comment on lines 14 to 15
Copy link
Member

@gmarull gmarull Jan 8, 2025

Choose a reason for hiding this comment

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

LOG_MODULE_REGISTER(soc, CONFIG_SOC_LOG_LEVEL);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We replaced LOG_MODULE_REGISTER(soc); with LOG_MODULE_REGISTER(soc, CONFIG_SOC_LOG_LEVEL);, thanks.

Copy link
Member

Choose a reason for hiding this comment

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

1P8 -> 1V8 ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We modified these relevant macros, thanks.

Add support for Realtek RTS5912 embedded controller (EC).

Signed-off-by: Lin Yu-Cheng <[email protected]>
Add Realtek RTS5912 chip and driver device tree files.

Signed-off-by: Lin Yu-Cheng <[email protected]>
Add clock controller driver for Realtek RTS5912.

Signed-off-by: Lin Yu-Cheng <[email protected]>
Add pinctrl driver for Realtek RTS5912.

Signed-off-by: Lin Yu-Cheng <[email protected]>
Add swj driver for Realtek RTS5912.

Signed-off-by: Lin Yu-Cheng <[email protected]>
Add timer driver for Realtek RTS5912.

Signed-off-by: Lin Yu-Cheng <[email protected]>
Add gpio driver for Realtek RTS5912.

Signed-off-by: Lin Yu-Cheng <[email protected]>
Add UART driver for Realtek RTS5912.

Signed-off-by: Lin Yu-Cheng <[email protected]>
Add support for Realtek rts5912_evb board

Signed-off-by: Lin Yu-Cheng <[email protected]>
This commit adds api and driver of Realtek EC to maintainers.yml

Signed-off-by: Lin Yu-Cheng <[email protected]>
@kartben kartben merged commit a4d0591 into zephyrproject-rtos:main Jan 10, 2025
25 checks passed
@github-actions
Copy link

Hi @JasonLin-RealTek!
Congratulations on getting your very first Zephyr pull request merged 🎉🥳. This is a fantastic achievement, and we're thrilled to have you as part of our community!

To celebrate this milestone and showcase your contribution, we'd love to award you the Zephyr Technical Contributor badge. If you're interested, please claim your badge by filling out this form: Claim Your Zephyr Badge.

Thank you for your valuable input, and we look forward to seeing more of your contributions in the future! 🪁

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area: Clock Control area: Comparator area: Devicetree Binding PR modifies or adds a Device Tree binding area: GPIO area: Pinctrl area: Process area: RTC Real Time Clock area: Timer Timer area: UART Universal Asynchronous Receiver-Transmitter

Projects

None yet

Development

Successfully merging this pull request may close these issues.