Messages in this thread | | | Date | Fri, 18 Sep 2020 11:04:19 -0500 | From | Michael Auchter <> | Subject | Re: RE: [PATCH v14 5/5] remoteproc: Add initial zynqmp R5 remoteproc driver |
| |
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@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.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 Simek > > <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 > > > > 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
| |