[PATCH 0/6] Add OPP_LOW support for J7200

[PATCH 0/6] Add OPP_LOW support for J7200

[Aniket Limaye]

This series adds OPP_LOW spec data in k3_avs driver and enables a config
option to select the OPP_LOW performance point.

AVS test logs (CONFIG_K3_OPP_LOW enabled and dummy efuse value):
https://gist.github.com/aniket-l/3bdb454d2416e1900dce795ce6090507

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 and
voltage as per the OPP_LOW spec.

[0]: https://www.ti.com/lit/gpn/dra821u  (J7200 Datasheet)


Reid Tonking (6):
  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: Add k3_check_opp function
  arm: mach-k3: j721e-init.c: J7200: Add support for CONFIG_K3_OPP_LOW
  configs: j7200_evm_r5_defconfig: Define K3_OPP_LOW
  DONOTMERGE: For testing only

 .../arm/dts/k3-j7200-r5-common-proc-board.dts | 10 ++--
 arch/arm/mach-k3/Kconfig                      |  6 +++
 arch/arm/mach-k3/j721e/j721e_init.c           | 54 ++++++++++++++++++-
 configs/j7200_evm_r5_defconfig                |  1 +
 drivers/misc/k3_avs.c                         | 36 +++++++++++++
 include/k3-avs.h                              |  2 +
 6 files changed, 103 insertions(+), 6 deletions(-)

-- 
2.34.1

[PATCH 1/6] arm: dts: k3-j7200-r5-common: Add msmc clk to a72 node

[Aniket Limaye]

From: Reid Tonking <reidt@ti.com>

Define the MSMC clk in the a72 node

Signed-off-by: Reid Tonking <reidt@ti.com>
Signed-off-by: Aniket Limaye <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>;
-- 
2.34.1

[PATCH 2/6] misc: k3_avs: Add OPP_LOW voltage and frequency to vd_data

[Aniket Limaye]

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
-- 
2.34.1

[PATCH 3/6] misc: k3_avs: Add k3_check_opp function

[Aniket Limaye]

From: Reid Tonking <reidt@ti.com>

Useful when trying to check if an opp efuse is burned in or not.

k3_avs driver checks opp_ids when probing and overwrites the voltage
values in vd_data for the respective board. This can be called to check
that data and returns 0 if valid.

Signed-off-by: Reid Tonking <reidt@ti.com>
Signed-off-by: Aniket Limaye <a-limaye@ti.com>
---
 drivers/misc/k3_avs.c | 25 +++++++++++++++++++++++++
 include/k3-avs.h      |  1 +
 2 files changed, 26 insertions(+)

diff --git a/drivers/misc/k3_avs.c b/drivers/misc/k3_avs.c
index 9d950d034a5..90cd9dfe7f9 100644
--- a/drivers/misc/k3_avs.c
+++ b/drivers/misc/k3_avs.c
@@ -192,6 +192,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
-- 
2.34.1

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

[Aniket Limaye]

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>
---
 arch/arm/mach-k3/j721e/j721e_init.c | 45 ++++++++++++++++++++++++++++-
 drivers/misc/k3_avs.c               |  5 ++++
 2 files changed, 49 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..0620759c36c 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)
diff --git a/drivers/misc/k3_avs.c b/drivers/misc/k3_avs.c
index 90cd9dfe7f9..932b355a5c1 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, "Fuse is not set for selected opp %d\n", opp_id);
+		return -EINVAL;
+	}
+
 	vd->opp = opp_id;
 	vd->flags |= VD_FLAG_INIT_DONE;
 
-- 
2.34.1

[PATCH 5/6] configs: j7200_evm_r5_defconfig: Define K3_OPP_LOW

[Aniket Limaye]

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 774a9eff439..7d43d8d702a 100644
--- a/configs/j7200_evm_r5_defconfig
+++ b/configs/j7200_evm_r5_defconfig
@@ -103,6 +103,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
-- 
2.34.1

[PATCH 6/6] DONOTMERGE: For testing only

[Aniket Limaye]

From: Reid Tonking <reidt@ti.com>

