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