[U-Boot,1/5] drivers: dma: Enable DMA-330 driver support

From: Tien Fong Chee <tien.fong.chee@intel.com>

Enable DMAC driver support for DMA-330 controller.
The driver is also compatible to PL330 product.

Signed-off-by: Tien Fong Chee <tien.fong.chee@intel.com>
---
 drivers/dma/Kconfig  |    9 +-
 drivers/dma/Makefile |    1 +
 drivers/dma/dma330.c | 1514 ++++++++++++++++++++++++++++++++++++++++++++++++++
 include/dma330.h     |  136 +++++
 4 files changed, 1659 insertions(+), 1 deletion(-)
 create mode 100644 drivers/dma/dma330.c
 create mode 100644 include/dma330.h

Hi Tien,

On 31 May 2018 at 02:08,  <tien.fong.chee@intel.com> wrote:
> From: Tien Fong Chee <tien.fong.chee@intel.com>
>
> Enable DMAC driver support for DMA-330 controller.
> The driver is also compatible to PL330 product.
>
> Signed-off-by: Tien Fong Chee <tien.fong.chee@intel.com>
> ---
>  drivers/dma/Kconfig  |    9 +-
>  drivers/dma/Makefile |    1 +
>  drivers/dma/dma330.c | 1514 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  include/dma330.h     |  136 +++++
>  4 files changed, 1659 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/dma/dma330.c
>  create mode 100644 include/dma330.h
>
> diff --git a/drivers/dma/Kconfig b/drivers/dma/Kconfig
> index 4ee6afa..6e77e07 100644
> --- a/drivers/dma/Kconfig
> +++ b/drivers/dma/Kconfig
> @@ -2,7 +2,7 @@ menu "DMA Support"
>
>  config DMA
>         bool "Enable Driver Model for DMA drivers"
> -       depends on DM
> +       depends on DM || SPL_DM
>         help
>           Enable driver model for DMA. DMA engines can do
>           asynchronous data transfers without involving the host
> @@ -34,4 +34,11 @@ config APBH_DMA_BURST8
>
>  endif
>
> +config DMA330_DMA
> +       bool "PL330/DMA-330 DMA Controller(DMAC) driver"
> +       depends on DMA
> +       help
> +        Enable the DMA controller driver for both PL330 and
> +        DMA-330 products.
> +
>  endmenu # menu "DMA Support"
> diff --git a/drivers/dma/Makefile b/drivers/dma/Makefile
> index 4eaef8a..bfad0dd 100644
> --- a/drivers/dma/Makefile
> +++ b/drivers/dma/Makefile
> @@ -11,3 +11,4 @@ obj-$(CONFIG_FSL_DMA) += fsl_dma.o
>  obj-$(CONFIG_TI_KSNAV) += keystone_nav.o keystone_nav_cfg.o
>  obj-$(CONFIG_TI_EDMA3) += ti-edma3.o
>  obj-$(CONFIG_DMA_LPC32XX) += lpc32xx_dma.o
> +obj-$(CONFIG_DMA330_DMA) += dma330.o
> diff --git a/drivers/dma/dma330.c b/drivers/dma/dma330.c
> new file mode 100644
> index 0000000..66575d8
> --- /dev/null
> +++ b/drivers/dma/dma330.c
> @@ -0,0 +1,1514 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2018 Intel Corporation <www.intel.com>
> + */
> +
> +#include <common.h>
> +#include <asm/io.h>
> +#include <dma330.h>
> +#include <dm.h>
> +#include <fdtdec.h>
> +#include <wait_bit.h>
> +#include <linux/unaligned/access_ok.h>
> +
> +/* Register and Bit field Definitions */
> +
> +/* DMA Status */
> +#define DS                     0x0
> +#define DS_ST_STOP             0x0
> +#define DS_ST_EXEC             0x1
> +#define DS_ST_CMISS            0x2
> +#define DS_ST_UPDTPC           0x3
> +#define DS_ST_WFE              0x4
> +#define DS_ST_ATBRR            0x5
> +#define DS_ST_QBUSY            0x6
> +#define DS_ST_WFP              0x7
> +#define DS_ST_KILL             0x8
> +#define DS_ST_CMPLT            0x9
> +#define DS_ST_FLTCMP           0xe
> +#define DS_ST_FAULT            0xf

It is possible to use enum for some of these?

enum {
   DS             = 0,
   DS_ST_STOP,
 ...
}

> +
> +/* DMA Program Count register */
> +#define DPC                    0x4
> +/* Interrupt Enable register */
> +#define INTEN                  0x20
> +/* event-Interrupt Raw Status register */
> +#define ES                     0x24
> +/* Interrupt Status register */
> +#define INTSTATUS              0x28
> +/* Interrupt Clear register */
> +#define INTCLR                 0x2c
> +/* Fault Status DMA Manager register */
> +#define FSM                    0x30
> +/* Fault Status DMA Channel register */
> +#define FSC                    0x34
> +/* Fault Type DMA Manager register */
> +#define FTM                    0x38
> +
> +/* Fault Type DMA Channel register */
> +#define _FTC                   0x40
> +#define FTC(n)                 (_FTC + (n) * 0x4)
> +
> +/* Channel Status register */
> +#define _CS                    0x100
> +#define CS(n)                  (_CS + (n) * 0x8)
> +#define CS_CNS                 BIT(21)
> +
> +/* Channel Program Counter register */
> +#define _CPC                   0x104
> +#define CPC(n)                 (_CPC + (n) * 0x8)
> +
> +/* Source Address register */
> +#define _SA                    0x400
> +#define SA(n)                  (_SA + (n) * 0x20)
> +
> +/* Destination Address register */
> +#define _DA                    0x404
> +#define DA(n)                  (_DA + (n) * 0x20)
> +
> +/* Channel Control register */
> +#define _CC                    0x408
> +#define CC(n)                  (_CC + (n) * 0x20)
> +
> +/* Channel Control register (CCR) Setting */
> +#define CC_SRCINC              BIT(0)
> +#define CC_DSTINC              BIT(14)
> +#define CC_SRCPRI              BIT(8)
> +#define CC_DSTPRI              BIT(22)
> +#define CC_SRCNS               BIT(9)
> +#define CC_DSTNS               BIT(23)
> +#define CC_SRCIA               BIT(10)
> +#define CC_DSTIA               BIT(24)
> +#define CC_SRCBRSTLEN_SHFT     4
> +#define CC_DSTBRSTLEN_SHFT     18
> +#define CC_SRCBRSTSIZE_SHFT    1
> +#define CC_DSTBRSTSIZE_SHFT    15
> +#define CC_SRCCCTRL_SHFT       11
> +#define CC_SRCCCTRL_MASK       0x7

Can you make the mask a shifted mask? Then it is easier to use in
clrsetbits_le32(), for example.

e.g.

#define CC_SRCCCTRL_MASK (7 << CC_SRCCCTRL_SHFT)

> +#define CC_DSTCCTRL_SHFT       25
> +#define CC_DRCCCTRL_MASK       0x7
> +#define CC_SWAP_SHFT           28
> +
> +/* Loop Counter 0 register */
> +#define _LC0                   0x40c
> +#define LC0(n)                 (_LC0 + (n) * 0x20)
> +
> +/* Loop Counter 1 register */
> +#define _LC1                   0x410
> +#define LC1(n)                 (_LC1 + (n) * 0x20)
> +
> +/* Debug Status register */
> +#define DBGSTATUS              0xd00
> +#define DBG_BUSY               BIT(0)
> +
> +/* Debug Command register */
> +#define DBGCMD                 0xd04
> +/* Debug Instruction 0 register */
> +#define DBGINST0               0xd08
> +/* Debug Instruction 1 register */
> +#define DBGINST1               0xd0c
> +
> +/* Configuration register */
> +#define CR0                    0xe00
> +#define CR1                    0xe04
> +#define CR2                    0xe08
> +#define CR3                    0xe0c
> +#define CR4                    0xe10
> +#define CRD                    0xe14
> +
> +/* Peripheral Identification register */
> +#define PERIPH_ID              0xfe0
> +/* Component Identification register */
> +#define PCELL_ID               0xff0
> +
> +/* Configuration register value */
> +#define CR0_PERIPH_REQ_SET     BIT(0)
> +#define CR0_BOOT_EN_SET                BIT(1)
> +#define CR0_BOOT_MAN_NS                BIT(2)
> +#define CR0_NUM_CHANS_SHIFT    4
> +#define CR0_NUM_CHANS_MASK     0x7
> +#define CR0_NUM_PERIPH_SHIFT   12
> +#define CR0_NUM_PERIPH_MASK    0x1f
> +#define CR0_NUM_EVENTS_SHIFT   17
> +#define CR0_NUM_EVENTS_MASK    0x1f
> +
> +/* Configuration register value */
> +#define CR1_ICACHE_LEN_SHIFT           0
> +#define CR1_ICACHE_LEN_MASK            0x7
> +#define CR1_NUM_ICACHELINES_SHIFT      4
> +#define CR1_NUM_ICACHELINES_MASK       0xf
> +
> +/* Configuration register value */
> +#define CRD_DATA_WIDTH_SHIFT   0
> +#define CRD_DATA_WIDTH_MASK    0x7
> +#define CRD_WR_CAP_SHIFT       4
> +#define CRD_WR_CAP_MASK                0x7
> +#define CRD_WR_Q_DEP_SHIFT     8
> +#define CRD_WR_Q_DEP_MASK      0xf
> +#define CRD_RD_CAP_SHIFT       12
> +#define CRD_RD_CAP_MASK                0x7
> +#define CRD_RD_Q_DEP_SHIFT     16
> +#define CRD_RD_Q_DEP_MASK      0xf
> +#define CRD_DATA_BUFF_SHIFT    20
> +#define CRD_DATA_BUFF_MASK     0x3ff
> +
> +/* Microcode opcode value */
> +#define CMD_DMAADDH            0x54
> +#define CMD_DMAEND             0x00
> +#define CMD_DMAFLUSHP          0x35
> +#define CMD_DMAGO              0xa0
> +#define CMD_DMALD              0x04
> +#define CMD_DMALDP             0x25
> +#define CMD_DMALP              0x20
> +#define CMD_DMALPEND           0x28
> +#define CMD_DMAKILL            0x01
> +#define CMD_DMAMOV             0xbc
> +#define CMD_DMANOP             0x18
> +#define CMD_DMARMB             0x12
> +#define CMD_DMASEV             0x34
> +#define CMD_DMAST              0x08
> +#define CMD_DMASTP             0x29
> +#define CMD_DMASTZ             0x0c
> +#define CMD_DMAWFE             0x36
> +#define CMD_DMAWFP             0x30
> +#define CMD_DMAWMB             0x13
> +
> +/* the size of opcode plus opcode required settings */
> +#define SZ_DMAADDH             3
> +#define SZ_DMAEND              1
> +#define SZ_DMAFLUSHP           2
> +#define SZ_DMALD               1
> +#define SZ_DMALDP              2
> +#define SZ_DMALP               2
> +#define SZ_DMALPEND            2
> +#define SZ_DMAKILL             1
> +#define SZ_DMAMOV              6
> +#define SZ_DMANOP              1
> +#define SZ_DMARMB              1
> +#define SZ_DMASEV              2
> +#define SZ_DMAST               1
> +#define SZ_DMASTP              2
> +#define SZ_DMASTZ              1
> +#define SZ_DMAWFE              2
> +#define SZ_DMAWFP              2
> +#define SZ_DMAWMB              1
> +#define SZ_DMAGO               6
> +
> +/* Use this _only_ to wait on transient states */
> +#define UNTIL(t, s)    do { \
> +                               WATCHDOG_RESET(); \
> +                       } while (!(dma330_getstate(t) & (s)))
> +
> +/* debug message printout */
> +#ifdef DEBUG
> +#define DMA330_DBGCMD_DUMP(off, x...)  do { \
> +                                               printf("%x bytes:", off); \
> +                                               printf(x); \
> +                                               WATCHDOG_RESET(); \
> +                                       } while (0)
> +#else
> +#define DMA330_DBGCMD_DUMP(off, x...)  do {} while (0)
> +#endif

Can DMA330_DBGCMD_DUMP be a static function, and lower case? It's only
used in one file, right?

> +
> +/* Enum declaration */

We know that :-)

Can you please add comments explaining what these enums are for, and
what the values mean?

> +enum dmamov_dst {
> +       SAR = 0,
> +       CCR,
> +       DAR,
> +};
> +
> +enum dma330_dst {
> +       SRC = 0,
> +       DST,
> +};
> +
> +enum dma330_cond {
> +       SINGLE,
> +       BURST,
> +       ALWAYS,
> +};
> +
> +/* Structure will be used by _emit_lpend function */
> +struct _arg_lpend {
> +       enum dma330_cond cond;
> +       int forever;
> +       u32 loop;
> +       u8 bjump;
> +};
> +
> +/* Structure will be used by _emit_go function */
> +struct _arg_go {

Please drop the _ on these struct names, and document the members here.

> +       u8 chan;
> +       u32 addr;
> +       u32 ns;
> +};
> +
> +/*
> + * _emit_end - Add opcode DMAEND into microcode (end).
> + *
> + * @buf: The buffer which stored the microcode program.
> + *
> + * Return: Size of opcode.
> + */
> +static inline u32 _emit_end(u8 buf[])

Why is everything inline? The compiler will do it automatically when
there is only one caller. Other than that, it just increases code size
I think.

