kinetis: Set LPUART clock source during uart_lpuart_init#8724
kinetis: Set LPUART clock source during uart_lpuart_init#8724jnohlgard merged 2 commits intoRIOT-OS:masterfrom
Conversation
jnohlgard
added
Platform: ARM
kYc0o
left a comment
kYc0o
left a comment
There was a problem hiding this comment.
I have some small doubts but looks ok in general.
| #define SIM_SOPT2_LPUART0SRC_MASK SIM_SOPT2_LPUARTSRC_MASK | ||
| #define SIM_SOPT2_LPUART0SRC_SHIFT SIM_SOPT2_LPUARTSRC_SHIFT | ||
| #define SIM_SOPT2_LPUART0SRC SIM_SOPT2_LPUARTSRC | ||
| #endif |
There was a problem hiding this comment.
Can these defines be aligned? Just to see that they're actually defining a value.
|
|
||
| /* Remember to select a module clock in board_init! (SIM->SOPT2[LPUART0SRC]) */ | ||
| /* Set LPUART clock source */ | ||
| #ifdef SIM_SOPT2_LPUART0SRC |
There was a problem hiding this comment.
The value of this definition doesn't matter? It only needs to be defined to execute the code below? It's just because above there is:
#define SIM_SOPT2_LPUART0SRC SIM_SOPT2_LPUARTSRCthus the value might be needed or evaluated.
There was a problem hiding this comment.
They are defined by the vendor headers for the CPUs which have the appropriate hardware module. The definition in cpu_conf_kinetis.h is only a compatibility definition for some CPU headers which have named the bit field LPUARTSRC instead of LPUART0SRC. See the other definitions in the same area in cpu_conf_kinetis.h.
|
|
||
| #ifndef LPUART_0_SRC | ||
| /* Default LPUART clock setting to avoid compilation failures, define this in | ||
| * periph_conf.h to set board specific configuration if using the LPUART. */ |
There was a problem hiding this comment.
Maybe this comment can be above the #ifndef?
| SIM_SOPT2_LPUART0SRC(LPUART_0_SRC); | ||
| } | ||
| #endif | ||
| #ifdef SIM_SOPT2_LPUART1SRC |
There was a problem hiding this comment.
Though I see a define for SIM_SOPT2_LPUART0SRC, I can't find for SIM_SOPT2_LPUART1SRC. Is this intentional? Same for SIM_SOPT2_LPUART1SRC_MASK used below, since there is a definition for SIM_SOPT2_LPUART0SRC_MASK.
There was a problem hiding this comment.
LPUART1SRC only exist for CPUs which have two or more lpuart modules (LPUART0, LPUART1). Therefore we need the preprocessor conditional to avoid compilation errors on the CPUs which don't have these modules.
There was a problem hiding this comment.
Ok I see, that's right for me.
|
@kYc0o aligned definitions in cpu_conf_kinetis.h |
jnohlgard
force-pushed
the
pr/kinetis-lpuart-clocking
branch
from
65b4ac7 to
fea2d71
Compare
|
Perfect, I think everything is in order here, so you may squash and merge once everything is green. |
jnohlgard
force-pushed
the
pr/kinetis-lpuart-clocking
branch
from
fea2d71 to
2bdf0bf
Compare
|
@kYc0o squashed, missing ACK |
2 participants
Contribution description
The Kinetis LPUART hardware module can choose from multiple clock sources, to use the module the user needs to select a clock source by setting a bitfield in the SIM_SOPT2 register. This PR moves this configuration into the periph uart driver, to reduce the code duplication in board_init between different boards and to make it easier to switch UART settings on an existing board.
The only existing board which has a LPUART module is the FRDM-K22F, but the module is not used in the default configuration, so this PR is mainly in preparation for the FRDM-KW41Z in #6995 which only has LPUART type UART hardware modules.
Issues/PRs references
Split from #6995
Required by #6995 for functional UART.