[PATCH v4 1/4] mtd: rawnand: Add destructive operation

[PATCH v4 1/4] mtd: rawnand: Add destructive operation

[dregan]

From: Boris Brezillon <bbrezillon@kernel.org>

Erase and program operations need the write protect (wp) pin to be
de-asserted to take effect. Add the concept of destructive
operation and pass the information to exec_op() so controllers know
when they should de-assert this pin without having to decode
the command opcode.

Signed-off-by: Boris Brezillon <bbrezillon@kernel.org>
Signed-off-by: David Regan <dregan@broadcom.com>
---
Changes in v4: none

Changes in v3: updated comments and email address

Changes in v2: gave credit to Boris Brezillon
---
 drivers/mtd/nand/raw/nand_base.c | 6 ++++--
 include/linux/mtd/rawnand.h      | 9 +++++++++
 2 files changed, 13 insertions(+), 2 deletions(-)

diff --git a/drivers/mtd/nand/raw/nand_base.c b/drivers/mtd/nand/raw/nand_base.c
index d4b55155aeae..47cc2c35153b 100644
--- a/drivers/mtd/nand/raw/nand_base.c
+++ b/drivers/mtd/nand/raw/nand_base.c
@@ -1494,7 +1494,8 @@ static int nand_exec_prog_page_op(struct nand_chip *chip, unsigned int page,
 			    NAND_COMMON_TIMING_NS(conf, tWB_max)),
 		NAND_OP_WAIT_RDY(NAND_COMMON_TIMING_MS(conf, tPROG_max), 0),
 	};
-	struct nand_operation op = NAND_OPERATION(chip->cur_cs, instrs);
+	struct nand_operation op = NAND_DESTRUCTIVE_OPERATION(chip->cur_cs,
+							      instrs);
 	int naddrs = nand_fill_column_cycles(chip, addrs, offset_in_page);
 
 	if (naddrs < 0)
@@ -1917,7 +1918,8 @@ int nand_erase_op(struct nand_chip *chip, unsigned int eraseblock)
 			NAND_OP_WAIT_RDY(NAND_COMMON_TIMING_MS(conf, tBERS_max),
 					 0),
 		};
-		struct nand_operation op = NAND_OPERATION(chip->cur_cs, instrs);
+		struct nand_operation op = NAND_DESTRUCTIVE_OPERATION(chip->cur_cs,
+								      instrs);
 
 		if (chip->options & NAND_ROW_ADDR_3)
 			instrs[1].ctx.addr.naddrs++;
