On 2023/8/22 4:47, Arnd Bergmann wrote:
> 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.
Ok, I will add generic IO helpers ioread128/iowrite128 for this,
keep it consistent with ioread32/iowrite32, submit patchset later.
And remove them from the driver.
>
>> 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.
The input parameter data has been endian-swap.
>
>> "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.
OK.
>
>> : "=&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.
Yeah, the memory can be removed.
>
>> 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?
If the interface is implemented in the driver, the driver runs only on the ARM64 platform.
Therefore, there is no need to implement.
>
> Arnd
> .
>
Thanks,
Weili
