[microTVM][Zephyr] Fix PLL freq. in overlay for nucleo_l4r5zi board #12756
Conversation
Commit 1d32c40 ("Add project overlay to overwrite device tree configs") added overlay for setting 'clock-frequency' property of node 'rcc' to 120 MHz, however to effectively change the PLL frequency that drivers the core it's necessary also to overlay the attributes for the 'pll' node. This commit does that. Signed-off-by: Gustavo Romero <[email protected]>
gromero
force-pushed
the
set_nucleo_l4r5zi_pll120
branch
from
487c32e to
6eafc97
Compare
| div-p = <7>; | ||
| div-q = <2>; |
There was a problem hiding this comment.
Note: To save power, you can just remove these if these PLL outputs are not required.
There was a problem hiding this comment.
@erwango Thanks a lot for the reivew!
Interesting, it's good to keep in mind so that using the PLL48M1CLK and PLLSAI3CLK clocks can effect the power consumption. Since microTVM is not using any SAI (although we can use it in future for some voice recognition model), SMMDC, or USB OTG subsystem I think we can turn off these clocks, yep. But I'm scratching my head because I understand that even if properties div-p and div-q are removed from the overlay the dtc compiler will anyway "merge" the overlay properties for node pll with the values for these properties found in https://github.com/zephyrproject-rtos/zephyr/blob/main/boards/arm/nucleo_l4r5zi/nucleo_l4r5zi.dts#L76-L77 which are the same as the ones found in the overlay. But I thought also that even if these tuning values are set in the RCC_PLLCFGR it would be necessary to actually enable them explicitly by setting bits PLLQEN -- bit 20 -- and PLLPEN -- bit 16 -- in that register to effectively enable the clocks. Is my assumption wrong? Or even should we disable explicitly these clocks to save power? I haven't check what's the state for these bits because I don't have that board on hands ...
Now, re-reading my comments about Q param/property in the patch I think it's wrong.div-q = 2 => Q = 6 looks wrong to me because the value for the property in the .dts/overlay file should be 6, not the value for bit field (2), according to the semantics in https://github.com/zephyrproject-rtos/zephyr/blob/main/dts/bindings/clock/st%2Cstm32l4-pll-clock.yaml#L68 . So actually it seems div-q = 2 implies Q = 0 (bit field in register equal to zero), so actually by setting div-q = 2 we have PLL48M1CLK freq = 240 / 2 = 120 Mhz, not 240 / 6 = 40 MHz as in my comment. The Reference manual says Caution: The software has to set these bits correctly not to exceed 120 MHz on this domain. and with div-q=2 we don't violate this (we have it as 120 MHz), but I wonder if we should actually use div-q=6 to "honor" the clock signal name which implies it's a 48 MHz or so clock?
Since @mehrdadh plans to squash this patch with #12741 and contribute back to Zephyr I also I'd like to know if this comment #12741 should be update to 120 MHz accordingly.
There was a problem hiding this comment.
I oversaw this indeed. Prop values are the values to be used in the computation and driver makes the translation in register space.
Regarding the need for a 48MHz this depends on your needs. If required this can impose the use of a 96MHz Max freq on core. But note that USB, entropy and sdmmc are the only consumer of 48MHz clock available today in main zephyr tree.
erwango
left a comment
erwango
left a comment
There was a problem hiding this comment.
One comment depending on your usage, otherwise LGTM
@erwango Thanks for the review! I've got some additional question about the |
Remove div-p and div-q properties from the overlay file since values for these properties will be inherited from the 'pll' that is overlaid. Since currently microTVM does not use any subsystem which relies on clocks associated to either P or Q params, these params can be left unchanged for now.
…pache#12756) * [microTVM][Zephyr] Fix PLL freq. in overlay for nucleo_l4r5zi board Commit 1d32c40 ("Add project overlay to overwrite device tree configs") added overlay for setting 'clock-frequency' property of node 'rcc' to 120 MHz, however to effectively change the PLL frequency that drivers the core it's necessary also to overlay the attributes for the 'pll' node. This commit does that. Signed-off-by: Gustavo Romero <[email protected]> * Remove div-p and div-q properties from overlay Remove div-p and div-q properties from the overlay file since values for these properties will be inherited from the 'pll' that is overlaid. Since currently microTVM does not use any subsystem which relies on clocks associated to either P or Q params, these params can be left unchanged for now. Signed-off-by: Gustavo Romero <[email protected]>
…12756) * [microTVM][Zephyr] Fix PLL freq. in overlay for nucleo_l4r5zi board Commit 1d32c40 ("Add project overlay to overwrite device tree configs") added overlay for setting 'clock-frequency' property of node 'rcc' to 120 MHz, however to effectively change the PLL frequency that drivers the core it's necessary also to overlay the attributes for the 'pll' node. This commit does that. Signed-off-by: Gustavo Romero <[email protected]> * Remove div-p and div-q properties from overlay Remove div-p and div-q properties from the overlay file since values for these properties will be inherited from the 'pll' that is overlaid. Since currently microTVM does not use any subsystem which relies on clocks associated to either P or Q params, these params can be left unchanged for now. Signed-off-by: Gustavo Romero <[email protected]>
3 participants
Commit 1d32c40 ("Add project overlay to overwrite device tree configs") added overlay for setting 'clock-frequency' property of node 'rcc' to 120 MHz, however to effectively change the PLL frequency that drivers the core it's necessary also to overlay the attributes for the 'pll' node. This commit does that.
Signed-off-by: Gustavo Romero [email protected]
cc @alanmacd @mehrdadh