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

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

On 8/22/24 03:46, Sudeep Holla wrote:
> Hi Florian,
> 
> Sorry for getting late to this party, I wasn't able to review this before.
> Overall changes look correct. But my main concern is that is SCMI the right
> place to have such IO accessors. It is better to run it through Arnd if
> he is happy with it before I send him the pull request containing these.

Sure, would definitively want more eyes to review.

> 
> On Fri, Aug 16, 2024 at 03:42:21PM -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.
>>
> 
> Is this limitation on the hardware for both read and writes ?

This applies to both reads and writes. We have to make accesses on a 4 
byte boundary and of exactly 4 bytes in size.

> The reason I ask is I see arm64 does have memcpy_toio_aligned() or
> __iowrite32_copy_full() for 32 bit aligned writes.
> 

That appears to work nicely on ARM64 and ARM 32-bit, thanks for the 
suggestion! One needs to be careful that __io{read,write}32_copy takes 
32-bit units, not bytes!

FWIW, here is my diff between v3 and v4:

diff --git a/drivers/firmware/arm_scmi/common.h 
b/drivers/firmware/arm_scmi/common.h
index 73bb496fac01..a13f79b37c99 100644
--- a/drivers/firmware/arm_scmi/common.h
+++ b/drivers/firmware/arm_scmi/common.h
@@ -319,9 +319,9 @@ enum scmi_bad_msg {
  /* 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,
+typedef void (*shmem_copy_toio_t)(void __iomem *to, const void *from,
                                   size_t count);
-typedef void (*shmem_copy_fromio_t)(void *to, const volatile void 
__iomem *from,
+typedef void (*shmem_copy_fromio_t)(void *to, const void __iomem *from,
                                     size_t count);

  /**
diff --git a/drivers/firmware/arm_scmi/shmem.c 
b/drivers/firmware/arm_scmi/shmem.c
index aded5f1cd49f..e9f30ab671a8 100644
--- a/drivers/firmware/arm_scmi/shmem.c
+++ b/drivers/firmware/arm_scmi/shmem.c
@@ -35,33 +35,25 @@ struct scmi_shared_mem {
  };

  static inline void shmem_memcpy_fromio32(void *to,
-                                        const volatile void __iomem *from,
+                                        const void __iomem *from,
                                          size_t count)
  {
-       while (count) {
-               *(u32 *)to = __raw_readl(from);
-               from += 4;
-               to += 4;
-               count -= 4;
-       }
+       WARN_ON(!IS_ALIGNED((unsigned long)from, 4) ||
+               !IS_ALIGNED((unsigned long)to, 4) ||
+               count % 4);

-       /* Ensure all reads from I/O have completed */
-       rmb();
+       __ioread32_copy(to, from, count / 4);
  }

-static inline void shmem_memcpy_toio32(volatile void __iomem *to,
+static inline void shmem_memcpy_toio32(void __iomem *to,
                                        const void *from,
                                        size_t count)
  {
-       while (count) {
-               __raw_writel(*(u32 *)from, to);
-               from += 4;
-               to += 4;
-               count -= 4;
-       }
+       WARN_ON(!IS_ALIGNED((unsigned long)to, 4) ||
+               !IS_ALIGNED((unsigned long)from, 4) ||
+               count % 4);

-       /* Ensure all writes to I/O have completed */
-       wmb();
+       __iowrite32_copy(to, from, count / 4);
  }

  static struct scmi_shmem_io_ops shmem_io_ops32 = {
@@ -73,13 +65,13 @@ static struct scmi_shmem_io_ops shmem_io_ops32 = {
   * pre-processor.
   */
  static inline void shmem_memcpy_fromio(void *to,
-                                      const volatile void __iomem *from,
+                                      const void __iomem *from,
                                        size_t count)
  {
         memcpy_fromio(to, from, count);
  }

-static inline void shmem_memcpy_toio(volatile void __iomem *to,
+static inline void shmem_memcpy_toio(void __iomem *to,
                                      const void *from,
                                      size_t count)
  {

-- 
Florian