Re: [PATCH v2 2/2] remoteproc: k3-r5: Use separate compatible string for TI AM62 SoC family
From: Tanmay Shah
Date: Wed Nov 30 2022 - 12:58:01 EST
Hi Devarsh,
Please find my comments below.
On 11/30/22 6:40 PM, Devarsh Thakkar wrote:
CAUTION: This message has originated from an External Source. Please use proper judgment and caution when opening attachments, clicking links, or responding to this email.
AM62 and AM62A SoCs use single core R5F which is a new scenario
different than the one being used with CLUSTER_MODE_SINGLECPU
which is for utilizing a single core from a set of cores available
in R5F cluster present in the SoC.
To support this single core scenario map it with
newly defined CLUSTER_MODE_NONE and use it when
compatible is set to ti,am62-r5fss.
Signed-off-by: Devarsh Thakkar <devarsht@xxxxxx>
---
V2: Fix indentation and ordering issues as per review comments
---
drivers/remoteproc/ti_k3_r5_remoteproc.c | 55 ++++++++++++++++++------
1 file changed, 43 insertions(+), 12 deletions(-)
diff --git a/drivers/remoteproc/ti_k3_r5_remoteproc.c b/drivers/remoteproc/ti_k3_r5_remoteproc.c
index 0481926c6975..9698b29a0b56 100644
--- a/drivers/remoteproc/ti_k3_r5_remoteproc.c
+++ b/drivers/remoteproc/ti_k3_r5_remoteproc.c
@@ -74,11 +74,13 @@ struct k3_r5_mem {
* Split mode : AM65x, J721E, J7200 and AM64x SoCs
* LockStep mode : AM65x, J721E and J7200 SoCs
* Single-CPU mode : AM64x SoCs only
+ * None : AM62x, AM62A SoCs
*/
enum cluster_mode {
CLUSTER_MODE_SPLIT = 0,
CLUSTER_MODE_LOCKSTEP,
CLUSTER_MODE_SINGLECPU,
+ CLUSTER_MODE_NONE,
};
/**
@@ -86,11 +88,13 @@ enum cluster_mode {
* @tcm_is_double: flag to denote the larger unified TCMs in certain modes
* @tcm_ecc_autoinit: flag to denote the auto-initialization of TCMs for ECC
* @single_cpu_mode: flag to denote if SoC/IP supports Single-CPU mode
+ * @is_single_core: flag to denote if SoC/IP has only single core R5
*/
struct k3_r5_soc_data {
bool tcm_is_double;
bool tcm_ecc_autoinit;
bool single_cpu_mode;
+ bool is_single_core;
If you are providing this data, then ignore parsing cluster-mode
property. This will make change very simple.
I believe this would save you any modification in bindings as well as
cluster-mode property is optional anyway.
Also, "enum cluster_mode" reflects cluster-mode values from bindings
document except proper soc compatible. I don't see new value added in
bindings document i.e. only
[0 -> split, 1 -> lockstep, 2 -> single cpu] are defined. If new enum is
introduced in driver, it is expected to reflect in bindings i.e. [3 ->
cluster-mode none] to avoid any confusion.
I believe it is duplicate logic if you are providing "is_single_core"
information here and introduce CLUSTER_MODE_NONE as well.
May be I am missing something, but I don't see any use of providing
extra value CLUSTER_MODE_NONE if "is_single_core" is set in the driver.
So, simple solutions is just to avoid parsing cluster-mode property if
is_single_core is set in the driver. Hope this helps.
Thanks,
Tanmay
};
/**
@@ -838,7 +842,8 @@ static int k3_r5_rproc_configure(struct k3_r5_rproc *kproc)
core0 = list_first_entry(&cluster->cores, struct k3_r5_core, elem);
if (cluster->mode == CLUSTER_MODE_LOCKSTEP ||
- cluster->mode == CLUSTER_MODE_SINGLECPU) {
+ cluster->mode == CLUSTER_MODE_SINGLECPU ||
+ cluster->mode == CLUSTER_MODE_NONE) {
core = core0;
} else {
core = kproc->core;
@@ -853,7 +858,7 @@ static int k3_r5_rproc_configure(struct k3_r5_rproc *kproc)
boot_vec, cfg, ctrl, stat);
/* check if only Single-CPU mode is supported on applicable SoCs */
- if (cluster->soc_data->single_cpu_mode) {
+ if (cluster->soc_data->single_cpu_mode || cluster->soc_data->is_single_core) {
single_cpu =
!!(stat & PROC_BOOT_STATUS_FLAG_R5_SINGLECORE_ONLY);
if (single_cpu && cluster->mode == CLUSTER_MODE_SPLIT) {
@@ -1074,6 +1079,7 @@ static void k3_r5_adjust_tcm_sizes(struct k3_r5_rproc *kproc)
if (cluster->mode == CLUSTER_MODE_LOCKSTEP ||
cluster->mode == CLUSTER_MODE_SINGLECPU ||
+ cluster->mode == CLUSTER_MODE_NONE ||
!cluster->soc_data->tcm_is_double)
return;
@@ -1147,7 +1153,9 @@ static int k3_r5_rproc_configure_mode(struct k3_r5_rproc *kproc)
atcm_enable = cfg & PROC_BOOT_CFG_FLAG_R5_ATCM_EN ? 1 : 0;
btcm_enable = cfg & PROC_BOOT_CFG_FLAG_R5_BTCM_EN ? 1 : 0;
loczrama = cfg & PROC_BOOT_CFG_FLAG_R5_TCM_RSTBASE ? 1 : 0;
- if (cluster->soc_data->single_cpu_mode) {
+ if (cluster->soc_data->is_single_core) {
+ mode = CLUSTER_MODE_NONE;
+ } else if (cluster->soc_data->single_cpu_mode) {
mode = cfg & PROC_BOOT_CFG_FLAG_R5_SINGLE_CORE ?
CLUSTER_MODE_SINGLECPU : CLUSTER_MODE_SPLIT;
} else {
@@ -1271,7 +1279,8 @@ static int k3_r5_cluster_rproc_init(struct platform_device *pdev)
/* create only one rproc in lockstep mode or single-cpu mode */
if (cluster->mode == CLUSTER_MODE_LOCKSTEP ||
- cluster->mode == CLUSTER_MODE_SINGLECPU)
+ cluster->mode == CLUSTER_MODE_SINGLECPU ||
+ cluster->mode == CLUSTER_MODE_NONE)
break;
}
@@ -1704,21 +1713,32 @@ static int k3_r5_probe(struct platform_device *pdev)
* default to most common efuse configurations - Split-mode on AM64x
* and LockStep-mode on all others
*/
- cluster->mode = data->single_cpu_mode ?
+ if (!data->is_single_core)
+ cluster->mode = data->single_cpu_mode ?
CLUSTER_MODE_SPLIT : CLUSTER_MODE_LOCKSTEP;
+ else
+ cluster->mode = CLUSTER_MODE_NONE;
+
cluster->soc_data = data;
INIT_LIST_HEAD(&cluster->cores);
- ret = of_property_read_u32(np, "ti,cluster-mode", &cluster->mode);
- if (ret < 0 && ret != -EINVAL) {
- dev_err(dev, "invalid format for ti,cluster-mode, ret = %d\n",
- ret);
- return ret;
+ if (!data->is_single_core) {
+ ret = of_property_read_u32(np, "ti,cluster-mode", &cluster->mode);
+ if (ret < 0 && ret != -EINVAL) {
+ dev_err(dev, "invalid format for ti,cluster-mode, ret = %d\n", ret);
+ return ret;
+ }
}
num_cores = of_get_available_child_count(np);
- if (num_cores != 2) {
- dev_err(dev, "MCU cluster requires both R5F cores to be enabled, num_cores = %d\n",
+ if (num_cores != 2 && !data->is_single_core) {
+ dev_err(dev, "MCU cluster requires both R5F cores to be enabled but num_cores is set to = %d\n",
+ num_cores);
+ return -ENODEV;
+ }
+
+ if (num_cores != 1 && data->is_single_core) {
+ dev_err(dev, "SoC supports only single core R5 but num_cores is set to %d\n",
num_cores);
return -ENODEV;
}
@@ -1760,18 +1780,28 @@ static const struct k3_r5_soc_data am65_j721e_soc_data = {
.tcm_is_double = false,
.tcm_ecc_autoinit = false,
.single_cpu_mode = false,
+ .is_single_core = false,
};
static const struct k3_r5_soc_data j7200_j721s2_soc_data = {
.tcm_is_double = true,
.tcm_ecc_autoinit = true,
.single_cpu_mode = false,
+ .is_single_core = false,
};
static const struct k3_r5_soc_data am64_soc_data = {
.tcm_is_double = true,
.tcm_ecc_autoinit = true,
.single_cpu_mode = true,
+ .is_single_core = false,
+};
+
+static const struct k3_r5_soc_data am62_soc_data = {
+ .tcm_is_double = false,
+ .tcm_ecc_autoinit = true,
+ .single_cpu_mode = false,
+ .is_single_core = true,
};
static const struct of_device_id k3_r5_of_match[] = {
@@ -1779,6 +1809,7 @@ static const struct of_device_id k3_r5_of_match[] = {
{ .compatible = "ti,j721e-r5fss", .data = &am65_j721e_soc_data, },
{ .compatible = "ti,j7200-r5fss", .data = &j7200_j721s2_soc_data, },
{ .compatible = "ti,am64-r5fss", .data = &am64_soc_data, },
+ { .compatible = "ti,am62-r5fss", .data = &am62_soc_data, },
{ .compatible = "ti,j721s2-r5fss", .data = &j7200_j721s2_soc_data, },
{ /* sentinel */ },
};
--
2.17.1