Message-ID: <02098767-19ce-407e-88be-24c6259c4053@gmail.com>
Date: Wed, 9 Oct 2024 08:55:14 +0800
From: Hui-Ping Chen <hpchen0nvt@...il.com>
To: Miquel Raynal <miquel.raynal@...tlin.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

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

Powered by blists - more mailing lists