Messages in this thread | | | From | Geert Uytterhoeven <> | Date | Mon, 8 May 2023 13:20:46 +0200 | Subject | Re: [PATCH 1/2] sh: dma: fix `dmaor_read_reg`/`dmaor_write_reg` macros |
| |
Hi Artur,
On Sun, May 7, 2023 at 11:43 AM Artur Rojek <contact@artur-rojek.eu> wrote: > On 2023-05-07 10:39, John Paul Adrian Glaubitz wrote: > > On Sat, 2023-05-06 at 16:17 +0200, Artur Rojek wrote: > >> Squash two bugs introduced into said macros in 7f47c7189b3e, > >> preventing > >> them from proper operation: > >> 1) Add DMAOR register offset into the address of the hw reg access, > >> 2) Correct a nasty typo in the DMAOR base calculation for > >> `dmaor_write_reg`. > >> > >> Signed-off-by: Artur Rojek <contact@artur-rojek.eu>\
Thanks for your patch!
> >> --- a/arch/sh/drivers/dma/dma-sh.c > >> +++ b/arch/sh/drivers/dma/dma-sh.c > >> @@ -254,8 +254,11 @@ static int sh_dmac_get_dma_residue(struct > >> dma_channel *chan) > >> * DMAOR bases are broken out amongst channel groups. DMAOR0 manages > >> * channels 0 - 5, DMAOR1 6 - 11 (optional). > >> */ > >> -#define dmaor_read_reg(n) __raw_readw(dma_find_base((n)*6)) > >> -#define dmaor_write_reg(n, data) __raw_writew(data, > >> dma_find_base(n)*6) > >> +#define dmaor_read_reg(n) __raw_readw(dma_find_base((n) * 6) + \ > >> + DMAOR) > >> +#define dmaor_write_reg(n, data) __raw_writew(data, \ > >> + dma_find_base((n) * 6) + \ > >> + DMAOR)
Fixes: 7f47c7189b3e8f19 ("sh: dma: More legacy cpu dma chainsawing.") Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
> >> static inline int dmaor_reset(int no) > >> { > > > > I have looked through the changes and the code and I agree that there > > is a typo > > in dmaor_write_regn() that needs to be fixed and that the DMAOR offset > > is missing > > although I don't understand why that didn't break the kernel on other > > SuperH systems > > such as my SH-7785LCR evaluation board or the LANDISK board which Geert > > uses. > > I also wondered that. On SH7709 it's a hard panic, it should be the same > elsewhere.
Landisk does not seem to use DMA. I did have CONFIG_SH_DMA=n, but enabling it does not make any difference.
> > What I also don't understand is the factor 6 the DMA channel number is > > multiplied > > with. When looking at the definition of dma_find_base(), it seems that > > every channel > > equal to 6 or higher will return SH_DMAC_BASE1 as DMA base address. > > But if we multiply > > the parameter with 6, this will apply to every n > 0. Is that correct? > > As confusing as they look, those macros take dmaor index (a number in > range 0 <= n < NR_DMAOR) as parameter, then multiply it by 6 to convert > it to a format compatible with `dma_find_base` (which expects a channel > index). In practice `n` will be either 0 or 1, so dma_find_base(0 * 6) > will return SH_DMAC_BASE0, while dma_find_base(1 * 6) SH_DMAC_BASE1.
Looks like this is still broken on e.g. SH7751R, which has 8 channels, both handled by a single DMAOR register at offset 0x40...
While e.g. dma_base_addr() seems to have some provision for this (cfr. the "chan >= 9" (not "8") check), dma_find_base() will fail, as arch/sh/include/cpu-sh4/cpu/dma.h defines SH_DMAC_BASE1. Anyway, that's not new, so I have no objection to your patch.
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
| |