Skip to content

Conversation

kenbell
Copy link
Member

@kenbell kenbell commented Mar 27, 2021

See #1750

This is the first step in cleaning up the PLL and oscillator initialization logic for STM32. This first step moves the code into the machine package to avoid linker hacks enabling the tick count to be used by existing logic in the machine package.

This change has been verified on:
bluepill (stm32f103)
nucleo-f722ze
nucleo-l552ze
nucleo-l432kc
nucleo-l031k6

@kenbell kenbell changed the title Draft: Move osc clk to machine pkg Draft: Move osc and clk to machine pkg Mar 27, 2021
@kenbell kenbell force-pushed the move-osc-clk-to-machine-pkg branch 2 times, most recently from 5618222 to fd7000b Compare March 29, 2021 05:58
@kenbell kenbell force-pushed the move-osc-clk-to-machine-pkg branch from fd7000b to b7cb928 Compare April 10, 2021 14:31
@kenbell kenbell marked this pull request as ready for review April 10, 2021 15:21
@kenbell kenbell changed the title Draft: Move osc and clk to machine pkg stm32: Move osc and clk to machine pkg Apr 10, 2021
@kenbell
Copy link
Member Author

kenbell commented Apr 10, 2021

It failed on assert-test-linux, but I can't see the build output. I'm assuming this is a problem with the CI process?

@kenbell
Copy link
Member Author

kenbell commented Apr 10, 2021

@ofauchon - fyi, since I know you're working on another STM32

@deadprogram
Copy link
Member

I think this is good. See the little diagram at #1750 (comment) for how I understand the implications of this change.

@aykevl or anyone else have any comments before I merge?

@aykevl
Copy link
Member

aykevl commented Apr 13, 2021

Moving this to the machine package overall sounds like a good idea to me. However, there are a few things that may be better specified:

  • What are the semantics of InitializeClocks? Currently it has no documentation. I think it is only supposed to be called once by the runtime package, but nothing indicates this. Is user code allowed to call it? What should happen in such a case?
  • What is the value from Ticks() and is it API-stable? Is user code allowed to call it and what can it expect from it? Shouldn't user code use time.Now instead, and why is it exported in that case?

Here is a proposal:

  • Specify exactly what InitializeClocks is and is not. It's one option to say something like "don't touch this, it's only exported for the runtime".
  • Make generic timer implementations, possibly with the same API as the improved PWM support (machine: refactor PWM support #1121). Then the runtime could simply use one (or two) of the available timers for timekeeping, with a simple API (just like the runtime already calls some UART methods while the implementation lives in the machine package).

This second point may be too much for this PR, maybe it can be done separately.

The reason I'm so worried is that once you provide an API (even one that is not intended for use by user code), it is very hard to change it. Therefore, I very much prefer if the API is as simple, obvious and portable as possible so that there is no worry in changing the implementation later or porting code to a different chip. We already have a few APIs I'm not very happy about and would like to change, but changing it now is difficult. I would want to avoid to add this API to the list.

@kenbell
Copy link
Member Author

kenbell commented Apr 14, 2021

In my mind InitializeClocks and Ticks are exclusively for use by the runtime package. I would like for user code to have the ability to change the clock configuration, but wanted to hold that for a later PR since it needs thinking through in some depth.

In the current change, this is the full interface from machine to runtime: (see runtime_stm32.go)

  • InitializeClocks() - sets up the Oscillator & PLLs per a default (high-performance) config
  • UART0 - so putchar works
  • Ticks() - is the time in 'ticks' since start, used by the scheduler which works in 'ticks' not ns?
  • TimerSleep(ns) - to sleep for X ns to implement 'runtime.sleepTicks()`
  • TICKS_PER_NS - constant so runtime can convert between ticks and ns so runtime can convert

Ultimately, i think the tick counter needs to be in the machine package so that SPI, I2C, etc code can reliably implement timeouts and delays across the broad range of CPU clock speeds (since machine using tick counter from runtime would create a circular dependency loop). I can think of three ways to do this (there may be more):

  1. keep tick construct into runtime, and have the linker hack to expose to machine (current state)
  2. put the tick construct into machine (this change, or something like it)
  3. determine that all STM32's will use SysTick to implement ticks (not using timers)

Option 3 is what the ST HAL does. If machine exposed a 'Systick' peripheral which accumulated the tick count, then runtime could configure it - but machine would still have access.

The more I think about it, option 3 may be the cleaner way to do it, and it would free up a timer peripheral for app usage.

What do you think about option 3? - which I'd summarize as:

  • Expose PLLs and Oscillators from machine package with a clear / documented / clean interface
  • Change STM32 to use Systick (expose from machine package) for tick count
  • Expose STM32 timers as PWM devices
  • Either have runtime use Systick for sleep (reduce sleep resolution to 1 'tick') or allow runtime to use a PWM for higher-resolution sleep.

@deadprogram
Copy link
Member

@aykevl any feedback on the last comment from @kenbell?

@kenbell
Copy link
Member Author

kenbell commented May 23, 2021

Closing this as it's superseded by other work to abstract TIM objects.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants