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

  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>
---
 arch/sh/drivers/dma/dma-sh.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)
  

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

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?

Adrian
  

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@artur-rojek.eu>
>> ---
>>  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.

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

Cheers,
Artur

> 
> Adrian
  

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@artur-rojek.eu>
> > > ---
> > >  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
  

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
  

Hi Geert!

On Mon, 2023-05-08 at 13:20 +0200, Geert Uytterhoeven wrote:
> 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.

Was SH7751R broken by 7f47c7189b3e8f19 as well?

Adrian
  

Hi Adrian,

On Mon, May 8, 2023 at 1:28 PM John Paul Adrian Glaubitz
<glaubitz@physik.fu-berlin.de> wrote:
> On Mon, 2023-05-08 at 13:20 +0200, Geert Uytterhoeven wrote:
> > 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.
>
> Was SH7751R broken by 7f47c7189b3e8f19 as well?

I think so.
Before, the code to use 1 or 2 DMA engine relied on the presence of
DMAE1_IRQ, which is/was defined in arch/sh/include/cpu-sh4a/cpu/dma.h,
but not in arch/sh/include/cpu-sh4/cpu/dma.h.

It might be sufficient to fix this by just dropping the SH_DMAC_BASE1
definition from arch/sh/include/cpu-sh4/cpu/dma.h.  I'm actually
wondering why it was added (in commit 71b973a42c545682 ("sh: dma-sh
updates for multi IRQ and new SH-4A CPUs.")), because it looks like
none of the SH4-based (not SH4A!) SoCs have a second base...

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
  

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>
> ---
>  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)
>  {

Reviewed-by: John Paul Adrian Glaubitz <glaubitz@physik.fu-berlin.de>
  

On Sun, 2023-05-07 at 12:32 +0200, John Paul Adrian Glaubitz wrote:
> > 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.

I have successfully booted a current kernel with both patches applied on my
SH7785LCR board and will let it run for a few days to make sure it runs stable
and then apply the two patches to my sh-linux tree.

Thanks,
Adrian
  

On 2023-05-08 13:20, Geert Uytterhoeven wrote:
> 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.

Yikes!
If this series hasn't been merged yet, perhaps we could fix this issue
in v2. I have something like this in mind (untested):
```
diff --git a/arch/sh/drivers/dma/dma-sh.c b/arch/sh/drivers/dma/dma-sh.c
index 14c18ebda400..306fba1564e5 100644
--- a/arch/sh/drivers/dma/dma-sh.c
+++ b/arch/sh/drivers/dma/dma-sh.c
@@ -18,6 +18,18 @@
  #include <cpu/dma-register.h>
  #include <cpu/dma.h>

+/*
+ * Some of the SoCs feature two DMAC modules. In such a case, the 
channels are
+ * distributed equally among them.
+ */
+#ifdef SH_DMAC_BASE1
+#define        SH_DMAC_NR_MD_CH        (CONFIG_NR_ONCHIP_DMA_CHANNELS / 
2)
+#else
+#define        SH_DMAC_NR_MD_CH        CONFIG_NR_ONCHIP_DMA_CHANNELS
+#endif
+
+#define        SH_DMAC_CH_SZ           0x10
+
  /*
   * Define the default configuration for dual address memory-memory 
transfer.
   * The 0x400 value represents auto-request, external->external.
@@ -29,7 +41,7 @@ static unsigned long dma_find_base(unsigned int chan)
         unsigned long base = SH_DMAC_BASE0;

  #ifdef SH_DMAC_BASE1
-       if (chan >= 6)
+       if (chan >= SH_DMAC_NR_MD_CH)
                 base = SH_DMAC_BASE1;
  #endif

@@ -40,13 +52,13 @@ static unsigned long dma_base_addr(unsigned int 
chan)
  {
         unsigned long base = dma_find_base(chan);

-       /* Normalize offset calculation */
-       if (chan >= 9)
-               chan -= 6;
-       if (chan >= 4)
-               base += 0x10;
+       chan = (chan % SH_DMAC_NR_MD_CH) * SH_DMAC_CH_SZ;
+
+       /* DMAOR is placed inside the channel register space. Step over 
it. */
+       if (chan >= DMAOR)
+               base += SH_DMAC_CH_SZ;

-       return base + (chan * 0x10);
+       return base + chan;
  }

  #ifdef CONFIG_SH_DMA_IRQ_MULTI
@@ -250,15 +262,11 @@ static int sh_dmac_get_dma_residue(struct 
dma_channel *chan)
  #define NR_DMAOR       1
  #endif

-/*
- * 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) + \
-                                                   DMAOR)
+#define dmaor_read_reg(n)              __raw_readw(dma_find_base((n) * 
\
+                                                   SH_DMAC_NR_MD_CH) + 
DMAOR)
  #define dmaor_write_reg(n, data)       __raw_writew(data, \
-                                                    dma_find_base((n) * 
6) + \
-                                                    DMAOR)
+                                                    dma_find_base((n) * 
\
+                                                    SH_DMAC_NR_MD_CH) + 
DMAOR)

  static inline int dmaor_reset(int no)
  {

```
Otherwise, I'll send it in separately. Of course we'll also need to fix
`SH_DMAC_BASE1` so that it's set only for SoCs that feature two DMAC
modules...

Cheers,
Artur

> Anyway, that's not new, so I have no objection to your patch.
> 
> Gr{oetje,eeting}s,
> 
>                         Geert
  

Hi Artur!

On Sat, 2023-05-13 at 13:41 +0200, Artur Rojek wrote:
> Yikes!
> If this series hasn't been merged yet, perhaps we could fix this issue
> in v2. I have something like this in mind (untested):
> (...)
> Otherwise, I'll send it in separately. Of course we'll also need to fix
> `SH_DMAC_BASE1` so that it's set only for SoCs that feature two DMAC
> modules...

No worries, nothing has been merged yet. For one, the merge windows for 6.4
has been closed and I also haven't merged your patches into my tree yet. Please
take your time to spin up a v2 of your patch set and test them properly.

Maybe you're also interested in the clean-up that Geert suggested in this
thread (ordering of the CPU subtypes and capitalization issues)?

Also, can you write "processor manual" instead of "PM" in the other patch
as well as don't use backticks for the macro names? In fact, I would suggest
retitling the subject to:

	sh: dma: Fix dmaor_read_reg() and dmaor_write_reg() macros

Oh, and I will retest your v2 patches before merging them, of course ;-).

Thanks,
Adrian
  

On 2023-05-13 16:45, John Paul Adrian Glaubitz wrote:
> Hi Artur!
> 
> On Sat, 2023-05-13 at 13:41 +0200, Artur Rojek wrote:
>> Yikes!
>> If this series hasn't been merged yet, perhaps we could fix this issue
>> in v2. I have something like this in mind (untested):
>> (...)
>> Otherwise, I'll send it in separately. Of course we'll also need to 
>> fix
>> `SH_DMAC_BASE1` so that it's set only for SoCs that feature two DMAC
>> modules...
> 
> No worries, nothing has been merged yet. For one, the merge windows for 
> 6.4
> has been closed and I also haven't merged your patches into my tree 
> yet. Please
> take your time to spin up a v2 of your patch set and test them 
> properly.

Great!

> 
> Maybe you're also interested in the clean-up that Geert suggested in 
> this
> thread (ordering of the CPU subtypes and capitalization issues)?

Sure, why not - the more clean-up we do, the better :)

