v01d commented on pull request #2177:
URL: https://github.com/apache/incubator-nuttx/pull/2177#issuecomment-720642168
> Hi @v01d ,
>
> I wasn't aware of this, I thought that these functions were only used by
the start-up code.
>
> So, let me present some points. Let's think how we can make this better.
>
> * I did a search on were `stm32_clockenable()` is used. It is true
that it is called only from start-up code. No other code in STM32 arch is using
it. The only other use of this function is in `stm3210e-eval/src/stm32_idle.c`,
which is a board-level function. Maybe this specific board is to be fixed?
The calls I mention will be from IDLE loop (maybe not directly, maybe via
some other function) since that is the point when the board exits sleep and
needs to restart clock sources, PLLs, etc. That is the only point where there
will be clock changes (besides w.r.t. default clock when booting).
> * I think clock configuration needs different routines for the
start-up code, and for dynamically changing frequency. Starting the system
usually makes many assumptions for its state (that do not apply during
run-time). Also, usually, some of the initial configuration is not required to
be repeated later.
This is not so much a dynamic frequency change, it is during a very specific
moment and the goal is to restore whatever was changed before going to sleep.
Usually any waits for clock stability need to be waited again, but this will
change a lot on different chips/archs.
There are some cases (don't remember which one) where the initial clock
config was separate from a "reconfig" after sleep. But I think this is not done
for all chips. The problem is that there will be much repeated code between
these two routines. Maybe there should be checks before doing something with a
clock (for STM32L476 I remember I added a check before waiting for stability,
instead of blindly doing so). That may be better than repeating everything.
> * Based on the assumption that `stm32_stdclockconfig()` is called
outside `stm32_start()`, then core-overdrive is also broken on STM32F4 targets,
I think. It has similar requirements.
No idea about overdrive but it is likely a similar case. There was some
discussion about this recently on the list.
> * If we want to make this perfect (and take the most performance
available from these chips), then there should also be a way to state the
voltage range that the MCU works. Maybe in `board.h`?
You mean core clock voltage? This is done according to clock frequency as
per datasheet recommendations. Again, I did this for STM32L476, but I'm not
sure if it is 100% enabled as I didn't trust it while I encountered the issue I
mentioned, which turned out to be related to flash instead.
> * Having multiple `*_rcc.c` files is very very annoying when
maintaining code. Maybe we should first try to merge all these files in one?
This was discussed many times. Merging similarly looking code was many times
not wanted since it may end up in a very complex code needs to support every
variation of the SoC line and every specific chip. And if not considered right,
when adding support for a chip, it may break many others. I would welcome
simplification to improve maintenance, but be aware this is a very critical
part of NuttX and most likely would require testing with various chips if you
start modifying a lot of this part of the code.
This is one of the reasons as why the different STM32 families are
separated. I think ST does not make this simple since two chips of different
families may be similar in one aspect but very different in another.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
