drivers/remoteproc/xlnx_r5_remoteproc.c | 29 ++++++++++++++++++++++++- 1 file changed, 28 insertions(+), 1 deletion(-)
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.1Good 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
>
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
>>
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
> >>
>
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
>>>>
>>
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
> >>>>
> >>
>
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
>>>>>>
>>>>
>>
© 2016 - 2026 Red Hat, Inc.