Message-ID: <20241008105230.7fd25438@xps-13>
Date: Tue, 8 Oct 2024 10:52:30 +0200
From: Miquel Raynal <miquel.raynal@...tlin.com>
To: Hui-Ping Chen <hpchen0nvt@...il.com>
Cc: richard@....at, vigneshr@...com, robh@...nel.org, krzk+dt@...nel.org,
 conor+dt@...nel.org, nikita.shubin@...uefel.me, arnd@...db.de,
 vkoul@...nel.org, esben@...nix.com, linux-arm-kernel@...ts.infradead.org,
 linux-mtd@...ts.infradead.org, devicetree@...r.kernel.org,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH v6 2/2] mtd: rawnand: nuvoton: add new driver for the
 Nuvoton MA35 SoC

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.

> >> +	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.

> >> +
> >> +	/* 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.

> 
> 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?

> 
> 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();

> 
> 
> >> +
> >> +	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.
> 
> 
> >> +
> >> +	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

>          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.


...

> >> +
> >> +	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.

Thanks,
Miquèl

Powered by blists - more mailing lists