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

Hi Arnd,

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

On Thu, Nov 19, 2020 at 5:13 PM Geert Uytterhoeven geert@linux-m68k.org wrote:

...

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:

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.

Not necessarily: I think superh is more like the old 'BE32' variant of Arm big-endian, in that on-chip registers are accessed in CPU-endian byte order, while access to external RAM is byte-swapped.

That's indeed quite likely: according to the SH7724 docs, the endianness of "the system" is configured by an external pin at power-on reset time, and cannot be changed dynamically. Hence testing this would require a big-endian boot loader, too.

...

...

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"?

I think I'd just leave it as it is. Unless someone wants to try out this

OK.

board in both big-endian and little-endian configurations and also listen to the audio output, it's impossible to know whether it is actually broken. sound/soc/sh/dma-sh7760.c does have a comment from 2007 saying "// FIXME: little-endian only for now".

SH7760 does not use the FSI driver. A few SH defconfig files have CONFIG_CPU_BIG_ENDIAN=y, but the later SH4A parts all seem to be used in little-endian mode.

Gr{oetje,eeting}s,

Geert