Re: [PATCH 1/3] mtd: nand: fsmc: use ->exec_op()

[Boris Brezillon <boris.brezillon@free-electrons.com>]

Hi Miquel,

On Tue, 14 Nov 2017 16:46:20 +0100
Miquel Raynal <miquel.raynal@free-electrons.com> wrote:

> Remove the decrecated ->cmd_ctrl() implementation to use ->exec_op() in
> the fsmc_nand driver.
> 
> Implement the ->select_chip() hook to avoid having to support the hack
> from the core that send a NAND_CMD_NONE with NAND_NCE to signal a
> deassertion of nCE.
> 
> Also remove the use of IO_ADDR_[R|W] in this driver and use a pointer to
> the control registers to avoid doing several arithmetic operations
> (including a multiplication) each time a control register is read or
> written.

Can you split that in 2 patches (one that gets rid of IO_ADDR_[R|W] and
a second one switching from ->cmd_ctrl() to ->exec_op()
+ ->select_chip())?

> 
> Signed-off-by: Miquel Raynal <miquel.raynal@free-electrons.com>
> ---
>  drivers/mtd/nand/fsmc_nand.c | 234 ++++++++++++++++++++++++-------------------
>  1 file changed, 132 insertions(+), 102 deletions(-)
> 
> diff --git a/drivers/mtd/nand/fsmc_nand.c b/drivers/mtd/nand/fsmc_nand.c
> index b44e5c6545e0..76246d904c28 100644
> --- a/drivers/mtd/nand/fsmc_nand.c
> +++ b/drivers/mtd/nand/fsmc_nand.c
> @@ -103,10 +103,6 @@
>  #define ECC3			0x1C
>  #define FSMC_NAND_BANK_SZ	0x20
>  
> -#define FSMC_NAND_REG(base, bank, reg)		(base + FSMC_NOR_REG_SIZE + \
> -						(FSMC_NAND_BANK_SZ * (bank)) + \
> -						reg)
> -
>  #define FSMC_BUSY_WAIT_TIMEOUT	(1 * HZ)
>  
>  struct fsmc_nand_timings {
> @@ -144,6 +140,7 @@ enum access_mode {
>   * @cmd_va:		NAND port for Command.
>   * @addr_va:		NAND port for Address.
>   * @regs_va:		FSMC regs base address.
> + * @regs:		Registers base address for a given bank.
>   */
>  struct fsmc_nand_data {
>  	u32			pid;
> @@ -166,6 +163,7 @@ struct fsmc_nand_data {
>  	void __iomem		*cmd_va;
>  	void __iomem		*addr_va;
>  	void __iomem		*regs_va;
> +	void __iomem		*regs;

I'm almost sure you don't need an extra field here. Just re-use reg_va
and make it point to the appropriate bank registers instead of FSMC base.

>  };
>  
>  static int fsmc_ecc1_ooblayout_ecc(struct mtd_info *mtd, int section,
> @@ -258,45 +256,6 @@ static inline struct fsmc_nand_data *mtd_to_fsmc(struct mtd_info *mtd)
>  }
>  
>  /*
> - * fsmc_cmd_ctrl - For facilitaing Hardware access
> - * This routine allows hardware specific access to control-lines(ALE,CLE)
> - */
> -static void fsmc_cmd_ctrl(struct mtd_info *mtd, int cmd, unsigned int ctrl)
> -{
> -	struct nand_chip *this = mtd_to_nand(mtd);
> -	struct fsmc_nand_data *host = mtd_to_fsmc(mtd);
> -	void __iomem *regs = host->regs_va;
> -	unsigned int bank = host->bank;
> -
> -	if (ctrl & NAND_CTRL_CHANGE) {
> -		u32 pc;
> -
> -		if (ctrl & NAND_CLE) {
> -			this->IO_ADDR_R = host->cmd_va;
> -			this->IO_ADDR_W = host->cmd_va;
> -		} else if (ctrl & NAND_ALE) {
> -			this->IO_ADDR_R = host->addr_va;
> -			this->IO_ADDR_W = host->addr_va;
> -		} else {
> -			this->IO_ADDR_R = host->data_va;
> -			this->IO_ADDR_W = host->data_va;
> -		}
> -
> -		pc = readl(FSMC_NAND_REG(regs, bank, PC));
> -		if (ctrl & NAND_NCE)
> -			pc |= FSMC_ENABLE;
> -		else
> -			pc &= ~FSMC_ENABLE;
> -		writel_relaxed(pc, FSMC_NAND_REG(regs, bank, PC));
> -	}
> -
> -	mb();
> -
> -	if (cmd != NAND_CMD_NONE)
> -		writeb_relaxed(cmd, this->IO_ADDR_W);
> -}
> -
> -/*
>   * fsmc_nand_setup - FSMC (Flexible Static Memory Controller) init routine
>   *
>   * This routine initializes timing parameters related to NAND memory access in
> @@ -307,8 +266,6 @@ static void fsmc_nand_setup(struct fsmc_nand_data *host,
>  {
>  	uint32_t value = FSMC_DEVTYPE_NAND | FSMC_ENABLE | FSMC_WAITON;
>  	uint32_t tclr, tar, thiz, thold, twait, tset;
> -	unsigned int bank = host->bank;
> -	void __iomem *regs = host->regs_va;
>  
>  	tclr = (tims->tclr & FSMC_TCLR_MASK) << FSMC_TCLR_SHIFT;
>  	tar = (tims->tar & FSMC_TAR_MASK) << FSMC_TAR_SHIFT;
> @@ -318,18 +275,13 @@ static void fsmc_nand_setup(struct fsmc_nand_data *host,
>  	tset = (tims->tset & FSMC_TSET_MASK) << FSMC_TSET_SHIFT;
>  
>  	if (host->nand.options & NAND_BUSWIDTH_16)
> -		writel_relaxed(value | FSMC_DEVWID_16,
> -				FSMC_NAND_REG(regs, bank, PC));
> +		writel_relaxed(value | FSMC_DEVWID_16, host->regs + PC);
>  	else
> -		writel_relaxed(value | FSMC_DEVWID_8,
> -				FSMC_NAND_REG(regs, bank, PC));
> -
> -	writel_relaxed(readl(FSMC_NAND_REG(regs, bank, PC)) | tclr | tar,
> -			FSMC_NAND_REG(regs, bank, PC));
> -	writel_relaxed(thiz | thold | twait | tset,
> -			FSMC_NAND_REG(regs, bank, COMM));
> -	writel_relaxed(thiz | thold | twait | tset,
> -			FSMC_NAND_REG(regs, bank, ATTRIB));
> +		writel_relaxed(value | FSMC_DEVWID_8, host->regs + PC);
> +
> +	writel_relaxed(readl(host->regs + PC) | tclr | tar, host->regs + PC);
> +	writel_relaxed(thiz | thold | twait | tset, host->regs + COMM);
> +	writel_relaxed(thiz | thold | twait | tset, host->regs + ATTRIB);
>  }
>  
>  static int fsmc_calc_timings(struct fsmc_nand_data *host,
> @@ -419,15 +371,11 @@ static int fsmc_setup_data_interface(struct mtd_info *mtd, int csline,
>  static void fsmc_enable_hwecc(struct mtd_info *mtd, int mode)
>  {
>  	struct fsmc_nand_data *host = mtd_to_fsmc(mtd);
> -	void __iomem *regs = host->regs_va;
> -	uint32_t bank = host->bank;
> -
> -	writel_relaxed(readl(FSMC_NAND_REG(regs, bank, PC)) & ~FSMC_ECCPLEN_256,
> -			FSMC_NAND_REG(regs, bank, PC));
> -	writel_relaxed(readl(FSMC_NAND_REG(regs, bank, PC)) & ~FSMC_ECCEN,
> -			FSMC_NAND_REG(regs, bank, PC));
> -	writel_relaxed(readl(FSMC_NAND_REG(regs, bank, PC)) | FSMC_ECCEN,
> -			FSMC_NAND_REG(regs, bank, PC));
> +
> +	writel_relaxed(readl(host->regs + PC) & ~FSMC_ECCPLEN_256,
> +		       host->regs + PC);
> +	writel_relaxed(readl(host->regs + PC) & ~FSMC_ECCEN, host->regs + PC);
> +	writel_relaxed(readl(host->regs + PC) | FSMC_ECCEN, host->regs + PC);
>  }
>  
>  /*
> @@ -439,13 +387,11 @@ static int fsmc_read_hwecc_ecc4(struct mtd_info *mtd, const uint8_t *data,
>  				uint8_t *ecc)
>  {
>  	struct fsmc_nand_data *host = mtd_to_fsmc(mtd);
> -	void __iomem *regs = host->regs_va;
> -	uint32_t bank = host->bank;
>  	uint32_t ecc_tmp;
>  	unsigned long deadline = jiffies + FSMC_BUSY_WAIT_TIMEOUT;
>  
>  	do {
> -		if (readl_relaxed(FSMC_NAND_REG(regs, bank, STS)) & FSMC_CODE_RDY)
> +		if (readl_relaxed(host->regs + STS) & FSMC_CODE_RDY)
>  			break;
>  		else
>  			cond_resched();
> @@ -456,25 +402,25 @@ static int fsmc_read_hwecc_ecc4(struct mtd_info *mtd, const uint8_t *data,
>  		return -ETIMEDOUT;
>  	}
>  
> -	ecc_tmp = readl_relaxed(FSMC_NAND_REG(regs, bank, ECC1));
> +	ecc_tmp = readl_relaxed(host->regs + ECC1);
>  	ecc[0] = (uint8_t) (ecc_tmp >> 0);
>  	ecc[1] = (uint8_t) (ecc_tmp >> 8);
>  	ecc[2] = (uint8_t) (ecc_tmp >> 16);
>  	ecc[3] = (uint8_t) (ecc_tmp >> 24);
>  
> -	ecc_tmp = readl_relaxed(FSMC_NAND_REG(regs, bank, ECC2));
> +	ecc_tmp = readl_relaxed(host->regs + ECC2);
>  	ecc[4] = (uint8_t) (ecc_tmp >> 0);
>  	ecc[5] = (uint8_t) (ecc_tmp >> 8);
>  	ecc[6] = (uint8_t) (ecc_tmp >> 16);
>  	ecc[7] = (uint8_t) (ecc_tmp >> 24);
>  
> -	ecc_tmp = readl_relaxed(FSMC_NAND_REG(regs, bank, ECC3));
> +	ecc_tmp = readl_relaxed(host->regs + ECC3);
>  	ecc[8] = (uint8_t) (ecc_tmp >> 0);
>  	ecc[9] = (uint8_t) (ecc_tmp >> 8);
>  	ecc[10] = (uint8_t) (ecc_tmp >> 16);
>  	ecc[11] = (uint8_t) (ecc_tmp >> 24);
>  
> -	ecc_tmp = readl_relaxed(FSMC_NAND_REG(regs, bank, STS));
> +	ecc_tmp = readl_relaxed(host->regs + STS);
>  	ecc[12] = (uint8_t) (ecc_tmp >> 16);
>  
>  	return 0;
> @@ -489,11 +435,9 @@ static int fsmc_read_hwecc_ecc1(struct mtd_info *mtd, const uint8_t *data,
>  				uint8_t *ecc)
>  {
>  	struct fsmc_nand_data *host = mtd_to_fsmc(mtd);
> -	void __iomem *regs = host->regs_va;
> -	uint32_t bank = host->bank;
>  	uint32_t ecc_tmp;
>  
> -	ecc_tmp = readl_relaxed(FSMC_NAND_REG(regs, bank, ECC1));
> +	ecc_tmp = readl_relaxed(host->regs + ECC1);
>  	ecc[0] = (uint8_t) (ecc_tmp >> 0);
>  	ecc[1] = (uint8_t) (ecc_tmp >> 8);
>  	ecc[2] = (uint8_t) (ecc_tmp >> 16);
> @@ -598,18 +542,18 @@ static int dma_xfer(struct fsmc_nand_data *host, void *buffer, int len,
>   */
>  static void fsmc_write_buf(struct mtd_info *mtd, const uint8_t *buf, int len)
>  {
> +	struct fsmc_nand_data *host  = mtd_to_fsmc(mtd);
>  	int i;
> -	struct nand_chip *chip = mtd_to_nand(mtd);
>  
>  	if (IS_ALIGNED((uint32_t)buf, sizeof(uint32_t)) &&
>  			IS_ALIGNED(len, sizeof(uint32_t))) {
>  		uint32_t *p = (uint32_t *)buf;
>  		len = len >> 2;
>  		for (i = 0; i < len; i++)
> -			writel_relaxed(p[i], chip->IO_ADDR_W);
> +			writel_relaxed(p[i], host->data_va);
>  	} else {
>  		for (i = 0; i < len; i++)
> -			writeb_relaxed(buf[i], chip->IO_ADDR_W);
> +			writeb_relaxed(buf[i], host->data_va);
>  	}
>  }
>  
> @@ -621,18 +565,18 @@ static void fsmc_write_buf(struct mtd_info *mtd, const uint8_t *buf, int len)
>   */
>  static void fsmc_read_buf(struct mtd_info *mtd, uint8_t *buf, int len)
>  {
> +	struct fsmc_nand_data *host  = mtd_to_fsmc(mtd);
>  	int i;
> -	struct nand_chip *chip = mtd_to_nand(mtd);
>  
>  	if (IS_ALIGNED((uint32_t)buf, sizeof(uint32_t)) &&
>  			IS_ALIGNED(len, sizeof(uint32_t))) {
>  		uint32_t *p = (uint32_t *)buf;
>  		len = len >> 2;
>  		for (i = 0; i < len; i++)
> -			p[i] = readl_relaxed(chip->IO_ADDR_R);
> +			p[i] = readl_relaxed(host->data_va);
>  	} else {
>  		for (i = 0; i < len; i++)
> -			buf[i] = readb_relaxed(chip->IO_ADDR_R);
> +			buf[i] = readb_relaxed(host->data_va);
>  	}
>  }
>  
> @@ -663,6 +607,102 @@ static void fsmc_write_buf_dma(struct mtd_info *mtd, const uint8_t *buf,
>  	dma_xfer(host, (void *)buf, len, DMA_TO_DEVICE);
>  }
>  
> +/* fsmc_select_chip - assert or deassert nCE */
> +static void fsmc_select_chip(struct mtd_info *mtd, int chipnr)
> +{
> +	struct fsmc_nand_data *host = mtd_to_fsmc(mtd);
> +	u32 pc;
> +
> +	/* Support only one CS */
> +	if (chipnr > 0)
> +		return;
> +
> +	pc = readl(host->regs + PC);
> +	if (chipnr < 0)
> +		writel_relaxed(pc & ~FSMC_ENABLE, host->regs + PC);
> +	else
> +		writel_relaxed(pc | FSMC_ENABLE, host->regs + PC);
> +
> +	/* nCE line must be asserted before starting any operation */
> +	mb();
> +}
> +
> +/*
> + * fsmc_exec_op - hook called by the core to execute NAND operations
> + *
> + * This controller is simple enough and thus does not need to use the parser
> + * provided by the core, instead, handle every situation here.
> + */
> +static int fsmc_exec_op(struct nand_chip *chip, const struct nand_operation *op,
> +			bool check_only)
> +{
> +	struct mtd_info *mtd = nand_to_mtd(chip);
> +	struct fsmc_nand_data *host = mtd_to_fsmc(mtd);
> +	const struct nand_op_instr *instr = NULL;
> +	int ret = 0;
> +	unsigned int op_id;
> +	int i;
> +
> +	pr_debug("Executing operation [%d instructions]:\n", op->ninstrs);
> +	for (op_id = 0; op_id < op->ninstrs; op_id++) {
> +		instr = &op->instrs[op_id];
> +
> +		switch (instr->type) {
> +		case NAND_OP_CMD_INSTR:
> +			pr_debug("  ->CMD      [0x%02x]\n",
> +				 instr->ctx.cmd.opcode);
> +
> +			writeb_relaxed(instr->ctx.cmd.opcode, host->cmd_va);
> +			break;
> +
> +		case NAND_OP_ADDR_INSTR:
> +			pr_debug("  ->ADDR     [%d cyc]",
> +				 instr->ctx.addr.naddrs);
> +
> +			for (i = 0; i < instr->ctx.addr.naddrs; i++)
> +				writeb_relaxed(instr->ctx.addr.addrs[i],
> +					       host->addr_va);
> +			break;
> +
> +		case NAND_OP_DATA_IN_INSTR:
> +			pr_debug("  ->DATA_IN  [%d B%s]\n", instr->ctx.data.len,
> +				 instr->ctx.data.force_8bit ?
> +				 ", force 8-bit" : "");
> +
> +			if (host->mode == USE_DMA_ACCESS)
> +				fsmc_read_buf_dma(mtd, instr->ctx.data.buf.in,
> +						  instr->ctx.data.len);
> +			else
> +				fsmc_read_buf(mtd, instr->ctx.data.buf.in,
> +					      instr->ctx.data.len);
> +			break;
> +
> +		case NAND_OP_DATA_OUT_INSTR:
> +			pr_debug("  ->DATA_OUT [%d B%s]\n", instr->ctx.data.len,
> +				 instr->ctx.data.force_8bit ?
> +				 ", force 8-bit" : "");
> +
> +			if (host->mode == USE_DMA_ACCESS)
> +				fsmc_write_buf_dma(mtd, instr->ctx.data.buf.out,
> +						   instr->ctx.data.len);
> +			else
> +				fsmc_write_buf(mtd, instr->ctx.data.buf.out,
> +					       instr->ctx.data.len);
> +			break;
> +
> +		case NAND_OP_WAITRDY_INSTR:
> +			pr_debug("  ->WAITRDY  [max %d ms]\n",
> +				 instr->ctx.waitrdy.timeout_ms);
> +
> +			ret = nand_soft_waitrdy(chip,
> +						instr->ctx.waitrdy.timeout_ms);

I guess nand_soft_waitrdy() will be part of the next version of your
->exec_op() series, right?

> +			break;
> +		}
> +	}
> +
> +	return ret;
> +}
> +
>  /*
>   * fsmc_read_page_hwecc
>   * @mtd:	mtd info structure
> @@ -754,13 +794,11 @@ static int fsmc_bch8_correct_data(struct mtd_info *mtd, uint8_t *dat,
>  {
>  	struct nand_chip *chip = mtd_to_nand(mtd);
>  	struct fsmc_nand_data *host = mtd_to_fsmc(mtd);
> -	void __iomem *regs = host->regs_va;
> -	unsigned int bank = host->bank;
>  	uint32_t err_idx[8];
>  	uint32_t num_err, i;
>  	uint32_t ecc1, ecc2, ecc3, ecc4;
>  
> -	num_err = (readl_relaxed(FSMC_NAND_REG(regs, bank, STS)) >> 10) & 0xF;
> +	num_err = (readl_relaxed(host->regs + STS) >> 10) & 0xF;
>  
>  	/* no bit flipping */
>  	if (likely(num_err == 0))
> @@ -803,10 +841,10 @@ static int fsmc_bch8_correct_data(struct mtd_info *mtd, uint8_t *dat,
>  	 * uint64_t array and error offset indexes are populated in err_idx
>  	 * array
>  	 */
> -	ecc1 = readl_relaxed(FSMC_NAND_REG(regs, bank, ECC1));
> -	ecc2 = readl_relaxed(FSMC_NAND_REG(regs, bank, ECC2));
> -	ecc3 = readl_relaxed(FSMC_NAND_REG(regs, bank, ECC3));
> -	ecc4 = readl_relaxed(FSMC_NAND_REG(regs, bank, STS));
> +	ecc1 = readl_relaxed(host->regs + ECC1);
> +	ecc2 = readl_relaxed(host->regs + ECC2);
> +	ecc3 = readl_relaxed(host->regs + ECC3);
> +	ecc4 = readl_relaxed(host->regs + STS);
>  
>  	err_idx[0] = (ecc1 >> 0) & 0x1FFF;
>  	err_idx[1] = (ecc1 >> 13) & 0x1FFF;
> @@ -933,6 +971,9 @@ static int __init fsmc_nand_probe(struct platform_device *pdev)
>  		return PTR_ERR(host->clk);
>  	}
>  
> +	host->regs = host->regs_va + FSMC_NOR_REG_SIZE +
> +		     (host->bank * FSMC_NAND_BANK_SZ);
> +
>  	ret = clk_prepare_enable(host->clk);
>  	if (ret)
>  		return ret;
> @@ -960,9 +1001,8 @@ static int __init fsmc_nand_probe(struct platform_device *pdev)
>  	nand_set_flash_node(nand, pdev->dev.of_node);
>  
>  	mtd->dev.parent = &pdev->dev;
> -	nand->IO_ADDR_R = host->data_va;
> -	nand->IO_ADDR_W = host->data_va;
> -	nand->cmd_ctrl = fsmc_cmd_ctrl;
> +	nand->exec_op = fsmc_exec_op;
> +	nand->select_chip = fsmc_select_chip;
>  	nand->chip_delay = 30;
>  
>  	/*
> @@ -974,8 +1014,7 @@ static int __init fsmc_nand_probe(struct platform_device *pdev)
>  	nand->ecc.size = 512;
>  	nand->badblockbits = 7;
>  
> -	switch (host->mode) {
> -	case USE_DMA_ACCESS:
> +	if (host->mode == USE_DMA_ACCESS) {
>  		dma_cap_zero(mask);
>  		dma_cap_set(DMA_MEMCPY, mask);
>  		host->read_dma_chan = dma_request_channel(mask, filter, NULL);
> @@ -988,15 +1027,6 @@ static int __init fsmc_nand_probe(struct platform_device *pdev)
>  			dev_err(&pdev->dev, "Unable to get write dma channel\n");
>  			goto err_req_write_chnl;
>  		}
> -		nand->read_buf = fsmc_read_buf_dma;
> -		nand->write_buf = fsmc_write_buf_dma;
> -		break;
> -
> -	default:
> -	case USE_WORD_ACCESS:
> -		nand->read_buf = fsmc_read_buf;
> -		nand->write_buf = fsmc_write_buf;
> -		break;
>  	}
>  
>  	if (host->dev_timings)