[PATCH v4 3/3] remoteproc: zynqmp: get TCM from device-tree
Mathieu Poirier
mathieu.poirier at linaro.org
Thu Sep 7 11:08:44 PDT 2023
On Wed, Sep 06, 2023 at 05:02:40PM -0500, Tanmay Shah wrote:
> HI Mathieu,
>
> Thanks for reviews. Please find my comments below.
>
I took another look after reading your comment and found more problems...
>
> On 9/6/23 2:47 PM, Mathieu Poirier wrote:
> > Hi Tanmay,
> >
> > On Tue, Aug 29, 2023 at 11:19:00AM -0700, Tanmay Shah wrote:
> > > Use new dt bindings to get TCM address and size
> > > information. Also make sure that driver stays
> > > compatible with previous device-tree bindings.
> > > So, if TCM information isn't available in device-tree
> > > for zynqmp platform, hard-coded address of TCM will
> > > be used.
> > >
> > > New platforms that are compatible with this
> > > driver must add TCM support in device-tree as per new
> > > bindings.
> > >
> > > Signed-off-by: Tanmay Shah <tanmay.shah at amd.com>
> > > ---
> > > drivers/remoteproc/xlnx_r5_remoteproc.c | 279 +++++++++++++++++++-----
> > > 1 file changed, 221 insertions(+), 58 deletions(-)
> > >
> > > diff --git a/drivers/remoteproc/xlnx_r5_remoteproc.c b/drivers/remoteproc/xlnx_r5_remoteproc.c
> > > index feca6de68da2..4eb62eb545c2 100644
> > > --- a/drivers/remoteproc/xlnx_r5_remoteproc.c
> > > +++ b/drivers/remoteproc/xlnx_r5_remoteproc.c
> > > @@ -39,15 +39,19 @@ enum zynqmp_r5_cluster_mode {
> > > * struct mem_bank_data - Memory Bank description
> > > *
> > > * @addr: Start address of memory bank
> > > + * @da: device address for this tcm bank
> > > * @size: Size of Memory bank
> > > * @pm_domain_id: Power-domains id of memory bank for firmware to turn on/off
> > > + * @pm_domain_id2: second core's corresponding TCM's pm_domain_id
> > > * @bank_name: name of the bank for remoteproc framework
> > > */
> > > struct mem_bank_data {
> > > - phys_addr_t addr;
> > > - size_t size;
> > > + u32 addr;
> > > + u32 da;
> > > + u32 size;
> >
> > Why are the types of @addr and @size changed?
>
> So, R5 can access 32-bit address range only. Before I had missed this.
>
> In Devce-tree bindings I am keeping address-cells and size-cells as 2.
>
> So, out of 64-bits only 32-bits will be used to get address of TCM. Same for size.
>
> This motivated me to change the type of @addr and @size fields. It doesn't have any side effects.
It doesn't have an effect but it also doesn't need to be in this patch,
especially since it is not documented.
This patch needs to be broken in 3 parts:
1) One patch that deals with the addition of the static mem_bank_data for
lockstep mode.
2) One patch that deals with the addition of ->pm_domain_id2 and the potential
bug I may have highlighted below.
3) One patch that deals with extracting the TCM information from the DT.
Everything else needs to be in another patch.
>
>
> >
> > > u32 pm_domain_id;
> > > - char *bank_name;
> > > + u32 pm_domain_id2;
> > > + char bank_name[32];
> >
> > Same
>
> Now we have "reg-names" property in dts so, when that is available, I try to use it.
>
> So, instead of keeping simple pointer, I copy name from "struct resources". So, I changed bank_name
>
> from pointer to array.
>
I'll look at that part again when the rest of may comments are addressed.
>
> >
> > > };
> > >
> > > /**
> > > @@ -75,11 +79,17 @@ struct mbox_info {
> > > * Hardcoded TCM bank values. This will be removed once TCM bindings are
> > > * accepted for system-dt specifications and upstreamed in linux kernel
> > > */
> > > -static const struct mem_bank_data zynqmp_tcm_banks[] = {
> > > - {0xffe00000UL, 0x10000UL, PD_R5_0_ATCM, "atcm0"}, /* TCM 64KB each */
> > > - {0xffe20000UL, 0x10000UL, PD_R5_0_BTCM, "btcm0"},
> > > - {0xffe90000UL, 0x10000UL, PD_R5_1_ATCM, "atcm1"},
> > > - {0xffeb0000UL, 0x10000UL, PD_R5_1_BTCM, "btcm1"},
> > > +static const struct mem_bank_data zynqmp_tcm_banks_split[] = {
> > > + {0xffe00000, 0x0, 0x10000, PD_R5_0_ATCM, 0, "atcm0"}, /* TCM 64KB each */
> > > + {0xffe20000, 0x20000, 0x10000, PD_R5_0_BTCM, 0, "btcm0"},
> >
> > Here the device address for btcm0 is 0x20000 while in the cover letter it is
> > 0x2000.
>
> Thanks for catching this. This is actually typo in cover-letter. It should be 0x20000 in cover-letter.
>
> >
> > > + {0xffe90000, 0x0, 0x10000, PD_R5_1_ATCM, 0, "atcm1"},
> > > + {0xffeb0000, 0x20000, 0x10000, PD_R5_1_BTCM, 0, "btcm1"},
> >
> > Same
>
> Same here: It should be 0x20000 in cover-letter.
>
> >
> > > +};
> > > +
> > > +/* TCM 128KB each */
> > > +static const struct mem_bank_data zynqmp_tcm_banks_lockstep[] = {
> > > + {0xffe00000, 0x0, 0x20000, PD_R5_0_ATCM, PD_R5_1_ATCM, "atcm0"},
> > > + {0xffe20000, 0x20000, 0x20000, PD_R5_0_BTCM, PD_R5_1_BTCM, "btcm0"},
> > > };
> > >
> > > /**
> > > @@ -422,6 +432,7 @@ static int zynqmp_r5_mem_region_unmap(struct rproc *rproc,
> > > struct rproc_mem_entry *mem)
> > > {
> > > iounmap((void __iomem *)mem->va);
> > > +
> >
> > Spurious change
> Sure, I will remove it.
> >
> > > return 0;
> > > }
> > >
> > > @@ -526,30 +537,6 @@ static int tcm_mem_map(struct rproc *rproc,
> > > /* clear TCMs */
> > > memset_io(va, 0, mem->len);
> > >
> > > - /*
> > > - * The R5s expect their TCM banks to be at address 0x0 and 0x2000,
> > > - * while on the Linux side they are at 0xffexxxxx.
> > > - *
> > > - * Zero out the high 12 bits of the address. This will give
> > > - * expected values for TCM Banks 0A and 0B (0x0 and 0x20000).
> > > - */
> > > - mem->da &= 0x000fffff;
> > > -
> > > - /*
> > > - * TCM Banks 1A and 1B still have to be translated.
> > > - *
> > > - * Below handle these two banks' absolute addresses (0xffe90000 and
> > > - * 0xffeb0000) and convert to the expected relative addresses
> > > - * (0x0 and 0x20000).
> > > - */
> > > - if (mem->da == 0x90000 || mem->da == 0xB0000)
> > > - mem->da -= 0x90000;
> > > -
> > > - /* if translated TCM bank address is not valid report error */
> > > - if (mem->da != 0x0 && mem->da != 0x20000) {
> > > - dev_err(&rproc->dev, "invalid TCM address: %x\n", mem->da);
> > > - return -EINVAL;
> > > - }
> > > return 0;
> > > }
> > >
> > > @@ -571,6 +558,7 @@ static int add_tcm_carveout_split_mode(struct rproc *rproc)
> > > u32 pm_domain_id;
> > > size_t bank_size;
> > > char *bank_name;
> > > + u32 da;
> > >
> > > r5_core = rproc->priv;
> > > dev = r5_core->dev;
> > > @@ -586,6 +574,7 @@ static int add_tcm_carveout_split_mode(struct rproc *rproc)
> > > bank_name = r5_core->tcm_banks[i]->bank_name;
> > > bank_size = r5_core->tcm_banks[i]->size;
> > > pm_domain_id = r5_core->tcm_banks[i]->pm_domain_id;
> > > + da = r5_core->tcm_banks[i]->da;
> > >
> > > ret = zynqmp_pm_request_node(pm_domain_id,
> > > ZYNQMP_PM_CAPABILITY_ACCESS, 0,
> > > @@ -599,7 +588,7 @@ static int add_tcm_carveout_split_mode(struct rproc *rproc)
> > > bank_name, bank_addr, bank_size);
> > >
> > > rproc_mem = rproc_mem_entry_init(dev, NULL, bank_addr,
> > > - bank_size, bank_addr,
> > > + bank_size, da,
> > > tcm_mem_map, tcm_mem_unmap,
> > > bank_name);
> > > if (!rproc_mem) {
> > > @@ -632,14 +621,14 @@ static int add_tcm_carveout_split_mode(struct rproc *rproc)
> > > */
> > > static int add_tcm_carveout_lockstep_mode(struct rproc *rproc)
> > > {
> > > + u32 pm_domain_id, da, pm_domain_id2;
> > > struct rproc_mem_entry *rproc_mem;
> > > struct zynqmp_r5_core *r5_core;
> > > int i, num_banks, ret;
> > > - phys_addr_t bank_addr;
> > > - size_t bank_size = 0;
> > > + u32 bank_size = 0;
Why is this changed to a u32 when rproc_mem_entry_init() takes a size_t for
@len? This is especially concerning since add_tcm_carveout_split_mode() still
uses a size_t.
> > > struct device *dev;
> > > - u32 pm_domain_id;
> > > char *bank_name;
> > > + u32 bank_addr;
> > >
> > > r5_core = rproc->priv;
> > > dev = r5_core->dev;
> > > @@ -653,12 +642,16 @@ static int add_tcm_carveout_lockstep_mode(struct rproc *rproc)
> > > * So, Enable each TCM block individually, but add their size
> > > * to create contiguous memory region.
> > > */
> > > - bank_addr = r5_core->tcm_banks[0]->addr;
> > > - bank_name = r5_core->tcm_banks[0]->bank_name;
> > > -
> > > for (i = 0; i < num_banks; i++) {
> > > - bank_size += r5_core->tcm_banks[i]->size;
> > > + bank_addr = r5_core->tcm_banks[i]->addr;
> > > + bank_name = r5_core->tcm_banks[i]->bank_name;
> > > + bank_size = r5_core->tcm_banks[i]->size;
> > > pm_domain_id = r5_core->tcm_banks[i]->pm_domain_id;
> > > + pm_domain_id2 = r5_core->tcm_banks[i]->pm_domain_id2;
> > > + da = r5_core->tcm_banks[i]->da;
> > > +
> > > + dev_dbg(dev, "TCM %s addr=0x%x, size=0x%x",
> > > + bank_name, bank_addr, bank_size);
> > >
> > > /* Turn on each TCM bank individually */
> > > ret = zynqmp_pm_request_node(pm_domain_id,
> > > @@ -668,23 +661,28 @@ static int add_tcm_carveout_lockstep_mode(struct rproc *rproc)
> > > dev_err(dev, "failed to turn on TCM 0x%x", pm_domain_id);
> > > goto release_tcm_lockstep;
> > > }
> > > - }
> > >
> > > - dev_dbg(dev, "TCM add carveout lockstep mode %s addr=0x%llx, size=0x%lx",
> > > - bank_name, bank_addr, bank_size);
> > > -
> > > - /* Register TCM address range, TCM map and unmap functions */
> > > - rproc_mem = rproc_mem_entry_init(dev, NULL, bank_addr,
> > > - bank_size, bank_addr,
> > > - tcm_mem_map, tcm_mem_unmap,
> > > - bank_name);
> > > - if (!rproc_mem) {
> > > - ret = -ENOMEM;
> > > - goto release_tcm_lockstep;
> > > - }
> > > + /* Turn on each TCM bank individually */
> > > + ret = zynqmp_pm_request_node(pm_domain_id2,
> > > + ZYNQMP_PM_CAPABILITY_ACCESS, 0,
> > > + ZYNQMP_PM_REQUEST_ACK_BLOCKING);
> > > + if (ret < 0) {
> > > + dev_err(dev, "failed to turn on TCM 0x%x", pm_domain_id2);
> > > + goto release_tcm_lockstep;
> > > + }
> > >
> > > - /* If registration is success, add carveouts */
> > > - rproc_add_carveout(rproc, rproc_mem);
> > > + /* Register TCM address range, TCM map and unmap functions */
> > > + rproc_mem = rproc_mem_entry_init(dev, NULL, bank_addr,
> > > + bank_size, da,
> > > + tcm_mem_map, tcm_mem_unmap,
> > > + bank_name);
The original code adds a single carveout while this code is adding one for each
memory bank? Is this done on purpose or is it a bug? No comment is provided.
> > > + if (!rproc_mem) {
> > > + ret = -ENOMEM;
> > > + goto release_tcm_lockstep;
> > > + }
> > > +
> > > + rproc_add_carveout(rproc, rproc_mem);
> > > + }
> > >
> > > return 0;
> > >
> > > @@ -693,7 +691,12 @@ static int add_tcm_carveout_lockstep_mode(struct rproc *rproc)
> > > for (i--; i >= 0; i--) {
> > > pm_domain_id = r5_core->tcm_banks[i]->pm_domain_id;
> > > zynqmp_pm_release_node(pm_domain_id);
> > > + if (pm_domain_id2) {
> > > + pm_domain_id2 = r5_core->tcm_banks[i]->pm_domain_id2;
> > > + zynqmp_pm_release_node(pm_domain_id2);
> > > + }
> > > }
> > > +
> > > return ret;
> > > }
> > >
> > > @@ -800,17 +803,23 @@ static int zynqmp_r5_rproc_prepare(struct rproc *rproc)
> > > */
> > > static int zynqmp_r5_rproc_unprepare(struct rproc *rproc)
> > > {
> > > + u32 pm_domain_id, pm_domain_id2;
> > > struct zynqmp_r5_core *r5_core;
> > > - u32 pm_domain_id;
> > > int i;
> > >
> > > r5_core = rproc->priv;
> > >
> > > for (i = 0; i < r5_core->tcm_bank_count; i++) {
> > > pm_domain_id = r5_core->tcm_banks[i]->pm_domain_id;
> > > + pm_domain_id2 = r5_core->tcm_banks[i]->pm_domain_id2;
> > > if (zynqmp_pm_release_node(pm_domain_id))
> > > dev_warn(r5_core->dev,
> > > "can't turn off TCM bank 0x%x", pm_domain_id);
> > > + if (pm_domain_id2 && zynqmp_pm_release_node(pm_domain_id2))
> > > + dev_warn(r5_core->dev,
> > > + "can't turn off TCM bank 0x%x", pm_domain_id2);
> > > + dev_dbg(r5_core->dev, "pm_domain_id=%d, pm_domain_id2=%d\n",
> > > + pm_domain_id, pm_domain_id2);
> > > }
> > >
> > > return 0;
> > > @@ -883,6 +892,137 @@ static struct zynqmp_r5_core *zynqmp_r5_add_rproc_core(struct device *cdev)
> > > return ERR_PTR(ret);
> > > }
> > >
> > > +static int zynqmp_r5_get_tcm_node_from_dt(struct zynqmp_r5_cluster *cluster)
> > > +{
> > > + int i, j, tcm_bank_count, ret = -EINVAL;
> > > + struct zynqmp_r5_core *r5_core;
> > > + struct of_phandle_args out_arg;
> > > + struct platform_device *cpdev;
> > > + struct resource *res = NULL;
> > > + u64 abs_addr = 0, size = 0;
> > > + struct mem_bank_data *tcm;
> > > + struct device_node *np, *np1 = NULL;
> > > + struct device *dev;
As far as I can tell @ret, @res and @np1 don't need initilisation. It may also
be the case for @abs_addr and @size.
> > > +
> > > + for (i = 0; i < cluster->core_count; i++) {
> > > + r5_core = cluster->r5_cores[i];
> > > + dev = r5_core->dev;
> > > + np = dev_of_node(dev);
> > > +
> > > + /* we have address cell 2 and size cell as 2 */
> > > + ret = of_property_count_elems_of_size(np, "reg",
> > > + 4 * sizeof(u32));
> > > + if (ret <= 0) {
> > > + ret = -EINVAL;
> > > + goto fail_tcm;
> > > + }
> > > +
> > > + tcm_bank_count = ret;
> > > +
> > > + r5_core->tcm_banks = devm_kcalloc(dev, tcm_bank_count,
> > > + sizeof(struct mem_bank_data *),
> > > + GFP_KERNEL);
> > > + if (!r5_core->tcm_banks) {
> > > + ret = -ENOMEM;
> > > + goto fail_tcm;
> > > + }
> > > +
> > > + r5_core->tcm_bank_count = tcm_bank_count;
> > > + for (j = 0; j < tcm_bank_count; j++) {
> > > + tcm = kzalloc(sizeof(struct mem_bank_data *), GFP_KERNEL);
> > > + if (!tcm) {
> > > + ret = -ENOMEM;
> > > + goto fail_tcm;
> > > + }
> > > +
> > > + r5_core->tcm_banks[j] = tcm;
> > > + /* get tcm address without translation */
> > > + ret = of_property_read_reg(np, j, &abs_addr, &size);
> > > + if (ret) {
> > > + dev_err(dev, "failed to get reg property\n");
> > > + goto fail_tcm;
> > > + }
> > > +
> > > + /*
> > > + * remote processor can address only 32 bits
> > > + * so convert 64-bits into 32-bits. This will discard
> > > + * any unwanted upper 32-bits.
> > > + */
> > > + tcm->da = (u32)abs_addr;
> > > + tcm->size = (u32)size;
> > > +
> > > + cpdev = to_platform_device(dev);
> > > + res = platform_get_resource(cpdev, IORESOURCE_MEM, j);
> > > + if (!res) {
> > > + dev_err(dev, "failed to get tcm resource\n");
> > > + ret = -EINVAL;
> > > + goto fail_tcm;
> > > + }
> > > +
> > > + tcm->addr = (u32)res->start;
> > > + res = devm_request_mem_region(dev, tcm->addr, tcm->size, res->name);
> > > + if (!res) {
> > > + dev_err(dev, "failed to request tcm resource\n");
> > > + ret = -EINVAL;
> > > + goto fail_tcm;
> > > + }
> > > +
> > > + memcpy(tcm->bank_name, res->name, ARRAY_SIZE(tcm->bank_name));
> > > + np = of_node_get(dev_of_node(dev));
> > > + /*
> > > + * In dt power-domains are described in this order:
> > > + * <RPU core>, <atcm>, <btcm>
> > > + * parse power domains for tcm accordingly
> > > + */
> > > + of_parse_phandle_with_args(np, "power-domains",
> > > + "#power-domain-cells",
> > > + j + 1, &out_arg);
> > > + tcm->pm_domain_id = out_arg.args[0];
> > > + of_node_put(out_arg.np);
> > > +
> > > + dev_dbg(dev, "TCM: %s, dma=0x%x, da=0x%x, size=0x%x\n",
> > > + tcm->bank_name, tcm->addr, tcm->da, tcm->size);
> > > + dev_dbg(dev, "tcm pm domain id %d\n", tcm->pm_domain_id);
> > > +
> > > + if (cluster->mode == SPLIT_MODE)
> > > + continue;
> > > +
> > > + /* Turn on core-1's TCM as well */
> > > + np1 = of_get_next_child(dev_of_node(cluster->dev),
> > > + r5_core->np);
> > > + if (!np1) {
> > > + of_node_put(np1);
> > > + np1 = NULL;
> > > + goto fail_tcm;
> > > + }
> > > +
> > > + of_parse_phandle_with_args(np1, "power-domains",
> > > + "#power-domain-cells",
> > > + j + 1, &out_arg);
> > > + tcm->pm_domain_id2 = out_arg.args[0];
> > > + of_node_put(out_arg.np);
> > > + dev_dbg(dev, "tcm pm domain id %d\n", tcm->pm_domain_id2);
> > > + }
> > > + }
> > > +
> > > + return 0;
> > > +
> > > +fail_tcm:
> > > + while (i >= 0) {
> > > + r5_core = cluster->r5_cores[i];
> > > + for (j = 0; j < r5_core->tcm_bank_count; j++) {
> > > + if (!r5_core->tcm_banks)
> > > + continue;
> > > + tcm = r5_core->tcm_banks[j];
> > > + kfree(tcm);
> > > + }
> > > + kfree(r5_core->tcm_banks);
> > > + i--;
> > > + }
> > > +
> > > + return ret;
> > > +}
> > > +
> > > /**
> > > * zynqmp_r5_get_tcm_node()
> > > * Ideally this function should parse tcm node and store information
> > > @@ -895,12 +1035,20 @@ static struct zynqmp_r5_core *zynqmp_r5_add_rproc_core(struct device *cdev)
> > > */
> > > static int zynqmp_r5_get_tcm_node(struct zynqmp_r5_cluster *cluster)
> > > {
> > > + const struct mem_bank_data *zynqmp_tcm_banks;
> > > struct device *dev = cluster->dev;
> > > struct zynqmp_r5_core *r5_core;
> > > int tcm_bank_count, tcm_node;
> > > int i, j;
> > >
> > > - tcm_bank_count = ARRAY_SIZE(zynqmp_tcm_banks);
> > > + if (cluster->mode == SPLIT_MODE) {
> > > + zynqmp_tcm_banks = zynqmp_tcm_banks_split;
> > > + tcm_bank_count = ARRAY_SIZE(zynqmp_tcm_banks_split);
> > > + } else {
> > > + zynqmp_tcm_banks = zynqmp_tcm_banks_lockstep;
> > > + tcm_bank_count = ARRAY_SIZE(zynqmp_tcm_banks_lockstep);
> > > + }
> >
> > Why are the changes to get TCM bank information from the DT and enhancement to
> > support lockstep mode in the same patch?
>
> Actually TCM in lockstep mode was supported before as well. It's just I was using same table in lockstep mode before.
>
> However, now I am having two tables for split mode and lockstep mode.
>
> I had to do this as I have introduced "da" field in "struct mem_bank_data" object. This makes it easy to process
>
> "device address" derived from device-tree.
>
> And as I have introduced "u32 da", I had to modify table as well and remove hardcoding of "da" calculation in "tcm_mem_map" function.
>
> As all of this is connected, I have them in same patch. No new functionality is added, but just code refactoring.
>
> > > +
> > >
> > > /* count per core tcm banks */
> > > tcm_bank_count = tcm_bank_count / cluster->core_count;
> > > @@ -951,10 +1099,25 @@ static int zynqmp_r5_core_init(struct zynqmp_r5_cluster *cluster,
> > > enum rpu_tcm_comb tcm_mode)
> > > {
> > > struct device *dev = cluster->dev;
> > > + struct device_node *np;
> > > struct zynqmp_r5_core *r5_core;
> > > int ret, i;
> > >
> > > - ret = zynqmp_r5_get_tcm_node(cluster);
> > > + /*
> > > + * try to get tcm nodes from dt but if fail, use hardcode addresses only
> > > + * for zynqmp platform. New platforms must use dt bindings for TCM.
> > > + */
> > > + ret = zynqmp_r5_get_tcm_node_from_dt(cluster);
> > > + if (ret) {
> > > + np = of_get_compatible_child(dev_of_node(dev), "xlnx,zynqmp-r5f");
> > > + if (np) {
> >
> > Why was this check added?
>
> We want to maintain backward compatibility with previous bindings only for zynqmp platform.
>
That check has nothing to do with backward compatibility.
> So, hardcode table is used only for "zynqmp" platform if getting "reg" information from device-tree fails.
>
> If node is not compatible with "xlnx,zynqmp-r5f" then it is new platform and we must not use hardcode
>
> table instead we should fail.
>
So this is the real reason for the check, but zynqmp-r5f is still the only
platform supported by this driver. Please remove and re-introduce if/when a new
platform is added.
>
> > So far there are too many unanswered questions with this patchset and as such I
> > will stop here.
>
> No problem. Please let me know if you have any further questions.
>
>
> > Mathieu
> >
> > > + ret = zynqmp_r5_get_tcm_node(cluster);
> > > + } else {
> > > + dev_err(dev, "tcm not found\n");
> > > + return -EINVAL;
> > > + }
> > > + }
> > > +
> > > if (ret < 0) {
> > > dev_err(dev, "can't get tcm node, err %d\n", ret);
> > > return ret;
> > > --
> > > 2.25.1
> > >
More information about the linux-arm-kernel
mailing list