Hi Dinh, On 9 December 2016 at 12:03, Dinh Nguyen <dingu...@kernel.org> wrote: > From: Dinh Nguyen <dingu...@opensource.altera.com> > > Adopted from the Linux kernel PL330 DMA driver. > > Signed-off-by: Dinh Nguyen <dingu...@kernel.org> > --- > v2: Add Kconfig CONFIG_PL330_DMA entry > --- > arch/arm/include/asm/pl330.h | 105 +++++ > drivers/dma/Kconfig | 4 + > drivers/dma/Makefile | 1 + > drivers/dma/pl330.c | 942 > +++++++++++++++++++++++++++++++++++++++++++ > 4 files changed, 1052 insertions(+) > create mode 100644 arch/arm/include/asm/pl330.h > create mode 100644 drivers/dma/pl330.c
Reviewed-by: Simon Glass <s...@chromium.org> nit below. > > diff --git a/arch/arm/include/asm/pl330.h b/arch/arm/include/asm/pl330.h > new file mode 100644 > index 0000000..dd19b4c > --- /dev/null > +++ b/arch/arm/include/asm/pl330.h > @@ -0,0 +1,105 @@ > +/* > + * Copyright (C) 2010 Samsung Electronics Co. Ltd. > + * Jaswinder Singh <jassi.b...@samsung.com> > + * > + * SPDX-License-Identifier: GPL-2.0+ > + * > + * adapted from linux kernel pl330.h > + */ > + > +#ifndef __PL330_H_ > +#define __PL330_H_ > + > +#define PL330_STATE_STOPPED (1 << 0) > +#define PL330_STATE_EXECUTING (1 << 1) > +#define PL330_STATE_WFE (1 << 2) > +#define PL330_STATE_FAULTING (1 << 3) > +#define PL330_STATE_COMPLETING (1 << 4) > +#define PL330_STATE_WFP (1 << 5) > +#define PL330_STATE_KILLING (1 << 6) > +#define PL330_STATE_FAULT_COMPLETING (1 << 7) > +#define PL330_STATE_CACHEMISS (1 << 8) > +#define PL330_STATE_UPDTPC (1 << 9) > +#define PL330_STATE_ATBARRIER (1 << 10) > +#define PL330_STATE_QUEUEBUSY (1 << 11) > +#define PL330_STATE_INVALID (1 << 15) > + > +#define PL330_DMA_MAX_BURST_SIZE 3 > + > +/* structure to be passed in for pl330_transfer_x */ > +struct pl330_transfer_struct { Is this used anywhere? Is this describing hardware registers? > + void __iomem *reg_base; > + u32 channel_num; > + u32 src_addr; > + u32 dst_addr; > + u32 len; > + u32 brst_size; > + u32 single_brst_size; > + u32 brst_len; > + u32 peripheral_id; > + u32 transfer_type; > + u32 enable_cache1; > + u32 buf_size; > + u8 *buf; > +}; > + > +enum pl330_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 */ You might consider naming these with their meaning instead of the number/ > +}; > + > +enum pl330_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 pl330_byteswap { > + SWAP_NO = 0, > + SWAP_2, > + SWAP_4, > + SWAP_8, > + SWAP_16, > +}; > + > +/* > + * Request Configuration. > + * The PL330 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 pl330_reqcfg { > + /* Address Incrementing */ > + unsigned dst_inc:1; > + unsigned src_inc:1; Is this mapping a hardware register? The use of bitfields worries me. If it is not mapping hardware, can you please add struct member comments? > + > + /* > + * For now, the SRC & DST protection levels > + * and burst size/length are assumed same. > + */ > + int nonsecure; > + int privileged; > + int insnaccess; > + unsigned brst_len:5; > + unsigned brst_size:3; /* in power of 2 */ > + > + enum pl330_dstcachectrl dcctl; > + enum pl330_srccachectrl scctl; > + enum pl330_byteswap swap; > +}; > + > +void arm_pl330_transfer(struct pl330_transfer_struct *pl330); > +#endif /* __PL330_H_ */ > diff --git a/drivers/dma/Kconfig b/drivers/dma/Kconfig > index 1b92c77..d95f959 100644 > --- a/drivers/dma/Kconfig > +++ b/drivers/dma/Kconfig > @@ -19,4 +19,8 @@ config TI_EDMA3 > This driver support data transfer between memory > regions. > > +config PL330_DMA > + bool "PL330 DMA driver" > + help > + Enable the ARM PL330 DMA driver. Please expand a bit - what is the PL330 - where is it found, what can it do? > endmenu # menu "DMA Support" > diff --git a/drivers/dma/Makefile b/drivers/dma/Makefile > index 39b78b2..d287512 100644 > --- a/drivers/dma/Makefile > +++ b/drivers/dma/Makefile > @@ -13,3 +13,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_PL330_DMA) += pl330.o > diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c > new file mode 100644 > index 0000000..a97cd9f > --- /dev/null > +++ b/drivers/dma/pl330.c > @@ -0,0 +1,942 @@ > +/* > + * Copyright (c) 2012 Samsung Electronics Co., Ltd. > + * http://www.samsung.com > + * > + * Copyright (C) 2010 Samsung Electronics Co. Ltd. > + * Jaswinder Singh <jassi.b...@samsung.com> > + * > + * SPDX-License-Identifier: GPL-2.0+ > + */ > + > +#include <asm/io.h> > +#include <common.h> > +#include <dma.h> > +#include <dm/device.h> > +#include <asm/pl330.h> > +#include <asm/processor.h> > + > +struct dma_pl330_platdata { > + u32 base; > +}; > + > +/* Register and Bit field Definitions */ > +#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 > + > +#define DPC 0x4 > +#define INTEN 0x20 > +#define ES 0x24 > +#define INTSTATUS 0x28 > +#define INTCLR 0x2c > +#define FSM 0x30 > +#define FSC 0x34 > +#define FTM 0x38 > + > +#define _FTC 0x40 > +#define FTC(n) (_FTC + (n)*0x4) Can you add a space either side of * ? > + > +#define _CS 0x100 > +#define CS(n) (_CS + (n)*0x8) > +#define CS_CNS (1 << 21) > + > +#define _CPC 0x104 > +#define CPC(n) (_CPC + (n)*0x8) > + > +#define _SA 0x400 > +#define SA(n) (_SA + (n)*0x20) > + > +#define _DA 0x404 > +#define DA(n) (_DA + (n)*0x20) > + > +#define _CC 0x408 > +#define CC(n) (_CC + (n)*0x20) > + > +#define CC_SRCINC (1 << 0) > +#define CC_DSTINC (1 << 14) > +#define CC_SRCPRI (1 << 8) > +#define CC_DSTPRI (1 << 22) > +#define CC_SRCNS (1 << 9) > +#define CC_DSTNS (1 << 23) > +#define CC_SRCIA (1 << 10) > +#define CC_DSTIA (1 << 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 > +#define CC_DSTCCTRL_SHFT 25 > +#define CC_DRCCCTRL_MASK 0x7 > +#define CC_SWAP_SHFT 28 > + > +#define _LC0 0x40c > +#define LC0(n) (_LC0 + (n)*0x20) > + > +#define _LC1 0x410 > +#define LC1(n) (_LC1 + (n)*0x20) > + > +#define DBGSTATUS 0xd00 > +#define DBG_BUSY (1 << 0) > + > +#define DBGCMD 0xd04 > +#define DBGINST0 0xd08 > +#define DBGINST1 0xd0c > + > +#define CR0 0xe00 > +#define CR1 0xe04 > +#define CR2 0xe08 > +#define CR3 0xe0c > +#define CR4 0xe10 > +#define CRD 0xe14 > + > +#define PERIPH_ID 0xfe0 > +#define PERIPH_REV_SHIFT 20 > +#define PERIPH_REV_MASK 0xf > +#define PERIPH_REV_R0P0 0 > +#define PERIPH_REV_R1P0 1 > +#define PERIPH_REV_R1P1 2 > + > +#define CR0_PERIPH_REQ_SET (1 << 0) > +#define CR0_BOOT_EN_SET (1 << 1) > +#define CR0_BOOT_MAN_NS (1 << 2) > +#define CR0_NUM_CHANS_SHIFT 4 > +#define CR0_NUM_CHANS_MASK 0x7 I think it is better to have the mask defined as #define CR0_NUM_CHANS_MASK (0x7 << CR0_NUM_CHANS_SHIFT) Then you can mask without shifting, or before shifting. That said, I cannot see where this is used in this patch. > +#define CR0_NUM_PERIPH_SHIFT 12 > +#define CR0_NUM_PERIPH_MASK 0x1f > +#define CR0_NUM_EVENTS_SHIFT 17 > +#define CR0_NUM_EVENTS_MASK 0x1f > + > +#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) while (!(_state(t) & (s))) cpu_relax(); Could this be a static function instead? It looks like you could pass in the flags to check and the required value of those flags as two separate parameters. > + > +/* Enum declaration */ > +enum dmamov_dst { > + SAR = 0, > + CCR, > + DAR, > +}; > + > +enum pl330_dst { > + SRC = 0, > + DST, > +}; > + > +enum pl330_cond { > + SINGLE, > + BURST, > + ALWAYS, > +}; > + > +/* Structure will be used by _emit_LPEND function */ > +struct _arg_LPEND { > + enum pl330_cond cond; > + int forever; > + unsigned loop; > + u8 bjump; > +}; > + > +/* Structure will be used by _emit_GO function */ > +struct _arg_GO { > + u8 chan; > + u32 addr; > + unsigned ns; > +}; > + > +/* > + * Function: add opcode DMAEND into microcode (end) > + * Return: size of opcode > + * Parameter: buf -> the buffer which stored the microcode program > + */ > +static inline u32 _emit_END(u8 buf[]) > +{ > + buf[0] = CMD_DMAEND; > + > + return SZ_DMAEND; > +} > + > +static inline u32 _emit_FLUSHP(u8 buf[], u8 peri) > +{ > + buf[0] = CMD_DMAFLUSHP; > + > + peri &= 0x1f; > + peri <<= 3; > + buf[1] = peri; > + > + return SZ_DMAFLUSHP; > +} > + > +static inline u32 _emit_LD(u8 buf[], enum pl330_cond cond) > +{ > + buf[0] = CMD_DMALD; > + > + if (cond == SINGLE) > + buf[0] |= (0 << 1) | (1 << 0); > + else if (cond == BURST) > + buf[0] |= (1 << 1) | (1 << 0); > + > + return SZ_DMALD; > +} > + > +static inline u32 _emit_LDP(u8 buf[], enum pl330_cond cond, u8 peri) > +{ > + buf[0] = CMD_DMALDP; > + > + if (cond == BURST) > + buf[0] |= (1 << 1); > + > + peri &= 0x1f; > + peri <<= 3; > + buf[1] = peri; > + > + return SZ_DMALDP; > +} > + > +static inline u32 _emit_LP(u8 buf[], unsigned loop, u8 cnt) > +{ > + buf[0] = CMD_DMALP; > + > + if (loop) > + buf[0] |= (1 << 1); > + > + cnt--; /* DMAC increments by 1 internally */ > + buf[1] = cnt; > + > + return SZ_DMALP; > +} > + > +static inline u32 _emit_LPEND(u8 buf[], const struct _arg_LPEND *arg) > +{ > + enum pl330_cond cond = arg->cond; > + bool forever = arg->forever; > + unsigned loop = arg->loop; > + u8 bjump = arg->bjump; > + > + buf[0] = CMD_DMALPEND; > + > + if (loop) > + buf[0] |= (1 << 2); > + > + if (!forever) > + buf[0] |= (1 << 4); > + > + if (cond == SINGLE) > + buf[0] |= (0 << 1) | (1 << 0); > + else if (cond == BURST) > + buf[0] |= (1 << 1) | (1 << 0); > + > + buf[1] = bjump; > + > + return SZ_DMALPEND; > +} > + > +static inline u32 _emit_KILL(u8 buf[]) > +{ > + buf[0] = CMD_DMAKILL; > + > + return SZ_DMAKILL; > +} > + > +static inline u32 _emit_MOV(u8 buf[], enum dmamov_dst dst, u32 val) > +{ > + buf[0] = CMD_DMAMOV; > + buf[1] = dst; > + > + buf[2] = val & 0xFF; > + buf[3] = (val >> 8) & 0xFF; > + buf[4] = (val >> 16) & 0xFF; > + buf[5] = (val >> 24) & 0xFF; > + > + return SZ_DMAMOV; > +} > + > +static inline u32 _emit_NOP(u8 buf[]) > +{ > + buf[0] = CMD_DMANOP; > + > + return SZ_DMANOP; > +} > + > +static inline u32 _emit_RMB(u8 buf[]) > +{ > + buf[0] = CMD_DMARMB; > + > + return SZ_DMARMB; > +} > + > +static inline u32 _emit_SEV(u8 buf[], u8 ev) Consider using unsigned instead of u8. Non-native arguments do bloat the code. > +{ > + buf[0] = CMD_DMASEV; > + > + ev &= 0x1f; Or at least drop this mask. > + ev <<= 3; > + buf[1] = ev; > + > + return SZ_DMASEV; > +} > + > +static inline u32 _emit_ST(u8 buf[], enum pl330_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); > + > + return SZ_DMAST; > +} > + > +static inline u32 _emit_STP(u8 buf[], enum pl330_cond cond, u8 peri) > +{ > + buf[0] = CMD_DMASTP; > + > + if (cond == BURST) > + buf[0] |= (1 << 1); > + > + peri &= 0x1f; > + peri <<= 3; > + buf[1] = peri; > + > + return SZ_DMASTP; > +} > + > +static inline u32 _emit_STZ(u8 buf[]) > +{ > + buf[0] = CMD_DMASTZ; > + > + return SZ_DMASTZ; > +} > + > +static inline u32 _emit_WFE(u8 buf[], u8 ev, unsigned invalidate) > +{ > + buf[0] = CMD_DMAWFE; > + > + ev &= 0x1f; > + ev <<= 3; > + buf[1] = ev; > + > + if (invalidate) > + buf[1] |= (1 << 1); > + > + return SZ_DMAWFE; > +} > + > +static inline u32 _emit_WFP(u8 buf[], enum pl330_cond cond, u8 peri) > +{ > + buf[0] = CMD_DMAWFP; > + > + if (cond == SINGLE) > + buf[0] |= (0 << 1) | (0 << 0); > + else if (cond == BURST) > + buf[0] |= (1 << 1) | (0 << 0); > + else > + buf[0] |= (0 << 1) | (1 << 0); > + > + peri &= 0x1f; > + peri <<= 3; > + buf[1] = peri; > + > + return SZ_DMAWFP; > +} > + > +static inline u32 _emit_WMB(u8 buf[]) > +{ > + buf[0] = CMD_DMAWMB; > + > + return SZ_DMAWMB; > +} > + > +static inline u32 _emit_GO(u8 buf[], > + const struct _arg_GO *arg) > +{ > + u8 chan = arg->chan; > + u32 addr = arg->addr; > + unsigned ns = arg->ns; > + > + buf[0] = CMD_DMAGO; > + buf[0] |= (ns << 1); > + > + buf[1] = chan & 0x7; > + buf[2] = addr & 0xFF; > + buf[3] = (addr >> 8) & 0xFF; > + buf[4] = (addr >> 16) & 0xFF; > + buf[5] = (addr >> 24) & 0xFF; Does put_unaligned_le32() help here? > + return SZ_DMAGO; > +} > + > +/* > + * Function: Populate the CCR register > + * Parameter: rqc -> Request Configuration. > + */ > +static inline u32 _prepare_ccr(const struct pl330_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); > + > + ccr |= (rqc->swap << CC_SWAP_SHFT); > + return ccr; > +} > + > +/* > + * Function: wait until DMA Manager is idle > + * Return: 1 = error / timeout ocurred before idle > + * Parameter: loop -> number of loop before timeout ocurred > + */ > +static int _until_dmac_idle(struct pl330_transfer_struct *pl330, int loops) > +{ > + void __iomem *regs = pl330->reg_base; > + > + do { > + /* Until Manager is Idle */ > + if (!(readl(regs + DBGSTATUS) & DBG_BUSY)) > + break; > + > + cpu_relax(); > + } while (--loops); > + > + if (!loops) > + return true; > + > + return false; > +} > + > +static inline void _execute_DBGINSN(struct pl330_transfer_struct *pl330, > + u8 insn[], bool as_manager, int > timeout_loops) > +{ > + void __iomem *regs = pl330->reg_base; > + u32 val; > + > + val = (insn[0] << 16) | (insn[1] << 24); > + if (!as_manager) { > + val |= (1 << 0); > + val |= (pl330->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 (_until_dmac_idle(pl330, timeout_loops)) { > + printf("DMAC halted!\n"); > + return; > + } > + > + /* Get going */ > + writel(0, regs + DBGCMD); > +} > + > +static inline u32 _state(struct pl330_transfer_struct *pl330) > +{ > + void __iomem *regs = pl330->reg_base; > + u32 val; > + > + val = readl(regs + CS(pl330->channel_num)) & 0xf; > + > + udelay(1); > + > + switch (val) { > + case DS_ST_STOP: > + return PL330_STATE_STOPPED; > + case DS_ST_EXEC: > + return PL330_STATE_EXECUTING; > + case DS_ST_CMISS: > + return PL330_STATE_CACHEMISS; > + case DS_ST_UPDTPC: > + return PL330_STATE_UPDTPC; > + case DS_ST_WFE: > + return PL330_STATE_WFE; > + case DS_ST_FAULT: > + return PL330_STATE_FAULTING; > + case DS_ST_ATBRR: > + return PL330_STATE_ATBARRIER; > + case DS_ST_QBUSY: > + return PL330_STATE_QUEUEBUSY; > + case DS_ST_WFP: > + return PL330_STATE_WFP; > + case DS_ST_KILL: > + return PL330_STATE_KILLING; > + case DS_ST_CMPLT: > + return PL330_STATE_COMPLETING; > + case DS_ST_FLTCMP: > + return PL330_STATE_FAULT_COMPLETING; > + default: > + return PL330_STATE_INVALID; > + } > +} > + > +static void _stop(struct pl330_transfer_struct *pl330, int timeout_loops) > +{ > + u8 insn[6] = {0, 0, 0, 0, 0, 0}; > + > + if (_state(pl330) == PL330_STATE_FAULT_COMPLETING) > + UNTIL(pl330, PL330_STATE_FAULTING | PL330_STATE_KILLING); > + > + /* Return if nothing needs to be done */ > + if (_state(pl330) == PL330_STATE_COMPLETING > + || _state(pl330) == PL330_STATE_KILLING > + || _state(pl330) == PL330_STATE_STOPPED) > + return; > + > + _emit_KILL(insn); > + > + _execute_DBGINSN(pl330, insn, 0, timeout_loops); > +} > + > +static bool _trigger(struct pl330_transfer_struct *pl330, u8 *buffer, > + int timeout_loops) Please comment this function, its args and return value. > +{ > + void __iomem *regs = pl330->reg_base; > + struct _arg_GO go; > + u8 insn[6] = {0, 0, 0, 0, 0, 0}; > + > + /* Return if already ACTIVE */ > + if (_state(pl330) != PL330_STATE_STOPPED) > + return true; > + > + go.chan = pl330->channel_num; > + go.addr = (u32)buffer; > + > + /* determine security. Assume secure */ > + if (readl(regs + CS(go.chan)) & CS_CNS) > + go.ns = 1; > + else > + go.ns = 0; > + _emit_GO(insn, &go); > + > + /* Only manager can execute GO */ > + _execute_DBGINSN(pl330, insn, true, timeout_loops); > + > + return false; > +} > + > +static bool _start(struct pl330_transfer_struct *pl330, int timeout_loops) Can you rename this function? We use 'start' as the start of U-Boot so I think it might be confusing to see this in the symbol list. Although it may be inlined... > +{ > + switch (_state(pl330)) { > + case PL330_STATE_FAULT_COMPLETING: > + UNTIL(pl330, PL330_STATE_FAULTING | PL330_STATE_KILLING); > + > + if (_state(pl330) == PL330_STATE_KILLING) > + UNTIL(pl330, PL330_STATE_STOPPED) > + > + case PL330_STATE_FAULTING: > + _stop(pl330, timeout_loops); > + > + case PL330_STATE_KILLING: > + case PL330_STATE_COMPLETING: > + UNTIL(pl330, PL330_STATE_STOPPED) > + > + case PL330_STATE_STOPPED: > + return _trigger(pl330, pl330->buf, timeout_loops); > + > + case PL330_STATE_WFP: > + case PL330_STATE_QUEUEBUSY: > + case PL330_STATE_ATBARRIER: > + case PL330_STATE_UPDTPC: > + case PL330_STATE_CACHEMISS: > + case PL330_STATE_EXECUTING: > + return true; > + > + case PL330_STATE_WFE: /* For RESUME, nothing yet */ > + default: > + return false; > + } > +} > + > +/* > + * DMA run or start > + * Return: 1 for error or not successful > + */ > +static int pl330_transfer_start(struct pl330_transfer_struct *pl330) > +{ > + /* Timeout loop */ > + int timeout_loops = 10000; Is this intended to wait for a specific number of loops or a specific time? Please add a comment as to how you chose this value. > + > + /* Execute the command list */ > + return _start(pl330, timeout_loops); > +} > + > +/* > + * DMA poll until finish or error > + * Return: 1 for error or not successful > + * channel_num - channel number assigned, valid from 0 to 7 > + */ > +static int pl330_transfer_finish(struct pl330_transfer_struct *pl330) > +{ > + /* Wait until finish execution to ensure we compared correct result*/ > + UNTIL(pl330, PL330_STATE_STOPPED | PL330_STATE_FAULTING); > + > + /* check the state */ > + if (_state(pl330) == PL330_STATE_FAULTING) { > + printf("FAULT Mode: Channel %u Faulting, FTR = 0x%08x, " > + "CPC = 0x%08x\n", pl330->channel_num, > + readl(pl330->reg_base + FTC(pl330->channel_num)), > + ((u32)readl(pl330->reg_base + CPC(pl330->channel_num)) > + - (u32)pl330->buf)); > + return 1; > + } > + return 0; > +} > + > +/* > + * DMA transfer setup (DMA_SUPPORTS_MEM_TO_MEM, DMA_SUPPORTS_MEM_TO_DEV or > + DMA_SUPPORTS_DEV_TO_MEM) > + * For Peripheral transfer, the FIFO threshold value is expected at > + * 2 ^ pl330->brst_size * pl330->brst_len. > + * Return: 1 for error or not successful > + * > + * channel_num - channel number assigned, valid from 0 to 7 It would be good to use the standard format for these comments, e.g. @channel_num: desc @return ... > + * src_addr - address to transfer from / source > + * dst_addr - address to transfer to / destination > + * len - number of bytes to be transferred > + * brst_size - valid from 0 - 3 > + * where 0 = 1 (2 ^ 0) bytes and 3 = 8 bytes (2 ^ 3) > + * single_brst_size - single transfer size (from 0 - 3) > + * brst_len - valid from 1 - 16 where each burst can trasfer 1 - 16 > + * data chunk (each chunk size equivalent to brst_size) > + * peripheral_id assigned peripheral_id, valid from 0 to 31 > + * transfer_type DMA_SUPPORTS_MEM_TO_MEM, DMA_SUPPORTS_MEM_TO_DEV or > + * DMA_SUPPORTS_DEV_TO_MEM > + * buf_size - sizeof(buf) > + * buf - buffer handler which will point to the memory > + * allocated for dma microcode > + */ > +static int pl330_transfer_setup(struct pl330_transfer_struct *pl330) > +{ > + /* Variable declaration */ > + int off = 0; /* buffer offset clear to 0 */ > + int ret = 0; I don't think this needs to be assigned here. Perhaps some others are the same. > + unsigned loopjmp0, loopjmp1; /* for DMALPEND */ > + unsigned lcnt0 = 0; /* loop count 0 */ > + unsigned lcnt1 = 0; /* loop count 1 */ > + unsigned burst_size = 0; > + unsigned len = pl330->len; > + u32 ccr = 0; /* Channel Control Register */ > + struct pl330_reqcfg reqcfg; > + > + /* for burst, always use the maximum burst size and length */ > + pl330->brst_size = PL330_DMA_MAX_BURST_SIZE; > + pl330->brst_len = 16; > + pl330->single_brst_size = 1; > + > + /* burst_size = 2 ^ brst_size */ > + burst_size = 1 << pl330->brst_size; > + > + pl330->src_addr = (u32)&pl330->buf; > + if (pl330->dst_addr & (burst_size - 1)) { > + puts("ERROR PL330 : destination address unaligned\n"); printf() - we use that instead of puts() these days, except perhaps in tiny SPL areas. > + return 1; > + } > + > + /* DMAMOV DAR, x->dst_addr */ > + off += _emit_MOV(&pl330->buf[off], DAR, pl330->dst_addr); > + /* DMAFLUSHP P(periheral_id) */ > + if (pl330->transfer_type != DMA_SUPPORTS_MEM_TO_MEM) > + off += _emit_FLUSHP(&pl330->buf[off], pl330->peripheral_id); > + > + /* Preparing the CCR value */ > + if (pl330->transfer_type == DMA_SUPPORTS_MEM_TO_DEV) { > + reqcfg.dst_inc = 0; /* disable auto increment */ > + reqcfg.src_inc = 1; /* enable auto increment */ > + } else if (pl330->transfer_type == DMA_SUPPORTS_DEV_TO_MEM) { > + reqcfg.dst_inc = 1; > + reqcfg.src_inc = 0; > + } else { > + /* DMA_SUPPORTS_MEM_TO_MEM */ > + reqcfg.dst_inc = 1; > + reqcfg.src_inc = 1; > + } > + > + reqcfg.nonsecure = 0; /* Secure mode */ > + reqcfg.dcctl = 0x1; /* noncacheable but bufferable */ > + reqcfg.scctl = 0x1; > + reqcfg.privileged = 1; /* 1 - Priviledge */ > + reqcfg.insnaccess = 0; /* 0 - data access */ > + reqcfg.swap = 0; /* 0 - no endian swap */ > + reqcfg.brst_len = pl330->brst_len; /* DMA burst length */ > + reqcfg.brst_size = pl330->brst_size; /* DMA burst size */ > + /* Preparing the CCR value */ > + ccr = _prepare_ccr(&reqcfg); > + /* DMAMOV CCR, ccr */ > + off += _emit_MOV(&pl330->buf[off], CCR, ccr); > + > + /* BURST */ > + /* Can initiate a burst? */ > + while (len >= burst_size * pl330->brst_len) { > + lcnt0 = len / (burst_size * pl330->brst_len); > + lcnt1 = 0; > + if (lcnt0 >= 256 * 256) > + lcnt0 = lcnt1 = 256; checkpatch / patman should warn about missing {} here? > + else if (lcnt0 >= 256) { > + lcnt1 = lcnt0 / 256; > + lcnt0 = 256; > + } > + len = len - > + (burst_size * pl330->brst_len * lcnt0 * lcnt1); > + > + if (lcnt1) { > + /* DMALP1 */ > + off += _emit_LP(&pl330->buf[off], 1, lcnt1); > + loopjmp1 = off; > + } > + /* DMALP0 */ > + off += _emit_LP(&pl330->buf[off], 0, lcnt0); > + loopjmp0 = off; > + > + off += _emit_STZ(&pl330->buf[off]); > + /* DMALP0END */ > + struct _arg_LPEND lpend; > + lpend.cond = ALWAYS; > + lpend.forever = 0; > + lpend.loop = 0; /* loop cnt 0 */ > + lpend.bjump = off - loopjmp0; > + off += _emit_LPEND(&pl330->buf[off], &lpend); > + /* DMALP1END */ > + if (lcnt1) { > + struct _arg_LPEND lpend; blank line here (between decls and code) > + lpend.cond = ALWAYS; > + lpend.forever = 0; > + lpend.loop = 1; /* loop cnt 1*/ > + lpend.bjump = off - loopjmp1; > + off += _emit_LPEND(&pl330->buf[off], &lpend); > + } > + /* ensure the microcode don't exceed buffer size */ > + if (off > pl330->buf_size) { > + puts("ERROR PL330 : Exceed buffer size\n"); > + return 1; > + } > + } > + > + /* SINGLE */ > + pl330->brst_size = pl330->single_brst_size; > + pl330->brst_len = 1; > + /* burst_size = 2 ^ brst_size */ > + burst_size = (1 << pl330->brst_size); > + lcnt0 = len / (burst_size * pl330->brst_len); > + > + /* ensure all data will be transfered */ > + len = len - > + (burst_size * pl330->brst_len * lcnt0); Join those two lines? > + if (len) > + puts("ERROR PL330 : Detected the possibility of untransfered" > + "data. Please ensure correct single burst size\n"); > + > + if (lcnt0) { > + /* Preparing the CCR value */ > + reqcfg.brst_len = pl330->brst_len; /* DMA burst length */ > + reqcfg.brst_size = pl330->brst_size; /* DMA burst size */ > + ccr = _prepare_ccr(&reqcfg); > + /* DMAMOV CCR, ccr */ > + off += _emit_MOV(&pl330->buf[off], CCR, ccr); > + > + /* DMALP0 */ > + off += _emit_LP(&pl330->buf[off], 0, lcnt0); > + loopjmp0 = off; > + > + off += _emit_STZ(&pl330->buf[off]); > + struct _arg_LPEND lpend1; > + lpend1.cond = ALWAYS; > + lpend1.forever = 0; > + lpend1.loop = 0; /* loop cnt 0 */ > + lpend1.bjump = off - loopjmp0; > + off += _emit_LPEND(&pl330->buf[off], &lpend1); > + /* ensure the microcode don't exceed buffer size */ > + if (off > pl330->buf_size) { > + puts("ERROR PL330 : Exceed buffer size\n"); > + return 1; > + } > + } > + > + /* DMAEND */ > + off += _emit_END(&pl330->buf[off]); > + > + ret = pl330_transfer_start(pl330); > + if (ret) > + return ret; > + > + ret = pl330_transfer_finish(pl330); > + if (ret) > + return ret; > + > + return 0; > +} > + > +#ifndef CONFIG_DMA Does this means it supports both driver model and not? Is it necessary for this to work without driver model? > +void arm_pl330_transfer(struct pl330_transfer_struct *pl330) > +{ > + pl330_transfer_setup(pl330); > +} > + > +#else > +static int pl330_transfer(struct udevice *dev, int direction, void *dst, > + void *src, size_t len) > +{ > + int ret = 0; > + struct dma_pl330_platdata *priv = dev_get_priv(dev); > + struct pl330_transfer_struct *pl330; > + > + /* Allocate a new DMAC and its Channels */ > + pl330 = devm_kzalloc(dev, sizeof(*pl330), GFP_KERNEL); > + if (!pl330) > + return -ENOMEM; > + > + pl330->reg_base = priv->base; > + > + pl330->dst_addr = (unsigned int) (dst); > + pl330->src_addr = (unsigned int) (src); > + pl330->len = len; > + > + /* channel 1 */ > + pl330->channel_num = 1; > + > + switch(direction) { > + case DMA_MEM_TO_MEM: > + pl330->transfer_type = DMA_SUPPORTS_MEM_TO_MEM; > + break; > + case DMA_MEM_TO_DEV: > + pl330->transfer_type = DMA_SUPPORTS_MEM_TO_DEV; > + break; > + case DMA_DEV_TO_MEM: > + pl330->transfer_type = DMA_SUPPORTS_DEV_TO_MEM; > + break; > + } > + > + ret = pl330_transfer_setup(pl330); > + > + return ret; > +} > + > +static int pl330_ofdata_to_platdata(struct udevice *dev) > +{ > + struct dma_pl330_platdata *priv = dev_get_priv(dev); > + > + priv->base = dev_get_addr(dev); > + > + return 0; > +} > + > +static int pl330_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 pl330_ops = { > + .transfer = pl330_transfer, > +}; > + > +static const struct udevice_id pl330_ids[] = { > + { .compatible = "arm,pl330" }, > + { /* sentinel */ } > +}; > + > +U_BOOT_DRIVER(dma_pl330) = { > + .name = "dma_pl330", > + .id = UCLASS_DMA, > + .of_match = pl330_ids, > + .ops = &pl330_ops, > + .ofdata_to_platdata = pl330_ofdata_to_platdata, > + .probe = pl330_probe, > + .priv_auto_alloc_size = sizeof(struct dma_pl330_platdata), > +}; > +#endif /* CONFIG_DMA */ > -- > 2.7.4 > Regards, Simon _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot