Messages in this thread | | | Date | Tue, 14 Dec 2021 01:01:03 +0530 | Subject | Re: [PATCH v2 6/6] drivers: remoteproc: Add Xilinx r5 remoteproc driver | From | Tanmay Shah <> |
| |
On 12/13/21 4:38 PM, Lars-Peter Clausen wrote: > On 11/23/21 7:20 AM, Tanmay Shah wrote: >> [...] >> +/* >> + * zynqmp_r5_rproc_mem_map >> + * @rproc: single R5 core's corresponding rproc instance >> + * @mem: mem entry to map >> + * >> + * Callback to map va for memory-region's carveout. >> + * >> + * return 0 on success, otherwise non-zero value on failure >> + */ >> +static int zynqmp_r5_rproc_mem_map(struct rproc *rproc, >> + struct rproc_mem_entry *mem) >> +{ >> + void __iomem *va; >> + >> + va = ioremap_wc(mem->dma, mem->len); > > Since you want normal memory and not IO memory a better choice might > be memremap() with MEMREMAP_WC. Internally memremap() will call > ioremap_wc(), but this will make the intention clear and you do not > have to deal with the __iomem type cast. >
Thanks Lars-Peter. Yes you are right. I found this article https://lwn.net/Articles/653585/ about use of memremap after I posted this patchset and I have planned to replace ioremap_wc function with memremap with MEMREMAP_WC flag.
>> + if (IS_ERR_OR_NULL(va)) >> + return -ENOMEM; >> + >> + mem->va = (void *)va; >> + >> + return 0; >> +} >> [...] >> >> +static int add_tcm_banks(struct rproc *rproc) >> +{ >> + struct device *dev; >> + struct platform_device *parent_pdev; >> + struct zynqmp_r5_cluster *cluster; >> + struct zynqmp_r5_core *r5_core; >> + >> + r5_core = (struct zynqmp_r5_core *)rproc->priv; >> + if (!r5_core) >> + return -EINVAL; >> + >> + dev = r5_core->dev; >> + if (!dev) { >> + pr_err("r5 core device unavailable\n"); >> + return -ENODEV; >> + } >> + >> + parent_pdev = to_platform_device(dev->parent); >> + if (!parent_pdev) { >> + dev_err(dev, "parent platform dev unavailable\n"); >> + return -ENODEV; >> + } >> + >> + cluster = platform_get_drvdata(parent_pdev); > > You could just use dev_get_drvdata() without having to cast back to > the platform_device first. > > >> + if (!cluster) { >> + dev_err(&parent_pdev->dev, "Invalid driver data\n"); >> + return -EINVAL; >> + } >> + >> + if (cluster->mode == SPLIT_MODE) >> + return add_tcm_carveout_split_mode(rproc); >> + else if (cluster->mode == LOCKSTEP_MODE) >> + return add_tcm_carveout_lockstep_mode(rproc); >> + >> + dev_err(cluster->dev, "invalid cluster mode\n"); >> + return -EINVAL; >> +} >> + >> [...] >> + >> +static struct rproc_ops zynqmp_r5_rproc_ops = { > const >> + .start = zynqmp_r5_rproc_start, >> + .stop = zynqmp_r5_rproc_stop, >> + .load = rproc_elf_load_segments, >> + .parse_fw = zynqmp_r5_parse_fw, >> + .find_loaded_rsc_table = rproc_elf_find_loaded_rsc_table, >> + .sanity_check = rproc_elf_sanity_check, >> + .get_boot_addr = rproc_elf_get_boot_addr, >> +}; >> [....] >> +static int zynqmp_r5_get_mem_region_node(struct zynqmp_r5_core >> *r5_core) >> +{ >> [...] >> + >> + for (i = 0; i < res_mem_count; i++) { >> + rmem_np = of_parse_phandle(np, "memory-region", i); >> + if (!rmem_np) >> + return -EINVAL; >> + >> + rmem = of_reserved_mem_lookup(rmem_np); >> + if (!rmem) { >> + of_node_put(rmem_np); >> + return -EINVAL; >> + } >> + >> + memcpy(&r5_core->res_mem[i], rmem, >> + sizeof(struct reserved_mem)); > > r5_core->res_mem[i] = *mem; > > This will give you proper type checking and is also a bit shorter. > >> + of_node_put(rmem_np); >> + } >> + >> + r5_core->res_mem_count = res_mem_count; >> + >> + return 0; >> +} >> [...] >> + >> +static int zynqmp_r5_cluster_init(struct zynqmp_r5_cluster *cluster) >> +{ >> [...] >> + >> >> + i = 0; >> + for_each_available_child_of_node(dev_node, child) { >> + child_pdev = of_find_device_by_node(child); >> + if (!child_pdev) > A return or a break in a for_each_available_child_of_node() will leak > the reference to the child node.
Do you mean I have to use of_put_node for each child?
>> [...] >> + } >> + >> [...] >> + >> + return 0; >> +} >> + >> +static void zynqmp_r5_cluster_exit(void *data) >> +{ >> + struct platform_device *pdev = (struct platform_device *)data; >> + >> + platform_set_drvdata(pdev, NULL); > This is not needed. The device driver core will set drvdata to NULL > when the device is removed. >> + >> + pr_info("Exit r5f subsystem driver\n"); > This is probably also not needed. >> +} > >
| |