dma: lpc32xx: add DMA driver
Subsystems (3)
PATCH v5 1/5 dma: lpc32xx: add DMA driver
From: Sylvain Lemieux <[email protected]>
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.
---
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
arch/arm/cpu/arm926ejs/lpc32xx/devices.c +6 -0
@@ -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 ); +} +void lpc32xx_i2c_init (unsigned int devnum) {/* Enable I2C interface */
arch/arm/include/asm/arch-lpc32xx/clk.h +3 -0
@@ -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)
arch/arm/include/asm/arch-lpc32xx/dma.h new file +37 -0
@@ -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); + +#endif /* _LPC32XX_DMA_H */
arch/arm/include/asm/arch-lpc32xx/sys_proto.h +1 -0
@@ -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 );void lpc32xx_i2c_init (unsigned int devnum);void lpc32xx_ssp_init (void );#if defined(CONFIG_SPL_BUILD )
drivers/dma/Makefile +1 -0
@@ -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
drivers/dma/lpc32xx_dma.c new file +163 -0
163 lines not on this page · raw message
Signature · 1 line
1.8.3.1
PATCH v5 1/5 dma: lpc32xx: add DMA driver
Quoted 11 lines · slemieux.tyco at gmail.com
From: Sylvain Lemieux <[email protected]>
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 <[email protected]>
[...]
Quoted 4 lines
drivers/dma/lpc32xx_dma.c new file +4
-0+int lpc32xx_dma_wait_status (int channel) +{ +unsigned long start; +u32 reg;
drivers/dma/lpc32xx_dma.c line 133
Quoted 10 lines
drivers/dma/lpc32xx_dma.c new file +10
-0+ + 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 ) +return -1 ;
return -ETIMEDOUT;
drivers/dma/lpc32xx_dma.c line 146
Quoted 12 lines
drivers/dma/lpc32xx_dma.c new file +12
-0+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)); +return -1 ; + } +setbits_le32 (&dma->int_tc_clear,BIT_MASK (channel)); +setbits_le32 (&dma->raw_tc_stat,BIT_MASK (channel)); +return 0 ; +}
Best regards,
Marek Vasut
Vladimir Zapolskiy
maintainerPATCH v5 1/5 dma: lpc32xx: add DMA driver
In reply to slemieux.tyco at gmail.comHi Sylvain,
Quoted 66 lines · slemieux.tyco at gmail.com
From: Sylvain Lemieux <[email protected]>
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 <[email protected]>---
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.carch/arm/cpu/arm926ejs/lpc32xx/devices.c +0
-0
arch/arm/cpu/arm926ejs/lpc32xx/devices.c void lpc32xx_slc_nand_init(void) · line 71
Quoted 7 lines
arch/arm/cpu/arm926ejs/lpc32xx/devices.c +4
-0@@ -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.
Quoted 50 lines
arch/arm/cpu/arm926ejs/lpc32xx/devices.c +2
-0+} +void lpc32xx_i2c_init (unsigned int devnum) {/* Enable I2C interface */ arch/arm/include/asm/arch-lpc32xx/clk.h +3
-0@@ -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)arch/arm/include/asm/arch-lpc32xx/dma.h new file +25
-0@@ -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;
arch/arm/include/asm/arch-lpc32xx/dma.h line 35
Quoted 10 lines
arch/arm/include/asm/arch-lpc32xx/dma.h new file +10
-0+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?
arch/arm/include/asm/arch-lpc32xx/sys_proto.h void lpc32xx_uart_init(unsigned int uart_id); · line 16
Quoted 11 lines
arch/arm/include/asm/arch-lpc32xx/dma.h new file +2
-0+ +#endif /* _LPC32XX_DMA_H */ arch/arm/include/asm/arch-lpc32xx/sys_proto.h +1
-0@@ -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.
Quoted 31 lines
arch/arm/include/asm/arch-lpc32xx/sys_proto.h +0
-0void lpc32xx_i2c_init (unsigned int devnum);void lpc32xx_ssp_init (void );#if defined(CONFIG_SPL_BUILD )drivers/dma/Makefile +1
-0@@ -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.odrivers/dma/lpc32xx_dma.c new file +13
-0@@ -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>
drivers/dma/lpc32xx_dma.c line 23
Quoted 10 lines
drivers/dma/lpc32xx_dma.c new file +10
-0+#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.
Quoted 70 lines
drivers/dma/lpc32xx_dma.c new file +70
-0+/* 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 */
drivers/dma/lpc32xx_dma.c line 104
Quoted 10 lines
drivers/dma/lpc32xx_dma.c new file +10
-0+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.
drivers/dma/lpc32xx_dma.c line 105
Quoted 1 line
drivers/dma/lpc32xx_dma.c new file +1
-0+ 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.
drivers/dma/lpc32xx_dma.c line 107
Quoted 2 lines
drivers/dma/lpc32xx_dma.c new file +2
-0+ 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.
Quoted 15 lines
drivers/dma/lpc32xx_dma.c new file +15
-0+ } +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;
drivers/dma/lpc32xx_dma.c line 132
Quoted 10 lines
drivers/dma/lpc32xx_dma.c new file +10
-0+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.
drivers/dma/lpc32xx_dma.c line 140
Quoted 8 lines
drivers/dma/lpc32xx_dma.c new file +8
-0+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.
Quoted 14 lines
drivers/dma/lpc32xx_dma.c new file +13
-0+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);
drivers/dma/lpc32xx_dma.c line 163
Quoted 10 lines
drivers/dma/lpc32xx_dma.c new file +10
-0+ +/* 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
LEMIEUX
PATCH v5 1/5 dma: lpc32xx: add DMA driver
Hi Vladimir,
Thanks for the feedback;
Marek, Vladimir,
there is a question below; I will wait for feedback before sending an updated version of the patch.
Sylvain
Quoted 17 lines
-----Original Message-----
From: Vladimir Zapolskiy [mailto:vz at mleia.com]Hi Sylvain,
Quoted 11 lines · slemieux.tyco at gmail.com
From: Sylvain Lemieux <[email protected]>
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 <[email protected]>---
[...]
Quoted 10 lines
Quoted 10 lines
arch/arm/include/asm/arch-lpc32xx/dma.h new file +3
-0@@ -0,0 +1,37 @@ +/* +* LPC32xx DMA Controller Interface +*
[...]
Quoted 10 lines
arch/arm/include/asm/arch-lpc32xx/dma.h line 35
Quoted 6 lines
arch/arm/include/asm/arch-lpc32xx/dma.h new file +6
-0+ +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?
The legacy NXP BSP driver was providing the support the get and release a channel; I kept it there, knowing is currently not used.
Do we want to keep this feature or remove it and only allowed channel allocation only?
[...]
Quoted 3 lines
Quoted 3 lines
+++
b/drivers/dma/lpc32xx_dma.carch/arm/include/asm/arch-lpc32xx/dma.h new file +1
-0@@ -0,0 +1,163 @@ +/*
[...]
Quoted 11 lines
arch/arm/include/asm/arch-lpc32xx/dma.h
Quoted 5 lines
arch/arm/include/asm/arch-lpc32xx/dma.h new file +5
-0+ +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.
This was the legacy NXP BSP original behavior.
I will change the channel to "unsigned int" for the transfer API;
the reserve API will continue to be return the channel as "int", to allowed validation; the NAND SLC will keep the channel number as an "unsigned int".
Quoted 3 lines
Quoted 2 lines
+ printf("ERR: Request for xfer on unallocated channel %d\r\n",
[...]
Quoted 7 lines
arch/arm/include/asm/arch-lpc32xx/dma.h
Quoted 3 lines
arch/arm/include/asm/arch-lpc32xx/dma.h new file +2
-0+ 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.
I will modify it to return -1 on error and check on client side; The client side will generate the fatal BUG().
Quoted 1 line
[...]
Quoted 4 lines
--
With best wishes,
Vladimir
________________________________
This e-mail contains privileged and confidential information intended for the use of the addressees named above. If you are not the intended recipient of this e-mail, you are hereby notified that you must not disseminate, copy or take any action in respect of any information contained in it. If you have received this e-mail in error, please notify the sender immediately by e-mail and immediately destroy this e-mail and its attachments.
Vladimir Zapolskiy
maintainerPATCH v5 1/5 dma: lpc32xx: add DMA driver
Hi Sylvain,
Quoted 54 lines · SYLVAIN
Hi Vladimir,
Thanks for the feedback;
Marek, Vladimir,
there is a question below; I will wait for feedback before sending an updated version of the patch.
Sylvain
Quoted 17 lines
-----Original Message-----
From: Vladimir Zapolskiy [mailto:vz at mleia.com]Hi Sylvain,
Quoted 11 lines · slemieux.tyco at gmail.com
From: Sylvain Lemieux <[email protected]>
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 <[email protected]>---
[...]
Quoted 10 lines
Quoted 10 lines
arch/arm/include/asm/arch-lpc32xx/dma.h new file +3
-0@@ -0,0 +1,37 @@ +/* +* LPC32xx DMA Controller Interface +* [...]
Quoted 10 lines
arch/arm/include/asm/arch-lpc32xx/dma.h line 35
Quoted 6 lines
arch/arm/include/asm/arch-lpc32xx/dma.h new file +6
-0+ +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?
Quoted 6 lines
The legacy NXP BSP driver was providing the support the get and release a channel; I kept it there, knowing is currently not used.
Do we want to keep this feature or remove it and only allowed channel allocation only?
[...]
I have no definite answer, in my opinion dead code should be removed, if its use is not planned. I would recommend to get the answer from a maintainer, who accepts this code (Tom Rini ?).
Quoted 15 lines
Quoted 3 lines
Quoted 3 lines
+++
b/drivers/dma/lpc32xx_dma.carch/arm/include/asm/arch-lpc32xx/dma.h new file +1
-0@@ -0,0 +1,163 @@ +/* [...]
Quoted 8 lines
arch/arm/include/asm/arch-lpc32xx/dma.h
Quoted 5 lines
arch/arm/include/asm/arch-lpc32xx/dma.h new file +5
-0+ +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?
Quoted 8 lines
Quoted 3 lines
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.
This was the legacy NXP BSP original behavior.
I will change the channel to "unsigned int" for the transfer API;
the reserve API will continue to be return the channel as "int", to allowed validation;
Generally there is no need to preserve any correspondence between mainlined code and code found in legacy BSP.
Quoted 1 line
the NAND SLC will keep the channel number as an "unsigned int".
Okay, in this case the correctness of the passed argument is offloaded to a client. Fortunately the sole client NAND SLC does not pass -1 here (the statement is based on code review).
Quoted 8 lines
Quoted 3 lines
Quoted 2 lines
+ printf("ERR: Request for xfer on unallocated channel %d\r\n",
[...]
Quoted 4 lines
Quoted 3 lines
arch/arm/include/asm/arch-lpc32xx/dma.h new file +2
-0+ channel); +BUG ();
Quoted 7 lines
Quoted 3 lines
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.I will modify it to return -1 on error and check on client side; The client side will generate the fatal
BUG().
Sounds good, I can imagine a potential client, which may gracefully handle this error, but NAND drivers are not in the list. If problem is encountered here, it means that the problem is on the client side and it is fair enough to place BUG() right on that client side.
--
With best wishes,
Vladimir
LEMIEUX
PATCH v5 1/5 dma: lpc32xx: add DMA driver
Quoted 65 lines
-----Original Message-----
From: Vladimir Zapolskiy [mailto:vz at mleia.com]
Sent: 6-Aug-15 2:43 PMHi Sylvain,
Quoted 57 lines · SYLVAIN
Hi Vladimir,
Thanks for the feedback;
Marek, Vladimir,
there is a question below; I will wait for feedback before sending an updated version of the patch.
Sylvain
Quoted 17 lines
-----Original Message-----
From: Vladimir Zapolskiy [mailto:vz at mleia.com]Hi Sylvain,
Quoted 11 lines · slemieux.tyco at gmail.com
From: Sylvain Lemieux <[email protected]>
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 <[email protected]>---
[...]
Quoted 10 lines
Quoted 10 lines
arch/arm/include/asm/arch-lpc32xx/dma.h new file +3
-0@@ -0,0 +1,37 @@ +/* +* LPC32xx DMA Controller Interface +* [...]
Quoted 10 lines
arch/arm/include/asm/arch-lpc32xx/dma.h line 35
Quoted 6 lines
arch/arm/include/asm/arch-lpc32xx/dma.h new file +6
-0+ +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?The legacy NXP BSP driver was providing the support the get and release a channel; I kept it there, knowing is currently not used.
Quoted 8 lines
Quoted 3 lines
Do we want to keep this feature or remove it and only allowed channel allocation only?
[...]
I have no definite answer, in my opinion dead code should be removed, if its use is not planned. I would recommend to get the answer from a maintainer, who accepts this code (Tom Rini ?).
Agree with you regarding dead code;
This code will only be useful if we were managing DMA channel allocation and release. I will remove it, as there is currently no need to support this functionality. If it is require one day, it can be added later.
[...]
Quoted 4 lines
--
With best wishes,
Vladimir
________________________________
This e-mail contains privileged and confidential information intended for the use of the addressees named above. If you are not the intended recipient of this e-mail, you are hereby notified that you must not disseminate, copy or take any action in respect of any information contained in it. If you have received this e-mail in error, please notify the sender immediately by e-mail and immediately destroy this e-mail and its attachments.