devicetree.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Ben Levinsky <BLEVINSK@xilinx.com>
To: Michael Auchter <michael.auchter@ni.com>
Cc: "Ed T. Mooring" <emooring@xilinx.com>,
	Stefano Stabellini <stefanos@xilinx.com>,
	Michal Simek <michals@xilinx.com>,
	"devicetree@vger.kernel.org" <devicetree@vger.kernel.org>,
	"mathieu.poirier@linaro.org" <mathieu.poirier@linaro.org>,
	"linux-remoteproc@vger.kernel.org"
	<linux-remoteproc@vger.kernel.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"robh+dt@kernel.org" <robh+dt@kernel.org>,
	"linux-arm-kernel@lists.infradead.org" 
	<linux-arm-kernel@lists.infradead.org>
Subject: RE: RE: RE: [PATCH v18 5/5] remoteproc: Add initial zynqmp R5 remoteproc driver
Date: Wed, 7 Oct 2020 14:31:15 +0000	[thread overview]
Message-ID: <BYAPR02MB44071E3F9D431CBC72920E4AB50A0@BYAPR02MB4407.namprd02.prod.outlook.com> (raw)
In-Reply-To: <20201006222031.GA809209@xaphan>



> -----Original Message-----
> From: Michael Auchter <michael.auchter@ni.com>
> Sent: Tuesday, October 6, 2020 3:21 PM
> To: Ben Levinsky <BLEVINSK@xilinx.com>
> Cc: Ed T. Mooring <emooring@xilinx.com>; 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: RE: [PATCH v18 5/5] remoteproc: Add initial zynqmp R5
> remoteproc driver
> 
> On Tue, Oct 06, 2020 at 09:46:38PM +0000, Ben Levinsky wrote:
> >
> >
> > > -----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.
> 
> I see how this works, but it feels a bit weird to me to be overriding
> the remoteproc dev's of_node ptr. Personally I find the
> devm_of_platform_populate() approach a bit less confusing.
> 
> But, it's also not my call to make ;). Perhaps a remoteproc maintainer
> can chime in here.
> 
Fair enough. The way I see this, there is still a need for a struct device* in the zynqmp R5 rproc structure.

If we look at the TI K3 R5 remoteproc patch, https://patchwork.kernel.org/patch/11763795/ ,
there is a call to devm_of_platform_populate in k3_r5_probe similar to your suggestion. I can look to emulate it similarly in the Xilinx remoteproc driver.

Even still there is a usage of the struct device * in the TI K3 R5 remoteproc structure for the same reason of setting up mailbox for each core as detailed below (can be found in the same link I posted):


** here is where the device* is stored at probe
- k3_r5_probe calls k3_r5_cluster_rproc_init 

static int k3_r5_cluster_rproc_init(struct platform_device *pdev) {
    struct k3_r5_rproc *kproc;
    struct device *cdev;
    ......<snip>...
	core1 = list_last_entry(&cluster->cores, struct k3_r5_core, elem);
	list_for_each_entry(core, &cluster->cores, elem) {
		cdev = core->dev;
                           ......<snip>...
                           kproc->dev = cdev;
...
}


and then back in the mailbox initialization:
static int k3_r5_rproc_start(struct rproc *rproc) {
    ...
    struct k3_r5_rproc *kproc = rproc->priv;
    struct device *dev = kproc->dev;
    ...
    client->dev = dev; <--- this is needed when requesting the mailbox
This needs to be device with the corresponding of_node that has the mbox-related properties in it




This is the example I based my usage of the struct device* field in the Soc Specific remoteproc structure off of.
Given that I can still proceed and update with the other suggestions?

Thanks
Ben

> >
> >
> > 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

  reply	other threads:[~2020-10-07 14:31 UTC|newest]

Thread overview: 20+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-10-05 16:06 [PATCH v18 0/5] Provide basic driver to control Arm R5 co-processor found on Xilinx ZynqMP Ben Levinsky
2020-10-05 16:06 ` [PATCH v18 1/5] firmware: xilinx: Add ZynqMP firmware ioctl enums for RPU configuration Ben Levinsky
2020-10-05 16:06 ` [PATCH v18 2/5] firmware: xilinx: Add shutdown/wakeup APIs Ben Levinsky
2020-10-05 16:06 ` [PATCH v18 3/5] firmware: xilinx: Add RPU configuration APIs Ben Levinsky
2020-10-05 16:06 ` [PATCH v18 4/5] dt-bindings: remoteproc: Add documentation for ZynqMP R5 rproc bindings Ben Levinsky
2020-10-08 12:37   ` Linus Walleij
2020-10-08 14:21     ` Ben Levinsky
2020-10-08 16:45       ` Ben Levinsky
2020-10-08 20:22       ` Stefano Stabellini
2020-10-08 20:54       ` Linus Walleij
2020-10-05 16:06 ` [PATCH v18 5/5] remoteproc: Add initial zynqmp R5 remoteproc driver Ben Levinsky
2020-10-05 19:34   ` Michael Auchter
2020-10-06 19:15     ` Ben Levinsky
2020-10-06 21:31       ` Michael Auchter
2020-10-06 21:46         ` Ben Levinsky
2020-10-06 22:20           ` Michael Auchter
2020-10-07 14:31             ` Ben Levinsky [this message]
2020-10-15 18:31             ` Ben Levinsky
2020-10-19 20:43   ` Stefano Stabellini
2020-10-19 21:33     ` Ben Levinsky

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=BYAPR02MB44071E3F9D431CBC72920E4AB50A0@BYAPR02MB4407.namprd02.prod.outlook.com \
    --to=blevinsk@xilinx.com \
    --cc=devicetree@vger.kernel.org \
    --cc=emooring@xilinx.com \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-remoteproc@vger.kernel.org \
    --cc=mathieu.poirier@linaro.org \
    --cc=michael.auchter@ni.com \
    --cc=michals@xilinx.com \
    --cc=robh+dt@kernel.org \
    --cc=stefanos@xilinx.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).