lkml.org 
[lkml]   [2020]   [Oct]   [6]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRE: RE: [PATCH v18 5/5] remoteproc: Add initial zynqmp R5 remoteproc driver
Date


> -----Original Message-----
> From: Michael Auchter <michael.auchter@ni.com>
> Sent: Tuesday, October 6, 2020 2:32 PM
> To: Ben Levinsky <BLEVINSK@xilinx.com>
> Cc: Ed T. Mooring <emooring@xilinx.com>; 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; linux-remoteproc@vger.kernel.org; linux-
> kernel@vger.kernel.org; robh+dt@kernel.org; linux-arm-
> kernel@lists.infradead.org
> Subject: Re: RE: [PATCH v18 5/5] remoteproc: Add initial zynqmp R5
> remoteproc driver
>
> On Tue, Oct 06, 2020 at 07:15:49PM +0000, Ben Levinsky wrote:
> >
> > Hi Michael,
> >
> > Thanks for the review
> >
>
> < ... snip ... >
>
> > > > + z_rproc = rproc->priv;
> > > > + z_rproc->dev.release = zynqmp_r5_release;
> > >
> > > This is the only field of z_rproc->dev that's actually initialized, and
> > > this device is not registered with the core at all, so zynqmp_r5_release
> > > will never be called.
> > >
> > > Since it doesn't look like there's a need to create this additional
> > > device, I'd suggest:
> > > - Dropping the struct device from struct zynqmp_r5_rproc
> > > - Performing the necessary cleanup in the driver remove
> > > callback instead of trying to tie it to device release
> >
> > For the most part I agree. I believe the device is still needed for
> > the mailbox client setup.
> >
> > As the call to mbox_request_channel_byname() requires its own device
> > that has the corresponding child node with the corresponding
> > mbox-related properties.
> >
> > With that in mind, is it still ok to keep the device node?
>
> Ah, I see. Thanks for the clarification!
>
> Instead of manually dealing with the device node creation for the
> individual processors, perhaps it makes more sense to use
> devm_of_platform_populate() to create them. This is also consistent with
> the way the TI K3 R5F remoteproc driver does things.
>
> Cheers,
> Michael

I've been working on this today for a way around it and found one that I think works with your initial suggestion,
- in z_rproc, change dev from struct device to struct device*
^ the above is shown the usage thereof below. It is there for the mailbox setup.
- in driver probe:
- add list_head to keep track of each core's z_rproc and for the driver remove clean up
- in each core's probe (zynqmp_r5_probe) dothe following:


rproc_ptr = rproc_alloc(dev, dev_name(dev), &zynqmp_r5_rproc_ops,
NULL, sizeof(struct zynqmp_r5_rproc));
if (!rproc_ptr)
return -ENOMEM;
z_rproc = rproc_ptr->priv;
z_rproc->dt_node = node;
z_rproc->rproc = rproc_ptr;
z_rproc->dev = &rproc_ptr->dev;
z_rproc->dev->of_node = node;
where node is the specific R5 core's of_node/ Device tree node.

the above preserves most of the mailbox setup code.


With this, I have already successfully done the following in a v19 patch
- move all the previous driver release code to remove
- able to probe, start/stop r5, driver remove repeatedly

Also, this mimics the TI R5 driver code as each core's rproc has a list_head and they have a structure for the cluster which among other things maintains a linked list of the cores' specific rproc information.

Thanks
Ben

\
 
 \ /
  Last update: 2020-10-06 23:47    [W:0.105 / U:0.084 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site