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

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

[Geert Uytterhoeven]

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>
---
Assembler output difference on SuperH checked.
---
 sound/soc/sh/fsi.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/sound/soc/sh/fsi.c b/sound/soc/sh/fsi.c
index 3c574792231bc5c3..518d4b0c4b8b99fa 100644
--- a/sound/soc/sh/fsi.c
+++ b/sound/soc/sh/fsi.c
@@ -313,12 +313,12 @@ static void __fsi_reg_write(u32 __iomem *reg, u32 data)
 	/* valid data area is 24bit */
 	data &= 0x00ffffff;
 
-	__raw_writel(data, reg);
+	writel(data, reg);
 }
 
 static u32 __fsi_reg_read(u32 __iomem *reg)
 {
-	return __raw_readl(reg);
+	return readl(reg);
 }
 
 static void __fsi_reg_mask_set(u32 __iomem *reg, u32 mask, u32 data)
-- 
2.25.1

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

[Geert Uytterhoeven]

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>
---
Assembler output difference on SuperH checked.
---
 sound/soc/sh/fsi.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/sound/soc/sh/fsi.c b/sound/soc/sh/fsi.c
index 3c574792231bc5c3..518d4b0c4b8b99fa 100644
--- a/sound/soc/sh/fsi.c
+++ b/sound/soc/sh/fsi.c
@@ -313,12 +313,12 @@ static void __fsi_reg_write(u32 __iomem *reg, u32 data)
 	/* valid data area is 24bit */
 	data &= 0x00ffffff;
 
-	__raw_writel(data, reg);
+	writel(data, reg);
 }
 
 static u32 __fsi_reg_read(u32 __iomem *reg)
 {
-	return __raw_readl(reg);
+	return readl(reg);
 }
 
 static void __fsi_reg_mask_set(u32 __iomem *reg, u32 mask, u32 data)
-- 
2.25.1

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

[Arnd Bergmann]

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.

> ---
> 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.

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

     Arnd

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

[Arnd Bergmann]

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.

> ---
> 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.

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

     Arnd

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

[Geert Uytterhoeven]

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

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

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

[Geert Uytterhoeven]

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

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

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

[Arnd Bergmann]

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.

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

      Arnd

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

[Arnd Bergmann]

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.

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

      Arnd

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

[Geert Uytterhoeven]

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

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

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

[Geert Uytterhoeven]

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

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

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

[Mark Brown]

On Thu, 19 Nov 2020 13:53:18 +0100, Geert Uytterhoeven 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.

Applied to

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next

Thanks!

[1/1] ASoC: fsi: Stop using __raw_*() I/O accessors
      (no commit info)

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark