Hi Aniket,
On 15:40-20241029, Manorit Chawdhry wrote:
> Hi Aniket,
>
> On 18:27-20241023, Aniket Limaye wrote:
> > From: Reid Tonking <re...@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 <re...@ti.com>
> > Signed-off-by: Aniket Limaye <a-lim...@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-lim...@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?
> > +{
> > + 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.
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
> >
