[PATCH v4 0/12] Introduce atomic support for SCMI transports

[PATCH v4 0/12] Introduce atomic support for SCMI transports

[Cristian Marussi]

Hi all,

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

At first in [2/12], 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.no_completion_irq,
typically during .chan_setup(). The usual per-command polling selection
behaviour based on hdr.poll_completion is preserved as before.

Then in [3/12], 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 too from sleeping on the correspondent RX-path
even when using completion IRQs.

In [6/12] 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 [7/12], can optionally support atomic operations when
operating on an atomically configured transport.

In [8/12] virtio transport is declared atomic.

Finally there are 4 *tentative" RFC patch related to SMC transport.

At first [9/12] ports SMC to use the common core completions when
completion interrupt is available or otherwise revert to use common core
polling mechanism above introduced: this avoids the improper call of
scmi_rx_callback directly in smc_send_message.
Then in [10/12] SMC is converted to be .atomic_capable by substituting
the mutexes with busy-waiting to keep the channel 'locked'.

With [11/12] I introduce a flag to allow a transport to signal to the core
that upon return of a .send_message() the requested command execution can
be assumed by the core to have been fully completed by the platform, so
that the response payload (if any) can be immediately fetched without the
need to poll the channel.

Finally in [12/12] I enable the above flag for SMC transport.

As a side note on the sync_cmds_atomic_replies flag added in [11/12], note
that such flag assumes that a specific transport can set it and so
univocally assure to the SCMI core that the described behavior holds true
anytime for the said transport; in reality, in a transport like SMC such
behavior could also be dependent on the effective placement of the SCMI
platform server: while a pure EL-3 SCMI server could reasonably assure
that upon smc call termination the command is fully completed, the same
could not be true for an SCMI server living in S-EL1.

For such reasons a possible evolution could be to add also some DT property
to enable the SMC transport to conditionally enable the flag at run-time
depending on the effective runtime platform configuration in terms of
SCMI server placement.

This whole matter is up for discussion, though, maybe it's not even a real
possibility an S-EL1 SCMI server using an SMC transport, so the DT patch
has not been included in this series.

Moreover, 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, so they are marked as RFCs.

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

The series is based on linux-next/master on top of tag next-20210824 so as
to include the recently queued series on SCMI virtio and its core changes
currently queued also on sudeep/for-next/scmi [1].

Given I'm still gathering feedback on this, I still not have CCed any
maintainer out of SCMI subsystem.

Any feedback welcome.

Thanks,

Cristian

[1]: https://git.kernel.org/pub/scm/linux/kernel/git/sudeep.holla/linux.git/log/?h=for-next/scmi 

---

V3 --> V4
- rebased on linux-next/master next-20210824
- renamed .needs_polling to .no_completion_irq
- added .sync_cmds_atomic_replies
- make SMC use .sync_cmd_atomic_replies

V2 --> v3
- rebased on SCMI VirtIO V6 which in turn is based on v5.14-rc1


Cristian Marussi (12):
  firmware: arm_scmi: Perform earlier cinfo lookup call in do_xfer
  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: Declare virtio transport .atomic_capable
  [RFC] firmware: arm_scmi: Make smc transport use common completions
  [RFC] firmware: arm_scmi: Make smc transport atomic
  [RFC] firmware: arm_scmi: Add sync_cmds_atomic_replies transport flag
  [RFC] firmware: arm_scmi: Make smc support atomic commands replies

 drivers/clk/clk-scmi.c             |  44 ++++--
 drivers/firmware/arm_scmi/common.h |  22 +++
 drivers/firmware/arm_scmi/driver.c | 224 +++++++++++++++++++++++------
 drivers/firmware/arm_scmi/smc.c    |  61 +++++---
 drivers/firmware/arm_scmi/virtio.c |   1 +
 include/linux/scmi_protocol.h      |   8 ++
 include/trace/events/scmi.h        |  28 ++++
 7 files changed, 311 insertions(+), 77 deletions(-)

-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v4 01/12] firmware: arm_scmi: Perform earlier cinfo lookup call in do_xfer

[Cristian Marussi]

Lookup cinfo data early in do_xfer so as to avoid any further init work
on xfer structure in case of error.

No functional change.

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

diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c
index b406b3f78f46..01c79a8fa77f 100644
--- a/drivers/firmware/arm_scmi/driver.c
+++ b/drivers/firmware/arm_scmi/driver.c
@@ -766,6 +766,10 @@ static int do_xfer(const struct scmi_protocol_handle *ph,
 		return -EINVAL;
 	}
 
