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

[Arseniy Krasnov <avkrasnov@sberdevices.ru>]

On 12.04.2023 16:30, Liang Yang wrote:
> Hi,
> 
> On 2023/4/12 20:03, Arseniy Krasnov wrote:
>> [ EXTERNAL EMAIL ]
>>
>>
>>
>> On 12.04.2023 13:24, Arseniy Krasnov wrote:
>>>
>>>
>>> On 12.04.2023 12:37, Liang Yang wrote:
>>>> 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))
>>
>> Sorry, I can see this define as (and it is used in the driver):
>> #define NFC_CMD_RB_INT          BIT(14)
>>
>> in drivers/mtd/nand/raw/meson_nand.c
>>
>> Which one is correct ?
> 
> we need to modify the define 'NFC_CMD_RB_INT' as ((0xb << 10) | BIT(18)).
> 

Ok, NFC_CMD_RB_INT - it is "Ready/Busy_Interrupt" ? You suppose that currently it is
defined incorrectly, so irq waiting does not work?

Thanks, Arseniy

>>
>> Thanks, Arseniy
>>
>>>>
>>>>      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);
>>>>      ......
>>>>
>>>
>>>      Thanks for reply! I'll try this code! One more question about OOB processing in this
>>> driver (as You are author of it):
>>>
>>>     OOB size is 64 bytes, but for example if I have 1K ECC, 2 bytes user bytes and 14
>>>     bytes for ECC code for each 1K. In this case I have access to only 32 bytes of OOB:
>>>     2 x (2 user bytes + 14 ECC bytes). Correct me if i'm wrong, but rest of OOB (next
>>>     32 bytes) become unavailable (in both raw and ECC modes) ?
>>>
>>> Thanks, Arseniy
>>>
>>>>>    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);
>>>>
>>
> 

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