Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S965318AbbLHQEe (ORCPT ); Tue, 8 Dec 2015 11:04:34 -0500 Received: from mailapp01.imgtec.com ([195.59.15.196]:3558 "EHLO mailapp01.imgtec.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S965194AbbLHQEG (ORCPT ); Tue, 8 Dec 2015 11:04:06 -0500 From: Harvey Hunt Subject: Re: [PATCH v9 2/3] mtd: nand: jz4780: driver for NAND devices on JZ4780 SoCs To: Boris Brezillon References: <1449144142-24004-1-git-send-email-harvey.hunt@imgtec.com> <1449144142-24004-3-git-send-email-harvey.hunt@imgtec.com> <20151208151436.11ce72f4@bbrezillon> CC: , Zubair Lutfullah Kakakhel , , Alex Smith , Brian Norris , "David Woodhouse" Message-ID: <5666FF6B.5090309@imgtec.com> Date: Tue, 8 Dec 2015 16:03:55 +0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.4.0 MIME-Version: 1.0 In-Reply-To: <20151208151436.11ce72f4@bbrezillon> Content-Type: text/plain; charset="windows-1252"; format=flowed Content-Transfer-Encoding: 7bit X-Originating-IP: [192.168.154.22] Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 29404 Lines: 962 Hi Boris, Thanks for the review. On 08/12/15 14:14, Boris Brezillon wrote: > On Thu, 3 Dec 2015 12:02:21 +0000 > Harvey Hunt wrote: > >> From: Alex Smith >> >> Add a driver for NAND devices connected to the NEMC on JZ4780 SoCs, as >> well as the hardware BCH controller. DMA is not currently implemented. >> >> While older 47xx SoCs also have a BCH controller, they are incompatible >> with the one in the 4780 due to differing register/bit positions, which >> would make implementing a common driver for them quite messy. >> >> Signed-off-by: Alex Smith >> Cc: Zubair Lutfullah Kakakhel >> Cc: David Woodhouse >> Cc: Brian Norris >> Cc: linux-mtd@lists.infradead.org >> Cc: linux-kernel@vger.kernel.org >> Signed-off-by: Harvey Hunt >> --- >> v8 -> v9: >> - No change. >> >> v7 -> v8: >> - Rebase to 4.4-rc3. >> - Add _US suffixes to time constants. >> - Add locking to BCH hardware accesses. >> - Don't print ECC info if ECC is not being used. >> - Default to No ECC. >> - Let the NAND core handle ECC layout in certain cases. >> - Use the gpio_desc consumer interface. >> - Removed gpio active low flags. >> - Check for the BCH controller before initialising a chip. >> - Add a jz4780_nand_controller struct. >> - Initialise chips by iterating over DT child nodes. >> >> v6 -> v7: >> - Add nand-ecc-mode to DT bindings. >> - Add nand-on-flash-bbt to DT bindings. >> >> v5 -> v6: >> - No change. >> >> v4 -> v5: >> - Rename ingenic,bch-device to ingenic,bch-controller to fit with >> existing convention. >> >> v3 -> v4: >> - No change >> >> v2 -> v3: >> - Rebase to 4.0-rc6 >> - Changed ingenic,ecc-size to common nand-ecc-step-size >> - Changed ingenic,ecc-strength to common nand-ecc-strength >> - Changed ingenic,busy-gpio to common rb-gpios >> - Changed ingenic,wp-gpio to common wp-gpios >> >> v1 -> v2: >> - Rebase to 4.0-rc3 >> >> drivers/mtd/nand/Kconfig | 7 + >> drivers/mtd/nand/Makefile | 1 + >> drivers/mtd/nand/jz4780_bch.c | 361 +++++++++++++++++++++++++++++++++++ >> drivers/mtd/nand/jz4780_bch.h | 42 +++++ >> drivers/mtd/nand/jz4780_nand.c | 420 +++++++++++++++++++++++++++++++++++++++++ >> 5 files changed, 831 insertions(+) >> create mode 100644 drivers/mtd/nand/jz4780_bch.c >> create mode 100644 drivers/mtd/nand/jz4780_bch.h >> create mode 100644 drivers/mtd/nand/jz4780_nand.c >> >> diff --git a/drivers/mtd/nand/Kconfig b/drivers/mtd/nand/Kconfig >> index 2896640..b742adc 100644 >> --- a/drivers/mtd/nand/Kconfig >> +++ b/drivers/mtd/nand/Kconfig >> @@ -519,6 +519,13 @@ config MTD_NAND_JZ4740 >> help >> Enables support for NAND Flash on JZ4740 SoC based boards. >> >> +config MTD_NAND_JZ4780 >> + tristate "Support for NAND on JZ4780 SoC" >> + depends on MACH_JZ4780 && JZ4780_NEMC >> + help >> + Enables support for NAND Flash connected to the NEMC on JZ4780 SoC >> + based boards, using the BCH controller for hardware error correction. >> + >> config MTD_NAND_FSMC >> tristate "Support for NAND on ST Micros FSMC" >> depends on PLAT_SPEAR || ARCH_NOMADIK || ARCH_U8500 || MACH_U300 >> diff --git a/drivers/mtd/nand/Makefile b/drivers/mtd/nand/Makefile >> index 2c7f014..9e36233 100644 >> --- a/drivers/mtd/nand/Makefile >> +++ b/drivers/mtd/nand/Makefile >> @@ -49,6 +49,7 @@ obj-$(CONFIG_MTD_NAND_MPC5121_NFC) += mpc5121_nfc.o >> obj-$(CONFIG_MTD_NAND_VF610_NFC) += vf610_nfc.o >> obj-$(CONFIG_MTD_NAND_RICOH) += r852.o >> obj-$(CONFIG_MTD_NAND_JZ4740) += jz4740_nand.o >> +obj-$(CONFIG_MTD_NAND_JZ4780) += jz4780_nand.o jz4780_bch.o >> obj-$(CONFIG_MTD_NAND_GPMI_NAND) += gpmi-nand/ >> obj-$(CONFIG_MTD_NAND_XWAY) += xway_nand.o >> obj-$(CONFIG_MTD_NAND_BCM47XXNFLASH) += bcm47xxnflash/ >> diff --git a/drivers/mtd/nand/jz4780_bch.c b/drivers/mtd/nand/jz4780_bch.c >> new file mode 100644 >> index 0000000..0c472f4 >> --- /dev/null >> +++ b/drivers/mtd/nand/jz4780_bch.c >> @@ -0,0 +1,361 @@ >> +/* >> + * JZ4780 BCH controller >> + * >> + * Copyright (c) 2015 Imagination Technologies >> + * Author: Alex Smith >> + * >> + * This program is free software; you can redistribute it and/or modify it >> + * under the terms of the GNU General Public License version 2 as published >> + * by the Free Software Foundation. >> + */ >> + >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> + >> +#include "jz4780_bch.h" >> + >> +#define BCH_BHCR 0x0 >> +#define BCH_BHCCR 0x8 >> +#define BCH_BHCNT 0xc >> +#define BCH_BHDR 0x10 >> +#define BCH_BHPAR0 0x14 >> +#define BCH_BHERR0 0x84 >> +#define BCH_BHINT 0x184 >> +#define BCH_BHINTES 0x188 >> +#define BCH_BHINTEC 0x18c >> +#define BCH_BHINTE 0x190 >> + >> +#define BCH_BHCR_BSEL_SHIFT 4 >> +#define BCH_BHCR_BSEL_MASK (0x7f << BCH_BHCR_BSEL_SHIFT) >> +#define BCH_BHCR_ENCE BIT(2) >> +#define BCH_BHCR_INIT BIT(1) >> +#define BCH_BHCR_BCHE BIT(0) >> + >> +#define BCH_BHCNT_PARITYSIZE_SHIFT 16 >> +#define BCH_BHCNT_PARITYSIZE_MASK (0x7f << BCH_BHCNT_PARITYSIZE_SHIFT) >> +#define BCH_BHCNT_BLOCKSIZE_SHIFT 0 >> +#define BCH_BHCNT_BLOCKSIZE_MASK (0x7ff << BCH_BHCNT_BLOCKSIZE_SHIFT) >> + >> +#define BCH_BHERR_MASK_SHIFT 16 >> +#define BCH_BHERR_MASK_MASK (0xffff << BCH_BHERR_MASK_SHIFT) >> +#define BCH_BHERR_INDEX_SHIFT 0 >> +#define BCH_BHERR_INDEX_MASK (0x7ff << BCH_BHERR_INDEX_SHIFT) >> + >> +#define BCH_BHINT_ERRC_SHIFT 24 >> +#define BCH_BHINT_ERRC_MASK (0x7f << BCH_BHINT_ERRC_SHIFT) >> +#define BCH_BHINT_TERRC_SHIFT 16 >> +#define BCH_BHINT_TERRC_MASK (0x7f << BCH_BHINT_TERRC_SHIFT) >> +#define BCH_BHINT_DECF BIT(3) >> +#define BCH_BHINT_ENCF BIT(2) >> +#define BCH_BHINT_UNCOR BIT(1) >> +#define BCH_BHINT_ERR BIT(0) >> + >> +#define BCH_CLK_RATE (200 * 1000 * 1000) >> + >> +/* Timeout for BCH calculation/correction. */ >> +#define BCH_TIMEOUT_US 100000 >> + >> +struct jz4780_bch { >> + void __iomem *base; >> + struct clk *clk; >> + spinlock_t lock; > > Do you really need to protect accesses to the ECC engine with a > spinlock... > >> +}; >> + > > [...] > >> + >> +/** >> + * jz4780_bch_calculate() - calculate ECC for a data buffer >> + * @dev: BCH device. >> + * @params: BCH parameters. >> + * @buf: input buffer with raw data. >> + * @ecc_code: output buffer with ECC. >> + * >> + * Return: 0 on success, -ETIMEDOUT if timed out while waiting for BCH >> + * controller. >> + */ >> +int jz4780_bch_calculate(struct device *dev, struct jz4780_bch_params *params, >> + const uint8_t *buf, uint8_t *ecc_code) >> +{ >> + struct jz4780_bch *bch = dev_get_drvdata(dev); >> + int ret = 0; >> + unsigned long flags; >> + >> + spin_lock_irqsave(&bch->lock, flags); > > ... and disable the interrupts while doing so? I mean, the MTD layer is > not supposed to call ->read()/->write() methods in irq context, so > using a mutex here should be perfectly fine (at least for the NAND > usage). > You're right, I underestimated the amount of time the spinlock would be held for. I also didn't realise that ->read()/->write() weren't called in IRQ context - I'll replace the spinlock with a mutex. >> + >> + jz4780_bch_init(bch, params, true); >> + jz4780_bch_write_data(bch, buf, params->size); >> + >> + if (jz4780_bch_wait_complete(bch, BCH_BHINT_ENCF, NULL)) { >> + jz4780_bch_read_parity(bch, ecc_code, params->bytes); >> + } else { >> + dev_err(dev, "timed out while calculating ECC\n"); >> + ret = -ETIMEDOUT; >> + } >> + >> + spin_unlock_irqrestore(&bch->lock, flags); >> + jz4780_bch_disable(bch); >> + return ret; >> +} >> +EXPORT_SYMBOL(jz4780_bch_calculate); >> + >> +/** >> + * jz4780_bch_correct() - detect and correct bit errors >> + * @dev: BCH device. >> + * @params: BCH parameters. >> + * @buf: raw data read from the chip. >> + * @ecc_code: ECC read from the chip. >> + * >> + * Given the raw data and the ECC read from the NAND device, detects and >> + * corrects errors in the data. >> + * >> + * Return: the number of bit errors corrected, or -1 if there are too many >> + * errors to correct or we timed out waiting for the controller. >> + */ >> +int jz4780_bch_correct(struct device *dev, struct jz4780_bch_params *params, >> + uint8_t *buf, uint8_t *ecc_code) >> +{ >> + struct jz4780_bch *bch = dev_get_drvdata(dev); >> + uint32_t reg, mask, index; > > Prefer u32/u16/u8 to the standard uint32_t/uint16_t/uint8_t definitions > when you are developing kernel code. > Ack. >> + int i, ret, count; >> + unsigned long flags; >> + >> + spin_lock_irqsave(&bch->lock, flags); >> + >> + jz4780_bch_init(bch, params, false); >> + jz4780_bch_write_data(bch, buf, params->size); >> + jz4780_bch_write_data(bch, ecc_code, params->bytes); >> + >> + if (!jz4780_bch_wait_complete(bch, BCH_BHINT_DECF, ®)) { >> + dev_err(dev, "timed out while correcting data\n"); >> + ret = -1; >> + goto out; >> + } >> + >> + if (reg & BCH_BHINT_UNCOR) { >> + dev_warn(dev, "uncorrectable ECC error\n"); >> + ret = -1; >> + goto out; >> + } >> + >> + /* Correct any detected errors. */ >> + if (reg & BCH_BHINT_ERR) { >> + count = (reg & BCH_BHINT_ERRC_MASK) >> BCH_BHINT_ERRC_SHIFT; >> + ret = (reg & BCH_BHINT_TERRC_MASK) >> BCH_BHINT_TERRC_SHIFT; >> + >> + for (i = 0; i < count; i++) { >> + reg = readl(bch->base + BCH_BHERR0 + (i * 4)); >> + mask = (reg & BCH_BHERR_MASK_MASK) >> >> + BCH_BHERR_MASK_SHIFT; >> + index = (reg & BCH_BHERR_INDEX_MASK) >> >> + BCH_BHERR_INDEX_SHIFT; >> + buf[(index * 2) + 0] ^= mask; >> + buf[(index * 2) + 1] ^= mask >> 8; >> + } >> + } else { >> + ret = 0; >> + } >> + >> +out: >> + spin_unlock_irqrestore(&bch->lock, flags); >> + jz4780_bch_disable(bch); >> + return ret; >> +} >> +EXPORT_SYMBOL(jz4780_bch_correct); >> + >> +/** >> + * jz4780_bch_get() - get the BCH controller device >> + * @np: BCH device tree node. >> + * @dev: where to store pointer to BCH controller device. >> + * >> + * Gets the BCH controller device from the specified device tree node. The >> + * device must be released with jz4780_bch_release() when it is no longer being >> + * used. >> + * >> + * Return: 0 on success, -EPROBE_DEFER if the controller has not yet been >> + * initialised. >> + */ >> +int jz4780_bch_get(struct device_node *np, struct device **dev) > > You can just return a struct device * value and use the ERR_PTR() macro > to cast an error code to a pointer. The caller can then test for the > error case by doing IS_ERR(ret), and extract the corresponding error > using PTR_ERR(). Okay, will do. > Also, I don't think you need to return a struct device pointer here. How > about just returning a struct jz4780_bch pointer and defining an opaque > type in jz4780_bch.h (a single 'struct jz4780_bch;' statement). > This would ease public jz4780_bch_xx() functions implementation (no > need to extract the jz4780_bch pointer from the device one) and hide the > jz4780_bch internals (the user doesn't have to know that the jz4780_bch > device is actually attached to a struct device). > I like the idea of using an opaque type, I'll do this. > >> +{ >> + struct platform_device *pdev; >> + struct jz4780_bch *bch; >> + >> + pdev = of_find_device_by_node(np); >> + if (!pdev || !platform_get_drvdata(pdev)) >> + return -EPROBE_DEFER; >> + >> + get_device(&pdev->dev); >> + >> + bch = platform_get_drvdata(pdev); >> + clk_prepare_enable(bch->clk); >> + >> + *dev = &pdev->dev; >> + return 0; >> +} >> +EXPORT_SYMBOL(jz4780_bch_get); >> + >> +/** >> + * jz4780_bch_release() - release the BCH controller device >> + * @dev: BCH device. >> + */ >> +void jz4780_bch_release(struct device *dev) >> +{ >> + struct jz4780_bch *bch = dev_get_drvdata(dev); >> + >> + clk_disable_unprepare(bch->clk); >> + put_device(dev); >> +} >> +EXPORT_SYMBOL(jz4780_bch_release); >> + >> +static int jz4780_bch_probe(struct platform_device *pdev) >> +{ >> + struct device *dev = &pdev->dev; >> + struct jz4780_bch *bch; >> + struct resource *res; >> + >> + bch = devm_kzalloc(dev, sizeof(*bch), GFP_KERNEL); >> + if (!bch) >> + return -ENOMEM; >> + >> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); >> + bch->base = devm_ioremap_resource(dev, res); >> + if (IS_ERR(bch->base)) >> + return PTR_ERR(bch->base); >> + >> + jz4780_bch_disable(bch); >> + >> + bch->clk = devm_clk_get(dev, NULL); >> + if (IS_ERR(bch->clk)) { >> + dev_err(dev, "failed to get clock: %ld\n", PTR_ERR(bch->clk)); >> + return PTR_ERR(bch->clk); >> + } >> + >> + clk_set_rate(bch->clk, BCH_CLK_RATE); >> + >> + spin_lock_init(&bch->lock); >> + >> + platform_set_drvdata(pdev, bch); >> + return 0; >> +} >> + >> +static const struct of_device_id jz4780_bch_dt_match[] = { >> + { .compatible = "ingenic,jz4780-bch" }, >> + {}, >> +}; >> +MODULE_DEVICE_TABLE(of, jz4780_bch_dt_match); >> + >> +static struct platform_driver jz4780_bch_driver = { >> + .probe = jz4780_bch_probe, >> + .driver = { >> + .name = "jz4780-bch", >> + .of_match_table = of_match_ptr(jz4780_bch_dt_match), >> + }, >> +}; >> +module_platform_driver(jz4780_bch_driver); >> + >> +MODULE_AUTHOR("Alex Smith "); >> +MODULE_DESCRIPTION("Ingenic JZ4780 BCH error correction driver"); >> +MODULE_LICENSE("GPL v2"); >> diff --git a/drivers/mtd/nand/jz4780_bch.h b/drivers/mtd/nand/jz4780_bch.h >> new file mode 100644 >> index 0000000..a5dfde5 >> --- /dev/null >> +++ b/drivers/mtd/nand/jz4780_bch.h >> @@ -0,0 +1,42 @@ >> +/* >> + * JZ4780 BCH controller >> + * >> + * Copyright (c) 2015 Imagination Technologies >> + * Author: Alex Smith >> + * >> + * This program is free software; you can redistribute it and/or modify it >> + * under the terms of the GNU General Public License version 2 as published >> + * by the Free Software Foundation. >> + */ >> + >> +#ifndef __DRIVERS_MTD_NAND_JZ4780_BCH_H__ >> +#define __DRIVERS_MTD_NAND_JZ4780_BCH_H__ >> + >> +#include >> + >> +struct device; >> +struct device_node; >> + >> +/** >> + * struct jz4780_bch_params - BCH parameters >> + * @size: data bytes per ECC step. >> + * @bytes: ECC bytes per step. >> + * @strength: number of correctable bits per ECC step. >> + */ >> +struct jz4780_bch_params { >> + int size; >> + int bytes; >> + int strength; >> +}; >> + >> +extern int jz4780_bch_calculate(struct device *dev, >> + struct jz4780_bch_params *params, >> + const uint8_t *buf, uint8_t *ecc_code); >> +extern int jz4780_bch_correct(struct device *dev, >> + struct jz4780_bch_params *params, uint8_t *buf, >> + uint8_t *ecc_code); >> + >> +extern int jz4780_bch_get(struct device_node *np, struct device **dev); >> +extern void jz4780_bch_release(struct device *dev); >> + >> +#endif /* __DRIVERS_MTD_NAND_JZ4780_BCH_H__ */ >> diff --git a/drivers/mtd/nand/jz4780_nand.c b/drivers/mtd/nand/jz4780_nand.c >> new file mode 100644 >> index 0000000..b4d0acb >> --- /dev/null >> +++ b/drivers/mtd/nand/jz4780_nand.c >> @@ -0,0 +1,420 @@ >> +/* >> + * JZ4780 NAND driver >> + * >> + * Copyright (c) 2015 Imagination Technologies >> + * Author: Alex Smith >> + * >> + * This program is free software; you can redistribute it and/or modify it >> + * under the terms of the GNU General Public License version 2 as published >> + * by the Free Software Foundation. >> + */ >> + >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> + >> +#include >> + >> +#include "jz4780_bch.h" >> + >> +#define DRV_NAME "jz4780-nand" >> + >> +#define OFFSET_DATA 0x00000000 >> +#define OFFSET_CMD 0x00400000 >> +#define OFFSET_ADDR 0x00800000 >> + >> +/* Command delay when there is no R/B pin. */ >> +#define RB_DELAY_US 100 >> + >> +struct jz4780_nand_cs { >> + unsigned int bank; >> + void __iomem *base; >> +}; >> + >> +struct jz4780_nand_controller { >> + struct device *dev; >> + struct device *bch; >> + struct nand_hw_control controller; >> + unsigned int num_banks; >> + struct list_head chips; >> + int selected; >> + struct jz4780_nand_cs cs[]; >> +}; >> + >> +struct jz4780_nand_chip { >> + struct mtd_info mtd; >> + struct nand_chip chip; >> + struct list_head chip_list; >> + >> + struct nand_ecclayout ecclayout; >> + >> + struct gpio_desc *busy_gpio; >> + struct gpio_desc *wp_gpio; >> + unsigned int reading: 1; >> +}; >> + >> +static inline struct jz4780_nand_chip *to_jz4780_nand_chip(struct mtd_info *mtd) >> +{ >> + return container_of(mtd, struct jz4780_nand_chip, mtd); >> +} >> + >> +static inline struct jz4780_nand_controller *to_jz4780_nand_controller(struct nand_hw_control *ctrl) >> +{ >> + return container_of(ctrl, struct jz4780_nand_controller, controller); >> +} >> + >> +static void jz4780_nand_select_chip(struct mtd_info *mtd, int chipnr) >> +{ >> + struct jz4780_nand_chip *nand = to_jz4780_nand_chip(mtd); >> + struct jz4780_nand_controller *nfc = to_jz4780_nand_controller(nand->chip.controller); >> + struct jz4780_nand_cs *cs; >> + >> + if (chipnr == -1) { >> + /* Ensure the currently selected chip is deasserted. */ >> + if (nfc->selected >= 0) { >> + cs = &nfc->cs[nfc->selected]; >> + jz4780_nemc_assert(nfc->dev, cs->bank, false); >> + } >> + } else { >> + cs = &nfc->cs[chipnr]; >> + nand->chip.IO_ADDR_R = cs->base + OFFSET_DATA; >> + nand->chip.IO_ADDR_W = cs->base + OFFSET_DATA; > > IO_ADDR_R/IO_ADDR_W assignments should only be done once when > instantiating/initializing the nand_chip device... Ack. > >> + } >> + >> + nfc->selected = chipnr; >> +} >> + >> +static void jz4780_nand_cmd_ctrl(struct mtd_info *mtd, int cmd, >> + unsigned int ctrl) >> +{ >> + struct jz4780_nand_chip *nand = to_jz4780_nand_chip(mtd); >> + struct jz4780_nand_controller *nfc = to_jz4780_nand_controller(nand->chip.controller); >> + struct jz4780_nand_cs *cs; >> + >> + if (WARN_ON(nfc->selected < 0)) >> + return; >> + >> + cs = &nfc->cs[nfc->selected]; >> + >> + if (ctrl & NAND_CTRL_CHANGE) { >> + if (ctrl & NAND_ALE) >> + nand->chip.IO_ADDR_W = cs->base + OFFSET_ADDR; >> + else if (ctrl & NAND_CLE) >> + nand->chip.IO_ADDR_W = cs->base + OFFSET_CMD; >> + else >> + nand->chip.IO_ADDR_W = cs->base + OFFSET_DATA; >> + jz4780_nemc_assert(nfc->dev, cs->bank, ctrl & NAND_NCE); >> + } >> + >> + if (cmd != NAND_CMD_NONE) >> + writeb(cmd, nand->chip.IO_ADDR_W); >> +} > > ... and, as I said in my previous review, I don't think this IO_ADDR_W > pointer assignment dance is worth it. AFAICS, the only thing it is used > for are the read/write_byte/buf/word default implementation. > > The following code does exactly the same, and is, IMHO, clearer than > changing the IO_ADDR_W address depending on the operation. > > static void jz4780_nand_cmd_ctrl(struct mtd_info *mtd, int cmd, > unsigned int ctrl) > { > struct jz4780_nand_chip *nand = to_jz4780_nand_chip(mtd); > struct jz4780_nand_controller *nfc = to_jz4780_nand_controller(nand->chip.controller); > struct jz4780_nand_cs *cs; > > if (WARN_ON(nfc->selected < 0)) > return; > > cs = &nfc->cs[nfc->selected]; > > if (ctrl & NAND_CTRL_CHANGE) { > if (cmd != NAND_CMD_NONE) { > if (ctrl & NAND_ALE) > writeb(cmd, cs->base + OFFSET_ADDR); > else if (ctrl & NAND_CLE) > writeb(cmd, cs->base + OFFSET_CMD); > } > > jz4780_nemc_assert(nfc->dev, cs->bank, ctrl & NAND_NCE); > } > } > Okay, I understand your point now. I would also have to implement the read/write functions to replace the defaults, correct? If so, it feels strange to add functions to reimplement the default ones. >> + >> +static int jz4780_nand_dev_ready(struct mtd_info *mtd) >> +{ >> + struct jz4780_nand_chip *nand = to_jz4780_nand_chip(mtd); >> + >> + return !gpiod_get_value_cansleep(nand->busy_gpio); >> +} >> + >> +static void jz4780_nand_ecc_hwctl(struct mtd_info *mtd, int mode) >> +{ >> + struct jz4780_nand_chip *nand = to_jz4780_nand_chip(mtd); >> + >> + nand->reading = (mode == NAND_ECC_READ); >> +} >> + >> +static int jz4780_nand_ecc_calculate(struct mtd_info *mtd, const uint8_t *dat, >> + uint8_t *ecc_code) >> +{ >> + struct jz4780_nand_chip *nand = to_jz4780_nand_chip(mtd); >> + struct jz4780_nand_controller *nfc = to_jz4780_nand_controller(nand->chip.controller); >> + struct jz4780_bch_params params; >> + >> + /* >> + * Don't need to generate the ECC when reading, BCH does it for us as >> + * part of decoding/correction. >> + */ >> + if (nand->reading) >> + return 0; >> + >> + params.size = nand->chip.ecc.size; >> + params.bytes = nand->chip.ecc.bytes; >> + params.strength = nand->chip.ecc.strength; >> + >> + return jz4780_bch_calculate(nfc->bch, ¶ms, dat, ecc_code); >> +} >> + >> +static int jz4780_nand_ecc_correct(struct mtd_info *mtd, uint8_t *dat, >> + uint8_t *read_ecc, uint8_t *calc_ecc) >> +{ >> + struct jz4780_nand_chip *nand = to_jz4780_nand_chip(mtd); >> + struct jz4780_nand_controller *nfc = to_jz4780_nand_controller(nand->chip.controller); >> + struct jz4780_bch_params params; >> + >> + params.size = nand->chip.ecc.size; >> + params.bytes = nand->chip.ecc.bytes; >> + params.strength = nand->chip.ecc.strength; >> + >> + return jz4780_bch_correct(nfc->bch, ¶ms, dat, read_ecc); >> +} >> + >> +static int jz4780_nand_init_ecc(struct jz4780_nand_chip *nand, struct device *dev) >> +{ >> + struct mtd_info *mtd = &nand->mtd; >> + struct nand_chip *chip = &nand->chip; >> + struct jz4780_nand_controller *nfc = to_jz4780_nand_controller(nand->chip.controller); >> + struct nand_ecclayout *layout = &nand->ecclayout; >> + uint32_t start, i; >> + >> + chip->ecc.bytes = fls((1 + 8) * chip->ecc.size) * >> + (chip->ecc.strength / 8); >> + >> + if (nfc->bch && chip->ecc.mode == NAND_ECC_HW) { >> + chip->ecc.hwctl = jz4780_nand_ecc_hwctl; >> + chip->ecc.calculate = jz4780_nand_ecc_calculate; >> + chip->ecc.correct = jz4780_nand_ecc_correct; >> + } else if (!nfc->bch && chip->ecc.mode == NAND_ECC_HW) { >> + dev_err(dev, "HW BCH selected, but BCH controller not found\n"); >> + return -ENODEV; >> + } >> + >> + if (chip->ecc.mode != NAND_ECC_NONE) >> + dev_info(dev, "using %s BCH (strength %d, size %d, bytes %d)\n", > > if mode == NAND_ECC_SOFT we're not using BCH but hamming. Maybe you > don't have to be so specific. Just saying that you're using software > or hardware ECC should be enough. Okay, I'll fix that. > >> + (nfc->bch) ? "hardware" : "software", chip->ecc.strength, >> + chip->ecc.size, chip->ecc.bytes); >> + else >> + dev_info(dev, "not using ECC\n"); >> + >> + /* The NAND core will generate the ECC layout. */ >> + if (chip->ecc.mode == NAND_ECC_SOFT || chip->ecc.mode == NAND_ECC_SOFT_BCH) >> + return 0; >> + >> + /* Generate ECC layout. ECC codes are right aligned in the OOB area. */ >> + layout->eccbytes = mtd->writesize / chip->ecc.size * chip->ecc.bytes; > > You don't seem to check if the number of eccbytes fit into the available > oobsize. > > if (layout->eccbytes > mtd->oobsize - 2) { > dev_err(dev, > "invalid ECC config: required %d ECC bytes, but only %d are available", > layout->eccbytes, mtd->oobsize - 2); > return -EINVAL; > } I'll add this check. > >> + start = mtd->oobsize - layout->eccbytes; >> + for (i = 0; i < layout->eccbytes; i++) >> + layout->eccpos[i] = start + i; >> + >> + layout->oobfree[0].offset = 2; >> + layout->oobfree[0].length = mtd->oobsize - layout->eccbytes - 2; >> + >> + chip->ecc.layout = layout; >> + return 0; >> +} >> + >> +static int jz4780_nand_init_chip(struct platform_device *pdev, >> + struct jz4780_nand_controller *nfc, >> + struct device_node *np, >> + unsigned int chipnr) >> +{ >> + struct device *dev = &pdev->dev; >> + struct jz4780_nand_chip *nand; >> + struct jz4780_nand_cs *cs; >> + struct resource *res; >> + struct nand_chip *chip; >> + struct mtd_info *mtd; >> + const __be32 *reg; >> + struct mtd_part_parser_data ppdata; >> + int ret = 0; >> + >> + cs = &nfc->cs[chipnr]; >> + >> + reg = of_get_property(np, "reg", NULL); >> + if (reg == NULL) >> + return -EINVAL; >> + >> + cs->bank = be32_to_cpu(*reg); >> + >> + jz4780_nemc_set_type(nfc->dev, cs->bank, JZ4780_NEMC_BANK_NAND); >> + >> + res = platform_get_resource(pdev, IORESOURCE_MEM, chipnr); >> + cs->base = devm_ioremap_resource(dev, res); >> + if (IS_ERR(cs->base)) >> + return PTR_ERR(cs->base); >> + >> + nand = devm_kzalloc(dev, sizeof(*nand), GFP_KERNEL); >> + if (!nand) >> + return -ENOMEM; >> + >> + nand->busy_gpio = devm_gpiod_get_optional(dev, "rb", GPIOD_IN); >> + >> + if (IS_ERR(nand->busy_gpio)) { >> + ret = PTR_ERR(nand->busy_gpio); >> + dev_err(dev, "failed to request busy GPIO: %d\n", ret); >> + return ret; >> + } else if (nand->busy_gpio) { >> + nand->chip.dev_ready = jz4780_nand_dev_ready; >> + } >> + >> + nand->wp_gpio = devm_gpiod_get_optional(dev, "wp", GPIOD_OUT_LOW); >> + >> + if (IS_ERR(nand->wp_gpio)) { >> + ret = PTR_ERR(nand->wp_gpio); >> + dev_err(dev, "failed to request WP GPIO: %d\n", ret); >> + return ret; >> + } >> + >> + mtd = &nand->mtd; >> + chip = &nand->chip; >> + mtd->priv = chip; >> + mtd->owner = THIS_MODULE; >> + mtd->name = DRV_NAME; >> + mtd->dev.parent = dev; >> + >> + chip->flash_node = np; > > Use the recently introduced > nand_set_flash_node(chip, np); Okay, will do. As mentioned on IRC, I'll rebase onto l2-mtd. > >> + chip->chip_delay = RB_DELAY_US; >> + chip->options = NAND_NO_SUBPAGE_WRITE; >> + chip->select_chip = jz4780_nand_select_chip; >> + chip->cmd_ctrl = jz4780_nand_cmd_ctrl; >> + chip->ecc.mode = NAND_ECC_HW; >> + chip->controller = &nfc->controller; >> + >> + ret = nand_scan_ident(mtd, 1, NULL); >> + if (ret) >> + return ret; >> + >> + ret = jz4780_nand_init_ecc(nand, dev); >> + if (ret) >> + return ret; >> + >> + ret = nand_scan_tail(mtd); >> + if (ret) >> + goto err_release_bch; >> + >> + ppdata.of_node = np; >> + ret = mtd_device_parse_register(mtd, NULL, &ppdata, NULL, 0); > > This has recently changed: you don't have to pass the of_node through > the pdata structure anymore, it is automatically extracted from the > mtd device if you've used nand_set_flash_node(). > > Replace this call by > > ret = mtd_device_register(mtd, NULL, 0); That looks much neater, I'll switch to that for the next version. > > >> + if (ret) >> + goto err_release_nand; >> + >> + return 0; >> + >> +err_release_nand: >> + nand_release(mtd); >> + >> +err_release_bch: >> + if (nfc->bch) >> + jz4780_bch_release(nfc->bch); >> + >> + return ret; >> +} >> + >> +static int jz4780_nand_init_chips(struct jz4780_nand_controller *nfc, >> + struct platform_device *pdev) >> +{ >> + struct device *dev = &pdev->dev; >> + struct device_node *np; >> + int i = 0; >> + int ret; >> + int num_chips = of_get_child_count(dev->of_node); >> + >> + if (num_chips > nfc->num_banks) { >> + dev_err(dev, "found %d chips but only %d banks\n", num_chips, nfc->num_banks); >> + return -EINVAL; >> + } >> + >> + for_each_child_of_node(dev->of_node, np) { >> + ret = jz4780_nand_init_chip(pdev, nfc, np, i); >> + if (ret) >> + return ret; >> + >> + i++; >> + } >> + >> + return 0; >> +} >> + >> + >> +static int jz4780_nand_probe(struct platform_device *pdev) >> +{ >> + struct device *dev = &pdev->dev; >> + unsigned int num_banks; >> + struct jz4780_nand_controller *nfc; >> + struct device_node *bch_np; >> + int ret; >> + >> + num_banks = jz4780_nemc_num_banks(dev); >> + if (num_banks == 0) { >> + dev_err(dev, "no banks found\n"); >> + return -ENODEV; >> + } >> + >> + nfc = devm_kzalloc(dev, sizeof(*nfc) + (sizeof(nfc->cs[0]) * num_banks), GFP_KERNEL); >> + if (!nfc) >> + return -ENOMEM; >> + >> + /* >> + * Check for BCH HW before we call nand_scan_ident, to prevent us from >> + * having to call it again if the BCH driver returns -EPROBE_DEFER. >> + */ >> + bch_np = of_parse_phandle(dev->of_node, >> + "ingenic,bch-controller", 0); >> + if (bch_np) { >> + ret = jz4780_bch_get(bch_np, &nfc->bch); >> + of_node_put(bch_np); >> + if (ret) >> + return ret; >> + } > > This should probably be part of the API exposed by the jz4780_bch driver. > Something like: > > struct jz4780_bch *of_jz4780_bch_get(struct device_node *np) > { > ... > } > > You could even provide a devm_ variant to simplify the cleanup path... I agree, it'll look nicer - I'll make that change also. > > That's all I got for now :-). > BTW, thanks for reworking the driver to match the controller/chip > model. > No problem. :-) Thanks again, Harvey -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/