diff --git a/include/linux/mtd/rawnand.h b/include/linux/mtd/rawnand.h
index 90a141ba2a5a..31aceda8616c 100644
--- a/include/linux/mtd/rawnand.h
+++ b/include/linux/mtd/rawnand.h
@@ -1008,6 +1008,7 @@ struct nand_op_parser {
  */
 struct nand_operation {
 	unsigned int cs;
+	bool deassert_wp;
 	const struct nand_op_instr *instrs;
 	unsigned int ninstrs;
 };
@@ -1019,6 +1020,14 @@ struct nand_operation {
 		.ninstrs = ARRAY_SIZE(_instrs),			\
 	}
 
+#define NAND_DESTRUCTIVE_OPERATION(_cs, _instrs)		\
+	{							\
+		.cs = _cs,					\
+		.deassert_wp = true,				\
+		.instrs = _instrs,				\
+		.ninstrs = ARRAY_SIZE(_instrs),			\
+	}
+
 int nand_op_parser_exec_op(struct nand_chip *chip,
 			   const struct nand_op_parser *parser,
 			   const struct nand_operation *op, bool check_only);
-- 
2.37.3


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

[PATCH v4 2/4] mtd: rawnand: NAND controller write protect

[dregan]

From: David Regan <dregan@broadcom.com>

Allow NAND controller to be responsible for write protect pin
handling during fast path and exec_op destructive operation
when controller_wp flag is set.

Signed-off-by: David Regan <dregan@broadcom.com>
---
Changes in v4: none

Changes in v3: update comments

Changes in v2: none
---
 drivers/mtd/nand/raw/nand_base.c | 4 ++++
 include/linux/mtd/rawnand.h      | 2 ++
 2 files changed, 6 insertions(+)

diff --git a/drivers/mtd/nand/raw/nand_base.c b/drivers/mtd/nand/raw/nand_base.c
index 47cc2c35153b..38ed0ced5b8e 100644
--- a/drivers/mtd/nand/raw/nand_base.c
+++ b/drivers/mtd/nand/raw/nand_base.c
@@ -367,6 +367,10 @@ static int nand_check_wp(struct nand_chip *chip)
 	if (chip->options & NAND_BROKEN_XD)
 		return 0;
 
+	/* controller responsible for NAND write protect */
+	if (chip->controller->controller_wp)
+		return 0;
+
 	/* Check the WP bit */
 	ret = nand_status_op(chip, &status);
 	if (ret)
diff --git a/include/linux/mtd/rawnand.h b/include/linux/mtd/rawnand.h
index 31aceda8616c..fcad94aa0515 100644
--- a/include/linux/mtd/rawnand.h
+++ b/include/linux/mtd/rawnand.h
@@ -1111,6 +1111,7 @@ struct nand_controller_ops {
  *			the bus without restarting an entire read operation nor
  *			changing the column.
  * @supported_op.cont_read: The controller supports sequential cache reads.
+ * @controller_wp:	the controller is in charge of handling the WP pin.
  */
 struct nand_controller {
 	struct mutex lock;
@@ -1119,6 +1120,7 @@ struct nand_controller {
 		unsigned int data_only_read: 1;
 		unsigned int cont_read: 1;
 	} supported_op;
+	bool controller_wp;
 };
 
 static inline void nand_controller_init(struct nand_controller *nfc)
-- 
2.37.3


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

Re: [PATCH v4 2/4] mtd: rawnand: NAND controller write protect

[William Zhang]

[-- Attachment #1.1: Type: text/plain, Size: 594 bytes --]



On 10/23/2023 10:14 AM, dregan@broadcom.com wrote:
> From: David Regan <dregan@broadcom.com>
> 
> Allow NAND controller to be responsible for write protect pin
> handling during fast path and exec_op destructive operation
> when controller_wp flag is set.
> 
> Signed-off-by: David Regan <dregan@broadcom.com>
> ---
> Changes in v4: none
> 
> Changes in v3: update comments
> 
> Changes in v2: none
> ---
>   drivers/mtd/nand/raw/nand_base.c | 4 ++++
>   include/linux/mtd/rawnand.h      | 2 ++
>   2 files changed, 6 insertions(+)
> 

Reviewed-by: William Zhang <william.zhang@broadcom.com>

[-- Attachment #1.2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4212 bytes --]

[-- Attachment #2: Type: text/plain, Size: 144 bytes --]

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

[PATCH v4 3/4] mtd: rawnand: brcmnand: pass host struct to bcmnand_ctrl_poll_status

[dregan]

From: David Regan <dregan@broadcom.com>

Pass host struct to bcmnand_ctrl_poll_status instead of ctrl struct
since real time status requires host, and ctrl is a member of host.
Real time status is required for low level commands vs cached status
since the NAND controller will not do an automatic status read at the
end of a low level command as it would with a high level command.

Signed-off-by: David Regan <dregan@broadcom.com>
---
Changes in v4: none

Changes in v3: none

Changes in v2: added this patch in series
---
 drivers/mtd/nand/raw/brcmnand/brcmnand.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/drivers/mtd/nand/raw/brcmnand/brcmnand.c b/drivers/mtd/nand/raw/brcmnand/brcmnand.c
index 440bef477930..8d429eb3b72a 100644
--- a/drivers/mtd/nand/raw/brcmnand/brcmnand.c
+++ b/drivers/mtd/nand/raw/brcmnand/brcmnand.c
@@ -1061,10 +1061,11 @@ enum {
 	CS_SELECT_AUTO_DEVICE_ID_CFG		= BIT(30),
 };
 
-static int bcmnand_ctrl_poll_status(struct brcmnand_controller *ctrl,
+static int bcmnand_ctrl_poll_status(struct brcmnand_host *host,
 				    u32 mask, u32 expected_val,
 				    unsigned long timeout_ms)
 {
+	struct brcmnand_controller *ctrl = host->ctrl;
 	unsigned long limit;
 	u32 val;
 
@@ -1379,7 +1380,7 @@ static void brcmnand_wp(struct mtd_info *mtd, int wp)
 		 * make sure ctrl/flash ready before and after
 		 * changing state of #WP pin
 		 */
-		ret = bcmnand_ctrl_poll_status(ctrl, NAND_CTRL_RDY |
+		ret = bcmnand_ctrl_poll_status(host, NAND_CTRL_RDY |
 					       NAND_STATUS_READY,
 					       NAND_CTRL_RDY |
 					       NAND_STATUS_READY, 0);
@@ -1389,7 +1390,7 @@ static void brcmnand_wp(struct mtd_info *mtd, int wp)
 		brcmnand_set_wp(ctrl, wp);
 		nand_status_op(chip, NULL);
 		/* NAND_STATUS_WP 0x00 = protected, 0x80 = not protected */
-		ret = bcmnand_ctrl_poll_status(ctrl,
+		ret = bcmnand_ctrl_poll_status(host,
 					       NAND_CTRL_RDY |
 					       NAND_STATUS_READY |
 					       NAND_STATUS_WP,
@@ -1629,13 +1630,13 @@ static void brcmnand_send_cmd(struct brcmnand_host *host, int cmd)
 	 */
 	if (oops_in_progress) {
 		if (ctrl->cmd_pending &&
-			bcmnand_ctrl_poll_status(ctrl, NAND_CTRL_RDY, NAND_CTRL_RDY, 0))
+			bcmnand_ctrl_poll_status(host, NAND_CTRL_RDY, NAND_CTRL_RDY, 0))
 			return;
 	} else
 		BUG_ON(ctrl->cmd_pending != 0);
 	ctrl->cmd_pending = cmd;
 
-	ret = bcmnand_ctrl_poll_status(ctrl, NAND_CTRL_RDY, NAND_CTRL_RDY, 0);
+	ret = bcmnand_ctrl_poll_status(host, NAND_CTRL_RDY, NAND_CTRL_RDY, 0);
 	WARN_ON(ret);
 
 	mb(); /* flush previous writes */
@@ -1664,7 +1665,7 @@ static bool brcmstb_nand_wait_for_completion(struct nand_chip *chip)
 	if (mtd->oops_panic_write || ctrl->irq < 0) {
 		/* switch to interrupt polling and PIO mode */
 		disable_ctrl_irqs(ctrl);
-		sts = bcmnand_ctrl_poll_status(ctrl, NAND_CTRL_RDY,
+		sts = bcmnand_ctrl_poll_status(host, NAND_CTRL_RDY,
 					       NAND_CTRL_RDY, 0);
 		err = sts < 0;
 	} else {
-- 
2.37.3


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

Re: [PATCH v4 3/4] mtd: rawnand: brcmnand: pass host struct to bcmnand_ctrl_poll_status

[William Zhang]

[-- Attachment #1.1: Type: text/plain, Size: 785 bytes --]



On 10/23/2023 10:14 AM, dregan@broadcom.com wrote:
> From: David Regan <dregan@broadcom.com>
> 
> Pass host struct to bcmnand_ctrl_poll_status instead of ctrl struct
> since real time status requires host, and ctrl is a member of host.
> Real time status is required for low level commands vs cached status
> since the NAND controller will not do an automatic status read at the
> end of a low level command as it would with a high level command.
> 
> Signed-off-by: David Regan <dregan@broadcom.com>
> ---
> Changes in v4: none
> 
> Changes in v3: none
> 
> Changes in v2: added this patch in series
> ---
>   drivers/mtd/nand/raw/brcmnand/brcmnand.c | 13 +++++++------
>   1 file changed, 7 insertions(+), 6 deletions(-)
> 

Reviewed-by: William Zhang <william.zhang@broadcom.com>

[-- Attachment #1.2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4212 bytes --]

[-- Attachment #2: Type: text/plain, Size: 144 bytes --]

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

[PATCH v4 4/4] mtd: rawnand: brcmnand: exec_op implementation

[dregan]

From: David Regan <dregan@broadcom.com>

exec_op implementation for Broadcom STB, Broadband and iProc SoC
This adds exec_op and removes the legacy interface. Based on changes
proposed by Boris Brezillon.

https://github.com/bbrezillon/linux/commit/4ec6f8d8d83f5aaca5d1877f02d48da96d41fcba
https://github.com/bbrezillon/linux/commit/11b4acffd761c4928652d7028d19fcd6f45e4696

Signed-off-by: David Regan <dregan@broadcom.com>
---
Changes in v4: made helper functions static

Changes in v3: moved WAITRDY out of loop

Changes in v2: moved status and reset command detect out of loop
---
 drivers/mtd/nand/raw/brcmnand/brcmnand.c | 380 ++++++++++-------------
 1 file changed, 157 insertions(+), 223 deletions(-)

diff --git a/drivers/mtd/nand/raw/brcmnand/brcmnand.c b/drivers/mtd/nand/raw/brcmnand/brcmnand.c
index 8d429eb3b72a..8e65ae97ba77 100644
--- a/drivers/mtd/nand/raw/brcmnand/brcmnand.c
+++ b/drivers/mtd/nand/raw/brcmnand/brcmnand.c
@@ -625,6 +625,8 @@ enum {
 /* Only for v7.2 */
 #define	ACC_CONTROL_ECC_EXT_SHIFT		13
 
+static u8 brcmnand_status(struct brcmnand_host *host);
+
 static inline bool brcmnand_non_mmio_ops(struct brcmnand_controller *ctrl)
 {
 #if IS_ENABLED(CONFIG_MTD_NAND_BRCMNAND_BCMA)
@@ -1022,19 +1024,6 @@ static inline int brcmnand_sector_1k_shift(struct brcmnand_controller *ctrl)
 		return -1;
 }
 
-static int brcmnand_get_sector_size_1k(struct brcmnand_host *host)
-{
-	struct brcmnand_controller *ctrl = host->ctrl;
-	int shift = brcmnand_sector_1k_shift(ctrl);
-	u16 acc_control_offs = brcmnand_cs_offset(ctrl, host->cs,
-						  BRCMNAND_CS_ACC_CONTROL);
-
-	if (shift < 0)
-		return 0;
-
-	return (nand_readreg(ctrl, acc_control_offs) >> shift) & 0x1;
-}
-
 static void brcmnand_set_sector_size_1k(struct brcmnand_host *host, int val)
 {
 	struct brcmnand_controller *ctrl = host->ctrl;
@@ -1074,6 +1063,9 @@ static int bcmnand_ctrl_poll_status(struct brcmnand_host *host,
 
 	limit = jiffies + msecs_to_jiffies(timeout_ms);
 	do {
+		if (mask & INTFC_FLASH_STATUS)
+			brcmnand_status(host);
+
 		val = brcmnand_read_reg(ctrl, BRCMNAND_INTFC_STATUS);
 		if ((val & mask) == expected_val)
 			return 0;
@@ -1388,7 +1380,8 @@ static void brcmnand_wp(struct mtd_info *mtd, int wp)
 			return;
 
 		brcmnand_set_wp(ctrl, wp);
-		nand_status_op(chip, NULL);
+		/* force controller operation to update internal copy of NAND chip status */
+		brcmnand_status(host);
 		/* NAND_STATUS_WP 0x00 = protected, 0x80 = not protected */
 		ret = bcmnand_ctrl_poll_status(host,
 					       NAND_CTRL_RDY |
@@ -1644,16 +1637,6 @@ static void brcmnand_send_cmd(struct brcmnand_host *host, int cmd)
 			   cmd << brcmnand_cmd_shift(ctrl));
 }
 
-/***********************************************************************
- * NAND MTD API: read/program/erase
- ***********************************************************************/
-
-static void brcmnand_cmd_ctrl(struct nand_chip *chip, int dat,
-			      unsigned int ctrl)
-{
-	/* intentionally left blank */
-}
-
 static bool brcmstb_nand_wait_for_completion(struct nand_chip *chip)
 {
 	struct brcmnand_host *host = nand_get_controller_data(chip);
@@ -1704,6 +1687,17 @@ static int brcmnand_waitfunc(struct nand_chip *chip)
 				 INTFC_FLASH_STATUS;
 }
 
+static u8 brcmnand_status(struct brcmnand_host *host)
+{
+	struct nand_chip *chip = &host->chip;
+	struct mtd_info *mtd = nand_to_mtd(chip);
+
+	brcmnand_set_cmd_addr(mtd, 0);
+	brcmnand_send_cmd(host, CMD_STATUS_READ);
+
+	return brcmnand_waitfunc(chip);
+}
+
 enum {
 	LLOP_RE				= BIT(16),
 	LLOP_WE				= BIT(17),
@@ -1753,190 +1747,6 @@ static int brcmnand_low_level_op(struct brcmnand_host *host,
 	return brcmnand_waitfunc(chip);
 }
 
-static void brcmnand_cmdfunc(struct nand_chip *chip, unsigned command,
-			     int column, int page_addr)
-{
-	struct mtd_info *mtd = nand_to_mtd(chip);
-	struct brcmnand_host *host = nand_get_controller_data(chip);
-	struct brcmnand_controller *ctrl = host->ctrl;
-	u64 addr = (u64)page_addr << chip->page_shift;
-	int native_cmd = 0;
-
-	if (command == NAND_CMD_READID || command == NAND_CMD_PARAM ||
-			command == NAND_CMD_RNDOUT)
-		addr = (u64)column;
-	/* Avoid propagating a negative, don't-care address */
-	else if (page_addr < 0)
-		addr = 0;
-
-	dev_dbg(ctrl->dev, "cmd 0x%x addr 0x%llx\n", command,
-		(unsigned long long)addr);
-
-	host->last_cmd = command;
-	host->last_byte = 0;
-	host->last_addr = addr;
-
-	switch (command) {
-	case NAND_CMD_RESET:
-		native_cmd = CMD_FLASH_RESET;
-		break;
-	case NAND_CMD_STATUS:
-		native_cmd = CMD_STATUS_READ;
-		break;
-	case NAND_CMD_READID:
-		native_cmd = CMD_DEVICE_ID_READ;
-		break;
-	case NAND_CMD_READOOB:
-		native_cmd = CMD_SPARE_AREA_READ;
-		break;
-	case NAND_CMD_ERASE1:
-		native_cmd = CMD_BLOCK_ERASE;
-		brcmnand_wp(mtd, 0);
-		break;
-	case NAND_CMD_PARAM:
-		native_cmd = CMD_PARAMETER_READ;
-		break;
-	case NAND_CMD_SET_FEATURES:
-	case NAND_CMD_GET_FEATURES:
-		brcmnand_low_level_op(host, LL_OP_CMD, command, false);
-		brcmnand_low_level_op(host, LL_OP_ADDR, column, false);
-		break;
-	case NAND_CMD_RNDOUT:
-		native_cmd = CMD_PARAMETER_CHANGE_COL;
-		addr &= ~((u64)(FC_BYTES - 1));
-		/*
-		 * HW quirk: PARAMETER_CHANGE_COL requires SECTOR_SIZE_1K=0
-		 * NB: hwcfg.sector_size_1k may not be initialized yet
-		 */
-		if (brcmnand_get_sector_size_1k(host)) {
-			host->hwcfg.sector_size_1k =
-				brcmnand_get_sector_size_1k(host);
-			brcmnand_set_sector_size_1k(host, 0);
-		}
-		break;
-	}
-
-	if (!native_cmd)
-		return;
-
-	brcmnand_set_cmd_addr(mtd, addr);
-	brcmnand_send_cmd(host, native_cmd);
-	brcmnand_waitfunc(chip);
-
-	if (native_cmd == CMD_PARAMETER_READ ||
-			native_cmd == CMD_PARAMETER_CHANGE_COL) {
-		/* Copy flash cache word-wise */
-		u32 *flash_cache = (u32 *)ctrl->flash_cache;
-		int i;
-
-		brcmnand_soc_data_bus_prepare(ctrl->soc, true);
-
-		/*
-		 * Must cache the FLASH_CACHE now, since changes in
-		 * SECTOR_SIZE_1K may invalidate it
-		 */
-		for (i = 0; i < FC_WORDS; i++)
-			/*
-			 * Flash cache is big endian for parameter pages, at
-			 * least on STB SoCs
-			 */
-			flash_cache[i] = be32_to_cpu(brcmnand_read_fc(ctrl, i));
-
-		brcmnand_soc_data_bus_unprepare(ctrl->soc, true);
-
-		/* Cleanup from HW quirk: restore SECTOR_SIZE_1K */
-		if (host->hwcfg.sector_size_1k)
-			brcmnand_set_sector_size_1k(host,
-						    host->hwcfg.sector_size_1k);
-	}
-
-	/* Re-enable protection is necessary only after erase */
-	if (command == NAND_CMD_ERASE1)
-		brcmnand_wp(mtd, 1);
-}
-
-static uint8_t brcmnand_read_byte(struct nand_chip *chip)
-{
-	struct brcmnand_host *host = nand_get_controller_data(chip);
-	struct brcmnand_controller *ctrl = host->ctrl;
-	uint8_t ret = 0;
-	int addr, offs;
-
-	switch (host->last_cmd) {
-	case NAND_CMD_READID:
-		if (host->last_byte < 4)
-			ret = brcmnand_read_reg(ctrl, BRCMNAND_ID) >>
-				(24 - (host->last_byte << 3));
-		else if (host->last_byte < 8)
-			ret = brcmnand_read_reg(ctrl, BRCMNAND_ID_EXT) >>
-				(56 - (host->last_byte << 3));
-		break;
-
-	case NAND_CMD_READOOB:
-		ret = oob_reg_read(ctrl, host->last_byte);
-		break;
-
-	case NAND_CMD_STATUS:
-		ret = brcmnand_read_reg(ctrl, BRCMNAND_INTFC_STATUS) &
-					INTFC_FLASH_STATUS;
-		if (wp_on) /* hide WP status */
-			ret |= NAND_STATUS_WP;
-		break;
-
-	case NAND_CMD_PARAM:
-	case NAND_CMD_RNDOUT:
-		addr = host->last_addr + host->last_byte;
-		offs = addr & (FC_BYTES - 1);
-
-		/* At FC_BYTES boundary, switch to next column */
-		if (host->last_byte > 0 && offs == 0)
-			nand_change_read_column_op(chip, addr, NULL, 0, false);
-
-		ret = ctrl->flash_cache[offs];
-		break;
-	case NAND_CMD_GET_FEATURES:
-		if (host->last_byte >= ONFI_SUBFEATURE_PARAM_LEN) {
-			ret = 0;
-		} else {
-			bool last = host->last_byte ==
-				ONFI_SUBFEATURE_PARAM_LEN - 1;
-			brcmnand_low_level_op(host, LL_OP_RD, 0, last);
-			ret = brcmnand_read_reg(ctrl, BRCMNAND_LL_RDATA) & 0xff;
-		}
-	}
-
-	dev_dbg(ctrl->dev, "read byte = 0x%02x\n", ret);
-	host->last_byte++;
-
-	return ret;
-}
-
-static void brcmnand_read_buf(struct nand_chip *chip, uint8_t *buf, int len)
-{
-	int i;
-
-	for (i = 0; i < len; i++, buf++)
-		*buf = brcmnand_read_byte(chip);
-}
-
-static void brcmnand_write_buf(struct nand_chip *chip, const uint8_t *buf,
-			       int len)
-{
-	int i;
-	struct brcmnand_host *host = nand_get_controller_data(chip);
-
-	switch (host->last_cmd) {
-	case NAND_CMD_SET_FEATURES:
-		for (i = 0; i < len; i++)
-			brcmnand_low_level_op(host, LL_OP_WR, buf[i],
-						  (i + 1) == len);
-		break;
-	default:
-		BUG();
-		break;
-	}
-}
-
 /*
  *  Kick EDU engine
  */
@@ -2346,8 +2156,9 @@ static int brcmnand_read_page(struct nand_chip *chip, uint8_t *buf,
 	struct mtd_info *mtd = nand_to_mtd(chip);
 	struct brcmnand_host *host = nand_get_controller_data(chip);
 	u8 *oob = oob_required ? (u8 *)chip->oob_poi : NULL;
+	u64 addr = (u64)page << chip->page_shift;
 
-	nand_read_page_op(chip, page, 0, NULL, 0);
+	host->last_addr = addr;
 
 	return brcmnand_read(mtd, chip, host->last_addr,
 			mtd->writesize >> FC_SHIFT, (u32 *)buf, oob);
@@ -2360,8 +2171,9 @@ static int brcmnand_read_page_raw(struct nand_chip *chip, uint8_t *buf,
 	struct mtd_info *mtd = nand_to_mtd(chip);
 	u8 *oob = oob_required ? (u8 *)chip->oob_poi : NULL;
 	int ret;
+	u64 addr = (u64)page << chip->page_shift;
 
-	nand_read_page_op(chip, page, 0, NULL, 0);
+	host->last_addr = addr;
 
 	brcmnand_set_ecc_enabled(host, 0);
 	ret = brcmnand_read(mtd, chip, host->last_addr,
@@ -2469,11 +2281,11 @@ static int brcmnand_write_page(struct nand_chip *chip, const uint8_t *buf,
 	struct mtd_info *mtd = nand_to_mtd(chip);
 	struct brcmnand_host *host = nand_get_controller_data(chip);
 	void *oob = oob_required ? chip->oob_poi : NULL;
+	u64 addr = (u64)page << chip->page_shift;
 
-	nand_prog_page_begin_op(chip, page, 0, NULL, 0);
-	brcmnand_write(mtd, chip, host->last_addr, (const u32 *)buf, oob);
+	host->last_addr = addr;
 
-	return nand_prog_page_end_op(chip);
+	return brcmnand_write(mtd, chip, host->last_addr, (const u32 *)buf, oob);
 }
 
 static int brcmnand_write_page_raw(struct nand_chip *chip, const uint8_t *buf,
@@ -2482,13 +2294,15 @@ static int brcmnand_write_page_raw(struct nand_chip *chip, const uint8_t *buf,
 	struct mtd_info *mtd = nand_to_mtd(chip);
 	struct brcmnand_host *host = nand_get_controller_data(chip);
 	void *oob = oob_required ? chip->oob_poi : NULL;
+	u64 addr = (u64)page << chip->page_shift;
+	int ret = 0;
 
-	nand_prog_page_begin_op(chip, page, 0, NULL, 0);
+	host->last_addr = addr;
 	brcmnand_set_ecc_enabled(host, 0);
-	brcmnand_write(mtd, chip, host->last_addr, (const u32 *)buf, oob);
+	ret = brcmnand_write(mtd, chip, host->last_addr, (const u32 *)buf, oob);
 	brcmnand_set_ecc_enabled(host, 1);
 
-	return nand_prog_page_end_op(chip);
+	return ret;
 }
 
 static int brcmnand_write_oob(struct nand_chip *chip, int page)
@@ -2512,6 +2326,131 @@ static int brcmnand_write_oob_raw(struct nand_chip *chip, int page)
 	return ret;
 }
 
+static int brcmnand_exec_instr(struct brcmnand_host *host, int i,
+				const struct nand_operation *op)
+{
+	struct brcmnand_controller *ctrl = host->ctrl;
+	const struct nand_op_instr *instr = &op->instrs[i];
+	const u8 *out;
+	u8 *in;
+	int ret = 0;
+	bool last_op;
+
+	/*
+	 * if we are on the last command in the sequence (not including
+	 * waitrdy which is not a NAND command) then flag the controller
+	 */
+	last_op = (((i == (op->ninstrs - 1)) &&
+			(instr->type != NAND_OP_WAITRDY_INSTR)) ||
+			((i == (op->ninstrs - 2)) &&
+			(op->instrs[i+1].type == NAND_OP_WAITRDY_INSTR)));
+
+	switch (instr->type) {
+	case NAND_OP_CMD_INSTR:
+		brcmnand_low_level_op(host, LL_OP_CMD,
+				      instr->ctx.cmd.opcode, last_op);
+		break;
+
+	case NAND_OP_ADDR_INSTR:
+		for (i = 0; i < instr->ctx.addr.naddrs; i++)
+			brcmnand_low_level_op(host, LL_OP_ADDR,
+					      instr->ctx.addr.addrs[i],
+					      last_op &&
+						  i == (instr->ctx.addr.naddrs - 1));
+		break;
+
+	case NAND_OP_DATA_IN_INSTR:
+		in = instr->ctx.data.buf.in;
+		for (i = 0; i < instr->ctx.data.len; i++) {
+			brcmnand_low_level_op(host, LL_OP_RD, 0, last_op &&
+						  i == (instr->ctx.data.len - 1));
+			in[i] = brcmnand_read_reg(host->ctrl,
+						  BRCMNAND_LL_RDATA);
+		}
+		break;
+
+	case NAND_OP_DATA_OUT_INSTR:
+		out = instr->ctx.data.buf.out;
+		for (i = 0; i < instr->ctx.data.len; i++)
+			brcmnand_low_level_op(host, LL_OP_WR, out[i], last_op &&
+						  i == (instr->ctx.data.len - 1));
+		break;
+
+	case NAND_OP_WAITRDY_INSTR:
+		ret = bcmnand_ctrl_poll_status(host, NAND_CTRL_RDY, NAND_CTRL_RDY, 0);
+		break;
+
+	default:
+		dev_err(ctrl->dev, "unsupported instruction type: %d\n",
+			instr->type);
+		ret = -EINVAL;
+		break;
+	}
+
+	return ret;
+}
+
+static int brcmnand_exec_op_is_status(const struct nand_operation *op)
+{
+	if ((op->ninstrs == 2) &&
+		(op->instrs[0].type == NAND_OP_CMD_INSTR) &&
+		(op->instrs[0].ctx.cmd.opcode == NAND_CMD_STATUS) &&
+		(op->instrs[1].type == NAND_OP_DATA_IN_INSTR))
+		return 1;
+
+	return 0;
+}
+
+static int brcmnand_exec_op_is_reset(const struct nand_operation *op)
+{
+	if ((op->ninstrs == 1) &&
+		(op->instrs[0].type == NAND_OP_CMD_INSTR) &&
+		(op->instrs[0].ctx.cmd.opcode == NAND_CMD_RESET))
+		return 1;
+
+	return 0;
+}
+
+static int brcmnand_exec_op(struct nand_chip *chip,
+			    const struct nand_operation *op,
+			    bool check_only)
+{
+	struct brcmnand_host *host = nand_get_controller_data(chip);
+	struct mtd_info *mtd = nand_to_mtd(chip);
+	u8 *status;
+	unsigned int i;
+	int ret = 0;
+
+	if (check_only)
+		return 0;
+
+	if (brcmnand_exec_op_is_status(op)) {
+		status = op->instrs[1].ctx.data.buf.in;
+		*status = brcmnand_status(host);
+
+		return 0;
+	}
+
+	if (op->deassert_wp)
+		brcmnand_wp(mtd, 0);
+
+	for (i = 0; i < op->ninstrs; i++) {
+		ret = brcmnand_exec_instr(host, i, op);
+		if (ret)
+			break;
+	}
+
+	if (op->deassert_wp)
+		brcmnand_wp(mtd, 1);
+
+	if (brcmnand_exec_op_is_reset(op)) {
+		brcmnand_wp(mtd, 1);
+		brcmnand_status(host);
+	}
+
+	return ret;
+}
+
 /***********************************************************************
  * Per-CS setup (1 NAND device)
  ***********************************************************************/
@@ -2822,6 +2761,7 @@ static int brcmnand_attach_chip(struct nand_chip *chip)
 
 static const struct nand_controller_ops brcmnand_controller_ops = {
 	.attach_chip = brcmnand_attach_chip,
+	.exec_op = brcmnand_exec_op,
 };
 
 static int brcmnand_init_cs(struct brcmnand_host *host,
@@ -2846,13 +2786,6 @@ static int brcmnand_init_cs(struct brcmnand_host *host,
 	mtd->owner = THIS_MODULE;
 	mtd->dev.parent = dev;
 
-	chip->legacy.cmd_ctrl = brcmnand_cmd_ctrl;
-	chip->legacy.cmdfunc = brcmnand_cmdfunc;
-	chip->legacy.waitfunc = brcmnand_waitfunc;
-	chip->legacy.read_byte = brcmnand_read_byte;
-	chip->legacy.read_buf = brcmnand_read_buf;
-	chip->legacy.write_buf = brcmnand_write_buf;
-
 	chip->ecc.engine_type = NAND_ECC_ENGINE_TYPE_ON_HOST;
 	chip->ecc.read_page = brcmnand_read_page;
 	chip->ecc.write_page = brcmnand_write_page;
@@ -2864,6 +2797,7 @@ static int brcmnand_init_cs(struct brcmnand_host *host,
 	chip->ecc.write_oob = brcmnand_write_oob;
 
 	chip->controller = &ctrl->controller;
+	ctrl->controller.controller_wp = 1;
 
 	/*
 	 * The bootloader might have configured 16bit mode but
-- 
2.37.3


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

Re: [PATCH v4 4/4] mtd: rawnand: brcmnand: exec_op implementation

[William Zhang]

[-- Attachment #1.1: Type: text/plain, Size: 861 bytes --]



On 10/23/2023 10:14 AM, dregan@broadcom.com wrote:
> From: David Regan <dregan@broadcom.com>
> 
> exec_op implementation for Broadcom STB, Broadband and iProc SoC
> This adds exec_op and removes the legacy interface. Based on changes
> proposed by Boris Brezillon.
> 
> https://github.com/bbrezillon/linux/commit/4ec6f8d8d83f5aaca5d1877f02d48da96d41fcba
> https://github.com/bbrezillon/linux/commit/11b4acffd761c4928652d7028d19fcd6f45e4696
> 
> Signed-off-by: David Regan <dregan@broadcom.com>
> ---
> Changes in v4: made helper functions static
> 
> Changes in v3: moved WAITRDY out of loop
> 
> Changes in v2: moved status and reset command detect out of loop
> ---
>   drivers/mtd/nand/raw/brcmnand/brcmnand.c | 380 ++++++++++-------------
>   1 file changed, 157 insertions(+), 223 deletions(-)
> 

Reviewed-by: William Zhang <william.zhang@broadcom.com>

[-- Attachment #1.2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4212 bytes --]

[-- Attachment #2: Type: text/plain, Size: 144 bytes --]

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

Re: [PATCH v4 4/4] mtd: rawnand: brcmnand: exec_op implementation

[Miquel Raynal]

Hello,

dregan@broadcom.com wrote on Mon, 23 Oct 2023 10:14:44 -0700:

> From: David Regan <dregan@broadcom.com>
> 
> exec_op implementation for Broadcom STB, Broadband and iProc SoC
> This adds exec_op and removes the legacy interface. Based on changes
> proposed by Boris Brezillon.
> 
> https://github.com/bbrezillon/linux/commit/4ec6f8d8d83f5aaca5d1877f02d48da96d41fcba
> https://github.com/bbrezillon/linux/commit/11b4acffd761c4928652d7028d19fcd6f45e4696
> 
> Signed-off-by: David Regan <dregan@broadcom.com>

I'm fine with patches 1-3, a few minor nits on this version as well,
nothing big. I guess I'll let some time to Florian as well to give his
feedback and perhaps test the ->exec_op() implementation.

...

> +static int brcmnand_exec_instr(struct brcmnand_host *host, int i,
> +				const struct nand_operation *op)
> +{
> +	struct brcmnand_controller *ctrl = host->ctrl;
> +	const struct nand_op_instr *instr = &op->instrs[i];
> +	const u8 *out;
> +	u8 *in;
> +	int ret = 0;
> +	bool last_op;
> +
> +	/*
> +	 * if we are on the last command in the sequence (not including
> +	 * waitrdy which is not a NAND command) then flag the controller

May I suggest:

	/*
	 * The controller needs to be aware of the last command in the operation
	 * (WAITRDY excepted).
	 */

> +	 */
> +	last_op = (((i == (op->ninstrs - 1)) &&
> +			(instr->type != NAND_OP_WAITRDY_INSTR)) ||

You can cross the 80 chars boundary. Please use this form:

	last_op = ((i == (op->ninstrs - 1)) && (instr->type != NAND_OP_WAITRDY_INSTR)) ||
		  ((i == (op->ninstrs - 2)) && (op->instrs[i+1].type == NAND_OP_WAITRDY_INSTR));

> +			((i == (op->ninstrs - 2)) &&
> +			(op->instrs[i+1].type == NAND_OP_WAITRDY_INSTR)));
> +
> +	switch (instr->type) {
> +	case NAND_OP_CMD_INSTR:
> +		brcmnand_low_level_op(host, LL_OP_CMD,
> +				      instr->ctx.cmd.opcode, last_op);
> +		break;
> +
> +	case NAND_OP_ADDR_INSTR:
> +		for (i = 0; i < instr->ctx.addr.naddrs; i++)
> +			brcmnand_low_level_op(host, LL_OP_ADDR,
> +					      instr->ctx.addr.addrs[i],
> +					      last_op &&
> +						  i == (instr->ctx.addr.naddrs - 1));
> +		break;
> +
> +	case NAND_OP_DATA_IN_INSTR:
> +		in = instr->ctx.data.buf.in;
> +		for (i = 0; i < instr->ctx.data.len; i++) {
> +			brcmnand_low_level_op(host, LL_OP_RD, 0, last_op &&
> +						  i == (instr->ctx.data.len - 1));
> +			in[i] = brcmnand_read_reg(host->ctrl,
> +						  BRCMNAND_LL_RDATA);
> +		}
> +		break;
> +
> +	case NAND_OP_DATA_OUT_INSTR:
> +		out = instr->ctx.data.buf.out;
> +		for (i = 0; i < instr->ctx.data.len; i++)
> +			brcmnand_low_level_op(host, LL_OP_WR, out[i], last_op &&
> +						  i == (instr->ctx.data.len - 1));
> +		break;
> +
> +	case NAND_OP_WAITRDY_INSTR:
> +		ret = bcmnand_ctrl_poll_status(host, NAND_CTRL_RDY, NAND_CTRL_RDY, 0);
> +		break;
> +
> +	default:
> +		dev_err(ctrl->dev, "unsupported instruction type: %d\n",
> +			instr->type);
> +		ret = -EINVAL;
> +		break;
> +	}
> +
> +	return ret;
> +}
> +
> +static int brcmnand_exec_op_is_status(const struct nand_operation *op)

brcmnand_op_is_status() would make more sense

> +{
> +	if ((op->ninstrs == 2) &&
> +		(op->instrs[0].type == NAND_OP_CMD_INSTR) &&
> +		(op->instrs[0].ctx.cmd.opcode == NAND_CMD_STATUS) &&
> +		(op->instrs[1].type == NAND_OP_DATA_IN_INSTR))
> +		return 1;
> +
> +	return 0;
> +}
> +
> +static int brcmnand_exec_op_is_reset(const struct nand_operation *op)

same here, please s/exec_//

> +{
> +	if ((op->ninstrs == 1) &&
> +		(op->instrs[0].type == NAND_OP_CMD_INSTR) &&
> +		(op->instrs[0].ctx.cmd.opcode == NAND_CMD_RESET))
> +		return 1;
> +
> +	return 0;
> +}
> +
> +static int brcmnand_exec_op(struct nand_chip *chip,
> +			    const struct nand_operation *op,
> +			    bool check_only)
> +{
> +	struct brcmnand_host *host = nand_get_controller_data(chip);
> +	struct mtd_info *mtd = nand_to_mtd(chip);
> +	u8 *status;
> +	unsigned int i;
> +	int ret = 0;
> +
> +	if (check_only)
> +		return 0;
> +
> +	if (brcmnand_exec_op_is_status(op)) {
> +		status = op->instrs[1].ctx.data.buf.in;
> +		*status = brcmnand_status(host);
> +
> +		return 0;
> +	}

I would add the below chunk here:

	} else if (brcmnand_exec_op_is_reset(op)) {
		...

		return ...
	}

> +
> +	if (op->deassert_wp)
> +		brcmnand_wp(mtd, 0);
> +
> +	for (i = 0; i < op->ninstrs; i++) {
> +		ret = brcmnand_exec_instr(host, i, op);
> +		if (ret)
> +			break;
> +	}
> +
> +	if (op->deassert_wp)
> +		brcmnand_wp(mtd, 1);
> +
> +	if (brcmnand_exec_op_is_reset(op)) {
> +		brcmnand_wp(mtd, 1);
> +		brcmnand_status(host);
> +	}
> +
> +	return ret;
> +}

Thanks,
Miquèl

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

Re: [PATCH v4 4/4] mtd: rawnand: brcmnand: exec_op implementation

[David Regan]

Hi Miquèl,

On Mon, Nov 20, 2023 at 2:28 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote:
>
> Hello,
>
> dregan@broadcom.com wrote on Mon, 23 Oct 2023 10:14:44 -0700:
>
> > From: David Regan <dregan@broadcom.com>
> >
> > exec_op implementation for Broadcom STB, Broadband and iProc SoC
> > This adds exec_op and removes the legacy interface. Based on changes
> > proposed by Boris Brezillon.
> >
> > https://github.com/bbrezillon/linux/commit/4ec6f8d8d83f5aaca5d1877f02d48da96d41fcba
> > https://github.com/bbrezillon/linux/commit/11b4acffd761c4928652d7028d19fcd6f45e4696
> >
> > Signed-off-by: David Regan <dregan@broadcom.com>
>
> I'm fine with patches 1-3, a few minor nits on this version as well,
> nothing big. I guess I'll let some time to Florian as well to give his
> feedback and perhaps test the ->exec_op() implementation.

Thank you very much for your time, sorry for my missteps.
In the meantime I'll update and everyone can have additional
time to look.

>
> ...
>
> > +static int brcmnand_exec_instr(struct brcmnand_host *host, int i,
> > +                             const struct nand_operation *op)
> > +{
> > +     struct brcmnand_controller *ctrl = host->ctrl;
> > +     const struct nand_op_instr *instr = &op->instrs[i];
> > +     const u8 *out;
> > +     u8 *in;
> > +     int ret = 0;
> > +     bool last_op;
> > +
> > +     /*
> > +      * if we are on the last command in the sequence (not including
> > +      * waitrdy which is not a NAND command) then flag the controller
>
> May I suggest:
>
>         /*
>          * The controller needs to be aware of the last command in the operation
>          * (WAITRDY excepted).
>          */
>

Will change.

> > +      */
> > +     last_op = (((i == (op->ninstrs - 1)) &&
> > +                     (instr->type != NAND_OP_WAITRDY_INSTR)) ||
>
> You can cross the 80 chars boundary. Please use this form:
>
>         last_op = ((i == (op->ninstrs - 1)) && (instr->type != NAND_OP_WAITRDY_INSTR)) ||
>                   ((i == (op->ninstrs - 2)) && (op->instrs[i+1].type == NAND_OP_WAITRDY_INSTR));
>

Will do.

...

> > +static int brcmnand_exec_op_is_status(const struct nand_operation *op)
>
> brcmnand_op_is_status() would make more sense
>
> > +{
> > +     if ((op->ninstrs == 2) &&
> > +             (op->instrs[0].type == NAND_OP_CMD_INSTR) &&
> > +             (op->instrs[0].ctx.cmd.opcode == NAND_CMD_STATUS) &&
> > +             (op->instrs[1].type == NAND_OP_DATA_IN_INSTR))
> > +             return 1;
> > +
> > +     return 0;
> > +}
> > +
> > +static int brcmnand_exec_op_is_reset(const struct nand_operation *op)
>
> same here, please s/exec_//
>

I'll update the names.

> > +{
> > +     if ((op->ninstrs == 1) &&
> > +             (op->instrs[0].type == NAND_OP_CMD_INSTR) &&
> > +             (op->instrs[0].ctx.cmd.opcode == NAND_CMD_RESET))
> > +             return 1;
> > +
> > +     return 0;
> > +}
> > +
> > +static int brcmnand_exec_op(struct nand_chip *chip,
> > +                         const struct nand_operation *op,
> > +                         bool check_only)
> > +{
> > +     struct brcmnand_host *host = nand_get_controller_data(chip);
> > +     struct mtd_info *mtd = nand_to_mtd(chip);
> > +     u8 *status;
> > +     unsigned int i;
> > +     int ret = 0;
> > +
> > +     if (check_only)
> > +             return 0;
> > +
> > +     if (brcmnand_exec_op_is_status(op)) {
> > +             status = op->instrs[1].ctx.data.buf.in;
> > +             *status = brcmnand_status(host);
> > +
> > +             return 0;
> > +     }
>
> I would add the below chunk here:
>
>         } else if (brcmnand_exec_op_is_reset(op)) {
>                 ...
>
>                 return ...
>         }

Good idea will do.

...

>
> Thanks,
> Miquèl

Thanks!

-Dave

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

Re: [PATCH v4 1/4] mtd: rawnand: Add destructive operation

[William Zhang]

[-- Attachment #1.1: Type: text/plain, Size: 853 bytes --]



On 10/23/2023 10:14 AM, dregan@broadcom.com wrote:
> From: Boris Brezillon <bbrezillon@kernel.org>
> 
> Erase and program operations need the write protect (wp) pin to be
> de-asserted to take effect. Add the concept of destructive
> operation and pass the information to exec_op() so controllers know
> when they should de-assert this pin without having to decode
> the command opcode.
> 
> Signed-off-by: Boris Brezillon <bbrezillon@kernel.org>
> Signed-off-by: David Regan <dregan@broadcom.com>
> ---
> Changes in v4: none
> 
> Changes in v3: updated comments and email address
> 
> Changes in v2: gave credit to Boris Brezillon
> ---
>   drivers/mtd/nand/raw/nand_base.c | 6 ++++--
>   include/linux/mtd/rawnand.h      | 9 +++++++++
>   2 files changed, 13 insertions(+), 2 deletions(-)
> 

Reviewed-by: William Zhang <william.zhang@broadcom.com>

[-- Attachment #1.2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4212 bytes --]

[-- Attachment #2: Type: text/plain, Size: 144 bytes --]

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

Re: [PATCH v4 1/4] mtd: rawnand: Add destructive operation

[David Regan]

Hi Miquèl,

Can you please have a look at this update?
This patch has not received any change requests after two weeks but
let me know if you see anything that should be fixed.

Thanks!

-Dave

On Wed, Oct 25, 2023 at 5:25 PM William Zhang
<william.zhang@broadcom.com> wrote:
>
>
>
> On 10/23/2023 10:14 AM, dregan@broadcom.com wrote:
> > From: Boris Brezillon <bbrezillon@kernel.org>
> >
> > Erase and program operations need the write protect (wp) pin to be
> > de-asserted to take effect. Add the concept of destructive
> > operation and pass the information to exec_op() so controllers know
> > when they should de-assert this pin without having to decode
> > the command opcode.
> >
> > Signed-off-by: Boris Brezillon <bbrezillon@kernel.org>
> > Signed-off-by: David Regan <dregan@broadcom.com>
> > ---
> > Changes in v4: none
> >
> > Changes in v3: updated comments and email address
> >
> > Changes in v2: gave credit to Boris Brezillon
> > ---
> >   drivers/mtd/nand/raw/nand_base.c | 6 ++++--
> >   include/linux/mtd/rawnand.h      | 9 +++++++++
> >   2 files changed, 13 insertions(+), 2 deletions(-)
> >
>
> Reviewed-by: William Zhang <william.zhang@broadcom.com>

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