[PATCH v2 0/5] Add OPP_LOW support for J7200
This series adds OPP_LOW spec data in k3_avs driver and enables a config option to select the OPP_LOW performance point.
J7200 SOC supports OPP_LOW and OPP_NOM as two Operating Performance Points as per (7.5 Operating Performance Points) section in the Datasheet [0]. - A72SS/MSMC at 2 GHz/1GHz operation must use OPP_NOM. - A72SS/MSMC at 1 GHz/500 MHz operation can use OPP_NOM or OPP_LOW voltage (though OPP_LOW voltage is recommended to reduce power consumption).
The actual OPP voltage for the device is read from the efuse and updated in k3_avs_probe().
The default j7200 devicetree and k3_avs driver set OPP_NOM spec frequency and voltage.
In the board init file, if K3_OPP_LOW config is enabled, Check if OPP_LOW AVS voltage read from efuse is valid and update frequency (A72 and MSMC) and voltage (VDD_CPU) as per the OPP_LOW spec.
[0]: https://www.ti.com/lit/gpn/dra821u (J7200 Datasheet)
Test logs: https://gist.github.com/aniket-l/ec76679ebcb399cf10426bd7a4a154c3 With series applied on master plus below: - CONFIG_K3_OPP_LOW enabled in config - Additional helpful prints added in code - Logs shown with and without efuse register programmed for OPP_0 (Errors out if OPP_0 not found, programs OPP_LOW spec if found)
--- v2: * Neha - Fix indentation - Updates to commit msgs
- Re-format patches 3/5 and 4/5 with logical changes in each patch
- Link to v1: https://lore.kernel.org/u-boot/20241017062911.2241167-1-a-limaye@ti.com/
Reid Tonking (5): arm: dts: k3-j7200-r5-common: Add msmc clk to a72 node misc: k3_avs: Add OPP_LOW voltage and frequency to vd_data misc: k3_avs: Check validity of efuse voltage data arm: mach-k3: j721e-init.c: J7200: Add support for CONFIG_K3_OPP_LOW configs: j7200_evm_r5_defconfig: Define K3_OPP_LOW
.../arm/dts/k3-j7200-r5-common-proc-board.dts | 10 ++--- arch/arm/mach-k3/Kconfig | 6 +++ arch/arm/mach-k3/j721e/j721e_init.c | 45 ++++++++++++++++++- configs/j7200_evm_r5_defconfig | 1 + drivers/misc/k3_avs.c | 34 ++++++++++++++ include/k3-avs.h | 2 + 6 files changed, 92 insertions(+), 6 deletions(-)
From: Reid Tonking reidt@ti.com
The j7200 SOC has a single DDR controller and hence no need for configuring the MSMC interleaver. Hence we do not have an explicit node for MSMC in j7200 DT, unlike j721s2/j784s4.
Also, MSMC clk id is described under A72SS0_CORE0 Device in TISCI documentation [0].
Considering the above, define the MSMC clk in the a72 node.
[0]: https://software-dl.ti.com/tisci/esd/latest/5_soc_doc/j7200/clocks.html#cloc...
Signed-off-by: Reid Tonking reidt@ti.com Signed-off-by: Aniket Limaye a-limaye@ti.com
--- v2: - Update commit msg to justify location of msmc clk in DT - Link to v1: https://lore.kernel.org/u-boot/20241017062911.2241167-2-a-limaye@ti.com/ --- arch/arm/dts/k3-j7200-r5-common-proc-board.dts | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/arch/arm/dts/k3-j7200-r5-common-proc-board.dts b/arch/arm/dts/k3-j7200-r5-common-proc-board.dts index f096b102793..759a1e83456 100644 --- a/arch/arm/dts/k3-j7200-r5-common-proc-board.dts +++ b/arch/arm/dts/k3-j7200-r5-common-proc-board.dts @@ -23,11 +23,11 @@ <&k3_pds 202 TI_SCI_PD_EXCLUSIVE>, <&k3_pds 4 TI_SCI_PD_EXCLUSIVE>; resets = <&k3_reset 202 0>; - clocks = <&k3_clks 61 1>, <&k3_clks 202 2>; - clock-names = "gtc", "core"; - assigned-clocks = <&k3_clks 202 2>, <&k3_clks 61 1>, <&k3_clks 323 0>; - assigned-clock-parents= <0>, <0>, <&k3_clks 323 2>; - assigned-clock-rates = <2000000000>, <200000000>; + clocks = <&k3_clks 61 1>, <&k3_clks 4 1>, <&k3_clks 202 2> ; + clock-names = "gtc", "msmc", "core"; + assigned-clocks = <&k3_clks 202 2>, <&k3_clks 61 1>, <&k3_clks 4 1>, <&k3_clks 323 0>; + assigned-clock-parents= <0>, <0>, <0>, <&k3_clks 323 2>; + assigned-clock-rates = <2000000000>, <200000000>, <1000000000>; ti,sci = <&dmsc>; ti,sci-proc-id = <32>; ti,sci-host-id = <10>;
From: Reid Tonking reidt@ti.com
J7200 SOC supports OPP_LOW and OPP_NOM as two Operating Performance Points as per (7.5 Operating Performance Points) section in the Datasheet [0]. - A72SS/MSMC at 2 GHz/1GHz operation must use OPP_NOM. - A72SS/MSMC at 1 GHz/500 MHz operation can use OPP_NOM or OPP_LOW voltage (though OPP_LOW voltage is recommended to reduce power consumption).
Add OPP_LOW frequency->voltage entry to vd_data.
The actual OPP voltage for the device is read from the efuse and updated in k3_avs_probe(). OPP_NOM corresponds to OPP_1 and OPP_LOW to OPP_0 efuse register fields, as described in the Datasheet [0] The register offsets and fields are described in the TRM (5.2.6.1.5 WKUP_VTM_VD_OPPVID_j Register) [1].
[0]: https://www.ti.com/lit/gpn/dra821u (J7200 Datasheet) [1]: https://www.ti.com/lit/pdf/spruiu1 (J7200 TRM)
Signed-off-by: Reid Tonking reidt@ti.com Signed-off-by: Aniket Limaye a-limaye@ti.com --- drivers/misc/k3_avs.c | 4 ++++ include/k3-avs.h | 1 + 2 files changed, 5 insertions(+)
diff --git a/drivers/misc/k3_avs.c b/drivers/misc/k3_avs.c index 99a18a109b7..9d950d034a5 100644 --- a/drivers/misc/k3_avs.c +++ b/drivers/misc/k3_avs.c @@ -501,6 +501,10 @@ static struct vd_data j721e_vd_data[] = { .dev_id = 202, /* J721E_DEV_A72SS0_CORE0 */ .clk_id = 2, /* ARM clock */ .opps = { + [AM6_OPP_LOW] = { + .volt = 0, /* voltage TBD after OPP fuse reading */ + .freq = 1000000000, + }, [AM6_OPP_NOM] = { .volt = 880000, /* TBD in DM */ .freq = 2000000000, diff --git a/include/k3-avs.h b/include/k3-avs.h index 1014d5d114d..f6f1031c9cc 100644 --- a/include/k3-avs.h +++ b/include/k3-avs.h @@ -20,6 +20,7 @@
#define NUM_OPPS 4
+#define AM6_OPP_LOW 0 #define AM6_OPP_NOM 1 #define AM6_OPP_OD 2 #define AM6_OPP_TURBO 3
From: Reid Tonking reidt@ti.com
k3_avs driver checks opp_ids when probing and overwrites the voltage values in vd_data for the respective board. The new k3_check_opp() can be called from board files to check the efuse data and returns 0 if valid.
Also add the same check in k3_avs_program_voltage() to error out if the efuse data was not valid.
Signed-off-by: Reid Tonking reidt@ti.com Signed-off-by: Aniket Limaye a-limaye@ti.com
--- v2: - Update to also handle invalid efuse voltage reading in k3_avs_program_voltage() and reflect the same change in commit msg. - Link to v1: https://lore.kernel.org/u-boot/20241017062911.2241167-4-a-limaye@ti.com/ --- drivers/misc/k3_avs.c | 30 ++++++++++++++++++++++++++++++ include/k3-avs.h | 1 + 2 files changed, 31 insertions(+)
diff --git a/drivers/misc/k3_avs.c b/drivers/misc/k3_avs.c index 9d950d034a5..32eaaf37846 100644 --- a/drivers/misc/k3_avs.c +++ b/drivers/misc/k3_avs.c @@ -121,6 +121,11 @@ static int k3_avs_program_voltage(struct k3_avs_privdata *priv, if (!vd->supply) return -ENODEV;
+ if (!volt) { + dev_err(priv->dev, "No efuse found for opp_%d\n", opp_id); + return -EINVAL; + } + vd->opp = opp_id; vd->flags |= VD_FLAG_INIT_DONE;
@@ -192,6 +197,31 @@ static int match_opp(struct vd_data *vd, u32 freq) return -EINVAL; }
+/** + * k3_check_opp: Check for presence of opp efuse + * @opp_id: opp id to check if voltage is present + * + * Checks to see if an opp has voltage. k3_avs probe will populate + * votlage data if efuse is present. Returns 0 if data is valid. + */ +int k3_check_opp(struct udevice *dev, int vdd_id, int opp_id) +{ + struct k3_avs_privdata *priv = dev_get_priv(dev); + struct vd_data *vd; + int volt; + + vd = get_vd(priv, vdd_id); + if (!vd) + return -EINVAL; + + volt = vd->opps[opp_id].volt; + if (volt) + return 0; + + printf("No efuse found for opp_%d\n", opp_id); + return -EINVAL; +} + /** * k3_avs_notify_freq: Notify clock rate change towards AVS subsystem * @dev_id: Device ID for the clock to be changed diff --git a/include/k3-avs.h b/include/k3-avs.h index f6f1031c9cc..41a1da36356 100644 --- a/include/k3-avs.h +++ b/include/k3-avs.h @@ -27,5 +27,6 @@
int k3_avs_set_opp(struct udevice *dev, int vdd_id, int opp_id); int k3_avs_notify_freq(int dev_id, int clk_id, u32 freq); +int k3_check_opp(struct udevice *dev, int vdd_id, int opp_id);
#endif
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)) { + 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)
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
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
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
From: Reid Tonking reidt@ti.com
Adds the default config for K3_OPP_LOW in J7200
Signed-off-by: Reid Tonking reidt@ti.com Signed-off-by: Aniket Limaye a-limaye@ti.com --- arch/arm/mach-k3/Kconfig | 6 ++++++ configs/j7200_evm_r5_defconfig | 1 + 2 files changed, 7 insertions(+)
diff --git a/arch/arm/mach-k3/Kconfig b/arch/arm/mach-k3/Kconfig index f3f42b39213..a095d8a29ae 100644 --- a/arch/arm/mach-k3/Kconfig +++ b/arch/arm/mach-k3/Kconfig @@ -103,6 +103,12 @@ config SYS_K3_BOOT_CORE_ID int default 16
+config K3_OPP_LOW + bool "Enable OPP_LOW on supported TI K3 SoCs" + help + Enabling this will allow Socs with the proper efuse to run at a lower + MPU core voltage and adjust frequency according to SoC trm + config K3_EARLY_CONS bool "Activate to allow for an early console during SPL" depends on SPL diff --git a/configs/j7200_evm_r5_defconfig b/configs/j7200_evm_r5_defconfig index 1db92512dc2..bf6f3c6136d 100644 --- a/configs/j7200_evm_r5_defconfig +++ b/configs/j7200_evm_r5_defconfig @@ -102,6 +102,7 @@ CONFIG_K3_SEC_PROXY=y CONFIG_FS_LOADER=y CONFIG_SPL_FS_LOADER=y CONFIG_K3_AVS0=y +# CONFIG_K3_OPP_LOW is not set CONFIG_SUPPORT_EMMC_BOOT=y CONFIG_SPL_MMC_HS400_SUPPORT=y CONFIG_MMC_SDHCI=y
participants (2)
-
Aniket Limaye -
Manorit Chawdhry