Skip to content

[microTVM][Zephyr] Fix PLL freq. in overlay for nucleo_l4r5zi board #12756

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
mehrdadh merged 2 commits into from
Sep 14, 2022
Merged

[microTVM][Zephyr] Fix PLL freq. in overlay for nucleo_l4r5zi board #12756

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6eafc97
[microTVM][Zephyr] Fix PLL freq. in overlay for nucleo_l4r5zi board
gromero Sep 12, 2022
64f23d8
Remove div-p and div-q properties from overlay
gromero Sep 14, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 26 additions & 0 deletions apps/microtvm/zephyr/template_project/app-overlay/nucleo_l4r5zi.overlay
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,3 +21,29 @@
&rcc {
clock-frequency = <DT_FREQ_M(120)>;
};

/* Set PLL, where:

VCO freq = PLL clock input freq (HSI: 16 MHz) * N / M,
Core freq = VCO freq / R,
PLL48M1CLK freq = VCO freq / Q, and
PLLSAI3CLK freq = VCO freq / P,

Hence, since div-q = 2 => Q = 6 and div-p = 7 => P = 7:

VCO freq = 16 * 30 / 2 = 240 MHz

Core freq = 240 MHz / 2 = 120 MHz
PLL48M1CLK freq = 240 MHz / PLLQ = 40 MHz
PLLSAI3CLK freq = 240 MHz / PLLP = 34.28571 MHz
*/

&pll {
div-m = <2>;
mul-n = <30>;
div-p = <7>;
div-q = <2>;

@erwango erwango Sep 13, 2022

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: To save power, you can just remove these if these PLL outputs are not required.

@gromero gromero Sep 13, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

@erwango erwango Sep 14, 2022

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

div-r = <2>;
clocks = <&clk_hsi>;
status = "okay";
};