Re: [PATCH v3 2/2] firmware: arm_scmi: Support 'reg-io-width' property for shared memory

Florian Fainelli <florian.fainelli@xxxxxxxxxxxx> · Tue, 20 Aug 2024 09:34:59 -0700

On 8/19/24 19:49, Peng Fan wrote:
Subject: [PATCH v3 2/2] firmware: arm_scmi: Support 'reg-io-width'
property for shared memory

Some shared memory areas might only support a certain access width,
such as 32-bit, which memcpy_{from,to}_io() does not adhere to at
least on ARM64 by making both 8-bit and 64-bit accesses to such
memory.

Update the shmem layer to support reading from and writing to such
shared memory area using the specified I/O width in the Device Tree.
The various transport layers making use of the shmem.c code are
updated accordingly to pass the I/O accessors that they store.

Signed-off-by: Florian Fainelli <florian.fainelli@xxxxxxxxxxxx>
---
  drivers/firmware/arm_scmi/common.h            | 32 ++++++-
  .../arm_scmi/scmi_transport_mailbox.c         | 13 ++-
  .../firmware/arm_scmi/scmi_transport_optee.c  | 11 ++-
  .../firmware/arm_scmi/scmi_transport_smc.c    | 11 ++-
  drivers/firmware/arm_scmi/shmem.c             | 86 +++++++++++++++++-
-
  5 files changed, 132 insertions(+), 21 deletions(-)

diff --git a/drivers/firmware/arm_scmi/common.h
b/drivers/firmware/arm_scmi/common.h
index 69928bbd01c2..73bb496fac01 100644
--- a/drivers/firmware/arm_scmi/common.h
+++ b/drivers/firmware/arm_scmi/common.h
@@ -316,6 +316,26 @@ enum scmi_bad_msg {
  	MSG_MBOX_SPURIOUS = -5,
  };

+/* Used for compactness and signature validation of the function
+pointers being
+ * passed.
+ */
+typedef void (*shmem_copy_toio_t)(volatile void __iomem *to, const
void *from,
+				  size_t count);
+typedef void (*shmem_copy_fromio_t)(void *to, const volatile void
__iomem *from,
+				    size_t count);
+
+/**
+ * struct scmi_shmem_io_ops  - I/O operations to read from/write to
+ * Shared Memory
+ *
+ * @toio: Copy data to the shared memory area
+ * @fromio: Copy data from the shared memory area  */ struct
+scmi_shmem_io_ops {
+	shmem_copy_fromio_t fromio;
+	shmem_copy_toio_t toio;
+};
+
  /* shmem related declarations */
  struct scmi_shared_mem;

@@ -336,13 +356,16 @@ struct scmi_shared_mem;  struct
scmi_shared_mem_operations {
  	void (*tx_prepare)(struct scmi_shared_mem __iomem
*shmem,
  			   struct scmi_xfer *xfer,
-			   struct scmi_chan_info *cinfo);
+			   struct scmi_chan_info *cinfo,
+			   shmem_copy_toio_t toio);
  	u32 (*read_header)(struct scmi_shared_mem __iomem
*shmem);

  	void (*fetch_response)(struct scmi_shared_mem __iomem
*shmem,
-			       struct scmi_xfer *xfer);
+			       struct scmi_xfer *xfer,
+			       shmem_copy_fromio_t fromio);
  	void (*fetch_notification)(struct scmi_shared_mem __iomem
*shmem,
-				   size_t max_len, struct scmi_xfer
*xfer);
+				   size_t max_len, struct scmi_xfer
*xfer,
+				   shmem_copy_fromio_t fromio);
  	void (*clear_channel)(struct scmi_shared_mem __iomem
*shmem);
  	bool (*poll_done)(struct scmi_shared_mem __iomem *shmem,
  			  struct scmi_xfer *xfer);
@@ -350,7 +373,8 @@ struct scmi_shared_mem_operations {
  	bool (*channel_intr_enabled)(struct scmi_shared_mem
__iomem *shmem);
  	void __iomem *(*setup_iomap)(struct scmi_chan_info *cinfo,
  				     struct device *dev,
-				     bool tx, struct resource *res);
+				     bool tx, struct resource *res,
+				     struct scmi_shmem_io_ops **ops);
  };

  const struct scmi_shared_mem_operations
*scmi_shared_mem_operations_get(void);
diff --git a/drivers/firmware/arm_scmi/scmi_transport_mailbox.c
b/drivers/firmware/arm_scmi/scmi_transport_mailbox.c
index dc5ca894d5eb..1a2e90e5c765 100644
--- a/drivers/firmware/arm_scmi/scmi_transport_mailbox.c
+++ b/drivers/firmware/arm_scmi/scmi_transport_mailbox.c
@@ -25,6 +25,7 @@
   * @chan_platform_receiver: Optional Platform Receiver mailbox
unidirectional channel
   * @cinfo: SCMI channel info
   * @shmem: Transmit/Receive shared memory area
+ * @io_ops: Transport specific I/O operations
   */
  struct scmi_mailbox {
  	struct mbox_client cl;
@@ -33,6 +34,7 @@ struct scmi_mailbox {
  	struct mbox_chan *chan_platform_receiver;
  	struct scmi_chan_info *cinfo;
  	struct scmi_shared_mem __iomem *shmem;
+	struct scmi_shmem_io_ops *io_ops;
  };

  #define client_to_scmi_mailbox(c) container_of(c, struct scmi_mailbox,
cl) @@ -43,7 +45,8 @@ static void tx_prepare(struct mbox_client *cl,
void *m)  {
  	struct scmi_mailbox *smbox = client_to_scmi_mailbox(cl);

-	core->shmem->tx_prepare(smbox->shmem, m, smbox->cinfo);
+	core->shmem->tx_prepare(smbox->shmem, m, smbox->cinfo,
+				smbox->io_ops->toio);
  }

  static void rx_callback(struct mbox_client *cl, void *m) @@ -197,7
+200,8 @@ static int mailbox_chan_setup(struct scmi_chan_info
*cinfo, struct device *dev,
  	if (!smbox)
  		return -ENOMEM;

-	smbox->shmem = core->shmem->setup_iomap(cinfo, dev, tx,
NULL);
+	smbox->shmem = core->shmem->setup_iomap(cinfo, dev, tx,
NULL,
+						&smbox->io_ops);
  	if (IS_ERR(smbox->shmem))
  		return PTR_ERR(smbox->shmem);

@@ -298,7 +302,7 @@ static void mailbox_fetch_response(struct
scmi_chan_info *cinfo,  {
  	struct scmi_mailbox *smbox = cinfo->transport_info;

-	core->shmem->fetch_response(smbox->shmem, xfer);
+	core->shmem->fetch_response(smbox->shmem, xfer,
+smbox->io_ops->fromio);
  }

  static void mailbox_fetch_notification(struct scmi_chan_info *cinfo,
@@ -306,7 +310,8 @@ static void mailbox_fetch_notification(struct
scmi_chan_info *cinfo,  {
  	struct scmi_mailbox *smbox = cinfo->transport_info;

-	core->shmem->fetch_notification(smbox->shmem, max_len,
xfer);
+	core->shmem->fetch_notification(smbox->shmem, max_len,
xfer,
+					smbox->io_ops->fromio);
  }

  static void mailbox_clear_channel(struct scmi_chan_info *cinfo) diff --
git a/drivers/firmware/arm_scmi/scmi_transport_optee.c
b/drivers/firmware/arm_scmi/scmi_transport_optee.c
index 08911f40d1ff..2be4124c6826 100644
--- a/drivers/firmware/arm_scmi/scmi_transport_optee.c
+++ b/drivers/firmware/arm_scmi/scmi_transport_optee.c
@@ -114,6 +114,7 @@ enum scmi_optee_pta_cmd {
   * @req.shmem: Virtual base address of the shared memory
   * @req.msg: Shared memory protocol handle for SCMI request and
   *   synchronous response
+ * @io_ops: Transport specific I/O operations
   * @tee_shm: TEE shared memory handle @req or NULL if using
IOMEM shmem
   * @link: Reference in agent's channel list
   */
@@ -128,6 +129,7 @@ struct scmi_optee_channel {
  		struct scmi_shared_mem __iomem *shmem;
  		struct scmi_msg_payld *msg;
  	} req;
+	struct scmi_shmem_io_ops *io_ops;
  	struct tee_shm *tee_shm;
  	struct list_head link;
  };
@@ -350,7 +352,8 @@ static int setup_dynamic_shmem(struct device
*dev, struct scmi_optee_channel *ch  static int
setup_static_shmem(struct device *dev, struct scmi_chan_info *cinfo,
  			      struct scmi_optee_channel *channel)  {
-	channel->req.shmem = core->shmem->setup_iomap(cinfo, dev,
true, NULL);
+	channel->req.shmem = core->shmem->setup_iomap(cinfo, dev,
true, NULL,
+						      &channel-
io_ops);
  	if (IS_ERR(channel->req.shmem))
  		return PTR_ERR(channel->req.shmem);

@@ -465,7 +468,8 @@ static int scmi_optee_send_message(struct
scmi_chan_info *cinfo,
  		ret = invoke_process_msg_channel(channel,
  						 core->msg-
command_size(xfer));
  	} else {
-		core->shmem->tx_prepare(channel->req.shmem, xfer,
cinfo);
+		core->shmem->tx_prepare(channel->req.shmem, xfer,
cinfo,
+					channel->io_ops->toio);
  		ret = invoke_process_smt_channel(channel);
  	}

@@ -484,7 +488,8 @@ static void scmi_optee_fetch_response(struct
scmi_chan_info *cinfo,
  		core->msg->fetch_response(channel->req.msg,
  					  channel->rx_len, xfer);
  	else
-		core->shmem->fetch_response(channel->req.shmem,
xfer);
+		core->shmem->fetch_response(channel->req.shmem,
xfer,
+					    channel->io_ops->fromio);
  }

  static void scmi_optee_mark_txdone(struct scmi_chan_info *cinfo, int
ret, diff --git a/drivers/firmware/arm_scmi/scmi_transport_smc.c
b/drivers/firmware/arm_scmi/scmi_transport_smc.c
index c6c69a17a9cc..04e715ec1570 100644
--- a/drivers/firmware/arm_scmi/scmi_transport_smc.c
+++ b/drivers/firmware/arm_scmi/scmi_transport_smc.c
@@ -45,6 +45,7 @@
   * @irq: An optional IRQ for completion
   * @cinfo: SCMI channel info
   * @shmem: Transmit/Receive shared memory area
+ * @io_ops: Transport specific I/O operations
   * @shmem_lock: Lock to protect access to Tx/Rx shared memory area.
   *		Used when NOT operating in atomic mode.
   * @inflight: Atomic flag to protect access to Tx/Rx shared memory
area.
@@ -60,6 +61,7 @@ struct scmi_smc {
  	int irq;
  	struct scmi_chan_info *cinfo;
  	struct scmi_shared_mem __iomem *shmem;
+	struct scmi_shmem_io_ops *io_ops;
  	/* Protect access to shmem area */
  	struct mutex shmem_lock;
  #define INFLIGHT_NONE	MSG_TOKEN_MAX
@@ -144,7 +146,8 @@ static int smc_chan_setup(struct
scmi_chan_info *cinfo, struct device *dev,
  	if (!scmi_info)
  		return -ENOMEM;

-	scmi_info->shmem = core->shmem->setup_iomap(cinfo, dev,
tx, &res);
+	scmi_info->shmem = core->shmem->setup_iomap(cinfo, dev,
tx, &res,
+						    &scmi_info-
io_ops);
  	if (IS_ERR(scmi_info->shmem))
  		return PTR_ERR(scmi_info->shmem);

@@ -229,7 +232,8 @@ static int smc_send_message(struct
scmi_chan_info *cinfo,
  	 */
  	smc_channel_lock_acquire(scmi_info, xfer);

-	core->shmem->tx_prepare(scmi_info->shmem, xfer, cinfo);
+	core->shmem->tx_prepare(scmi_info->shmem, xfer, cinfo,
+				scmi_info->io_ops->toio);

  	if (scmi_info->cap_id != ULONG_MAX)
  		arm_smccc_1_1_invoke(scmi_info->func_id,
scmi_info->cap_id, 0, @@ -253,7 +257,8 @@ static void
smc_fetch_response(struct scmi_chan_info *cinfo,  {
  	struct scmi_smc *scmi_info = cinfo->transport_info;

-	core->shmem->fetch_response(scmi_info->shmem, xfer);
+	core->shmem->fetch_response(scmi_info->shmem, xfer,
+				    scmi_info->io_ops->fromio);
  }

  static void smc_mark_txdone(struct scmi_chan_info *cinfo, int ret, diff
--git a/drivers/firmware/arm_scmi/shmem.c
b/drivers/firmware/arm_scmi/shmem.c
index 01d8a9398fe8..aded5f1cd49f 100644
--- a/drivers/firmware/arm_scmi/shmem.c
+++ b/drivers/firmware/arm_scmi/shmem.c
@@ -34,9 +34,67 @@ struct scmi_shared_mem {
  	u8 msg_payload[];
  };

+static inline void shmem_memcpy_fromio32(void *to,
+					 const volatile void __iomem
*from,
+					 size_t count)
+{
+	while (count) {
+		*(u32 *)to = __raw_readl(from);
+		from += 4;
+		to += 4;
+		count -= 4;

This may not have issue, considering the 'count % 4' will be 0
and 'from' is 4 bytes aligned.

Correct, this cannot possibly happen today by virtue of msg->payload 
being naturally aligned on a 4 bytes boundary.


But I think it maybe better to add check if 'count' and 'from'
are not 4 bytes aligned.

Humm, what would be the fallback, or should we just WARN()?


+	}
+
+	/* Ensure all reads from I/O have completed */
+	rmb();

Is there a need to use rmb here? I am not sure, just wonder.

I personally do not think there is because there is an implicit data 
dependency eventually we will be consuming data that was copied into 
msg->payload and that will ensure that the data is there.


+}
+
+static inline void shmem_memcpy_toio32(volatile void __iomem *to,
+				       const void *from,
+				       size_t count)
+{
+	while (count) {
+		__raw_writel(*(u32 *)from, to);
+		from += 4;
+		to += 4;
+		count -= 4;
+	}

Ditto.

+
+	/* Ensure all writes to I/O have completed */
+	wmb();

This maybe not needed.
The mailbox will use "writel", the SMC will use "smc",
the virtio will have "hvc", both will have barrier I think.

Yes, those are all implicit barriers, I was attempting to address 
Christian's concerns raised in v2:

https://lore.kernel.org/all/Zr-GJts3Gu6GEkhC@pluto/
--
Florian