+	cinfo = idr_find(&info->tx_idr, pi->proto->id);
+	if (unlikely(!cinfo))
+		return -EINVAL;
+
 	/*
 	 * Initialise protocol id now from protocol handle to avoid it being
 	 * overridden by mistake (or malice) by the protocol code mangling with
@@ -774,10 +778,6 @@ static int do_xfer(const struct scmi_protocol_handle *ph,
 	xfer->hdr.protocol_id = pi->proto->id;
 	reinit_completion(&xfer->done);
 
-	cinfo = idr_find(&info->tx_idr, xfer->hdr.protocol_id);
-	if (unlikely(!cinfo))
-		return -EINVAL;
-
 	trace_scmi_xfer_begin(xfer->transfer_id, xfer->hdr.id,
 			      xfer->hdr.protocol_id, xfer->hdr.seq,
 			      xfer->hdr.poll_completion);
-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v4 02/12] firmware: arm_scmi: Add configurable polling mode for transports

[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 evaluate the transport-specific .poll_done() function to
determine if and when a request was fully 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.no_completion_irq: 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>
---
v3 --> v4:
- renamed .needs_polling flag to .no_completion_irq
- refactored error path when polling needed but not supported
---
 drivers/firmware/arm_scmi/common.h | 11 +++++++++++
 drivers/firmware/arm_scmi/driver.c | 29 +++++++++++++++++++++++------
 2 files changed, 34 insertions(+), 6 deletions(-)

diff --git a/drivers/firmware/arm_scmi/common.h b/drivers/firmware/arm_scmi/common.h
index dea1bfbe1052..67c761141a48 100644
--- a/drivers/firmware/arm_scmi/common.h
+++ b/drivers/firmware/arm_scmi/common.h
@@ -339,11 +339,19 @@ 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
+ * @no_completion_irq: 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 no_completion_irq;
 	void *transport_info;
 };
 
@@ -402,6 +410,8 @@ struct scmi_device *scmi_child_dev_find(struct device *parent,
  *	be pending simultaneously in the system. May be overridden by the
  *	get_max_msg op.
  * @max_msg_size: Maximum size of data per message that can be handled.
+ * @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 (*transport_init)(void);
@@ -410,6 +420,7 @@ struct scmi_desc {
 	int max_rx_timeout_ms;
 	int max_msg;
 	int max_msg_size;
+	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 01c79a8fa77f..a3700f49e8ac 100644
--- a/drivers/firmware/arm_scmi/driver.c
+++ b/drivers/firmware/arm_scmi/driver.c
@@ -656,6 +656,15 @@ static void scmi_handle_response(struct scmi_chan_info *cinfo,
 		return;
 	}
 
+	/* 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;
@@ -760,16 +769,19 @@ static int do_xfer(const struct scmi_protocol_handle *ph,
 	struct device *dev = info->dev;
 	struct scmi_chan_info *cinfo;
 
-	if (xfer->hdr.poll_completion && !info->desc->ops->poll_done) {
-		dev_warn_once(dev,
-			      "Polling mode is not supported by transport.\n");
-		return -EINVAL;
-	}
-
 	cinfo = idr_find(&info->tx_idr, pi->proto->id);
 	if (unlikely(!cinfo))
 		return -EINVAL;
 
+	if (info->desc->force_polling || cinfo->no_completion_irq) {
+		if (!info->desc->ops->poll_done) {
+			dev_warn_once(dev,
+				      "Polling mode is not supported by transport.\n");
+			return -EINVAL;
+		}
+		xfer->hdr.poll_completion = true;
+	}
+
 	/*
 	 * Initialise protocol id now from protocol handle to avoid it being
 	 * overridden by mistake (or malice) by the protocol code mangling with
@@ -1499,6 +1511,11 @@ static int scmi_chan_setup(struct scmi_info *info, struct device *dev,
 	if (ret)
 		return ret;
 
+	if (tx && (cinfo->no_completion_irq || 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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v4 03/12] firmware: arm_scmi: Add support for atomic transports

[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 | 167 ++++++++++++++++++++++-------
 2 files changed, 132 insertions(+), 38 deletions(-)

diff --git a/drivers/firmware/arm_scmi/common.h b/drivers/firmware/arm_scmi/common.h
index 67c761141a48..4ab310c2eae5 100644
--- a/drivers/firmware/arm_scmi/common.h
+++ b/drivers/firmware/arm_scmi/common.h
@@ -412,6 +412,8 @@ struct scmi_device *scmi_child_dev_find(struct device *parent,
  * @max_msg_size: Maximum size of data per message that can be handled.
  * @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 (*transport_init)(void);
@@ -421,6 +423,7 @@ struct scmi_desc {
 	int max_msg;
 	int max_msg_size;
 	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 a3700f49e8ac..2ca1602afd80 100644
--- a/drivers/firmware/arm_scmi/driver.c
+++ b/drivers/firmware/arm_scmi/driver.c
@@ -681,6 +681,10 @@ static void scmi_handle_response(struct scmi_chan_info *cinfo,
 		scmi_clear_channel(info, cinfo);
 		complete(xfer->async_done);
 	} else {
+		/*
+		 * This same xfer->done completion is used in atomic mode as a
+		 * flag for polling.
+		 */
 		complete(&xfer->done);
 	}
 
@@ -733,8 +737,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)
 {
@@ -749,6 +751,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
  *
@@ -763,7 +849,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;
@@ -810,36 +895,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);
 
@@ -861,7 +917,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
@@ -870,22 +926,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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v4 04/12] include: trace: Add new scmi_xfer_response_wait event

[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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v4 05/12] firmware: arm_scmi: Use new trace event scmi_xfer_response_wait

[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 2ca1602afd80..1320a00a6339 100644
--- a/drivers/firmware/arm_scmi/driver.c
+++ b/drivers/firmware/arm_scmi/driver.c
@@ -786,6 +786,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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v4 06/12] firmware: arm_scmi: Add is_transport_atomic() handle method

[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 1320a00a6339..852baac0e614 100644
--- a/drivers/firmware/arm_scmi/driver.c
+++ b/drivers/firmware/arm_scmi/driver.c
@@ -1416,6 +1416,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)
 {
@@ -1948,6 +1963,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 80e781c51ddc..9f895cb81818 100644
--- a/include/linux/scmi_protocol.h
+++ b/include/linux/scmi_protocol.h
@@ -612,6 +612,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 {
@@ -622,6 +629,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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v4 07/12] clk: scmi: Support atomic enable/disable API

[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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v4 08/12] firmware: arm_scmi: Declare virtio transport .atomic_capable

[Cristian Marussi]

SCMI virtio transport support does not contain any sleeping pattern, so
declare it as .atomic_capable.

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

diff --git a/drivers/firmware/arm_scmi/virtio.c b/drivers/firmware/arm_scmi/virtio.c
index 224577f86928..eabe430595f0 100644
--- a/drivers/firmware/arm_scmi/virtio.c
+++ b/drivers/firmware/arm_scmi/virtio.c
@@ -488,4 +488,5 @@ const struct scmi_desc scmi_virtio_desc = {
 	.max_rx_timeout_ms = 60000, /* for non-realtime virtio devices */
 	.max_msg = 0, /* overridden by virtio_get_max_msg() */
 	.max_msg_size = VIRTIO_SCMI_MAX_MSG_SIZE,
+	.atomic_capable = true,
 };
-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v4 09/12] [RFC] firmware: arm_scmi: Make smc transport use common completions

[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>
---
v3 --> v4
- renamed usage of .needs_polling to .no_completion_irq
---
 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 4effecc3bb46..adaa40df3855 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,14 @@ 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), NULL);
 
 	return IRQ_HANDLED;
 }
@@ -111,8 +108,8 @@ 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 {
+		cinfo->no_completion_irq = true;
 	}
 
 	scmi_info->func_id = func_id;
@@ -142,26 +139,21 @@ 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), NULL);
-
-	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;
 }
 
@@ -173,6 +165,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)
 {
@@ -186,6 +185,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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v4 10/12] [RFC] firmware: arm_scmi: Make smc transport atomic

[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 adaa40df3855..c13edaace8a3 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,17 +152,18 @@ 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;
 	}
 
@@ -169,7 +182,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
@@ -195,4 +208,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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v4 11/12] [RFC] firmware: arm_scmi: Add sync_cmds_atomic_replies transport flag

[Cristian Marussi]

A flag is added to let the transport signal the core that its handling of
synchronous command messages implies that, after .send_message has returned
successfully, the requested command can be assumed to be fully and
completely executed on SCMI platform side so that any possible response
value is already immediately available to be retrieved by a .fetch_reponse:
in other words the polling phase can be skipped in such a case and the
response values accessed straight away.

Note that all of the above applies only when polling mode of operation was
selected by the core: if instead a completion IRQ was found to be available
the normal response processing path based on completions will still be
followed.

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

diff --git a/drivers/firmware/arm_scmi/common.h b/drivers/firmware/arm_scmi/common.h
index 4ab310c2eae5..2ce6b38d1270 100644
--- a/drivers/firmware/arm_scmi/common.h
+++ b/drivers/firmware/arm_scmi/common.h
@@ -412,6 +412,13 @@ struct scmi_device *scmi_child_dev_find(struct device *parent,
  * @max_msg_size: Maximum size of data per message that can be handled.
  * @force_polling: Flag to force this whole transport to use SCMI core polling
  *		   mechanism instead of completion interrupts even if available.
+ * @sync_cmds_atomic_replies: Flag to indicate that the transport assures
+ *			      synchronous-command messages are atomically
+ *			      completed on .send_message: no need to poll
+ *			      actively waiting for a response.
+ *			      Used by core internally only when polling is
+ *			      selected as a waiting for reply method: i.e.
+ *			      if a completion irq was found use that anyway.
  * @atomic_capable: Flag to indicate that this transport is assured not to sleep
  *		    on the TX path.
  */
@@ -423,6 +430,7 @@ struct scmi_desc {
 	int max_msg;
 	int max_msg_size;
 	bool force_polling;
+	bool sync_cmds_atomic_replies;
 	bool atomic_capable;
 };
 
diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c
index 852baac0e614..e17285f11ce3 100644
--- a/drivers/firmware/arm_scmi/driver.c
+++ b/drivers/firmware/arm_scmi/driver.c
@@ -809,14 +809,24 @@ static int scmi_wait_for_message_response(struct scmi_chan_info *cinfo,
 					(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);
+		if (!info->desc->sync_cmds_atomic_replies) {
+			/*
+			 * 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_after(ktime_get(), stop)) {
+				dev_err(dev,
+					"timed out in resp(caller: %pS) - polling\n",
+					(void *)_RET_IP_);
+				ret = -ETIMEDOUT;
+			}
+		}
 
-		spin_until_cond(scmi_xfer_done_no_timeout(cinfo, xfer, stop));
-		if (ktime_before(ktime_get(), stop)) {
+		if (!ret) {
 			unsigned long flags;
 
 			/*
@@ -829,11 +839,6 @@ static int scmi_wait_for_message_response(struct scmi_chan_info *cinfo,
 				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;
 		}
 	}
 
-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v4 12/12] [RFC] firmware: arm_scmi: Make smc support atomic commands replies

[Cristian Marussi]

Enable sync_cmds_atomic_replies in the SMC transport descriptor.

Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
---
NOTE THAT this flag is probably better to be also optionally settable
using an optional DT property to address the fact that the same
transport could expose or not this feature depending on where the SCMI
server sits.
(e.g. an SCMI server in S-EL1 accessed via smc dispatcher sitting in EL3)
---
 drivers/firmware/arm_scmi/smc.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/firmware/arm_scmi/smc.c b/drivers/firmware/arm_scmi/smc.c
index c13edaace8a3..479382f3cc96 100644
--- a/drivers/firmware/arm_scmi/smc.c
+++ b/drivers/firmware/arm_scmi/smc.c
@@ -209,4 +209,5 @@ const struct scmi_desc scmi_smc_desc = {
 	.max_msg = 20,
 	.max_msg_size = 128,
 	.atomic_capable = true,
+	.sync_cmds_atomic_replies = true,
 };
-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 03/12] firmware: arm_scmi: Add support for atomic transports

[Jim Quinlan]

[-- Attachment #1.1: Type: text/plain, Size: 12748 bytes --]

Hi Christian,

On Tue, Aug 24, 2021 at 10:00 AM Cristian Marussi
<cristian.marussi@arm.com> wrote:
>
> 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 | 167 ++++++++++++++++++++++-------
>  2 files changed, 132 insertions(+), 38 deletions(-)
>
> diff --git a/drivers/firmware/arm_scmi/common.h b/drivers/firmware/arm_scmi/common.h
> index 67c761141a48..4ab310c2eae5 100644
> --- a/drivers/firmware/arm_scmi/common.h
> +++ b/drivers/firmware/arm_scmi/common.h
> @@ -412,6 +412,8 @@ struct scmi_device *scmi_child_dev_find(struct device *parent,
>   * @max_msg_size: Maximum size of data per message that can be handled.
>   * @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 (*transport_init)(void);
> @@ -421,6 +423,7 @@ struct scmi_desc {
>         int max_msg;
>         int max_msg_size;
>         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 a3700f49e8ac..2ca1602afd80 100644
> --- a/drivers/firmware/arm_scmi/driver.c
> +++ b/drivers/firmware/arm_scmi/driver.c
> @@ -681,6 +681,10 @@ static void scmi_handle_response(struct scmi_chan_info *cinfo,
>                 scmi_clear_channel(info, cinfo);
>                 complete(xfer->async_done);
>         } else {
> +               /*
> +                * This same xfer->done completion is used in atomic mode as a
> +                * flag for polling.
> +                */
>                 complete(&xfer->done);
>         }
>
> @@ -733,8 +737,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)
>  {
> @@ -749,6 +751,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);
We use the SMC transport with a completion interrupt but would prefer
for the above to use wait_for_completion(...) instead of  using
spin_for_completion().  A few of our SCMI commands can take a while to
complete execution so we do not want to be spinning or polling while
waiting.

We could probably go back to a mailbox-based transport and use delayed
messages here for these "long" SCMI messages,  but we do not have full
control over the platform FW (plus there are backwards compatibility
issues).  FWIW, the platform FW was setup before SCMI on Linux
implemented delayed/async  messages.

Regards,
Jim Quinlan
Broadcom STB


> +                       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
>   *
> @@ -763,7 +849,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;
> @@ -810,36 +895,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);
>
> @@ -861,7 +917,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
> @@ -870,22 +926,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
>

[-- Attachment #1.2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4210 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 01/12] firmware: arm_scmi: Perform earlier cinfo lookup call in do_xfer

[Florian Fainelli]

On 8/24/2021 3:59 PM, Cristian Marussi wrote:
> Lookup cinfo data early in do_xfer so as to avoid any further init work
> on xfer structure in case of error.
> 
> No functional change.
> 
> Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
-- 
Florian

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 02/12] firmware: arm_scmi: Add configurable polling mode for transports

[Florian Fainelli]

On 8/24/2021 3:59 PM, Cristian Marussi wrote:
> 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 evaluate the transport-specific .poll_done() function to
> determine if and when a request was fully 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.no_completion_irq: 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>

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
-- 
Florian

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 04/12] include: trace: Add new scmi_xfer_response_wait event

[Florian Fainelli]

On 8/24/2021 3:59 PM, Cristian Marussi wrote:
> 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>

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
-- 
Florian

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 05/12] firmware: arm_scmi: Use new trace event scmi_xfer_response_wait

[Florian Fainelli]

On 8/24/2021 3:59 PM, Cristian Marussi wrote:
> Use new trace event to mark start of waiting for response section.
> 
> Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>

Might be worth squashing into patch 4?

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
-- 
Florian

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 06/12] firmware: arm_scmi: Add is_transport_atomic() handle method

[Florian Fainelli]

On 8/24/2021 3:59 PM, Cristian Marussi wrote:
> 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>

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
-- 
Florian

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 07/12] clk: scmi: Support atomic enable/disable API

[Florian Fainelli]

On 8/24/2021 3:59 PM, Cristian Marussi wrote:
> 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>

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
-- 
Florian

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 09/12] [RFC] firmware: arm_scmi: Make smc transport use common completions

[Florian Fainelli]

On 8/24/2021 3:59 PM, Cristian Marussi wrote:
> 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>

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
-- 
Florian

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 11/12] [RFC] firmware: arm_scmi: Add sync_cmds_atomic_replies transport flag

[Florian Fainelli]

On 8/24/2021 3:59 PM, Cristian Marussi wrote:
> A flag is added to let the transport signal the core that its handling of
> synchronous command messages implies that, after .send_message has returned
> successfully, the requested command can be assumed to be fully and
> completely executed on SCMI platform side so that any possible response
> value is already immediately available to be retrieved by a .fetch_reponse:
> in other words the polling phase can be skipped in such a case and the
> response values accessed straight away.
> 
> Note that all of the above applies only when polling mode of operation was
> selected by the core: if instead a completion IRQ was found to be available
> the normal response processing path based on completions will still be
> followed.

This might actually have to be settable on a per-message basis ideally 
since we may be transporting short lived SCMI messages for which the 
completion can be done at SMC time, and long lived SCMI messages (e.g.: 
involving a voltage change) for which we would prefer a completion 
interrupt. Jim, what do you think?
-- 
Florian

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 11/12] [RFC] firmware: arm_scmi: Add sync_cmds_atomic_replies transport flag

[Jim Quinlan]

