| Message ID | 20240813180747.1439034-1-florian.fainelli@broadcom.com |
|---|---|
| Headers | show |
| Series | Support for I/O width within ARM SCMI SHMEM | expand |
On Tue, Aug 13, 2024 at 11:07:47AM -0700, Florian Fainelli wrote: > 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. > Hi Florian, I gave it ago at this on a JUNO regarding the mailbox/shmem transport without any issue. I'll have a go later on an OPTEE/shmem scenario too. This looks fundamentally good to me, since you moved all ops setup at setup time and you keep the pointers per-channel instead of global... A few remarks down below. > 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 | 10 ++- > .../firmware/arm_scmi/scmi_transport_smc.c | 11 ++- > drivers/firmware/arm_scmi/shmem.c | 81 +++++++++++++++++-- > 5 files changed, 126 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..2a624870954d 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 > + * @shmem_io_ops: Transport specific I/O operations Fix the doxy param name...it s op_ops now. > */ > 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..fba312128426 100644 > --- a/drivers/firmware/arm_scmi/scmi_transport_optee.c > +++ b/drivers/firmware/arm_scmi/scmi_transport_optee.c > @@ -128,6 +128,7 @@ struct scmi_optee_channel { > struct scmi_shared_mem __iomem *shmem; > struct scmi_msg_payld *msg; > } req; > + struct scmi_shmem_io_ops *io_ops; Describe this in the doxy.... > struct tee_shm *tee_shm; > struct list_head link; > }; > @@ -350,7 +351,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 +467,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 +487,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..1de06c2fc63a 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 > + * @shmem_io_ops: Transport specific I/O operations Fix doxy param name > * @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..139bbbc4b2ee 100644 > --- a/drivers/firmware/arm_scmi/shmem.c > +++ b/drivers/firmware/arm_scmi/shmem.c > @@ -34,9 +34,62 @@ struct scmi_shared_mem { > u8 msg_payload[]; > }; > > +#define SHMEM_IO_OPS(w, s, amt) \ > +static inline void shmem_memcpy_fromio##s(void *to, \ > + const volatile void __iomem *from, \ > + size_t count) \ > +{ \ > + while (count) { \ > + *(u##s *)to = __raw_read##w(from); \ > + from += amt; \ > + to += amt; \ > + count -= amt; \ > + } \ > +} ... I may be missing a lot here...bear with me...so... ... AFAIU, as suggested by Peng, you moved away from iowrite##s and ioread##s in favour of __raw_write/read##w so as to avoid the implicit barriers on each loop iteration...(I suppose..) ...but should we place some sort of final io barrier (similarly to iowrite) at the end of the loop ? > +static inline void shmem_memcpy_toio##s(volatile void __iomem *to, \ > + const void *from, \ > + size_t count) \ > +{ \ > + while (count) { \ > + __raw_write##w(*(u##s *)from, to); \ > + from += amt; \ > + to += amt; \ > + count -= amt; \ > + } \ > +} ...same concern here > +static struct scmi_shmem_io_ops shmem_io_ops##s = { \ > + .fromio = shmem_memcpy_fromio##s, \ > + .toio = shmem_memcpy_toio##s, \ > +}; > + There are a bunch of warn/errs from checkpatch --strict, beside the volatile here and on the previous typedefs, also about args reuse and trailing semicolon in these macros... Thanks, Cristian
On 8/16/24 10:02, Cristian Marussi wrote: > On Tue, Aug 13, 2024 at 11:07:47AM -0700, Florian Fainelli wrote: >> 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. >> > > Hi Florian, > > I gave it ago at this on a JUNO regarding the mailbox/shmem transport > without any issue. I'll have a go later on an OPTEE/shmem scenario too. > > This looks fundamentally good to me, since you moved all ops setup at > setup time and you keep the pointers per-channel instead of global... Thanks! > > A few remarks down below. > >> Signed-off-by: Florian Fainelli <florian.fainelli@broadcom.com> [snip] > > ... I may be missing a lot here...bear with me...so... > > ... AFAIU, as suggested by Peng, you moved away from iowrite##s and ioread##s > in favour of __raw_write/read##w so as to avoid the implicit barriers on each > loop iteration...(I suppose..) > > ...but should we place some sort of final io barrier (similarly to iowrite) > at the end of the loop ? There is no leading or trailing barrier with the ARM64 memcpy_{to,from}io routines which is why this was carried forward as-is. There is an implicit barrier with the iowrite32() in the tx_prepare(), so I suppose we are somewhat safe on that part. Likewise with the fetch_response()/fetch_notification() we have an implicit barrier within the ioread32() and then there is a data dependency since we ought to be consuming the response/notification. For ARM 32-bit the implementation uses readb()/writeb() which does include barriers. > >> +static inline void shmem_memcpy_toio##s(volatile void __iomem *to, \ >> + const void *from, \ >> + size_t count) \ >> +{ \ >> + while (count) { \ >> + __raw_write##w(*(u##s *)from, to); \ >> + from += amt; \ >> + to += amt; \ >> + count -= amt; \ >> + } \ >> +} > > ...same concern here > >> +static struct scmi_shmem_io_ops shmem_io_ops##s = { \ >> + .fromio = shmem_memcpy_fromio##s, \ >> + .toio = shmem_memcpy_toio##s, \ >> +}; The macro might be a tad too much given that we only support one width, in case we needed to add a specific size in the future we could use a macro again I suppose, for now, just inlined the implementation for the 4-byte / 32-bit size. >> + > > There are a bunch of warn/errs from checkpatch --strict, beside the volatile > here and on the previous typedefs, also about args reuse and trailing semicolon > in these macros... I don't think we can silence the volatile ones, checkpatch --strict did not complain about the typedefs in my case, what did it look like in yours?
On Fri, Aug 16, 2024 at 10:39:42AM -0700, Florian Fainelli wrote: > On 8/16/24 10:02, Cristian Marussi wrote: > > On Tue, Aug 13, 2024 at 11:07:47AM -0700, Florian Fainelli wrote: > > > 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. > > > > > > > Hi Florian, > > Hi, > > I gave it ago at this on a JUNO regarding the mailbox/shmem transport > > without any issue. I'll have a go later on an OPTEE/shmem scenario too. > > > > This looks fundamentally good to me, since you moved all ops setup at > > setup time and you keep the pointers per-channel instead of global... > > Thanks! > [snip] > > > + > > > > There are a bunch of warn/errs from checkpatch --strict, beside the volatile > > here and on the previous typedefs, also about args reuse and trailing semicolon > > in these macros... > > I don't think we can silence the volatile ones, checkpatch --strict did not > complain about the typedefs in my case, what did it look like in yours? ...I dont get warns on new typedefs..only on volatile and macro args reuse ---8<--- WARNING: Use of volatile is usually wrong: see Documentation/process/volatile-considered-harmful.rst #36: FILE: drivers/firmware/arm_scmi/common.h:322: +typedef void (*shmem_copy_toio_t)(volatile void __iomem *to, const void *from, WARNING: Use of volatile is usually wrong: see Documentation/process/volatile-considered-harmful.rst #38: FILE: drivers/firmware/arm_scmi/common.h:324: +typedef void (*shmem_copy_fromio_t)(void *to, const volatile void __iomem *from, CHECK: Macro argument reuse 'amt' - possible side-effects? #94: FILE: drivers/firmware/arm_scmi/shmem.c:37: +#define SHMEM_IO_OPS(w, s, amt) \ +static inline void shmem_memcpy_fromio##s(void *to, \ + const volatile void __iomem *from, \ + size_t count) \ +{ \ + while (count) { \ + *(u##s *)to = __raw_read##w(from); \ + from += amt; \ + to += amt; \ + count -= amt; \ + } \ +} \ +static inline void shmem_memcpy_toio##s(volatile void __iomem *to, \ + const void *from, \ + size_t count) \ +{ \ + while (count) { \ + __raw_write##w(*(u##s *)from, to); \ + from += amt; \ + to += amt; \ + count -= amt; \ + } \ +} \ +static struct scmi_shmem_io_ops shmem_io_ops##s = { \ + .fromio = shmem_memcpy_fromio##s, \ + .toio = shmem_memcpy_toio##s, \ +}; WARNING: macros should not use a trailing semicolon #94: FILE: drivers/firmware/arm_scmi/shmem.c:37: +#define SHMEM_IO_OPS(w, s, amt) \ +static inline void shmem_memcpy_fromio##s(void *to, \ + const volatile void __iomem *from, \ + size_t count) \ +{ \ + while (count) { \ + *(u##s *)to = __raw_read##w(from); \ + from += amt; \ + to += amt; \ + count -= amt; \ + } \ +} \ +static inline void shmem_memcpy_toio##s(volatile void __iomem *to, \ + const void *from, \ + size_t count) \ +{ \ + while (count) { \ + __raw_write##w(*(u##s *)from, to); \ + from += amt; \ + to += amt; \ + count -= amt; \ + } \ +} \ +static struct scmi_shmem_io_ops shmem_io_ops##s = { \ + .fromio = shmem_memcpy_fromio##s, \ + .toio = shmem_memcpy_toio##s, \ +}; WARNING: Use of volatile is usually wrong: see Documentation/process/volatile-considered-harmful.rst #96: FILE: drivers/firmware/arm_scmi/shmem.c:39: + const volatile void __iomem *from, \ WARNING: Use of volatile is usually wrong: see Documentation/process/volatile-considered-harmful.rst #106: FILE: drivers/firmware/arm_scmi/shmem.c:49: +static inline void shmem_memcpy_toio##s(volatile void __iomem *to, \ WARNING: Use of volatile is usually wrong: see Documentation/process/volatile-considered-harmful.rst #128: FILE: drivers/firmware/arm_scmi/shmem.c:71: + const volatile void __iomem *from, WARNING: Use of volatile is usually wrong: see Documentation/process/volatile-considered-harmful.rst #134: FILE: drivers/firmware/arm_scmi/shmem.c:77: +static inline void shmem_memcpy_toio(volatile void __iomem *to, total: 0 errors, 7 warnings, 1 checks, 312 lines checked NOTE: For some of the reported defects, checkpatch may be able to mechanically convert to the typical style using --fix or --fix-inplace. "[PATCH] firmware: arm_scmi: Support 'reg-io-width' property for" has style problems, please review. NOTE: If any of the errors are false positives, please report them to the maintainer, see CHECKPATCH in MAINTAINERS. ---8<----