Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20230506141703.65605-1-contact@artur-rojek.eu>
 <20230506141703.65605-2-contact@artur-rojek.eu> <65f873585db0cd9f79a84eb48707413775a9ba5b.camel@physik.fu-berlin.de>
 <2f73b2ac1ec15a6b0f78d8d3a7f12266@artur-rojek.eu>
In-Reply-To: <2f73b2ac1ec15a6b0f78d8d3a7f12266@artur-rojek.eu>
From:   Geert Uytterhoeven <geert@linux-m68k.org>
Date:   Mon, 8 May 2023 13:20:46 +0200
Message-ID: <CAMuHMdXFFaRqPxvUqgJCtZG1B5gpULL-N4VpNPyPF=_+mtn7Dg@mail.gmail.com>
Subject: Re: [PATCH 1/2] sh: dma: fix `dmaor_read_reg`/`dmaor_write_reg` macros
To:     Artur Rojek <contact@artur-rojek.eu>
Cc:     John Paul Adrian Glaubitz <glaubitz@physik.fu-berlin.de>,
        Yoshinori Sato <ysato@users.sourceforge.jp>,
        Rich Felker <dalias@libc.org>,
        Rafael Ignacio Zurita <rafaelignacio.zurita@gmail.com>,
        linux-sh@vger.kernel.org, linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 8BIT
Precedence: bulk

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