> 
> Also, can you write "processor manual" instead of "PM" in the other 
> patch
> as well as don't use backticks for the macro names? In fact, I would 
> suggest
> retitling the subject to:
> 
> 	sh: dma: Fix dmaor_read_reg() and dmaor_write_reg() macros
> 

Of course.
On a side note, it was supposed to be "programming manual", however I
now see that Renesas named that document as "hardware manual", so that's
what I'll put into the commit description, if you don't mind.

cheers,
Artur

> Oh, and I will retest your v2 patches before merging them, of course 
> ;-).
> 
> Thanks,
> Adrian
  

Hi Artur!

On Sat, 2023-05-13 at 16:57 +0200, Artur Rojek wrote:
> > Maybe you're also interested in the clean-up that Geert suggested in 
> > this
> > thread (ordering of the CPU subtypes and capitalization issues)?
> 
> Sure, why not - the more clean-up we do, the better :)

Great, thanks a lot!

> > Also, can you write "processor manual" instead of "PM" in the other 
> > patch
> > as well as don't use backticks for the macro names? In fact, I would 
> > suggest
> > retitling the subject to:
> > 
> > 	sh: dma: Fix dmaor_read_reg() and dmaor_write_reg() macros
> > 
> 
> Of course.
> On a side note, it was supposed to be "programming manual", however I
> now see that Renesas named that document as "hardware manual", so that's
> what I'll put into the commit description, if you don't mind.

Absolutely not! Looking forward to your v2 series and please take your time!

Adrian