| Message ID | 20231003042215.142678-2-shravan.chippa@microchip.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | dma: sf-pdma: various sf-pdma updates for the mpfs platform | expand |
shravan chippa wrote: > From: Shravan Chippa <shravan.chippa@microchip.com> > > Update sf-pdma driver to adopt generic DMA device tree bindings. > It calls of_dma_controller_register() with sf-pdma specific > of_dma_xlate to get the generic DMA device tree helper support > and the DMA clients can look up the sf-pdma controller using > standard APIs. > > Signed-off-by: Shravan Chippa <shravan.chippa@microchip.com> > --- > drivers/dma/sf-pdma/sf-pdma.c | 44 +++++++++++++++++++++++++++++++++++ > 1 file changed, 44 insertions(+) > > diff --git a/drivers/dma/sf-pdma/sf-pdma.c b/drivers/dma/sf-pdma/sf-pdma.c > index d1c6956af452..06a0912a12a1 100644 > --- a/drivers/dma/sf-pdma/sf-pdma.c > +++ b/drivers/dma/sf-pdma/sf-pdma.c > @@ -20,6 +20,7 @@ > #include <linux/mod_devicetable.h> > #include <linux/dma-mapping.h> > #include <linux/of.h> > +#include <linux/of_dma.h> > #include <linux/slab.h> > > #include "sf-pdma.h" > @@ -490,6 +491,33 @@ static void sf_pdma_setup_chans(struct sf_pdma *pdma) > } > } > > +static struct dma_chan *sf_pdma_of_xlate(struct of_phandle_args *dma_spec, > + struct of_dma *ofdma) > +{ > + struct sf_pdma *pdma = ofdma->of_dma_data; > + struct device *dev = pdma->dma_dev.dev; > + struct sf_pdma_chan *chan; If you're respinning this series anyway, you have two spaces here. > + struct dma_chan *c; > + u32 channel_id; > + > + if (dma_spec->args_count != 1) { > + dev_err(dev, "Bad number of cells\n"); > + return NULL; > + } > + > + channel_id = dma_spec->args[0]; > + > + chan = &pdma->chans[channel_id]; > + > + c = dma_get_slave_channel(&chan->vchan.chan); > + if (!c) { > + dev_err(dev, "No more channels available\n"); > + return NULL; > + } > + > + return c; > +} > + > static int sf_pdma_probe(struct platform_device *pdev) > { > struct sf_pdma *pdma; > @@ -563,7 +591,20 @@ static int sf_pdma_probe(struct platform_device *pdev) > return ret; > } > > + ret = of_dma_controller_register(pdev->dev.of_node, > + sf_pdma_of_xlate, pdma); > + if (ret < 0) { > + dev_err(&pdev->dev, > + "Can't register SiFive Platform OF_DMA. (%d)\n", ret); > + goto err_unregister; > + } > + > return 0; > + > +err_unregister: > + dma_async_device_unregister(&pdma->dma_dev); > + > + return ret; > } > > static int sf_pdma_remove(struct platform_device *pdev) > @@ -583,6 +624,9 @@ static int sf_pdma_remove(struct platform_device *pdev) > tasklet_kill(&ch->err_tasklet); > } > > + if (pdev->dev.of_node) > + of_dma_controller_free(pdev->dev.of_node); > + > dma_async_device_unregister(&pdma->dma_dev); > > return 0; > -- > 2.34.1 > > > _______________________________________________ > linux-riscv mailing list > linux-riscv@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-riscv
Hi, On 2023-10-02 11:22 PM, shravan chippa wrote: > From: Shravan Chippa <shravan.chippa@microchip.com> > > Update sf-pdma driver to adopt generic DMA device tree bindings. > It calls of_dma_controller_register() with sf-pdma specific > of_dma_xlate to get the generic DMA device tree helper support > and the DMA clients can look up the sf-pdma controller using > standard APIs. > > Signed-off-by: Shravan Chippa <shravan.chippa@microchip.com> > --- > drivers/dma/sf-pdma/sf-pdma.c | 44 +++++++++++++++++++++++++++++++++++ > 1 file changed, 44 insertions(+) > > diff --git a/drivers/dma/sf-pdma/sf-pdma.c b/drivers/dma/sf-pdma/sf-pdma.c > index d1c6956af452..06a0912a12a1 100644 > --- a/drivers/dma/sf-pdma/sf-pdma.c > +++ b/drivers/dma/sf-pdma/sf-pdma.c > @@ -20,6 +20,7 @@ > #include <linux/mod_devicetable.h> > #include <linux/dma-mapping.h> > #include <linux/of.h> > +#include <linux/of_dma.h> > #include <linux/slab.h> > > #include "sf-pdma.h" > @@ -490,6 +491,33 @@ static void sf_pdma_setup_chans(struct sf_pdma *pdma) > } > } > > +static struct dma_chan *sf_pdma_of_xlate(struct of_phandle_args *dma_spec, > + struct of_dma *ofdma) > +{ > + struct sf_pdma *pdma = ofdma->of_dma_data; > + struct device *dev = pdma->dma_dev.dev; > + struct sf_pdma_chan *chan; > + struct dma_chan *c; > + u32 channel_id; > + > + if (dma_spec->args_count != 1) { > + dev_err(dev, "Bad number of cells\n"); > + return NULL; > + } > + > + channel_id = dma_spec->args[0]; > + > + chan = &pdma->chans[channel_id]; > + > + c = dma_get_slave_channel(&chan->vchan.chan); This does not look right to me. All of the channels in the controller are identical and support arbitrary addresses, so there is no need to use a specific physical channel. And unless Microchip has added something on top, the only way to trigger a transfer is through the MMIO interface, so there is no request ID to differentiate virtual channels either. So it seems to me that #dma-cells should really be 0, and this function should just call dma_get_any_slave_channel(). Regards, Samuel > + if (!c) { > + dev_err(dev, "No more channels available\n"); > + return NULL; > + } > + > + return c; > +} > + > static int sf_pdma_probe(struct platform_device *pdev) > { > struct sf_pdma *pdma; > @@ -563,7 +591,20 @@ static int sf_pdma_probe(struct platform_device *pdev) > return ret; > } > > + ret = of_dma_controller_register(pdev->dev.of_node, > + sf_pdma_of_xlate, pdma); > + if (ret < 0) { > + dev_err(&pdev->dev, > + "Can't register SiFive Platform OF_DMA. (%d)\n", ret); > + goto err_unregister; > + } > + > return 0; > + > +err_unregister: > + dma_async_device_unregister(&pdma->dma_dev); > + > + return ret; > } > > static int sf_pdma_remove(struct platform_device *pdev) > @@ -583,6 +624,9 @@ static int sf_pdma_remove(struct platform_device *pdev) > tasklet_kill(&ch->err_tasklet); > } > > + if (pdev->dev.of_node) > + of_dma_controller_free(pdev->dev.of_node); > + > dma_async_device_unregister(&pdma->dma_dev); > > return 0;
Hi, > -----Original Message----- > From: Samuel Holland <samuel.holland@sifive.com> > Sent: Wednesday, October 18, 2023 12:03 AM > To: shravan Chippa - I35088 <Shravan.Chippa@microchip.com>; > green.wan@sifive.com; vkoul@kernel.org; robh+dt@kernel.org; > krzysztof.kozlowski+dt@linaro.org; palmer@dabbelt.com; > paul.walmsley@sifive.com; conor+dt@kernel.org > Cc: dmaengine@vger.kernel.org; devicetree@vger.kernel.org; linux- > riscv@lists.infradead.org; linux-kernel@vger.kernel.org; Nagasuresh Relli - > I67208 <Nagasuresh.Relli@microchip.com>; Praveen Kumar - I30718 > <Praveen.Kumar@microchip.com> > Subject: Re: [PATCH v2 1/4] dmaengine: sf-pdma: Support > of_dma_controller_register() > > [You don't often get email from samuel.holland@sifive.com. Learn why this is > important at https://aka.ms/LearnAboutSenderIdentification ] > > EXTERNAL EMAIL: Do not click links or open attachments unless you know the > content is safe > > Hi, > > On 2023-10-02 11:22 PM, shravan chippa wrote: > > From: Shravan Chippa <shravan.chippa@microchip.com> > > > > Update sf-pdma driver to adopt generic DMA device tree bindings. > > It calls of_dma_controller_register() with sf-pdma specific > > of_dma_xlate to get the generic DMA device tree helper support and the > > DMA clients can look up the sf-pdma controller using standard APIs. > > > > Signed-off-by: Shravan Chippa <shravan.chippa@microchip.com> > > --- > > drivers/dma/sf-pdma/sf-pdma.c | 44 > > +++++++++++++++++++++++++++++++++++ > > 1 file changed, 44 insertions(+) > > > > diff --git a/drivers/dma/sf-pdma/sf-pdma.c > > b/drivers/dma/sf-pdma/sf-pdma.c index d1c6956af452..06a0912a12a1 > > 100644 > > --- a/drivers/dma/sf-pdma/sf-pdma.c > > +++ b/drivers/dma/sf-pdma/sf-pdma.c > > @@ -20,6 +20,7 @@ > > #include <linux/mod_devicetable.h> > > #include <linux/dma-mapping.h> > > #include <linux/of.h> > > +#include <linux/of_dma.h> > > #include <linux/slab.h> > > > > #include "sf-pdma.h" > > @@ -490,6 +491,33 @@ static void sf_pdma_setup_chans(struct sf_pdma > *pdma) > > } > > } > > > > +static struct dma_chan *sf_pdma_of_xlate(struct of_phandle_args > *dma_spec, > > + struct of_dma *ofdma) { > > + struct sf_pdma *pdma = ofdma->of_dma_data; > > + struct device *dev = pdma->dma_dev.dev; > > + struct sf_pdma_chan *chan; > > + struct dma_chan *c; > > + u32 channel_id; > > + > > + if (dma_spec->args_count != 1) { > > + dev_err(dev, "Bad number of cells\n"); > > + return NULL; > > + } > > + > > + channel_id = dma_spec->args[0]; > > + > > + chan = &pdma->chans[channel_id]; > > + > > + c = dma_get_slave_channel(&chan->vchan.chan); > > This does not look right to me. All of the channels in the controller are identical > and support arbitrary addresses, so there is no need to use a specific physical > channel. And unless Microchip has added something on top, the only way to > trigger a transfer is through the MMIO interface, so there is no request ID to > differentiate virtual channels either. > > So it seems to me that #dma-cells should really be 0, and this function should > just call dma_get_any_slave_channel(). > Thanks for your comment, yes all the channels are identical. I have tested with dma_get_any_slave_channel() it is not working. dma_get_any_slave_channel() function searching for the DMA channel which has "DMA_SLAVE" capabilities. But sf-pdma has only "DMA_MEMCPY" capabilities. ***************************** struct dma_chan *dma_get_any_slave_channel(struct dma_device *device) { dma_cap_zero(mask); dma_cap_set(DMA_SLAVE, mask); chan = find_candidate(device, &mask, NULL, NULL); } ****************************** So "dma_request_chan()" function throws an error like "sf-pdma 3000000.dma-controller: No more channels available" Thanks, Shravan. > Regards, > Samuel > > > + if (!c) { > > + dev_err(dev, "No more channels available\n"); > > + return NULL; > > + } > > + > > + return c; > > +} > > + > > static int sf_pdma_probe(struct platform_device *pdev) { > > struct sf_pdma *pdma; > > @@ -563,7 +591,20 @@ static int sf_pdma_probe(struct platform_device > *pdev) > > return ret; > > } > > > > + ret = of_dma_controller_register(pdev->dev.of_node, > > + sf_pdma_of_xlate, pdma); > > + if (ret < 0) { > > + dev_err(&pdev->dev, > > + "Can't register SiFive Platform OF_DMA. (%d)\n", ret); > > + goto err_unregister; > > + } > > + > > return 0; > > + > > +err_unregister: > > + dma_async_device_unregister(&pdma->dma_dev); > > + > > + return ret; > > } > > > > static int sf_pdma_remove(struct platform_device *pdev) @@ -583,6 > > +624,9 @@ static int sf_pdma_remove(struct platform_device *pdev) > > tasklet_kill(&ch->err_tasklet); > > } > > > > + if (pdev->dev.of_node) > > + of_dma_controller_free(pdev->dev.of_node); > > + > > dma_async_device_unregister(&pdma->dma_dev); > > > > return 0;
diff --git a/drivers/dma/sf-pdma/sf-pdma.c b/drivers/dma/sf-pdma/sf-pdma.c index d1c6956af452..06a0912a12a1 100644 --- a/drivers/dma/sf-pdma/sf-pdma.c +++ b/drivers/dma/sf-pdma/sf-pdma.c @@ -20,6 +20,7 @@ #include <linux/mod_devicetable.h> #include <linux/dma-mapping.h> #include <linux/of.h> +#include <linux/of_dma.h> #include <linux/slab.h> #include "sf-pdma.h" @@ -490,6 +491,33 @@ static void sf_pdma_setup_chans(struct sf_pdma *pdma) } } +static struct dma_chan *sf_pdma_of_xlate(struct of_phandle_args *dma_spec, + struct of_dma *ofdma) +{ + struct sf_pdma *pdma = ofdma->of_dma_data; + struct device *dev = pdma->dma_dev.dev; + struct sf_pdma_chan *chan; + struct dma_chan *c; + u32 channel_id; + + if (dma_spec->args_count != 1) { + dev_err(dev, "Bad number of cells\n"); + return NULL; + } + + channel_id = dma_spec->args[0]; + + chan = &pdma->chans[channel_id]; + + c = dma_get_slave_channel(&chan->vchan.chan); + if (!c) { + dev_err(dev, "No more channels available\n"); + return NULL; + } + + return c; +} + static int sf_pdma_probe(struct platform_device *pdev) { struct sf_pdma *pdma; @@ -563,7 +591,20 @@ static int sf_pdma_probe(struct platform_device *pdev) return ret; } + ret = of_dma_controller_register(pdev->dev.of_node, + sf_pdma_of_xlate, pdma); + if (ret < 0) { + dev_err(&pdev->dev, + "Can't register SiFive Platform OF_DMA. (%d)\n", ret); + goto err_unregister; + } + return 0; + +err_unregister: + dma_async_device_unregister(&pdma->dma_dev); + + return ret; } static int sf_pdma_remove(struct platform_device *pdev) @@ -583,6 +624,9 @@ static int sf_pdma_remove(struct platform_device *pdev) tasklet_kill(&ch->err_tasklet); } + if (pdev->dev.of_node) + of_dma_controller_free(pdev->dev.of_node); + dma_async_device_unregister(&pdma->dma_dev); return 0;