| Message ID | 65c295f4b305330162795b0cc2508a207ed23a98.1459732726.git.horms+renesas@verge.net.au |
|---|---|
| State | New |
| Headers | show |
Hi, Commenting on this random patch in the series, but I'm guessing the same applies for others. First, I think it's somewhat silly to split this up into 31 patches instead of just doing one. But it's not bad enough that it really matters. My bigger concern is: On Sun, Apr 3, 2016 at 6:23 PM, Simon Horman <horms+renesas@verge.net.au> wrote: > diff --git a/arch/arm/boot/dts/r8a7794.dtsi b/arch/arm/boot/dts/r8a7794.dtsi > index eacb2b291361..4256557ca041 100644 > --- a/arch/arm/boot/dts/r8a7794.dtsi > +++ b/arch/arm/boot/dts/r8a7794.dtsi > @@ -296,8 +296,9 @@ > interrupts = <GIC_SPI 144 IRQ_TYPE_LEVEL_HIGH>; > clocks = <&mstp2_clks R8A7794_CLK_SCIFA0>; > clock-names = "fck"; > - dmas = <&dmac0 0x21>, <&dmac0 0x22>; > - dma-names = "tx", "rx"; > + dmas = <&dmac0 0x21>, <&dmac0 0x22>, > + <&dmac1 0x21>, <&dmac1 0x22>; > + dma-names = "tx", "rx", "tx", "rx"; > power-domains = <&cpg_clocks>; > status = "disabled"; > }; The names are no longer unique. So a get_<foo>_by_name() function no longer can work as expected. This should be fixed. -Olof
Hi Olof, On Wed, Apr 13, 2016 at 9:48 PM, Olof Johansson <olof@lixom.net> wrote: > On Sun, Apr 3, 2016 at 6:23 PM, Simon Horman <horms+renesas@verge.net.au> wrote: >> diff --git a/arch/arm/boot/dts/r8a7794.dtsi b/arch/arm/boot/dts/r8a7794.dtsi >> index eacb2b291361..4256557ca041 100644 >> --- a/arch/arm/boot/dts/r8a7794.dtsi >> +++ b/arch/arm/boot/dts/r8a7794.dtsi >> @@ -296,8 +296,9 @@ >> interrupts = <GIC_SPI 144 IRQ_TYPE_LEVEL_HIGH>; >> clocks = <&mstp2_clks R8A7794_CLK_SCIFA0>; >> clock-names = "fck"; >> - dmas = <&dmac0 0x21>, <&dmac0 0x22>; >> - dma-names = "tx", "rx"; >> + dmas = <&dmac0 0x21>, <&dmac0 0x22>, >> + <&dmac1 0x21>, <&dmac1 0x22>; >> + dma-names = "tx", "rx", "tx", "rx"; >> power-domains = <&cpg_clocks>; >> status = "disabled"; >> }; > > The names are no longer unique. So a get_<foo>_by_name() function no > longer can work as expected. That's intentional, and a relic of how dma_request_slave_channel_compat() works: if e.g. the first "tx" channel can't be gotten (e.g. because the referenced DMAC instance ran out of channels), it will try the next one. 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 Wed, Apr 13, 2016 at 09:55:14PM +0200, Geert Uytterhoeven wrote: > Hi Olof, > > On Wed, Apr 13, 2016 at 9:48 PM, Olof Johansson <olof@lixom.net> wrote: > > On Sun, Apr 3, 2016 at 6:23 PM, Simon Horman <horms+renesas@verge.net.au> wrote: > >> diff --git a/arch/arm/boot/dts/r8a7794.dtsi b/arch/arm/boot/dts/r8a7794.dtsi > >> index eacb2b291361..4256557ca041 100644 > >> --- a/arch/arm/boot/dts/r8a7794.dtsi > >> +++ b/arch/arm/boot/dts/r8a7794.dtsi > >> @@ -296,8 +296,9 @@ > >> interrupts = <GIC_SPI 144 IRQ_TYPE_LEVEL_HIGH>; > >> clocks = <&mstp2_clks R8A7794_CLK_SCIFA0>; > >> clock-names = "fck"; > >> - dmas = <&dmac0 0x21>, <&dmac0 0x22>; > >> - dma-names = "tx", "rx"; > >> + dmas = <&dmac0 0x21>, <&dmac0 0x22>, > >> + <&dmac1 0x21>, <&dmac1 0x22>; > >> + dma-names = "tx", "rx", "tx", "rx"; > >> power-domains = <&cpg_clocks>; > >> status = "disabled"; > >> }; > > > > The names are no longer unique. So a get_<foo>_by_name() function no > > longer can work as expected. > > That's intentional, and a relic of how dma_request_slave_channel_compat() > works: if e.g. the first "tx" channel can't be gotten (e.g. because > the referenced > DMAC instance ran out of channels), it will try the next one. Hi Olof, is the above acceptable to you?
On Mon, Apr 18, 2016 at 01:40:59PM +1000, Simon Horman wrote: > On Wed, Apr 13, 2016 at 09:55:14PM +0200, Geert Uytterhoeven wrote: > > Hi Olof, > > > > On Wed, Apr 13, 2016 at 9:48 PM, Olof Johansson <olof@lixom.net> wrote: > > > On Sun, Apr 3, 2016 at 6:23 PM, Simon Horman <horms+renesas@verge.net.au> wrote: > > >> diff --git a/arch/arm/boot/dts/r8a7794.dtsi b/arch/arm/boot/dts/r8a7794.dtsi > > >> index eacb2b291361..4256557ca041 100644 > > >> --- a/arch/arm/boot/dts/r8a7794.dtsi > > >> +++ b/arch/arm/boot/dts/r8a7794.dtsi > > >> @@ -296,8 +296,9 @@ > > >> interrupts = <GIC_SPI 144 IRQ_TYPE_LEVEL_HIGH>; > > >> clocks = <&mstp2_clks R8A7794_CLK_SCIFA0>; > > >> clock-names = "fck"; > > >> - dmas = <&dmac0 0x21>, <&dmac0 0x22>; > > >> - dma-names = "tx", "rx"; > > >> + dmas = <&dmac0 0x21>, <&dmac0 0x22>, > > >> + <&dmac1 0x21>, <&dmac1 0x22>; > > >> + dma-names = "tx", "rx", "tx", "rx"; > > >> power-domains = <&cpg_clocks>; > > >> status = "disabled"; > > >> }; > > > > > > The names are no longer unique. So a get_<foo>_by_name() function no > > > longer can work as expected. > > > > That's intentional, and a relic of how dma_request_slave_channel_compat() > > works: if e.g. the first "tx" channel can't be gotten (e.g. because > > the referenced > > DMAC instance ran out of channels), it will try the next one. > > Hi Olof, > > is the above acceptable to you? In order to allow the other patches in this series, and others queued up since, to progress independently of a resolution to this question I have dropped this and all similar patches for now.
On 2016-04-18 13:40:59 +1000, Simon Horman wrote: > On Wed, Apr 13, 2016 at 09:55:14PM +0200, Geert Uytterhoeven wrote: > > Hi Olof, > > > > On Wed, Apr 13, 2016 at 9:48 PM, Olof Johansson <olof@lixom.net> wrote: > > > On Sun, Apr 3, 2016 at 6:23 PM, Simon Horman <horms+renesas@verge.net.au> wrote: > > >> diff --git a/arch/arm/boot/dts/r8a7794.dtsi b/arch/arm/boot/dts/r8a7794.dtsi > > >> index eacb2b291361..4256557ca041 100644 > > >> --- a/arch/arm/boot/dts/r8a7794.dtsi > > >> +++ b/arch/arm/boot/dts/r8a7794.dtsi > > >> @@ -296,8 +296,9 @@ > > >> interrupts = <GIC_SPI 144 IRQ_TYPE_LEVEL_HIGH>; > > >> clocks = <&mstp2_clks R8A7794_CLK_SCIFA0>; > > >> clock-names = "fck"; > > >> - dmas = <&dmac0 0x21>, <&dmac0 0x22>; > > >> - dma-names = "tx", "rx"; > > >> + dmas = <&dmac0 0x21>, <&dmac0 0x22>, > > >> + <&dmac1 0x21>, <&dmac1 0x22>; > > >> + dma-names = "tx", "rx", "tx", "rx"; > > >> power-domains = <&cpg_clocks>; > > >> status = "disabled"; > > >> }; > > > > > > The names are no longer unique. So a get_<foo>_by_name() function no > > > longer can work as expected. > > > > That's intentional, and a relic of how dma_request_slave_channel_compat() > > works: if e.g. the first "tx" channel can't be gotten (e.g. because > > the referenced > > DMAC instance ran out of channels), it will try the next one. > > Hi Olof, > > is the above acceptable to you? Hi Olof, ping.
Hi Arnd and Olof, It seems Arnd have been dropped from this thread and I would appreciate your feedback on this. Do you think it is a good idea to reference both DMA controllers in this fashion to be able to get channels from either of the two DMA controllers? Also to both of you if this is a good solution would you like me to squash the patches per SoC and resend it? Olof pointed out it might be a bit silly to do this in so many patches. On 2016-04-13 21:55:14 +0200, Geert Uytterhoeven wrote: > Hi Olof, > > On Wed, Apr 13, 2016 at 9:48 PM, Olof Johansson <olof@lixom.net> wrote: > > On Sun, Apr 3, 2016 at 6:23 PM, Simon Horman <horms+renesas@verge.net.au> wrote: > >> diff --git a/arch/arm/boot/dts/r8a7794.dtsi b/arch/arm/boot/dts/r8a7794.dtsi > >> index eacb2b291361..4256557ca041 100644 > >> --- a/arch/arm/boot/dts/r8a7794.dtsi > >> +++ b/arch/arm/boot/dts/r8a7794.dtsi > >> @@ -296,8 +296,9 @@ > >> interrupts = <GIC_SPI 144 IRQ_TYPE_LEVEL_HIGH>; > >> clocks = <&mstp2_clks R8A7794_CLK_SCIFA0>; > >> clock-names = "fck"; > >> - dmas = <&dmac0 0x21>, <&dmac0 0x22>; > >> - dma-names = "tx", "rx"; > >> + dmas = <&dmac0 0x21>, <&dmac0 0x22>, > >> + <&dmac1 0x21>, <&dmac1 0x22>; > >> + dma-names = "tx", "rx", "tx", "rx"; > >> power-domains = <&cpg_clocks>; > >> status = "disabled"; > >> }; > > > > The names are no longer unique. So a get_<foo>_by_name() function no > > longer can work as expected. > > That's intentional, and a relic of how dma_request_slave_channel_compat() > works: if e.g. the first "tx" channel can't be gotten (e.g. because > the referenced > DMAC instance ran out of channels), it will try the next one.
diff --git a/arch/arm/boot/dts/r8a7794.dtsi b/arch/arm/boot/dts/r8a7794.dtsi index eacb2b291361..4256557ca041 100644 --- a/arch/arm/boot/dts/r8a7794.dtsi +++ b/arch/arm/boot/dts/r8a7794.dtsi @@ -296,8 +296,9 @@ interrupts = <GIC_SPI 144 IRQ_TYPE_LEVEL_HIGH>; clocks = <&mstp2_clks R8A7794_CLK_SCIFA0>; clock-names = "fck"; - dmas = <&dmac0 0x21>, <&dmac0 0x22>; - dma-names = "tx", "rx"; + dmas = <&dmac0 0x21>, <&dmac0 0x22>, + <&dmac1 0x21>, <&dmac1 0x22>; + dma-names = "tx", "rx", "tx", "rx"; power-domains = <&cpg_clocks>; status = "disabled"; }; @@ -309,8 +310,9 @@ interrupts = <GIC_SPI 145 IRQ_TYPE_LEVEL_HIGH>; clocks = <&mstp2_clks R8A7794_CLK_SCIFA1>; clock-names = "fck"; - dmas = <&dmac0 0x25>, <&dmac0 0x26>; - dma-names = "tx", "rx"; + dmas = <&dmac0 0x25>, <&dmac0 0x26>, + <&dmac1 0x25>, <&dmac1 0x26>; + dma-names = "tx", "rx", "tx", "rx"; power-domains = <&cpg_clocks>; status = "disabled"; }; @@ -322,8 +324,9 @@ interrupts = <GIC_SPI 151 IRQ_TYPE_LEVEL_HIGH>; clocks = <&mstp2_clks R8A7794_CLK_SCIFA2>; clock-names = "fck"; - dmas = <&dmac0 0x27>, <&dmac0 0x28>; - dma-names = "tx", "rx"; + dmas = <&dmac0 0x27>, <&dmac0 0x28>, + <&dmac1 0x27>, <&dmac1 0x28>; + dma-names = "tx", "rx", "tx", "rx"; power-domains = <&cpg_clocks>; status = "disabled"; }; @@ -335,8 +338,9 @@ interrupts = <GIC_SPI 29 IRQ_TYPE_LEVEL_HIGH>; clocks = <&mstp11_clks R8A7794_CLK_SCIFA3>; clock-names = "fck"; - dmas = <&dmac0 0x1b>, <&dmac0 0x1c>; - dma-names = "tx", "rx"; + dmas = <&dmac0 0x1b>, <&dmac0 0x1c>, + <&dmac1 0x1b>, <&dmac1 0x1c>; + dma-names = "tx", "rx", "tx", "rx"; power-domains = <&cpg_clocks>; status = "disabled"; }; @@ -348,8 +352,9 @@ interrupts = <GIC_SPI 30 IRQ_TYPE_LEVEL_HIGH>; clocks = <&mstp11_clks R8A7794_CLK_SCIFA4>; clock-names = "fck"; - dmas = <&dmac0 0x1f>, <&dmac0 0x20>; - dma-names = "tx", "rx"; + dmas = <&dmac0 0x1f>, <&dmac0 0x20>, + <&dmac1 0x1f>, <&dmac1 0x20>; + dma-names = "tx", "rx", "tx", "rx"; power-domains = <&cpg_clocks>; status = "disabled"; }; @@ -361,8 +366,9 @@ interrupts = <GIC_SPI 31 IRQ_TYPE_LEVEL_HIGH>; clocks = <&mstp11_clks R8A7794_CLK_SCIFA5>; clock-names = "fck"; - dmas = <&dmac0 0x23>, <&dmac0 0x24>; - dma-names = "tx", "rx"; + dmas = <&dmac0 0x23>, <&dmac0 0x24>, + <&dmac1 0x23>, <&dmac1 0x24>; + dma-names = "tx", "rx", "tx", "rx"; power-domains = <&cpg_clocks>; status = "disabled"; };