[PATCH v2 4/5] arm: mach-k3: j721e-init.c: J7200: Add support for CONFIG_K3_OPP_LOW

[Aniket Limaye]

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 at 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 at ti.com>
>>> Signed-off-by: Aniket Limaye <a-limaye at 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
>>>