From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-6.8 required=3.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, INCLUDES_PATCH,MAILING_LIST_MULTI,SIGNED_OFF_BY,SPF_PASS,URIBL_BLOCKED autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 9B124C67863 for ; Thu, 18 Oct 2018 19:33:18 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 579612083A for ; Thu, 18 Oct 2018 19:33:18 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 579612083A Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=bootlin.com Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1727026AbeJSDfp (ORCPT ); Thu, 18 Oct 2018 23:35:45 -0400 Received: from mail.bootlin.com ([62.4.15.54]:45167 "EHLO mail.bootlin.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726465AbeJSDfp (ORCPT ); Thu, 18 Oct 2018 23:35:45 -0400 Received: by mail.bootlin.com (Postfix, from userid 110) id A09C5209DA; Thu, 18 Oct 2018 21:33:13 +0200 (CEST) Received: from bbrezillon (unknown [91.160.177.164]) by mail.bootlin.com (Postfix) with ESMTPSA id 1BF89207C4; Thu, 18 Oct 2018 21:33:13 +0200 (CEST) Date: Thu, 18 Oct 2018 21:33:08 +0200 From: Boris Brezillon To: Jianxin Pan Cc: , Rob Herring , Hanjie Lin , Victor Wan , Neil Armstrong , Martin Blumenstingl , Richard Weinberger , Yixun Lan , linux-kernel@vger.kernel.org, Marek Vasut , Liang Yang , Jian Hu , Kevin Hilman , Carlo Caione , linux-amlogic@lists.infradead.org, Brian Norris , David Woodhouse , linux-arm-kernel@lists.infradead.org, Jerome Brunet Subject: Re: [PATCH v5 2/2] mtd: rawnand: meson: add support for Amlogic NAND flash controller Message-ID: <20181018213308.3f8a5279@bbrezillon> In-Reply-To: <1539839345-14021-3-git-send-email-jianxin.pan@amlogic.com> References: <1539839345-14021-1-git-send-email-jianxin.pan@amlogic.com> <1539839345-14021-3-git-send-email-jianxin.pan@amlogic.com> X-Mailer: Claws Mail 3.15.0-dirty (GTK+ 2.24.31; x86_64-pc-linux-gnu) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, 18 Oct 2018 13:09:05 +0800 Jianxin Pan wrote: > From: Liang Yang > > Add initial support for the Amlogic NAND flash controller which found > in the Meson-GXBB/GXL/AXG SoCs. > > Signed-off-by: Liang Yang > Signed-off-by: Yixun Lan > Signed-off-by: Jianxin Pan > --- > drivers/mtd/nand/raw/Kconfig | 10 + > drivers/mtd/nand/raw/Makefile | 1 + > drivers/mtd/nand/raw/meson_nand.c | 1370 +++++++++++++++++++++++++++++++++++++ > 3 files changed, 1381 insertions(+) > create mode 100644 drivers/mtd/nand/raw/meson_nand.c > > diff --git a/drivers/mtd/nand/raw/Kconfig b/drivers/mtd/nand/raw/Kconfig > index c7efc31..223b041 100644 > --- a/drivers/mtd/nand/raw/Kconfig > +++ b/drivers/mtd/nand/raw/Kconfig > @@ -541,4 +541,14 @@ config MTD_NAND_TEGRA > is supported. Extra OOB bytes when using HW ECC are currently > not supported. > > +config MTD_NAND_MESON > + tristate "Support for NAND controller on Amlogic's Meson SoCs" > + depends on ARCH_MESON || COMPILE_TEST > + depends on COMMON_CLK_AMLOGIC > + select COMMON_CLK_REGMAP_MESON > + select MFD_SYSCON > + help > + Enables support for NAND controller on Amlogic's Meson SoCs. > + This controller is found on Meson GXBB, GXL, AXG SoCs. > + > endif # MTD_NAND > diff --git a/drivers/mtd/nand/raw/Makefile b/drivers/mtd/nand/raw/Makefile > index 57159b3..a2cc2fe 100644 > --- a/drivers/mtd/nand/raw/Makefile > +++ b/drivers/mtd/nand/raw/Makefile > @@ -56,6 +56,7 @@ obj-$(CONFIG_MTD_NAND_BRCMNAND) += brcmnand/ > obj-$(CONFIG_MTD_NAND_QCOM) += qcom_nandc.o > obj-$(CONFIG_MTD_NAND_MTK) += mtk_ecc.o mtk_nand.o > obj-$(CONFIG_MTD_NAND_TEGRA) += tegra_nand.o > +obj-$(CONFIG_MTD_NAND_MESON) += meson_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/meson_nand.c b/drivers/mtd/nand/raw/meson_nand.c > new file mode 100644 > index 0000000..bcaac53 > --- /dev/null > +++ b/drivers/mtd/nand/raw/meson_nand.c > @@ -0,0 +1,1370 @@ > +// SPDX-License-Identifier: (GPL-2.0+ OR MIT) > +/* > + * Amlogic Meson Nand Flash Controller Driver > + * > + * Copyright (c) 2018 Amlogic, inc. > + * Author: Liang Yang > + */ > + > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > + > +#define NFC_REG_CMD 0x00 > +#define NFC_CMD_DRD (0x8 << 14) > +#define NFC_CMD_IDLE (0xc << 14) > +#define NFC_CMD_DWR (0x4 << 14) > +#define NFC_CMD_CLE (0x5 << 14) > +#define NFC_CMD_ALE (0x6 << 14) > +#define NFC_CMD_ADL ((0 << 16) | (3 << 20)) > +#define NFC_CMD_ADH ((1 << 16) | (3 << 20)) > +#define NFC_CMD_AIL ((2 << 16) | (3 << 20)) > +#define NFC_CMD_AIH ((3 << 16) | (3 << 20)) > +#define NFC_CMD_SEED ((8 << 16) | (3 << 20)) > +#define NFC_CMD_M2N ((0 << 17) | (2 << 20)) > +#define NFC_CMD_N2M ((1 << 17) | (2 << 20)) > +#define NFC_CMD_RB BIT(20) > +#define NFC_CMD_IO6 ((0xb << 10) | (1 << 18)) > + > +#define NFC_REG_CFG 0x04 > +#define NFC_REG_DADR 0x08 > +#define NFC_REG_IADR 0x0c > +#define NFC_REG_BUF 0x10 > +#define NFC_REG_INFO 0x14 > +#define NFC_REG_DC 0x18 > +#define NFC_REG_ADR 0x1c > +#define NFC_REG_DL 0x20 > +#define NFC_REG_DH 0x24 > +#define NFC_REG_CADR 0x28 > +#define NFC_REG_SADR 0x2c > +#define NFC_REG_PINS 0x30 > +#define NFC_REG_VER 0x38 > + > +#define NFC_RB_IRQ_EN BIT(21) > +#define NFC_INT_MASK (3 << 20) > + > +#define CMDRWGEN(cmd_dir, ran, bch, short_mode, page_size, pages) \ > + ( \ > + (cmd_dir) | \ > + ((ran) << 19) | \ > + ((bch) << 14) | \ > + ((short_mode) << 13) | \ > + (((page_size) & 0x7f) << 6) | \ > + ((pages) & 0x3f) \ > + ) > + > +#define GENCMDDADDRL(adl, addr) ((adl) | ((addr) & 0xffff)) > +#define GENCMDDADDRH(adh, addr) ((adh) | (((addr) >> 16) & 0xffff)) > +#define GENCMDIADDRL(ail, addr) ((ail) | ((addr) & 0xffff)) > +#define GENCMDIADDRH(aih, addr) ((aih) | (((addr) >> 16) & 0xffff)) > + > +#define RB_STA(x) (1 << (26 + (x))) > +#define DMA_DIR(dir) ((dir) ? NFC_CMD_N2M : NFC_CMD_M2N) > + > +#define ECC_CHECK_RETURN_FF (-1) > + > +#define NAND_CE0 (0xe << 10) > +#define NAND_CE1 (0xd << 10) > + > +#define DMA_BUSY_TIMEOUT 0x100000 > +#define CMD_FIFO_EMPTY_TIMEOUT 1000 > + > +#define MAX_CE_NUM 2 > + > +/* eMMC clock register, misc control */ > +#define SD_EMMC_CLOCK 0x00 > +#define CLK_ALWAYS_ON BIT(28) > +#define CLK_SELECT_NAND BIT(31) > +#define CLK_DIV_MASK GENMASK(5, 0) > + > +#define NFC_CLK_CYCLE 6 > + > +/* nand flash controller delay 3 ns */ > +#define NFC_DEFAULT_DELAY 3000 > + > +#define MAX_ECC_INDEX 10 > + > +#define MUX_CLK_NUM_PARENTS 2 > + > +#define ROW_ADDER(page, index) (((page) >> (8 * (index))) & 0xff) > +#define MAX_CYCLE_ROW_ADDRS 3 > +#define MAX_CYCLE_COLUMN_ADDRS 2 > +#define DIRREAD 1 > +#define DIRWRITE 0 > + > +#define ECC_PARITY_BCH8_512B 14 > + > +struct meson_nfc_info_format { > + u16 info_bytes; > + > + /* bit[5:0] are valid */ > + u8 zero_cnt; > + struct ecc_sta { > + u8 eccerr_cnt : 6; > + u8 notused : 1; > + u8 completed : 1; > + } ecc; It's usually a bad idea to use bitfields for things like HW regs/descriptors fields because it's hard to tell where the bits are actually placed (not even sure the C standard defines how this should be done). > + u32 reserved; > +}; How about defining that the HW returns an array of __le64 instead and then define the following macros which you can use after converting in the CPU endianness #define ECC_GET_PROTECTED_OOB_BYTE(x, y) (((x) >> (8 * (1 + y)) & GENMASK(7, 0)) #define ECC_COMPLETE BIT(31) #define ECC_ERR_CNT(x) (((x) >> 24) & GENMASK(5, 0)) (I'm not entirely sure the field positions are correct, but I'll let you check that). > + > +#define PER_INFO_BYTE (sizeof(struct meson_nfc_info_format)) > + > +struct meson_nfc_nand_chip { > + struct list_head node; > + struct nand_chip nand; > + bool is_scramble; I think I already mentioned the NAND_NEED_SCRAMBLING flag []. Please drop this field and test (chip->flags & NAND_NEED_SCRAMBLING) instead. > + int bch_mode; > + int nsels; > + u8 sels[0]; > +}; > + > +struct meson_nand_ecc { > + int bch; > + int strength; > +}; > + > +struct meson_nfc_data { > + const struct nand_ecc_caps *ecc_caps; > +}; > + > +struct meson_nfc_param { > + int chip_select; > + int rb_select; > +}; > + > +struct nand_rw_cmd { > + int cmd0; > + int col[MAX_CYCLE_COLUMN_ADDRS]; > + int row[MAX_CYCLE_ROW_ADDRS]; > + int cmd1; > +}; > + > +struct nand_timing { > + int twb; > + int tadl; > + int twhr; > +}; > + > +struct meson_nfc { > + struct nand_controller controller; > + struct clk *core_clk; > + struct clk *device_clk; > + struct clk *phase_tx; > + struct clk *phase_rx; > + > + struct device *dev; > + void __iomem *reg_base; > + struct regmap *reg_clk; > + struct completion completion; > + struct list_head chips; > + const struct meson_nfc_data *data; > + struct meson_nfc_param param; > + struct nand_timing timing; > + union { > + int cmd[32]; > + struct nand_rw_cmd rw; > + } cmdfifo; > + > + dma_addr_t data_dma; > + dma_addr_t info_dma; > + > + unsigned long assigned_cs; > + > + u8 *data_buf; > + u8 *info_buf; > +}; > + > +enum { > + NFC_ECC_BCH8_1K = 2, > + NFC_ECC_BCH24_1K, > + NFC_ECC_BCH30_1K, > + NFC_ECC_BCH40_1K, > + NFC_ECC_BCH50_1K, > + NFC_ECC_BCH60_1K, > +}; > + > +#define MESON_ECC_DATA(b, s) { .bch = (b), .strength = (s)} > + > +static int meson_nand_calc_ecc_bytes(int step_size, int strength) > +{ > + int ecc_bytes; > + > + if (step_size == 512 && strength == 8) > + return ECC_PARITY_BCH8_512B; > + > + ecc_bytes = DIV_ROUND_UP(strength * fls(step_size * 8), 8); > + if (ecc_bytes % 2) > + ecc_bytes++; Just a tiny detail, but this can be replaced by ALIGN(ecc_bytes, 2). > + > + return ecc_bytes; > +} > + > +NAND_ECC_CAPS_SINGLE(meson_gxl_ecc_caps, > + meson_nand_calc_ecc_bytes, 1024, 8, 24, 30, 40, 50, 60); > +NAND_ECC_CAPS_SINGLE(meson_axg_ecc_caps, > + meson_nand_calc_ecc_bytes, 1024, 8); > + > +static inline > +struct meson_nfc_nand_chip *to_meson_nand(struct nand_chip *nand) > +{ > + return container_of(nand, struct meson_nfc_nand_chip, nand); > +} > + > +static void meson_nfc_select_chip(struct nand_chip *nand, int chip) > +{ > + struct meson_nfc_nand_chip *meson_chip = to_meson_nand(nand); > + struct meson_nfc *nfc = nand_get_controller_data(nand); > + > + if (chip < 0 || chip > MAX_CE_NUM) You can add a WARN_ON_ONCE() around chip > MAX_CE_NUM, because this should never happen. > + return; > + > + nfc->param.chip_select = meson_chip->sels[chip] ? NAND_CE1 : NAND_CE0; > + nfc->param.rb_select = nfc->param.chip_select; > +} > + > +static inline void meson_nfc_cmd_idle(struct meson_nfc *nfc, u32 time) > +{ > + writel(nfc->param.chip_select | NFC_CMD_IDLE | (time & 0x3ff), > + nfc->reg_base + NFC_REG_CMD); > +} > + > +static inline void meson_nfc_cmd_seed(struct meson_nfc *nfc, u32 seed) > +{ > + writel(NFC_CMD_SEED | (0xc2 + (seed & 0x7fff)), > + nfc->reg_base + NFC_REG_CMD); > +} > + > +static void meson_nfc_cmd_access(struct meson_nfc *nfc, > + struct mtd_info *mtd, int raw, bool dir) Just pass a nand_chip or a meson_nfc_nand_chip object here, nfc and mtd can be extracted from there. > +{ > + struct nand_chip *nand = mtd_to_nand(mtd); > + struct meson_nfc_nand_chip *meson_chip = to_meson_nand(nand); > + u32 cmd, pagesize, pages; > + int bch = meson_chip->bch_mode; > + int len = mtd->writesize; > + int scramble = meson_chip->is_scramble ? 1 : 0; > + > + pagesize = nand->ecc.size; > + > + if (raw) { > + bch = NAND_ECC_NONE; You set bch to NAND_ECC_NONE but never use the variable afterwards, is that normal? > + len = mtd->writesize + mtd->oobsize; > + cmd = (len & 0x3fff) | (scramble << 19) | DMA_DIR(dir); Please use a macro to describe bit 19: #define NFC_CMD_SCRAMBLER_ENABLE BIT(19) > + writel(cmd, nfc->reg_base + NFC_REG_CMD); > + return; > + } > + > + pages = len / nand->ecc.size; > + > + cmd = CMDRWGEN(DMA_DIR(dir), scramble, bch, 0, pagesize, pages); > + > + writel(cmd, nfc->reg_base + NFC_REG_CMD); > +} > + > +static inline void meson_nfc_drain_cmd(struct meson_nfc *nfc) > +{ > + /* > + * Insert two commands to make sure all valid commands are finished. > + * > + * The Nand flash controller is designed as two stages pipleline - > + * a) fetch and b) excute. > + * There might be cases when the driver see command queue is empty, > + * but the Nand flash controller still has two commands buffered, > + * one is fetched into NFC request queue (ready to run), and another > + * is actively executing. So pushing 2 "IDLE" commands guarantees that > + * the pipeline is emptied. > + */ > + meson_nfc_cmd_idle(nfc, 0); > + meson_nfc_cmd_idle(nfc, 0); > +} > + > +static int meson_nfc_wait_cmd_finish(struct meson_nfc *nfc, > + unsigned int timeout_ms) > +{ > + u32 cmd_size = 0; > + int ret; > + > + /* wait cmd fifo is empty */ > + ret = readl_poll_timeout(nfc->reg_base + NFC_REG_CMD, cmd_size, > + !((cmd_size >> 22) & 0x1f), Define a macro to extract the cmd_size: #define NFC_CMD_GET_SIZE(x) (((x) >> 22) & GENMASK(4, 0)) > + 10, timeout_ms * 1000); > + if (ret) > + dev_err(nfc->dev, "wait for empty cmd FIFO time out\n"); > + > + return ret; > +} > + > +static int meson_nfc_wait_dma_finish(struct meson_nfc *nfc) > +{ > + meson_nfc_drain_cmd(nfc); > + > + return meson_nfc_wait_cmd_finish(nfc, DMA_BUSY_TIMEOUT); > +} > + > +static inline > +struct meson_nfc_info_format *nfc_info_ptr(struct meson_nfc *nfc, > + int index) > +{ > + return (struct meson_nfc_info_format *)&nfc->info_buf[index * 8]; As said previously, I think ->info_buf should be an array of __le64, and all you should do here is return le64_to_cpu(nfc->info_buf[index]); Then you can use the macros defined above to extract the results you need. > +} > + > +static u8 *meson_nfc_oob_ptr(struct meson_nfc *nfc, > + struct mtd_info *mtd, int i) > +{ > + struct nand_chip *nand = mtd_to_nand(mtd); > + int len; > + > + len = nand->ecc.size * (i + 1) + (nand->ecc.bytes + 2) * i; > + > + return nfc->data_buf + len; > +} > + > +static u8 *meson_nfc_data_ptr(struct meson_nfc *nfc, > + struct mtd_info *mtd, int i) > +{ > + struct nand_chip *nand = mtd_to_nand(mtd); > + int len; > + int temp; > + > + temp = nand->ecc.size + nand->ecc.bytes; > + len = (temp + 2) * i; > + > + return nfc->data_buf + len; > +} > + > +static void meson_nfc_prase_data_oob(struct meson_nfc *nfc, What does prase mean? Maybe _set_ and _get_ would be better that _prase_ and _format_. > + struct mtd_info *mtd, u8 *buf, u8 *oobbuf) As said previously, please stop passing mtd objects around. Same goes for nfc if you already have to pass a nand_chip object. > +{ > + struct nand_chip *nand = mtd_to_nand(mtd); > + int i, oob_len = 0; > + u8 *dsrc, *osrc; > + > + for (i = 0; i < mtd->writesize / nand->ecc.size; i++) { You have ecc->steps for that, no need to do the division everytime. > + if (buf) { > + dsrc = meson_nfc_data_ptr(nfc, mtd, i); > + memcpy(buf, dsrc, nand->ecc.size); > + buf += nand->ecc.size; > + } > + oob_len = nand->ecc.bytes + 2; Looks like oob_len does not change, so you can move the oob_len assignment outside of this loop. > + osrc = meson_nfc_oob_ptr(nfc, mtd, i); > + memcpy(oobbuf, osrc, oob_len); > + oobbuf += oob_len; > + } > +} > + > +static void meson_nfc_format_data_oob(struct meson_nfc *nfc, > + struct mtd_info *mtd, > + const u8 *buf, u8 *oobbuf) > +{ > + struct nand_chip *nand = mtd_to_nand(mtd); > + int i, oob_len = 0; > + u8 *dsrc, *osrc; > + > + for (i = 0; i < mtd->writesize / nand->ecc.size; i++) { > + if (buf) { > + dsrc = meson_nfc_data_ptr(nfc, mtd, i); > + memcpy(dsrc, buf, nand->ecc.size); > + buf += nand->ecc.size; > + } > + oob_len = nand->ecc.bytes + 2; > + osrc = meson_nfc_oob_ptr(nfc, mtd, i); > + memcpy(osrc, oobbuf, oob_len); > + oobbuf += oob_len; > + } > +} > + > +static int meson_nfc_queue_rb(struct meson_nfc *nfc, int timeout_ms) > +{ > + u32 cmd, cfg; > + int ret = 0; > + > + meson_nfc_cmd_idle(nfc, nfc->timing.twb); > + meson_nfc_drain_cmd(nfc); > + meson_nfc_wait_cmd_finish(nfc, CMD_FIFO_EMPTY_TIMEOUT); > + > + cfg = readl(nfc->reg_base + NFC_REG_CFG); > + cfg |= (1 << 21); > + writel(cfg, nfc->reg_base + NFC_REG_CFG); > + > + init_completion(&nfc->completion); > + > + cmd = NFC_CMD_RB | (0x1 << 14) | nfc->param.chip_select | 0x18; Please define macros for all those magic values (0x18 and 1 << 14) > + writel(cmd, nfc->reg_base + NFC_REG_CMD); > + > + ret = wait_for_completion_timeout(&nfc->completion, > + msecs_to_jiffies(timeout_ms)); > + if (ret == 0) { > + dev_err(nfc->dev, "wait nand irq timeout\n"); > + ret = -1; -ETIMEDOUT; > + } > + return ret; > +} > + > +static void meson_nfc_set_user_byte(struct mtd_info *mtd, > + struct nand_chip *chip, u8 *oob_buf) > +{ > + struct meson_nfc *nfc = nand_get_controller_data(chip); > + struct meson_nfc_info_format *info; > + int i, count; > + > + for (i = 0, count = 0; i < chip->ecc.steps; i++, count += 2) { > + info = nfc_info_ptr(nfc, i); > + info->info_bytes = > + oob_buf[count] | (oob_buf[count + 1] << 8); Use a macro to set/get protected OOB bytes. > + } > +} > + From mboxrd@z Thu Jan 1 00:00:00 1970 From: boris.brezillon@bootlin.com (Boris Brezillon) Date: Thu, 18 Oct 2018 21:33:08 +0200 Subject: [PATCH v5 2/2] mtd: rawnand: meson: add support for Amlogic NAND flash controller In-Reply-To: <1539839345-14021-3-git-send-email-jianxin.pan@amlogic.com> References: <1539839345-14021-1-git-send-email-jianxin.pan@amlogic.com> <1539839345-14021-3-git-send-email-jianxin.pan@amlogic.com> Message-ID: <20181018213308.3f8a5279@bbrezillon> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On Thu, 18 Oct 2018 13:09:05 +0800 Jianxin Pan wrote: > From: Liang Yang > > Add initial support for the Amlogic NAND flash controller which found > in the Meson-GXBB/GXL/AXG SoCs. > > Signed-off-by: Liang Yang > Signed-off-by: Yixun Lan > Signed-off-by: Jianxin Pan > --- > drivers/mtd/nand/raw/Kconfig | 10 + > drivers/mtd/nand/raw/Makefile | 1 + > drivers/mtd/nand/raw/meson_nand.c | 1370 +++++++++++++++++++++++++++++++++++++ > 3 files changed, 1381 insertions(+) > create mode 100644 drivers/mtd/nand/raw/meson_nand.c > > diff --git a/drivers/mtd/nand/raw/Kconfig b/drivers/mtd/nand/raw/Kconfig > index c7efc31..223b041 100644 > --- a/drivers/mtd/nand/raw/Kconfig > +++ b/drivers/mtd/nand/raw/Kconfig > @@ -541,4 +541,14 @@ config MTD_NAND_TEGRA > is supported. Extra OOB bytes when using HW ECC are currently > not supported. > > +config MTD_NAND_MESON > + tristate "Support for NAND controller on Amlogic's Meson SoCs" > + depends on ARCH_MESON || COMPILE_TEST > + depends on COMMON_CLK_AMLOGIC > + select COMMON_CLK_REGMAP_MESON > + select MFD_SYSCON > + help > + Enables support for NAND controller on Amlogic's Meson SoCs. > + This controller is found on Meson GXBB, GXL, AXG SoCs. > + > endif # MTD_NAND > diff --git a/drivers/mtd/nand/raw/Makefile b/drivers/mtd/nand/raw/Makefile > index 57159b3..a2cc2fe 100644 > --- a/drivers/mtd/nand/raw/Makefile > +++ b/drivers/mtd/nand/raw/Makefile > @@ -56,6 +56,7 @@ obj-$(CONFIG_MTD_NAND_BRCMNAND) += brcmnand/ > obj-$(CONFIG_MTD_NAND_QCOM) += qcom_nandc.o > obj-$(CONFIG_MTD_NAND_MTK) += mtk_ecc.o mtk_nand.o > obj-$(CONFIG_MTD_NAND_TEGRA) += tegra_nand.o > +obj-$(CONFIG_MTD_NAND_MESON) += meson_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/meson_nand.c b/drivers/mtd/nand/raw/meson_nand.c > new file mode 100644 > index 0000000..bcaac53 > --- /dev/null > +++ b/drivers/mtd/nand/raw/meson_nand.c > @@ -0,0 +1,1370 @@ > +// SPDX-License-Identifier: (GPL-2.0+ OR MIT) > +/* > + * Amlogic Meson Nand Flash Controller Driver > + * > + * Copyright (c) 2018 Amlogic, inc. > + * Author: Liang Yang > + */ > + > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > + > +#define NFC_REG_CMD 0x00 > +#define NFC_CMD_DRD (0x8 << 14) > +#define NFC_CMD_IDLE (0xc << 14) > +#define NFC_CMD_DWR (0x4 << 14) > +#define NFC_CMD_CLE (0x5 << 14) > +#define NFC_CMD_ALE (0x6 << 14) > +#define NFC_CMD_ADL ((0 << 16) | (3 << 20)) > +#define NFC_CMD_ADH ((1 << 16) | (3 << 20)) > +#define NFC_CMD_AIL ((2 << 16) | (3 << 20)) > +#define NFC_CMD_AIH ((3 << 16) | (3 << 20)) > +#define NFC_CMD_SEED ((8 << 16) | (3 << 20)) > +#define NFC_CMD_M2N ((0 << 17) | (2 << 20)) > +#define NFC_CMD_N2M ((1 << 17) | (2 << 20)) > +#define NFC_CMD_RB BIT(20) > +#define NFC_CMD_IO6 ((0xb << 10) | (1 << 18)) > + > +#define NFC_REG_CFG 0x04 > +#define NFC_REG_DADR 0x08 > +#define NFC_REG_IADR 0x0c > +#define NFC_REG_BUF 0x10 > +#define NFC_REG_INFO 0x14 > +#define NFC_REG_DC 0x18 > +#define NFC_REG_ADR 0x1c > +#define NFC_REG_DL 0x20 > +#define NFC_REG_DH 0x24 > +#define NFC_REG_CADR 0x28 > +#define NFC_REG_SADR 0x2c > +#define NFC_REG_PINS 0x30 > +#define NFC_REG_VER 0x38 > + > +#define NFC_RB_IRQ_EN BIT(21) > +#define NFC_INT_MASK (3 << 20) > + > +#define CMDRWGEN(cmd_dir, ran, bch, short_mode, page_size, pages) \ > + ( \ > + (cmd_dir) | \ > + ((ran) << 19) | \ > + ((bch) << 14) | \ > + ((short_mode) << 13) | \ > + (((page_size) & 0x7f) << 6) | \ > + ((pages) & 0x3f) \ > + ) > + > +#define GENCMDDADDRL(adl, addr) ((adl) | ((addr) & 0xffff)) > +#define GENCMDDADDRH(adh, addr) ((adh) | (((addr) >> 16) & 0xffff)) > +#define GENCMDIADDRL(ail, addr) ((ail) | ((addr) & 0xffff)) > +#define GENCMDIADDRH(aih, addr) ((aih) | (((addr) >> 16) & 0xffff)) > + > +#define RB_STA(x) (1 << (26 + (x))) > +#define DMA_DIR(dir) ((dir) ? NFC_CMD_N2M : NFC_CMD_M2N) > + > +#define ECC_CHECK_RETURN_FF (-1) > + > +#define NAND_CE0 (0xe << 10) > +#define NAND_CE1 (0xd << 10) > + > +#define DMA_BUSY_TIMEOUT 0x100000 > +#define CMD_FIFO_EMPTY_TIMEOUT 1000 > + > +#define MAX_CE_NUM 2 > + > +/* eMMC clock register, misc control */ > +#define SD_EMMC_CLOCK 0x00 > +#define CLK_ALWAYS_ON BIT(28) > +#define CLK_SELECT_NAND BIT(31) > +#define CLK_DIV_MASK GENMASK(5, 0) > + > +#define NFC_CLK_CYCLE 6 > + > +/* nand flash controller delay 3 ns */ > +#define NFC_DEFAULT_DELAY 3000 > + > +#define MAX_ECC_INDEX 10 > + > +#define MUX_CLK_NUM_PARENTS 2 > + > +#define ROW_ADDER(page, index) (((page) >> (8 * (index))) & 0xff) > +#define MAX_CYCLE_ROW_ADDRS 3 > +#define MAX_CYCLE_COLUMN_ADDRS 2 > +#define DIRREAD 1 > +#define DIRWRITE 0 > + > +#define ECC_PARITY_BCH8_512B 14 > + > +struct meson_nfc_info_format { > + u16 info_bytes; > + > + /* bit[5:0] are valid */ > + u8 zero_cnt; > + struct ecc_sta { > + u8 eccerr_cnt : 6; > + u8 notused : 1; > + u8 completed : 1; > + } ecc; It's usually a bad idea to use bitfields for things like HW regs/descriptors fields because it's hard to tell where the bits are actually placed (not even sure the C standard defines how this should be done). > + u32 reserved; > +}; How about defining that the HW returns an array of __le64 instead and then define the following macros which you can use after converting in the CPU endianness #define ECC_GET_PROTECTED_OOB_BYTE(x, y) (((x) >> (8 * (1 + y)) & GENMASK(7, 0)) #define ECC_COMPLETE BIT(31) #define ECC_ERR_CNT(x) (((x) >> 24) & GENMASK(5, 0)) (I'm not entirely sure the field positions are correct, but I'll let you check that). > + > +#define PER_INFO_BYTE (sizeof(struct meson_nfc_info_format)) > + > +struct meson_nfc_nand_chip { > + struct list_head node; > + struct nand_chip nand; > + bool is_scramble; I think I already mentioned the NAND_NEED_SCRAMBLING flag []. Please drop this field and test (chip->flags & NAND_NEED_SCRAMBLING) instead. > + int bch_mode; > + int nsels; > + u8 sels[0]; > +}; > + > +struct meson_nand_ecc { > + int bch; > + int strength; > +}; > + > +struct meson_nfc_data { > + const struct nand_ecc_caps *ecc_caps; > +}; > + > +struct meson_nfc_param { > + int chip_select; > + int rb_select; > +}; > + > +struct nand_rw_cmd { > + int cmd0; > + int col[MAX_CYCLE_COLUMN_ADDRS]; > + int row[MAX_CYCLE_ROW_ADDRS]; > + int cmd1; > +}; > + > +struct nand_timing { > + int twb; > + int tadl; > + int twhr; > +}; > + > +struct meson_nfc { > + struct nand_controller controller; > + struct clk *core_clk; > + struct clk *device_clk; > + struct clk *phase_tx; > + struct clk *phase_rx; > + > + struct device *dev; > + void __iomem *reg_base; > + struct regmap *reg_clk; > + struct completion completion; > + struct list_head chips; > + const struct meson_nfc_data *data; > + struct meson_nfc_param param; > + struct nand_timing timing; > + union { > + int cmd[32]; > + struct nand_rw_cmd rw; > + } cmdfifo; > + > + dma_addr_t data_dma; > + dma_addr_t info_dma; > + > + unsigned long assigned_cs; > + > + u8 *data_buf; > + u8 *info_buf; > +}; > + > +enum { > + NFC_ECC_BCH8_1K = 2, > + NFC_ECC_BCH24_1K, > + NFC_ECC_BCH30_1K, > + NFC_ECC_BCH40_1K, > + NFC_ECC_BCH50_1K, > + NFC_ECC_BCH60_1K, > +}; > + > +#define MESON_ECC_DATA(b, s) { .bch = (b), .strength = (s)} > + > +static int meson_nand_calc_ecc_bytes(int step_size, int strength) > +{ > + int ecc_bytes; > + > + if (step_size == 512 && strength == 8) > + return ECC_PARITY_BCH8_512B; > + > + ecc_bytes = DIV_ROUND_UP(strength * fls(step_size * 8), 8); > + if (ecc_bytes % 2) > + ecc_bytes++; Just a tiny detail, but this can be replaced by ALIGN(ecc_bytes, 2). > + > + return ecc_bytes; > +} > + > +NAND_ECC_CAPS_SINGLE(meson_gxl_ecc_caps, > + meson_nand_calc_ecc_bytes, 1024, 8, 24, 30, 40, 50, 60); > +NAND_ECC_CAPS_SINGLE(meson_axg_ecc_caps, > + meson_nand_calc_ecc_bytes, 1024, 8); > + > +static inline > +struct meson_nfc_nand_chip *to_meson_nand(struct nand_chip *nand) > +{ > + return container_of(nand, struct meson_nfc_nand_chip, nand); > +} > + > +static void meson_nfc_select_chip(struct nand_chip *nand, int chip) > +{ > + struct meson_nfc_nand_chip *meson_chip = to_meson_nand(nand); > + struct meson_nfc *nfc = nand_get_controller_data(nand); > + > + if (chip < 0 || chip > MAX_CE_NUM) You can add a WARN_ON_ONCE() around chip > MAX_CE_NUM, because this should never happen. > + return; > + > + nfc->param.chip_select = meson_chip->sels[chip] ? NAND_CE1 : NAND_CE0; > + nfc->param.rb_select = nfc->param.chip_select; > +} > + > +static inline void meson_nfc_cmd_idle(struct meson_nfc *nfc, u32 time) > +{ > + writel(nfc->param.chip_select | NFC_CMD_IDLE | (time & 0x3ff), > + nfc->reg_base + NFC_REG_CMD); > +} > + > +static inline void meson_nfc_cmd_seed(struct meson_nfc *nfc, u32 seed) > +{ > + writel(NFC_CMD_SEED | (0xc2 + (seed & 0x7fff)), > + nfc->reg_base + NFC_REG_CMD); > +} > + > +static void meson_nfc_cmd_access(struct meson_nfc *nfc, > + struct mtd_info *mtd, int raw, bool dir) Just pass a nand_chip or a meson_nfc_nand_chip object here, nfc and mtd can be extracted from there. > +{ > + struct nand_chip *nand = mtd_to_nand(mtd); > + struct meson_nfc_nand_chip *meson_chip = to_meson_nand(nand); > + u32 cmd, pagesize, pages; > + int bch = meson_chip->bch_mode; > + int len = mtd->writesize; > + int scramble = meson_chip->is_scramble ? 1 : 0; > + > + pagesize = nand->ecc.size; > + > + if (raw) { > + bch = NAND_ECC_NONE; You set bch to NAND_ECC_NONE but never use the variable afterwards, is that normal? > + len = mtd->writesize + mtd->oobsize; > + cmd = (len & 0x3fff) | (scramble << 19) | DMA_DIR(dir); Please use a macro to describe bit 19: #define NFC_CMD_SCRAMBLER_ENABLE BIT(19) > + writel(cmd, nfc->reg_base + NFC_REG_CMD); > + return; > + } > + > + pages = len / nand->ecc.size; > + > + cmd = CMDRWGEN(DMA_DIR(dir), scramble, bch, 0, pagesize, pages); > + > + writel(cmd, nfc->reg_base + NFC_REG_CMD); > +} > + > +static inline void meson_nfc_drain_cmd(struct meson_nfc *nfc) > +{ > + /* > + * Insert two commands to make sure all valid commands are finished. > + * > + * The Nand flash controller is designed as two stages pipleline - > + * a) fetch and b) excute. > + * There might be cases when the driver see command queue is empty, > + * but the Nand flash controller still has two commands buffered, > + * one is fetched into NFC request queue (ready to run), and another > + * is actively executing. So pushing 2 "IDLE" commands guarantees that > + * the pipeline is emptied. > + */ > + meson_nfc_cmd_idle(nfc, 0); > + meson_nfc_cmd_idle(nfc, 0); > +} > + > +static int meson_nfc_wait_cmd_finish(struct meson_nfc *nfc, > + unsigned int timeout_ms) > +{ > + u32 cmd_size = 0; > + int ret; > + > + /* wait cmd fifo is empty */ > + ret = readl_poll_timeout(nfc->reg_base + NFC_REG_CMD, cmd_size, > + !((cmd_size >> 22) & 0x1f), Define a macro to extract the cmd_size: #define NFC_CMD_GET_SIZE(x) (((x) >> 22) & GENMASK(4, 0)) > + 10, timeout_ms * 1000); > + if (ret) > + dev_err(nfc->dev, "wait for empty cmd FIFO time out\n"); > + > + return ret; > +} > + > +static int meson_nfc_wait_dma_finish(struct meson_nfc *nfc) > +{ > + meson_nfc_drain_cmd(nfc); > + > + return meson_nfc_wait_cmd_finish(nfc, DMA_BUSY_TIMEOUT); > +} > + > +static inline > +struct meson_nfc_info_format *nfc_info_ptr(struct meson_nfc *nfc, > + int index) > +{ > + return (struct meson_nfc_info_format *)&nfc->info_buf[index * 8]; As said previously, I think ->info_buf should be an array of __le64, and all you should do here is return le64_to_cpu(nfc->info_buf[index]); Then you can use the macros defined above to extract the results you need. > +} > + > +static u8 *meson_nfc_oob_ptr(struct meson_nfc *nfc, > + struct mtd_info *mtd, int i) > +{ > + struct nand_chip *nand = mtd_to_nand(mtd); > + int len; > + > + len = nand->ecc.size * (i + 1) + (nand->ecc.bytes + 2) * i; > + > + return nfc->data_buf + len; > +} > + > +static u8 *meson_nfc_data_ptr(struct meson_nfc *nfc, > + struct mtd_info *mtd, int i) > +{ > + struct nand_chip *nand = mtd_to_nand(mtd); > + int len; > + int temp; > + > + temp = nand->ecc.size + nand->ecc.bytes; > + len = (temp + 2) * i; > + > + return nfc->data_buf + len; > +} > + > +static void meson_nfc_prase_data_oob(struct meson_nfc *nfc, What does prase mean? Maybe _set_ and _get_ would be better that _prase_ and _format_. > + struct mtd_info *mtd, u8 *buf, u8 *oobbuf) As said previously, please stop passing mtd objects around. Same goes for nfc if you already have to pass a nand_chip object. > +{ > + struct nand_chip *nand = mtd_to_nand(mtd); > + int i, oob_len = 0; > + u8 *dsrc, *osrc; > + > + for (i = 0; i < mtd->writesize / nand->ecc.size; i++) { You have ecc->steps for that, no need to do the division everytime. > + if (buf) { > + dsrc = meson_nfc_data_ptr(nfc, mtd, i); > + memcpy(buf, dsrc, nand->ecc.size); > + buf += nand->ecc.size; > + } > + oob_len = nand->ecc.bytes + 2; Looks like oob_len does not change, so you can move the oob_len assignment outside of this loop. > + osrc = meson_nfc_oob_ptr(nfc, mtd, i); > + memcpy(oobbuf, osrc, oob_len); > + oobbuf += oob_len; > + } > +} > + > +static void meson_nfc_format_data_oob(struct meson_nfc *nfc, > + struct mtd_info *mtd, > + const u8 *buf, u8 *oobbuf) > +{ > + struct nand_chip *nand = mtd_to_nand(mtd); > + int i, oob_len = 0; > + u8 *dsrc, *osrc; > + > + for (i = 0; i < mtd->writesize / nand->ecc.size; i++) { > + if (buf) { > + dsrc = meson_nfc_data_ptr(nfc, mtd, i); > + memcpy(dsrc, buf, nand->ecc.size); > + buf += nand->ecc.size; > + } > + oob_len = nand->ecc.bytes + 2; > + osrc = meson_nfc_oob_ptr(nfc, mtd, i); > + memcpy(osrc, oobbuf, oob_len); > + oobbuf += oob_len; > + } > +} > + > +static int meson_nfc_queue_rb(struct meson_nfc *nfc, int timeout_ms) > +{ > + u32 cmd, cfg; > + int ret = 0; > + > + meson_nfc_cmd_idle(nfc, nfc->timing.twb); > + meson_nfc_drain_cmd(nfc); > + meson_nfc_wait_cmd_finish(nfc, CMD_FIFO_EMPTY_TIMEOUT); > + > + cfg = readl(nfc->reg_base + NFC_REG_CFG); > + cfg |= (1 << 21); > + writel(cfg, nfc->reg_base + NFC_REG_CFG); > + > + init_completion(&nfc->completion); > + > + cmd = NFC_CMD_RB | (0x1 << 14) | nfc->param.chip_select | 0x18; Please define macros for all those magic values (0x18 and 1 << 14) > + writel(cmd, nfc->reg_base + NFC_REG_CMD); > + > + ret = wait_for_completion_timeout(&nfc->completion, > + msecs_to_jiffies(timeout_ms)); > + if (ret == 0) { > + dev_err(nfc->dev, "wait nand irq timeout\n"); > + ret = -1; -ETIMEDOUT; > + } > + return ret; > +} > + > +static void meson_nfc_set_user_byte(struct mtd_info *mtd, > + struct nand_chip *chip, u8 *oob_buf) > +{ > + struct meson_nfc *nfc = nand_get_controller_data(chip); > + struct meson_nfc_info_format *info; > + int i, count; > + > + for (i = 0, count = 0; i < chip->ecc.steps; i++, count += 2) { > + info = nfc_info_ptr(nfc, i); > + info->info_bytes = > + oob_buf[count] | (oob_buf[count + 1] << 8); Use a macro to set/get protected OOB bytes. > + } > +} > + From mboxrd@z Thu Jan 1 00:00:00 1970 From: boris.brezillon@bootlin.com (Boris Brezillon) Date: Thu, 18 Oct 2018 21:33:08 +0200 Subject: [PATCH v5 2/2] mtd: rawnand: meson: add support for Amlogic NAND flash controller In-Reply-To: <1539839345-14021-3-git-send-email-jianxin.pan@amlogic.com> References: <1539839345-14021-1-git-send-email-jianxin.pan@amlogic.com> <1539839345-14021-3-git-send-email-jianxin.pan@amlogic.com> Message-ID: <20181018213308.3f8a5279@bbrezillon> To: linus-amlogic@lists.infradead.org List-Id: linus-amlogic.lists.infradead.org On Thu, 18 Oct 2018 13:09:05 +0800 Jianxin Pan wrote: > From: Liang Yang > > Add initial support for the Amlogic NAND flash controller which found > in the Meson-GXBB/GXL/AXG SoCs. > > Signed-off-by: Liang Yang > Signed-off-by: Yixun Lan > Signed-off-by: Jianxin Pan > --- > drivers/mtd/nand/raw/Kconfig | 10 + > drivers/mtd/nand/raw/Makefile | 1 + > drivers/mtd/nand/raw/meson_nand.c | 1370 +++++++++++++++++++++++++++++++++++++ > 3 files changed, 1381 insertions(+) > create mode 100644 drivers/mtd/nand/raw/meson_nand.c > > diff --git a/drivers/mtd/nand/raw/Kconfig b/drivers/mtd/nand/raw/Kconfig > index c7efc31..223b041 100644 > --- a/drivers/mtd/nand/raw/Kconfig > +++ b/drivers/mtd/nand/raw/Kconfig > @@ -541,4 +541,14 @@ config MTD_NAND_TEGRA > is supported. Extra OOB bytes when using HW ECC are currently > not supported. > > +config MTD_NAND_MESON > + tristate "Support for NAND controller on Amlogic's Meson SoCs" > + depends on ARCH_MESON || COMPILE_TEST > + depends on COMMON_CLK_AMLOGIC > + select COMMON_CLK_REGMAP_MESON > + select MFD_SYSCON > + help > + Enables support for NAND controller on Amlogic's Meson SoCs. > + This controller is found on Meson GXBB, GXL, AXG SoCs. > + > endif # MTD_NAND > diff --git a/drivers/mtd/nand/raw/Makefile b/drivers/mtd/nand/raw/Makefile > index 57159b3..a2cc2fe 100644 > --- a/drivers/mtd/nand/raw/Makefile > +++ b/drivers/mtd/nand/raw/Makefile > @@ -56,6 +56,7 @@ obj-$(CONFIG_MTD_NAND_BRCMNAND) += brcmnand/ > obj-$(CONFIG_MTD_NAND_QCOM) += qcom_nandc.o > obj-$(CONFIG_MTD_NAND_MTK) += mtk_ecc.o mtk_nand.o > obj-$(CONFIG_MTD_NAND_TEGRA) += tegra_nand.o > +obj-$(CONFIG_MTD_NAND_MESON) += meson_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/meson_nand.c b/drivers/mtd/nand/raw/meson_nand.c > new file mode 100644 > index 0000000..bcaac53 > --- /dev/null > +++ b/drivers/mtd/nand/raw/meson_nand.c > @@ -0,0 +1,1370 @@ > +// SPDX-License-Identifier: (GPL-2.0+ OR MIT) > +/* > + * Amlogic Meson Nand Flash Controller Driver > + * > + * Copyright (c) 2018 Amlogic, inc. > + * Author: Liang Yang > + */ > + > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > + > +#define NFC_REG_CMD 0x00 > +#define NFC_CMD_DRD (0x8 << 14) > +#define NFC_CMD_IDLE (0xc << 14) > +#define NFC_CMD_DWR (0x4 << 14) > +#define NFC_CMD_CLE (0x5 << 14) > +#define NFC_CMD_ALE (0x6 << 14) > +#define NFC_CMD_ADL ((0 << 16) | (3 << 20)) > +#define NFC_CMD_ADH ((1 << 16) | (3 << 20)) > +#define NFC_CMD_AIL ((2 << 16) | (3 << 20)) > +#define NFC_CMD_AIH ((3 << 16) | (3 << 20)) > +#define NFC_CMD_SEED ((8 << 16) | (3 << 20)) > +#define NFC_CMD_M2N ((0 << 17) | (2 << 20)) > +#define NFC_CMD_N2M ((1 << 17) | (2 << 20)) > +#define NFC_CMD_RB BIT(20) > +#define NFC_CMD_IO6 ((0xb << 10) | (1 << 18)) > + > +#define NFC_REG_CFG 0x04 > +#define NFC_REG_DADR 0x08 > +#define NFC_REG_IADR 0x0c > +#define NFC_REG_BUF 0x10 > +#define NFC_REG_INFO 0x14 > +#define NFC_REG_DC 0x18 > +#define NFC_REG_ADR 0x1c > +#define NFC_REG_DL 0x20 > +#define NFC_REG_DH 0x24 > +#define NFC_REG_CADR 0x28 > +#define NFC_REG_SADR 0x2c > +#define NFC_REG_PINS 0x30 > +#define NFC_REG_VER 0x38 > + > +#define NFC_RB_IRQ_EN BIT(21) > +#define NFC_INT_MASK (3 << 20) > + > +#define CMDRWGEN(cmd_dir, ran, bch, short_mode, page_size, pages) \ > + ( \ > + (cmd_dir) | \ > + ((ran) << 19) | \ > + ((bch) << 14) | \ > + ((short_mode) << 13) | \ > + (((page_size) & 0x7f) << 6) | \ > + ((pages) & 0x3f) \ > + ) > + > +#define GENCMDDADDRL(adl, addr) ((adl) | ((addr) & 0xffff)) > +#define GENCMDDADDRH(adh, addr) ((adh) | (((addr) >> 16) & 0xffff)) > +#define GENCMDIADDRL(ail, addr) ((ail) | ((addr) & 0xffff)) > +#define GENCMDIADDRH(aih, addr) ((aih) | (((addr) >> 16) & 0xffff)) > + > +#define RB_STA(x) (1 << (26 + (x))) > +#define DMA_DIR(dir) ((dir) ? NFC_CMD_N2M : NFC_CMD_M2N) > + > +#define ECC_CHECK_RETURN_FF (-1) > + > +#define NAND_CE0 (0xe << 10) > +#define NAND_CE1 (0xd << 10) > + > +#define DMA_BUSY_TIMEOUT 0x100000 > +#define CMD_FIFO_EMPTY_TIMEOUT 1000 > + > +#define MAX_CE_NUM 2 > + > +/* eMMC clock register, misc control */ > +#define SD_EMMC_CLOCK 0x00 > +#define CLK_ALWAYS_ON BIT(28) > +#define CLK_SELECT_NAND BIT(31) > +#define CLK_DIV_MASK GENMASK(5, 0) > + > +#define NFC_CLK_CYCLE 6 > + > +/* nand flash controller delay 3 ns */ > +#define NFC_DEFAULT_DELAY 3000 > + > +#define MAX_ECC_INDEX 10 > + > +#define MUX_CLK_NUM_PARENTS 2 > + > +#define ROW_ADDER(page, index) (((page) >> (8 * (index))) & 0xff) > +#define MAX_CYCLE_ROW_ADDRS 3 > +#define MAX_CYCLE_COLUMN_ADDRS 2 > +#define DIRREAD 1 > +#define DIRWRITE 0 > + > +#define ECC_PARITY_BCH8_512B 14 > + > +struct meson_nfc_info_format { > + u16 info_bytes; > + > + /* bit[5:0] are valid */ > + u8 zero_cnt; > + struct ecc_sta { > + u8 eccerr_cnt : 6; > + u8 notused : 1; > + u8 completed : 1; > + } ecc; It's usually a bad idea to use bitfields for things like HW regs/descriptors fields because it's hard to tell where the bits are actually placed (not even sure the C standard defines how this should be done). > + u32 reserved; > +}; How about defining that the HW returns an array of __le64 instead and then define the following macros which you can use after converting in the CPU endianness #define ECC_GET_PROTECTED_OOB_BYTE(x, y) (((x) >> (8 * (1 + y)) & GENMASK(7, 0)) #define ECC_COMPLETE BIT(31) #define ECC_ERR_CNT(x) (((x) >> 24) & GENMASK(5, 0)) (I'm not entirely sure the field positions are correct, but I'll let you check that). > + > +#define PER_INFO_BYTE (sizeof(struct meson_nfc_info_format)) > + > +struct meson_nfc_nand_chip { > + struct list_head node; > + struct nand_chip nand; > + bool is_scramble; I think I already mentioned the NAND_NEED_SCRAMBLING flag []. Please drop this field and test (chip->flags & NAND_NEED_SCRAMBLING) instead. > + int bch_mode; > + int nsels; > + u8 sels[0]; > +}; > + > +struct meson_nand_ecc { > + int bch; > + int strength; > +}; > + > +struct meson_nfc_data { > + const struct nand_ecc_caps *ecc_caps; > +}; > + > +struct meson_nfc_param { > + int chip_select; > + int rb_select; > +}; > + > +struct nand_rw_cmd { > + int cmd0; > + int col[MAX_CYCLE_COLUMN_ADDRS]; > + int row[MAX_CYCLE_ROW_ADDRS]; > + int cmd1; > +}; > + > +struct nand_timing { > + int twb; > + int tadl; > + int twhr; > +}; > + > +struct meson_nfc { > + struct nand_controller controller; > + struct clk *core_clk; > + struct clk *device_clk; > + struct clk *phase_tx; > + struct clk *phase_rx; > + > + struct device *dev; > + void __iomem *reg_base; > + struct regmap *reg_clk; > + struct completion completion; > + struct list_head chips; > + const struct meson_nfc_data *data; > + struct meson_nfc_param param; > + struct nand_timing timing; > + union { > + int cmd[32]; > + struct nand_rw_cmd rw; > + } cmdfifo; > + > + dma_addr_t data_dma; > + dma_addr_t info_dma; > + > + unsigned long assigned_cs; > + > + u8 *data_buf; > + u8 *info_buf; > +}; > + > +enum { > + NFC_ECC_BCH8_1K = 2, > + NFC_ECC_BCH24_1K, > + NFC_ECC_BCH30_1K, > + NFC_ECC_BCH40_1K, > + NFC_ECC_BCH50_1K, > + NFC_ECC_BCH60_1K, > +}; > + > +#define MESON_ECC_DATA(b, s) { .bch = (b), .strength = (s)} > + > +static int meson_nand_calc_ecc_bytes(int step_size, int strength) > +{ > + int ecc_bytes; > + > + if (step_size == 512 && strength == 8) > + return ECC_PARITY_BCH8_512B; > + > + ecc_bytes = DIV_ROUND_UP(strength * fls(step_size * 8), 8); > + if (ecc_bytes % 2) > + ecc_bytes++; Just a tiny detail, but this can be replaced by ALIGN(ecc_bytes, 2). > + > + return ecc_bytes; > +} > + > +NAND_ECC_CAPS_SINGLE(meson_gxl_ecc_caps, > + meson_nand_calc_ecc_bytes, 1024, 8, 24, 30, 40, 50, 60); > +NAND_ECC_CAPS_SINGLE(meson_axg_ecc_caps, > + meson_nand_calc_ecc_bytes, 1024, 8); > + > +static inline > +struct meson_nfc_nand_chip *to_meson_nand(struct nand_chip *nand) > +{ > + return container_of(nand, struct meson_nfc_nand_chip, nand); > +} > + > +static void meson_nfc_select_chip(struct nand_chip *nand, int chip) > +{ > + struct meson_nfc_nand_chip *meson_chip = to_meson_nand(nand); > + struct meson_nfc *nfc = nand_get_controller_data(nand); > + > + if (chip < 0 || chip > MAX_CE_NUM) You can add a WARN_ON_ONCE() around chip > MAX_CE_NUM, because this should never happen. > + return; > + > + nfc->param.chip_select = meson_chip->sels[chip] ? NAND_CE1 : NAND_CE0; > + nfc->param.rb_select = nfc->param.chip_select; > +} > + > +static inline void meson_nfc_cmd_idle(struct meson_nfc *nfc, u32 time) > +{ > + writel(nfc->param.chip_select | NFC_CMD_IDLE | (time & 0x3ff), > + nfc->reg_base + NFC_REG_CMD); > +} > + > +static inline void meson_nfc_cmd_seed(struct meson_nfc *nfc, u32 seed) > +{ > + writel(NFC_CMD_SEED | (0xc2 + (seed & 0x7fff)), > + nfc->reg_base + NFC_REG_CMD); > +} > + > +static void meson_nfc_cmd_access(struct meson_nfc *nfc, > + struct mtd_info *mtd, int raw, bool dir) Just pass a nand_chip or a meson_nfc_nand_chip object here, nfc and mtd can be extracted from there. > +{ > + struct nand_chip *nand = mtd_to_nand(mtd); > + struct meson_nfc_nand_chip *meson_chip = to_meson_nand(nand); > + u32 cmd, pagesize, pages; > + int bch = meson_chip->bch_mode; > + int len = mtd->writesize; > + int scramble = meson_chip->is_scramble ? 1 : 0; > + > + pagesize = nand->ecc.size; > + > + if (raw) { > + bch = NAND_ECC_NONE; You set bch to NAND_ECC_NONE but never use the variable afterwards, is that normal? > + len = mtd->writesize + mtd->oobsize; > + cmd = (len & 0x3fff) | (scramble << 19) | DMA_DIR(dir); Please use a macro to describe bit 19: #define NFC_CMD_SCRAMBLER_ENABLE BIT(19) > + writel(cmd, nfc->reg_base + NFC_REG_CMD); > + return; > + } > + > + pages = len / nand->ecc.size; > + > + cmd = CMDRWGEN(DMA_DIR(dir), scramble, bch, 0, pagesize, pages); > + > + writel(cmd, nfc->reg_base + NFC_REG_CMD); > +} > + > +static inline void meson_nfc_drain_cmd(struct meson_nfc *nfc) > +{ > + /* > + * Insert two commands to make sure all valid commands are finished. > + * > + * The Nand flash controller is designed as two stages pipleline - > + * a) fetch and b) excute. > + * There might be cases when the driver see command queue is empty, > + * but the Nand flash controller still has two commands buffered, > + * one is fetched into NFC request queue (ready to run), and another > + * is actively executing. So pushing 2 "IDLE" commands guarantees that > + * the pipeline is emptied. > + */ > + meson_nfc_cmd_idle(nfc, 0); > + meson_nfc_cmd_idle(nfc, 0); > +} > + > +static int meson_nfc_wait_cmd_finish(struct meson_nfc *nfc, > + unsigned int timeout_ms) > +{ > + u32 cmd_size = 0; > + int ret; > + > + /* wait cmd fifo is empty */ > + ret = readl_poll_timeout(nfc->reg_base + NFC_REG_CMD, cmd_size, > + !((cmd_size >> 22) & 0x1f), Define a macro to extract the cmd_size: #define NFC_CMD_GET_SIZE(x) (((x) >> 22) & GENMASK(4, 0)) > + 10, timeout_ms * 1000); > + if (ret) > + dev_err(nfc->dev, "wait for empty cmd FIFO time out\n"); > + > + return ret; > +} > + > +static int meson_nfc_wait_dma_finish(struct meson_nfc *nfc) > +{ > + meson_nfc_drain_cmd(nfc); > + > + return meson_nfc_wait_cmd_finish(nfc, DMA_BUSY_TIMEOUT); > +} > + > +static inline > +struct meson_nfc_info_format *nfc_info_ptr(struct meson_nfc *nfc, > + int index) > +{ > + return (struct meson_nfc_info_format *)&nfc->info_buf[index * 8]; As said previously, I think ->info_buf should be an array of __le64, and all you should do here is return le64_to_cpu(nfc->info_buf[index]); Then you can use the macros defined above to extract the results you need. > +} > + > +static u8 *meson_nfc_oob_ptr(struct meson_nfc *nfc, > + struct mtd_info *mtd, int i) > +{ > + struct nand_chip *nand = mtd_to_nand(mtd); > + int len; > + > + len = nand->ecc.size * (i + 1) + (nand->ecc.bytes + 2) * i; > + > + return nfc->data_buf + len; > +} > + > +static u8 *meson_nfc_data_ptr(struct meson_nfc *nfc, > + struct mtd_info *mtd, int i) > +{ > + struct nand_chip *nand = mtd_to_nand(mtd); > + int len; > + int temp; > + > + temp = nand->ecc.size + nand->ecc.bytes; > + len = (temp + 2) * i; > + > + return nfc->data_buf + len; > +} > + > +static void meson_nfc_prase_data_oob(struct meson_nfc *nfc, What does prase mean? Maybe _set_ and _get_ would be better that _prase_ and _format_. > + struct mtd_info *mtd, u8 *buf, u8 *oobbuf) As said previously, please stop passing mtd objects around. Same goes for nfc if you already have to pass a nand_chip object. > +{ > + struct nand_chip *nand = mtd_to_nand(mtd); > + int i, oob_len = 0; > + u8 *dsrc, *osrc; > + > + for (i = 0; i < mtd->writesize / nand->ecc.size; i++) { You have ecc->steps for that, no need to do the division everytime. > + if (buf) { > + dsrc = meson_nfc_data_ptr(nfc, mtd, i); > + memcpy(buf, dsrc, nand->ecc.size); > + buf += nand->ecc.size; > + } > + oob_len = nand->ecc.bytes + 2; Looks like oob_len does not change, so you can move the oob_len assignment outside of this loop. > + osrc = meson_nfc_oob_ptr(nfc, mtd, i); > + memcpy(oobbuf, osrc, oob_len); > + oobbuf += oob_len; > + } > +} > + > +static void meson_nfc_format_data_oob(struct meson_nfc *nfc, > + struct mtd_info *mtd, > + const u8 *buf, u8 *oobbuf) > +{ > + struct nand_chip *nand = mtd_to_nand(mtd); > + int i, oob_len = 0; > + u8 *dsrc, *osrc; > + > + for (i = 0; i < mtd->writesize / nand->ecc.size; i++) { > + if (buf) { > + dsrc = meson_nfc_data_ptr(nfc, mtd, i); > + memcpy(dsrc, buf, nand->ecc.size); > + buf += nand->ecc.size; > + } > + oob_len = nand->ecc.bytes + 2; > + osrc = meson_nfc_oob_ptr(nfc, mtd, i); > + memcpy(osrc, oobbuf, oob_len); > + oobbuf += oob_len; > + } > +} > + > +static int meson_nfc_queue_rb(struct meson_nfc *nfc, int timeout_ms) > +{ > + u32 cmd, cfg; > + int ret = 0; > + > + meson_nfc_cmd_idle(nfc, nfc->timing.twb); > + meson_nfc_drain_cmd(nfc); > + meson_nfc_wait_cmd_finish(nfc, CMD_FIFO_EMPTY_TIMEOUT); > + > + cfg = readl(nfc->reg_base + NFC_REG_CFG); > + cfg |= (1 << 21); > + writel(cfg, nfc->reg_base + NFC_REG_CFG); > + > + init_completion(&nfc->completion); > + > + cmd = NFC_CMD_RB | (0x1 << 14) | nfc->param.chip_select | 0x18; Please define macros for all those magic values (0x18 and 1 << 14) > + writel(cmd, nfc->reg_base + NFC_REG_CMD); > + > + ret = wait_for_completion_timeout(&nfc->completion, > + msecs_to_jiffies(timeout_ms)); > + if (ret == 0) { > + dev_err(nfc->dev, "wait nand irq timeout\n"); > + ret = -1; -ETIMEDOUT; > + } > + return ret; > +} > + > +static void meson_nfc_set_user_byte(struct mtd_info *mtd, > + struct nand_chip *chip, u8 *oob_buf) > +{ > + struct meson_nfc *nfc = nand_get_controller_data(chip); > + struct meson_nfc_info_format *info; > + int i, count; > + > + for (i = 0, count = 0; i < chip->ecc.steps; i++, count += 2) { > + info = nfc_info_ptr(nfc, i); > + info->info_bytes = > + oob_buf[count] | (oob_buf[count + 1] << 8); Use a macro to set/get protected OOB bytes. > + } > +} > +