[U-Boot] [PATCH 4/8] spl: nand: sunxi: Enhancements and cleaning

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

Hi Boris,

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

> On Wed, 24 Jan 2018 01:44:50 +0100
> Miquel Raynal <miquel.raynal@free-electrons.com> wrote:
> 
> > Do some cleaning in sunxi NAND SPL driver like adding helpers and
> > clearing flags at the right spot
> > 
> > Signed-off-by: Miquel Raynal <miquel.raynal@free-electrons.com>
> > ---
> >  drivers/mtd/nand/sunxi_nand_spl.c | 64 +++++++++++++++++++++++++--------------
> >  1 file changed, 41 insertions(+), 23 deletions(-)
> > 
> > diff --git a/drivers/mtd/nand/sunxi_nand_spl.c b/drivers/mtd/nand/sunxi_nand_spl.c
> > index 06695fc15f..2488d5cb51 100644
> > --- a/drivers/mtd/nand/sunxi_nand_spl.c
> > +++ b/drivers/mtd/nand/sunxi_nand_spl.c
> > @@ -55,8 +55,8 @@
> >  
> >  
> >  #define NFC_ADDR_NUM_OFFSET        16
> > -#define NFC_SEND_ADR               (1 << 19)
> >  #define NFC_ACCESS_DIR             (1 << 20)
> > +#define NFC_SEND_ADDR              (1 << 19)  
> 
> This typo fix probably deserves a separate patch.

Indeed.

> 
> >  #define NFC_DATA_TRANS             (1 << 21)
> >  #define NFC_SEND_CMD1              (1 << 22)
> >  #define NFC_WAIT_FLAG              (1 << 23)
> > @@ -155,6 +155,30 @@ static inline int check_value_negated(int offset, int unexpected_bits,
> >  	return check_value_inner(offset, unexpected_bits, timeout_us, 1);
> >  }
> >  
> > +static int nand_wait_cmd_fifo_empty(void)
> > +{
> > +	if (!check_value_negated(SUNXI_NFC_BASE + NFC_ST, NFC_ST_CMD_FIFO_STAT,
> > +				 DEFAULT_TIMEOUT_US)) {
> > +		printf("nand: timeout waiting for empty cmd FIFO\n");
> > +		return -ETIMEDOUT;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static int nand_wait_int(void)
> > +{
> > +	if (!check_value(SUNXI_NFC_BASE + NFC_ST, NFC_ST_CMD_INT_FLAG,
> > +			 DEFAULT_TIMEOUT_US)) {
> > +		printf("nand: timeout waiting for interruption\n");
> > +		return -ETIMEDOUT;
> > +	}
> > +
> > +	udelay(1);  
> 
> I'm pretty sure this udelay() is not required. Probably some leftover
> of your debug session ;-).

That is right -> removed.

> 
> > +
> > +	return 0;
> > +}
> > +
> >  void nand_init(void)
> >  {
> >  	uint32_t val;
> > @@ -172,22 +196,21 @@ void nand_init(void)
> >  	}
> >  
> >  	/* reset NAND */
> > +	nand_wait_cmd_fifo_empty();
> > +
> >  	writel(NFC_ST_CMD_INT_FLAG, SUNXI_NFC_BASE + NFC_ST);
> >  	writel(NFC_SEND_CMD1 | NFC_WAIT_FLAG | NAND_CMD_RESET,
> >  	       SUNXI_NFC_BASE + NFC_CMD);
> >  
> > -	if (!check_value(SUNXI_NFC_BASE + NFC_ST, NFC_ST_CMD_INT_FLAG,
> > -			 DEFAULT_TIMEOUT_US)) {
> > -		printf("Error timeout waiting for nand reset\n");
> > -		return;
> > -	}
> > -	writel(NFC_ST_CMD_INT_FLAG, SUNXI_NFC_BASE + NFC_ST);
> > +	nand_wait_int();  
> 
> I would go even further and create an helper that takes care of the
> 'wait_fifo_empty+clear_int+write_cmd+wait_int' sequence:
> 
> static int nand_exec_cmd(u32 cmd)
> {
> 	int ret;
> 
> 	ret = nand_wait_cmd_fifo_empty();
> 	if (ret)
> 		return ret;
> 
> 	writel(NFC_ST_CMD_INT_FLAG, SUNXI_NFC_BASE + NFC_ST);
> 	writel(cmd, SUNXI_NFC_BASE + NFC_CMD);
> 
> 	return nand_wait_int();
> }

It makes sense, I added another patch to introduce this helper.

> 
> >  }
> >  
> >  static void nand_apply_config(const struct nfc_config *conf)
> >  {
> >  	u32 val;
> >  
> > +	nand_wait_cmd_fifo_empty();
> > +
> >  	val = readl(SUNXI_NFC_BASE + NFC_CTL);
> >  	val &= ~NFC_CTL_PAGE_SIZE_MASK;
> >  	writel(val | NFC_CTL_RAM_METHOD | NFC_CTL_PAGE_SIZE(conf->page_size),
> > @@ -200,6 +223,8 @@ static int nand_load_page(const struct nfc_config *conf, u32 offs)
> >  {
> >  	int page = offs / conf->page_size;
> >  
> > +	nand_wait_cmd_fifo_empty();
> > +
> >  	writel((NFC_CMD_RNDOUTSTART << NFC_RANDOM_READ_CMD1_OFFSET) |
> >  	       (NFC_CMD_RNDOUT << NFC_RANDOM_READ_CMD0_OFFSET) |
> >  	       (NFC_CMD_READSTART << NFC_READ_CMD_OFFSET),
> > @@ -208,38 +233,32 @@ static int nand_load_page(const struct nfc_config *conf, u32 offs)
> >  	writel((page >> 16) & 0xFF, SUNXI_NFC_BASE + NFC_ADDR_HIGH);
> >  	writel(NFC_ST_CMD_INT_FLAG, SUNXI_NFC_BASE + NFC_ST);
> >  	writel(NFC_SEND_CMD1 | NFC_SEND_CMD2 | NFC_RAW_CMD | NFC_WAIT_FLAG |
> > -	       ((conf->addr_cycles - 1) << NFC_ADDR_NUM_OFFSET) | NFC_SEND_ADR,
> > +	       ((conf->addr_cycles - 1) << NFC_ADDR_NUM_OFFSET) | NFC_SEND_ADDR,
> >  	       SUNXI_NFC_BASE + NFC_CMD);
> >  
> > -	if (!check_value(SUNXI_NFC_BASE + NFC_ST, NFC_ST_CMD_INT_FLAG,
> > -			 DEFAULT_TIMEOUT_US)) {
> > -		printf("Error while initializing dma interrupt\n");
> > -		return -EIO;
> > -	}
> > -
> > -	return 0;
> > +	return nand_wait_int();
> >  }
> >  
> >  static int nand_reset_column(void)
> >  {
> > +	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(0, SUNXI_NFC_BASE + NFC_ADDR_LOW);
> >  	writel(NFC_SEND_CMD1 | NFC_SEND_CMD2 | NFC_RAW_CMD |
> > -	       (1 << NFC_ADDR_NUM_OFFSET) | NFC_SEND_ADR | NFC_CMD_RNDOUT,
> > +	       (1 << NFC_ADDR_NUM_OFFSET) | NFC_SEND_ADDR | NFC_CMD_RNDOUT,
> >  	       SUNXI_NFC_BASE + NFC_CMD);
> >  
> > -	if (!check_value(SUNXI_NFC_BASE + NFC_ST, NFC_ST_CMD_INT_FLAG,
> > -			 DEFAULT_TIMEOUT_US)) {
> > -		printf("Error while initializing dma interrupt\n");
> > -		return -1;
> > -	}
> > +	return nand_wait_int();  
> 
> Here, I think you should wait, just to make sure we don't read data
> before tCCS. udelay(1) should be enough.

Done and explained in another patch.

> 
> >  
> > -	return 0;
> >  }
> >  
> > +static const int ecc_bytes[] = {32, 46, 54, 60, 74, 88, 102, 110, 116};
> > +  
> 
> Putting ecc_bytes out of nand_max_ecc_strength() is not really related
> to this patch, and should probably go in patch 5.

Right, I will move this to the patch where it belongs.

> 
> >  static int nand_read_page(const struct nfc_config *conf, u32 offs,
> >  			  void *dest, int len)
> >  {
> > @@ -327,7 +346,6 @@ static int nand_read_page(const struct nfc_config *conf, u32 offs,
> >  
> >  static int nand_max_ecc_strength(struct nfc_config *conf)
> >  {
> > -	static const int ecc_bytes[] = { 32, 46, 54, 60, 74, 88, 102, 110, 116 };
> >  	int max_oobsize, max_ecc_bytes;
> >  	int nsectors = conf->page_size / conf->ecc_size;
> >  	int i;  
> 

Thanks,
Miquèl