-
Notifications
You must be signed in to change notification settings - Fork 8.3k
WIP: Add useful DT spec macros to unify clock control consumer codes and remove coupling of specific drivers. Converting NXP platforms WIP #96315
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -15,9 +15,48 @@ | |
| #include <zephyr/logging/log.h> | ||
| LOG_MODULE_REGISTER(clock_control); | ||
|
|
||
| struct lpc_syscon_spec { | ||
| uint32_t name; | ||
| }; | ||
|
|
||
| static int lpc_syscon_get_spec(clock_control_subsys_t subsys, struct lpc_syscon_spec *spec) | ||
| { | ||
| struct clock_control_dt_spec *dt_spec = subsys; | ||
|
|
||
| if (dt_spec->len != 1) { | ||
| return -EINVAL; | ||
| } | ||
|
|
||
| spec->name = *(dt_spec->cells); | ||
|
|
||
| return 0; | ||
| } | ||
|
Comment on lines
+18
to
+33
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just to be clear: this is merely for example, and it would be fine for drivers to do instead +#define LPC_CELL_NAME 0
static int mcux_lpc_syscon_clock_control_on(const struct device *dev,
clock_control_subsys_t sub_system)
{
#if defined(CONFIG_CAN_MCUX_MCAN)
- if ((uint32_t)sub_system == MCUX_MCAN_CLK) {
+ if (sub_system->cells[LPC_CELL_NAME] == MCUX_MCAN_CLK) {
CLOCK_EnableClock(kCLOCK_Mcan);
}
#endif /* defined(CONFIG_CAN_MCUX_MCAN) */for example?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The drivers can do whatever they want. This lpc syscon driver right now is really bad right now and probably needs the DT binding actually changed and driver optimized so that it isn't 5 million ifs, and is some more useful info instead. The benefit of unifying under these macros is that type of change can be done without having to edit any clock control consumer's code. And nxp has like 10 clock control drivers so I will be leaving these optimization activities to other NXP developer, but right now I'm trying to make it so that they can do that work without all of them having to update 200 consumer drivers for each PR |
||
|
|
||
| static int lpc_syscon_get_name(clock_control_subsys_t dt_spec, uint32_t *name) | ||
| { | ||
| struct lpc_syscon_spec spec; | ||
| int ret; | ||
|
|
||
| ret = lpc_syscon_get_spec(dt_spec, &spec); | ||
| if (ret) { | ||
| return ret; | ||
| } | ||
|
|
||
| *name = spec.name; | ||
|
|
||
| return 0; | ||
| } | ||
|
|
||
| static int mcux_lpc_syscon_clock_control_on(const struct device *dev, | ||
| clock_control_subsys_t sub_system) | ||
| clock_control_subsys_t subsys) | ||
| { | ||
| uint32_t sub_system; | ||
|
|
||
| lpc_syscon_get_name(subsys, &sub_system); | ||
| if (ret) { | ||
| return ret; | ||
| } | ||
|
|
||
| #if defined(CONFIG_CAN_MCUX_MCAN) | ||
| if ((uint32_t)sub_system == MCUX_MCAN_CLK) { | ||
| CLOCK_EnableClock(kCLOCK_Mcan); | ||
|
|
@@ -149,7 +188,12 @@ static int mcux_lpc_syscon_clock_control_get_subsys_rate(const struct device *de | |
| clock_control_subsys_t sub_system, | ||
| uint32_t *rate) | ||
| { | ||
| uint32_t clock_name = (uint32_t)sub_system; | ||
| uint32_t clock_name = 0; | ||
|
|
||
| lpc_syscon_get_name(sub_system, &clock_name); | ||
| if (ret) { | ||
| return ret; | ||
| } | ||
|
|
||
| switch (clock_name) { | ||
|
|
||
|
|
@@ -602,9 +646,14 @@ __weak int flexspi_clock_set_freq(uint32_t clock_name, uint32_t freq) | |
| static int SYSCON_SET_FUNC_ATTR mcux_lpc_syscon_clock_control_set_subsys_rate( | ||
| const struct device *dev, clock_control_subsys_t subsys, clock_control_subsys_rate_t rate) | ||
| { | ||
| uint32_t clock_name = (uintptr_t)subsys; | ||
| uint32_t clock_name; | ||
| uint32_t clock_rate = (uintptr_t)rate; | ||
|
|
||
| lpc_syscon_get_name(subsys, &clock_name); | ||
| if (ret) { | ||
| return ret; | ||
| } | ||
|
|
||
| switch (clock_name) { | ||
| case MCUX_FLEXSPI_CLK: | ||
| #if defined(CONFIG_MEMC) | ||
|
|
@@ -632,10 +681,16 @@ static int SYSCON_SET_FUNC_ATTR mcux_lpc_syscon_clock_control_set_subsys_rate( | |
| static int mcux_lpc_syscon_clock_control_configure(const struct device *dev, | ||
| clock_control_subsys_t sub_system, void *data) | ||
| { | ||
| uint32_t clock_name = 0; | ||
| int flexcomm_num = -1; | ||
|
|
||
| lpc_syscon_get_name(sub_system, &clock_name); | ||
| if (ret) { | ||
| return ret; | ||
| } | ||
|
|
||
| #ifdef CONFIG_SOC_SERIES_RW6XX | ||
| #define FLEXCOMM_LP_CLK_DECODE(n) (n & 0x80) | ||
| uint32_t clock_name = (uint32_t)sub_system; | ||
| int flexcomm_num = -1; | ||
|
|
||
| switch (clock_name) { | ||
| case MCUX_FLEXCOMM0_CLK: | ||
|
|
||
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.
Considering 99.99% of uage will be with with prop=
clocks, I'd make specialized versions of the macro that include it directly, e.g.Ditto for
DEFINE.Uh oh!
There was an error while loading. Please reload this page.
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.
Originally when I was making this PR, I did do something about this where you optionally don't have to say "clocks", by using variable arguments but I decided for first pass to just be explicit