| Message ID | 20130619174047.GB1403@sirena.org.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wednesday 19 of June 2013 18:40:47 Mark Brown wrote: > On Sun, Jun 16, 2013 at 10:54:07PM +0200, Tomasz Figa wrote: > > One of the biggest roadblocks on the way of S3C64xx to DeviceTree > > support is its DMA driver, which is completely platform-specific and > > provides private API (s3c-dma), not even saying that its design is > > completely against multiplatform-awareness. > > I tried to test this on my s3c64xx based system but it gave me a kernel > that didn't boot far enough to give console output (there's some early > init stuff that uses SPI...). That said, I needed: > > diff --git a/drivers/dma/amba-pl08x.c b/drivers/dma/amba-pl08x.c > index 210a893..0f49707 100644 > --- a/drivers/dma/amba-pl08x.c > +++ b/drivers/dma/amba-pl08x.c > @@ -313,7 +313,7 @@ static int pl08x_request_mux(struct pl08x_dma_chan > *plchan) int ret; > > if (plchan->mux_use++ == 0 && pd->get_signal) { > - ret = pd->get_signal(plchan->cd); > + ret = (pd->get_signal)(plchan->cd); Hmm, that's strange. The former is a completely valid piece of code... > if (ret < 0) { > plchan->mux_use = 0; > return ret; > > to get it to build which makes me suspect the compiler a bit as well... > the system has audio, SPI and MMC enabled. > > I was applying this to -next, are there any other dependencies I need or > anything? Hmm, I've been testing this on top of my common clock framework and device tree patches, but I don't think this had any effect. Did you add necessary clkdev lookups to the clock driver? In Samsung CCF alias notation it looks like this: + ALIAS(HCLK_DMA1, "dma-pl080s.1", "apb_pclk"), + ALIAS(HCLK_DMA0, "dma-pl080s.0", "apb_pclk"), Not sure how hard it will be to add such lookups to the old clock driver, though. I will test this applied directly on top of current linux-next when I find some time, but for now you might check out my v3.11-devel branch on my github: https://github.com/tom3q/linux.git Best regards, Tomasz
On Wednesday 19 June 2013, Tomasz Figa wrote: > On Wednesday 19 of June 2013 18:40:47 Mark Brown wrote: > > On Sun, Jun 16, 2013 at 10:54:07PM +0200, Tomasz Figa wrote: > > > One of the biggest roadblocks on the way of S3C64xx to DeviceTree > > > support is its DMA driver, which is completely platform-specific and > > > provides private API (s3c-dma), not even saying that its design is > > > completely against multiplatform-awareness. > > > > I tried to test this on my s3c64xx based system but it gave me a kernel > > that didn't boot far enough to give console output (there's some early > > init stuff that uses SPI...). That said, I needed: > > > > diff --git a/drivers/dma/amba-pl08x.c b/drivers/dma/amba-pl08x.c > > index 210a893..0f49707 100644 > > --- a/drivers/dma/amba-pl08x.c > > +++ b/drivers/dma/amba-pl08x.c > > @@ -313,7 +313,7 @@ static int pl08x_request_mux(struct pl08x_dma_chan > > *plchan) int ret; > > > > if (plchan->mux_use++ == 0 && pd->get_signal) { > > - ret = pd->get_signal(plchan->cd); > > + ret = (pd->get_signal)(plchan->cd); > > Hmm, that's strange. The former is a completely valid piece of code... get_signal is a macro defined in include/linux/signal.h. If that header gets included, neither of the two is valid. Arnd
On Wed, Jun 19, 2013 at 08:26:12PM +0200, Tomasz Figa wrote: > On Wednesday 19 of June 2013 18:40:47 Mark Brown wrote: > > - ret = pd->get_signal(plchan->cd); > > + ret = (pd->get_signal)(plchan->cd); > Hmm, that's strange. The former is a completely valid piece of code... I know, hence... > > to get it to build which makes me suspect the compiler a bit as well... ...my comment about suspecting the compiler. > > I was applying this to -next, are there any other dependencies I need or > > anything? > Hmm, I've been testing this on top of my common clock framework and device > tree patches, but I don't think this had any effect. Did you add necessary > clkdev lookups to the clock driver? No, I didn't - that's most likely it, I didn't really investigate. I didn't test the watchdog stuff as the clocks didn't get sent to me. > In Samsung CCF alias notation it looks like this: > + ALIAS(HCLK_DMA1, "dma-pl080s.1", "apb_pclk"), > + ALIAS(HCLK_DMA0, "dma-pl080s.0", "apb_pclk"), > Not sure how hard it will be to add such lookups to the old clock driver, > though. It's pretty much the same providing you know which clock needs to be used. > I will test this applied directly on top of current linux-next when I find > some time, but for now you might check out my v3.11-devel branch on my > github: > https://github.com/tom3q/linux.git Will try to get round to it.
On Wed, Jun 19, 2013 at 09:01:33PM +0200, Arnd Bergmann wrote: > On Wednesday 19 June 2013, Tomasz Figa wrote: > > > if (plchan->mux_use++ == 0 && pd->get_signal) { > > > - ret = pd->get_signal(plchan->cd); > > > + ret = (pd->get_signal)(plchan->cd); > > Hmm, that's strange. The former is a completely valid piece of code... > get_signal is a macro defined in include/linux/signal.h. If that header > gets included, neither of the two is valid. Ah, that'll be it. I'll post a patch to rename...
On Wednesday 19 of June 2013 20:22:11 Mark Brown wrote: > On Wed, Jun 19, 2013 at 08:26:12PM +0200, Tomasz Figa wrote: > > On Wednesday 19 of June 2013 18:40:47 Mark Brown wrote: > > > - ret = pd->get_signal(plchan->cd); > > > + ret = (pd->get_signal)(plchan->cd); > > > > Hmm, that's strange. The former is a completely valid piece of code... > > I know, hence... > > > > to get it to build which makes me suspect the compiler a bit as > > > well... > > ...my comment about suspecting the compiler. > > > > I was applying this to -next, are there any other dependencies I > > > need or anything? > > > > Hmm, I've been testing this on top of my common clock framework and > > device tree patches, but I don't think this had any effect. Did you > > add necessary clkdev lookups to the clock driver? > > No, I didn't - that's most likely it, I didn't really investigate. I > didn't test the watchdog stuff as the clocks didn't get sent to me. I always try to keep you on Cc of my patches for s3c64xx, as you are the most active user of this platform (if not the only one other than me) and this was the case for clock patches as well, just checked that. Seems like I forgot to add you to watchdog patches, sorry. But you didn't miss anything, since they were rather trivial ones. > > In Samsung CCF alias notation it looks like this: > > > > + ALIAS(HCLK_DMA1, "dma-pl080s.1", "apb_pclk"), > > + ALIAS(HCLK_DMA0, "dma-pl080s.0", "apb_pclk"), > > > > Not sure how hard it will be to add such lookups to the old clock > > driver, though. > > It's pretty much the same providing you know which clock needs to be > used. > > > I will test this applied directly on top of current linux-next when I > > find some time, but for now you might check out my v3.11-devel branch > > on my github: > > > > https://github.com/tom3q/linux.git > > Will try to get round to it. OK. Best regards, Tomasz
On Wed, Jun 19, 2013 at 09:32:44PM +0200, Tomasz Figa wrote: > On Wednesday 19 of June 2013 20:22:11 Mark Brown wrote: > > No, I didn't - that's most likely it, I didn't really investigate. I > > didn't test the watchdog stuff as the clocks didn't get sent to me. > I always try to keep you on Cc of my patches for s3c64xx, as you are the > most active user of this platform (if not the only one other than me) and > this was the case for clock patches as well, just checked that. Yes, I had the clock patches but they depended on something or other I think. I did test a previous verion IIRC. There are other users I'm aware of (you can probably take a wild guess where from the board) though they tend not to be terribly active upstream.
(Apologies if this is mangled, fighting both Outlook and remote desktop :-( ) linux-kernel-owner@vger.kernel.org wrote on Behalf Of Mark Brown > On Wed, Jun 19, 2013 at 08:26:12PM +0200, Tomasz Figa wrote: > > On Wednesday 19 of June 2013 18:40:47 Mark Brown wrote: > > > > - ret = pd->get_signal(plchan->cd); > > > + ret = (pd->get_signal)(plchan->cd); > > > Hmm, that's strange. The former is a completely valid piece of > code... > > I know, hence... > > > > to get it to build which makes me suspect the compiler a bit as > well... > > ...my comment about suspecting the compiler. Can you just make that minimal change, and diff the objdump of the two .o's? It would be worth a bug-report against the toolchain if different code was being generated. If objdump spews huge numbers of diffs (due to one address changing and pushing everything else out of kilter), then feel free to forward both .o's or both objdumps to me, and I can run a script over them, which knows to ignore unimportant address changes. Praying that this mail is readable to you, as it's not readable as I write it, Phil
On Thu, Jun 20, 2013 at 12:24:47PM +0300, Phil Carmody wrote: > Can you just make that minimal change, and diff the objdump of the two .o's? > It would be worth a bug-report against the toolchain if different code was > being generated. If objdump spews huge numbers of diffs (due to one address > changing and pushing everything else out of kilter), then feel free to > forward both .o's or both objdumps to me, and I can run a script over them, > which knows to ignore unimportant address changes. See Arnd's followup - this looks like a collision with the get_signal macro in signal.h.
> -----Original Message----- > On Thu, Jun 20, 2013 at 12:24:47PM +0300, Phil Carmody wrote: > > Can you just make that minimal change, and diff the objdump of the > two .o's? > > It would be worth a bug-report against the toolchain if different > code > > was being generated. If objdump spews huge numbers of diffs (due to > > one address changing and pushing everything else out of kilter), then > > feel free to forward both .o's or both objdumps to me, and I can run > a > > script over them, which knows to ignore unimportant address changes. > > See Arnd's followup - this looks like a collision with the get_signal > macro in signal.h. With my language-lawyer hat on, I'd suggest ``(get_signal)'' to prevent the macro expansion: /tmp$ cat crap.c #define fnlikemacro(foo) foo+ int x(int y) { int (fnlikemacro) = y; return fnlikemacro(y)(fnlikemacro); } /tmp$ gcc -E crap.c int x(int y) { int (fnlikemacro) = y; return y+(fnlikemacro); } (and yes, that compiles.) However, it's more tempting (i.e. sensible) to just rename the one with the weaker claim to the name. Phil
On Thu, Jun 20, 2013 at 02:14:53PM +0300, Phil Carmody wrote: > With my language-lawyer hat on, I'd suggest ``(get_signal)'' to prevent the > macro expansion: Right, which the patch I posted in the mail was doing IIRC. > However, it's more tempting (i.e. sensible) to just rename the > one with the weaker claim to the name. I sent a patch for that which Vinod has applied.
diff --git a/drivers/dma/amba-pl08x.c b/drivers/dma/amba-pl08x.c index 210a893..0f49707 100644 --- a/drivers/dma/amba-pl08x.c +++ b/drivers/dma/amba-pl08x.c @@ -313,7 +313,7 @@ static int pl08x_request_mux(struct pl08x_dma_chan *plchan) int ret; if (plchan->mux_use++ == 0 && pd->get_signal) { - ret = pd->get_signal(plchan->cd); + ret = (pd->get_signal)(plchan->cd); if (ret < 0) { plchan->mux_use = 0; return ret;