[PATCH 0/9] mtd: rawnand: bcm47xx: Convert the driver exec_op()

[PATCH 0/9] mtd: rawnand: bcm47xx: Convert the driver exec_op()

[Boris Brezillon]

Hello,

Recently I've been CC-ed on a few new driver submissions that were
implementing the legacy interface, which made me realize the only way
to prevent that from happening was to:

1/ convert all existing drivers to exec_op()
2/ document the new way of doing things

#2 is definitely needed, but I don't think it's be enough, as people
tend to re-shuffle what they had in their downstream kernel when
they submit something upstream, and those downstream drivers were most
likely based on the legacy cmd_ctrl/cmdfunc() interface.

So here I am, trying to convert existing drivers one by one. I'd be
grateful if someone from the OpenWRT community (Rafal?) could test/help
me debug that one as I don't have the HW myself.

Regards,

Boris

Boris Brezillon (9):
  mtd: rawnand: Add an is_last flag to nand_subop
  mtd: rawnand: bcm47xx: Drop dependency on BCMA
  mtd: rawnand: bcm47xx: Allow compiling the driver when COMPILE_TEST=y
  mtd: rawnand: bcm47xx: Demistify a few more things
  mtd: rawnand: bcm47xx: Implement the exec_op() interface
  mtd: rawnand: bcm47xx: Get rid of the legacy implementation
  mtd: rawnand: bcm47xx: Simplify the init() function
  mtd: rawnand: bcm47xx: Merge all source files
  mtd: rawnand: bcm47xx: Move the driver to drivers/mtd/nand/raw/

 drivers/mtd/nand/raw/Kconfig                  |   3 +-
 drivers/mtd/nand/raw/Makefile                 |   1 +
 drivers/mtd/nand/raw/bcm47xxnflash.c          | 353 ++++++++++++++
 drivers/mtd/nand/raw/bcm47xxnflash/Makefile   |   5 -
 .../nand/raw/bcm47xxnflash/bcm47xxnflash.h    |  26 -
 drivers/mtd/nand/raw/bcm47xxnflash/main.c     |  77 ---
 .../mtd/nand/raw/bcm47xxnflash/ops_bcm4706.c  | 450 ------------------
 drivers/mtd/nand/raw/nand_base.c              |   2 +
 include/linux/mtd/rawnand.h                   |   2 +
 9 files changed, 359 insertions(+), 560 deletions(-)
 create mode 100644 drivers/mtd/nand/raw/bcm47xxnflash.c
 delete mode 100644 drivers/mtd/nand/raw/bcm47xxnflash/Makefile
 delete mode 100644 drivers/mtd/nand/raw/bcm47xxnflash/bcm47xxnflash.h
 delete mode 100644 drivers/mtd/nand/raw/bcm47xxnflash/main.c
 delete mode 100644 drivers/mtd/nand/raw/bcm47xxnflash/ops_bcm4706.c

-- 
2.25.2


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

[PATCH 1/9] mtd: rawnand: Add an is_last flag to nand_subop

[Boris Brezillon]

Some controllers need to know when they're passed the last subop so
they can de-assert the CE pin.

Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
---
 drivers/mtd/nand/raw/nand_base.c | 2 ++
 include/linux/mtd/rawnand.h      | 2 ++
 2 files changed, 4 insertions(+)

diff --git a/drivers/mtd/nand/raw/nand_base.c b/drivers/mtd/nand/raw/nand_base.c
index fa9ac18e97a1..f81b54634061 100644
--- a/drivers/mtd/nand/raw/nand_base.c
+++ b/drivers/mtd/nand/raw/nand_base.c
@@ -2101,6 +2101,8 @@ nand_op_parser_match_pat(const struct nand_op_parser_pattern *pat,
 	 */
 	ctx->subop.ninstrs = ninstrs;
 	ctx->subop.last_instr_end_off = instr_offset;
+	if (ctx->subop.instrs + ninstrs == end && !instr_offset)
+		ctx->subop.is_last = true;
 
 	return true;
 }
