[U-Boot] [PATCH v5 1/5] dma: lpc32xx: add DMA driver
Vladimir Zapolskiy
vz at mleia.com
Wed Aug 5 22:48:24 CEST 2015
Hi Sylvain,
On 05.08.2015 21:31, slemieux.tyco at gmail.com wrote:
> From: Sylvain Lemieux <slemieux at tycoint.com>
>
> Incorporate DMA driver from legacy LPCLinux NXP BSP.
> The files taken from the legacy patch are:
> - lpc32xx DMA driver
> - lpc3250 header file DMA registers definition.
>
> The legacy driver was updated and clean-up as part of the integration with the latest u-boot.
>
> Signed-off-by: Sylvain Lemieux <slemieux at tycoint.com>
> ---
> Changes from v4 to v5:
> * Addressed Marek's comments on LPC32xx DMA driver:
> - Move macro use by the DMA client to write the next
> DMA linked list item address to NAND SLC driver.
> - updated "lpc32xx_dma_wait_status()" implementation
> to make if stetement easier to read.
> - updated multiline comments style.
>
> Changes from v3 to v4:
> * Addressed Marek's comments on LPC32xx DMA driver:
> - updated multiline comments style.
> - use "u32" instead of "uint32_t".
> - fixed unbounded loop.
>
> Changes from v2 to v3:
> * Addressed Marek's comments on LPC32xx DMA driver:
> - use defined in header file instead of local definition.
> - remove for loop and use "ffz()".
> * Provide define to assign next DMA linked list item address
> to removed typecast in DMA client (ex. NAND SLC).
>
> Changes from v1 to v2:
> * Moved the DMA patch as the first patch of the series.
> * The NAND SLC patch (link below) is applied before this
> patch: https://patchwork.ozlabs.org/patch/497308/
>
> Update to the legacy driver to integrate with the latest u-boot:
> 1) Fixed checkpatch script output in legacy code.
> 2) Use LPC32xx definition from "cpu.h" and "clk.h".
> 3) Incorporate DMA specific register definition from "lpc3250.h"
> header file from legacy BSP patch from LPCLinux.
> 4) Use u-boot API for register access to remove the volatile
> in register definition taken from "lpc3250.h" header file.
> 5) Add DMA interface to "dma.h".
> 6) Add dma clock control register bits (clk.h).
> 7) Add functions to initialize the DMA clock.
>
> The legacy BSP patch (u-boot-2009.03_lpc32x0-v1.07.patch.tar.bz2)
> was downloaded from the LPCLinux Web site.
>
> arch/arm/cpu/arm926ejs/lpc32xx/devices.c | 6 +
> arch/arm/include/asm/arch-lpc32xx/clk.h | 3 +
> arch/arm/include/asm/arch-lpc32xx/dma.h | 37 ++++++
> arch/arm/include/asm/arch-lpc32xx/sys_proto.h | 1 +
> drivers/dma/Makefile | 1 +
> drivers/dma/lpc32xx_dma.c | 163 ++++++++++++++++++++++++++
> 6 files changed, 211 insertions(+)
> create mode 100644 arch/arm/include/asm/arch-lpc32xx/dma.h
> create mode 100644 drivers/dma/lpc32xx_dma.c
>
> diff --git a/arch/arm/cpu/arm926ejs/lpc32xx/devices.c b/arch/arm/cpu/arm926ejs/lpc32xx/devices.c
> index c0c9c6c..0d2ef7a 100644
> --- a/arch/arm/cpu/arm926ejs/lpc32xx/devices.c
> +++ b/arch/arm/cpu/arm926ejs/lpc32xx/devices.c
> @@ -65,6 +65,12 @@ void lpc32xx_slc_nand_init(void)
> writel(CLK_NAND_SLC | CLK_NAND_SLC_SELECT, &clk->flashclk_ctrl);
> }
>
> +void lpc32xx_dma_init(void)
> +{
> + /* Enable DMA interface */
> + setbits_le32(&clk->dmaclk_ctrl, DMA_CLK_ENABLE);
it is better to have writel() here, since &clk->dmaclk_ctrl can be
contaminated, e.g. in case of soft reset or if uboot is executed from
another bootloader.
> +}
> +
> void lpc32xx_i2c_init(unsigned int devnum)
> {
> /* Enable I2C interface */
> diff --git a/arch/arm/include/asm/arch-lpc32xx/clk.h b/arch/arm/include/asm/arch-lpc32xx/clk.h
> index 010211a..663f6bc 100644
> --- a/arch/arm/include/asm/arch-lpc32xx/clk.h
> +++ b/arch/arm/include/asm/arch-lpc32xx/clk.h
> @@ -158,6 +158,9 @@ struct clk_pm_regs {
> #define CLK_NAND_SLC_SELECT (1 << 2)
> #define CLK_NAND_MLC_INT (1 << 5)
>
> +/* DMA Clock Control Register bits */
> +#define DMA_CLK_ENABLE (1 << 0)
> +
> /* SSP Clock Control Register bits */
> #define CLK_SSP0_ENABLE_CLOCK (1 << 0)
>
> diff --git a/arch/arm/include/asm/arch-lpc32xx/dma.h b/arch/arm/include/asm/arch-lpc32xx/dma.h
> new file mode 100644
> index 0000000..15d829c
> --- /dev/null
> +++ b/arch/arm/include/asm/arch-lpc32xx/dma.h
> @@ -0,0 +1,37 @@
> +/*
> + * LPC32xx DMA Controller Interface
> + *
> + * Copyright (C) 2008-2015 by NXP Semiconductors
> + * All rights reserved.
> + *
> + * @Author: Kevin Wells
> + * @Descr: Definitions for LPC3250 chip
> + * @References: NXP LPC3250 User's Guide
> + *
> + * SPDX-License-Identifier: GPL-2.0+
> + */
> +
> +#ifndef _LPC32XX_DMA_H
> +#define _LPC32XX_DMA_H
> +
> +#include <common.h>
> +
> +/*
> + * DMA linked list structure used with a channel's LLI register;
> + * refer to UM10326, "LPC32x0 and LPC32x0/01 User manual" - Rev. 3
> + * tables 84, 85, 86 & 87 for details.
> + */
> +struct lpc32xx_dmac_ll {
> + u32 dma_src;
> + u32 dma_dest;
> + u32 next_lli;
> + u32 next_ctrl;
> +};
> +
> +int lpc32xx_dma_get_channel(void);
> +int lpc32xx_dma_start_xfer(int channel, const struct lpc32xx_dmac_ll *desc,
> + u32 config);
> +int lpc32xx_dma_wait_status(int channel);
> +void lpc32xx_dma_put_channel(int channel);
There is no users of lpc32xx_dma_put_channel() interface, do you have
them in mind?
> +
> +#endif /* _LPC32XX_DMA_H */
> diff --git a/arch/arm/include/asm/arch-lpc32xx/sys_proto.h b/arch/arm/include/asm/arch-lpc32xx/sys_proto.h
> index 0845f83..7f997d9 100644
> --- a/arch/arm/include/asm/arch-lpc32xx/sys_proto.h
> +++ b/arch/arm/include/asm/arch-lpc32xx/sys_proto.h
> @@ -13,6 +13,7 @@ void lpc32xx_uart_init(unsigned int uart_id);
> void lpc32xx_mac_init(void);
> void lpc32xx_mlc_nand_init(void);
> void lpc32xx_slc_nand_init(void);
> +void lpc32xx_dma_init(void);
I would propose to try to follow alphabetic ordering here, yes,
lpc32xx_i2c_init() violates it, but please move lpc32xx_dma_init() upper.
> void lpc32xx_i2c_init(unsigned int devnum);
> void lpc32xx_ssp_init(void);
> #if defined(CONFIG_SPL_BUILD)
> diff --git a/drivers/dma/Makefile b/drivers/dma/Makefile
> index 4c8fcc2..f95fe70 100644
> --- a/drivers/dma/Makefile
> +++ b/drivers/dma/Makefile
> @@ -10,3 +10,4 @@ obj-$(CONFIG_APBH_DMA) += apbh_dma.o
> 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
> diff --git a/drivers/dma/lpc32xx_dma.c b/drivers/dma/lpc32xx_dma.c
> new file mode 100644
> index 0000000..0452013
> --- /dev/null
> +++ b/drivers/dma/lpc32xx_dma.c
> @@ -0,0 +1,163 @@
> +/*
> + * Copyright (C) 2008-2015 by NXP Semiconductors
> + * All rights reserved.
> + *
> + * @Author: Kevin Wells
> + * @Descr: LPC3250 DMA controller interface support functions
> + *
> + * SPDX-License-Identifier: GPL-2.0+
> + */
> +
> +#include <common.h>
> +#include <asm/arch/dma.h>
> +#include <asm/arch/cpu.h>
> +#include <asm/arch/clk.h>
> +#include <asm/arch/sys_proto.h>
> +#include <asm/io.h>
> +
> +/*
> + * Notes:
> + *
> + * Only bitfield masks/values which are actually used by the driver
> + * are defined.
> + */
The note can be removed, it does not add any useful information.
> +/* DMA controller channel register structure */
> +struct dmac_chan_reg {
> + u32 src_addr;
> + u32 dest_addr;
> + u32 lli;
> + u32 control;
> + u32 config_ch;
> + u32 reserved[3];
> +};
> +
> +/* DMA controller register structures */
> +struct dma_reg {
> + u32 int_stat;
> + u32 int_tc_stat;
> + u32 int_tc_clear;
> + u32 int_err_stat;
> + u32 int_err_clear;
> + u32 raw_tc_stat;
> + u32 raw_err_stat;
> + u32 chan_enable;
> + u32 sw_burst_req;
> + u32 sw_single_req;
> + u32 sw_last_burst_req;
> + u32 sw_last_single_req;
> + u32 config;
> + u32 sync;
> + u32 reserved[50];
> + struct dmac_chan_reg dma_chan[8];
> +};
> +
> +/* Macro pointing to DMA registers */
> +#define DMA_NO_OF_CHANNELS 8
> +
> +/* config register definitions */
> +#define DMAC_CTRL_ENABLE (1 << 0) /* For enabling the DMA controller */
> +
> +static u32 alloc_ch;
> +
> +static struct dma_reg *dma = (struct dma_reg *)DMA_BASE;
> +
> +int lpc32xx_dma_get_channel(void)
> +{
> + int i;
> +
> + if (!alloc_ch) { /* First time caller */
> + /*
> + * DMA clock are enable by "lpc32xx_dma_init()" and should
> + * be call by board "board_early_init_f()" function.
> + */
> +
> + /*
> + * Make sure DMA controller and all channels are disabled.
> + * Controller is in little-endian mode. Disable sync signals.
> + */
> + writel(0, &dma->config);
> + writel(0, &dma->sync);
> +
> + /* Clear interrupt and error statuses */
> + writel(0xFF, &dma->int_tc_clear);
> + writel(0xFF, &dma->raw_tc_stat);
> + writel(0xFF, &dma->int_err_clear);
> + writel(0xFF, &dma->raw_err_stat);
> +
> + /* Enable DMA controller */
> + writel(DMAC_CTRL_ENABLE, &dma->config);
> + }
> +
> + i = ffz(alloc_ch);
> +
> + /* Check if all the available channels are busy */
> + if (unlikely(i == DMA_NO_OF_CHANNELS))
> + return -1;
> + alloc_ch |= BIT_MASK(i);
> + return i;
> +}
> +
> +int lpc32xx_dma_start_xfer(int channel, const struct lpc32xx_dmac_ll *desc,
> + u32 config)
> +{
> + if (unlikely((BIT_MASK(channel) & alloc_ch) == 0)) {
Would it be possible to change "int channel" to some unsigned type?
If the intention is to reserve (channel < 0) for potentially
invalid/unallocated channel, as it is done in NAND SLC, please add a
check for this case here.
> + printf("ERR: Request for xfer on unallocated channel %d\r\n",
I believe "\r\n" can be replaced with "\n", both drivers of LPC32xx
UARTs handle this correctly.
> + channel);
> + BUG();
The function returns int type, but in fact it returns only 0, either
this function return type can be changed to void, or IMO better to
return error here instead of fatal BUG() and add a check on client side.
> + }
> + writel(BIT_MASK(channel), &dma->int_tc_clear);
> + writel(BIT_MASK(channel), &dma->int_err_clear);
> + writel(desc->dma_src, &dma->dma_chan[channel].src_addr);
> + writel(desc->dma_dest, &dma->dma_chan[channel].dest_addr);
> + writel(desc->next_lli, &dma->dma_chan[channel].lli);
> + writel(desc->next_ctrl, &dma->dma_chan[channel].control);
> + writel(config, &dma->dma_chan[channel].config_ch);
> +
> + return 0;
> +}
> +
> +int lpc32xx_dma_wait_status(int channel)
> +{
> + unsigned long start;
> + u32 reg;
> +
> + start = get_timer(0);
> + while (1) {
> + reg = readl(&dma->raw_tc_stat);
> + reg |= readl(dma->raw_err_stat);
> + if (reg & BIT_MASK(channel))
> + break;
> +
> + if (get_timer(start) > 10000)
Please define the delay value as a macro, macro name should contain _MS
suffix.
Also I would propose to add an explanatory pr_err() here.
> + return -1;
> +
> + udelay(1);
> + }
> +
> + if (unlikely(readl(&dma->raw_err_stat) & BIT_MASK(channel))) {
> + setbits_le32(&dma->int_err_clear, BIT_MASK(channel));
> + setbits_le32(&dma->raw_err_stat, BIT_MASK(channel));
I would propose to add explanatory pr_err() here to let user know what
happens.
> + return -1;
> + }
> + setbits_le32(&dma->int_tc_clear, BIT_MASK(channel));
> + setbits_le32(&dma->raw_tc_stat, BIT_MASK(channel));
> + return 0;
> +}
> +
> +void lpc32xx_dma_put_channel(int channel)
> +{
> + /* Check if given channel no is valid */
> + if (channel >= DMA_NO_OF_CHANNELS || channel < 0)
> + return;
> + alloc_ch &= ~BIT_MASK(channel);
> +
> + /* Shut down channel */
> + writel(0, &dma->dma_chan[channel].control);
> + writel(0, &dma->dma_chan[channel].config_ch);
> + clrbits_le32(&dma->sync, BIT_MASK(channel));
> +
> + if (!alloc_ch)
> + /* Disable DMA controller */
> + clrbits_le32(&dma->config, DMAC_CTRL_ENABLE);
> +}
>
--
With best wishes,
Vladimir
More information about the U-Boot
mailing list