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

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

Hi Maxime,

I have some doubts about your comments, see below.

On Wed, 24 Jan 2018 09:06:26 +0100
Maxime Ripard <maxime.ripard@free-electrons.com> wrote:

> Hi,
> 
> On Wed, Jan 24, 2018 at 01:44:51AM +0100, Miquel Raynal 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>  
> 
> I guess the issue is slightly different than what you're saying, or at
> least should be expressed differently.
> 
> The issue is that the SPL support has be done to support only the
> earlier generations of Allwinner SoCs, and only really enabled on the
> A13 / GR8. However, those old SoCs had a DMA engine that has been
> replaced since the A31 by another DMA controller that is no longer
> compatible.
> 
> Since the code directly uses that DMA controller, it cannot operate
> properly on the later SoCs, while the NAND controller hasn't changed.
> 
> There's two pathes forward, the first one would be to add support for
> that DMA controller too, or you can just remove the DMA usage entirely
> and rely on PIO that will make the driver simpler.
> 
> Explaining why you chose PIO would be great too.
> 
> > ---
> >  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));  
> 
> That has nothing to do with PIO vs DMA. It should be in another patch.

Ok.

> 
> >  #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));  
> 
> So you're de-asserting the reset line for the DMA controller, while
> the commit is about removing the need for that DMA controller ? :)
> 
> That whole block can actually be removed (in a separate patch) now
> that you're not using DMA anymore.

Shall I remove only what is after the else statement? Or does MACH_SUN9I
use the same code so I can remove the whole #ifdef/#else/#endif
statement?

> 
> >  #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)
> > -  
> 
> We should probably keep those values. They're not used anymore, but
> the NAND controller has been under-documented on most of the datasheet
> we've had. It's good to keep it at least for reference.

I will remove DMA related defines, and keep the other ones.

> 
> >  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();
> >  }
> >  
> >  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);  
> 
> It seems that you're also converting this code to some of your new
> helpers. This should be done in the patch introducing them.

Actually no, it is close but this is DMA specific, and does not fit
with the previously created helpers. The first reason why this block is
removed is because it is related to DMA.

> 
> > +		/*
> > +		 * 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;
> > -	}  
> 
> Ditto.

Same as above.

Thanks for reviewing,
Miquèl