Re: [PATCH v6 2/2] mtd: rawnand: nuvoton: add new driver for the Nuvoton MA35 SoC

Hui-Ping Chen
2024-10-09T00:55:19Z

Dear Miquel,

Thank you for your reply.

On 2024/10/8 下午 04:52, Miquel Raynal wrote:
> Hi Hui-Ping,
>
>>>> + return 0;
>>>> + }
>>>> +
>>>> + ma35_nand_dmac_init(nand);
>>>> +
>>>> + writel(mtd->oobsize, nand->regs + MA35_NFI_REG_NANDRACTL);
>>>> +
>>>> + /* setup and start DMA using dma_addr */
>>>> + dma_addr = dma_map_single(nand->dev, (void *)addr, len, DMA_FROM_DEVICE);
>>>> + ret = dma_mapping_error(nand->dev, dma_addr);
>>>> + if (ret) {
>>>> + dev_err(nand->dev, "dma mapping error\n");
>>>> + return -EINVAL;
>>>> + }
>>>> +
>>>> + writel((unsigned long)dma_addr, nand->regs + MA35_NFI_REG_DMASA);
>>> Please enforce a dma mask of 32 (even though it might be the fault).
>> I will change it to dma_addr & 0xffffffff.
> That's not what I mean, I believe you should use the dma API to ask for
> a mapping within the accessible 32-bit address range. The
> dma_mapping_error() check should return an error if that's not the
> case. Then you can safely write the value.

Here is my misunderstanding: just fill in the dma_addr directly,

no type conversion is needed. I have already tested it.

