| Message ID | 20201119125318.4066097-1-geert+renesas@glider.be (mailing list archive) |
|---|---|
| State | New |
| Delegated to: | Kieran Bingham |
| Headers | show |
| Series | ASoC: fsi: Stop using __raw_*() I/O accessors | expand |
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
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
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
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
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
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)
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(-)