linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [RFC PATCH v2 0/8] Introduce atomic support for SCMI transports
@ 2021-07-05 17:10 Cristian Marussi
  2021-07-05 17:10 ` [RFC PATCH v2 1/8] firmware: arm_scmi: Add configurable polling mode for transports Cristian Marussi
                   ` (7 more replies)
  0 siblings, 8 replies; 9+ messages in thread
From: Cristian Marussi @ 2021-07-05 17:10 UTC (permalink / raw)
  To: linux-kernel, linux-arm-kernel
  Cc: sudeep.holla, james.quinlan, Jonathan.Cameron, f.fainelli,
	etienne.carriere, vincent.guittot, souvik.chakravarty,
	cristian.marussi

Hi all,

This RFC series mainly aims to introduce atomic support for transports
that can support it.

At first in [1/8], as a closely related addition, it is introduced a
common way for a transport to signal to the SCMI core that it does not
offer completion interrupts, so that the usual polling behaviour based
on .poll_done() will be required: this can be done enabling statically
a global polling behaviour for the whole transport with flag
scmi_desc.force_polling OR dynamically enabling at runtime such polling
behaviour on a per-channel basis with scmi_chan_info.needs_polling,
typically during .chan_setup(). The usual per-command polling selection
behaviour based on hdr.poll_completion is preserved as before.

Then in [2/8], a transport that supports atomic operations on its tx path
can now declare itself as .atomic_capable and as a consequence the SCMI
core will refrain itself from sleeping on the correspondent rx-path.

In [5/8] a simple method is introduced so that an SCMI driver can easily
query the core to check if the currently used transport is configured to
behave in an atomic manner: in this way, interested SCMI driver users, like
Clock framework [6/8], can optionally support atomic operations when
operating on an atomically configured transport.

Finally there are 2 *tentative" patch for SMC transport: at first [7/8]
ports SMC to use the common core completions when completion interrupt is
available or otherwise revert to use common core polling mechanism above
introduced; then in [8/8] SMC is converted to be .atomic_capable by
substituting the mutexes with busy-waiting to keep the channel 'locked'.

SMC changes have NOT been tested so far (I cannot), AND they are just a
proposal at this stage to try to better abstract and unify behaviour with
the SCMI core; both patches are completely intended as RFCs, though, not
only regarding their implementation but even their mere existence is RFC:
I mean maybe we just don't want to do such kind of unification/abstraction,
and I can just drop those SMC patches if unwanted; any feedback welcome.

Atomic support has been minimally tested against the upcoming virtio
transport V5 series, while polling has been tested with mailbox transports.

The series is based on SCMI VirtIO Transport support V5 [1] (since it will
be the main prospective user of atomic mode) and, as such, it is also
publicly available from ARM GitLab [2].
(Note that in order to use/test atomic mode on virtio you'll have to enable
 it setting .atomic_capable = true in virtio.c::scmi_virtio_desc)

Given the RFC status of the series in general I still not have CCed any
maintainer out of SCMI subsystem.

Any feedback welcome.

Thanks,

Cristian

---

[1]:https://lore.kernel.org/linux-arm-kernel/20210705144914.35094-1-cristian.marussi@arm.com/
[2]:https://gitlab.arm.com/linux-arm/linux-cm/-/commits/scmi_atomic_transport_V2_on_virtio/

Cristian Marussi (8):
  firmware: arm_scmi: Add configurable polling mode for transports
  firmware: arm_scmi: Add support for atomic transports
  include: trace: Add new scmi_xfer_response_wait event
  firmware: arm_scmi: Use new trace event scmi_xfer_response_wait
  firmware: arm_scmi: Add is_transport_atomic() handle method
  clk: scmi: Support atomic enable/disable API
  firmware: arm_scmi: Make smc transport use common completions
  firmware: arm_scmi: Make smc transport atomic

 drivers/clk/clk-scmi.c             |  44 ++++--
 drivers/firmware/arm_scmi/common.h |  13 ++
 drivers/firmware/arm_scmi/driver.c | 206 +++++++++++++++++++++++------
 drivers/firmware/arm_scmi/smc.c    |  60 +++++----
 include/linux/scmi_protocol.h      |   8 ++
 include/trace/events/scmi.h        |  28 ++++
 6 files changed, 288 insertions(+), 71 deletions(-)

-- 
2.17.1


^ permalink raw reply	[flat|nested] 9+ messages in thread

* [RFC PATCH v2 1/8] firmware: arm_scmi: Add configurable polling mode for transports
  2021-07-05 17:10 [RFC PATCH v2 0/8] Introduce atomic support for SCMI transports Cristian Marussi
@ 2021-07-05 17:10 ` Cristian Marussi
  2021-07-05 17:10 ` [RFC PATCH v2 2/8] firmware: arm_scmi: Add support for atomic transports Cristian Marussi
                   ` (6 subsequent siblings)
  7 siblings, 0 replies; 9+ messages in thread
From: Cristian Marussi @ 2021-07-05 17:10 UTC (permalink / raw)
  To: linux-kernel, linux-arm-kernel
  Cc: sudeep.holla, james.quinlan, Jonathan.Cameron, f.fainelli,
	etienne.carriere, vincent.guittot, souvik.chakravarty,
	cristian.marussi

SCMI communications along TX channels can optionally be provided of a
completion interrupt; when such interrupt is not available, command
transactions should rely on polling, where the SCMI core takes care to
repeatedly evaluates the transport-specific .poll_done() function to
determine if a request was completed or timed out.

Such mechanism is already present and working on a single transfer base:
SCMI protocols can indeed enable hdr.poll_completion on specific commands
ahead of each transfer and cause that transaction to be handled with
polling.

Introduce a couple of flags to be able to enforce such polling behaviour
globally at will:

 - scmi_desc.force_polling: to statically switch the whole transport to
 			    polling mode.

 - scmi_chan_info.needs_polling: to switch a single channel dynamically to
 				 polling mode if at runtime is determined
				 that no completion interrupt was available
				 for such channel.

Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
---
 drivers/firmware/arm_scmi/common.h | 10 ++++++++++
 drivers/firmware/arm_scmi/driver.c | 17 +++++++++++++++++
 2 files changed, 27 insertions(+)

diff --git a/drivers/firmware/arm_scmi/common.h b/drivers/firmware/arm_scmi/common.h
index 61f1d7e1742b..eb45f2dced03 100644
--- a/drivers/firmware/arm_scmi/common.h
+++ b/drivers/firmware/arm_scmi/common.h
@@ -320,11 +320,18 @@ void scmi_protocol_release(const struct scmi_handle *handle, u8 protocol_id);
  * @dev: Reference to device in the SCMI hierarchy corresponding to this
  *	 channel
  * @handle: Pointer to SCMI entity handle
+ * @needs_polling: Flag to indicate that this channel has no completion
+ *		   interrupt mechanism, so it needs SCMI core to poll, using
+ *		   .poll_done(), to determine when a command has completed.
+ *		   This can be dynamically set by transports at run-time inside
+ *		   their provided .chan_setup() when they determine no
+ *		   completion interrupt is available.
  * @transport_info: Transport layer related information
  */
 struct scmi_chan_info {
 	struct device *dev;
 	struct scmi_handle *handle;
+	bool needs_polling;
 	void *transport_info;
 };
 
@@ -386,6 +393,8 @@ struct scmi_device *scmi_child_dev_find(struct device *parent,
  * @using_xfers_delegation: A flag to indicate if the described transport will
  *			    handle delegated xfers, so the core can derive
  *			    proper related assumptions.
+ * @force_polling: Flag to force this whole transport to use SCMI core polling
+ *		   mechanism instead of completion interrupts even if available.
  */
 struct scmi_desc {
 	int (*init)(void);
@@ -395,6 +404,7 @@ struct scmi_desc {
 	int max_msg;
 	int max_msg_size;
 	bool using_xfers_delegation;
+	bool force_polling;
 };
 
 #ifdef CONFIG_ARM_SCMI_TRANSPORT_MAILBOX
diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c
index 218901492229..d1814c58f4ba 100644
--- a/drivers/firmware/arm_scmi/driver.c
+++ b/drivers/firmware/arm_scmi/driver.c
@@ -816,6 +816,15 @@ static void scmi_handle_response(struct scmi_chan_info *cinfo, u32 msg_hdr)
 
 	scmi_xfer_state_update(xfer);
 
+	/* Discard unexpected messages when polling is active. */
+	if (xfer->hdr.type != MSG_TYPE_DELAYED_RESP &&
+	    xfer->hdr.poll_completion) {
+		WARN_ON_ONCE(1);
+		dev_dbg(cinfo->dev,
+			"Completion IRQ received but using polling. Ignore.\n");
+		return;
+	}
+
 	/* rx.len could be shrunk in the sync do_xfer, so reset to maxsz */
 	if (xfer->hdr.type == MSG_TYPE_DELAYED_RESP)
 		xfer->rx.len = info->desc->max_msg_size;
@@ -929,6 +938,9 @@ static int do_xfer(const struct scmi_protocol_handle *ph,
 	if (unlikely(!cinfo))
 		return -EINVAL;
 
+	if (info->desc->force_polling || cinfo->needs_polling)
+		xfer->hdr.poll_completion = true;
+
 	trace_scmi_xfer_begin(xfer->transfer_id, xfer->hdr.id,
 			      xfer->hdr.protocol_id, xfer->hdr.seq,
 			      xfer->hdr.poll_completion);
@@ -1643,6 +1655,11 @@ static int scmi_chan_setup(struct scmi_info *info, struct device *dev,
 	if (ret)
 		return ret;
 
+	if (tx && (cinfo->needs_polling || info->desc->force_polling))
+		dev_info(dev,
+			 "Enabled polling mode for TX channel - prot_id:%d\n",
+			 prot_id);
+
 idr_alloc:
 	ret = idr_alloc(idr, cinfo, prot_id, prot_id + 1, GFP_KERNEL);
 	if (ret != prot_id) {
-- 
2.17.1


^ permalink raw reply	[flat|nested] 9+ messages in thread

* [RFC PATCH v2 2/8] firmware: arm_scmi: Add support for atomic transports
  2021-07-05 17:10 [RFC PATCH v2 0/8] Introduce atomic support for SCMI transports Cristian Marussi
  2021-07-05 17:10 ` [RFC PATCH v2 1/8] firmware: arm_scmi: Add configurable polling mode for transports Cristian Marussi
