Hi Michael,

Thanks for the comments,

-----Original Message-----
From: Michael Auchter <redacted>
Sent: Thursday, September 17, 2020 3:11 PM
To: Ben Levinsky <redacted>
Cc: punit1.agrawal@toshiba.co.jp; Stefano Stabellini <redacted>;
Michal Simek [off-list ref]; devicetree@vger.kernel.org;
mathieu.poirier@linaro.org; Ed T. Mooring [off-list ref]; linux-
remoteproc@vger.kernel.org; linux-kernel@vger.kernel.org; Jiaying Liang
[off-list ref]; robh+dt@kernel.org; linux-arm-
kernel@lists.infradead.org; Jiaying Liang [off-list ref]; Michal Simek
[off-list ref]; Ed T. Mooring [off-list ref]; Jason Wu
[off-list ref]
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:

quoted

+/**
+ * RPU core configuration
+ */
+static enum rpu_oper_mode rpu_mode;
+

<.. snip ..>

quoted

+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:

quoted

+  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:

quoted

+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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel