[PATCH 0/2] remoteproc xlnx driver enhancements

[PATCH 0/2] remoteproc xlnx driver enhancements

[Tanmay Shah]

- Disable unsupported features from platform driver
- Fix kernle-doc warnings to make documentation better

Tanmay Shah (2):
  remoteproc: xlnx: disable unsupported features
  remoteproc: xlnx: fix kernel-doc warnings

 drivers/remoteproc/xlnx_r5_remoteproc.c | 18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)


base-commit: 76970c009cefc4f9308b57d063beb169762b9164
-- 
2.34.1

[PATCH 1/2] remoteproc: xlnx: disable unsupported features

[Tanmay Shah]

AMD-Xilinx platform driver does not support iommu or recovery mechanism
yet. Disable both features in platform driver.

Signed-off-by: Tanmay Shah <tanmay.shah@amd.com>
---
 drivers/remoteproc/xlnx_r5_remoteproc.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/remoteproc/xlnx_r5_remoteproc.c b/drivers/remoteproc/xlnx_r5_remoteproc.c
index a51523456c6e..0ffd26a47685 100644
--- a/drivers/remoteproc/xlnx_r5_remoteproc.c
+++ b/drivers/remoteproc/xlnx_r5_remoteproc.c
@@ -938,6 +938,8 @@ static struct zynqmp_r5_core *zynqmp_r5_add_rproc_core(struct device *cdev)
 
 	rproc_coredump_set_elf_info(r5_rproc, ELFCLASS32, EM_ARM);
 
+	r5_rproc->recovery_disabled = true;
+	r5_rproc->has_iommu = false;
 	r5_rproc->auto_boot = false;
 	r5_core = r5_rproc->priv;
 	r5_core->dev = cdev;
-- 
2.34.1

[PATCH 2/2] remoteproc: xlnx: fix kernel-doc warnings

[Tanmay Shah]

Fix kernel-doc warnings generated by following command:

`scripts/kernel-doc -Werror -Wshort-desc -Wall \
drivers/remoteproc/xlnx_r5_remoteproc.c > /dev/null`

warning: missing initial short description on line:
 * struct mbox_info

...

Total 8 warnings fixed

Signed-off-by: Tanmay Shah <tanmay.shah@amd.com>
---
 drivers/remoteproc/xlnx_r5_remoteproc.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/remoteproc/xlnx_r5_remoteproc.c b/drivers/remoteproc/xlnx_r5_remoteproc.c
