* [PATCH v2 2/4] remoteproc: qcom: q6v5: Add shutdown-ack irq
2018-12-24 8:48 [PATCH v2 1/4] dt-bindings: remoteproc: qcom: Introduce shutdown-ack irq for Q6V5 Sibi Sankar
@ 2018-12-24 8:48 ` Sibi Sankar
2018-12-24 8:48 ` [PATCH v2 3/4] remoteproc: qcom: Wait for shutdown-ack/ind on sysmon shutdown Sibi Sankar
` (2 subsequent siblings)
3 siblings, 0 replies; 7+ messages in thread
From: Sibi Sankar @ 2018-12-24 8:48 UTC (permalink / raw)
To: bjorn.andersson, robh+dt, andy.gross, david.brown
Cc: linux-arm-msm, linux-soc, devicetree, linux-kernel, tsoni, clew,
akdwived, ohad, mark.rutland, linux-remoteproc, dianders,
Sibi Sankar
Add shutdown-ack irq handling for Q6V5. This patch includes enabling
shutdown-ack irq on those Q6V5 instances with "has_shutdown_irq"
flag set and exposing Q6V5 state information to the sysmon instance
which is required to ascertain graceful shutdown completion.
Signed-off-by: Sibi Sankar <sibis@codeaurora.org>
---
v2:
Move shutdown-irq get to Q6V5 from sysmon to handle
-EPROBE_DEFER cases
Correct the shutdown-irq wait time to 10 * HZ
drivers/remoteproc/qcom_common.h | 7 ++--
drivers/remoteproc/qcom_q6v5.c | 53 +++++++++++++++++++++++++++++
drivers/remoteproc/qcom_q6v5.h | 5 +++
drivers/remoteproc/qcom_q6v5_adsp.c | 3 +-
drivers/remoteproc/qcom_q6v5_mss.c | 3 +-
drivers/remoteproc/qcom_q6v5_pas.c | 3 +-
drivers/remoteproc/qcom_sysmon.c | 6 +++-
drivers/remoteproc/qcom_wcnss.c | 3 +-
8 files changed, 76 insertions(+), 7 deletions(-)
diff --git a/drivers/remoteproc/qcom_common.h b/drivers/remoteproc/qcom_common.h
index 58de71e4781c..d938b09ad02c 100644
--- a/drivers/remoteproc/qcom_common.h
+++ b/drivers/remoteproc/qcom_common.h
@@ -7,6 +7,7 @@
#include <linux/soc/qcom/qmi.h>
struct qcom_sysmon;
+struct qcom_q6v5;
struct qcom_rproc_glink {
struct rproc_subdev subdev;
@@ -45,12 +46,14 @@ void qcom_remove_ssr_subdev(struct rproc *rproc, struct qcom_rproc_ssr *ssr);
#if IS_ENABLED(CONFIG_QCOM_SYSMON)
struct qcom_sysmon *qcom_add_sysmon_subdev(struct rproc *rproc,
const char *name,
- int ssctl_instance);
+ int ssctl_instance,
+ struct qcom_q6v5 *q6v5);
void qcom_remove_sysmon_subdev(struct qcom_sysmon *sysmon);
#else
static inline struct qcom_sysmon *qcom_add_sysmon_subdev(struct rproc *rproc,
const char *name,
- int ssctl_instance)
+ int ssctl_instance,
+ struct qcom_q6v5 *q6v5)
{
return NULL;
}
diff --git a/drivers/remoteproc/qcom_q6v5.c b/drivers/remoteproc/qcom_q6v5.c
index 0d33e3079f0d..a4c2ecae6a0f 100644
--- a/drivers/remoteproc/qcom_q6v5.c
+++ b/drivers/remoteproc/qcom_q6v5.c
@@ -25,6 +25,7 @@ int qcom_q6v5_prepare(struct qcom_q6v5 *q6v5)
{
reinit_completion(&q6v5->start_done);
reinit_completion(&q6v5->stop_done);
+ reinit_completion(&q6v5->shutdown_done);
q6v5->running = true;
q6v5->handover_issued = false;
@@ -141,6 +142,35 @@ static irqreturn_t q6v5_stop_interrupt(int irq, void *data)
return IRQ_HANDLED;
}
+static irqreturn_t q6v5_shutdown_interrupt(int irq, void *data)
+{
+ struct qcom_q6v5 *q6v5 = data;
+
+ complete(&q6v5->shutdown_done);
+
+ return IRQ_HANDLED;
+}
+
+/**
+ * qcom_q6v5_wait_for_shutdown() - wait for remote processor shutdown signal
+ * @q6v5: reference to qcom_q6v5 context
+ * @timeout: timeout to wait for the event, in jiffies
+ *
+ * Return: 0 on success, -ETIMEDOUT on timeout
+ */
+int qcom_q6v5_wait_for_shutdown(struct qcom_q6v5 *q6v5, int timeout)
+{
+ int ret;
+
+ if (!q6v5->has_shutdown_irq)
+ return 0;
+
+ ret = wait_for_completion_timeout(&q6v5->shutdown_done, timeout);
+
+ return !ret ? -ETIMEDOUT : 0;
+}
+EXPORT_SYMBOL_GPL(qcom_q6v5_wait_for_shutdown);
+
/**
* qcom_q6v5_request_stop() - request the remote processor to stop
* @q6v5: reference to qcom_q6v5 context
@@ -185,6 +215,7 @@ int qcom_q6v5_init(struct qcom_q6v5 *q6v5, struct platform_device *pdev,
init_completion(&q6v5->start_done);
init_completion(&q6v5->stop_done);
+ init_completion(&q6v5->shutdown_done);
q6v5->wdog_irq = platform_get_irq_byname(pdev, "wdog");
if (q6v5->wdog_irq < 0) {
@@ -277,6 +308,28 @@ int qcom_q6v5_init(struct qcom_q6v5 *q6v5, struct platform_device *pdev,
return ret;
}
+ if (q6v5->has_shutdown_irq) {
+ q6v5->shutdown_irq = platform_get_irq_byname(pdev,
+ "shutdown-ack");
+ if (q6v5->shutdown_irq < 0) {
+ if (q6v5->shutdown_irq != -EPROBE_DEFER)
+ dev_err(&pdev->dev,
+ "failed to get shutdown-ack IRQ: %d\n",
+ q6v5->shutdown_irq);
+ return q6v5->shutdown_irq;
+ }
+
+ ret = devm_request_threaded_irq(&pdev->dev, q6v5->shutdown_irq,
+ NULL, q6v5_shutdown_interrupt,
+ IRQF_TRIGGER_RISING |
+ IRQF_ONESHOT,
+ "q6v5 shutdown", q6v5);
+ if (ret) {
+ dev_err(&pdev->dev, "failed to acquire shutdown IRQ\n");
+ return ret;
+ }
+ }
+
q6v5->state = qcom_smem_state_get(&pdev->dev, "stop", &q6v5->stop_bit);
if (IS_ERR(q6v5->state)) {
dev_err(&pdev->dev, "failed to acquire stop state\n");
diff --git a/drivers/remoteproc/qcom_q6v5.h b/drivers/remoteproc/qcom_q6v5.h
index 7ac92c1e0f49..5cbaf2564c3a 100644
--- a/drivers/remoteproc/qcom_q6v5.h
+++ b/drivers/remoteproc/qcom_q6v5.h
@@ -21,11 +21,15 @@ struct qcom_q6v5 {
int ready_irq;
int handover_irq;
int stop_irq;
+ int shutdown_irq;
+
+ u8 has_shutdown_irq;
bool handover_issued;
struct completion start_done;
struct completion stop_done;
+ struct completion shutdown_done;
int crash_reason;
@@ -42,5 +46,6 @@ int qcom_q6v5_prepare(struct qcom_q6v5 *q6v5);
int qcom_q6v5_unprepare(struct qcom_q6v5 *q6v5);
int qcom_q6v5_request_stop(struct qcom_q6v5 *q6v5);
int qcom_q6v5_wait_for_start(struct qcom_q6v5 *q6v5, int timeout);
+int qcom_q6v5_wait_for_shutdown(struct qcom_q6v5 *q6v5, int timeout);
#endif
diff --git a/drivers/remoteproc/qcom_q6v5_adsp.c b/drivers/remoteproc/qcom_q6v5_adsp.c
index 79374d1de311..5fc42d38a1cd 100644
--- a/drivers/remoteproc/qcom_q6v5_adsp.c
+++ b/drivers/remoteproc/qcom_q6v5_adsp.c
@@ -438,7 +438,8 @@ static int adsp_probe(struct platform_device *pdev)
qcom_add_ssr_subdev(rproc, &adsp->ssr_subdev, desc->ssr_name);
adsp->sysmon = qcom_add_sysmon_subdev(rproc,
desc->sysmon_name,
- desc->ssctl_id);
+ desc->ssctl_id,
+ &adsp->q6v5);
ret = rproc_add(rproc);
if (ret)
diff --git a/drivers/remoteproc/qcom_q6v5_mss.c b/drivers/remoteproc/qcom_q6v5_mss.c
index 01be7314e176..3bc2dec85928 100644
--- a/drivers/remoteproc/qcom_q6v5_mss.c
+++ b/drivers/remoteproc/qcom_q6v5_mss.c
@@ -1340,7 +1340,8 @@ static int q6v5_probe(struct platform_device *pdev)
qcom_add_glink_subdev(rproc, &qproc->glink_subdev);
qcom_add_smd_subdev(rproc, &qproc->smd_subdev);
qcom_add_ssr_subdev(rproc, &qproc->ssr_subdev, "mpss");
- qproc->sysmon = qcom_add_sysmon_subdev(rproc, "modem", 0x12);
+ qproc->sysmon = qcom_add_sysmon_subdev(rproc, "modem", 0x12,
+ &qproc->q6v5);
ret = rproc_add(rproc);
if (ret)
diff --git a/drivers/remoteproc/qcom_q6v5_pas.c b/drivers/remoteproc/qcom_q6v5_pas.c
index b1e63fcd5fdf..920a39ea6609 100644
--- a/drivers/remoteproc/qcom_q6v5_pas.c
+++ b/drivers/remoteproc/qcom_q6v5_pas.c
@@ -303,7 +303,8 @@ static int adsp_probe(struct platform_device *pdev)
qcom_add_ssr_subdev(rproc, &adsp->ssr_subdev, desc->ssr_name);
adsp->sysmon = qcom_add_sysmon_subdev(rproc,
desc->sysmon_name,
- desc->ssctl_id);
+ desc->ssctl_id,
+ &adsp->q6v5);
ret = rproc_add(rproc);
if (ret)
diff --git a/drivers/remoteproc/qcom_sysmon.c b/drivers/remoteproc/qcom_sysmon.c
index e976a602b015..c0d6ee8de995 100644
--- a/drivers/remoteproc/qcom_sysmon.c
+++ b/drivers/remoteproc/qcom_sysmon.c
@@ -14,12 +14,14 @@
#include <linux/rpmsg.h>
#include "qcom_common.h"
+#include "qcom_q6v5.h"
static BLOCKING_NOTIFIER_HEAD(sysmon_notifiers);
struct qcom_sysmon {
struct rproc_subdev subdev;
struct rproc *rproc;
+ struct qcom_q6v5 *q6v5;
struct list_head node;
@@ -442,7 +444,8 @@ static int sysmon_notify(struct notifier_block *nb, unsigned long event,
*/
struct qcom_sysmon *qcom_add_sysmon_subdev(struct rproc *rproc,
const char *name,
- int ssctl_instance)
+ int ssctl_instance,
+ struct qcom_q6v5 *q6v5)
{
struct qcom_sysmon *sysmon;
int ret;
@@ -456,6 +459,7 @@ struct qcom_sysmon *qcom_add_sysmon_subdev(struct rproc *rproc,
sysmon->name = name;
sysmon->ssctl_instance = ssctl_instance;
+ sysmon->q6v5 = q6v5;
init_completion(&sysmon->comp);
mutex_init(&sysmon->lock);
diff --git a/drivers/remoteproc/qcom_wcnss.c b/drivers/remoteproc/qcom_wcnss.c
index b0e07e9f42d5..af13cade35da 100644
--- a/drivers/remoteproc/qcom_wcnss.c
+++ b/drivers/remoteproc/qcom_wcnss.c
@@ -552,7 +552,8 @@ static int wcnss_probe(struct platform_device *pdev)
}
qcom_add_smd_subdev(rproc, &wcnss->smd_subdev);
- wcnss->sysmon = qcom_add_sysmon_subdev(rproc, "wcnss", WCNSS_SSCTL_ID);
+ wcnss->sysmon = qcom_add_sysmon_subdev(rproc, "wcnss", WCNSS_SSCTL_ID,
+ NULL);
ret = rproc_add(rproc);
if (ret)
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
^ permalink raw reply related [flat|nested] 7+ messages in thread* [PATCH v2 3/4] remoteproc: qcom: Wait for shutdown-ack/ind on sysmon shutdown
2018-12-24 8:48 [PATCH v2 1/4] dt-bindings: remoteproc: qcom: Introduce shutdown-ack irq for Q6V5 Sibi Sankar
2018-12-24 8:48 ` [PATCH v2 2/4] remoteproc: qcom: q6v5: Add shutdown-ack irq Sibi Sankar
@ 2018-12-24 8:48 ` Sibi Sankar
2019-01-03 23:33 ` Bjorn Andersson
2018-12-24 8:48 ` [PATCH v2 4/4] remoteproc: qcom: Enable shutdown-ack irq on Q6V5 Sibi Sankar
2018-12-27 21:55 ` [PATCH v2 1/4] dt-bindings: remoteproc: qcom: Introduce shutdown-ack irq for Q6V5 Rob Herring
3 siblings, 1 reply; 7+ messages in thread
From: Sibi Sankar @ 2018-12-24 8:48 UTC (permalink / raw)
To: bjorn.andersson, robh+dt, andy.gross, david.brown
Cc: linux-arm-msm, linux-soc, devicetree, linux-kernel, tsoni, clew,
akdwived, ohad, mark.rutland, linux-remoteproc, dianders,
Sibi Sankar
After sending a sysmon shutdown request to the SSCTL service on the
subsystem, wait for the service to send shutdown-ack interrupt or
an indication message to signal the completion of graceful shutdown.
Signed-off-by: Sibi Sankar <sibis@codeaurora.org>
---
drivers/remoteproc/qcom_sysmon.c | 40 +++++++++++++++++++++++++++++++-
1 file changed, 39 insertions(+), 1 deletion(-)
diff --git a/drivers/remoteproc/qcom_sysmon.c b/drivers/remoteproc/qcom_sysmon.c
index c0d6ee8de995..0da83638ca99 100644
--- a/drivers/remoteproc/qcom_sysmon.c
+++ b/drivers/remoteproc/qcom_sysmon.c
@@ -36,6 +36,7 @@ struct qcom_sysmon {
struct rpmsg_endpoint *ept;
struct completion comp;
+ struct completion ind_comp;
struct mutex lock;
bool ssr_ack;
@@ -139,6 +140,7 @@ static int sysmon_callback(struct rpmsg_device *rpdev, void *data, int count,
}
#define SSCTL_SHUTDOWN_REQ 0x21
+#define SSCTL_SHUTDOWN_READY_IND 0x21
#define SSCTL_SUBSYS_EVENT_REQ 0x23
#define SSCTL_MAX_MSG_LEN 7
@@ -254,6 +256,29 @@ static struct qmi_elem_info ssctl_subsys_event_resp_ei[] = {
{}
};
+static struct qmi_elem_info ssctl_shutdown_ind_ei[] = {
+ {}
+};
+
+static void sysmon_ind_cb(struct qmi_handle *qmi, struct sockaddr_qrtr *sq,
+ struct qmi_txn *txn, const void *data)
+{
+ struct qcom_sysmon *sysmon = container_of(qmi, struct qcom_sysmon, qmi);
+
+ complete(&sysmon->ind_comp);
+}
+
+static struct qmi_msg_handler qmi_indication_handler[] = {
+ {
+ .type = QMI_INDICATION,
+ .msg_id = SSCTL_SHUTDOWN_READY_IND,
+ .ei = ssctl_shutdown_ind_ei,
+ .decoded_size = 0,
+ .fn = sysmon_ind_cb
+ },
+ {}
+};
+
/**
* ssctl_request_shutdown() - request shutdown via SSCTL QMI service
* @sysmon: sysmon context
@@ -264,6 +289,7 @@ static void ssctl_request_shutdown(struct qcom_sysmon *sysmon)
struct qmi_txn txn;
int ret;
+ reinit_completion(&sysmon->ind_comp);
ret = qmi_txn_init(&sysmon->qmi, &txn, ssctl_shutdown_resp_ei, &resp);
if (ret < 0) {
dev_err(sysmon->dev, "failed to allocate QMI txn\n");
@@ -285,6 +311,16 @@ static void ssctl_request_shutdown(struct qcom_sysmon *sysmon)
dev_err(sysmon->dev, "shutdown request failed\n");
else
dev_dbg(sysmon->dev, "shutdown request completed\n");
+
+ if (sysmon->q6v5) {
+ ret = qcom_q6v5_wait_for_shutdown(sysmon->q6v5, 10 * HZ);
+ if (ret) {
+ ret = try_wait_for_completion(&sysmon->ind_comp);
+ if (!ret)
+ dev_err(sysmon->dev,
+ "timeout waiting for shutdown ack\n");
+ }
+ }
}
/**
@@ -462,9 +498,11 @@ struct qcom_sysmon *qcom_add_sysmon_subdev(struct rproc *rproc,
sysmon->q6v5 = q6v5;
init_completion(&sysmon->comp);
+ init_completion(&sysmon->ind_comp);
mutex_init(&sysmon->lock);
- ret = qmi_handle_init(&sysmon->qmi, SSCTL_MAX_MSG_LEN, &ssctl_ops, NULL);
+ ret = qmi_handle_init(&sysmon->qmi, SSCTL_MAX_MSG_LEN, &ssctl_ops,
+ qmi_indication_handler);
if (ret < 0) {
dev_err(sysmon->dev, "failed to initialize qmi handle\n");
kfree(sysmon);
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
^ permalink raw reply related [flat|nested] 7+ messages in thread* Re: [PATCH v2 3/4] remoteproc: qcom: Wait for shutdown-ack/ind on sysmon shutdown
2018-12-24 8:48 ` [PATCH v2 3/4] remoteproc: qcom: Wait for shutdown-ack/ind on sysmon shutdown Sibi Sankar
@ 2019-01-03 23:33 ` Bjorn Andersson
2019-01-08 10:14 ` Sibi Sankar
0 siblings, 1 reply; 7+ messages in thread
From: Bjorn Andersson @ 2019-01-03 23:33 UTC (permalink / raw)
To: Sibi Sankar
Cc: robh+dt, andy.gross, david.brown, linux-arm-msm, linux-soc,
devicetree, linux-kernel, tsoni, clew, akdwived, ohad,
mark.rutland, linux-remoteproc, dianders
On Mon 24 Dec 00:48 PST 2018, Sibi Sankar wrote:
> After sending a sysmon shutdown request to the SSCTL service on the
> subsystem, wait for the service to send shutdown-ack interrupt or
> an indication message to signal the completion of graceful shutdown.
>
> Signed-off-by: Sibi Sankar <sibis@codeaurora.org>
I prefer something closer to v1, where you kept the handling of the
interrupt within the sysmon driver.
What I didn't like was the fact that you resolved the mss
platform_device to get to the irq, not that you grabbed the irq from the
parent's DT node from within the sysmon device.
You can get the remoteproc's DT node by rproc->dev.parent->of_node and
use of_irq_get_byname() to get an irq number, which you can request in
the sysmon device - which will work regardless of the remoteproc driver
being a platform_driver or something else.
All the logic looks sound, but by shuffling things around we should get
less coupling of the implementation (DT binding looks good).
Regards,
Bjorn
> ---
> drivers/remoteproc/qcom_sysmon.c | 40 +++++++++++++++++++++++++++++++-
> 1 file changed, 39 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/remoteproc/qcom_sysmon.c b/drivers/remoteproc/qcom_sysmon.c
> index c0d6ee8de995..0da83638ca99 100644
> --- a/drivers/remoteproc/qcom_sysmon.c
> +++ b/drivers/remoteproc/qcom_sysmon.c
> @@ -36,6 +36,7 @@ struct qcom_sysmon {
>
> struct rpmsg_endpoint *ept;
> struct completion comp;
> + struct completion ind_comp;
> struct mutex lock;
>
> bool ssr_ack;
> @@ -139,6 +140,7 @@ static int sysmon_callback(struct rpmsg_device *rpdev, void *data, int count,
> }
>
> #define SSCTL_SHUTDOWN_REQ 0x21
> +#define SSCTL_SHUTDOWN_READY_IND 0x21
> #define SSCTL_SUBSYS_EVENT_REQ 0x23
>
> #define SSCTL_MAX_MSG_LEN 7
> @@ -254,6 +256,29 @@ static struct qmi_elem_info ssctl_subsys_event_resp_ei[] = {
> {}
> };
>
> +static struct qmi_elem_info ssctl_shutdown_ind_ei[] = {
> + {}
> +};
> +
> +static void sysmon_ind_cb(struct qmi_handle *qmi, struct sockaddr_qrtr *sq,
> + struct qmi_txn *txn, const void *data)
> +{
> + struct qcom_sysmon *sysmon = container_of(qmi, struct qcom_sysmon, qmi);
> +
> + complete(&sysmon->ind_comp);
> +}
> +
> +static struct qmi_msg_handler qmi_indication_handler[] = {
> + {
> + .type = QMI_INDICATION,
> + .msg_id = SSCTL_SHUTDOWN_READY_IND,
> + .ei = ssctl_shutdown_ind_ei,
> + .decoded_size = 0,
> + .fn = sysmon_ind_cb
> + },
> + {}
> +};
> +
> /**
> * ssctl_request_shutdown() - request shutdown via SSCTL QMI service
> * @sysmon: sysmon context
> @@ -264,6 +289,7 @@ static void ssctl_request_shutdown(struct qcom_sysmon *sysmon)
> struct qmi_txn txn;
> int ret;
>
> + reinit_completion(&sysmon->ind_comp);
> ret = qmi_txn_init(&sysmon->qmi, &txn, ssctl_shutdown_resp_ei, &resp);
> if (ret < 0) {
> dev_err(sysmon->dev, "failed to allocate QMI txn\n");
> @@ -285,6 +311,16 @@ static void ssctl_request_shutdown(struct qcom_sysmon *sysmon)
> dev_err(sysmon->dev, "shutdown request failed\n");
> else
> dev_dbg(sysmon->dev, "shutdown request completed\n");
> +
> + if (sysmon->q6v5) {
> + ret = qcom_q6v5_wait_for_shutdown(sysmon->q6v5, 10 * HZ);
> + if (ret) {
> + ret = try_wait_for_completion(&sysmon->ind_comp);
> + if (!ret)
> + dev_err(sysmon->dev,
> + "timeout waiting for shutdown ack\n");
> + }
> + }
> }
>
> /**
> @@ -462,9 +498,11 @@ struct qcom_sysmon *qcom_add_sysmon_subdev(struct rproc *rproc,
> sysmon->q6v5 = q6v5;
>
> init_completion(&sysmon->comp);
> + init_completion(&sysmon->ind_comp);
> mutex_init(&sysmon->lock);
>
> - ret = qmi_handle_init(&sysmon->qmi, SSCTL_MAX_MSG_LEN, &ssctl_ops, NULL);
> + ret = qmi_handle_init(&sysmon->qmi, SSCTL_MAX_MSG_LEN, &ssctl_ops,
> + qmi_indication_handler);
> if (ret < 0) {
> dev_err(sysmon->dev, "failed to initialize qmi handle\n");
> kfree(sysmon);
> --
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project
>
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH v2 3/4] remoteproc: qcom: Wait for shutdown-ack/ind on sysmon shutdown
2019-01-03 23:33 ` Bjorn Andersson
@ 2019-01-08 10:14 ` Sibi Sankar
0 siblings, 0 replies; 7+ messages in thread
From: Sibi Sankar @ 2019-01-08 10:14 UTC (permalink / raw)
To: Bjorn Andersson
Cc: robh+dt, andy.gross, david.brown, linux-arm-msm, linux-soc,
devicetree, linux-kernel, tsoni, clew, akdwived, ohad,
mark.rutland, linux-remoteproc, dianders, linux-kernel-owner
Hi Bjorn,
Thanks for the review!
On 2019-01-04 05:03, Bjorn Andersson wrote:
> On Mon 24 Dec 00:48 PST 2018, Sibi Sankar wrote:
>
>> After sending a sysmon shutdown request to the SSCTL service on the
>> subsystem, wait for the service to send shutdown-ack interrupt or
>> an indication message to signal the completion of graceful shutdown.
>>
>> Signed-off-by: Sibi Sankar <sibis@codeaurora.org>
>
> I prefer something closer to v1, where you kept the handling of the
> interrupt within the sysmon driver.
>
> What I didn't like was the fact that you resolved the mss
> platform_device to get to the irq, not that you grabbed the irq from
> the
> parent's DT node from within the sysmon device.
>
>
> You can get the remoteproc's DT node by rproc->dev.parent->of_node and
> use of_irq_get_byname() to get an irq number, which you can request in
> the sysmon device - which will work regardless of the remoteproc driver
> being a platform_driver or something else.
>
> All the logic looks sound, but by shuffling things around we should get
> less coupling of the implementation (DT binding looks good).
>
will make it closer to v1 in the next re-spin
> Regards,
> Bjorn
>
>> ---
>> drivers/remoteproc/qcom_sysmon.c | 40
>> +++++++++++++++++++++++++++++++-
>> 1 file changed, 39 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/remoteproc/qcom_sysmon.c
>> b/drivers/remoteproc/qcom_sysmon.c
>> index c0d6ee8de995..0da83638ca99 100644
>> --- a/drivers/remoteproc/qcom_sysmon.c
>> +++ b/drivers/remoteproc/qcom_sysmon.c
>> @@ -36,6 +36,7 @@ struct qcom_sysmon {
>>
>> struct rpmsg_endpoint *ept;
>> struct completion comp;
>> + struct completion ind_comp;
>> struct mutex lock;
>>
>> bool ssr_ack;
>> @@ -139,6 +140,7 @@ static int sysmon_callback(struct rpmsg_device
>> *rpdev, void *data, int count,
>> }
>>
>> #define SSCTL_SHUTDOWN_REQ 0x21
>> +#define SSCTL_SHUTDOWN_READY_IND 0x21
>> #define SSCTL_SUBSYS_EVENT_REQ 0x23
>>
>> #define SSCTL_MAX_MSG_LEN 7
>> @@ -254,6 +256,29 @@ static struct qmi_elem_info
>> ssctl_subsys_event_resp_ei[] = {
>> {}
>> };
>>
>> +static struct qmi_elem_info ssctl_shutdown_ind_ei[] = {
>> + {}
>> +};
>> +
>> +static void sysmon_ind_cb(struct qmi_handle *qmi, struct
>> sockaddr_qrtr *sq,
>> + struct qmi_txn *txn, const void *data)
>> +{
>> + struct qcom_sysmon *sysmon = container_of(qmi, struct qcom_sysmon,
>> qmi);
>> +
>> + complete(&sysmon->ind_comp);
>> +}
>> +
>> +static struct qmi_msg_handler qmi_indication_handler[] = {
>> + {
>> + .type = QMI_INDICATION,
>> + .msg_id = SSCTL_SHUTDOWN_READY_IND,
>> + .ei = ssctl_shutdown_ind_ei,
>> + .decoded_size = 0,
>> + .fn = sysmon_ind_cb
>> + },
>> + {}
>> +};
>> +
>> /**
>> * ssctl_request_shutdown() - request shutdown via SSCTL QMI service
>> * @sysmon: sysmon context
>> @@ -264,6 +289,7 @@ static void ssctl_request_shutdown(struct
>> qcom_sysmon *sysmon)
>> struct qmi_txn txn;
>> int ret;
>>
>> + reinit_completion(&sysmon->ind_comp);
>> ret = qmi_txn_init(&sysmon->qmi, &txn, ssctl_shutdown_resp_ei,
>> &resp);
>> if (ret < 0) {
>> dev_err(sysmon->dev, "failed to allocate QMI txn\n");
>> @@ -285,6 +311,16 @@ static void ssctl_request_shutdown(struct
>> qcom_sysmon *sysmon)
>> dev_err(sysmon->dev, "shutdown request failed\n");
>> else
>> dev_dbg(sysmon->dev, "shutdown request completed\n");
>> +
>> + if (sysmon->q6v5) {
>> + ret = qcom_q6v5_wait_for_shutdown(sysmon->q6v5, 10 * HZ);
>> + if (ret) {
>> + ret = try_wait_for_completion(&sysmon->ind_comp);
>> + if (!ret)
>> + dev_err(sysmon->dev,
>> + "timeout waiting for shutdown ack\n");
>> + }
>> + }
>> }
>>
>> /**
>> @@ -462,9 +498,11 @@ struct qcom_sysmon *qcom_add_sysmon_subdev(struct
>> rproc *rproc,
>> sysmon->q6v5 = q6v5;
>>
>> init_completion(&sysmon->comp);
>> + init_completion(&sysmon->ind_comp);
>> mutex_init(&sysmon->lock);
>>
>> - ret = qmi_handle_init(&sysmon->qmi, SSCTL_MAX_MSG_LEN, &ssctl_ops,
>> NULL);
>> + ret = qmi_handle_init(&sysmon->qmi, SSCTL_MAX_MSG_LEN, &ssctl_ops,
>> + qmi_indication_handler);
>> if (ret < 0) {
>> dev_err(sysmon->dev, "failed to initialize qmi handle\n");
>> kfree(sysmon);
>> --
>> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora
>> Forum,
>> a Linux Foundation Collaborative Project
>>
--
-- Sibi Sankar --
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project.
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH v2 4/4] remoteproc: qcom: Enable shutdown-ack irq on Q6V5
2018-12-24 8:48 [PATCH v2 1/4] dt-bindings: remoteproc: qcom: Introduce shutdown-ack irq for Q6V5 Sibi Sankar
2018-12-24 8:48 ` [PATCH v2 2/4] remoteproc: qcom: q6v5: Add shutdown-ack irq Sibi Sankar
2018-12-24 8:48 ` [PATCH v2 3/4] remoteproc: qcom: Wait for shutdown-ack/ind on sysmon shutdown Sibi Sankar
@ 2018-12-24 8:48 ` Sibi Sankar
2018-12-27 21:55 ` [PATCH v2 1/4] dt-bindings: remoteproc: qcom: Introduce shutdown-ack irq for Q6V5 Rob Herring
3 siblings, 0 replies; 7+ messages in thread
From: Sibi Sankar @ 2018-12-24 8:48 UTC (permalink / raw)
To: bjorn.andersson, robh+dt, andy.gross, david.brown
Cc: linux-arm-msm, linux-soc, devicetree, linux-kernel, tsoni, clew,
akdwived, ohad, mark.rutland, linux-remoteproc, dianders,
Sibi Sankar
Enable shutdown-ack irq handling for MSS Q6V5 on SDM845/MSM8996
SoCs and for WCSS Q6V5 on QCS404 SoC.
Signed-off-by: Sibi Sankar <sibis@codeaurora.org>
---
drivers/remoteproc/qcom_q6v5_mss.c | 11 ++++++++---
drivers/remoteproc/qcom_q6v5_pas.c | 9 +++++++--
2 files changed, 15 insertions(+), 5 deletions(-)
diff --git a/drivers/remoteproc/qcom_q6v5_mss.c b/drivers/remoteproc/qcom_q6v5_mss.c
index 3bc2dec85928..fdb18d1ea177 100644
--- a/drivers/remoteproc/qcom_q6v5_mss.c
+++ b/drivers/remoteproc/qcom_q6v5_mss.c
@@ -134,6 +134,7 @@ struct rproc_hexagon_res {
int version;
bool need_mem_protection;
bool has_alt_reset;
+ u8 has_shutdown_irq;
};
struct q6v5 {
@@ -1252,6 +1253,7 @@ static int q6v5_probe(struct platform_device *pdev)
{
const struct rproc_hexagon_res *desc;
struct q6v5 *qproc;
+ struct qcom_q6v5 *q6v5;
struct rproc *rproc;
int ret;
@@ -1273,6 +1275,7 @@ static int q6v5_probe(struct platform_device *pdev)
qproc->dev = &pdev->dev;
qproc->rproc = rproc;
platform_set_drvdata(pdev, qproc);
+ q6v5 = &qproc->q6v5;
ret = q6v5_init_mem(qproc, pdev);
if (ret)
@@ -1330,7 +1333,8 @@ static int q6v5_probe(struct platform_device *pdev)
qproc->version = desc->version;
qproc->need_mem_protection = desc->need_mem_protection;
- ret = qcom_q6v5_init(&qproc->q6v5, pdev, rproc, MPSS_CRASH_REASON_SMEM,
+ q6v5->has_shutdown_irq = desc->has_shutdown_irq;
+ ret = qcom_q6v5_init(q6v5, pdev, rproc, MPSS_CRASH_REASON_SMEM,
qcom_msa_handover);
if (ret)
goto free_rproc;
@@ -1340,8 +1344,7 @@ static int q6v5_probe(struct platform_device *pdev)
qcom_add_glink_subdev(rproc, &qproc->glink_subdev);
qcom_add_smd_subdev(rproc, &qproc->smd_subdev);
qcom_add_ssr_subdev(rproc, &qproc->ssr_subdev, "mpss");
- qproc->sysmon = qcom_add_sysmon_subdev(rproc, "modem", 0x12,
- &qproc->q6v5);
+ qproc->sysmon = qcom_add_sysmon_subdev(rproc, "modem", 0x12, q6v5);
ret = rproc_add(rproc);
if (ret)
@@ -1391,6 +1394,7 @@ static const struct rproc_hexagon_res sdm845_mss = {
},
.need_mem_protection = true,
.has_alt_reset = true,
+ .has_shutdown_irq = true,
.version = MSS_SDM845,
};
@@ -1410,6 +1414,7 @@ static const struct rproc_hexagon_res msm8996_mss = {
},
.need_mem_protection = true,
.has_alt_reset = false,
+ .has_shutdown_irq = true,
.version = MSS_MSM8996,
};
diff --git a/drivers/remoteproc/qcom_q6v5_pas.c b/drivers/remoteproc/qcom_q6v5_pas.c
index 920a39ea6609..2d6ff59575cd 100644
--- a/drivers/remoteproc/qcom_q6v5_pas.c
+++ b/drivers/remoteproc/qcom_q6v5_pas.c
@@ -43,6 +43,7 @@ struct adsp_data {
const char *ssr_name;
const char *sysmon_name;
int ssctl_id;
+ u8 has_shutdown_irq;
};
struct qcom_adsp {
@@ -257,6 +258,7 @@ static int adsp_probe(struct platform_device *pdev)
{
const struct adsp_data *desc;
struct qcom_adsp *adsp;
+ struct qcom_q6v5 *q6v5;
struct rproc *rproc;
int ret;
@@ -280,6 +282,7 @@ static int adsp_probe(struct platform_device *pdev)
adsp->pas_id = desc->pas_id;
adsp->has_aggre2_clk = desc->has_aggre2_clk;
platform_set_drvdata(pdev, adsp);
+ q6v5 = &adsp->q6v5;
ret = adsp_alloc_memory_region(adsp);
if (ret)
@@ -293,7 +296,8 @@ static int adsp_probe(struct platform_device *pdev)
if (ret)
goto free_rproc;
- ret = qcom_q6v5_init(&adsp->q6v5, pdev, rproc, desc->crash_reason_smem,
+ q6v5->has_shutdown_irq = desc->has_shutdown_irq;
+ ret = qcom_q6v5_init(q6v5, pdev, rproc, desc->crash_reason_smem,
qcom_pas_handover);
if (ret)
goto free_rproc;
@@ -304,7 +308,7 @@ static int adsp_probe(struct platform_device *pdev)
adsp->sysmon = qcom_add_sysmon_subdev(rproc,
desc->sysmon_name,
desc->ssctl_id,
- &adsp->q6v5);
+ q6v5);
ret = rproc_add(rproc);
if (ret)
@@ -366,6 +370,7 @@ static const struct adsp_data slpi_resource_init = {
static const struct adsp_data wcss_resource_init = {
.crash_reason_smem = 421,
.firmware_name = "wcnss.mdt",
+ .has_shutdown_irq = true,
.pas_id = 6,
.ssr_name = "mpss",
.sysmon_name = "wcnss",
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
^ permalink raw reply related [flat|nested] 7+ messages in thread* Re: [PATCH v2 1/4] dt-bindings: remoteproc: qcom: Introduce shutdown-ack irq for Q6V5
2018-12-24 8:48 [PATCH v2 1/4] dt-bindings: remoteproc: qcom: Introduce shutdown-ack irq for Q6V5 Sibi Sankar
` (2 preceding siblings ...)
2018-12-24 8:48 ` [PATCH v2 4/4] remoteproc: qcom: Enable shutdown-ack irq on Q6V5 Sibi Sankar
@ 2018-12-27 21:55 ` Rob Herring
3 siblings, 0 replies; 7+ messages in thread
From: Rob Herring @ 2018-12-27 21:55 UTC (permalink / raw)
To: Sibi Sankar
Cc: bjorn.andersson, robh+dt, andy.gross, david.brown, linux-arm-msm,
linux-soc, devicetree, linux-kernel, tsoni, clew, akdwived, ohad,
mark.rutland, linux-remoteproc, dianders, Sibi Sankar
On Mon, 24 Dec 2018 14:18:21 +0530, Sibi Sankar wrote:
> Introduce shutdown-irq binding required for sysmon shutdown for Q6V5 MSS
> on SDM845/MSM8996 SoCs and for WCSS Q6V5 on QCS404 SoC.
>
> Signed-off-by: Sibi Sankar <sibis@codeaurora.org>
> ---
>
> v2:
> Make shutdown-ack mandatory for MSS on SDM845/MSM8996 and
> for WCSS on QCS404 (Dropping Rob's reviewed-by due to this)
>
> .../bindings/remoteproc/qcom,adsp.txt | 17 ++++++++++++++---
> .../bindings/remoteproc/qcom,q6v5.txt | 15 ++++++++++++---
> 2 files changed, 26 insertions(+), 6 deletions(-)
>
Reviewed-by: Rob Herring <robh@kernel.org>
^ permalink raw reply [flat|nested] 7+ messages in thread