[-- Attachment #1.1: Type: text/plain, Size: 1595 bytes --]

On Wed, Aug 25, 2021 at 12:38 PM Florian Fainelli <f.fainelli@gmail.com> wrote:
>
>
>
> On 8/24/2021 3:59 PM, Cristian Marussi wrote:
> > A flag is added to let the transport signal the core that its handling of
> > synchronous command messages implies that, after .send_message has returned
> > successfully, the requested command can be assumed to be fully and
> > completely executed on SCMI platform side so that any possible response
> > value is already immediately available to be retrieved by a .fetch_reponse:
> > in other words the polling phase can be skipped in such a case and the
> > response values accessed straight away.
> >
> > Note that all of the above applies only when polling mode of operation was
> > selected by the core: if instead a completion IRQ was found to be available
> > the normal response processing path based on completions will still be
> > followed.
>
> This might actually have to be settable on a per-message basis ideally
> since we may be transporting short lived SCMI messages for which the
> completion can be done at SMC time, and long lived SCMI messages (e.g.:
> involving a voltage change) for which we would prefer a completion
> interrupt. Jim, what do you think?
Even if the SCMI main driver could be configured this way in an
elegant manner, I'm not sure that there is a clean way of specifying
this  attribute on a per-message basis.  Certainly we could do this
with our own protocols, but  many of our "long lived" messages are the
Perf protocol's set_level command.  At any rate, let me give it some
thought.

Regards,
Jim
> --
> Florian

[-- Attachment #1.2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4210 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 03/12] firmware: arm_scmi: Add support for atomic transports

[Cristian Marussi]

On Wed, Aug 25, 2021 at 12:18:31PM -0400, Jim Quinlan wrote:
> Hi Christian,
> 

Hi Jim,

thanks for the review first of all.

> On Tue, Aug 24, 2021 at 10:00 AM Cristian Marussi
> <cristian.marussi@arm.com> wrote:
> >
> > 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 | 167 ++++++++++++++++++++++-------
> >  2 files changed, 132 insertions(+), 38 deletions(-)
> >
> > diff --git a/drivers/firmware/arm_scmi/common.h b/drivers/firmware/arm_scmi/common.h
> > index 67c761141a48..4ab310c2eae5 100644
> > --- a/drivers/firmware/arm_scmi/common.h
> > +++ b/drivers/firmware/arm_scmi/common.h
> > @@ -412,6 +412,8 @@ struct scmi_device *scmi_child_dev_find(struct device *parent,
> >   * @max_msg_size: Maximum size of data per message that can be handled.
> >   * @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 (*transport_init)(void);
> > @@ -421,6 +423,7 @@ struct scmi_desc {
> >         int max_msg;
> >         int max_msg_size;
> >         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 a3700f49e8ac..2ca1602afd80 100644
> > --- a/drivers/firmware/arm_scmi/driver.c
> > +++ b/drivers/firmware/arm_scmi/driver.c
> > @@ -681,6 +681,10 @@ static void scmi_handle_response(struct scmi_chan_info *cinfo,
> >                 scmi_clear_channel(info, cinfo);
> >                 complete(xfer->async_done);
> >         } else {
> > +               /*
> > +                * This same xfer->done completion is used in atomic mode as a
> > +                * flag for polling.
> > +                */
> >                 complete(&xfer->done);
> >         }
> >
> > @@ -733,8 +737,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)
> >  {
> > @@ -749,6 +751,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);
> We use the SMC transport with a completion interrupt but would prefer
> for the above to use wait_for_completion(...) instead of  using
> spin_for_completion().  A few of our SCMI commands can take a while to
> complete execution so we do not want to be spinning or polling while
> waiting.

Busy-waiting when using a completion IRQ is used indeed only if the
transport has been declared .atomic_capable: the idea was that if
the specific transport does not sleep, the core can avoid sleeping too
and so enable users like the clock framework to switch to 'atomic' mode
and being called from atomic context.

So if you drop indeed the SMC patch later in the series that mmake it
atomic

[PATCH v4 10/12] [RFC] firmware: arm_scmi: Make smc transport atomic

you'll end up using the wait_for here when completion IRQ is used.
(instead to avoid any polling while in polling mode (O_o) the idea was
to use the new .sync_cmds_atomic_replies) 

BUT the problem is that some other partner could possibly want to use
instead this same transport in atomic mode, so maybe it could be worth
introducing some sort of configurability so that atomic operations are
enabled only if (say) .atomic_capable && .atomic_enable.

Maybe this could be done on a per-channel base, so you could dedicate a
channel to a protocol and ask only for it to be atomic_enable (if the
whole transport is atomic_capable); mixing atomic/non_atomic behaviour
inside the same protocol/channel across different messages seems a bit
of a hell (...and I think a previous attempt led to a lot of issues in
the past)

Anyway, not sure really where to put this possible configuration bit
though...this not being really a FW config/description seems likely to
be a NACK for a placement in the DT :D

> 
> We could probably go back to a mailbox-based transport and use delayed
> messages here for these "long" SCMI messages,  but we do not have full
> control over the platform FW (plus there are backwards compatibility
> issues).  FWIW, the platform FW was setup before SCMI on Linux
> implemented delayed/async  messages.
> 

Having a completion IRQ couldn't you use it to send delayed responses or
notification even in this smc transport ? (well it'd need FW to be
updated in fact and maybe I'm missing something here)

Thanks a lot for the feedback, may I assume this initial series it sort
of worked (beside noted limitations) in your SMC test setup :D ?

Thanks,
Cristian

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 05/12] firmware: arm_scmi: Use new trace event scmi_xfer_response_wait

[Cristian Marussi]

On Wed, Aug 25, 2021 at 06:31:13PM +0200, Florian Fainelli wrote:
> 
> 
> On 8/24/2021 3:59 PM, Cristian Marussi wrote:
> > Use new trace event to mark start of waiting for response section.
> > 
> > Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
> 
Hi Florian,

Thanks for the review and the feedback across this series.

> Might be worth squashing into patch 4?
> 

Yes indeed I never know for sure when to squash when stuff crosses
subsystems/maintaners boundaries.

Thanks,
Cristian


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 11/12] [RFC] firmware: arm_scmi: Add sync_cmds_atomic_replies transport flag

[Cristian Marussi]

On Wed, Aug 25, 2021 at 01:17:47PM -0400, Jim Quinlan wrote:
> On Wed, Aug 25, 2021 at 12:38 PM Florian Fainelli <f.fainelli@gmail.com> wrote:
> >
> >
> >

Hi Florian and Jim,

> > On 8/24/2021 3:59 PM, Cristian Marussi wrote:
> > > A flag is added to let the transport signal the core that its handling of
> > > synchronous command messages implies that, after .send_message has returned
> > > successfully, the requested command can be assumed to be fully and
> > > completely executed on SCMI platform side so that any possible response
> > > value is already immediately available to be retrieved by a .fetch_reponse:
> > > in other words the polling phase can be skipped in such a case and the
> > > response values accessed straight away.
> > >
> > > Note that all of the above applies only when polling mode of operation was
> > > selected by the core: if instead a completion IRQ was found to be available
> > > the normal response processing path based on completions will still be
> > > followed.
> >
> > This might actually have to be settable on a per-message basis ideally
> > since we may be transporting short lived SCMI messages for which the
> > completion can be done at SMC time, and long lived SCMI messages (e.g.:
> > involving a voltage change) for which we would prefer a completion
> > interrupt. Jim, what do you think?
> Even if the SCMI main driver could be configured this way in an
> elegant manner, I'm not sure that there is a clean way of specifying
> this  attribute on a per-message basis.  Certainly we could do this
> with our own protocols, but  many of our "long lived" messages are the
> Perf protocol's set_level command.  At any rate, let me give it some
> thought.
> 

The new flag .sync_cmds_atomic_replies applies only when polling mode
has been selected for a specific cmd transaction, which means when no
completion IRQ was found available OR if xfer.poll_completion was
excplicitly set for a specific command.

At the moment in this series (unknown bugs apart :D), if you have a
channel configured with a completion IRQ and the .sync_cmds_atomic_replies
set for the transport, this latter flag would be generally ignored and a
wait_for_completion() will be normally used upon reception of the
completionIRQ, UNLESS you specify that one specific command has to be
polled using the per message xfer.poll_completion flag: so you should be
already able to selectively use a polling which immediately returns after
the smc by setting xfer.poll_completion for that specific short lived
message (since sync_cmds_atomic_replies is set and applies to pollmode).
On the other side any other LONG lived message will be naturally handled
via completionIRQ + wait_for_completion. (at least that was the aim..)

!!! NOTE that you'll have also to drop

 [PATCH v4 10/12] [RFC] firmware: arm_scmi: Make smc transport atomic

from this series for the wait_completion to happen as you wish.

As said I'm not sure that this whole mixing of polling and IRQs on the
same channel on a regular won't cause any issues: any feedback on this
from your setup is much appreciated.
(maybe it's fine for SMC transport, but it led to a bit of hell in the
past with mboxes AFAIK...)

Thanks a lot again for your feedback, I'll have to chat with Sudeep
about the various issues/configs possibility that we discussed and I'll
keep you in the loop.

Thanks,
Cristian

P.S.: I'll be off for a few weeks, so even though I'll keep an eye on
the mail, I cannot guarantee any responsiveness :D

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 11/12] [RFC] firmware: arm_scmi: Add sync_cmds_atomic_replies transport flag

[Jim Quinlan]

[-- Attachment #1.1: Type: text/plain, Size: 4271 bytes --]

On Wed, Aug 25, 2021 at 2:49 PM Cristian Marussi
<cristian.marussi@arm.com> wrote:
>
> On Wed, Aug 25, 2021 at 01:17:47PM -0400, Jim Quinlan wrote:
> > On Wed, Aug 25, 2021 at 12:38 PM Florian Fainelli <f.fainelli@gmail.com> wrote:
> > >
> > >
> > >
>
> Hi Florian and Jim,
>
> > > On 8/24/2021 3:59 PM, Cristian Marussi wrote:
> > > > A flag is added to let the transport signal the core that its handling of
> > > > synchronous command messages implies that, after .send_message has returned
> > > > successfully, the requested command can be assumed to be fully and
> > > > completely executed on SCMI platform side so that any possible response
> > > > value is already immediately available to be retrieved by a .fetch_reponse:
> > > > in other words the polling phase can be skipped in such a case and the
> > > > response values accessed straight away.
> > > >
> > > > Note that all of the above applies only when polling mode of operation was
> > > > selected by the core: if instead a completion IRQ was found to be available
> > > > the normal response processing path based on completions will still be
> > > > followed.
> > >
> > > This might actually have to be settable on a per-message basis ideally
> > > since we may be transporting short lived SCMI messages for which the
> > > completion can be done at SMC time, and long lived SCMI messages (e.g.:
> > > involving a voltage change) for which we would prefer a completion
> > > interrupt. Jim, what do you think?
> > Even if the SCMI main driver could be configured this way in an
> > elegant manner, I'm not sure that there is a clean way of specifying
> > this  attribute on a per-message basis.  Certainly we could do this
> > with our own protocols, but  many of our "long lived" messages are the
> > Perf protocol's set_level command.  At any rate, let me give it some
> > thought.
> >
>
> The new flag .sync_cmds_atomic_replies applies only when polling mode
> has been selected for a specific cmd transaction, which means when no
> completion IRQ was found available OR if xfer.poll_completion was
> excplicitly set for a specific command.
>
> At the moment in this series (unknown bugs apart :D), if you have a
> channel configured with a completion IRQ and the .sync_cmds_atomic_replies
> set for the transport, this latter flag would be generally ignored and a
> wait_for_completion() will be normally used upon reception of the
> completionIRQ, UNLESS you specify that one specific command has to be
> polled using the per message xfer.poll_completion flag: so you should be
> already able to selectively use a polling which immediately returns after
> the smc by setting xfer.poll_completion for that specific short lived
> message (since sync_cmds_atomic_replies is set and applies to pollmode).
> On the other side any other LONG lived message will be naturally handled
> via completionIRQ + wait_for_completion. (at least that was the aim..)
>
> !!! NOTE that you'll have also to drop
>
>  [PATCH v4 10/12] [RFC] firmware: arm_scmi: Make smc transport atomic
>
> from this series for the wait_completion to happen as you wish.

Hi Cristian,

I've tested all commits on our SMC-based system.  I also tested all commits
minus  "10/12 [RFC] firmware: arm_scmi: Make smc transport atomic".
This was a basic stress test, not a comprehensive one.  So

Tested-by: Jim Quinlan <james.quinlan@broadcom.com>

Of course I have a strong preference for omitting  "10/12 [RFC]" :-).
FWIW, if you are not planning on dropping this commit, perhaps there
could be a transport
node in the DT, and that could contain the  a bool  property
"smc-atomic-capable"?

Regards,
Jim Quinlan
Broadcom STB

>
> As said I'm not sure that this whole mixing of polling and IRQs on the
> same channel on a regular won't cause any issues: any feedback on this
> from your setup is much appreciated.
> (maybe it's fine for SMC transport, but it led to a bit of hell in the
> past with mboxes AFAIK...)
>
> Thanks a lot again for your feedback, I'll have to chat with Sudeep
> about the various issues/configs possibility that we discussed and I'll
> keep you in the loop.
>
> Thanks,
> Cristian
>
> P.S.: I'll be off for a few weeks, so even though I'll keep an eye on
> the mail, I cannot guarantee any responsiveness :D

[-- Attachment #1.2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4210 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 11/12] [RFC] firmware: arm_scmi: Add sync_cmds_atomic_replies transport flag

[Cristian Marussi]

On Thu, Aug 26, 2021 at 02:29:21PM -0400, Jim Quinlan wrote:
> On Wed, Aug 25, 2021 at 2:49 PM Cristian Marussi
> <cristian.marussi@arm.com> wrote:
> >
> > On Wed, Aug 25, 2021 at 01:17:47PM -0400, Jim Quinlan wrote:
> > > On Wed, Aug 25, 2021 at 12:38 PM Florian Fainelli <f.fainelli@gmail.com> wrote:
> > > >
> > > >
> > > >
> >
> > Hi Florian and Jim,
> >
> > > > On 8/24/2021 3:59 PM, Cristian Marussi wrote:
> > > > > A flag is added to let the transport signal the core that its handling of
> > > > > synchronous command messages implies that, after .send_message has returned
> > > > > successfully, the requested command can be assumed to be fully and
> > > > > completely executed on SCMI platform side so that any possible response
> > > > > value is already immediately available to be retrieved by a .fetch_reponse:
> > > > > in other words the polling phase can be skipped in such a case and the
> > > > > response values accessed straight away.
> > > > >
> > > > > Note that all of the above applies only when polling mode of operation was
> > > > > selected by the core: if instead a completion IRQ was found to be available
> > > > > the normal response processing path based on completions will still be
> > > > > followed.
> > > >
> > > > This might actually have to be settable on a per-message basis ideally
> > > > since we may be transporting short lived SCMI messages for which the
> > > > completion can be done at SMC time, and long lived SCMI messages (e.g.:
> > > > involving a voltage change) for which we would prefer a completion
> > > > interrupt. Jim, what do you think?
> > > Even if the SCMI main driver could be configured this way in an
> > > elegant manner, I'm not sure that there is a clean way of specifying
> > > this  attribute on a per-message basis.  Certainly we could do this
> > > with our own protocols, but  many of our "long lived" messages are the
> > > Perf protocol's set_level command.  At any rate, let me give it some
> > > thought.
> > >
> >
> > The new flag .sync_cmds_atomic_replies applies only when polling mode
> > has been selected for a specific cmd transaction, which means when no
> > completion IRQ was found available OR if xfer.poll_completion was
> > excplicitly set for a specific command.
> >
> > At the moment in this series (unknown bugs apart :D), if you have a
> > channel configured with a completion IRQ and the .sync_cmds_atomic_replies
> > set for the transport, this latter flag would be generally ignored and a
> > wait_for_completion() will be normally used upon reception of the
> > completionIRQ, UNLESS you specify that one specific command has to be
> > polled using the per message xfer.poll_completion flag: so you should be
> > already able to selectively use a polling which immediately returns after
> > the smc by setting xfer.poll_completion for that specific short lived
> > message (since sync_cmds_atomic_replies is set and applies to pollmode).
> > On the other side any other LONG lived message will be naturally handled
> > via completionIRQ + wait_for_completion. (at least that was the aim..)
> >
> > !!! NOTE that you'll have also to drop
> >
> >  [PATCH v4 10/12] [RFC] firmware: arm_scmi: Make smc transport atomic
> >
> > from this series for the wait_completion to happen as you wish.
> 
> Hi Cristian,
> 
Hi Jim,

> I've tested all commits on our SMC-based system.  I also tested all commits
> minus  "10/12 [RFC] firmware: arm_scmi: Make smc transport atomic".
> This was a basic stress test, not a comprehensive one.  So
> 
> Tested-by: Jim Quinlan <james.quinlan@broadcom.com>
> 

Thanks a lot for this testing.

> Of course I have a strong preference for omitting  "10/12 [RFC]" :-).
> FWIW, if you are not planning on dropping this commit, perhaps there
> could be a transport
> node in the DT, and that could contain the  a bool  property
> "smc-atomic-capable"?

Indeed, as I was saying more than one customer/partner is asking for this
configurability so this atomic mode should be definitely configurable.
(as it could be teh case similarly with the sync_cmds_atomic_replies
depedning on SCMI server placement..)

I'll talk with Sudeep in general about the series and this configurations;
in fact I can exclude that I'll commit this series with 10/12 as it is right
now.

Thanks for the feedback !

Cristian

> 
> Regards,
> Jim Quinlan
> Broadcom STB
> 
> >
> > As said I'm not sure that this whole mixing of polling and IRQs on the
> > same channel on a regular won't cause any issues: any feedback on this
> > from your setup is much appreciated.
> > (maybe it's fine for SMC transport, but it led to a bit of hell in the
> > past with mboxes AFAIK...)
> >
> > Thanks a lot again for your feedback, I'll have to chat with Sudeep
> > about the various issues/configs possibility that we discussed and I'll
> > keep you in the loop.
> >
> > Thanks,
> > Cristian
> >
> > P.S.: I'll be off for a few weeks, so even though I'll keep an eye on
> > the mail, I cannot guarantee any responsiveness :D



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 11/12] [RFC] firmware: arm_scmi: Add sync_cmds_atomic_replies transport flag