@ 2021-07-05 17:10 ` Cristian Marussi
  2021-07-05 17:10 ` [RFC PATCH v2 3/8] include: trace: Add new scmi_xfer_response_wait event Cristian Marussi
                   ` (5 subsequent siblings)
  7 siblings, 0 replies; 9+ messages in thread
From: Cristian Marussi @ 2021-07-05 17:10 UTC (permalink / raw)
  To: linux-kernel, linux-arm-kernel
  Cc: sudeep.holla, james.quinlan, Jonathan.Cameron, f.fainelli,
	etienne.carriere, vincent.guittot, souvik.chakravarty,
	cristian.marussi

An SCMI transport can declare itself as .atomic_capable in order to signal
to the SCMI core that all its transmit path can be executed in atomic
context: the core as a consequence will take care not to sleep to in the
corresponding rx path while waiting for a response or a delayed response.

Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
---
 drivers/firmware/arm_scmi/common.h |   3 +
 drivers/firmware/arm_scmi/driver.c | 168 ++++++++++++++++++++++-------
 2 files changed, 132 insertions(+), 39 deletions(-)

diff --git a/drivers/firmware/arm_scmi/common.h b/drivers/firmware/arm_scmi/common.h
index eb45f2dced03..9be04e4bb2f5 100644
--- a/drivers/firmware/arm_scmi/common.h
+++ b/drivers/firmware/arm_scmi/common.h
@@ -395,6 +395,8 @@ struct scmi_device *scmi_child_dev_find(struct device *parent,
  *			    proper related assumptions.
  * @force_polling: Flag to force this whole transport to use SCMI core polling
  *		   mechanism instead of completion interrupts even if available.
+ * @atomic_capable: Flag to indicate that this transport is assured not to sleep
+ *		    on the TX path.
  */
 struct scmi_desc {
 	int (*init)(void);
@@ -405,6 +407,7 @@ struct scmi_desc {
 	int max_msg_size;
 	bool using_xfers_delegation;
 	bool force_polling;
+	bool atomic_capable;
 };
 
 #ifdef CONFIG_ARM_SCMI_TRANSPORT_MAILBOX
diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c
index d1814c58f4ba..2e920df73e46 100644
--- a/drivers/firmware/arm_scmi/driver.c
+++ b/drivers/firmware/arm_scmi/driver.c
@@ -839,6 +839,10 @@ static void scmi_handle_response(struct scmi_chan_info *cinfo, u32 msg_hdr)
 		info->desc->ops->clear_channel(cinfo);
 		complete(xfer->async_done);
 	} else {
+		/*
+		 * This same xfer->done completion is used in atomic mode as a
+		 * flag for polling.
+		 */
 		complete(&xfer->done);
 	}
 
@@ -890,8 +894,6 @@ static void xfer_put(const struct scmi_protocol_handle *ph,
 	__scmi_xfer_put(&info->tx_minfo, xfer);
 }
 
-#define SCMI_MAX_POLL_TO_NS	(100 * NSEC_PER_USEC)
-
 static bool scmi_xfer_done_no_timeout(struct scmi_chan_info *cinfo,
 				      struct scmi_xfer *xfer, ktime_t stop)
 {
@@ -906,6 +908,90 @@ static bool scmi_xfer_done_no_timeout(struct scmi_chan_info *cinfo,
 	       ktime_after(ktime_get(), stop);
 }
 
+static bool xfer_complete_or_timeout(struct completion *done, ktime_t stop)
+{
+	return try_wait_for_completion(done) || ktime_after(ktime_get(), stop);
+}
+
+static int spin_for_completion_timeout(struct completion *done, int timeout_ms)
+{
+	ktime_t stop = ktime_add_ms(ktime_get(), timeout_ms);
+
+	spin_until_cond(xfer_complete_or_timeout(done, stop));
+	if (ktime_after(ktime_get(), stop))
+		return -ETIMEDOUT;
+
+	return 0;
+}
+
+/**
+ * scmi_wait_for_message_response  - An helper to group all the possible ways of
+ * waiting for a synchronous message response.
+ *
+ * @cinfo: SCMI channel info
+ * @xfer: Reference to the transfer being waited for.
+ *
+ * Chooses waiting strategy (sleep-waiting vs busy-waiting) depending on flags
+ * configuration like xfer->hdr.poll_completion and scmi_desc.atomic.capable.
+ *
+ * Return: 0 on Success, error otherwise.
+ */
+static int scmi_wait_for_message_response(struct scmi_chan_info *cinfo,
+					  struct scmi_xfer *xfer)
+{
+	struct scmi_info *info = handle_to_scmi_info(cinfo->handle);
+	struct device *dev = info->dev;
+	int ret = 0, timeout_ms = info->desc->max_rx_timeout_ms;
+
+	if (!xfer->hdr.poll_completion) {
+		if (!info->desc->atomic_capable) {
+			if (!wait_for_completion_timeout(&xfer->done,
+							 msecs_to_jiffies(timeout_ms))) {
+				dev_err(dev, "timed out in resp(caller: %pS)\n",
+					(void *)_RET_IP_);
+				ret = -ETIMEDOUT;
+			}
+		} else {
+			/* Poll on xfer->done waiting for completion by interrupt */
+			ret = spin_for_completion_timeout(&xfer->done,
+							  timeout_ms);
+			if (ret)
+				dev_err(dev,
+					"timed out in resp(caller: %pS) - atomic\n",
+					(void *)_RET_IP_);
+		}
+	} else {
+		/*
+		 * Poll on xfer using transport provided .poll_done();
+		 * assumes no completion interrupt was available.
+		 */
+		ktime_t stop = ktime_add_ms(ktime_get(), timeout_ms);
+
+		spin_until_cond(scmi_xfer_done_no_timeout(cinfo, xfer, stop));
+		if (ktime_before(ktime_get(), stop)) {
+			unsigned long flags;
+
+			/*
+			 * Do not fetch_response if an out-of-order delayed
+			 * response is being processed.
+			 */
+			spin_lock_irqsave(&xfer->lock, flags);
+			if (xfer->state == SCMI_XFER_SENT_OK) {
+				info->desc->ops->fetch_response(cinfo, xfer);
+				xfer->state = SCMI_XFER_RESP_OK;
+			}
+			spin_unlock_irqrestore(&xfer->lock, flags);
+		} else {
+			dev_err(dev,
+				"timed out in resp(caller: %pS) - polling\n",
+				(void *)_RET_IP_);
+			ret = -ETIMEDOUT;
+		}
+	}
+
+	return ret;
+}
+
 /**
  * do_xfer() - Do one transfer
  *
@@ -920,7 +1006,6 @@ static int do_xfer(const struct scmi_protocol_handle *ph,
 		   struct scmi_xfer *xfer)
 {
 	int ret;
-	int timeout;
 	const struct scmi_protocol_instance *pi = ph_to_pi(ph);
 	struct scmi_info *info = handle_to_scmi_info(pi->handle);
 	struct device *dev = info->dev;
@@ -952,37 +1037,7 @@ static int do_xfer(const struct scmi_protocol_handle *ph,
 		return ret;
 	}
 
-	if (xfer->hdr.poll_completion) {
-		ktime_t stop = ktime_add_ns(ktime_get(), SCMI_MAX_POLL_TO_NS);
-
-		spin_until_cond(scmi_xfer_done_no_timeout(cinfo, xfer, stop));
-
-		if (ktime_before(ktime_get(), stop)) {
-			unsigned long flags;
-
-			/*
-			 * Do not fetch_response if an out-of-order delayed
-			 * response is being processed.
-			 */
-			spin_lock_irqsave(&xfer->lock, flags);
-			if (xfer->state == SCMI_XFER_SENT_OK) {
-				info->desc->ops->fetch_response(cinfo, xfer);
-				xfer->state = SCMI_XFER_RESP_OK;
-			}
-			spin_unlock_irqrestore(&xfer->lock, flags);
-		} else {
-			ret = -ETIMEDOUT;
-		}
-	} else {
-		/* And we wait for the response. */
-		timeout = msecs_to_jiffies(info->desc->max_rx_timeout_ms);
-		if (!wait_for_completion_timeout(&xfer->done, timeout)) {
-			dev_err(dev, "timed out in resp(caller: %pS)\n",
-				(void *)_RET_IP_);
-			ret = -ETIMEDOUT;
-		}
-	}
-
+	ret = scmi_wait_for_message_response(cinfo, xfer);
 	if (!ret && xfer->hdr.status)
 		ret = scmi_to_linux_errno(xfer->hdr.status);
 
@@ -1004,7 +1059,7 @@ static void reset_rx_to_maxsz(const struct scmi_protocol_handle *ph,
 	xfer->rx.len = info->desc->max_msg_size;
 }
 
-#define SCMI_MAX_RESPONSE_TIMEOUT	(2 * MSEC_PER_SEC)
+#define SCMI_DRESP_TIMEOUT	(2 * MSEC_PER_SEC)
 
 /**
  * do_xfer_with_response() - Do one transfer and wait until the delayed
@@ -1013,22 +1068,57 @@ static void reset_rx_to_maxsz(const struct scmi_protocol_handle *ph,
  * @ph: Pointer to SCMI protocol handle
  * @xfer: Transfer to initiate and wait for response
  *
+ * Avois sleeping in favour of busy-waiting if the underlying transport was
+ * declared as .atomic_capable.
+ *
+ * Note that using asynchronous commands when running on top of atomic
+ * transports should be avoided since it could cause long busy-waiting here,
+ * but, once a transport is declared atomic, upper layers using the SCMI stack
+ * can freely make assumptions about the 'non-sleeping' nature of the stack
+ * (e.g. Clock framework) and it cannot be excluded that asynchronous commands
+ * could be exposed by the platform and so used.
+ *
+ * The only other option would have been to refrain from using any asynchronous
+ * command even if made available, when an atomic transport is detected, and
+ * instead forcibly use the synchronous version (thing that can be easily
+ * attained at the protocol layer), but this would also have led to longer
+ * stalls of the channel for synchronous commands and possibly timeouts.
+ * (in other words there is usually a good reason if a platform provides an
+ *  asynchronous version of a command and we should prefer to use it)
+ *
  * Return: -ETIMEDOUT in case of no delayed response, if transmit error,
  *	return corresponding error, else if all goes well, return 0.
  */
 static int do_xfer_with_response(const struct scmi_protocol_handle *ph,
 				 struct scmi_xfer *xfer)
 {
-	int ret, timeout = msecs_to_jiffies(SCMI_MAX_RESPONSE_TIMEOUT);
+	int ret, timeout = msecs_to_jiffies(SCMI_DRESP_TIMEOUT);
+	const struct scmi_protocol_instance *pi = ph_to_pi(ph);
+	struct scmi_info *info = handle_to_scmi_info(pi->handle);
 	DECLARE_COMPLETION_ONSTACK(async_response);
 
 	xfer->async_done = &async_response;
 
 	ret = do_xfer(ph, xfer);
 	if (!ret) {
-		if (!wait_for_completion_timeout(xfer->async_done, timeout))
-			ret = -ETIMEDOUT;
-		else if (xfer->hdr.status)
+		if (!info->desc->atomic_capable) {
+			if (!wait_for_completion_timeout(xfer->async_done,
+							 timeout)) {
+				dev_err(ph->dev,
+					"timed out in delayed resp(caller: %pS)\n",
+					(void *)_RET_IP_);
+				ret = -ETIMEDOUT;
+			}
+		} else {
+			ret = spin_for_completion_timeout(xfer->async_done,
+							  SCMI_DRESP_TIMEOUT);
+			if (ret)
+				dev_err(ph->dev,
+					"timed out in delayed resp(caller: %pS) - atomic\n",
+					(void *)_RET_IP_);
+		}
+
+		if (!ret && xfer->hdr.status)
 			ret = scmi_to_linux_errno(xfer->hdr.status);
 	}
 
-- 
2.17.1


^ permalink raw reply	[flat|nested] 9+ messages in thread

* [RFC PATCH v2 3/8] include: trace: Add new scmi_xfer_response_wait event
  2021-07-05 17:10 [RFC PATCH v2 0/8] Introduce atomic support for SCMI transports Cristian Marussi
  2021-07-05 17:10 ` [RFC PATCH v2 1/8] firmware: arm_scmi: Add configurable polling mode for transports Cristian Marussi
  2021-07-05 17:10 ` [RFC PATCH v2 2/8] firmware: arm_scmi: Add support for atomic transports Cristian Marussi
@ 2021-07-05 17:10 ` Cristian Marussi
  2021-07-05 17:10 ` [RFC PATCH v2 4/8] firmware: arm_scmi: Use new trace event scmi_xfer_response_wait Cristian Marussi
                   ` (4 subsequent siblings)
  7 siblings, 0 replies; 9+ messages in thread
