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

[Florian Fainelli <florian.fainelli@broadcom.com>]

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@broadcom.com>
>> ---
>>   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