Re: [PATCH v5] remoteproc: xlnx: add attach detach support
On 6/18/24 11:55 AM, Mathieu Poirier wrote:
> On Tue, Jun 18, 2024 at 11:45:28AM -0500, Tanmay Shah wrote:
>>
>>
>> On 6/17/24 10:40 AM, Mathieu Poirier wrote:
>> > Good day,
>> >
>> > On Mon, Jun 10, 2024 at 08:42:27AM -0700, Tanmay Shah wrote:
>> >> It is possible that remote processor is already running before
>> >> linux boot or remoteproc platform driver probe. Implement required
>> >> remoteproc framework ops to provide resource table address and
>> >> connect or disconnect with remote processor in such case.
>> >>
>> >> Signed-off-by: Tanmay Shah
>> >> ---
>> >>
>> >> Changes in v5:
>> >> - Fix comment on assigning DETACHED state to remoteproc instance
>> >> during driver probe.
>> >> - Fix patch subject and remove "drivers"
>> >>
>> >> Changes in v4:
>> >> - Move change log out of commit text
>> >>
>> >> Changes in v3:
>> >> - Drop SRAM patch from the series
>> >> - Change type from "struct resource_table *" to void __iomem *
>> >> - Change comment format from /** to /*
>> >> - Remove unmap of resource table va address during detach, allowing
>> >> attach-detach-reattach use case.
>> >> - Unmap rsc_data_va after retrieving resource table data structure.
>> >> - Unmap resource table va during driver remove op
>> >>
>> >> Changes in v2:
>> >> - Fix typecast warnings reported using sparse tool.
>> >> - Fix following sparse warnings:
>> >>
>> >> drivers/remoteproc/xlnx_r5_remoteproc.c | 173 +++-
>> >> 1 file changed, 169 insertions(+), 4 deletions(-)
>> >>
>> >> diff --git a/drivers/remoteproc/xlnx_r5_remoteproc.c
>> >> b/drivers/remoteproc/xlnx_r5_remoteproc.c
>> >> index 84243d1dff9f..6ddce5650f95 100644
>> >> --- a/drivers/remoteproc/xlnx_r5_remoteproc.c
>> >> +++ b/drivers/remoteproc/xlnx_r5_remoteproc.c
>> >> @@ -25,6 +25,10 @@
>> >> /* RX mailbox client buffer max length */
>> >> #define MBOX_CLIENT_BUF_MAX (IPI_BUF_LEN_MAX + \
>> >>sizeof(struct zynqmp_ipi_message))
>> >> +
>> >> +#define RSC_TBL_XLNX_MAGIC ((uint32_t)'x' << 24 | (uint32_t)'a' <<
>> >> 16 | \
>> >> + (uint32_t)'m' << 8 | (uint32_t)'p')
>> >> +
>> >> /*
>> >> * settings for RPU cluster mode which
>> >> * reflects possible values of xlnx,cluster-mode dt-property
>> >> @@ -73,6 +77,26 @@ struct mbox_info {
>> >> struct mbox_chan *rx_chan;
>> >> };
>> >>
>> >> +/**
>> >> + * struct rsc_tbl_data
>> >> + *
>> >> + * Platform specific data structure used to sync resource table address.
>> >> + * It's important to maintain order and size of each field on remote
>> >> side.
>> >> + *
>> >> + * @version: version of data structure
>> >> + * @magic_num: 32-bit magic number.
>> >> + * @comp_magic_num: complement of above magic number
>> >> + * @rsc_tbl_size: resource table size
>> >> + * @rsc_tbl: resource table address
>> >> + */
>> >> +struct rsc_tbl_data {
>> >> + const int version;
>> >> + const u32 magic_num;
>> >> + const u32 comp_magic_num;
>> >> + const u32 rsc_tbl_size;
>> >> + const uintptr_t rsc_tbl;
>> >> +} __packed;
>> >> +
>> >> /*
>> >> * Hardcoded TCM bank values. This will stay in driver to maintain
>> >> backward
>> >> * compatibility with device-tree that does not have TCM information.
>> >> @@ -95,20 +119,24 @@ static const struct mem_bank_data
>> >> zynqmp_tcm_banks_lockstep[] = {
>> >> /**
>> >> * struct zynqmp_r5_core
>> >> *
>> >> + * @rsc_tbl_va: resource table virtual address
>> >> * @dev: device of RPU instance
>> >> * @np: device node of RPU instance
>> >> * @tcm_bank_count: number TCM banks accessible to this RPU
>> >> * @tcm_banks: array of each TCM bank data
>> >> * @rproc: rproc handle
>> >> + * @rsc_tbl_size: resource table size retrieved from remote
>> >> * @pm_domain_id: RPU CPU power domain id
>> >> * @ipi: pointer to mailbox information
>> >> */
>> >> struct zynqmp_r5_core {
>> >> + void __iomem *rsc_tbl_va;
>> >> struct device *dev;
>> >> struct device_node *np;
>> >> int tcm_bank_count;
>> >> struct mem_bank_data **tcm_banks;
>> >> struct rproc *rproc;
>> >> + u32 rsc_tbl_size;
>> >> u32 pm_domain_id;
>> >> struct mbox_info *ipi;
>> >> };
>> >> @@ -621,10 +649,19 @@ static int zynqmp_r5_rproc_prepare(struct rproc
>> >> *rproc)
>> >> {
>> >> int ret;
>> >>
>> >> - ret = add_tcm_banks(rproc);
>> >> - if (ret) {
>> >> - dev_err(>dev, "failed to get TCM banks, err %d\n", ret);
>> >> - return ret;
>> >> + /*
>> >> + * For attach/detach use case, Firmware is already loaded so
>> >> + * TCM isn't really needed at all. Also, for security TCM can be
>> >> + * locked in such case and linux may not have access at all.
>> >> + * So avoid adding TCM banks. TCM power-domains requested during attach
>> >> + * callback.
>> >> + */
>> >> + if (rproc->state != RPROC_DETACHED) {
>> >> + ret = add_tcm_banks(rproc);
>> >> + if (ret) {
>> >> + dev_err(>dev, "failed to get TCM
Re: [PATCH v5] remoteproc: xlnx: add attach detach support
On Tue, Jun 18, 2024 at 11:45:28AM -0500, Tanmay Shah wrote:
>
>
> On 6/17/24 10:40 AM, Mathieu Poirier wrote:
> > Good day,
> >
> > On Mon, Jun 10, 2024 at 08:42:27AM -0700, Tanmay Shah wrote:
> >> It is possible that remote processor is already running before
> >> linux boot or remoteproc platform driver probe. Implement required
> >> remoteproc framework ops to provide resource table address and
> >> connect or disconnect with remote processor in such case.
> >>
> >> Signed-off-by: Tanmay Shah
> >> ---
> >>
> >> Changes in v5:
> >> - Fix comment on assigning DETACHED state to remoteproc instance
> >> during driver probe.
> >> - Fix patch subject and remove "drivers"
> >>
> >> Changes in v4:
> >> - Move change log out of commit text
> >>
> >> Changes in v3:
> >> - Drop SRAM patch from the series
> >> - Change type from "struct resource_table *" to void __iomem *
> >> - Change comment format from /** to /*
> >> - Remove unmap of resource table va address during detach, allowing
> >> attach-detach-reattach use case.
> >> - Unmap rsc_data_va after retrieving resource table data structure.
> >> - Unmap resource table va during driver remove op
> >>
> >> Changes in v2:
> >> - Fix typecast warnings reported using sparse tool.
> >> - Fix following sparse warnings:
> >>
> >> drivers/remoteproc/xlnx_r5_remoteproc.c | 173 +++-
> >> 1 file changed, 169 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/drivers/remoteproc/xlnx_r5_remoteproc.c
> >> b/drivers/remoteproc/xlnx_r5_remoteproc.c
> >> index 84243d1dff9f..6ddce5650f95 100644
> >> --- a/drivers/remoteproc/xlnx_r5_remoteproc.c
> >> +++ b/drivers/remoteproc/xlnx_r5_remoteproc.c
> >> @@ -25,6 +25,10 @@
> >> /* RX mailbox client buffer max length */
> >> #define MBOX_CLIENT_BUF_MAX (IPI_BUF_LEN_MAX + \
> >> sizeof(struct zynqmp_ipi_message))
> >> +
> >> +#define RSC_TBL_XLNX_MAGIC((uint32_t)'x' << 24 | (uint32_t)'a' <<
> >> 16 | \
> >> + (uint32_t)'m' << 8 | (uint32_t)'p')
> >> +
> >> /*
> >> * settings for RPU cluster mode which
> >> * reflects possible values of xlnx,cluster-mode dt-property
> >> @@ -73,6 +77,26 @@ struct mbox_info {
> >>struct mbox_chan *rx_chan;
> >> };
> >>
> >> +/**
> >> + * struct rsc_tbl_data
> >> + *
> >> + * Platform specific data structure used to sync resource table address.
> >> + * It's important to maintain order and size of each field on remote side.
> >> + *
> >> + * @version: version of data structure
> >> + * @magic_num: 32-bit magic number.
> >> + * @comp_magic_num: complement of above magic number
> >> + * @rsc_tbl_size: resource table size
> >> + * @rsc_tbl: resource table address
> >> + */
> >> +struct rsc_tbl_data {
> >> + const int version;
> >> + const u32 magic_num;
> >> + const u32 comp_magic_num;
> >> + const u32 rsc_tbl_size;
> >> + const uintptr_t rsc_tbl;
> >> +} __packed;
> >> +
> >> /*
> >> * Hardcoded TCM bank values. This will stay in driver to maintain
> >> backward
> >> * compatibility with device-tree that does not have TCM information.
> >> @@ -95,20 +119,24 @@ static const struct mem_bank_data
> >> zynqmp_tcm_banks_lockstep[] = {
> >> /**
> >> * struct zynqmp_r5_core
> >> *
> >> + * @rsc_tbl_va: resource table virtual address
> >> * @dev: device of RPU instance
> >> * @np: device node of RPU instance
> >> * @tcm_bank_count: number TCM banks accessible to this RPU
> >> * @tcm_banks: array of each TCM bank data
> >> * @rproc: rproc handle
> >> + * @rsc_tbl_size: resource table size retrieved from remote
> >> * @pm_domain_id: RPU CPU power domain id
> >> * @ipi: pointer to mailbox information
> >> */
> >> struct zynqmp_r5_core {
> >> + void __iomem *rsc_tbl_va;
> >>struct device *dev;
> >>struct device_node *np;
> >>int tcm_bank_count;
> >>struct mem_bank_data **tcm_banks;
> >>struct rproc *rproc;
> >> + u32 rsc_tbl_size;
> >>u32 pm_domain_id;
> >>struct mbox_info *ipi;
> >> };
> >> @@ -621,10 +649,19 @@ static int zynqmp_r5_rproc_prepare(struct rproc
> >> *rproc)
> >> {
> >>int ret;
> >>
> >> - ret = add_tcm_banks(rproc);
> >> - if (ret) {
> >> - dev_err(>dev, "failed to get TCM banks, err %d\n", ret);
> >> - return ret;
> >> + /*
> >> + * For attach/detach use case, Firmware is already loaded so
> >> + * TCM isn't really needed at all. Also, for security TCM can be
> >> + * locked in such case and linux may not have access at all.
> >> + * So avoid adding TCM banks. TCM power-domains requested during attach
> >> + * callback.
> >> + */
> >> + if (rproc->state != RPROC_DETACHED) {
> >> + ret = add_tcm_banks(rproc);
> >> + if (ret) {
> >> + dev_err(>dev, "failed to get TCM banks, err
> >> %d\n", ret);
> >> + return ret;
> >> + }
> >
> > In the normal case function add_tcm_banks() will call
> >
Re: [PATCH v5] remoteproc: xlnx: add attach detach support
On 6/17/24 10:40 AM, Mathieu Poirier wrote:
> Good day,
>
> On Mon, Jun 10, 2024 at 08:42:27AM -0700, Tanmay Shah wrote:
>> It is possible that remote processor is already running before
>> linux boot or remoteproc platform driver probe. Implement required
>> remoteproc framework ops to provide resource table address and
>> connect or disconnect with remote processor in such case.
>>
>> Signed-off-by: Tanmay Shah
>> ---
>>
>> Changes in v5:
>> - Fix comment on assigning DETACHED state to remoteproc instance
>> during driver probe.
>> - Fix patch subject and remove "drivers"
>>
>> Changes in v4:
>> - Move change log out of commit text
>>
>> Changes in v3:
>> - Drop SRAM patch from the series
>> - Change type from "struct resource_table *" to void __iomem *
>> - Change comment format from /** to /*
>> - Remove unmap of resource table va address during detach, allowing
>> attach-detach-reattach use case.
>> - Unmap rsc_data_va after retrieving resource table data structure.
>> - Unmap resource table va during driver remove op
>>
>> Changes in v2:
>> - Fix typecast warnings reported using sparse tool.
>> - Fix following sparse warnings:
>>
>> drivers/remoteproc/xlnx_r5_remoteproc.c | 173 +++-
>> 1 file changed, 169 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/remoteproc/xlnx_r5_remoteproc.c
>> b/drivers/remoteproc/xlnx_r5_remoteproc.c
>> index 84243d1dff9f..6ddce5650f95 100644
>> --- a/drivers/remoteproc/xlnx_r5_remoteproc.c
>> +++ b/drivers/remoteproc/xlnx_r5_remoteproc.c
>> @@ -25,6 +25,10 @@
>> /* RX mailbox client buffer max length */
>> #define MBOX_CLIENT_BUF_MAX (IPI_BUF_LEN_MAX + \
>> sizeof(struct zynqmp_ipi_message))
>> +
>> +#define RSC_TBL_XLNX_MAGIC ((uint32_t)'x' << 24 | (uint32_t)'a' << 16 | \
>> + (uint32_t)'m' << 8 | (uint32_t)'p')
>> +
>> /*
>> * settings for RPU cluster mode which
>> * reflects possible values of xlnx,cluster-mode dt-property
>> @@ -73,6 +77,26 @@ struct mbox_info {
>> struct mbox_chan *rx_chan;
>> };
>>
>> +/**
>> + * struct rsc_tbl_data
>> + *
>> + * Platform specific data structure used to sync resource table address.
>> + * It's important to maintain order and size of each field on remote side.
>> + *
>> + * @version: version of data structure
>> + * @magic_num: 32-bit magic number.
>> + * @comp_magic_num: complement of above magic number
>> + * @rsc_tbl_size: resource table size
>> + * @rsc_tbl: resource table address
>> + */
>> +struct rsc_tbl_data {
>> +const int version;
>> +const u32 magic_num;
>> +const u32 comp_magic_num;
>> +const u32 rsc_tbl_size;
>> +const uintptr_t rsc_tbl;
>> +} __packed;
>> +
>> /*
>> * Hardcoded TCM bank values. This will stay in driver to maintain backward
>> * compatibility with device-tree that does not have TCM information.
>> @@ -95,20 +119,24 @@ static const struct mem_bank_data
>> zynqmp_tcm_banks_lockstep[] = {
>> /**
>> * struct zynqmp_r5_core
>> *
>> + * @rsc_tbl_va: resource table virtual address
>> * @dev: device of RPU instance
>> * @np: device node of RPU instance
>> * @tcm_bank_count: number TCM banks accessible to this RPU
>> * @tcm_banks: array of each TCM bank data
>> * @rproc: rproc handle
>> + * @rsc_tbl_size: resource table size retrieved from remote
>> * @pm_domain_id: RPU CPU power domain id
>> * @ipi: pointer to mailbox information
>> */
>> struct zynqmp_r5_core {
>> +void __iomem *rsc_tbl_va;
>> struct device *dev;
>> struct device_node *np;
>> int tcm_bank_count;
>> struct mem_bank_data **tcm_banks;
>> struct rproc *rproc;
>> +u32 rsc_tbl_size;
>> u32 pm_domain_id;
>> struct mbox_info *ipi;
>> };
>> @@ -621,10 +649,19 @@ static int zynqmp_r5_rproc_prepare(struct rproc *rproc)
>> {
>> int ret;
>>
>> -ret = add_tcm_banks(rproc);
>> -if (ret) {
>> -dev_err(>dev, "failed to get TCM banks, err %d\n", ret);
>> -return ret;
>> +/*
>> + * For attach/detach use case, Firmware is already loaded so
>> + * TCM isn't really needed at all. Also, for security TCM can be
>> + * locked in such case and linux may not have access at all.
>> + * So avoid adding TCM banks. TCM power-domains requested during attach
>> + * callback.
>> + */
>> +if (rproc->state != RPROC_DETACHED) {
>> +ret = add_tcm_banks(rproc);
>> +if (ret) {
>> +dev_err(>dev, "failed to get TCM banks, err
>> %d\n", ret);
>> +return ret;
>> +}
>
> In the normal case function add_tcm_banks() will call zynqmp_pm_request_node()
> but in the attach case, that gets done in zynqmp_r5_attach(). Either way,
> zynqmp_pm_release_node() is called in zynqmp_r5_rproc_unprepare(). This is
> highly confusing.
>
> I suggest adding a check to see if the remote processor is being attached to
> in
Re: [PATCH v5] remoteproc: xlnx: add attach detach support
Good day,
On Mon, Jun 10, 2024 at 08:42:27AM -0700, Tanmay Shah wrote:
> It is possible that remote processor is already running before
> linux boot or remoteproc platform driver probe. Implement required
> remoteproc framework ops to provide resource table address and
> connect or disconnect with remote processor in such case.
>
> Signed-off-by: Tanmay Shah
> ---
>
> Changes in v5:
> - Fix comment on assigning DETACHED state to remoteproc instance
> during driver probe.
> - Fix patch subject and remove "drivers"
>
> Changes in v4:
> - Move change log out of commit text
>
> Changes in v3:
> - Drop SRAM patch from the series
> - Change type from "struct resource_table *" to void __iomem *
> - Change comment format from /** to /*
> - Remove unmap of resource table va address during detach, allowing
> attach-detach-reattach use case.
> - Unmap rsc_data_va after retrieving resource table data structure.
> - Unmap resource table va during driver remove op
>
> Changes in v2:
> - Fix typecast warnings reported using sparse tool.
> - Fix following sparse warnings:
>
> drivers/remoteproc/xlnx_r5_remoteproc.c | 173 +++-
> 1 file changed, 169 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/remoteproc/xlnx_r5_remoteproc.c
> b/drivers/remoteproc/xlnx_r5_remoteproc.c
> index 84243d1dff9f..6ddce5650f95 100644
> --- a/drivers/remoteproc/xlnx_r5_remoteproc.c
> +++ b/drivers/remoteproc/xlnx_r5_remoteproc.c
> @@ -25,6 +25,10 @@
> /* RX mailbox client buffer max length */
> #define MBOX_CLIENT_BUF_MAX (IPI_BUF_LEN_MAX + \
>sizeof(struct zynqmp_ipi_message))
> +
> +#define RSC_TBL_XLNX_MAGIC ((uint32_t)'x' << 24 | (uint32_t)'a' << 16 | \
> + (uint32_t)'m' << 8 | (uint32_t)'p')
> +
> /*
> * settings for RPU cluster mode which
> * reflects possible values of xlnx,cluster-mode dt-property
> @@ -73,6 +77,26 @@ struct mbox_info {
> struct mbox_chan *rx_chan;
> };
>
> +/**
> + * struct rsc_tbl_data
> + *
> + * Platform specific data structure used to sync resource table address.
> + * It's important to maintain order and size of each field on remote side.
> + *
> + * @version: version of data structure
> + * @magic_num: 32-bit magic number.
> + * @comp_magic_num: complement of above magic number
> + * @rsc_tbl_size: resource table size
> + * @rsc_tbl: resource table address
> + */
> +struct rsc_tbl_data {
> + const int version;
> + const u32 magic_num;
> + const u32 comp_magic_num;
> + const u32 rsc_tbl_size;
> + const uintptr_t rsc_tbl;
> +} __packed;
> +
> /*
> * Hardcoded TCM bank values. This will stay in driver to maintain backward
> * compatibility with device-tree that does not have TCM information.
> @@ -95,20 +119,24 @@ static const struct mem_bank_data
> zynqmp_tcm_banks_lockstep[] = {
> /**
> * struct zynqmp_r5_core
> *
> + * @rsc_tbl_va: resource table virtual address
> * @dev: device of RPU instance
> * @np: device node of RPU instance
> * @tcm_bank_count: number TCM banks accessible to this RPU
> * @tcm_banks: array of each TCM bank data
> * @rproc: rproc handle
> + * @rsc_tbl_size: resource table size retrieved from remote
> * @pm_domain_id: RPU CPU power domain id
> * @ipi: pointer to mailbox information
> */
> struct zynqmp_r5_core {
> + void __iomem *rsc_tbl_va;
> struct device *dev;
> struct device_node *np;
> int tcm_bank_count;
> struct mem_bank_data **tcm_banks;
> struct rproc *rproc;
> + u32 rsc_tbl_size;
> u32 pm_domain_id;
> struct mbox_info *ipi;
> };
> @@ -621,10 +649,19 @@ static int zynqmp_r5_rproc_prepare(struct rproc *rproc)
> {
> int ret;
>
> - ret = add_tcm_banks(rproc);
> - if (ret) {
> - dev_err(>dev, "failed to get TCM banks, err %d\n", ret);
> - return ret;
> + /*
> + * For attach/detach use case, Firmware is already loaded so
> + * TCM isn't really needed at all. Also, for security TCM can be
> + * locked in such case and linux may not have access at all.
> + * So avoid adding TCM banks. TCM power-domains requested during attach
> + * callback.
> + */
> + if (rproc->state != RPROC_DETACHED) {
> + ret = add_tcm_banks(rproc);
> + if (ret) {
> + dev_err(>dev, "failed to get TCM banks, err
> %d\n", ret);
> + return ret;
> + }
In the normal case function add_tcm_banks() will call zynqmp_pm_request_node()
but in the attach case, that gets done in zynqmp_r5_attach(). Either way,
zynqmp_pm_release_node() is called in zynqmp_r5_rproc_unprepare(). This is
highly confusing.
I suggest adding a check to see if the remote processor is being attached to in
add_tcm_banks() and skip the rest of the TCM initialization if it is the case.
> }
>
> ret = add_mem_regions_carveout(rproc);
> @@ -662,6