This just provides some useful print outs and the proper config to
test the functionality. For J7200 boards that do not have efuse
burned in, the hardcoded 760000 is needed in the vd_data, since
it would be populated otherwise

Signed-off-by: Reid Tonking <reidt@ti.com>
Signed-off-by: Aniket Limaye <a-limaye@ti.com>
---
 arch/arm/mach-k3/j721e/j721e_init.c | 9 +++++++++
 configs/j7200_evm_r5_defconfig      | 2 +-
 drivers/misc/k3_avs.c               | 4 +++-
 3 files changed, 13 insertions(+), 2 deletions(-)

diff --git a/arch/arm/mach-k3/j721e/j721e_init.c b/arch/arm/mach-k3/j721e/j721e_init.c
index 0620759c36c..19865f7dd40 100644
--- a/arch/arm/mach-k3/j721e/j721e_init.c
+++ b/arch/arm/mach-k3/j721e/j721e_init.c
@@ -152,6 +152,8 @@ int fix_freq(const void *fdt)
 {
 		int node, ret;
 		u32 opp_low_freq[3];
+		int len;
+		const u32 *reg;
 
 		node = fdt_node_offset_by_compatible(fdt, -1, "ti,am654-rproc");
 			if (node < 0) {
@@ -159,6 +161,11 @@ int fix_freq(const void *fdt)
 				return node;
 			}
 
+		/* check freqs */
+		reg = fdt_getprop(fdt, node, "assigned-clock-rates", &len);
+		printf("OPP_NOM: arm_clk:%d | gtc_clk:%d | msmc_clk:%d\n",
+				fdt32_to_cpu(reg[0]), fdt32_to_cpu(reg[1]), fdt32_to_cpu(reg[2]));
+
 		/* 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 */
@@ -170,6 +177,8 @@ int fix_freq(const void *fdt)
 				printf("%s: Can not set value\n", __func__);
 				return ret;
 			}
+		printf("OPP_LOW: arm_clk:%d | gtc_clk:%d | msmc_clk:%d\n",
+				fdt32_to_cpu(opp_low_freq[0]), fdt32_to_cpu(opp_low_freq[1]), fdt32_to_cpu(opp_low_freq[2]));
 
 		return 0;
 }
diff --git a/configs/j7200_evm_r5_defconfig b/configs/j7200_evm_r5_defconfig
index 7d43d8d702a..6fdf34c9644 100644
--- a/configs/j7200_evm_r5_defconfig
+++ b/configs/j7200_evm_r5_defconfig
@@ -103,7 +103,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_K3_OPP_LOW=y
 CONFIG_SUPPORT_EMMC_BOOT=y
 CONFIG_SPL_MMC_HS400_SUPPORT=y
 CONFIG_MMC_SDHCI=y
diff --git a/drivers/misc/k3_avs.c b/drivers/misc/k3_avs.c
index 932b355a5c1..e93aa267d51 100644
--- a/drivers/misc/k3_avs.c
+++ b/drivers/misc/k3_avs.c
@@ -121,6 +121,8 @@ static int k3_avs_program_voltage(struct k3_avs_privdata *priv,
 	if (!vd->supply)
 		return -ENODEV;
 
+  printf("Program voltage: opp_id:%d | opp_volt:%d\n", opp_id, volt);
+
 	if (!volt) {
 		dev_err(priv->dev, "Fuse is not set for selected opp %d\n", opp_id);
 		return -EINVAL;
@@ -532,7 +534,7 @@ static struct vd_data j721e_vd_data[] = {
 		.clk_id = 2, /* ARM clock */
 		.opps = {
 			[AM6_OPP_LOW] = {
-				.volt = 0, /* voltage TBD after OPP fuse reading */
+				.volt = 760000, /* voltage TBD after OPP fuse reading */
 				.freq = 1000000000,
 			},
 			[AM6_OPP_NOM] = {
-- 
2.34.1

Re: [PATCH 6/6] DONOTMERGE: For testing only

[Lothar Waßmann]

Hi,

On Thu, 17 Oct 2024 11:59:13 +0530 Aniket Limaye wrote:
> From: Reid Tonking <reidt@ti.com>
> 
> This just provides some useful print outs and the proper config to
> test the functionality. For J7200 boards that do not have efuse
> burned in, the hardcoded 760000 is needed in the vd_data, since
> it would be populated otherwise
> 
> Signed-off-by: Reid Tonking <reidt@ti.com>
> Signed-off-by: Aniket Limaye <a-limaye@ti.com>
> ---
>  arch/arm/mach-k3/j721e/j721e_init.c | 9 +++++++++
>  configs/j7200_evm_r5_defconfig      | 2 +-
>  drivers/misc/k3_avs.c               | 4 +++-
>  3 files changed, 13 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm/mach-k3/j721e/j721e_init.c b/arch/arm/mach-k3/j721e/j721e_init.c
> index 0620759c36c..19865f7dd40 100644
> --- a/arch/arm/mach-k3/j721e/j721e_init.c
> +++ b/arch/arm/mach-k3/j721e/j721e_init.c
> @@ -152,6 +152,8 @@ int fix_freq(const void *fdt)
>  {
>  		int node, ret;
>  		u32 opp_low_freq[3];
> +		int len;
> +		const u32 *reg;
>  
>  		node = fdt_node_offset_by_compatible(fdt, -1, "ti,am654-rproc");
>  			if (node < 0) {
> @@ -159,6 +161,11 @@ int fix_freq(const void *fdt)
>  				return node;
>  			}
>  
> +		/* check freqs */
> +		reg = fdt_getprop(fdt, node, "assigned-clock-rates", &len);
> +		printf("OPP_NOM: arm_clk:%d | gtc_clk:%d | msmc_clk:%d\n",
> +				fdt32_to_cpu(reg[0]), fdt32_to_cpu(reg[1]), fdt32_to_cpu(reg[2]));
> +
>  		/* 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 */
> @@ -170,6 +177,8 @@ int fix_freq(const void *fdt)
>  				printf("%s: Can not set value\n", __func__);
>  				return ret;
>  			}
> +		printf("OPP_LOW: arm_clk:%d | gtc_clk:%d | msmc_clk:%d\n",
> +				fdt32_to_cpu(opp_low_freq[0]), fdt32_to_cpu(opp_low_freq[1]), fdt32_to_cpu(opp_low_freq[2]));
>  
>  		return 0;
>  }
> diff --git a/configs/j7200_evm_r5_defconfig b/configs/j7200_evm_r5_defconfig
> index 7d43d8d702a..6fdf34c9644 100644
> --- a/configs/j7200_evm_r5_defconfig
> +++ b/configs/j7200_evm_r5_defconfig
> @@ -103,7 +103,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_K3_OPP_LOW=y
>  CONFIG_SUPPORT_EMMC_BOOT=y
>  CONFIG_SPL_MMC_HS400_SUPPORT=y
>  CONFIG_MMC_SDHCI=y
> diff --git a/drivers/misc/k3_avs.c b/drivers/misc/k3_avs.c
> index 932b355a5c1..e93aa267d51 100644
> --- a/drivers/misc/k3_avs.c
> +++ b/drivers/misc/k3_avs.c
> @@ -121,6 +121,8 @@ static int k3_avs_program_voltage(struct k3_avs_privdata *priv,
>  	if (!vd->supply)
>  		return -ENODEV;
>  
> +  printf("Program voltage: opp_id:%d | opp_volt:%d\n", opp_id, volt);
> +
buggy indentation.


Lothar Waßmann

Re: [PATCH 1/6] arm: dts: k3-j7200-r5-common: Add msmc clk to a72 node

[Neha Malcom Francis]

Hi Aniket

On 17/10/24 11:59, Aniket Limaye wrote:
> From: Reid Tonking <reidt@ti.com>
> 
> Define the MSMC clk in the a72 node

The usage of MSMC and A72SS interchangeably in this series is confusing. Could 
you expand on it in the cover letter why in J7200 this clock is defined as part 
of the A72 node as opposed to devices that have MSMC as a separate module 
altogether with its own clock (like J721S2 and J784S4)?
> 
> Signed-off-by: Reid Tonking <reidt@ti.com>
> Signed-off-by: Aniket Limaye <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>;

-- 
Thanking You
Neha Malcom Francis

Re: [PATCH 3/6] misc: k3_avs: Add k3_check_opp function

[Neha Malcom Francis]

Hi Aniket

On 17/10/24 11:59, Aniket Limaye wrote:
> From: Reid Tonking <reidt@ti.com>
> 
> Useful when trying to check if an opp efuse is burned in or not.
> 
> k3_avs driver checks opp_ids when probing and overwrites the voltage
> values in vd_data for the respective board. This can be called to check
> that data and returns 0 if valid.
> 
> Signed-off-by: Reid Tonking <reidt@ti.com>
> Signed-off-by: Aniket Limaye <a-limaye@ti.com>
> ---
>   drivers/misc/k3_avs.c | 25 +++++++++++++++++++++++++
>   include/k3-avs.h      |  1 +
>   2 files changed, 26 insertions(+)
> 
> diff --git a/drivers/misc/k3_avs.c b/drivers/misc/k3_avs.c
> index 9d950d034a5..90cd9dfe7f9 100644
> --- a/drivers/misc/k3_avs.c
> +++ b/drivers/misc/k3_avs.c
> @@ -192,6 +192,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)

Can this be a static function?

> +{
> +	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

-- 
Thanking You
Neha Malcom Francis

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

[Neha Malcom Francis]

Hi Aniket

On 17/10/24 11:59, 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>
> ---
>   arch/arm/mach-k3/j721e/j721e_init.c | 45 ++++++++++++++++++++++++++++-
>   drivers/misc/k3_avs.c               |  5 ++++
>   2 files changed, 49 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..0620759c36c 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;
> +			}

Indentation seems to be off.

> +
> +		/* 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)
> diff --git a/drivers/misc/k3_avs.c b/drivers/misc/k3_avs.c
> index 90cd9dfe7f9..932b355a5c1 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, "Fuse is not set for selected opp %d\n", opp_id);

s/Fuse/Efuse
> +		return -EINVAL;
> +	}
> +
>   	vd->opp = opp_id;
>   	vd->flags |= VD_FLAG_INIT_DONE;
>   

-- 
Thanking You
Neha Malcom Francis

Re: [PATCH 6/6] DONOTMERGE: For testing only

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 673 bytes --]

On Thu, Oct 17, 2024 at 11:59:13AM +0530, Aniket Limaye wrote:

> From: Reid Tonking <reidt@ti.com>
> 
> This just provides some useful print outs and the proper config to
> test the functionality. For J7200 boards that do not have efuse
> burned in, the hardcoded 760000 is needed in the vd_data, since
> it would be populated otherwise
> 
> Signed-off-by: Reid Tonking <reidt@ti.com>
> Signed-off-by: Aniket Limaye <a-limaye@ti.com>

I can usually remember to use "b4 shazam -P ..." to omit these patches,
but it's generally best to not include them and either mark the whole
series as RFC instead if anyone else is likely to use them, thanks.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH 6/6] DONOTMERGE: For testing only

[Aniket Limaye]

On 17/10/24 23:24, Tom Rini wrote:
> On Thu, Oct 17, 2024 at 11:59:13AM +0530, Aniket Limaye wrote:
> 
>> From: Reid Tonking <reidt@ti.com>
>>
>> This just provides some useful print outs and the proper config to
>> test the functionality. For J7200 boards that do not have efuse
>> burned in, the hardcoded 760000 is needed in the vd_data, since
>> it would be populated otherwise
>>
>> Signed-off-by: Reid Tonking <reidt@ti.com>
>> Signed-off-by: Aniket Limaye <a-limaye@ti.com>
> 
> I can usually remember to use "b4 shazam -P ..." to omit these patches,
> but it's generally best to not include them and either mark the whole
> series as RFC instead if anyone else is likely to use them, thanks.
> 

That makes sense. Got it, will remember to do that in the future.

Thanks,
Aniket

Re: [PATCH 3/6] misc: k3_avs: Add k3_check_opp function

[Limaye, Aniket]

On 10/17/2024 4:06 PM, Neha Malcom Francis wrote:
> Hi Aniket
> 
> On 17/10/24 11:59, Aniket Limaye wrote:
>> From: Reid Tonking <reidt@ti.com>
>>
>> Useful when trying to check if an opp efuse is burned in or not.
>>
>> k3_avs driver checks opp_ids when probing and overwrites the voltage
>> values in vd_data for the respective board. This can be called to check
>> that data and returns 0 if valid.
>>
>> Signed-off-by: Reid Tonking <reidt@ti.com>
>> Signed-off-by: Aniket Limaye <a-limaye@ti.com>
>> ---
>>   drivers/misc/k3_avs.c | 25 +++++++++++++++++++++++++
>>   include/k3-avs.h      |  1 +
>>   2 files changed, 26 insertions(+)
>>
>> diff --git a/drivers/misc/k3_avs.c b/drivers/misc/k3_avs.c
>> index 9d950d034a5..90cd9dfe7f9 100644
>> --- a/drivers/misc/k3_avs.c
>> +++ b/drivers/misc/k3_avs.c
>> @@ -192,6 +192,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)
> 
> Can this be a static function?

It is being used in the board file (j721e-init.c) in the following 
commit... to check if efuse was read correctly, before attempting to set 
an OPP.

> 
>> +{
>> +    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
> 

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

[Limaye, Aniket]

On 10/17/2024 4:10 PM, Neha Malcom Francis wrote:
> Hi Aniket
> 
> On 17/10/24 11:59, 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>
>> ---
>>   arch/arm/mach-k3/j721e/j721e_init.c | 45 ++++++++++++++++++++++++++++-
>>   drivers/misc/k3_avs.c               |  5 ++++
>>   2 files changed, 49 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..0620759c36c 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;
>> +            }
> 
> Indentation seems to be off.
> 

Ahh noted... will correct in v2.

>> +
>> +        /* 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)
>> diff --git a/drivers/misc/k3_avs.c b/drivers/misc/k3_avs.c
>> index 90cd9dfe7f9..932b355a5c1 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, "Fuse is not set for selected opp %d\n", 
>> opp_id);
> 
> s/Fuse/Efuse

Yep, will update.

Thanks,
Aniket


>> +        return -EINVAL;
>> +    }
>> +
>>       vd->opp = opp_id;
>>       vd->flags |= VD_FLAG_INIT_DONE;
> 

Re: [PATCH 1/6] arm: dts: k3-j7200-r5-common: Add msmc clk to a72 node

[Aniket Limaye]

On 17/10/24 16:00, Neha Malcom Francis wrote:
> Hi Aniket
> 
> On 17/10/24 11:59, Aniket Limaye wrote:
>> From: Reid Tonking <reidt@ti.com>
>>
>> Define the MSMC clk in the a72 node
> 
> The usage of MSMC and A72SS interchangeably in this series is confusing. 
> Could you expand on it in the cover letter why in J7200 this clock is 
> defined as part of the A72 node as opposed to devices that have MSMC as 
> a separate module altogether with its own clock (like J721S2 and J784S4)?

Yeah sure. will elaborate in v2:

The msmc clock frequency needs to be updated as per selected OPP (in 
PATCH 4). But we don't have a msmc node for j721e/j7200, unlike those in 
j721s2/j784s4. So we are defining the msmc clock in the a72_0 node, such 
that it's frequency can be updated along with core clock frequency when 
OPP_LOW config is selected.

I am open to suggestions if this is not the right place for it.
Thanks for the reviews!

Regards,
Aniket

>>
>> Signed-off-by: Reid Tonking <reidt@ti.com>
>> Signed-off-by: Aniket Limaye <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>;
> 

Re: [PATCH 1/6] arm: dts: k3-j7200-r5-common: Add msmc clk to a72 node

[Neha Malcom Francis]

Hi Aniket

On 23/10/24 11:42, Aniket Limaye wrote:
> 
> On 17/10/24 16:00, Neha Malcom Francis wrote:
>> Hi Aniket
>>
>> On 17/10/24 11:59, Aniket Limaye wrote:
>>> From: Reid Tonking <reidt@ti.com>
>>>
>>> Define the MSMC clk in the a72 node
>>
>> The usage of MSMC and A72SS interchangeably in this series is confusing. Could 
>> you expand on it in the cover letter why in J7200 this clock is defined as 
>> part of the A72 node as opposed to devices that have MSMC as a separate module 
>> altogether with its own clock (like J721S2 and J784S4)?
> 
> Yeah sure. will elaborate in v2:
> 
> The msmc clock frequency needs to be updated as per selected OPP (in PATCH 4). 
> But we don't have a msmc node for j721e/j7200, unlike those in j721s2/j784s4. So 
> we are defining the msmc clock in the a72_0 node, such that it's frequency can 
> be updated along with core clock frequency when OPP_LOW config is selected.
> 
> I am open to suggestions if this is not the right place for it.
> Thanks for the reviews!

Configuring the MSMC interleaver is only required in devices that have more than 
one DDR controller i.e. J721S2 and J784S4. This is what is being done by the 
MSMC driver present in U-Boot (see drivers/ram/k3-ddrss/k3-ddrss.c). This 
configuration is not needed in this device and is probably why we don't have an 
explicit MSMC node.

I was thinking the patch is okay initially considering this, but now am 
wondering whether this is a hacky way to set a clock and should we introduce the 
MSMC as a node cleanly, after all it is hardware that needs to be described. 
What do you think?

> 
> Regards,
> Aniket
> 
>>>
>>> Signed-off-by: Reid Tonking <reidt@ti.com>
>>> Signed-off-by: Aniket Limaye <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>;
>>

-- 
Thanking You
Neha Malcom Francis

Re: [PATCH 1/6] arm: dts: k3-j7200-r5-common: Add msmc clk to a72 node

[Aniket Limaye]

On 23/10/24 12:17, Neha Malcom Francis wrote:
> Hi Aniket
> 
> On 23/10/24 11:42, Aniket Limaye wrote:
>>
>> On 17/10/24 16:00, Neha Malcom Francis wrote:
>>> Hi Aniket
>>>
>>> On 17/10/24 11:59, Aniket Limaye wrote:
>>>> From: Reid Tonking <reidt@ti.com>
>>>>
>>>> Define the MSMC clk in the a72 node
>>>
>>> The usage of MSMC and A72SS interchangeably in this series is 
>>> confusing. Could you expand on it in the cover letter why in J7200 
>>> this clock is defined as part of the A72 node as opposed to devices 
>>> that have MSMC as a separate module altogether with its own clock 
>>> (like J721S2 and J784S4)?
>>
>> Yeah sure. will elaborate in v2:
>>
>> The msmc clock frequency needs to be updated as per selected OPP (in 
>> PATCH 4). But we don't have a msmc node for j721e/j7200, unlike those 
>> in j721s2/j784s4. So we are defining the msmc clock in the a72_0 node, 
>> such that it's frequency can be updated along with core clock 
>> frequency when OPP_LOW config is selected.
>>
>> I am open to suggestions if this is not the right place for it.
>> Thanks for the reviews!
> 
> Configuring the MSMC interleaver is only required in devices that have 
> more than one DDR controller i.e. J721S2 and J784S4. This is what is 
> being done by the MSMC driver present in U-Boot (see 
> drivers/ram/k3-ddrss/k3-ddrss.c). This configuration is not needed in 
> this device and is probably why we don't have an explicit MSMC node.
> 
> I was thinking the patch is okay initially considering this, but now am 
> wondering whether this is a hacky way to set a clock and should we 
> introduce the MSMC as a node cleanly, after all it is hardware that 
> needs to be described. What do you think?
> 

Yeah I am also not quite sure where the MSMC clk description should go: 
in a new separate node, or in a72_0.

However, MSMC clk id is described under A72SS0_CORE0 Device in TISCI 
documentation [0]. Which makes a case for putting this under a72_0 node 
itself (along with the fact that there isn't a separate msmc node for 
j721e/j7200 which have a single DDR controller). So I guess I vote for 
keeping it as is, under a72_0 node.

[0]: 
https://software-dl.ti.com/tisci/esd/latest/5_soc_doc/j7200/clocks.html#clocks-for-a72ss0-core0-device

Thanks,
Aniket

>>
>> Regards,
>> Aniket
>>
>>>>
>>>> Signed-off-by: Reid Tonking <reidt@ti.com>
>>>> Signed-off-by: Aniket Limaye <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>;
>>>
> 

Re: [PATCH 1/6] arm: dts: k3-j7200-r5-common: Add msmc clk to a72 node

[Neha Malcom Francis]

Hi Aniket

On 23/10/24 13:28, Aniket Limaye wrote:
> 
> 
> On 23/10/24 12:17, Neha Malcom Francis wrote:
>> Hi Aniket
>>
>> On 23/10/24 11:42, Aniket Limaye wrote:
>>>
>>> On 17/10/24 16:00, Neha Malcom Francis wrote:
>>>> Hi Aniket
>>>>
>>>> On 17/10/24 11:59, Aniket Limaye wrote:
>>>>> From: Reid Tonking <reidt@ti.com>
>>>>>
>>>>> Define the MSMC clk in the a72 node
>>>>
>>>> The usage of MSMC and A72SS interchangeably in this series is confusing. 
>>>> Could you expand on it in the cover letter why in J7200 this clock is 
>>>> defined as part of the A72 node as opposed to devices that have MSMC as a 
>>>> separate module altogether with its own clock (like J721S2 and J784S4)?
>>>
>>> Yeah sure. will elaborate in v2:
>>>
>>> The msmc clock frequency needs to be updated as per selected OPP (in PATCH 
>>> 4). But we don't have a msmc node for j721e/j7200, unlike those in 
>>> j721s2/j784s4. So we are defining the msmc clock in the a72_0 node, such that 
>>> it's frequency can be updated along with core clock frequency when OPP_LOW 
>>> config is selected.
>>>
>>> I am open to suggestions if this is not the right place for it.
>>> Thanks for the reviews!
>>
>> Configuring the MSMC interleaver is only required in devices that have more 
>> than one DDR controller i.e. J721S2 and J784S4. This is what is being done by 
>> the MSMC driver present in U-Boot (see drivers/ram/k3-ddrss/k3-ddrss.c). This 
>> configuration is not needed in this device and is probably why we don't have 
>> an explicit MSMC node.
>>
>> I was thinking the patch is okay initially considering this, but now am 
>> wondering whether this is a hacky way to set a clock and should we introduce 
>> the MSMC as a node cleanly, after all it is hardware that needs to be 
>> described. What do you think?
>>
> 
> Yeah I am also not quite sure where the MSMC clk description should go: in a new 
> separate node, or in a72_0.
> 
> However, MSMC clk id is described under A72SS0_CORE0 Device in TISCI 
> documentation [0]. Which makes a case for putting this under a72_0 node itself 
> (along with the fact that there isn't a separate msmc node for j721e/j7200 which 
> have a single DDR controller). So I guess I vote for keeping it as is, under 
> a72_0 node.


I am content with this argument, especially since the intention of the MSMC 
driver in U-Boot is exclusively for setting up the interleaver. (I should 
probably target cleaning that up and making it a generic driver as a future action!)

> 
> [0]: 
> https://software-dl.ti.com/tisci/esd/latest/5_soc_doc/j7200/clocks.html#clocks-for-a72ss0-core0-device
> 
> Thanks,
> Aniket
> 
>>>
>>> Regards,
>>> Aniket
>>>
>>>>>
>>>>> Signed-off-by: Reid Tonking <reidt@ti.com>
>>>>> Signed-off-by: Aniket Limaye <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>;
>>>>
>>

-- 
Thanking You
Neha Malcom Francis