Received-SPF: pass (google.com: domain of linux-kernel+bounces-155543-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Message-ID: <06c3431f-badf-4e50-8e8f-90363c598b38@amd.com>
Date: Tue, 23 Apr 2024 11:17:33 -0500
User-Agent: Mozilla Thunderbird Beta
Subject: Re: [PATCH] drivers: remoteproc: xlnx: Add Versal and Versal-NET
 support
To: Nathan Chancellor <nathan@kernel.org>
Cc: andersson@kernel.org, mathieu.poirier@linaro.org,
 linux-remoteproc@vger.kernel.org, linux-kernel@vger.kernel.org,
 llvm@lists.linux.dev
References: <20240418220125.744322-1-tanmay.shah@amd.com>
 <20240423160644.GA554932@dev-arch.thelio-3990X>
Content-Language: en-US
From: Tanmay Shah <tanmay.shah@amd.com>
In-Reply-To: <20240423160644.GA554932@dev-arch.thelio-3990X>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0:
	=?utf-8?B?aUlKcnBuQlVyR1AwVXBxc2YwUDRYcE1Ncld2akdRZWMwTVE0dTlSamRFUDhD?=
 =?utf-8?B?dGpmRE4ycnJ4c3Mxdjdxa3pRanQ1R1YvYzk4Skd1dFlZWldzeUc3ZnBnclFZ?=
 =?utf-8?B?aWgwZmNlTjJ2cGFUMDkrQkxKZUo4aXIwYnlkK2FzQURNUFBCWFFiS0NraTZz?=
 =?utf-8?B?aWNsQVVGdldjSmtabGpzcWVScTMyOXZWK3pnVEJaTjNZclZpYjg4M09uWU1W?=
 =?utf-8?B?azRTdEdEV1pTRXJsbjhMbmNTL2lGbG5FdDRHV25MVnhZWG1ESjRBazBuUXlu?=
 =?utf-8?B?VGI3K2VQVWN4czZWRFhUdXN2MGpGdVhuYmo3TmpFelpQcFdHdktPcDFLTGQy?=
 =?utf-8?B?RktmL3ZyZzJwK0kwK0JxeDVTSFIzOE1Tdmo4aHo3VUl6REUrMk5ZRnIwSmZY?=
 =?utf-8?B?RVpPeTVtNmtYa2h5amtGZGNZT0tFZjhTSW4yUnA0Zk1PTVNoa2xLc1BJSURX?=
 =?utf-8?B?QWk2VFA3aVhkWmZLZzc3cnhMd210ck95NEtGcmhDWlF1UW4wWkpvTTFuZEwx?=
 =?utf-8?B?OXJVODNuZjJHQW8vdTVKYUs5SzdMb2dFL0Z2Y1NrTXBNTVVWY3pHdnVTSGpZ?=
 =?utf-8?B?bU44UUN4WnNnZEprYm5wUUxWWDRSazNDS0hqMTdSZjhGTWxnQWtGdlpjcktS?=
 =?utf-8?B?SVNHMVdObGFRQlVlbVBSbk1mMzRVYktnZWZROHhRRC9WeEhNY29mcjZpLzFu?=
 =?utf-8?B?RDhSc1hCZjBnSWlTb2sxQ0NGcW9xKzdYd0NXSmpLMm1nQXQvMlhVbStYck9N?=
 =?utf-8?B?dEFiWEhmUWJxMy8xcEhtSDRBTENlYno2MHZ5MmdnMUxxMnBWbFNaMGpTOGg1?=
 =?utf-8?B?VDJQMW9SMmMwVDFhdVFQdmc3SWUzRk5jVzNDQWx4bTF3eU13czRwKzZ3Yy9E?=
 =?utf-8?B?b3FGMGNzdGxHaVcwQ2h2eWprTS83czF5ZVpVME85dU5OM081RjVnYXUvdHF4?=
 =?utf-8?B?TG85TUg3SXdiN2w0Q2dBbkc4L3AzT3lpM0QwMnF3SVBFNHRRQnNJaThvK1hk?=
 =?utf-8?B?RUwwMWhYZ3Z1WW5DQlRidEplQ1RneW40T2pkTnVaeFFRTVcvemxYbkVRbUll?=
 =?utf-8?B?YlQ3Q2dyVHJ2clFiSm5nRjlFNkRkNENLcmpnSUkwelpmdXV0ZmFKWU1xU0JT?=
 =?utf-8?B?UXc4OTR2RDRNYkxFeTF1MkV4dmx1TGYzZ2NOejkvM2g4Q3dIQmYvaUsyODRR?=
 =?utf-8?B?RlVDMWdpVzdld3BMejBwbmF5R2tleHNiTWV3ejFuNGVoUG9nbW1ReHAyNGJs?=
 =?utf-8?B?Y1NQWHF3aDRDKzh4bE9KSU9PT2VIY0Iya04rQmNFeWFEWVA0V3RHdDAzWFhR?=
 =?utf-8?B?M1pxMThGNk16blNobXdJdFI0QW8vZVhxRTYxVXFzRGE5aktxdkZhYWdXT2dm?=
 =?utf-8?B?UHJwRlRCTEZOS3hEdFVyUVA2UHlVZjBRb0N0dEFYVHo0YXFLOXA1QWpuV1d6?=
 =?utf-8?B?WG8zeEI2VmEwUnVWS1NNcjVlRTlnUE9vOXhqelR2N2g2MGo5elpJazhWbUhV?=
 =?utf-8?B?YjFHUThwUnI3aDJaVWVlSXpzSnFhMXNkcXdjLzE2TnNLUjc5d285ZDAvVmlE?=
 =?utf-8?B?ZFkrbkpwZmw5N1FsLzZVYy9YTnRwalloU0ZTTi9jTWozRlhnSG1iK3ZtVDhV?=
 =?utf-8?B?RlFFMHRyTWVzNlFzZ3N2bkNyaTdqTnNDOGtRbTVTZ1VCU0ZpQVFSQUQxNUhJ?=
 =?utf-8?B?aUlDeHU3QlNIVzhlVWt4U2NWRzFDKzZhMXF1VkxZaHE4MjFMZDFxN0psbng3?=
 =?utf-8?B?dmoxQ0FMaFVZZVNFamFUeTNXNE5ZSy8zNGtDT2NCZmJFWXp0Z2Z0L2I1MEIx?=
 =?utf-8?B?YUEzV3VzQUNyUWgvNGdLOGlKRkYvZnhDQjlpd0hxSisvTGx5d2llZzI0Ykdz?=
 =?utf-8?B?dWxGZ3R0dHFycVNRYnl0UTk5NjBteEdHeFoyMUVldWNtTElEamR1eHB0emZH?=
 =?utf-8?B?aGNqYW1HVGN4VW5BZXFCV2V1V25OQm5mNGNIaWhtUWxJMWFuWWpiMEpLeWNP?=
 =?utf-8?B?RzM5V3FNdS9BWEkvcVVyMUx6UWdKZUt2a21uOGlpTDQwVXJvSXI3VXZwc2Iz?=
 =?utf-8?B?VDA5a2tNVjJpV0xOTVNORXN4Z2FzN1I2V0grc1hWdTdwaEY0RU8zYzlzeVJz?=
 =?utf-8?Q?RL0jbk5YafR5IAjgERJhJ40xn?=
X-OriginatorOrg: amd.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 63414fed-1f62-4f05-9b92-08dc63b0e31e
X-MS-Exchange-CrossTenant-AuthSource: BL1PR12MB5874.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 23 Apr 2024 16:17:35.6998
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 3dd8961f-e488-4e60-8e11-a82d994e183d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: 1qMygwWxiWyXcv/M64GduHDCKLKU/o9vIM4fKKzZhNGrU60HNcmGhRoEQMoRroac
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MN2PR12MB4472


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 <tanmay.shah@amd.com>
>> ---
>>  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
>>