[Cristian Marussi]

On Thu, Aug 26, 2021 at 02:29:21PM -0400, Jim Quinlan wrote:
> On Wed, Aug 25, 2021 at 2:49 PM Cristian Marussi
> <cristian.marussi@arm.com> wrote:
> >
> > On Wed, Aug 25, 2021 at 01:17:47PM -0400, Jim Quinlan wrote:
> > > On Wed, Aug 25, 2021 at 12:38 PM Florian Fainelli <f.fainelli@gmail.com> wrote:
> > > >
> > > >
> > > >
> >
> > Hi Florian and Jim,
> >
> > > > On 8/24/2021 3:59 PM, Cristian Marussi wrote:
> > > > > A flag is added to let the transport signal the core that its handling of
> > > > > synchronous command messages implies that, after .send_message has returned
> > > > > successfully, the requested command can be assumed to be fully and
> > > > > completely executed on SCMI platform side so that any possible response
> > > > > value is already immediately available to be retrieved by a .fetch_reponse:
> > > > > in other words the polling phase can be skipped in such a case and the
> > > > > response values accessed straight away.
> > > > >
> > > > > Note that all of the above applies only when polling mode of operation was
> > > > > selected by the core: if instead a completion IRQ was found to be available
> > > > > the normal response processing path based on completions will still be
> > > > > followed.
> > > >
> > > > This might actually have to be settable on a per-message basis ideally
> > > > since we may be transporting short lived SCMI messages for which the
> > > > completion can be done at SMC time, and long lived SCMI messages (e.g.:
> > > > involving a voltage change) for which we would prefer a completion
> > > > interrupt. Jim, what do you think?
> > > Even if the SCMI main driver could be configured this way in an
> > > elegant manner, I'm not sure that there is a clean way of specifying
> > > this  attribute on a per-message basis.  Certainly we could do this
> > > with our own protocols, but  many of our "long lived" messages are the
> > > Perf protocol's set_level command.  At any rate, let me give it some
> > > thought.
> > >
> >
> > The new flag .sync_cmds_atomic_replies applies only when polling mode
> > has been selected for a specific cmd transaction, which means when no
> > completion IRQ was found available OR if xfer.poll_completion was
> > excplicitly set for a specific command.
> >
> > At the moment in this series (unknown bugs apart :D), if you have a
> > channel configured with a completion IRQ and the .sync_cmds_atomic_replies
> > set for the transport, this latter flag would be generally ignored and a
> > wait_for_completion() will be normally used upon reception of the
> > completionIRQ, UNLESS you specify that one specific command has to be
> > polled using the per message xfer.poll_completion flag: so you should be
> > already able to selectively use a polling which immediately returns after
> > the smc by setting xfer.poll_completion for that specific short lived
> > message (since sync_cmds_atomic_replies is set and applies to pollmode).
> > On the other side any other LONG lived message will be naturally handled
> > via completionIRQ + wait_for_completion. (at least that was the aim..)
> >
> > !!! NOTE that you'll have also to drop
> >
> >  [PATCH v4 10/12] [RFC] firmware: arm_scmi: Make smc transport atomic
> >
> > from this series for the wait_completion to happen as you wish.
> 
> Hi Cristian,
> 

