AMD-Xilinx Versal platform is successor of ZynqMP platform.
Real-time Processing Unit R5 cluster IP on Versal is same as
of ZynqMP Platform. Power-domains ids for Versal platform is
different than ZynqMP.
AMD-Xilinx Versal-NET platform is successor of Versal platform.
Versal-NET Real-Time Processing Unit has two clusters and each
cluster contains dual core ARM Cortex-R52 processors. Each R52
core is assigned 128KB of TCM memory.
Signed-off-by: Tanmay Shah <[email protected]>
---
drivers/remoteproc/xlnx_r5_remoteproc.c | 53 ++++++++-----------------
1 file changed, 17 insertions(+), 36 deletions(-)
diff --git a/drivers/remoteproc/xlnx_r5_remoteproc.c b/drivers/remoteproc/xlnx_r5_remoteproc.c
index 7b1c12108bff..a6d8ac7394e7 100644
--- a/drivers/remoteproc/xlnx_r5_remoteproc.c
+++ b/drivers/remoteproc/xlnx_r5_remoteproc.c
@@ -300,36 +300,6 @@ static void zynqmp_r5_rproc_kick(struct rproc *rproc, int vqid)
dev_warn(dev, "failed to send message\n");
}
-/*
- * zynqmp_r5_set_mode()
- *
- * set RPU cluster and TCM operation mode
- *
- * @r5_core: pointer to zynqmp_r5_core type object
- * @fw_reg_val: value expected by firmware to configure RPU cluster mode
- * @tcm_mode: value expected by fw to configure TCM mode (lockstep or split)
- *
- * Return: 0 for success and < 0 for failure
- */
-static int zynqmp_r5_set_mode(struct zynqmp_r5_core *r5_core,
- enum rpu_oper_mode fw_reg_val,
- enum rpu_tcm_comb tcm_mode)
-{
- int ret;
-
- ret = zynqmp_pm_set_rpu_mode(r5_core->pm_domain_id, fw_reg_val);
- if (ret < 0) {
- dev_err(r5_core->dev, "failed to set RPU mode\n");
- return ret;
- }
-
- ret = zynqmp_pm_set_tcm_config(r5_core->pm_domain_id, tcm_mode);
- if (ret < 0)
- dev_err(r5_core->dev, "failed to configure TCM\n");
-
- return ret;
-}
-
/*
* zynqmp_r5_rproc_start()
* @rproc: single R5 core's corresponding rproc instance
@@ -941,7 +911,7 @@ static int zynqmp_r5_core_init(struct zynqmp_r5_cluster *cluster,
/* Maintain backward compatibility for zynqmp by using hardcode TCM address. */
if (of_find_property(r5_core->np, "reg", NULL))
ret = zynqmp_r5_get_tcm_node_from_dt(cluster);
- else
+ else if (device_is_compatible(dev, "xlnx,zynqmp-r5fss"))
ret = zynqmp_r5_get_tcm_node(cluster);
if (ret) {
@@ -960,12 +930,21 @@ static int zynqmp_r5_core_init(struct zynqmp_r5_cluster *cluster,
return ret;
}
- ret = zynqmp_r5_set_mode(r5_core, fw_reg_val, tcm_mode);
- if (ret) {
- dev_err(dev, "failed to set r5 cluster mode %d, err %d\n",
- cluster->mode, ret);
+ ret = zynqmp_pm_set_rpu_mode(r5_core->pm_domain_id, fw_reg_val);
+ if (ret < 0) {
+ dev_err(r5_core->dev, "failed to set RPU mode\n");
return ret;
}
+
+ if (of_find_property(dev_of_node(dev), "xlnx,tcm-mode", NULL) ||
+ device_is_compatible(dev, "xlnx,zynqmp-r5fss")) {
+ ret = zynqmp_pm_set_tcm_config(r5_core->pm_domain_id,
+ tcm_mode);
+ if (ret < 0) {
+ dev_err(r5_core->dev, "failed to configure TCM\n");
+ return ret;
+ }
+ }
}
return 0;
@@ -1022,7 +1001,7 @@ static int zynqmp_r5_cluster_init(struct zynqmp_r5_cluster *cluster)
ret = of_property_read_u32(dev_node, "xlnx,tcm-mode", (u32 *)&tcm_mode);
if (ret)
return ret;
- } else {
+ } else if (device_is_compatible(dev, "xlnx,zynqmp-r5fss")) {
if (cluster_mode == LOCKSTEP_MODE)
tcm_mode = PM_RPU_TCM_COMB;
else
@@ -1212,6 +1191,8 @@ static int zynqmp_r5_remoteproc_probe(struct platform_device *pdev)
/* Match table for OF platform binding */
static const struct of_device_id zynqmp_r5_remoteproc_match[] = {
+ { .compatible = "xlnx,versal-net-r52fss", },
+ { .compatible = "xlnx,versal-r5fss", },
{ .compatible = "xlnx,zynqmp-r5fss", },
{ /* end of list */ },
};
base-commit: 912ebe48bec5927e2049e91b0e8a9cc682a709d2
--
2.25.1
On Thu, Apr 18, 2024 at 03:01:25PM -0700, Tanmay Shah wrote:
> AMD-Xilinx Versal platform is successor of ZynqMP platform.
> Real-time Processing Unit R5 cluster IP on Versal is same as
> of ZynqMP Platform. Power-domains ids for Versal platform is
> different than ZynqMP.
>
> AMD-Xilinx Versal-NET platform is successor of Versal platform.
> Versal-NET Real-Time Processing Unit has two clusters and each
> cluster contains dual core ARM Cortex-R52 processors. Each R52
> core is assigned 128KB of TCM memory.
>
> Signed-off-by: Tanmay Shah <[email protected]>
> ---
> drivers/remoteproc/xlnx_r5_remoteproc.c | 53 ++++++++-----------------
> 1 file changed, 17 insertions(+), 36 deletions(-)
>
Applied.
Thanks,
Mathieu
> diff --git a/drivers/remoteproc/xlnx_r5_remoteproc.c b/drivers/remoteproc/xlnx_r5_remoteproc.c
> index 7b1c12108bff..a6d8ac7394e7 100644
> --- a/drivers/remoteproc/xlnx_r5_remoteproc.c
> +++ b/drivers/remoteproc/xlnx_r5_remoteproc.c
> @@ -300,36 +300,6 @@ static void zynqmp_r5_rproc_kick(struct rproc *rproc, int vqid)
> dev_warn(dev, "failed to send message\n");
> }
>
> -/*
> - * zynqmp_r5_set_mode()
> - *
> - * set RPU cluster and TCM operation mode
> - *
> - * @r5_core: pointer to zynqmp_r5_core type object
> - * @fw_reg_val: value expected by firmware to configure RPU cluster mode
> - * @tcm_mode: value expected by fw to configure TCM mode (lockstep or split)
> - *
> - * Return: 0 for success and < 0 for failure
> - */
> -static int zynqmp_r5_set_mode(struct zynqmp_r5_core *r5_core,
> - enum rpu_oper_mode fw_reg_val,
> - enum rpu_tcm_comb tcm_mode)
> -{
> - int ret;
> -
> - ret = zynqmp_pm_set_rpu_mode(r5_core->pm_domain_id, fw_reg_val);
> - if (ret < 0) {
> - dev_err(r5_core->dev, "failed to set RPU mode\n");
> - return ret;
> - }
> -
> - ret = zynqmp_pm_set_tcm_config(r5_core->pm_domain_id, tcm_mode);
> - if (ret < 0)
> - dev_err(r5_core->dev, "failed to configure TCM\n");
> -
> - return ret;
> -}
> -
> /*
> * zynqmp_r5_rproc_start()
> * @rproc: single R5 core's corresponding rproc instance
> @@ -941,7 +911,7 @@ static int zynqmp_r5_core_init(struct zynqmp_r5_cluster *cluster,
> /* Maintain backward compatibility for zynqmp by using hardcode TCM address. */
> if (of_find_property(r5_core->np, "reg", NULL))
> ret = zynqmp_r5_get_tcm_node_from_dt(cluster);
> - else
> + else if (device_is_compatible(dev, "xlnx,zynqmp-r5fss"))
> ret = zynqmp_r5_get_tcm_node(cluster);
>
> if (ret) {
> @@ -960,12 +930,21 @@ static int zynqmp_r5_core_init(struct zynqmp_r5_cluster *cluster,
> return ret;
> }
>
> - ret = zynqmp_r5_set_mode(r5_core, fw_reg_val, tcm_mode);
> - if (ret) {
> - dev_err(dev, "failed to set r5 cluster mode %d, err %d\n",
> - cluster->mode, ret);
> + ret = zynqmp_pm_set_rpu_mode(r5_core->pm_domain_id, fw_reg_val);
> + if (ret < 0) {
> + dev_err(r5_core->dev, "failed to set RPU mode\n");
> return ret;
> }
> +
> + if (of_find_property(dev_of_node(dev), "xlnx,tcm-mode", NULL) ||
> + device_is_compatible(dev, "xlnx,zynqmp-r5fss")) {
> + ret = zynqmp_pm_set_tcm_config(r5_core->pm_domain_id,
> + tcm_mode);
> + if (ret < 0) {
> + dev_err(r5_core->dev, "failed to configure TCM\n");
> + return ret;
> + }
> + }
> }
>
> return 0;
> @@ -1022,7 +1001,7 @@ static int zynqmp_r5_cluster_init(struct zynqmp_r5_cluster *cluster)
> ret = of_property_read_u32(dev_node, "xlnx,tcm-mode", (u32 *)&tcm_mode);
> if (ret)
> return ret;
> - } else {
> + } else if (device_is_compatible(dev, "xlnx,zynqmp-r5fss")) {
> if (cluster_mode == LOCKSTEP_MODE)
> tcm_mode = PM_RPU_TCM_COMB;
> else
> @@ -1212,6 +1191,8 @@ static int zynqmp_r5_remoteproc_probe(struct platform_device *pdev)
>
> /* Match table for OF platform binding */
> static const struct of_device_id zynqmp_r5_remoteproc_match[] = {
> + { .compatible = "xlnx,versal-net-r52fss", },
> + { .compatible = "xlnx,versal-r5fss", },
> { .compatible = "xlnx,zynqmp-r5fss", },
> { /* end of list */ },
> };
>
> base-commit: 912ebe48bec5927e2049e91b0e8a9cc682a709d2
> --
> 2.25.1
>
Hi Tanmay,
On Thu, Apr 18, 2024 at 03:01:25PM -0700, Tanmay Shah wrote:
> AMD-Xilinx Versal platform is successor of ZynqMP platform.
> Real-time Processing Unit R5 cluster IP on Versal is same as
> of ZynqMP Platform. Power-domains ids for Versal platform is
> different than ZynqMP.
>
> AMD-Xilinx Versal-NET platform is successor of Versal platform.
> Versal-NET Real-Time Processing Unit has two clusters and each
> cluster contains dual core ARM Cortex-R52 processors. Each R52
> core is assigned 128KB of TCM memory.
>
> Signed-off-by: Tanmay Shah <[email protected]>
> ---
> drivers/remoteproc/xlnx_r5_remoteproc.c | 53 ++++++++-----------------
> 1 file changed, 17 insertions(+), 36 deletions(-)
>
> diff --git a/drivers/remoteproc/xlnx_r5_remoteproc.c b/drivers/remoteproc/xlnx_r5_remoteproc.c
> index 7b1c12108bff..a6d8ac7394e7 100644
> --- a/drivers/remoteproc/xlnx_r5_remoteproc.c
> +++ b/drivers/remoteproc/xlnx_r5_remoteproc.c
> @@ -300,36 +300,6 @@ static void zynqmp_r5_rproc_kick(struct rproc *rproc, int vqid)
> dev_warn(dev, "failed to send message\n");
> }
>
> -/*
> - * zynqmp_r5_set_mode()
> - *
> - * set RPU cluster and TCM operation mode
> - *
> - * @r5_core: pointer to zynqmp_r5_core type object
> - * @fw_reg_val: value expected by firmware to configure RPU cluster mode
> - * @tcm_mode: value expected by fw to configure TCM mode (lockstep or split)
> - *
> - * Return: 0 for success and < 0 for failure
> - */
> -static int zynqmp_r5_set_mode(struct zynqmp_r5_core *r5_core,
> - enum rpu_oper_mode fw_reg_val,
> - enum rpu_tcm_comb tcm_mode)
> -{
> - int ret;
> -
> - ret = zynqmp_pm_set_rpu_mode(r5_core->pm_domain_id, fw_reg_val);
> - if (ret < 0) {
> - dev_err(r5_core->dev, "failed to set RPU mode\n");
> - return ret;
> - }
> -
> - ret = zynqmp_pm_set_tcm_config(r5_core->pm_domain_id, tcm_mode);
> - if (ret < 0)
> - dev_err(r5_core->dev, "failed to configure TCM\n");
> -
> - return ret;
> -}
> -
> /*
> * zynqmp_r5_rproc_start()
> * @rproc: single R5 core's corresponding rproc instance
> @@ -941,7 +911,7 @@ static int zynqmp_r5_core_init(struct zynqmp_r5_cluster *cluster,
> /* Maintain backward compatibility for zynqmp by using hardcode TCM address. */
> if (of_find_property(r5_core->np, "reg", NULL))
> ret = zynqmp_r5_get_tcm_node_from_dt(cluster);
> - else
> + else if (device_is_compatible(dev, "xlnx,zynqmp-r5fss"))
> ret = zynqmp_r5_get_tcm_node(cluster);
This change breaks the build with clang:
drivers/remoteproc/xlnx_r5_remoteproc.c:914:11: error: variable 'ret' is used uninitialized whenever 'if' condition is false [-Werror,-Wsometimes-uninitialized]
914 | else if (device_is_compatible(dev, "xlnx,zynqmp-r5fss"))
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
drivers/remoteproc/xlnx_r5_remoteproc.c:917:6: note: uninitialized use occurs here
917 | if (ret) {
| ^~~
drivers/remoteproc/xlnx_r5_remoteproc.c:914:7: note: remove the 'if' if its condition is always true
914 | else if (device_is_compatible(dev, "xlnx,zynqmp-r5fss"))
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
915 | ret = zynqmp_r5_get_tcm_node(cluster);
drivers/remoteproc/xlnx_r5_remoteproc.c:907:9: note: initialize the variable 'ret' to silence this warning
907 | int ret, i;
| ^
| = 0
1 error generated.
Should ret be initialized to zero or should there be an else statement?
Cheers,
Nathan
>
> if (ret) {
> @@ -960,12 +930,21 @@ static int zynqmp_r5_core_init(struct zynqmp_r5_cluster *cluster,
> return ret;
> }
>
> - ret = zynqmp_r5_set_mode(r5_core, fw_reg_val, tcm_mode);
> - if (ret) {
> - dev_err(dev, "failed to set r5 cluster mode %d, err %d\n",
> - cluster->mode, ret);
> + ret = zynqmp_pm_set_rpu_mode(r5_core->pm_domain_id, fw_reg_val);
> + if (ret < 0) {
> + dev_err(r5_core->dev, "failed to set RPU mode\n");
> return ret;
> }
> +
> + if (of_find_property(dev_of_node(dev), "xlnx,tcm-mode", NULL) ||
> + device_is_compatible(dev, "xlnx,zynqmp-r5fss")) {
> + ret = zynqmp_pm_set_tcm_config(r5_core->pm_domain_id,
> + tcm_mode);
> + if (ret < 0) {
> + dev_err(r5_core->dev, "failed to configure TCM\n");
> + return ret;
> + }
> + }
> }
>
> return 0;
> @@ -1022,7 +1001,7 @@ static int zynqmp_r5_cluster_init(struct zynqmp_r5_cluster *cluster)
> ret = of_property_read_u32(dev_node, "xlnx,tcm-mode", (u32 *)&tcm_mode);
> if (ret)
> return ret;
> - } else {
> + } else if (device_is_compatible(dev, "xlnx,zynqmp-r5fss")) {
> if (cluster_mode == LOCKSTEP_MODE)
> tcm_mode = PM_RPU_TCM_COMB;
> else
> @@ -1212,6 +1191,8 @@ static int zynqmp_r5_remoteproc_probe(struct platform_device *pdev)
>
> /* Match table for OF platform binding */
> static const struct of_device_id zynqmp_r5_remoteproc_match[] = {
> + { .compatible = "xlnx,versal-net-r52fss", },
> + { .compatible = "xlnx,versal-r5fss", },
> { .compatible = "xlnx,zynqmp-r5fss", },
> { /* end of list */ },
> };
>
> base-commit: 912ebe48bec5927e2049e91b0e8a9cc682a709d2
> --
> 2.25.1
>
On 4/23/24 11:06 AM, Nathan Chancellor wrote:
> Hi Tanmay,
>
> On Thu, Apr 18, 2024 at 03:01:25PM -0700, Tanmay Shah wrote:
>> AMD-Xilinx Versal platform is successor of ZynqMP platform.
>> Real-time Processing Unit R5 cluster IP on Versal is same as
>> of ZynqMP Platform. Power-domains ids for Versal platform is
>> different than ZynqMP.
>>
>> AMD-Xilinx Versal-NET platform is successor of Versal platform.
>> Versal-NET Real-Time Processing Unit has two clusters and each
>> cluster contains dual core ARM Cortex-R52 processors. Each R52
>> core is assigned 128KB of TCM memory.
>>
>> Signed-off-by: Tanmay Shah <[email protected]>
>> ---
>> drivers/remoteproc/xlnx_r5_remoteproc.c | 53 ++++++++-----------------
>> 1 file changed, 17 insertions(+), 36 deletions(-)
>>
>> diff --git a/drivers/remoteproc/xlnx_r5_remoteproc.c b/drivers/remoteproc/xlnx_r5_remoteproc.c
>> index 7b1c12108bff..a6d8ac7394e7 100644
>> --- a/drivers/remoteproc/xlnx_r5_remoteproc.c
>> +++ b/drivers/remoteproc/xlnx_r5_remoteproc.c
>> @@ -300,36 +300,6 @@ static void zynqmp_r5_rproc_kick(struct rproc *rproc, int vqid)
>> dev_warn(dev, "failed to send message\n");
>> }
>>
>> -/*
>> - * zynqmp_r5_set_mode()
>> - *
>> - * set RPU cluster and TCM operation mode
>> - *
>> - * @r5_core: pointer to zynqmp_r5_core type object
>> - * @fw_reg_val: value expected by firmware to configure RPU cluster mode
>> - * @tcm_mode: value expected by fw to configure TCM mode (lockstep or split)
>> - *
>> - * Return: 0 for success and < 0 for failure
>> - */
>> -static int zynqmp_r5_set_mode(struct zynqmp_r5_core *r5_core,
>> - enum rpu_oper_mode fw_reg_val,
>> - enum rpu_tcm_comb tcm_mode)
>> -{
>> - int ret;
>> -
>> - ret = zynqmp_pm_set_rpu_mode(r5_core->pm_domain_id, fw_reg_val);
>> - if (ret < 0) {
>> - dev_err(r5_core->dev, "failed to set RPU mode\n");
>> - return ret;
>> - }
>> -
>> - ret = zynqmp_pm_set_tcm_config(r5_core->pm_domain_id, tcm_mode);
>> - if (ret < 0)
>> - dev_err(r5_core->dev, "failed to configure TCM\n");
>> -
>> - return ret;
>> -}
>> -
>> /*
>> * zynqmp_r5_rproc_start()
>> * @rproc: single R5 core's corresponding rproc instance
>> @@ -941,7 +911,7 @@ static int zynqmp_r5_core_init(struct zynqmp_r5_cluster *cluster,
>> /* Maintain backward compatibility for zynqmp by using hardcode TCM address. */
>> if (of_find_property(r5_core->np, "reg", NULL))
>> ret = zynqmp_r5_get_tcm_node_from_dt(cluster);
>> - else
>> + else if (device_is_compatible(dev, "xlnx,zynqmp-r5fss"))
>> ret = zynqmp_r5_get_tcm_node(cluster);
>
> This change breaks the build with clang:
>
> drivers/remoteproc/xlnx_r5_remoteproc.c:914:11: error: variable 'ret' is used uninitialized whenever 'if' condition is false [-Werror,-Wsometimes-uninitialized]
> 914 | else if (device_is_compatible(dev, "xlnx,zynqmp-r5fss"))
> | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> drivers/remoteproc/xlnx_r5_remoteproc.c:917:6: note: uninitialized use occurs here
> 917 | if (ret) {
> | ^~~
> drivers/remoteproc/xlnx_r5_remoteproc.c:914:7: note: remove the 'if' if its condition is always true
> 914 | else if (device_is_compatible(dev, "xlnx,zynqmp-r5fss"))
> | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> 915 | ret = zynqmp_r5_get_tcm_node(cluster);
> drivers/remoteproc/xlnx_r5_remoteproc.c:907:9: note: initialize the variable 'ret' to silence this warning
> 907 | int ret, i;
> | ^
> | = 0
> 1 error generated.
>
> Should ret be initialized to zero or should there be an else statement?
Hello,
Thanks for analysis. ret should be initialized with -EINVAL, so else can be avoided.
I will send patch by EOD.
Thanks,
Tanmay
>
> Cheers,
> Nathan
>
>>
>> if (ret) {
>> @@ -960,12 +930,21 @@ static int zynqmp_r5_core_init(struct zynqmp_r5_cluster *cluster,
>> return ret;
>> }
>>
>> - ret = zynqmp_r5_set_mode(r5_core, fw_reg_val, tcm_mode);
>> - if (ret) {
>> - dev_err(dev, "failed to set r5 cluster mode %d, err %d\n",
>> - cluster->mode, ret);
>> + ret = zynqmp_pm_set_rpu_mode(r5_core->pm_domain_id, fw_reg_val);
>> + if (ret < 0) {
>> + dev_err(r5_core->dev, "failed to set RPU mode\n");
>> return ret;
>> }
>> +
>> + if (of_find_property(dev_of_node(dev), "xlnx,tcm-mode", NULL) ||
>> + device_is_compatible(dev, "xlnx,zynqmp-r5fss")) {
>> + ret = zynqmp_pm_set_tcm_config(r5_core->pm_domain_id,
>> + tcm_mode);
>> + if (ret < 0) {
>> + dev_err(r5_core->dev, "failed to configure TCM\n");
>> + return ret;
>> + }
>> + }
>> }
>>
>> return 0;
>> @@ -1022,7 +1001,7 @@ static int zynqmp_r5_cluster_init(struct zynqmp_r5_cluster *cluster)
>> ret = of_property_read_u32(dev_node, "xlnx,tcm-mode", (u32 *)&tcm_mode);
>> if (ret)
>> return ret;
>> - } else {
>> + } else if (device_is_compatible(dev, "xlnx,zynqmp-r5fss")) {
>> if (cluster_mode == LOCKSTEP_MODE)
>> tcm_mode = PM_RPU_TCM_COMB;
>> else
>> @@ -1212,6 +1191,8 @@ static int zynqmp_r5_remoteproc_probe(struct platform_device *pdev)
>>
>> /* Match table for OF platform binding */
>> static const struct of_device_id zynqmp_r5_remoteproc_match[] = {
>> + { .compatible = "xlnx,versal-net-r52fss", },
>> + { .compatible = "xlnx,versal-r5fss", },
>> { .compatible = "xlnx,zynqmp-r5fss", },
>> { /* end of list */ },
>> };
>>
>> base-commit: 912ebe48bec5927e2049e91b0e8a9cc682a709d2
>> --
>> 2.25.1
>>