Hey Bjorn,
Thanks for taking time to review the series.
On 7/1/22 12:58 AM, Bjorn Andersson wrote:
On Fri 20 May 02:28 CDT 2022, Sibi Sankar wrote:
The initial shutdown request to modem on SM8450 SoCs would start the
decryption process and will keep returning errors until the modem shutdown
is complete. Fix this by retrying shutdowns in fixed intervals.
Err Logs on modem shutdown:
qcom_q6v5_pas 4080000.remoteproc: failed to shutdown: -22
remoteproc remoteproc3: can't stop rproc: -22
Fixes: 5cef9b48458d ("remoteproc: qcom: pas: Add SM8450 remoteproc support")
Signed-off-by: Sibi Sankar <quic_sibis@xxxxxxxxxxx>
Looks reasonable, just two inquiries below.
---
drivers/remoteproc/qcom_q6v5_pas.c | 67 +++++++++++++++++++++++++++++++++++++-
1 file changed, 66 insertions(+), 1 deletion(-)
diff --git a/drivers/remoteproc/qcom_q6v5_pas.c b/drivers/remoteproc/qcom_q6v5_pas.c
index 6ae39c5653b1..d04c4b877e12 100644
--- a/drivers/remoteproc/qcom_q6v5_pas.c
+++ b/drivers/remoteproc/qcom_q6v5_pas.c
@@ -8,6 +8,7 @@
*/
#include <linux/clk.h>
+#include <linux/delay.h>
#include <linux/firmware.h>
#include <linux/interrupt.h>
#include <linux/kernel.h>
@@ -29,6 +30,8 @@
#include "qcom_q6v5.h"
#include "remoteproc_internal.h"
+#define ADSP_DECRYPT_SHUTDOWN_DELAY_MS 100
+
struct adsp_data {
int crash_reason_smem;
const char *firmware_name;
@@ -36,6 +39,7 @@ struct adsp_data {
unsigned int minidump_id;
bool has_aggre2_clk;
bool auto_boot;
+ bool decrypt_shutdown;
char **proxy_pd_names;
@@ -65,6 +69,7 @@ struct qcom_adsp {
unsigned int minidump_id;
int crash_reason_smem;
bool has_aggre2_clk;
+ bool decrypt_shutdown;
const char *info_name;
struct completion start_done;
@@ -128,6 +133,20 @@ static void adsp_pds_disable(struct qcom_adsp *adsp, struct device **pds,
}
}
+static int adsp_decrypt_shutdown(struct qcom_adsp *adsp)
+{
+ int retry_num = 50;
Seems unsigned to me.
ack
+ int ret = -EINVAL;
+
+ while (retry_num && ret) {
+ msleep(ADSP_DECRYPT_SHUTDOWN_DELAY_MS);
+ ret = qcom_scm_pas_shutdown(adsp->pas_id);
+ retry_num--;
+ }
Will qcom_scm_pas_shutdown() ever return any other errors than -EINVAL?
Would it make sense to make this:
do {
...;
} while (ret == -EINVAL && --retry_num);
Just checking on ret would cover the -EINVAL case as well but like you
said pas_shutdown() won't return any other error. So I'll just stick
with your suggestion.
+
+ return ret;
+}
+
static int adsp_unprepare(struct rproc *rproc)
{
struct qcom_adsp *adsp = (struct qcom_adsp *)rproc->priv;
@@ -249,6 +268,9 @@ static int adsp_stop(struct rproc *rproc)
dev_err(adsp->dev, "timed out on wait\n");
ret = qcom_scm_pas_shutdown(adsp->pas_id);
+ if (ret && adsp->decrypt_shutdown)
+ ret = adsp_decrypt_shutdown(adsp);
+
if (ret)
dev_err(adsp->dev, "failed to shutdown: %d\n", ret);
@@ -459,6 +481,7 @@ static int adsp_probe(struct platform_device *pdev)
adsp->pas_id = desc->pas_id;
adsp->has_aggre2_clk = desc->has_aggre2_clk;
adsp->info_name = desc->sysmon_name;
+ adsp->decrypt_shutdown = desc->decrypt_shutdown;
platform_set_drvdata(pdev, adsp);
device_wakeup_enable(adsp->dev);
@@ -533,6 +556,7 @@ static const struct adsp_data adsp_resource_init = {
.pas_id = 1,
.has_aggre2_clk = false,
.auto_boot = true,
+ .decrypt_shutdown = false,
With all these booleans, I would prefer if we cleaned it up to not list
the disabled options. That would make it quicker to spot which features
are actually enabled for each remoteproc.
ack and ack to the adsp_shutdown_poll_decrypt() as well.
Regards,
Bjorn
