Re: [PATCH v2] mtd: rawnand: brcmnand: Initial exec_op implementation

From: William Zhang
Date: Mon Oct 02 2023 - 15:57:08 EST


Hi Miquel,

On 10/02/2023 05:35 AM, Miquel Raynal wrote:
Hi David,

dregan@xxxxxxxx wrote on Sat, 30 Sep 2023 03:57:35 +0200:

Initial exec_op implementation for Broadcom STB, Broadband and iProc SoC
This adds exec_op and removes the legacy interface.

Signed-off-by: David Regan <dregan@xxxxxxxx>
Reviewed-by: William Zhang <william.zhang@xxxxxxxxxxxx>

---


...

+static int brcmnand_parser_exec_matched_op(struct nand_chip *chip,
+ const struct nand_subop *subop)
+{
+ struct brcmnand_host *host = nand_get_controller_data(chip);
+ struct brcmnand_controller *ctrl = host->ctrl;
+ struct mtd_info *mtd = nand_to_mtd(chip);
+ const struct nand_op_instr *instr = &subop->instrs[0];
+ unsigned int i;
+ int ret = 0;
+
+ for (i = 0; i < subop->ninstrs; i++) {
+ instr = &subop->instrs[i];
+
+ if ((instr->type == NAND_OP_CMD_INSTR) &&
+ (instr->ctx.cmd.opcode == NAND_CMD_STATUS))
+ ctrl->status_cmd = 1;
+ else if (ctrl->status_cmd && (instr->type == NAND_OP_DATA_IN_INSTR)) {
+ /*
+ * need to fake the nand device write protect because nand_base does a
+ * nand_check_wp which calls nand_status_op NAND_CMD_STATUS which checks
+ * that the nand is not write protected before an operation starts.
+ * The problem with this is it's done outside exec_op so the nand is
+ * write protected and this check will fail until the write or erase
+ * or write back operation actually happens where we turn off wp.
+ */
+ u8 *in;
+
+ ctrl->status_cmd = 0;
+
+ instr = &subop->instrs[i];
+ in = instr->ctx.data.buf.in;
+ in[0] = brcmnand_status(host) | NAND_STATUS_WP; /* hide WP status */

I don't understand why you are faking the WP bit. If it's set,
brcmnand_status() should return it and you should not care about it. If
it's not however, can you please give me the path used when we have
this issue? Either we need to modify the core or we need to provide
additional helpers in this driver to circumvent the faulty path.

The reason we have to hide wp status for status command is because
nand_base calls nand_check_wp at the very beginning of write and erase
function. This applies to both exec_op path and legacy path. With
Broadcom nand controller and most of our board design using the WP pin
and have it asserted by default, the nand_check_wp function will fail
and write/erase aborts. This workaround has been there before this
exec_op patch.

I agree it is ugly and better to be addressed in the nand base code. And
I understand Broadcom's WP approach may sound a bit over cautious but we
want to make sure no spurious erase/write can happen under any
circumstance except software explicitly want to write and erase. WP is
standard nand chip pin and I think most the nand controller has that
that pin in the design too but it is possible it is not used and
bootloader can de-assert the pin and have a always-writable nand flash
for linux. So maybe we can add nand controller dts option "nand-use-wp".
If this property exist and set to 1, wp control is in use and nand
driver need to control the pin on/ff as needed when doing write and
erase function. Also nand base code should not call nand_check_wp when
wp is in use. Then we can remove the faking WP status workaround.


+ } else if (instr->type == NAND_OP_WAITRDY_INSTR) {
+ ret = bcmnand_ctrl_poll_status(host, NAND_CTRL_RDY, NAND_CTRL_RDY, 0);
+ if (ctrl->wp_cmd) {
+ ctrl->wp_cmd = 0;
+ brcmnand_wp(mtd, 1);

This ideally should disappear.

Maybe we can have the destructive operation patch from Borris.
Controller driver still need to assert/deassert the pin if it uses nand
wp feature but at least it does not need to guess the op code.

+ }
+ } else { /* otherwise pass to low level implementation */
+ if ((instr->type == NAND_OP_CMD_INSTR) &&
+ (instr->ctx.cmd.opcode == NAND_CMD_RESET)) {
+ brcmnand_status(host);
+ ctrl->status_cmd = 0;
+ ctrl->wp_cmd = 0;
+ brcmnand_wp(mtd, 1);

Same

+ }
+
+ if ((instr->type == NAND_OP_CMD_INSTR) &&
+ ((instr->ctx.cmd.opcode == NAND_CMD_ERASE1) ||
+ (instr->ctx.cmd.opcode == NAND_CMD_SEQIN))) {
+ brcmnand_wp(mtd, 0);
+ ctrl->wp_cmd = 1;

Same

+ }
+
+ ret = brcmnand_exec_instr(host, instr, i == (subop->ninstrs - 1));
+ }
+ }
+
+ return ret;
+}



Thanks,
Miquèl

Attachment: smime.p7s
Description: S/MIME Cryptographic Signature