[v3,2/4] mtd: rawnand: Add Macronix MX25F0A NAND controller
diff mbox series

Message ID 1555320234-15802-3-git-send-email-masonccyang@mxic.com.tw
State New
Headers show
Series
  • Add Macronix MX25F0A MFD driver for raw nand and spi
Related show

Commit Message

Mason Yang April 15, 2019, 9:23 a.m. UTC
Add a driver for Macronix MX25F0A NAND controller.

Signed-off-by: Mason Yang <masonccyang@mxic.com.tw>
---
 drivers/mtd/nand/raw/Kconfig     |   6 +
 drivers/mtd/nand/raw/Makefile    |   1 +
 drivers/mtd/nand/raw/mxic_nand.c | 294 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 301 insertions(+)
 create mode 100644 drivers/mtd/nand/raw/mxic_nand.c

Comments

Miquel Raynal May 12, 2019, 1:18 p.m. UTC | #1
Hi Mason,

Mason Yang <masonccyang@mxic.com.tw> wrote on Mon, 15 Apr 2019 17:23:52
+0800:

> Add a driver for Macronix MX25F0A NAND controller.
> 
> Signed-off-by: Mason Yang <masonccyang@mxic.com.tw>
> ---
>  drivers/mtd/nand/raw/Kconfig     |   6 +
>  drivers/mtd/nand/raw/Makefile    |   1 +
>  drivers/mtd/nand/raw/mxic_nand.c | 294 +++++++++++++++++++++++++++++++++++++++
>  3 files changed, 301 insertions(+)
>  create mode 100644 drivers/mtd/nand/raw/mxic_nand.c
> 
> diff --git a/drivers/mtd/nand/raw/Kconfig b/drivers/mtd/nand/raw/Kconfig
> index e604625..e0329cc 100644
> --- a/drivers/mtd/nand/raw/Kconfig
> +++ b/drivers/mtd/nand/raw/Kconfig
> @@ -522,6 +522,12 @@ config MTD_NAND_QCOM
>  	  Enables support for NAND flash chips on SoCs containing the EBI2 NAND
>  	  controller. This controller is found on IPQ806x SoC.
>  
> +config MTD_NAND_MXIC
> +	tristate "Macronix MX25F0A NAND controller"
> +	depends on HAS_IOMEM
> +	help
> +	  This selects the Macronix MX25F0A NAND controller driver.
> +
>  config MTD_NAND_MTK
>  	tristate "Support for NAND controller on MTK SoCs"
>  	depends on ARCH_MEDIATEK || COMPILE_TEST
> diff --git a/drivers/mtd/nand/raw/Makefile b/drivers/mtd/nand/raw/Makefile
> index 5a5a72f..c8a6790 100644
> --- a/drivers/mtd/nand/raw/Makefile
> +++ b/drivers/mtd/nand/raw/Makefile
> @@ -54,6 +54,7 @@ obj-$(CONFIG_MTD_NAND_SUNXI)		+= sunxi_nand.o
>  obj-$(CONFIG_MTD_NAND_HISI504)	        += hisi504_nand.o
>  obj-$(CONFIG_MTD_NAND_BRCMNAND)		+= brcmnand/
>  obj-$(CONFIG_MTD_NAND_QCOM)		+= qcom_nandc.o
> +obj-$(CONFIG_MTD_NAND_MXIC)		+= mxic_nand.o
>  obj-$(CONFIG_MTD_NAND_MTK)		+= mtk_ecc.o mtk_nand.o
>  obj-$(CONFIG_MTD_NAND_TEGRA)		+= tegra_nand.o
>  obj-$(CONFIG_MTD_NAND_STM32_FMC2)	+= stm32_fmc2_nand.o
> diff --git a/drivers/mtd/nand/raw/mxic_nand.c b/drivers/mtd/nand/raw/mxic_nand.c
> new file mode 100644
> index 0000000..689fae2
> --- /dev/null
> +++ b/drivers/mtd/nand/raw/mxic_nand.c
> @@ -0,0 +1,294 @@
> +// SPDX-License-Identifier: GPL-2.0
> +//
> +// Copyright (C) 2019 Macronix International Co., Ltd.
> +//
> +// Authors:
> +//	Mason Yang <masonccyang@mxic.com.tw>
> +//	zhengxunli <zhengxunli@mxic.com.tw>

This is not a valid name.

Also if he appears here I suppose he should be credited in the
module_authors() macro too.

> +//

As a personal taste, I prefer when the header uses /* */ and only the
SPDX tag uses //.

> +
> +#include <linux/mfd/mxic-mx25f0a.h>
> +#include <linux/mtd/rawnand.h>
> +#include <linux/mtd/nand_ecc.h>
> +
> +#include "internals.h"
> +
> +struct mxic_nand_ctlr {
> +	struct mx25f0a_mfd *mfd;
> +	struct nand_controller base;
> +	struct nand_chip nand;

Even if this controller only supports one CS (and then, one chip),
please have a clear separation between the NAND controller and the NAND
chip by having one structure for the NAND chips and one structure for
the NAND controller.

> +};
> +
> +static void mxic_host_init(struct mxic_nand_ctlr *mxic)

Please choose a constant prefix for all functions, right now there is:
mxic_
mxic_nand_
mx25f0a_nand_

I think mxic_nand_ or mx25f0a_nand_ is wise.

> +{
> +	writel(DATA_STROB_EDO_EN, mxic->mfd->regs + DATA_STROB);
> +	writel(INT_STS_ALL, mxic->mfd->regs + INT_STS_EN);
> +	writel(0x0, mxic->mfd->regs + ONFI_DIN_CNT(0));
> +	writel(HC_CFG_NIO(8) | HC_CFG_SLV_ACT(0) | HC_CFG_IDLE_SIO_LVL(1) |
> +	       HC_CFG_TYPE(1, HC_CFG_TYPE_RAW_NAND) | HC_CFG_MAN_CS_EN,
> +	       mxic->mfd->regs + HC_CFG);
> +	writel(0x0, mxic->mfd->regs + HC_EN);
> +}
> +
> +static int  mxic_nand_wait_ready(struct nand_chip *chip)
> +{
> +	struct mxic_nand_ctlr *mxic = nand_get_controller_data(chip);
> +	u32 sts;
> +
> +	return readl_poll_timeout(mxic->mfd->regs + INT_STS, sts,
> +				  sts & INT_RDY_PIN, 0, USEC_PER_SEC);
> +}
> +
> +static void mxic_nand_select_chip(struct nand_chip *chip, int chipnr)

_select_target() is preferred now

> +{
> +	struct mxic_nand_ctlr *mxic = nand_get_controller_data(chip);
> +
> +	switch (chipnr) {
> +	case 0:
> +	case 1:
> +		writel(HC_EN_BIT, mxic->mfd->regs + HC_EN);
> +		writel(HC_CFG_MAN_CS_ASSERT | readl(mxic->mfd->regs + HC_CFG),
> +		       mxic->mfd->regs + HC_CFG);

In both case I would prefer a:

        reg = readl(...);
        reg &= ~xxx;
	reg |= yyy;
	writel(reg, ...);

Much easier to read.

> +		break;
> +
> +	case -1:
> +		writel(~HC_CFG_MAN_CS_ASSERT & readl(mxic->mfd->regs + HC_CFG),
> +		       mxic->mfd->regs + HC_CFG);
> +		writel(0, mxic->mfd->regs + HC_EN);
> +		break;
> +
> +	default:

Error?

> +		break;
> +	}
> +}
> +
> +static int mxic_nand_data_xfer(struct mxic_nand_ctlr *mxic, const void *txbuf,
> +			       void *rxbuf, unsigned int len)
> +{

There is not so much code shared, why not separating this function for
rx and tx cases?

> +	unsigned int pos = 0;
> +
> +	while (pos < len) {
> +		unsigned int nbytes = len - pos;
> +		u32 data = 0xffffffff;
> +		u32 sts;
> +		int ret;
> +
> +		if (nbytes > 4)
> +			nbytes = 4;
> +
> +		if (txbuf)
> +			memcpy(&data, txbuf + pos, nbytes);
> +
> +		ret = readl_poll_timeout(mxic->mfd->regs + INT_STS, sts,
> +					 sts & INT_TX_EMPTY, 0, USEC_PER_SEC);

Using USEC_PER_SEC for a delay is weird

> +		if (ret)
> +			return ret;
> +
> +		writel(data, mxic->mfd->regs + TXD(nbytes % 4));
> +
> +		if (rxbuf) {
> +			ret = readl_poll_timeout(mxic->mfd->regs + INT_STS, sts,
> +						 sts & INT_TX_EMPTY, 0,
> +						 USEC_PER_SEC);
> +			if (ret)
> +				return ret;
> +
> +			ret = readl_poll_timeout(mxic->mfd->regs + INT_STS, sts,
> +						 sts & INT_RX_NOT_EMPTY, 0,
> +						 USEC_PER_SEC);
> +			if (ret)
> +				return ret;
> +
> +			data = readl(mxic->mfd->regs + RXD);
> +			data >>= (8 * (4 - nbytes));

What is this? Are you trying to handle some endianness issue?

> +			memcpy(rxbuf + pos, &data, nbytes);
> +			WARN_ON(readl(mxic->mfd->regs + INT_STS) &
> +				INT_RX_NOT_EMPTY);
> +		} else {
> +			readl(mxic->mfd->regs + RXD);
> +		}
> +		WARN_ON(readl(mxic->mfd->regs + INT_STS) & INT_RX_NOT_EMPTY);
> +
> +		pos += nbytes;
> +	}
> +
> +	return 0;
> +}
> +
> +static int mxic_nand_exec_op(struct nand_chip *chip,
> +			     const struct nand_operation *op, bool check_only)
> +{
> +	struct mxic_nand_ctlr *mxic = nand_get_controller_data(chip);
> +	const struct nand_op_instr *instr = NULL;
> +	int i, len = 0, ret = 0;
> +	unsigned int op_id;
> +	unsigned char cmdcnt = 0, addr_cnt = 0, cmd_addr[8] = {0};
> +
> +	for (op_id = 0; op_id < op->ninstrs; op_id++) {
> +		instr = &op->instrs[op_id];
> +
> +		switch (instr->type) {
> +		case NAND_OP_CMD_INSTR:
> +			cmd_addr[len++] = instr->ctx.cmd.opcode;
> +			cmdcnt++;
> +			break;
> +
> +		case NAND_OP_ADDR_INSTR:
> +			for (i = 0; i < instr->ctx.addr.naddrs; i++)
> +				cmd_addr[len++] = instr->ctx.addr.addrs[i];
> +			addr_cnt = i;
> +			break;
> +
> +		case NAND_OP_DATA_IN_INSTR:
> +			break;
> +
> +		case NAND_OP_DATA_OUT_INSTR:
> +			writel(instr->ctx.data.len,
> +			       mxic->mfd->regs + ONFI_DIN_CNT(0));
> +			break;
> +
> +		case NAND_OP_WAITRDY_INSTR:
> +			break;
> +		}
> +	}
> +
> +	if (op_id == 5 && instr->type == NAND_OP_WAITRDY_INSTR) {
> +		/*
> +		 * In case cmd-addr-data-cmd-wait in a sequence,
> +		 * separate the 2'nd command, i.e,. nand_prog_page_op()
> +		 */

I think this is the kind of limitation that could be described very
easily with a nand_op_parser structure and used by
nand_op_parser_exec_op() (see marvell_nand.c).

> +		writel(OP_CMD_BUSW(OP_BUSW_8) | OP_ADDR_BUSW(OP_BUSW_8) |
> +		       OP_DATA_BUSW(OP_BUSW_8) | OP_DUMMY_CYC(0x3F) |
> +		       OP_ADDR_BYTES(addr_cnt) |
> +		       OP_CMD_BYTES(1), mxic->mfd->regs + SS_CTRL(0));
> +		writel(0, mxic->mfd->regs + HC_EN);
> +		writel(HC_EN_BIT, mxic->mfd->regs + HC_EN);
> +
> +		mxic_nand_data_xfer(mxic, cmd_addr, NULL, len - 1);
> +
> +		mxic_nand_data_xfer(mxic, instr->ctx.data.buf.out, NULL,
> +				    instr->ctx.data.len);
> +
> +		writel(0, mxic->mfd->regs + HC_EN);
> +		writel(HC_EN_BIT, mxic->mfd->regs + HC_EN);
> +		mxic_nand_data_xfer(mxic, &cmd_addr[--len], NULL, 1);
> +		ret = mxic_nand_wait_ready(chip);
> +		if (ret)
> +			goto err_out;
> +		return ret;
> +	}
> +
> +	if (len) {
> +		writel(OP_CMD_BUSW(OP_BUSW_8) | OP_ADDR_BUSW(OP_BUSW_8) |
> +		       OP_DATA_BUSW(OP_BUSW_8) | OP_DUMMY_CYC(0x3F) |
> +		       OP_ADDR_BYTES(addr_cnt) |
> +		       OP_CMD_BYTES(cmdcnt > 0 ? cmdcnt : 0),
> +		       mxic->mfd->regs + SS_CTRL(0));
> +		writel(0, mxic->mfd->regs + HC_EN);
> +		writel(HC_EN_BIT, mxic->mfd->regs + HC_EN);
> +
> +		mxic_nand_data_xfer(mxic, cmd_addr, NULL, len);
> +	}
> +
> +	for (op_id = 0; op_id < op->ninstrs; op_id++) {
> +		instr = &op->instrs[op_id];
> +
> +		switch (instr->type) {
> +		case NAND_OP_CMD_INSTR:
> +		case NAND_OP_ADDR_INSTR:
> +			break;
> +
> +		case NAND_OP_DATA_IN_INSTR:
> +			writel(0x0, mxic->mfd->regs + ONFI_DIN_CNT(0));
> +			writel(readl(mxic->mfd->regs + SS_CTRL(0)) | OP_READ,
> +			       mxic->mfd->regs + SS_CTRL(0));
> +			mxic_nand_data_xfer(mxic, NULL, instr->ctx.data.buf.in,
> +					    instr->ctx.data.len);
> +			break;
> +
> +		case NAND_OP_DATA_OUT_INSTR:
> +			mxic_nand_data_xfer(mxic, instr->ctx.data.buf.out, NULL,
> +					    instr->ctx.data.len);
> +			break;
> +
> +		case NAND_OP_WAITRDY_INSTR:
> +			ret = mxic_nand_wait_ready(chip);
> +			if (ret)
> +				goto err_out;
> +			break;
> +		}
> +	}
> +
> +err_out:
> +	return ret;

Ditto, please return directly if there is nothing in the error path.

> +}
> +
> +static const struct nand_controller_ops mxic_nand_controller_ops = {
> +	.exec_op = mxic_nand_exec_op,
> +};
> +
> +static int mx25f0a_nand_probe(struct platform_device *pdev)
> +{
> +	struct mtd_info *mtd;
> +	struct mx25f0a_mfd *mfd = dev_get_drvdata(pdev->dev.parent);
> +	struct mxic_nand_ctlr *mxic;
> +	struct nand_chip *nand_chip;
> +	int err;
> +
> +	mxic = devm_kzalloc(&pdev->dev, sizeof(struct mxic_nand_ctlr),
> +			    GFP_KERNEL);

mxic for a NAND controller structure is probably not a name meaningful
enough.

> +	if (!mxic)
> +		return -ENOMEM;
> +
> +	nand_chip = &mxic->nand;
> +	mtd = nand_to_mtd(nand_chip);
> +	mtd->dev.parent = pdev->dev.parent;
> +	nand_chip->ecc.priv = NULL;
> +	nand_set_flash_node(nand_chip, pdev->dev.parent->of_node);
> +	nand_chip->priv = mxic;
> +
> +	mxic->mfd = mfd;
> +
> +	nand_chip->legacy.select_chip = mxic_nand_select_chip;

Please don't implement legacy interfaces. You can check in
marvell_nand.c how this is handled now:

b25251414f6e mtd: rawnand: marvell: Stop implementing ->select_chip()

> +	mxic->base.ops = &mxic_nand_controller_ops;
> +	nand_controller_init(&mxic->base);
> +	nand_chip->controller = &mxic->base;
> +
> +	mxic_host_init(mxic);
> +
> +	err = nand_scan(nand_chip, 1);
> +	if (err)
> +		goto fail;

You can return directly as there is nothing to clean in the error path.

> +
> +	err = mtd_device_register(mtd, NULL, 0);
> +	if (err)
> +		goto fail;

Ditto

> +
> +	platform_set_drvdata(pdev, mxic);
> +
> +	return 0;
> +fail:
> +	return err;
> +}
> +
> +static int mx25f0a_nand_remove(struct platform_device *pdev)
> +{
> +	struct mxic_nand_ctlr *mxic = platform_get_drvdata(pdev);
> +
> +	nand_release(&mxic->nand);
> +
> +	return 0;
> +}
> +
> +static struct platform_driver mx25f0a_nand_driver = {
> +	.probe		= mx25f0a_nand_probe,

Please don't align '=' on tabs.

> +	.remove		= mx25f0a_nand_remove,
> +	.driver		= {
> +		.name	= "mxic-nand-ctlr",
> +	},
> +};
> +module_platform_driver(mx25f0a_nand_driver);

