[U-Boot] [PATCH 5/8] spl: nand: sunxi: use PIO instead of DMA

[Miquel Raynal <miquel.raynal@free-electrons.com>]

Hi Boris,

On Wed, 24 Jan 2018 09:16:19 +0100
Boris Brezillon <boris.brezillon@free-electrons.com> wrote:

> On Wed, 24 Jan 2018 01:44:51 +0100
> Miquel Raynal <miquel.raynal@free-electrons.com> wrote:
> 
> > Because using DMA implementation is not generic and was not developped
> > to work on SoCs like A33, migrate the SPL to use PIO.
> > 
> > Signed-off-by: Miquel Raynal <miquel.raynal@free-electrons.com>
> > ---
> >  board/sunxi/board.c               |   4 +-
> >  drivers/mtd/nand/sunxi_nand_spl.c | 167 +++++++++++++++++---------------------
> >  2 files changed, 79 insertions(+), 92 deletions(-)
> > 
> > diff --git a/board/sunxi/board.c b/board/sunxi/board.c
> > index 70e01437c4..512e2c17a6 100644
> > --- a/board/sunxi/board.c
> > +++ b/board/sunxi/board.c
> > @@ -266,11 +266,13 @@ static void nand_clock_setup(void)
> >  	struct sunxi_ccm_reg *const ccm =
> >  		(struct sunxi_ccm_reg *)SUNXI_CCM_BASE;
> >  
> > -	setbits_le32(&ccm->ahb_gate0, (CLK_GATE_OPEN << AHB_GATE_OFFSET_NAND0));
> > +	setbits_le32(&ccm->ahb_gate0, (1 << AHB_GATE_OFFSET_NAND0));
> > +	setbits_le32(&ccm->ahb_reset0_cfg, (1 << AHB_GATE_OFFSET_NAND0));
> >  #ifdef CONFIG_MACH_SUN9I
> >  	setbits_le32(&ccm->ahb_gate1, (1 << AHB_GATE_OFFSET_DMA));
> >  #else
> >  	setbits_le32(&ccm->ahb_gate0, (1 << AHB_GATE_OFFSET_DMA));
> > +	setbits_le32(&ccm->ahb_reset0_cfg, (1 << AHB_GATE_OFFSET_DMA));
> >  #endif
> >  	setbits_le32(&ccm->nand0_clk_cfg, CCM_NAND_CTRL_ENABLE | AHB_DIV_1);
> >  }
> > diff --git a/drivers/mtd/nand/sunxi_nand_spl.c b/drivers/mtd/nand/sunxi_nand_spl.c
> > index 2488d5cb51..5de6825994 100644
> > --- a/drivers/mtd/nand/sunxi_nand_spl.c
> > +++ b/drivers/mtd/nand/sunxi_nand_spl.c
> > @@ -10,6 +10,7 @@
> >  #include <common.h>
> >  #include <config.h>
> >  #include <nand.h>
> > +#include <linux/ctype.h>
> >  
> >  /* registers */
> >  #define NFC_CTL                    0x00000000
> > @@ -45,32 +46,22 @@
> >  #define NFC_CTL_PAGE_SIZE_MASK     (0xf << 8)
> >  #define NFC_CTL_PAGE_SIZE(a)       ((fls(a) - 11) << 8)
> >  
> > -
> >  #define NFC_ECC_EN                 (1 << 0)
> > -#define NFC_ECC_PIPELINE           (1 << 3)
> >  #define NFC_ECC_EXCEPTION          (1 << 4)
> >  #define NFC_ECC_BLOCK_SIZE         (1 << 5)
> >  #define NFC_ECC_RANDOM_EN          (1 << 9)
> > -#define NFC_ECC_RANDOM_DIRECTION   (1 << 10)
> > -
> >  
> >  #define NFC_ADDR_NUM_OFFSET        16
> > -#define NFC_ACCESS_DIR             (1 << 20)
> >  #define NFC_SEND_ADDR              (1 << 19)
> >  #define NFC_DATA_TRANS             (1 << 21)
> >  #define NFC_SEND_CMD1              (1 << 22)
> >  #define NFC_WAIT_FLAG              (1 << 23)
> >  #define NFC_SEND_CMD2              (1 << 24)
> > -#define NFC_SEQ                    (1 << 25)
> > -#define NFC_DATA_SWAP_METHOD       (1 << 26)
> > -#define NFC_ROW_AUTO_INC           (1 << 27)
> > -#define NFC_SEND_CMD3              (1 << 28)
> > -#define NFC_SEND_CMD4              (1 << 29)
> >  #define NFC_RAW_CMD                (0 << 30)
> > -#define NFC_PAGE_CMD               (2 << 30)
> > +#define NFC_ECC_CMD                (1 << 30)
> >  
> >  #define NFC_ST_CMD_INT_FLAG        (1 << 1)
> > -#define NFC_ST_DMA_INT_FLAG        (1 << 2)
> > +#define NFC_ST_CMD_FIFO_STAT       (1 << 3)
> >  
> >  #define NFC_READ_CMD_OFFSET         0
> >  #define NFC_RANDOM_READ_CMD0_OFFSET 8
> > @@ -80,22 +71,6 @@
> >  #define NFC_CMD_RNDOUT             0x05
> >  #define NFC_CMD_READSTART          0x30
> >  
> > -#define SUNXI_DMA_CFG_REG0              0x300
> > -#define SUNXI_DMA_SRC_START_ADDR_REG0   0x304
> > -#define SUNXI_DMA_DEST_START_ADDRR_REG0 0x308
> > -#define SUNXI_DMA_DDMA_BC_REG0          0x30C
> > -#define SUNXI_DMA_DDMA_PARA_REG0        0x318
> > -
> > -#define SUNXI_DMA_DDMA_CFG_REG_LOADING  (1 << 31)
> > -#define SUNXI_DMA_DDMA_CFG_REG_DMA_DEST_DATA_WIDTH_32 (2 << 25)
> > -#define SUNXI_DMA_DDMA_CFG_REG_DDMA_DST_DRQ_TYPE_DRAM (1 << 16)
> > -#define SUNXI_DMA_DDMA_CFG_REG_DMA_SRC_DATA_WIDTH_32 (2 << 9)
> > -#define SUNXI_DMA_DDMA_CFG_REG_DMA_SRC_ADDR_MODE_IO (1 << 5)
> > -#define SUNXI_DMA_DDMA_CFG_REG_DDMA_SRC_DRQ_TYPE_NFC (3 << 0)
> > -
> > -#define SUNXI_DMA_DDMA_PARA_REG_SRC_WAIT_CYC (0x0F << 0)
> > -#define SUNXI_DMA_DDMA_PARA_REG_SRC_BLK_SIZE (0x7F << 8)
> > -
> >  struct nfc_config {
> >  	int page_size;
> >  	int ecc_strength;
> > @@ -254,7 +229,23 @@ static int nand_reset_column(void)
> >  	       SUNXI_NFC_BASE + NFC_CMD);
> >  
> >  	return nand_wait_int();
> > +}
> >  
> > +static int nand_change_column(u16 column)
> > +{
> > +	nand_wait_cmd_fifo_empty();
> > +
> > +	writel((NFC_CMD_RNDOUTSTART << NFC_RANDOM_READ_CMD1_OFFSET) |
> > +	       (NFC_CMD_RNDOUT << NFC_RANDOM_READ_CMD0_OFFSET) |
> > +	       (NFC_CMD_RNDOUTSTART << NFC_READ_CMD_OFFSET),
> > +	       SUNXI_NFC_BASE + NFC_RCMD_SET);
> > +	writel(NFC_ST_CMD_INT_FLAG, SUNXI_NFC_BASE + NFC_ST);
> > +	writel(column, SUNXI_NFC_BASE + NFC_ADDR_LOW);
> > +	writel(NFC_SEND_CMD1 | NFC_SEND_CMD2 | NFC_RAW_CMD |
> > +	       (1 << NFC_ADDR_NUM_OFFSET) | NFC_SEND_ADDR | NFC_CMD_RNDOUT,
> > +	       SUNXI_NFC_BASE + NFC_CMD);
> > +
> > +	return nand_wait_int();
> >  }  
> 
> nand_reset_column() and nand_change_column() are almost the same,
> except one is taking an argument to specify where you want to place the
> pointer. I'd recommend getting rid of nand_reset_column() entirely and
> patching all call sites to call nand_change_column(0) instead.

Done in a separate commit.


> 
> >  
> >  static const int ecc_bytes[] = {32, 46, 54, 60, 74, 88, 102, 110, 116};
> > @@ -262,86 +253,80 @@ static const int ecc_bytes[] = {32, 46, 54, 60, 74, 88, 102, 110, 116};
> >  static int nand_read_page(const struct nfc_config *conf, u32 offs,
> >  			  void *dest, int len)
> >  {
> > -	dma_addr_t dst = (dma_addr_t)dest;
> >  	int nsectors = len / conf->ecc_size;
> >  	u16 rand_seed = 0;
> > -	u32 val;
> > -	int page;
> > -
> > -	page = offs / conf->page_size;
> > +	int oob_chunk_sz = ecc_bytes[conf->ecc_strength];
> > +	int page = offs / conf->page_size;
> > +	u32 ecc_st;
> > +	int i;
> >  
> >  	if (offs % conf->page_size || len % conf->ecc_size ||
> >  	    len > conf->page_size || len < 0)
> >  		return -EINVAL;
> >  
> > -	/* clear ecc status */
> > -	writel(0, SUNXI_NFC_BASE + NFC_ECC_ST);
> > -
> >  	/* Choose correct seed if randomized */
> >  	if (conf->randomize)
> >  		rand_seed = random_seed[page % conf->nseeds];
> >  
> > -	writel((rand_seed << 16) | (conf->ecc_strength << 12) |
> > -		(conf->randomize ? NFC_ECC_RANDOM_EN : 0) |
> > -		(conf->ecc_size == 512 ? NFC_ECC_BLOCK_SIZE : 0) |
> > -		NFC_ECC_EN | NFC_ECC_PIPELINE | NFC_ECC_EXCEPTION,
> > -		SUNXI_NFC_BASE + NFC_ECC_CTL);
> > -
> > -	flush_dcache_range(dst, ALIGN(dst + conf->ecc_size, ARCH_DMA_MINALIGN));
> > -
> > -	/* SUNXI_DMA */
> > -	writel(0x0, SUNXI_DMA_BASE + SUNXI_DMA_CFG_REG0); /* clr dma cmd */
> > -	/* read from REG_IO_DATA */
> > -	writel(SUNXI_NFC_BASE + NFC_IO_DATA,
> > -	       SUNXI_DMA_BASE + SUNXI_DMA_SRC_START_ADDR_REG0);
> > -	/* read to RAM */
> > -	writel(dst, SUNXI_DMA_BASE + SUNXI_DMA_DEST_START_ADDRR_REG0);
> > -	writel(SUNXI_DMA_DDMA_PARA_REG_SRC_WAIT_CYC |
> > -	       SUNXI_DMA_DDMA_PARA_REG_SRC_BLK_SIZE,
> > -	       SUNXI_DMA_BASE + SUNXI_DMA_DDMA_PARA_REG0);
> > -	writel(len, SUNXI_DMA_BASE + SUNXI_DMA_DDMA_BC_REG0);
> > -	writel(SUNXI_DMA_DDMA_CFG_REG_LOADING |
> > -	       SUNXI_DMA_DDMA_CFG_REG_DMA_DEST_DATA_WIDTH_32 |
> > -	       SUNXI_DMA_DDMA_CFG_REG_DDMA_DST_DRQ_TYPE_DRAM |
> > -	       SUNXI_DMA_DDMA_CFG_REG_DMA_SRC_DATA_WIDTH_32 |
> > -	       SUNXI_DMA_DDMA_CFG_REG_DMA_SRC_ADDR_MODE_IO |
> > -	       SUNXI_DMA_DDMA_CFG_REG_DDMA_SRC_DRQ_TYPE_NFC,
> > -	       SUNXI_DMA_BASE + SUNXI_DMA_CFG_REG0);
> > -
> > -	writel(nsectors, SUNXI_NFC_BASE + NFC_SECTOR_NUM);
> > -	writel(NFC_ST_DMA_INT_FLAG, SUNXI_NFC_BASE + NFC_ST);
> > -	writel(NFC_DATA_TRANS |	NFC_PAGE_CMD | NFC_DATA_SWAP_METHOD,
> > -	       SUNXI_NFC_BASE + NFC_CMD);
> > +	/* Retrieve data from SRAM (PIO) */
> > +	for (i = 0; i < nsectors; i++) {
> > +		int data_off = i * conf->ecc_size;
> > +		int oob_off = conf->page_size + (i * oob_chunk_sz);
> > +		u8 *data = dest + data_off;
> > +
> > +		/* Clear ECC status and restart ECC engine */
> > +		writel(0, SUNXI_NFC_BASE + NFC_ECC_ST);
> > +		writel((rand_seed << 16) | (conf->ecc_strength << 12) |
> > +		       (conf->randomize ? NFC_ECC_RANDOM_EN : 0) |
> > +		       (conf->ecc_size == 512 ? NFC_ECC_BLOCK_SIZE : 0) |
> > +		       NFC_ECC_EN | NFC_ECC_EXCEPTION,
> > +		       SUNXI_NFC_BASE + NFC_ECC_CTL);
> > +
> > +		/* Move the data in SRAM */
> > +		nand_change_column(data_off);
> > +		nand_wait_cmd_fifo_empty();
> > +		writel(NFC_ST_CMD_INT_FLAG, SUNXI_NFC_BASE + NFC_ST);
> > +		writel(conf->ecc_size, SUNXI_NFC_BASE + NFC_CNT);
> > +		writel(NFC_DATA_TRANS, SUNXI_NFC_BASE + NFC_CMD);
> > +		nand_wait_int();
> >  
> > -	if (!check_value(SUNXI_NFC_BASE + NFC_ST, NFC_ST_DMA_INT_FLAG,
> > -			 DEFAULT_TIMEOUT_US)) {
> > -		printf("Error while initializing dma interrupt\n");
> > -		return -EIO;
> > -	}
> > -	writel(NFC_ST_DMA_INT_FLAG, SUNXI_NFC_BASE + NFC_ST);
> > +		/*
> > +		 * Let the ECC engine consume the ECC bytes and possibly correct
> > +		 * the data.
> > +		 */
> > +		nand_change_column(oob_off);
> > +		nand_wait_cmd_fifo_empty();
> > +		writel(NFC_ST_CMD_INT_FLAG, SUNXI_NFC_BASE + NFC_ST);
> > +		writel(NFC_DATA_TRANS | NFC_ECC_CMD, SUNXI_NFC_BASE + NFC_CMD);
> > +		nand_wait_int();
> >  
> > -	if (!check_value_negated(SUNXI_DMA_BASE + SUNXI_DMA_CFG_REG0,
> > -				 SUNXI_DMA_DDMA_CFG_REG_LOADING,
> > -				 DEFAULT_TIMEOUT_US)) {
> > -		printf("Error while waiting for dma transfer to finish\n");
> > -		return -EIO;
> > -	}
> > +		/* Get the ECC status */
> > +		ecc_st = readl(SUNXI_NFC_BASE + NFC_ECC_ST);
> >  
> > -	invalidate_dcache_range(dst,
> > -				ALIGN(dst + conf->ecc_size, ARCH_DMA_MINALIGN));
> > +		/* Retrieve the data from SRAM */
> > +		memcpy_fromio(data, SUNXI_NFC_BASE + NFC_RAM0_BASE,
> > +			      conf->ecc_size);  
> 
> Not a big deal, but should we really copy the data if the ECC engine
> reports an error? Maybe you can move this memcpy_fromio() after checking
> ecc_st.

Ok.

> 
> >  
> > -	val = readl(SUNXI_NFC_BASE + NFC_ECC_ST);
> > +		/* Stop ECC engine */
> > +		writel(readl(SUNXI_NFC_BASE + NFC_ECC_CTL) & ~NFC_ECC_EN,
> > +		       SUNXI_NFC_BASE + NFC_ECC_CTL);
> >  
> > -	/* ECC error detected. */
> > -	if (val & 0xffff)
> > -		return -EIO;
> > +		/* ECC error detected. */
> > +		if (ecc_st & 0xffff)
> > +			return -EIO;
> >  
> > -	/*
> > -	 * Return 1 if the page is empty.
> > -	 * We consider the page as empty if the first ECC block is marked
> > -	 * empty.
> > -	 */
> > -	return (val & 0x10000) ? 1 : 0;
> > +		/*
> > +		 * Return 1 if the page is empty. We consider the page as empty
> > +		 * if the first ECC block is marked empty.
> > +		 */
> > +		if (ecc_st & 0x10000)
> > +			return 1;  
> 
> This is no longer valid, because now you iterate over all ECC chunks
> manually. The test should be:
> 
> 		if (!i && (ecc_st & 0x10000))
> 

That is right and could have caused troubles, thanks for spotting it!

> > +
> > +		if (data_off + conf->ecc_size >= len)
> > +			break;
> > +	}
> > +
> > +	return 0;
> >  }
> >  
> >  static int nand_max_ecc_strength(struct nfc_config *conf)  
> 

Thanks for reviewing,
Miquèl

-- 
Miquel Raynal, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com