Conversation
zephyrbot
requested review from
MaureenHelm,
bbolen,
carlocaione,
decsny,
galak,
gmarull,
henrikbrixandersen,
ithinuel,
microbuilder,
mnkp,
nordic-krch,
soburi and
stephanosio
avolkov-1221
force-pushed
the
renesas_ra2_poc
branch
3 times, most recently
from
46aa789 to
d683a60
Compare
avolkov-1221
force-pushed
the
renesas_ra2_poc
branch
2 times, most recently
from
feb2064 to
c1b5e80
Compare
avolkov-1221
force-pushed
the
renesas_ra2_poc
branch
from
c1b5e80 to
cbd588b
Compare
soburi
left a comment
soburi
left a comment
There was a problem hiding this comment.
There are several PRs to add support for RA2, including my own.
In my opinion, having multiple implementations makes maintenance difficult, so I think we should consolidate them.
I would also like to align RA4M1, which I created previously, with this one.
It would be easier to discuss if you submitted it as a patch to the existing drivers rather than adding an alternative driver.
Sorry, but could you give me this PR number ? I only see my and Renesas PRs.
It will definitely be good to at least merge our works. I didn't do this because I don't have the RA4M1 hardware and can't test it. Part of our implementations are close enough and didn't depends on external modules, like Renesas port do. However Renesas provide very, very useful stuff like documentation and eval. board implementations. |
avolkov-1221
force-pushed
the
renesas_ra2_poc
branch
3 times, most recently
from
2a5807f to
05bc562
Compare
avolkov-1221
force-pushed
the
renesas_ra2_poc
branch
6 times, most recently
from
33eb5cd to
15956c4
Compare
…ount" The "channels-num" should not be used here, other system's parts are using "channel-count" instead for the same purpose. Also property's description has been сorrected. Signed-off-by: Andrey VOLKOV <andrey.volkov@munic.io>
Add basic support for Renesas RA2L1 SOC series. Signed-off-by: Andrey VOLKOV <andrey.volkov@munic.io>
Add initial clock control drivers set for Renesas RA2L1. Signed-off-by: Andrey VOLKOV <andrey.volkov@munic.io>
…ture Add initial Renesas RA LPM support and reference driver for Renesas RA2L1 SOC. Signed-off-by: Andrey VOLKOV <andrey.volkov@munic.io>
Add initial pinctrl support for Renesas RA2L1. Signed-off-by: Andrey VOLKOV <andrey.volkov@munic.io>
…RA2L1 Add initial Interrupt Controller Unit (ICU) driver for Renesas RA2L1. Signed-off-by: Andrey VOLKOV <andrey.volkov@munic.io>
Add initial GPIO driver for Renesas RA2L1. Signed-off-by: Andrey VOLKOV <andrey.volkov@munic.io>
Add initial SCI UART driver for Renesas RA2L1. Signed-off-by: Andrey VOLKOV <andrey.volkov@munic.io>
Add initial flash controller support for Renesas RA2L1. Signed-off-by: Andrey VOLKOV <andrey.volkov@munic.io>
Add inital support for the Renesas EK-RA2L1 evaluation board. Signed-off-by: Andrey VOLKOV <andrey.volkov@munic.io>
avolkov-1221
force-pushed
the
renesas_ra2_poc
branch
from
15956c4 to
f5d70ce
Compare
| @@ -0,0 +1,3 @@ | |||
| # SPDX-License-Identifier: Apache-2.0 | |||
|
|
||
| source "drivers/clock_control/Kconfig.renesas_ra_cgc" | ||
|
|
||
| source "drivers/clock_control/Kconfig.renesas_ra2" |
There was a problem hiding this comment.
Since having multiple implementations of the same control target is not desirable, I request that the existing implementation be used for the clock controller or improved.
The clock controller does not essentially depend on FSP, and I think the current implementation can work without FSP if some trivial functions are reimplemented.
Similarly, the interrupt controller and pinctrl are unrelated to FSP, so I think they should have a common implementation.
The interrupt controller will need to be addressed after merging #75946, which I have submitted, so please consider it after that.
As you pointed out, I think there are still some problems with the implementation that uses FSP for pinctrl, so I guess it is okay to leave it as it is for now.
There was a problem hiding this comment.
About the clocks: I'm really thinking about how to better utilize both of them. So far, it's not very straightforward: one of the ideas used in our implementation is the use of fine-grained ref counters, which would allow the PM to switch off unused clocks in sleep modes (the DT support of the counters already in place). A good example of this functionality is when the CAN module is the sole client of the MOSC clock. However, I don't see such features in the FSP implementations.
There was a problem hiding this comment.
About the clocks: I'm really thinking about how to better utilize both of them. So far, it's not very straightforward: one of the ideas used in our implementation is the use of fine-grained ref counters, which would allow the PM to switch off unused clocks in sleep modes (the DT support of the counters already in place). A good example of this functionality is when the CAN module is the sole client of the MOSC clock. However, I don't see such features in the FSP implementations.
Understood.
If you still need to consider whether it's optimal for other SoCs, I think it's fine to leave it as it is for now.
However, ultimately, there should be a single implementation for the RA family, so please design it assuming it will be integrated.
There was a problem hiding this comment.
There are many nodes without the respective DTS bindings, so it is not possible to crosscheck them.
Could you introduce the nodes when you add the drivers in follow-up PRs?
| return 0; | ||
| } | ||
|
|
||
| static int gpio_ra_port_set_masked_raw(const struct device *port, |
There was a problem hiding this comment.
gpio_port_set_masked_raw() may also set a pin to low physical level. Does the current implementation support this?
| # Copyright (c) 2021-2024 MUNIC SA | ||
| # SPDX-License-Identifier: Apache-2.0 | ||
|
|
||
| CONFIG_SYS_CLOCK_HW_CYCLES_PER_SEC=480000000 |
There was a problem hiding this comment.
@avolkov-1221 , Please recheck the value. Should this be only 48000000 (48MHz) as you're using hoco for sysclk source
There was a problem hiding this comment.
Oops, thank you.
| R_SCI_SEMR_RXDESEL_Msk | ||
| ); | ||
|
|
||
| uart_ra_configure(dev, &data->ucfg); |
There was a problem hiding this comment.
@avolkov-1221 , This function only available when CONFIG_UART_USE_RUNTIME_CONFIGURE is on, if user turn it off I assumed the SCI hardware wouldn't be configured?
| #ifndef __INCLUDE_DRIVERS_FLASH_FLASH_RA_H__ | ||
| #define __INCLUDE_DRIVERS_FLASH_FLASH_RA_H__ |
There was a problem hiding this comment.
| #ifndef __INCLUDE_DRIVERS_FLASH_FLASH_RA_H__ | |
| #define __INCLUDE_DRIVERS_FLASH_FLASH_RA_H__ | |
| #ifndef __ZEPHYR_INCLUDE_DRIVERS_FLASH_FLASH_RA2_H__ | |
| #define __ZEPHYR_INCLUDE_DRIVERS_FLASH_FLASH_RA2_H__ |
| #ifndef __CLOCK_CONTROL_RA_PRIV_H__ | ||
| #define __CLOCK_CONTROL_RA_PRIV_H__ |
There was a problem hiding this comment.
| #ifndef __CLOCK_CONTROL_RA_PRIV_H__ | |
| #define __CLOCK_CONTROL_RA_PRIV_H__ | |
| #ifndef __CLOCK_CONTROL_RA2_PRIV_H__ | |
| #define __CLOCK_CONTROL_RA2_PRIV_H__ |
| - name: ra8t1 | ||
| socs: | ||
| - name: r7fa8t1ahecbd | ||
| - name: ra2l1 |
There was a problem hiding this comment.
Done. Thank you.
BTW, could your team update the other SoCs (I mean RA8xx and RA6xx) according to this commit?
In my opinion, using the temperature and quality grade information in the names of the SoCs is useless. However, the socket type (referred to as "package type" in datasheets) defines the number of GPIOs and some peripherals, making it meaningful from a software perspective. Additionally, this can't be easily defined in DTS.
KhiemNguyenT
left a comment
KhiemNguyenT
left a comment
There was a problem hiding this comment.
It sounds to me that some improvement feedback has not been addressed, esp. reuse the existing drivers rather than adding the new ones. And there's no explanation in review comments or in commit messages.
@avolkov-1221 please understand that RA2L1 will not stand alone. RA2A1 is one of its brothers and others will come. Your contribution to existing drivers and next ones are welcomed, but the implementation must be aligned across RA2 devices and other RA series, if there's no big reason to create a new ones.
To accelerate the review and get the patches merged, please consider a " PR for minimal support", without any changes which still need more discussion, dependency to another PR. Next ones should be driver support, such as LPM, ICU, etc. If you want to get the PR merge quickly, please consider to split the PRs.
Last but not least, this PR consumed much effort of many reviewers. I hope we can have focus on smaller PR from now. This is a reasonable request, right ?
I haven't started the merge yet, except for the OFS, clocks, GPIO, and pinctrls, so I didn't have anything to reply to or comment on.
Well, it seems reasonable to separate the first commit from the others. But for the rest, that's not the case. They depend on each other.
Yes, I can freeze this PR and create new ones. Perhaps they won't be so ill-fated. |
|
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. |
11 participants
This is an initial support of Renesas RA2L1 chips family and EK-RA2L1 eval board. Unlike the Renesas port, it does not require additional HAL modules.