Message-ID: <06d627d5-947c-4da4-826a-76033386b575@kernel.org>
Date: Mon, 12 Aug 2024 08:30:20 +0200
From: Krzysztof Kozlowski <krzk@...nel.org>
To: Hui-Ping Chen <hpchen0nvt@...il.com>, miquel.raynal@...tlin.com,
 richard@....at, vigneshr@...com, robh@...nel.org, krzk+dt@...nel.org,
 conor+dt@...nel.org, sumit.semwal@...aro.org, christian.koenig@....com,
 esben@...nix.com
Cc: linux-arm-kernel@...ts.infradead.org, linux-mtd@...ts.infradead.org,
 devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
 linux-media@...r.kernel.org, dri-devel@...ts.freedesktop.org,
 linaro-mm-sig@...ts.linaro.org
Subject: Re: [PATCH 2/2] mtd: rawnand: nuvoton: add new driver for the Nuvoton
 MA35 SoC

On 12/08/2024 05:00, Hui-Ping Chen wrote:
> Nuvoton MA35 SoCs NAND Flash Interface Controller
> supports 2KB, 4KB and 8KB page size, and up to 8-bit,
> 12-bit, and 24-bit hardware ECC calculation circuit
> to protect data communication.
> 
> Signed-off-by: Hui-Ping Chen <hpchen0nvt@...il.com>
> ---
>  drivers/mtd/nand/raw/Kconfig               |    8 +
>  drivers/mtd/nand/raw/Makefile              |    1 +
>  drivers/mtd/nand/raw/nuvoton_ma35d1_nand.c | 1109 ++++++++++++++++++++
>  3 files changed, 1118 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
> +	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..7445eb489c38
> --- /dev/null
> +++ b/drivers/mtd/nand/raw/nuvoton_ma35d1_nand.c
> @@ -0,0 +1,1109 @@
> +// 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>
> +
> +#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)
> +#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_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 */
> +enum {
> +	eBCH_NONE = 0,
> +	eBCH_T8,
> +	eBCH_T12,
> +	eBCH_T24,
> +	eBCH_CNT
> +} E_BCHALGORITHM;
> +
> +static const int g_i32BCHAlgoIdx[eBCH_CNT] = {BCH_T8, BCH_T8, BCH_T12, BCH_T24};
> +static struct nand_ecclayout_user ma35_nand_oob;

Why this is file-scope?

> +static const int g_i32ParityNum[3][eBCH_CNT] = {


This is not a Linux coding style. Do not send Windows or some other
platform-independent patches to review.


> +	{0,  60,  92,  90},  /* for 2K */
> +	{0, 120, 184, 180},  /* for 4K */
> +	{0, 240, 368, 360},  /* for 8K */
> +};
> +
> +
> +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;
> +	struct nand_chip chip;
> +	struct mtd_partition *parts;
> +	int nr_parts;
> +
> +	int eBCHAlgo;

No, use Linux coding style.

> +	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);
> +	struct nand_ecc_ctrl *ecc = &chip->ecc;
> +
> +	if (section || !ecc->total)
> +		return -ERANGE;
> +
> +	oobregion->length = ecc->total;
> +	oobregion->offset = mtd->oobsize - oobregion->length;
> +
> +	return 0;
> +}

... this code looks poor, really poor.

> +}
> +
> +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 uStatus, i, j, i32FieldNum = 0;
> +	int uReportErrCnt = 0;
> +	int uErrorCnt = 0;
> +

More of weird coding style. Use Linux coding style. Patch is for Linux.

> +	if ((readl(nand->regs + MA35_NFI_REG_NANDCTL) & BCH_MASK) == BCH_T24)
> +		i32FieldNum = mtd->writesize / 1024;
> +	else
> +		i32FieldNum = mtd->writesize / 512;


...

> +static int ma35_nand_probe(struct platform_device *pdev)
> +{
> +	struct ma35_nand_info *nand;
> +	struct nand_chip *chip;
> +	struct mtd_info *mtd;
> +	int retval = 0;
> +
> +	nand = devm_kzalloc(&pdev->dev, sizeof(struct ma35_nand_info), GFP_KERNEL);

sizeof(*)

> +	if (!nand)
> +		return -ENOMEM;
> +
> +	nand_controller_init(&nand->controller);
> +
> +	nand->regs = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(nand->regs))
> +		return PTR_ERR(nand->regs);
> +
> +	nand->dev = &pdev->dev;
> +	chip = &nand->chip;
> +	mtd = nand_to_mtd(chip);
> +	nand_set_controller_data(chip, nand);
> +	nand_set_flash_node(chip, pdev->dev.of_node);
> +
> +	mtd->priv = chip;
> +	mtd->owner = THIS_MODULE;
> +	mtd->dev.parent = &pdev->dev;
> +
> +	nand->clk = of_clk_get(pdev->dev.of_node, 0);

No, no. That's not how you get a clock. Use devm_clk_get().

> +	if (IS_ERR(nand->clk))
> +		return dev_err_probe(&pdev->dev, PTR_ERR(nand->clk),
> +				     "failed to find nand clock\n");
> +
> +	retval = clk_prepare_enable(nand->clk);
> +	if (retval < 0) {
> +		dev_err(&pdev->dev, "Failed to enable clock\n");
> +		return -ENXIO;
> +	}
> +
> +	nand->chip.controller    = &nand->controller;
> +
> +	chip->legacy.cmdfunc     = ma35_nand_command;
> +	chip->legacy.waitfunc    = ma35_waitfunc;
> +	chip->legacy.read_byte   = ma35_nand_read_byte;
> +	chip->legacy.select_chip = ma35_nand_select_chip;
> +	chip->legacy.read_buf    = ma35_read_buf_dma;
> +	chip->legacy.write_buf   = ma35_write_buf_dma;
> +	chip->legacy.dev_ready   = ma35_nand_devready;
> +	chip->legacy.chip_delay  = 25; /* us */
> +
> +	/* Read OOB data first, then HW read page */
> +	chip->ecc.hwctl      = ma35_nand_enable_hwecc;
> +	chip->ecc.calculate  = ma35_nand_calculate_ecc;
> +	chip->ecc.correct    = ma35_nand_correct_data;
> +	chip->ecc.write_page = ma35_nand_write_page_hwecc;
> +	chip->ecc.read_page  = ma35_nand_read_page_hwecc_oob_first;
> +	chip->ecc.read_oob   = ma35_nand_read_oob_hwecc;
> +	chip->options |= (NAND_NO_SUBPAGE_WRITE | NAND_USES_DMA);
> +
> +	ma35_nand_initialize(nand);
> +	platform_set_drvdata(pdev, nand);
> +
> +	nand->controller.ops = &ma35_nand_controller_ops;
> +
> +	nand->irq = platform_get_irq(pdev, 0);
> +	if (nand->irq < 0) {
> +		dev_err(&pdev->dev, "failed to get platform irq\n");

Syntax is return dev_err_probe

> +		return -EINVAL;
> +	}
> +
> +	if (request_irq(nand->irq, (irq_handler_t)&ma35_nand_irq,
> +			IRQF_TRIGGER_HIGH, "ma35d1-nand", nand)) {
> +		dev_err(&pdev->dev, "Error requesting NAND IRQ\n");
> +		return -ENXIO;
> +	}
> +
> +	retval = nand_scan(chip, 1);
> +	if (retval)
> +		return retval;
> +
> +	if (mtd_device_register(mtd, nand->parts, nand->nr_parts)) {
> +		nand_cleanup(chip);
> +		devm_kfree(&pdev->dev, nand);
> +		return retval;
> +	}
> +
> +	pr_info("ma35-nfi: registered successfully! mtdid=%s\n", mtd->name);

Drop, does not help.

Drivers do not use pr_xxx anyway. Why your code is so inconsistent? It's
like random junk glued together.

> +
> +	return retval;
> +}
> +
> +static void ma35_nand_remove(struct platform_device *pdev)
> +{
> +	struct ma35_nand_info *nand = platform_get_drvdata(pdev);
> +	struct nand_chip *chip = &nand->chip;
> +	int ret;
> +
> +	ret = mtd_device_unregister(nand_to_mtd(chip));
> +	WARN_ON(ret);
> +	nand_cleanup(chip);
> +
> +	clk_disable_unprepare(nand->clk);
> +
> +	kfree(nand);
> +	platform_set_drvdata(pdev, NULL);
> +}
> +
> +/* PM Support */
> +#ifdef CONFIG_PM
> +static int ma35_nand_suspend(struct platform_device *pdev, pm_message_t pm)
> +{
> +	struct ma35_nand_info *nand = platform_get_drvdata(pdev);
> +	unsigned long timeo = jiffies + HZ/2;
> +
> +	/* wait DMAC to ready */
> +	while (1) {
> +		if ((readl(nand->regs + MA35_NFI_REG_DMACTL) & DMA_BUSY) == 0)
> +			break;
> +		if (time_after(jiffies, timeo))
> +			return -ETIMEDOUT;
> +	}
> +
> +	clk_disable(nand->clk);
> +
> +	return 0;
> +}
> +
> +static int ma35_nand_resume(struct platform_device *pdev)
> +{
> +	struct ma35_nand_info *nand = platform_get_drvdata(pdev);
> +
> +	clk_enable(nand->clk);
> +	ma35_nand_hwecc_init(nand);
> +	ma35_nand_dmac_init(nand);
> +
> +	return 0;
> +}
> +
> +#else
> +#define ma35_nand_suspend NULL
> +#define ma35_nand_resume NULL
> +#endif
> +
> +static const struct of_device_id ma35_nfi_of_match[] = {
> +	{ .compatible = "nuvoton,ma35d1-nand" },
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, ma35_nfi_of_match);
> +
> +static struct platform_driver ma35_nand_driver = {
> +		.driver = {
> +		.name   = "ma35d1-nand",
> +		.owner  = THIS_MODULE,
> +		.of_match_table = of_match_ptr(ma35_nfi_of_match),

Drop of_match_ptr. You have warnings here.

> +		},
> +		.probe = ma35_nand_probe,
> +		.remove_new = ma35_nand_remove,
> +		.suspend = ma35_nand_suspend,
> +		.resume = ma35_nand_resume,
> +};
> +
> +static int __init ma35_nand_init(void)
> +{
> +	int ret;
> +
> +	pr_info("ma35 mtd nand driver\n");

NAK, drop, useless and really pointless.

> +
> +	ret = platform_driver_register(&ma35_nand_driver);
> +	if (ret) {
> +		pr_warn("nand: failed to add device driver %s\n",
> +			ma35_nand_driver.driver.name);
> +		return ret;
> +	}
> +
> +	return ret;
> +}
> +
> +static void __exit ma35_nand_exit(void)
> +{
> +	platform_driver_unregister(&ma35_nand_driver);
> +	pr_info("nand: unregistered successfully!\n");

NAK, drop, useless and really pointless.

> +}
> +
> +module_init(ma35_nand_init);
> +module_exit(ma35_nand_exit);

Do not open-code these functions, there are wrappers for this.

> +
> +MODULE_DESCRIPTION("Nuvoton ma35 NAND driver");
> +MODULE_AUTHOR("Hui-Ping Chen <hpchen0nvt@...il.com>");
> +MODULE_LICENSE("GPL");

Best regards,
Krzysztof

Powered by blists - more mailing lists