On Tue, 22 Apr 2025 at 10:10, Tanmay Shah <[email protected]> wrote: > > > > On 4/22/25 10:59 AM, Mathieu Poirier wrote: > > Good morning, > > > > On Mon, Apr 14, 2025 at 11:46:01AM -0700, Tanmay Shah wrote: > >> Powering off RPU using force_pwrdwn call results in system failure > >> if there are multiple users of that RPU node. Better mechanism is to use > >> request_node and release_node EEMI calls. With use of these EEMI calls, > >> platform management controller will take-care of powering off RPU > >> when there is no user. > >> > >> Signed-off-by: Tanmay Shah <[email protected]> > >> --- > >> drivers/remoteproc/xlnx_r5_remoteproc.c | 29 ++++++++++++++++++++++++- > >> 1 file changed, 28 insertions(+), 1 deletion(-) > >> > >> diff --git a/drivers/remoteproc/xlnx_r5_remoteproc.c > >> b/drivers/remoteproc/xlnx_r5_remoteproc.c > >> index 5aeedeaf3c41..3597359c0fc8 100644 > >> --- a/drivers/remoteproc/xlnx_r5_remoteproc.c > >> +++ b/drivers/remoteproc/xlnx_r5_remoteproc.c > >> @@ -380,6 +380,18 @@ static int zynqmp_r5_rproc_start(struct rproc *rproc) > >> dev_dbg(r5_core->dev, "RPU boot addr 0x%llx from %s.", > >> rproc->bootaddr, > >> bootmem == PM_RPU_BOOTMEM_HIVEC ? "OCM" : "TCM"); > >> > >> + /* Request node before starting RPU core if new version of API is > >> supported */ > >> + if (zynqmp_pm_feature(PM_REQUEST_NODE) > 1) { > >> + ret = zynqmp_pm_request_node(r5_core->pm_domain_id, > >> + ZYNQMP_PM_CAPABILITY_ACCESS, 0, > >> + ZYNQMP_PM_REQUEST_ACK_BLOCKING); > >> + if (ret < 0) { > >> + dev_err(r5_core->dev, "failed to request 0x%x", > >> + r5_core->pm_domain_id); > >> + return ret; > >> + } > >> + } > >> + > >> ret = zynqmp_pm_request_wake(r5_core->pm_domain_id, 1, > >> bootmem, ZYNQMP_PM_REQUEST_ACK_NO); > >> if (ret) > >> @@ -401,10 +413,25 @@ static int zynqmp_r5_rproc_stop(struct rproc *rproc) > >> struct zynqmp_r5_core *r5_core = rproc->priv; > >> int ret; > >> > >> + /* Use release node API to stop core if new version of API is > >> supported */ > >> + if (zynqmp_pm_feature(PM_RELEASE_NODE) > 1) { > >> + ret = zynqmp_pm_release_node(r5_core->pm_domain_id); > >> + if (ret) > >> + dev_err(r5_core->dev, "failed to stop remoteproc RPU > >> %d\n", ret); > >> + return ret; > >> + } > >> + > >> + if (zynqmp_pm_feature(PM_FORCE_POWERDOWN) < 1) { > >> + dev_dbg(r5_core->dev, "EEMI interface %d not supported\n", > >> + PM_FORCE_POWERDOWN); > >> + return -EOPNOTSUPP; > >> + } > > > > Here I have to guess, because it is not documented, that it is the check to > > see > > if zynqmp_pm_force_pwrdwn() is available. I'm not sure why it is needed > > because > > zynqmp_pm_force_pwrdwn() returns and error code. > > > Hello, > > Thanks for reviews. Yes you are correct. Actually instead, the check > should be for version 1 of PM_FORCE_POWER_DOWN. If version 1 is > supported, only then execute the call otherwise print the error. > Hence, the check should be something like: > > if (zynqmp_pm_feature(PM_FORCE_POWERDOWN) != 1) { > error out. > } >
The above still doesn't answer my question, i.e _why_ is a check needed when zynqmp_pm_force_pwrdwn() returns an error code? To me, if something happens in zynqmp_pm_force_pwrdwn() then an error code is reported and the current implementation is able to deal with it. > I will fix and add comment as well. > > > Thanks, > > Mathieu > > > >> + > >> + /* maintain force pwr down for backward compatibility */ > >> ret = zynqmp_pm_force_pwrdwn(r5_core->pm_domain_id, > >> ZYNQMP_PM_REQUEST_ACK_BLOCKING); > >> if (ret) > >> - dev_err(r5_core->dev, "failed to stop remoteproc RPU %d\n", > >> ret); > >> + dev_err(r5_core->dev, "core force power down failed\n"); > >> > >> return ret; > >> } > >> > >> base-commit: 8532691d0a85ab2a826808207e904f7d62a9d804 > >> -- > >> 2.34.1 > >> >