Hi Jim,

> I've tested all commits on our SMC-based system.  I also tested all commits
> minus  "10/12 [RFC] firmware: arm_scmi: Make smc transport atomic".
> This was a basic stress test, not a comprehensive one.  So
> 
> Tested-by: Jim Quinlan <james.quinlan@broadcom.com>
> 
> Of course I have a strong preference for omitting  "10/12 [RFC]" :-).
> FWIW, if you are not planning on dropping this commit, perhaps there
> could be a transport
> node in the DT, and that could contain the  a bool  property
> "smc-atomic-capable"?
> 

I just posted V5 on this SCMI atomic transport series, where the atomic
mode behaviour of a transport can be selected by a Kconfig which is defined
as default N: so this new series should behave out-of-the-box like with the
previous one when you had dropped as a whole the SMC atomic patch.

Any feedback welcome.

Thanks,
Cristian


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 11/12] [RFC] firmware: arm_scmi: Add sync_cmds_atomic_replies transport flag

[Jim Quinlan]

[-- Attachment #1.1: Type: text/plain, Size: 4588 bytes --]

On Thu, Sep 23, 2021 at 11:03 AM Cristian Marussi
<cristian.marussi@arm.com> wrote:
>
> On Thu, Aug 26, 2021 at 02:29:21PM -0400, Jim Quinlan wrote:
> > On Wed, Aug 25, 2021 at 2:49 PM Cristian Marussi
> > <cristian.marussi@arm.com> wrote:
> > >
> > > On Wed, Aug 25, 2021 at 01:17:47PM -0400, Jim Quinlan wrote:
> > > > On Wed, Aug 25, 2021 at 12:38 PM Florian Fainelli <f.fainelli@gmail.com> wrote:
> > > > >
> > > > >
> > > > >
> > >
> > > Hi Florian and Jim,
> > >
> > > > > On 8/24/2021 3:59 PM, Cristian Marussi wrote:
> > > > > > A flag is added to let the transport signal the core that its handling of
> > > > > > synchronous command messages implies that, after .send_message has returned
> > > > > > successfully, the requested command can be assumed to be fully and
> > > > > > completely executed on SCMI platform side so that any possible response
> > > > > > value is already immediately available to be retrieved by a .fetch_reponse:
> > > > > > in other words the polling phase can be skipped in such a case and the
> > > > > > response values accessed straight away.
> > > > > >
> > > > > > Note that all of the above applies only when polling mode of operation was
> > > > > > selected by the core: if instead a completion IRQ was found to be available
> > > > > > the normal response processing path based on completions will still be
> > > > > > followed.
> > > > >
> > > > > This might actually have to be settable on a per-message basis ideally
> > > > > since we may be transporting short lived SCMI messages for which the
> > > > > completion can be done at SMC time, and long lived SCMI messages (e.g.:
> > > > > involving a voltage change) for which we would prefer a completion
> > > > > interrupt. Jim, what do you think?
> > > > Even if the SCMI main driver could be configured this way in an
> > > > elegant manner, I'm not sure that there is a clean way of specifying
> > > > this  attribute on a per-message basis.  Certainly we could do this
> > > > with our own protocols, but  many of our "long lived" messages are the
> > > > Perf protocol's set_level command.  At any rate, let me give it some
> > > > thought.
> > > >
> > >
> > > The new flag .sync_cmds_atomic_replies applies only when polling mode
> > > has been selected for a specific cmd transaction, which means when no
> > > completion IRQ was found available OR if xfer.poll_completion was
> > > excplicitly set for a specific command.
> > >
> > > At the moment in this series (unknown bugs apart :D), if you have a
> > > channel configured with a completion IRQ and the .sync_cmds_atomic_replies
> > > set for the transport, this latter flag would be generally ignored and a
> > > wait_for_completion() will be normally used upon reception of the
> > > completionIRQ, UNLESS you specify that one specific command has to be
> > > polled using the per message xfer.poll_completion flag: so you should be
> > > already able to selectively use a polling which immediately returns after
> > > the smc by setting xfer.poll_completion for that specific short lived
> > > message (since sync_cmds_atomic_replies is set and applies to pollmode).
> > > On the other side any other LONG lived message will be naturally handled
> > > via completionIRQ + wait_for_completion. (at least that was the aim..)
> > >
> > > !!! NOTE that you'll have also to drop
> > >
> > >  [PATCH v4 10/12] [RFC] firmware: arm_scmi: Make smc transport atomic
> > >
> > > from this series for the wait_completion to happen as you wish.
> >
> > Hi Cristian,
> >
>
> Hi Jim,
>
> > I've tested all commits on our SMC-based system.  I also tested all commits
> > minus  "10/12 [RFC] firmware: arm_scmi: Make smc transport atomic".
> > This was a basic stress test, not a comprehensive one.  So
> >
> > Tested-by: Jim Quinlan <james.quinlan@broadcom.com>
> >
> > Of course I have a strong preference for omitting  "10/12 [RFC]" :-).
> > FWIW, if you are not planning on dropping this commit, perhaps there
> > could be a transport
> > node in the DT, and that could contain the  a bool  property
> > "smc-atomic-capable"?
> >
>
> I just posted V5 on this SCMI atomic transport series, where the atomic
> mode behaviour of a transport can be selected by a Kconfig which is defined
> as default N: so this new series should behave out-of-the-box like with the
> previous one when you had dropped as a whole the SMC atomic patch.
>
> Any feedback welcome.

Hi Christian,

This is very much appreciated, thanks!    No feedback except

Tested-by: Jim Quinlan <james.quinlan@broadcom.com>

Thanks again,
Jim
>
>
> Thanks,
> Cristian
>

[-- Attachment #1.2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4210 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 11/12] [RFC] firmware: arm_scmi: Add sync_cmds_atomic_replies transport flag

[Cristian Marussi]

On Mon, Oct 04, 2021 at 01:50:04PM -0400, Jim Quinlan wrote:
> On Thu, Sep 23, 2021 at 11:03 AM Cristian Marussi
> <cristian.marussi@arm.com> wrote:
> >
> > On Thu, Aug 26, 2021 at 02:29:21PM -0400, Jim Quinlan wrote:
> > > On Wed, Aug 25, 2021 at 2:49 PM Cristian Marussi
> > > <cristian.marussi@arm.com> wrote:
> > > >
> > > > On Wed, Aug 25, 2021 at 01:17:47PM -0400, Jim Quinlan wrote:
> > > > > On Wed, Aug 25, 2021 at 12:38 PM Florian Fainelli <f.fainelli@gmail.com> wrote:
> > > > > >
> > > > > >
> > > > > >
> > > >
> > > > Hi Florian and Jim,
> > > >
> > > > > > On 8/24/2021 3:59 PM, Cristian Marussi wrote:
> > > > > > > A flag is added to let the transport signal the core that its handling of
> > > > > > > synchronous command messages implies that, after .send_message has returned
> > > > > > > successfully, the requested command can be assumed to be fully and
> > > > > > > completely executed on SCMI platform side so that any possible response
> > > > > > > value is already immediately available to be retrieved by a .fetch_reponse:
> > > > > > > in other words the polling phase can be skipped in such a case and the
> > > > > > > response values accessed straight away.
> > > > > > >
> > > > > > > Note that all of the above applies only when polling mode of operation was
> > > > > > > selected by the core: if instead a completion IRQ was found to be available
> > > > > > > the normal response processing path based on completions will still be
> > > > > > > followed.
> > > > > >
> > > > > > This might actually have to be settable on a per-message basis ideally
> > > > > > since we may be transporting short lived SCMI messages for which the
> > > > > > completion can be done at SMC time, and long lived SCMI messages (e.g.:
> > > > > > involving a voltage change) for which we would prefer a completion
> > > > > > interrupt. Jim, what do you think?
> > > > > Even if the SCMI main driver could be configured this way in an
> > > > > elegant manner, I'm not sure that there is a clean way of specifying
> > > > > this  attribute on a per-message basis.  Certainly we could do this
> > > > > with our own protocols, but  many of our "long lived" messages are the
> > > > > Perf protocol's set_level command.  At any rate, let me give it some
> > > > > thought.
> > > > >
> > > >
> > > > The new flag .sync_cmds_atomic_replies applies only when polling mode
> > > > has been selected for a specific cmd transaction, which means when no
> > > > completion IRQ was found available OR if xfer.poll_completion was
> > > > excplicitly set for a specific command.
> > > >
> > > > At the moment in this series (unknown bugs apart :D), if you have a
> > > > channel configured with a completion IRQ and the .sync_cmds_atomic_replies
> > > > set for the transport, this latter flag would be generally ignored and a
> > > > wait_for_completion() will be normally used upon reception of the
> > > > completionIRQ, UNLESS you specify that one specific command has to be
> > > > polled using the per message xfer.poll_completion flag: so you should be
> > > > already able to selectively use a polling which immediately returns after
> > > > the smc by setting xfer.poll_completion for that specific short lived
> > > > message (since sync_cmds_atomic_replies is set and applies to pollmode).
> > > > On the other side any other LONG lived message will be naturally handled
> > > > via completionIRQ + wait_for_completion. (at least that was the aim..)
> > > >
> > > > !!! NOTE that you'll have also to drop
> > > >
> > > >  [PATCH v4 10/12] [RFC] firmware: arm_scmi: Make smc transport atomic
> > > >
> > > > from this series for the wait_completion to happen as you wish.
> > >
> > > Hi Cristian,
> > >
> >
> > Hi Jim,
> >
> > > I've tested all commits on our SMC-based system.  I also tested all commits
> > > minus  "10/12 [RFC] firmware: arm_scmi: Make smc transport atomic".
> > > This was a basic stress test, not a comprehensive one.  So
> > >
> > > Tested-by: Jim Quinlan <james.quinlan@broadcom.com>
> > >
> > > Of course I have a strong preference for omitting  "10/12 [RFC]" :-).
> > > FWIW, if you are not planning on dropping this commit, perhaps there
> > > could be a transport
> > > node in the DT, and that could contain the  a bool  property
> > > "smc-atomic-capable"?
> > >
> >
> > I just posted V5 on this SCMI atomic transport series, where the atomic
> > mode behaviour of a transport can be selected by a Kconfig which is defined
> > as default N: so this new series should behave out-of-the-box like with the
> > previous one when you had dropped as a whole the SMC atomic patch.
> >
> > Any feedback welcome.
> 
> Hi Christian,
> 

Hi Jim,

> This is very much appreciated, thanks!    No feedback except
> 
> Tested-by: Jim Quinlan <james.quinlan@broadcom.com>
> 

Glad to hear that.
I'll see if I can gather more feedback from other partners that were
interested on using the atomic path (which was supposed to be the main
feature of this series at the end :D...)

Thanks for your testing.
Cristian

> Thanks again,
> Jim
> >
> >
> > Thanks,
> > Cristian
> >



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel