[PATCH V5 0/2] SCM: Add support for wait-queue aware firmware

[PATCH V5 0/2] SCM: Add support for wait-queue aware firmware

[Sibi Sankar]

This patch series enables the QCOM SCM driver to support firmware (FW) versions
that expect the high-level OS (HLOS) to be tolerant of SCM call requests not
being processed right away and, instead, being placed on a wait-queue in FW and
processed accordingly.

The problem this feature is fixing is as follows. In a scenario where there is
a VM in addition to HLOS (and an underlying hypervisor):

1. HLOS makes an SMC call on core 5
2. The hypervisor scheduling interrupt interrupts this SMC call.
3. The hypervisor schedules the VM on core 5.
4. The VM makes an SMC call on core 5.
5. The SMC call is non-interruptibly stuck on FW spinlock on core 5.
6. HLOS cannot reschedule since core 5 is not responding to Reschedule IPIs.
7. Watchdog timer expires waiting for core 5.

This problem is solved by FW returning a new return code SCM_WAITQ_SLEEP to
HLOS right away when it is overwhelmed by the VM's SMC call. HLOS then places
the call on a wait-queue and wakes it up when it receives an interrupt that
signifies "all-clear".

There are two new SMC calls also being defined in this design that, together
with one new return code, form the handshake protocol between Linux and FW.

This design is also backwards-compatible with existing firmware versions that
do not support this feature.

v5:
- Pick up R-b
- Handle the wake_one/wake_all flags [Guru]
- Rename flag handler to qcom_scm_waitq_wakeup [Bjorn]
- Resume scm call can return ebusy as well handle that scenario by retrying
  the original smc call and not the resume call

v4:
- platform_set_drvdata will be used by __scm_smc_do_quirk_handle_waitq to
  get access to scm struct from device so retain it
- Use a single completion as it satisfies all of the current usecases [Bjorn]
- Inline scm_get_wq_ctx [Bjorn]
- Convert all pr_err to dev_err [Bjorn]
- Handle idr_destroy in a thread safe manner [Bjorn]
- Misc. Style fixes [Bjorn]
- Qualify bindings [Krzysztoff]

v3:
- Drop allow-multi-call property since HLOS doesn't completely support it
  yet.
- Fixup irq handling so as not to affect SoCs without the interrupt.
- Fix warnings reported by kernel test-bot.

v2:
- Changes made to patches 4 and 5 are listed therein.
- Rebased dt-bindings on top of the YAML conversion patch [1].

Older version of the series:
[v4] https://patchwork.kernel.org/project/linux-arm-msm/cover/20221114082644.28739-1-quic_sibis@quicinc.com/
[v3] https://patchwork.kernel.org/project/linux-arm-msm/cover/1666086406-5452-1-git-send-email-quic_sibis@quicinc.com/ 
[v2] https://patchwork.kernel.org/project/linux-arm-msm/cover/1661898311-30126-1-git-send-email-quic_gurus@quicinc.com/
[v1] https://patchwork.kernel.org/project/linux-arm-msm/cover/1656359076-13018-1-git-send-email-quic_gurus@quicinc.com/


Guru Das Srinagesh (2):
  dt-bindings: firmware: qcom-scm: Add optional interrupt
  firmware: qcom: scm: Add wait-queue handling logic

 .../bindings/firmware/qcom,scm.yaml           |   6 +
 drivers/firmware/qcom_scm-smc.c               |  97 +++++++++++++--
 drivers/firmware/qcom_scm.c                   | 115 +++++++++++++++++-
 drivers/firmware/qcom_scm.h                   |   9 ++
 4 files changed, 219 insertions(+), 8 deletions(-)

-- 
2.17.1

[PATCH V5 1/2] dt-bindings: firmware: qcom-scm: Add optional interrupt

[Sibi Sankar]

From: Guru Das Srinagesh <quic_gurus@quicinc.com>

Add an interrupt specification to the bindings to support the wait-queue
feature.

Signed-off-by: Guru Das Srinagesh <quic_gurus@quicinc.com>
Signed-off-by: Sibi Sankar <quic_sibis@quicinc.com>
Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
---

v5:
- Pick up R-b

v4:
- Qualify bindings [Krzysztoff]

 Documentation/devicetree/bindings/firmware/qcom,scm.yaml | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/Documentation/devicetree/bindings/firmware/qcom,scm.yaml b/Documentation/devicetree/bindings/firmware/qcom,scm.yaml
index 25688571ee7c..aea6e0c86a89 100644
--- a/Documentation/devicetree/bindings/firmware/qcom,scm.yaml
+++ b/Documentation/devicetree/bindings/firmware/qcom,scm.yaml
@@ -73,6 +73,12 @@ properties:
   '#reset-cells':
     const: 1
 
+  interrupts:
+    description:
+      The wait-queue interrupt that firmware raises as part of handshake
+      protocol to handle sleeping SCM calls.
+    maxItems: 1
+
   qcom,dload-mode:
     $ref: /schemas/types.yaml#/definitions/phandle-array
     items:
-- 
2.17.1

[PATCH V5 2/2] firmware: qcom: scm: Add wait-queue handling logic

[Sibi Sankar]

From: Guru Das Srinagesh <quic_gurus@quicinc.com>

When the firmware (FW) supports multiple requests per VM, multiple requests
from the same/different VM can reach the firmware at the same time. Since
the firmware currently being used has limited resources, it guards them
with a resource lock and puts requests on a wait-queue internally and
signals to HLOS that it is doing so. It does this by returning a new return
value in addition to success or error: SCM_WAITQ_SLEEP. A sleeping SCM call
can be woken up by an interrupt that the FW raises.

  1) SCM_WAITQ_SLEEP:

  	When an SCM call receives this return value instead of success
  	or error, FW has placed this call on a wait-queue and has signalled
	HLOS to put it to non-interruptible sleep.

	Along with this return value, FW also passes to HLOS `wq_ctx` -
	a unique number (UID) identifying the wait-queue that it has put
	the call on, internally. This is to help HLOS with its own
	bookkeeping to wake this sleeping call later.

	Additionally, FW also passes to HLOS `smc_call_ctx` - a UID
	identifying the SCM call thus being put to sleep. This is also
	for HLOS' bookkeeping to wake this call up later.

	These two additional values are passed via the a1 and a2
	registers.

	N.B.: The "ctx" in the above UID names = "context".

The handshake mechanism that HLOS uses to talk to FW about wait-queue
operations involves two new SMC calls.

  1) get_wq_ctx():

    	Arguments: 	None
    	Returns:	wq_ctx, flags, more_pending

    	Get the wait-queue context, and wake up either one or all of the
    	sleeping SCM calls associated with that wait-queue.

    	Additionally, repeat this if there are more wait-queues that are
    	ready to have their requests woken up (`more_pending`).

  2) wq_resume(smc_call_ctx):

  	Arguments:	smc_call_ctx

  	HLOS needs to issue this in response to receiving an
  	IRQ, passing to FW the same smc_call_ctx that FW
  	receives from HLOS via the get_wq_ctx() call.

(The mechanism to wake a SMC call back up is described in detail below)

 VM_1                     VM_2                            Firmware
   │                        │                                 │
   │                        │                                 │
   │                        │                                 │
   │                        │                                 │
   │      REQUEST_1         │                                 │
   ├────────────────────────┼─────────────────────────────────┤
   │                        │                                 │
   │                        │                              ┌──┼──┐
   │                        │                              │  │  │
   │                        │     REQUEST_2                │  │  │
   │                        ├──────────────────────────────┼──┤  │
   │                        │                              │  │  │Resource
   │                        │                              │  │  │is busy
   │                        │       {WQ_SLEEP}             │  │  │
   │                        │◄─────────────────────────────┼──┤  │
   │                        │  wq_ctx, smc_call_ctx        │  │  │
   │                        │                              └──┼──┘
   │   REQUEST_1 COMPLETE   │                                 │
   │◄───────────────────────┼─────────────────────────────────┤
   │                        │                                 │
   │                        │         IRQ                     │
   │                        │◄─-------------------------------│
   │                        │                                 │
   │                        │      get_wq_ctx()               │
   │                        ├────────────────────────────────►│
   │                        │                                 │
   │                        │                                 │
   │                        │◄────────────────────────────────┤
   │                        │   wq_ctx, flags, and            │
   │                        │        more_pending             │
   │                        │                                 │
   │                        │                                 │
   │                        │ wq_resume(smc_call_ctx)         │
   │                        ├────────────────────────────────►│
   │                        │                                 │
   │                        │                                 │
   │                        │      REQUEST_2 COMPLETE         │
   │                        │◄────────────────────────────────┤
   │                        │                                 │
   │                        │                                 │

With the exception of get_wq_ctx(), the other SMC call wq_resume() can
return WQ_SLEEP (these nested rounds of WQ_SLEEP are not shown in the
above diagram for the sake of simplicity). Therefore, introduce a new
do-while loop to handle multiple WQ_SLEEP return values for the same
parent SCM call.

Request Completion in the above diagram refers to either a success
return value (zero) or error (and not SMC_WAITQ_SLEEP)

Also add the interrupt handler that wakes up a sleeping SCM call.

Signed-off-by: Guru Das Srinagesh <quic_gurus@quicinc.com>
Co-developed-by: Sibi Sankar <quic_sibis@quicinc.com>
Signed-off-by: Sibi Sankar <quic_sibis@quicinc.com>
---

v5:
- Handle the wake_one/wake_all flags [Guru]
- Rename flag handler to qcom_scm_waitq_wakeup [Bjorn]
- Resume scm call can return ebusy as well handle that scenario by retrying
  the original smc call and not the resume call

