| Message ID | 1455225798-9510-5-git-send-email-robert.jarzmik@free.fr (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
On Thu, Feb 11, 2016 at 10:23:18PM +0100, Robert Jarzmik wrote: > @@ -1399,13 +1405,17 @@ static int pxad_probe(struct platform_device *op) > return PTR_ERR(pdev->base); > > of_id = of_match_device(pxad_dt_ids, &op->dev); > - if (of_id) > + if (of_id) { > of_property_read_u32(op->dev.of_node, "#dma-channels", > &dma_channels); > - else if (pdata && pdata->dma_channels) > + of_property_read_u32(op->dev.of_node, "#requestors", > + &nb_requestors); I think we should check the return value here. This might be err in case when we have older DT on platform, but still should work with default in that case > + } else if (pdata && pdata->dma_channels) { > dma_channels = pdata->dma_channels; > - else > + nb_requestors = pdata->nb_requestors; > + } else { > dma_channels = 32; /* default 32 channel */ > + }
Vinod Koul <vinod.koul@intel.com> writes: > On Thu, Feb 11, 2016 at 10:23:18PM +0100, Robert Jarzmik wrote: >> @@ -1399,13 +1405,17 @@ static int pxad_probe(struct platform_device *op) >> return PTR_ERR(pdev->base); >> >> of_id = of_match_device(pxad_dt_ids, &op->dev); >> - if (of_id) >> + if (of_id) { >> of_property_read_u32(op->dev.of_node, "#dma-channels", >> &dma_channels); >> - else if (pdata && pdata->dma_channels) >> + of_property_read_u32(op->dev.of_node, "#requestors", >> + &nb_requestors); > > I think we should check the return value here. This might be err in case > when we have older DT on platform, but still should work with default in > that case Okay, but how should the code react to the err case, more specifically to -EINVAL or -ENODATA ? As this property is optional as per the device-tree description, the current code leaves nb_requestors = 0, as is specified in the description, and fits the mmp_pdma case. What do you think should be done ? A warning message ? Something else ? Cheers. -- Robert -- 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, Feb 15, 2016 at 06:24:57PM +0100, Robert Jarzmik wrote: > Vinod Koul <vinod.koul@intel.com> writes: > > > On Thu, Feb 11, 2016 at 10:23:18PM +0100, Robert Jarzmik wrote: > >> @@ -1399,13 +1405,17 @@ static int pxad_probe(struct platform_device *op) > >> return PTR_ERR(pdev->base); > >> > >> of_id = of_match_device(pxad_dt_ids, &op->dev); > >> - if (of_id) > >> + if (of_id) { > >> of_property_read_u32(op->dev.of_node, "#dma-channels", > >> &dma_channels); > >> - else if (pdata && pdata->dma_channels) > >> + of_property_read_u32(op->dev.of_node, "#requestors", > >> + &nb_requestors); > > > > I think we should check the return value here. This might be err in case > > when we have older DT on platform, but still should work with default in > > that case > > Okay, but how should the code react to the err case, more specifically to > -EINVAL or -ENODATA ? As this property is optional as per the device-tree > description, the current code leaves nb_requestors = 0, as is specified in the > description, and fits the mmp_pdma case. > > What do you think should be done ? A warning message ? Something else ? Message is fine, but in order for not to regress we should set this to 32 (IIRC default before this, right) and not zero.
Vinod Koul <vinod.koul@intel.com> writes: > On Mon, Feb 15, 2016 at 06:24:57PM +0100, Robert Jarzmik wrote: >> Vinod Koul <vinod.koul@intel.com> writes: >> >> > On Thu, Feb 11, 2016 at 10:23:18PM +0100, Robert Jarzmik wrote: >> >> @@ -1399,13 +1405,17 @@ static int pxad_probe(struct platform_device *op) >> >> return PTR_ERR(pdev->base); >> >> >> >> of_id = of_match_device(pxad_dt_ids, &op->dev); >> >> - if (of_id) >> >> + if (of_id) { >> >> of_property_read_u32(op->dev.of_node, "#dma-channels", >> >> &dma_channels); >> >> - else if (pdata && pdata->dma_channels) >> >> + of_property_read_u32(op->dev.of_node, "#requestors", >> >> + &nb_requestors); >> > >> > I think we should check the return value here. This might be err in case >> > when we have older DT on platform, but still should work with default in >> > that case >> >> Okay, but how should the code react to the err case, more specifically to >> -EINVAL or -ENODATA ? As this property is optional as per the device-tree >> description, the current code leaves nb_requestors = 0, as is specified in the >> description, and fits the mmp_pdma case. >> >> What do you think should be done ? A warning message ? Something else ? > > Message is fine, but in order for not to regress we should set this to 32 > (IIRC default before this, right) and not zero. Okay, got you. For v2 I'll implement exactly this, ie. a warning message and a default to 32. I'll amend the device-tree description also to match the 32 (and not the 0 I wrote earlier). Cheers.
diff --git a/drivers/dma/pxa_dma.c b/drivers/dma/pxa_dma.c index ca6c088edc8a..bc51d6271373 100644 --- a/drivers/dma/pxa_dma.c +++ b/drivers/dma/pxa_dma.c @@ -122,6 +122,7 @@ struct pxad_chan { struct pxad_device { struct dma_device slave; int nr_chans; + int nr_requestors; void __iomem *base; struct pxad_phy *phys; spinlock_t phy_lock; /* Phy association */ @@ -473,7 +474,7 @@ static void pxad_free_phy(struct pxad_chan *chan) return; /* clear the channel mapping in DRCMR */ - if (chan->drcmr <= DRCMR_CHLNUM) { + if (chan->drcmr <= pdev->nr_requestors) { reg = pxad_drcmr(chan->drcmr); writel_relaxed(0, chan->phy->base + reg); } @@ -509,6 +510,7 @@ static bool is_running_chan_misaligned(struct pxad_chan *chan) static void phy_enable(struct pxad_phy *phy, bool misaligned) { + struct pxad_device *pdev; u32 reg, dalgn; if (!phy->vchan) @@ -518,7 +520,8 @@ static void phy_enable(struct pxad_phy *phy, bool misaligned) "%s(); phy=%p(%d) misaligned=%d\n", __func__, phy, phy->idx, misaligned); - if (phy->vchan->drcmr <= DRCMR_CHLNUM) { + pdev = to_pxad_dev(phy->vchan->vc.chan.device); + if (phy->vchan->drcmr <= pdev->nr_requestors) { reg = pxad_drcmr(phy->vchan->drcmr); writel_relaxed(DRCMR_MAPVLD | phy->idx, phy->base + reg); } @@ -914,6 +917,7 @@ static void pxad_get_config(struct pxad_chan *chan, { u32 maxburst = 0, dev_addr = 0; enum dma_slave_buswidth width = DMA_SLAVE_BUSWIDTH_UNDEFINED; + struct pxad_device *pdev = to_pxad_dev(chan->vc.chan.device); *dcmd = 0; if (dir == DMA_DEV_TO_MEM) { @@ -922,7 +926,7 @@ static void pxad_get_config(struct pxad_chan *chan, dev_addr = chan->cfg.src_addr; *dev_src = dev_addr; *dcmd |= PXA_DCMD_INCTRGADDR; - if (chan->drcmr <= DRCMR_CHLNUM) + if (chan->drcmr <= pdev->nr_requestors) *dcmd |= PXA_DCMD_FLOWSRC; } if (dir == DMA_MEM_TO_DEV) { @@ -931,7 +935,7 @@ static void pxad_get_config(struct pxad_chan *chan, dev_addr = chan->cfg.dst_addr; *dev_dst = dev_addr; *dcmd |= PXA_DCMD_INCSRCADDR; - if (chan->drcmr <= DRCMR_CHLNUM) + if (chan->drcmr <= pdev->nr_requestors) *dcmd |= PXA_DCMD_FLOWTRG; } if (dir == DMA_MEM_TO_MEM) @@ -1341,13 +1345,15 @@ static struct dma_chan *pxad_dma_xlate(struct of_phandle_args *dma_spec, static int pxad_init_dmadev(struct platform_device *op, struct pxad_device *pdev, - unsigned int nr_phy_chans) + unsigned int nr_phy_chans, + unsigned int nr_requestors) { int ret; unsigned int i; struct pxad_chan *c; pdev->nr_chans = nr_phy_chans; + pdev->nr_requestors = nr_requestors; INIT_LIST_HEAD(&pdev->slave.channels); pdev->slave.device_alloc_chan_resources = pxad_alloc_chan_resources; pdev->slave.device_free_chan_resources = pxad_free_chan_resources; @@ -1382,7 +1388,7 @@ static int pxad_probe(struct platform_device *op) const struct of_device_id *of_id; struct mmp_dma_platdata *pdata = dev_get_platdata(&op->dev); struct resource *iores; - int ret, dma_channels = 0; + int ret, dma_channels = 0, nb_requestors = 0; const enum dma_slave_buswidth widths = DMA_SLAVE_BUSWIDTH_1_BYTE | DMA_SLAVE_BUSWIDTH_2_BYTES | DMA_SLAVE_BUSWIDTH_4_BYTES; @@ -1399,13 +1405,17 @@ static int pxad_probe(struct platform_device *op) return PTR_ERR(pdev->base); of_id = of_match_device(pxad_dt_ids, &op->dev); - if (of_id) + if (of_id) { of_property_read_u32(op->dev.of_node, "#dma-channels", &dma_channels); - else if (pdata && pdata->dma_channels) + of_property_read_u32(op->dev.of_node, "#requestors", + &nb_requestors); + } else if (pdata && pdata->dma_channels) { dma_channels = pdata->dma_channels; - else + nb_requestors = pdata->nb_requestors; + } else { dma_channels = 32; /* default 32 channel */ + } dma_cap_set(DMA_SLAVE, pdev->slave.cap_mask); dma_cap_set(DMA_MEMCPY, pdev->slave.cap_mask); @@ -1423,7 +1433,7 @@ static int pxad_probe(struct platform_device *op) pdev->slave.descriptor_reuse = true; pdev->slave.dev = &op->dev; - ret = pxad_init_dmadev(op, pdev, dma_channels); + ret = pxad_init_dmadev(op, pdev, dma_channels, nb_requestors); if (ret) { dev_err(pdev->slave.dev, "unable to register\n"); return ret; @@ -1442,7 +1452,8 @@ static int pxad_probe(struct platform_device *op) platform_set_drvdata(op, pdev); pxad_init_debugfs(pdev); - dev_info(pdev->slave.dev, "initialized %d channels\n", dma_channels); + dev_info(pdev->slave.dev, "initialized %d channels on %d requestors\n", + dma_channels, nb_requestors); return 0; }
The current number of requestor lines is limited to 31. This was an error of a previous commit, as this number is platform dependent, and is actually : - for pxa25x: 40 requestor lines - for pxa27x: 75 requestor lines - for pxa3xx: 100 requestor lines The previous testing did not reveal the faulty constant as on pxa[23]xx platforms, only camera, MSL and USB are above requestor 32, and in these only the camera has a driver using dma. Fixes: e87ffbdf0697 ("dmaengine: pxa_dma: fix the no-requestor case") Signed-off-by: Robert Jarzmik <robert.jarzmik@free.fr> --- drivers/dma/pxa_dma.c | 33 ++++++++++++++++++++++----------- 1 file changed, 22 insertions(+), 11 deletions(-)