Hi Miquel, Sorry for the late reply. On Mon, Oct 7, 2024 at 11:00 PM Miquel Raynal <miquel.raynal@xxxxxxxxxxx> wrote: > > Hi Keguang, > > devnull+keguang.zhang.gmail.com@xxxxxxxxxx wrote on Wed, 02 Oct 2024 > 16:10:46 +0800: > > > From: Keguang Zhang <keguang.zhang@xxxxxxxxx> > > > > Add NAND controller driver for Loongson-1 SoCs. > > > > Signed-off-by: Keguang Zhang <keguang.zhang@xxxxxxxxx> > > --- > > Changes in v10: > > - Fix the build error reported by kernel test robot. > > Link: https://lore.kernel.org/oe-kbuild-all/202409220010.vctkHddZ-lkp@xxxxxxxxx > > > > Changes in v9: > > - Change the compatible to 'loongson,ls1*-nand-controller'. > > - Update MAINTAINERS file accordingly. > > - Rebasing due to recent upstream changes. > > > > Changes in v8: > > - Drop NAND_MONOLITHIC_READ and add support for real subpage read instead. > > - Simplify the logic of ls1b_nand_parse_address() and ls1c_nand_parse_address(). > > - Split ls1x_nand_set_controller() into ls1x_nand_parse_instructions() > > and ls1x_nand_trigger_op(). > > - Implement ls1x_nand_op_cmd_mapping() to convert the opcodes instead of forcing them. > > - Add ls1x_nand_check_op(). > > - Remove struct ls1x_nand after moving its members to struct ls1x_nfc. > > - Add the prefix 'LS1X_' for all registers and their bits. > > - Drop the macros: nand_readl() and nand_writel(). > > - Some minor fixes and improvements. > > > > Changes in v7: > > - Rename the Kconfig dependency to LOONGSON1_APB_DMA > > > > Changes in v6: > > - Amend Kconfig > > - Add DT support > > - Use DT data instead of platform data > > - Remove MAX_ID_SIZE > > - Remove case NAND_OP_CMD_INSTR in ls1x_nand_set_controller() > > - Move ECC configuration to ls1x_nand_attach_chip() > > - Rename variable "nand" to "ls1x" > > - Rename variable "nc" to "nfc" > > - Some minor fixes > > - Link to v5: https://lore.kernel.org/all/20210520224213.7907-1-keguang.zhang@xxxxxxxxx > > > > Changes in v5: > > - Update the driver to fit the raw NAND framework. > > - Implement exec_op() instead of legacy cmdfunc(). > > - Use dma_request_chan() instead of dma_request_channel(). > > - Some minor fixes and cleanups. > > > > Changes in v4: > > - Retrieve the controller from nand_hw_control. > > > > Changes in v3: > > - Replace __raw_readl/__raw_writel with readl/writel. > > - Split ls1x_nand into two structures: > > ls1x_nand_chip and ls1x_nand_controller. > > > > Changes in v2: > > - Modify the dependency in Kconfig due to the changes of DMA module. > > --- > > MAINTAINERS | 1 + > > drivers/mtd/nand/raw/Kconfig | 7 + > > drivers/mtd/nand/raw/Makefile | 1 + > > drivers/mtd/nand/raw/loongson1_nand.c | 825 ++++++++++++++++++++++++++++++++++ > > 4 files changed, 834 insertions(+) > > > > diff --git a/MAINTAINERS b/MAINTAINERS > > index 94cb3186865f..10f5d329c625 100644 > > --- a/MAINTAINERS > > +++ b/MAINTAINERS > > @@ -15595,6 +15595,7 @@ F: Documentation/devicetree/bindings/*/loongson,ls1*.yaml > > F: arch/mips/include/asm/mach-loongson32/ > > F: arch/mips/loongson32/ > > F: drivers/*/*loongson1* > > +F: drivers/mtd/nand/raw/loongson1_nand.c > > F: drivers/net/ethernet/stmicro/stmmac/dwmac-loongson1.c > > > > MIPS/LOONGSON2EF ARCHITECTURE > > diff --git a/drivers/mtd/nand/raw/Kconfig b/drivers/mtd/nand/raw/Kconfig > > index d0aaccf72d78..54ad16a6a64e 100644 > > --- a/drivers/mtd/nand/raw/Kconfig > > +++ b/drivers/mtd/nand/raw/Kconfig > > @@ -454,6 +454,13 @@ config MTD_NAND_TS72XX > > help > > Enables support for NAND controller on ts72xx SBCs. > > > > +config MTD_NAND_LOONGSON1 > > + tristate "Loongson1 NAND controller" > > + depends on LOONGSON1_APB_DMA || COMPILE_TEST > > + select REGMAP_MMIO > > + help > > + Enables support for NAND controller on Loongson1 SoCs. > > + > > comment "Misc" > > > > config MTD_SM_COMMON > > diff --git a/drivers/mtd/nand/raw/Makefile b/drivers/mtd/nand/raw/Makefile > > index d0b0e6b83568..9ec0c38b4ee7 100644 > > --- a/drivers/mtd/nand/raw/Makefile > > +++ b/drivers/mtd/nand/raw/Makefile > > @@ -58,6 +58,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_LOONGSON1) += loongson1_nand.o > > loongson1-nand-controller.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/loongson1_nand.c b/drivers/mtd/nand/raw/loongson1_nand.c > > new file mode 100644 > > index 000000000000..89e8a048b1a5 > > --- /dev/null > > +++ b/drivers/mtd/nand/raw/loongson1_nand.c > > @@ -0,0 +1,825 @@ > > +// SPDX-License-Identifier: GPL-2.0-or-later > > +/* > > + * NAND Controller Driver for Loongson-1 SoC > > + * > > + * Copyright (C) 2015-2024 Keguang Zhang <keguang.zhang@xxxxxxxxx> > > + */ > > + > > +#include <linux/kernel.h> > > +#include <linux/module.h> > > +#include <linux/dmaengine.h> > > +#include <linux/dma-mapping.h> > > +#include <linux/iopoll.h> > > +#include <linux/mtd/mtd.h> > > +#include <linux/mtd/rawnand.h> > > +#include <linux/of.h> > > +#include <linux/platform_device.h> > > +#include <linux/regmap.h> > > +#include <linux/sizes.h> > > + > > +/* Loongson-1 NAND Controller Registers */ > > +#define LS1X_NAND_CMD 0x0 > > +#define LS1X_NAND_ADDR1 0x4 > > +#define LS1X_NAND_ADDR2 0x8 > > +#define LS1X_NAND_TIMING 0xc > > +#define LS1X_NAND_IDL 0x10 > > +#define LS1X_NAND_IDH_STATUS 0x14 > > +#define LS1X_NAND_PARAM 0x18 > > +#define LS1X_NAND_OP_NUM 0x1c > > + > > +/* NAND Command Register Bits */ > > +#define LS1X_NAND_CMD_OP_DONE BIT(10) > > +#define LS1X_NAND_CMD_OP_SPARE BIT(9) > > +#define LS1X_NAND_CMD_OP_MAIN BIT(8) > > +#define LS1X_NAND_CMD_STATUS BIT(7) > > +#define LS1X_NAND_CMD_RESET BIT(6) > > +#define LS1X_NAND_CMD_READID BIT(5) > > +#define LS1X_NAND_CMD_BLOCKS_ERASE BIT(4) > > +#define LS1X_NAND_CMD_ERASE BIT(3) > > +#define LS1X_NAND_CMD_WRITE BIT(2) > > +#define LS1X_NAND_CMD_READ BIT(1) > > +#define LS1X_NAND_CMD_VALID BIT(0) > > + > > +#define LS1X_NAND_CMD_OP_AREA_MASK GENMASK(9, 8) > > +#define LS1X_NAND_WAIT_CYCLE_MASK GENMASK(7, 0) > > +#define LS1X_NAND_HOLD_CYCLE_MASK GENMASK(15, 8) > > +#define LS1X_NAND_CELL_SIZE_MASK GENMASK(11, 8) > > + > > +#define LS1X_NAND_MAX_ADDR_CYC 5U > > +#define LS1X_NAND_DMA_ADDR 0x1fe78040 > > Please, never hardcode register physical values in a driver like that > :-) > > This is a resource you should get from DT. Furthermore, when using it > as DMA address you should first call dma_map_resource() on it. > Will do. > > + > > +#define BITS_PER_WORD (4 * BITS_PER_BYTE) > > + > > +struct ls1x_nfc_op { > > + char addrs[LS1X_NAND_MAX_ADDR_CYC]; > > + unsigned int naddrs; > > + unsigned int addrs_offset; > > + unsigned int addr1_reg; > > + unsigned int addr2_reg; > > + unsigned int aligned_offset; > > + unsigned int row_shift; > > + unsigned int cmd_reg; > > + unsigned int rdy_timeout_ms; > > + unsigned int len; > > + size_t dma_len; > > + bool restore_row; > > + bool is_write; > > + char *buf; > > +}; > > + > > +struct ls1x_nfc_data { > > + unsigned int status_field; > > + unsigned int op_scope_field; > > + unsigned int hold_cycle; > > + unsigned int wait_cycle; > > + void (*parse_address)(struct ls1x_nfc_op *op); > > +}; > > + > > +struct ls1x_nfc { > > + struct device *dev; > > + struct nand_chip chip; > > + struct nand_controller controller; > > + const struct ls1x_nfc_data *data; > > + void __iomem *reg_base; > > + struct regmap *regmap; > > + /* DMA Engine stuff */ > > + struct dma_chan *dma_chan; > > + dma_cookie_t dma_cookie; > > + struct completion dma_complete; > > +}; > > + > > +static const struct regmap_config ls1x_nand_regmap_config = { > > + .reg_bits = 32, > > + .val_bits = 32, > > + .reg_stride = 4, > > +}; > > + > > +static int ls1x_nand_op_cmd_mapping(struct nand_chip *chip, > > + struct ls1x_nfc_op *op, u8 opcode) > > +{ > > + struct ls1x_nfc *nfc = nand_get_controller_data(chip); > > + int ret = 0; > > + > > + op->row_shift = chip->page_shift + 1; > > Looks like you're hardcoding the third row for large nands? > What about using chip->options & NAND_ROW_ADDR_3 instead? > No, this is not about handling the third row. It involves the start bits of the row address in LS1X_NAND_ADDR1. Sorry for the misleading name. I will rename 'row_shift' to 'row_start'. > > + > > + /* The controller abstracts the following NAND operations. */ > > + switch (opcode) { > > + case NAND_CMD_RESET: > > + op->cmd_reg = LS1X_NAND_CMD_RESET; > > + break; > > + case NAND_CMD_READID: > > + op->cmd_reg = LS1X_NAND_CMD_READID; > > + break; > > + case NAND_CMD_ERASE1: > > + case NAND_CMD_ERASE2: > > + op->cmd_reg = LS1X_NAND_CMD_ERASE; > > + op->addrs_offset = 2; > > + op->row_shift = chip->page_shift; > > + break; > > + case NAND_CMD_STATUS: > > + op->cmd_reg = LS1X_NAND_CMD_STATUS; > > + break; > > + case NAND_CMD_SEQIN: > > + case NAND_CMD_PAGEPROG: > > This is typically something that scares me. Mapping two distinct > commands to a single register value. How can this be valid? You should > never guess what the core wants to do. You have all the operation, so > if you want to map two commands to a single register value, then please > check what you do is valid and do not iterate blindly across commands > like that. Otherwise please error out if that's unsupported. > > Please carefully try to describe all possible cases and error what when > they are not supported. > > Also, based on this feedback, your check_op() function might need a > little bit of polishing. > Will improve this function and check_op(). > > + op->cmd_reg = LS1X_NAND_CMD_WRITE; > > + op->is_write = true; > > + break; > > + case NAND_CMD_RNDOUT: > > + case NAND_CMD_RNDOUTSTART: > > + op->restore_row = true; > > + fallthrough; > > + case NAND_CMD_READ0: > > + case NAND_CMD_READSTART: > > + op->cmd_reg = LS1X_NAND_CMD_READ; > > + break; > > + default: > > + dev_err(nfc->dev, "Opcode not supported: %u\n", opcode); > > + return -EOPNOTSUPP; > > + } > > + > > + return ret; > > +} > > + > > +static int ls1x_nand_parse_instructions(struct nand_chip *chip, > > + const struct nand_subop *subop, > > + struct ls1x_nfc_op *op) > > +{ > > + unsigned int op_id; > > + int ret; > > + > > + for (op_id = 0; op_id < subop->ninstrs; op_id++) { > > + const struct nand_op_instr *instr = &subop->instrs[op_id]; > > + unsigned int offset, naddrs; > > + const u8 *addrs; > > + > > + switch (instr->type) { > > + case NAND_OP_CMD_INSTR: > > + ret = ls1x_nand_op_cmd_mapping(chip, op, > > + instr->ctx.cmd.opcode); > > + if (ret < 0) > > + return ret; > > + break; > > + case NAND_OP_ADDR_INSTR: > > + naddrs = nand_subop_get_num_addr_cyc(subop, op_id); > > + if (naddrs > LS1X_NAND_MAX_ADDR_CYC) > > + return -EOPNOTSUPP; > > + op->naddrs = naddrs; > > + offset = nand_subop_get_addr_start_off(subop, op_id); > > + addrs = &instr->ctx.addr.addrs[offset]; > > + memcpy(op->addrs + op->addrs_offset, addrs, naddrs); > > + break; > > + case NAND_OP_DATA_IN_INSTR: > > + case NAND_OP_DATA_OUT_INSTR: > > + offset = nand_subop_get_data_start_off(subop, op_id); > > + op->len = nand_subop_get_data_len(subop, op_id); > > + if (instr->type == NAND_OP_DATA_IN_INSTR) > > + op->buf = instr->ctx.data.buf.in + offset; > > + else if (instr->type == NAND_OP_DATA_OUT_INSTR) > > + op->buf = > > + (void *)instr->ctx.data.buf.out + offset; > > + > > + break; > > + case NAND_OP_WAITRDY_INSTR: > > + op->rdy_timeout_ms = instr->ctx.waitrdy.timeout_ms; > > + break; > > + default: > > + break; > > + } > > + } > > + > > + return 0; > > +} > > + > > +static void ls1b_nand_parse_address(struct ls1x_nfc_op *op) > > +{ > > + int i; > > + > > + for (i = 0; i < LS1X_NAND_MAX_ADDR_CYC; i++) { > > + if (i < 2) > > + op->addr1_reg |= (u32)op->addrs[i] << i * BITS_PER_BYTE; > > + else if (i < 4) > > + op->addr1_reg |= > > + (u32)op->addrs[i] << (op->row_shift + > > + (i - 2) * BITS_PER_BYTE); > > + else > > + op->addr2_reg |= > > + (u32)op->addrs[i] >> (BITS_PER_WORD - > > + op->row_shift - (i - 4) * > > + BITS_PER_BYTE); > > Please break these lines at 100 char, not 80, it will make the reading > easier. > > > + } > > +} > > + > > +static void ls1c_nand_parse_address(struct ls1x_nfc_op *op) > > +{ > > + int i; > > + > > + for (i = 0; i < LS1X_NAND_MAX_ADDR_CYC; i++) { > > + if (i < 2) > > + op->addr1_reg |= (u32)op->addrs[i] << i * BITS_PER_BYTE; > > + else > > + op->addr2_reg |= > > + (u32)op->addrs[i] << (i - 2) * BITS_PER_BYTE; > > Same > > > + } > > +} > > + > > +static void ls1x_nand_trigger_op(struct ls1x_nfc *nfc, struct ls1x_nfc_op *op) > > +{ > > + struct nand_chip *chip = &nfc->chip; > > + struct mtd_info *mtd = nand_to_mtd(chip); > > + int col0 = op->addrs[0]; > > + short col; > > + > > + /* restore row address for column change */ > > + if (op->restore_row) { > > + op->addr2_reg = readl(nfc->reg_base + LS1X_NAND_ADDR2); > > + op->addr1_reg = readl(nfc->reg_base + LS1X_NAND_ADDR1); > > + op->addr1_reg &= ~(mtd->writesize - 1); > > This is strange and cannot work during the identification phase while > mtd->writesize is not yet known. > This condition will only become true when doing random data output (NAND_CMD_RNDOUT), which doesn't take effect during the identification phase. > > + } > > + > > + if (!IS_ALIGNED(col0, chip->buf_align)) { > > + col0 = ALIGN_DOWN(op->addrs[0], chip->buf_align); > > + op->aligned_offset = op->addrs[0] - col0; > > + op->addrs[0] = col0; > > + } > > + > > + if (nfc->data->parse_address) > > + nfc->data->parse_address(op); > > + > > + /* set address */ > > + writel(op->addr1_reg, nfc->reg_base + LS1X_NAND_ADDR1); > > + writel(op->addr2_reg, nfc->reg_base + LS1X_NAND_ADDR2); > > + > > + /* set data length */ > > + op->dma_len = ALIGN(op->len + op->aligned_offset, chip->buf_align); > > + if (op->cmd_reg & LS1X_NAND_CMD_ERASE) > > + writel(1, nfc->reg_base + LS1X_NAND_OP_NUM); > > + else > > + writel(op->dma_len, nfc->reg_base + LS1X_NAND_OP_NUM); > > + > > + /* set operation area */ > > + col = op->addrs[1] << BITS_PER_BYTE | op->addrs[0]; > > + if (op->len) { > > + if (col < mtd->writesize) > > should'nt this be <= mtd->writesize? > column address = mtd->writesize is the start address of spare area, right? > Also what about startup-times again, while we don't yet know writesize? > Exactly. I will correct the logic. > > + op->cmd_reg |= LS1X_NAND_CMD_OP_MAIN; > > + > > + op->cmd_reg |= LS1X_NAND_CMD_OP_SPARE; > > + } > > + > > + /* set operation scope */ > > + if (nfc->data->op_scope_field) { > > + int op_area = op->cmd_reg & LS1X_NAND_CMD_OP_AREA_MASK; > > + unsigned int op_scope; > > + > > + switch (op_area) { > > + case LS1X_NAND_CMD_OP_MAIN: > > + op_scope = mtd->writesize; > > + break; > > + case LS1X_NAND_CMD_OP_SPARE: > > + op_scope = mtd->oobsize; > > + break; > > + case LS1X_NAND_CMD_OP_AREA_MASK: > > + op_scope = mtd->writesize + mtd->oobsize; > > Can you please just use the length of the operation? > I strictly adhered to the guidance provided in the user manual. Additionally, when these bits are set to op->len, the controller does not work. > > + break; > > + default: > > + op_scope = 0; > > + break; > > + } > > + > > + op_scope <<= __ffs(nfc->data->op_scope_field); > > + regmap_update_bits(nfc->regmap, LS1X_NAND_PARAM, > > + nfc->data->op_scope_field, op_scope); > > + } > > + > > + /* set command */ > > + writel(op->cmd_reg, nfc->reg_base + LS1X_NAND_CMD); > > + > > + /* trigger operation */ > > + regmap_write_bits(nfc->regmap, LS1X_NAND_CMD, > > + LS1X_NAND_CMD_VALID, LS1X_NAND_CMD_VALID); > > +} > > + > > +static int ls1x_nand_wait_for_op_done(struct ls1x_nfc *nfc, > > + struct ls1x_nfc_op *op) > > +{ > > + unsigned int val; > > + int ret = 0; > > + > > + if (op->rdy_timeout_ms) { > > + ret = regmap_read_poll_timeout(nfc->regmap, LS1X_NAND_CMD, > > + val, val & LS1X_NAND_CMD_OP_DONE, > > + 0, op->rdy_timeout_ms * 1000); > > * MSECS_PER_SEC? > > > + if (ret) > > + dev_err(nfc->dev, "operation failed\n"); > > + } > > + > > + return ret; > > +} > > + > > +static void ls1x_nand_dma_callback(void *data) > > +{ > > + struct ls1x_nfc *nfc = (struct ls1x_nfc *)data; > > + struct dma_chan *chan = nfc->dma_chan; > > + struct device *dev = chan->device->dev; > > + enum dma_status status; > > + > > + status = dmaengine_tx_status(chan, nfc->dma_cookie, NULL); > > + if (likely(status == DMA_COMPLETE)) > > + dev_dbg(dev, "DMA complete with cookie=%d\n", nfc->dma_cookie); > > + else > > + dev_err(dev, "DMA error with cookie=%d\n", nfc->dma_cookie); > > + > > + complete(&nfc->dma_complete); > > Do you gracefully handle the dma error condition? You should not return > normally to userspace if DMA failed and I see no mechanism to forward > the error up. > Will correct the logic. > > +} > > + > > +static int ls1x_nand_dma_transfer(struct ls1x_nfc *nfc, > > + struct ls1x_nfc_op *op) > > +{ > > + struct nand_chip *chip = &nfc->chip; > > + struct dma_chan *chan = nfc->dma_chan; > > + struct device *dev = chan->device->dev; > > + struct dma_async_tx_descriptor *desc; > > + enum dma_data_direction data_dir = > > + op->is_write ? DMA_TO_DEVICE : DMA_FROM_DEVICE; > > + enum dma_transfer_direction xfer_dir = > > + op->is_write ? DMA_MEM_TO_DEV : DMA_DEV_TO_MEM; > > + void *buf = op->buf; > > + char *dma_buf = NULL; > > + dma_addr_t dma_addr; > > + int ret; > > + > > + if (IS_ALIGNED((uintptr_t)buf, chip->buf_align) && > > + IS_ALIGNED(op->len, chip->buf_align)) { > > + dma_addr = dma_map_single(dev, buf, op->len, data_dir); > > + if (dma_mapping_error(dev, dma_addr)) { > > + dev_err(dev, "failed to map DMA buffer\n"); > > + return -ENXIO; > > + } > > + } else if (!op->is_write) { > > + dma_buf = dma_alloc_coherent(dev, op->dma_len, &dma_addr, > > + GFP_KERNEL); > > + if (!dma_buf) > > + return -ENOMEM; > > + } else { > > + dev_err(dev, "subpage writing not supported\n"); > > + return -EOPNOTSUPP; > > + } > > + > > + desc = dmaengine_prep_slave_single(chan, dma_addr, op->dma_len, > > + xfer_dir, DMA_PREP_INTERRUPT); > > + if (!desc) { > > + dev_err(dev, "failed to prepare DMA descriptor\n"); > > + ret = PTR_ERR(desc); > > + goto err; > > + } > > + desc->callback = ls1x_nand_dma_callback; > > + desc->callback_param = nfc; > > + > > + nfc->dma_cookie = dmaengine_submit(desc); > > + ret = dma_submit_error(nfc->dma_cookie); > > + if (ret) { > > + dev_err(dev, "failed to submit DMA descriptor\n"); > > + goto err; > > + } > > + > > + dev_dbg(dev, "issue DMA with cookie=%d\n", nfc->dma_cookie); > > + dma_async_issue_pending(chan); > > + > > + ret = wait_for_completion_timeout(&nfc->dma_complete, > > + msecs_to_jiffies(2000)); > > + if (!ret) { > > + dmaengine_terminate_sync(chan); > > + reinit_completion(&nfc->dma_complete); > > + ret = -ETIMEDOUT; > > + goto err; > > + } > > + ret = 0; > > + > > + if (dma_buf) > > + memcpy(buf, dma_buf + op->aligned_offset, op->len); > > +err: > > + if (dma_buf) > > + dma_free_coherent(dev, op->dma_len, dma_buf, dma_addr); > > + else > > + dma_unmap_single(dev, dma_addr, op->len, data_dir); > > + > > + return ret; > > +} > > + > > +static int ls1x_nand_data_type_exec(struct nand_chip *chip, > > + const struct nand_subop *subop) > > +{ > > + struct ls1x_nfc *nfc = nand_get_controller_data(chip); > > + struct ls1x_nfc_op op = { }; > > + int ret; > > + > > + ret = ls1x_nand_parse_instructions(chip, subop, &op); > > + if (ret) > > + return ret; > > + > > + ls1x_nand_trigger_op(nfc, &op); > > + > > + ret = ls1x_nand_dma_transfer(nfc, &op); > > + if (ret) > > + return ret; > > + > > + return ls1x_nand_wait_for_op_done(nfc, &op); > > +} > > + > > +static int ls1x_nand_misc_type_exec(struct nand_chip *chip, > > + const struct nand_subop *subop, > > + struct ls1x_nfc_op *op) > > +{ > > + struct ls1x_nfc *nfc = nand_get_controller_data(chip); > > + int ret; > > + > > + ret = ls1x_nand_parse_instructions(chip, subop, op); > > + if (ret) > > + return ret; > > + > > + ls1x_nand_trigger_op(nfc, op); > > + > > + return ls1x_nand_wait_for_op_done(nfc, op); > > +} > > + > > +static int ls1x_nand_zerolen_type_exec(struct nand_chip *chip, > > + const struct nand_subop *subop) > > +{ > > + struct ls1x_nfc_op op = { }; > > You don't need this space ^ > > > + > > + return ls1x_nand_misc_type_exec(chip, subop, &op); > > +} > > + > > +static int ls1x_nand_read_id_type_exec(struct nand_chip *chip, > > + const struct nand_subop *subop) > > +{ > > + struct ls1x_nfc *nfc = nand_get_controller_data(chip); > > + struct ls1x_nfc_op op = { }; > > + int i, ret; > > + union { > > + char ids[5]; > > + struct { > > + int idl; > > + char idh; > > + }; > > + } nand_id; > > + > > + ret = ls1x_nand_misc_type_exec(chip, subop, &op); > > + if (ret) { > > + dev_err(nfc->dev, "failed to read ID! %d\n", ret); > > + return ret; > > + } > > + > > + nand_id.idl = readl(nfc->reg_base + LS1X_NAND_IDL); > > + nand_id.idh = readb(nfc->reg_base + LS1X_NAND_IDH_STATUS); > > + > > + for (i = 0; i < min(sizeof(nand_id.ids), op.len); i++) > > + op.buf[i] = nand_id.ids[sizeof(nand_id.ids) - 1 - i]; > > + > > + return ret; > > +} > > + > > +static int ls1x_nand_read_status_type_exec(struct nand_chip *chip, > > + const struct nand_subop *subop) > > +{ > > + struct ls1x_nfc *nfc = nand_get_controller_data(chip); > > + struct ls1x_nfc_op op = { }; > > + int val, ret; > > + > > + ret = ls1x_nand_misc_type_exec(chip, subop, &op); > > + if (ret) { > > + dev_err(nfc->dev, "failed to read status! %d\n", ret); > > + return ret; > > + } > > + > > + val = readl(nfc->reg_base + > > + LS1X_NAND_IDH_STATUS) & ~nfc->data->status_field; > > Just split this into: > > val = readl(); > val &= ~mask; > > > + op.buf[0] = val << ffs(nfc->data->status_field); > > + > > + return ret; > > +} > > + > > The rest look good. > > Thanks, > Miquèl Thanks for your review! -- Best regards, Keguang Zhang