v4:
- platform_set_drvdata will be used by __scm_smc_do_quirk_handle_waitq to
  get access to scm struct from device so retain it
- Use a single completion as it satisfies all of the current usecases [Bjorn]
- Inline scm_get_wq_ctx [Bjorn]
- Convert all pr_err to dev_err [Bjorn]
- Handle idr_destroy in a thread safe manner [Bjorn]
- Misc. Style fixes [Bjorn]

v3:
- Fixup irq handling so as not to affect SoCs without the interrupt.
- Fix warnings reported by kernel test-bot.

 drivers/firmware/qcom_scm-smc.c |  97 +++++++++++++++++++++++++--
 drivers/firmware/qcom_scm.c     | 115 +++++++++++++++++++++++++++++++-
 drivers/firmware/qcom_scm.h     |   9 +++
 3 files changed, 213 insertions(+), 8 deletions(-)

diff --git a/drivers/firmware/qcom_scm-smc.c b/drivers/firmware/qcom_scm-smc.c
index d111833364ba..385615d76e94 100644
--- a/drivers/firmware/qcom_scm-smc.c
+++ b/drivers/firmware/qcom_scm-smc.c
@@ -52,29 +52,108 @@ static void __scm_smc_do_quirk(const struct arm_smccc_args *smc,
 	} while (res->a0 == QCOM_SCM_INTERRUPTED);
 }
 
-static void __scm_smc_do(const struct arm_smccc_args *smc,
-			 struct arm_smccc_res *res, bool atomic)
+static void fill_wq_resume_args(struct arm_smccc_args *resume, u32 smc_call_ctx)
 {
-	int retry_count = 0;
+	memset(resume->args, 0, sizeof(resume->args[0]) * ARRAY_SIZE(resume->args));
+
+	resume->args[0] = ARM_SMCCC_CALL_VAL(ARM_SMCCC_STD_CALL,
+					ARM_SMCCC_SMC_64, ARM_SMCCC_OWNER_SIP,
+					SCM_SMC_FNID(QCOM_SCM_SVC_WAITQ, QCOM_SCM_WAITQ_RESUME));
+
+	resume->args[1] = QCOM_SCM_ARGS(1);
+
+	resume->args[2] = smc_call_ctx;
+}
+
+int scm_get_wq_ctx(u32 *wq_ctx, u32 *flags, u32 *more_pending)
+{
+	int ret;
+	struct arm_smccc_args get_wq_ctx = {0};
+	struct arm_smccc_res get_wq_res;
+
+	get_wq_ctx.args[0] = ARM_SMCCC_CALL_VAL(ARM_SMCCC_STD_CALL,
+				ARM_SMCCC_SMC_64, ARM_SMCCC_OWNER_SIP,
+				SCM_SMC_FNID(QCOM_SCM_SVC_WAITQ, QCOM_SCM_WAITQ_GET_WQ_CTX));
+
+	/* Guaranteed to return only success or error, no WAITQ_* */
+	__scm_smc_do_quirk(&get_wq_ctx, &get_wq_res);
+	ret = get_wq_res.a0;
+	if (ret)
+		return ret;
+
+	*wq_ctx = get_wq_res.a1;
+	*flags  = get_wq_res.a2;
+	*more_pending = get_wq_res.a3;
+
+	return 0;
+}
+
+static int __scm_smc_do_quirk_handle_waitq(struct device *dev, struct arm_smccc_args *waitq,
+					   struct arm_smccc_res *res)
+{
+	struct qcom_scm *scm;
+	struct completion *wq = NULL;
+	struct arm_smccc_args resume;
+	u32 wq_ctx, smc_call_ctx, flags;
+	struct arm_smccc_args *smc = waitq;
+
+	do {
+		__scm_smc_do_quirk(smc, res);
+
+		if (res->a0 == QCOM_SCM_WAITQ_SLEEP) {
+			wq_ctx = res->a1;
+			smc_call_ctx = res->a2;
+			flags = res->a3;
+
+			if (!dev)
+				return -EPROBE_DEFER;
+
+			scm = dev_get_drvdata(dev);
+			wq = qcom_scm_lookup_wq(scm, wq_ctx);
+			if (IS_ERR_OR_NULL(wq)) {
+				dev_err(dev, "No waitqueue found for wq_ctx %d: %ld\n",
+					wq_ctx, PTR_ERR(wq));
+				return PTR_ERR(wq) ? : -EINVAL;
+			}
+
+			wait_for_completion(wq);
+			fill_wq_resume_args(&resume, smc_call_ctx);
+			smc = &resume;
+			wq = NULL;
+		}
+	} while (res->a0 == QCOM_SCM_WAITQ_SLEEP);
+
+	return 0;
+}
+
+static int __scm_smc_do(struct device *dev, struct arm_smccc_args *smc,
+			struct arm_smccc_res *res, bool atomic)
+{
+	int ret, retry_count = 0;
 
 	if (atomic) {
 		__scm_smc_do_quirk(smc, res);
-		return;
+		return 0;
 	}
 
 	do {
 		mutex_lock(&qcom_scm_lock);
 
-		__scm_smc_do_quirk(smc, res);
+		ret = __scm_smc_do_quirk_handle_waitq(dev, smc, res);
 
 		mutex_unlock(&qcom_scm_lock);
 
+		if (ret)
+			return ret;
+
 		if (res->a0 == QCOM_SCM_V2_EBUSY) {
 			if (retry_count++ > QCOM_SCM_EBUSY_MAX_RETRY)
 				break;
 			msleep(QCOM_SCM_EBUSY_WAIT_MS);
 		}
 	}  while (res->a0 == QCOM_SCM_V2_EBUSY);
+
+	return 0;
 }
 
 
@@ -83,7 +162,7 @@ int __scm_smc_call(struct device *dev, const struct qcom_scm_desc *desc,
 		   struct qcom_scm_res *res, bool atomic)
 {
 	int arglen = desc->arginfo & 0xf;
-	int i;
+	int i, ret;
 	dma_addr_t args_phys = 0;
 	void *args_virt = NULL;
 	size_t alloc_len;
@@ -135,13 +214,17 @@ int __scm_smc_call(struct device *dev, const struct qcom_scm_desc *desc,
 		smc.args[SCM_SMC_LAST_REG_IDX] = args_phys;
 	}
 
-	__scm_smc_do(&smc, &smc_res, atomic);
+	/* ret error check follows after args_virt cleanup*/
+	ret = __scm_smc_do(dev, &smc, &smc_res, atomic);
 
 	if (args_virt) {
 		dma_unmap_single(dev, args_phys, alloc_len, DMA_TO_DEVICE);
 		kfree(args_virt);
 	}
 
+	if (ret)
+		return ret;
+
 	if (res) {
 		res->result[0] = smc_res.a1;
 		res->result[1] = smc_res.a2;
diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
index cdbfe54c8146..396080138eaa 100644
--- a/drivers/firmware/qcom_scm.c
+++ b/drivers/firmware/qcom_scm.c
@@ -3,7 +3,9 @@
  * Copyright (C) 2015 Linaro Ltd.
  */
 #include <linux/platform_device.h>
+#include <linux/idr.h>
 #include <linux/init.h>
+#include <linux/interrupt.h>
 #include <linux/cpumask.h>
 #include <linux/export.h>
 #include <linux/dma-mapping.h>
@@ -13,9 +15,12 @@
 #include <linux/qcom_scm.h>
 #include <linux/of.h>
 #include <linux/of_address.h>
+#include <linux/of_irq.h>
 #include <linux/of_platform.h>
 #include <linux/clk.h>
 #include <linux/reset-controller.h>
+#include <linux/spinlock.h>
+#include <linux/ktime.h>
 #include <linux/arm-smccc.h>
 
 #include "qcom_scm.h"
@@ -27,12 +32,20 @@ module_param(download_mode, bool, 0);
 #define SCM_HAS_IFACE_CLK	BIT(1)
 #define SCM_HAS_BUS_CLK		BIT(2)
 
+struct qcom_scm_waitq {
+	struct idr idr;
+	/* control access to IDR */
+	spinlock_t idr_lock;
+	struct completion waitq_comp;
+};
+
 struct qcom_scm {
 	struct device *dev;
 	struct clk *core_clk;
 	struct clk *iface_clk;
 	struct clk *bus_clk;
 	struct icc_path *path;
+	struct qcom_scm_waitq waitq;
 	struct reset_controller_dev reset;
 
 	/* control access to the interconnect path */
@@ -63,6 +76,9 @@ static const u8 qcom_scm_cpu_warm_bits[QCOM_SCM_BOOT_MAX_CPUS] = {
 	BIT(2), BIT(1), BIT(4), BIT(6)
 };
 
+#define QCOM_SMC_WAITQ_FLAG_WAKE_ONE	BIT(0)
+#define QCOM_SMC_WAITQ_FLAG_WAKE_ALL	BIT(1)
+
 static const char * const qcom_scm_convention_names[] = {
 	[SMC_CONVENTION_UNKNOWN] = "unknown",
 	[SMC_CONVENTION_ARM_32] = "smc arm 32",
@@ -1325,11 +1341,82 @@ bool qcom_scm_is_available(void)
 }
 EXPORT_SYMBOL(qcom_scm_is_available);
 
+struct completion *qcom_scm_lookup_wq(struct qcom_scm *scm, u32 wq_ctx)
+{
+	int err;
+	unsigned long flags;
+	u32 wq_ctx_idr = wq_ctx;
+	struct completion *wq = NULL;
+
+	spin_lock_irqsave(&scm->waitq.idr_lock, flags);
+	wq = idr_find(&scm->waitq.idr, wq_ctx);
+	if (wq)
+		goto out;
+
+	wq = &scm->waitq.waitq_comp;
+
+	err = idr_alloc_u32(&scm->waitq.idr, wq, &wq_ctx_idr,
+			    U32_MAX, GFP_ATOMIC);
+	if (err < 0)
+		wq = ERR_PTR(err);
+
+out:
+	spin_unlock_irqrestore(&scm->waitq.idr_lock, flags);
+	return wq;
+}
+
+static int qcom_scm_waitq_wakeup(struct qcom_scm *scm, unsigned int wq_ctx, bool wake_all)
+{
+	struct completion *wq_to_wake;
+
+	wq_to_wake = qcom_scm_lookup_wq(scm, wq_ctx);
+	if (IS_ERR_OR_NULL(wq_to_wake)) {
+		dev_err(scm->dev, "No waitqueue found for wq_ctx %d: %ld\n",
+			wq_ctx, PTR_ERR(wq_to_wake));
+		return PTR_ERR(wq_to_wake) ? : -EINVAL;
+	}
+
+	if (wake_all)
+		complete_all(wq_to_wake);
+	else
+		complete(wq_to_wake);
+
+	return 0;
+}
+
+static irqreturn_t qcom_scm_irq_handler(int irq, void *data)
+{
+	int ret;
+	struct qcom_scm *scm = data;
+	u32 wq_ctx, flags, more_pending = 0;
+
+	do {
+		ret = scm_get_wq_ctx(&wq_ctx, &flags, &more_pending);
+		if (ret) {
+			dev_err(scm->dev, "GET_WQ_CTX SMC call failed: %d\n", ret);
+			goto out;
+		}
+
+		if (flags != QCOM_SMC_WAITQ_FLAG_WAKE_ONE &&
+		    flags != QCOM_SMC_WAITQ_FLAG_WAKE_ALL) {
+			dev_err(scm->dev, "Invalid flags found for wq_ctx: %u\n", flags);
+			goto out;
+		}
+
+		ret = qcom_scm_waitq_wakeup(scm, wq_ctx, !!(flags & QCOM_SMC_WAITQ_FLAG_WAKE_ALL));
+		if (ret)
+			goto out;
+	} while (more_pending);
+
+out:
+	return IRQ_HANDLED;
+}
+
 static int qcom_scm_probe(struct platform_device *pdev)
 {
 	struct qcom_scm *scm;
 	unsigned long clks;
-	int ret;
+	int irq, ret;
 
 	scm = devm_kzalloc(&pdev->dev, sizeof(*scm), GFP_KERNEL);
 	if (!scm)
@@ -1399,9 +1486,29 @@ static int qcom_scm_probe(struct platform_device *pdev)
 	if (ret)
 		return ret;
 
+	platform_set_drvdata(pdev, scm);
+
 	__scm = scm;
 	__scm->dev = &pdev->dev;
 
+	spin_lock_init(&__scm->waitq.idr_lock);
+	idr_init(&__scm->waitq.idr);
+	init_completion(&__scm->waitq.waitq_comp);
+
+	irq = platform_get_irq(pdev, 0);
+	if (irq < 0) {
+		if (irq != -ENXIO)
+			return irq;
+	} else {
+		ret = devm_request_threaded_irq(__scm->dev, irq, NULL, qcom_scm_irq_handler,
+						IRQF_ONESHOT, "qcom-scm", __scm);
+		if (ret < 0) {
+			dev_err(scm->dev, "Failed to request qcom-scm irq: %d\n", ret);
+			idr_destroy(&__scm->waitq.idr);
+			return ret;
+		}
+	}
+
 	__get_convention();
 
 	/*
@@ -1417,6 +1524,12 @@ static int qcom_scm_probe(struct platform_device *pdev)
 
 static void qcom_scm_shutdown(struct platform_device *pdev)
 {
+	unsigned long flags;
+
+	spin_lock_irqsave(&__scm->waitq.idr_lock, flags);
+	idr_destroy(&__scm->waitq.idr);
+	spin_unlock_irqrestore(&__scm->waitq.idr_lock, flags);
+
 	/* Clean shutdown, disable download mode to allow normal restart */
 	if (download_mode)
 		qcom_scm_set_download_mode(false);
diff --git a/drivers/firmware/qcom_scm.h b/drivers/firmware/qcom_scm.h
index db3d08a01209..323cb49d4976 100644
--- a/drivers/firmware/qcom_scm.h
+++ b/drivers/firmware/qcom_scm.h
@@ -60,6 +60,10 @@ struct qcom_scm_res {
 	u64 result[MAX_QCOM_SCM_RETS];
 };
 
+struct qcom_scm;
+extern struct completion *qcom_scm_lookup_wq(struct qcom_scm *scm, u32 wq_ctx);
+extern int scm_get_wq_ctx(u32 *wq_ctx, u32 *flags, u32 *more_pending);
+
 #define SCM_SMC_FNID(s, c)	((((s) & 0xFF) << 8) | ((c) & 0xFF))
 extern int __scm_smc_call(struct device *dev, const struct qcom_scm_desc *desc,
 			  enum qcom_scm_convention qcom_convention,
@@ -129,6 +133,10 @@ extern int scm_legacy_call(struct device *dev, const struct qcom_scm_desc *desc,
 #define QCOM_SCM_SMMU_CONFIG_ERRATA1		0x03
 #define QCOM_SCM_SMMU_CONFIG_ERRATA1_CLIENT_ALL	0x02
 
+#define QCOM_SCM_SVC_WAITQ			0x24
+#define QCOM_SCM_WAITQ_RESUME			0x02
+#define QCOM_SCM_WAITQ_GET_WQ_CTX		0x03
+
 /* common error codes */
 #define QCOM_SCM_V2_EBUSY	-12
 #define QCOM_SCM_ENOMEM		-5
@@ -137,6 +145,7 @@ extern int scm_legacy_call(struct device *dev, const struct qcom_scm_desc *desc,
 #define QCOM_SCM_EINVAL_ARG	-2
 #define QCOM_SCM_ERROR		-1
 #define QCOM_SCM_INTERRUPTED	1
+#define QCOM_SCM_WAITQ_SLEEP	2
 
 static inline int qcom_scm_remap_error(int err)
 {
-- 
2.17.1

Re: [PATCH V5 1/2] dt-bindings: firmware: qcom-scm: Add optional interrupt

[Krzysztof Kozlowski]

On 23/11/2022 21:46, Sibi Sankar wrote:
> From: Guru Das Srinagesh <quic_gurus@quicinc.com>
> 
> Add an interrupt specification to the bindings to support the wait-queue
> feature.

Subject - this is qcom,scm, not qcom-scm.


> 
> Signed-off-by: Guru Das Srinagesh <quic_gurus@quicinc.com>
> Signed-off-by: Sibi Sankar <quic_sibis@quicinc.com>
> Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> ---
> 
> v5:
> - Pick up R-b
> 
> v4:
> - Qualify bindings [Krzysztoff]
> 
>  Documentation/devicetree/bindings/firmware/qcom,scm.yaml | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/firmware/qcom,scm.yaml b/Documentation/devicetree/bindings/firmware/qcom,scm.yaml
> index 25688571ee7c..aea6e0c86a89 100644
> --- a/Documentation/devicetree/bindings/firmware/qcom,scm.yaml
> +++ b/Documentation/devicetree/bindings/firmware/qcom,scm.yaml
> @@ -73,6 +73,12 @@ properties:
>    '#reset-cells':
>      const: 1
>  
> +  interrupts:
> +    description:
> +      The wait-queue interrupt that firmware raises as part of handshake
> +      protocol to handle sleeping SCM calls.
> +    maxItems: 1

Which devices have interrupts?

We talked about it here:
https://lore.kernel.org/all/2464d90f-64e9-5e3c-404b-10394c3bc302@quicinc.com/
and here:
https://lore.kernel.org/all/c20edd0d-7613-5683-60e7-54317cac6e0b@linaro.org/

But I still don't get which devices support it and which do not.


BTW:
https://lore.kernel.org/all/20221122092345.44369-2-krzysztof.kozlowski@linaro.org/


Best regards,
Krzysztof

Re: [PATCH V5 2/2] firmware: qcom: scm: Add wait-queue handling logic

[Krzysztof Kozlowski]

On 23/11/2022 21:46, Sibi Sankar wrote:
> From: Guru Das Srinagesh <quic_gurus@quicinc.com>
> 
> When the firmware (FW) supports multiple requests per VM, multiple requests
> from the same/different VM can reach the firmware at the same time. Since
> the firmware currently being used has limited resources, it guards them
> with a resource lock and puts requests on a wait-queue internally and
> signals to HLOS that it is doing so. It does this by returning a new return
> value in addition to success or error: SCM_WAITQ_SLEEP. A sleeping SCM call
> can be woken up by an interrupt that the FW raises.

Just two nitpicks while browsing the code...

>  static int qcom_scm_probe(struct platform_device *pdev)
>  {
>  	struct qcom_scm *scm;
>  	unsigned long clks;
> -	int ret;
> +	int irq, ret;
>  
>  	scm = devm_kzalloc(&pdev->dev, sizeof(*scm), GFP_KERNEL);
>  	if (!scm)
> @@ -1399,9 +1486,29 @@ static int qcom_scm_probe(struct platform_device *pdev)
>  	if (ret)
>  		return ret;
>  
> +	platform_set_drvdata(pdev, scm);
> +
>  	__scm = scm;
>  	__scm->dev = &pdev->dev;
>  
> +	spin_lock_init(&__scm->waitq.idr_lock);
> +	idr_init(&__scm->waitq.idr);
> +	init_completion(&__scm->waitq.waitq_comp);
> +
> +	irq = platform_get_irq(pdev, 0);
> +	if (irq < 0) {
> +		if (irq != -ENXIO)
> +			return irq;
> +	} else {
> +		ret = devm_request_threaded_irq(__scm->dev, irq, NULL, qcom_scm_irq_handler,
> +						IRQF_ONESHOT, "qcom-scm", __scm);
> +		if (ret < 0) {
> +			dev_err(scm->dev, "Failed to request qcom-scm irq: %d\n", ret);
> +			idr_destroy(&__scm->waitq.idr);
> +			return ret;

return dev_err_probe().

> +		}
> +	}
> +
>  	__get_convention();
>  
>  	/*
> @@ -1417,6 +1524,12 @@ static int qcom_scm_probe(struct platform_device *pdev)
>  
>  static void qcom_scm_shutdown(struct platform_device *pdev)
>  {
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&__scm->waitq.idr_lock, flags);
> +	idr_destroy(&__scm->waitq.idr);
> +	spin_unlock_irqrestore(&__scm->waitq.idr_lock, flags);
> +
>  	/* Clean shutdown, disable download mode to allow normal restart */
>  	if (download_mode)
>  		qcom_scm_set_download_mode(false);
> diff --git a/drivers/firmware/qcom_scm.h b/drivers/firmware/qcom_scm.h
> index db3d08a01209..323cb49d4976 100644
> --- a/drivers/firmware/qcom_scm.h
> +++ b/drivers/firmware/qcom_scm.h
> @@ -60,6 +60,10 @@ struct qcom_scm_res {
>  	u64 result[MAX_QCOM_SCM_RETS];
>  };
>  
> +struct qcom_scm;
> +extern struct completion *qcom_scm_lookup_wq(struct qcom_scm *scm, u32 wq_ctx);
> +extern int scm_get_wq_ctx(u32 *wq_ctx, u32 *flags, u32 *more_pending);

No need for externs for new entries.

Best regards,
Krzysztof

Re: [PATCH V5 1/2] dt-bindings: firmware: qcom-scm: Add optional interrupt

[Sibi Sankar]

Hey Krzysztof,

Thanks for taking time to review the series.

On 11/24/22 16:43, Krzysztof Kozlowski wrote:
> On 23/11/2022 21:46, Sibi Sankar wrote:
>> From: Guru Das Srinagesh <quic_gurus@quicinc.com>
>>
>> Add an interrupt specification to the bindings to support the wait-queue
>> feature.
> 
> Subject - this is qcom,scm, not qcom-scm.

ack

> 
> 
>>
>> Signed-off-by: Guru Das Srinagesh <quic_gurus@quicinc.com>
>> Signed-off-by: Sibi Sankar <quic_sibis@quicinc.com>
>> Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
>> ---
>>
>> v5:
>> - Pick up R-b
>>
>> v4:
>> - Qualify bindings [Krzysztoff]
>>
>>   Documentation/devicetree/bindings/firmware/qcom,scm.yaml | 6 ++++++
>>   1 file changed, 6 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/firmware/qcom,scm.yaml b/Documentation/devicetree/bindings/firmware/qcom,scm.yaml
>> index 25688571ee7c..aea6e0c86a89 100644
>> --- a/Documentation/devicetree/bindings/firmware/qcom,scm.yaml
>> +++ b/Documentation/devicetree/bindings/firmware/qcom,scm.yaml
>> @@ -73,6 +73,12 @@ properties:
>>     '#reset-cells':
>>       const: 1
>>   
>> +  interrupts:
>> +    description:
>> +      The wait-queue interrupt that firmware raises as part of handshake
>> +      protocol to handle sleeping SCM calls.
>> +    maxItems: 1
> 
> Which devices have interrupts?
> 
> We talked about it here:
> https://lore.kernel.org/all/2464d90f-64e9-5e3c-404b-10394c3bc302@quicinc.com/
> and here:
> https://lore.kernel.org/all/c20edd0d-7613-5683-60e7-54317cac6e0b@linaro.org/
> 
> But I still don't get which devices support it and which do not.

lol, I thought we reached a consensus earlier because of the "ok" and
R-b. Like I explained earlier the bootloader would be adding interrupt
on the fly, wouldn't such cases cause binding check failure if we list
all the devices supporting it? Also some of the SM8450 devices in the
wild would be running firmware not having the feature but I guess
eventually most of the them will end up supporting the feature in the
end.

> 
> 
> BTW: > 
https://lore.kernel.org/all/20221122092345.44369-2-krzysztof.kozlowski@linaro.org/

Yup had a look at ^^, IIRC there are additional SoCs that can have the
interconnects specified in their device tree.

> 
> 
> Best regards,
> Krzysztof
> 

Re: [PATCH V5 1/2] dt-bindings: firmware: qcom-scm: Add optional interrupt

[Krzysztof Kozlowski]

On 28/11/2022 06:57, Sibi Sankar wrote:

>>
>> Which devices have interrupts?
>>
>> We talked about it here:
>> https://lore.kernel.org/all/2464d90f-64e9-5e3c-404b-10394c3bc302@quicinc.com/
>> and here:
>> https://lore.kernel.org/all/c20edd0d-7613-5683-60e7-54317cac6e0b@linaro.org/
>>
>> But I still don't get which devices support it and which do not.
> 
> lol, I thought we reached a consensus earlier because of the "ok" and
> R-b. Like I explained earlier the bootloader would be adding interrupt
> on the fly, wouldn't such cases cause binding check failure if we list
> all the devices supporting it? 

What type of failure? I don't get. Is this interrupt valid for SM8250?
SDM845? MSM8996? and so on? Now you make it valid.

> Also some of the SM8450 devices in the
> wild would be running firmware not having the feature but I guess
> eventually most of the them will end up supporting the feature in the
> end.

That's not what I meant. Your patch describes the case for one variant
but you are affecting all of them.



Best regards,
Krzysztof

Re: [PATCH V5 1/2] dt-bindings: firmware: qcom-scm: Add optional interrupt

[Sibi Sankar]

On 11/28/22 14:00, Krzysztof Kozlowski wrote:
> On 28/11/2022 06:57, Sibi Sankar wrote:
> 
>>>
>>> Which devices have interrupts?
>>>
>>> We talked about it here:
>>> https://lore.kernel.org/all/2464d90f-64e9-5e3c-404b-10394c3bc302@quicinc.com/
>>> and here:
>>> https://lore.kernel.org/all/c20edd0d-7613-5683-60e7-54317cac6e0b@linaro.org/
>>>
>>> But I still don't get which devices support it and which do not.
>>
>> lol, I thought we reached a consensus earlier because of the "ok" and
>> R-b. Like I explained earlier the bootloader would be adding interrupt
>> on the fly, wouldn't such cases cause binding check failure if we list
>> all the devices supporting it?
> 
> What type of failure? I don't get. Is this interrupt valid for SM8250?
> SDM845? MSM8996? and so on? Now you make it valid.

ok if we mark the interrupt as required for SM8450 and not specify the
interrupt in the board file (since the bootloader will be adding it on
the fly), dtbs_check will throw 'interrupts' is a required property for
the board file. This was the failure I was talking about.

> 
>> Also some of the SM8450 devices in the
>> wild would be running firmware not having the feature but I guess
>> eventually most of the them will end up supporting the feature in the
>> end.
> 
> That's not what I meant. Your patch describes the case for one variant
> but you are affecting all of them.

Not really, the driver treats interrupts as optional. If the interrupt
isn't present we assume that the feature isn't supported. If the
bootloader adds the property during boot then we assume the fw has
waitqueue support.

- Sibi

> 
> 
> 
> Best regards,
> Krzysztof
> 

Re: [PATCH V5 1/2] dt-bindings: firmware: qcom-scm: Add optional interrupt

[Krzysztof Kozlowski]

On 29/11/2022 11:05, Sibi Sankar wrote:
> On 11/28/22 14:00, Krzysztof Kozlowski wrote:
>> On 28/11/2022 06:57, Sibi Sankar wrote:
>>
>>>>
>>>> Which devices have interrupts?
>>>>
>>>> We talked about it here:
>>>> https://lore.kernel.org/all/2464d90f-64e9-5e3c-404b-10394c3bc302@quicinc.com/
>>>> and here:
>>>> https://lore.kernel.org/all/c20edd0d-7613-5683-60e7-54317cac6e0b@linaro.org/
>>>>
>>>> But I still don't get which devices support it and which do not.
>>>
>>> lol, I thought we reached a consensus earlier because of the "ok" and
>>> R-b. Like I explained earlier the bootloader would be adding interrupt
>>> on the fly, wouldn't such cases cause binding check failure if we list
>>> all the devices supporting it?
>>
>> What type of failure? I don't get. Is this interrupt valid for SM8250?
>> SDM845? MSM8996? and so on? Now you make it valid.
> 
> ok if we mark the interrupt as required for SM8450 and not specify the
> interrupt in the board file (since the bootloader will be adding it on
> the fly), dtbs_check will throw 'interrupts' is a required property for
> the board file. This was the failure I was talking about.

OK, but no one said here about making it required. So how this issue can
happen?

Please read above chapter again. I said nothing about required, but I
said "valid".

> 
>>
>>> Also some of the SM8450 devices in the
>>> wild would be running firmware not having the feature but I guess
>>> eventually most of the them will end up supporting the feature in the
>>> end.
>>
>> That's not what I meant. Your patch describes the case for one variant
>> but you are affecting all of them.
> 
> Not really, the driver treats interrupts as optional. 

Linux implementation matters less. We talk about device/hardware
(firmware in this case).

> If the interrupt
> isn't present we assume that the feature isn't supported. If the
> bootloader adds the property during boot then we assume the fw has
> waitqueue support.

Sure, my question stays. Which devices do not support it at all?

Best regards,
Krzysztof