| Message ID | 1459710079-8525-1-git-send-email-linus.walleij@linaro.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
Hi Linus, On 3 April 2016 at 21:01, 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 v2->v3: > - Drop the patch and variable for fixed signal assignment. > - Always assign a fixed channel number when registering > the channels. > ChangeLog v1->v2: > - Rely on the number of signals for the variant coming in > from vendor data. > > Joachim: I hope this works with LPC, I assign the signal number > now. Did a quick test and it works now. Thanks! Tested-by: Joachim Eastwood <manabian@gmail.com> Couple of comments below. > drivers/dma/amba-pl08x.c | 85 +++++++++++++++++++++++++++++++--------------- > include/linux/amba/pl08x.h | 2 +- > 2 files changed, 58 insertions(+), 29 deletions(-) > > static int pl08x_of_probe(struct amba_device *adev, > @@ -2091,9 +2094,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) > @@ -2195,6 +2200,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); devm_kcalloc() would be a better fit here I think. > + if (!chanp) > + return -ENOMEM; Nit: new line after return. > + 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, > @@ -2234,6 +2259,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; > @@ -2284,10 +2313,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; > @@ -2438,6 +2463,7 @@ out_no_pl08x: > static struct vendor_data vendor_pl080 = { > .config_offset = PL080_CH_CONFIG, > .channels = 8, > + .signals = 16, > .dualmaster = true, > .max_transfer_size = PL080_CONTROL_TRANSFER_SIZE_MASK, > }; > @@ -2445,6 +2471,7 @@ static struct vendor_data vendor_pl080 = { > static struct vendor_data vendor_nomadik = { > .config_offset = PL080_CH_CONFIG, > .channels = 8, > + .signals = 32, > .dualmaster = true, > .nomadik = true, > .max_transfer_size = PL080_CONTROL_TRANSFER_SIZE_MASK, > @@ -2453,6 +2480,7 @@ static struct vendor_data vendor_nomadik = { > static struct vendor_data vendor_pl080s = { > .config_offset = PL080S_CH_CONFIG, > .channels = 8, > + .signals = 32, > .pl080s = true, > .max_transfer_size = PL080S_CONTROL_TRANSFER_SIZE_MASK, > }; > @@ -2460,6 +2488,7 @@ static struct vendor_data vendor_pl080s = { > static struct vendor_data vendor_pl081 = { > .config_offset = PL080_CH_CONFIG, > .channels = 2, > + .signals = 16, > .dualmaster = false, > .max_transfer_size = PL080_CONTROL_TRANSFER_SIZE_MASK, > }; I see you added a signals member to struct vendor_data. You could also retrieve this information from the standard 'dma-requests' DT property. See: Documentation/devicetree/bindings/dma/dma.txt Note that I think your approach is fine too and may be better in this case. Guess we could always implement an override from DT if ever needed. regards, Joachim Eastwood -- 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
Hi Linus, On Sun, Apr 03, 2016 at 09:01:19PM +0200, 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 v2->v3: > - Drop the patch and variable for fixed signal assignment. > - Always assign a fixed channel number when registering > the channels. Your patch looks good, and I just tested it and it also works :-) Perfect! Thanks, 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
diff --git a/drivers/dma/amba-pl08x.c b/drivers/dma/amba-pl08x.c index 9b42c0588550..fafec2d27c72 100644 --- a/drivers/dma/amba-pl08x.c +++ b/drivers/dma/amba-pl08x.c @@ -107,16 +107,20 @@ struct pl08x_driver_data; /** * struct vendor_data - vendor-specific config parameters for PL08x derivatives * @channels: the number of channels available in this variant + * @signals: the number of request signals available from the hardware * @dualmaster: whether this version supports dual AHB masters or not. * @nomadik: whether the channels have Nomadik security extension bits * that need to be checked for permission before use and some registers are * missing * @pl080s: whether this version is a PL080S, which has separate register and * LLI word for transfer size. + * @max_transfer_size: the maximum single element transfer size for this + * PL08x variant. */ struct vendor_data { u8 config_offset; u8 channels; + u8 signals; bool dualmaster; bool nomadik; bool pl080s; @@ -235,7 +239,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; @@ -1909,6 +1913,12 @@ static int pl08x_dma_init_virtual_channels(struct pl08x_driver_data *pl08x, if (slave) { chan->cd = &pl08x->pd->slave_channels[i]; + /* + * Some implementations have muxed signals, whereas some + * use a mux in front of the signals and need dynamic + * assignment of signals. + */ + chan->signal = i; pl08x_dma_slave_init(chan); } else { chan->cd = &pl08x->pd->memcpy_channel; @@ -2050,40 +2060,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, @@ -2091,9 +2094,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) @@ -2195,6 +2200,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, @@ -2234,6 +2259,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; @@ -2284,10 +2313,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; @@ -2438,6 +2463,7 @@ out_no_pl08x: static struct vendor_data vendor_pl080 = { .config_offset = PL080_CH_CONFIG, .channels = 8, + .signals = 16, .dualmaster = true, .max_transfer_size = PL080_CONTROL_TRANSFER_SIZE_MASK, }; @@ -2445,6 +2471,7 @@ static struct vendor_data vendor_pl080 = { static struct vendor_data vendor_nomadik = { .config_offset = PL080_CH_CONFIG, .channels = 8, + .signals = 32, .dualmaster = true, .nomadik = true, .max_transfer_size = PL080_CONTROL_TRANSFER_SIZE_MASK, @@ -2453,6 +2480,7 @@ static struct vendor_data vendor_nomadik = { static struct vendor_data vendor_pl080s = { .config_offset = PL080S_CH_CONFIG, .channels = 8, + .signals = 32, .pl080s = true, .max_transfer_size = PL080S_CONTROL_TRANSFER_SIZE_MASK, }; @@ -2460,6 +2488,7 @@ static struct vendor_data vendor_pl080s = { static struct vendor_data vendor_pl081 = { .config_offset = PL080_CH_CONFIG, .channels = 2, + .signals = 16, .dualmaster = false, .max_transfer_size = PL080_CONTROL_TRANSFER_SIZE_MASK, }; 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 v2->v3: - Drop the patch and variable for fixed signal assignment. - Always assign a fixed channel number when registering the channels. ChangeLog v1->v2: - Rely on the number of signals for the variant coming in from vendor data. Joachim: I hope this works with LPC, I assign the signal number now. --- drivers/dma/amba-pl08x.c | 85 +++++++++++++++++++++++++++++++--------------- include/linux/amba/pl08x.h | 2 +- 2 files changed, 58 insertions(+), 29 deletions(-)