| Message ID | 1458949866-4726-1-git-send-email-linus.walleij@linaro.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
Hi Linus, On 26 March 2016 at 00:51, Linus Walleij <linus.walleij@linaro.org> wrote: > The current OF translation of channels can never work with > any DMA client using the DMA channels directly: the only way > to get the channels initialized properly is in the > dma_async_device_register() call, where chan->dev etc is > allocated and initialized. > > Allocate and initialize all possible DMA channels and > only augment a target channel with the periph_buses at > of_xlate(). Remove some const settings to make things work. > > Cc: Maxime Ripard <maxime.ripard@free-electrons.com> > Cc: Joachim Eastwood <manabian@gmail.com> > Cc: Johannes Stezenbach <js@sig21.net> > Signed-off-by: Linus Walleij <linus.walleij@linaro.org> > --- > ChangeLog v1->v2: > - Rely on the number of signals for the variant coming in > from vendor data. > > Joachim: Can you please help to try to figure out if this > still crashes with LPC, it must be something with the mux > layer thing. This is doing the right thing and is needed > for atleast 2 platforms to use this driver. First of all; sorry about the screw up with the attempt to do dynamic channel allocation in the xlate. As Linus noted in [1] the dmaengine framework doesn't support this as channels are not initialized properly and it only seemed to work properly because of the mux. Sorry about all this! Note the LPC18xx didn't really crash with this patch. It was only unable to find the dma channels [2]. When the initial patches was posted I didn't have the time to debug it properly and I have since forgotten about it. The reason for this, as discovered by Johannes [3], is that chan->signal is not properly setup. This variable is set to -1 in pl08x_dma_init_virtual_channels() and when pl08x_find_chan_id() is called from xlate it doesn't find any channels because the chan->id is still -1 (I have verified this with debug prints). Setting chan->id make the channel assignment work on LPC18xx again. Linus: Could you send an updated version that sets the chan->id variable in pl08x_dma_init_virtual_channels()? Maybe it would make sense to reorder the patches since this patch is a fix. Thanks to both Linus and Johannes for figuring out the issues and fixing it. Sorry about the mess from my side! regards, Joachim Eastwood [1] http://www.spinics.net/lists/arm-kernel/msg492763.html [2] http://slexy.org/raw/s20nJ4Gpyx [3] http://www.spinics.net/lists/arm-kernel/msg492285.html -- To unsubscribe from this list: send the line "unsubscribe dmaengine" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sat, Mar 26, 2016 at 12:51:06AM +0100, Linus Walleij wrote: > The current OF translation of channels can never work with > any DMA client using the DMA channels directly: the only way > to get the channels initialized properly is in the > dma_async_device_register() call, where chan->dev etc is > allocated and initialized. > > Allocate and initialize all possible DMA channels and > only augment a target channel with the periph_buses at > of_xlate(). Remove some const settings to make things work. > > Cc: Maxime Ripard <maxime.ripard@free-electrons.com> > Cc: Joachim Eastwood <manabian@gmail.com> > Cc: Johannes Stezenbach <js@sig21.net> > Signed-off-by: Linus Walleij <linus.walleij@linaro.org> > --- > ChangeLog v1->v2: > - Rely on the number of signals for the variant coming in > from vendor data. This patch looks good to me, I'd just fold the previous patch into it with the changes I had recommended. But either way, thanks for taking care of it! Johannes -- To unsubscribe from this list: send the line "unsubscribe dmaengine" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Mar 28, 2016 at 7:32 PM, Joachim Eastwood <manabian@gmail.com> wrote: > On 26 March 2016 at 00:51, Linus Walleij <linus.walleij@linaro.org> wrote: >> Joachim: Can you please help to try to figure out if this >> still crashes with LPC, it must be something with the mux >> layer thing. This is doing the right thing and is needed >> for atleast 2 platforms to use this driver. > > First of all; sorry about the screw up with the attempt to do dynamic > channel allocation in the xlate. As Linus noted in [1] the dmaengine > framework doesn't support this as channels are not initialized > properly and it only seemed to work properly because of the mux. Sorry > about all this! Hey we all make mistakes. More often than not, it's actually me. > Note the LPC18xx didn't really crash with this patch. It was only > unable to find the dma channels [2]. When the initial patches was > posted I didn't have the time to debug it properly and I have since > forgotten about it. > > The reason for this, as discovered by Johannes [3], is that > chan->signal is not properly setup. This variable is set to -1 in > pl08x_dma_init_virtual_channels() and when pl08x_find_chan_id() is > called from xlate it doesn't find any channels because the chan->id is > still -1 (I have verified this with debug prints). Setting chan->id > make the channel assignment work on LPC18xx again. OK I think what you mean is that chan->signal must be assigned by default so the channel can me found? This should be fixed with the v3 patch I just sent out. I hope. > Linus: Could you send an updated version that sets the chan->id > variable in pl08x_dma_init_virtual_channels()? chan->signal you mean? Enjoy! Else comment inline in the patch exactly what I need to do. > Maybe it would make sense to reorder the patches since this patch is a fix. Nah I just dropped the first one as Johannes suggested. It's down to one now. Tested on the Nomadik, works like a charm. Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe dmaengine" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/dma/amba-pl08x.c b/drivers/dma/amba-pl08x.c index 2ef80200d110..2252e48da1f1 100644 --- a/drivers/dma/amba-pl08x.c +++ b/drivers/dma/amba-pl08x.c @@ -243,7 +243,7 @@ struct pl08x_dma_chan { struct virt_dma_chan vc; struct pl08x_phy_chan *phychan; const char *name; - const struct pl08x_channel_data *cd; + struct pl08x_channel_data *cd; struct dma_slave_config cfg; struct pl08x_txd *at; struct pl08x_driver_data *host; @@ -2082,40 +2082,33 @@ static struct dma_chan *pl08x_of_xlate(struct of_phandle_args *dma_spec, struct of_dma *ofdma) { struct pl08x_driver_data *pl08x = ofdma->of_dma_data; - struct pl08x_channel_data *data; - struct pl08x_dma_chan *chan; struct dma_chan *dma_chan; + struct pl08x_dma_chan *plchan; if (!pl08x) return NULL; - if (dma_spec->args_count != 2) + if (dma_spec->args_count != 2) { + dev_err(&pl08x->adev->dev, + "DMA channel translation requires two cells\n"); return NULL; + } dma_chan = pl08x_find_chan_id(pl08x, dma_spec->args[0]); - if (dma_chan) - return dma_get_slave_channel(dma_chan); - - chan = devm_kzalloc(pl08x->slave.dev, sizeof(*chan) + sizeof(*data), - GFP_KERNEL); - if (!chan) + if (!dma_chan) { + dev_err(&pl08x->adev->dev, + "DMA slave channel not found\n"); return NULL; + } - data = (void *)&chan[1]; - data->bus_id = "(none)"; - data->periph_buses = dma_spec->args[1]; - - chan->cd = data; - chan->host = pl08x; - chan->slave = true; - chan->name = data->bus_id; - chan->state = PL08X_CHAN_IDLE; - chan->signal = dma_spec->args[0]; - chan->vc.desc_free = pl08x_desc_free; - - vchan_init(&chan->vc, &pl08x->slave); + plchan = to_pl08x_chan(dma_chan); + dev_dbg(&pl08x->adev->dev, + "translated channel for signal %d\n", + dma_spec->args[0]); - return dma_get_slave_channel(&chan->vc.chan); + /* Augment channel data for applicable AHB buses */ + plchan->cd->periph_buses = dma_spec->args[1]; + return dma_get_slave_channel(dma_chan); } static int pl08x_of_probe(struct amba_device *adev, @@ -2123,9 +2116,11 @@ static int pl08x_of_probe(struct amba_device *adev, struct device_node *np) { struct pl08x_platform_data *pd; + struct pl08x_channel_data *chanp = NULL; u32 cctl_memcpy = 0; u32 val; int ret; + int i; pd = devm_kzalloc(&adev->dev, sizeof(*pd), GFP_KERNEL); if (!pd) @@ -2227,6 +2222,26 @@ static int pl08x_of_probe(struct amba_device *adev, /* Use the buses that can access memory, obviously */ pd->memcpy_channel.periph_buses = pd->mem_buses; + /* + * Allocate channel data for all possible slave channels (one + * for each possible signal), channels will then be allocated + * for a device and have it's AHB interfaces set up at + * translation time. + */ + chanp = devm_kzalloc(&adev->dev, + pl08x->vd->signals * + sizeof(struct pl08x_channel_data), + GFP_KERNEL); + if (!chanp) + return -ENOMEM; + pd->slave_channels = chanp; + for (i = 0; i < pl08x->vd->signals; i++) { + /* chanp->periph_buses will be assigned at translation */ + chanp->bus_id = kasprintf(GFP_KERNEL, "slave%d", i); + chanp++; + } + pd->num_slave_channels = pl08x->vd->signals; + pl08x->pd = pd; return of_dma_controller_register(adev->dev.of_node, pl08x_of_xlate, @@ -2266,6 +2281,10 @@ static int pl08x_probe(struct amba_device *adev, const struct amba_id *id) goto out_no_pl08x; } + /* Assign useful pointers to the driver state */ + pl08x->adev = adev; + pl08x->vd = vd; + /* Initialize memcpy engine */ dma_cap_set(DMA_MEMCPY, pl08x->memcpy.cap_mask); pl08x->memcpy.dev = &adev->dev; @@ -2316,10 +2335,6 @@ static int pl08x_probe(struct amba_device *adev, const struct amba_id *id) } } - /* Assign useful pointers to the driver state */ - pl08x->adev = adev; - pl08x->vd = vd; - /* By default, AHB1 only. If dualmaster, from platform */ pl08x->lli_buses = PL08X_AHB1; pl08x->mem_buses = PL08X_AHB1; diff --git a/include/linux/amba/pl08x.h b/include/linux/amba/pl08x.h index 10fe2a211c2e..27e9ec8778eb 100644 --- a/include/linux/amba/pl08x.h +++ b/include/linux/amba/pl08x.h @@ -86,7 +86,7 @@ struct pl08x_channel_data { * @mem_buses: buses which memory can be accessed from: PL08X_AHB1 | PL08X_AHB2 */ struct pl08x_platform_data { - const struct pl08x_channel_data *slave_channels; + struct pl08x_channel_data *slave_channels; unsigned int num_slave_channels; struct pl08x_channel_data memcpy_channel; int (*get_xfer_signal)(const struct pl08x_channel_data *);
The current OF translation of channels can never work with any DMA client using the DMA channels directly: the only way to get the channels initialized properly is in the dma_async_device_register() call, where chan->dev etc is allocated and initialized. Allocate and initialize all possible DMA channels and only augment a target channel with the periph_buses at of_xlate(). Remove some const settings to make things work. Cc: Maxime Ripard <maxime.ripard@free-electrons.com> Cc: Joachim Eastwood <manabian@gmail.com> Cc: Johannes Stezenbach <js@sig21.net> Signed-off-by: Linus Walleij <linus.walleij@linaro.org> --- ChangeLog v1->v2: - Rely on the number of signals for the variant coming in from vendor data. Joachim: Can you please help to try to figure out if this still crashes with LPC, it must be something with the mux layer thing. This is doing the right thing and is needed for atleast 2 platforms to use this driver. --- drivers/dma/amba-pl08x.c | 71 ++++++++++++++++++++++++++++------------------ include/linux/amba/pl08x.h | 2 +- 2 files changed, 44 insertions(+), 29 deletions(-)