| Message ID | 20200921161406.11929-1-ben.levinsky@xilinx.com |
|---|---|
| Headers | show |
| Series | Provide basic driver to control Arm R5 co-processor found on Xilinx ZynqMP | expand |
Hey Ben, Thanks for sending out the new series, this patchset is functional for booting both R5 0 and R5 1 in split mode. A few comments below, still working my way through the rest of the code though now that this works. On Mon, Sep 21, 2020 at 09:14:06AM -0700, Ben Levinsky wrote: <...> > +static int zynqmp_r5_remoteproc_probe(struct platform_device *pdev) > +{ > + int ret, i = 0; > + struct device *dev = &pdev->dev; > + struct device_node *nc; > + > + rpu_mode = of_get_property(dev->of_node, "lockstep-mode", NULL) ? > + PM_RPU_MODE_LOCKSTEP : PM_RPU_MODE_SPLIT; Extra whitespace, and of_property_read_bool would read a bit nicer here (does the same thing in the end, though). Since rpu_mode is only used here and in r5_set_mode, I think it'd be better to plumb it through zynqmp_r5_probe instead of making it global in this file. > + > + dev_dbg(dev, "RPU configuration: %s\n", > + rpu_mode == PM_RPU_MODE_LOCKSTEP ? "lockstep" : "split"); > + > + for_each_available_child_of_node(dev->of_node, nc) { > + /* > + * if 2 RPUs provided but one is lockstep, then we have an > + * invalid configuration. > + */ > + if (i > 0 && rpu_mode == PM_RPU_MODE_LOCKSTEP) > + return -EINVAL; > + > + /* only call zynqmp_r5_probe if proper # of rpu's */ > + ret = (i < MAX_RPROCS) ? zynqmp_r5_probe(&rpus[i], pdev, nc) : > + -EINVAL; > + dev_dbg(dev, "%s to probe rpu %pOF\n", > + ret ? "Failed" : "Able", > + nc); It'd be cleaner to check the child node count before the loop: rpu_nodes = of_get_available_child_count(nc) if ((rpu_mode == PM_RPU_MODE_LOCKSTEP && rpu_nodes != 1) || rpu_nodes > 2) return -EINVAL; > + > + if (ret) > + return ret; > + > + i++; > + } > + > + return 0; > +} > + > +static int zynqmp_r5_remoteproc_remove(struct platform_device *pdev) > +{ > + int i; > + > + for (i = 0; i < MAX_RPROCS; i++) { > + struct zynqmp_r5_pdata *pdata = &rpus[i]; > + struct rproc *rproc; > + > + /* only do clean up for pdata with active rpu */ > + if (pdata->pnode_id == 0) > + continue; This seems like a bit of a hack, resulting from the use of a static array for holding the zynqmp_r5_pdata for each rpu. Consider allocating zynqmp_r5_pdata in zynqmp_r5_probe, and adding each instance to a linked-list at file scope. - memory is only allocated RPUs actually in use - no need for this pnode_id == 0 hack - MAX_RPROCS can be eliminated, just traverse that list in remove - No reuse of the pdata across probe/removes, so all of the e.g. condtionals below ("if (rproc)") and NULL assignments can be eliminated. > + > + rproc = pdata->rproc; > + if (rproc) { > + rproc_del(rproc); > + rproc_free(rproc); > + pdata->rproc = NULL; > + } > + if (pdata->tx_chan) { > + mbox_free_channel(pdata->tx_chan); > + pdata->tx_chan = NULL; > + } > + if (pdata->rx_chan) { > + mbox_free_channel(pdata->rx_chan); > + pdata->rx_chan = NULL; > + } > + if (&(&pdata->dev)->dma_pools) > + device_unregister(&pdata->dev); The condition here looks very wrong to me, as it will always be true. What is this trying to achieve? > + } > + > + return 0; > +} > + > +/* Match table for OF platform binding */ > +static const struct of_device_id zynqmp_r5_remoteproc_match[] = { > + { .compatible = "xlnx,zynqmp-r5-remoteproc-1.0", }, > + { /* end of list */ }, > +}; > +MODULE_DEVICE_TABLE(of, zynqmp_r5_remoteproc_match); > + > +static struct platform_driver zynqmp_r5_remoteproc_driver = { > + .probe = zynqmp_r5_remoteproc_probe, > + .remove = zynqmp_r5_remoteproc_remove, > + .driver = { > + .name = "zynqmp_r5_remoteproc", > + .of_match_table = zynqmp_r5_remoteproc_match, > + }, > +}; > +module_platform_driver(zynqmp_r5_remoteproc_driver); > + > +MODULE_AUTHOR("Ben Levinsky <ben.levinsky@xilinx.com>"); > +MODULE_LICENSE("GPL v2"); > -- > 2.17.1 > Thanks, Michael
Hi Michael, > -----Original Message----- > From: Michael Auchter <michael.auchter@ni.com> > Sent: Monday, September 21, 2020 3:12 PM > To: Ben Levinsky <BLEVINSK@xilinx.com> > Cc: sunnyliangjy@gmail.com; punit1.agrawal@toshiba.co.jp; Stefano Stabellini > <stefanos@xilinx.com>; Michal Simek <michals@xilinx.com>; > devicetree@vger.kernel.org; mathieu.poirier@linaro.org; Ed T. Mooring > <emooring@xilinx.com>; linux-remoteproc@vger.kernel.org; linux- > kernel@vger.kernel.org; robh+dt@kernel.org; linux-arm- > kernel@lists.infradead.org; Jiaying Liang <jliang@xilinx.com>; Michal Simek > <michals@xilinx.com>; Ed T. Mooring <emooring@xilinx.com>; Jason Wu > <j.wu@xilinx.com> > Subject: Re: [PATCH v15 5/5] remoteproc: Add initial zynqmp R5 remoteproc > driver > > Hey Ben, > > Thanks for sending out the new series, this patchset is functional for > booting both R5 0 and R5 1 in split mode. > > A few comments below, still working my way through the rest of the code > though now that this works. > > On Mon, Sep 21, 2020 at 09:14:06AM -0700, Ben Levinsky wrote: > <...> > > +static int zynqmp_r5_remoteproc_probe(struct platform_device *pdev) > > +{ > > + int ret, i = 0; > > + struct device *dev = &pdev->dev; > > + struct device_node *nc; > > + > > + rpu_mode = of_get_property(dev->of_node, "lockstep-mode", NULL) > ? > > + PM_RPU_MODE_LOCKSTEP : PM_RPU_MODE_SPLIT; > > Extra whitespace, and of_property_read_bool would read a bit nicer here > (does the same thing in the end, though). > > Since rpu_mode is only used here and in r5_set_mode, I think it'd be > better to plumb it through zynqmp_r5_probe instead of making it global > in this file. > [Ben Levinsky] will do > > + > > + dev_dbg(dev, "RPU configuration: %s\n", > > + rpu_mode == PM_RPU_MODE_LOCKSTEP ? "lockstep" : > "split"); > > + > > + for_each_available_child_of_node(dev->of_node, nc) { > > + /* > > + * if 2 RPUs provided but one is lockstep, then we have an > > + * invalid configuration. > > + */ > > + if (i > 0 && rpu_mode == PM_RPU_MODE_LOCKSTEP) > > + return -EINVAL; > > + > > + /* only call zynqmp_r5_probe if proper # of rpu's */ > > + ret = (i < MAX_RPROCS) ? zynqmp_r5_probe(&rpus[i], pdev, > nc) : > > + -EINVAL; > > + dev_dbg(dev, "%s to probe rpu %pOF\n", > > + ret ? "Failed" : "Able", > > + nc); > > It'd be cleaner to check the child node count before the loop: > > rpu_nodes = of_get_available_child_count(nc) > if ((rpu_mode == PM_RPU_MODE_LOCKSTEP && rpu_nodes != 1) || > rpu_nodes > 2) > return -EINVAL; > [Ben Levinsky] will do > > + > > + if (ret) > > + return ret; > > + > > + i++; > > + } > > + > > + return 0; > > +} > > + > > +static int zynqmp_r5_remoteproc_remove(struct platform_device *pdev) > > +{ > > + int i; > > + > > + for (i = 0; i < MAX_RPROCS; i++) { > > + struct zynqmp_r5_pdata *pdata = &rpus[i]; > > + struct rproc *rproc; > > + > > + /* only do clean up for pdata with active rpu */ > > + if (pdata->pnode_id == 0) > > + continue; > > This seems like a bit of a hack, resulting from the use of a static > array for holding the zynqmp_r5_pdata for each rpu. > > Consider allocating zynqmp_r5_pdata in zynqmp_r5_probe, and adding each > instance to a linked-list at file scope. > - memory is only allocated RPUs actually in use > - no need for this pnode_id == 0 hack > - MAX_RPROCS can be eliminated, just traverse that list in > remove > - No reuse of the pdata across probe/removes, so all of the e.g. > condtionals below ("if (rproc)") and NULL assignments can be > eliminated. > [Ben Levinsky] so parts of this I can do.. - can make the rpus a static list of ptr's which I think is equivalent To what you are describing - can eliminate the pnode_id == 0 hack For the rproc_del, rproc_free fn calls, these should stay. Just as other upstream remoteproc drivers do, this is being done similarly. For mbox handling, I am mimic'ing upstream ST and TI drivers https://github.com/torvalds/linux/blob/v5.9-rc3/drivers/remoteproc/stm32_rproc.c they similarly check if the mbox channel is not NULL, and if so call mbox_free_channel. This is similar for Xilinx remoteproc R5 use case as the mbox can be unused in 1 remoteproc node. Also, similar to TI and ST driver, https://github.com/torvalds/linux/blob/v5.9-rc3/drivers/remoteproc/stm32_rproc.c#L321 , I am setting the mbox to NULL at remove > > + > > + rproc = pdata->rproc; > > + if (rproc) { > > + rproc_del(rproc); > > + rproc_free(rproc); > > + pdata->rproc = NULL; > > + } > > + if (pdata->tx_chan) { > > + mbox_free_channel(pdata->tx_chan); > > + pdata->tx_chan = NULL; > > + } > > + if (pdata->rx_chan) { > > + mbox_free_channel(pdata->rx_chan); > > + pdata->rx_chan = NULL; > > + } > > + if (&(&pdata->dev)->dma_pools) > > + device_unregister(&pdata->dev); > > The condition here looks very wrong to me, as it will always be true. > What is this trying to achieve? > This was originally because of the static rpu declaration. By instead using ptr's this can be removed as the zynqmp_r5_pdata ptr will be NULL so I can check that instead. So will remove this. Thank you for the review Ben > > + } > > + > > + return 0; > > +} > > + > > +/* Match table for OF platform binding */ > > +static const struct of_device_id zynqmp_r5_remoteproc_match[] = { > > + { .compatible = "xlnx,zynqmp-r5-remoteproc-1.0", }, > > + { /* end of list */ }, > > +}; > > +MODULE_DEVICE_TABLE(of, zynqmp_r5_remoteproc_match); > > + > > +static struct platform_driver zynqmp_r5_remoteproc_driver = { > > + .probe = zynqmp_r5_remoteproc_probe, > > + .remove = zynqmp_r5_remoteproc_remove, > > + .driver = { > > + .name = "zynqmp_r5_remoteproc", > > + .of_match_table = zynqmp_r5_remoteproc_match, > > + }, > > +}; > > +module_platform_driver(zynqmp_r5_remoteproc_driver); > > + > > +MODULE_AUTHOR("Ben Levinsky <ben.levinsky@xilinx.com>"); > > +MODULE_LICENSE("GPL v2"); > > -- > > 2.17.1 > > > > Thanks, > Michael
The driver was tested on Xilinx ZynqMP QEMU For sake of ease of review, only support ZynqMP. Once accepted, then add support for Versal platform and R5 loading onto OCM. v2: - remove domain struct v3: - add xilinx-related platform mgmt fn's instead of wrapping around function pointer in xilinx eemi ops struct - update zynqmp_r5 yaml parsing to not raise warnings for extra information in children of R5 node. The warning "node has a unit name, but no reg or ranges property" will still be raised though as this particular node is needed to describe the '#address-cells' and '#size-cells' information. v4: - add default values for enums - fix formatting as per checkpatch.pl --strict. Note that 1 warning and 1 check are still raised as each is due to fixing the warning results in that particular line going over 80 characters. - remove warning '/example-0/rpu@ff9a0000/r5@0: node has a unit name, but no reg or ranges property' by adding reg to r5 node. v5: - update device tree sample and yaml parsing to not raise any warnings - description for memory-region in yaml parsing - compatible string in yaml parsing for TCM - parse_fw change from use of rproc_of_resm_mem_entry_init to rproc_mem_entry_init and use of alloc/release - var's of type zynqmp_r5_pdata all have same local variable name - use dev_dbg instead of dev_info v6: - adding memory carveouts is handled much more similarly. All mem carveouts are now described in reserved memory as needed. That is, TCM nodes are not coupled to remoteproc anymore. This is reflected in the remoteproc R5 driver and the device tree binding. - remove mailbox from device tree binding as it is not necessary for elf loading v7: - remove unused headers - zynqmp_r5_remoteproc_probe:lockstep_mode from u32* to u32 - device-tree binding "lockstep-mode" to "xlnx,cluster-mode" - remove zynqmp_r5_mem_probe and loop to Probe R5 memory devices at probe() - remove is_r5_mode_set from zynqmp rpu remote processor private data - do not error out if no mailbox is provided since mailboxes are optional - remove zynqmp_r5_remoteproc_probe call of platform_set_drvdata as pdata is handled in zynqmp_r5_remoteproc_remove v8: - remove old acks, reviewed-by's in commit message v9: - if zynqmp_r5_remoteproc.c pdata->tx_mc_skbs not initialized, then do not call skb_queue_empty - update arguments and documentation for zynqmp_pm_set_rpu_mode - in fn zynqmp_pm_force_powerdown, change arg 'target' to 'node' - zynqmp_pm_request_wakeup update code style - edit 3/5 patch commit message - document zynqmp_pm_set_tcm_config and zynqmp_pm_get_rpu_mode documentation to include expected return val - remove unused fn zynqmp_pm_get_node_status - update 5/5 patch commit message to document supported configurations and how they are booted by the driver. - remove copyrights other than SPDX from zynqmp_r5_remoteproc.c - compilation warnings no longer raised - remove unused includes from zynqmp_r5_remoteproc.c - remove unused var autoboot from zynqmp_r5_remoteproc.c - reorder zynqmp_r5_pdata fpr small mem savings due to alignment - zynqmp_pm_set_tcm_config and zynqmp_pm_set_rpu_mode uses second arg - zynqmp_r5_remoteproc.c use of zynqmp_pm_set_tcm_config now does not have output arg - in tcm handling, unconditionally use &= 0x000fffff mask since all nodes in this fn are for tcm - update comments for translating dma field in tcm handling to device address - update calls to rproc_mem_entry_init in parse_mem_regions so that there are only 2 cases for types of carveouts instead of 3 - in parse_mem_regions, check if device tree node is null before using it - add example device tree nodes used in parse_mem_regions and tcm parsing - add comment for vring id node length - add check for string length so that vring id is at least min length - move tcm nodes from reserved mem to instead own device tree nodes and only use them if enabled in device tree - add comment for explaining handling of rproc_elf_load_rsc_table - remove obsolete check for "if (vqid < 0)" in zynqmp_r5_rproc_kick - remove unused field mems in struct zynqmp_r5_pdata - remove call to zynqmp_r5_mem_probe and the fn itself as tcm handling is done by zyqmp_r5_pm_request_tcm - remove obsolete setting of dma_ops and parent device dma_mask - remove obsolete use of of_dma_configure - add comment for call to r5_set_mode fn - make mbox usage optional and gracefully inform user via dev_dbg if not present - change lockstep_mode from u32* to u32 - update zynqmp_pm_set_rpu_mode and zynqmp_pm_set_rpu_mode documentation and remove unused args v10: - add include types.h to xlnx-zynqmp.h for compilation v11: - update usage of zynqmp_pm_get_rpu_mode to return rpu mode in enum - update zynqmp_pm_set_tcm_config and zynqmp_pm_set_rpu_mode arguments to remove unused args - use enums instead of u32 where possible in zynqmp_r5_remoteproc - zynqmp_r5_remoteproc: update prints to not use carriage return, just newline - zynqmp_r5_remoteproc: look up tcm banks via property instead of string name - print device tree nodes with %pOF instead of %s with node name field - update tcm release to unmap VA - handle r5-1 use case - device tree binding r5 node to have link to TCMs via meta-memory-regions property - fix device tree binding so no warnings from 'make dt_binding_check' v12: - update signed off by so that latest developer name is last - in drivers/firmware/zynqmp.c, update zynqmp_pm_set_rpu_mode so rpu_mode is only set if no error - update args for zynqmp_pm_set_rpu_mode, zynqmp_pm_set_tcm_config fn arg's to reflect what is expected in the function and the usage in zynqmp_r5_remoteproc accordingly v13: - zynqmp_pm_get_rpu_mode argument zynqmp_pm_get_rpu_mode is only set if no error v14: - rebase off v5.9-rc3 kernel - concerns were raised about the new property meta-memory-regions. There is no clear direction so for the moment I kept it in the series - in device tree binding, place IPC nodes in RAM in the reserved memory section - change zynqmp_r5_remoteproc::rpus and rpu_mode to static - fix typo - zynqmp_r5_remoteproc::r5_set_mode set rpu mode from property specified in device tree - use u32 instead of u32* to store in remoteproc memory entry private data for pnode_id information - always call r5_set_mode on probe - remove alloc of zynqmp_r5_pdata in zynqmp_r5_remoteproc::zynqmp_r5_remoteproc_probe as there is static allocation already - error at probe time if lockstep-mode property not present in device tree - update commit message as per review - remove dependency on MAILBOX in makefile as ZYNQMP_IPI_MBOX is present - remove unused macros - update comment ordering of zynqmp_r5_pdata to match struct definition - zynqmp_r5_remoteproc::tcm_mem_release error if pnode id is invalid - remove obsolete TODOs - only call zynqmp_r5_remoteproc::zynqmp_r5_probe if the index is valid - remove uneven dev_dbg/dev_err fn calls v15: - change lockstep mode device tree property from acting as boolean, to instead being used as, if it is present, then r5 is in lockstep mode. otherwise in split. - add safeguard for if 2 rpu remoteproc instances are provided then err out Ben Levinsky (5): firmware: xilinx: Add ZynqMP firmware ioctl enums for RPU configuration. firmware: xilinx: Add shutdown/wakeup APIs firmware: xilinx: Add RPU configuration APIs dt-bindings: remoteproc: Add documentation for ZynqMP R5 rproc bindings remoteproc: Add initial zynqmp R5 remoteproc driver .../xilinx,zynqmp-r5-remoteproc.yaml | 120 +++ drivers/firmware/xilinx/zynqmp.c | 96 +++ drivers/remoteproc/Kconfig | 8 + drivers/remoteproc/Makefile | 1 + drivers/remoteproc/zynqmp_r5_remoteproc.c | 769 ++++++++++++++++++ include/linux/firmware/xlnx-zynqmp.h | 60 ++ 6 files changed, 1054 insertions(+) create mode 100644 Documentation/devicetree/bindings/remoteproc/xilinx,zynqmp-r5-remoteproc.yaml create mode 100644 drivers/remoteproc/zynqmp_r5_remoteproc.c