Re: [PATCH v10 2/2] mtd: rawnand: Add Loongson-1 NAND Controller Driver

[Miquel Raynal]

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.

> +
> +#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?

> +
> +	/* 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.

> +		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.

> +	}
> +
> +	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?

Also what about startup-times again, while we don't yet know writesize?

> +			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?

> +			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.

> +}
> +
> +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