index 0ffd26a47685..0b7b173d0d26 100644
--- a/drivers/remoteproc/xlnx_r5_remoteproc.c
+++ b/drivers/remoteproc/xlnx_r5_remoteproc.c
@@ -68,7 +68,7 @@ struct zynqmp_sram_bank {
 };
 
 /**
- * struct mbox_info
+ * struct mbox_info - mailbox channel data
  *
  * @rx_mc_buf: to copy data from mailbox rx channel
  * @tx_mc_buf: to copy data to mailbox tx channel
@@ -89,7 +89,7 @@ struct mbox_info {
 };
 
 /**
- * struct rsc_tbl_data
+ * struct rsc_tbl_data - resource table metadata
  *
  * Platform specific data structure used to sync resource table address.
  * It's important to maintain order and size of each field on remote side.
@@ -128,7 +128,7 @@ static const struct mem_bank_data zynqmp_tcm_banks_lockstep[] = {
 };
 
 /**
- * struct zynqmp_r5_core
+ * struct zynqmp_r5_core - remoteproc core's internal data
  *
  * @rsc_tbl_va: resource table virtual address
  * @sram: Array of sram memories assigned to this core
@@ -157,7 +157,7 @@ struct zynqmp_r5_core {
 };
 
 /**
- * struct zynqmp_r5_cluster
+ * struct zynqmp_r5_cluster - remoteproc cluster's internal data
  *
  * @dev: r5f subsystem cluster device node
  * @mode: cluster mode of type zynqmp_r5_cluster_mode
@@ -732,7 +732,7 @@ static int zynqmp_r5_parse_fw(struct rproc *rproc, const struct firmware *fw)
 }
 
 /**
- * zynqmp_r5_rproc_prepare()
+ * zynqmp_r5_rproc_prepare() - prepare core to boot/attach
  * adds carveouts for TCM bank and reserved memory regions
  *
  * @rproc: Device node of each rproc
@@ -765,7 +765,7 @@ static int zynqmp_r5_rproc_prepare(struct rproc *rproc)
 }
 
 /**
- * zynqmp_r5_rproc_unprepare()
+ * zynqmp_r5_rproc_unprepare() - programming sequence after stop/detach.
  * Turns off TCM banks using power-domain id
  *
  * @rproc: Device node of each rproc
@@ -908,7 +908,7 @@ static const struct rproc_ops zynqmp_r5_rproc_ops = {
 };
 
 /**
- * zynqmp_r5_add_rproc_core()
+ * zynqmp_r5_add_rproc_core() - Add core data to framework.
  * Allocate and add struct rproc object for each r5f core
  * This is called for each individual r5f core
  *
@@ -1144,7 +1144,7 @@ static int zynqmp_r5_get_tcm_node_from_dt(struct zynqmp_r5_cluster *cluster)
 }
 
 /**
- * zynqmp_r5_get_tcm_node()
+ * zynqmp_r5_get_tcm_node() - Get TCM info
  * Ideally this function should parse tcm node and store information
  * in r5_core instance. For now, Hardcoded TCM information is used.
  * This approach is used as TCM bindings for system-dt is being developed
-- 
2.34.1

Re: [PATCH 1/2] remoteproc: xlnx: disable unsupported features

[Peng Fan]

On Wed, Jul 16, 2025 at 02:30:47PM -0700, Tanmay Shah wrote:
>AMD-Xilinx platform driver does not support iommu or recovery mechanism
>yet. Disable both features in platform driver.
>
>Signed-off-by: Tanmay Shah <tanmay.shah@amd.com>
>---
> drivers/remoteproc/xlnx_r5_remoteproc.c | 2 ++
> 1 file changed, 2 insertions(+)
>
>diff --git a/drivers/remoteproc/xlnx_r5_remoteproc.c b/drivers/remoteproc/xlnx_r5_remoteproc.c
>index a51523456c6e..0ffd26a47685 100644
>--- a/drivers/remoteproc/xlnx_r5_remoteproc.c
>+++ b/drivers/remoteproc/xlnx_r5_remoteproc.c
>@@ -938,6 +938,8 @@ static struct zynqmp_r5_core *zynqmp_r5_add_rproc_core(struct device *cdev)
> 
> 	rproc_coredump_set_elf_info(r5_rproc, ELFCLASS32, EM_ARM);
> 
>+	r5_rproc->recovery_disabled = true;

This does not block sysfs write if my understanding is correct.
recovery_store does not do any check. So even you set it to true,
user could still write sysfs to set to false.

>+	r5_rproc->has_iommu = false;

The default value should already be false. Is there a need to
set it to false?

Regards,
Peng

> 	r5_rproc->auto_boot = false;
> 	r5_core = r5_rproc->priv;
> 	r5_core->dev = cdev;
>-- 
>2.34.1
>

Re: [PATCH 1/2] remoteproc: xlnx: disable unsupported features

[Mathieu Poirier]

Good morning,

On Wed, Jul 16, 2025 at 02:30:47PM -0700, Tanmay Shah wrote:
> AMD-Xilinx platform driver does not support iommu or recovery mechanism
> yet. Disable both features in platform driver.
> 
> Signed-off-by: Tanmay Shah <tanmay.shah@amd.com>
> ---
>  drivers/remoteproc/xlnx_r5_remoteproc.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/remoteproc/xlnx_r5_remoteproc.c b/drivers/remoteproc/xlnx_r5_remoteproc.c
> index a51523456c6e..0ffd26a47685 100644
> --- a/drivers/remoteproc/xlnx_r5_remoteproc.c
> +++ b/drivers/remoteproc/xlnx_r5_remoteproc.c
> @@ -938,6 +938,8 @@ static struct zynqmp_r5_core *zynqmp_r5_add_rproc_core(struct device *cdev)
>  
>  	rproc_coredump_set_elf_info(r5_rproc, ELFCLASS32, EM_ARM);
>  
> +	r5_rproc->recovery_disabled = true;

If recovery is not supported, and it is set explicitly here, does it mean the
present upstream code is broken?  And if it is broken, how was this tested in
the first place?

> +	r5_rproc->has_iommu = false;
>  	r5_rproc->auto_boot = false;
>  	r5_core = r5_rproc->priv;
>  	r5_core->dev = cdev;
> -- 
> 2.34.1
> 

Re: [PATCH 1/2] remoteproc: xlnx: disable unsupported features

[Mathieu Poirier]

On Mon, Jul 21, 2025 at 02:57:24PM +0800, Peng Fan wrote:
> On Wed, Jul 16, 2025 at 02:30:47PM -0700, Tanmay Shah wrote:
> >AMD-Xilinx platform driver does not support iommu or recovery mechanism
> >yet. Disable both features in platform driver.
> >
> >Signed-off-by: Tanmay Shah <tanmay.shah@amd.com>
> >---
> > drivers/remoteproc/xlnx_r5_remoteproc.c | 2 ++
> > 1 file changed, 2 insertions(+)
> >
> >diff --git a/drivers/remoteproc/xlnx_r5_remoteproc.c b/drivers/remoteproc/xlnx_r5_remoteproc.c
> >index a51523456c6e..0ffd26a47685 100644
> >--- a/drivers/remoteproc/xlnx_r5_remoteproc.c
> >+++ b/drivers/remoteproc/xlnx_r5_remoteproc.c
> >@@ -938,6 +938,8 @@ static struct zynqmp_r5_core *zynqmp_r5_add_rproc_core(struct device *cdev)
> > 
> > 	rproc_coredump_set_elf_info(r5_rproc, ELFCLASS32, EM_ARM);
> > 
> >+	r5_rproc->recovery_disabled = true;
> 
> This does not block sysfs write if my understanding is correct.
> recovery_store does not do any check. So even you set it to true,
> user could still write sysfs to set to false.

That is the case for all drivers and not specific to this one.  

> 
> >+	r5_rproc->has_iommu = false;
> 
> The default value should already be false. Is there a need to
> set it to false?

I never mind to see things set explicitly.  

> 
> Regards,
> Peng
> 
> > 	r5_rproc->auto_boot = false;
> > 	r5_core = r5_rproc->priv;
> > 	r5_core->dev = cdev;
> >-- 
> >2.34.1
> >

Re: [PATCH 1/2] remoteproc: xlnx: disable unsupported features

[Tanmay Shah]

On 7/21/25 10:24 AM, Mathieu Poirier wrote:
> Good morning,
> 
> On Wed, Jul 16, 2025 at 02:30:47PM -0700, Tanmay Shah wrote:
>> AMD-Xilinx platform driver does not support iommu or recovery mechanism
>> yet. Disable both features in platform driver.
>>
>> Signed-off-by: Tanmay Shah <tanmay.shah@amd.com>
>> ---
>>   drivers/remoteproc/xlnx_r5_remoteproc.c | 2 ++
>>   1 file changed, 2 insertions(+)
>>
>> diff --git a/drivers/remoteproc/xlnx_r5_remoteproc.c b/drivers/remoteproc/xlnx_r5_remoteproc.c
>> index a51523456c6e..0ffd26a47685 100644
>> --- a/drivers/remoteproc/xlnx_r5_remoteproc.c
>> +++ b/drivers/remoteproc/xlnx_r5_remoteproc.c
>> @@ -938,6 +938,8 @@ static struct zynqmp_r5_core *zynqmp_r5_add_rproc_core(struct device *cdev)
>>   
>>   	rproc_coredump_set_elf_info(r5_rproc, ELFCLASS32, EM_ARM);
>>   
>> +	r5_rproc->recovery_disabled = true;
> 
> If recovery is not supported, and it is set explicitly here, does it mean the
> present upstream code is broken?  And if it is broken, how was this tested in
> the first place?

I think upstream code can be improved to change "recovery_disabled" to 
"recovery_enabled" and set it in each platform driver if feature is 
supported and keep it disabled by default.

When upstreaming base driver I wasn't aware that there is recovery 
feature in remoteproc until recently when I started working on it. I 
guess too focused on fixing base driver features. That is how I missed 
to add recovery_disabled and test it as well.

That is why it's better idea to replace "recovery_disabled" with 
"recovery_enabled" and keep recovery feature disabled by default. Same 
as "has_iommu". So platform drivers enable it if they actually support it.

I will add recovery feature for xlnx platform later, but until then it's 
better to disable it in the platform driver.

Thanks,
Tanmay

> 
>> +	r5_rproc->has_iommu = false;
>>   	r5_rproc->auto_boot = false;
>>   	r5_core = r5_rproc->priv;
>>   	r5_core->dev = cdev;
>> -- 
>> 2.34.1
>>

Re: [PATCH 1/2] remoteproc: xlnx: disable unsupported features

[Tanmay Shah]

On 7/21/25 10:26 AM, Mathieu Poirier wrote:
> On Mon, Jul 21, 2025 at 02:57:24PM +0800, Peng Fan wrote:
>> On Wed, Jul 16, 2025 at 02:30:47PM -0700, Tanmay Shah wrote:
>>> AMD-Xilinx platform driver does not support iommu or recovery mechanism
>>> yet. Disable both features in platform driver.
>>>
>>> Signed-off-by: Tanmay Shah <tanmay.shah@amd.com>
>>> ---
>>> drivers/remoteproc/xlnx_r5_remoteproc.c | 2 ++
>>> 1 file changed, 2 insertions(+)
>>>
>>> diff --git a/drivers/remoteproc/xlnx_r5_remoteproc.c b/drivers/remoteproc/xlnx_r5_remoteproc.c
>>> index a51523456c6e..0ffd26a47685 100644
>>> --- a/drivers/remoteproc/xlnx_r5_remoteproc.c
>>> +++ b/drivers/remoteproc/xlnx_r5_remoteproc.c
>>> @@ -938,6 +938,8 @@ static struct zynqmp_r5_core *zynqmp_r5_add_rproc_core(struct device *cdev)
>>>
>>> 	rproc_coredump_set_elf_info(r5_rproc, ELFCLASS32, EM_ARM);
>>>
>>> +	r5_rproc->recovery_disabled = true;
>>
>> This does not block sysfs write if my understanding is correct.
>> recovery_store does not do any check. So even you set it to true,
>> user could still write sysfs to set to false.
> 
> That is the case for all drivers and not specific to this one.

Yup, may be we can make recovery node read-only. Or introduce new flag 
in remoteproc structure and set by platform driver that decides if 
recovery can be controlled via sys-fs or not.

But that's a separate discussion and patch.

> 
>>
>>> +	r5_rproc->has_iommu = false;
>>
>> The default value should already be false. Is there a need to
>> set it to false?
> 
> I never mind to see things set explicitly.
> 
>>
>> Regards,
>> Peng
>>
>>> 	r5_rproc->auto_boot = false;
>>> 	r5_core = r5_rproc->priv;
>>> 	r5_core->dev = cdev;
>>> -- 
>>> 2.34.1
>>>
> 

Re: [PATCH 1/2] remoteproc: xlnx: disable unsupported features

[Mathieu Poirier]

On Mon, Jul 21, 2025 at 11:37:23AM -0500, Tanmay Shah wrote:
> 
> 
> On 7/21/25 10:24 AM, Mathieu Poirier wrote:
> > Good morning,
> > 
> > On Wed, Jul 16, 2025 at 02:30:47PM -0700, Tanmay Shah wrote:
> > > AMD-Xilinx platform driver does not support iommu or recovery mechanism
> > > yet. Disable both features in platform driver.
> > > 
> > > Signed-off-by: Tanmay Shah <tanmay.shah@amd.com>
> > > ---
> > >   drivers/remoteproc/xlnx_r5_remoteproc.c | 2 ++
> > >   1 file changed, 2 insertions(+)
> > > 
> > > diff --git a/drivers/remoteproc/xlnx_r5_remoteproc.c b/drivers/remoteproc/xlnx_r5_remoteproc.c
> > > index a51523456c6e..0ffd26a47685 100644
> > > --- a/drivers/remoteproc/xlnx_r5_remoteproc.c
> > > +++ b/drivers/remoteproc/xlnx_r5_remoteproc.c
> > > @@ -938,6 +938,8 @@ static struct zynqmp_r5_core *zynqmp_r5_add_rproc_core(struct device *cdev)
> > >   	rproc_coredump_set_elf_info(r5_rproc, ELFCLASS32, EM_ARM);
> > > +	r5_rproc->recovery_disabled = true;
> > 
> > If recovery is not supported, and it is set explicitly here, does it mean the
> > present upstream code is broken?  And if it is broken, how was this tested in
> > the first place?
> 
> I think upstream code can be improved to change "recovery_disabled" to
> "recovery_enabled" and set it in each platform driver if feature is
> supported and keep it disabled by default.
> 
> When upstreaming base driver I wasn't aware that there is recovery feature
> in remoteproc until recently when I started working on it. I guess too
> focused on fixing base driver features. That is how I missed to add
> recovery_disabled and test it as well.
> 
> That is why it's better idea to replace "recovery_disabled" with
> "recovery_enabled" and keep recovery feature disabled by default. Same as
> "has_iommu". So platform drivers enable it if they actually support it.
> 
> I will add recovery feature for xlnx platform later, but until then it's
> better to disable it in the platform driver.
>

I have applied this set.
 
> Thanks,
> Tanmay
> 
> > 
> > > +	r5_rproc->has_iommu = false;
> > >   	r5_rproc->auto_boot = false;
> > >   	r5_core = r5_rproc->priv;
> > >   	r5_core->dev = cdev;
> > > -- 
> > > 2.34.1
> > > 
>