Hi Aniket,
On 13:02-20241119, Aniket Limaye wrote:
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...
I mean you did make the function so I think might as well put it in the correct location but it's a non blocker for me, was just a suggestion. Would let you see how you want to do it.
+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.
Ah okay, thanks for explaining!
Reviewed-by: Manorit Chawdhry m-chawdhry@ti.com
Regards, Manorit
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