From: Cristian Marussi @ 2021-07-05 17:10 UTC (permalink / raw)
  To: linux-kernel, linux-arm-kernel
  Cc: sudeep.holla, james.quinlan, Jonathan.Cameron, f.fainelli,
	etienne.carriere, vincent.guittot, souvik.chakravarty,
	cristian.marussi

Having a new step to trace SCMI stack while it waits for synchronous
responses is useful to analyze system performance when changing waiting
mode between polling and interrupt completion.

Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
---
 include/trace/events/scmi.h | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/include/trace/events/scmi.h b/include/trace/events/scmi.h
index f3a4b4d60714..ba82082f30d7 100644
--- a/include/trace/events/scmi.h
+++ b/include/trace/events/scmi.h
@@ -33,6 +33,34 @@ TRACE_EVENT(scmi_xfer_begin,
 		__entry->seq, __entry->poll)
 );
 
+TRACE_EVENT(scmi_xfer_response_wait,
+	TP_PROTO(int transfer_id, u8 msg_id, u8 protocol_id, u16 seq,
+		 bool poll, bool atomic),
+	TP_ARGS(transfer_id, msg_id, protocol_id, seq, poll, atomic),
+
+	TP_STRUCT__entry(
+		__field(int, transfer_id)
+		__field(u8, msg_id)
+		__field(u8, protocol_id)
+		__field(u16, seq)
+		__field(bool, poll)
+		__field(bool, atomic)
+	),
+
+	TP_fast_assign(
+		__entry->transfer_id = transfer_id;
+		__entry->msg_id = msg_id;
+		__entry->protocol_id = protocol_id;
+		__entry->seq = seq;
+		__entry->poll = poll;
+		__entry->atomic = atomic;
+	),
+
+	TP_printk("transfer_id=%d msg_id=%u protocol_id=%u seq=%u poll=%u atomic=%u",
+		__entry->transfer_id, __entry->msg_id, __entry->protocol_id,
+		__entry->seq, __entry->poll, __entry->atomic)
+);
+
 TRACE_EVENT(scmi_xfer_end,
 	TP_PROTO(int transfer_id, u8 msg_id, u8 protocol_id, u16 seq,
 		 int status),
-- 
2.17.1


^ permalink raw reply	[flat|nested] 9+ messages in thread

* [RFC PATCH v2 4/8] firmware: arm_scmi: Use new trace event scmi_xfer_response_wait
  2021-07-05 17:10 [RFC PATCH v2 0/8] Introduce atomic support for SCMI transports Cristian Marussi
                   ` (2 preceding siblings ...)
  2021-07-05 17:10 ` [RFC PATCH v2 3/8] include: trace: Add new scmi_xfer_response_wait event Cristian Marussi
@ 2021-07-05 17:10 ` Cristian Marussi
  2021-07-05 17:10 ` [RFC PATCH v2 5/8] firmware: arm_scmi: Add is_transport_atomic() handle method Cristian Marussi
                   ` (3 subsequent siblings)
  7 siblings, 0 replies; 9+ messages in thread
From: Cristian Marussi @ 2021-07-05 17:10 UTC (permalink / raw)
  To: linux-kernel, linux-arm-kernel
  Cc: sudeep.holla, james.quinlan, Jonathan.Cameron, f.fainelli,
	etienne.carriere, vincent.guittot, souvik.chakravarty,
	cristian.marussi

Use new trace event to mark start of waiting for response section.

Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
---
 drivers/firmware/arm_scmi/driver.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c
index 2e920df73e46..43aff6b26571 100644
--- a/drivers/firmware/arm_scmi/driver.c
+++ b/drivers/firmware/arm_scmi/driver.c
@@ -943,6 +943,11 @@ static int scmi_wait_for_message_response(struct scmi_chan_info *cinfo,
 	struct device *dev = info->dev;
 	int ret = 0, timeout_ms = info->desc->max_rx_timeout_ms;
 
+	trace_scmi_xfer_response_wait(xfer->transfer_id, xfer->hdr.id,
+				      xfer->hdr.protocol_id, xfer->hdr.seq,
+				      xfer->hdr.poll_completion,
+				      info->desc->atomic_capable);
+
 	if (!xfer->hdr.poll_completion) {
 		if (!info->desc->atomic_capable) {
 			if (!wait_for_completion_timeout(&xfer->done,
-- 
2.17.1


^ permalink raw reply	[flat|nested] 9+ messages in thread

* [RFC PATCH v2 5/8] firmware: arm_scmi: Add is_transport_atomic() handle method
  2021-07-05 17:10 [RFC PATCH v2 0/8] Introduce atomic support for SCMI transports Cristian Marussi
                   ` (3 preceding siblings ...)
  2021-07-05 17:10 ` [RFC PATCH v2 4/8] firmware: arm_scmi: Use new trace event scmi_xfer_response_wait Cristian Marussi
@ 2021-07-05 17:10 ` Cristian Marussi
  2021-07-05 17:10 ` [RFC PATCH v2 6/8] clk: scmi: Support atomic enable/disable API Cristian Marussi
                   ` (2 subsequent siblings)
  7 siblings, 0 replies; 9+ messages in thread
From: Cristian Marussi @ 2021-07-05 17:10 UTC (permalink / raw)
  To: linux-kernel, linux-arm-kernel
  Cc: sudeep.holla, james.quinlan, Jonathan.Cameron, f.fainelli,
	etienne.carriere, vincent.guittot, souvik.chakravarty,
	cristian.marussi

Add a method to check if the underlying transport configured for an SCMI
instance is configured to support atomic transaction of SCMI commands.

Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
---
 drivers/firmware/arm_scmi/driver.c | 16 ++++++++++++++++
 include/linux/scmi_protocol.h      |  8 ++++++++
 2 files changed, 24 insertions(+)

diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c
index 43aff6b26571..22173f747fdb 100644
--- a/drivers/firmware/arm_scmi/driver.c
+++ b/drivers/firmware/arm_scmi/driver.c
@@ -1558,6 +1558,21 @@ static void scmi_devm_protocol_put(struct scmi_device *sdev, u8 protocol_id)
 	WARN_ON(ret);
 }
 
