Draft: STM32 periph clk: centralize conditional code #20532
Conversation
| { | ||
| assert(periph); | ||
|
|
||
| const int irq_state = irq_disable(); |
There was a problem hiding this comment.
the existing periph_clk_en() implementation (I believe) is not thread safe. So I have added critical sections to this implementation. Is my thinking correct that this is needed?
| @@ -113,6 +113,12 @@ typedef enum { | |||
| #endif | |||
| } bus_t; | |||
|
|
|||
| typedef struct periph_t | |||
There was a problem hiding this comment.
oops :)
| typedef struct periph_t | |
| typedef struct |
| { | ||
| volatile uint32_t *en_reg; | ||
| uint32_t en_mask; | ||
| } periph_t; |
There was a problem hiding this comment.
thoughts on the name of this sturct?
There was a problem hiding this comment.
the name seems a bit generic, at least the usage in this PR seems like rcc_register_t or more specific
cpu/stm32/include/periph/cpu_timer.h
Outdated
| static const periph_t periph_timer2 = { | ||
| #if defined(RCC_APB1ENR_TIM2EN) | ||
| .en_reg = &RCC->APB1ENR, | ||
| .en_mask = RCC_APB1ENR_TIM2EN, | ||
| #elif defined(RCC_APB1ENR1_TIM2EN) | ||
| .en_reg = &RCC->APB1ENR1, | ||
| .en_mask = RCC_APB1ENR1_TIM2EN, | ||
| #elif defined(RCC_MC_APB1ENSETR_TIM2EN) | ||
| .en_reg = &RCC->MC_APB1ENSETR, | ||
| .en_mask = RCC_MC_APB1ENSETR_TIM2EN, | ||
| #endif | ||
| }; | ||
|
|
There was a problem hiding this comment.
Is this the right file to define this in?
cpu/stm32/include/periph/cpu_timer.h
Outdated
| /** | ||
| * @brief Timer configuration | ||
| */ | ||
| typedef struct { | ||
| TIM_TypeDef *dev; /**< timer device */ | ||
| const periph_t *rcc_dev; |
There was a problem hiding this comment.
Is there a better name for this pointer?
There was a problem hiding this comment.
| const periph_t *rcc_dev; | |
| const rcc_register_t *rcc_register; |
cpu/stm32/include/periph/cpu_timer.h
Outdated
| uint32_t rcc_mask; /**< corresponding bit in the RCC register */ | ||
| uint8_t bus; /**< APBx bus the timer is clock from */ |
There was a problem hiding this comment.
These will go away when this work is complete.
| { | ||
| volatile uint32_t *en_reg; /**< RCC enable reg */ | ||
| uint32_t en_mask; /**< RCC enable reg mask */ | ||
| } periph_base_t; |
There was a problem hiding this comment.
While every/most periph need to be clocked via the rcc this is certainly not the base of that peripheral, but more like a "RCC configuration" of said periph. (rcc_config_t or periph_clock_config_t)
If the base of an periph would be pointed at somewhere, I would expect is to point to that periphs base adress eg: for timer 1 to TIM1_CR1 (add an offset to that base and you find the other registers)
There was a problem hiding this comment.
Makes since. I will fix it.
|
I now believe that this approach was flawed. I have created a PR with an alternate approach (#20609), which is better in my opinion. |
Contribution description
This is a work in progress. I wanted to put it out there to get some feedback. See review comments for areas where feedback is especially requested.
Testing procedure
TBD
Issues/PRs references
This work supports my work to port RIOT to the STM32H7 family.