Re: [PATCH 3/3] drivers: remoteproc: add Versal and Versal-NET support
On 3/19/24 12:25 AM, Krzysztof Kozlowski wrote:
> 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
---
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.
Okay understood. I believe then removing hardcode addresses makes more sense in
that
case. So, driver will become same for all the devices.
>
> Best regards,
> Krzysztof
>
Re: [PATCH 3/3] drivers: remoteproc: add Versal and Versal-NET support
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
>>> ---
>>> 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
Re: [PATCH 3/3] drivers: remoteproc: add Versal and Versal-NET support
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
>> ---
>> 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.
Thanks.
>
>
> Best regards,
> Krzysztof
>
Re: [PATCH 3/3] drivers: remoteproc: add Versal and Versal-NET support
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
> ---
> 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.
Best regards,
Krzysztof
