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

Liang Yang
2023-04-12T09:38:53Z

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