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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



> Subject: Re: [PATCH v3 2/2] firmware: arm_scmi: Support 'reg-io-
> width' property for shared memory
> 
> 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()?

This is just to avoid potential issues if there is change in future spec
That some entries are not 4 bytes aligned. I maybe over thinking
here.

A "WARN" is ok from my view.

Regards,
Peng.





[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux