On 28/03/2023 19:08, Devarsh Thakkar wrote:
> Hi Roger,
>
> On 28/03/23 13:22, Roger Quadros wrote:
>> Hi Devarsh,
>>
>> On 17/03/2023 18:17, Mathieu Poirier wrote:
>>> On Fri, Mar 10, 2023 at 09:55:44PM +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_SINGLECORE and use it when compatible is set to
>>>> ti,am62-r5fss.
>>>>
>>>> Also set PROC_BOOT_CFG_FLAG_R5_SINGLE_CORE config for
>>>> CLUSTER_MODE_SINGLECORE too as it is required by R5 core when it is
>>>> being as general purpose core instead of device manager.
>>>>
>>>> Signed-off-by: Devarsh Thakkar <devarsht@xxxxxx>
>>>> ---
>>>> 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)
>>>> V6:
>>>> - Use CLUSTER_MODE_SINGLECORE for AM62x
>>>> - Set PROC_BOOT_CFG_FLAG_R5_SINGLE_CORE for single core.
>>>> V7:
>>>> - Simplify and rebase on top of base commit "[PATCH v7] remoteproc: k3-r5: Simplify cluster
>>>> mode setting"
>>>> ---
>>>> drivers/remoteproc/ti_k3_r5_remoteproc.c | 59 +++++++++++++++++++-----
>>>> 1 file changed, 48 insertions(+), 11 deletions(-)
>>>>
>>>> diff --git a/drivers/remoteproc/ti_k3_r5_remoteproc.c b/drivers/remoteproc/ti_k3_r5_remoteproc.c
>>>> index c2ec0f432921..df32f6bc4325 100644
>>>> --- a/drivers/remoteproc/ti_k3_r5_remoteproc.c
>>>> +++ b/drivers/remoteproc/ti_k3_r5_remoteproc.c
>>>> @@ -71,14 +71,16 @@ struct k3_r5_mem {
>>>> /*
>>>> * All cluster mode values are not applicable on all SoCs. The following
>>>> * are the modes supported on various SoCs:
>>>> - * Split mode : AM65x, J721E, J7200 and AM64x SoCs
>>>> - * LockStep mode : AM65x, J721E and J7200 SoCs
>>>> - * Single-CPU mode : AM64x SoCs only
>>>> + * Split mode : AM65x, J721E, J7200 and AM64x SoCs
>>>> + * LockStep mode : AM65x, J721E and J7200 SoCs
>>>> + * Single-CPU mode : AM64x SoCs only
>>>> + * Single-Core mode : AM62x, AM62A SoCs
>>>> */
>>>> enum cluster_mode {
>>>> CLUSTER_MODE_SPLIT = 0,
>>>> CLUSTER_MODE_LOCKSTEP,
>>>> CLUSTER_MODE_SINGLECPU,
>>>> + CLUSTER_MODE_SINGLECORE
>>
>> What is the difference in device driver behaviour between
>> SINGLECPU and SINGLECORE?
>>
> Yeah there is quite a bit of common code flow between the two but the fundamental difference is that you use CLUSTER_MODE_SINGLECPU when
I still didn't get what is the difference between the two from SW point of view.
What happens if you just use CLUSTER_MODE_SINGLECPU for AM62 SoC?
> you have two R5F cores but you want to use only single R5F core albeit
> with using TCM of both the cores whereas CLUSTER_MODE_SINGLECORE is
> for the scenario where you have single core R5F's only.
>
> Also the bindings for CLUSTER_MODE_SINGLECPU are already upstream so did
> not want to break them either : https://gitlab.com/linux-kernel/linux-next/-/blob/next-20230328/Documentation/devicetree/bindings/remoteproc/ti%2Ck3-r5f-rproc.yaml#L20.
>
> Regards
> Devarsh
>
>> If there is no difference then you should not introduce
>> a new enum. >
>>>> };
>>>> /**
>>>> @@ -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_SINGLECORE) {
>>>> core = core0;
>>>> } else {
>>>> core = kproc->core;
>>>> @@ -877,7 +882,8 @@ static int k3_r5_rproc_configure(struct k3_r5_rproc *kproc)
>>>> * with the bit configured, so program it only on
>>>> * permitted cores
>>>> */
>>>> - if (cluster->mode == CLUSTER_MODE_SINGLECPU) {
>>>> + if (cluster->mode == CLUSTER_MODE_SINGLECPU ||
>>>> + cluster->mode == CLUSTER_MODE_SINGLECORE) {
>>>> set_cfg = PROC_BOOT_CFG_FLAG_R5_SINGLE_CORE;
>>>> } else {
>>>> /*
>>>> @@ -1069,6 +1075,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_SINGLECORE ||
>>>> !cluster->soc_data->tcm_is_double)
>>>> return;
>>>> @@ -1145,6 +1152,8 @@ static int k3_r5_rproc_configure_mode(struct k3_r5_rproc *kproc)
>>>> if (cluster->soc_data->single_cpu_mode) {
>>>> mode = cfg & PROC_BOOT_CFG_FLAG_R5_SINGLE_CORE ?
>>>> CLUSTER_MODE_SINGLECPU : CLUSTER_MODE_SPLIT;
>>>> + } else if (cluster->soc_data->is_single_core) {
>>>> + mode = CLUSTER_MODE_SINGLECORE;
>>>
>>> I have commented twice on this before - whether it is soc_data->single_cpu_mode or
>>> soc_data->is_single_core, I don't want to see them used elsewhere than in a
>>> single function. Either in probe() or another function, use them once to set
>>> cluster->mode and never again.
>>>
>>> I will silently drop any other patchset that doesn't address this.
>>>
>>>> } else {
>>>> mode = cfg & PROC_BOOT_CFG_FLAG_R5_LOCKSTEP ?
>>>> CLUSTER_MODE_LOCKSTEP : CLUSTER_MODE_SPLIT;
>>>> @@ -1264,9 +1273,12 @@ static int k3_r5_cluster_rproc_init(struct platform_device *pdev)
>>>> goto err_add;
>>>> }
>>>> - /* create only one rproc in lockstep mode or single-cpu mode */
>>>> + /* create only one rproc in lockstep, single-cpu or
>>>> + * single core mode
>>>> + */
>>>> if (cluster->mode == CLUSTER_MODE_LOCKSTEP ||
>>>> - cluster->mode == CLUSTER_MODE_SINGLECPU)
>>>> + cluster->mode == CLUSTER_MODE_SINGLECPU ||
>>>> + cluster->mode == CLUSTER_MODE_SINGLECORE)
>>>> break;
>>>> }
>>>> @@ -1709,19 +1721,33 @@ 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
>>>> + * default to most common efuse configurations -
>>>> + * Split-mode on AM64x
>>>> + * Single core on AM62x
>>>> + * 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_SINGLECORE;
>>>> }
>>>> - if (cluster->mode == CLUSTER_MODE_SINGLECPU && !data->single_cpu_mode) {
>>>> + if ((cluster->mode == CLUSTER_MODE_SINGLECPU && !data->single_cpu_mode) ||
>>>> + (cluster->mode == CLUSTER_MODE_SINGLECORE && !data->is_single_core)) {
>>>> dev_err(dev, "Cluster mode = %d is not supported on this SoC\n", cluster->mode);
>>>> return -EINVAL;
>>>> }
>>>> 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;
>>>> }
>>>> @@ -1763,18 +1789,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[] = {
>>>> @@ -1782,6 +1818,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.34.1
>>>>
>>
--
cheers,
-roger
