-
Notifications
You must be signed in to change notification settings - Fork 8.2k
soc: renesas: ra: configure option settings memory #71017
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
soc: renesas: ra: configure option settings memory #71017
Conversation
soc/renesas/ra/ra4m1/Kconfig.soc
Outdated
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.
Move to Kconfig
soc/renesas/ra/ra4m1/Kconfig.soc
Outdated
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.
Please refer to the devicetree definitions.
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.
zephyr/dts/arm/renesas/ra/ra-cm4-common.dtsi
Lines 39 to 44 in f643f3a
| hoco: hoco { | |
| compatible = "fixed-clock"; | |
| clock-frequency = <24000000>; | |
| status = "okay"; | |
| #clock-cells = <0>; | |
| }; |
Please use this definition for determining HOCO frequency.
2193478 to
950fa1f
Compare
soc/renesas/ra/ra4m1/Kconfig
Outdated
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.
default n not needed
soc/renesas/ra/ra4m1/Kconfig
Outdated
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.
these should be dts entries
soc/renesas/ra/ra4m1/Kconfig
Outdated
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.
settings here should have SOC_ prefix
a20b1c7 to
33bb06c
Compare
soc/renesas/ra/ra4m1/soc.c
Outdated
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 couldn't estimate what the design intention from the "Set all fields to 1” description.
Since WDTSTPCTL is 1, the watchdog will not start, so it would be better to state that.
33bb06c to
680fb60
Compare
soc/renesas/ra/ra4m1/Kconfig
Outdated
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 voltage detecting and enabling HOCO on reset setting seems good to be managed by hwinfo driver.
If you are having difficulty creating a suitable devicetree at the moment,
I think it's okay to set a fixed value at this point.
|
I have created PR #71174 based on these patches. |
680fb60 to
87f9c94
Compare
7426bdf to
f8e5701
Compare
soc/renesas/ra/ra4m1/Kconfig
Outdated
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.
We can use the devicetree definition to determine hoco frequency by the following code.
DT_PROP(DT_PATH(clocks, hoco), clock_frequency)
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.
hi @soburi, i'm new to zephyr so i could be wrong about this but is seems like I can't use:
DT_PROP(DT_PATH(clocks, hoco), clock_frequency)
inside a Kconfig file to access a devicetree property, however i can use this within a 'C' file to access a devicetree property.
when i check other Kconfig files that refer to the devicetree they all seem to use:
dt_node_xxx_yyy
see soc/nxp/mcx/mcxnx4x/Kconfig for an example
let me know what you think, I probably missed something obvious :)
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 part ...
can rewrite as follows.
We can refer the hoco clock frequency directly in code, so we no need to define hoco freq in Kconfig.
#define HOCO_FREQ DT_PROP(DT_PATH(clocks, hoco), clock_frequency)
#if HOCO_FREQ == 24000000
#define OFS1_HOCO_FREQ 0
#elif HOCO_FREQ == 32000000
#define OFS1_HOCO_FREQ 2
#elif HOCO_FREQ == 48000000
#define OFS1_HOCO_FREQ 4
#elif HOCO_FREQ == 64000000
#define OFS1_HOCO_FREQ 5
#else
#error "Unsupported HOCO frequency"
#endif
soc/renesas/ra/ra4m1/Kconfig
Outdated
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.
Whether the hoco is valid or invalid, I don't think there is any problem with judging it by the following code.
DT_NODE_HAS_STATUS(DT_PATH(clocks, hoco), okay)
If you are considering a case where the not enabled the hoco at reset but is enabled during startup processing, add a property to dts/bindings/clock/renesas,ra-clock-generation-circuit.yaml. I think it's also good. (But this might be overthinking.)
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 is same as SOC_OPTION_SETTING_HOCO_FREQ.
This line can rewrite
.HOCOEN = !DT_NODE_HAS_STATUS(DT_PATH(clocks, hoco), okay),
And we can remove this Kconfig option.
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.
It seems to be a good. We can remove the config.
config SOC_OPTION_SETTING_HOCO_DISABLE
soc/renesas/ra/ra4m1/Kconfig
Outdated
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 it's better to create dts/bindings/hwinfo/renesas,ra-lvd.yaml and define it in the devicetree node.
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 don't think this setting needs to be done in this PR.
Since the hwinfo driver is used to obtain the startup factor after a reset, I think it would be a good idea to create a devicetree definition related to this and use that.
(If you are new to Zephyr development, creating a hwinfo driver brings good experience.)
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.
@soburi, Agreed, I would like to implement this setting via the device tree but do it in another PR. Shall I leave the VDSEL as it is for this PR or maybe remove the option?
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.
Don't leave options that don't work, as they confuse users.
soc/renesas/ra/ra4m1/soc.c
Outdated
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 OFS is used widely in the RA series.
It should place a common directory.
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.
@soburi, while it's true that the OFS is used in a lot of RA microcontrollers it seems the contents of the registers, and their memory location is different in pretty much every series. A quick check of the device datasheets shows that RA6 series has different OSF1 register contents than RA4 series, as does the RA0 series. Therefore, I think it is better to leave this where it is...
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.
If so, let's implement the individual implementation in soc.c.
eef6ecf to
c5e15f7
Compare
An area of flash memory on the RA4M1 MCU is used to store information used to configure the device following a reset. This patch instructs the linker to reserve this memory area and provides kconfig options that are used to populate it (at build time) with the desired device configuration. Signed-off-by: Ian Morris <[email protected]>
|
@soburi, i think i have addressed all the comments. let me know if i missed something or if you have anything further. |
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 would like to make this OK for now.
Renesas has officially started supporting it, so there may be things to consider in the future.
|
Hi @thedjnK, I think I have addressed your comments. When you get a minute could you take a look and approve (or let me know if further changes are required)? |
| config SOC_OPTION_SETTING_MEMORY | ||
| bool "Option Setting Memory" | ||
| default y |
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.
Shouldn't this have a description? Never used these parts but "option setting memory" to me has no meaning, I have no idea what it is or what disabling it would do. Non-blocking comment and can be updated in a future PR.
An area of flash memory on the RA4M1 MCU is used to store information used to configure the device following a reset. This patch instructs the linker to reserve this memory area and provides kconfig options that are used to populate it (at build time) with the desired device configuration.
NOTES:
The only board that currently uses the RA4M1 soc port is the Ardunio UNO R4 Minima. This board uses a bootloader to load the application. As this bootloader is located in the lower part of the RA4M1 flash memory it handles the reservation (and configuration) of the option setting memory area. In this case (and others where the application does not reside in the area that contains the option setting memory) this functionality can be disabled via the OPTION_SETTING_MEMORY kconfig option, however leaving it enabled does not cause any issues other than wasting 256 bytes of flash memory.
This option MUST be enabled if the application resides in the area that contains the option setting memory (which is usually the case if a bootloader is not used) - otherwise unintended behavior can occur due to misconfiguration of the device.