Re: [PATCH 2/2] dmaengine: Add DW AXI DMAC driver

[Vinod Koul]

On Wed, Jan 25, 2017 at 06:34:17PM +0300, Eugeniy Paltsev wrote:
> This patch adds support for the DW AXI DMAC controller.
> 
> DW AXI DMAC is a part of upcoming development board from Synopsys.

How different is AXI from DW Synopsys?

Is the spec publicly available?

> +config AXI_DW_DMAC
> + tristate "Synopsys DesignWare AXI DMA support"
> + depends on OF && !64BIT

why not 64 bit, can you also add compile test

> +#define AXI_DMA_BUSWIDTHS  \
> + (DMA_SLAVE_BUSWIDTH_UNDEFINED   | \

DMA_SLAVE_BUSWIDTH_UNDEFINED??

> +static irqreturn_t dw_axi_dma_intretupt(int irq, void *dev_id)
> +{
> + struct axi_dma_chip *chip = dev_id;
> +
> + /* Disable DMAC inerrupts. We'll enable them in the tasklet */
> + axi_dma_irq_disable(chip);
> +
> + tasklet_schedule(>dw->tasklet);

This is very inefficient, we would want to submit next txn here and not in
tasklet

> +static void axi_chan_block_xfer_complete(struct axi_dma_chan *chan)
> +{
> + struct virt_dma_desc *vd;
> + unsigned long flags;
> +
> + spin_lock_irqsave(>vc.lock, flags);
> + if (unlikely(axi_chan_is_hw_enable(chan))) {
> + dev_err(chan2dev(chan), "BUG: %s catched DWAXIDMAC_IRQ_DMA_TRF, 
> but channel not idle!\n",
> + axi_chan_name(chan));
> + axi_chan_disable(chan);
> + }
> +
> + /* The completed descriptor currently is in the head of vc list */
> + vd = vchan_next_desc(>vc);
> + /* Remove the completed descriptor from issued list before completing */
> + list_del(>node);
> + vchan_cookie_complete(vd);

this should be called from irq handler


> +static int dw_remove(struct platform_device *pdev)
> +{
> + struct axi_dma_chip *chip = platform_get_drvdata(pdev);
> + struct dw_axi_dma *dw = chip->dw;
> + struct axi_dma_chan *chan, *_chan;
> + u32 i;
> +
> + axi_dma_irq_disable(chip);
> + for (i = 0; i < dw->hdata->nr_channels; i++) {
> + axi_chan_disable(>dw->chan[i]);
> + axi_chan_irq_disable(>dw->chan[i], DWAXIDMAC_IRQ_ALL);
> + }
> + axi_dma_disable(chip);
> +
> + tasklet_kill(>tasklet);
> +
> + list_for_each_entry_safe(chan, _chan, >dma.channels,
> + vc.chan.device_node) {
> + list_del(>vc.chan.device_node);
> + tasklet_kill(>vc.task);
> + }

very nice :)

But please freeup irq as well

> +module_platform_driver(dw_driver);
> +
> +static int __init dw_init(void)
> +{
> + return platform_driver_register(_driver);
> +}
> +subsys_initcall(dw_init);

why subsys_initcall??

> +/* Common registers offset */
> +#define DMAC_ID  0x000 // R DMAC ID
> +#define DMAC_COMPVER 0x008 // R DMAC Component Version
> +#define DMAC_CFG 0x010 // R/W DMAC Configuration
> +#define DMAC_CHEN0x018 // R/W DMAC Channel Enable
> +#define DMAC_CHEN_L  0x018 // R/W DMAC Channel Enable 00-31
> +#define DMAC_CHEN_H  0x01C // R/W DMAC Channel Enable 32-63
> +#define DMAC_INTSTATUS   0x030 // R DMAC Interrupt Status
> +#define DMAC_COMMON_INTCLEAR 0x038 // W DMAC Interrupt Clear
> +#define DMAC_COMMON_INTSTATUS_ENA 0x040 // R DMAC Interrupt Status Enable
> +#define DMAC_COMMON_INTSIGNAL_ENA 0x048 // R/W DMAC Interrupt Signal Enable
> +#define DMAC_COMMON_INTSTATUS0x050 // R DMAC Interrupt Status
> +#define DMAC_RESET   0x058 // R DMAC Reset Register1

DMAC is a generic term, AX_DMAC and no C99 style comment, checkpatch would
have cribbed

Use BITS and GENMASK here

> +
> +/* DMA channel registers offset */
> +#define CH_SAR   0x000 // R/W Chan Source Address
> +#define CH_DAR   0x008 // R/W Chan Destination Address
> +#define CH_BLOCK_TS  0x010 // R/W Chan Block Transfer Size
> +#define CH_CTL   0x018 // R/W Chan Control
> +#define CH_CTL_L 0x018 // R/W Chan Control 00-31
> +#define CH_CTL_H 0x01C // R/W Chan Control 32-63
> +#define CH_CFG   0x020 // R/W Chan Configuration
> +#define CH_CFG_L 0x020 // R/W Chan Configuration 00-31
> +#define CH_CFG_H 0x024 // R/W Chan Configuration 32-63
> +#define CH_LLP   0x028 // R/W Chan Linked List Pointer
> +#define CH_STATUS0x030 // R Chan Status
> +#define CH_SWHSSRC   0x038 // R/W Chan SW Handshake Source
> +#define CH_SWHSDST   0x040 // R/W Chan SW Handshake Destination
> +#define CH_BLK_TFR_RESUMEREQ 0x048 // W Chan Block Transfer Resume Req
> +#define CH_AXI_ID0x050 // R/W Chan AXI ID
> +#define CH_AXI_QOS   0x058 // R/W Chan AXI QOS
> +#define CH_SSTAT 0x060 // R Chan Source Status
> +#define CH_DSTAT 0x068 // R Chan Destination Status
> +#define CH_SSTATAR   0x070 // R/W Chan Source Status Fetch Addr
> +#define CH_DSTATAR   0x078 // R/W Chan Destination Status Fetch Addr

RE: [PATCH] ARCv2: intc: Disable all core interrupts by default

[Yuriy Kolerov]

Hi Vineet,

> -Original Message-
> From: Vineet Gupta [mailto:vgu...@synopsys.com]
> Sent: Wednesday, February 08, 2017 7:08 PM
> To: Yuriy Kolerov ; linux-snps-
> a...@lists.infradead.org
> Cc: alexey.brod...@synopsys.com; linux-ker...@vger.kernel.org;
> marc.zyng...@arm.com
> Subject: Re: [PATCH] ARCv2: intc: Disable all core interrupts by default
> 
> On 02/07/2017 03:04 PM, Yuriy Kolerov wrote:
> > The kernel emits a lot of warnings about unexpected IRQs when an
> > appropriate driver is not presented. It happens because all interrupts
> > in the core controller are enabled by default after reset. It would be
> > wise to keep all interrupts masked by default.
> >
> > Thus disable all local and common interrupts. If CPU consists of only
> > 1 core without IDU then it is necessary to disable all interrupts in
> > the core interrupt controller. If CPU contains IDU it means that there
> > are may be more than 1 cores and common interrupts (>= FIRST_EXT_IRQ)
> > must be disabled in IDU.
> 
> This is not elegant. core intc needs to do same thing to all interrupts coming
> in
> - irrespective of whether they are funneled via IDU or not.
> 
> 
> > Signed-off-by: Yuriy Kolerov 
> > ---
> >  arch/arc/kernel/intc-arcv2.c | 19 +++
> 
> [snip...]
> 
> > +
> > for (i = NR_EXCEPTIONS; i < irq_bcr.irqs + NR_EXCEPTIONS; i++) {
> > write_aux_reg(AUX_IRQ_SELECT, i);
> > write_aux_reg(AUX_IRQ_PRIORITY, ARCV2_IRQ_DEF_PRIO);
> > +
> > +   /*
> > +* If IDU exists then all common interrupts >= FIRST_EXT_IRQ
> > +* are masked by IDU thus disable only local interrupts (below
> > +* FIRST_EXT_IRQ). Otherwise disable all interrupts.
> > +*/
> > +   if (!mp.idu || i < FIRST_EXT_IRQ)
> > +   write_aux_reg(AUX_IRQ_ENABLE, 0);
> 
> So you seem to assume that anything > FIRST_EXT_IRQ is coming in via IDU
> which may not be case.
> external Interrupts can be wired to core directly - not via IDU
>  - 16 .. 23 are cpu private reserved irq
>  - 24 .. C are common irqs (via IDU)
>  - C + 1 .. N are cpu private external irqs
> 
> so better to disable all irq_bcr.irqs independent of how they are hooked up !

All IRQs >= 24 in ARC are marked as common interrupts and it is reasonable. It 
means that when "enable" or "mask" is called for hwirq < 24 then these 
functions are called on all cores. On the other hand these functions are called 
once only on 1 core for common interrupts. Then we have 3 cases:

1. We have UP and everything is easy. Just disable everything by default. When 
a driver comes up an appropriate IRQ is enable on single core.
2. We have SMP with IDU. We can enable/disable common interrupts in IDU and 
keep core interrupts enabled/unmasked by default. But what happens if a device 
is connected to one of the cores directly (is it even possible on SMP 
systems?)? Device Tree does not contain such information and the kernel does 
not know how it enable external IRQ for the specific core (as far as I know).
3. Assume that all core interrupts on all cores are disabled by default. When 
chained IRQ is created (IDU -> core intc) IDU automatically calls "enable" for 
an appropriate IRQ of core intc. But this "enable" is called only for 1 core so 
it is necessary to find a way to call "enable" on the rest of cores. I have a 
solution which solves this problem using "smp_call_function_single_async" in 
"enable" function of the core intc for IRQs >= 24 but I think that it may be 
overkill for such problem. Anyway I cannot find a solution for the same problem 
in other archs...

> -Vineet
___
linux-snps-arc mailing list
linux-snps-arc@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-snps-arc

Re: [PATCH 2/2] dmaengine: Add DW AXI DMAC driver

[Andy Shevchenko]

On Thu, 2017-02-09 at 13:58 +, Eugeniy Paltsev wrote:
  
> > > +static void axi_desc_put(struct axi_dma_desc *desc)
> > > +{
> > > + struct axi_dma_chan *chan = desc->chan;
> > > + struct dw_axi_dma *dw = chan->chip->dw;
> > > + struct axi_dma_desc *child, *_next;
> > > + unsigned int descs_put = 0;
> > > +
> > >  
> > > + if (unlikely(!desc))
> > > + return;
> > 
> > Would it be the case?
> 
> Yes, I checked the code - this NULL check is unnecessary, so I'll
> remove it.
> 
> Also about your previous question about likely/unlikely:
> I checked disassembly - gcc generates different code when I use likely
> and unlikely. So, I guess they are useful. 

They are, but in rare cases.

I assume you have read the following article and other material on LWN.

https://lwn.net/Articles/420019/
 
> > > + /* ASSERT: channel is idle */
> > > + if (axi_chan_is_hw_enable(chan))
> > > + dev_err(dchan2dev(dchan), "%s is non-idle!\n",
> > > + axi_chan_name(chan));
> > 
> > Yeah, as I said, dw_dmac is not a good example. And this one can be
> > managed by runtime PM I suppose.
> 
> See comment below.
> 
> > > > > +static inline bool axi_dma_is_sw_enable(struct axi_dma_chip
> 
> *chip)
> > > > > +{
> > > > > +   struct dw_axi_dma *dw = chip->dw;
> > > > > +   u32 i;
> > > > > +
> > > > > +   for (i = 0; i < dw->hdata->nr_channels; i++) {
> > > > > +   if (dw->chan[i].in_use)
> > > > 
> > > > Hmm... I know why we have such flag in dw_dmac, but I doubt it's
> > > > needed
> > > > in this driver. Just double check the need of it.
> > > 
> > > I use this flag to check state of channel (used now/unused) to
> 
> disable
> > > dmac if all channels are unused for now.
> > 
> >  
> > Okay, but wouldn't be easier to use runtime PM for that? You will
> > not
> > need any special counter and runtime PM will take case of
> > enabling/disabling the device.
> 
> Now in_use variable has several purposes - it is also used to mask
> interrupts from channels, which are not used - that is the reason I
> prefer leave it untouched.

And this is indeed a good argument to move it to runtime PM callbacks!

> Also all existing SoCs with this DMAC don't support power management -
> so there is no really profit from implementing PM.

Disabling IRQ, managing clocks or alike are quite suitable tasks for
runtime PM even if there is no actual *power* management done.

If unsure, ask other developers for their opinions: from Qualcomm,
nVidia, TI, etc.

Besides Vinod I would suggest Tony Lindgren, for example.

> > > + while (true) {
> > 
> > Usually it makes readability harder and rises a red flag to the
> > code.
> 
> I can write something like next code. Not sure is it looks better.
> --->8--
> while ((dst_len || dst_sg && dst_nents) &&
>        (src_len || src_sg && src_nents)) {

Just give a thought about it once more. It might be not easy, but the
result should be quite better than "while (true)" approach.

> > >   /* Manage transfer list (xfer_list) */
> > > + if (!first) {
> > > + first = desc;
> > > + } else {
> > > + list_add_tail(>xfer_list, 
> > > >  
> > > > xfer_list);
> > > 
> > > + write_desc_llp(prev, desc->vd.tx.phys |
> > > lms);
> > > + }
> > > + prev = desc;
> > > +
> > > + /* update the lengths and addresses for the next
> > > loop
> > > cycle */
> > > + dst_len -= xfer_len;
> > > + src_len -= xfer_len;
> > > + dst_adr += xfer_len;
> > > + src_adr += xfer_len;
> > > +
> > > + total_len += xfer_len;
> > 
> > I would suggest to leave this on caller.
> 
> I don't think it is a good idea. Caller doesn't know value of internal
> parameters like max data width and max block size, but we know these
> values and can organize LLI the most optimal way.
> And if the caller has already prepared suitable SG list there will be
> really little overhead, so I prefer leave this code unchanged.

The problem with DMA is a performance in case you need to supply new
chain in fastest possible way.

So, first rationale is to deduplicate the allocation and preparation of
SG list (especially in cases when you have not much nodes in it).

Besides above, caller is the best one who knows *how* to split data in
the *best* way taking into account *all possible limitations*.

That is a second point.

If you still disagree, I would leave it to other maintainers and
experienced engineers to share their opinions.

> > At some point, if no one
> > else
> > do this faster than me, I would like to introduce something like
> > struct
> > dma_parms per DMA channel to allow caller prepare SG list suitable
> > for the DMA device.
> 
> In my opinion it is driver responsibility, not the caller.

See above.

> + }
> > > +
> > > + chan->is_paused = true;
> > 
> > This is indeed property of channel.
> > That said, you may do a trick and use descriptor status for it.
> > You 

Re: [PATCH] net: phy: dp83867: Fall-back to default values of clock delay and FIFO depth

[Alexey Brodkin]

Hi Florian,

On Wed, 2017-02-08 at 10:20 -0800, Florian Fainelli wrote:
> On 02/08/2017 10:15 AM, David Miller wrote:
> > 
> > From: Alexey Brodkin 
> > Date: Mon,  6 Feb 2017 22:24:45 +0300
> > 
> > > 
> > > Given there're default values mentioned in the PHY datasheet
> > > fall-back gracefully to them instead of silently return an error
> > > through the whole call-chain.
> > > 
> > > This allows to use minimalistic description in DT if no special
> > > features are required.
> > > 
> > > Signed-off-by: Alexey Brodkin 
> > 
> > If these defaults are legitimate to use, then you should probably also
> > set them in the non-CONFIG_OF_MDIO case implementation of
> > dp83867_of_init().
> 
> IIRC this is what Alexey is doing already, in case of errors, instead of
> returning the error code, he changes the default values and does not
> propagate the error code.
> 
> Alexey, you are essentially making dp83867_of_init() impossible to fail
> (or nearly) now, even with invalid properties, so I agree with David
> here, you should probably have a function that runs after
> dp83867_of_init() whose job is to set default values, if none have been
> previously set through Device Tree.

But why do we need to return error code from dp83867_of_init()?
The point is this function doesn't do any hardware setup as well,
in fact it doesn't even reads anything from real hardware.

So we may indeed report an error to above callers (which is not super convenient
because the error comes many levels above without any error message making
understanding of the failure pretty complicated - go trace where it came from)
but what would it mean to a user? What do we want our user to do?
Set at least something in required properties? Then why don't we do that 
ourselves
right in the driver? Moreover since we're talking about defaults mentioned by
the PHY manufacturer we will just use "factory settings". Still allowing others 
to
override defaults with their .dts.

I agree with David that it really worth to do the same settings for 
non_CONFIG_OF_MDIO
case.

-Alexey
___
linux-snps-arc mailing list
linux-snps-arc@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-snps-arc

[PATCH] [ARC] Handle aux registers differences on ARC700 vs. HS38 cpus

[Vineet Gupta]

opcodes/
2017-02-09  Vineet Gupta 

* arc-regs.h: Distinguish some of the registers different on
ARC700 and HS38 cpus.

gas/
2017-02-09  Vineet Gupta 

* testsuite/gas/arc/st.d: Update for 0xe having a name now

Signed-off-by: Vineet Gupta 
---
 gas/ChangeLog  |  4 
 gas/testsuite/gas/arc/st.d |  2 +-
 opcodes/ChangeLog  |  5 +
 opcodes/arc-regs.h | 48 ++
 4 files changed, 46 insertions(+), 13 deletions(-)

diff --git a/gas/ChangeLog b/gas/ChangeLog
index d96516f93b57..3a4c2ce496ce 100644
--- a/gas/ChangeLog
+++ b/gas/ChangeLog
@@ -1,3 +1,7 @@
+2017-02-09  Vineet Gupta 
+
+   * testsuite/gas/arc/st.d: Update for 0xe having a name now
+
 2017-02-02  Maciej W. Rozycki  
 
* doc/as.texinfo (Overview): Select MIPS options for man page
diff --git a/gas/testsuite/gas/arc/st.d b/gas/testsuite/gas/arc/st.d
index 3f75d40c367f..6fe5b88aefa1 100644
--- a/gas/testsuite/gas/arc/st.d
+++ b/gas/testsuite/gas/arc/st.d
@@ -22,7 +22,7 @@ Disassembly of section .text:
   38:  1c04 1f80   st  0,\[r12,4\]
3c: R_ARC_32_ME .text\+0x40
   40:  212b 0080   sr  r1,\[r2\]
-  44:  216b 0380   sr  r1,\[0xe\]
+  44:  216b 0380   sr  r1,\[aux_irq_ctrl\]
   48:  262b 7040  03e8 sr  0x3e8,\[r1\]
   50:  262b 7080  0064 sr  0x64,\[r2\]
   58:  212b 0f80  2710 sr  r1,\[0x2710\]
diff --git a/opcodes/ChangeLog b/opcodes/ChangeLog
index 2185484ecec5..68ef4610240c 100644
--- a/opcodes/ChangeLog
+++ b/opcodes/ChangeLog
@@ -1,3 +1,8 @@
+2017-02-09  Vineet Gupta 
+
+   * arc-regs.h: Distinguish some of the registers different on
+   ARC700 and HS38 cpus.
+
 2017-02-03  Nick Clifton  
 
PR 21096
diff --git a/opcodes/arc-regs.h b/opcodes/arc-regs.h
index ae1f4c66f66f..0bdbd92a949c 100644
--- a/opcodes/arc-regs.h
+++ b/opcodes/arc-regs.h
@@ -19,8 +19,10 @@
along with this program; if not, write to the Free Software Foundation,
Inc., 51 Franklin Street - Fifth Floor, Boston, MA 02110-1301, USA.  */
 
-DEF (0x0,   ARC_OPCODE_ARCALL, NONE, status)
-DEF (0x1,   ARC_OPCODE_ARCALL, NONE, semaphore)
+DEF (0x0,   ARC_OPCODE_ARC600, NONE, status)
+DEF (0x0,   ARC_OPCODE_ARC700, NONE, status)
+DEF (0x1,   ARC_OPCODE_ARC600, NONE, semaphore)
+DEF (0x1,   ARC_OPCODE_ARC700, NONE, semaphore)
 DEF (0x2,   ARC_OPCODE_ARCALL, NONE, lp_start)
 DEF (0x3,   ARC_OPCODE_ARCALL, NONE, lp_end)
 DEF (0x4,   ARC_OPCODE_ARCALL, NONE, identity)
@@ -30,8 +32,16 @@ DEF (0x7,   ARC_OPCODE_ARCALL, NONE, adcr)
 DEF (0x8,   ARC_OPCODE_ARCALL, NONE, apcr)
 DEF (0x9,   ARC_OPCODE_ARCALL, NONE, acr)
 DEF (0xa,   ARC_OPCODE_ARCALL, NONE, status32)
-DEF (0xb,   ARC_OPCODE_ARCALL, NONE, status32_l1)
-DEF (0xc,   ARC_OPCODE_ARCALL, NONE, status32_l2)
+DEF (0xb,   ARC_OPCODE_ARC600, NONE, status32_l1)
+DEF (0xb,   ARC_OPCODE_ARC700, NONE, status32_l1)
+DEF (0xb,   ARC_OPCODE_ARCv2EM, NONE, status32_p0)
+DEF (0xb,   ARC_OPCODE_ARCv2HS, NONE, status32_p0)
+DEF (0xc,   ARC_OPCODE_ARC600, NONE, status32_l2)
+DEF (0xc,   ARC_OPCODE_ARC700, NONE, status32_l2)
+DEF (0xd,   ARC_OPCODE_ARCv2EM, NONE, aux_user_sp)
+DEF (0xd,   ARC_OPCODE_ARCv2HS, NONE, aux_user_sp)
+DEF (0xe,   ARC_OPCODE_ARCv2EM, NONE, aux_irq_ctrl)
+DEF (0xe,   ARC_OPCODE_ARCv2HS, NONE, aux_irq_ctrl)
 DEF (0xf,   ARC_OPCODE_ARCALL, NONE, bpu_flush)
 DEF (0x10,  ARC_OPCODE_ARCALL, NONE, ivic)
 DEF (0x10,  ARC_OPCODE_ARCALL, NONE, ic_ivic)
@@ -87,7 +97,10 @@ DEF (0x3d,  ARC_OPCODE_NONE,   NONE, burstval)
 DEF (0x40,  ARC_OPCODE_ARCALL, NONE, xtp_newval)
 DEF (0x41,  ARC_OPCODE_ARCALL, NONE, aux_macmode)
 DEF (0x42,  ARC_OPCODE_ARCALL, NONE, lsp_newval)
-DEF (0x43,  ARC_OPCODE_ARCALL, NONE, aux_irq_lv12)
+DEF (0x43,  ARC_OPCODE_ARC600, NONE, aux_irq_lv12)
+DEF (0x43,  ARC_OPCODE_ARC700, NONE, aux_irq_lv12)
+DEF (0x43,  ARC_OPCODE_ARCv2EM, NONE, aux_irq_act)
+DEF (0x43,  ARC_OPCODE_ARCv2HS, NONE, aux_irq_act)
 DEF (0x44,  ARC_OPCODE_ARCALL, NONE, aux_xmac0)
 DEF (0x45,  ARC_OPCODE_ARCALL, NONE, aux_xmac1)
 DEF (0x46,  ARC_OPCODE_ARCALL, NONE, aux_xmac2)
@@ -204,9 +217,14 @@ DEF (0x102, ARC_OPCODE_ARCALL, NONE, limit1)
 DEF (0x103, ARC_OPCODE_ARCALL, NONE, timer_xx)
 DEF (0x120, ARC_OPCODE_ARCALL, NONE, arcangel_periph_xx)
 DEF (0x140, ARC_OPCODE_ARCALL, NONE, periph_xx)
-DEF (0x200, ARC_OPCODE_ARCALL, NONE, aux_irq_lev)
+DEF (0x200, ARC_OPCODE_ARC600, NONE, aux_irq_lev)
+DEF (0x200, ARC_OPCODE_ARC700, NONE, aux_irq_lev)
+DEF (0x200, ARC_OPCODE_ARCv2EM, NONE, irq_priority_pending)
+DEF (0x200, ARC_OPCODE_ARCv2HS, NONE, irq_priority_pending)
 DEF (0x201, ARC_OPCODE_ARCALL, NONE, aux_irq_hint)
 DEF (0x202, ARC_OPCODE_ARCALL, NONE, aux_inter_core_interrupt)
+DEF (0x206, ARC_OPCODE_ARCv2EM, NONE, irq_priority)
+DEF (0x206, ARC_OPCODE_ARCv2HS, 

Re: [PATCH 2/2] dmaengine: Add DW AXI DMAC driver

[Eugeniy Paltsev]

Thanks for response.
My comments are given inline below.

On Wed, 2017-01-25 at 19:25 +0200, Andy Shevchenko wrote:
> On Wed, 2017-01-25 at 18:34 +0300, Eugeniy Paltsev wrote:
> > 
> > This patch adds support for the DW AXI DMAC controller.
> > 
> > DW AXI DMAC is a part of upcoming development board from Synopsys.
> > 
> > In this driver implementation only DMA_MEMCPY and DMA_SG transfers
> > are supported.
> > 
> Few more comments on top of not addressed/answered yet.
> 
> > 
> > +static inline void axi_chan_disable(struct axi_dma_chan *chan)
> > +{
> > +   u32 val;
> > +
> > +   val = axi_dma_ioread32(chan->chip, DMAC_CHEN);
> > +   val &= ~(BIT(chan->id) << DMAC_CHAN_EN_SHIFT);
> > +   val |=  (BIT(chan->id) << DMAC_CHAN_EN_WE_SHIFT);
> > +   axi_dma_iowrite32(chan->chip, DMAC_CHEN, val);
> > +}
> > +
> > +static inline void axi_chan_enable(struct axi_dma_chan *chan)
> > +{
> > +   u32 val;
> > +
> > +   val = axi_dma_ioread32(chan->chip, DMAC_CHEN);
> > 
> > +   val |= (BIT(chan->id) << DMAC_CHAN_EN_SHIFT |
> > +   BIT(chan->id) << DMAC_CHAN_EN_WE_SHIFT);
> > 
> Redundant parens.
Will be fixed.

> > 
> > +   axi_dma_iowrite32(chan->chip, DMAC_CHEN, val);
> > +}
> 
> > 
> > +static void axi_desc_put(struct axi_dma_desc *desc)
> > +{
> > +   struct axi_dma_chan *chan = desc->chan;
> > +   struct dw_axi_dma *dw = chan->chip->dw;
> > +   struct axi_dma_desc *child, *_next;
> > +   unsigned int descs_put = 0;
> > +
> > 
> > +   if (unlikely(!desc))
> > +   return;
> Would it be the case?
Yes, I checked the code - this NULL check is unnecessary, so I'll
remove it.

Also about your previous question about likely/unlikely:
I checked disassembly - gcc generates different code when I use likely
and unlikely. So, I guess they are useful. 

> > 
> > +static void dma_chan_issue_pending(struct dma_chan *dchan)
> > +{
> > +   struct axi_dma_chan *chan = dchan_to_axi_dma_chan(dchan);
> > +   unsigned long flags;
> > +
> > 
> > +   dev_dbg(dchan2dev(dchan), "%s: %s\n", __func__,
> > axi_chan_name(chan));
> Messages like this kinda redundant.
> Either you use function tracer and see them anyway, or you are using
> Dynamic Debug, which may include function name.
> 
> Basically you wrote an equivalent to something like
> 
> dev_dbg(dev, "%s\n", channame);
Agreed. I'll remove dev_dbg from here because I also have it in 
axi_chan_start_first_queued.

> > 
> > +
> > +   spin_lock_irqsave(>vc.lock, flags);
> > +   if (vchan_issue_pending(>vc))
> > +   axi_chan_start_first_queued(chan);
> > +   spin_unlock_irqrestore(>vc.lock, flags);
> ...and taking into account the function itself one might expect
> something useful printed in _start_first_queued().
> 
> For some cases there is also dev_vdbg().
> 
> > 
> > +}
> > +
> > +static int dma_chan_alloc_chan_resources(struct dma_chan *dchan)
> > +{
> > +   struct axi_dma_chan *chan = dchan_to_axi_dma_chan(dchan);
> > +
> > +   /* ASSERT: channel is idle */
> > +   if (axi_chan_is_hw_enable(chan)) {
> > +   dev_err(chan2dev(chan), "%s is non-idle!\n",
> > +   axi_chan_name(chan));
> > +   return -EBUSY;
> > +   }
> > +
> > 
> > +   dev_dbg(dchan2dev(dchan), "%s: allocating\n",
> > axi_chan_name(chan));
> Can you try to enable DMADEVICES_DEBUG and VDEBUG and see what is
> useful
> and what is not?
> 
> Give a chance to function tracer as well.
Yep, this dev_dbg is redundant, so I'll remove it.

> > 
> > +static void dma_chan_free_chan_resources(struct dma_chan *dchan)
> > +{
> > +   struct axi_dma_chan *chan = dchan_to_axi_dma_chan(dchan);
> > +
> > 
> > 
> > +   /* ASSERT: channel is idle */
> > +   if (axi_chan_is_hw_enable(chan))
> > +   dev_err(dchan2dev(dchan), "%s is non-idle!\n",
> > +   axi_chan_name(chan));
> Yeah, as I said, dw_dmac is not a good example. And this one can be
> managed by runtime PM I suppose.
See comment below.

> >> > +static inline bool axi_dma_is_sw_enable(struct axi_dma_chip
*chip)
> >> > +{
> >> > +   struct dw_axi_dma *dw = chip->dw;
> >> > +   u32 i;
> >> > +
> >> > +   for (i = 0; i < dw->hdata->nr_channels; i++) {
> >> > +   if (dw->chan[i].in_use)
> >> Hmm... I know why we have such flag in dw_dmac, but I doubt it's
> >> needed
> >> in this driver. Just double check the need of it.
> > I use this flag to check state of channel (used now/unused) to
disable
> > dmac if all channels are unused for now.
> 
> Okay, but wouldn't be easier to use runtime PM for that? You will not
> need any special counter and runtime PM will take case of
> enabling/disabling the device.

Now in_use variable has several purposes - it is also used to mask
interrupts from channels, which are not used - that is the reason I
prefer leave it untouched.
Also all existing SoCs with this DMAC don't support power management -
so there is no really profit from implementing PM.

> > 
> > +
> > +/* TODO: REVISIT: how we should choose AXI master for mem-to-mem
> > transfer? */
> Read datasheet