> +{
> +       buf[0] = CMD_DMAEND;
> +       DMA330_DBGCMD_DUMP(SZ_DMAEND, "\tDMAEND\n");
> +       return SZ_DMAEND;
> +}
> +
> +/*
> + * _emit_flushp - Add opcode DMAFLUSHP into microcode (flush peripheral).
> + *
> + * @buf -> The buffer which stored the microcode program.
> + * @peri -> Peripheral ID as listed in DMA NPP.
> + *
> + * Return: Size of opcode.
> + */
> +static inline u32 _emit_flushp(u8 buf[], u8 peri)
> +{
> +       u8 *buffer = buf;
> +
> +       buffer[0] = CMD_DMAFLUSHP;
> +       peri &= 0x1f;
> +       peri <<= 3;

These should be enums too so we don't have open-coded values in the
code, Please check throughout.

> +       buffer[1] = peri;
> +       DMA330_DBGCMD_DUMP(SZ_DMAFLUSHP, "\tDMAFLUSHP %u\n", peri >> 3);
> +       return SZ_DMAFLUSHP;
> +}
> +
> +/**
> + * _emit_ld - Add opcode DMALD into microcode (load).
> + *
> + * @buf: The buffer which stored the microcode program.
> + * @cond: Execution criteria such as single, burst or always.
> + *
> + * Return: Size of opcode.
> + */
> +static inline u32 _emit_ld(u8 buf[], enum dma330_cond cond)
> +{
> +       buf[0] = CMD_DMALD;
> +       if (cond == SINGLE)
> +               buf[0] |= (0 << 1) | (1 << 0);

More open-coded things

> +       else if (cond == BURST)
> +               buf[0] |= (1 << 1) | (1 << 0);
> +       DMA330_DBGCMD_DUMP(SZ_DMALD, "\tDMALD%c\n",
> +                         cond == SINGLE ? 'S' : (cond == BURST ? 'B' : 'A'));
> +       return SZ_DMALD;
> +}
> +
> +/**
> + * _emit_lp - Add opcode DMALP into microcode (loop).
> + *
> + * @buf: The buffer which stored the microcode program.
> + * @loop: Selection of using counter 0 or 1  (valid value 0 or 1).
> + * @cnt: number of iteration (valid value 1-256).
> + *
> + * Return: Size of opcode.
> + */
> +static inline u32 _emit_lp(u8 buf[], u32 loop, u32 cnt)
> +{
> +       u8 *buffer = buf;
> +
> +       buffer[0] = CMD_DMALP;
> +       if (loop)
> +               buffer[0] |= (1 << 1);
> +       /* DMAC increments by 1 internally */
> +       cnt--;
> +       buffer[1] = cnt;
> +       DMA330_DBGCMD_DUMP(SZ_DMALP, "\tDMALP_%c %u\n", loop ? '1' : '0', cnt);
> +       return SZ_DMALP;
> +}
> +
> +/**
> + * _emit_lpend - Add opcode DMALPEND into microcode (loop end).
> + *
> + * @buf: The buffer which stored the microcode program.
> + * @arg: Structure _arg_lpend which contain all needed info.
> + *     arg->cond -> Execution criteria such as single, burst or always
> + *     arg->forever -> Forever loop? used if DMALPFE started the loop
> + *     arg->bjump -> Backwards jump (relative location of
> + *                   1st instruction in the loop.

The members of the struct should be documented in the struct above, not here.

> + *
> + * Return: Size of opcode.
> + */
> +static inline u32 _emit_lpend(u8 buf[], const struct _arg_lpend *arg)
> +{
> +       u8 *buffer = buf;
> +       enum dma330_cond cond = arg->cond;
> +       int forever = arg->forever;
> +       u32 loop = arg->loop;
> +       u8 bjump = arg->bjump;

Why have these? Can you not use arg-bjump, etc., directly?

> +
> +       buffer[0] = CMD_DMALPEND;
> +       if (loop)
> +               buffer[0] |= (1 << 2);

More open-coded things

> +       if (!forever)
> +               buffer[0] |= (1 << 4);
> +       if (cond == SINGLE)
> +               buffer[0] |= (0 << 1) | (1 << 0);
> +       else if (cond == BURST)
> +               buffer[0] |= (1 << 1) | (1 << 0);
> +
> +       buffer[1] = bjump;
> +       DMA330_DBGCMD_DUMP(SZ_DMALPEND, "\tDMALP%s%c_%c bjmpto_%x\n",
> +                         forever ? "FE" : "END",
> +                         cond == SINGLE ? 'S' : (cond == BURST ? 'B' : 'A'),
> +                         loop ? '1' : '0',
> +                         bjump);
> +       return SZ_DMALPEND;
> +}
> +
> +/**
> + * _emit_kill - Add opcode DMAKILL into microcode (kill).
> + *
> + * @buf: The buffer which stored the microcode program.
> + *
> + * Return: Size of opcode.
> + */
> +static inline u32 _emit_kill(u8 buf[])
> +{
> +       buf[0] = CMD_DMAKILL;
> +       return SZ_DMAKILL;
> +}
> +
> +/**
> + * _emit_mov - Add opcode DMAMOV into microcode (move).
> + *
> + * @buf: The buffer which stored the microcode program.
> + * @dst: Destination register (valid value SAR[0b000], DAR[0b010],
> + *      or CCR[0b001]).
> + * @val: 32bit value that to be written into destination register.
> + *
> + * Return: Size of opcode.
> + */
> +static inline u32 _emit_mov(u8 buf[], enum dmamov_dst dst, u32 val)
> +{
> +       u8 *buffer = buf;
> +
> +       buffer[0] = CMD_DMAMOV;
> +       buffer[1] = dst;
> +       buffer[2] = val & 0xFF;
> +       buffer[3] = (val >> 8) & 0xFF;
> +       buffer[4] = (val >> 16) & 0xFF;
> +       buffer[5] = (val >> 24) & 0xFF;

Can you use put_unaligned_le32() ? Also check the same thing below.


> +       DMA330_DBGCMD_DUMP(SZ_DMAMOV, "\tDMAMOV %s 0x%x\n",
> +                         dst == SAR ? "SAR" : (dst == DAR ? "DAR" : "CCR"),
> +                          val);
> +       return SZ_DMAMOV;
> +}
> +
> +/**
> + * _emit_nop - Add opcode DMANOP into microcode (no operation).
> + *
> + * @buf: The buffer which stored the microcode program.
> + *
> + * Return: Size of opcode.
> + */
> +static inline u32 _emit_nop(u8 buf[])
> +{
> +       buf[0] = CMD_DMANOP;
> +       DMA330_DBGCMD_DUMP(SZ_DMANOP, "\tDMANOP\n");

please add a blank line before 'return'

> +       return SZ_DMANOP;
> +}
> +
> +/**
> + * _emit_rmb - Add opcode DMARMB into microcode (read memory barrier).
> + *
> + * @buf: The buffer which stored the microcode program.
> + *
> + * Return: Size of opcode.
> + */
> +static inline u32 _emit_rmb(u8 buf[])
> +{
> +       buf[0] = CMD_DMARMB;
> +       DMA330_DBGCMD_DUMP(SZ_DMARMB, "\tDMARMB\n");
> +       return SZ_DMARMB;
> +}
> +
> +/**
> + * _emit_sev - Add opcode DMASEV into microcode (send event).
> + *
> + * @buf: The buffer which stored the microcode program.
> + * @ev: The event number (valid 0 - 31).
> + *
> + * Return: Size of opcode.
> + */
> +static inline u32 _emit_sev(u8 buf[], u8 ev)
> +{
> +       u8 *buffer = buf;
> +
> +       buffer[0] = CMD_DMASEV;
> +       ev &= 0x1f;
> +       ev <<= 3;
> +       buffer[1] = ev;
> +       DMA330_DBGCMD_DUMP(SZ_DMASEV, "\tDMASEV %u\n", ev >> 3);
> +       return SZ_DMASEV;
> +}
> +
> +/**
> + * _emit_st - Add opcode DMAST into microcode (store).
> + *
> + * @buf: The buffer which stored the microcode program.
> + * @cond: Execution criteria such as single, burst or always.
> + *
> + * Return: Size of opcode.
> + */
> +static inline u32 _emit_st(u8 buf[], enum dma330_cond cond)
> +{
> +       buf[0] = CMD_DMAST;
> +       if (cond == SINGLE)
> +               buf[0] |= (0 << 1) | (1 << 0);
> +       else if (cond == BURST)
> +               buf[0] |= (1 << 1) | (1 << 0);
> +
> +       DMA330_DBGCMD_DUMP(SZ_DMAST, "\tDMAST%c\n",
> +               cond == SINGLE ? 'S' : (cond == BURST ? 'B' : 'A'));
> +       return SZ_DMAST;
> +}
> +
> +/**
> + * _emit_stp - Add opcode DMASTP into microcode (store and notify peripheral).
> + *
> + * @buf: The buffer which stored the microcode program.
> + * @cond: Execution criteria such as single, burst or always.
> + * @peri: Peripheral ID as listed in DMA NPP.
> + *
> + * Return: Size of opcode.
> + */
> +static inline u32 _emit_stp(u8 buf[], enum dma330_cond cond, u8 peri)
> +{
> +       u8 *buffer = buf;
> +
> +       buffer[0] = CMD_DMASTP;
> +       if (cond == BURST)
> +               buf[0] |= (1 << 1);
> +       peri &= 0x1f;
> +       peri <<= 3;
> +       buffer[1] = peri;
> +       DMA330_DBGCMD_DUMP(SZ_DMASTP, "\tDMASTP%c %u\n",
> +               cond == SINGLE ? 'S' : 'B', peri >> 3);
> +       return SZ_DMASTP;
> +}
> +
> +/**
> + * _emit_stz - Add opcode DMASTZ into microcode (store zeros).
> + *
> + * @buf -> The buffer which stored the microcode program.
> + *
> + * Return: Size of opcode.
> + */
> +static inline u32 _emit_stz(u8 buf[])
> +{
> +       buf[0] = CMD_DMASTZ;
> +       DMA330_DBGCMD_DUMP(SZ_DMASTZ, "\tDMASTZ\n");
> +       return SZ_DMASTZ;
> +}
> +
> +/**
> + * _emit_wfp - Add opcode DMAWFP into microcode (wait for peripheral).
> + *
> + * @buf: The buffer which stored the microcode program.
> + * @cond: Execution criteria such as single, burst or always.
> + * @peri: Peripheral ID as listed in DMA NPP.
> + *
> + * Return: Size of opcode.
> + */
> +static inline u32 _emit_wfp(u8 buf[], enum dma330_cond cond, u8 peri)
> +{
> +       u8 *buffer = buf;
> +
> +       buffer[0] = CMD_DMAWFP;
> +       if (cond == SINGLE)
> +               buffer[0] |= (0 << 1) | (0 << 0);
> +       else if (cond == BURST)
> +               buffer[0] |= (1 << 1) | (0 << 0);
> +       else
> +               buffer[0] |= (0 << 1) | (1 << 0);

Perhaps these three values are the same for each command? If so, the
three expressions (0, 1, 2) could go in an enum.

> +
> +       peri &= 0x1f;
> +       peri <<= 3;
> +       buffer[1] = peri;
> +       DMA330_DBGCMD_DUMP(SZ_DMAWFP, "\tDMAWFP%c %u\n",
> +               cond == SINGLE ? 'S' : (cond == BURST ? 'B' : 'P'), peri >> 3);
> +       return SZ_DMAWFP;
> +}
> +
> +/**
> + * _emit_wmb - Add opcode DMAWMB into microcode (write memory barrier).
> + *
> + * @buf: The buffer which stored the microcode program.
> + *
> + * Return: Size of opcode.
> + */
> +static inline u32 _emit_wmb(u8 buf[])
> +{
> +       buf[0] = CMD_DMAWMB;
> +       DMA330_DBGCMD_DUMP(SZ_DMAWMB, "\tDMAWMB\n");
> +       return SZ_DMAWMB;
> +}
> +
> +/**
> + * _emit_go - Add opcode DMALGO into microcode (go).
> + *
> + * @buf: The buffer which stored the microcode program.
> + * @arg: structure _arg_go which contain all needed info
> + *     arg->chan -> channel number
> + *     arg->addr -> start address of the microcode program
> + *                  which will be wrote into CPC register
> + *     arg->ns -> 1 for non secure, 0 for secure
> + *                (if only DMA Manager is in secure).
> + *
> + * Return: Size of opcode.
> + */
> +static inline u32 _emit_go(u8 buf[], const struct _arg_go *arg)
> +{
> +       u8 *buffer = buf;
> +       u8 chan = arg->chan;
> +       u32 addr = arg->addr;
> +       u32 ns = arg->ns;

These local vars seem pointless as the are only used once. Maybe it's
worth it for 'addr', but even there you should use the put_unaligned
thing.
> +
> +       buffer[0] = CMD_DMAGO;
> +       buffer[0] |= (ns << 1);
> +       buffer[1] = chan & 0x7;
> +       buf[2] = addr & 0xFF;
> +       buf[3] = (addr >> 8) & 0xFF;
> +       buf[4] = (addr >> 16) & 0xFF;
> +       buf[5] = (addr >> 24) & 0xFF;
> +       return SZ_DMAGO;
> +}
> +
> +/**
> + * _prepare_ccr - Populate the CCR register.
> + * @rqc: Request Configuration.
> + *
> + * Return: Channel Control register (CCR) Setting.
> + */
> +static inline u32 _prepare_ccr(const struct dma330_reqcfg *rqc)
> +{
> +       u32 ccr = 0;
> +
> +       if (rqc->src_inc)
> +               ccr |= CC_SRCINC;
> +       if (rqc->dst_inc)
> +               ccr |= CC_DSTINC;
> +
> +       /* We set same protection levels for Src and DST for now */
> +       if (rqc->privileged)
> +               ccr |= CC_SRCPRI | CC_DSTPRI;
> +       if (rqc->nonsecure)
> +               ccr |= CC_SRCNS | CC_DSTNS;
> +       if (rqc->insnaccess)
> +               ccr |= CC_SRCIA | CC_DSTIA;
> +
> +       ccr |= (((rqc->brst_len - 1) & 0xf) << CC_SRCBRSTLEN_SHFT);
> +       ccr |= (((rqc->brst_len - 1) & 0xf) << CC_DSTBRSTLEN_SHFT);
> +
> +       ccr |= (rqc->brst_size << CC_SRCBRSTSIZE_SHFT);
> +       ccr |= (rqc->brst_size << CC_DSTBRSTSIZE_SHFT);
> +
> +       ccr |= (rqc->scctl << CC_SRCCCTRL_SHFT);
> +       ccr |= (rqc->dcctl << CC_DSTCCTRL_SHFT);

You don't need the outer brackets on these.

> +
> +       ccr |= (rqc->swap << CC_SWAP_SHFT);
> +       return ccr;
> +}
> +
> +/**
> + * dma330_until_dmac_idle - Wait until DMA Manager is idle.
> + *
> + * @plat: Pointer to struct dma_dma330_platdata.
> + * @timeout_ms: Timeout (in miliseconds).
> + *
> + * Return: Negative value for error / timeout ocurred before idle,
> + *        0 for successful.
> + */
> +static int dma330_until_dmac_idle(struct dma_dma330_platdata *plat,
> +                                 const u32 timeout_ms)
> +{
> +       void __iomem *regs = plat->base;
> +
> +       return wait_for_bit(__func__,
> +                           (const u32 *)(uintptr_t)(regs + DBGSTATUS),
> +                           DBG_BUSY, 0, timeout_ms, false);
> +}
> +
> +/**
> + * dma330_execute_dbginsn - Execute debug instruction such as DMAGO and DMAKILL.
> + *
> + * @insn: The buffer which stored the debug instruction.
> + * @plat: Pointer to struct dma_dma330_platdata.
> + * @timeout_ms: Timeout (in miliseconds).
> + *
> + * Return: void.
> + */
> +static inline void dma330_execute_dbginsn(u8 insn[],
> +                                         struct dma_dma330_platdata *plat,
> +                                         const u32 timeout_ms)
> +{
> +       void __iomem *regs = plat->base;
> +       u32 val;
> +
> +       val = (insn[0] << 16) | (insn[1] << 24);
> +       if (!plat->dma330.channel0_manager1)
> +               val |= (1 << 0);
> +       val |= (plat->dma330.channel_num << 8); /* Channel Number */
> +       writel(val, regs + DBGINST0);
> +       val = insn[2];
> +       val = val | (insn[3] << 8);
> +       val = val | (insn[4] << 16);
> +       val = val | (insn[5] << 24);
> +       writel(val, regs + DBGINST1);
> +
> +       /* If timed out due to halted state-machine */
> +       if (dma330_until_dmac_idle(plat, timeout_ms)) {
> +               debug("DMAC halted!\n");
> +               return;
> +       }
> +       /* Get going */
> +       writel(0, regs + DBGCMD);
> +}
> +
> +/**
> + * dma330_getstate - Get the status of current channel or manager.
> + *
> + * @plat: Pointer to struct dma_dma330_platdata.
> + *
> + * Return: Status state of current channel or current manager.
> + */
> +static inline u32 dma330_getstate(struct dma_dma330_platdata *plat)
> +{
> +       void __iomem *regs = plat->base;
> +       u32 val;
> +
> +       if (plat->dma330.channel0_manager1)
> +               val = readl((uintptr_t)(regs + DS)) & 0xf;
> +       else
> +               val = readl((uintptr_t)(regs + CS(plat->dma330.channel_num))) &
> +                           0xf;
> +
> +       switch (val) {
> +       case DS_ST_STOP:
> +               return DMA330_STATE_STOPPED;
> +       case DS_ST_EXEC:
> +               return DMA330_STATE_EXECUTING;
> +       case DS_ST_CMISS:
> +               return DMA330_STATE_CACHEMISS;
> +       case DS_ST_UPDTPC:
> +               return DMA330_STATE_UPDTPC;
> +       case DS_ST_WFE:
> +               return DMA330_STATE_WFE;
> +       case DS_ST_FAULT:
> +               return DMA330_STATE_FAULTING;
> +       case DS_ST_ATBRR:
> +               if (plat->dma330.channel0_manager1)
> +                       return DMA330_STATE_INVALID;
> +               else
> +                       return DMA330_STATE_ATBARRIER;
> +       case DS_ST_QBUSY:
> +               if (plat->dma330.channel0_manager1)
> +                       return DMA330_STATE_INVALID;
> +               else
> +                       return DMA330_STATE_QUEUEBUSY;
> +       case DS_ST_WFP:
> +               if (plat->dma330.channel0_manager1)
> +                       return DMA330_STATE_INVALID;
> +               else
> +                       return DMA330_STATE_WFP;
> +       case DS_ST_KILL:
> +               if (plat->dma330.channel0_manager1)
> +                       return DMA330_STATE_INVALID;
> +               else
> +                       return DMA330_STATE_KILLING;
> +       case DS_ST_CMPLT:
> +               if (plat->dma330.channel0_manager1)
> +                       return DMA330_STATE_INVALID;
> +               else
> +                       return DMA330_STATE_COMPLETING;
> +       case DS_ST_FLTCMP:
> +               if (plat->dma330.channel0_manager1)
> +                       return DMA330_STATE_INVALID;
> +               else
> +                       return DMA330_STATE_FAULT_COMPLETING;
> +       default:
> +               return DMA330_STATE_INVALID;
> +       }
> +}
> +
> +/**
> + * dma330_trigger - Execute the DMAGO through debug instruction.
> + *
> + * @plat: Pointer to struct dma_dma330_platdata.
> + * @timeout_ms: Timeout (in miliseconds).
> + *
> + * When the DMA manager executes Go for a DMA channel that is in the Stopped
> + * state, it moves a 32-bit immediate into the program counter, then setting
> + * its security state and updating DMA channel to the Executing state.
> + *
> + * Return: Negative value for error as DMA is not ready. 0 for successful.
> + */
> +static int dma330_trigger(struct dma_dma330_platdata *plat,
> +                         const u32 timeout_ms)
> +{
> +       u8 insn[6] = {0, 0, 0, 0, 0, 0};
> +       struct _arg_go go;
> +
> +       /*
> +        * Return if already ACTIVE. It will ensure DMAGO is executed at
> +        * STOPPED state too
> +        */
> +       plat->dma330.channel0_manager1 = 0;
> +       if (dma330_getstate(plat) !=
> +               DMA330_STATE_STOPPED)
> +               return -EPERM;
> +
> +       go.chan = plat->dma330.channel_num;
> +       go.addr = (uintptr_t)plat->dma330.buf;
> +
> +       if (!plat->dma330.secure)
> +               go.ns = 1; /* non-secure condition */
> +       else
> +               go.ns = 0; /* secure condition */
> +
> +       _emit_go(insn, &go);
> +
> +       /* Only manager can execute GO */
> +       plat->dma330.channel0_manager1 = 1;
> +       dma330_execute_dbginsn(insn, plat, timeout_ms);
> +       return 0;
> +}
> +
> +/**
> + * dma330_stop - Stop the manager or channel.
> + *
> + * @plat: Pointer to struct dma_dma330_platdata.
> + * @timeout_ms: Timeout (in miliseconds).
> + *
> + * Stop the manager/channel or killing them and ensure they reach to stop
> + * state.
> + *
> + * Return: void.
> + */
> +static void dma330_stop(struct dma_dma330_platdata *plat, const u32 timeout_ms)
> +{
> +       u8 insn[6] = {0, 0, 0, 0, 0, 0};
> +
> +       /* If fault completing, wait until reach faulting and killing state */
> +       if (dma330_getstate(plat) == DMA330_STATE_FAULT_COMPLETING)
> +               UNTIL(plat, DMA330_STATE_FAULTING | DMA330_STATE_KILLING);
> +
> +       /* Return if nothing needs to be done */
> +       if (dma330_getstate(plat) == DMA330_STATE_COMPLETING ||
> +           dma330_getstate(plat) == DMA330_STATE_KILLING ||
> +           dma330_getstate(plat) == DMA330_STATE_STOPPED)
> +               return;
> +
> +       /* Kill it to ensure it reach to stop state */
> +       _emit_kill(insn);
> +
> +       /* Execute the KILL instruction through debug registers */
> +       dma330_execute_dbginsn(insn, plat, timeout_ms);
> +}
> +
> +/**
> + * dma330_start - Execute the command list through DMAGO and debug instruction.
> + *
> + * @plat: Pointer to struct dma_dma330_platdata.
> + * @timeout_ms: Timeout (in miliseconds).
> + *
> + * Return: Negative value for error where DMA is not ready. 0 for successful.
> + */
> +static int dma330_start(struct dma_dma330_platdata *plat, const u32 timeout_ms)
> +{
> +       debug("INFO: DMA BASE Address = 0x%08x\n",
> +             (u32)(uintptr_t)plat->base);
> +
> +       switch (dma330_getstate(plat)) {
> +       case DMA330_STATE_FAULT_COMPLETING:
> +               UNTIL(plat, DMA330_STATE_FAULTING | DMA330_STATE_KILLING);
> +
> +               if (dma330_getstate(plat) == DMA330_STATE_KILLING)
> +                       UNTIL(plat, DMA330_STATE_STOPPED);
> +
> +       case DMA330_STATE_FAULTING:
> +               dma330_stop(plat, timeout_ms);
> +
> +       case DMA330_STATE_KILLING:
> +       case DMA330_STATE_COMPLETING:
> +               UNTIL(plat, DMA330_STATE_STOPPED);
> +
> +       case DMA330_STATE_STOPPED:
> +               return dma330_trigger(plat, timeout_ms);
> +
> +       case DMA330_STATE_WFP:
> +       case DMA330_STATE_QUEUEBUSY:
> +       case DMA330_STATE_ATBARRIER:
> +       case DMA330_STATE_UPDTPC:
> +       case DMA330_STATE_CACHEMISS:
> +       case DMA330_STATE_EXECUTING:
> +               return -ESRCH;
> +       /* For RESUME, nothing yet */
> +       case DMA330_STATE_WFE:
> +       default:
> +               return -ESRCH;
> +       }
> +}
> +
> +/**
> + * dma330_transfer_start - DMA start to run.
> + *
> + * @plat: Pointer to struct dma_dma330_platdata.
> + *
> + * DMA start to excecute microcode command list.
> + *
> + * Return: Negative value for error or not successful. 0 for successful.
> + */
> +static int dma330_transfer_start(struct dma_dma330_platdata *plat)
> +{
> +       const u32 timeout_ms = 1000;
> +
> +       /* Execute the command list */
> +       return dma330_start(plat, timeout_ms);
> +}
> +
> +/**
> + * dma330_transfer_finish - DMA polling until execution finish or error.
> + *
> + * @plat: Pointer to struct dma_dma330_platdata.
> + *
> + * DMA polling until excution finish and checking the state status.
> + *
> + * Return: Negative value for state error or not successful. 0 for successful.
> + */
> +static int dma330_transfer_finish(struct dma_dma330_platdata *plat)
> +{
> +       void __iomem *regs = plat->base;
> +
> +       if (!plat->dma330.buf) {
> +               debug("ERROR DMA330 : DMA Microcode buffer pointer is NULL\n");
> +               return -EINVAL;
> +       }
> +
> +       plat->dma330.channel0_manager1 = 0;
> +
> +       /* Wait until finish execution to ensure we compared correct result*/
> +       UNTIL(plat, DMA330_STATE_STOPPED | DMA330_STATE_FAULTING);
> +
> +       /* check the state */
> +       if (dma330_getstate(plat) == DMA330_STATE_FAULTING) {
> +               debug("FAULT Mode: Channel %d Faulting, FTR = 0x%08x,",
> +                    plat->dma330.channel_num,
> +                    readl(regs + FTC(plat->dma330.channel_num)));
> +               debug("CPC = 0x%08lx\n",
> +                    (readl(regs + CPC(plat->dma330.channel_num))
> +                    - (uintptr_t)plat->dma330.buf));
> +               return -EPROTO;
> +       }
> +       return 0;
> +}
> +
> +/**
> + * dma330_transfer_setup - DMA transfer setup (DMA_MEM_TO_MEM, DMA_MEM_TO_DEV
> + *                        or DMA_DEV_TO_MEM).
> + *
> + * @plat: Pointer to struct dma_dma330_platdata.
> + *
> + * For Peripheral transfer, the FIFO threshold value is expected at
> + * 2 ^ plat->brst_size * plat->brst_len.
> + *
> + * Return: Negative value for error or not successful. 0 for successful.
> + */
> +static int dma330_transfer_setup(struct dma_dma330_platdata *plat)

