Re: [PATCH v1 1/5] mtd: rawnand: meson: fix NAND access for read/write

[Liang Yang <liang.yang@amlogic.com>]

Hi Arseniy,

Thanks for pointing out this problem. also comment inline.

On 2023/4/12 14:16, Arseniy Krasnov wrote:
> [ EXTERNAL EMAIL ]
> 
> This fixes read/write functionality. New command sequences were ported
> from old vendor's driver. Without this patch driver works unstable. This
> change is tested with 'nanddump'/'nandwrite' utilities and mounting
> JFFS2 filesystem on AXG family (A113X SoC).
> 
> Fixes: 8fae856c5350 ("mtd: rawnand: meson: add support for Amlogic NAND flash controller")
> Signed-off-by: Arseniy Krasnov <AVKrasnov@sberdevices.ru>
> ---
>   drivers/mtd/nand/raw/meson_nand.c | 116 ++++++++++++++++++++++++++----
>   1 file changed, 101 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/mtd/nand/raw/meson_nand.c b/drivers/mtd/nand/raw/meson_nand.c
> index 074e14225c06..256c37c76526 100644
> --- a/drivers/mtd/nand/raw/meson_nand.c
> +++ b/drivers/mtd/nand/raw/meson_nand.c
> @@ -26,6 +26,7 @@
>   #define NFC_CMD_IDLE		(0xc << 14)
>   #define NFC_CMD_CLE		(0x5 << 14)
>   #define NFC_CMD_ALE		(0x6 << 14)
> +#define NFC_CMD_DRD		(0x8 << 14)
>   #define NFC_CMD_ADL		((0 << 16) | (3 << 20))
>   #define NFC_CMD_ADH		((1 << 16) | (3 << 20))
>   #define NFC_CMD_AIL		((2 << 16) | (3 << 20))
> @@ -84,6 +85,7 @@
>   
>   #define DMA_BUSY_TIMEOUT	0x100000
>   #define CMD_FIFO_EMPTY_TIMEOUT	1000
> +#define DEVICE_READY_TIMEOUT	1000
>   
>   #define MAX_CE_NUM		2
>   
> @@ -255,8 +257,26 @@ static void meson_nfc_select_chip(struct nand_chip *nand, int chip)
>   	}
>   }
>   
> +static int meson_nfc_wait_cmd_finish(struct meson_nfc *nfc,
> +				     unsigned int timeout_ms)
> +{
> +	u32 cmd_size = 0;
> +	int ret;
> +
> +	/* wait cmd fifo is empty */
> +	ret = readl_relaxed_poll_timeout(nfc->reg_base + NFC_REG_CMD, cmd_size,
> +					 !NFC_CMD_GET_SIZE(cmd_size),
> +					 10, timeout_ms * 1000);
> +	if (ret)
> +		dev_err(nfc->dev, "wait for empty CMD FIFO timed out\n");
> +
> +	return ret;
> +}
> +
>   static void meson_nfc_cmd_idle(struct meson_nfc *nfc, u32 time)
>   {
> +	meson_nfc_wait_cmd_finish(nfc, 0);
> +
>   	writel(nfc->param.chip_select | NFC_CMD_IDLE | (time & 0x3ff),
>   	       nfc->reg_base + NFC_REG_CMD);
>   }
> @@ -308,23 +328,9 @@ static void meson_nfc_drain_cmd(struct meson_nfc *nfc)
>   	 */
>   	meson_nfc_cmd_idle(nfc, 0);
>   	meson_nfc_cmd_idle(nfc, 0);
> +	meson_nfc_wait_cmd_finish(nfc, 1000);
>   }
>   
> -static int meson_nfc_wait_cmd_finish(struct meson_nfc *nfc,
> -				     unsigned int timeout_ms)
> -{
> -	u32 cmd_size = 0;
> -	int ret;
> -
> -	/* wait cmd fifo is empty */
> -	ret = readl_relaxed_poll_timeout(nfc->reg_base + NFC_REG_CMD, cmd_size,
> -					 !NFC_CMD_GET_SIZE(cmd_size),
> -					 10, timeout_ms * 1000);
> -	if (ret)
> -		dev_err(nfc->dev, "wait for empty CMD FIFO time out\n");
> -
> -	return ret;
> -}
>   
>   static int meson_nfc_wait_dma_finish(struct meson_nfc *nfc)
>   {
> @@ -631,6 +637,48 @@ static int meson_nfc_rw_cmd_prepare_and_execute(struct nand_chip *nand,
>   	return 0;
>   }
>   
> +static uint8_t meson_nfc_read_byte(struct nand_chip *nand)
> +{
> +	struct meson_nfc *nfc = nand_get_controller_data(nand);
> +
> +	writel(NFC_CMD_DRD, nfc->reg_base + NFC_REG_CMD);
> +	meson_nfc_cmd_idle(nfc, nfc->timing.twb);
> +	meson_nfc_drain_cmd(nfc);
> +
> +	return readl(nfc->reg_base + NFC_REG_BUF);
> +}
> +
> +static int meson_nfc_wait_dev_ready(struct nand_chip *nand)
> +{
> +	struct meson_nfc *nfc = nand_get_controller_data(nand);
> +	u32 cs = nfc->param.chip_select;
> +	unsigned long cnt = 0;
> +
> +	meson_nfc_drain_cmd(nfc);
> +
> +	writel(cs | NFC_CMD_CLE | NAND_CMD_STATUS, nfc->reg_base + NFC_REG_CMD);
> +
> +	/* 10 ms. */
> +	while (cnt < DEVICE_READY_TIMEOUT) {
> +		uint8_t status;
> +
> +		status = meson_nfc_read_byte(nand);
> +
> +		if (status & NAND_STATUS_READY)
> +			break;
> +
> +		usleep_range(10, 11);
> +		cnt++;
> +	}
> +
> +	if (cnt == DEVICE_READY_TIMEOUT) {
> +		dev_err(nfc->dev, "device ready timeout\n");
> +		return -ETIMEDOUT;
> +	}
> +
> +	return 0;
> +}
> +
>   static int meson_nfc_write_page_sub(struct nand_chip *nand,
>   				    int page, int raw)
>   {
> @@ -643,6 +691,10 @@ static int meson_nfc_write_page_sub(struct nand_chip *nand,
>   	u32 cmd;
>   	int ret;
>   
> +	ret = meson_nfc_wait_dev_ready(nand);
> +	if (ret)
> +		return ret;
> +
>   	meson_nfc_select_chip(nand, nand->cur_cs);
>   
>   	data_len =  mtd->writesize + mtd->oobsize;
> @@ -667,12 +719,20 @@ static int meson_nfc_write_page_sub(struct nand_chip *nand,
>   				     NFC_CMD_SCRAMBLER_DISABLE);
>   	}
>   
> +	ret = meson_nfc_wait_dma_finish(nfc);
> +	if (ret)
> +		return ret;
> +
>   	cmd = nfc->param.chip_select | NFC_CMD_CLE | NAND_CMD_PAGEPROG;
>   	writel(cmd, nfc->reg_base + NFC_REG_CMD);
>   	meson_nfc_queue_rb(nfc, PSEC_TO_MSEC(sdr->tPROG_max));
>   
>   	meson_nfc_dma_buffer_release(nand, data_len, info_len, DMA_TO_DEVICE);
>   
> +	ret = meson_nfc_wait_dev_ready(nand);
> +	if (ret)
> +		return ret;
> +
>   	return ret;
>   }
>   
> @@ -720,6 +780,21 @@ static void meson_nfc_check_ecc_pages_valid(struct meson_nfc *nfc,
>   	} while (!ret);
>   }
>   
> +static inline int meson_nfc_send_read(struct nand_chip *nand)
> +{
> +	struct meson_nfc *nfc = nand_get_controller_data(nand);
> +	u32 cs = nfc->param.chip_select;
> +	int ret;
> +
> +	ret = meson_nfc_wait_dev_ready(nand);
> +	if (ret)
> +		return ret;
> +
> +	writel(cs | NFC_CMD_CLE | NAND_CMD_READ0, nfc->reg_base + NFC_REG_CMD);
> +
> +	return 0;
> +}
> +

