Re: [PATCH] ASoC: fsi: Stop using __raw_*() I/O accessors

Hi Arnd,

On Thu, Nov 19, 2020 at 4:54 PM Arnd Bergmann arnd@kernel.org wrote:

On Thu, Nov 19, 2020 at 1:53 PM Geert Uytterhoeven geert+renesas@glider.be wrote:

...

There is no reason to keep on using the __raw_{read,write}l() I/O accessors in Renesas ARM or SuperH driver code. Switch to using the plain {read,write}l() I/O accessors, to have a chance that this works on big-endian.

Suggested-by: Arnd Bergmann arnd@kernel.org Signed-off-by: Geert Uytterhoeven geert+renesas@glider.be

I don't think this one is correct, as based on

static void fsi_pio_push16(struct fsi_priv *fsi, u8 *_buf, int samples) { int i;

    if (fsi_is_enable_stream(fsi)) {
            /*
             * stream mode
             * see
             *      fsi_pio_push_init()
             */
            u32 *buf = (u32 *)_buf;

            for (i = 0; i < samples / 2; i++)
                    fsi_reg_write(fsi, DODT, buf[i]);
    } else {
            /* normal mode */
            u16 *buf = (u16 *)_buf;

            for (i = 0; i < samples; i++)
                    fsi_reg_write(fsi, DODT, ((u32)*(buf + i) << 8));
    }

}

the same helper is used for both accessing endianness-sensitive register values (which need the converrsion in writel()) and possibly in-memory byte streams that need the __raw_writel() behavior of copying the input bytes in sequence rather than sets of native-endian 16-bit or 32-bit samples.

Thanks, good catch!

...

---

Assembler output difference on SuperH checked.

I'm also not sure whether changing this breaks big-endian SuperH, which defines the accessors differently from Arm and most other architectures.

On SH, this driver is only used on SH7724 systems. Compiling an ecovec24_defconfig kernel with CONFIG_CPU_BIG_ENDIAN=y shows that the same code (native 32-bit access) is generated for big-endian as for little-endian, which means that it indeed must be broken for one of them. But this is not changed by my patch.

Maybe better just mark the driver as 'depends on SH || !CPU_BIG_ENDIAN' as it is clearly broken on big-endian Arm.

"depends on !CPU_BIG_ENDIAN"?

Gr{oetje,eeting}s,

Geert