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

[John Paul Adrian Glaubitz]

On Sun, 2023-05-07 at 11:34 +0200, Artur Rojek wrote:
> Hi Adrian,
> 
> 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>
> > > ---
> > >  arch/sh/drivers/dma/dma-sh.c | 7 +++++--
> > >  1 file changed, 5 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/arch/sh/drivers/dma/dma-sh.c 
> > > b/arch/sh/drivers/dma/dma-sh.c
> > > index 96c626c2cd0a..14c18ebda400 100644
> > > --- 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)
> > > 
> > >  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.

I will give the patch a spin on my SH-7785LCR and see if it breaks anything.

Maybe Geert can test it on his LANDISK board as well as Rob on the J2 Turtleboard,
just to be safe.

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

OK, thanks for the clarification. Let's wait what Geert has to say on this
next week when he is back online.

Adrian

-- 
 .''`.  John Paul Adrian Glaubitz
: :' :  Debian Developer
`. `'   Physicist
  `-    GPG: 62FF 8A75 84E0 2956 9546  0006 7426 3B37 F5B5 F913