-
Notifications
You must be signed in to change notification settings - Fork 8.1k
Support Retronix Sparrow Hawk board based on Renesas R-Car V4H SoC #97783
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
base: main
Are you sure you want to change the base?
Conversation
|
Hello @DuyDang007, and thank you very much for your first pull request to the Zephyr project! |
| config ARCH | ||
| default "arm" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I removed this
| # Generic timer clock frequency in Hz | ||
| CONFIG_SYS_CLOCK_HW_CYCLES_PER_SEC=16666667 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
goes in soc Kconfig.defconfig and must come from dts property using functions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I moved it to Kconfig.defconfig.r8a779g0.
Could you explain "and must come from dts property using functions"? I see that the driver read from this config directly, not the dts
| CONFIG_FLASH_SIZE=0 | ||
| CONFIG_FLASH_BASE_ADDRESS=0x40040000 | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this board has been designed improperly, this is not correct
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I deleted these configs. This board uses bootloader, not booting from flash. So, these configs are unused.
| zephyr_library_sources(pfc_r8a77951.c) | ||
| elseif (CONFIG_SOC_R8A779F0_R52 OR CONFIG_SOC_R8A779F0_A55) | ||
| zephyr_library_sources(pfc_r8a779f0.c) | ||
| elseif (CONFIG_SOC_R8A779G0_R52) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no space between if and bracket in cmake
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I fixed it
soc/renesas/rcar/rcar_gen4/soc.c
Outdated
|
|
||
| #ifdef CONFIG_UART_CONSOLE | ||
| /* Support early console log */ | ||
| int arch_printk_char_out(int _c) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is this here at all?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At the beginning, there's no console log. I tried adding this. But now the UART console can work after removing this. Maybe another issue.
I deleted this file
| Here are the current supported features: | ||
|
|
||
| +-----------+------------------------------+--------------------------------+ | ||
| | Interface | Driver/components | Support level | | ||
| +===========+==============================+================================+ | ||
| | PINMUX | pin-ctrl | | | ||
| +-----------+------------------------------+--------------------------------+ | ||
| | CLOCK | clock_control | | | ||
| +-----------+------------------------------+--------------------------------+ | ||
| | GPIO | gpio | Not yet | | ||
| +-----------+------------------------------+--------------------------------+ | ||
| | UART | uart | serial port-polling | | ||
| + + + + | ||
| | | FT2232H | serial port-interrupt | | ||
| +-----------+------------------------------+--------------------------------+ | ||
| | I2C | i2c | Not yet | | ||
| +-----------+------------------------------+--------------------------------+ | ||
| | Timer | timer | Not yet | | ||
| +-----------+------------------------------+--------------------------------+ | ||
| | PWM | pwm | Not yet | | ||
| +-----------+------------------------------+--------------------------------+ | ||
| | SPI | spi | Not yet | | ||
| +-----------+------------------------------+--------------------------------+ | ||
|
|
||
| More features will be supported soon. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
whole table to go, use hw directive
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I updated it
78b2f0c to
2cd6abc
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for your comments.
I used this image
boards/retronix/sparrowhawk_rcar_v4h/doc/sparrow_hawk_rcar_v4h_r52.rst
Outdated
Show resolved
Hide resolved
boards/retronix/sparrowhawk_rcar_v4h/doc/sparrow_hawk_rcar_v4h_r52.rst
Outdated
Show resolved
Hide resolved
boards/retronix/sparrowhawk_rcar_v4h/doc/sparrow_hawk_rcar_v4h_r52.rst
Outdated
Show resolved
Hide resolved
2cd6abc to
1472554
Compare
1472554 to
1905b0e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 for docs, thanks!
| hscif1_data_tx_default: hscif1_data_tx_default { | ||
| pin = <PIN_MSIOF2_SYNC FUNC_HRX1>; | ||
| }; | ||
| hscif1_data_rx_default: hscif1_data_rx_default { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| hscif1_data_rx_default: hscif1_data_rx_default { | |
| hscif1_data_rx_default: hscif1_data_rx_default { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR adds support for the Retronix Sparrow Hawk board based on the Renesas R-Car V4H SoC (r8a779g0) running on the Cortex-R52 processor. The implementation provides UART driver support and has been validated with the Hello World sample application.
Key Changes
- Added SoC-level support for r8a779g0 with Cortex-R52 processor configuration
- Implemented board-specific files for Sparrow Hawk including device tree, pinctrl, and documentation
- Extended pinctrl and clock control drivers to support the new r8a779g0 SoC
Reviewed Changes
Copilot reviewed 27 out of 28 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| soc/renesas/rcar/soc.yml | Added r8a779g0 SoC with r52 CPU cluster definition |
| soc/renesas/rcar/rcar_gen4/Kconfig*.* | Added Kconfig options and default configurations for r8a779g0_r52 |
| soc/renesas/rcar/rcar_gen4/CMakeLists.txt | Added build configuration for r8a779g0_r52 target |
| include/zephyr/dt-bindings/pinctrl/renesas/* | Added pinctrl definitions and macros for r8a779g0 |
| include/zephyr/dt-bindings/clock/* | Added clock controller bindings for r8a779g0 |
| dts/bindings/vendor-prefixes.txt | Added retronix vendor prefix |
| dts/bindings/clock/* | Added YAML binding for r8a779g0 CPG MSSR |
| dts/arm/renesas/rcar/gen4/* | Added device tree includes for r8a779g0 SoC |
| drivers/pinctrl/renesas/rcar/* | Added pinctrl driver implementation for r8a779g0 |
| drivers/clock_control/* | Added clock control driver for r8a779g0 CPG MSSR |
| boards/retronix/sparrowhawk_rcar_v4h/* | Added complete board support files including DTS, Kconfig, and documentation |
| boards/retronix/index.rst | Added Retronix vendor board index |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
1905b0e to
2729622
Compare
2729622 to
8c63905
Compare
| /* Using domain 0 | ||
| */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why does this comment need to space over 2 lines?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I fixed that.
By the way, I remove one pair of reg node since they have the same base address.
| interrupts = <GIC_PPI 13 IRQ_TYPE_LEVEL | ||
| IRQ_DEFAULT_PRIORITY>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the formatting of this is bad, put each value on a line each? e.g. <GIC_PPI 13 IRQ_TYPE_LEVEL IRQ_DEFAULT_PRIORITY>,
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I fixed the alignment issues.
| pfc: pin-controller@e6050000 { | ||
| compatible = "renesas,rcar-pfc"; | ||
| reg = <0xe6050000 0x1d8>, <0xe6050800 0x1d8>, | ||
| <0xe6058000 0x1d8>, <0xe6058800 0x1d8>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
bad alignment
|
|
||
| # Arm Generic Timer frequency (Hz) | ||
| config SYS_CLOCK_HW_CYCLES_PER_SEC | ||
| default 16666667 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this needs to come from dts using a dt function
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This config is used in arm_arch_timer.c for system tick. The driver read CONFIG_SYS_CLOCK_HW_CYCLES_PER_SEC value directly so I cannot make use of DT function.
In V4H SoC, 16.667MHz is the fixed frequency of Generic Timer
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then add a dts node for it as other SoCs do, it's hardware in the silicon
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, I will add the frequency value to its timer node like below:
rcar_gen4_v4h_cr52.dtsi:
timer {
compatible = "arm,armv8-timer";
interrupt-parent = <&gic>;
interrupts = <GIC_PPI 13 IRQ_TYPE_LEVEL IRQ_DEFAULT_PRIORITY>,
<GIC_PPI 14 IRQ_TYPE_LEVEL IRQ_DEFAULT_PRIORITY>,
<GIC_PPI 11 IRQ_TYPE_LEVEL IRQ_DEFAULT_PRIORITY>,
<GIC_PPI 10 IRQ_TYPE_LEVEL IRQ_DEFAULT_PRIORITY>;
+ clock-frequency = <0xfe502b>;
};
Is it correct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
clock-frequency = <16666667>; is fine, yes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and update the commit to get it from dt using the dt functions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mean update in arm_arch_timer.c? It affects other board build. I think it should be in another improvement PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, I mean to use the dt functions in the Kconfig file to get the value from devicetree, see https://github.com/zephyrproject-rtos/zephyr/blob/main/soc/gd/gd32/gd32a50x/Kconfig.defconfig.gd32a503#L6 for an example
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah, I see. Sorry for misunderstood. I updated it
| }; | ||
|
|
||
| &gpio0 { | ||
| status = "disabled"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
gpio should be disabled by default in the base dts file, why would it need to be disabled by the board?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed. It's a duplicated "disabled"
|
|
||
| # GPIO | ||
| CONFIG_GPIO=n |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should not be needed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Diff to address formatting issues
@@ -24,15 +24,15 @@
timer {
compatible = "arm,armv8-timer";
interrupt-parent = <&gic>;
interrupts = <GIC_PPI 13 IRQ_TYPE_LEVEL
- IRQ_DEFAULT_PRIORITY>,
- <GIC_PPI 14 IRQ_TYPE_LEVEL
- IRQ_DEFAULT_PRIORITY>,
- <GIC_PPI 11 IRQ_TYPE_LEVEL
- IRQ_DEFAULT_PRIORITY>,
- <GIC_PPI 10 IRQ_TYPE_LEVEL
- IRQ_DEFAULT_PRIORITY>;
+ IRQ_DEFAULT_PRIORITY>,
+ <GIC_PPI 14 IRQ_TYPE_LEVEL
+ IRQ_DEFAULT_PRIORITY>,
+ <GIC_PPI 11 IRQ_TYPE_LEVEL
+ IRQ_DEFAULT_PRIORITY>,
+ <GIC_PPI 10 IRQ_TYPE_LEVEL
+ IRQ_DEFAULT_PRIORITY>;
};
soc {
interrupt-parent = <&gic>;
@@ -44,9 +44,9 @@
gic: interrupt-controller@f0000000 {
compatible = "arm,gic-v3", "arm,gic";
reg = <0xf0000000 0x1000>,
- <0xf0100000 0x20000>;
+ <0xf0100000 0x20000>;
interrupt-controller;
#interrupt-cells = <4>;
status = "okay";
};There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for you suggestion. I updated it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This causes the error with compliance check due to mixed tab and space indent:
CODE_INDENT: code indent should use tabs where possible
File:dts/arm/renesas/rcar/gen4/rcar_gen4_v4h_cr52.dtsi
Line:28
CODE_INDENT: code indent should use tabs where possible
File:dts/arm/renesas/rcar/gen4/rcar_gen4_v4h_cr52.dtsi
Line:29
CODE_INDENT: code indent should use tabs where possible
File:dts/arm/renesas/rcar/gen4/rcar_gen4_v4h_cr52.dtsi
Line:30
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I changed the indent for better look and pass compliance check.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Diff to address formatting issues
@@ -12,20 +12,20 @@
soc {
pfc: pin-controller@e6050000 {
compatible = "renesas,rcar-pfc";
reg = <0xe6050000 0x1d8>, <0xe6050800 0x1d8>,
- <0xe6058000 0x1d8>, <0xe6058800 0x1d8>,
- <0xe6060000 0x1d8>, <0xe6060800 0x1d8>,
- <0xe6061000 0x1d8>, <0xe6061800 0x1d8>,
- <0xe6068000 0x1d8>, <0xe6078000 0x0e8>;
+ <0xe6058000 0x1d8>, <0xe6058800 0x1d8>,
+ <0xe6060000 0x1d8>, <0xe6060800 0x1d8>,
+ <0xe6061000 0x1d8>, <0xe6061800 0x1d8>,
+ <0xe6068000 0x1d8>, <0xe6078000 0x0e8>;
};
/* Using domain 0
*/
cpg: clock-controller@e6150000 {
compatible = "renesas,r8a779g0-cpg-mssr";
reg = <0xe6150000 0x8a4>, /* GPG */
- <0xe6150000 0x2e74>; /* Module standby, software reset */
+ <0xe6150000 0x2e74>; /* Module standby, software reset */
#clock-cells = <2>;
};
gpio0: gpio@e6050000 {
|
re-assigning to renesas r-car maintainer |
8c63905 to
d79d2ca
Compare
Arm Generic Timer clock frequency is fixed per silicon. This value should be defined in DTS. Signed-off-by: Duy Dang <[email protected]>
d79d2ca to
b54cb40
Compare
b54cb40 to
7d6ba30
Compare
This commit adds initial support for V4H SoC, Cortex-R52 core. Signed-off-by: Duy Dang <[email protected]>
This adds support for Retronix Sparrow Hawk board. The board is based on R-Car V4H v3.0 (R8A779G3) SoC. Signed-off-by: Duy Dang <[email protected]>
Support Clock Pulse Generator driver for V4H Signed-off-by: Duy Dang <[email protected]>
Add pin definition for R-Car V4H SoC. Signed-off-by: Duy Dang <[email protected]>
Adds a page for Retronix Sparrow Hawk in Supported boards. Signed-off-by: Duy Dang <[email protected]>
7d6ba30 to
1c122f8
Compare
|




Hi everyone,
I would like to submit the code to support Retronix Sparrow Hawk board.
The board information can be found here:
https://www.renesas.com/en/support/partners/r-car-consortium/r-car-consortium-proactive-partner-list/rtx-sparrow-hawk?srsltid=AfmBOorxqVwVgVuRQ4Mi1N5FJYzQpdxNyP39oBwpRTHV3wtzNFtAVfCz
https://rcar-community.github.io/Sparrow-Hawk/index.html#introduction
Currently it supports only UART driver and confirmed with Hello World sample app. More features will be supported in the future