Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
From:   Ben Levinsky <BLEVINSK@xilinx.com>
To:     "linux-remoteproc@vger.kernel.org" <linux-remoteproc@vger.kernel.org>
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>,
        "michael.auchter@ni.com" <michael.auchter@ni.com>,
        "mathieu.poirier@linaro.org" <mathieu.poirier@linaro.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        Rob Herring <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
Thread-Topic: RE: RE: [PATCH v18 5/5] remoteproc: Add initial zynqmp R5
 remoteproc driver
Thread-Index: AQHWmzGLZdXOwIwjxE6bV2X+UlYToKmJZsqAgAGMKFCAACbWgIAAAZmggAAMCYCADeKZ4A==
Date:   Thu, 15 Oct 2020 18:31:20 +0000
Message-ID: <BYAPR02MB44072A6C1ACAA0C459390895B5020@BYAPR02MB4407.namprd02.prod.outlook.com>
References: <20201005160614.3749-1-ben.levinsky@xilinx.com>
 <20201005160614.3749-6-ben.levinsky@xilinx.com>
 <20201005193449.GA701433@xaphan>
 <BYAPR02MB4407B7F06962DB30ED90761FB50D0@BYAPR02MB4407.namprd02.prod.outlook.com>
 <20201006213143.GD701433@xaphan>
 <BYAPR02MB4407B356B56B9A1D561950B7B50D0@BYAPR02MB4407.namprd02.prod.outlook.com>
 <20201006222031.GA809209@xaphan>
In-Reply-To: <20201006222031.GA809209@xaphan>
Accept-Language: en-US
Content-Language: en-US
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-Network-Message-Id: de157a34-4a40-47ae-c7fb-08d871388393
X-MS-Exchange-CrossTenant-originalarrivaltime: 15 Oct 2020 18:31:21.0044
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 657af505-d5df-48d0-8300-c31994686c5c
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: 9M+XqgW7tPzgBGMxzFhG5jG/fZDIWaQbo+Zg+DTVnEhpQW5JuV6Zf3pb0AKfxwEU2t14z5Ndl2RJ5+qYYKy9MA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BY5PR02MB6420
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

Hi All,

> -----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
>=20
> 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 =3D rproc->priv;
> > > > > > +	z_rproc->dev.release =3D zynqmp_r5_release;
> > > > >
> > > > > This is the only field of z_rproc->dev that's actually initialize=
d, 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 addition=
al
> > > > > 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 devic=
e
> > > > 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 w=
ith
> > > 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 drive=
r
> remove clean up
> > 	- in each core's probe (zynqmp_r5_probe) dothe following:
> >
> >
> >        rproc_ptr =3D rproc_alloc(dev, dev_name(dev), &zynqmp_r5_rproc_o=
ps,
> >                                                   NULL, sizeof(struct z=
ynqmp_r5_rproc));
> >         if (!rproc_ptr)
> >                 return -ENOMEM;
> >         z_rproc =3D rproc_ptr->priv;
> >         z_rproc->dt_node =3D node;
> >         z_rproc->rproc =3D rproc_ptr;
> >         z_rproc->dev =3D &rproc_ptr->dev;
> >         z_rproc->dev->of_node =3D node;
> > where node is the specific R5 core's of_node/ Device tree node.
> >
> > the above preserves most of the mailbox setup code.
>=20
> 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.
>=20
> But, it's also not my call to make ;). Perhaps a remoteproc maintainer
> can chime in here.
>=20
> >

Ping for comments here.

I looked at the TI R5 remoteproc driver and from what I can see, it seems t=
he crux of the line:=20
z_rproc->dev->of_node =3D node;=20
is as follows:

the TI driver only has 1 R5-related remoteproc node. But in this it has inf=
ormation for both cores so
the rproc_alloc's device that is passed in is sufficient for subsequent mai=
lbox calls. This is because the device
here also has a device_node that has the mbox information.

The Xilinx driver differs in that while there is a cluster device tree node=
 that has the remoteproc-related
Information, it ALSO has child R5 cores that have their own TCM bank and mb=
ox information.=20

As a result of this difference the use of devm_of_populate would not remove=
 the use of the line of code in question because the mailbox setup calls la=
ter on still require the device field to have a corresponding device tree n=
ode that
Has the mailbox information.

If it is desired to see the use of devm_of_populate and more close alignmen=
t to the TI driver that has been merged then the Xilinx R5 driver bindings =
can instead have the TCM bank info, memory-regions, meta-memory-regions int=
o R5 core-specific lists which resembles how the TI R5 driver has R5 core-s=
pecific properties. At this point just trying to suss out some direction in=
 this patch series.

Your feedback and review is much appreciated,
Ben


> >
> > With this, I have already successfully done the following in a v19 patc=
h
> > - 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 mainta=
ins
> a linked list of the cores' specific rproc information.
> >
> > Thanks
> > Ben