diff --git a/include/linux/mtd/rawnand.h b/include/linux/mtd/rawnand.h
index 8e8d1a61e2fb..99f4ac47c8d3 100644
--- a/include/linux/mtd/rawnand.h
+++ b/include/linux/mtd/rawnand.h
@@ -701,6 +701,7 @@ struct nand_op_instr {
  *			   of the sub-operation
  * @last_instr_end_off: offset to end at (excluded) for the last instruction
  *			of the sub-operation
+ * @is_last: this sub-operation is the last one
  *
  * Both @first_instr_start_off and @last_instr_end_off only apply to data or
  * address instructions.
@@ -715,6 +716,7 @@ struct nand_subop {
 	unsigned int ninstrs;
 	unsigned int first_instr_start_off;
 	unsigned int last_instr_end_off;
+	bool is_last;
 };
 
 unsigned int nand_subop_get_addr_start_off(const struct nand_subop *subop,
-- 
2.25.2


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

[PATCH 2/9] mtd: rawnand: bcm47xx: Drop dependency on BCMA

[Boris Brezillon]

BCMA_NFLASH can only be selected if BCMA is enabled and we already
depend on BCMA_NFLASH, making the dependency on BCMA useless.

Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
---
 drivers/mtd/nand/raw/Kconfig | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/mtd/nand/raw/Kconfig b/drivers/mtd/nand/raw/Kconfig
index a80a46bb5b8b..60c4eb9d382f 100644
--- a/drivers/mtd/nand/raw/Kconfig
+++ b/drivers/mtd/nand/raw/Kconfig
@@ -243,7 +243,6 @@ config MTD_NAND_BRCMNAND
 config MTD_NAND_BCM47XXNFLASH
 	tristate "BCM4706 BCMA NAND controller"
 	depends on BCMA_NFLASH
-	depends on BCMA
 	help
 	  BCMA bus can have various flash memories attached, they are
 	  registered by bcma as platform devices. This enables driver for
-- 
2.25.2


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

[PATCH 3/9] mtd: rawnand: bcm47xx: Allow compiling the driver when COMPILE_TEST=y

[Boris Brezillon]

Makes it easier to spot compile-time issues.

Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
---
 drivers/mtd/nand/raw/Kconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/mtd/nand/raw/Kconfig b/drivers/mtd/nand/raw/Kconfig
index 60c4eb9d382f..1f8aa353f764 100644
--- a/drivers/mtd/nand/raw/Kconfig
+++ b/drivers/mtd/nand/raw/Kconfig
@@ -242,7 +242,7 @@ config MTD_NAND_BRCMNAND
 
 config MTD_NAND_BCM47XXNFLASH
 	tristate "BCM4706 BCMA NAND controller"
-	depends on BCMA_NFLASH
+	depends on BCMA_NFLASH || COMPILE_TEST
 	help
 	  BCMA bus can have various flash memories attached, they are
 	  registered by bcma as platform devices. This enables driver for
-- 
2.25.2


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

[PATCH 4/9] mtd: rawnand: bcm47xx: Demistify a few more things

[Boris Brezillon]

There were a few places were raw hex values were used instead of the
macro def.

We also add macros to help forming the conf value (note that we still
have one magic bit whose meaning I couldn't extract from the code), and
add an extra macro to specify the number of DATA cycles to issue when
the READ or WRITE flag is set.

Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
---
 .../mtd/nand/raw/bcm47xxnflash/ops_bcm4706.c  | 34 +++++++++++++++----
 1 file changed, 27 insertions(+), 7 deletions(-)

diff --git a/drivers/mtd/nand/raw/bcm47xxnflash/ops_bcm4706.c b/drivers/mtd/nand/raw/bcm47xxnflash/ops_bcm4706.c
index 591775173034..fbb7acebc8f7 100644
--- a/drivers/mtd/nand/raw/bcm47xxnflash/ops_bcm4706.c
+++ b/drivers/mtd/nand/raw/bcm47xxnflash/ops_bcm4706.c
@@ -25,12 +25,29 @@
 #define NCTL_CMD1W			0x00080000
 #define NCTL_READ			0x00100000
 #define NCTL_WRITE			0x00200000
+/* When the SPECADDR is set CMD1 is interpreted as a single ADDR cycle */
 #define NCTL_SPECADDR			0x01000000
 #define NCTL_READY			0x04000000
 #define NCTL_ERR			0x08000000
+/*
+ * Number of DATA cycles to issue when NCTL_{READ,WRITE} is set. The minimum
+ * value is 1 and the maximum value is 4. Those bytes are then stored in the
+ * BCMA_CC_NFLASH_DATA register.
+ */
+#define NCTL_DATA_CYCLES(x)		((((x) - 1) & 0x3) << 28)
+/*
+ * The CS pin seems to be asserted even if NCTL_CSA is not set. All this bit
+ * seems to encode is whether the CS line should stay asserted after the
+ * operation has been executed. In other words, you should only set it if if
+ * you intend to do more operations on the NAND bus.
+ */
 #define NCTL_CSA			0x40000000
 #define NCTL_START			0x80000000
 
+#define CONF_MAGIC_BIT			0x00000002
+#define CONF_COL_BYTES(x)		(((x) - 1) << 4)
+#define CONF_ROW_BYTES(x)		(((x) - 1) << 6)
+
 /**************************************************
  * Various helpers
  **************************************************/
@@ -118,7 +135,7 @@ static void bcm47xxnflash_ops_bcm4706_read(struct mtd_info *mtd, uint8_t *buf,
 
 		/* Eventually read some data :) */
 		for (i = 0; i < toread; i += 4, dest++) {
-			ctlcode = NCTL_CSA | 0x30000000 | NCTL_READ;
+			ctlcode = NCTL_CSA | NCTL_DATA_CYCLES(4) | NCTL_READ;
 			if (i == toread - 4) /* Last read goes without that */
 				ctlcode &= ~NCTL_CSA;
 			if (bcm47xxnflash_ops_bcm4706_ctl_cmd(b47n->cc,
@@ -150,7 +167,7 @@ static void bcm47xxnflash_ops_bcm4706_write(struct mtd_info *mtd,
 	for (i = 0; i < len; i += 4, data++) {
 		bcma_cc_write32(cc, BCMA_CC_NFLASH_DATA, *data);
 
-		ctlcode = NCTL_CSA | 0x30000000 | NCTL_WRITE;
+		ctlcode = NCTL_CSA | NCTL_DATA_CYCLES(4) | NCTL_WRITE;
 		if (i == len - 4) /* Last read goes without that */
 			ctlcode &= ~NCTL_CSA;
 		if (bcm47xxnflash_ops_bcm4706_ctl_cmd(cc, ctlcode)) {
@@ -229,7 +246,7 @@ static void bcm47xxnflash_ops_bcm4706_cmdfunc(struct nand_chip *nand_chip,
 		nand_wait_ready(nand_chip);
 		break;
 	case NAND_CMD_READID:
-		ctlcode = NCTL_CSA | 0x01000000 | NCTL_CMD1W | NCTL_CMD0;
+		ctlcode = NCTL_CSA | NCTL_SPECADDR | NCTL_CMD1W | NCTL_CMD0;
 		ctlcode |= NAND_CMD_READID;
 		if (bcm47xxnflash_ops_bcm4706_ctl_cmd(b47n->cc, ctlcode)) {
 			pr_err("READID error\n");
@@ -242,7 +259,7 @@ static void bcm47xxnflash_ops_bcm4706_cmdfunc(struct nand_chip *nand_chip,
 		 * to perform, so cache everything.
 		 */
 		for (i = 0; i < ARRAY_SIZE(b47n->id_data); i++) {
-			ctlcode = NCTL_CSA | NCTL_READ;
+			ctlcode = NCTL_CSA | NCTL_READ | NCTL_DATA_CYCLES(1);
 			if (i == ARRAY_SIZE(b47n->id_data) - 1)
 				ctlcode &= ~NCTL_CSA;
 			if (bcm47xxnflash_ops_bcm4706_ctl_cmd(b47n->cc,
@@ -285,7 +302,7 @@ static void bcm47xxnflash_ops_bcm4706_cmdfunc(struct nand_chip *nand_chip,
 				b47n->curr_page_addr);
 
 		/* Prepare to write */
-		ctlcode = 0x40000000 | NCTL_ROW | NCTL_COL | NCTL_CMD0;
+		ctlcode = NCTL_CSA | NCTL_ROW | NCTL_COL | NCTL_CMD0;
 		ctlcode |= NAND_CMD_SEQIN;
 		if (bcm47xxnflash_ops_bcm4706_ctl_cmd(cc, ctlcode))
 			pr_err("SEQIN failed\n");
@@ -320,7 +337,9 @@ static u8 bcm47xxnflash_ops_bcm4706_read_byte(struct nand_chip *nand_chip)
 		}
 		return b47n->id_data[b47n->curr_column++];
 	case NAND_CMD_STATUS:
-		if (bcm47xxnflash_ops_bcm4706_ctl_cmd(cc, NCTL_READ))
+		if (bcm47xxnflash_ops_bcm4706_ctl_cmd(cc,
+						      NCTL_READ |
+						      NCTL_DATA_CYCLES(1)))
 			return 0;
 		return bcma_cc_read32(cc, BCMA_CC_NFLASH_DATA) & 0xff;
 	case NAND_CMD_READOOB:
@@ -439,7 +458,8 @@ int bcm47xxnflash_ops_bcm4706_init(struct bcm47xxnflash *b47n)
 	row_bits = tbits - col_bits + 1;
 	row_bsize = (row_bits + 7) / 8;
 
-	val = ((row_bsize - 1) << 6) | ((col_size - 1) << 4) | 2;
+	val = CONF_ROW_BYTES(row_bsize) | CONF_COL_BYTES(col_size) |
+	      CONF_MAGIC_BIT;
 	bcma_cc_write32(b47n->cc, BCMA_CC_NFLASH_CONF, val);
 
 exit:
-- 
2.25.2


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

[PATCH 5/9] mtd: rawnand: bcm47xx: Implement the exec_op() interface

[Boris Brezillon]

Implement the exec_op() interface so we can get rid of the convoluted
cmdfunc() implementation.

Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
---
This is based on my understanding of how this controller works, and I
think it covers all the use cases covered by the custom cmdfunc()
implementation. I might be wrong of course, so it'd be great to have
someone test on real HW.
---
 .../nand/raw/bcm47xxnflash/bcm47xxnflash.h    |   1 +
 .../mtd/nand/raw/bcm47xxnflash/ops_bcm4706.c  | 150 ++++++++++++++++++
 2 files changed, 151 insertions(+)

diff --git a/drivers/mtd/nand/raw/bcm47xxnflash/bcm47xxnflash.h b/drivers/mtd/nand/raw/bcm47xxnflash/bcm47xxnflash.h
index 201b9baa52a0..00d0974b73cb 100644
--- a/drivers/mtd/nand/raw/bcm47xxnflash/bcm47xxnflash.h
+++ b/drivers/mtd/nand/raw/bcm47xxnflash/bcm47xxnflash.h
@@ -10,6 +10,7 @@
 #include <linux/mtd/rawnand.h>
 
 struct bcm47xxnflash {
+	struct nand_controller base;
 	struct bcma_drv_cc *cc;
 
 	struct nand_chip nand_chip;
diff --git a/drivers/mtd/nand/raw/bcm47xxnflash/ops_bcm4706.c b/drivers/mtd/nand/raw/bcm47xxnflash/ops_bcm4706.c
index fbb7acebc8f7..184f78b3d45a 100644
--- a/drivers/mtd/nand/raw/bcm47xxnflash/ops_bcm4706.c
+++ b/drivers/mtd/nand/raw/bcm47xxnflash/ops_bcm4706.c
@@ -382,6 +382,153 @@ static void bcm47xxnflash_ops_bcm4706_write_buf(struct nand_chip *nand_chip,
 	pr_err("Invalid command for buf write: 0x%X\n", b47n->curr_command);
 }
 
+static int
+bcm47xxnflash_ops_bcm4706_exec_cmd_addr(struct nand_chip *chip,
+					const struct nand_subop *subop)
+{
+	struct bcm47xxnflash *b47n = nand_get_controller_data(chip);
+	u32 nctl = 0, col = 0, row = 0, ncols = 0, nrows = 0;
+	unsigned int i, j;
+
+	for (i = 0; i < subop->ninstrs; i++) {
+		const struct nand_op_instr *instr = &subop->instrs[i];
+
+		switch (instr->type) {
+		case NAND_OP_CMD_INSTR:
+			if (WARN_ON_ONCE((nctl & NCTL_CMD0) &&
+					 (nctl & NCTL_CMD1W)))
+				return -EINVAL;
+			else if (nctl & NCTL_CMD0)
+				nctl |= NCTL_CMD1W |
+					((u32)instr->ctx.cmd.opcode << 8);
+			else
+				nctl |= NCTL_CMD0 | instr->ctx.cmd.opcode;
+			break;
+		case NAND_OP_ADDR_INSTR:
+			for (j = 0; j < instr->ctx.addr.naddrs; j++) {
+				u32 addr = instr->ctx.addr.addrs[j];
+
+				if (i < 2) {
+					col |= addr << i * 8;
+					nctl |= NCTL_COL;
+					ncols++;
+				} else {
+					row |= addr << (i - 2) * 8;
+					nctl |= NCTL_ROW;
+					nrows++;
+				}
+			}
+			break;
+		default:
+			WARN_ON_ONCE(1);
+			return -EINVAL;
+		}
+	}
+
+	/* Keep the CS line asserted if there's something else to execute. */
+	if (!subop->is_last)
+		nctl |= NCTL_CSA;
+
+	bcma_cc_write32(b47n->cc, BCMA_CC_NFLASH_CONF,
+			CONF_MAGIC_BIT |
+			CONF_COL_BYTES(ncols) |
+			CONF_ROW_BYTES(nrows));
+	return bcm47xxnflash_ops_bcm4706_ctl_cmd(b47n->cc, nctl);
+}
+
+static int
+bcm47xxnflash_ops_bcm4706_exec_waitrdy(struct nand_chip *chip,
+				       const struct nand_subop *subop)
+{
+	struct bcm47xxnflash *b47n = nand_get_controller_data(chip);
+	const struct nand_op_instr *instr = &subop->instrs[0];
+	unsigned long timeout_jiffies = jiffies;
+
+	if (WARN_ON(subop->ninstrs != 1 ||
+		    instr->type != NAND_OP_DATA_IN_INSTR))
+		return -EINVAL;
+
+	timeout_jiffies += msecs_to_jiffies(instr->ctx.waitrdy.timeout_ms) + 1;
+	do {
+		if (bcma_cc_read32(b47n->cc, BCMA_CC_NFLASH_CTL) & NCTL_READY)
+			return 0;
+
+		usleep_range(10, 100);
+	} while (time_before(jiffies, timeout_jiffies));
+
+	return -ETIMEDOUT;
+}
+
+static int
+bcm47xxnflash_ops_bcm4706_exec_rw(struct nand_chip *chip,
+				  const struct nand_subop *subop)
+{
+	struct bcm47xxnflash *b47n = nand_get_controller_data(chip);
+	const struct nand_op_instr *instr = &subop->instrs[0];
+	unsigned int i;
+	int ret;
+
+	if (WARN_ON(subop->ninstrs != 1 ||
+		    (instr->type != NAND_OP_DATA_IN_INSTR &&
+		     instr->type != NAND_OP_DATA_OUT_INSTR)))
+		return -EINVAL;
+
+	for (i = 0; i < instr->ctx.data.len; i += 4) {
+		unsigned int nbytes = min_t(unsigned int,
+					    instr->ctx.data.len - i, 4);
+		u32 nctl, data;
+
+		nctl = NCTL_CSA | NCTL_DATA_CYCLES(nbytes);
+		if (instr->type == NAND_OP_DATA_IN_INSTR) {
+			nctl |= NCTL_READ;
+		} else {
+			nctl |= NCTL_WRITE;
+			memcpy(&data, instr->ctx.data.buf.in + i, nbytes);
+			bcma_cc_write32(b47n->cc, BCMA_CC_NFLASH_DATA, data);
+		}
+
+		if (i + nbytes < instr->ctx.data.len)
+			nctl |= NCTL_CSA;
+
+		ret = bcm47xxnflash_ops_bcm4706_ctl_cmd(b47n->cc, nctl);
+		if (ret)
+			return ret;
+
+		if (instr->type == NAND_OP_DATA_IN_INSTR) {
+			data = bcma_cc_read32(b47n->cc, BCMA_CC_NFLASH_DATA);
+			memcpy(instr->ctx.data.buf.in + i, &data, nbytes);
+		}
+	}
+
+	return 0;
+}
+
+static const struct nand_op_parser bcm47xxnflash_op_parser = NAND_OP_PARSER(
+	NAND_OP_PARSER_PATTERN(bcm47xxnflash_ops_bcm4706_exec_cmd_addr,
+			       NAND_OP_PARSER_PAT_CMD_ELEM(true),
+			       NAND_OP_PARSER_PAT_ADDR_ELEM(true, 5),
+			       NAND_OP_PARSER_PAT_CMD_ELEM(true)),
+	NAND_OP_PARSER_PATTERN(bcm47xxnflash_ops_bcm4706_exec_waitrdy,
+			       NAND_OP_PARSER_PAT_WAITRDY_ELEM(false)),
+	NAND_OP_PARSER_PATTERN(bcm47xxnflash_ops_bcm4706_exec_rw,
+			       NAND_OP_PARSER_PAT_DATA_IN_ELEM(false, 0x200)),
+	NAND_OP_PARSER_PATTERN(bcm47xxnflash_ops_bcm4706_exec_rw,
+			       NAND_OP_PARSER_PAT_DATA_OUT_ELEM(false, 0x200)),
+);
+
+static int
+bcm47xxnflash_ops_bcm4706_exec_op(struct nand_chip *chip,
+				  const struct nand_operation *op,
+				  bool check_only)
+{
+	return nand_op_parser_exec_op(chip, &bcm47xxnflash_op_parser, op,
+				      check_only);
+}
+
+static const struct nand_controller_ops bcm47xxnflash_ops = {
+	.exec_op = bcm47xxnflash_ops_bcm4706_exec_op,
+};
+
 /**************************************************
  * Init
  **************************************************/
@@ -398,6 +545,9 @@ int bcm47xxnflash_ops_bcm4706_init(struct bcm47xxnflash *b47n)
 	u8 tbits, col_bits, col_size, row_bits, row_bsize;
 	u32 val;
 
+	nand_controller_init(&b47n->base);
+	b47n->base.ops = &bcm47xxnflash_ops;
+	b47n->nand_chip.controller = &b47n->base;
 	nand_chip->legacy.select_chip = bcm47xxnflash_ops_bcm4706_select_chip;
 	nand_chip->legacy.cmd_ctrl = bcm47xxnflash_ops_bcm4706_cmd_ctrl;
 	nand_chip->legacy.dev_ready = bcm47xxnflash_ops_bcm4706_dev_ready;
-- 
2.25.2


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

[PATCH 6/9] mtd: rawnand: bcm47xx: Get rid of the legacy implementation

[Boris Brezillon]

Now that exec_op() is implemented we don't need to implement all those
legacy hooks, which simplifies the code quite a bit.

Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
---
 .../nand/raw/bcm47xxnflash/bcm47xxnflash.h    |   6 -
 .../mtd/nand/raw/bcm47xxnflash/ops_bcm4706.c  | 315 ------------------
 2 files changed, 321 deletions(-)

diff --git a/drivers/mtd/nand/raw/bcm47xxnflash/bcm47xxnflash.h b/drivers/mtd/nand/raw/bcm47xxnflash/bcm47xxnflash.h
index 00d0974b73cb..8de0e7e0a3a4 100644
--- a/drivers/mtd/nand/raw/bcm47xxnflash/bcm47xxnflash.h
+++ b/drivers/mtd/nand/raw/bcm47xxnflash/bcm47xxnflash.h
@@ -14,12 +14,6 @@ struct bcm47xxnflash {
 	struct bcma_drv_cc *cc;
 
 	struct nand_chip nand_chip;
-
-	unsigned curr_command;
-	int curr_page_addr;
-	int curr_column;
-
-	u8 id_data[8];
 };
 
 int bcm47xxnflash_ops_bcm4706_init(struct bcm47xxnflash *b47n);
diff --git a/drivers/mtd/nand/raw/bcm47xxnflash/ops_bcm4706.c b/drivers/mtd/nand/raw/bcm47xxnflash/ops_bcm4706.c
index 184f78b3d45a..4e38b685d207 100644
--- a/drivers/mtd/nand/raw/bcm47xxnflash/ops_bcm4706.c
+++ b/drivers/mtd/nand/raw/bcm47xxnflash/ops_bcm4706.c
@@ -75,313 +75,10 @@ static int bcm47xxnflash_ops_bcm4706_ctl_cmd(struct bcma_drv_cc *cc, u32 code)
 	return 0;
 }
 
-static int bcm47xxnflash_ops_bcm4706_poll(struct bcma_drv_cc *cc)
-{
-	int i;
-
-	for (i = 0; i < NFLASH_READY_RETRIES; i++) {
-		if (bcma_cc_read32(cc, BCMA_CC_NFLASH_CTL) & NCTL_READY) {
-			if (bcma_cc_read32(cc, BCMA_CC_NFLASH_CTL) &
-			    BCMA_CC_NFLASH_CTL_ERR) {
-				pr_err("Error on polling\n");
-				return -EBUSY;
-			} else {
-				return 0;
-			}
-		}
-	}
-
-	pr_err("Polling timeout!\n");
-	return -EBUSY;
-}
-
-/**************************************************
- * R/W
- **************************************************/
-
-static void bcm47xxnflash_ops_bcm4706_read(struct mtd_info *mtd, uint8_t *buf,
-					   int len)
-{
-	struct nand_chip *nand_chip = mtd_to_nand(mtd);
-	struct bcm47xxnflash *b47n = nand_get_controller_data(nand_chip);
-
-	u32 ctlcode;
-	u32 *dest = (u32 *)buf;
-	int i;
-	int toread;
-
-	BUG_ON(b47n->curr_page_addr & ~nand_chip->pagemask);
-	/* Don't validate column using nand_chip->page_shift, it may be bigger
-	 * when accessing OOB */
-
-	while (len) {
-		/* We can read maximum of 0x200 bytes at once */
-		toread = min(len, 0x200);
-
-		/* Set page and column */
-		bcma_cc_write32(b47n->cc, BCMA_CC_NFLASH_COL_ADDR,
-				b47n->curr_column);
-		bcma_cc_write32(b47n->cc, BCMA_CC_NFLASH_ROW_ADDR,
-				b47n->curr_page_addr);
-
-		/* Prepare to read */
-		ctlcode = NCTL_CSA | NCTL_CMD1W | NCTL_ROW | NCTL_COL |
-			  NCTL_CMD0;
-		ctlcode |= NAND_CMD_READSTART << 8;
-		if (bcm47xxnflash_ops_bcm4706_ctl_cmd(b47n->cc, ctlcode))
-			return;
-		if (bcm47xxnflash_ops_bcm4706_poll(b47n->cc))
-			return;
-
-		/* Eventually read some data :) */
-		for (i = 0; i < toread; i += 4, dest++) {
-			ctlcode = NCTL_CSA | NCTL_DATA_CYCLES(4) | NCTL_READ;
-			if (i == toread - 4) /* Last read goes without that */
-				ctlcode &= ~NCTL_CSA;
-			if (bcm47xxnflash_ops_bcm4706_ctl_cmd(b47n->cc,
-							      ctlcode))
-				return;
-			*dest = bcma_cc_read32(b47n->cc, BCMA_CC_NFLASH_DATA);
-		}
-
-		b47n->curr_column += toread;
-		len -= toread;
-	}
-}
-
-static void bcm47xxnflash_ops_bcm4706_write(struct mtd_info *mtd,
-					    const uint8_t *buf, int len)
-{
-	struct nand_chip *nand_chip = mtd_to_nand(mtd);
-	struct bcm47xxnflash *b47n = nand_get_controller_data(nand_chip);
-	struct bcma_drv_cc *cc = b47n->cc;
-
-	u32 ctlcode;
-	const u32 *data = (u32 *)buf;
-	int i;
-
-	BUG_ON(b47n->curr_page_addr & ~nand_chip->pagemask);
-	/* Don't validate column using nand_chip->page_shift, it may be bigger
-	 * when accessing OOB */
-
-	for (i = 0; i < len; i += 4, data++) {
-		bcma_cc_write32(cc, BCMA_CC_NFLASH_DATA, *data);
-
-		ctlcode = NCTL_CSA | NCTL_DATA_CYCLES(4) | NCTL_WRITE;
-		if (i == len - 4) /* Last read goes without that */
-			ctlcode &= ~NCTL_CSA;
-		if (bcm47xxnflash_ops_bcm4706_ctl_cmd(cc, ctlcode)) {
-			pr_err("%s ctl_cmd didn't work!\n", __func__);
-			return;
-		}
-	}
-
-	b47n->curr_column += len;
-}
-
 /**************************************************
  * NAND chip ops
  **************************************************/
 
-static void bcm47xxnflash_ops_bcm4706_cmd_ctrl(struct nand_chip *nand_chip,
-					       int cmd, unsigned int ctrl)
-{
-	struct bcm47xxnflash *b47n = nand_get_controller_data(nand_chip);
-	u32 code = 0;
-
-	if (cmd == NAND_CMD_NONE)
-		return;
-
-	if (cmd & NAND_CTRL_CLE)
-		code = cmd | NCTL_CMD0;
-
-	/* nCS is not needed for reset command */
-	if (cmd != NAND_CMD_RESET)
-		code |= NCTL_CSA;
-
-	bcm47xxnflash_ops_bcm4706_ctl_cmd(b47n->cc, code);
-}
-
-/* Default nand_select_chip calls cmd_ctrl, which is not used in BCM4706 */
-static void bcm47xxnflash_ops_bcm4706_select_chip(struct nand_chip *chip,
-						  int cs)
-{
-	return;
-}
-
-static int bcm47xxnflash_ops_bcm4706_dev_ready(struct nand_chip *nand_chip)
-{
-	struct bcm47xxnflash *b47n = nand_get_controller_data(nand_chip);
-
-	return !!(bcma_cc_read32(b47n->cc, BCMA_CC_NFLASH_CTL) & NCTL_READY);
-}
-
-/*
- * Default nand_command and nand_command_lp don't match BCM4706 hardware layout.
- * For example, reading chip id is performed in a non-standard way.
- * Setting column and page is also handled differently, we use a special
- * registers of ChipCommon core. Hacking cmd_ctrl to understand and convert
- * standard commands would be much more complicated.
- */
-static void bcm47xxnflash_ops_bcm4706_cmdfunc(struct nand_chip *nand_chip,
-					      unsigned command, int column,
-					      int page_addr)
-{
-	struct mtd_info *mtd = nand_to_mtd(nand_chip);
-	struct bcm47xxnflash *b47n = nand_get_controller_data(nand_chip);
-	struct bcma_drv_cc *cc = b47n->cc;
-	u32 ctlcode;
-	int i;
-
-	if (column != -1)
-		b47n->curr_column = column;
-	if (page_addr != -1)
-		b47n->curr_page_addr = page_addr;
-
-	switch (command) {
-	case NAND_CMD_RESET:
-		nand_chip->legacy.cmd_ctrl(nand_chip, command, NAND_CTRL_CLE);
-
-		ndelay(100);
-		nand_wait_ready(nand_chip);
-		break;
-	case NAND_CMD_READID:
-		ctlcode = NCTL_CSA | NCTL_SPECADDR | NCTL_CMD1W | NCTL_CMD0;
-		ctlcode |= NAND_CMD_READID;
-		if (bcm47xxnflash_ops_bcm4706_ctl_cmd(b47n->cc, ctlcode)) {
-			pr_err("READID error\n");
-			break;
-		}
-
-		/*
-		 * Reading is specific, last one has to go without NCTL_CSA
-		 * bit. We don't know how many reads NAND subsystem is going
-		 * to perform, so cache everything.
-		 */
-		for (i = 0; i < ARRAY_SIZE(b47n->id_data); i++) {
-			ctlcode = NCTL_CSA | NCTL_READ | NCTL_DATA_CYCLES(1);
-			if (i == ARRAY_SIZE(b47n->id_data) - 1)
-				ctlcode &= ~NCTL_CSA;
-			if (bcm47xxnflash_ops_bcm4706_ctl_cmd(b47n->cc,
-							      ctlcode)) {
-				pr_err("READID error\n");
-				break;
-			}
-			b47n->id_data[i] =
-				bcma_cc_read32(b47n->cc, BCMA_CC_NFLASH_DATA)
-				& 0xFF;
-		}
-
-		break;
-	case NAND_CMD_STATUS:
-		ctlcode = NCTL_CSA | NCTL_CMD0 | NAND_CMD_STATUS;
-		if (bcm47xxnflash_ops_bcm4706_ctl_cmd(cc, ctlcode))
-			pr_err("STATUS command error\n");
-		break;
-	case NAND_CMD_READ0:
-		break;
-	case NAND_CMD_READOOB:
-		if (page_addr != -1)
-			b47n->curr_column += mtd->writesize;
-		break;
-	case NAND_CMD_ERASE1:
-		bcma_cc_write32(cc, BCMA_CC_NFLASH_ROW_ADDR,
-				b47n->curr_page_addr);
-		ctlcode = NCTL_ROW | NCTL_CMD1W | NCTL_CMD0 |
-			  NAND_CMD_ERASE1 | (NAND_CMD_ERASE2 << 8);
-		if (bcm47xxnflash_ops_bcm4706_ctl_cmd(cc, ctlcode))
-			pr_err("ERASE1 failed\n");
-		break;
-	case NAND_CMD_ERASE2:
-		break;
-	case NAND_CMD_SEQIN:
-		/* Set page and column */
-		bcma_cc_write32(cc, BCMA_CC_NFLASH_COL_ADDR,
-				b47n->curr_column);
-		bcma_cc_write32(cc, BCMA_CC_NFLASH_ROW_ADDR,
-				b47n->curr_page_addr);
-
-		/* Prepare to write */
-		ctlcode = NCTL_CSA | NCTL_ROW | NCTL_COL | NCTL_CMD0;
-		ctlcode |= NAND_CMD_SEQIN;
-		if (bcm47xxnflash_ops_bcm4706_ctl_cmd(cc, ctlcode))
-			pr_err("SEQIN failed\n");
-		break;
-	case NAND_CMD_PAGEPROG:
-		if (bcm47xxnflash_ops_bcm4706_ctl_cmd(cc, NCTL_CMD0 |
-							  NAND_CMD_PAGEPROG))
-			pr_err("PAGEPROG failed\n");
-		if (bcm47xxnflash_ops_bcm4706_poll(cc))
-			pr_err("PAGEPROG not ready\n");
-		break;
-	default:
-		pr_err("Command 0x%X unsupported\n", command);
-		break;
-	}
-	b47n->curr_command = command;
-}
-
-static u8 bcm47xxnflash_ops_bcm4706_read_byte(struct nand_chip *nand_chip)
-{
-	struct mtd_info *mtd = nand_to_mtd(nand_chip);
-	struct bcm47xxnflash *b47n = nand_get_controller_data(nand_chip);
-	struct bcma_drv_cc *cc = b47n->cc;
-	u32 tmp = 0;
-
-	switch (b47n->curr_command) {
-	case NAND_CMD_READID:
-		if (b47n->curr_column >= ARRAY_SIZE(b47n->id_data)) {
-			pr_err("Requested invalid id_data: %d\n",
-			       b47n->curr_column);
-			return 0;
-		}
-		return b47n->id_data[b47n->curr_column++];
-	case NAND_CMD_STATUS:
-		if (bcm47xxnflash_ops_bcm4706_ctl_cmd(cc,
-						      NCTL_READ |
-						      NCTL_DATA_CYCLES(1)))
-			return 0;
-		return bcma_cc_read32(cc, BCMA_CC_NFLASH_DATA) & 0xff;
-	case NAND_CMD_READOOB:
-		bcm47xxnflash_ops_bcm4706_read(mtd, (u8 *)&tmp, 4);
-		return tmp & 0xFF;
-	}
-
-	pr_err("Invalid command for byte read: 0x%X\n", b47n->curr_command);
-	return 0;
-}
-
-static void bcm47xxnflash_ops_bcm4706_read_buf(struct nand_chip *nand_chip,
-					       uint8_t *buf, int len)
-{
-	struct bcm47xxnflash *b47n = nand_get_controller_data(nand_chip);
-
-	switch (b47n->curr_command) {
-	case NAND_CMD_READ0:
-	case NAND_CMD_READOOB:
-		bcm47xxnflash_ops_bcm4706_read(nand_to_mtd(nand_chip), buf,
-					       len);
-		return;
-	}
-
-	pr_err("Invalid command for buf read: 0x%X\n", b47n->curr_command);
-}
-
-static void bcm47xxnflash_ops_bcm4706_write_buf(struct nand_chip *nand_chip,
-						const uint8_t *buf, int len)
-{
-	struct bcm47xxnflash *b47n = nand_get_controller_data(nand_chip);
-
-	switch (b47n->curr_command) {
-	case NAND_CMD_SEQIN:
-		bcm47xxnflash_ops_bcm4706_write(nand_to_mtd(nand_chip), buf,
-						len);
-		return;
-	}
-
-	pr_err("Invalid command for buf write: 0x%X\n", b47n->curr_command);
-}
-
 static int
 bcm47xxnflash_ops_bcm4706_exec_cmd_addr(struct nand_chip *chip,
 					const struct nand_subop *subop)
@@ -535,7 +232,6 @@ static const struct nand_controller_ops bcm47xxnflash_ops = {
 
 int bcm47xxnflash_ops_bcm4706_init(struct bcm47xxnflash *b47n)
 {
-	struct nand_chip *nand_chip = (struct nand_chip *)&b47n->nand_chip;
 	int err;
 	u32 freq;
 	u16 clock;
@@ -548,17 +244,6 @@ int bcm47xxnflash_ops_bcm4706_init(struct bcm47xxnflash *b47n)
 	nand_controller_init(&b47n->base);
 	b47n->base.ops = &bcm47xxnflash_ops;
 	b47n->nand_chip.controller = &b47n->base;
-	nand_chip->legacy.select_chip = bcm47xxnflash_ops_bcm4706_select_chip;
-	nand_chip->legacy.cmd_ctrl = bcm47xxnflash_ops_bcm4706_cmd_ctrl;
-	nand_chip->legacy.dev_ready = bcm47xxnflash_ops_bcm4706_dev_ready;
-	b47n->nand_chip.legacy.cmdfunc = bcm47xxnflash_ops_bcm4706_cmdfunc;
-	b47n->nand_chip.legacy.read_byte = bcm47xxnflash_ops_bcm4706_read_byte;
-	b47n->nand_chip.legacy.read_buf = bcm47xxnflash_ops_bcm4706_read_buf;
-	b47n->nand_chip.legacy.write_buf = bcm47xxnflash_ops_bcm4706_write_buf;
-	b47n->nand_chip.legacy.set_features = nand_get_set_features_notsupp;
-	b47n->nand_chip.legacy.get_features = nand_get_set_features_notsupp;
-
-	nand_chip->legacy.chip_delay = 50;
 	b47n->nand_chip.bbt_options = NAND_BBT_USE_FLASH;
 	b47n->nand_chip.ecc.mode = NAND_ECC_NONE; /* TODO: implement ECC */
 
-- 
2.25.2


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

[PATCH 7/9] mtd: rawnand: bcm47xx: Simplify the init() function

[Boris Brezillon]

The row and column sizes are now set as part of the exec_op() procedure
and adjusted to match the requested number of address cycles. No need
to set them in the init() function since those values will be
overwritten anyway. As for the other sanity check that was done on the
chip size, I don't think it's really needed.

Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
---
 .../mtd/nand/raw/bcm47xxnflash/ops_bcm4706.c  | 31 ++-----------------
 1 file changed, 2 insertions(+), 29 deletions(-)

diff --git a/drivers/mtd/nand/raw/bcm47xxnflash/ops_bcm4706.c b/drivers/mtd/nand/raw/bcm47xxnflash/ops_bcm4706.c
index 4e38b685d207..ab989de54cfc 100644
--- a/drivers/mtd/nand/raw/bcm47xxnflash/ops_bcm4706.c
+++ b/drivers/mtd/nand/raw/bcm47xxnflash/ops_bcm4706.c
@@ -237,10 +237,6 @@ int bcm47xxnflash_ops_bcm4706_init(struct bcm47xxnflash *b47n)
 	u16 clock;
 	u8 w0, w1, w2, w3, w4;
 
-	unsigned long chipsize; /* MiB */
-	u8 tbits, col_bits, col_size, row_bits, row_bsize;
-	u32 val;
-
 	nand_controller_init(&b47n->base);
 	b47n->base.ops = &bcm47xxnflash_ops;
 	b47n->nand_chip.controller = &b47n->base;
@@ -274,32 +270,9 @@ int bcm47xxnflash_ops_bcm4706_init(struct bcm47xxnflash *b47n)
 	err = nand_scan(&b47n->nand_chip, 1);
 	if (err) {
 		pr_err("Could not scan NAND flash: %d\n", err);
-		goto exit;
-	}
-
-	/* Configure FLASH */
-	chipsize = nanddev_target_size(&b47n->nand_chip.base) >> 20;
-	tbits = ffs(chipsize); /* find first bit set */
-	if (!tbits || tbits != fls(chipsize)) {
-		pr_err("Invalid flash size: 0x%lX\n", chipsize);
-		err = -ENOTSUPP;
-		goto exit;
-	}
-	tbits += 19; /* Broadcom increases *index* by 20, we increase *pos* */
-
-	col_bits = b47n->nand_chip.page_shift + 1;
-	col_size = (col_bits + 7) / 8;
-
-	row_bits = tbits - col_bits + 1;
-	row_bsize = (row_bits + 7) / 8;
-
-	val = CONF_ROW_BYTES(row_bsize) | CONF_COL_BYTES(col_size) |
-	      CONF_MAGIC_BIT;
-	bcma_cc_write32(b47n->cc, BCMA_CC_NFLASH_CONF, val);
-
-exit:
-	if (err)
 		bcma_cc_mask32(b47n->cc, BCMA_CC_4706_FLASHSCFG,
 			       ~BCMA_CC_4706_FLASHSCFG_NF1);
+	}
+
 	return err;
 }
-- 
2.25.2


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

[PATCH 8/9] mtd: rawnand: bcm47xx: Merge all source files

[Boris Brezillon]

Given the number of lines it doesn't make them to split the code. Let's
merge everything in main.c and rename the file into bcm47xxnflash.c.

Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
---
 drivers/mtd/nand/raw/bcm47xxnflash/Makefile   |  3 -
 .../{ops_bcm4706.c => bcm47xxnflash.c}        | 81 ++++++++++++++++++-
 .../nand/raw/bcm47xxnflash/bcm47xxnflash.h    | 21 -----
 drivers/mtd/nand/raw/bcm47xxnflash/main.c     | 77 ------------------
 4 files changed, 78 insertions(+), 104 deletions(-)
 rename drivers/mtd/nand/raw/bcm47xxnflash/{ops_bcm4706.c => bcm47xxnflash.c} (82%)
 delete mode 100644 drivers/mtd/nand/raw/bcm47xxnflash/bcm47xxnflash.h
 delete mode 100644 drivers/mtd/nand/raw/bcm47xxnflash/main.c

diff --git a/drivers/mtd/nand/raw/bcm47xxnflash/Makefile b/drivers/mtd/nand/raw/bcm47xxnflash/Makefile
index b531a630c9cf..71a953078799 100644
--- a/drivers/mtd/nand/raw/bcm47xxnflash/Makefile
+++ b/drivers/mtd/nand/raw/bcm47xxnflash/Makefile
@@ -1,5 +1,2 @@
 # SPDX-License-Identifier: GPL-2.0-only
-bcm47xxnflash-y				+= main.o
-bcm47xxnflash-y				+= ops_bcm4706.o
-
 obj-$(CONFIG_MTD_NAND_BCM47XXNFLASH)	+= bcm47xxnflash.o
diff --git a/drivers/mtd/nand/raw/bcm47xxnflash/ops_bcm4706.c b/drivers/mtd/nand/raw/bcm47xxnflash/bcm47xxnflash.c
similarity index 82%
rename from drivers/mtd/nand/raw/bcm47xxnflash/ops_bcm4706.c
rename to drivers/mtd/nand/raw/bcm47xxnflash/bcm47xxnflash.c
index ab989de54cfc..ae0391c1ee28 100644
--- a/drivers/mtd/nand/raw/bcm47xxnflash/ops_bcm4706.c
+++ b/drivers/mtd/nand/raw/bcm47xxnflash/bcm47xxnflash.c
@@ -5,13 +5,14 @@
  * Copyright (C) 2012 Rafał Miłecki <zajec5@gmail.com>
  */
 
-#include "bcm47xxnflash.h"
-
+#include <linux/delay.h>
 #include <linux/module.h>
 #include <linux/kernel.h>
 #include <linux/slab.h>
-#include <linux/delay.h>
+#include <linux/platform_device.h>
 #include <linux/bcma/bcma.h>
+#include <linux/mtd/mtd.h>
+#include <linux/mtd/rawnand.h>
 
 /* Broadcom uses 1'000'000 but it seems to be too many. Tests on WNDR4500 has
  * shown ~1000 retries as maxiumum. */
@@ -48,6 +49,17 @@
 #define CONF_COL_BYTES(x)		(((x) - 1) << 4)
 #define CONF_ROW_BYTES(x)		(((x) - 1) << 6)
 
+#ifndef pr_fmt
+#define pr_fmt(fmt)		KBUILD_MODNAME ": " fmt
+#endif
+
+struct bcm47xxnflash {
+	struct nand_controller base;
+	struct bcma_drv_cc *cc;
+
+	struct nand_chip nand_chip;
+};
+
 /**************************************************
  * Various helpers
  **************************************************/
@@ -276,3 +288,66 @@ int bcm47xxnflash_ops_bcm4706_init(struct bcm47xxnflash *b47n)
 
 	return err;
 }
+
+static const char *probes[] = { "bcm47xxpart", NULL };
+
+static int bcm47xxnflash_probe(struct platform_device *pdev)
+{
+	struct bcma_nflash *nflash = dev_get_platdata(&pdev->dev);
+	struct bcm47xxnflash *b47n;
+	struct mtd_info *mtd;
+	int err = 0;
+
+	b47n = devm_kzalloc(&pdev->dev, sizeof(*b47n), GFP_KERNEL);
+	if (!b47n)
+		return -ENOMEM;
+
+	nand_set_controller_data(&b47n->nand_chip, b47n);
+	mtd = nand_to_mtd(&b47n->nand_chip);
+	mtd->dev.parent = &pdev->dev;
+	b47n->cc = container_of(nflash, struct bcma_drv_cc, nflash);
+
+	if (b47n->cc->core->bus->chipinfo.id == BCMA_CHIP_ID_BCM4706) {
+		err = bcm47xxnflash_ops_bcm4706_init(b47n);
+	} else {
+		pr_err("Device not supported\n");
+		err = -ENOTSUPP;
+	}
+	if (err) {
+		pr_err("Initialization failed: %d\n", err);
+		return err;
+	}
+
+	platform_set_drvdata(pdev, b47n);
+
+	err = mtd_device_parse_register(mtd, probes, NULL, NULL, 0);
+	if (err) {
+		pr_err("Failed to register MTD device: %d\n", err);
+		return err;
+	}
+
+	return 0;
+}
+
+static int bcm47xxnflash_remove(struct platform_device *pdev)
+{
+	struct bcm47xxnflash *nflash = platform_get_drvdata(pdev);
+
+	nand_release(&nflash->nand_chip);
+
+	return 0;
+}
+
+static struct platform_driver bcm47xxnflash_driver = {
+	.probe	= bcm47xxnflash_probe,
+	.remove = bcm47xxnflash_remove,
+	.driver = {
+		.name = "bcma_nflash",
+	},
+};
+module_platform_driver(bcm47xxnflash_driver);
+
+MODULE_DESCRIPTION("NAND flash driver for BCMA bus");
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Rafał Miłecki");
+
diff --git a/drivers/mtd/nand/raw/bcm47xxnflash/bcm47xxnflash.h b/drivers/mtd/nand/raw/bcm47xxnflash/bcm47xxnflash.h
deleted file mode 100644
index 8de0e7e0a3a4..000000000000
--- a/drivers/mtd/nand/raw/bcm47xxnflash/bcm47xxnflash.h
+++ /dev/null
@@ -1,21 +0,0 @@
-/* SPDX-License-Identifier: GPL-2.0 */
-#ifndef __BCM47XXNFLASH_H
-#define __BCM47XXNFLASH_H
-
-#ifndef pr_fmt
-#define pr_fmt(fmt)		KBUILD_MODNAME ": " fmt
-#endif
-
-#include <linux/mtd/mtd.h>
-#include <linux/mtd/rawnand.h>
-
-struct bcm47xxnflash {
-	struct nand_controller base;
-	struct bcma_drv_cc *cc;
-
-	struct nand_chip nand_chip;
-};
-
-int bcm47xxnflash_ops_bcm4706_init(struct bcm47xxnflash *b47n);
-
-#endif /* BCM47XXNFLASH */
diff --git a/drivers/mtd/nand/raw/bcm47xxnflash/main.c b/drivers/mtd/nand/raw/bcm47xxnflash/main.c
deleted file mode 100644
index 8dae97c1dbe7..000000000000
--- a/drivers/mtd/nand/raw/bcm47xxnflash/main.c
+++ /dev/null
@@ -1,77 +0,0 @@
-// SPDX-License-Identifier: GPL-2.0-only
-/*
- * BCM47XX NAND flash driver
- *
- * Copyright (C) 2012 Rafał Miłecki <zajec5@gmail.com>
- */
-
-#include "bcm47xxnflash.h"
-
-#include <linux/module.h>
-#include <linux/kernel.h>
-#include <linux/slab.h>
-#include <linux/platform_device.h>
-#include <linux/bcma/bcma.h>
-
-MODULE_DESCRIPTION("NAND flash driver for BCMA bus");
-MODULE_LICENSE("GPL");
-MODULE_AUTHOR("Rafał Miłecki");
-
-static const char *probes[] = { "bcm47xxpart", NULL };
-
-static int bcm47xxnflash_probe(struct platform_device *pdev)
-{
-	struct bcma_nflash *nflash = dev_get_platdata(&pdev->dev);
-	struct bcm47xxnflash *b47n;
-	struct mtd_info *mtd;
-	int err = 0;
-
-	b47n = devm_kzalloc(&pdev->dev, sizeof(*b47n), GFP_KERNEL);
-	if (!b47n)
-		return -ENOMEM;
-
-	nand_set_controller_data(&b47n->nand_chip, b47n);
-	mtd = nand_to_mtd(&b47n->nand_chip);
-	mtd->dev.parent = &pdev->dev;
-	b47n->cc = container_of(nflash, struct bcma_drv_cc, nflash);
-
-	if (b47n->cc->core->bus->chipinfo.id == BCMA_CHIP_ID_BCM4706) {
-		err = bcm47xxnflash_ops_bcm4706_init(b47n);
-	} else {
-		pr_err("Device not supported\n");
-		err = -ENOTSUPP;
-	}
-	if (err) {
-		pr_err("Initialization failed: %d\n", err);
-		return err;
-	}
-
-	platform_set_drvdata(pdev, b47n);
-
-	err = mtd_device_parse_register(mtd, probes, NULL, NULL, 0);
-	if (err) {
-		pr_err("Failed to register MTD device: %d\n", err);
-		return err;
-	}
-
-	return 0;
-}
-
-static int bcm47xxnflash_remove(struct platform_device *pdev)
-{
-	struct bcm47xxnflash *nflash = platform_get_drvdata(pdev);
-
-	nand_release(&nflash->nand_chip);
-
-	return 0;
-}
-
-static struct platform_driver bcm47xxnflash_driver = {
-	.probe	= bcm47xxnflash_probe,
-	.remove = bcm47xxnflash_remove,
-	.driver = {
-		.name = "bcma_nflash",
-	},
-};
-
-module_platform_driver(bcm47xxnflash_driver);
-- 
2.25.2


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

[PATCH 9/9] mtd: rawnand: bcm47xx: Move the driver to drivers/mtd/nand/raw/

[Boris Brezillon]

Now that we have a single we can move it to the directory where all
single source file drivers live.

Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
---
 drivers/mtd/nand/raw/Makefile                            | 1 +
 drivers/mtd/nand/raw/{bcm47xxnflash => }/bcm47xxnflash.c | 0
 drivers/mtd/nand/raw/bcm47xxnflash/Makefile              | 2 --
 3 files changed, 1 insertion(+), 2 deletions(-)
 rename drivers/mtd/nand/raw/{bcm47xxnflash => }/bcm47xxnflash.c (100%)
 delete mode 100644 drivers/mtd/nand/raw/bcm47xxnflash/Makefile

diff --git a/drivers/mtd/nand/raw/Makefile b/drivers/mtd/nand/raw/Makefile
index 2d136b158fb7..703d696c2d61 100644
--- a/drivers/mtd/nand/raw/Makefile
+++ b/drivers/mtd/nand/raw/Makefile
@@ -58,6 +58,7 @@ obj-$(CONFIG_MTD_NAND_TEGRA)		+= tegra_nand.o
 obj-$(CONFIG_MTD_NAND_STM32_FMC2)	+= stm32_fmc2_nand.o
 obj-$(CONFIG_MTD_NAND_MESON)		+= meson_nand.o
 obj-$(CONFIG_MTD_NAND_CADENCE)		+= cadence-nand-controller.o
+obj-$(CONFIG_MTD_NAND_BCM47XXNFLASH)	+= bcm47xxnflash.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/bcm47xxnflash/bcm47xxnflash.c b/drivers/mtd/nand/raw/bcm47xxnflash.c
similarity index 100%
rename from drivers/mtd/nand/raw/bcm47xxnflash/bcm47xxnflash.c
rename to drivers/mtd/nand/raw/bcm47xxnflash.c
diff --git a/drivers/mtd/nand/raw/bcm47xxnflash/Makefile b/drivers/mtd/nand/raw/bcm47xxnflash/Makefile
deleted file mode 100644
index 71a953078799..000000000000
--- a/drivers/mtd/nand/raw/bcm47xxnflash/Makefile
+++ /dev/null
@@ -1,2 +0,0 @@
-# SPDX-License-Identifier: GPL-2.0-only
-obj-$(CONFIG_MTD_NAND_BCM47XXNFLASH)	+= bcm47xxnflash.o
-- 
2.25.2


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH 9/9] mtd: rawnand: bcm47xx: Move the driver to drivers/mtd/nand/raw/

[Boris Brezillon]

On Sun, 19 Apr 2020 14:51:40 +0200
Boris Brezillon <boris.brezillon@collabora.com> wrote:

> Now that we have a single we can move it to the directory where all
> single source file drivers live.
> 
> Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
> ---
>  drivers/mtd/nand/raw/Makefile                            | 1 +
>  drivers/mtd/nand/raw/{bcm47xxnflash => }/bcm47xxnflash.c | 0
>  drivers/mtd/nand/raw/bcm47xxnflash/Makefile              | 2 --
>  3 files changed, 1 insertion(+), 2 deletions(-)
>  rename drivers/mtd/nand/raw/{bcm47xxnflash => }/bcm47xxnflash.c (100%)
>  delete mode 100644 drivers/mtd/nand/raw/bcm47xxnflash/Makefile
> 
> diff --git a/drivers/mtd/nand/raw/Makefile b/drivers/mtd/nand/raw/Makefile
> index 2d136b158fb7..703d696c2d61 100644
> --- a/drivers/mtd/nand/raw/Makefile
> +++ b/drivers/mtd/nand/raw/Makefile
> @@ -58,6 +58,7 @@ obj-$(CONFIG_MTD_NAND_TEGRA)		+= tegra_nand.o
>  obj-$(CONFIG_MTD_NAND_STM32_FMC2)	+= stm32_fmc2_nand.o
>  obj-$(CONFIG_MTD_NAND_MESON)		+= meson_nand.o
>  obj-$(CONFIG_MTD_NAND_CADENCE)		+= cadence-nand-controller.o
> +obj-$(CONFIG_MTD_NAND_BCM47XXNFLASH)	+= bcm47xxnflash.o

I forgot to remove the 

obj-$(CONFIG_MTD_NAND_BCM47XXNFLASH)	+= bcm47xxnflash/

line. Will fix it in v2.

>  
>  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/bcm47xxnflash/bcm47xxnflash.c b/drivers/mtd/nand/raw/bcm47xxnflash.c
> similarity index 100%
> rename from drivers/mtd/nand/raw/bcm47xxnflash/bcm47xxnflash.c
> rename to drivers/mtd/nand/raw/bcm47xxnflash.c
> diff --git a/drivers/mtd/nand/raw/bcm47xxnflash/Makefile b/drivers/mtd/nand/raw/bcm47xxnflash/Makefile
> deleted file mode 100644
> index 71a953078799..000000000000
> --- a/drivers/mtd/nand/raw/bcm47xxnflash/Makefile
> +++ /dev/null
> @@ -1,2 +0,0 @@
> -# SPDX-License-Identifier: GPL-2.0-only
> -obj-$(CONFIG_MTD_NAND_BCM47XXNFLASH)	+= bcm47xxnflash.o


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH 1/9] mtd: rawnand: Add an is_last flag to nand_subop

[Miquel Raynal]

Hi Boris,

Boris Brezillon <boris.brezillon@collabora.com> wrote on Sun, 19 Apr
2020 14:51:32 +0200:

> Some controllers need to know when they're passed the last subop so
> they can de-assert the CE pin.
> 
> Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
> ---

Reviewed-by: Miquel Raynal <miquel.raynal@bootlin.com>


Thanks,
Miquèl

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH 1/9] mtd: rawnand: Add an is_last flag to nand_subop

[Miquel Raynal]

Hi Boris,

Boris Brezillon <boris.brezillon@collabora.com> wrote on Sun, 19 Apr
2020 14:51:32 +0200:

> Some controllers need to know when they're passed the last subop so
> they can de-assert the CE pin.
> 
> Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
> ---

Reviewed-by: Miquel Raynal <miquel.raynal@bootlin.com>


Thanks,
Miquèl

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH 1/9] mtd: rawnand: Add an is_last flag to nand_subop

[Miquel Raynal]

Hi Boris,

Boris Brezillon <boris.brezillon@collabora.com> wrote on Sun, 19 Apr
2020 14:51:32 +0200:

> Some controllers need to know when they're passed the last subop so
> they can de-assert the CE pin.
> 
> Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
> ---

Reviewed-by: Miquel Raynal <miquel.raynal@bootlin.com>


Thanks,
Miquèl

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH 4/9] mtd: rawnand: bcm47xx: Demistify a few more things

[Miquel Raynal]

Hi Boris,

Boris Brezillon <boris.brezillon@collabora.com> wrote on Sun, 19 Apr
2020 14:51:35 +0200:

> There were a few places were raw hex values were used instead of the

                          where

> macro def.

        def? :)

> 
> We also add macros to help forming the conf value (note that we still
> have one magic bit whose meaning I couldn't extract from the code), and
> add an extra macro to specify the number of DATA cycles to issue when
> the READ or WRITE flag is set.
> 
> Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
> ---
>  .../mtd/nand/raw/bcm47xxnflash/ops_bcm4706.c  | 34 +++++++++++++++----
>  1 file changed, 27 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/mtd/nand/raw/bcm47xxnflash/ops_bcm4706.c b/drivers/mtd/nand/raw/bcm47xxnflash/ops_bcm4706.c
> index 591775173034..fbb7acebc8f7 100644
> --- a/drivers/mtd/nand/raw/bcm47xxnflash/ops_bcm4706.c
> +++ b/drivers/mtd/nand/raw/bcm47xxnflash/ops_bcm4706.c
> @@ -25,12 +25,29 @@
>  #define NCTL_CMD1W			0x00080000
>  #define NCTL_READ			0x00100000
>  #define NCTL_WRITE			0x00200000
> +/* When the SPECADDR is set CMD1 is interpreted as a single ADDR cycle */
>  #define NCTL_SPECADDR			0x01000000
>  #define NCTL_READY			0x04000000
>  #define NCTL_ERR			0x08000000
> +/*
> + * Number of DATA cycles to issue when NCTL_{READ,WRITE} is set. The minimum
> + * value is 1 and the maximum value is 4. Those bytes are then stored in the
> + * BCMA_CC_NFLASH_DATA register.
> + */
> +#define NCTL_DATA_CYCLES(x)		((((x) - 1) & 0x3) << 28)
> +/*
> + * The CS pin seems to be asserted even if NCTL_CSA is not set. All this bit
> + * seems to encode is whether the CS line should stay asserted after the
> + * operation has been executed. In other words, you should only set it if if

s/it if if/it if/

> + * you intend to do more operations on the NAND bus.
> + */
>  #define NCTL_CSA			0x40000000
>  #define NCTL_START			0x80000000
>  
> +#define CONF_MAGIC_BIT			0x00000002
> +#define CONF_COL_BYTES(x)		(((x) - 1) << 4)
> +#define CONF_ROW_BYTES(x)		(((x) - 1) << 6)
> +


With the above corrected,

Reviewed-by: Miquel Raynal <miquel.raynal@bootlin.com>



Thanks,
Miquèl

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH 5/9] mtd: rawnand: bcm47xx: Implement the exec_op() interface

[Miquel Raynal]

Hi Boris,

Boris Brezillon <boris.brezillon@collabora.com> wrote on Sun, 19 Apr
2020 14:51:36 +0200:

> Implement the exec_op() interface so we can get rid of the convoluted
> cmdfunc() implementation.
> 
> Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
> ---
> This is based on my understanding of how this controller works, and I
> think it covers all the use cases covered by the custom cmdfunc()
> implementation. I might be wrong of course, so it'd be great to have
> someone test on real HW.
> ---
>  .../nand/raw/bcm47xxnflash/bcm47xxnflash.h    |   1 +
>  .../mtd/nand/raw/bcm47xxnflash/ops_bcm4706.c  | 150 ++++++++++++++++++
>  2 files changed, 151 insertions(+)
> 
> diff --git a/drivers/mtd/nand/raw/bcm47xxnflash/bcm47xxnflash.h b/drivers/mtd/nand/raw/bcm47xxnflash/bcm47xxnflash.h
> index 201b9baa52a0..00d0974b73cb 100644
> --- a/drivers/mtd/nand/raw/bcm47xxnflash/bcm47xxnflash.h
> +++ b/drivers/mtd/nand/raw/bcm47xxnflash/bcm47xxnflash.h
> @@ -10,6 +10,7 @@
>  #include <linux/mtd/rawnand.h>
>  
>  struct bcm47xxnflash {
> +	struct nand_controller base;
>  	struct bcma_drv_cc *cc;
>  
>  	struct nand_chip nand_chip;
> diff --git a/drivers/mtd/nand/raw/bcm47xxnflash/ops_bcm4706.c b/drivers/mtd/nand/raw/bcm47xxnflash/ops_bcm4706.c
> index fbb7acebc8f7..184f78b3d45a 100644
> --- a/drivers/mtd/nand/raw/bcm47xxnflash/ops_bcm4706.c
> +++ b/drivers/mtd/nand/raw/bcm47xxnflash/ops_bcm4706.c
> @@ -382,6 +382,153 @@ static void bcm47xxnflash_ops_bcm4706_write_buf(struct nand_chip *nand_chip,
>  	pr_err("Invalid command for buf write: 0x%X\n", b47n->curr_command);
>  }
>  
> +static int
> +bcm47xxnflash_ops_bcm4706_exec_cmd_addr(struct nand_chip *chip,
> +					const struct nand_subop *subop)
> +{
> +	struct bcm47xxnflash *b47n = nand_get_controller_data(chip);
> +	u32 nctl = 0, col = 0, row = 0, ncols = 0, nrows = 0;
> +	unsigned int i, j;
> +
> +	for (i = 0; i < subop->ninstrs; i++) {
> +		const struct nand_op_instr *instr = &subop->instrs[i];
> +
> +		switch (instr->type) {
> +		case NAND_OP_CMD_INSTR:
> +			if (WARN_ON_ONCE((nctl & NCTL_CMD0) &&
> +					 (nctl & NCTL_CMD1W)))
> +				return -EINVAL;
> +			else if (nctl & NCTL_CMD0)
> +				nctl |= NCTL_CMD1W |
> +					((u32)instr->ctx.cmd.opcode << 8);
> +			else
> +				nctl |= NCTL_CMD0 | instr->ctx.cmd.opcode;
> +			break;
> +		case NAND_OP_ADDR_INSTR:
> +			for (j = 0; j < instr->ctx.addr.naddrs; j++) {
> +				u32 addr = instr->ctx.addr.addrs[j];
> +
> +				if (i < 2) {

Don't you mean j here?              ^

> +					col |= addr << i * 8;

I'm not sure this will work, addr is 32-bit and col as well, I bet you
won't end up with what you expect.

> +					nctl |= NCTL_COL;
> +					ncols++;
> +				} else {
> +					row |= addr << (i - 2) * 8;
> +					nctl |= NCTL_ROW;
> +					nrows++;
> +				}
> +			}
> +			break;
> +		default:
> +			WARN_ON_ONCE(1);
> +			return -EINVAL;
> +		}
> +	}
> +
> +	/* Keep the CS line asserted if there's something else to execute. */
> +	if (!subop->is_last)
> +		nctl |= NCTL_CSA;
> +
> +	bcma_cc_write32(b47n->cc, BCMA_CC_NFLASH_CONF,
> +			CONF_MAGIC_BIT |
> +			CONF_COL_BYTES(ncols) |
> +			CONF_ROW_BYTES(nrows));
> +	return bcm47xxnflash_ops_bcm4706_ctl_cmd(b47n->cc, nctl);
> +}
> +
> +static int
> +bcm47xxnflash_ops_bcm4706_exec_waitrdy(struct nand_chip *chip,
> +				       const struct nand_subop *subop)
> +{
> +	struct bcm47xxnflash *b47n = nand_get_controller_data(chip);
> +	const struct nand_op_instr *instr = &subop->instrs[0];
> +	unsigned long timeout_jiffies = jiffies;
> +
> +	if (WARN_ON(subop->ninstrs != 1 ||
> +		    instr->type != NAND_OP_DATA_IN_INSTR))
> +		return -EINVAL;

Same remark as for the atmel migration, I doubt all these checks are
useful as long as we use the "official" parser to call these helpers. I
would rather prefer to drop them all.

> +
> +	timeout_jiffies += msecs_to_jiffies(instr->ctx.waitrdy.timeout_ms) + 1;
> +	do {
> +		if (bcma_cc_read32(b47n->cc, BCMA_CC_NFLASH_CTL) & NCTL_READY)
> +			return 0;
> +
> +		usleep_range(10, 100);
> +	} while (time_before(jiffies, timeout_jiffies));
> +
> +	return -ETIMEDOUT;
> +}



Thanks,
Miquèl

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH 6/9] mtd: rawnand: bcm47xx: Get rid of the legacy implementation

[Miquel Raynal]

Hi Boris,

Boris Brezillon <boris.brezillon@collabora.com> wrote on Sun, 19 Apr
2020 14:51:37 +0200:

> Now that exec_op() is implemented we don't need to implement all those
> legacy hooks, which simplifies the code quite a bit.
> 
> Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
> ---
>  .../nand/raw/bcm47xxnflash/bcm47xxnflash.h    |   6 -
>  .../mtd/nand/raw/bcm47xxnflash/ops_bcm4706.c  | 315 ------------------
>  2 files changed, 321 deletions(-)

Love the diffstat!

Reviewed-by: Miquel Raynal <miquel.raynal@bootlin.com>

Thanks,
Miquèl

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH 7/9] mtd: rawnand: bcm47xx: Simplify the init() function

[Miquel Raynal]

Hi Boris,

Boris Brezillon <boris.brezillon@collabora.com> wrote on Sun, 19 Apr
2020 14:51:38 +0200:

> The row and column sizes are now set as part of the exec_op() procedure
> and adjusted to match the requested number of address cycles. No need
> to set them in the init() function since those values will be
> overwritten anyway. As for the other sanity check that was done on the
> chip size, I don't think it's really needed.
> 
> Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
> ---

Reviewed-by: Miquel Raynal <miquel.raynal@bootlin.com>


Thanks,
Miquèl

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH 9/9] mtd: rawnand: bcm47xx: Move the driver to drivers/mtd/nand/raw/

[Miquel Raynal]

Hi Boris,

Boris Brezillon <boris.brezillon@collabora.com> wrote on Sun, 19 Apr
2020 14:51:40 +0200:

> Now that we have a single we can move it to the directory where all
> single source file drivers live.
> 
> Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
> ---
>  drivers/mtd/nand/raw/Makefile                            | 1 +
>  drivers/mtd/nand/raw/{bcm47xxnflash => }/bcm47xxnflash.c | 0
>  drivers/mtd/nand/raw/bcm47xxnflash/Makefile              | 2 --
>  3 files changed, 1 insertion(+), 2 deletions(-)
>  rename drivers/mtd/nand/raw/{bcm47xxnflash => }/bcm47xxnflash.c (100%)
>  delete mode 100644 drivers/mtd/nand/raw/bcm47xxnflash/Makefile

Here are two independent comments:

1/ I think calling the file bcm47xxn-nand-controller.c would best fit
todays policy.

2/ I am not sure there is an interest in doing the merge +
move/rename in separate steps. It's always a pain to follow changes in
a file with git blame when code get's moved around so I would prefer
doing this in a single change, what do you think?

> 
> diff --git a/drivers/mtd/nand/raw/Makefile b/drivers/mtd/nand/raw/Makefile
> index 2d136b158fb7..703d696c2d61 100644
> --- a/drivers/mtd/nand/raw/Makefile
> +++ b/drivers/mtd/nand/raw/Makefile
> @@ -58,6 +58,7 @@ obj-$(CONFIG_MTD_NAND_TEGRA)		+= tegra_nand.o
>  obj-$(CONFIG_MTD_NAND_STM32_FMC2)	+= stm32_fmc2_nand.o
>  obj-$(CONFIG_MTD_NAND_MESON)		+= meson_nand.o
>  obj-$(CONFIG_MTD_NAND_CADENCE)		+= cadence-nand-controller.o
> +obj-$(CONFIG_MTD_NAND_BCM47XXNFLASH)	+= bcm47xxnflash.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/bcm47xxnflash/bcm47xxnflash.c b/drivers/mtd/nand/raw/bcm47xxnflash.c
> similarity index 100%
> rename from drivers/mtd/nand/raw/bcm47xxnflash/bcm47xxnflash.c
> rename to drivers/mtd/nand/raw/bcm47xxnflash.c
> diff --git a/drivers/mtd/nand/raw/bcm47xxnflash/Makefile b/drivers/mtd/nand/raw/bcm47xxnflash/Makefile
> deleted file mode 100644
> index 71a953078799..000000000000
> --- a/drivers/mtd/nand/raw/bcm47xxnflash/Makefile
> +++ /dev/null
> @@ -1,2 +0,0 @@
> -# SPDX-License-Identifier: GPL-2.0-only
> -obj-$(CONFIG_MTD_NAND_BCM47XXNFLASH)	+= bcm47xxnflash.o

Thanks,
Miquèl

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH 8/9] mtd: rawnand: bcm47xx: Merge all source files

[Miquel Raynal]

Hi Boris,

Boris Brezillon <boris.brezillon@collabora.com> wrote on Sun, 19 Apr
2020 14:51:39 +0200:

> Given the number of lines it doesn't make them to split the code. Let's

                                            sense?

> merge everything in main.c and rename the file into bcm47xxnflash.c.
> 
> Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
> ---

Thanks,
Miquèl

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH 4/9] mtd: rawnand: bcm47xx: Demistify a few more things

[Boris Brezillon]

On Mon, 27 Apr 2020 19:07:01 +0200
Miquel Raynal <miquel.raynal@bootlin.com> wrote:

> Hi Boris,
> 
> Boris Brezillon <boris.brezillon@collabora.com> wrote on Sun, 19 Apr
> 2020 14:51:35 +0200:
> 
> > There were a few places were raw hex values were used instead of the  
> 
>                           where
> 
> > macro def.  
> 
>         def? :)

Will fix the commit message :-).

> 
> > 
> > We also add macros to help forming the conf value (note that we still
> > have one magic bit whose meaning I couldn't extract from the code), and
> > add an extra macro to specify the number of DATA cycles to issue when
> > the READ or WRITE flag is set.
> > 
> > Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
> > ---
> >  .../mtd/nand/raw/bcm47xxnflash/ops_bcm4706.c  | 34 +++++++++++++++----
> >  1 file changed, 27 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/mtd/nand/raw/bcm47xxnflash/ops_bcm4706.c b/drivers/mtd/nand/raw/bcm47xxnflash/ops_bcm4706.c
> > index 591775173034..fbb7acebc8f7 100644
> > --- a/drivers/mtd/nand/raw/bcm47xxnflash/ops_bcm4706.c
> > +++ b/drivers/mtd/nand/raw/bcm47xxnflash/ops_bcm4706.c
> > @@ -25,12 +25,29 @@
> >  #define NCTL_CMD1W			0x00080000
> >  #define NCTL_READ			0x00100000
> >  #define NCTL_WRITE			0x00200000
> > +/* When the SPECADDR is set CMD1 is interpreted as a single ADDR cycle */
> >  #define NCTL_SPECADDR			0x01000000
> >  #define NCTL_READY			0x04000000
> >  #define NCTL_ERR			0x08000000
> > +/*
> > + * Number of DATA cycles to issue when NCTL_{READ,WRITE} is set. The minimum
> > + * value is 1 and the maximum value is 4. Those bytes are then stored in the
> > + * BCMA_CC_NFLASH_DATA register.
> > + */
> > +#define NCTL_DATA_CYCLES(x)		((((x) - 1) & 0x3) << 28)
> > +/*
> > + * The CS pin seems to be asserted even if NCTL_CSA is not set. All this bit
> > + * seems to encode is whether the CS line should stay asserted after the
> > + * operation has been executed. In other words, you should only set it if if  
> 
> s/it if if/it if/
> 

And drop this duplicate.

> > + * you intend to do more operations on the NAND bus.
> > + */
> >  #define NCTL_CSA			0x40000000
> >  #define NCTL_START			0x80000000
> >  
> > +#define CONF_MAGIC_BIT			0x00000002
> > +#define CONF_COL_BYTES(x)		(((x) - 1) << 4)
> > +#define CONF_ROW_BYTES(x)		(((x) - 1) << 6)
> > +  
> 
> 
> With the above corrected,
> 
> Reviewed-by: Miquel Raynal <miquel.raynal@bootlin.com>
> 
> 
> 
> Thanks,
> Miquèl


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH 5/9] mtd: rawnand: bcm47xx: Implement the exec_op() interface

[Boris Brezillon]

On Mon, 27 Apr 2020 19:18:11 +0200
Miquel Raynal <miquel.raynal@bootlin.com> wrote:

> Hi Boris,
> 
> Boris Brezillon <boris.brezillon@collabora.com> wrote on Sun, 19 Apr
> 2020 14:51:36 +0200:
> 
> > Implement the exec_op() interface so we can get rid of the convoluted
> > cmdfunc() implementation.
> > 
> > Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
> > ---
> > This is based on my understanding of how this controller works, and I
> > think it covers all the use cases covered by the custom cmdfunc()
> > implementation. I might be wrong of course, so it'd be great to have
> > someone test on real HW.
> > ---
> >  .../nand/raw/bcm47xxnflash/bcm47xxnflash.h    |   1 +
> >  .../mtd/nand/raw/bcm47xxnflash/ops_bcm4706.c  | 150 ++++++++++++++++++
> >  2 files changed, 151 insertions(+)
> > 
> > diff --git a/drivers/mtd/nand/raw/bcm47xxnflash/bcm47xxnflash.h b/drivers/mtd/nand/raw/bcm47xxnflash/bcm47xxnflash.h
> > index 201b9baa52a0..00d0974b73cb 100644
> > --- a/drivers/mtd/nand/raw/bcm47xxnflash/bcm47xxnflash.h
> > +++ b/drivers/mtd/nand/raw/bcm47xxnflash/bcm47xxnflash.h
> > @@ -10,6 +10,7 @@
> >  #include <linux/mtd/rawnand.h>
> >  
> >  struct bcm47xxnflash {
> > +	struct nand_controller base;
> >  	struct bcma_drv_cc *cc;
> >  
> >  	struct nand_chip nand_chip;
> > diff --git a/drivers/mtd/nand/raw/bcm47xxnflash/ops_bcm4706.c b/drivers/mtd/nand/raw/bcm47xxnflash/ops_bcm4706.c
> > index fbb7acebc8f7..184f78b3d45a 100644
> > --- a/drivers/mtd/nand/raw/bcm47xxnflash/ops_bcm4706.c
> > +++ b/drivers/mtd/nand/raw/bcm47xxnflash/ops_bcm4706.c
> > @@ -382,6 +382,153 @@ static void bcm47xxnflash_ops_bcm4706_write_buf(struct nand_chip *nand_chip,
> >  	pr_err("Invalid command for buf write: 0x%X\n", b47n->curr_command);
> >  }
> >  
> > +static int
> > +bcm47xxnflash_ops_bcm4706_exec_cmd_addr(struct nand_chip *chip,
> > +					const struct nand_subop *subop)
> > +{
> > +	struct bcm47xxnflash *b47n = nand_get_controller_data(chip);
> > +	u32 nctl = 0, col = 0, row = 0, ncols = 0, nrows = 0;
> > +	unsigned int i, j;
> > +
> > +	for (i = 0; i < subop->ninstrs; i++) {
> > +		const struct nand_op_instr *instr = &subop->instrs[i];
> > +
> > +		switch (instr->type) {
> > +		case NAND_OP_CMD_INSTR:
> > +			if (WARN_ON_ONCE((nctl & NCTL_CMD0) &&
> > +					 (nctl & NCTL_CMD1W)))
> > +				return -EINVAL;
> > +			else if (nctl & NCTL_CMD0)
> > +				nctl |= NCTL_CMD1W |
> > +					((u32)instr->ctx.cmd.opcode << 8);
> > +			else
> > +				nctl |= NCTL_CMD0 | instr->ctx.cmd.opcode;
> > +			break;
> > +		case NAND_OP_ADDR_INSTR:
> > +			for (j = 0; j < instr->ctx.addr.naddrs; j++) {
> > +				u32 addr = instr->ctx.addr.addrs[j];
> > +
> > +				if (i < 2) {  
> 
> Don't you mean j here?              ^
> 

Nice catch! Indeed, it should be j.

> > +					col |= addr << i * 8;  
> 
> I'm not sure this will work, addr is 32-bit and col as well, I bet you
> won't end up with what you expect.

Well, assuming I use j that's really what I want. addr is an u32 to
allow for a shift greater than 8, but the value has be extracted
from the instr->ctx.addr.addrs array which is an u8 array, thus
making addr <= 0xff.

> 
> > +					nctl |= NCTL_COL;
> > +					ncols++;
> > +				} else {
> > +					row |= addr << (i - 2) * 8;

And it's j here as well.

> > +					nctl |= NCTL_ROW;
> > +					nrows++;
> > +				}
> > +			}
> > +			break;
> > +		default:
> > +			WARN_ON_ONCE(1);
> > +			return -EINVAL;
> > +		}
> > +	}
> > +
> > +	/* Keep the CS line asserted if there's something else to execute. */
> > +	if (!subop->is_last)
> > +		nctl |= NCTL_CSA;
> > +
> > +	bcma_cc_write32(b47n->cc, BCMA_CC_NFLASH_CONF,
> > +			CONF_MAGIC_BIT |
> > +			CONF_COL_BYTES(ncols) |
> > +			CONF_ROW_BYTES(nrows));
> > +	return bcm47xxnflash_ops_bcm4706_ctl_cmd(b47n->cc, nctl);
> > +}
> > +
> > +static int
> > +bcm47xxnflash_ops_bcm4706_exec_waitrdy(struct nand_chip *chip,
> > +				       const struct nand_subop *subop)
> > +{
> > +	struct bcm47xxnflash *b47n = nand_get_controller_data(chip);
> > +	const struct nand_op_instr *instr = &subop->instrs[0];
> > +	unsigned long timeout_jiffies = jiffies;
> > +
> > +	if (WARN_ON(subop->ninstrs != 1 ||
> > +		    instr->type != NAND_OP_DATA_IN_INSTR))
> > +		return -EINVAL;  
> 
> Same remark as for the atmel migration, I doubt all these checks are
> useful as long as we use the "official" parser to call these helpers. I
> would rather prefer to drop them all.

Agreed.

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH 6/9] mtd: rawnand: bcm47xx: Get rid of the legacy implementation

[Boris Brezillon]

On Mon, 27 Apr 2020 19:19:22 +0200
Miquel Raynal <miquel.raynal@bootlin.com> wrote:

> Hi Boris,
> 
> Boris Brezillon <boris.brezillon@collabora.com> wrote on Sun, 19 Apr
> 2020 14:51:37 +0200:
> 
> > Now that exec_op() is implemented we don't need to implement all those
> > legacy hooks, which simplifies the code quite a bit.
> > 
> > Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
> > ---
> >  .../nand/raw/bcm47xxnflash/bcm47xxnflash.h    |   6 -
> >  .../mtd/nand/raw/bcm47xxnflash/ops_bcm4706.c  | 315 ------------------
> >  2 files changed, 321 deletions(-)  
> 
> Love the diffstat!

Well, the real diffstat is in the cover letter, but yes, it's nice to
see how exec_op() can simplify things quite a bit. I'm more impressed
by the readability improvement though ;-).

> 
> Reviewed-by: Miquel Raynal <miquel.raynal@bootlin.com>
> 
> Thanks,
> Miquèl


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH 9/9] mtd: rawnand: bcm47xx: Move the driver to drivers/mtd/nand/raw/

[Boris Brezillon]

On Mon, 27 Apr 2020 19:24:17 +0200
Miquel Raynal <miquel.raynal@bootlin.com> wrote:

> Hi Boris,
> 
> Boris Brezillon <boris.brezillon@collabora.com> wrote on Sun, 19 Apr
> 2020 14:51:40 +0200:
> 
> > Now that we have a single we can move it to the directory where all
> > single source file drivers live.
> > 
> > Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
> > ---
> >  drivers/mtd/nand/raw/Makefile                            | 1 +
> >  drivers/mtd/nand/raw/{bcm47xxnflash => }/bcm47xxnflash.c | 0
> >  drivers/mtd/nand/raw/bcm47xxnflash/Makefile              | 2 --
> >  3 files changed, 1 insertion(+), 2 deletions(-)
> >  rename drivers/mtd/nand/raw/{bcm47xxnflash => }/bcm47xxnflash.c (100%)
> >  delete mode 100644 drivers/mtd/nand/raw/bcm47xxnflash/Makefile  
> 
> Here are two independent comments:
> 
> 1/ I think calling the file bcm47xxn-nand-controller.c would best fit
> todays policy.

Sure I can do that.

> 
> 2/ I am not sure there is an interest in doing the merge +
> move/rename in separate steps. It's always a pain to follow changes in
> a file with git blame when code get's moved around so I would prefer
> doing this in a single change, what do you think?

Great minds think alike: I was planning to merge those 2 patches in v2
;-). It's indeed not necessary to do it in 2 steps.

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH 5/9] mtd: rawnand: bcm47xx: Implement the exec_op() interface

