| Message ID | 20241023130033.1826413-5-a-limaye@ti.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | Add OPP_LOW support for J7200 | expand |
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) > +{ > + 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.. 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 >
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? > > +{ > > + 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 > >
Hi Neha, Aniket, On 13:31-20241030, 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> > > > > > > --- [...] > > > > > > +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? Do you think ft_system_setup is a good place for doing this fixup btw? Was wondering that it would be good to know what all DT fixups are being done at some common place instead of multiple scattered places but not sure when does ft_system_setup kick in, would it be before we startup a72? Regards, Manorit
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 >>>
On 30/10/24 13:58, Manorit Chawdhry wrote: > Hi Neha, Aniket, > > On 13:31-20241030, 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> >>>> >>>> --- > [...] >>>> >>>> +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? > > Do you think ft_system_setup is a good place for doing this fixup btw? > Was wondering that it would be good to know what all DT fixups are being > done at some common place instead of multiple scattered places but not > sure when does ft_system_setup kick in, would it be before we startup > a72? So it looks like ft_system_setup is being called only before booting into linux Kernel. As discussed offline, another place to handle fdt_fixups cleanly could be spl_perform_fixups() in board/ti/j721e/evm.c I would agree that this dt fixup would be better placed there instead of in board_init_f() directly. Will try doing that. Thanks, Aniket > > Regards, > Manorit
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) +{ + 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)) { + 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); + 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)