mx25f0a_nand_controller_driver would be better

> +
> +MODULE_AUTHOR("Mason Yang <masonccyang@mxic.com.tw>");
> +MODULE_DESCRIPTION("MX25F0A RAW NAND controller driver");
> +MODULE_LICENSE("GPL v2");

Thanks,
Miquèl
Mason Yang May 15, 2019, 8:48 a.m. UTC | #2
Hi Miquel,

> > +// SPDX-License-Identifier: GPL-2.0
> > +//
> > +// Copyright (C) 2019 Macronix International Co., Ltd.
> > +//
> > +// Authors:
> > +//   Mason Yang <masonccyang@mxic.com.tw>
> > +//   zhengxunli <zhengxunli@mxic.com.tw>
> 
> This is not a valid name.
> 
> Also if he appears here I suppose he should be credited in the
> module_authors() macro too.

I think Li should maintain this NAND driver later, 
anyway, I may remove him.

> 
> > +//
> 
> As a personal taste, I prefer when the header uses /* */ and only the
> SPDX tag uses //.

okay, only SPDX tag use //

> 
> > +
> > +#include <linux/mfd/mxic-mx25f0a.h>
> > +#include <linux/mtd/rawnand.h>
> > +#include <linux/mtd/nand_ecc.h>
> > +
> > +#include "internals.h"
> > +
> > +struct mxic_nand_ctlr {
> > +   struct mx25f0a_mfd *mfd;
> > +   struct nand_controller base;
> > +   struct nand_chip nand;
> 
> Even if this controller only supports one CS (and then, one chip),
> please have a clear separation between the NAND controller and the NAND
> chip by having one structure for the NAND chips and one structure for
> the NAND controller.

okay, will patch it.

> 
> > +};
> > +
> > +static void mxic_host_init(struct mxic_nand_ctlr *mxic)
> 
> Please choose a constant prefix for all functions, right now there is:
> mxic_
> mxic_nand_
> mx25f0a_nand_
> 
> I think mxic_nand_ or mx25f0a_nand_ is wise.

okay, will use mxic_nand_ as prefix. 

> 
> > +{
> > +   writel(DATA_STROB_EDO_EN, mxic->mfd->regs + DATA_STROB);
> > +   writel(INT_STS_ALL, mxic->mfd->regs + INT_STS_EN);
> > +   writel(0x0, mxic->mfd->regs + ONFI_DIN_CNT(0));
> > +   writel(HC_CFG_NIO(8) | HC_CFG_SLV_ACT(0) | HC_CFG_IDLE_SIO_LVL(1) 
|
> > +          HC_CFG_TYPE(1, HC_CFG_TYPE_RAW_NAND) | HC_CFG_MAN_CS_EN,
> > +          mxic->mfd->regs + HC_CFG);
> > +   writel(0x0, mxic->mfd->regs + HC_EN);
> > +}
> > +
> > +static int  mxic_nand_wait_ready(struct nand_chip *chip)
> > +{
> > +   struct mxic_nand_ctlr *mxic = nand_get_controller_data(chip);
> > +   u32 sts;
> > +
> > +   return readl_poll_timeout(mxic->mfd->regs + INT_STS, sts,
> > +              sts & INT_RDY_PIN, 0, USEC_PER_SEC);
> > +}
> > +
> > +static void mxic_nand_select_chip(struct nand_chip *chip, int chipnr)
> 
> _select_target() is preferred now
> 
> > +{
> > +   struct mxic_nand_ctlr *mxic = nand_get_controller_data(chip);
> > +
> > +   switch (chipnr) {
> > +   case 0:
> > +   case 1:
> > +      writel(HC_EN_BIT, mxic->mfd->regs + HC_EN);
> > +      writel(HC_CFG_MAN_CS_ASSERT | readl(mxic->mfd->regs + HC_CFG),
> > +             mxic->mfd->regs + HC_CFG);
> 
> In both case I would prefer a:
> 
>         reg = readl(...);
>         reg &= ~xxx;
>    reg |= yyy;
>    writel(reg, ...);
> 
> Much easier to read.
> 
> > +      break;
> > +
> > +   case -1:
> > +      writel(~HC_CFG_MAN_CS_ASSERT & readl(mxic->mfd->regs + HC_CFG),
> > +             mxic->mfd->regs + HC_CFG);
> > +      writel(0, mxic->mfd->regs + HC_EN);
> > +      break;
> > +
> > +   default:
> 
> Error?
> 
> > +      break;
> > +   }
> > +}
> > +
> > +static int mxic_nand_data_xfer(struct mxic_nand_ctlr *mxic, const 
void *txbuf,
> > +                void *rxbuf, unsigned int len)
> > +{
> 
> There is not so much code shared, why not separating this function for
> rx and tx cases?
> 
> > +   unsigned int pos = 0;
> > +
> > +   while (pos < len) {
> > +      unsigned int nbytes = len - pos;
> > +      u32 data = 0xffffffff;
> > +      u32 sts;
> > +      int ret;
> > +
> > +      if (nbytes > 4)
> > +         nbytes = 4;
> > +
> > +      if (txbuf)
> > +         memcpy(&data, txbuf + pos, nbytes);
> > +
> > +      ret = readl_poll_timeout(mxic->mfd->regs + INT_STS, sts,
> > +                sts & INT_TX_EMPTY, 0, USEC_PER_SEC);
> 
> Using USEC_PER_SEC for a delay is weird
> 
> > +      if (ret)
> > +         return ret;
> > +
> > +      writel(data, mxic->mfd->regs + TXD(nbytes % 4));
> > +
> > +      if (rxbuf) {
> > +         ret = readl_poll_timeout(mxic->mfd->regs + INT_STS, sts,
> > +                   sts & INT_TX_EMPTY, 0,
> > +                   USEC_PER_SEC);
> > +         if (ret)
> > +            return ret;
> > +
> > +         ret = readl_poll_timeout(mxic->mfd->regs + INT_STS, sts,
> > +                   sts & INT_RX_NOT_EMPTY, 0,
> > +                   USEC_PER_SEC);
> > +         if (ret)
> > +            return ret;
> > +
> > +         data = readl(mxic->mfd->regs + RXD);
> > +         data >>= (8 * (4 - nbytes));
> 
> What is this? Are you trying to handle some endianness issue?

yes,

> 
> > +         memcpy(rxbuf + pos, &data, nbytes);
> > +         WARN_ON(readl(mxic->mfd->regs + INT_STS) &
> > +            INT_RX_NOT_EMPTY);
> > +      } else {
> > +         readl(mxic->mfd->regs + RXD);
> > +      }
> > +      WARN_ON(readl(mxic->mfd->regs + INT_STS) & INT_RX_NOT_EMPTY);
> > +
> > +      pos += nbytes;
> > +   }
> > +
> > +   return 0;
> > +}






> > +
> > +static int mxic_nand_exec_op(struct nand_chip *chip,
> > +              const struct nand_operation *op, bool check_only)
> > +{
> > +   struct mxic_nand_ctlr *mxic = nand_get_controller_data(chip);
> > +   const struct nand_op_instr *instr = NULL;
> > +   int i, len = 0, ret = 0;
> > +   unsigned int op_id;
> > +   unsigned char cmdcnt = 0, addr_cnt = 0, cmd_addr[8] = {0};
> > +
> > +   for (op_id = 0; op_id < op->ninstrs; op_id++) {
> > +      instr = &op->instrs[op_id];
> > +
> > +      switch (instr->type) {
> > +      case NAND_OP_CMD_INSTR:
> > +         cmd_addr[len++] = instr->ctx.cmd.opcode;
> > +         cmdcnt++;
> > +         break;
> > +
> > +      case NAND_OP_ADDR_INSTR:
> > +         for (i = 0; i < instr->ctx.addr.naddrs; i++)
> > +            cmd_addr[len++] = instr->ctx.addr.addrs[i];
> > +         addr_cnt = i;
> > +         break;
> > +
> > +      case NAND_OP_DATA_IN_INSTR:
> > +         break;
> > +
> > +      case NAND_OP_DATA_OUT_INSTR:
> > +         writel(instr->ctx.data.len,
> > +                mxic->mfd->regs + ONFI_DIN_CNT(0));
> > +         break;
> > +
> > +      case NAND_OP_WAITRDY_INSTR:
> > +         break;
> > +      }
> > +   }
> > +
> > +   if (op_id == 5 && instr->type == NAND_OP_WAITRDY_INSTR) {
> > +      /*
> > +       * In case cmd-addr-data-cmd-wait in a sequence,
> > +       * separate the 2'nd command, i.e,. nand_prog_page_op()
> > +       */
> 
> I think this is the kind of limitation that could be described very
> easily with a nand_op_parser structure and used by
> nand_op_parser_exec_op() (see marvell_nand.c).

okay, thanks for your information.
Will check and patch it.

> 
> > +      writel(OP_CMD_BUSW(OP_BUSW_8) | OP_ADDR_BUSW(OP_BUSW_8) |
> > +             OP_DATA_BUSW(OP_BUSW_8) | OP_DUMMY_CYC(0x3F) |
> > +             OP_ADDR_BYTES(addr_cnt) |
> > +             OP_CMD_BYTES(1), mxic->mfd->regs + SS_CTRL(0));
> > +      writel(0, mxic->mfd->regs + HC_EN);
> > +      writel(HC_EN_BIT, mxic->mfd->regs + HC_EN);
> > +
> > +      mxic_nand_data_xfer(mxic, cmd_addr, NULL, len - 1);
> > +
> > +      mxic_nand_data_xfer(mxic, instr->ctx.data.buf.out, NULL,
> > +                instr->ctx.data.len);
> > +
> > +      writel(0, mxic->mfd->regs + HC_EN);
> > +      writel(HC_EN_BIT, mxic->mfd->regs + HC_EN);
> > +      mxic_nand_data_xfer(mxic, &cmd_addr[--len], NULL, 1);
> > +      ret = mxic_nand_wait_ready(chip);
> > +      if (ret)
> > +         goto err_out;
> > +      return ret;
> > +   }
> > +
> > +   if (len) {
> > +      writel(OP_CMD_BUSW(OP_BUSW_8) | OP_ADDR_BUSW(OP_BUSW_8) |
> > +             OP_DATA_BUSW(OP_BUSW_8) | OP_DUMMY_CYC(0x3F) |
> > +             OP_ADDR_BYTES(addr_cnt) |
> > +             OP_CMD_BYTES(cmdcnt > 0 ? cmdcnt : 0),
> > +             mxic->mfd->regs + SS_CTRL(0));
> > +      writel(0, mxic->mfd->regs + HC_EN);
> > +      writel(HC_EN_BIT, mxic->mfd->regs + HC_EN);
> > +
> > +      mxic_nand_data_xfer(mxic, cmd_addr, NULL, len);
> > +   }
> > +
> > +   for (op_id = 0; op_id < op->ninstrs; op_id++) {
> > +      instr = &op->instrs[op_id];
> > +
> > +      switch (instr->type) {
> > +      case NAND_OP_CMD_INSTR:
> > +      case NAND_OP_ADDR_INSTR:
> > +         break;
> > +
> > +      case NAND_OP_DATA_IN_INSTR:
> > +         writel(0x0, mxic->mfd->regs + ONFI_DIN_CNT(0));
> > +         writel(readl(mxic->mfd->regs + SS_CTRL(0)) | OP_READ,
> > +                mxic->mfd->regs + SS_CTRL(0));
> > +         mxic_nand_data_xfer(mxic, NULL, instr->ctx.data.buf.in,
> > +                   instr->ctx.data.len);
> > +         break;
> > +
> > +      case NAND_OP_DATA_OUT_INSTR:
> > +         mxic_nand_data_xfer(mxic, instr->ctx.data.buf.out, NULL,
> > +                   instr->ctx.data.len);
> > +         break;
> > +
> > +      case NAND_OP_WAITRDY_INSTR:
> > +         ret = mxic_nand_wait_ready(chip);
> > +         if (ret)
> > +            goto err_out;
> > +         break;
> > +      }
> > +   }
> > +
> > +err_out:
> > +   return ret;
> 
> Ditto, please return directly if there is nothing in the error path.

okay

> 
> > +}
> > +
> > +static const struct nand_controller_ops mxic_nand_controller_ops = {
> > +   .exec_op = mxic_nand_exec_op,
> > +};
> > +
> > +static int mx25f0a_nand_probe(struct platform_device *pdev)
> > +{
> > +   struct mtd_info *mtd;
> > +   struct mx25f0a_mfd *mfd = dev_get_drvdata(pdev->dev.parent);
> > +   struct mxic_nand_ctlr *mxic;
> > +   struct nand_chip *nand_chip;
> > +   int err;
> > +
> > +   mxic = devm_kzalloc(&pdev->dev, sizeof(struct mxic_nand_ctlr),
> > +             GFP_KERNEL);
> 
> mxic for a NAND controller structure is probably not a name meaningful
> enough.

How about *fmc or *mxic_fmc ?

> 
> > +   if (!mxic)
> > +      return -ENOMEM;
> > +
> > +   nand_chip = &mxic->nand;
> > +   mtd = nand_to_mtd(nand_chip);
> > +   mtd->dev.parent = pdev->dev.parent;
> > +   nand_chip->ecc.priv = NULL;
> > +   nand_set_flash_node(nand_chip, pdev->dev.parent->of_node);
> > +   nand_chip->priv = mxic;
> > +
> > +   mxic->mfd = mfd;
> > +
> > +   nand_chip->legacy.select_chip = mxic_nand_select_chip;
> 
> Please don't implement legacy interfaces. You can check in
> marvell_nand.c how this is handled now:
> 
> b25251414f6e mtd: rawnand: marvell: Stop implementing ->select_chip()

got it, will check & patch.

> 
> > +   mxic->base.ops = &mxic_nand_controller_ops;
> > +   nand_controller_init(&mxic->base);
> > +   nand_chip->controller = &mxic->base;
> > +
> > +   mxic_host_init(mxic);
> > +
> > +   err = nand_scan(nand_chip, 1);
> > +   if (err)
> > +      goto fail;
> 
> You can return directly as there is nothing to clean in the error path.
> 
> > +
> > +   err = mtd_device_register(mtd, NULL, 0);
> > +   if (err)
> > +      goto fail;
> 
> Ditto
> 
> > +
> > +   platform_set_drvdata(pdev, mxic);
> > +
> > +   return 0;
> > +fail:
> > +   return err;
> > +}
> > +
> > +static int mx25f0a_nand_remove(struct platform_device *pdev)
> > +{
> > +   struct mxic_nand_ctlr *mxic = platform_get_drvdata(pdev);
> > +
> > +   nand_release(&mxic->nand);
> > +
> > +   return 0;
> > +}
> > +
> > +static struct platform_driver mx25f0a_nand_driver = {
> > +   .probe      = mx25f0a_nand_probe,
> 
> Please don't align '=' on tabs.

okay, will fix and also remove "mx25f0a" char.
patch to:
mxic_nand_driver & mxic_nand_probe.


thanks for your review.
best regards,
Mason

--






CONFIDENTIALITY NOTE:

This e-mail and any attachments may contain confidential information 
and/or personal data, which is protected by applicable laws. Please be 
reminded that duplication, disclosure, distribution, or use of this e-mail 
(and/or its attachments) or any part thereof is prohibited. If you receive 
this e-mail in error, please notify us immediately and delete this mail as 
well as its attachment(s) from your system. In addition, please be 
informed that collection, processing, and/or use of personal data is 
prohibited unless expressly permitted by personal data protection laws. 
Thank you for your attention and cooperation.

Macronix International Co., Ltd.

=====================================================================



============================================================================

CONFIDENTIALITY NOTE:

This e-mail and any attachments may contain confidential information and/or personal data, which is protected by applicable laws. Please be reminded that duplication, disclosure, distribution, or use of this e-mail (and/or its attachments) or any part thereof is prohibited. If you receive this e-mail in error, please notify us immediately and delete this mail as well as its attachment(s) from your system. In addition, please be informed that collection, processing, and/or use of personal data is prohibited unless expressly permitted by personal data protection laws. Thank you for your attention and cooperation.

Macronix International Co., Ltd.

=====================================================================
Mason Yang May 15, 2019, 9:18 a.m. UTC | #3
Hi Miquel,

Sorry, previous email missed this mxic_nand_data_xfer() reply.

This Flash Memory Controller implemented the Buffer read-write data 
transfer 
for SPI mode and raw NAND mode.

That is each time driver write to the transmit of the TXD port and the 
data 
is shifted out, new data is received in RXD port. 
By transmitting the entire buffer without reading any data, driver are 
losing
the received data.

Actually the mxic_nand_data_xfer() is a copy of mxic_spi_data_xfer().

https://github.com/torvalds/linux/blame/master/drivers/spi/spi-mxic.c 

therefore, I plan to patch this part to MFD as a common code for 
both raw NAND and SPI.

i.e,. 
In driver/mfd/mxic-mfd.c, we have mxic_mfd_data_xfer() and 

here call mxic_mfd_data_xfer() for raw NAND data read-write.


> > > +static int mxic_nand_data_xfer(struct mxic_nand_ctlr *mxic, const 
void *txbuf,
> > > +                void *rxbuf, unsigned int len)
> > > +{
> > 
> > There is not so much code shared, why not separating this function for
> > rx and tx cases?
> > 
> > > +   unsigned int pos = 0;
> > > +
> > > +   while (pos < len) {
> > > +      unsigned int nbytes = len - pos;
> > > +      u32 data = 0xffffffff;
> > > +      u32 sts;
> > > +      int ret;
> > > +
> > > +      if (nbytes > 4)
> > > +         nbytes = 4;
> > > +
> > > +      if (txbuf)
> > > +         memcpy(&data, txbuf + pos, nbytes);
> > > +
> > > +      ret = readl_poll_timeout(mxic->mfd->regs + INT_STS, sts,
> > > +                sts & INT_TX_EMPTY, 0, USEC_PER_SEC);
> > 
> > Using USEC_PER_SEC for a delay is weird
> > 
> > > +      if (ret)
> > > +         return ret;
> > > +
> > > +      writel(data, mxic->mfd->regs + TXD(nbytes % 4));
> > > +
> > > +      if (rxbuf) {
> > > +         ret = readl_poll_timeout(mxic->mfd->regs + INT_STS, sts,
> > > +                   sts & INT_TX_EMPTY, 0,
> > > +                   USEC_PER_SEC);
> > > +         if (ret)
> > > +            return ret;
> > > +
> > > +         ret = readl_poll_timeout(mxic->mfd->regs + INT_STS, sts,
> > > +                   sts & INT_RX_NOT_EMPTY, 0,
> > > +                   USEC_PER_SEC);
> > > +         if (ret)
> > > +            return ret;
> > > +
> > > +         data = readl(mxic->mfd->regs + RXD);
> > > +         data >>= (8 * (4 - nbytes));
> > 
> > What is this? Are you trying to handle some endianness issue?
> 
> yes,
> 
> > 
> > > +         memcpy(rxbuf + pos, &data, nbytes);
> > > +         WARN_ON(readl(mxic->mfd->regs + INT_STS) &
> > > +            INT_RX_NOT_EMPTY);
> > > +      } else {
> > > +         readl(mxic->mfd->regs + RXD);
> > > +      }
> > > +      WARN_ON(readl(mxic->mfd->regs + INT_STS) & INT_RX_NOT_EMPTY);
> > > +
> > > +      pos += nbytes;
> > > +   }
> > > +
> > > +   return 0;
> > > +}

thanks for your review.

best regards,
Mason

--

CONFIDENTIALITY NOTE:

This e-mail and any attachments may contain confidential information 
and/or personal data, which is protected by applicable laws. Please be 
reminded that duplication, disclosure, distribution, or use of this e-mail 
(and/or its attachments) or any part thereof is prohibited. If you receive 
this e-mail in error, please notify us immediately and delete this mail as 
well as its attachment(s) from your system. In addition, please be 
informed that collection, processing, and/or use of personal data is 
prohibited unless expressly permitted by personal data protection laws. 
Thank you for your attention and cooperation.

Macronix International Co., Ltd.

=====================================================================



============================================================================

CONFIDENTIALITY NOTE:

This e-mail and any attachments may contain confidential information and/or personal data, which is protected by applicable laws. Please be reminded that duplication, disclosure, distribution, or use of this e-mail (and/or its attachments) or any part thereof is prohibited. If you receive this e-mail in error, please notify us immediately and delete this mail as well as its attachment(s) from your system. In addition, please be informed that collection, processing, and/or use of personal data is prohibited unless expressly permitted by personal data protection laws. Thank you for your attention and cooperation.

Macronix International Co., Ltd.

=====================================================================
Miquel Raynal May 15, 2019, 12:08 p.m. UTC | #4
Hi masonccyang@mxic.com.tw,

masonccyang@mxic.com.tw wrote on Wed, 15 May 2019 16:48:46 +0800:

> Hi Miquel,
> 
> > > +// SPDX-License-Identifier: GPL-2.0
> > > +//
> > > +// Copyright (C) 2019 Macronix International Co., Ltd.
> > > +//
> > > +// Authors:
> > > +//   Mason Yang <masonccyang@mxic.com.tw>
> > > +//   zhengxunli <zhengxunli@mxic.com.tw>  
> > 
> > This is not a valid name.
> > 
> > Also if he appears here I suppose he should be credited in the
> > module_authors() macro too.  
> 
> I think Li should maintain this NAND driver later, 

This entry is for the authors of the driver.

If he will maintain the driver, then add a new entry in MAINTAINERS.

> > > +}
> > > +
> > > +static const struct nand_controller_ops mxic_nand_controller_ops = {
> > > +   .exec_op = mxic_nand_exec_op,
> > > +};
> > > +
> > > +static int mx25f0a_nand_probe(struct platform_device *pdev)
> > > +{
> > > +   struct mtd_info *mtd;
> > > +   struct mx25f0a_mfd *mfd = dev_get_drvdata(pdev->dev.parent);
> > > +   struct mxic_nand_ctlr *mxic;
> > > +   struct nand_chip *nand_chip;
> > > +   int err;
> > > +
> > > +   mxic = devm_kzalloc(&pdev->dev, sizeof(struct mxic_nand_ctlr),
> > > +             GFP_KERNEL);  
> > 
> > mxic for a NAND controller structure is probably not a name meaningful
> > enough.  
> 
> How about *fmc or *mxic_fmc ?

fmc is fine, even if I personally prefer nfc for NAND flash controller.
Here the 'm' in fmc stands for 'memory' but I am not sure if the
controller can manage something else than NAND flash anyway?


Thanks,
Miquèl
Mason Yang May 17, 2019, 9:30 a.m. UTC | #5
Hi Miquel,

> > +
> > +static void mxic_nand_select_chip(struct nand_chip *chip, int chipnr)
> 
> _select_target() is preferred now

Do you mean I implement mxic_nand_select_target() to control #CS ?

If so, I need to call mxic_nand_select_target( ) to control #CS ON
and then #CS OFF in _exec_op() due to nand_select_target()<in nand_base,c>
is still calling chip->legacy.select_chip ?

> 
> > +{
> > +   struct mxic_nand_ctlr *mxic = nand_get_controller_data(chip);
> > +
> > +   switch (chipnr) {
> > +   case 0:
> > +   case 1:
> > +      writel(HC_EN_BIT, mxic->mfd->regs + HC_EN);
> > +      writel(HC_CFG_MAN_CS_ASSERT | readl(mxic->mfd->regs + HC_CFG),
> > +             mxic->mfd->regs + HC_CFG);
> 
> In both case I would prefer a:
> 
>         reg = readl(...);
>         reg &= ~xxx;
>    reg |= yyy;
>    writel(reg, ...);
> 
> Much easier to read.
> 
> > +      break;
> > +
> > +   case -1:
> > +      writel(~HC_CFG_MAN_CS_ASSERT & readl(mxic->mfd->regs + HC_CFG),
> > +             mxic->mfd->regs + HC_CFG);
> > +      writel(0, mxic->mfd->regs + HC_EN);
> > +      break;
> > +
> > +   default:
> 
> Error?
> 
> > +      break;
> > +   }
> > +}
> > +

> > +static int mx25f0a_nand_probe(struct platform_device *pdev)
> > +{
> > +   struct mtd_info *mtd;
> > +   struct mx25f0a_mfd *mfd = dev_get_drvdata(pdev->dev.parent);
> > +   struct mxic_nand_ctlr *mxic;
> > +   struct nand_chip *nand_chip;
> > +   int err;
> > +
> > +   mxic = devm_kzalloc(&pdev->dev, sizeof(struct mxic_nand_ctlr),
> > +             GFP_KERNEL);
> 
> mxic for a NAND controller structure is probably not a name meaningful
> enough.
> 
> > +   if (!mxic)
> > +      return -ENOMEM;
> > +
> > +   nand_chip = &mxic->nand;
> > +   mtd = nand_to_mtd(nand_chip);
> > +   mtd->dev.parent = pdev->dev.parent;
> > +   nand_chip->ecc.priv = NULL;
> > +   nand_set_flash_node(nand_chip, pdev->dev.parent->of_node);
> > +   nand_chip->priv = mxic;
> > +
> > +   mxic->mfd = mfd;
> > +
> > +   nand_chip->legacy.select_chip = mxic_nand_select_chip;
> 
> Please don't implement legacy interfaces. You can check in
> marvell_nand.c how this is handled now:
> 
> b25251414f6e mtd: rawnand: marvell: Stop implementing ->select_chip()
> 

Does it mean chip->legacy.select_chip() will phase-out ?


thanks & best regards,
Mason

CONFIDENTIALITY NOTE:

This e-mail and any attachments may contain confidential information 
and/or personal data, which is protected by applicable laws. Please be 
reminded that duplication, disclosure, distribution, or use of this e-mail 
(and/or its attachments) or any part thereof is prohibited. If you receive 
this e-mail in error, please notify us immediately and delete this mail as 
well as its attachment(s) from your system. In addition, please be 
informed that collection, processing, and/or use of personal data is 
prohibited unless expressly permitted by personal data protection laws. 
Thank you for your attention and cooperation.

Macronix International Co., Ltd.

=====================================================================



============================================================================

CONFIDENTIALITY NOTE:

This e-mail and any attachments may contain confidential information and/or personal data, which is protected by applicable laws. Please be reminded that duplication, disclosure, distribution, or use of this e-mail (and/or its attachments) or any part thereof is prohibited. If you receive this e-mail in error, please notify us immediately and delete this mail as well as its attachment(s) from your system. In addition, please be informed that collection, processing, and/or use of personal data is prohibited unless expressly permitted by personal data protection laws. Thank you for your attention and cooperation.

Macronix International Co., Ltd.

=====================================================================
Miquel Raynal May 20, 2019, 12:23 p.m. UTC | #6
Hi Mason,

masonccyang@mxic.com.tw wrote on Fri, 17 May 2019 17:30:21 +0800:

> Hi Miquel,
> 
> > > +
> > > +static void mxic_nand_select_chip(struct nand_chip *chip, int chipnr)  
> > 
> > _select_target() is preferred now  
> 
> Do you mean I implement mxic_nand_select_target() to control #CS ?
> 
> If so, I need to call mxic_nand_select_target( ) to control #CS ON
> and then #CS OFF in _exec_op() due to nand_select_target()<in nand_base,c>
> is still calling chip->legacy.select_chip ?

You must forget about the ->select_chip() callback. Now it should be
handled directly from the controller driver. Please have a look at the
commit pointed against the marvell_nand.c driver.

[...]

> > > +   if (!mxic)
> > > +      return -ENOMEM;
> > > +
> > > +   nand_chip = &mxic->nand;
> > > +   mtd = nand_to_mtd(nand_chip);
> > > +   mtd->dev.parent = pdev->dev.parent;
> > > +   nand_chip->ecc.priv = NULL;
> > > +   nand_set_flash_node(nand_chip, pdev->dev.parent->of_node);
> > > +   nand_chip->priv = mxic;
> > > +
> > > +   mxic->mfd = mfd;
> > > +
> > > +   nand_chip->legacy.select_chip = mxic_nand_select_chip;  
> > 
> > Please don't implement legacy interfaces. You can check in
> > marvell_nand.c how this is handled now:
> > 
> > b25251414f6e mtd: rawnand: marvell: Stop implementing ->select_chip()
> >   
> 
> Does it mean chip->legacy.select_chip() will phase-out ?

Yes it will.

Thanks,
Miquèl
Mason Yang May 23, 2019, 8:58 a.m. UTC | #7
Hi Miquel,

> > 
> > > > +
> > > > +static void mxic_nand_select_chip(struct nand_chip *chip, int 
chipnr) 
> > > 
> > > _select_target() is preferred now 
> > 
> > Do you mean I implement mxic_nand_select_target() to control #CS ?
> > 
> > If so, I need to call mxic_nand_select_target( ) to control #CS ON
> > and then #CS OFF in _exec_op() due to nand_select_target()<in 
nand_base,c>
> > is still calling chip->legacy.select_chip ?
> 
> You must forget about the ->select_chip() callback. Now it should be
> handled directly from the controller driver. Please have a look at the
> commit pointed against the marvell_nand.c driver.

I have no Marvell NFC datasheet and have one question.

In marvell_nand.c, there is no xxx_deselect_target() or 
something like that doing #CS OFF.
marvell_nfc_select_target() seems always to make one of chip or die
#CS keep low.

Is it right ?

How to make all #CS keep high for NAND to enter 
low-power standby mode if driver don't use "legacy.select_chip()" ?

thanks & best regards,
Mason

CONFIDENTIALITY NOTE:

This e-mail and any attachments may contain confidential information 
and/or personal data, which is protected by applicable laws. Please be 
reminded that duplication, disclosure, distribution, or use of this e-mail 
(and/or its attachments) or any part thereof is prohibited. If you receive 
this e-mail in error, please notify us immediately and delete this mail as 
well as its attachment(s) from your system. In addition, please be 
informed that collection, processing, and/or use of personal data is 
prohibited unless expressly permitted by personal data protection laws. 
Thank you for your attention and cooperation.

Macronix International Co., Ltd.

=====================================================================



============================================================================

CONFIDENTIALITY NOTE:

This e-mail and any attachments may contain confidential information and/or personal data, which is protected by applicable laws. Please be reminded that duplication, disclosure, distribution, or use of this e-mail (and/or its attachments) or any part thereof is prohibited. If you receive this e-mail in error, please notify us immediately and delete this mail as well as its attachment(s) from your system. In addition, please be informed that collection, processing, and/or use of personal data is prohibited unless expressly permitted by personal data protection laws. Thank you for your attention and cooperation.

Macronix International Co., Ltd.

=====================================================================
Miquel Raynal May 27, 2019, 12:42 p.m. UTC | #8
Hi Mason,

masonccyang@mxic.com.tw wrote on Thu, 23 May 2019 16:58:02 +0800:

> Hi Miquel,
> 
> > >   
> > > > > +
> > > > > +static void mxic_nand_select_chip(struct nand_chip *chip, int   
> chipnr) 
> > > > 
> > > > _select_target() is preferred now   
> > > 
> > > Do you mean I implement mxic_nand_select_target() to control #CS ?
> > > 
> > > If so, I need to call mxic_nand_select_target( ) to control #CS ON
> > > and then #CS OFF in _exec_op() due to nand_select_target()<in   
> nand_base,c>  
> > > is still calling chip->legacy.select_chip ?  
> > 
> > You must forget about the ->select_chip() callback. Now it should be
> > handled directly from the controller driver. Please have a look at the
> > commit pointed against the marvell_nand.c driver.  
> 
> I have no Marvell NFC datasheet and have one question.
> 
> In marvell_nand.c, there is no xxx_deselect_target() or 
> something like that doing #CS OFF.
> marvell_nfc_select_target() seems always to make one of chip or die
> #CS keep low.
> 
> Is it right ?

Yes, AFAIR there is no "de-assert" mechanism in this controller.

> 
> How to make all #CS keep high for NAND to enter 
> low-power standby mode if driver don't use "legacy.select_chip()" ?

See commit 02b4a52604a4 ("mtd: rawnand: Make ->select_chip() optional
when ->exec_op() is implemented") which states:

        "When [->select_chip() is] not implemented, the core is assuming
	the CS line is automatically asserted/deasserted by the driver
	->exec_op() implementation."

Of course, the above is right only when the controller driver supports
the ->exec_op() interface. 

So if you think it is not too time consuming and worth the trouble to
assert/deassert the CS at each operation, you may do it in your driver.


Thanks,
Miquèl
Mason Yang May 29, 2019, 3:12 a.m. UTC | #9
Hi Miquel,

> > > > > > +static void mxic_nand_select_chip(struct nand_chip *chip, int 
 
> > chipnr) 
> > > > > 
> > > > > _select_target() is preferred now 
> > > > 
> > > > Do you mean I implement mxic_nand_select_target() to control #CS ?
> > > > 
> > > > If so, I need to call mxic_nand_select_target( ) to control #CS ON
> > > > and then #CS OFF in _exec_op() due to nand_select_target()<in 
> > nand_base,c> 
> > > > is still calling chip->legacy.select_chip ? 
> > > 
> > > You must forget about the ->select_chip() callback. Now it should be
> > > handled directly from the controller driver. Please have a look at 
the
> > > commit pointed against the marvell_nand.c driver. 
> > 
> > I have no Marvell NFC datasheet and have one question.
> > 
> > In marvell_nand.c, there is no xxx_deselect_target() or 
> > something like that doing #CS OFF.
> > marvell_nfc_select_target() seems always to make one of chip or die
> > #CS keep low.
> > 
> > Is it right ?
> 
> Yes, AFAIR there is no "de-assert" mechanism in this controller.
> 
> > 
> > How to make all #CS keep high for NAND to enter 
> > low-power standby mode if driver don't use "legacy.select_chip()" ?
> 
> See commit 02b4a52604a4 ("mtd: rawnand: Make ->select_chip() optional
> when ->exec_op() is implemented") which states:
> 
>         "When [->select_chip() is] not implemented, the core is assuming
>    the CS line is automatically asserted/deasserted by the driver
>    ->exec_op() implementation."
> 
> Of course, the above is right only when the controller driver supports
> the ->exec_op() interface. 

Currently, it seems that we will get the incorrect data and error
operation due to CS in error toggling if CS line is controlled in 
->exec_op().
i.e,. 

1) In nand_onfi_detect() to call nand_exec_op() twice by 
nand_read_param_page_op() and annd_read_data_op()

2) In nand_write_page_xxx to call nand_exec_op() many times by
nand_prog_page_begin_op(), nand_write_data_op() and 
nand_prog_page_end_op().


Should we consider to add a CS line controller in struct nand_controller
i.e,.

struct nand_controller {
         struct mutex lock;
         const struct nand_controller_ops *ops;
+          void (*select_chip)(struct nand_chip *chip, int cs);
};

to replace legacy.select_chip() ?


To patch in nand_select_target() and nand_deselect_target()

void nand_select_target(struct nand_chip *chip, unsigned int cs)
{
        /*
         * cs should always lie between 0 and chip->numchips, when that's 
not
         * the case it's a bug and the caller should be fixed.
         */
        if (WARN_ON(cs > chip->numchips))
                return;

        chip->cur_cs = cs;

+       if (chip->controller->select_chip)
+               chip->controller->select_chip(chip, cs);
+
        if (chip->legacy.select_chip)
                chip->legacy.select_chip(chip, cs);
}

void nand_deselect_target(struct nand_chip *chip)
{
+       if (chip->controller->select_chip)
+               chip->controller->select_chip(chip, -1);
+
        if (chip->legacy.select_chip)
                chip->legacy.select_chip(chip, -1);

        chip->cur_cs = -1;
}


> 
> So if you think it is not too time consuming and worth the trouble to
> assert/deassert the CS at each operation, you may do it in your driver.
> 
> 
> Thanks,
> Miquèl

thanks & best regards,
Mason

CONFIDENTIALITY NOTE:

This e-mail and any attachments may contain confidential information 
and/or personal data, which is protected by applicable laws. Please be 
reminded that duplication, disclosure, distribution, or use of this e-mail 
(and/or its attachments) or any part thereof is prohibited. If you receive 
this e-mail in error, please notify us immediately and delete this mail as 
well as its attachment(s) from your system. In addition, please be 
informed that collection, processing, and/or use of personal data is 
prohibited unless expressly permitted by personal data protection laws. 
Thank you for your attention and cooperation.

Macronix International Co., Ltd.

=====================================================================



============================================================================

CONFIDENTIALITY NOTE:

This e-mail and any attachments may contain confidential information and/or personal data, which is protected by applicable laws. Please be reminded that duplication, disclosure, distribution, or use of this e-mail (and/or its attachments) or any part thereof is prohibited. If you receive this e-mail in error, please notify us immediately and delete this mail as well as its attachment(s) from your system. In addition, please be informed that collection, processing, and/or use of personal data is prohibited unless expressly permitted by personal data protection laws. Thank you for your attention and cooperation.

Macronix International Co., Ltd.

=====================================================================

Patch
diff mbox series

diff --git a/drivers/mtd/nand/raw/Kconfig b/drivers/mtd/nand/raw/Kconfig
index e604625..e0329cc 100644
--- a/drivers/mtd/nand/raw/Kconfig
+++ b/drivers/mtd/nand/raw/Kconfig
@@ -522,6 +522,12 @@  config MTD_NAND_QCOM
 	  Enables support for NAND flash chips on SoCs containing the EBI2 NAND
 	  controller. This controller is found on IPQ806x SoC.
 
+config MTD_NAND_MXIC
+	tristate "Macronix MX25F0A NAND controller"
+	depends on HAS_IOMEM
+	help
+	  This selects the Macronix MX25F0A NAND controller driver.
+
 config MTD_NAND_MTK
 	tristate "Support for NAND controller on MTK SoCs"
 	depends on ARCH_MEDIATEK || COMPILE_TEST
diff --git a/drivers/mtd/nand/raw/Makefile b/drivers/mtd/nand/raw/Makefile
index 5a5a72f..c8a6790 100644
--- a/drivers/mtd/nand/raw/Makefile
+++ b/drivers/mtd/nand/raw/Makefile
@@ -54,6 +54,7 @@  obj-$(CONFIG_MTD_NAND_SUNXI)		+= sunxi_nand.o
 obj-$(CONFIG_MTD_NAND_HISI504)	        += hisi504_nand.o
 obj-$(CONFIG_MTD_NAND_BRCMNAND)		+= brcmnand/
 obj-$(CONFIG_MTD_NAND_QCOM)		+= qcom_nandc.o
+obj-$(CONFIG_MTD_NAND_MXIC)		+= mxic_nand.o
 obj-$(CONFIG_MTD_NAND_MTK)		+= mtk_ecc.o mtk_nand.o
 obj-$(CONFIG_MTD_NAND_TEGRA)		+= tegra_nand.o
 obj-$(CONFIG_MTD_NAND_STM32_FMC2)	+= stm32_fmc2_nand.o
diff --git a/drivers/mtd/nand/raw/mxic_nand.c b/drivers/mtd/nand/raw/mxic_nand.c
new file mode 100644
index 0000000..689fae2
--- /dev/null
+++ b/drivers/mtd/nand/raw/mxic_nand.c
@@ -0,0 +1,294 @@ 
+// SPDX-License-Identifier: GPL-2.0
+//
+// Copyright (C) 2019 Macronix International Co., Ltd.
+//
+// Authors:
+//	Mason Yang <masonccyang@mxic.com.tw>
+//	zhengxunli <zhengxunli@mxic.com.tw>
+//
+
+#include <linux/mfd/mxic-mx25f0a.h>
+#include <linux/mtd/rawnand.h>
+#include <linux/mtd/nand_ecc.h>
+
+#include "internals.h"
+
+struct mxic_nand_ctlr {
+	struct mx25f0a_mfd *mfd;
+	struct nand_controller base;
+	struct nand_chip nand;
+};
+
+static void mxic_host_init(struct mxic_nand_ctlr *mxic)
+{
+	writel(DATA_STROB_EDO_EN, mxic->mfd->regs + DATA_STROB);
+	writel(INT_STS_ALL, mxic->mfd->regs + INT_STS_EN);
+	writel(0x0, mxic->mfd->regs + ONFI_DIN_CNT(0));
+	writel(HC_CFG_NIO(8) | HC_CFG_SLV_ACT(0) | HC_CFG_IDLE_SIO_LVL(1) |
+	       HC_CFG_TYPE(1, HC_CFG_TYPE_RAW_NAND) | HC_CFG_MAN_CS_EN,
+	       mxic->mfd->regs + HC_CFG);
+	writel(0x0, mxic->mfd->regs + HC_EN);
+}
+
+static int  mxic_nand_wait_ready(struct nand_chip *chip)
+{
+	struct mxic_nand_ctlr *mxic = nand_get_controller_data(chip);
+	u32 sts;
+
+	return readl_poll_timeout(mxic->mfd->regs + INT_STS, sts,
+				  sts & INT_RDY_PIN, 0, USEC_PER_SEC);
+}
+
+static void mxic_nand_select_chip(struct nand_chip *chip, int chipnr)
+{
+	struct mxic_nand_ctlr *mxic = nand_get_controller_data(chip);
+
+	switch (chipnr) {
+	case 0:
+	case 1:
+		writel(HC_EN_BIT, mxic->mfd->regs + HC_EN);
+		writel(HC_CFG_MAN_CS_ASSERT | readl(mxic->mfd->regs + HC_CFG),
+		       mxic->mfd->regs + HC_CFG);
+		break;
+
+	case -1:
+		writel(~HC_CFG_MAN_CS_ASSERT & readl(mxic->mfd->regs + HC_CFG),
+		       mxic->mfd->regs + HC_CFG);
+		writel(0, mxic->mfd->regs + HC_EN);
+		break;
+
+	default:
+		break;
+	}
+}
+
+static int mxic_nand_data_xfer(struct mxic_nand_ctlr *mxic, const void *txbuf,
+			       void *rxbuf, unsigned int len)
+{
+	unsigned int pos = 0;
+
+	while (pos < len) {
+		unsigned int nbytes = len - pos;
+		u32 data = 0xffffffff;
+		u32 sts;
+		int ret;
+
+		if (nbytes > 4)
+			nbytes = 4;
+
+		if (txbuf)
+			memcpy(&data, txbuf + pos, nbytes);
+
+		ret = readl_poll_timeout(mxic->mfd->regs + INT_STS, sts,
+					 sts & INT_TX_EMPTY, 0, USEC_PER_SEC);
+		if (ret)
+			return ret;
+
+		writel(data, mxic->mfd->regs + TXD(nbytes % 4));
+
+		if (rxbuf) {
+			ret = readl_poll_timeout(mxic->mfd->regs + INT_STS, sts,
+						 sts & INT_TX_EMPTY, 0,
+						 USEC_PER_SEC);
+			if (ret)
+				return ret;
+
+			ret = readl_poll_timeout(mxic->mfd->regs + INT_STS, sts,
+						 sts & INT_RX_NOT_EMPTY, 0,
+						 USEC_PER_SEC);
+			if (ret)
+				return ret;
+
+			data = readl(mxic->mfd->regs + RXD);
+			data >>= (8 * (4 - nbytes));
+			memcpy(rxbuf + pos, &data, nbytes);
+			WARN_ON(readl(mxic->mfd->regs + INT_STS) &
+				INT_RX_NOT_EMPTY);
+		} else {
+			readl(mxic->mfd->regs + RXD);
+		}
+		WARN_ON(readl(mxic->mfd->regs + INT_STS) & INT_RX_NOT_EMPTY);
+
+		pos += nbytes;
+	}
+
+	return 0;
+}
+
+static int mxic_nand_exec_op(struct nand_chip *chip,
+			     const struct nand_operation *op, bool check_only)
+{
+	struct mxic_nand_ctlr *mxic = nand_get_controller_data(chip);
+	const struct nand_op_instr *instr = NULL;
+	int i, len = 0, ret = 0;
+	unsigned int op_id;
+	unsigned char cmdcnt = 0, addr_cnt = 0, cmd_addr[8] = {0};
+
+	for (op_id = 0; op_id < op->ninstrs; op_id++) {
+		instr = &op->instrs[op_id];
+
+		switch (instr->type) {
+		case NAND_OP_CMD_INSTR:
+			cmd_addr[len++] = instr->ctx.cmd.opcode;
+			cmdcnt++;
+			break;
+
+		case NAND_OP_ADDR_INSTR:
+			for (i = 0; i < instr->ctx.addr.naddrs; i++)
+				cmd_addr[len++] = instr->ctx.addr.addrs[i];
+			addr_cnt = i;
+			break;
+
+		case NAND_OP_DATA_IN_INSTR:
+			break;
+
+		case NAND_OP_DATA_OUT_INSTR:
+			writel(instr->ctx.data.len,
+			       mxic->mfd->regs + ONFI_DIN_CNT(0));
+			break;
+
+		case NAND_OP_WAITRDY_INSTR:
+			break;
+		}
+	}
+
+	if (op_id == 5 && instr->type == NAND_OP_WAITRDY_INSTR) {
+		/*
+		 * In case cmd-addr-data-cmd-wait in a sequence,
+		 * separate the 2'nd command, i.e,. nand_prog_page_op()
+		 */
+		writel(OP_CMD_BUSW(OP_BUSW_8) | OP_ADDR_BUSW(OP_BUSW_8) |
+		       OP_DATA_BUSW(OP_BUSW_8) | OP_DUMMY_CYC(0x3F) |
+		       OP_ADDR_BYTES(addr_cnt) |
+		       OP_CMD_BYTES(1), mxic->mfd->regs + SS_CTRL(0));
+		writel(0, mxic->mfd->regs + HC_EN);
+		writel(HC_EN_BIT, mxic->mfd->regs + HC_EN);
+
+		mxic_nand_data_xfer(mxic, cmd_addr, NULL, len - 1);
+
+		mxic_nand_data_xfer(mxic, instr->ctx.data.buf.out, NULL,
+				    instr->ctx.data.len);
+
+		writel(0, mxic->mfd->regs + HC_EN);
+		writel(HC_EN_BIT, mxic->mfd->regs + HC_EN);
+		mxic_nand_data_xfer(mxic, &cmd_addr[--len], NULL, 1);
+		ret = mxic_nand_wait_ready(chip);
+		if (ret)
+			goto err_out;
+		return ret;
+	}
+
+	if (len) {
+		writel(OP_CMD_BUSW(OP_BUSW_8) | OP_ADDR_BUSW(OP_BUSW_8) |
+		       OP_DATA_BUSW(OP_BUSW_8) | OP_DUMMY_CYC(0x3F) |
+		       OP_ADDR_BYTES(addr_cnt) |
+		       OP_CMD_BYTES(cmdcnt > 0 ? cmdcnt : 0),
+		       mxic->mfd->regs + SS_CTRL(0));
+		writel(0, mxic->mfd->regs + HC_EN);
+		writel(HC_EN_BIT, mxic->mfd->regs + HC_EN);
+
+		mxic_nand_data_xfer(mxic, cmd_addr, NULL, len);
+	}
+
+	for (op_id = 0; op_id < op->ninstrs; op_id++) {
+		instr = &op->instrs[op_id];
+
+		switch (instr->type) {
+		case NAND_OP_CMD_INSTR:
+		case NAND_OP_ADDR_INSTR:
+			break;
+
+		case NAND_OP_DATA_IN_INSTR:
+			writel(0x0, mxic->mfd->regs + ONFI_DIN_CNT(0));
+			writel(readl(mxic->mfd->regs + SS_CTRL(0)) | OP_READ,
+			       mxic->mfd->regs + SS_CTRL(0));
+			mxic_nand_data_xfer(mxic, NULL, instr->ctx.data.buf.in,
+					    instr->ctx.data.len);
+			break;
+
+		case NAND_OP_DATA_OUT_INSTR:
+			mxic_nand_data_xfer(mxic, instr->ctx.data.buf.out, NULL,
+					    instr->ctx.data.len);
+			break;
+
+		case NAND_OP_WAITRDY_INSTR:
+			ret = mxic_nand_wait_ready(chip);
+			if (ret)
+				goto err_out;
+			break;
+		}
+	}
+
+err_out:
+	return ret;
+}
+
+static const struct nand_controller_ops mxic_nand_controller_ops = {
+	.exec_op = mxic_nand_exec_op,
+};
+
+static int mx25f0a_nand_probe(struct platform_device *pdev)
+{
+	struct mtd_info *mtd;
+	struct mx25f0a_mfd *mfd = dev_get_drvdata(pdev->dev.parent);
+	struct mxic_nand_ctlr *mxic;
+	struct nand_chip *nand_chip;
+	int err;
+
+	mxic = devm_kzalloc(&pdev->dev, sizeof(struct mxic_nand_ctlr),
+			    GFP_KERNEL);
+	if (!mxic)
+		return -ENOMEM;
+
+	nand_chip = &mxic->nand;
+	mtd = nand_to_mtd(nand_chip);
+	mtd->dev.parent = pdev->dev.parent;
+	nand_chip->ecc.priv = NULL;
+	nand_set_flash_node(nand_chip, pdev->dev.parent->of_node);
+	nand_chip->priv = mxic;
+
+	mxic->mfd = mfd;
+
+	nand_chip->legacy.select_chip = mxic_nand_select_chip;
+	mxic->base.ops = &mxic_nand_controller_ops;
+	nand_controller_init(&mxic->base);
+	nand_chip->controller = &mxic->base;
+
+	mxic_host_init(mxic);
+
+	err = nand_scan(nand_chip, 1);
+	if (err)
+		goto fail;
+
+	err = mtd_device_register(mtd, NULL, 0);
+	if (err)
+		goto fail;
+
+	platform_set_drvdata(pdev, mxic);
+
+	return 0;
+fail:
+	return err;
+}
+
+static int mx25f0a_nand_remove(struct platform_device *pdev)
+{
+	struct mxic_nand_ctlr *mxic = platform_get_drvdata(pdev);
+
+	nand_release(&mxic->nand);
+
+	return 0;
+}
+
+static struct platform_driver mx25f0a_nand_driver = {
+	.probe		= mx25f0a_nand_probe,
+	.remove		= mx25f0a_nand_remove,
+	.driver		= {
+		.name	= "mxic-nand-ctlr",
+	},
+};
+module_platform_driver(mx25f0a_nand_driver);
+
+MODULE_AUTHOR("Mason Yang <masonccyang@mxic.com.tw>");
+MODULE_DESCRIPTION("MX25F0A RAW NAND controller driver");
+MODULE_LICENSE("GPL v2");