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

[Hui-Ping Chen]

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