Hi Manorit,
On 19/11/24 11:42, Manorit Chawdhry wrote:
Hi Aniket,
On 06:02-20241119, Aniket Limaye wrote:
The default j7200 devicetree and k3_avs driver set 2GHz/1GHz frequency for A72/MSMC clks and the OPP_NOM voltage.
J7200 SOCs may support OPP_LOW Operating Performance Point: 1GHz/500MHz clks for A72/MSMC and OPP_LOW AVS voltage read from efuse.
Hence, add a config check in board_init_f() to select OPP_LOW specs:
- Check if OPP_LOW AVS voltage read from efuse is valid.
- Use the device IDs and clock IDs (TISCI docs [0]) to find the A72 and MSMC clock frequencies in the devicetree.
- Fixup the clock frequencies in devicetree as per OPP_LOW spec.
k3_avs driver programs the OPP_LOW AVS voltage for VDD_CPU through k3_avs_notify_freq() callback from clk_k3.
Signed-off-by: Aniket Limaye a-limaye@ti.com
[..]
arch/arm/mach-k3/j721e/j721e_init.c | 92 ++++++++++++++++++++++++++++- 1 file changed, 91 insertions(+), 1 deletion(-)
diff --git a/arch/arm/mach-k3/j721e/j721e_init.c b/arch/arm/mach-k3/j721e/j721e_init.c index e9ed8cb267c..805b28af8e4 100644 --- a/arch/arm/mach-k3/j721e/j721e_init.c +++ b/arch/arm/mach-k3/j721e/j721e_init.c @@ -19,6 +19,7 @@
[..]
+#if defined(CONFIG_CPU_V7R) && defined(CONFIG_K3_AVS0) +static int get_clock_index_by_dev_id(ofnode node, u32 dev_id, u32 clk_id)
I think the following function can be re-usable by other K3 socs too, can we put it in some common place instead of j721e_init? Maybe clk-k3.c and exported from there?
Yeah that does make sense to do... There are already similar usages, I think, in other k3 platforms. Having said that I would like to take that up as a separate series since the DT index search is not directly related to OPP_LOW Perf Point setting for j7200.
Let me know if you think that the k3 DT clock index search belongs in the same series itself...
+static int fdt_fixup_a72ss_clock_frequency(void) +{
- int index, size;
- u32 *rates;
- ofnode node;
- node = ofnode_by_compatible(ofnode_null(), "ti,am654-rproc");
- if (!ofnode_valid(node)) {
printf("%s: A72 not found\n", __func__);return -ENODEV;- }
- rates = fdt_getprop_w(ofnode_to_fdt(node), ofnode_to_offset(node),
"assigned-clock-rates", &size);- if (!rates) {
printf("%s: Wrong A72 assigned-clocks-rates configuration\n", __func__);return -1;- }
- /* Update A72 Clock Frequency to OPP_LOW spec */
- index = get_clock_index_by_dev_id(node,
DEV_A72SS0_CORE0_0_ID,DEV_A72SS0_CORE0_0_ARM_CLK_CLK_ID);- if (index < 0 || index >= (size / sizeof(u32))) {
When would we encounter the second if..?
Assuming you mean: index >= (size / sizeof(u32))
The RHS is the count of "assigned-clock-rates" in the node and the LHS (index) is the id of the "assigned-clocks". It's possible (and I see it in a few cases too) that count of "assigned-clocks" > count of "assigned-clock-rates". In that case the "index" here, even if parsed correctly, could turn out to be outside of the bounds of the array "rates". Which should be check against.
Regards, Aniket
printf("%s: Wrong A72 assigned-clocks configuration\n", __func__);return -1;- }
- rates[index] = cpu_to_fdt32(1000000000);
- printf("Changed A72 CPU frequency to %dHz in DT\n", 1000000000);
- /* Update MSMC Clock Frequency to OPP_LOW spec */
- index = get_clock_index_by_dev_id(node,
DEV_A72SS0_CORE0_ID,DEV_A72SS0_CORE0_MSMC_CLK_ID);- if (index < 0 || index >= (size / sizeof(u32))) {
printf("%s: Wrong A72 assigned-clocks configuration\n", __func__);return -1;- }
- rates[index] = cpu_to_fdt32(500000000);
- printf("Changed MSMC frequency to %dHz in DT\n", 500000000);
- return 0;
+} +#endif
[..]
Regards, Manorit