On Thu, Sep 17, 2020 at 10:18:39PM +0000, Ben Levinsky wrote:
> 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?
I'm testing exclusively split mode configuration. I load and run
firmware on R5 0, and then do the same on R5 1.
Given the logic error, I admit that I'm confused how this could have
worked in your tests, unless the device tree you used to test split mode
contained "lockstep-mode = <1>", and the lockstep device tree contained
"lockstep-mode = <0>".
But if that was the case, then that means the device trees used for
testing changed this property's value between v13 and v14, for seemingly
no reason.
> > 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?
It might not be incorrect, depending on how you change the binding.
If you update the binding documentation to say "lockstep-mode: 0 is
lockstep, 1 is split", then this line would be fine. However, that would
seem strange to me, as this reads like a boolean: setting this to 0
would logically indicate that the device is not configured in lockstep
mode.
I don't think this is what you were proposing, but I'm not sure.
v13 did this correctly, and lockstep-mode == 0 implied split mode:
of_property_read_u32(dev->of_node, "lockstep-mode", &lockstep_mode);
if (!lockstep_mode) {
rpu_mode = PM_RPU_MODE_SPLIT;
} else if (lockstep_mode == 1) {
rpu_mode = PM_RPU_MODE_LOCKSTEP;
}
Changing this is what broke v14.
>
> Thank you for the review
> Ben
>
> > - Michael
