Re: [PATCH] dmaengine: Add hisilicon k3 DMA engine driver
On Monday 24 June 2013 16:49:08 zhangfei gao wrote: Dear Arnd Vinod The suggestion of using dma_get_slave_channel instead of filter works here. Dma driver should modify accordingly. The changes all look good to me, thanks a lot for following up! However, you should really follow the procedure for posting patches even if it's just for the sake of discussion: * always provide a signed-off-by and a patch description * use an email client that doesn't break whitespace * I would have split this up into two separate patches, one for the subsystem and one for the new driver. Arnd ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH] dmaengine: Add hisilicon k3 DMA engine driver
On Mon, Jun 17, 2013 at 12:54:32PM +0800, Zhangfei Gao wrote:
Add dmaengine driver for hisilicon k3 platform based on virt_dma
Signed-off-by: Zhangfei Gao zhangfei@linaro.org
Tested-by: Kai Yang jean.yang...@huawei.com
---
[snip]
+#define to_k3_dma(dmadev) container_of(dmadev, struct k3_dma_dev, slave)
+
+static struct k3_dma_chan *to_k3_chan(struct dma_chan *chan)
+{
+ return container_of(chan, struct k3_dma_chan, vc.chan);
+}
+
+static void terminate_chan(struct k3_dma_phy *phy, struct k3_dma_dev *d)
namespace pls
+{
+ u32 val = 0;
+
+ val = readl_relaxed(phy-base + CX_CONFIG);
+ val = ~CCFG_EN;
+ writel_relaxed(val, phy-base + CX_CONFIG);
+
+ val = 0x1 phy-idx;
+ writel_relaxed(val, d-base + INT_TC1_RAW);
+ writel_relaxed(val, d-base + INT_ERR1_RAW);
+ writel_relaxed(val, d-base + INT_ERR2_RAW);
+}
+
+static void set_desc(struct k3_dma_phy *phy, struct k3_desc_hw *hw)
namespace pls
+{
+ writel_relaxed(hw-lli, phy-base + CX_LLI);
+ writel_relaxed(hw-count, phy-base + CX_CNT);
+ writel_relaxed(hw-saddr, phy-base + CX_SRC);
+ writel_relaxed(hw-daddr, phy-base + CX_DST);
+ writel_relaxed(hw-config, phy-base + CX_CONFIG);
+}
+
+static u32 get_curr_cnt(struct k3_dma_dev *d, struct k3_dma_phy *phy)
ditto
+{
+ u32 cnt = 0;
+
+ cnt = readl_relaxed(d-base + CX_CUR_CNT + phy-idx * 0x10);
+ cnt = 0x;
+ return cnt;
+}
+
+static u32 get_curr_lli(struct k3_dma_phy *phy)
+{
+ return readl_relaxed(phy-base + CX_LLI);
+}
+
+static u32 get_chan_stat(struct k3_dma_dev *d)
+{
+ return readl_relaxed(d-base + CH_STAT);
+}
+
+static void trigger_dma(struct k3_dma_dev *d, bool on)
ditto
+{
+ if (on) {
+ /* set same priority */
+ writel_relaxed(0x0, d-base + CH_PRI);
+
+ /* unmask irq */
+ writel_relaxed(0x, d-base + INT_TC1_MASK);
+ writel_relaxed(0x, d-base + INT_ERR1_MASK);
+ writel_relaxed(0x, d-base + INT_ERR2_MASK);
+ } else {
+ /* mask irq */
+ writel_relaxed(0x0, d-base + INT_TC1_MASK);
+ writel_relaxed(0x0, d-base + INT_ERR1_MASK);
+ writel_relaxed(0x0, d-base + INT_ERR2_MASK);
+ }
+}
+
+static irqreturn_t k3_dma_int_handler(int irq, void *dev_id)
+{
+ struct k3_dma_dev *d = (struct k3_dma_dev *)dev_id;
+ struct k3_dma_phy *p;
+ u32 stat = readl_relaxed(d-base + INT_STAT);
+ u32 tc1 = readl_relaxed(d-base + INT_TC1);
+ u32 err1 = readl_relaxed(d-base + INT_ERR1);
+ u32 err2 = readl_relaxed(d-base + INT_ERR2);
+ u32 i, irq_chan = 0;
+
+ while (stat) {
+ i = __ffs(stat);
+ stat = (stat - 1);
+ if (likely(tc1 BIT(i))) {
+ p = d-phy[i];
+ p-ds_done = p-ds_run;
+ vchan_cookie_complete(p-ds_run-vd);
+ irq_chan |= BIT(i);
+ }
+ if (unlikely((err1 BIT(i)) || (err2 BIT(i
+ dev_warn(d-slave.dev, DMA ERR\n);
+ }
+
+ writel_relaxed(irq_chan, d-base + INT_TC1_RAW);
+ writel_relaxed(err1, d-base + INT_ERR1_RAW);
+ writel_relaxed(err2, d-base + INT_ERR2_RAW);
+
+ if (irq_chan) {
+ tasklet_schedule(d-task);
+ return IRQ_HANDLED;
+ } else
+ return IRQ_NONE;
+}
+
+static int k3_dma_start_txd(struct k3_dma_chan *c)
+{
+ struct k3_dma_dev *d = to_k3_dma(c-vc.chan.device);
+ struct virt_dma_desc *vd = vchan_next_desc(c-vc);
+
+ if (BIT(c-phy-idx) get_chan_stat(d))
+ return -EAGAIN;
+
+ if (vd) {
+ struct k3_dma_desc_sw *ds =
+ container_of(vd, struct k3_dma_desc_sw, vd);
+ /*
+ * fetch and remove request from vc-desc_issued
+ * so vc-desc_issued only contains desc pending
+ */
+ list_del(ds-vd.node);
+ c-phy-ds_run = ds;
+ c-phy-ds_done = NULL;
+ /* start dma */
+ set_desc(c-phy, ds-desc_hw[0]);
+ return 0;
+ }
+ c-phy-ds_done = NULL;
+ c-phy-ds_run = NULL;
+ return -EAGAIN;
+}
+
+static void k3_dma_tasklet(unsigned long arg)
+{
+ struct k3_dma_dev *d = (struct k3_dma_dev *)arg;
+ struct k3_dma_phy *p;
+ struct k3_dma_chan *c;
+ unsigned pch, pch_alloc = 0;
+
+ dev_dbg(d-slave.dev, tasklet enter\n);
+
+ /* check new dma request of running channel in vc-desc_issued */
+ list_for_each_entry(c, d-slave.channels, vc.chan.device_node) {
this should use _safe, you might be adding a new txn while executing this
+ spin_lock_irq(c-vc.lock);
+ p = c-phy;
+ if (p p-ds_done) {
+ if (k3_dma_start_txd(c)) {
+ /*
Re: [PATCH] dmaengine: Add hisilicon k3 DMA engine driver
On Mon, Jun 17, 2013 at 10:58:07PM +0200, Arnd Bergmann wrote:
On Monday 17 June 2013, Zhangfei Gao wrote:
Add dmaengine driver for hisilicon k3 platform based on virt_dma
Signed-off-by: Zhangfei Gao zhangfei@linaro.org
Tested-by: Kai Yang jean.yang...@huawei.com
Acked-by: Arnd Bergmann a...@arndb.de
This filter function works only as long as there is no more than
one DMA engine in the system, which is something that needs to
be documented better. Unfortunately, providing a filter
function to be called by .xlate is currently the only way that
the dma-engine API supports, but we should really get over that.
Vinod: I think we need to add a way for a dmaengine driver to
just return one of its channels to the xlate function. The
current method is getting very silly, and it adds run-time and
code complexity without any need.
How about something like
int dma_get_slave_channel(struct dma_chan *chan)
{
/* lock against __dma_request_channel */
mutex_lock(dma_list_mutex);
if (chan-client_count == 0)
ret = dma_chan_get(chan);
else
ret = -EBUSY;
mutex_unlock(dma_list_mutex);
return ret;
}
EXPORT_SYMBOL_GPL(dma_get_slave_channel);
and you add filter on top?
This is getting you any channel and maynot work where we need to do some
filtering.
--
~vinod
--
___
devicetree-discuss mailing list
devicetree-discuss@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH] dmaengine: Add hisilicon k3 DMA engine driver
On Tue, Jun 18, 2013 at 04:09:14PM +0200, Arnd Bergmann wrote:
On Tuesday 18 June 2013, zhangfei gao wrote:
On Tue, Jun 18, 2013 at 4:58 AM, Arnd Bergmann a...@arndb.de wrote:
+static struct of_dma_filter_info k3_dma_filter;
+static bool k3_dma_filter_fn(struct dma_chan *chan, void *param)
+{
+ return (*(int *)param == chan-chan_id);
+}
This filter function works only as long as there is no more than
one DMA engine in the system, which is something that needs to
be documented better. Unfortunately, providing a filter
function to be called by .xlate is currently the only way that
the dma-engine API supports, but we should really get over that.
Vinod: I think we need to add a way for a dmaengine driver to
just return one of its channels to the xlate function. The
current method is getting very silly, and it adds run-time and
code complexity without any need.
How about something like
int dma_get_slave_channel(struct dma_chan *chan)
{
/* lock against __dma_request_channel */
mutex_lock(dma_list_mutex);
if (chan-client_count == 0)
ret = dma_chan_get(chan);
else
ret = -EBUSY;
mutex_unlock(dma_list_mutex);
return ret;
}
EXPORT_SYMBOL_GPL(dma_get_slave_channel);
Want to double check here.
Device need specific channel via request line, the requirement still
can be met, right?
The driver can have a simple array of pointers that is indexed by
the request number, so you end up with something like
struct dma_chan *of_dma_simple_xlate(struct of_phandle_args *dma_spec,
struct of_dma *ofdma)
{
struct k3_dma_dev *dev = ofdma-of_dma_data;
unsigned int vchan = dma_spec-args[0];
if (vchan dev-nr_channels)
return NULL;
return dma_get_slave_channel(dev-vchan[vchan]);
}
With no need to have a filter function.
for SW muxes this may work well IMO
--
~Vinod
___
devicetree-discuss mailing list
devicetree-discuss@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH] dmaengine: Add hisilicon k3 DMA engine driver
On Tue, Jun 18, 2013 at 05:08:01PM +0200, Arnd Bergmann wrote: On Tuesday 18 June 2013 22:22:17 zhangfei wrote: With no need to have a filter function. Cool, then I would like to wait for the patch. Maybe you can try to add the dma_get_slave_channel() function I proposed here as a first patch and add your driver on top. There may be issues I missed, and Vinod needs to agree to the concept first, but that would probably get his attention. You have it now :) Or you could send your the new interface as an add-on patch and convert your driver along with adding it. patch is always welcome -- ~Vinod ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH] dmaengine: Add hisilicon k3 DMA engine driver
On Friday 21 June 2013, Vinod Koul wrote:
On Mon, Jun 17, 2013 at 10:58:07PM +0200, Arnd Bergmann wrote:
On Monday 17 June 2013, Zhangfei Gao wrote:
int dma_get_slave_channel(struct dma_chan *chan)
{
/* lock against __dma_request_channel */
mutex_lock(dma_list_mutex);
if (chan-client_count == 0)
ret = dma_chan_get(chan);
else
ret = -EBUSY;
mutex_unlock(dma_list_mutex);
return ret;
}
EXPORT_SYMBOL_GPL(dma_get_slave_channel);
and you add filter on top?
No, the idea is to no longer require a filter function when
we use dma_request_slave_channel with a DT specifier.
This is getting you any channel and maynot work where we need to do some
filtering.
This function would be called only by the dmaengine driver's
xlate function. That driver obviously has to ensure that the
channel works for the specification from DT (or ACPI or
something else), but that part is easy, since that is
the same information that we currently pass into the filter
function.
Arnd
___
devicetree-discuss mailing list
devicetree-discuss@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH] dmaengine: Add hisilicon k3 DMA engine driver
On Tuesday 18 June 2013, zhangfei gao wrote:
On Tue, Jun 18, 2013 at 4:58 AM, Arnd Bergmann a...@arndb.de wrote:
+static struct of_dma_filter_info k3_dma_filter;
+static bool k3_dma_filter_fn(struct dma_chan *chan, void *param)
+{
+ return (*(int *)param == chan-chan_id);
+}
This filter function works only as long as there is no more than
one DMA engine in the system, which is something that needs to
be documented better. Unfortunately, providing a filter
function to be called by .xlate is currently the only way that
the dma-engine API supports, but we should really get over that.
Vinod: I think we need to add a way for a dmaengine driver to
just return one of its channels to the xlate function. The
current method is getting very silly, and it adds run-time and
code complexity without any need.
How about something like
int dma_get_slave_channel(struct dma_chan *chan)
{
/* lock against __dma_request_channel */
mutex_lock(dma_list_mutex);
if (chan-client_count == 0)
ret = dma_chan_get(chan);
else
ret = -EBUSY;
mutex_unlock(dma_list_mutex);
return ret;
}
EXPORT_SYMBOL_GPL(dma_get_slave_channel);
Want to double check here.
Device need specific channel via request line, the requirement still
can be met, right?
The driver can have a simple array of pointers that is indexed by
the request number, so you end up with something like
struct dma_chan *of_dma_simple_xlate(struct of_phandle_args *dma_spec,
struct of_dma *ofdma)
{
struct k3_dma_dev *dev = ofdma-of_dma_data;
unsigned int vchan = dma_spec-args[0];
if (vchan dev-nr_channels)
return NULL;
return dma_get_slave_channel(dev-vchan[vchan]);
}
With no need to have a filter function.
Arnd
___
devicetree-discuss mailing list
devicetree-discuss@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH] dmaengine: Add hisilicon k3 DMA engine driver
On Tuesday 18 June 2013 22:22:17 zhangfei wrote: With no need to have a filter function. Cool, then I would like to wait for the patch. Maybe you can try to add the dma_get_slave_channel() function I proposed here as a first patch and add your driver on top. There may be issues I missed, and Vinod needs to agree to the concept first, but that would probably get his attention. Or you could send your the new interface as an add-on patch and convert your driver along with adding it. Arnd ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH] dmaengine: Add hisilicon k3 DMA engine driver
On Monday 17 June 2013, Zhangfei Gao wrote:
Add dmaengine driver for hisilicon k3 platform based on virt_dma
Signed-off-by: Zhangfei Gao zhangfei@linaro.org
Tested-by: Kai Yang jean.yang...@huawei.com
Acked-by: Arnd Bergmann a...@arndb.de
diff --git a/Documentation/devicetree/bindings/dma/k3dma.txt
b/Documentation/devicetree/bindings/dma/k3dma.txt
new file mode 100644
index 000..cf156f2
--- /dev/null
+++ b/Documentation/devicetree/bindings/dma/k3dma.txt
@@ -0,0 +1,44 @@
+* Hisilicon K3 DMA controller
+
+See dma.txt first
+
+Required properties:
+- compatible: Should be hisilicon,k3-dma-1.0
+- reg: Should contain DMA registers location and length.
+- interrupts: Should contain one interrupt shared by all channel
+- #dma-cells: see dma.txt, should be 1, para number
+- dma-channels: virtual channels supported, each virtual channel
+ have specific request line
+- clocks: clock required
+
+Example:
+
+Controller:
+ dma0: dma@fcd02000 {
+ compatible = hisilicon,k3-dma-1.0;
+ reg = 0xfcd02000 0x1000;
+ #dma-cells = 1;
+ dma-channels = 27;
+ interrupts = 0 12 4;
+ clocks = pclk;
+ status = disable;
+ };
+
+Client:
+Use specific request line passing from dmax
+For example, i2c0 read channel request line is 18, while write channel use 19
+
+ i2c0: i2c@fcb08000 {
+ compatible = snps,designware-i2c;
+ dmas = dma0 18 /* read channel */
+ dma0 19;/* write channel */
+ dma-names = rx, tx;
+ };
+
+ i2c1: i2c@fcb09000 {
+ compatible = snps,designware-i2c;
+ dmas = dma0 20 /* read channel */
+ dma0 21;/* write channel */
+ dma-names = rx, tx;
+ };
The binding looks good to me.
I'd like to make sure the dma-channels property is right though:
You specify that number in DT but ...
+#define DRIVER_NAME k3-dma
+#define NR_PHY_CHAN 16
+#define DMA_ALIGN3
... you also hardcode the number to 16. Shouldn't the channels be
allocated dynamically based on the dma-channels property?
You do allocate virtual channels based on the dma-channels
later. Wouldn't that be request line numbers, i.e. dma-requests
rather than dma-channels?
+static struct of_dma_filter_info k3_dma_filter;
+static bool k3_dma_filter_fn(struct dma_chan *chan, void *param)
+{
+ return (*(int *)param == chan-chan_id);
+}
This filter function works only as long as there is no more than
one DMA engine in the system, which is something that needs to
be documented better. Unfortunately, providing a filter
function to be called by .xlate is currently the only way that
the dma-engine API supports, but we should really get over that.
Vinod: I think we need to add a way for a dmaengine driver to
just return one of its channels to the xlate function. The
current method is getting very silly, and it adds run-time and
code complexity without any need.
How about something like
int dma_get_slave_channel(struct dma_chan *chan)
{
/* lock against __dma_request_channel */
mutex_lock(dma_list_mutex);
if (chan-client_count == 0)
ret = dma_chan_get(chan);
else
ret = -EBUSY;
mutex_unlock(dma_list_mutex);
return ret;
}
EXPORT_SYMBOL_GPL(dma_get_slave_channel);
+ /* init virtual channel */
+ for (i = 0; i dma_channels; i++) {
+ struct k3_dma_chan *c;
+
+ c = devm_kzalloc(op-dev,
+ sizeof(struct k3_dma_chan), GFP_KERNEL);
+ if (c == NULL)
+ return -ENOMEM;
+
+ INIT_LIST_HEAD(c-node);
+ c-vc.desc_free = k3_dma_free_desc;
+ vchan_init(c-vc, d-slave);
+ }
Note that a single devm_kzalloc would be slightly more space efficient
here.
Arnd
___
devicetree-discuss mailing list
devicetree-discuss@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss
