From: Michael Auchter <michael.auchter@ni.com> To: Ben Levinsky <BLEVINSK@xilinx.com> Cc: "punit1.agrawal@toshiba.co.jp" <punit1.agrawal@toshiba.co.jp>, Stefano Stabellini <stefanos@xilinx.com>, Michal Simek <michals@xilinx.com>, "devicetree@vger.kernel.org" <devicetree@vger.kernel.org>, "mathieu.poirier@linaro.org" <mathieu.poirier@linaro.org>, "Ed T. Mooring" <emooring@xilinx.com>, "linux-remoteproc@vger.kernel.org" <linux-remoteproc@vger.kernel.org>, "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>, Jiaying Liang <jliang@xilinx.com>, "robh+dt@kernel.org" <robh+dt@kernel.org>, "linux-arm-kernel@lists.infradead.org" <linux-arm-kernel@lists.infradead.org>, Jason Wu <j.wu@xilinx.com> Subject: Re: RE: [PATCH v14 5/5] remoteproc: Add initial zynqmp R5 remoteproc driver Date: Fri, 18 Sep 2020 11:04:19 -0500 [thread overview] Message-ID: <20200918160419.GC15530@xaphan> (raw) In-Reply-To: <BYAPR02MB44073E7A3BEA401FF4684E95B53E0@BYAPR02MB4407.namprd02.prod.outlook.com> 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
WARNING: multiple messages have this Message-ID (diff)
From: Michael Auchter <michael.auchter@ni.com> To: Ben Levinsky <BLEVINSK@xilinx.com> Cc: Stefano Stabellini <stefanos@xilinx.com>, "Ed T. Mooring" <emooring@xilinx.com>, "punit1.agrawal@toshiba.co.jp" <punit1.agrawal@toshiba.co.jp>, "mathieu.poirier@linaro.org" <mathieu.poirier@linaro.org>, "devicetree@vger.kernel.org" <devicetree@vger.kernel.org>, "linux-remoteproc@vger.kernel.org" <linux-remoteproc@vger.kernel.org>, "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>, Jiaying Liang <jliang@xilinx.com>, "robh+dt@kernel.org" <robh+dt@kernel.org>, Michal Simek <michals@xilinx.com>, Jason Wu <j.wu@xilinx.com>, "linux-arm-kernel@lists.infradead.org" <linux-arm-kernel@lists.infradead.org> Subject: Re: RE: [PATCH v14 5/5] remoteproc: Add initial zynqmp R5 remoteproc driver Date: Fri, 18 Sep 2020 11:04:19 -0500 [thread overview] Message-ID: <20200918160419.GC15530@xaphan> (raw) In-Reply-To: <BYAPR02MB44073E7A3BEA401FF4684E95B53E0@BYAPR02MB4407.namprd02.prod.outlook.com> 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 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
next prev parent reply other threads:[~2020-09-18 16:30 UTC|newest] Thread overview: 38+ messages / expand[flat|nested] mbox.gz Atom feed top 2020-09-17 19:43 [PATCH v14 0/5] Provide basic driver to control Arm R5 co-processor found on Xilinx ZynqMP Ben Levinsky 2020-09-17 19:43 ` Ben Levinsky 2020-09-17 19:43 ` [PATCH v14 1/5] firmware: xilinx: Add ZynqMP firmware ioctl enums for RPU configuration Ben Levinsky 2020-09-17 19:43 ` Ben Levinsky 2020-09-17 19:43 ` [PATCH v14 2/5] firmware: xilinx: Add shutdown/wakeup APIs Ben Levinsky 2020-09-17 19:43 ` Ben Levinsky 2020-09-17 19:43 ` [PATCH v14 3/5] firmware: xilinx: Add RPU configuration APIs Ben Levinsky 2020-09-17 19:43 ` Ben Levinsky 2020-09-17 19:43 ` [PATCH v14 4/5] dt-bindings: remoteproc: Add documentation for ZynqMP R5 rproc bindings Ben Levinsky 2020-09-17 19:43 ` Ben Levinsky 2020-09-18 6:28 ` Punit Agrawal 2020-09-18 6:28 ` Punit Agrawal 2020-09-17 19:43 ` [PATCH v14 5/5] remoteproc: Add initial zynqmp R5 remoteproc driver Ben Levinsky 2020-09-17 19:43 ` Ben Levinsky 2020-09-17 22:11 ` Michael Auchter 2020-09-17 22:11 ` Michael Auchter 2020-09-17 22:18 ` Ben Levinsky 2020-09-17 22:18 ` Ben Levinsky 2020-09-17 22:50 ` Ben Levinsky 2020-09-17 22:50 ` Ben Levinsky 2020-09-18 16:07 ` Michael Auchter 2020-09-18 16:07 ` Michael Auchter 2020-09-18 18:01 ` Ben Levinsky 2020-09-18 18:01 ` Ben Levinsky 2020-09-18 19:06 ` Michael Auchter 2020-09-18 19:06 ` Michael Auchter 2020-09-19 1:53 ` Wendy Liang 2020-09-19 1:53 ` Wendy Liang 2020-09-20 23:16 ` Ben Levinsky 2020-09-20 23:16 ` Ben Levinsky 2020-09-21 5:09 ` Wendy Liang 2020-09-21 5:09 ` Wendy Liang 2020-09-21 5:11 ` Wendy Liang 2020-09-21 5:11 ` Wendy Liang 2020-09-21 10:04 ` Punit Agrawal 2020-09-21 10:04 ` Punit Agrawal 2020-09-18 16:04 ` Michael Auchter [this message] 2020-09-18 16:04 ` Michael Auchter
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20200918160419.GC15530@xaphan \ --to=michael.auchter@ni.com \ --cc=BLEVINSK@xilinx.com \ --cc=devicetree@vger.kernel.org \ --cc=emooring@xilinx.com \ --cc=j.wu@xilinx.com \ --cc=jliang@xilinx.com \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-remoteproc@vger.kernel.org \ --cc=mathieu.poirier@linaro.org \ --cc=michals@xilinx.com \ --cc=punit1.agrawal@toshiba.co.jp \ --cc=robh+dt@kernel.org \ --cc=stefanos@xilinx.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.