gromero commented on code in PR #12756:
URL: https://github.com/apache/tvm/pull/12756#discussion_r969771963
##########
apps/microtvm/zephyr/template_project/app-overlay/nucleo_l4r5zi.overlay:
##########
@@ -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>;
Review 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 sho
uld 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
https://github.com/apache/tvm/pull/12741 and contribute back to Zephyr I also
I'd like to know if this comment https://github.com/apache/tvm/pull/12741
should be update to `120 MHz ` accordingly.
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
