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
