> 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.
