Hi Dinh,
On 9 December 2016 at 12:03, Dinh Nguyen dinguyen@kernel.org wrote:
From: Dinh Nguyen dinguyen@opensource.altera.com
Adopted from the Linux kernel PL330 DMA driver.
Signed-off-by: Dinh Nguyen dinguyen@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 sjg@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.brar@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"helpEnable 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.brar@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);elsebuf[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;elsego.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