Re: [PATCH 1/2] sh: dma: fix `dmaor_read_reg`/`dmaor_write_reg` macros

[Geert Uytterhoeven]

Hi Artur,

On Sun, May 7, 2023 at 11:43 AM Artur Rojek <contact@xxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxx>\

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@xxxxxxxxx>

> >>  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@xxxxxxxxxxxxxx

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