This function is extremely long, Can it be split into separete
sub-functions which perform the work, if the work can be split
sensibly into different parts?

> +{
> +       /* Variable declaration */
> +       /* Buffer offset clear to 0 */
> +       int off = 0;
> +       int ret = 0;
> +       /* For DMALPEND */
> +       u32 loopjmp0, loopjmp1;
> +       /* Loop count 0 */
> +       u32 lcnt0 = 0;
> +       /* Loop count 1 */
> +       u32 lcnt1 = 0;
> +       u32 brst_size = 0;
> +       u32 brst_len = 0;
> +       u32 data_size_byte = plat->dma330.size_byte;
> +       /* Strong order memory is required to store microcode command list */
> +       u8 *buf = (u8 *)plat->dma330.buf;
> +       /* Channel Control Register */
> +       u32 ccr = 0;
> +       struct dma330_reqcfg reqcfg;
> +
> +       if (!buf) {
> +               debug("ERROR DMA330 : DMA Microcode buffer pointer is NULL\n");
> +               return -EINVAL;
> +       }
> +
> +       if (plat->dma330.transfer_type == DMA_MEM_TO_DEV)
> +               debug("INFO: mem2perip");
> +       else if (plat->dma330.transfer_type == DMA_DEV_TO_MEM)
> +               debug("INFO: perip2mem");
> +       else
> +               debug("INFO: DMA_MEM_TO_MEM");
> +
> +       debug(" - 0x%x%x -> 0x%x%x\nsize=%08x brst_size=2^%d ",
> +             (u32)(plat->dma330.src_addr >> 32),
> +             (u32)plat->dma330.src_addr,
> +             (u32)(plat->dma330.dst_addr >> 32),
> +             (u32)plat->dma330.dst_addr,
> +             data_size_byte,
> +             plat->brst_size);
> +
> +       debug("brst_len=%d singles_brst_size=2^%d\n", plat->brst_len,
> +             plat->dma330.single_brst_size);
> +
> +       /* brst_size = 2 ^ plat->brst_size */
> +       brst_size = 1 << plat->brst_size;
> +
> +       /* Fool proof checking */
> +       if (plat->brst_size < 0 || plat->brst_size >
> +               plat->max_brst_size) {
> +               debug("ERROR DMA330: brst_size must 0-%d (not %d)\n",
> +                       plat->max_brst_size, plat->brst_size);
> +               return -EINVAL;
> +       }
> +       if (plat->dma330.single_brst_size < 0 ||
> +               plat->dma330.single_brst_size > plat->max_brst_size) {
> +               debug("ERROR DMA330 : single_brst_size must 0-%d (not %d)\n",
> +                       plat->max_brst_size, plat->dma330.single_brst_size);
> +               return -EINVAL;
> +       }
> +       if (plat->brst_len < 1 || plat->brst_len > 16) {
> +               debug("ERROR DMA330 : brst_len must 1-16 (not %d)\n",
> +                       plat->brst_len);
> +               return -EINVAL;
> +       }
> +       if (plat->dma330.src_addr & (brst_size - 1)) {
> +               debug("ERROR DMA330 : Source address unaligned\n");
> +               return -EINVAL;
> +       }
> +       if (plat->dma330.dst_addr & (brst_size - 1)) {
> +               debug("ERROR DMA330 : Destination address unaligned\n");
> +               return -EINVAL;
> +       }
> +
> +       /* Setup the command list */
> +       /* DMAMOV SAR, x->src_addr */
> +       off += _emit_mov(&buf[off], SAR, plat->dma330.src_addr);
> +       /* DMAMOV DAR, x->dst_addr */
> +       off += _emit_mov(&buf[off], DAR, plat->dma330.dst_addr);
> +       /* DMAFLUSHP P(periheral_id) */
> +       if (plat->dma330.transfer_type != DMA_MEM_TO_MEM)
> +               off += _emit_flushp(&buf[off], plat->dma330.peripheral_id);
> +
> +       /* Preparing the CCR value */
> +       if (plat->dma330.transfer_type == DMA_MEM_TO_DEV) {
> +               /* Disable auto increment */
> +               reqcfg.dst_inc = 0;
> +               /* Enable auto increment */
> +               reqcfg.src_inc = 1;
> +       } else if (plat->dma330.transfer_type == DMA_DEV_TO_MEM) {
> +               reqcfg.dst_inc = 1;
> +               reqcfg.src_inc = 0;
> +       } else {
> +               /* DMA_MEM_TO_MEM */
> +               reqcfg.dst_inc = 1;
> +               reqcfg.src_inc = 1;
> +       }
> +
> +       if (!plat->dma330.secure)
> +               reqcfg.nonsecure = 1; /* Non Secure mode */
> +       else
> +               reqcfg.nonsecure = 0; /* Secure mode */
> +
> +       if (plat->dma330.enable_cache1 == 0) {
> +               /* Noncacheable but bufferable */
> +               reqcfg.dcctl = 0x1;
> +               reqcfg.scctl = 0x1;
> +       } else {
> +               if (plat->dma330.transfer_type == DMA_MEM_TO_DEV) {
> +                       reqcfg.dcctl = 0x1;
> +                       /* Cacheable write back */
> +                       reqcfg.scctl = 0x7;
> +               } else if (plat->dma330.transfer_type == DMA_DEV_TO_MEM) {
> +                       reqcfg.dcctl = 0x7;
> +                       reqcfg.scctl = 0x1;
> +               } else {
> +                       reqcfg.dcctl = 0x7;
> +                       reqcfg.scctl = 0x7;
> +               }
> +       }
> +       /* 1 - Privileged  */
> +       reqcfg.privileged = 1;
> +       /* 0 - Data access */
> +       reqcfg.insnaccess = 0;
> +       /* 0 - No endian swap */

I am not keen on these comments. The struct members should be
documented when the struct is declared, not here.

> +       reqcfg.swap = 0;
> +       /* DMA burst length */
> +       reqcfg.brst_len = plat->brst_len;
> +       /* DMA burst size */
> +       reqcfg.brst_size = plat->brst_size;
> +       /* Preparing the CCR value */
> +       ccr = _prepare_ccr(&reqcfg);
> +       /* DMAMOV CCR, ccr */
> +       off += _emit_mov(&buf[off], CCR, ccr);
> +
> +       /* BURST */
> +       /* Can initiate a burst? */
These two comments should be joined

> +       while (data_size_byte >= brst_size * plat->brst_len) {
> +               lcnt0 = data_size_byte / (brst_size * plat->brst_len);
> +               lcnt1 = 0;
> +               if (lcnt0 >= 256 * 256) {
> +                       lcnt0 = 256;
> +                       lcnt1 = 256;
> +               } else if (lcnt0 >= 256) {
> +                       lcnt1 = lcnt0 / 256;
> +                       lcnt0 = 256;
> +               }
> +               data_size_byte = data_size_byte -
> +                       (brst_size * plat->brst_len * lcnt0 * lcnt1);
> +
> +               debug("Transferring 0x%08x Remain 0x%08x\n", (brst_size *
> +                       plat->brst_len * lcnt0 * lcnt1), data_size_byte);
> +               debug("Running burst - brst_size=2^%d, brst_len=%d, ",
> +                     plat->brst_size, plat->brst_len);
> +               debug("lcnt0=%d, lcnt1=%d\n", lcnt0, lcnt1);
> +
> +               if (lcnt1) {
> +                       /* DMALP1 */
> +                       off += _emit_lp(&buf[off], 1, lcnt1);
> +                       loopjmp1 = off;
> +               }
> +               /* DMALP0 */

What does this comment mean?

> +               off += _emit_lp(&buf[off], 0, lcnt0);
> +               loopjmp0 = off;
> +               /* DMAWFP periheral_id, burst */
> +               if (plat->dma330.transfer_type != DMA_MEM_TO_MEM)
> +                       off += _emit_wfp(&buf[off], BURST,
> +                               plat->dma330.peripheral_id);
> +               /* DMALD */
> +               off += _emit_ld(&buf[off], ALWAYS);
> +
> +               WATCHDOG_RESET();
> +
> +               /* DMARMB */
> +               off += _emit_rmb(&buf[off]);
> +               /* DMASTPB peripheral_id */
> +               if (plat->dma330.transfer_type != DMA_MEM_TO_MEM)
> +                       off += _emit_stp(&buf[off], BURST,
> +                               plat->dma330.peripheral_id);
> +               else
> +                       off += _emit_st(&buf[off], ALWAYS);
> +               /* DMAWMB */
> +               off += _emit_wmb(&buf[off]);
> +               /* DMALP0END */
> +               struct _arg_lpend lpend;
> +
> +               lpend.cond = ALWAYS;
> +               lpend.forever = 0;
> +               /* Loop cnt 0 */
> +               lpend.loop = 0;
> +               lpend.bjump = off - loopjmp0;
> +               off += _emit_lpend(&buf[off], &lpend);
> +               /* DMALP1END */
> +               if (lcnt1) {
> +                       struct _arg_lpend lpend;
> +
> +                       lpend.cond = ALWAYS;
> +                       lpend.forever = 0;
> +                       lpend.loop = 1;         /* loop cnt 1*/
> +                       lpend.bjump = off - loopjmp1;
> +                       off += _emit_lpend(&buf[off], &lpend);
> +               }
> +               /* Ensure the microcode don't exceed buffer size */
> +               if (off > plat->buf_size) {
> +                       debug("ERROR DMA330 : Exceed buffer size\n");
> +                       return -ENOMEM;
> +               }
> +       }
> +
> +       /* SINGLE */
> +       brst_len = 1;
> +       /* brst_size = 2 ^ plat->dma330.single_brst_size; */
> +       brst_size = (1 << plat->dma330.single_brst_size);
> +       lcnt0 = data_size_byte / (brst_size * brst_len);
> +
> +       /* Ensure all data will be transferred */
> +       data_size_byte = data_size_byte -
> +               (brst_size * brst_len * lcnt0);
> +       if (data_size_byte) {
> +               debug("ERROR DMA330 : Detected the possibility of ");
> +               debug("untransfered data. Please ensure correct single ");
> +               debug("burst size\n");
> +       }
> +
> +       if (lcnt0) {
> +               debug("Transferring 0x%08x Remain 0x%08x\n", (brst_size *
> +                       brst_len * lcnt0 * lcnt1), data_size_byte);
> +               debug("Running single - brst_size=2^%d, brst_len=%d, ",
> +                     brst_size, brst_len);
> +               debug("lcnt0=%d\n", lcnt0);
> +
> +               /* Preparing the CCR value */
> +               /* DMA burst length */
> +               reqcfg.brst_len = brst_len;
> +               /* DMA burst size */
> +               reqcfg.brst_size = brst_size;
> +               ccr = _prepare_ccr(&reqcfg);
> +               /* DMAMOV CCR, ccr */
> +               off += _emit_mov(&buf[off], CCR, ccr);
> +
> +               /* DMALP0 */
> +               off += _emit_lp(&buf[off], 0, lcnt0);
> +               loopjmp0 = off;
> +               /* DMAWFP peripheral_id, single */
> +               if (plat->dma330.transfer_type != DMA_MEM_TO_MEM)
> +                       off += _emit_wfp(&buf[off], SINGLE,
> +                               plat->dma330.peripheral_id);
> +
> +               /* DMALD */
> +               off += _emit_ld(&buf[off], ALWAYS);
> +               /* DMARMB */
> +               off += _emit_rmb(&buf[off]);
> +               /* DMASTPS peripheral_id */
> +               if (plat->dma330.transfer_type != DMA_MEM_TO_MEM)
> +                       off += _emit_stp(&buf[off], SINGLE,
> +                               plat->dma330.peripheral_id);
> +               else
> +                       off += _emit_st(&buf[off], ALWAYS);
> +
> +               /* DMAWMB */
> +               off += _emit_wmb(&buf[off]);
> +               /* DMALPEND */
> +               struct _arg_lpend lpend1;
> +
> +               lpend1.cond = ALWAYS;
> +               lpend1.forever = 0;
> +               /* loop cnt 0 */
> +               lpend1.loop = 0;
> +               lpend1.bjump = off - loopjmp0;
> +               off += _emit_lpend(&buf[off], &lpend1);
> +               /* Ensure the microcode don't exceed buffer size */
> +               if (off > plat->buf_size) {
> +                       puts("ERROR DMA330 : Exceed buffer size\n");
> +                       return -ENOMEM;
> +               }
> +       }
> +
> +       /* DMAEND */
> +       off += _emit_end(&buf[off]);
> +
> +       ret = dma330_transfer_start(plat);
> +       if (ret)
> +               return ret;
> +
> +       return dma330_transfer_finish(plat);
> +}
> +
> +/**
> + * dma330_transfer_zeroes - DMA transfer zeros.
> + *
> + * @plat: Pointer to struct dma_dma330_platdata.
> + *
> + * Used to write zeros to a memory chunk for memory scrubbing purpose.
> + *
> + * Return: Negative value for error or not successful. 0 for successful.
> + */
> +static int dma330_transfer_zeroes_setup(struct dma_dma330_platdata *plat)

Can this function be combined with the above somehow? It seems like a
lot of duplicate code.

