Hi Michael, > -----Original Message----- > From: Michael Auchter <[email protected]> > Sent: Monday, September 21, 2020 3:12 PM > To: Ben Levinsky <[email protected]> > Cc: [email protected]; [email protected]; Stefano Stabellini > <[email protected]>; Michal Simek <[email protected]>; > [email protected]; [email protected]; Ed T. Mooring > <[email protected]>; [email protected]; linux- > [email protected]; [email protected]; linux-arm- > [email protected]; Jiaying Liang <[email protected]>; Michal Simek > <[email protected]>; Ed T. Mooring <[email protected]>; Jason Wu > <[email protected]> > 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 <[email protected]>"); > > +MODULE_LICENSE("GPL v2"); > > -- > > 2.17.1 > > > > Thanks, > Michael
