Attach detach ops are needed to connect to remote processor that is
running before remoteproc driver is probed. Implement remoteproc
framework ops that enables such use case on AMD-Xilinx platforms.
Remote processor can also use On Chip sram Memory (OCM) for various
purpose. For example, for fast code execution or data access compare
to DDR memory. Such sram region is made available to remoteproc nodes
via "sram" property. Add support in driver to parse and use OCM memory
via sram property.
Changes in v2:
- Fix following sparse warnings
drivers/remoteproc/xlnx_r5_remoteproc.c:827:21: sparse: expected struct rsc_tbl_data *rsc_data_va
drivers/remoteproc/xlnx_r5_remoteproc.c:844:18: sparse: expected struct resource_table *rsc_addr
drivers/remoteproc/xlnx_r5_remoteproc.c:898:24: sparse: expected void volatile [noderef] __iomem *addr
drivers/remoteproc/xlnx_r5_remoteproc.c:995:26: sparse: warning: Using plain integer as NULL pointer
Tanmay Shah (2):
drivers: remoteproc: xlnx: add attach detach support
drivers: remoteproc: xlnx: add sram support
drivers/remoteproc/xlnx_r5_remoteproc.c | 385 +++++++++++++++++++++++-
1 file changed, 380 insertions(+), 5 deletions(-)
base-commit: c8d8f841e95bcc07ac8c5621fc171a24f1fd5cdb
--
2.25.1
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 <[email protected]>
---
Changes in v2:
- Fix following sparse warnings
drivers/remoteproc/xlnx_r5_remoteproc.c:827:21: sparse: expected struct rsc_tbl_data *rsc_data_va
drivers/remoteproc/xlnx_r5_remoteproc.c:844:18: sparse: expected struct resource_table *rsc_addr
drivers/remoteproc/xlnx_r5_remoteproc.c:898:24: sparse: expected void volatile [noderef] __iomem *addr
drivers/remoteproc/xlnx_r5_remoteproc.c | 164 +++++++++++++++++++++++-
1 file changed, 160 insertions(+), 4 deletions(-)
diff --git a/drivers/remoteproc/xlnx_r5_remoteproc.c b/drivers/remoteproc/xlnx_r5_remoteproc.c
index 84243d1dff9f..039370cffa32 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,15 @@ struct mbox_info {
struct mbox_chan *rx_chan;
};
+/* Xilinx Platform specific data structure */
+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 +108,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 {
+ struct resource_table *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 +638,19 @@ static int zynqmp_r5_rproc_prepare(struct rproc *rproc)
{
int ret;
- ret = add_tcm_banks(rproc);
- if (ret) {
- dev_err(&rproc->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(&rproc->dev, "failed to get TCM banks, err %d\n", ret);
+ return ret;
+ }
}
ret = add_mem_regions_carveout(rproc);
@@ -662,6 +688,123 @@ static int zynqmp_r5_rproc_unprepare(struct rproc *rproc)
return 0;
}
+static struct resource_table *zynqmp_r5_get_loaded_rsc_table(struct rproc *rproc,
+ size_t *size)
+{
+ struct zynqmp_r5_core *r5_core;
+
+ r5_core = rproc->priv;
+
+ *size = r5_core->rsc_tbl_size;
+
+ return r5_core->rsc_tbl_va;
+}
+
+static int zynqmp_r5_get_rsc_table_va(struct zynqmp_r5_core *r5_core)
+{
+ struct device *dev = r5_core->dev;
+ struct rsc_tbl_data *rsc_data_va;
+ struct resource_table *rsc_addr;
+ struct resource res_mem;
+ struct device_node *np;
+ int ret;
+
+ /**
+ * It is expected from remote processor firmware to provide resource
+ * table address via struct rsc_tbl_data data structure.
+ * Start address of first entry under "memory-region" property list
+ * contains that data structure which holds resource table address, size
+ * and some magic number to validate correct resource table entry.
+ */
+ np = of_parse_phandle(r5_core->np, "memory-region", 0);
+ if (!np) {
+ dev_err(dev, "failed to get memory region dev node\n");
+ return -EINVAL;
+ }
+
+ ret = of_address_to_resource(np, 0, &res_mem);
+ if (ret) {
+ dev_err(dev, "failed to get memory-region resource addr\n");
+ return -EINVAL;
+ }
+
+ rsc_data_va = (struct rsc_tbl_data *)devm_ioremap_wc(dev, res_mem.start,
+ sizeof(struct rsc_tbl_data));
+ if (!rsc_data_va) {
+ dev_err(dev, "failed to map resource table data address\n");
+ return -EIO;
+ }
+
+ /**
+ * If RSC_TBL_XLNX_MAGIC number and its complement isn't found then
+ * do not consider resource table address valid and don't attach
+ */
+ if (rsc_data_va->magic_num != RSC_TBL_XLNX_MAGIC ||
+ rsc_data_va->comp_magic_num != ~RSC_TBL_XLNX_MAGIC) {
+ dev_dbg(dev, "invalid magic number, won't attach\n");
+ return -EINVAL;
+ }
+
+ rsc_addr = (struct resource_table *)ioremap_wc(rsc_data_va->rsc_tbl,
+ rsc_data_va->rsc_tbl_size);
+ if (!rsc_addr) {
+ dev_err(dev, "failed to get rsc_addr\n");
+ return -EINVAL;
+ }
+
+ /**
+ * As of now resource table version 1 is expected. Don't fail to attach
+ * but warn users about it.
+ */
+ if (rsc_addr->ver != 1)
+ dev_warn(dev, "unexpected resource table version %d\n",
+ rsc_addr->ver);
+
+ r5_core->rsc_tbl_size = rsc_data_va->rsc_tbl_size;
+ r5_core->rsc_tbl_va = rsc_addr;
+
+ return 0;
+}
+
+static int zynqmp_r5_attach(struct rproc *rproc)
+{
+ struct zynqmp_r5_core *r5_core = rproc->priv;
+ int i, pm_domain_id, ret;
+
+ /*
+ * Firmware is loaded in TCM. Request TCM power domains to notify
+ * platform management controller that TCM is in use. This will be
+ * released during unprepare callback.
+ */
+ for (i = 0; i < r5_core->tcm_bank_count; i++) {
+ pm_domain_id = r5_core->tcm_banks[i]->pm_domain_id;
+ ret = zynqmp_pm_request_node(pm_domain_id,
+ ZYNQMP_PM_CAPABILITY_ACCESS, 0,
+ ZYNQMP_PM_REQUEST_ACK_BLOCKING);
+ if (ret < 0)
+ pr_warn("TCM %d can't be requested\n", i);
+ }
+
+ return 0;
+}
+
+static int zynqmp_r5_detach(struct rproc *rproc)
+{
+ struct zynqmp_r5_core *r5_core = rproc->priv;
+
+ /*
+ * Generate last notification to remote after clearing virtio flag.
+ * Remote can avoid polling on virtio reset flag if kick is generated
+ * during detach by host and check virtio reset flag on kick interrupt.
+ */
+ zynqmp_r5_rproc_kick(rproc, 0);
+
+ iounmap((void __iomem *)r5_core->rsc_tbl_va);
+ r5_core->rsc_tbl_va = NULL;
+
+ return 0;
+}
+
static const struct rproc_ops zynqmp_r5_rproc_ops = {
.prepare = zynqmp_r5_rproc_prepare,
.unprepare = zynqmp_r5_rproc_unprepare,
@@ -673,6 +816,9 @@ static const struct rproc_ops zynqmp_r5_rproc_ops = {
.sanity_check = rproc_elf_sanity_check,
.get_boot_addr = rproc_elf_get_boot_addr,
.kick = zynqmp_r5_rproc_kick,
+ .get_loaded_rsc_table = zynqmp_r5_get_loaded_rsc_table,
+ .attach = zynqmp_r5_attach,
+ .detach = zynqmp_r5_detach,
};
/**
@@ -723,6 +869,16 @@ static struct zynqmp_r5_core *zynqmp_r5_add_rproc_core(struct device *cdev)
goto free_rproc;
}
+ /*
+ * Move rproc state to DETACHED to give one time opportunity to attach
+ * if firmware is already available in the memory. This can happen if
+ * firmware is loaded via debugger or by any other agent in the system.
+ * If firmware isn't available in the memory and resource table isn't found,
+ * then rproc state stay OFFLINE.
+ */
+ if (!zynqmp_r5_get_rsc_table_va(r5_core))
+ r5_rproc->state = RPROC_DETACHED;
+
r5_core->rproc = r5_rproc;
return r5_core;
--
2.25.1
AMD-Xilinx zynqmp platform contains on-chip sram memory (OCM).
R5 cores can access OCM and access is faster than DDR memory but slower
than TCM memories available. Sram region can have optional multiple
power-domains.
Signed-off-by: Tanmay Shah <[email protected]>
---
Changes in v2:
- Fix integer assignement to variable that also fixes following sparse warnings
drivers/remoteproc/xlnx_r5_remoteproc.c:995:26: sparse: warning: Using plain integer as NULL pointer
drivers/remoteproc/xlnx_r5_remoteproc.c | 221 +++++++++++++++++++++++-
1 file changed, 220 insertions(+), 1 deletion(-)
diff --git a/drivers/remoteproc/xlnx_r5_remoteproc.c b/drivers/remoteproc/xlnx_r5_remoteproc.c
index 039370cffa32..e0a78a5d4ad5 100644
--- a/drivers/remoteproc/xlnx_r5_remoteproc.c
+++ b/drivers/remoteproc/xlnx_r5_remoteproc.c
@@ -56,6 +56,21 @@ struct mem_bank_data {
char *bank_name;
};
+/**
+ * struct zynqmp_sram_bank - sram bank description
+ *
+ * @sram_res: sram address region information
+ * @power_domains: Array of pm domain id
+ * @num_pd: total pm domain id count
+ * @da: device address of sram
+ */
+struct zynqmp_sram_bank {
+ struct resource sram_res;
+ int *power_domains;
+ int num_pd;
+ u32 da;
+};
+
/**
* struct mbox_info
*
@@ -109,6 +124,8 @@ static const struct mem_bank_data zynqmp_tcm_banks_lockstep[] = {
* struct zynqmp_r5_core
*
* @rsc_tbl_va: resource table virtual address
+ * @sram: Array of sram memories assigned to this core
+ * @num_sram: number of sram for this core
* @dev: device of RPU instance
* @np: device node of RPU instance
* @tcm_bank_count: number TCM banks accessible to this RPU
@@ -120,6 +137,8 @@ static const struct mem_bank_data zynqmp_tcm_banks_lockstep[] = {
*/
struct zynqmp_r5_core {
struct resource_table *rsc_tbl_va;
+ struct zynqmp_sram_bank **sram;
+ int num_sram;
struct device *dev;
struct device_node *np;
int tcm_bank_count;
@@ -483,6 +502,69 @@ static int add_mem_regions_carveout(struct rproc *rproc)
return 0;
}
+static int add_sram_carveouts(struct rproc *rproc)
+{
+ struct zynqmp_r5_core *r5_core = rproc->priv;
+ struct rproc_mem_entry *rproc_mem;
+ struct zynqmp_sram_bank *sram;
+ dma_addr_t dma_addr;
+ int da, i, j, ret;
+ size_t len;
+
+ for (i = 0; i < r5_core->num_sram; i++) {
+ sram = r5_core->sram[i];
+
+ dma_addr = (dma_addr_t)sram->sram_res.start;
+ len = resource_size(&sram->sram_res);
+ da = sram->da;
+
+ for (j = 0; j < sram->num_pd; j++) {
+ ret = zynqmp_pm_request_node(sram->power_domains[j],
+ ZYNQMP_PM_CAPABILITY_ACCESS, 0,
+ ZYNQMP_PM_REQUEST_ACK_BLOCKING);
+ if (ret < 0) {
+ dev_err(r5_core->dev,
+ "failed to request on SRAM pd 0x%x",
+ sram->power_domains[j]);
+ goto fail_sram;
+ } else {
+ pr_err("sram pd 0x%x request success\n",
+ sram->power_domains[j]);
+ }
+ }
+
+ /* Register associated reserved memory regions */
+ rproc_mem = rproc_mem_entry_init(&rproc->dev, NULL,
+ (dma_addr_t)dma_addr,
+ len, da,
+ zynqmp_r5_mem_region_map,
+ zynqmp_r5_mem_region_unmap,
+ sram->sram_res.name);
+
+ rproc_add_carveout(rproc, rproc_mem);
+ rproc_coredump_add_segment(rproc, da, len);
+
+ dev_err(&rproc->dev, "sram carveout %s addr=%llx, da=0x%x, size=0x%lx",
+ sram->sram_res.name, dma_addr, da, len);
+ }
+
+ return 0;
+
+fail_sram:
+ /* Release current sram pd. */
+ while (--j >= 0)
+ zynqmp_pm_release_node(sram->power_domains[j]);
+
+ /* Release previously requested sram pd. */
+ while (--i >= 0) {
+ sram = r5_core->sram[i];
+ for (j = 0; j < sram->num_pd; j++)
+ zynqmp_pm_release_node(sram->power_domains[j]);
+ }
+
+ return ret;
+}
+
/*
* tcm_mem_unmap()
* @rproc: single R5 core's corresponding rproc instance
@@ -659,6 +741,12 @@ static int zynqmp_r5_rproc_prepare(struct rproc *rproc)
return ret;
}
+ ret = add_sram_carveouts(rproc);
+ if (ret) {
+ dev_err(&rproc->dev, "failed to get sram carveout %d\n", ret);
+ return ret;
+ }
+
return 0;
}
@@ -673,8 +761,9 @@ static int zynqmp_r5_rproc_prepare(struct rproc *rproc)
static int zynqmp_r5_rproc_unprepare(struct rproc *rproc)
{
struct zynqmp_r5_core *r5_core;
+ struct zynqmp_sram_bank *sram;
u32 pm_domain_id;
- int i;
+ int i, j;
r5_core = rproc->priv;
@@ -685,6 +774,13 @@ static int zynqmp_r5_rproc_unprepare(struct rproc *rproc)
"can't turn off TCM bank 0x%x", pm_domain_id);
}
+ /* Release sram power-domains. */
+ for (i = 0; i < r5_core->num_sram; i++) {
+ sram = r5_core->sram[i];
+ for (j = 0; j < sram->num_pd; j++)
+ zynqmp_pm_release_node(sram->power_domains[j]);
+ }
+
return 0;
}
@@ -887,6 +983,123 @@ static struct zynqmp_r5_core *zynqmp_r5_add_rproc_core(struct device *cdev)
return ERR_PTR(ret);
}
+static int zynqmp_r5_get_sram_pd(struct device *r5_core_dev,
+ struct device_node *sram_np, int **power_domains,
+ int *num_pd)
+{
+ struct of_phandle_args out_args;
+ int pd_count, i, ret;
+ int *pd_list;
+
+ if (!of_find_property(sram_np, "power-domains", NULL)) {
+ *num_pd = 0;
+ return 0;
+ }
+
+ pd_count = of_count_phandle_with_args(sram_np, "power-domains",
+ "#power-domain-cells");
+
+ pd_list = devm_kcalloc(r5_core_dev, pd_count, sizeof(int), GFP_KERNEL);
+ if (!pd_list)
+ return -ENOMEM;
+
+ for (i = 0; i < pd_count; i++) {
+ ret = of_parse_phandle_with_args(sram_np, "power-domains",
+ "#power-domain-cells",
+ i, &out_args);
+ if (ret) {
+ dev_err(r5_core_dev, "%s: power-domains idx %d parsing failed\n",
+ sram_np->name, i);
+ return ret;
+ }
+
+ of_node_put(out_args.np);
+ pd_list[i] = out_args.args[0];
+ }
+
+ *power_domains = pd_list;
+ *num_pd = pd_count;
+
+ return 0;
+}
+
+static int zynqmp_r5_get_sram_banks(struct zynqmp_r5_core *r5_core)
+{
+ struct zynqmp_sram_bank **sram, *sram_data;
+ struct device_node *np = r5_core->np;
+ struct device *dev = r5_core->dev;
+ struct device_node *sram_np;
+ int num_sram, i, ret;
+ u64 abs_addr, size;
+
+ num_sram = of_property_count_elems_of_size(np, "sram", sizeof(phandle));
+ if (num_sram <= 0) {
+ dev_err(dev, "Invalid sram property, ret = %d\n",
+ num_sram);
+ return -EINVAL;
+ }
+
+ sram = devm_kcalloc(dev, num_sram,
+ sizeof(struct zynqmp_sram_bank *), GFP_KERNEL);
+ if (!sram)
+ return -ENOMEM;
+
+ for (i = 0; i < num_sram; i++) {
+ sram_data = devm_kzalloc(dev, sizeof(struct zynqmp_sram_bank),
+ GFP_KERNEL);
+ if (!sram_data)
+ return -ENOMEM;
+
+ sram_np = of_parse_phandle(np, "sram", i);
+ if (!sram_np) {
+ dev_err(dev, "failed to get sram %d phandle\n", i);
+ return -EINVAL;
+ }
+
+ if (!of_device_is_available(sram_np)) {
+ of_node_put(sram_np);
+ dev_err(dev, "sram device not available\n");
+ return -EINVAL;
+ }
+
+ ret = of_address_to_resource(sram_np, 0, &sram_data->sram_res);
+ of_node_put(sram_np);
+ if (ret) {
+ dev_err(dev, "addr to res failed\n");
+ return ret;
+ }
+
+ /* Get SRAM device address */
+ ret = of_property_read_reg(sram_np, i, &abs_addr, &size);
+ if (ret) {
+ dev_err(dev, "failed to get reg property\n");
+ return ret;
+ }
+
+ sram_data->da = (u32)abs_addr;
+
+ ret = zynqmp_r5_get_sram_pd(r5_core->dev, sram_np,
+ &sram_data->power_domains,
+ &sram_data->num_pd);
+ if (ret) {
+ dev_err(dev, "failed to get power-domains for %d sram\n", i);
+ return ret;
+ }
+
+ sram[i] = sram_data;
+
+ dev_dbg(dev, "sram %d: name=%s, addr=0x%llx, da=0x%x, size=0x%llx, num_pd=%d\n",
+ i, sram[i]->sram_res.name, sram[i]->sram_res.start,
+ sram[i]->da, resource_size(&sram[i]->sram_res),
+ sram[i]->num_pd);
+ }
+
+ r5_core->sram = sram;
+ r5_core->num_sram = num_sram;
+
+ return 0;
+}
+
static int zynqmp_r5_get_tcm_node_from_dt(struct zynqmp_r5_cluster *cluster)
{
int i, j, tcm_bank_count, ret, tcm_pd_idx, pd_count;
@@ -1101,6 +1314,12 @@ static int zynqmp_r5_core_init(struct zynqmp_r5_cluster *cluster,
return ret;
}
}
+
+ if (of_find_property(r5_core->np, "sram", NULL)) {
+ ret = zynqmp_r5_get_sram_banks(r5_core);
+ if (ret)
+ return ret;
+ }
}
return 0;
--
2.25.1
Hi Tanmay,
On Fri, May 10, 2024 at 05:51:25PM -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 <[email protected]>
> ---
>
> Changes in v2:
> - Fix following sparse warnings
>
> drivers/remoteproc/xlnx_r5_remoteproc.c:827:21: sparse: expected struct rsc_tbl_data *rsc_data_va
> drivers/remoteproc/xlnx_r5_remoteproc.c:844:18: sparse: expected struct resource_table *rsc_addr
> drivers/remoteproc/xlnx_r5_remoteproc.c:898:24: sparse: expected void volatile [noderef] __iomem *addr
>
> drivers/remoteproc/xlnx_r5_remoteproc.c | 164 +++++++++++++++++++++++-
> 1 file changed, 160 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/remoteproc/xlnx_r5_remoteproc.c b/drivers/remoteproc/xlnx_r5_remoteproc.c
> index 84243d1dff9f..039370cffa32 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,15 @@ struct mbox_info {
> struct mbox_chan *rx_chan;
> };
>
> +/* Xilinx Platform specific data structure */
> +struct rsc_tbl_data {
> + const int version;
> + const u32 magic_num;
> + const u32 comp_magic_num;
Why is a complement magic number needed?
> + 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 +108,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 {
> + struct resource_table *rsc_tbl_va;
Shouldn't this be of type "void __iomem *"? Did sparse give you trouble on that
one?
> 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 +638,19 @@ static int zynqmp_r5_rproc_prepare(struct rproc *rproc)
> {
> int ret;
>
> - ret = add_tcm_banks(rproc);
> - if (ret) {
> - dev_err(&rproc->dev, "failed to get TCM banks, err %d\n", ret);
> - return ret;
> + /**
Using "/**" is for comments that will endup in the documentation, which I don't
think is needed here. Please correct throughout the patch.
> + * 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(&rproc->dev, "failed to get TCM banks, err %d\n", ret);
> + return ret;
> + }
> }
>
> ret = add_mem_regions_carveout(rproc);
> @@ -662,6 +688,123 @@ static int zynqmp_r5_rproc_unprepare(struct rproc *rproc)
> return 0;
> }
>
> +static struct resource_table *zynqmp_r5_get_loaded_rsc_table(struct rproc *rproc,
> + size_t *size)
> +{
> + struct zynqmp_r5_core *r5_core;
> +
> + r5_core = rproc->priv;
> +
> + *size = r5_core->rsc_tbl_size;
> +
> + return r5_core->rsc_tbl_va;
> +}
> +
> +static int zynqmp_r5_get_rsc_table_va(struct zynqmp_r5_core *r5_core)
> +{
> + struct device *dev = r5_core->dev;
> + struct rsc_tbl_data *rsc_data_va;
> + struct resource_table *rsc_addr;
> + struct resource res_mem;
> + struct device_node *np;
> + int ret;
> +
> + /**
> + * It is expected from remote processor firmware to provide resource
> + * table address via struct rsc_tbl_data data structure.
> + * Start address of first entry under "memory-region" property list
> + * contains that data structure which holds resource table address, size
> + * and some magic number to validate correct resource table entry.
> + */
> + np = of_parse_phandle(r5_core->np, "memory-region", 0);
> + if (!np) {
> + dev_err(dev, "failed to get memory region dev node\n");
> + return -EINVAL;
> + }
> +
> + ret = of_address_to_resource(np, 0, &res_mem);
> + if (ret) {
> + dev_err(dev, "failed to get memory-region resource addr\n");
> + return -EINVAL;
> + }
> +
> + rsc_data_va = (struct rsc_tbl_data *)devm_ioremap_wc(dev, res_mem.start,
> + sizeof(struct rsc_tbl_data));
There is no point in holding memory until the driver is unloaded. Please use
ioremap_wc() and free at the end of the function.
> + if (!rsc_data_va) {
> + dev_err(dev, "failed to map resource table data address\n");
> + return -EIO;
> + }
> +
> + /**
> + * If RSC_TBL_XLNX_MAGIC number and its complement isn't found then
> + * do not consider resource table address valid and don't attach
> + */
> + if (rsc_data_va->magic_num != RSC_TBL_XLNX_MAGIC ||
> + rsc_data_va->comp_magic_num != ~RSC_TBL_XLNX_MAGIC) {
> + dev_dbg(dev, "invalid magic number, won't attach\n");
> + return -EINVAL;
> + }
> +
> + rsc_addr = (struct resource_table *)ioremap_wc(rsc_data_va->rsc_tbl,
> + rsc_data_va->rsc_tbl_size);
> + if (!rsc_addr) {
> + dev_err(dev, "failed to get rsc_addr\n");
> + return -EINVAL;
> + }
> +
> + /**
> + * As of now resource table version 1 is expected. Don't fail to attach
> + * but warn users about it.
> + */
> + if (rsc_addr->ver != 1)
> + dev_warn(dev, "unexpected resource table version %d\n",
> + rsc_addr->ver);
> +
> + r5_core->rsc_tbl_size = rsc_data_va->rsc_tbl_size;
> + r5_core->rsc_tbl_va = rsc_addr;
> +
> + return 0;
> +}
> +
> +static int zynqmp_r5_attach(struct rproc *rproc)
> +{
> + struct zynqmp_r5_core *r5_core = rproc->priv;
> + int i, pm_domain_id, ret;
> +
> + /*
> + * Firmware is loaded in TCM. Request TCM power domains to notify
> + * platform management controller that TCM is in use. This will be
> + * released during unprepare callback.
> + */
> + for (i = 0; i < r5_core->tcm_bank_count; i++) {
> + pm_domain_id = r5_core->tcm_banks[i]->pm_domain_id;
> + ret = zynqmp_pm_request_node(pm_domain_id,
> + ZYNQMP_PM_CAPABILITY_ACCESS, 0,
> + ZYNQMP_PM_REQUEST_ACK_BLOCKING);
> + if (ret < 0)
> + pr_warn("TCM %d can't be requested\n", i);
> + }
> +
> + return 0;
> +}
> +
> +static int zynqmp_r5_detach(struct rproc *rproc)
> +{
> + struct zynqmp_r5_core *r5_core = rproc->priv;
> +
> + /*
> + * Generate last notification to remote after clearing virtio flag.
> + * Remote can avoid polling on virtio reset flag if kick is generated
> + * during detach by host and check virtio reset flag on kick interrupt.
> + */
> + zynqmp_r5_rproc_kick(rproc, 0);
> +
> + iounmap((void __iomem *)r5_core->rsc_tbl_va);
> + r5_core->rsc_tbl_va = NULL;
This is puzzling... What happens to ->tsc_tbl_va when the remote processor is
re-attached?
I will not look at the SRAM part. Please re-submit when we are done with the
attach/detach feature.
Thanks,
Mathieu
> +
> + return 0;
> +}
> +
> static const struct rproc_ops zynqmp_r5_rproc_ops = {
> .prepare = zynqmp_r5_rproc_prepare,
> .unprepare = zynqmp_r5_rproc_unprepare,
> @@ -673,6 +816,9 @@ static const struct rproc_ops zynqmp_r5_rproc_ops = {
> .sanity_check = rproc_elf_sanity_check,
> .get_boot_addr = rproc_elf_get_boot_addr,
> .kick = zynqmp_r5_rproc_kick,
> + .get_loaded_rsc_table = zynqmp_r5_get_loaded_rsc_table,
> + .attach = zynqmp_r5_attach,
> + .detach = zynqmp_r5_detach,
> };
>
> /**
> @@ -723,6 +869,16 @@ static struct zynqmp_r5_core *zynqmp_r5_add_rproc_core(struct device *cdev)
> goto free_rproc;
> }
>
> + /*
> + * Move rproc state to DETACHED to give one time opportunity to attach
> + * if firmware is already available in the memory. This can happen if
> + * firmware is loaded via debugger or by any other agent in the system.
> + * If firmware isn't available in the memory and resource table isn't found,
> + * then rproc state stay OFFLINE.
> + */
> + if (!zynqmp_r5_get_rsc_table_va(r5_core))
> + r5_rproc->state = RPROC_DETACHED;
> +
> r5_core->rproc = r5_rproc;
> return r5_core;
>
> --
> 2.25.1
>
On 5/21/24 12:56 PM, Mathieu Poirier wrote:
> Hi Tanmay,
>
> On Fri, May 10, 2024 at 05:51:25PM -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 <[email protected]>
>> ---
>>
>> Changes in v2:
>> - Fix following sparse warnings
>>
>> drivers/remoteproc/xlnx_r5_remoteproc.c:827:21: sparse: expected struct rsc_tbl_data *rsc_data_va
>> drivers/remoteproc/xlnx_r5_remoteproc.c:844:18: sparse: expected struct resource_table *rsc_addr
>> drivers/remoteproc/xlnx_r5_remoteproc.c:898:24: sparse: expected void volatile [noderef] __iomem *addr
>>
>> drivers/remoteproc/xlnx_r5_remoteproc.c | 164 +++++++++++++++++++++++-
>> 1 file changed, 160 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/remoteproc/xlnx_r5_remoteproc.c b/drivers/remoteproc/xlnx_r5_remoteproc.c
>> index 84243d1dff9f..039370cffa32 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,15 @@ struct mbox_info {
>> struct mbox_chan *rx_chan;
>> };
>>
>> +/* Xilinx Platform specific data structure */
>> +struct rsc_tbl_data {
>> + const int version;
>> + const u32 magic_num;
>> + const u32 comp_magic_num;
>
> Why is a complement magic number needed?
Actually magic number is 64-bit. There is good chance that
firmware can have 32-bit op-code or data same as magic number, but very less
chance of its complement in the next address. So, we can assume magic number
is 64-bit.
>
>> + 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 +108,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 {
>> + struct resource_table *rsc_tbl_va;
>
> Shouldn't this be of type "void __iomem *"? Did sparse give you trouble on that
> one?
I fixed sparse warnings with typecast below [1].
>
>> 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 +638,19 @@ static int zynqmp_r5_rproc_prepare(struct rproc *rproc)
>> {
>> int ret;
>>
>> - ret = add_tcm_banks(rproc);
>> - if (ret) {
>> - dev_err(&rproc->dev, "failed to get TCM banks, err %d\n", ret);
>> - return ret;
>> + /**
>
> Using "/**" is for comments that will endup in the documentation, which I don't
> think is needed here. Please correct throughout the patch.
Thanks. Ack, I will use only /* format.
>
>> + * 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(&rproc->dev, "failed to get TCM banks, err %d\n", ret);
>> + return ret;
>> + }
>> }
>>
>> ret = add_mem_regions_carveout(rproc);
>> @@ -662,6 +688,123 @@ static int zynqmp_r5_rproc_unprepare(struct rproc *rproc)
>> return 0;
>> }
>>
>> +static struct resource_table *zynqmp_r5_get_loaded_rsc_table(struct rproc *rproc,
>> + size_t *size)
>> +{
>> + struct zynqmp_r5_core *r5_core;
>> +
>> + r5_core = rproc->priv;
>> +
>> + *size = r5_core->rsc_tbl_size;
>> +
>> + return r5_core->rsc_tbl_va;
>> +}
>> +
>> +static int zynqmp_r5_get_rsc_table_va(struct zynqmp_r5_core *r5_core)
>> +{
>> + struct device *dev = r5_core->dev;
>> + struct rsc_tbl_data *rsc_data_va;
>> + struct resource_table *rsc_addr;
>> + struct resource res_mem;
>> + struct device_node *np;
>> + int ret;
>> +
>> + /**
>> + * It is expected from remote processor firmware to provide resource
>> + * table address via struct rsc_tbl_data data structure.
>> + * Start address of first entry under "memory-region" property list
>> + * contains that data structure which holds resource table address, size
>> + * and some magic number to validate correct resource table entry.
>> + */
>> + np = of_parse_phandle(r5_core->np, "memory-region", 0);
>> + if (!np) {
>> + dev_err(dev, "failed to get memory region dev node\n");
>> + return -EINVAL;
>> + }
>> +
>> + ret = of_address_to_resource(np, 0, &res_mem);
>> + if (ret) {
>> + dev_err(dev, "failed to get memory-region resource addr\n");
>> + return -EINVAL;
>> + }
>> +
>> + rsc_data_va = (struct rsc_tbl_data *)devm_ioremap_wc(dev, res_mem.start,
>> + sizeof(struct rsc_tbl_data));
>
> There is no point in holding memory until the driver is unloaded. Please use
> ioremap_wc() and free at the end of the function.
>
Ack.
>> + if (!rsc_data_va) {
>> + dev_err(dev, "failed to map resource table data address\n");
>> + return -EIO;
>> + }
>> +
>> + /**
>> + * If RSC_TBL_XLNX_MAGIC number and its complement isn't found then
>> + * do not consider resource table address valid and don't attach
>> + */
>> + if (rsc_data_va->magic_num != RSC_TBL_XLNX_MAGIC ||
>> + rsc_data_va->comp_magic_num != ~RSC_TBL_XLNX_MAGIC) {
>> + dev_dbg(dev, "invalid magic number, won't attach\n");
>> + return -EINVAL;
>> + }
>> +
>> + rsc_addr = (struct resource_table *)ioremap_wc(rsc_data_va->rsc_tbl,
>> + rsc_data_va->rsc_tbl_size);
[1] Here typecast is done.
>> + if (!rsc_addr) {
>> + dev_err(dev, "failed to get rsc_addr\n");
>> + return -EINVAL;
>> + }
>> +
>> + /**
>> + * As of now resource table version 1 is expected. Don't fail to attach
>> + * but warn users about it.
>> + */
>> + if (rsc_addr->ver != 1)
>> + dev_warn(dev, "unexpected resource table version %d\n",
>> + rsc_addr->ver);
>> +
>> + r5_core->rsc_tbl_size = rsc_data_va->rsc_tbl_size;
>> + r5_core->rsc_tbl_va = rsc_addr;
>> +
>> + return 0;
>> +}
>> +
>> +static int zynqmp_r5_attach(struct rproc *rproc)
>> +{
>> + struct zynqmp_r5_core *r5_core = rproc->priv;
>> + int i, pm_domain_id, ret;
>> +
>> + /*
>> + * Firmware is loaded in TCM. Request TCM power domains to notify
>> + * platform management controller that TCM is in use. This will be
>> + * released during unprepare callback.
>> + */
>> + for (i = 0; i < r5_core->tcm_bank_count; i++) {
>> + pm_domain_id = r5_core->tcm_banks[i]->pm_domain_id;
>> + ret = zynqmp_pm_request_node(pm_domain_id,
>> + ZYNQMP_PM_CAPABILITY_ACCESS, 0,
>> + ZYNQMP_PM_REQUEST_ACK_BLOCKING);
>> + if (ret < 0)
>> + pr_warn("TCM %d can't be requested\n", i);
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static int zynqmp_r5_detach(struct rproc *rproc)
>> +{
>> + struct zynqmp_r5_core *r5_core = rproc->priv;
>> +
>> + /*
>> + * Generate last notification to remote after clearing virtio flag.
>> + * Remote can avoid polling on virtio reset flag if kick is generated
>> + * during detach by host and check virtio reset flag on kick interrupt.
>> + */
>> + zynqmp_r5_rproc_kick(rproc, 0);
>> +
>> + iounmap((void __iomem *)r5_core->rsc_tbl_va);
>> + r5_core->rsc_tbl_va = NULL;
>
> This is puzzling... What happens to ->tsc_tbl_va when the remote processor is
> re-attached?
Actually I don't see re-attach in life cycle. I might be missing something.
Following is lifecycle I have tested:
1) During driver probe, if resource table is found in memory, then state is
moved to detach.
2) Then user executes echo start > remoteproc* command, and state moved to attach.
3) After work is done with remote, user executes echo stop > remoteproc* command,
and state is moved to offline.
From here, remote is offline state, and I can't re-attach to it without loading
firmware again. which is regular start/stop states. Please let me know if I am missing
something.
From here, load firmware, and executing echo start > remoteproc* moves
rproc state to running. Load firmware loads resource table from elf.
So, I believe attach is happening only during probe. And then, once r5 stops, user
needs to load firmware and start R5. I think this use case is good for now.
>
> I will not look at the SRAM part. Please re-submit when we are done with the
> attach/detach feature.
>
Okay sounds good to me.
Reviews are still welcomed if anyone in the community decides to review it.
Thanks,
Tanmay
> Thanks,
> Mathieu
>
>> +
>> + return 0;
>> +}
>> +
>> static const struct rproc_ops zynqmp_r5_rproc_ops = {
>> .prepare = zynqmp_r5_rproc_prepare,
>> .unprepare = zynqmp_r5_rproc_unprepare,
>> @@ -673,6 +816,9 @@ static const struct rproc_ops zynqmp_r5_rproc_ops = {
>> .sanity_check = rproc_elf_sanity_check,
>> .get_boot_addr = rproc_elf_get_boot_addr,
>> .kick = zynqmp_r5_rproc_kick,
>> + .get_loaded_rsc_table = zynqmp_r5_get_loaded_rsc_table,
>> + .attach = zynqmp_r5_attach,
>> + .detach = zynqmp_r5_detach,
>> };
>>
>> /**
>> @@ -723,6 +869,16 @@ static struct zynqmp_r5_core *zynqmp_r5_add_rproc_core(struct device *cdev)
>> goto free_rproc;
>> }
>>
>> + /*
>> + * Move rproc state to DETACHED to give one time opportunity to attach
>> + * if firmware is already available in the memory. This can happen if
>> + * firmware is loaded via debugger or by any other agent in the system.
>> + * If firmware isn't available in the memory and resource table isn't found,
>> + * then rproc state stay OFFLINE.
>> + */
>> + if (!zynqmp_r5_get_rsc_table_va(r5_core))
>> + r5_rproc->state = RPROC_DETACHED;
>> +
>> r5_core->rproc = r5_rproc;
>> return r5_core;
>>
>> --
>> 2.25.1
>>
On Wed, May 22, 2024 at 09:36:26AM -0500, Tanmay Shah wrote:
>
>
> On 5/21/24 12:56 PM, Mathieu Poirier wrote:
> > Hi Tanmay,
> >
> > On Fri, May 10, 2024 at 05:51:25PM -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 <[email protected]>
> >> ---
> >>
> >> Changes in v2:
> >> - Fix following sparse warnings
> >>
> >> drivers/remoteproc/xlnx_r5_remoteproc.c:827:21: sparse: expected struct rsc_tbl_data *rsc_data_va
> >> drivers/remoteproc/xlnx_r5_remoteproc.c:844:18: sparse: expected struct resource_table *rsc_addr
> >> drivers/remoteproc/xlnx_r5_remoteproc.c:898:24: sparse: expected void volatile [noderef] __iomem *addr
> >>
> >> drivers/remoteproc/xlnx_r5_remoteproc.c | 164 +++++++++++++++++++++++-
> >> 1 file changed, 160 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/drivers/remoteproc/xlnx_r5_remoteproc.c b/drivers/remoteproc/xlnx_r5_remoteproc.c
> >> index 84243d1dff9f..039370cffa32 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,15 @@ struct mbox_info {
> >> struct mbox_chan *rx_chan;
> >> };
> >>
> >> +/* Xilinx Platform specific data structure */
> >> +struct rsc_tbl_data {
> >> + const int version;
> >> + const u32 magic_num;
> >> + const u32 comp_magic_num;
> >
> > Why is a complement magic number needed?
>
> Actually magic number is 64-bit. There is good chance that
> firmware can have 32-bit op-code or data same as magic number, but very less
> chance of its complement in the next address. So, we can assume magic number
> is 64-bit.
>
So why not having a magic number that is a u64?
> >
> >> + 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 +108,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 {
> >> + struct resource_table *rsc_tbl_va;
> >
> > Shouldn't this be of type "void __iomem *"? Did sparse give you trouble on that
> > one?
>
> I fixed sparse warnings with typecast below [1].
>
My point is, ioremap_wc() returns a "void__iomem *" so why not using that
instead of a "struct resource_table *"?
> >
> >> 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 +638,19 @@ static int zynqmp_r5_rproc_prepare(struct rproc *rproc)
> >> {
> >> int ret;
> >>
> >> - ret = add_tcm_banks(rproc);
> >> - if (ret) {
> >> - dev_err(&rproc->dev, "failed to get TCM banks, err %d\n", ret);
> >> - return ret;
> >> + /**
> >
> > Using "/**" is for comments that will endup in the documentation, which I don't
> > think is needed here. Please correct throughout the patch.
>
> Thanks. Ack, I will use only /* format.
>
> >
> >> + * 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(&rproc->dev, "failed to get TCM banks, err %d\n", ret);
> >> + return ret;
> >> + }
> >> }
> >>
> >> ret = add_mem_regions_carveout(rproc);
> >> @@ -662,6 +688,123 @@ static int zynqmp_r5_rproc_unprepare(struct rproc *rproc)
> >> return 0;
> >> }
> >>
> >> +static struct resource_table *zynqmp_r5_get_loaded_rsc_table(struct rproc *rproc,
> >> + size_t *size)
> >> +{
> >> + struct zynqmp_r5_core *r5_core;
> >> +
> >> + r5_core = rproc->priv;
> >> +
> >> + *size = r5_core->rsc_tbl_size;
> >> +
> >> + return r5_core->rsc_tbl_va;
> >> +}
> >> +
> >> +static int zynqmp_r5_get_rsc_table_va(struct zynqmp_r5_core *r5_core)
> >> +{
> >> + struct device *dev = r5_core->dev;
> >> + struct rsc_tbl_data *rsc_data_va;
> >> + struct resource_table *rsc_addr;
> >> + struct resource res_mem;
> >> + struct device_node *np;
> >> + int ret;
> >> +
> >> + /**
> >> + * It is expected from remote processor firmware to provide resource
> >> + * table address via struct rsc_tbl_data data structure.
> >> + * Start address of first entry under "memory-region" property list
> >> + * contains that data structure which holds resource table address, size
> >> + * and some magic number to validate correct resource table entry.
> >> + */
> >> + np = of_parse_phandle(r5_core->np, "memory-region", 0);
> >> + if (!np) {
> >> + dev_err(dev, "failed to get memory region dev node\n");
> >> + return -EINVAL;
> >> + }
> >> +
> >> + ret = of_address_to_resource(np, 0, &res_mem);
> >> + if (ret) {
> >> + dev_err(dev, "failed to get memory-region resource addr\n");
> >> + return -EINVAL;
> >> + }
> >> +
> >> + rsc_data_va = (struct rsc_tbl_data *)devm_ioremap_wc(dev, res_mem.start,
> >> + sizeof(struct rsc_tbl_data));
> >
> > There is no point in holding memory until the driver is unloaded. Please use
> > ioremap_wc() and free at the end of the function.
> >
>
> Ack.
>
> >> + if (!rsc_data_va) {
> >> + dev_err(dev, "failed to map resource table data address\n");
> >> + return -EIO;
> >> + }
> >> +
> >> + /**
> >> + * If RSC_TBL_XLNX_MAGIC number and its complement isn't found then
> >> + * do not consider resource table address valid and don't attach
> >> + */
> >> + if (rsc_data_va->magic_num != RSC_TBL_XLNX_MAGIC ||
> >> + rsc_data_va->comp_magic_num != ~RSC_TBL_XLNX_MAGIC) {
> >> + dev_dbg(dev, "invalid magic number, won't attach\n");
> >> + return -EINVAL;
> >> + }
> >> +
> >> + rsc_addr = (struct resource_table *)ioremap_wc(rsc_data_va->rsc_tbl,
> >> + rsc_data_va->rsc_tbl_size);
>
> [1] Here typecast is done.
>
> >> + if (!rsc_addr) {
> >> + dev_err(dev, "failed to get rsc_addr\n");
> >> + return -EINVAL;
> >> + }
> >> +
> >> + /**
> >> + * As of now resource table version 1 is expected. Don't fail to attach
> >> + * but warn users about it.
> >> + */
> >> + if (rsc_addr->ver != 1)
> >> + dev_warn(dev, "unexpected resource table version %d\n",
> >> + rsc_addr->ver);
> >> +
> >> + r5_core->rsc_tbl_size = rsc_data_va->rsc_tbl_size;
> >> + r5_core->rsc_tbl_va = rsc_addr;
> >> +
> >> + return 0;
> >> +}
> >> +
> >> +static int zynqmp_r5_attach(struct rproc *rproc)
> >> +{
> >> + struct zynqmp_r5_core *r5_core = rproc->priv;
> >> + int i, pm_domain_id, ret;
> >> +
> >> + /*
> >> + * Firmware is loaded in TCM. Request TCM power domains to notify
> >> + * platform management controller that TCM is in use. This will be
> >> + * released during unprepare callback.
> >> + */
> >> + for (i = 0; i < r5_core->tcm_bank_count; i++) {
> >> + pm_domain_id = r5_core->tcm_banks[i]->pm_domain_id;
> >> + ret = zynqmp_pm_request_node(pm_domain_id,
> >> + ZYNQMP_PM_CAPABILITY_ACCESS, 0,
> >> + ZYNQMP_PM_REQUEST_ACK_BLOCKING);
> >> + if (ret < 0)
> >> + pr_warn("TCM %d can't be requested\n", i);
> >> + }
> >> +
> >> + return 0;
> >> +}
> >> +
> >> +static int zynqmp_r5_detach(struct rproc *rproc)
> >> +{
> >> + struct zynqmp_r5_core *r5_core = rproc->priv;
> >> +
> >> + /*
> >> + * Generate last notification to remote after clearing virtio flag.
> >> + * Remote can avoid polling on virtio reset flag if kick is generated
> >> + * during detach by host and check virtio reset flag on kick interrupt.
> >> + */
> >> + zynqmp_r5_rproc_kick(rproc, 0);
> >> +
> >> + iounmap((void __iomem *)r5_core->rsc_tbl_va);
> >> + r5_core->rsc_tbl_va = NULL;
> >
> > This is puzzling... What happens to ->tsc_tbl_va when the remote processor is
> > re-attached?
>
> Actually I don't see re-attach in life cycle. I might be missing something.
> Following is lifecycle I have tested:
>
> 1) During driver probe, if resource table is found in memory, then state is
> moved to detach.
Right.
> 2) Then user executes echo start > remoteproc* command, and state moved to attach.
Right.
> 3) After work is done with remote, user executes echo stop > remoteproc* command,
> and state is moved to offline.
>
Right. But you have an ops::detach() function, which means you expect users to
be able to detach() and re-attach() as many times as they want.
> From here, remote is offline state, and I can't re-attach to it without loading
> firmware again. which is regular start/stop states. Please let me know if I am missing
> something.
>
> From here, load firmware, and executing echo start > remoteproc* moves
> rproc state to running. Load firmware loads resource table from elf.
>
> So, I believe attach is happening only during probe. And then, once r5 stops, user
> needs to load firmware and start R5. I think this use case is good for now.
>
If you don't want people to detach() and re-attach(), remove ops::detach()
entirely. But if you go this way it is only a matter of time before
someone asks for the feature or provide a fix for it.
> >
> > I will not look at the SRAM part. Please re-submit when we are done with the
> > attach/detach feature.
> >
>
> Okay sounds good to me.
> Reviews are still welcomed if anyone in the community decides to review it.
>
> Thanks,
> Tanmay
> > Thanks,
> > Mathieu
> >
> >> +
> >> + return 0;
> >> +}
> >> +
> >> static const struct rproc_ops zynqmp_r5_rproc_ops = {
> >> .prepare = zynqmp_r5_rproc_prepare,
> >> .unprepare = zynqmp_r5_rproc_unprepare,
> >> @@ -673,6 +816,9 @@ static const struct rproc_ops zynqmp_r5_rproc_ops = {
> >> .sanity_check = rproc_elf_sanity_check,
> >> .get_boot_addr = rproc_elf_get_boot_addr,
> >> .kick = zynqmp_r5_rproc_kick,
> >> + .get_loaded_rsc_table = zynqmp_r5_get_loaded_rsc_table,
> >> + .attach = zynqmp_r5_attach,
> >> + .detach = zynqmp_r5_detach,
> >> };
> >>
> >> /**
> >> @@ -723,6 +869,16 @@ static struct zynqmp_r5_core *zynqmp_r5_add_rproc_core(struct device *cdev)
> >> goto free_rproc;
> >> }
> >>
> >> + /*
> >> + * Move rproc state to DETACHED to give one time opportunity to attach
> >> + * if firmware is already available in the memory. This can happen if
> >> + * firmware is loaded via debugger or by any other agent in the system.
> >> + * If firmware isn't available in the memory and resource table isn't found,
> >> + * then rproc state stay OFFLINE.
> >> + */
> >> + if (!zynqmp_r5_get_rsc_table_va(r5_core))
> >> + r5_rproc->state = RPROC_DETACHED;
> >> +
> >> r5_core->rproc = r5_rproc;
> >> return r5_core;
> >>
> >> --
> >> 2.25.1
> >>
>
On 5/23/24 12:05 PM, Mathieu Poirier wrote:
> On Wed, May 22, 2024 at 09:36:26AM -0500, Tanmay Shah wrote:
>>
>>
>> On 5/21/24 12:56 PM, Mathieu Poirier wrote:
>> > Hi Tanmay,
>> >
>> > On Fri, May 10, 2024 at 05:51:25PM -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 <[email protected]>
>> >> ---
>> >>
>> >> Changes in v2:
>> >> - Fix following sparse warnings
>> >>
>> >> drivers/remoteproc/xlnx_r5_remoteproc.c:827:21: sparse: expected struct rsc_tbl_data *rsc_data_va
>> >> drivers/remoteproc/xlnx_r5_remoteproc.c:844:18: sparse: expected struct resource_table *rsc_addr
>> >> drivers/remoteproc/xlnx_r5_remoteproc.c:898:24: sparse: expected void volatile [noderef] __iomem *addr
>> >>
>> >> drivers/remoteproc/xlnx_r5_remoteproc.c | 164 +++++++++++++++++++++++-
>> >> 1 file changed, 160 insertions(+), 4 deletions(-)
>> >>
>> >> diff --git a/drivers/remoteproc/xlnx_r5_remoteproc.c b/drivers/remoteproc/xlnx_r5_remoteproc.c
>> >> index 84243d1dff9f..039370cffa32 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,15 @@ struct mbox_info {
>> >> struct mbox_chan *rx_chan;
>> >> };
>> >>
>> >> +/* Xilinx Platform specific data structure */
>> >> +struct rsc_tbl_data {
>> >> + const int version;
>> >> + const u32 magic_num;
>> >> + const u32 comp_magic_num;
>> >
>> > Why is a complement magic number needed?
>>
>> Actually magic number is 64-bit. There is good chance that
>> firmware can have 32-bit op-code or data same as magic number, but very less
>> chance of its complement in the next address. So, we can assume magic number
>> is 64-bit.
>>
>
> So why not having a magic number that is a u64?
>
>> >
>> >> + 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 +108,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 {
>> >> + struct resource_table *rsc_tbl_va;
>> >
>> > Shouldn't this be of type "void __iomem *"? Did sparse give you trouble on that
>> > one?
>>
>> I fixed sparse warnings with typecast below [1].
>>
>
> My point is, ioremap_wc() returns a "void__iomem *" so why not using that
> instead of a "struct resource_table *"?
Ack.
>
>
>> >
>> >> 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 +638,19 @@ static int zynqmp_r5_rproc_prepare(struct rproc *rproc)
>> >> {
>> >> int ret;
>> >>
>> >> - ret = add_tcm_banks(rproc);
>> >> - if (ret) {
>> >> - dev_err(&rproc->dev, "failed to get TCM banks, err %d\n", ret);
>> >> - return ret;
>> >> + /**
>> >
>> > Using "/**" is for comments that will endup in the documentation, which I don't
>> > think is needed here. Please correct throughout the patch.
>>
>> Thanks. Ack, I will use only /* format.
>>
>> >
>> >> + * 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(&rproc->dev, "failed to get TCM banks, err %d\n", ret);
>> >> + return ret;
>> >> + }
>> >> }
>> >>
>> >> ret = add_mem_regions_carveout(rproc);
>> >> @@ -662,6 +688,123 @@ static int zynqmp_r5_rproc_unprepare(struct rproc *rproc)
>> >> return 0;
>> >> }
>> >>
>> >> +static struct resource_table *zynqmp_r5_get_loaded_rsc_table(struct rproc *rproc,
>> >> + size_t *size)
>> >> +{
>> >> + struct zynqmp_r5_core *r5_core;
>> >> +
>> >> + r5_core = rproc->priv;
>> >> +
>> >> + *size = r5_core->rsc_tbl_size;
>> >> +
>> >> + return r5_core->rsc_tbl_va;
>> >> +}
>> >> +
>> >> +static int zynqmp_r5_get_rsc_table_va(struct zynqmp_r5_core *r5_core)
>> >> +{
>> >> + struct device *dev = r5_core->dev;
>> >> + struct rsc_tbl_data *rsc_data_va;
>> >> + struct resource_table *rsc_addr;
>> >> + struct resource res_mem;
>> >> + struct device_node *np;
>> >> + int ret;
>> >> +
>> >> + /**
>> >> + * It is expected from remote processor firmware to provide resource
>> >> + * table address via struct rsc_tbl_data data structure.
>> >> + * Start address of first entry under "memory-region" property list
>> >> + * contains that data structure which holds resource table address, size
>> >> + * and some magic number to validate correct resource table entry.
>> >> + */
>> >> + np = of_parse_phandle(r5_core->np, "memory-region", 0);
>> >> + if (!np) {
>> >> + dev_err(dev, "failed to get memory region dev node\n");
>> >> + return -EINVAL;
>> >> + }
>> >> +
>> >> + ret = of_address_to_resource(np, 0, &res_mem);
>> >> + if (ret) {
>> >> + dev_err(dev, "failed to get memory-region resource addr\n");
>> >> + return -EINVAL;
>> >> + }
>> >> +
>> >> + rsc_data_va = (struct rsc_tbl_data *)devm_ioremap_wc(dev, res_mem.start,
>> >> + sizeof(struct rsc_tbl_data));
>> >
>> > There is no point in holding memory until the driver is unloaded. Please use
>> > ioremap_wc() and free at the end of the function.
>> >
>>
>> Ack.
>>
>> >> + if (!rsc_data_va) {
>> >> + dev_err(dev, "failed to map resource table data address\n");
>> >> + return -EIO;
>> >> + }
>> >> +
>> >> + /**
>> >> + * If RSC_TBL_XLNX_MAGIC number and its complement isn't found then
>> >> + * do not consider resource table address valid and don't attach
>> >> + */
>> >> + if (rsc_data_va->magic_num != RSC_TBL_XLNX_MAGIC ||
>> >> + rsc_data_va->comp_magic_num != ~RSC_TBL_XLNX_MAGIC) {
>> >> + dev_dbg(dev, "invalid magic number, won't attach\n");
>> >> + return -EINVAL;
>> >> + }
>> >> +
>> >> + rsc_addr = (struct resource_table *)ioremap_wc(rsc_data_va->rsc_tbl,
>> >> + rsc_data_va->rsc_tbl_size);
>>
>> [1] Here typecast is done.
>>
>> >> + if (!rsc_addr) {
>> >> + dev_err(dev, "failed to get rsc_addr\n");
>> >> + return -EINVAL;
>> >> + }
>> >> +
>> >> + /**
>> >> + * As of now resource table version 1 is expected. Don't fail to attach
>> >> + * but warn users about it.
>> >> + */
>> >> + if (rsc_addr->ver != 1)
>> >> + dev_warn(dev, "unexpected resource table version %d\n",
>> >> + rsc_addr->ver);
>> >> +
>> >> + r5_core->rsc_tbl_size = rsc_data_va->rsc_tbl_size;
>> >> + r5_core->rsc_tbl_va = rsc_addr;
>> >> +
>> >> + return 0;
>> >> +}
>> >> +
>> >> +static int zynqmp_r5_attach(struct rproc *rproc)
>> >> +{
>> >> + struct zynqmp_r5_core *r5_core = rproc->priv;
>> >> + int i, pm_domain_id, ret;
>> >> +
>> >> + /*
>> >> + * Firmware is loaded in TCM. Request TCM power domains to notify
>> >> + * platform management controller that TCM is in use. This will be
>> >> + * released during unprepare callback.
>> >> + */
>> >> + for (i = 0; i < r5_core->tcm_bank_count; i++) {
>> >> + pm_domain_id = r5_core->tcm_banks[i]->pm_domain_id;
>> >> + ret = zynqmp_pm_request_node(pm_domain_id,
>> >> + ZYNQMP_PM_CAPABILITY_ACCESS, 0,
>> >> + ZYNQMP_PM_REQUEST_ACK_BLOCKING);
>> >> + if (ret < 0)
>> >> + pr_warn("TCM %d can't be requested\n", i);
>> >> + }
>> >> +
>> >> + return 0;
>> >> +}
>> >> +
>> >> +static int zynqmp_r5_detach(struct rproc *rproc)
>> >> +{
>> >> + struct zynqmp_r5_core *r5_core = rproc->priv;
>> >> +
>> >> + /*
>> >> + * Generate last notification to remote after clearing virtio flag.
>> >> + * Remote can avoid polling on virtio reset flag if kick is generated
>> >> + * during detach by host and check virtio reset flag on kick interrupt.
>> >> + */
>> >> + zynqmp_r5_rproc_kick(rproc, 0);
>> >> +
>> >> + iounmap((void __iomem *)r5_core->rsc_tbl_va);
>> >> + r5_core->rsc_tbl_va = NULL;
>> >
>> > This is puzzling... What happens to ->tsc_tbl_va when the remote processor is
>> > re-attached?
>>
>> Actually I don't see re-attach in life cycle. I might be missing something.
>> Following is lifecycle I have tested:
>>
>> 1) During driver probe, if resource table is found in memory, then state is
>> moved to detach.
>
> Right.
>
>> 2) Then user executes echo start > remoteproc* command, and state moved to attach.
>
> Right.
>
>> 3) After work is done with remote, user executes echo stop > remoteproc* command,
>> and state is moved to offline.
>>
>
> Right. But you have an ops::detach() function, which means you expect users to
> be able to detach() and re-attach() as many times as they want.
>
>> From here, remote is offline state, and I can't re-attach to it without loading
>> firmware again. which is regular start/stop states. Please let me know if I am missing
>> something.
>>
>> From here, load firmware, and executing echo start > remoteproc* moves
>> rproc state to running. Load firmware loads resource table from elf.
>>
>> So, I believe attach is happening only during probe. And then, once r5 stops, user
>> needs to load firmware and start R5. I think this use case is good for now.
>>
>
> If you don't want people to detach() and re-attach(), remove ops::detach()
> entirely. But if you go this way it is only a matter of time before
> someone asks for the feature or provide a fix for it.
>
Does that mean implement whatever is in detach, in ops::stop() ?
I am okay with that. Current use case is expected to attach only during boot time.
>> >
>> > I will not look at the SRAM part. Please re-submit when we are done with the
>> > attach/detach feature.
>> >
>>
>> Okay sounds good to me.
>> Reviews are still welcomed if anyone in the community decides to review it.
>>
>> Thanks,
>> Tanmay
>> > Thanks,
>> > Mathieu
>> >
>> >> +
>> >> + return 0;
>> >> +}
>> >> +
>> >> static const struct rproc_ops zynqmp_r5_rproc_ops = {
>> >> .prepare = zynqmp_r5_rproc_prepare,
>> >> .unprepare = zynqmp_r5_rproc_unprepare,
>> >> @@ -673,6 +816,9 @@ static const struct rproc_ops zynqmp_r5_rproc_ops = {
>> >> .sanity_check = rproc_elf_sanity_check,
>> >> .get_boot_addr = rproc_elf_get_boot_addr,
>> >> .kick = zynqmp_r5_rproc_kick,
>> >> + .get_loaded_rsc_table = zynqmp_r5_get_loaded_rsc_table,
>> >> + .attach = zynqmp_r5_attach,
>> >> + .detach = zynqmp_r5_detach,
>> >> };
>> >>
>> >> /**
>> >> @@ -723,6 +869,16 @@ static struct zynqmp_r5_core *zynqmp_r5_add_rproc_core(struct device *cdev)
>> >> goto free_rproc;
>> >> }
>> >>
>> >> + /*
>> >> + * Move rproc state to DETACHED to give one time opportunity to attach
>> >> + * if firmware is already available in the memory. This can happen if
>> >> + * firmware is loaded via debugger or by any other agent in the system.
>> >> + * If firmware isn't available in the memory and resource table isn't found,
>> >> + * then rproc state stay OFFLINE.
>> >> + */
>> >> + if (!zynqmp_r5_get_rsc_table_va(r5_core))
>> >> + r5_rproc->state = RPROC_DETACHED;
>> >> +
>> >> r5_core->rproc = r5_rproc;
>> >> return r5_core;
>> >>
>> >> --
>> >> 2.25.1
>> >>
>>