RE: [PATCH v14 5/5] remoteproc: Add initial zynqmp R5 remoteproc driver

[Ben Levinsky]

> -----Original Message-----
> From: Ben Levinsky <BLEVINSK@xxxxxxxxxx>
> Sent: Thursday, September 17, 2020 3:19 PM
> To: Michael Auchter <michael.auchter@xxxxxx>
> Cc: punit1.agrawal@xxxxxxxxxxxxx; Stefano Stabellini <stefanos@xxxxxxxxxx>;
> Michal Simek <michals@xxxxxxxxxx>; devicetree@xxxxxxxxxxxxxxx;
> mathieu.poirier@xxxxxxxxxx; Ed T. Mooring <emooring@xxxxxxxxxx>; linux-
> remoteproc@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; Jiaying Liang
> <jliang@xxxxxxxxxx>; robh+dt@xxxxxxxxxx; linux-arm-
> kernel@xxxxxxxxxxxxxxxxxxx; Jiaying Liang <jliang@xxxxxxxxxx>; Michal Simek
> <michals@xxxxxxxxxx>; Ed T. Mooring <emooring@xxxxxxxxxx>; Jason Wu
> <j.wu@xxxxxxxxxx>
> Subject: RE: [PATCH v14 5/5] remoteproc: Add initial zynqmp R5 remoteproc
> driver
> 
> Hi Michael,
> 
> Thanks for the comments,
> 
> > -----Original Message-----
> > From: Michael Auchter <michael.auchter@xxxxxx>
> > Sent: Thursday, September 17, 2020 3:11 PM
> > To: Ben Levinsky <BLEVINSK@xxxxxxxxxx>
> > Cc: punit1.agrawal@xxxxxxxxxxxxx; Stefano Stabellini <stefanos@xxxxxxxxxx>;
> > Michal Simek <michals@xxxxxxxxxx>; devicetree@xxxxxxxxxxxxxxx;
> > mathieu.poirier@xxxxxxxxxx; Ed T. Mooring <emooring@xxxxxxxxxx>; linux-
> > remoteproc@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; Jiaying Liang
> > <jliang@xxxxxxxxxx>; robh+dt@xxxxxxxxxx; linux-arm-
> > kernel@xxxxxxxxxxxxxxxxxxx; Jiaying Liang <jliang@xxxxxxxxxx>; Michal Simek
> > <michals@xxxxxxxxxx>; Ed T. Mooring <emooring@xxxxxxxxxx>; Jason Wu
> > <j.wu@xxxxxxxxxx>
> > Subject: Re: [PATCH v14 5/5] remoteproc: Add initial zynqmp R5 remoteproc
> > driver
> >
> > Hey Ben,
> >
> > Split mode is still not functional in this patch series (as was the case
> > 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 successfully
> load, start and establish IPC links
> 
> That being said, I will update the to reflect the values between the enum for
> rpu operation mode and the documentation in the binding.
> 
> 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?
> 
In addition to device tree, is there particular linker script you use for your R5 application? For example with OCM? As presently this driver only has DDR and TCM as supported regions to load into 
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 = 0;
> > > +	u32 lockstep_mode;
> > > +	struct device *dev = &pdev->dev;
> > > +	struct device_node *nc;
> > > +
> > > +	ret = of_property_read_u32(dev->of_node,
> > > +				   "lockstep-mode",
> > > +				   &lockstep_mode);
> > > +	if (ret < 0) {
> > > +		return ret;
> > > +	} else if (lockstep_mode != PM_RPU_MODE_LOCKSTEP &&
> > > +	    lockstep_mode != PM_RPU_MODE_SPLIT) {
> > > +		dev_err(dev,
> > > +			"Invalid lockstep-mode %x in %pOF\n",
> > > +			lockstep_mode, dev->of_node);
> > > +		return -EINVAL;
> > > +	}
> > > +
> > > +	rpu_mode = 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 accurately
> reflected.
> 
> > (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 = 0,
> > > +       PM_RPU_MODE_SPLIT = 1,
> > > +};
> >
> > so the assignment "rpu_mode = 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?
> 
> Thank you for the review
> Ben
> 
> > - Michael