> +{
> +       /* Variable declaration */
> +       /* Buffer offset clear to 0 */
> +       int off = 0;
> +       int ret = 0;
> +       /* For DMALPEND */
> +       u32 loopjmp0, loopjmp1;
> +       /* Loop count 0 */
> +       u32 lcnt0 = 0;
> +       /* Loop count 1 */
> +       u32 lcnt1 = 0;
> +       u32 brst_size = 0;
> +       u32 brst_len = 0;
> +       u32 data_size_byte = plat->dma330.size_byte;
> +       /* Strong order memory is required to store microcode command list */
> +       u8 *buf = (u8 *)plat->dma330.buf;
> +       /* Channel Control Register */
> +       u32 ccr = 0;
> +       struct dma330_reqcfg reqcfg;
> +
> +       if (!buf) {
> +               debug("ERROR DMA330 : DMA Microcode buffer pointer is NULL\n");
> +               return -EINVAL;
> +       }
> +
> +       debug("INFO: Write zeros -> ");
> +       debug("0x%x%x size=0x%08x\n",
> +             (u32)(plat->dma330.dst_addr >> 32),
> +             (u32)plat->dma330.dst_addr,
> +             data_size_byte);
> +
> +       plat->dma330.single_brst_size = 1;
> +
> +       /* burst_size = 2 ^ plat->brst_size */
> +       brst_size = 1 << plat->brst_size;
> +
> +       /* Setup the command list */
> +       /* DMAMOV DAR, x->dst_addr */
> +       off += _emit_mov(&buf[off], DAR, plat->dma330.dst_addr);
> +
> +       /* Preparing the CCR value */
> +       /* Enable auto increment */
> +       reqcfg.dst_inc = 1;
> +       /* Disable auto increment (not applicable) */
> +       reqcfg.src_inc = 0;
> +       /* Noncacheable but bufferable */
> +       reqcfg.dcctl = 0x1;
> +       /* Noncacheable and bufferable */
> +       reqcfg.scctl = 0x1;
> +       /* 1 - Privileged  */
> +       reqcfg.privileged = 1;
> +       /* 0 - Data access */
> +       reqcfg.insnaccess = 0;
> +       /* 0 - No endian swap */
> +       reqcfg.swap = 0;
> +
> +       if (!plat->dma330.secure)
> +               reqcfg.nonsecure = 1; /* Non Secure mode */
> +       else
> +               reqcfg.nonsecure = 0; /* Secure mode */
> +
> +       /* DMA burst length */
> +       reqcfg.brst_len = plat->brst_len;
> +       /* DMA burst size */
> +       reqcfg.brst_size = plat->brst_size;
> +       /* Preparing the CCR value */
> +       ccr = _prepare_ccr(&reqcfg);
> +       /* DMAMOV CCR, ccr */
> +       off += _emit_mov(&buf[off], CCR, ccr);
> +
> +       /* BURST */
> +       /* Can initiate a burst? */
> +       while (data_size_byte >= brst_size * plat->brst_len) {
> +               lcnt0 = data_size_byte / (brst_size * plat->brst_len);
> +               lcnt1 = 0;
> +               if (lcnt0 >= 256 * 256) {
> +                       lcnt0 = 256;
> +                       lcnt1 = 256;
> +               } else if (lcnt0 >= 256) {
> +                       lcnt1 = lcnt0 / 256;
> +                       lcnt0 = 256;
> +               }
> +
> +               data_size_byte = data_size_byte -
> +                       (brst_size * plat->brst_len * lcnt0 * lcnt1);
> +
> +               debug("Transferring 0x%08x Remain 0x%08x\n", (brst_size *
> +                       plat->brst_len * lcnt0 * lcnt1), data_size_byte);
> +               debug("Running burst - brst_size=2^%d, brst_len=%d,",
> +                     plat->brst_size, plat->brst_len);
> +               debug("lcnt0=%d, lcnt1=%d\n", lcnt0, lcnt1);
> +
> +               if (lcnt1) {
> +                       /* DMALP1 */
> +                       off += _emit_lp(&buf[off], 1, lcnt1);
> +                       loopjmp1 = off;
> +               }
> +               /* DMALP0 */
> +               off += _emit_lp(&buf[off], 0, lcnt0);
> +               loopjmp0 = off;
> +               /* DMALSTZ */
> +               off += _emit_stz(&buf[off]);
> +
> +               WATCHDOG_RESET();
> +
> +               /* DMALP0END */
> +               struct _arg_lpend lpend;
> +
> +               lpend.cond = ALWAYS;
> +               lpend.forever = 0;
> +               /* Loop cnt 0 */
> +               lpend.loop = 0;
> +               lpend.bjump = off - loopjmp0;
> +               off += _emit_lpend(&buf[off], &lpend);
> +               /* DMALP1END */
> +               if (lcnt1) {
> +                       struct _arg_lpend lpend;
> +
> +                       lpend.cond = ALWAYS;
> +                       lpend.forever = 0;
> +                       /* Loop cnt 1*/
> +                       lpend.loop = 1;
> +                       lpend.bjump = off - loopjmp1;
> +                       off += _emit_lpend(&buf[off], &lpend);
> +               }
> +               /* Ensure the microcode don't exceed buffer size */
> +               if (off > plat->buf_size) {
> +                       printf("off = %d\n", off);
> +                       debug("ERROR DMA330 : Exceed buffer size off %d\n",
> +                             off);
> +                       return -ENOMEM;
> +               }
> +       }
> +
> +       /* SINGLE */
> +       brst_len = 1;
> +       /* brst_size = 2 ^ plat->dma330.single_brst_size */
> +       brst_size = (1 << plat->dma330.single_brst_size);
> +       lcnt0 = data_size_byte / (brst_size * brst_len);
> +
> +       /* ensure all data will be transferred */
> +       data_size_byte = data_size_byte -
> +               (brst_size * brst_len * lcnt0);
> +       if (data_size_byte) {
> +               debug("ERROR DMA330 : Detected the possibility of ");
> +               debug("untransfered data. Please ensure correct single ");
> +               debug("burst size\n");
> +       }
> +
> +       if (lcnt0) {
> +               debug("Transferring 0x%08x Remain 0x%08x\n", (brst_size *
> +                    brst_len * lcnt0), data_size_byte);
> +               debug("Running single - brst_size=2^%d, brst_len=%d, ",
> +                     brst_size, brst_len);
> +               debug("lcnt0=%d\n", lcnt0);
> +
> +               /* Preparing the CCR value */
> +               /* DMA burst length */
> +               reqcfg.brst_len = brst_len;
> +               /* DMA burst size */
> +               reqcfg.brst_size = brst_size;
> +               ccr = _prepare_ccr(&reqcfg);
> +               /* DMAMOV CCR, ccr */
> +               off += _emit_mov(&buf[off], CCR, ccr);
> +
> +               /* DMALP0 */
> +               off += _emit_lp(&buf[off], 0, lcnt0);
> +               loopjmp0 = off;
> +               /* DMALSTZ */
> +               off += _emit_stz(&buf[off]);
> +               /* DMALPEND */
> +               struct _arg_lpend lpend1;
> +
> +               lpend1.cond = ALWAYS;
> +               lpend1.forever = 0;
> +               /* Loop cnt 0 */
> +               lpend1.loop = 0;
> +               lpend1.bjump = off - loopjmp0;
> +               off += _emit_lpend(&buf[off], &lpend1);
> +               /* Ensure the microcode don't exceed buffer size */
> +               if (off > plat->buf_size) {
> +                       debug("ERROR DMA330 : Exceed buffer size\n");
> +                       return -ENOMEM;
> +               }
> +       }
> +
> +       /* DMAEND */
> +       off += _emit_end(&buf[off]);
> +
> +       ret = dma330_transfer_start(plat);
> +       if (ret)
> +               return ret;
> +
> +       return dma330_transfer_finish(plat);
> +}
> +
> +static int dma330_transfer_zeroes(struct udevice *dev, void *dst, size_t len)
> +{
> +       struct dma_dma330_platdata *plat = dev->platdata;
> +       int ret = 0;
> +
> +       /* If DMA access is set to secure, base change to DMA secure base */
> +       if (plat->dma330.secure)
> +               plat->base += 0x1000;

What is this? I suggest you set up the secure base separately in
'plat', e.g. with a base_secure member. You should not change platform
data while running.

You can pass the base address to dma330_transfer_zeroes_setup, perhaps.

> +
> +       plat->dma330.dst_addr = (phys_size_t)(uintptr_t)dst;
> +       plat->dma330.size_byte = len;
> +
> +       ret = dma330_transfer_zeroes_setup(plat);
> +
> +       /* Revert back to non secure DMA base */
> +       if (plat->dma330.secure)
> +               plat->base -= 0x1000;
> +
> +       return ret;
> +}
> +
> +static int dma330_transfer(struct udevice *dev, int direction, void *dst,
> +                         void *src, size_t len)
> +{
> +       struct dma_dma330_platdata *plat = dev->platdata;
> +       int ret = 0;
> +
> +       /* If DMA access is set to secure, base change to DMA secure base */
> +       if (plat->dma330.secure)
> +               plat->base += 0x1000;

Same as above

> +
> +       plat->dma330.dst_addr = (phys_size_t)(uintptr_t)dst;
> +       plat->dma330.src_addr = (phys_size_t)(uintptr_t)src;
> +       plat->dma330.size_byte = len;
> +
> +       switch (direction) {
> +       case DMA_MEM_TO_MEM:
> +               plat->dma330.transfer_type = DMA_SUPPORTS_MEM_TO_MEM;
> +               break;
> +       case DMA_MEM_TO_DEV:
> +               plat->dma330.transfer_type = DMA_SUPPORTS_MEM_TO_DEV;
> +               break;
> +       case DMA_DEV_TO_MEM:
> +               plat->dma330.transfer_type = DMA_SUPPORTS_DEV_TO_MEM;
> +               break;
> +       }
> +
> +       ret = dma330_transfer_setup(plat);
> +
> +       /* Revert back to non secure DMA base */
> +       if (plat->dma330.secure)
> +               plat->base -= 0x1000;
> +
> +       return ret;
> +}
> +
> +static int dma330_ofdata_to_platdata(struct udevice *dev)
> +{
> +       struct dma_dma330_platdata *plat = dev->platdata;
> +       const void *blob = gd->fdt_blob;
> +       int node = dev_of_offset(dev);
> +
> +       plat->base = (void __iomem *)devfdt_get_addr(dev);

dev_read_addr()

> +       plat->max_brst_size = fdtdec_get_uint(blob, node, "dma-max-burst-size",
> +                                             2);

Use dev_read API please (live tree)

> +       plat->brst_size = fdtdec_get_uint(blob, node, "dma-burst-size", 2);
> +       plat->brst_len = fdtdec_get_uint(blob, node, "dma-burst-length", 2);
> +
> +       return 0;
> +}
> +
> +static int dma330_probe(struct udevice *adev)
> +{
> +       struct dma_dev_priv *uc_priv = dev_get_uclass_priv(adev);
> +
> +       uc_priv->supported = (DMA_SUPPORTS_MEM_TO_MEM |
> +                            DMA_SUPPORTS_MEM_TO_DEV |
> +                            DMA_SUPPORTS_DEV_TO_MEM);
> +       return 0;
> +}
> +
> +static const struct dma_ops dma330_ops = {
> +       .transfer = dma330_transfer,
> +       .transfer_zeroes = dma330_transfer_zeroes,
> +};
> +
> +static const struct udevice_id dma330_ids[] = {
> +       { .compatible = "arm,pl330" },
> +       { .compatible = "arm,dma330" },
> +       { /* sentinel */ }
> +};
> +
> +U_BOOT_DRIVER(dma_dma330) = {
> +       .name   = "dma_dma330",
> +       .id     = UCLASS_DMA,
> +       .of_match = dma330_ids,
> +       .ops    = &dma330_ops,
> +       .ofdata_to_platdata = dma330_ofdata_to_platdata,
> +       .probe = dma330_probe,
> +       .platdata_auto_alloc_size = sizeof(struct dma_dma330_platdata),
> +};
> diff --git a/include/dma330.h b/include/dma330.h
> new file mode 100644
> index 0000000..c054bf2
> --- /dev/null
> +++ b/include/dma330.h
> @@ -0,0 +1,136 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Copyright (C) 2018 Intel Corporation <www.intel.com>
> + */
> +
> +#include <dma.h>
> +#include <linux/sizes.h>
> +
> +#ifndef __DMA330_CORE_H
> +#define __DMA330_CORE_H
> +
> +#define DMA330_MAX_CHAN                8
> +#define DMA330_MAX_IRQS                32
> +#define DMA330_MAX_PERI                32
> +
> +#define DMA330_STATE_STOPPED           BIT(0)
> +#define DMA330_STATE_EXECUTING         BIT(1)
> +#define DMA330_STATE_WFE               BIT(2)
> +#define DMA330_STATE_FAULTING          BIT(3)
> +#define        DMA330_STATE_COMPLETING         BIT(4)
> +#define DMA330_STATE_WFP               BIT(5)
> +#define DMA330_STATE_KILLING           BIT(6)
> +#define DMA330_STATE_FAULT_COMPLETING  BIT(7)
> +#define DMA330_STATE_CACHEMISS         BIT(8)
> +#define DMA330_STATE_UPDTPC            BIT(9)
> +#define DMA330_STATE_ATBARRIER         BIT(10)
> +#define DMA330_STATE_QUEUEBUSY         BIT(11))
> +#define DMA330_STATE_INVALID           BIT(15)
> +
> +enum dma330_srccachectrl {
> +       SCCTRL0 = 0,    /* Noncacheable and nonbufferable */
> +       SCCTRL1,        /* Bufferable only */
> +       SCCTRL2,        /* Cacheable, but do not allocate */
> +       SCCTRL3,        /* Cacheable and bufferable, but do not allocate */
> +       SINVALID1,
> +       SINVALID2,
> +       SCCTRL6,        /* Cacheable write-through, allocate on reads only */
> +       SCCTRL7,        /* Cacheable write-back, allocate on reads only */
> +};
> +
> +enum dma330_dstcachectrl {
> +       DCCTRL0 = 0,    /* Noncacheable and nonbufferable */
> +       DCCTRL1,        /* Bufferable only */
> +       DCCTRL2,        /* Cacheable, but do not allocate */
> +       DCCTRL3,        /* Cacheable and bufferable, but do not allocate */
> +       DINVALID1 = 8,
> +       DINVALID2,
> +       DCCTRL6,        /* Cacheable write-through, allocate on writes only */
> +       DCCTRL7,        /* Cacheable write-back, allocate on writes only */
> +};
> +
> +enum dma330_byteswap {
> +       SWAP_NO = 0,
> +       SWAP_2,
> +       SWAP_4,
> +       SWAP_8,
> +       SWAP_16,
> +};
> +
> +/**
> + * dma330_reqcfg - Request Configuration.
> + *
> + * The DMA330 core does not modify this and uses the last
> + * working configuration if the request doesn't provide any.
> + *
> + * The Client may want to provide this info only for the
> + * first request and a request with new settings.
> + */
> +struct dma330_reqcfg {
> +       /* Address Incrementing */
> +       u8 dst_inc;
> +       u8 src_inc;
> +
> +       /*
> +        * For now, the SRC & DST protection levels
> +        * and burst size/length are assumed same.
> +        */
> +       u8 nonsecure;
> +       u8 privileged;
> +       u8 insnaccess;
> +       u8 brst_len;
> +       u8 brst_size; /* in power of 2 */
> +
> +       enum dma330_dstcachectrl dcctl;
> +       enum dma330_srccachectrl scctl;
> +       enum dma330_byteswap swap;
> +};
> +
> +/**
> + * dma330_transfer_struct - Structure to be passed in for dma330_transfer_x.
> + *
> + * @channel0_manager1: Switching configuration on DMA channel or DMA manager.
> + * @channel_num: Channel number assigned, valid from 0 to 7.
> + * @src_addr: Address to transfer from / source.
> + * @dst_addr: Address to transfer to / destination.
> + * @size_byte: Number of bytes to be transferred.
> + * @brst_size: Valid from 0 - 4,
> + *            where 0 = 1 (2 ^ 0) bytes and 3 = 16 bytes (2 ^ 4).
> + * @max_brst_size: Max transfer size (from 0 - 4).
> + * @single_brst_size: Single transfer size (from 0 - 4).
> + * @brst_len: Valid from 1 - 16 where each burst can transfer 1 - 16
> + *           Data chunk (each chunk size equivalent to brst_size).
> + * @peripheral_id: Assigned peripheral_id, valid from 0 to 31.
> + * @transfer_type: MEM2MEM, MEM2PERIPH or PERIPH2MEM.
> + * @enable_cache1: 1 for cache enabled for memory
> + *                (cacheable and bufferable, but do not allocate).
> + * @buf: Buffer handler which will point to the memory
> + *      allocated for dma microcode
> + * @secure: Set DMA channel in secure mode.

This is a good way to document structs. Please do it for the others.

> + *
> + * Description of the structure.

Yes?

> + */
> +struct dma330_transfer_struct {
> +       u32 channel0_manager1;
> +       u32 channel_num;
> +       phys_size_t src_addr;
> +       phys_size_t dst_addr;
> +       u32 size_byte;
> +       u32 single_brst_size;
> +       u32 peripheral_id;
> +       u32 transfer_type;
> +       u32 enable_cache1;
> +       u32 *buf;
> +       u8 secure;
> +};
> +
> +struct dma_dma330_platdata {
> +       void __iomem *base;
> +       u32 buf_size;
> +       u32 max_brst_size;
> +       u32 brst_size;
> +       u32 brst_len;
> +       struct dma330_transfer_struct dma330;
> +};
> +
> +#endif /* __DMA330_CORE_H */
> --
> 2.2.0
>

Regards,
Simon