+/**
+ * scmi_is_transport_atomic  - Method to check if underlying transport for an
+ * SCMI instance is configured as atomic.
+ *
+ * @handle: A reference to the SCMI platform instance.
+ *
+ * Return: True if transport is configured as atomic
+ */
+static bool scmi_is_transport_atomic(const struct scmi_handle *handle)
+{
+	struct scmi_info *info = handle_to_scmi_info(handle);
+
+	return info->desc->atomic_capable;
+}
+
 static inline
 struct scmi_handle *scmi_handle_get_from_info_unlocked(struct scmi_info *info)
 {
@@ -2074,6 +2089,7 @@ static int scmi_probe(struct platform_device *pdev)
 	handle->version = &info->version;
 	handle->devm_protocol_get = scmi_devm_protocol_get;
 	handle->devm_protocol_put = scmi_devm_protocol_put;
+	handle->is_transport_atomic = scmi_is_transport_atomic;
 
 	if (desc->ops->link_supplier) {
 		ret = desc->ops->link_supplier(dev);
diff --git a/include/linux/scmi_protocol.h b/include/linux/scmi_protocol.h
index 79d0a1237e6c..b28eb7c104e7 100644
--- a/include/linux/scmi_protocol.h
+++ b/include/linux/scmi_protocol.h
@@ -608,6 +608,13 @@ struct scmi_notify_ops {
  * @devm_protocol_get: devres managed method to acquire a protocol and get specific
  *		       operations and a dedicated protocol handler
  * @devm_protocol_put: devres managed method to release a protocol
+ * @is_transport_atomic: method to check if the underlying transport for this
+ *			 instance handle is configured to support atomic
+ *			 transactions for commands.
+ *			 Some users of the SCMI stack in the upper layers could
+ *			 be interested to know if they can assume SCMI
+ *			 command transactions associated to this handle will
+ *			 never sleep and act accordingly.
  * @notify_ops: pointer to set of notifications related operations
  */
 struct scmi_handle {
@@ -618,6 +625,7 @@ struct scmi_handle {
 		(*devm_protocol_get)(struct scmi_device *sdev, u8 proto,
 				     struct scmi_protocol_handle **ph);
 	void (*devm_protocol_put)(struct scmi_device *sdev, u8 proto);
+	bool (*is_transport_atomic)(const struct scmi_handle *handle);
 
 	const struct scmi_notify_ops *notify_ops;
 };
-- 
2.17.1


^ permalink raw reply	[flat|nested] 9+ messages in thread

* [RFC PATCH v2 6/8] clk: scmi: Support atomic enable/disable API
  2021-07-05 17:10 [RFC PATCH v2 0/8] Introduce atomic support for SCMI transports Cristian Marussi
                   ` (4 preceding siblings ...)
  2021-07-05 17:10 ` [RFC PATCH v2 5/8] firmware: arm_scmi: Add is_transport_atomic() handle method Cristian Marussi
@ 2021-07-05 17:10 ` Cristian Marussi
  2021-07-05 17:10 ` [RFC PATCH v2 7/8] firmware: arm_scmi: Make smc transport use common completions Cristian Marussi
  2021-07-05 17:10 ` [RFC PATCH v2 8/8] firmware: arm_scmi: Make smc transport atomic Cristian Marussi
  7 siblings, 0 replies; 9+ messages in thread
From: Cristian Marussi @ 2021-07-05 17:10 UTC (permalink / raw)
  To: linux-kernel, linux-arm-kernel
  Cc: sudeep.holla, james.quinlan, Jonathan.Cameron, f.fainelli,
	etienne.carriere, vincent.guittot, souvik.chakravarty,
	cristian.marussi

Support enable/disable clk_ops instead of prepare/unprepare when the
underlying SCMI transport is configured to support atomic transactions for
synchronous commands.

Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
---
 drivers/clk/clk-scmi.c | 44 +++++++++++++++++++++++++++++++++---------
 1 file changed, 35 insertions(+), 9 deletions(-)

diff --git a/drivers/clk/clk-scmi.c b/drivers/clk/clk-scmi.c
index 1e357d364ca2..a07117d5e5bd 100644
--- a/drivers/clk/clk-scmi.c
+++ b/drivers/clk/clk-scmi.c
@@ -88,21 +88,42 @@ static void scmi_clk_disable(struct clk_hw *hw)
 	scmi_proto_clk_ops->disable(clk->ph, clk->id);
 }
 
+/*
+ * We can provide enable/disable atomic callbacks only if the underlying SCMI
+ * transport for this SCMI instance is configured to handle synchronous commands
+ * in an atomic manner.
+ *
+ * When no SCMI atomic transport support is available we instead provide the
+ * prepare/unprepare API, as allowed by the clock framework where atomic calls
+ * are not available.
+ *
+ * Note that the provided clk_ops implementations are the same in both cases,
+ * using indeed the same underlying SCMI clock_protocol operations, it's just
+ * that they are assured to act in an atomic manner or not depending on the
+ * actual underlying SCMI transport configuration.
+ *
+ * Two distinct sets of clk_ops are provided since we could have multiple SCMI
+ * instances with different underlying transport quality, so they cannot be
+ * shared.
+ */
 static const struct clk_ops scmi_clk_ops = {
 	.recalc_rate = scmi_clk_recalc_rate,
 	.round_rate = scmi_clk_round_rate,
 	.set_rate = scmi_clk_set_rate,
-	/*
-	 * We can't provide enable/disable callback as we can't perform the same
-	 * in atomic context. Since the clock framework provides standard API
-	 * clk_prepare_enable that helps cases using clk_enable in non-atomic
-	 * context, it should be fine providing prepare/unprepare.
-	 */
 	.prepare = scmi_clk_enable,
 	.unprepare = scmi_clk_disable,
 };
 
-static int scmi_clk_ops_init(struct device *dev, struct scmi_clk *sclk)
+static const struct clk_ops scmi_atomic_clk_ops = {
+	.recalc_rate = scmi_clk_recalc_rate,
+	.round_rate = scmi_clk_round_rate,
+	.set_rate = scmi_clk_set_rate,
+	.enable = scmi_clk_enable,
+	.disable = scmi_clk_disable,
+};
+
+static int scmi_clk_ops_init(struct device *dev, struct scmi_clk *sclk,
+			     const struct clk_ops *scmi_ops)
 {
 	int ret;
 	unsigned long min_rate, max_rate;
@@ -110,7 +131,7 @@ static int scmi_clk_ops_init(struct device *dev, struct scmi_clk *sclk)
 	struct clk_init_data init = {
 		.flags = CLK_GET_RATE_NOCACHE,
 		.num_parents = 0,
-		.ops = &scmi_clk_ops,
+		.ops = scmi_ops,
 		.name = sclk->info->name,
 	};
 
@@ -145,6 +166,7 @@ static int scmi_clocks_probe(struct scmi_device *sdev)
 	struct device_node *np = dev->of_node;
 	const struct scmi_handle *handle = sdev->handle;
 	struct scmi_protocol_handle *ph;
+	bool is_atomic;
 
 	if (!handle)
 		return -ENODEV;
@@ -168,6 +190,8 @@ static int scmi_clocks_probe(struct scmi_device *sdev)
 	clk_data->num = count;
 	hws = clk_data->hws;
 
+	is_atomic = handle->is_transport_atomic(handle);
+
 	for (idx = 0; idx < count; idx++) {
 		struct scmi_clk *sclk;
 
@@ -184,7 +208,9 @@ static int scmi_clocks_probe(struct scmi_device *sdev)
 		sclk->id = idx;
 		sclk->ph = ph;
 
-		err = scmi_clk_ops_init(dev, sclk);
+		err = scmi_clk_ops_init(dev, sclk,
+					is_atomic ? &scmi_atomic_clk_ops :
+					&scmi_clk_ops);
 		if (err) {
 			dev_err(dev, "failed to register clock %d\n", idx);
 			devm_kfree(dev, sclk);
-- 
2.17.1


^ permalink raw reply	[flat|nested] 9+ messages in thread

* [RFC PATCH v2 7/8] firmware: arm_scmi: Make smc transport use common completions
  2021-07-05 17:10 [RFC PATCH v2 0/8] Introduce atomic support for SCMI transports Cristian Marussi
                   ` (5 preceding siblings ...)
  2021-07-05 17:10 ` [RFC PATCH v2 6/8] clk: scmi: Support atomic enable/disable API Cristian Marussi
@ 2021-07-05 17:10 ` Cristian Marussi
  2021-07-05 17:10 ` [RFC PATCH v2 8/8] firmware: arm_scmi: Make smc transport atomic Cristian Marussi
  7 siblings, 0 replies; 9+ messages in thread
From: Cristian Marussi @ 2021-07-05 17:10 UTC (permalink / raw)
  To: linux-kernel, linux-arm-kernel
  Cc: sudeep.holla, james.quinlan, Jonathan.Cameron, f.fainelli,
	etienne.carriere, vincent.guittot, souvik.chakravarty,
	cristian.marussi

When a completion irq is available use it and delegate command completion
handling to the core SCMI completion mechanism.

If no completion irq is available revert to polling, using the core common
polling machinery.

Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
---
 drivers/firmware/arm_scmi/smc.c | 40 ++++++++++++++++-----------------
 1 file changed, 20 insertions(+), 20 deletions(-)

diff --git a/drivers/firmware/arm_scmi/smc.c b/drivers/firmware/arm_scmi/smc.c
index bed5596c7209..cab1d0d50d8e 100644
--- a/drivers/firmware/arm_scmi/smc.c
+++ b/drivers/firmware/arm_scmi/smc.c
@@ -25,8 +25,6 @@
  * @shmem: Transmit/Receive shared memory area
  * @shmem_lock: Lock to protect access to Tx/Rx shared memory area
  * @func_id: smc/hvc call function id
- * @irq: Optional; employed when platforms indicates msg completion by intr.
- * @tx_complete: Optional, employed only when irq is valid.
  */
 
 struct scmi_smc {
@@ -34,15 +32,13 @@ struct scmi_smc {
 	struct scmi_shared_mem __iomem *shmem;
 	struct mutex shmem_lock;
 	u32 func_id;
-	int irq;
-	struct completion tx_complete;
 };
 
 static irqreturn_t smc_msg_done_isr(int irq, void *data)
 {
 	struct scmi_smc *scmi_info = data;
 
-	complete(&scmi_info->tx_complete);
+	scmi_rx_callback(scmi_info->cinfo, shmem_read_header(scmi_info->shmem));
 
 	return IRQ_HANDLED;
 }
@@ -111,8 +107,9 @@ static int smc_chan_setup(struct scmi_chan_info *cinfo, struct device *dev,
 			dev_err(dev, "failed to setup SCMI smc irq\n");
 			return ret;
 		}
-		init_completion(&scmi_info->tx_complete);
-		scmi_info->irq = irq;
+	} else {
+		/* No completion interrupt available */
+		cinfo->needs_polling = true;
 	}
 
 	scmi_info->func_id = func_id;
@@ -142,25 +139,20 @@ static int smc_send_message(struct scmi_chan_info *cinfo,
 	struct scmi_smc *scmi_info = cinfo->transport_info;
 	struct arm_smccc_res res;
 
+	/*
+	 * Channel lock will be released only once response has been
+	 * surely fully retrieved, so after .mark_txdone()
+	 */
 	mutex_lock(&scmi_info->shmem_lock);
-
 	shmem_tx_prepare(scmi_info->shmem, xfer);
 
-	if (scmi_info->irq)
-		reinit_completion(&scmi_info->tx_complete);
-
 	arm_smccc_1_1_invoke(scmi_info->func_id, 0, 0, 0, 0, 0, 0, 0, &res);
-
-	if (scmi_info->irq)
-		wait_for_completion(&scmi_info->tx_complete);
-
-	scmi_rx_callback(scmi_info->cinfo, shmem_read_header(scmi_info->shmem));
-
-	mutex_unlock(&scmi_info->shmem_lock);
-
 	/* Only SMCCC_RET_NOT_SUPPORTED is valid error code */
-	if (res.a0)
+	if (res.a0) {
+		mutex_unlock(&scmi_info->shmem_lock);
 		return -EOPNOTSUPP;
+	}
+
 	return 0;
 }
 
@@ -172,6 +164,13 @@ static void smc_fetch_response(struct scmi_chan_info *cinfo,
 	shmem_fetch_response(scmi_info->shmem, xfer);
 }
 
+static void smc_mark_txdone(struct scmi_chan_info *cinfo, int ret)
+{
+	struct scmi_smc *scmi_info = cinfo->transport_info;
+
+	mutex_unlock(&scmi_info->shmem_lock);
+}
+
 static bool
 smc_poll_done(struct scmi_chan_info *cinfo, struct scmi_xfer *xfer)
 {
@@ -185,6 +184,7 @@ static const struct scmi_transport_ops scmi_smc_ops = {
 	.chan_setup = smc_chan_setup,
 	.chan_free = smc_chan_free,
 	.send_message = smc_send_message,
+	.mark_txdone = smc_mark_txdone,
 	.fetch_response = smc_fetch_response,
 	.poll_done = smc_poll_done,
 };
-- 
2.17.1


^ permalink raw reply	[flat|nested] 9+ messages in thread

* [RFC PATCH v2 8/8] firmware: arm_scmi: Make smc transport atomic
  2021-07-05 17:10 [RFC PATCH v2 0/8] Introduce atomic support for SCMI transports Cristian Marussi
                   ` (6 preceding siblings ...)
  2021-07-05 17:10 ` [RFC PATCH v2 7/8] firmware: arm_scmi: Make smc transport use common completions Cristian Marussi
@ 2021-07-05 17:10 ` Cristian Marussi
  7 siblings, 0 replies; 9+ messages in thread
From: Cristian Marussi @ 2021-07-05 17:10 UTC (permalink / raw)
  To: linux-kernel, linux-arm-kernel
  Cc: sudeep.holla, james.quinlan, Jonathan.Cameron, f.fainelli,
	etienne.carriere, vincent.guittot, souvik.chakravarty,
	cristian.marussi

Substitute mutex usages with busy-waiting and declare smc transport as
.atomic_capable.

Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
---
 drivers/firmware/arm_scmi/smc.c | 28 +++++++++++++++++++++-------
 1 file changed, 21 insertions(+), 7 deletions(-)

diff --git a/drivers/firmware/arm_scmi/smc.c b/drivers/firmware/arm_scmi/smc.c
index cab1d0d50d8e..e33985a02068 100644
--- a/drivers/firmware/arm_scmi/smc.c
+++ b/drivers/firmware/arm_scmi/smc.c
@@ -7,6 +7,7 @@
  */
 
 #include <linux/arm-smccc.h>
+#include <linux/atomic.h>
 #include <linux/device.h>
 #include <linux/err.h>
 #include <linux/interrupt.h>
@@ -14,6 +15,7 @@
 #include <linux/of.h>
 #include <linux/of_address.h>
 #include <linux/of_irq.h>
+#include <linux/processor.h>
 #include <linux/slab.h>
 
 #include "common.h"
@@ -23,14 +25,15 @@
  *
  * @cinfo: SCMI channel info
  * @shmem: Transmit/Receive shared memory area
- * @shmem_lock: Lock to protect access to Tx/Rx shared memory area
+ * @inflight: Atomic flag to protect access to Tx/Rx shared memory area
  * @func_id: smc/hvc call function id
  */
 
 struct scmi_smc {
 	struct scmi_chan_info *cinfo;
 	struct scmi_shared_mem __iomem *shmem;
-	struct mutex shmem_lock;
+#define INFLIGHT_NONE	MSG_TOKEN_MAX
+	atomic_t inflight;
 	u32 func_id;
 };
 
@@ -114,7 +117,7 @@ static int smc_chan_setup(struct scmi_chan_info *cinfo, struct device *dev,
 
 	scmi_info->func_id = func_id;
 	scmi_info->cinfo = cinfo;
-	mutex_init(&scmi_info->shmem_lock);
+	atomic_set(&scmi_info->inflight, INFLIGHT_NONE);
 	cinfo->transport_info = scmi_info;
 
 	return 0;
@@ -133,6 +136,15 @@ static int smc_chan_free(int id, void *p, void *data)
 	return 0;
 }
 
+static bool smc_xfer_inflight(struct scmi_xfer *xfer, atomic_t *inflight)
+{
+	int ret;
+
+	ret = atomic_cmpxchg(inflight, INFLIGHT_NONE, xfer->hdr.seq);
+
+	return ret == INFLIGHT_NONE;
+}
+
 static int smc_send_message(struct scmi_chan_info *cinfo,
 			    struct scmi_xfer *xfer)
 {
@@ -140,16 +152,17 @@ static int smc_send_message(struct scmi_chan_info *cinfo,
 	struct arm_smccc_res res;
 
 	/*
-	 * Channel lock will be released only once response has been
+	 * Channel will be released only once response has been
 	 * surely fully retrieved, so after .mark_txdone()
 	 */
-	mutex_lock(&scmi_info->shmem_lock);
+	spin_until_cond(smc_xfer_inflight(xfer, &scmi_info->inflight));
+
 	shmem_tx_prepare(scmi_info->shmem, xfer);
 
 	arm_smccc_1_1_invoke(scmi_info->func_id, 0, 0, 0, 0, 0, 0, 0, &res);
 	/* Only SMCCC_RET_NOT_SUPPORTED is valid error code */
 	if (res.a0) {
-		mutex_unlock(&scmi_info->shmem_lock);
+		atomic_set(&scmi_info->inflight, INFLIGHT_NONE);
 		return -EOPNOTSUPP;
 	}
 
@@ -168,7 +181,7 @@ static void smc_mark_txdone(struct scmi_chan_info *cinfo, int ret)
 {
 	struct scmi_smc *scmi_info = cinfo->transport_info;
 
-	mutex_unlock(&scmi_info->shmem_lock);
+	atomic_set(&scmi_info->inflight, INFLIGHT_NONE);
 }
 
 static bool
@@ -194,4 +207,5 @@ const struct scmi_desc scmi_smc_desc = {
 	.max_rx_timeout_ms = 30,
 	.max_msg = 20,
 	.max_msg_size = 128,
+	.atomic_capable = true,
 };
-- 
2.17.1


^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2021-07-05 17:11 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-07-05 17:10 [RFC PATCH v2 0/8] Introduce atomic support for SCMI transports Cristian Marussi
2021-07-05 17:10 ` [RFC PATCH v2 1/8] firmware: arm_scmi: Add configurable polling mode for transports Cristian Marussi
2021-07-05 17:10 ` [RFC PATCH v2 2/8] firmware: arm_scmi: Add support for atomic transports Cristian Marussi
2021-07-05 17:10 ` [RFC PATCH v2 3/8] include: trace: Add new scmi_xfer_response_wait event Cristian Marussi
2021-07-05 17:10 ` [RFC PATCH v2 4/8] firmware: arm_scmi: Use new trace event scmi_xfer_response_wait Cristian Marussi
2021-07-05 17:10 ` [RFC PATCH v2 5/8] firmware: arm_scmi: Add is_transport_atomic() handle method Cristian Marussi
2021-07-05 17:10 ` [RFC PATCH v2 6/8] clk: scmi: Support atomic enable/disable API Cristian Marussi
2021-07-05 17:10 ` [RFC PATCH v2 7/8] firmware: arm_scmi: Make smc transport use common completions Cristian Marussi
2021-07-05 17:10 ` [RFC PATCH v2 8/8] firmware: arm_scmi: Make smc transport atomic Cristian Marussi

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).