| Message ID | 20240315211533.1996543-4-tanmay.shah@amd.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Add Versal and Versal-NET platform support | expand |
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@amd.com> > --- > 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
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@amd.com> >> --- >> 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 >
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@amd.com> >>> --- >>> 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
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 <tanmay.shah@amd.com> >>>> --- >>>> 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 >
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); + else + ret = -EINVAL; if (ret) { dev_err(dev, "can't get tcm, err %d\n", ret); @@ -1198,6 +1208,7 @@ 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,zynqmp-r5fss", }, + { .compatible = "xlnx,versal-net-r52fss", }, { /* end of list */ }, }; MODULE_DEVICE_TABLE(of, zynqmp_r5_remoteproc_match);
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@amd.com> --- drivers/remoteproc/xlnx_r5_remoteproc.c | 19 +++++++++++++++---- 1 file changed, 15 insertions(+), 4 deletions(-)