>>>> + writel(readl(nand->regs + MA35_NFI_REG_NANDCTL) | DMA_R_EN,
>>>> + nand->regs + MA35_NFI_REG_NANDCTL);
>>>> + ret = wait_for_completion_timeout(&nand->complete, msecs_to_jiffies(1000));
>>>> + if (!ret) {
>>>> + dev_err(nand->dev, "read timeout\n");
>>>> + ret = -ETIMEDOUT;
>>>> + }
>>>> +
>>>> + dma_unmap_single(nand->dev, dma_addr, len, DMA_FROM_DEVICE);
>>>> +
>>>> + reg = readl(nand->regs + MA35_NFI_REG_NANDINTSTS);
>>>> + if (reg & INT_ECC) {
>>>> + cnt = ma35_nfi_ecc_check(&nand->chip, addr);
>>>> + if (cnt < 0) {
>>>> + mtd->ecc_stats.failed++;
>>>> + writel(DMA_RST | DMA_EN, nand->regs + MA35_NFI_REG_DMACTL);
>>>> + writel(readl(nand->regs + MA35_NFI_REG_NANDCTL) | SWRST,
>>>> + nand->regs + MA35_NFI_REG_NANDCTL);
>>>> + } else {
>>>> + mtd->ecc_stats.corrected += cnt;
>>>> + nand->bitflips = cnt;
>>>> + }
>>>> + writel(INT_ECC, nand->regs + MA35_NFI_REG_NANDINTSTS);
>>>> + }
>>>> +
>>>> + return ret;
>>>> +}
>>>> +
>>>> +static int ma35_nand_write_page_hwecc(struct nand_chip *chip, const u8 *buf,
>>>> + int oob_required, int page)
>>>> +{
>>>> + struct mtd_info *mtd = nand_to_mtd(chip);
>>>> + void *ecc_calc = chip->ecc.calc_buf;
>>>> +
>>>> + ma35_clear_spare(chip, mtd->oobsize);
>>>> + ma35_write_spare(chip, mtd->oobsize - chip->ecc.total,
>>>> + (u32 *)chip->oob_poi);
>>>> +
>>>> + nand_prog_page_begin_op(chip, page, 0, buf, mtd->writesize);
>>>> + nand_prog_page_end_op(chip);
>>>> +
>>>> + /* Copy parity code in NANDRA to calc */
>>>> + ma35_read_spare(chip, chip->ecc.total, (u32 *)ecc_calc,
>>>> + mtd->oobsize - chip->ecc.total);
>>>> +
>>>> + /* Copy parity code in calc to oob_poi */
>>>> + memcpy(chip->oob_poi + (mtd->oobsize - chip->ecc.total),
>>>> + ecc_calc, chip->ecc.total);
>>>> +
>>>> + return 0;
>>>> +}
>>>> +
>>>> +static int ma35_nand_read_page_hwecc(struct nand_chip *chip, u8 *buf,
>>>> + int oob_required, int page)
>>>> +{
>>>> + struct ma35_nand_info *nand = nand_get_controller_data(chip);
>>>> + struct mtd_info *mtd = nand_to_mtd(chip);
>>>> + u32 reg;
>>>> +
>>>> + /* read the OOB area */
>>>> + nand_read_oob_op(chip, page, 0, chip->oob_poi, mtd->oobsize);
>>>> + nand->bitflips = 0;
>>> Why storing this value in your structure?
>> Because ecc.read_page needs to return bit flips, it is necessary to log them.
> I believe you don't need bitflips to be part of the ma35_nand_info
> structure.

I will remove this variable and then investigate where to obtain the
information from.

>>>> +
>>>> + /* copy OOB data to NANDRA for page read */
>>>> + ma35_write_spare(chip, mtd->oobsize, (u32 *)chip->oob_poi);
>>>> +
>>>> + reg = readl(nand->regs + MA35_NFI_REG_NANDRA0);
>>>> + if (reg & 0xffff0000) {
>>>> + memset((void *)buf, 0xff, mtd->writesize);
>>>> + } else {
>>>> + /* read data from nand */
>>>> + nand_read_page_op(chip, page, 0, buf, mtd->writesize);
>>> ret =
>>> if (ret)
>>> ...
>> I will add a return value here.
>>
>>
>>>> +
>>>> + /* restore OOB data from SMRA */
>>>> + ma35_read_spare(chip, mtd->oobsize, (u32 *)chip->oob_poi, 0);
>>> same
>> ma35_read_spare() is declared as void, with no return value.
>>
>> SMRA will be changed to NANDRA registers.
>>
>>
>>>> + }
>>>> +
>>>> + return nand->bitflips;
>>>> +}
>>>> +
>>>> +static int ma35_nand_read_oob_hwecc(struct nand_chip *chip, int page)
>>>> +{
>>>> + struct ma35_nand_info *nand = nand_get_controller_data(chip);
>>>> + struct mtd_info *mtd = nand_to_mtd(chip);
>>>> + u32 reg;
>>>> +
>>>> + nand_read_oob_op(chip, page, 0, chip->oob_poi, mtd->oobsize);
>>>> +
>>>> + /* copy OOB data to NANDRA for page read */
>>> What is NANDRA? does not mean anything to me.
>> NANDRA is the Redundant Area Word of the MA35 NAND controller.
> NANDRA is specific to your controller. "redundant area" is already more
> understandable.

I will change the comment to

/* copy oob data to controller redundant area for page read */

>> The controller reads ECC parity from this area for calculations or uses the information for check.
> This is what we generically call the "ECC bytes/area" I guess?

Yes

>> I will change NANDRA to NANDRA registers.
>>
>>
>>>> + ma35_write_spare(chip, mtd->oobsize, (u32 *)chip->oob_poi);
>>>> +
>>>> + reg = readl(nand->regs + MA35_NFI_REG_NANDRA0);
>>>> + if (reg & 0xffff0000)
>>>> + memset((void *)chip->oob_poi, 0xff, mtd->oobsize);
>>> What does this mean?
>> MA35 NAND controller checks oob bytes 2 and 3.
>>
>> If this page has been written, oob bytes 2 and 3 will be set to 0.
>>
>> Therefore, if bytes 2 and 3 are not 0, it indicates an empty page, and it will return 0xff.
> Ok, please define a macro for that, something along:
>
> #define PREFIX_RA_HAS_BEEN_WRITTEN(reg) FIELD_GET(GENMASK(..), (reg))
>
> if (!PREFIX_RA_HAS_BEEN_WRITTEN(reg))
> memset();

I will define a macro. Thank you.

>>
>>>> +
>>>> + return 0;
>>>> +}
>>>> +
>>>> +static irqreturn_t ma35_nand_irq(int irq, void *id)
>>>> +{
>>>> + struct ma35_nand_info *nand = (struct ma35_nand_info *)id;
>>>> + u32 isr;
>>>> +
>>>> + isr = readl(nand->regs + MA35_NFI_REG_NANDINTSTS);
>>>> + if (isr & INT_DMA) {
>>>> + writel(INT_DMA, nand->regs + MA35_NFI_REG_NANDINTSTS);
>>>> + complete(&nand->complete);
>>>> + }
>>> I guess a more future proof implementation would always writel(isr); to
>>> silence the interrupt. Otherwise of course you must call complete()
>>> only upon isr & INT_DMA.
>> This part can remove the if(), but writel(isr) is not feasible.
>>
>> The isr may contain the ECC flag, and clearing it could cause a miss in ECC handling.
>>
>> I will change it to:
>>
>>     writel(INT_DMA, nand->regs + MA35_NFI_REG_NANDINTSTS);
>>     complete(&nand->complete);
> In this case keep the if() but be sure to handle the timeout correctly.

OK.

>>
>>>> +
>>>> + return IRQ_HANDLED;
>>>> +}
>>>> +
>>>> +static int ma35_nand_attach_chip(struct nand_chip *chip)
>>>> +{
>>>> + struct ma35_nand_info *nand = nand_get_controller_data(chip);
>>>> + struct mtd_info *mtd = nand_to_mtd(chip);
>>>> + unsigned int reg;
>>>> +
>>>> + if (chip->options & NAND_BUSWIDTH_16) {
>>>> + dev_err(nand->dev, "16 bits bus width not supported");
>>>> + return -EINVAL;
>>>> + }
>>>> +
>>>> + /* support only ecc hw mode */
>>> Why ? Please don't.
>> I will add handling for other cases.
>>
>>
>>>> + if (chip->ecc.engine_type != NAND_ECC_ENGINE_TYPE_ON_HOST) {
>>>> + dev_err(nand->dev, "ecc.engine_type not supported\n");
>>>> + return -EINVAL;
>>>> + }
>>>> +
>>>> + nand->ecc_buf = devm_kzalloc(nand->dev, mtd->writesize + mtd->oobsize,
>>>> + GFP_KERNEL);
>>>> + if (!nand->ecc_buf)
>>>> + return -ENOMEM;
>>>> + chip->ecc.calc_buf = nand->ecc_buf;
>>> Are you sure you need this? I don't see the point.
>> This is unnecessary; I will remove it.
>>
>>
>>>> +
>>>> + /* Set PSize */
>>>> + reg = readl(nand->regs + MA35_NFI_REG_NANDCTL) & (~PSIZE_MASK);
>>>> + if (mtd->writesize == 2048)
>>>> + writel(reg | PSIZE_2K, nand->regs + MA35_NFI_REG_NANDCTL);
>>>> + else if (mtd->writesize == 4096)
>>>> + writel(reg | PSIZE_4K, nand->regs + MA35_NFI_REG_NANDCTL);
>>>> + else if (mtd->writesize == 8192)
>>>> + writel(reg | PSIZE_8K, nand->regs + MA35_NFI_REG_NANDCTL);
>>>> +
>>>> + chip->ecc.steps = mtd->writesize / chip->ecc.size;
>>>> + if (chip->ecc.strength == 0) {
>>>> + nand->bch = BCH_NONE; /* No ECC */
>>>> + chip->ecc.total = 0;
>>>> + } else if (chip->ecc.strength <= 8) {
>>>> + nand->bch = BCH_T8; /* T8 */
>>> bch is probably a bad name, and in general I don't see the point of
>>> saving this value. Just check the ECC strength in the above switch
>>> cases and don't use this intermediate variable.
>> I will change it to directly use ecc.strength for the check.
>>
>>
>>>> + chip->ecc.total = chip->ecc.steps * MA35_PARITY_BCH8;
>>>> + } else if (chip->ecc.strength <= 12) {
>>>> + nand->bch = BCH_T12; /* T12 */
>>>> + chip->ecc.total = chip->ecc.steps * MA35_PARITY_BCH12;
>>>> + } else if (chip->ecc.strength <= 24) {
>>>> + nand->bch = BCH_T24; /* T24 */
>>>> + chip->ecc.total = chip->ecc.steps * MA35_PARITY_BCH24;
>>>> + } else {
>>>> + dev_warn(nand->dev, "NAND Controller is not support this flash. (%d, %d)\n",
>>>> + mtd->writesize, mtd->oobsize);
>>> This must be a dev_err() and return an error immediately.
>>>
>>> Also the string is not correct.
>> I will change it to:
>>
>>         dev_err(nand->dev, "ECC strength error\n");
> unsupported

I will fix it.

>>         return -EINVAL;
>>
>>
> ...
>
>>>> +
>>>> +static int ma35_nfc_exec_op(struct nand_chip *chip,
>>>> + const struct nand_operation *op,
>>>> + bool check_only)
>>>> +{
>>>> + struct ma35_nand_info *nand = nand_get_controller_data(chip);
>>>> + int ret = 0;
>>>> + u32 i, reg;
>>>> +
>>>> + if (check_only)
>>>> + return 0;
>>>> +
>>>> + ma35_nand_target_enable(nand);
>>>> +
>>>> + reg = readl(nand->regs + MA35_NFI_REG_NANDINTSTS);
>>>> + reg |= INT_RB0;
>>> This RB pin looks like something hardcoded, whereas it should not :-)
>>>
>>> If you have several RB, it means you have several CS as well!
>> The MA35 NAND controller is indeed expected to support two sets of CS and RB.
>>
>> However, currently, the MA35 series only supports one set, so this name will be changed to RB.
> No, please use the correct names for RB and CS, but please read the
> value of the right RB and right CS from the DT (reg and nand-rb). These
> properties are standard and can be used to use eg CS1 and RB1. In this
> case the whole driver logic is the same, there is a single CS
> supported, but it becomes correct because you don't silently assume CS0
> and RB0 will always be used (you would be surprised by the number of
> designs not using the first CS/RB). It's just a bit of additional
> logic, nothing complex here.
>
Okay. I'll look into how to make the modifications.

> ...
>
>>>> +
>>>> + mtd = nand_to_mtd(chip);
>>>> + mtd->priv = chip;
>>>> + mtd->owner = THIS_MODULE;
>>>> + mtd->dev.parent = &pdev->dev;
>>>> +
>>>> + writel(NAND_EN, nand->regs + MA35_NFI_REG_GCTL);
>>> I would expect your reset bit to be set before this writel.
>> Are you referring to SWRST? I will call the reset engine before enabling it.
> Yes, thanks.

Thank you.

>
> Thanks,
> Miquèl

Best regards,

Hui-Ping Chen