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

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

On 8/10/2024 2:46 PM, Florian Fainelli wrote:
> Some shared memory areas might only support a certain access width,
> (e.g.: 32 bits accesses only). 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 width to the
> routines that need it.
> 
> Signed-off-by: Florian Fainelli <florian.fainelli@broadcom.com>

Doing a self review, though I will only resend after collecting 
additional feedback.

> ---
>   drivers/firmware/arm_scmi/common.h            | 14 +++-
>   .../arm_scmi/scmi_transport_mailbox.c         | 12 ++-
>   .../firmware/arm_scmi/scmi_transport_optee.c  |  7 +-
>   .../firmware/arm_scmi/scmi_transport_smc.c    | 10 ++-
>   drivers/firmware/arm_scmi/shmem.c             | 77 ++++++++++++++++---
>   5 files changed, 96 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/firmware/arm_scmi/common.h b/drivers/firmware/arm_scmi/common.h
> index 69928bbd01c2..97dae844a190 100644
> --- a/drivers/firmware/arm_scmi/common.h
> +++ b/drivers/firmware/arm_scmi/common.h
> @@ -170,6 +170,7 @@ void scmi_protocol_release(const struct scmi_handle *handle, u8 protocol_id);
>    *		       This can be dynamically set by transports at run-time
>    *		       inside their provided .chan_setup().
>    * @transport_info: Transport layer related information
> + * @shmem_io_width: I/O width in bytes of the shared memory area
>    */
>   struct scmi_chan_info {
>   	int id;
> @@ -178,6 +179,7 @@ struct scmi_chan_info {
>   	struct scmi_handle *handle;
>   	bool no_completion_irq;
>   	void *transport_info;
> +	u32 shmem_io_width;

This is not used because the individual transports store the 
shmem_io_width in their own transport_info structure.

[snip]

>   
> +#define __shmem_copy_toio_tpl(s)			\
> +	for (unsigned int i = 0; i < xfer->tx.len; i += shmem_io_width)		\
> +		iowrite##s(((u##s *)(xfer->tx.buf))[i / shmem_io_width],	\
> +			   shmem->msg_payload + i);
> +
> +#define __shmem_copy_fromio_tpl(s)			\
> +	for (unsigned int i = 0; i < xfer->rx.len; i += shmem_io_width)		\
> +		((u##s *)(xfer->rx.buf))[i / shmem_io_width] = 			\
> +			 ioread##s(shmem->msg_payload + shmem_io_width + i);

This needs to be shmem->msg_payload + 4 + i. This worked in my testing 
because I was precisely tseting with 'reg-io-width = <4>'.

[snip]

>   
>   static u32 shmem_read_header(struct scmi_shared_mem __iomem *shmem)
> @@ -81,8 +109,34 @@ static u32 shmem_read_header(struct scmi_shared_mem __iomem *shmem)
>   	return ioread32(&shmem->msg_header);
>   }
>   
> +static void __shmem_fetch_resp_notif_data(struct scmi_xfer *xfer,
> +					  struct scmi_shared_mem __iomem *shmem,
> +					  u32 shmem_io_width)
> +{
> +	/* Take a copy to the rx buffer.. */
> +	switch (shmem_io_width) {
> +	case 1:
> +		__shmem_copy_fromio_tpl(8);
> +		break;
> +	case 2:
> +		__shmem_copy_fromio_tpl(16);
> +		break;
> +	case 4:
> +		__shmem_copy_fromio_tpl(32);
> +		break;
> +	case 8:
> +		__shmem_copy_fromio_tpl(32);

This should be 64 and it looks like ioread64/iowrite64 is not 
necessarily defined for 32-bit configurations, so this needs to be gated 
with a CONFIG_64BIT define here since ioread64/iowrite64 in 
include/asm-generic/io.h is defined that way.
-- 
Florian