On Mon, Aug 21, 2023, at 08:45, Weili Qian wrote:
> On 2023/8/21 18:26, Will Deacon wrote:
>> On Mon, Aug 21, 2023 at 04:19:54PM +0800, Herbert Xu wrote:
> Thanks for the review.
>
> Since the HiSilicon accelerator devices are supported only on the ARM64
> platform,
> the following 128bit read and write interfaces are added to the driver,
> is this OK?
>
> #if defined(CONFIG_ARM64)
> static void qm_write128(void __iomem *addr, const void *buffer)
> {
The naming makes it specific to the driver, which is not
appropriate for a global definition. Just follow the
generic naming. I guess you wouldn't have to do both
the readl/writel and the ioread32/iowrite32 variants, so
just start with the ioread/iowrite version. That also
avoids having to come up with a new letter.
You have the arguments in the wrong order compared to iowrite32(),
which is very confusing.
Instead of the pointer to the buffer, I'd suggest passing the
value by value here, to make it behave like the other ones.
This does mean it won't build on targets that don't
define u128, but I think we can handle that in a Kconfig
symbol.
> unsigned long tmp0 = 0, tmp1 = 0;
Don't initialize local variable to zero, that is generally
a bad idea because it hides cases where they are not
initialized properly.
> asm volatile("ldp %0, %1, %3\n"
> "stp %0, %1, %2\n"
This is missing the endian-swap for big-endian kernels.
> "dmb oshst\n"
You have the barrier at the wrong end of the helper, it
needs to before the actual store to have the intended
effect.
Also, you should really use the generic __io_bw() helper
rather than open-coding it.
> : "=&r" (tmp0),
> "=&r" (tmp1),
The tmp0/tmp1 registers are technically a clobber, not
an in/out, though ideally these should be turned
into inputs.
> "+Q" (*((char __iomem *)addr))
> : "Q" (*((char *)buffer))
wrong length
> : "memory");
> }
The memory clobber is usually part of the barrier.
> static void qm_read128(void *buffer, const void __iomem *addr)
> {
> unsigned long tmp0 = 0, tmp1 = 0;
>
> asm volatile("ldp %0, %1, %3\n"
> "stp %0, %1, %2\n"
> "dmb oshst\n"
> : "=&r" (tmp0),
> "=&r" (tmp1),
> "+Q" (*((char *)buffer))
> : "Q" (*((char __iomem *)addr))
> : "memory");
> }
Same thing, but you are missing the control dependency
from __io_ar() here, rather than just open-coding it.
> #else
> static void qm_write128(void __iomem *addr, const void *buffer)
> {
>
> }
This is missing the entire implementation?
Arnd
