Hi Manorit,
On 30/10/24 13:31, Manorit Chawdhry wrote:
Hi Aniket,
On 15:40-20241029, Manorit Chawdhry wrote:
Hi Aniket,
On 18:27-20241023, Aniket Limaye wrote:
From: Reid Tonking reidt@ti.com
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 to select OPP_LOW specs:
- Check if OPP_LOW AVS voltage read from efuse is valid.
- Update the clock frequencies in devicetree.
- Program the OPP_LOW AVS voltage for VDD_CPU.
Signed-off-by: Reid Tonking reidt@ti.com Signed-off-by: Aniket Limaye a-limaye@ti.com
v2:
- Fix indentation in fix_freq()
- Remove the efuse data check addition from this commit, as it's not related to adding support for CONFIG_K3_OPP_LOW. The same addition was moved to the previous patch in this series.
- Link to v1: https://lore.kernel.org/u-boot/20241017062911.2241167-5-a-limaye@ti.com/
arch/arm/mach-k3/j721e/j721e_init.c | 45 ++++++++++++++++++++++++++++- 1 file changed, 44 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..de10517bb27 100644 --- a/arch/arm/mach-k3/j721e/j721e_init.c +++ b/arch/arm/mach-k3/j721e/j721e_init.c @@ -19,6 +19,7 @@ #include <fdtdec.h> #include <mmc.h> #include <remoteproc.h> +#include <k3-avs.h>
#include "../sysfw-loader.h" #include "../common.h" @@ -147,6 +148,32 @@ static void setup_navss_nb(void) writel(NB_THREADMAP_BIT1, (uintptr_t)NAVSS0_NBSS_NB1_CFG_NB_THREADMAP); }
+int fix_freq(const void *fdt)
I think this can be static as well btw, maybe rename it to something more descriptive as well. fdt_fixup_a72_clock_frequency?
Yeah this can be made static. Will do. Rename also makes sense... Since it's updating A72SS0_CORE0_0_ARM_CLK & A72SS0_CORE0_MSMC_CLK, what do you think about:
static int fdt_fixup_a72ss_clock_frequency(const void *fdt) ?
+{
- int node, ret;
- u32 opp_low_freq[3];
- node = fdt_node_offset_by_compatible(fdt, -1, "ti,am654-rproc");
- if (node < 0) {
printf("%s: A72 not found\n", __func__);return node;- }
- /* j7200 opp low values according to data sheet */
- opp_low_freq[0] = cpu_to_fdt32(1000000000); /* 202-2 -> A72SS0_CORE0_0_ARM_CLK */
- opp_low_freq[1] = cpu_to_fdt32(200000000); /* 61-1 -> GTC0_GTC_CLK */
- opp_low_freq[2] = cpu_to_fdt32(500000000); /* 4-1 -> A72SS0_CORE0_MSMC_CLK */
- ret = fdt_setprop((void *)fdt, node, "assigned-clock-rates",
opp_low_freq, sizeof(opp_low_freq));- if (ret) {
printf("%s: Can not set value\n", __func__);return ret;- }
- return 0;
+}
- /*
- This uninitialized global variable would normal end up in the .bss section,
- but the .bss is cleared between writing and reading this variable, so move
@@ -301,8 +328,24 @@ void board_init_f(ulong dummy) #if defined(CONFIG_CPU_V7R) && defined(CONFIG_K3_AVS0) ret = uclass_get_device_by_driver(UCLASS_MISC, DM_DRIVER_GET(k3_avs), &dev);
- if (ret)
- if (!ret) {
if (IS_ENABLED(CONFIG_K3_OPP_LOW)) {These ifs can be combined into one ig.
ret = k3_check_opp(dev, J721E_VDD_MPU, AM6_OPP_LOW);if (!ret) {ret = fix_freq(gd->fdt_blob);if (ret)printf("Failed to set OPP_LOW frequency\n");ret = k3_avs_set_opp(dev, J721E_VDD_MPU, AM6_OPP_LOW);Even if fix_freq fails, do we want to cascade and still run k3_avs_set_opp? Not sure what the best way to handle this is but seeing a lot of nesting so wondering if there is any better way to handle this control flow..
For this I think it might be a good idea to take this chunk out into another function like setup_opp_low as well and keep it there, might be more readable that way IMHO.
Hmm okay, I re-evaluated what we are trying to do here. Below is the logical path that should be taken:
board_init_f(): -> if CONFIG_K3_OPP_LOW -> if valid efuse for OPP_LOW -> fix_freq() # Update freq to OPP_LOW spec clk-k3: ti_clk_set_rate() -> k3_avs_notify_freq() -> if freq matches OPP_LOW spec -> k3_avs_set_voltage() # Update volt to OPP_LOW spec
For that, I propose the following code instead:
ret = uclass_get_device_by_driver(...) if(ret){ print("AVS init failed: %d\n", ret); } else if IS_ENABLED(CONFIG_K3_OPP_LOW) { ret = k3_check_opp(dev, J721E_VDD_MPU, AM6_OPP_LOW); if (ret) { printf("OPP_LOW: k3_check_opp failed: %d\n", ret); } else { ret = fix_freq(gd->fdt_blob); if (ret) printf("OPP_LOW: fix_freq failed: %d\n", ret); } }
Proposed Changes: - Add error codes to prints - Move error prints (with error codes) before else conditions. Helps with code readability to map error print with the erroneous function - Remove k3_avs_set_opp() from here altogether. Reasoning being that the value being set through k3_avs_set_opp() will anyway be (correctly) overridden by the k3_avs_notify_freq() call later in the boot process, when a72 freq is actually set from clk_k3.
Regards, Aniket
Regards, Manorit
Also, better to print ret as well in these cases btw.
Regards, Manorit
if (ret)printf("Failed to set OPP_LOW voltage\n");} else {printf("Failed to enable K3_OPP_LOW\n");}}} else { printf("AVS init failed: %d\n", ret);
} #endif
#if defined(CONFIG_K3_J721E_DDRSS)
-- 2.34.1