From: Mathieu Poirier <mathieu.poirier@linaro.org>
To: Devarsh Thakkar <devarsht@ti.com>
Cc: andersson@kernel.org, devicetree@vger.kernel.org,
p.zabel@pengutronix.de, linux-remoteproc@vger.kernel.org,
robh+dt@kernel.org, linux-kernel@vger.kernel.org,
krzysztof.kozlowski+dt@linaro.org, s-anna@ti.com,
hnagalla@ti.com, praneeth@ti.com, nm@ti.com, vigneshr@ti.com,
a-bhatia1@ti.com, j-luthra@ti.com
Subject: Re: [PATCH v5 2/2] remoteproc: k3-r5: Use separate compatible string for TI AM62 SoC family
Date: Tue, 10 Jan 2023 11:35:05 -0700 [thread overview]
Message-ID: <20230110183505.GA2741090@p14s> (raw)
In-Reply-To: <20221227145216.1524-3-devarsht@ti.com>
On Tue, Dec 27, 2022 at 08:22:16PM +0530, Devarsh Thakkar wrote:
> 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@ti.com>
> ---
> V2: Fix indentation and ordering issues as per review comments
> V3: Change CLUSTER_MODE_NONE value to -1
> V4: No change
> V5: No change (fixing typo in email address)
> ---
> drivers/remoteproc/ti_k3_r5_remoteproc.c | 57 ++++++++++++++++++------
> 1 file changed, 44 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/remoteproc/ti_k3_r5_remoteproc.c b/drivers/remoteproc/ti_k3_r5_remoteproc.c
> index 0481926c6975..127f1f68e592 100644
> --- a/drivers/remoteproc/ti_k3_r5_remoteproc.c
> +++ b/drivers/remoteproc/ti_k3_r5_remoteproc.c
> @@ -74,9 +74,11 @@ 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_NONE = -1,
s/CLUSTER_MODE_NONE/CLUSTER_MODE_ONECORE
And add it after CLUSTER_MODE_SINGLECPU
> + CLUSTER_MODE_SPLIT,
> CLUSTER_MODE_LOCKSTEP,
> CLUSTER_MODE_SINGLECPU,
> };
> @@ -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;
> };
>
> /**
> @@ -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) {
Everywhere other than k3_r5_probe(), cluster->mode should be used. Otherwise it
is wildly confusing and error prone. Please resend this set with an extra
preamble patch that fixes this.
> 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;
Same comment as above.
> } 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
> */
The above comment needs to be adjusted.
Thanks,
Mathieu
> - 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_s32(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
>
next prev parent reply other threads:[~2023-01-10 18:42 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-12-27 14:52 [PATCH v5 0/2] Add single core R5F IPC for AM62 SoC family Devarsh Thakkar
2022-12-27 14:52 ` [PATCH v5 1/2] dt-bindings: remoteproc: ti: Add new compatible " Devarsh Thakkar
2022-12-28 15:03 ` Krzysztof Kozlowski
2022-12-27 14:52 ` [PATCH v5 2/2] remoteproc: k3-r5: Use separate compatible string for TI " Devarsh Thakkar
2023-01-10 18:35 ` Mathieu Poirier [this message]
2023-01-16 6:28 ` Devarsh Thakkar
2023-01-16 16:41 ` Mathieu Poirier
2023-01-17 14:37 ` Devarsh Thakkar
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20230110183505.GA2741090@p14s \
--to=mathieu.poirier@linaro.org \
--cc=a-bhatia1@ti.com \
--cc=andersson@kernel.org \
--cc=devarsht@ti.com \
--cc=devicetree@vger.kernel.org \
--cc=hnagalla@ti.com \
--cc=j-luthra@ti.com \
--cc=krzysztof.kozlowski+dt@linaro.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-remoteproc@vger.kernel.org \
--cc=nm@ti.com \
--cc=p.zabel@pengutronix.de \
--cc=praneeth@ti.com \
--cc=robh+dt@kernel.org \
--cc=s-anna@ti.com \
--cc=vigneshr@ti.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.