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