Re: [PATCH 2/2] dmaengine: milbeaut-hdmac: Add HDMAC driver for Milbeaut platforms
On 15-08-19, 21:25, Jassi Brar wrote:
> On Mon, Jun 24, 2019 at 1:47 AM Vinod Koul wrote:
> >
> > On 12-06-19, 19:52, jassisinghb...@gmail.com wrote:
> >
> > > +#include
> > > +#include
> > > +#include
> > > +#include
> > > +#include
> > > +#include
> > > +#include
> > > +#include
> > > +#include
> > > +#include
> >
> > Do we need both, IIRC of_dma.h does include of.h!
> >
> OK
>
> > > +/* mc->vc.lock must be held by caller */
> > > +static void milbeaut_chan_start(struct milbeaut_hdmac_chan *mc,
> > > + struct milbeaut_hdmac_desc *md)
> > > +{
> > > + struct scatterlist *sg;
> > > + u32 cb, ca, src_addr, dest_addr, len;
> >^^
> > double space
> >
> OK
>
> > > +static irqreturn_t milbeaut_hdmac_interrupt(int irq, void *dev_id)
> > > +{
> > > + struct milbeaut_hdmac_chan *mc = dev_id;
> > > + struct milbeaut_hdmac_desc *md;
> > > + irqreturn_t ret = IRQ_HANDLED;
> > > + u32 val;
> > > +
> > > + spin_lock(>vc.lock);
> > > +
> > > + /* Ack and Disable irqs */
> > > + val = readl_relaxed(mc->reg_ch_base + MLB_HDMAC_DMACB);
> > > + val &= ~(FIELD_PREP(MLB_HDMAC_SS, 0x7));
> >
> > Magic ..?
> >
> OK, will define a macro for 7
>
> > > +static int milbeaut_hdmac_chan_pause(struct dma_chan *chan)
> > > +{
> > > + struct virt_dma_chan *vc = to_virt_chan(chan);
> > > + struct milbeaut_hdmac_chan *mc = to_milbeaut_hdmac_chan(vc);
> > > + u32 val;
> > > +
> > > + spin_lock(>vc.lock);
> > > + val = readl_relaxed(mc->reg_ch_base + MLB_HDMAC_DMACA);
> > > + val |= MLB_HDMAC_PB;
> > > + writel_relaxed(val, mc->reg_ch_base + MLB_HDMAC_DMACA);
> >
> > We really should have an updatel() and friends in kernel, feel free to
> > add in your driver though!
> >
> I'll pass on that for now.
>
> > > +static int milbeaut_hdmac_chan_init(struct platform_device *pdev,
> > > + struct milbeaut_hdmac_device *mdev,
> > > + int chan_id)
> > > +{
> > > + struct device *dev = >dev;
> > > + struct milbeaut_hdmac_chan *mc = >channels[chan_id];
> > > + char *irq_name;
> > > + int irq, ret;
> > > +
> > > + irq = platform_get_irq(pdev, chan_id);
> > > + if (irq < 0) {
> > > + dev_err(>dev, "failed to get IRQ number for ch%d\n",
> > > + chan_id);
> > > + return irq;
> > > + }
> > > +
> > > + irq_name = devm_kasprintf(dev, GFP_KERNEL, "milbeaut-hdmac-%d",
> > > + chan_id);
> > > + if (!irq_name)
> > > + return -ENOMEM;
> > > +
> > > + ret = devm_request_irq(dev, irq, milbeaut_hdmac_interrupt,
> > > +IRQF_SHARED, irq_name, mc);
> >
> > I tend to dislike using devm_request_irq(), we have no control over when
> > the irq is freed and what is a spirious irq is running while we are
> > unrolling, so IMHO it make sense to free up and ensure all tasklets are
> > quiesced when remove returns
> >
> If the code is written clean and tight we need not be so paranoid.
>
> > > + if (ret)
> > > + return ret;
> > > +
> > > + mc->mdev = mdev;
> > > + mc->reg_ch_base = mdev->reg_base + MLB_HDMAC_CH_STRIDE * (chan_id +
> > > 1);
> > > + mc->vc.desc_free = milbeaut_hdmac_desc_free;
> > > + vchan_init(>vc, >ddev);
> >
> > who kills the vc->task?
> >
> vchan_synchronize() called from milbeaut_hdmac_synchronize()
But that can be skipped by called, from driver pov we need to ensure
that it is killed
> > > +static int milbeaut_hdmac_remove(struct platform_device *pdev)
> > > +{
> > > + struct milbeaut_hdmac_device *mdev = platform_get_drvdata(pdev);
> > > + struct dma_chan *chan;
> > > + int ret;
> > > +
> > > + /*
> > > + * Before reaching here, almost all descriptors have been freed by
> > > the
> > > + * ->device_free_chan_resources() hook. However, each channel might
> > > + * be still holding one descriptor that was on-flight at that
> > > moment.
> > > + * Terminate it to make sure this hardware is no longer running.
> > > Then,
> > > + * free the channel resources once again to avoid memory leak.
> > > + */
> > > + list_for_each_entry(chan, >ddev.channels, device_node) {
> > > + ret = dmaengine_terminate_sync(chan);
> > > + if (ret)
> > > + return ret;
> > > + milbeaut_hdmac_free_chan_resources(chan);
> > > + }
> > > +
> > > + of_dma_controller_free(pdev->dev.of_node);
> > > + dma_async_device_unregister(>ddev);
> > > + clk_disable_unprepare(mdev->clk);
> >
> > And as suspected we have active tasklets and irq at this time :(
> >
> Not sure how is that
Since you use devm variants, and driver tasklet is not killed, irq can
be fired (spurious as well) and can run irq and schedule a tasklet. How
do you prevent that?
Thanks
--
Re: [PATCH 2/2] dmaengine: milbeaut-hdmac: Add HDMAC driver for Milbeaut platforms
On Mon, Jun 24, 2019 at 1:47 AM Vinod Koul wrote:
>
> On 12-06-19, 19:52, jassisinghb...@gmail.com wrote:
>
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
>
> Do we need both, IIRC of_dma.h does include of.h!
>
OK
> > +/* mc->vc.lock must be held by caller */
> > +static void milbeaut_chan_start(struct milbeaut_hdmac_chan *mc,
> > + struct milbeaut_hdmac_desc *md)
> > +{
> > + struct scatterlist *sg;
> > + u32 cb, ca, src_addr, dest_addr, len;
>^^
> double space
>
OK
> > +static irqreturn_t milbeaut_hdmac_interrupt(int irq, void *dev_id)
> > +{
> > + struct milbeaut_hdmac_chan *mc = dev_id;
> > + struct milbeaut_hdmac_desc *md;
> > + irqreturn_t ret = IRQ_HANDLED;
> > + u32 val;
> > +
> > + spin_lock(>vc.lock);
> > +
> > + /* Ack and Disable irqs */
> > + val = readl_relaxed(mc->reg_ch_base + MLB_HDMAC_DMACB);
> > + val &= ~(FIELD_PREP(MLB_HDMAC_SS, 0x7));
>
> Magic ..?
>
OK, will define a macro for 7
> > +static int milbeaut_hdmac_chan_pause(struct dma_chan *chan)
> > +{
> > + struct virt_dma_chan *vc = to_virt_chan(chan);
> > + struct milbeaut_hdmac_chan *mc = to_milbeaut_hdmac_chan(vc);
> > + u32 val;
> > +
> > + spin_lock(>vc.lock);
> > + val = readl_relaxed(mc->reg_ch_base + MLB_HDMAC_DMACA);
> > + val |= MLB_HDMAC_PB;
> > + writel_relaxed(val, mc->reg_ch_base + MLB_HDMAC_DMACA);
>
> We really should have an updatel() and friends in kernel, feel free to
> add in your driver though!
>
I'll pass on that for now.
> > +static int milbeaut_hdmac_chan_init(struct platform_device *pdev,
> > + struct milbeaut_hdmac_device *mdev,
> > + int chan_id)
> > +{
> > + struct device *dev = >dev;
> > + struct milbeaut_hdmac_chan *mc = >channels[chan_id];
> > + char *irq_name;
> > + int irq, ret;
> > +
> > + irq = platform_get_irq(pdev, chan_id);
> > + if (irq < 0) {
> > + dev_err(>dev, "failed to get IRQ number for ch%d\n",
> > + chan_id);
> > + return irq;
> > + }
> > +
> > + irq_name = devm_kasprintf(dev, GFP_KERNEL, "milbeaut-hdmac-%d",
> > + chan_id);
> > + if (!irq_name)
> > + return -ENOMEM;
> > +
> > + ret = devm_request_irq(dev, irq, milbeaut_hdmac_interrupt,
> > +IRQF_SHARED, irq_name, mc);
>
> I tend to dislike using devm_request_irq(), we have no control over when
> the irq is freed and what is a spirious irq is running while we are
> unrolling, so IMHO it make sense to free up and ensure all tasklets are
> quiesced when remove returns
>
If the code is written clean and tight we need not be so paranoid.
> > + if (ret)
> > + return ret;
> > +
> > + mc->mdev = mdev;
> > + mc->reg_ch_base = mdev->reg_base + MLB_HDMAC_CH_STRIDE * (chan_id +
> > 1);
> > + mc->vc.desc_free = milbeaut_hdmac_desc_free;
> > + vchan_init(>vc, >ddev);
>
> who kills the vc->task?
>
vchan_synchronize() called from milbeaut_hdmac_synchronize()
> > +static int milbeaut_hdmac_remove(struct platform_device *pdev)
> > +{
> > + struct milbeaut_hdmac_device *mdev = platform_get_drvdata(pdev);
> > + struct dma_chan *chan;
> > + int ret;
> > +
> > + /*
> > + * Before reaching here, almost all descriptors have been freed by the
> > + * ->device_free_chan_resources() hook. However, each channel might
> > + * be still holding one descriptor that was on-flight at that moment.
> > + * Terminate it to make sure this hardware is no longer running. Then,
> > + * free the channel resources once again to avoid memory leak.
> > + */
> > + list_for_each_entry(chan, >ddev.channels, device_node) {
> > + ret = dmaengine_terminate_sync(chan);
> > + if (ret)
> > + return ret;
> > + milbeaut_hdmac_free_chan_resources(chan);
> > + }
> > +
> > + of_dma_controller_free(pdev->dev.of_node);
> > + dma_async_device_unregister(>ddev);
> > + clk_disable_unprepare(mdev->clk);
>
> And as suspected we have active tasklets and irq at this time :(
>
Not sure how is that
thanks.
Re: [PATCH 2/2] dmaengine: milbeaut-hdmac: Add HDMAC driver for Milbeaut platforms
On 12-06-19, 19:52, jassisinghb...@gmail.com wrote:
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
Do we need both, IIRC of_dma.h does include of.h!
> +/* mc->vc.lock must be held by caller */
> +static void milbeaut_chan_start(struct milbeaut_hdmac_chan *mc,
> + struct milbeaut_hdmac_desc *md)
> +{
> + struct scatterlist *sg;
> + u32 cb, ca, src_addr, dest_addr, len;
^^
double space
> +static irqreturn_t milbeaut_hdmac_interrupt(int irq, void *dev_id)
> +{
> + struct milbeaut_hdmac_chan *mc = dev_id;
> + struct milbeaut_hdmac_desc *md;
> + irqreturn_t ret = IRQ_HANDLED;
> + u32 val;
> +
> + spin_lock(>vc.lock);
> +
> + /* Ack and Disable irqs */
> + val = readl_relaxed(mc->reg_ch_base + MLB_HDMAC_DMACB);
> + val &= ~(FIELD_PREP(MLB_HDMAC_SS, 0x7));
Magic ..?
> +static int milbeaut_hdmac_chan_pause(struct dma_chan *chan)
> +{
> + struct virt_dma_chan *vc = to_virt_chan(chan);
> + struct milbeaut_hdmac_chan *mc = to_milbeaut_hdmac_chan(vc);
> + u32 val;
> +
> + spin_lock(>vc.lock);
> + val = readl_relaxed(mc->reg_ch_base + MLB_HDMAC_DMACA);
> + val |= MLB_HDMAC_PB;
> + writel_relaxed(val, mc->reg_ch_base + MLB_HDMAC_DMACA);
We really should have an updatel() and friends in kernel, feel free to
add in your driver though!
> +static int milbeaut_hdmac_chan_init(struct platform_device *pdev,
> + struct milbeaut_hdmac_device *mdev,
> + int chan_id)
> +{
> + struct device *dev = >dev;
> + struct milbeaut_hdmac_chan *mc = >channels[chan_id];
> + char *irq_name;
> + int irq, ret;
> +
> + irq = platform_get_irq(pdev, chan_id);
> + if (irq < 0) {
> + dev_err(>dev, "failed to get IRQ number for ch%d\n",
> + chan_id);
> + return irq;
> + }
> +
> + irq_name = devm_kasprintf(dev, GFP_KERNEL, "milbeaut-hdmac-%d",
> + chan_id);
> + if (!irq_name)
> + return -ENOMEM;
> +
> + ret = devm_request_irq(dev, irq, milbeaut_hdmac_interrupt,
> +IRQF_SHARED, irq_name, mc);
I tend to dislike using devm_request_irq(), we have no control over when
the irq is freed and what is a spirious irq is running while we are
unrolling, so IMHO it make sense to free up and ensure all tasklets are
quiesced when remove returns
> + if (ret)
> + return ret;
> +
> + mc->mdev = mdev;
> + mc->reg_ch_base = mdev->reg_base + MLB_HDMAC_CH_STRIDE * (chan_id + 1);
> + mc->vc.desc_free = milbeaut_hdmac_desc_free;
> + vchan_init(>vc, >ddev);
who kills the vc->task?
> +static int milbeaut_hdmac_remove(struct platform_device *pdev)
> +{
> + struct milbeaut_hdmac_device *mdev = platform_get_drvdata(pdev);
> + struct dma_chan *chan;
> + int ret;
> +
> + /*
> + * Before reaching here, almost all descriptors have been freed by the
> + * ->device_free_chan_resources() hook. However, each channel might
> + * be still holding one descriptor that was on-flight at that moment.
> + * Terminate it to make sure this hardware is no longer running. Then,
> + * free the channel resources once again to avoid memory leak.
> + */
> + list_for_each_entry(chan, >ddev.channels, device_node) {
> + ret = dmaengine_terminate_sync(chan);
> + if (ret)
> + return ret;
> + milbeaut_hdmac_free_chan_resources(chan);
> + }
> +
> + of_dma_controller_free(pdev->dev.of_node);
> + dma_async_device_unregister(>ddev);
> + clk_disable_unprepare(mdev->clk);
And as suspected we have active tasklets and irq at this time :(
--
~Vinod
[PATCH 2/2] dmaengine: milbeaut-hdmac: Add HDMAC driver for Milbeaut platforms
From: Jassi Brar
Driver for Socionext Milbeaut HDMAC controller. The controller has
upto 8 floating channels, that need a predefined slave-id to work
from a set of slaves.
Signed-off-by: Jassi Brar
---
drivers/dma/Kconfig | 10 +
drivers/dma/Makefile | 1 +
drivers/dma/milbeaut-hdmac.c | 572 +++
3 files changed, 583 insertions(+)
create mode 100644 drivers/dma/milbeaut-hdmac.c
diff --git a/drivers/dma/Kconfig b/drivers/dma/Kconfig
index 703275cc29de..15a1d5263ca1 100644
--- a/drivers/dma/Kconfig
+++ b/drivers/dma/Kconfig
@@ -347,6 +347,16 @@ config MCF_EDMA
minimal intervention from a host processor.
This module can be found on Freescale ColdFire mcf5441x SoCs.
+config MILBEAUT_HDMAC
+ tristate "Milbeaut AHB DMA support"
+ depends on ARCH_MILBEAUT || COMPILE_TEST
+ depends on OF
+ select DMA_ENGINE
+ select DMA_VIRTUAL_CHANNELS
+ help
+ Say yes here to support the Socionext Milbeaut
+ HDMAC device.
+
config MMP_PDMA
bool "MMP PDMA support"
depends on ARCH_MMP || ARCH_PXA || COMPILE_TEST
diff --git a/drivers/dma/Makefile b/drivers/dma/Makefile
index 6126e1c3a875..d0a9f46726e8 100644
--- a/drivers/dma/Makefile
+++ b/drivers/dma/Makefile
@@ -45,6 +45,7 @@ obj-$(CONFIG_INTEL_IOP_ADMA) += iop-adma.o
obj-$(CONFIG_INTEL_MIC_X100_DMA) += mic_x100_dma.o
obj-$(CONFIG_K3_DMA) += k3dma.o
obj-$(CONFIG_LPC18XX_DMAMUX) += lpc18xx-dmamux.o
+obj-$(CONFIG_MILBEAUT_HDMAC) += milbeaut-hdmac.o
obj-$(CONFIG_MMP_PDMA) += mmp_pdma.o
obj-$(CONFIG_MMP_TDMA) += mmp_tdma.o
obj-$(CONFIG_MOXART_DMA) += moxart-dma.o
diff --git a/drivers/dma/milbeaut-hdmac.c b/drivers/dma/milbeaut-hdmac.c
new file mode 100644
index ..9c9fabdf8cdc
--- /dev/null
+++ b/drivers/dma/milbeaut-hdmac.c
@@ -0,0 +1,572 @@
+// SPDX-License-Identifier: GPL-2.0
+//
+// Copyright (C) 2019 Linaro Ltd.
+// Copyright (C) 2019 Socionext Inc.
+
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+#include "virt-dma.h"
+
+#define MLB_HDMAC_DMACR0x0 /* global */
+#define MLB_HDMAC_DE BIT(31)
+#define MLB_HDMAC_DS BIT(30)
+#define MLB_HDMAC_PR BIT(28)
+#define MLB_HDMAC_DH GENMASK(27, 24)
+
+#define MLB_HDMAC_CH_STRIDE0x10
+
+#define MLB_HDMAC_DMACA0x0 /* channel */
+#define MLB_HDMAC_EB BIT(31)
+#define MLB_HDMAC_PB BIT(30)
+#define MLB_HDMAC_ST BIT(29)
+#define MLB_HDMAC_IS GENMASK(28, 24)
+#define MLB_HDMAC_BT GENMASK(23, 20)
+#define MLB_HDMAC_BC GENMASK(19, 16)
+#define MLB_HDMAC_TC GENMASK(15, 0)
+#define MLB_HDMAC_DMACB0x4
+#define MLB_HDMAC_TT GENMASK(31, 30)
+#define MLB_HDMAC_MS GENMASK(29, 28)
+#define MLB_HDMAC_TW GENMASK(27, 26)
+#define MLB_HDMAC_FS BIT(25)
+#define MLB_HDMAC_FD BIT(24)
+#define MLB_HDMAC_RC BIT(23)
+#define MLB_HDMAC_RS BIT(22)
+#define MLB_HDMAC_RD BIT(21)
+#define MLB_HDMAC_EI BIT(20)
+#define MLB_HDMAC_CI BIT(19)
+#define MLB_HDMAC_SS GENMASK(18, 16)
+#define MLB_HDMAC_SP GENMASK(15, 12)
+#define MLB_HDMAC_DP GENMASK(11, 8)
+#define MLB_HDMAC_DMACSA 0x8
+#define MLB_HDMAC_DMACDA 0xc
+
+#define MLB_HDMAC_BUSWIDTHS(BIT(DMA_SLAVE_BUSWIDTH_1_BYTE) | \
+ BIT(DMA_SLAVE_BUSWIDTH_2_BYTES) | \
+ BIT(DMA_SLAVE_BUSWIDTH_4_BYTES))
+
+struct milbeaut_hdmac_desc {
+ struct virt_dma_desc vd;
+ struct scatterlist *sgl;
+ unsigned int sg_len;
+ unsigned int sg_cur;
+ enum dma_transfer_direction dir;
+};
+
+struct milbeaut_hdmac_chan {
+ struct virt_dma_chan vc;
+ struct milbeaut_hdmac_device *mdev;
+ struct milbeaut_hdmac_desc *md;
+ void __iomem *reg_ch_base;
+ unsigned int slave_id;
+ struct dma_slave_config cfg;
+};
+
+struct milbeaut_hdmac_device {
+ struct dma_device ddev;
+ struct clk *clk;
+ void __iomem *reg_base;
+ struct milbeaut_hdmac_chan channels[0];
+};
+
+static struct milbeaut_hdmac_chan *
+to_milbeaut_hdmac_chan(struct virt_dma_chan *vc)
+{
+ return container_of(vc, struct milbeaut_hdmac_chan, vc);
+}
+
+static struct milbeaut_hdmac_desc *
+to_milbeaut_hdmac_desc(struct virt_dma_desc *vd)
+{
+ return container_of(vd, struct milbeaut_hdmac_desc, vd);
+}
+
+/* mc->vc.lock must be held by caller */
+static struct milbeaut_hdmac_desc *
+milbeaut_hdmac_next_desc(struct milbeaut_hdmac_chan *mc)
+{
+ struct virt_dma_desc *vd;
+
+ vd = vchan_next_desc(>vc);
+ if (!vd) {
+ mc->md = NULL;
+ return NULL;
+ }
+
+ list_del(>node);
+
+