On Fri, 2018-06-01 at 08:24 -0600, Simon Glass wrote:
> Hi Tien,
> 
> On 31 May 2018 at 02:08,  <tien.fong.chee@intel.com> wrote:
> > 
> > From: Tien Fong Chee <tien.fong.chee@intel.com>
> > 
> > Enable DMAC driver support for DMA-330 controller.
> > The driver is also compatible to PL330 product.
> > 
> > Signed-off-by: Tien Fong Chee <tien.fong.chee@intel.com>
> > ---
> >  drivers/dma/Kconfig  |    9 +-
> >  drivers/dma/Makefile |    1 +
> >  drivers/dma/dma330.c | 1514
> > ++++++++++++++++++++++++++++++++++++++++++++++++++
> >  include/dma330.h     |  136 +++++
> >  4 files changed, 1659 insertions(+), 1 deletion(-)
> >  create mode 100644 drivers/dma/dma330.c
> >  create mode 100644 include/dma330.h
> > 
> > diff --git a/drivers/dma/Kconfig b/drivers/dma/Kconfig
> > index 4ee6afa..6e77e07 100644
> > --- a/drivers/dma/Kconfig
> > +++ b/drivers/dma/Kconfig
> > @@ -2,7 +2,7 @@ menu "DMA Support"
> > 
> >  config DMA
> >         bool "Enable Driver Model for DMA drivers"
> > -       depends on DM
> > +       depends on DM || SPL_DM
> >         help
> >           Enable driver model for DMA. DMA engines can do
> >           asynchronous data transfers without involving the host
> > @@ -34,4 +34,11 @@ config APBH_DMA_BURST8
> > 
> >  endif
> > 
> > +config DMA330_DMA
> > +       bool "PL330/DMA-330 DMA Controller(DMAC) driver"
> > +       depends on DMA
> > +       help
> > +        Enable the DMA controller driver for both PL330 and
> > +        DMA-330 products.
> > +
> >  endmenu # menu "DMA Support"
> > diff --git a/drivers/dma/Makefile b/drivers/dma/Makefile
> > index 4eaef8a..bfad0dd 100644
> > --- a/drivers/dma/Makefile
> > +++ b/drivers/dma/Makefile
> > @@ -11,3 +11,4 @@ obj-$(CONFIG_FSL_DMA) += fsl_dma.o
> >  obj-$(CONFIG_TI_KSNAV) += keystone_nav.o keystone_nav_cfg.o
> >  obj-$(CONFIG_TI_EDMA3) += ti-edma3.o
> >  obj-$(CONFIG_DMA_LPC32XX) += lpc32xx_dma.o
> > +obj-$(CONFIG_DMA330_DMA) += dma330.o
> > diff --git a/drivers/dma/dma330.c b/drivers/dma/dma330.c
> > new file mode 100644
> > index 0000000..66575d8
> > --- /dev/null
> > +++ b/drivers/dma/dma330.c
> > @@ -0,0 +1,1514 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Copyright (C) 2018 Intel Corporation <www.intel.com>
> > + */
> > +
> > +#include <common.h>
> > +#include <asm/io.h>
> > +#include <dma330.h>
> > +#include <dm.h>
> > +#include <fdtdec.h>
> > +#include <wait_bit.h>
> > +#include <linux/unaligned/access_ok.h>
> > +
> > +/* Register and Bit field Definitions */
> > +
> > +/* DMA Status */
> > +#define DS                     0x0
> > +#define DS_ST_STOP             0x0
> > +#define DS_ST_EXEC             0x1
> > +#define DS_ST_CMISS            0x2
> > +#define DS_ST_UPDTPC           0x3
> > +#define DS_ST_WFE              0x4
> > +#define DS_ST_ATBRR            0x5
> > +#define DS_ST_QBUSY            0x6
> > +#define DS_ST_WFP              0x7
> > +#define DS_ST_KILL             0x8
> > +#define DS_ST_CMPLT            0x9
> > +#define DS_ST_FLTCMP           0xe
> > +#define DS_ST_FAULT            0xf
> It is possible to use enum for some of these?
> 
> enum {
>    DS             = 0,
>    DS_ST_STOP,
>  ...
> }
> 
Okay.
> > 
> > +
> > +/* DMA Program Count register */
> > +#define DPC                    0x4
> > +/* Interrupt Enable register */
> > +#define INTEN                  0x20
> > +/* event-Interrupt Raw Status register */
> > +#define ES                     0x24
> > +/* Interrupt Status register */
> > +#define INTSTATUS              0x28
> > +/* Interrupt Clear register */
> > +#define INTCLR                 0x2c
> > +/* Fault Status DMA Manager register */
> > +#define FSM                    0x30
> > +/* Fault Status DMA Channel register */
> > +#define FSC                    0x34
> > +/* Fault Type DMA Manager register */
> > +#define FTM                    0x38
> > +
> > +/* Fault Type DMA Channel register */
> > +#define _FTC                   0x40
> > +#define FTC(n)                 (_FTC + (n) * 0x4)
> > +
> > +/* Channel Status register */
> > +#define _CS                    0x100
> > +#define CS(n)                  (_CS + (n) * 0x8)
> > +#define CS_CNS                 BIT(21)
> > +
> > +/* Channel Program Counter register */
> > +#define _CPC                   0x104
> > +#define CPC(n)                 (_CPC + (n) * 0x8)
> > +
> > +/* Source Address register */
> > +#define _SA                    0x400
> > +#define SA(n)                  (_SA + (n) * 0x20)
> > +
> > +/* Destination Address register */
> > +#define _DA                    0x404
> > +#define DA(n)                  (_DA + (n) * 0x20)
> > +
> > +/* Channel Control register */
> > +#define _CC                    0x408
> > +#define CC(n)                  (_CC + (n) * 0x20)
> > +
> > +/* Channel Control register (CCR) Setting */
> > +#define CC_SRCINC              BIT(0)
> > +#define CC_DSTINC              BIT(14)
> > +#define CC_SRCPRI              BIT(8)
> > +#define CC_DSTPRI              BIT(22)
> > +#define CC_SRCNS               BIT(9)
> > +#define CC_DSTNS               BIT(23)
> > +#define CC_SRCIA               BIT(10)
> > +#define CC_DSTIA               BIT(24)
> > +#define CC_SRCBRSTLEN_SHFT     4
> > +#define CC_DSTBRSTLEN_SHFT     18
> > +#define CC_SRCBRSTSIZE_SHFT    1
> > +#define CC_DSTBRSTSIZE_SHFT    15
> > +#define CC_SRCCCTRL_SHFT       11
> > +#define CC_SRCCCTRL_MASK       0x7
> Can you make the mask a shifted mask? Then it is easier to use in
> clrsetbits_le32(), for example.
> 
> e.g.
> 
> #define CC_SRCCCTRL_MASK (7 << CC_SRCCCTRL_SHFT)
> 
Okay.
> > 
> > +#define CC_DSTCCTRL_SHFT       25
> > +#define CC_DRCCCTRL_MASK       0x7
> > +#define CC_SWAP_SHFT           28
> > +
> > +/* Loop Counter 0 register */
> > +#define _LC0                   0x40c
> > +#define LC0(n)                 (_LC0 + (n) * 0x20)
> > +
> > +/* Loop Counter 1 register */
> > +#define _LC1                   0x410
> > +#define LC1(n)                 (_LC1 + (n) * 0x20)
> > +
> > +/* Debug Status register */
> > +#define DBGSTATUS              0xd00
> > +#define DBG_BUSY               BIT(0)
> > +
> > +/* Debug Command register */
> > +#define DBGCMD                 0xd04
> > +/* Debug Instruction 0 register */
> > +#define DBGINST0               0xd08
> > +/* Debug Instruction 1 register */
> > +#define DBGINST1               0xd0c
> > +
> > +/* Configuration register */
> > +#define CR0                    0xe00
> > +#define CR1                    0xe04
> > +#define CR2                    0xe08
> > +#define CR3                    0xe0c
> > +#define CR4                    0xe10
> > +#define CRD                    0xe14
> > +
> > +/* Peripheral Identification register */
> > +#define PERIPH_ID              0xfe0
> > +/* Component Identification register */
> > +#define PCELL_ID               0xff0
> > +
> > +/* Configuration register value */
> > +#define CR0_PERIPH_REQ_SET     BIT(0)
> > +#define CR0_BOOT_EN_SET                BIT(1)
> > +#define CR0_BOOT_MAN_NS                BIT(2)
> > +#define CR0_NUM_CHANS_SHIFT    4
> > +#define CR0_NUM_CHANS_MASK     0x7
> > +#define CR0_NUM_PERIPH_SHIFT   12
> > +#define CR0_NUM_PERIPH_MASK    0x1f
> > +#define CR0_NUM_EVENTS_SHIFT   17
> > +#define CR0_NUM_EVENTS_MASK    0x1f
> > +
> > +/* Configuration register value */
> > +#define CR1_ICACHE_LEN_SHIFT           0
> > +#define CR1_ICACHE_LEN_MASK            0x7
> > +#define CR1_NUM_ICACHELINES_SHIFT      4
> > +#define CR1_NUM_ICACHELINES_MASK       0xf
> > +
> > +/* Configuration register value */
> > +#define CRD_DATA_WIDTH_SHIFT   0
> > +#define CRD_DATA_WIDTH_MASK    0x7
> > +#define CRD_WR_CAP_SHIFT       4
> > +#define CRD_WR_CAP_MASK                0x7
> > +#define CRD_WR_Q_DEP_SHIFT     8
> > +#define CRD_WR_Q_DEP_MASK      0xf
> > +#define CRD_RD_CAP_SHIFT       12
> > +#define CRD_RD_CAP_MASK                0x7
> > +#define CRD_RD_Q_DEP_SHIFT     16
> > +#define CRD_RD_Q_DEP_MASK      0xf
> > +#define CRD_DATA_BUFF_SHIFT    20
> > +#define CRD_DATA_BUFF_MASK     0x3ff
> > +
> > +/* Microcode opcode value */
> > +#define CMD_DMAADDH            0x54
> > +#define CMD_DMAEND             0x00
> > +#define CMD_DMAFLUSHP          0x35
> > +#define CMD_DMAGO              0xa0
> > +#define CMD_DMALD              0x04
> > +#define CMD_DMALDP             0x25
> > +#define CMD_DMALP              0x20
> > +#define CMD_DMALPEND           0x28
> > +#define CMD_DMAKILL            0x01
> > +#define CMD_DMAMOV             0xbc
> > +#define CMD_DMANOP             0x18
> > +#define CMD_DMARMB             0x12
> > +#define CMD_DMASEV             0x34
> > +#define CMD_DMAST              0x08
> > +#define CMD_DMASTP             0x29
> > +#define CMD_DMASTZ             0x0c
> > +#define CMD_DMAWFE             0x36
> > +#define CMD_DMAWFP             0x30
> > +#define CMD_DMAWMB             0x13
> > +
> > +/* the size of opcode plus opcode required settings */
> > +#define SZ_DMAADDH             3
> > +#define SZ_DMAEND              1
> > +#define SZ_DMAFLUSHP           2
> > +#define SZ_DMALD               1
> > +#define SZ_DMALDP              2
> > +#define SZ_DMALP               2
> > +#define SZ_DMALPEND            2
> > +#define SZ_DMAKILL             1
> > +#define SZ_DMAMOV              6
> > +#define SZ_DMANOP              1
> > +#define SZ_DMARMB              1
> > +#define SZ_DMASEV              2
> > +#define SZ_DMAST               1
> > +#define SZ_DMASTP              2
> > +#define SZ_DMASTZ              1
> > +#define SZ_DMAWFE              2
> > +#define SZ_DMAWFP              2
> > +#define SZ_DMAWMB              1
> > +#define SZ_DMAGO               6
> > +
> > +/* Use this _only_ to wait on transient states */
> > +#define UNTIL(t, s)    do { \
> > +                               WATCHDOG_RESET(); \
> > +                       } while (!(dma330_getstate(t) & (s)))
> > +
> > +/* debug message printout */
> > +#ifdef DEBUG
> > +#define DMA330_DBGCMD_DUMP(off, x...)  do { \
> > +                                               printf("%x bytes:",
> > off); \
> > +                                               printf(x); \
> > +                                               WATCHDOG_RESET(); \
> > +                                       } while (0)
> > +#else
> > +#define DMA330_DBGCMD_DUMP(off, x...)  do {} while (0)
> > +#endif
> Can DMA330_DBGCMD_DUMP be a static function, and lower case? It's
> only
> used in one file, right?
> 
Okay, yes.
> > 
> > +
> > +/* Enum declaration */
> We know that :-)
> 
> Can you please add comments explaining what these enums are for, and
> what the values mean?
> 
Sure.
> > 
> > +enum dmamov_dst {
> > +       SAR = 0,
> > +       CCR,
> > +       DAR,
> > +};
> > +
> > +enum dma330_dst {
> > +       SRC = 0,
> > +       DST,
> > +};
> > +
> > +enum dma330_cond {
> > +       SINGLE,
> > +       BURST,
> > +       ALWAYS,
> > +};
> > +
> > +/* Structure will be used by _emit_lpend function */
> > +struct _arg_lpend {
> > +       enum dma330_cond cond;
> > +       int forever;
> > +       u32 loop;
> > +       u8 bjump;
> > +};
> > +
> > +/* Structure will be used by _emit_go function */
> > +struct _arg_go {
> Please drop the _ on these struct names, and document the members
> here.
> 
Okay.
> > 
> > +       u8 chan;
> > +       u32 addr;
> > +       u32 ns;
> > +};
> > +
> > +/*
> > + * _emit_end - Add opcode DMAEND into microcode (end).
> > + *
> > + * @buf: The buffer which stored the microcode program.
> > + *
> > + * Return: Size of opcode.
> > + */
> > +static inline u32 _emit_end(u8 buf[])
> Why is everything inline? The compiler will do it automatically when
> there is only one caller. Other than that, it just increases code
> size
> I think.
> 
Okay, will convert to static function.
> > 
> > +{
> > +       buf[0] = CMD_DMAEND;
> > +       DMA330_DBGCMD_DUMP(SZ_DMAEND, "\tDMAEND\n");
> > +       return SZ_DMAEND;
> > +}
> > +
> > +/*
> > + * _emit_flushp - Add opcode DMAFLUSHP into microcode (flush
> > peripheral).
> > + *
> > + * @buf -> The buffer which stored the microcode program.
> > + * @peri -> Peripheral ID as listed in DMA NPP.
> > + *
> > + * Return: Size of opcode.
> > + */
> > +static inline u32 _emit_flushp(u8 buf[], u8 peri)
> > +{
> > +       u8 *buffer = buf;
> > +
> > +       buffer[0] = CMD_DMAFLUSHP;
> > +       peri &= 0x1f;
> > +       peri <<= 3;
> These should be enums too so we don't have open-coded values in the
> code, Please check throughout.
> 
Okay, may be with #define or enum.
> > 
> > +       buffer[1] = peri;
> > +       DMA330_DBGCMD_DUMP(SZ_DMAFLUSHP, "\tDMAFLUSHP %u\n", peri
> > >> 3);
> > +       return SZ_DMAFLUSHP;
> > +}
> > +
> > +/**
> > + * _emit_ld - Add opcode DMALD into microcode (load).
> > + *
> > + * @buf: The buffer which stored the microcode program.
> > + * @cond: Execution criteria such as single, burst or always.
> > + *
> > + * Return: Size of opcode.
> > + */
> > +static inline u32 _emit_ld(u8 buf[], enum dma330_cond cond)
> > +{
> > +       buf[0] = CMD_DMALD;
> > +       if (cond == SINGLE)
> > +               buf[0] |= (0 << 1) | (1 << 0);
> More open-coded things
> 
Noted.
> > 
> > +       else if (cond == BURST)
> > +               buf[0] |= (1 << 1) | (1 << 0);
> > +       DMA330_DBGCMD_DUMP(SZ_DMALD, "\tDMALD%c\n",
> > +                         cond == SINGLE ? 'S' : (cond == BURST ?
> > 'B' : 'A'));
> > +       return SZ_DMALD;
> > +}
> > +
> > +/**
> > + * _emit_lp - Add opcode DMALP into microcode (loop).
> > + *
> > + * @buf: The buffer which stored the microcode program.
> > + * @loop: Selection of using counter 0 or 1  (valid value 0 or 1).
> > + * @cnt: number of iteration (valid value 1-256).
> > + *
> > + * Return: Size of opcode.
> > + */
> > +static inline u32 _emit_lp(u8 buf[], u32 loop, u32 cnt)
> > +{
> > +       u8 *buffer = buf;
> > +
> > +       buffer[0] = CMD_DMALP;
> > +       if (loop)
> > +               buffer[0] |= (1 << 1);
> > +       /* DMAC increments by 1 internally */
> > +       cnt--;
> > +       buffer[1] = cnt;
> > +       DMA330_DBGCMD_DUMP(SZ_DMALP, "\tDMALP_%c %u\n", loop ? '1'
> > : '0', cnt);
> > +       return SZ_DMALP;
> > +}
> > +
> > +/**
> > + * _emit_lpend - Add opcode DMALPEND into microcode (loop end).
> > + *
> > + * @buf: The buffer which stored the microcode program.
> > + * @arg: Structure _arg_lpend which contain all needed info.
> > + *     arg->cond -> Execution criteria such as single, burst or
> > always
> > + *     arg->forever -> Forever loop? used if DMALPFE started the
> > loop
> > + *     arg->bjump -> Backwards jump (relative location of
> > + *                   1st instruction in the loop.
> The members of the struct should be documented in the struct above,
> not here.
> 
Okay.
> > 
> > + *
> > + * Return: Size of opcode.
> > + */
> > +static inline u32 _emit_lpend(u8 buf[], const struct _arg_lpend
> > *arg)
> > +{
> > +       u8 *buffer = buf;
> > +       enum dma330_cond cond = arg->cond;
> > +       int forever = arg->forever;
> > +       u32 loop = arg->loop;
> > +       u8 bjump = arg->bjump;
> Why have these? Can you not use arg-bjump, etc., directly?
> 
Okay.
> > 
> > +
> > +       buffer[0] = CMD_DMALPEND;
> > +       if (loop)
> > +               buffer[0] |= (1 << 2);
> More open-coded things
> 
noted.
> > 
> > +       if (!forever)
> > +               buffer[0] |= (1 << 4);
> > +       if (cond == SINGLE)
> > +               buffer[0] |= (0 << 1) | (1 << 0);
> > +       else if (cond == BURST)
> > +               buffer[0] |= (1 << 1) | (1 << 0);
> > +
> > +       buffer[1] = bjump;
> > +       DMA330_DBGCMD_DUMP(SZ_DMALPEND, "\tDMALP%s%c_%c
> > bjmpto_%x\n",
> > +                         forever ? "FE" : "END",
> > +                         cond == SINGLE ? 'S' : (cond == BURST ?
> > 'B' : 'A'),
> > +                         loop ? '1' : '0',
> > +                         bjump);
> > +       return SZ_DMALPEND;
> > +}
> > +
> > +/**
> > + * _emit_kill - Add opcode DMAKILL into microcode (kill).
> > + *
> > + * @buf: The buffer which stored the microcode program.
> > + *
> > + * Return: Size of opcode.
> > + */
> > +static inline u32 _emit_kill(u8 buf[])
> > +{
> > +       buf[0] = CMD_DMAKILL;
> > +       return SZ_DMAKILL;
> > +}
> > +
> > +/**
> > + * _emit_mov - Add opcode DMAMOV into microcode (move).
> > + *
> > + * @buf: The buffer which stored the microcode program.
> > + * @dst: Destination register (valid value SAR[0b000], DAR[0b010],
> > + *      or CCR[0b001]).
> > + * @val: 32bit value that to be written into destination register.
> > + *
> > + * Return: Size of opcode.
> > + */
> > +static inline u32 _emit_mov(u8 buf[], enum dmamov_dst dst, u32
> > val)
> > +{
> > +       u8 *buffer = buf;
> > +
> > +       buffer[0] = CMD_DMAMOV;
> > +       buffer[1] = dst;
> > +       buffer[2] = val & 0xFF;
> > +       buffer[3] = (val >> 8) & 0xFF;
> > +       buffer[4] = (val >> 16) & 0xFF;
> > +       buffer[5] = (val >> 24) & 0xFF;
> Can you use put_unaligned_le32() ? Also check the same thing below.
> 
Okay.
> 
> > 
> > +       DMA330_DBGCMD_DUMP(SZ_DMAMOV, "\tDMAMOV %s 0x%x\n",
> > +                         dst == SAR ? "SAR" : (dst == DAR ? "DAR"
> > : "CCR"),
> > +                          val);
> > +       return SZ_DMAMOV;
> > +}
> > +
> > +/**
> > + * _emit_nop - Add opcode DMANOP into microcode (no operation).
> > + *
> > + * @buf: The buffer which stored the microcode program.
> > + *
> > + * Return: Size of opcode.
> > + */
> > +static inline u32 _emit_nop(u8 buf[])
> > +{
> > +       buf[0] = CMD_DMANOP;
> > +       DMA330_DBGCMD_DUMP(SZ_DMANOP, "\tDMANOP\n");
> please add a blank line before 'return'
> 
Okay.
> > 
> > +       return SZ_DMANOP;
> > +}
> > +
> > +/**
> > + * _emit_rmb - Add opcode DMARMB into microcode (read memory
> > barrier).
> > + *
> > + * @buf: The buffer which stored the microcode program.
> > + *
> > + * Return: Size of opcode.
> > + */
> > +static inline u32 _emit_rmb(u8 buf[])
> > +{
> > +       buf[0] = CMD_DMARMB;
> > +       DMA330_DBGCMD_DUMP(SZ_DMARMB, "\tDMARMB\n");
> > +       return SZ_DMARMB;
> > +}
> > +
> > +/**
> > + * _emit_sev - Add opcode DMASEV into microcode (send event).
> > + *
> > + * @buf: The buffer which stored the microcode program.
> > + * @ev: The event number (valid 0 - 31).
> > + *
> > + * Return: Size of opcode.
> > + */
> > +static inline u32 _emit_sev(u8 buf[], u8 ev)
> > +{
> > +       u8 *buffer = buf;
> > +
> > +       buffer[0] = CMD_DMASEV;
> > +       ev &= 0x1f;
> > +       ev <<= 3;
> > +       buffer[1] = ev;
> > +       DMA330_DBGCMD_DUMP(SZ_DMASEV, "\tDMASEV %u\n", ev >> 3);
> > +       return SZ_DMASEV;
> > +}
> > +
> > +/**
> > + * _emit_st - Add opcode DMAST into microcode (store).
> > + *
> > + * @buf: The buffer which stored the microcode program.
> > + * @cond: Execution criteria such as single, burst or always.
> > + *
> > + * Return: Size of opcode.
> > + */
> > +static inline u32 _emit_st(u8 buf[], enum dma330_cond cond)
> > +{
> > +       buf[0] = CMD_DMAST;
> > +       if (cond == SINGLE)
> > +               buf[0] |= (0 << 1) | (1 << 0);
> > +       else if (cond == BURST)
> > +               buf[0] |= (1 << 1) | (1 << 0);
> > +
> > +       DMA330_DBGCMD_DUMP(SZ_DMAST, "\tDMAST%c\n",
> > +               cond == SINGLE ? 'S' : (cond == BURST ? 'B' :
> > 'A'));
> > +       return SZ_DMAST;
> > +}
> > +
> > +/**
> > + * _emit_stp - Add opcode DMASTP into microcode (store and notify
> > peripheral).
> > + *
> > + * @buf: The buffer which stored the microcode program.
> > + * @cond: Execution criteria such as single, burst or always.
> > + * @peri: Peripheral ID as listed in DMA NPP.
> > + *
> > + * Return: Size of opcode.
> > + */
> > +static inline u32 _emit_stp(u8 buf[], enum dma330_cond cond, u8
> > peri)
> > +{
> > +       u8 *buffer = buf;
> > +
> > +       buffer[0] = CMD_DMASTP;
> > +       if (cond == BURST)
> > +               buf[0] |= (1 << 1);
> > +       peri &= 0x1f;
> > +       peri <<= 3;
> > +       buffer[1] = peri;
> > +       DMA330_DBGCMD_DUMP(SZ_DMASTP, "\tDMASTP%c %u\n",
> > +               cond == SINGLE ? 'S' : 'B', peri >> 3);
> > +       return SZ_DMASTP;
> > +}
> > +
> > +/**
> > + * _emit_stz - Add opcode DMASTZ into microcode (store zeros).
> > + *
> > + * @buf -> The buffer which stored the microcode program.
> > + *
> > + * Return: Size of opcode.
> > + */
> > +static inline u32 _emit_stz(u8 buf[])
> > +{
> > +       buf[0] = CMD_DMASTZ;
> > +       DMA330_DBGCMD_DUMP(SZ_DMASTZ, "\tDMASTZ\n");
> > +       return SZ_DMASTZ;
> > +}
> > +
> > +/**
> > + * _emit_wfp - Add opcode DMAWFP into microcode (wait for
> > peripheral).
> > + *
> > + * @buf: The buffer which stored the microcode program.
> > + * @cond: Execution criteria such as single, burst or always.
> > + * @peri: Peripheral ID as listed in DMA NPP.
> > + *
> > + * Return: Size of opcode.
> > + */
> > +static inline u32 _emit_wfp(u8 buf[], enum dma330_cond cond, u8
> > peri)
> > +{
> > +       u8 *buffer = buf;
> > +
> > +       buffer[0] = CMD_DMAWFP;
> > +       if (cond == SINGLE)
> > +               buffer[0] |= (0 << 1) | (0 << 0);
> > +       else if (cond == BURST)
> > +               buffer[0] |= (1 << 1) | (0 << 0);
> > +       else
> > +               buffer[0] |= (0 << 1) | (1 << 0);
> Perhaps these three values are the same for each command? If so, the
> three expressions (0, 1, 2) could go in an enum.
> 
Okay.
> > 
> > +
> > +       peri &= 0x1f;
> > +       peri <<= 3;
> > +       buffer[1] = peri;
> > +       DMA330_DBGCMD_DUMP(SZ_DMAWFP, "\tDMAWFP%c %u\n",
> > +               cond == SINGLE ? 'S' : (cond == BURST ? 'B' : 'P'),
> > peri >> 3);
> > +       return SZ_DMAWFP;
> > +}
> > +
> > +/**
> > + * _emit_wmb - Add opcode DMAWMB into microcode (write memory
> > barrier).
> > + *
> > + * @buf: The buffer which stored the microcode program.
> > + *
> > + * Return: Size of opcode.
> > + */
> > +static inline u32 _emit_wmb(u8 buf[])
> > +{
> > +       buf[0] = CMD_DMAWMB;
> > +       DMA330_DBGCMD_DUMP(SZ_DMAWMB, "\tDMAWMB\n");
> > +       return SZ_DMAWMB;
> > +}
> > +
> > +/**
> > + * _emit_go - Add opcode DMALGO into microcode (go).
> > + *
> > + * @buf: The buffer which stored the microcode program.
> > + * @arg: structure _arg_go which contain all needed info
> > + *     arg->chan -> channel number
> > + *     arg->addr -> start address of the microcode program
> > + *                  which will be wrote into CPC register
> > + *     arg->ns -> 1 for non secure, 0 for secure
> > + *                (if only DMA Manager is in secure).
> > + *
> > + * Return: Size of opcode.
> > + */
> > +static inline u32 _emit_go(u8 buf[], const struct _arg_go *arg)
> > +{
> > +       u8 *buffer = buf;
> > +       u8 chan = arg->chan;
> > +       u32 addr = arg->addr;
> > +       u32 ns = arg->ns;
> These local vars seem pointless as the are only used once. Maybe it's
> worth it for 'addr', but even there you should use the put_unaligned
> thing.
Okay.
> > 
> > +
> > +       buffer[0] = CMD_DMAGO;
> > +       buffer[0] |= (ns << 1);
> > +       buffer[1] = chan & 0x7;
> > +       buf[2] = addr & 0xFF;
> > +       buf[3] = (addr >> 8) & 0xFF;
> > +       buf[4] = (addr >> 16) & 0xFF;
> > +       buf[5] = (addr >> 24) & 0xFF;
> > +       return SZ_DMAGO;
> > +}
> > +
> > +/**
> > + * _prepare_ccr - Populate the CCR register.
> > + * @rqc: Request Configuration.
> > + *
> > + * Return: Channel Control register (CCR) Setting.
> > + */
> > +static inline u32 _prepare_ccr(const struct dma330_reqcfg *rqc)
> > +{
> > +       u32 ccr = 0;
> > +
> > +       if (rqc->src_inc)
> > +               ccr |= CC_SRCINC;
> > +       if (rqc->dst_inc)
> > +               ccr |= CC_DSTINC;
> > +
> > +       /* We set same protection levels for Src and DST for now */
> > +       if (rqc->privileged)
> > +               ccr |= CC_SRCPRI | CC_DSTPRI;
> > +       if (rqc->nonsecure)
> > +               ccr |= CC_SRCNS | CC_DSTNS;
> > +       if (rqc->insnaccess)
> > +               ccr |= CC_SRCIA | CC_DSTIA;
> > +
> > +       ccr |= (((rqc->brst_len - 1) & 0xf) << CC_SRCBRSTLEN_SHFT);
> > +       ccr |= (((rqc->brst_len - 1) & 0xf) << CC_DSTBRSTLEN_SHFT);
> > +
> > +       ccr |= (rqc->brst_size << CC_SRCBRSTSIZE_SHFT);
> > +       ccr |= (rqc->brst_size << CC_DSTBRSTSIZE_SHFT);
> > +
> > +       ccr |= (rqc->scctl << CC_SRCCCTRL_SHFT);
> > +       ccr |= (rqc->dcctl << CC_DSTCCTRL_SHFT);
> You don't need the outer brackets on these.
OKAY.
> 
> > 
> > +
> > +       ccr |= (rqc->swap << CC_SWAP_SHFT);
> > +       return ccr;
> > +}
> > +
> > +/**
> > + * dma330_until_dmac_idle - Wait until DMA Manager is idle.
> > + *
> > + * @plat: Pointer to struct dma_dma330_platdata.
> > + * @timeout_ms: Timeout (in miliseconds).
> > + *
> > + * Return: Negative value for error / timeout ocurred before idle,
> > + *        0 for successful.
> > + */
> > +static int dma330_until_dmac_idle(struct dma_dma330_platdata
> > *plat,
> > +                                 const u32 timeout_ms)
> > +{
> > +       void __iomem *regs = plat->base;
> > +
> > +       return wait_for_bit(__func__,
> > +                           (const u32 *)(uintptr_t)(regs +
> > DBGSTATUS),
> > +                           DBG_BUSY, 0, timeout_ms, false);
> > +}
> > +
> > +/**
> > + * dma330_execute_dbginsn - Execute debug instruction such as
> > DMAGO and DMAKILL.
> > + *
> > + * @insn: The buffer which stored the debug instruction.
> > + * @plat: Pointer to struct dma_dma330_platdata.
> > + * @timeout_ms: Timeout (in miliseconds).
> > + *
> > + * Return: void.
> > + */
> > +static inline void dma330_execute_dbginsn(u8 insn[],
> > +                                         struct
> > dma_dma330_platdata *plat,
> > +                                         const u32 timeout_ms)
> > +{
> > +       void __iomem *regs = plat->base;
> > +       u32 val;
> > +
> > +       val = (insn[0] << 16) | (insn[1] << 24);
> > +       if (!plat->dma330.channel0_manager1)
> > +               val |= (1 << 0);
> > +       val |= (plat->dma330.channel_num << 8); /* Channel Number
> > */
> > +       writel(val, regs + DBGINST0);
> > +       val = insn[2];
> > +       val = val | (insn[3] << 8);
> > +       val = val | (insn[4] << 16);
> > +       val = val | (insn[5] << 24);
> > +       writel(val, regs + DBGINST1);
> > +
> > +       /* If timed out due to halted state-machine */
> > +       if (dma330_until_dmac_idle(plat, timeout_ms)) {
> > +               debug("DMAC halted!\n");
> > +               return;
> > +       }
> > +       /* Get going */
> > +       writel(0, regs + DBGCMD);
> > +}
> > +
> > +/**
> > + * dma330_getstate - Get the status of current channel or manager.
> > + *
> > + * @plat: Pointer to struct dma_dma330_platdata.
> > + *
> > + * Return: Status state of current channel or current manager.
> > + */
> > +static inline u32 dma330_getstate(struct dma_dma330_platdata
> > *plat)
> > +{
> > +       void __iomem *regs = plat->base;
> > +       u32 val;
> > +
> > +       if (plat->dma330.channel0_manager1)
> > +               val = readl((uintptr_t)(regs + DS)) & 0xf;
> > +       else
> > +               val = readl((uintptr_t)(regs + CS(plat-
> > >dma330.channel_num))) &
> > +                           0xf;
> > +
> > +       switch (val) {
> > +       case DS_ST_STOP:
> > +               return DMA330_STATE_STOPPED;
> > +       case DS_ST_EXEC:
> > +               return DMA330_STATE_EXECUTING;
> > +       case DS_ST_CMISS:
> > +               return DMA330_STATE_CACHEMISS;
> > +       case DS_ST_UPDTPC:
> > +               return DMA330_STATE_UPDTPC;
> > +       case DS_ST_WFE:
> > +               return DMA330_STATE_WFE;
> > +       case DS_ST_FAULT:
> > +               return DMA330_STATE_FAULTING;
> > +       case DS_ST_ATBRR:
> > +               if (plat->dma330.channel0_manager1)
> > +                       return DMA330_STATE_INVALID;
> > +               else
> > +                       return DMA330_STATE_ATBARRIER;
> > +       case DS_ST_QBUSY:
> > +               if (plat->dma330.channel0_manager1)
> > +                       return DMA330_STATE_INVALID;
> > +               else
> > +                       return DMA330_STATE_QUEUEBUSY;
> > +       case DS_ST_WFP:
> > +               if (plat->dma330.channel0_manager1)
> > +                       return DMA330_STATE_INVALID;
> > +               else
> > +                       return DMA330_STATE_WFP;
> > +       case DS_ST_KILL:
> > +               if (plat->dma330.channel0_manager1)
> > +                       return DMA330_STATE_INVALID;
> > +               else
> > +                       return DMA330_STATE_KILLING;
> > +       case DS_ST_CMPLT:
> > +               if (plat->dma330.channel0_manager1)
> > +                       return DMA330_STATE_INVALID;
> > +               else
> > +                       return DMA330_STATE_COMPLETING;
> > +       case DS_ST_FLTCMP:
> > +               if (plat->dma330.channel0_manager1)
> > +                       return DMA330_STATE_INVALID;
> > +               else
> > +                       return DMA330_STATE_FAULT_COMPLETING;
> > +       default:
> > +               return DMA330_STATE_INVALID;
> > +       }
> > +}
> > +
> > +/**
> > + * dma330_trigger - Execute the DMAGO through debug instruction.
> > + *
> > + * @plat: Pointer to struct dma_dma330_platdata.
> > + * @timeout_ms: Timeout (in miliseconds).
> > + *
> > + * When the DMA manager executes Go for a DMA channel that is in
> > the Stopped
> > + * state, it moves a 32-bit immediate into the program counter,
> > then setting
> > + * its security state and updating DMA channel to the Executing
> > state.
> > + *
> > + * Return: Negative value for error as DMA is not ready. 0 for
> > successful.
> > + */
> > +static int dma330_trigger(struct dma_dma330_platdata *plat,
> > +                         const u32 timeout_ms)
> > +{
> > +       u8 insn[6] = {0, 0, 0, 0, 0, 0};
> > +       struct _arg_go go;
> > +
> > +       /*
> > +        * Return if already ACTIVE. It will ensure DMAGO is
> > executed at
> > +        * STOPPED state too
> > +        */
> > +       plat->dma330.channel0_manager1 = 0;
> > +       if (dma330_getstate(plat) !=
> > +               DMA330_STATE_STOPPED)
> > +               return -EPERM;
> > +
> > +       go.chan = plat->dma330.channel_num;
> > +       go.addr = (uintptr_t)plat->dma330.buf;
> > +
> > +       if (!plat->dma330.secure)
> > +               go.ns = 1; /* non-secure condition */
> > +       else
> > +               go.ns = 0; /* secure condition */
> > +
> > +       _emit_go(insn, &go);
> > +
> > +       /* Only manager can execute GO */
> > +       plat->dma330.channel0_manager1 = 1;
> > +       dma330_execute_dbginsn(insn, plat, timeout_ms);
> > +       return 0;
> > +}
> > +
> > +/**
> > + * dma330_stop - Stop the manager or channel.
> > + *
> > + * @plat: Pointer to struct dma_dma330_platdata.
> > + * @timeout_ms: Timeout (in miliseconds).
> > + *
> > + * Stop the manager/channel or killing them and ensure they reach
> > to stop
> > + * state.
> > + *
> > + * Return: void.
> > + */
> > +static void dma330_stop(struct dma_dma330_platdata *plat, const
> > u32 timeout_ms)
> > +{
> > +       u8 insn[6] = {0, 0, 0, 0, 0, 0};
> > +
> > +       /* If fault completing, wait until reach faulting and
> > killing state */
> > +       if (dma330_getstate(plat) == DMA330_STATE_FAULT_COMPLETING)
> > +               UNTIL(plat, DMA330_STATE_FAULTING |
> > DMA330_STATE_KILLING);
> > +
> > +       /* Return if nothing needs to be done */
> > +       if (dma330_getstate(plat) == DMA330_STATE_COMPLETING ||
> > +           dma330_getstate(plat) == DMA330_STATE_KILLING ||
> > +           dma330_getstate(plat) == DMA330_STATE_STOPPED)
> > +               return;
> > +
> > +       /* Kill it to ensure it reach to stop state */
> > +       _emit_kill(insn);
> > +
> > +       /* Execute the KILL instruction through debug registers */
> > +       dma330_execute_dbginsn(insn, plat, timeout_ms);
> > +}
> > +
> > +/**
> > + * dma330_start - Execute the command list through DMAGO and debug
> > instruction.
> > + *
> > + * @plat: Pointer to struct dma_dma330_platdata.
> > + * @timeout_ms: Timeout (in miliseconds).
> > + *
> > + * Return: Negative value for error where DMA is not ready. 0 for
> > successful.
> > + */
> > +static int dma330_start(struct dma_dma330_platdata *plat, const
> > u32 timeout_ms)
> > +{
> > +       debug("INFO: DMA BASE Address = 0x%08x\n",
> > +             (u32)(uintptr_t)plat->base);
> > +
> > +       switch (dma330_getstate(plat)) {
> > +       case DMA330_STATE_FAULT_COMPLETING:
> > +               UNTIL(plat, DMA330_STATE_FAULTING |
> > DMA330_STATE_KILLING);
> > +
> > +               if (dma330_getstate(plat) == DMA330_STATE_KILLING)
> > +                       UNTIL(plat, DMA330_STATE_STOPPED);
> > +
> > +       case DMA330_STATE_FAULTING:
> > +               dma330_stop(plat, timeout_ms);
> > +
> > +       case DMA330_STATE_KILLING:
> > +       case DMA330_STATE_COMPLETING:
> > +               UNTIL(plat, DMA330_STATE_STOPPED);
> > +
> > +       case DMA330_STATE_STOPPED:
> > +               return dma330_trigger(plat, timeout_ms);
> > +
> > +       case DMA330_STATE_WFP:
> > +       case DMA330_STATE_QUEUEBUSY:
> > +       case DMA330_STATE_ATBARRIER:
> > +       case DMA330_STATE_UPDTPC:
> > +       case DMA330_STATE_CACHEMISS:
> > +       case DMA330_STATE_EXECUTING:
> > +               return -ESRCH;
> > +       /* For RESUME, nothing yet */
> > +       case DMA330_STATE_WFE:
> > +       default:
> > +               return -ESRCH;
> > +       }
> > +}
> > +
> > +/**
> > + * dma330_transfer_start - DMA start to run.
> > + *
> > + * @plat: Pointer to struct dma_dma330_platdata.
> > + *
> > + * DMA start to excecute microcode command list.
> > + *
> > + * Return: Negative value for error or not successful. 0 for
> > successful.
> > + */
> > +static int dma330_transfer_start(struct dma_dma330_platdata *plat)
> > +{
> > +       const u32 timeout_ms = 1000;
> > +
> > +       /* Execute the command list */
> > +       return dma330_start(plat, timeout_ms);
> > +}
> > +
> > +/**
> > + * dma330_transfer_finish - DMA polling until execution finish or
> > error.
> > + *
> > + * @plat: Pointer to struct dma_dma330_platdata.
> > + *
> > + * DMA polling until excution finish and checking the state
> > status.
> > + *
> > + * Return: Negative value for state error or not successful. 0 for
> > successful.
> > + */
> > +static int dma330_transfer_finish(struct dma_dma330_platdata
> > *plat)
> > +{
> > +       void __iomem *regs = plat->base;
> > +
> > +       if (!plat->dma330.buf) {
> > +               debug("ERROR DMA330 : DMA Microcode buffer pointer
> > is NULL\n");
> > +               return -EINVAL;
> > +       }
> > +
> > +       plat->dma330.channel0_manager1 = 0;
> > +
> > +       /* Wait until finish execution to ensure we compared
> > correct result*/
> > +       UNTIL(plat, DMA330_STATE_STOPPED | DMA330_STATE_FAULTING);
> > +
> > +       /* check the state */
> > +       if (dma330_getstate(plat) == DMA330_STATE_FAULTING) {
> > +               debug("FAULT Mode: Channel %d Faulting, FTR =
> > 0x%08x,",
> > +                    plat->dma330.channel_num,
> > +                    readl(regs + FTC(plat->dma330.channel_num)));
> > +               debug("CPC = 0x%08lx\n",
> > +                    (readl(regs + CPC(plat->dma330.channel_num))
> > +                    - (uintptr_t)plat->dma330.buf));
> > +               return -EPROTO;
> > +       }
> > +       return 0;
> > +}
> > +
> > +/**
> > + * dma330_transfer_setup - DMA transfer setup (DMA_MEM_TO_MEM,
> > DMA_MEM_TO_DEV
> > + *                        or DMA_DEV_TO_MEM).
> > + *
> > + * @plat: Pointer to struct dma_dma330_platdata.
> > + *
> > + * For Peripheral transfer, the FIFO threshold value is expected
> > at
> > + * 2 ^ plat->brst_size * plat->brst_len.
> > + *
> > + * Return: Negative value for error or not successful. 0 for
> > successful.
> > + */
> > +static int dma330_transfer_setup(struct dma_dma330_platdata *plat)
> This function is extremely long, Can it be split into separete
> sub-functions which perform the work, if the work can be split
> sensibly into different parts?
> 
Let me see how the work can be further split into different parts.
> > 
> > +{
> > +       /* Variable declaration */
> > +       /* Buffer offset clear to 0 */
> > +       int off = 0;
> > +       int ret = 0;
> > +       /* For DMALPEND */
> > +       u32 loopjmp0, loopjmp1;
> > +       /* Loop count 0 */
> > +       u32 lcnt0 = 0;
> > +       /* Loop count 1 */
> > +       u32 lcnt1 = 0;
> > +       u32 brst_size = 0;
> > +       u32 brst_len = 0;
> > +       u32 data_size_byte = plat->dma330.size_byte;
> > +       /* Strong order memory is required to store microcode
> > command list */
> > +       u8 *buf = (u8 *)plat->dma330.buf;
> > +       /* Channel Control Register */
> > +       u32 ccr = 0;
> > +       struct dma330_reqcfg reqcfg;
> > +
> > +       if (!buf) {
> > +               debug("ERROR DMA330 : DMA Microcode buffer pointer
> > is NULL\n");
> > +               return -EINVAL;
> > +       }
> > +
> > +       if (plat->dma330.transfer_type == DMA_MEM_TO_DEV)
> > +               debug("INFO: mem2perip");
> > +       else if (plat->dma330.transfer_type == DMA_DEV_TO_MEM)
> > +               debug("INFO: perip2mem");
> > +       else
> > +               debug("INFO: DMA_MEM_TO_MEM");
> > +
> > +       debug(" - 0x%x%x -> 0x%x%x\nsize=%08x brst_size=2^%d ",
> > +             (u32)(plat->dma330.src_addr >> 32),
> > +             (u32)plat->dma330.src_addr,
> > +             (u32)(plat->dma330.dst_addr >> 32),
> > +             (u32)plat->dma330.dst_addr,
> > +             data_size_byte,
> > +             plat->brst_size);
> > +
> > +       debug("brst_len=%d singles_brst_size=2^%d\n", plat-
> > >brst_len,
> > +             plat->dma330.single_brst_size);
> > +
> > +       /* brst_size = 2 ^ plat->brst_size */
> > +       brst_size = 1 << plat->brst_size;
> > +
> > +       /* Fool proof checking */
> > +       if (plat->brst_size < 0 || plat->brst_size >
> > +               plat->max_brst_size) {
> > +               debug("ERROR DMA330: brst_size must 0-%d (not
> > %d)\n",
> > +                       plat->max_brst_size, plat->brst_size);
> > +               return -EINVAL;
> > +       }
> > +       if (plat->dma330.single_brst_size < 0 ||
> > +               plat->dma330.single_brst_size > plat-
> > >max_brst_size) {
> > +               debug("ERROR DMA330 : single_brst_size must 0-%d
> > (not %d)\n",
> > +                       plat->max_brst_size, plat-
> > >dma330.single_brst_size);
> > +               return -EINVAL;
> > +       }
> > +       if (plat->brst_len < 1 || plat->brst_len > 16) {
> > +               debug("ERROR DMA330 : brst_len must 1-16 (not
> > %d)\n",
> > +                       plat->brst_len);
> > +               return -EINVAL;
> > +       }
> > +       if (plat->dma330.src_addr & (brst_size - 1)) {
> > +               debug("ERROR DMA330 : Source address unaligned\n");
> > +               return -EINVAL;
> > +       }
> > +       if (plat->dma330.dst_addr & (brst_size - 1)) {
> > +               debug("ERROR DMA330 : Destination address
> > unaligned\n");
> > +               return -EINVAL;
> > +       }
> > +
> > +       /* Setup the command list */
> > +       /* DMAMOV SAR, x->src_addr */
> > +       off += _emit_mov(&buf[off], SAR, plat->dma330.src_addr);
> > +       /* DMAMOV DAR, x->dst_addr */
> > +       off += _emit_mov(&buf[off], DAR, plat->dma330.dst_addr);
> > +       /* DMAFLUSHP P(periheral_id) */
> > +       if (plat->dma330.transfer_type != DMA_MEM_TO_MEM)
> > +               off += _emit_flushp(&buf[off], plat-
> > >dma330.peripheral_id);
> > +
> > +       /* Preparing the CCR value */
> > +       if (plat->dma330.transfer_type == DMA_MEM_TO_DEV) {
> > +               /* Disable auto increment */
> > +               reqcfg.dst_inc = 0;
> > +               /* Enable auto increment */
> > +               reqcfg.src_inc = 1;
> > +       } else if (plat->dma330.transfer_type == DMA_DEV_TO_MEM) {
> > +               reqcfg.dst_inc = 1;
> > +               reqcfg.src_inc = 0;
> > +       } else {
> > +               /* DMA_MEM_TO_MEM */
> > +               reqcfg.dst_inc = 1;
> > +               reqcfg.src_inc = 1;
> > +       }
> > +
> > +       if (!plat->dma330.secure)
> > +               reqcfg.nonsecure = 1; /* Non Secure mode */
> > +       else
> > +               reqcfg.nonsecure = 0; /* Secure mode */
> > +
> > +       if (plat->dma330.enable_cache1 == 0) {
> > +               /* Noncacheable but bufferable */
> > +               reqcfg.dcctl = 0x1;
> > +               reqcfg.scctl = 0x1;
> > +       } else {
> > +               if (plat->dma330.transfer_type == DMA_MEM_TO_DEV) {
> > +                       reqcfg.dcctl = 0x1;
> > +                       /* Cacheable write back */
> > +                       reqcfg.scctl = 0x7;
> > +               } else if (plat->dma330.transfer_type ==
> > DMA_DEV_TO_MEM) {
> > +                       reqcfg.dcctl = 0x7;
> > +                       reqcfg.scctl = 0x1;
> > +               } else {
> > +                       reqcfg.dcctl = 0x7;
> > +                       reqcfg.scctl = 0x7;
> > +               }
> > +       }
> > +       /* 1 - Privileged  */
> > +       reqcfg.privileged = 1;
> > +       /* 0 - Data access */
> > +       reqcfg.insnaccess = 0;
> > +       /* 0 - No endian swap */
> I am not keen on these comments. The struct members should be
> documented when the struct is declared, not here.
> 
Okay.
> > 
> > +       reqcfg.swap = 0;
> > +       /* DMA burst length */
> > +       reqcfg.brst_len = plat->brst_len;
> > +       /* DMA burst size */
> > +       reqcfg.brst_size = plat->brst_size;
> > +       /* Preparing the CCR value */
> > +       ccr = _prepare_ccr(&reqcfg);
> > +       /* DMAMOV CCR, ccr */
> > +       off += _emit_mov(&buf[off], CCR, ccr);
> > +
> > +       /* BURST */
> > +       /* Can initiate a burst? */
> These two comments should be joined
> 
Okay. Actually comments helping to track the operation easily.
> > 
> > +       while (data_size_byte >= brst_size * plat->brst_len) {
> > +               lcnt0 = data_size_byte / (brst_size * plat-
> > >brst_len);
> > +               lcnt1 = 0;
> > +               if (lcnt0 >= 256 * 256) {
> > +                       lcnt0 = 256;
> > +                       lcnt1 = 256;
> > +               } else if (lcnt0 >= 256) {
> > +                       lcnt1 = lcnt0 / 256;
> > +                       lcnt0 = 256;
> > +               }
> > +               data_size_byte = data_size_byte -
> > +                       (brst_size * plat->brst_len * lcnt0 *
> > lcnt1);
> > +
> > +               debug("Transferring 0x%08x Remain 0x%08x\n",
> > (brst_size *
> > +                       plat->brst_len * lcnt0 * lcnt1),
> > data_size_byte);
> > +               debug("Running burst - brst_size=2^%d, brst_len=%d,
> > ",
> > +                     plat->brst_size, plat->brst_len);
> > +               debug("lcnt0=%d, lcnt1=%d\n", lcnt0, lcnt1);
> > +
> > +               if (lcnt1) {
> > +                       /* DMALP1 */
> > +                       off += _emit_lp(&buf[off], 1, lcnt1);
> > +                       loopjmp1 = off;
> > +               }
> > +               /* DMALP0 */
> What does this comment mean?
> 
This means below _emit_lp would insert the microcode DMALP(looping)
into buffer[off]
> > 
> > +               off += _emit_lp(&buf[off], 0, lcnt0);
> > +               loopjmp0 = off;
> > +               /* DMAWFP periheral_id, burst */
> > +               if (plat->dma330.transfer_type != DMA_MEM_TO_MEM)
> > +                       off += _emit_wfp(&buf[off], BURST,
> > +                               plat->dma330.peripheral_id);
> > +               /* DMALD */
> > +               off += _emit_ld(&buf[off], ALWAYS);
> > +
> > +               WATCHDOG_RESET();
> > +
> > +               /* DMARMB */
> > +               off += _emit_rmb(&buf[off]);
> > +               /* DMASTPB peripheral_id */
> > +               if (plat->dma330.transfer_type != DMA_MEM_TO_MEM)
> > +                       off += _emit_stp(&buf[off], BURST,
> > +                               plat->dma330.peripheral_id);
> > +               else
> > +                       off += _emit_st(&buf[off], ALWAYS);
> > +               /* DMAWMB */
> > +               off += _emit_wmb(&buf[off]);
> > +               /* DMALP0END */
> > +               struct _arg_lpend lpend;
> > +
> > +               lpend.cond = ALWAYS;
> > +               lpend.forever = 0;
> > +               /* Loop cnt 0 */
> > +               lpend.loop = 0;
> > +               lpend.bjump = off - loopjmp0;
> > +               off += _emit_lpend(&buf[off], &lpend);
> > +               /* DMALP1END */
> > +               if (lcnt1) {
> > +                       struct _arg_lpend lpend;
> > +
> > +                       lpend.cond = ALWAYS;
> > +                       lpend.forever = 0;
> > +                       lpend.loop = 1;         /* loop cnt 1*/
> > +                       lpend.bjump = off - loopjmp1;
> > +                       off += _emit_lpend(&buf[off], &lpend);
> > +               }
> > +               /* Ensure the microcode don't exceed buffer size */
> > +               if (off > plat->buf_size) {
> > +                       debug("ERROR DMA330 : Exceed buffer
> > size\n");
> > +                       return -ENOMEM;
> > +               }
> > +       }
> > +
> > +       /* SINGLE */
> > +       brst_len = 1;
> > +       /* brst_size = 2 ^ plat->dma330.single_brst_size; */
> > +       brst_size = (1 << plat->dma330.single_brst_size);
> > +       lcnt0 = data_size_byte / (brst_size * brst_len);
> > +
> > +       /* Ensure all data will be transferred */
> > +       data_size_byte = data_size_byte -
> > +               (brst_size * brst_len * lcnt0);
> > +       if (data_size_byte) {
> > +               debug("ERROR DMA330 : Detected the possibility of
> > ");
> > +               debug("untransfered data. Please ensure correct
> > single ");
> > +               debug("burst size\n");
> > +       }
> > +
> > +       if (lcnt0) {
> > +               debug("Transferring 0x%08x Remain 0x%08x\n",
> > (brst_size *
> > +                       brst_len * lcnt0 * lcnt1), data_size_byte);
> > +               debug("Running single - brst_size=2^%d,
> > brst_len=%d, ",
> > +                     brst_size, brst_len);
> > +               debug("lcnt0=%d\n", lcnt0);
> > +
> > +               /* Preparing the CCR value */
> > +               /* DMA burst length */
> > +               reqcfg.brst_len = brst_len;
> > +               /* DMA burst size */
> > +               reqcfg.brst_size = brst_size;
> > +               ccr = _prepare_ccr(&reqcfg);
> > +               /* DMAMOV CCR, ccr */
> > +               off += _emit_mov(&buf[off], CCR, ccr);
> > +
> > +               /* DMALP0 */
> > +               off += _emit_lp(&buf[off], 0, lcnt0);
> > +               loopjmp0 = off;
> > +               /* DMAWFP peripheral_id, single */
> > +               if (plat->dma330.transfer_type != DMA_MEM_TO_MEM)
> > +                       off += _emit_wfp(&buf[off], SINGLE,
> > +                               plat->dma330.peripheral_id);
> > +
> > +               /* DMALD */
> > +               off += _emit_ld(&buf[off], ALWAYS);
> > +               /* DMARMB */
> > +               off += _emit_rmb(&buf[off]);
> > +               /* DMASTPS peripheral_id */
> > +               if (plat->dma330.transfer_type != DMA_MEM_TO_MEM)
> > +                       off += _emit_stp(&buf[off], SINGLE,
> > +                               plat->dma330.peripheral_id);
> > +               else
> > +                       off += _emit_st(&buf[off], ALWAYS);
> > +
> > +               /* DMAWMB */
> > +               off += _emit_wmb(&buf[off]);
> > +               /* DMALPEND */
> > +               struct _arg_lpend lpend1;
> > +
> > +               lpend1.cond = ALWAYS;
> > +               lpend1.forever = 0;
> > +               /* loop cnt 0 */
> > +               lpend1.loop = 0;
> > +               lpend1.bjump = off - loopjmp0;
> > +               off += _emit_lpend(&buf[off], &lpend1);
> > +               /* Ensure the microcode don't exceed buffer size */
> > +               if (off > plat->buf_size) {
> > +                       puts("ERROR DMA330 : Exceed buffer
> > size\n");
> > +                       return -ENOMEM;
> > +               }
> > +       }
> > +
> > +       /* DMAEND */
> > +       off += _emit_end(&buf[off]);
> > +
> > +       ret = dma330_transfer_start(plat);
> > +       if (ret)
> > +               return ret;
> > +
> > +       return dma330_transfer_finish(plat);
> > +}
> > +
> > +/**
> > + * dma330_transfer_zeroes - DMA transfer zeros.
> > + *
> > + * @plat: Pointer to struct dma_dma330_platdata.
> > + *
> > + * Used to write zeros to a memory chunk for memory scrubbing
> > purpose.
> > + *
> > + * Return: Negative value for error or not successful. 0 for
> > successful.
> > + */
> > +static int dma330_transfer_zeroes_setup(struct dma_dma330_platdata
> > *plat)
> Can this function be combined with the above somehow? It seems like a
> lot of duplicate code.
> 
Let me see how to merge them.
> > 
> > +{
> > +       /* Variable declaration */
> > +       /* Buffer offset clear to 0 */
> > +       int off = 0;
> > +       int ret = 0;
> > +       /* For DMALPEND */
> > +       u32 loopjmp0, loopjmp1;
> > +       /* Loop count 0 */
> > +       u32 lcnt0 = 0;
> > +       /* Loop count 1 */
> > +       u32 lcnt1 = 0;
> > +       u32 brst_size = 0;
> > +       u32 brst_len = 0;
> > +       u32 data_size_byte = plat->dma330.size_byte;
> > +       /* Strong order memory is required to store microcode
> > command list */
> > +       u8 *buf = (u8 *)plat->dma330.buf;
> > +       /* Channel Control Register */
> > +       u32 ccr = 0;
> > +       struct dma330_reqcfg reqcfg;
> > +
> > +       if (!buf) {
> > +               debug("ERROR DMA330 : DMA Microcode buffer pointer
> > is NULL\n");
> > +               return -EINVAL;
> > +       }
> > +
> > +       debug("INFO: Write zeros -> ");
> > +       debug("0x%x%x size=0x%08x\n",
> > +             (u32)(plat->dma330.dst_addr >> 32),
> > +             (u32)plat->dma330.dst_addr,
> > +             data_size_byte);
> > +
> > +       plat->dma330.single_brst_size = 1;
> > +
> > +       /* burst_size = 2 ^ plat->brst_size */
> > +       brst_size = 1 << plat->brst_size;
> > +
> > +       /* Setup the command list */
> > +       /* DMAMOV DAR, x->dst_addr */
> > +       off += _emit_mov(&buf[off], DAR, plat->dma330.dst_addr);
> > +
> > +       /* Preparing the CCR value */
> > +       /* Enable auto increment */
> > +       reqcfg.dst_inc = 1;
> > +       /* Disable auto increment (not applicable) */
> > +       reqcfg.src_inc = 0;
> > +       /* Noncacheable but bufferable */
> > +       reqcfg.dcctl = 0x1;
> > +       /* Noncacheable and bufferable */
> > +       reqcfg.scctl = 0x1;
> > +       /* 1 - Privileged  */
> > +       reqcfg.privileged = 1;
> > +       /* 0 - Data access */
> > +       reqcfg.insnaccess = 0;
> > +       /* 0 - No endian swap */
> > +       reqcfg.swap = 0;
> > +
> > +       if (!plat->dma330.secure)
> > +               reqcfg.nonsecure = 1; /* Non Secure mode */
> > +       else
> > +               reqcfg.nonsecure = 0; /* Secure mode */
> > +
> > +       /* DMA burst length */
> > +       reqcfg.brst_len = plat->brst_len;
> > +       /* DMA burst size */
> > +       reqcfg.brst_size = plat->brst_size;
> > +       /* Preparing the CCR value */
> > +       ccr = _prepare_ccr(&reqcfg);
> > +       /* DMAMOV CCR, ccr */
> > +       off += _emit_mov(&buf[off], CCR, ccr);
> > +
> > +       /* BURST */
> > +       /* Can initiate a burst? */
> > +       while (data_size_byte >= brst_size * plat->brst_len) {
> > +               lcnt0 = data_size_byte / (brst_size * plat-
> > >brst_len);
> > +               lcnt1 = 0;
> > +               if (lcnt0 >= 256 * 256) {
> > +                       lcnt0 = 256;
> > +                       lcnt1 = 256;
> > +               } else if (lcnt0 >= 256) {
> > +                       lcnt1 = lcnt0 / 256;
> > +                       lcnt0 = 256;
> > +               }
> > +
> > +               data_size_byte = data_size_byte -
> > +                       (brst_size * plat->brst_len * lcnt0 *
> > lcnt1);
> > +
> > +               debug("Transferring 0x%08x Remain 0x%08x\n",
> > (brst_size *
> > +                       plat->brst_len * lcnt0 * lcnt1),
> > data_size_byte);
> > +               debug("Running burst - brst_size=2^%d,
> > brst_len=%d,",
> > +                     plat->brst_size, plat->brst_len);
> > +               debug("lcnt0=%d, lcnt1=%d\n", lcnt0, lcnt1);
> > +
> > +               if (lcnt1) {
> > +                       /* DMALP1 */
> > +                       off += _emit_lp(&buf[off], 1, lcnt1);
> > +                       loopjmp1 = off;
> > +               }
> > +               /* DMALP0 */
> > +               off += _emit_lp(&buf[off], 0, lcnt0);
> > +               loopjmp0 = off;
> > +               /* DMALSTZ */
> > +               off += _emit_stz(&buf[off]);
> > +
> > +               WATCHDOG_RESET();
> > +
> > +               /* DMALP0END */
> > +               struct _arg_lpend lpend;
> > +
> > +               lpend.cond = ALWAYS;
> > +               lpend.forever = 0;
> > +               /* Loop cnt 0 */
> > +               lpend.loop = 0;
> > +               lpend.bjump = off - loopjmp0;
> > +               off += _emit_lpend(&buf[off], &lpend);
> > +               /* DMALP1END */
> > +               if (lcnt1) {
> > +                       struct _arg_lpend lpend;
> > +
> > +                       lpend.cond = ALWAYS;
> > +                       lpend.forever = 0;
> > +                       /* Loop cnt 1*/
> > +                       lpend.loop = 1;
> > +                       lpend.bjump = off - loopjmp1;
> > +                       off += _emit_lpend(&buf[off], &lpend);
> > +               }
> > +               /* Ensure the microcode don't exceed buffer size */
> > +               if (off > plat->buf_size) {
> > +                       printf("off = %d\n", off);
> > +                       debug("ERROR DMA330 : Exceed buffer size
> > off %d\n",
> > +                             off);
> > +                       return -ENOMEM;
> > +               }
> > +       }
> > +
> > +       /* SINGLE */
> > +       brst_len = 1;
> > +       /* brst_size = 2 ^ plat->dma330.single_brst_size */
> > +       brst_size = (1 << plat->dma330.single_brst_size);
> > +       lcnt0 = data_size_byte / (brst_size * brst_len);
> > +
> > +       /* ensure all data will be transferred */
> > +       data_size_byte = data_size_byte -
> > +               (brst_size * brst_len * lcnt0);
> > +       if (data_size_byte) {
> > +               debug("ERROR DMA330 : Detected the possibility of
> > ");
> > +               debug("untransfered data. Please ensure correct
> > single ");
> > +               debug("burst size\n");
> > +       }
> > +
> > +       if (lcnt0) {
> > +               debug("Transferring 0x%08x Remain 0x%08x\n",
> > (brst_size *
> > +                    brst_len * lcnt0), data_size_byte);
> > +               debug("Running single - brst_size=2^%d,
> > brst_len=%d, ",
> > +                     brst_size, brst_len);
> > +               debug("lcnt0=%d\n", lcnt0);
> > +
> > +               /* Preparing the CCR value */
> > +               /* DMA burst length */
> > +               reqcfg.brst_len = brst_len;
> > +               /* DMA burst size */
> > +               reqcfg.brst_size = brst_size;
> > +               ccr = _prepare_ccr(&reqcfg);
> > +               /* DMAMOV CCR, ccr */
> > +               off += _emit_mov(&buf[off], CCR, ccr);
> > +
> > +               /* DMALP0 */
> > +               off += _emit_lp(&buf[off], 0, lcnt0);
> > +               loopjmp0 = off;
> > +               /* DMALSTZ */
> > +               off += _emit_stz(&buf[off]);
> > +               /* DMALPEND */
> > +               struct _arg_lpend lpend1;
> > +
> > +               lpend1.cond = ALWAYS;
> > +               lpend1.forever = 0;
> > +               /* Loop cnt 0 */
> > +               lpend1.loop = 0;
> > +               lpend1.bjump = off - loopjmp0;
> > +               off += _emit_lpend(&buf[off], &lpend1);
> > +               /* Ensure the microcode don't exceed buffer size */
> > +               if (off > plat->buf_size) {
> > +                       debug("ERROR DMA330 : Exceed buffer
> > size\n");
> > +                       return -ENOMEM;
> > +               }
> > +       }
> > +
> > +       /* DMAEND */
> > +       off += _emit_end(&buf[off]);
> > +
> > +       ret = dma330_transfer_start(plat);
> > +       if (ret)
> > +               return ret;
> > +
> > +       return dma330_transfer_finish(plat);
> > +}
> > +
> > +static int dma330_transfer_zeroes(struct udevice *dev, void *dst,
> > size_t len)
> > +{
> > +       struct dma_dma330_platdata *plat = dev->platdata;
> > +       int ret = 0;
> > +
> > +       /* If DMA access is set to secure, base change to DMA
> > secure base */
> > +       if (plat->dma330.secure)
> > +               plat->base += 0x1000;
> What is this? I suggest you set up the secure base separately in
> 'plat', e.g. with a base_secure member. You should not change
> platform
> data while running.
> 
> You can pass the base address to dma330_transfer_zeroes_setup,
> perhaps.
> 
Okay.
> > 
> > +
> > +       plat->dma330.dst_addr = (phys_size_t)(uintptr_t)dst;
> > +       plat->dma330.size_byte = len;
> > +
> > +       ret = dma330_transfer_zeroes_setup(plat);
> > +
> > +       /* Revert back to non secure DMA base */
> > +       if (plat->dma330.secure)
> > +               plat->base -= 0x1000;
> > +
> > +       return ret;
> > +}
> > +
> > +static int dma330_transfer(struct udevice *dev, int direction,
> > void *dst,
> > +                         void *src, size_t len)
> > +{
> > +       struct dma_dma330_platdata *plat = dev->platdata;
> > +       int ret = 0;
> > +
> > +       /* If DMA access is set to secure, base change to DMA
> > secure base */
> > +       if (plat->dma330.secure)
> > +               plat->base += 0x1000;
> Same as above
> 
Okay.
> > 
> > +
> > +       plat->dma330.dst_addr = (phys_size_t)(uintptr_t)dst;
> > +       plat->dma330.src_addr = (phys_size_t)(uintptr_t)src;
> > +       plat->dma330.size_byte = len;
> > +
> > +       switch (direction) {
> > +       case DMA_MEM_TO_MEM:
> > +               plat->dma330.transfer_type =
> > DMA_SUPPORTS_MEM_TO_MEM;
> > +               break;
> > +       case DMA_MEM_TO_DEV:
> > +               plat->dma330.transfer_type =
> > DMA_SUPPORTS_MEM_TO_DEV;
> > +               break;
> > +       case DMA_DEV_TO_MEM:
> > +               plat->dma330.transfer_type =
> > DMA_SUPPORTS_DEV_TO_MEM;
> > +               break;
> > +       }
> > +
> > +       ret = dma330_transfer_setup(plat);
> > +
> > +       /* Revert back to non secure DMA base */
> > +       if (plat->dma330.secure)
> > +               plat->base -= 0x1000;
> > +
> > +       return ret;
> > +}
> > +
> > +static int dma330_ofdata_to_platdata(struct udevice *dev)
> > +{
> > +       struct dma_dma330_platdata *plat = dev->platdata;
> > +       const void *blob = gd->fdt_blob;
> > +       int node = dev_of_offset(dev);
> > +
> > +       plat->base = (void __iomem *)devfdt_get_addr(dev);
> dev_read_addr()
> 
Okay.
> > 
> > 
> > +       plat->max_brst_size = fdtdec_get_uint(blob, node, "dma-max-
> > burst-size",
> > +                                             2);
> Use dev_read API please (live tree)
> 
Okay.
> > +       plat->brst_size = fdtdec_get_uint(blob, node, "dma-burst-
> > size", 2);
> > +       plat->brst_len = fdtdec_get_uint(blob, node, "dma-burst-
> > length", 2);
> > +
> > +       return 0;
> > +}
> > +
> > +static int dma330_probe(struct udevice *adev)
> > +{
> > +       struct dma_dev_priv *uc_priv = dev_get_uclass_priv(adev);
> > +
> > +       uc_priv->supported = (DMA_SUPPORTS_MEM_TO_MEM |
> > +                            DMA_SUPPORTS_MEM_TO_DEV |
> > +                            DMA_SUPPORTS_DEV_TO_MEM);
> > +       return 0;
> > +}
> > +
> > +static const struct dma_ops dma330_ops = {
> > +       .transfer = dma330_transfer,
> > +       .transfer_zeroes = dma330_transfer_zeroes,
> > +};
> > +
> > +static const struct udevice_id dma330_ids[] = {
> > +       { .compatible = "arm,pl330" },
> > +       { .compatible = "arm,dma330" },
> > +       { /* sentinel */ }
> > +};
> > +
> > +U_BOOT_DRIVER(dma_dma330) = {
> > +       .name   = "dma_dma330",
> > +       .id     = UCLASS_DMA,
> > +       .of_match = dma330_ids,
> > +       .ops    = &dma330_ops,
> > +       .ofdata_to_platdata = dma330_ofdata_to_platdata,
> > +       .probe = dma330_probe,
> > +       .platdata_auto_alloc_size = sizeof(struct
> > dma_dma330_platdata),
> > +};
> > diff --git a/include/dma330.h b/include/dma330.h
> > new file mode 100644
> > index 0000000..c054bf2
> > --- /dev/null
> > +++ b/include/dma330.h
> > @@ -0,0 +1,136 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +/*
> > + * Copyright (C) 2018 Intel Corporation <www.intel.com>
> > + */
> > +
> > +#include <dma.h>
> > +#include <linux/sizes.h>
> > +
> > +#ifndef __DMA330_CORE_H
> > +#define __DMA330_CORE_H
> > +
> > +#define DMA330_MAX_CHAN                8
> > +#define DMA330_MAX_IRQS                32
> > +#define DMA330_MAX_PERI                32
> > +
> > +#define DMA330_STATE_STOPPED           BIT(0)
> > +#define DMA330_STATE_EXECUTING         BIT(1)
> > +#define DMA330_STATE_WFE               BIT(2)
> > +#define DMA330_STATE_FAULTING          BIT(3)
> > +#define        DMA330_STATE_COMPLETING         BIT(4)
> > +#define DMA330_STATE_WFP               BIT(5)
> > +#define DMA330_STATE_KILLING           BIT(6)
> > +#define DMA330_STATE_FAULT_COMPLETING  BIT(7)
> > +#define DMA330_STATE_CACHEMISS         BIT(8)
> > +#define DMA330_STATE_UPDTPC            BIT(9)
> > +#define DMA330_STATE_ATBARRIER         BIT(10)
> > +#define DMA330_STATE_QUEUEBUSY         BIT(11))
> > +#define DMA330_STATE_INVALID           BIT(15)
> > +
> > +enum dma330_srccachectrl {
> > +       SCCTRL0 = 0,    /* Noncacheable and nonbufferable */
> > +       SCCTRL1,        /* Bufferable only */
> > +       SCCTRL2,        /* Cacheable, but do not allocate */
> > +       SCCTRL3,        /* Cacheable and bufferable, but do not
> > allocate */
> > +       SINVALID1,
> > +       SINVALID2,
> > +       SCCTRL6,        /* Cacheable write-through, allocate on
> > reads only */
> > +       SCCTRL7,        /* Cacheable write-back, allocate on reads
> > only */
> > +};
> > +
> > +enum dma330_dstcachectrl {
> > +       DCCTRL0 = 0,    /* Noncacheable and nonbufferable */
> > +       DCCTRL1,        /* Bufferable only */
> > +       DCCTRL2,        /* Cacheable, but do not allocate */
> > +       DCCTRL3,        /* Cacheable and bufferable, but do not
> > allocate */
> > +       DINVALID1 = 8,
> > +       DINVALID2,
> > +       DCCTRL6,        /* Cacheable write-through, allocate on
> > writes only */
> > +       DCCTRL7,        /* Cacheable write-back, allocate on writes
> > only */
> > +};
> > +
> > +enum dma330_byteswap {
> > +       SWAP_NO = 0,
> > +       SWAP_2,
> > +       SWAP_4,
> > +       SWAP_8,
> > +       SWAP_16,
> > +};
> > +
> > +/**
> > + * dma330_reqcfg - Request Configuration.
> > + *
> > + * The DMA330 core does not modify this and uses the last
> > + * working configuration if the request doesn't provide any.
> > + *
> > + * The Client may want to provide this info only for the
> > + * first request and a request with new settings.
> > + */
> > +struct dma330_reqcfg {
> > +       /* Address Incrementing */
> > +       u8 dst_inc;
> > +       u8 src_inc;
> > +
> > +       /*
> > +        * For now, the SRC & DST protection levels
> > +        * and burst size/length are assumed same.
> > +        */
> > +       u8 nonsecure;
> > +       u8 privileged;
> > +       u8 insnaccess;
> > +       u8 brst_len;
> > +       u8 brst_size; /* in power of 2 */
> > +
> > +       enum dma330_dstcachectrl dcctl;
> > +       enum dma330_srccachectrl scctl;
> > +       enum dma330_byteswap swap;
> > +};
> > +
> > +/**
> > + * dma330_transfer_struct - Structure to be passed in for
> > dma330_transfer_x.
> > + *
> > + * @channel0_manager1: Switching configuration on DMA channel or
> > DMA manager.
> > + * @channel_num: Channel number assigned, valid from 0 to 7.
> > + * @src_addr: Address to transfer from / source.
> > + * @dst_addr: Address to transfer to / destination.
> > + * @size_byte: Number of bytes to be transferred.
> > + * @brst_size: Valid from 0 - 4,
> > + *            where 0 = 1 (2 ^ 0) bytes and 3 = 16 bytes (2 ^ 4).
> > + * @max_brst_size: Max transfer size (from 0 - 4).
> > + * @single_brst_size: Single transfer size (from 0 - 4).
> > + * @brst_len: Valid from 1 - 16 where each burst can transfer 1 -
> > 16
> > + *           Data chunk (each chunk size equivalent to brst_size).
> > + * @peripheral_id: Assigned peripheral_id, valid from 0 to 31.
> > + * @transfer_type: MEM2MEM, MEM2PERIPH or PERIPH2MEM.
> > + * @enable_cache1: 1 for cache enabled for memory
> > + *                (cacheable and bufferable, but do not allocate).
> > + * @buf: Buffer handler which will point to the memory
> > + *      allocated for dma microcode
> > + * @secure: Set DMA channel in secure mode.
> This is a good way to document structs. Please do it for the others.
> 
> > 
> > + *
> > + * Description of the structure.
> Yes?
> 
Sure.
> > 
> > + */
> > +struct dma330_transfer_struct {
> > +       u32 channel0_manager1;
> > +       u32 channel_num;
> > +       phys_size_t src_addr;
> > +       phys_size_t dst_addr;
> > +       u32 size_byte;
> > +       u32 single_brst_size;
> > +       u32 peripheral_id;
> > +       u32 transfer_type;
> > +       u32 enable_cache1;
> > +       u32 *buf;
> > +       u8 secure;
> > +};
> > +
> > +struct dma_dma330_platdata {
> > +       void __iomem *base;
> > +       u32 buf_size;
> > +       u32 max_brst_size;
> > +       u32 brst_size;
> > +       u32 brst_len;
> > +       struct dma330_transfer_struct dma330;
> > +};
> > +
> > +#endif /* __DMA330_CORE_H */
> > --
> > 2.2.0
> > 
> Regards,
> Simon