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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Hui-Ping,

hpchen0nvt@xxxxxxxxx wrote on Wed, 21 Aug 2024 07:11:32 +0000:

> Nuvoton MA35 SoCs NAND Flash Interface Controller
> supports 2KB, 4KB and 8KB page size, and up to 8-bit,

Suffix is: kiB

> 12-bit, and 24-bit hardware ECC calculation circuit
> to protect data communication.

It's not the communication, it's the data itself.

> 
> Signed-off-by: Hui-Ping Chen <hpchen0nvt@xxxxxxxxx>
> ---
>  drivers/mtd/nand/raw/Kconfig               |    8 +
>  drivers/mtd/nand/raw/Makefile              |    1 +
>  drivers/mtd/nand/raw/nuvoton_ma35d1_nand.c | 1068 ++++++++++++++++++++
>  3 files changed, 1077 insertions(+)
>  create mode 100644 drivers/mtd/nand/raw/nuvoton_ma35d1_nand.c
> 
> diff --git a/drivers/mtd/nand/raw/Kconfig b/drivers/mtd/nand/raw/Kconfig
> index 614257308516..932bf2215470 100644
> --- a/drivers/mtd/nand/raw/Kconfig
> +++ b/drivers/mtd/nand/raw/Kconfig
> @@ -448,6 +448,14 @@ config MTD_NAND_RENESAS
>  	  Enables support for the NAND controller found on Renesas R-Car
>  	  Gen3 and RZ/N1 SoC families.
>  
> +config MTD_NAND_NVT_MA35

Is NVT so common or is it just one opportunity to save 4 chars in a
Kconfig file?? I'd prefer something more easy to understand.

> +	tristate "Nuvoton MA35 SoC NAND controller"
> +	depends on ARCH_MA35 || COMPILE_TEST
> +	depends on OF
> +	help
> +	  Enables support for the NAND controller found on
> +	  the Nuvoton MA35 series SoCs.
> +
>  comment "Misc"
>  
>  config MTD_SM_COMMON
> diff --git a/drivers/mtd/nand/raw/Makefile b/drivers/mtd/nand/raw/Makefile
> index 25120a4afada..cdfdfee3f5f3 100644
> --- a/drivers/mtd/nand/raw/Makefile
> +++ b/drivers/mtd/nand/raw/Makefile
> @@ -57,6 +57,7 @@ obj-$(CONFIG_MTD_NAND_INTEL_LGM)	+= intel-nand-controller.o
>  obj-$(CONFIG_MTD_NAND_ROCKCHIP)		+= rockchip-nand-controller.o
>  obj-$(CONFIG_MTD_NAND_PL35X)		+= pl35x-nand-controller.o
>  obj-$(CONFIG_MTD_NAND_RENESAS)		+= renesas-nand-controller.o
> +obj-$(CONFIG_MTD_NAND_NVT_MA35)	+= nuvoton_ma35d1_nand.o
>  
>  nand-objs := nand_base.o nand_legacy.o nand_bbt.o nand_timings.o nand_ids.o
>  nand-objs += nand_onfi.o
> diff --git a/drivers/mtd/nand/raw/nuvoton_ma35d1_nand.c b/drivers/mtd/nand/raw/nuvoton_ma35d1_nand.c
> new file mode 100644
> index 000000000000..b4586d7a7a45
> --- /dev/null
> +++ b/drivers/mtd/nand/raw/nuvoton_ma35d1_nand.c
> @@ -0,0 +1,1068 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2024 Nuvoton Technology Corp.
> + */
> +#include <linux/slab.h>
> +#include <linux/init.h>
> +#include <linux/module.h>
> +#include <linux/interrupt.h>
> +#include <linux/io.h>
> +#include <linux/platform_device.h>
> +#include <linux/delay.h>
> +#include <linux/clk.h>
> +#include <linux/err.h>
> +#include <linux/of.h>
> +

Not sure why you didn't sort the below includes with the ones above?

> +#include <linux/mtd/mtd.h>
> +#include <linux/mtd/partitions.h>
> +#include <linux/mtd/rawnand.h>
> +#include <linux/dma-mapping.h>
> +#include <linux/dmaengine.h>
> +
> +/* NFI DMA Registers */
> +#define MA35_NFI_REG_BUFFER0		(0x000)

You don't need all these parentheses

> +#define MA35_NFI_REG_DMACTL		(0x400)
> +#define   DMA_EN				BIT(0)
> +#define   DMA_RST				BIT(1)
> +#define   DMA_BUSY				BIT(9)
> +
> +#define MA35_NFI_REG_DMASA		(0x408)
> +#define MA35_NFI_REG_DMABCNT		(0x40C)
> +#define MA35_NFI_REG_DMAINTEN		(0x410)
> +#define MA35_NFI_REG_DMAINTSTS	(0x414)
> +
> +/* NFI Global Registers */
> +#define MA35_NFI_REG_GCTL		(0x800)
> +#define   NAND_EN				BIT(3)
> +#define MA35_NFI_REG_GINTEN		(0x804)
> +#define MA35_NFI_REG_GINTSTS		(0x808)
> +
> +/* NAND-type Flash Registers */
> +#define MA35_NFI_REG_NANDCTL		(0x8A0)
> +#define   SWRST				BIT(0)
> +#define   DMA_W_EN				BIT(1)
> +#define   DMA_R_EN				BIT(2)
> +#define   ECC_CHK				BIT(7)
> +#define   PROT3BEN				BIT(8)
> +#define   PSIZE_2K				(1 << 16)
> +#define   PSIZE_4K				(2 << 16)
> +#define   PSIZE_8K				(3 << 16)
> +#define   PSIZE_MASK				(3 << 16)
> +#define   BCH_T24				BIT(18)
> +#define   BCH_T8				BIT(20)
> +#define   BCH_T12				BIT(21)
> +#define   BCH_NONE				(0x0)
> +#define   BCH_MASK				(0x1f << 18)
> +#define   ECC_EN				BIT(23)
> +#define   DISABLE_CS0				BIT(25)
> +
> +#define MA35_NFI_REG_NANDTMCTL	(0x8A4)
> +#define MA35_NFI_REG_NANDINTEN	(0x8A8)
> +#define MA35_NFI_REG_NANDINTSTS	(0x8AC)
> +#define   INT_DMA				BIT(0)
> +#define   INT_ECC				BIT(2)
> +#define   INT_RB0				BIT(10)
> +#define   INT_RB0_STS				BIT(18)
> +
> +#define MA35_NFI_REG_NANDCMD		(0x8B0)
> +#define MA35_NFI_REG_NANDADDR		(0x8B4)
> +#define   ENDADDR				BIT(31)
> +
> +#define MA35_NFI_REG_NANDDATA		(0x8B8)
> +#define MA35_NFI_REG_NANDRACTL	(0x8BC)
> +#define MA35_NFI_REG_NANDECTL		(0x8C0)
> +#define   ENABLE_WP				(0x0)
> +#define   DISABLE_WP				BIT(0)
> +
> +#define MA35_NFI_REG_NANDECCES0	(0x8D0)
> +#define   ECC_STATUS_MASK			(0x3)
> +#define   ECC_ERR_CNT_MASK			(0x1f)
> +
> +#define MA35_NFI_REG_NANDECCES1	(0x8D4)
> +#define MA35_NFI_REG_NANDECCES2	(0x8D8)
> +#define MA35_NFI_REG_NANDECCES3	(0x8DC)
> +
> +/* NAND-type Flash BCH Error Address Registers */
> +#define MA35_NFI_REG_NANDECCEA0	(0x900)
> +#define MA35_NFI_REG_NANDECCEA1	(0x904)
> +#define MA35_NFI_REG_NANDECCEA2	(0x908)
> +#define MA35_NFI_REG_NANDECCEA3	(0x90C)
> +#define MA35_NFI_REG_NANDECCEA4	(0x910)
> +#define MA35_NFI_REG_NANDECCEA5	(0x914)
> +#define MA35_NFI_REG_NANDECCEA6	(0x918)
> +#define MA35_NFI_REG_NANDECCEA7	(0x91C)
> +#define MA35_NFI_REG_NANDECCEA8	(0x920)
> +#define MA35_NFI_REG_NANDECCEA9	(0x924)
> +#define MA35_NFI_REG_NANDECCEA10	(0x928)
> +#define MA35_NFI_REG_NANDECCEA11	(0x92C)
> +
> +/* NAND-type Flash BCH Error Data Registers */
> +#define MA35_NFI_REG_NANDECCED0	(0x960)
> +#define MA35_NFI_REG_NANDECCED1	(0x964)
> +#define MA35_NFI_REG_NANDECCED2	(0x968)
> +#define MA35_NFI_REG_NANDECCED3	(0x96C)
> +#define MA35_NFI_REG_NANDECCED4	(0x970)
> +#define MA35_NFI_REG_NANDECCED5	(0x974)
> +
> +/* NAND-type Flash Redundant Area Registers */
> +#define MA35_NFI_REG_NANDRA0		(0xA00)
> +#define MA35_NFI_REG_NANDRA1		(0xA04)
> +
> +#define SKIP_SPARE_BYTES	4
> +
> +/* BCH algorithm related constants and variables */
> +static const int ma35_parity[3][4] = {
> +	{0,  60,  92,  90},  /* for 2K */
> +	{0, 120, 184, 180},  /* for 4K */
> +	{0, 240, 368, 360},  /* for 8K */

Can you please create definitions for the matrix rows? (using
an enum seems appropriate)

And maybe an array of three structures would be best because I believe
you're defining offsets for something which is not clear to the reader.

> +};
> +
> +struct ma35_nand_info {
> +	struct nand_controller controller;
> +	struct device *dev;
> +	void __iomem *regs;
> +	int irq;
> +	struct clk *clk;
> +	struct completion complete;
> +
> +	struct mtd_info mtd;

Please have a look at nand_to_mtd()

> +	struct nand_chip chip;

Is there a single CS supported? Is there a single RB supported? 

> +	struct mtd_partition *parts;

No, this has nothing to do here.

> +	struct nand_ecclayout_user nand_oob;

Deprecated structure

> +	int nr_parts;
> +
> +	u32 bch;
> +	u8 *dma_buf;
> +	spinlock_t dma_lock;
> +	dma_addr_t dma_addr;
> +};
> +
> +static int ma35_ooblayout_ecc(struct mtd_info *mtd, int section,
> +			      struct mtd_oob_region *oobregion)
> +{
> +	struct nand_chip *chip = mtd_to_nand(mtd);
> +
> +	if (section)
> +		return -ERANGE;
> +
> +	oobregion->length = chip->ecc.total;
> +	oobregion->offset = mtd->oobsize - oobregion->length;
> +
> +	return 0;
> +}
> +
> +static int ma35_ooblayout_free(struct mtd_info *mtd, int section,
> +			       struct mtd_oob_region *oobregion)
> +{
> +	struct nand_chip *chip = mtd_to_nand(mtd);
> +
> +	if (section)
> +		return -ERANGE;
> +
> +	oobregion->length = mtd->oobsize - chip->ecc.total - 2;
> +	oobregion->offset = 2;
> +
> +	return 0;
> +}
> +
> +static const struct mtd_ooblayout_ops ma35_ooblayout_ops = {
> +	.free = ma35_ooblayout_free,
> +	.ecc = ma35_ooblayout_ecc,
> +};
> +
> +/*
> + * Initialize hardware ECC
> + */
> +static void ma35_nand_hwecc_init(struct ma35_nand_info *nand)
> +{
> +	struct mtd_info *mtd = nand_to_mtd(&nand->chip);
> +
> +	/* reset nand controller */

	   Reset NAND

> +	writel(readl(nand->regs + MA35_NFI_REG_NANDCTL) | SWRST,
> +		nand->regs + MA35_NFI_REG_NANDCTL);

I believe it's fine to do it on several lines and probably clearer.

	u32 reg = readl();
	reg |= SOMETHING;
	writel();

No wait after the reset?

> +	/* Redundant area size */
> +	writel(mtd->oobsize, nand->regs + MA35_NFI_REG_NANDRACTL);
> +
> +	/* Protect redundant 3 bytes */

What does that mean?

> +	writel(readl(nand->regs + MA35_NFI_REG_NANDCTL) | PROT3BEN,
> +		nand->regs + MA35_NFI_REG_NANDCTL);
> +
> +	/* Write the ECC parity codes automatically to NAND Flash */
> +	writel(readl(nand->regs + MA35_NFI_REG_NANDCTL) | ECC_CHK,
> +		nand->regs + MA35_NFI_REG_NANDCTL);

No, by default you should disabled the ECC engine. Then when you need
it you enable/use/disable it.

> +
> +	if (nand->bch == BCH_NONE) {
> +		/* Disable H/W ECC, ECC parity check enable bit during read page */
> +		writel(readl(nand->regs + MA35_NFI_REG_NANDCTL) & (~ECC_EN),
> +			nand->regs + MA35_NFI_REG_NANDCTL);
> +	} else {
> +		/* Set BCH algorithm */
> +		writel((readl(nand->regs + MA35_NFI_REG_NANDCTL) & (~BCH_MASK)) |
> +			nand->bch, nand->regs + MA35_NFI_REG_NANDCTL);
> +
> +		/* Enable H/W ECC, ECC parity check enable bit during read page */
> +		writel(readl(nand->regs + MA35_NFI_REG_NANDCTL) | ECC_EN,
> +			nand->regs + MA35_NFI_REG_NANDCTL);
> +	}
> +	spin_lock_init(&nand->dma_lock);
> +}
> +
> +static void ma35_nand_initialize(struct ma35_nand_info *nand)
> +{
> +	writel(NAND_EN, nand->regs + MA35_NFI_REG_GCTL);
> +}
> +
> +
> +/* Define some constants for BCH */

			for the BCH hardware ECC engine

> +/* define the total padding bytes for 512/1024 data segment */
> +#define BCH_PADDING_LEN_512	32
> +#define BCH_PADDING_LEN_1024	64
> +/* define the BCH parity code length for 512 bytes data pattern */
> +#define BCH_PARITY_LEN_T8	15
> +#define BCH_PARITY_LEN_T12	23
> +/* define the BCH parity code length for 1024 bytes data pattern */
> +#define BCH_PARITY_LEN_T24	45
> +

Is T the strength? Can we name it strength instead?

Please move the definitions at the top

> +/* Correct data by BCH alrogithm */
> +static void ma35_nfi_correctdata(struct ma35_nand_info *nand, u8 index,
> +				 u8 err_cnt, u8 *addr)

correctdata vs correct, the naming needs to be improved

> +{
> +	u8 *ptr = (u8 *)((long)nand->regs + MA35_NFI_REG_NANDRA0);

Haha, no, never.

Please compile with C=1 and see how this explodes.

Also, you can enable W=1

> +	u32 field_len, padding_len, parity_len;
> +	u32 temp_data[24], temp_addr[24];
> +	u32 total_field_num, page;
> +	u32 err_data[6];
> +	u8  *smra_index;
> +	u8  i, j;
> +
> +	/* assign parameters for different BCH and page size */

					   configurations

> +	switch (readl(nand->regs + MA35_NFI_REG_NANDCTL) & BCH_MASK) {
> +	case BCH_T24:
> +		field_len = 1024;
> +		parity_len = BCH_PARITY_LEN_T24;
> +		padding_len = BCH_PADDING_LEN_1024;
> +		break;
> +	case BCH_T12:
> +		field_len = 512;
> +		parity_len = BCH_PARITY_LEN_T12;
> +		padding_len = BCH_PADDING_LEN_512;
> +		break;
> +	case BCH_T8:
> +		field_len = 512;
> +		parity_len = BCH_PARITY_LEN_T8;
> +		padding_len = BCH_PADDING_LEN_512;
> +		break;
> +	default:
> +		pr_warn("NAND ERROR: invalid SMCR_BCH_TSEL = 0x%08X\n",
> +			(u32)(readl(nand->regs + MA35_NFI_REG_NANDCTL) & BCH_MASK));
> +		return;
> +	}
> +
> +	page = readl(nand->regs + MA35_NFI_REG_NANDCTL) & PSIZE_MASK;
> +	switch (page) {
> +	case PSIZE_8K:
> +		total_field_num = 8192 / field_len; break;
> +	case PSIZE_4K:
> +		total_field_num = 4096 / field_len; break;
> +	case PSIZE_2K:
> +		total_field_num = 2048 / field_len; break;

Break on a new line

> +	default:
> +		pr_warn("NAND ERROR: invalid SMCR_PSIZE = 0x%08X\n", page);
> +		return;
> +	}
> +
> +	/* got valid BCH_ECC_DATAx and parse them to temp_data[]
> +	 * got the valid register number of BCH_ECC_DATAx since
> +	 * one register include 4 error bytes
> +	 */
> +	j = err_cnt / 4;
> +	j++;
> +	if (j > 6)
> +		j = 6;	/* there are 6 BCH_ECC_DATAx registers to support BCH T24 */
> +
> +	for (i = 0; i < j; i++)
> +		err_data[i] = readl(nand->regs + MA35_NFI_REG_NANDECCED0 + i*4);
> +
> +	for (i = 0; i < j; i++) {
> +		temp_data[i*4+0] = err_data[i] & 0xff;
> +		temp_data[i*4+1] = (err_data[i] >> 8) & 0xff;
> +		temp_data[i*4+2] = (err_data[i] >> 16) & 0xff;
> +		temp_data[i*4+3] = (err_data[i] >> 24) & 0xff;
> +	}
> +
> +	/* got valid REG_BCH_ECC_ADDRx and parse them to temp_addr[]
> +	 * got the valid register number of REG_BCH_ECC_ADDRx since
> +	 * one register include 2 error addresses
> +	 */
> +	j = err_cnt / 2;
> +	j++;
> +	if (j > 12)
> +		j = 12; /* there are 12 REG_BCH_ECC_ADDRx registers to support BCH T24 */
> +
> +	for (i = 0; i < j; i++) {
> +		/* 11 bits for error address */
> +		temp_addr[i*2+0] = readl(nand->regs + MA35_NFI_REG_NANDECCEA0 + i*4) & 0x07ff;
> +		temp_addr[i*2+1] = (readl(nand->regs + MA35_NFI_REG_NANDECCEA0 + i*4)>>16) & 0x07ff;
> +	}
> +
> +	/* pointer to begin address of field that with data error */
> +	addr += (index-1) * field_len;
> +
> +	/* correct each error bytes */
> +	for (i = 0; i < err_cnt; i++) {
> +		/* for wrong data in field */
> +		if (temp_addr[i] < field_len)
> +			*(addr+temp_addr[i]) ^= temp_data[i];
> +
> +		/* for wrong first-3-bytes in redundancy area */
> +		else if (temp_addr[i] < (field_len+3)) {
> +			temp_addr[i] -= field_len;
> +			temp_addr[i] += (parity_len * (index-1));	/* field offset */
> +
> +			*(ptr + temp_addr[i]) ^= temp_data[i];
> +		}
> +		/* for wrong parity code in redundancy area */
> +		/* BCH_ERR_ADDRx = [data in field] + [3 bytes] + [xx] + [parity code]          */
> +		/*                                   |<--     padding bytes      -->|          */
> +		/* The BCH_ERR_ADDRx for last parity code always = field size + padding size.  */
> +		/* So, the first parity code = field size + padding size - parity code length. */
> +		/* For example, for BCH T12, the first parity code = 512 + 32 - 23 = 521.      */
> +		/* That is, error byte address offset within field is                          */
> +		else {
> +			temp_addr[i] = temp_addr[i] - (field_len + padding_len - parity_len);
> +
> +			/* smra_index point to the first parity code of
> +			 * first field in register SMRA0~n
> +			 */
> +			smra_index = (u8 *)(ptr +
> +				(readl(nand->regs+MA35_NFI_REG_NANDRACTL) & 0x1ff) -
> +				(parity_len * total_field_num));
> +
> +			/* final address = first parity code of first field + */
> +			/*                 offset of fields +                 */
> +			/*                 offset within field                */

Coding style

> +
> +			*((u8 *)smra_index + (parity_len * (index - 1)) + temp_addr[i])
> +				^= temp_data[i];

-ENOPARSE

> +		}
> +	} /* end of for (i < err_cnt) */

Useless comment

> +}
> +
> +static int ma35_nfi_correct(struct nand_chip *chip, unsigned long addr)
> +{
> +	struct ma35_nand_info *nand = nand_get_controller_data(chip);
> +	struct mtd_info *mtd = nand_to_mtd(chip);
> +	int status, i, j, field = 0;
> +	int report_err = 0;
> +	int err_cnt = 0;
> +
> +	if ((readl(nand->regs + MA35_NFI_REG_NANDCTL) & BCH_MASK) == BCH_T24)
> +		field = mtd->writesize / 1024;

Can we call this a nchunks? Also, you're supposed to expect some DT
properties (based on your bindings) and you're not using their values,
it's strange.

> +	else
> +		field = mtd->writesize / 512;
> +
> +	if (field < 4)
> +		field = 1;
> +	else
> +		field /= 4;
> +
> +	for (j = 0; j < field; j++) {
> +		status = readl(nand->regs + MA35_NFI_REG_NANDECCES0 + j*4);
> +		if (!status)
> +			continue;

Is this case relevant? Isn't it treated below?

> +
> +		for (i = 1; i < 5; i++) {


[Index of Archives]     [Linux Input]     [Video for Linux]     [Gstreamer Embedded]     [Mplayer Users]     [Linux USB Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]

  Powered by Linux