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:     Michael Auchter <michael.auchter@ni.com>
CC:     "devicetree@vger.kernel.org" <devicetree@vger.kernel.org>,
        "linux-remoteproc@vger.kernel.org" <linux-remoteproc@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "linux-arm-kernel@lists.infradead.org" 
        <linux-arm-kernel@lists.infradead.org>
Subject: RE: [PATCH v14 5/5] remoteproc: Add initial zynqmp R5 remoteproc
 driver
Thread-Topic: [PATCH v14 5/5] remoteproc: Add initial zynqmp R5 remoteproc
 driver
Thread-Index: AQHWjSzx2v36zrUuJk25DA+tFuTaOqltZJcAgAAAbzCAAAnbcA==
Date:   Thu, 17 Sep 2020 22:50:42 +0000
Message-ID: <BYAPR02MB4407A552ECBA907DFC3CEC91B53E0@BYAPR02MB4407.namprd02.prod.outlook.com>
References: <20200917194341.16272-1-ben.levinsky@xilinx.com>
 <20200917194341.16272-6-ben.levinsky@xilinx.com>
 <20200917221120.GA15530@xaphan>
 <BYAPR02MB44073E7A3BEA401FF4684E95B53E0@BYAPR02MB4407.namprd02.prod.outlook.com>
In-Reply-To: <BYAPR02MB44073E7A3BEA401FF4684E95B53E0@BYAPR02MB4407.namprd02.prod.outlook.com>
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: 3440f0b9-3af2-4945-7718-08d85b5c1b31
X-MS-Exchange-CrossTenant-originalarrivaltime: 17 Sep 2020 22:50:42.3041
 (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: zO6pj6LHN8Tj7+/keujHQwh5l2EzEuYZrkiR0Q/Dn3Tz1Ywr2MpF3maWApQyZe0a+Vnuu4VPcJrNZCGu6MaHSA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BYAPR02MB5127
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

> -----Original Message-----
> From: Ben Levinsky <BLEVINSK@xilinx.com>
> Sent: Thursday, September 17, 2020 3:19 PM
> To: Michael Auchter <michael.auchter@ni.com>
> Cc: 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; Jiaying Liang
> <jliang@xilinx.com>; robh+dt@kernel.org; linux-arm-
> kernel@lists.infradead.org; Jiaying Liang <jliang@xilinx.com>; Michal Sim=
ek
> <michals@xilinx.com>; Ed T. Mooring <emooring@xilinx.com>; Jason Wu
> <j.wu@xilinx.com>
> Subject: RE: [PATCH v14 5/5] remoteproc: Add initial zynqmp R5 remoteproc
> driver
>=20
> Hi Michael,
>=20
> Thanks for the comments,
>=20
> > -----Original Message-----
> > From: Michael Auchter <michael.auchter@ni.com>
> > Sent: Thursday, September 17, 2020 3:11 PM
> > To: Ben Levinsky <BLEVINSK@xilinx.com>
> > Cc: punit1.agrawal@toshiba.co.jp; Stefano Stabellini <stefanos@xilinx.c=
om>;
> > 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; Jiaying Liang
> > <jliang@xilinx.com>; robh+dt@kernel.org; linux-arm-
> > kernel@lists.infradead.org; Jiaying Liang <jliang@xilinx.com>; Michal S=
imek
> > <michals@xilinx.com>; Ed T. Mooring <emooring@xilinx.com>; Jason Wu
> > <j.wu@xilinx.com>
> > Subject: Re: [PATCH v14 5/5] remoteproc: Add initial zynqmp R5 remotepr=
oc
> > driver
> >
> > Hey Ben,
> >
> > Split mode is still not functional in this patch series (as was the cas=
e
> > with the last few revisions).
> >
> > Before sending out the next revision, can you _please_ ensure you're
> > testing all supported configurations?
> >
> [Ben Levinsky]  I will make sure to update in next revision.
>  As per review, I tested on QEMU and hardware firmware loading in split
> mode on R5 0 split, R5 1 split and R5 lockstep and  was  able to successf=
ully
> load, start and establish IPC links
>=20
> That being said, I will update the to reflect the values between the enum=
 for
> rpu operation mode and the documentation in the binding.
>=20
> For testing, I can provide a pointer to a publicly available device tree =
I am
> using if that helps. If not, can you expand on the testing of supported
> configurations?
>=20
In addition to device tree, is there particular linker script you use for y=
our R5 application? For example with OCM? As presently this driver only has=
 DDR and TCM as supported regions to load into=20
And I planned to add OCM support if/when the driver gets in.

Thanks,
Ben
> > On Thu, Sep 17, 2020 at 12:43:41PM -0700, Ben Levinsky wrote:
> > > +/**
> > > + * RPU core configuration
> > > + */
> > > +static enum rpu_oper_mode rpu_mode;
> > > +
> >
> > <.. snip ..>
> >
> > > +static int zynqmp_r5_remoteproc_probe(struct platform_device *pdev)
> > > +{
> > > +	int ret, i =3D 0;
> > > +	u32 lockstep_mode;
> > > +	struct device *dev =3D &pdev->dev;
> > > +	struct device_node *nc;
> > > +
> > > +	ret =3D of_property_read_u32(dev->of_node,
> > > +				   "lockstep-mode",
> > > +				   &lockstep_mode);
> > > +	if (ret < 0) {
> > > +		return ret;
> > > +	} else if (lockstep_mode !=3D PM_RPU_MODE_LOCKSTEP &&
> > > +	    lockstep_mode !=3D PM_RPU_MODE_SPLIT) {
> > > +		dev_err(dev,
> > > +			"Invalid lockstep-mode %x in %pOF\n",
> > > +			lockstep_mode, dev->of_node);
> > > +		return -EINVAL;
> > > +	}
> > > +
> > > +	rpu_mode =3D lockstep_mode;
> > > +
> > > +	dev_dbg(dev, "RPU configuration: %s\n",
> > > +		lockstep_mode ? "lockstep" : "split");
> >
> > The binding documents lockstep-mode as:
> >
> > > +  lockstep-mode:
> > > +    description:
> > > +      R5 core configuration (split is 0 or lock-step and 1)
> > > +    maxItems: 1
> >
> will update this as you note so that lockstep and split mode are accurate=
ly
> reflected.
>=20
> > (Which needs to be reworded, but it looks like the intent was "split is
> > 0 and lock-step is 1")
> >
> > However, rpu_oper_mode is defined as:
> >
> > > +enum rpu_oper_mode {
> > > +       PM_RPU_MODE_LOCKSTEP =3D 0,
> > > +       PM_RPU_MODE_SPLIT =3D 1,
> > > +};
> >
> > so the assignment "rpu_mode =3D lockstep_mode" is incorrect.
> >
> once the binding is updated, why would this still be incorrect? Assuming =
the
> documentation is updated, the above line would be ok, right?
>=20
> Thank you for the review
> Ben
>=20
> > - Michael