[Miquel Raynal]

Hi Boris,

> > > +static int
> > > +bcm47xxnflash_ops_bcm4706_exec_cmd_addr(struct nand_chip *chip,
> > > +					const struct nand_subop *subop)
> > > +{
> > > +	struct bcm47xxnflash *b47n = nand_get_controller_data(chip);
> > > +	u32 nctl = 0, col = 0, row = 0, ncols = 0, nrows = 0;
> > > +	unsigned int i, j;
> > > +
> > > +	for (i = 0; i < subop->ninstrs; i++) {
> > > +		const struct nand_op_instr *instr = &subop->instrs[i];
> > > +
> > > +		switch (instr->type) {
> > > +		case NAND_OP_CMD_INSTR:
> > > +			if (WARN_ON_ONCE((nctl & NCTL_CMD0) &&
> > > +					 (nctl & NCTL_CMD1W)))
> > > +				return -EINVAL;
> > > +			else if (nctl & NCTL_CMD0)
> > > +				nctl |= NCTL_CMD1W |
> > > +					((u32)instr->ctx.cmd.opcode << 8);
> > > +			else
> > > +				nctl |= NCTL_CMD0 | instr->ctx.cmd.opcode;
> > > +			break;
> > > +		case NAND_OP_ADDR_INSTR:
> > > +			for (j = 0; j < instr->ctx.addr.naddrs; j++) {
> > > +				u32 addr = instr->ctx.addr.addrs[j];
> > > +
> > > +				if (i < 2) {    
> > 
> > Don't you mean j here?              ^
> >   
> 
> Nice catch! Indeed, it should be j.
> 
> > > +					col |= addr << i * 8;    
> > 
> > I'm not sure this will work, addr is 32-bit and col as well, I bet you
> > won't end up with what you expect.  
> 
> Well, assuming I use j that's really what I want. addr is an u32 to
> allow for a shift greater than 8, but the value has be extracted
> from the instr->ctx.addr.addrs array which is an u8 array, thus
> making addr <= 0xff.

Oh that's absolutely right. It's fine then!

Thanks,
Miquèl

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/