it already calls meson_nfc_queue_rb() in 
meson_nfc_rw_cmd_prepare_and_execute(). Could you implements this in 
meson_nfc_queue_rb()? and we can use the irq method.
also without Ready/Busy pin, the meson_nfc_queue_rb() should change like 
below:
	......
	#define NFC_CMD_RB_INT	((0xb << 10) | BIT(18))

	meson_nfc_cmd_idle(nfc, 0);
	cmd = nfc->param.chip_select | NFC_CMD_CLE | NAND_CMD_STATUS;
	writel(cmd, nfc->reg_base + NFC_REG_CMD);
	meson_nfc_cmd_idle(nfc, 5);
	cmd = NFC_CMD_RB | NFC_CMD_RB_INT | nfc->timing.tbers_max;
	writel(cmd, nfc->reg_base + NFC_REG_CMD);

	ret = wait_for_completion_timeout(&nfc->completion,
					  msecs_to_jiffies(timeout_ms));
	if (ret == 0)
		ret = -1;

	writel(cs | NFC_CMD_CLE | NAND_CMD_READ0, nfc->reg_base + NFC_REG_CMD);
	......
	
>   static int meson_nfc_read_page_sub(struct nand_chip *nand,
>   				   int page, int raw)
>   {
> @@ -734,10 +809,18 @@ static int meson_nfc_read_page_sub(struct nand_chip *nand,
>   	data_len =  mtd->writesize + mtd->oobsize;
>   	info_len = nand->ecc.steps * PER_INFO_BYTE;
>   
> +	ret = meson_nfc_wait_dev_ready(nand);
> +	if (ret)
> +		return ret;
> +
>   	ret = meson_nfc_rw_cmd_prepare_and_execute(nand, page, DIRREAD);
>   	if (ret)
>   		return ret;
>   
> +	ret = meson_nfc_send_read(nand);
> +	if (ret)
> +		return ret;
> +
>   	ret = meson_nfc_dma_buffer_setup(nand, meson_chip->data_buf,
>   					 data_len, meson_chip->info_buf,
>   					 info_len, DMA_FROM_DEVICE);
> @@ -754,6 +837,9 @@ static int meson_nfc_read_page_sub(struct nand_chip *nand,
>   	}
>   
>   	ret = meson_nfc_wait_dma_finish(nfc);
> +	if (ret)
> +		return ret;
> +
>   	meson_nfc_check_ecc_pages_valid(nfc, nand, raw);
>   
>   	meson_nfc_dma_buffer_release(nand, data_len, info_len, DMA_FROM_DEVICE);

-- 
Thanks,
Liang

_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic