| Message ID | 20230506141703.65605-2-contact@artur-rojek.eu (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | SH7709 DMA fixes | expand |
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
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) {
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(-)