boards: arm: Add Renesas Spider board #56043
Conversation
aaillet
requested review from
nashif,
carlescufi,
galak,
MaureenHelm and
gmarull
as code owners
zephyrbot
added
area: Clock Control
platform: Renesas R-Car
zephyrbot
requested review from
cfriedt,
henrikbrixandersen,
mnkp and
nordic-krch
aaillet
force-pushed
the
for-upstream/add-spider
branch
from
c0112f8
to
c65c4d6
Compare
|
This PR is ready to be reviewed as the last CI failure is a false positive ! |
|
@gmarull @povergoing can you take a look please? |
| clocks = <&cpg CPG_MOD 702>, <&cpg CPG_CORE R8A779F0_CLK_S0D12_PER>; | ||
| }; | ||
|
|
||
| /* Linux console */ |
There was a problem hiding this comment.
Just out of curiosity, Why there is a Linux console?
There was a problem hiding this comment.
Thanks for your review !
On this SoC, Linux is running on A55s cores while Zephyr is running on the R52 !
Devices like serial lines are common for both "worlds".
I added both "physically accessible" serial lines to the device tree as users could have different use cases than running Linux and Zephyr at the same time.
I commented this serial line as "Linux console" for users to know that this one is already used in a Linux+Zephyr use case !
There was a problem hiding this comment.
AFAIK, Cortex A cluster uses HSCIF0 nowadays.
| const struct pfc_bias_reg *pfc_rcar_get_bias_regs(void); | ||
| const struct pfc_drive_reg *pfc_rcar_get_drive_regs(void); | ||
|
|
||
| /** | ||
| * @brief set the register index for a given pin | ||
| * | ||
| * @param the pin | ||
| * @param pointer for the resulting register index | ||
| * @return 0 if the register index is found, negative | ||
| * errno otherwise. | ||
| */ | ||
| int pfc_rcar_get_reg_index(uint8_t pin, uint8_t *reg_index); |
There was a problem hiding this comment.
this should not be part of pinctrl_soc, as it's something needed internally by drivers. please, create a new internal header so that drivers/pinctrl.h is not polluted with unnecessary stuff.
There was a problem hiding this comment.
Hi @gmarull ,
Thanks for your comment.
If my understanding is fine, you want me to move these common declaration to a new file here zephyr/include/zephyr/drivers/pinctrl/pinctrl_rcar_common.h ?
There was a problem hiding this comment.
What I mean is that this should only be exposed to code requiring it. An internal header is fine, but decoupled from pinctrl_soc.h (which ends up included in pinctrl.h)
There was a problem hiding this comment.
Fixed it, please tel me if it's ok now !
|
This pull request has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 14 days. Note, that you can always re-open a closed pull request at any time. |
| clocks = <&cpg CPG_MOD 702>, <&cpg CPG_CORE R8A779F0_CLK_S0D12_PER>; | ||
| }; | ||
|
|
||
| /* Linux console */ |
There was a problem hiding this comment.
AFAIK, Cortex A cluster uses HSCIF0 nowadays.
I think that we agree on this subject. Physically SCIF3 and HSCIF0 are using the same lines and Cortex A's are using HSCIF0. (HSCIF1 and SCIF0 are on the same line as well) For other use cases, Zephyr may use SCIF3 line. Please keep in mind that HSCIF0 and SCIF0 are not on the same line, they can work at the same without any issues. (On different physical port) I hope I answered your comments. |
| Overview | ||
| ******** | ||
|
|
||
| R-Car S4 enables the launch of Car Server/CoGW with high performance, high-speed networking, high security and high functional safety levels that are required as E/E architectures evolve into domains and zones. |
There was a problem hiding this comment.
can you wrap the lines to either 80 or 100 columns? sorry we don't have a check for this yet but that's how these docs are normally done
There was a problem hiding this comment.
Ok, but how should I manage links that are longer than 100 characters then ?
They are gathered at the bottom of the rst file
There was a problem hiding this comment.
those are fine, just wrap the normal text
| int ret = -ENOTSUP; | ||
| uint32_t reg; | ||
|
|
||
| enable = !!enable; |
There was a problem hiding this comment.
does not look like this is used, stale? guess you may want to remove the argument entirely
There was a problem hiding this comment.
It's not used yet as current driver don't need their clocks to be enabled or disabled manually.
This whole function is a placeholder to keep skeleton from other renesas clock driver.
It's also already planned that the arm64 support of this soc (coming just after this PR, see #60252 ) will need some clocks to be enable/disabled for some drivers.
More specifically, this argument is just as a boolean to know if this function is called to enable or disable the concerned clock.
That's all reasons why this function has to be here but is not used yet.
There was a problem hiding this comment.
ok, keep the argument, drop this line? or mark it as ARG_UNUSED
| struct cpg_clk_info_table *clk_info; | ||
| struct r8a779f0_cpg_mssr_data *data = dev->data; | ||
| k_spinlock_key_t key; | ||
| int ret = 0; |
There was a problem hiding this comment.
drop the initializer, return with return 0; at the end of the function
| clock_control_subsys_t sys, bool enable) | ||
| { | ||
| struct rcar_cpg_clk *clk = (struct rcar_cpg_clk *)sys; | ||
| int ret = -EINVAL; |
There was a problem hiding this comment.
no need to initialize this
There was a problem hiding this comment.
was getting "may be used uninitialized" warning when declaring ret uninitialized
adding an "else" initializing ret at -EINVAL to fix warning
|
|
||
| static uint32_t r8a779f0_get_div_helper(uint32_t reg_val, uint32_t module) | ||
| { | ||
| uint32_t divider = RCAR_CPG_NONE; |
There was a problem hiding this comment.
how about using explicit return and ditching the variable? easier to follow...
|
|
||
| static int r8a779f0_set_rate_helper(uint32_t module, uint32_t *divider, uint32_t *div_mask) | ||
| { | ||
| int ret = -ENOTSUP; |
There was a problem hiding this comment.
just return -ENOSUP for now, you can add stuff later with the structure that makes sense at that point
aaillet
force-pushed
the
for-upstream/add-spider
branch
from
83c5a7a
to
9d4d21d
Compare
|
Addressed comments from @fabiobaltieri concerning board documentation and clock driver. |
aaillet
force-pushed
the
for-upstream/add-spider
branch
from
9d4d21d
to
2cebd25
Compare
| } | ||
|
|
||
| int r8a779f0_cpg_mssr_start_stop(const struct device *dev, | ||
| clock_control_subsys_t sys, bool enable) |
There was a problem hiding this comment.
sorry one more indentation nitpick: under the previous parenthesis is how it's normally done here, you are mixing thigs up a bit in the file, here you are just doing a tab, in r8a779f0_cpg_mssr_start you are back one charcter, could you check these and make sure they are at least coherent within the file?
int r8a779f0_cpg_mssr_start_stop(const struct device *dev,
clock_control_subsys_t sys, bool enable)
static int r8a779f0_cpg_mssr_start(const struct device *dev,
clock_control_subsys_t sys)
is how it should be
There was a problem hiding this comment.
running clang-format on the entire file as it is a new one could be the right call ?
There was a problem hiding this comment.
clang-format mostly works but often does a bit of a disaster on complex macros, if you want to use it fine but don't take the output religiously, either commit locally, clang-format and then manually git checkout -p the messy chunks, or add some /* clang-format off */ /* clang-format on */ markers around the part it messes up, if any.
| if (!ret) { | ||
| rcar_cpg_write(DEVICE_MMIO_GET(dev), clk_info->offset, reg); | ||
| } | ||
| return ret; |
There was a problem hiding this comment.
would not you be better off just return -ENOTUP and then implement the whole thing once there's something to do? I understand you want to have the stubs but it just looks really really weird right now, plus the way you are using ret around is hard to follow and does not look quite right
There was a problem hiding this comment.
I just need to check if it is mandatory to write clk reg when errors are happening (l67) in the controller doc.
If it is, I'll just do this rcar_cpg_write before returning ENOTSUP 👍
Yeah, I totally understand that these placeholders are weird right now, at least keeping them with a return -ENOTSUP is already nice for the driver creation !
Add support of r8a779f0 cpg driver. r8a779f0 soc has its own clock tree. Gen4 SoCs common registers addresses have been added in header. Signed-off-by: Mykola Kvach <mykola_kvach@epam.com> Signed-off-by: Aymeric Aillet <aymeric.aillet@iot.bzh>
r8a779f0 is also know as S4, this SoC is part of the Gen4 SoC series, has 8 Cortex-A55 and a dual core lockstep Cortex-R52 processor. SCIF0 is dedicated to Zephyr and SCIF3 to Linux. **Control Domains** IMPORTANT: This SoC is divided into two "domains": - Application domain contains some peripherals as well as A55 & R52 cores. - Control domain that contain a G4MH/RH850 MCU and other peripherals. In order to access control domain peripherals such as gpio4-7 and CAN-FD from application domain, the G4MH MCU has to unlock a protection mechanism from control domain buses. "Protected" controllers will be flagged in gen4 device trees, warning users that they need to flash a custom G4MH firmware to unlock access to these controllers. **Clock controller** This SoC clock controller is offering "domains" for each world (Zephyr/Linux). These domains are several "entry points" to the clock controller which are arbitrated to avoid a world from turning off a clock needed by another one. We decided to use the same domain as Linux because the security mechanism as to be implemented before accessing another domain. Signed-off-by: Aymeric Aillet <aymeric.aillet@iot.bzh>
r8a779f0 SoC is part of the Renesas R-Car Gen4 SoC series. This SoC has a dual core lockstep Cortex-R52 CPU. Signed-off-by: Aymeric Aillet <aymeric.aillet@iot.bzh>
Renesas Spider board use a S4 SoC (r8a779f0). Add basic support for UART, GPIO and clock control. We are using SCIF0 as SCIF3 is used by Linux. Signed-off-by: Aymeric Aillet <aymeric.aillet@iot.bzh>
Adding Spider board documentation based on Renesas official documentation and following Zephyr guideline. The documentation is describing the board and its current Zephyr support. Signed-off-by: Aymeric Aillet <aymeric.aillet@iot.bzh>
aaillet
force-pushed
the
for-upstream/add-spider
branch
from
2cebd25
to
484ebac
Compare
|
Addressed last comments from @fabiobaltieri concerning clock driver |
Based on edit done at r8a779f0 driver creation (f5634a1) following comments on zephyrproject-rtos#56043. Signed-off-by: Aymeric Aillet <aymeric.aillet@iot.bzh>
Based on edit done at r8a779f0 driver creation (f5634a1) following comments on zephyrproject-rtos#56043. Signed-off-by: Aymeric Aillet <aymeric.aillet@iot.bzh>
Based on edit done at r8a779f0 driver creation (f5634a1) following comments on zephyrproject-rtos#56043. Signed-off-by: Aymeric Aillet <aymeric.aillet@iot.bzh>
Based on edit done at r8a779f0 driver creation (f5634a1) following comments on zephyrproject-rtos#56043. Signed-off-by: Aymeric Aillet <aymeric.aillet@iot.bzh>
Based on edit done at r8a779f0 driver creation (f5634a1) following comments on zephyrproject-rtos#56043. Signed-off-by: Aymeric Aillet <aymeric.aillet@iot.bzh>
This pull request is introducing Generation 4 of Renesas R-Car SoCs with the S4 SoC as well as the Spider board into Zephyr.
Gen4 SoCs vs Gen3 (Zephyr POV) :
Cortex-R52 is now used as Real Time core
We are now able to use embedded ARM timer
Some IPs are the same and will use existing drivers, sometime with few improvements (UART, GPIO...)
Introduction of new IPs
S4 SoC that is equipping multiple Gen4 boards have some Cortex-A55s and a Cortex-R52 core.
Spider board is the reference board for S4 SoC, developpment oriented, this board is providing a lot of available IOs.
Operation of basic samples such as hello_world and blinky have been validated.