Re: [PATCH 3/3] drivers: remoteproc: add Versal and Versal-NET support

[Krzysztof Kozlowski]

On 19/03/2024 02:06, Tanmay Shah wrote:
> 
> 
> On 3/17/24 1:55 PM, Krzysztof Kozlowski wrote:
>> On 15/03/2024 22:15, Tanmay Shah wrote:
>>> AMD-Xilinx Versal and Versal-NET are successor of ZynqMP platform. ZynqMP
>>> remoteproc driver is mostly compatible with new platforms except few
>>> platform specific differences.
>>>
>>> Versal has same IP of cortex-R5 cores hence maintained compatible string
>>> same as ZynqMP platform. However, hardcode TCM addresses are not
>>> supported for new platforms and must be provided in device-tree as per
>>> new bindings. This makes TCM representation data-driven and easy to
>>> maintain. This check is provided in the driver.
>>>
>>> For Versal-NET platform, TCM doesn't need to be configured in lockstep
>>> mode or split mode. Hence that call to PMC firmware is avoided in the
>>> driver for Versal-NET platform.
>>>
>>> Signed-off-by: Tanmay Shah <tanmay.shah@xxxxxxx>
>>> ---
>>>  drivers/remoteproc/xlnx_r5_remoteproc.c | 19 +++++++++++++++----
>>>  1 file changed, 15 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/remoteproc/xlnx_r5_remoteproc.c b/drivers/remoteproc/xlnx_r5_remoteproc.c
>>> index d4a22caebaad..193bc159d1b4 100644
>>> --- a/drivers/remoteproc/xlnx_r5_remoteproc.c
>>> +++ b/drivers/remoteproc/xlnx_r5_remoteproc.c
>>> @@ -323,9 +323,12 @@ static int zynqmp_r5_set_mode(struct zynqmp_r5_core *r5_core,
>>>  		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");
>>> +	/* TCM configuration is not needed in versal-net */
>>> +	if (device_is_compatible(r5_core->dev, "xlnx,zynqmp-r5f")) {
>>> +		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;
>>>  }
>>> @@ -933,10 +936,17 @@ static int zynqmp_r5_core_init(struct zynqmp_r5_cluster *cluster,
>>>  	int ret, i;
>>>  
>>>  	r5_core = cluster->r5_cores[0];
>>> +
>>> +	/*
>>> +	 * New platforms must use device tree for TCM parsing.
>>> +	 * Only ZynqMP uses hardcode TCM.
>>> +	 */
>>>  	if (of_find_property(r5_core->np, "reg", NULL))
>>>  		ret = zynqmp_r5_get_tcm_node_from_dt(cluster);
>>> -	else
>>> +	else if (of_machine_is_compatible("xlnx,zynqmp"))
>>>  		ret = zynqmp_r5_get_tcm_node(cluster);
>>
>> That's poor code. Your drivers should not depend on platform. I don't
>> understand why you need to do this and how is even related to this patch.
> 
> You are correct, ideally this shouldn't be needed. However, this driver contains
> hardcode TCM addresses that were used before TCM bindings were designed and available in
> device-tree. This check is provided to maintain backward compatibility with device-tree
> where TCM isn't expected.
> 
> For new platforms (Versal and Versal-NET) TCM must be provided in device-tree and for
> ZynqMP if it's not in device-tree then to maintain backward compatibility hardcode
> addresses are used.

That does not work like this. You cannot bind to some sort of different
compatible. If you disagree, please list the compatibles the driver
binds to.

Best regards,
Krzysztof