* [PATCH] remoteproc: xlnx: avoid RPU force power down @ 2025-04-14 18:46 Tanmay Shah 2025-04-22 15:59 ` Mathieu Poirier 0 siblings, 1 reply; 7+ messages in thread From: Tanmay Shah @ 2025-04-14 18:46 UTC (permalink / raw) To: andersson, mathieu.poirier; +Cc: linux-remoteproc, linux-kernel, Tanmay Shah 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 <tanmay.shah@amd.com> --- 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; + } + + /* 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 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] remoteproc: xlnx: avoid RPU force power down 2025-04-14 18:46 [PATCH] remoteproc: xlnx: avoid RPU force power down Tanmay Shah @ 2025-04-22 15:59 ` Mathieu Poirier 2025-04-22 16:10 ` Tanmay Shah 0 siblings, 1 reply; 7+ messages in thread From: Mathieu Poirier @ 2025-04-22 15:59 UTC (permalink / raw) To: Tanmay Shah; +Cc: andersson, linux-remoteproc, linux-kernel 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 <tanmay.shah@amd.com> > --- > 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. 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 > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] remoteproc: xlnx: avoid RPU force power down 2025-04-22 15:59 ` Mathieu Poirier @ 2025-04-22 16:10 ` Tanmay Shah 2025-04-22 17:49 ` Mathieu Poirier 0 siblings, 1 reply; 7+ messages in thread From: Tanmay Shah @ 2025-04-22 16:10 UTC (permalink / raw) To: Mathieu Poirier; +Cc: andersson, linux-remoteproc, linux-kernel 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 <tanmay.shah@amd.com> >> --- >> 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. } 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 >> ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] remoteproc: xlnx: avoid RPU force power down 2025-04-22 16:10 ` Tanmay Shah @ 2025-04-22 17:49 ` Mathieu Poirier 2025-04-22 18:30 ` Tanmay Shah 0 siblings, 1 reply; 7+ messages in thread From: Mathieu Poirier @ 2025-04-22 17:49 UTC (permalink / raw) To: tanmay.shah; +Cc: andersson, linux-remoteproc, linux-kernel On Tue, 22 Apr 2025 at 10:10, Tanmay Shah <tanmay.shah@amd.com> 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 <tanmay.shah@amd.com> > >> --- > >> 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 > >> > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] remoteproc: xlnx: avoid RPU force power down 2025-04-22 17:49 ` Mathieu Poirier @ 2025-04-22 18:30 ` Tanmay Shah 2025-04-22 19:10 ` Mathieu Poirier 0 siblings, 1 reply; 7+ messages in thread From: Tanmay Shah @ 2025-04-22 18:30 UTC (permalink / raw) To: Mathieu Poirier; +Cc: andersson, linux-remoteproc, linux-kernel On 4/22/25 12:49 PM, Mathieu Poirier wrote: > On Tue, 22 Apr 2025 at 10:10, Tanmay Shah <tanmay.shah@amd.com> 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 <tanmay.shah@amd.com> >>>> --- >>>> 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. > PM_FORCE_POWERDOWN will print redundant error messages from firmware if called for feature that is not supported. By doing above version check, we are avoiding those unnecessary error/warning messages. Other than that, you are correct we don't need to do version check as PM_FORCE_POWERDOWN will send respective error code and we will fail here. But version check helps to differentiate between actual error log from firmware when call is expected to work. >> 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 >>>> >> ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] remoteproc: xlnx: avoid RPU force power down 2025-04-22 18:30 ` Tanmay Shah @ 2025-04-22 19:10 ` Mathieu Poirier 2025-04-22 19:21 ` Tanmay Shah 0 siblings, 1 reply; 7+ messages in thread From: Mathieu Poirier @ 2025-04-22 19:10 UTC (permalink / raw) To: tanmay.shah; +Cc: andersson, linux-remoteproc, linux-kernel On Tue, 22 Apr 2025 at 12:30, Tanmay Shah <tanmay.shah@amd.com> wrote: > > > > On 4/22/25 12:49 PM, Mathieu Poirier wrote: > > On Tue, 22 Apr 2025 at 10:10, Tanmay Shah <tanmay.shah@amd.com> 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 <tanmay.shah@amd.com> > >>>> --- > >>>> 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. > > > > PM_FORCE_POWERDOWN will print redundant error messages from firmware if > called for feature that is not supported. By doing above version check, > we are avoiding those unnecessary error/warning messages. Other than > that, you are correct we don't need to do version check as > PM_FORCE_POWERDOWN will send respective error code and we will fail > here. But version check helps to differentiate between actual error log > from firmware when call is expected to work. > That is the kind of information that would be useful as comments in the code. Otherwise there is simply no way to tell... > >> 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 > >>>> > >> > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] remoteproc: xlnx: avoid RPU force power down 2025-04-22 19:10 ` Mathieu Poirier @ 2025-04-22 19:21 ` Tanmay Shah 0 siblings, 0 replies; 7+ messages in thread From: Tanmay Shah @ 2025-04-22 19:21 UTC (permalink / raw) To: Mathieu Poirier; +Cc: andersson, linux-remoteproc, linux-kernel On 4/22/25 2:10 PM, Mathieu Poirier wrote: > On Tue, 22 Apr 2025 at 12:30, Tanmay Shah <tanmay.shah@amd.com> wrote: >> >> >> >> On 4/22/25 12:49 PM, Mathieu Poirier wrote: >>> On Tue, 22 Apr 2025 at 10:10, Tanmay Shah <tanmay.shah@amd.com> 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 <tanmay.shah@amd.com> >>>>>> --- >>>>>> 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. >>> >> >> PM_FORCE_POWERDOWN will print redundant error messages from firmware if >> called for feature that is not supported. By doing above version check, >> we are avoiding those unnecessary error/warning messages. Other than >> that, you are correct we don't need to do version check as >> PM_FORCE_POWERDOWN will send respective error code and we will fail >> here. But version check helps to differentiate between actual error log >> from firmware when call is expected to work. >> > > That is the kind of information that would be useful as comments in > the code. Otherwise there is simply no way to tell... > Yes that makes sense. I will update comment accordingly. >>>> 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 >>>>>> >>>> >> ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2025-04-22 19:21 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2025-04-14 18:46 [PATCH] remoteproc: xlnx: avoid RPU force power down Tanmay Shah 2025-04-22 15:59 ` Mathieu Poirier 2025-04-22 16:10 ` Tanmay Shah 2025-04-22 17:49 ` Mathieu Poirier 2025-04-22 18:30 ` Tanmay Shah 2025-04-22 19:10 ` Mathieu Poirier 2025-04-22 19:21 ` Tanmay Shah
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.