| Message ID | trinity-06dd34f4-ab26-4c60-bcf8-f986f1d08058-1696039055941@3c-app-mailcom-lxa04 |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] mtd: rawnand: brcmnand: Initial exec_op implementation | expand |
Hi David, dregan@mail.com 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@mail.com> > Reviewed-by: William Zhang <william.zhang@broadcom.com> > > --- > ... > +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. > + } 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. > + } > + } 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
Hi Miquel, On 10/02/2023 05:35 AM, Miquel Raynal wrote: > Hi David, > > dregan@mail.com 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@mail.com> >> Reviewed-by: William Zhang <william.zhang@broadcom.com> >> >> --- >> > > ... > >> +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 >
Hi William, william.zhang@broadcom.com wrote on Mon, 2 Oct 2023 12:57:01 -0700: > Hi Miquel, > > On 10/02/2023 05:35 AM, Miquel Raynal wrote: > > Hi David, > > > > dregan@mail.com 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@mail.com> > >> Reviewed-by: William Zhang <william.zhang@broadcom.com> > >> > >> --- > >> > > > > ... > > > >> +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. Ah, yeah, I get it. Please be my guest, you can revive this patch series (might need light tweaking, nothing big) and also take inspiration from it if necessary: https://github.com/bbrezillon/linux/commit/e612e1f2c69a33ac5f2c91d13669f0f172d58717 https://github.com/bbrezillon/linux/commit/4ec6f8d8d83f5aaca5d1877f02d48da96d41fcba https://github.com/bbrezillon/linux/commit/11b4acffd761c4928652d7028d19fcd6f45e4696 Thanks, Miquèl
Hi Miquel, On 10/03/2023 02:28 AM, Miquel Raynal wrote: > Hi William, > > william.zhang@broadcom.com wrote on Mon, 2 Oct 2023 12:57:01 -0700: > >> Hi Miquel, >> >> On 10/02/2023 05:35 AM, Miquel Raynal wrote: >>> Hi David, >>> >>> dregan@mail.com 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@mail.com> >>>> Reviewed-by: William Zhang <william.zhang@broadcom.com> >>>> >>>> --- >>>> >>> >>> ... >>> >>>> +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. > > Ah, yeah, I get it. > > Please be my guest, you can revive this patch series (might need light > tweaking, nothing big) and also take inspiration from it if necessary: > https://github.com/bbrezillon/linux/commit/e612e1f2c69a33ac5f2c91d13669f0f172d58717 > https://github.com/bbrezillon/linux/commit/4ec6f8d8d83f5aaca5d1877f02d48da96d41fcba > https://github.com/bbrezillon/linux/commit/11b4acffd761c4928652d7028d19fcd6f45e4696 > Sure we will incorporate the destructive operation patch and provide a new revision. The WP status workaround will stay at least for this change. If you think my suggestion using a dts setting above is okay, we can provide a patch for that as well. Or if you have any other idea or suggestion, we'd like to hear too. > Thanks, > Miquèl >
Hi William, william.zhang@broadcom.com wrote on Tue, 3 Oct 2023 11:46:25 -0700: > Hi Miquel, > > On 10/03/2023 02:28 AM, Miquel Raynal wrote: > > Hi William, > > > > william.zhang@broadcom.com wrote on Mon, 2 Oct 2023 12:57:01 -0700: > > > >> Hi Miquel, > >> > >> On 10/02/2023 05:35 AM, Miquel Raynal wrote: > >>> Hi David, > >>> > >>> dregan@mail.com 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@mail.com> > >>>> Reviewed-by: William Zhang <william.zhang@broadcom.com> > >>>> > >>>> --- > >>>> >>> > >>> ... > >>> >>>> +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. > > > > Ah, yeah, I get it. > > > > Please be my guest, you can revive this patch series (might need light > > tweaking, nothing big) and also take inspiration from it if necessary: > > https://github.com/bbrezillon/linux/commit/e612e1f2c69a33ac5f2c91d13669f0f172d58717 > > https://github.com/bbrezillon/linux/commit/4ec6f8d8d83f5aaca5d1877f02d48da96d41fcba > > https://github.com/bbrezillon/linux/commit/11b4acffd761c4928652d7028d19fcd6f45e4696 > > > Sure we will incorporate the destructive operation patch and provide a > new revision. > > The WP status workaround will stay at least for this change. If you > think my suggestion using a dts setting above is okay, we can provide a > patch for that as well. Or if you have any other idea or suggestion, > we'd like to hear too. I thought this was not needed as Boris initial conversion did not need it. The goal is to get rid of this workaround. Thanks, Miquèl
On 10/03/2023 03:55 PM, Miquel Raynal wrote: > Hi William, > > william.zhang@broadcom.com wrote on Tue, 3 Oct 2023 11:46:25 -0700: > >> Hi Miquel, >> >> On 10/03/2023 02:28 AM, Miquel Raynal wrote: >>> Hi William, >>> >>> william.zhang@broadcom.com wrote on Mon, 2 Oct 2023 12:57:01 -0700: >>> >>>> Hi Miquel, >>>> >>>> On 10/02/2023 05:35 AM, Miquel Raynal wrote: >>>>> Hi David, >>>>> >>>>> dregan@mail.com 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@mail.com> >>>>>> Reviewed-by: William Zhang <william.zhang@broadcom.com> >>>>>> >>>>>> --- >>>>>> >>> >>>>> ... >>>>> >>>> +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. >>> >>> Ah, yeah, I get it. >>> >>> Please be my guest, you can revive this patch series (might need light >>> tweaking, nothing big) and also take inspiration from it if necessary: >>> https://github.com/bbrezillon/linux/commit/e612e1f2c69a33ac5f2c91d13669f0f172d58717 >>> https://github.com/bbrezillon/linux/commit/4ec6f8d8d83f5aaca5d1877f02d48da96d41fcba >>> https://github.com/bbrezillon/linux/commit/11b4acffd761c4928652d7028d19fcd6f45e4696 >>> >> Sure we will incorporate the destructive operation patch and provide a >> new revision. >> >> The WP status workaround will stay at least for this change. If you >> think my suggestion using a dts setting above is okay, we can provide a >> patch for that as well. Or if you have any other idea or suggestion, >> we'd like to hear too. > > I thought this was not needed as Boris initial conversion did not need > it. The goal is to get rid of this workaround. > Boris' initial patch did remove that workaround but it will break the board that uses WP pin because the nand_check_wp run before the exec_op and status returned is write-protected in the erase and write function. I explained that above and you can see the code here: https://elixir.bootlin.com/linux/v6.6-rc4/source/drivers/mtd/nand/raw/nand_base.c#L4599 I agree with your goal to remove this workaround and we have suggested one possible fix but we are also open to any other solution. > Thanks, > Miquèl >
Hi Miquel, On 10/03/2023 09:47 PM, William Zhang wrote: > > > On 10/03/2023 03:55 PM, Miquel Raynal wrote: >> Hi William, >> >> william.zhang@broadcom.com wrote on Tue, 3 Oct 2023 11:46:25 -0700: >> >>> Hi Miquel, >>> >>> On 10/03/2023 02:28 AM, Miquel Raynal wrote: >>>> Hi William, >>>> >>>> william.zhang@broadcom.com wrote on Mon, 2 Oct 2023 12:57:01 -0700: >>>>> Hi Miquel, >>>>> >>>>> On 10/02/2023 05:35 AM, Miquel Raynal wrote: >>>>>> Hi David, >>>>>> >>>>>> dregan@mail.com 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@mail.com> >>>>>>> Reviewed-by: William Zhang <william.zhang@broadcom.com> >>>>>>> >>>>>>> --- >>>>>>> >>> >>>>>> ... >>>>>> >>>> +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. >>>> >>>> Ah, yeah, I get it. >>>> >>>> Please be my guest, you can revive this patch series (might need light >>>> tweaking, nothing big) and also take inspiration from it if necessary: >>>> https://github.com/bbrezillon/linux/commit/e612e1f2c69a33ac5f2c91d13669f0f172d58717 >>>> >>>> https://github.com/bbrezillon/linux/commit/4ec6f8d8d83f5aaca5d1877f02d48da96d41fcba >>>> >>>> https://github.com/bbrezillon/linux/commit/11b4acffd761c4928652d7028d19fcd6f45e4696 >>>> >>> Sure we will incorporate the destructive operation patch and provide a >>> new revision. >>> >>> The WP status workaround will stay at least for this change. If you >>> think my suggestion using a dts setting above is okay, we can provide a >>> patch for that as well. Or if you have any other idea or suggestion, >>> we'd like to hear too. >> >> I thought this was not needed as Boris initial conversion did not need >> it. The goal is to get rid of this workaround. >> Boris' initial patch did remove that workaround but it will break the > board that uses WP pin because the nand_check_wp run before the exec_op > and status returned is write-protected in the erase and write function. > I explained that above and you can see the code here: > https://elixir.bootlin.com/linux/v6.6-rc4/source/drivers/mtd/nand/raw/nand_base.c#L4599 > > > I agree with your goal to remove this workaround and we have suggested > one possible fix but we are also open to any other solution. > We have integrated the destructive operation patch and are ready for the v3. If you don't think my proposal on the WP status fix is a good idea, can we get this exce_op conversion patch series going first? After all, we don't modify the WP status handling behavior in this patch. We can fix it in another patch whenever we agree on a solution. Please let me know and thanks a lot for all your comments and thoughts. Thanks, William > >> Thanks, >> Miquèl >>
Hi William, william.zhang@broadcom.com wrote on Thu, 5 Oct 2023 17:42:21 -0700: > Hi Miquel, > > On 10/03/2023 09:47 PM, William Zhang wrote: > > > > > > On 10/03/2023 03:55 PM, Miquel Raynal wrote: > >> Hi William, > >> > >> william.zhang@broadcom.com wrote on Tue, 3 Oct 2023 11:46:25 -0700: > >> > >>> Hi Miquel, > >>> > >>> On 10/03/2023 02:28 AM, Miquel Raynal wrote: > >>>> Hi William, > >>>> > >>>> william.zhang@broadcom.com wrote on Mon, 2 Oct 2023 12:57:01 -0700: > >>>>> Hi Miquel, > >>>>> > >>>>> On 10/02/2023 05:35 AM, Miquel Raynal wrote: > >>>>>> Hi David, > >>>>>> > >>>>>> dregan@mail.com 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@mail.com> > >>>>>>> Reviewed-by: William Zhang <william.zhang@broadcom.com> > >>>>>>> > >>>>>>> --- > >>>>>>> >>> > >>>>>> ... > >>>>>> >>>> +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. > >>>> > >>>> Ah, yeah, I get it. > >>>> > >>>> Please be my guest, you can revive this patch series (might need light > >>>> tweaking, nothing big) and also take inspiration from it if necessary: > >>>> https://github.com/bbrezillon/linux/commit/e612e1f2c69a33ac5f2c91d13669f0f172d58717 >>>> > >>>> https://github.com/bbrezillon/linux/commit/4ec6f8d8d83f5aaca5d1877f02d48da96d41fcba >>>> > >>>> https://github.com/bbrezillon/linux/commit/11b4acffd761c4928652d7028d19fcd6f45e4696 >>>> > >>> Sure we will incorporate the destructive operation patch and provide a > >>> new revision. > >>> > >>> The WP status workaround will stay at least for this change. If you > >>> think my suggestion using a dts setting above is okay, we can provide a > >>> patch for that as well. Or if you have any other idea or suggestion, > >>> we'd like to hear too. > >> > >> I thought this was not needed as Boris initial conversion did not need > >> it. The goal is to get rid of this workaround. > >> Boris' initial patch did remove that workaround but it will break the > > board that uses WP pin because the nand_check_wp run before the exec_op > and status returned is write-protected in the erase and write function. > > I explained that above and you can see the code here: > > https://elixir.bootlin.com/linux/v6.6-rc4/source/drivers/mtd/nand/raw/nand_base.c#L4599 > > > > I agree with your goal to remove this workaround and we have suggested > > one possible fix but we are also open to any other solution. > > > We have integrated the destructive operation patch and are ready for the > v3. If you don't think my proposal on the WP status fix is a good idea, > can we get this exce_op conversion patch series going first? After all, > we don't modify the WP status handling behavior in this patch. We can > fix it in another patch whenever we agree on a solution. Please let me > know and thanks a lot for all your comments and thoughts. The NAND core has been a playground for coding horrors sometimes, and this ->exec_op() conversion is us the way to a cleaner and mastered approach, I am not willing to let something that obvious get in, I'm sorry. For you it's just a workaround, for me it means any change in the core will just break with this controller. This is of course not against you or your work, perhaps I should emphasize that I strongly appreciate your efforts and, besides this workaround the code is clean. The problem is that the WP pin can be used in two different ways: internally and externally. When it's used externally, you expect it to be deasserted before you start a destructive operation. When you use it internally, you expect it to be deasserted during the destructive operation. The final solution needs to be approved by comparing with similar drivers which perform this internal procedure themselves as well. Maybe we could add a flag somewhere in the core's controller structure to tell the core not to perform these checks as we master the handling of the WP pin, telling the controller will handle it correctly as long as the destructive flag is passed. Thanks, Miquèl
On 10/06/2023 12:42 AM, Miquel Raynal wrote: > Hi William, > > william.zhang@broadcom.com wrote on Thu, 5 Oct 2023 17:42:21 -0700: > >> Hi Miquel, >> >> On 10/03/2023 09:47 PM, William Zhang wrote: >>> >>> >>> On 10/03/2023 03:55 PM, Miquel Raynal wrote: >>>> Hi William, >>>> >>>> william.zhang@broadcom.com wrote on Tue, 3 Oct 2023 11:46:25 -0700: >>>> >>>>> Hi Miquel, >>>>> >>>>> On 10/03/2023 02:28 AM, Miquel Raynal wrote: >>>>>> Hi William, >>>>>> >>>>>> william.zhang@broadcom.com wrote on Mon, 2 Oct 2023 12:57:01 -0700: >>>>>>> Hi Miquel, >>>>>>> >>>>>>> On 10/02/2023 05:35 AM, Miquel Raynal wrote: >>>>>>>> Hi David, >>>>>>>> >>>>>>>> dregan@mail.com 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@mail.com> >>>>>>>>> Reviewed-by: William Zhang <william.zhang@broadcom.com> >>>>>>>>> >>>>>>>>> --- >>>>>>>>> >>> >>>>>>>> ... >>>>>>>> >>>> +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. >>>>>> >>>>>> Ah, yeah, I get it. >>>>>> >>>>>> Please be my guest, you can revive this patch series (might need light >>>>>> tweaking, nothing big) and also take inspiration from it if necessary: >>>>>> https://github.com/bbrezillon/linux/commit/e612e1f2c69a33ac5f2c91d13669f0f172d58717 >>>> >>>>>> https://github.com/bbrezillon/linux/commit/4ec6f8d8d83f5aaca5d1877f02d48da96d41fcba >>>> >>>>>> https://github.com/bbrezillon/linux/commit/11b4acffd761c4928652d7028d19fcd6f45e4696 >>>> >>>>> Sure we will incorporate the destructive operation patch and provide a >>>>> new revision. >>>>> >>>>> The WP status workaround will stay at least for this change. If you >>>>> think my suggestion using a dts setting above is okay, we can provide a >>>>> patch for that as well. Or if you have any other idea or suggestion, >>>>> we'd like to hear too. >>>> >>>> I thought this was not needed as Boris initial conversion did not need >>>> it. The goal is to get rid of this workaround. >>>> Boris' initial patch did remove that workaround but it will break the >>> board that uses WP pin because the nand_check_wp run before the exec_op > and status returned is write-protected in the erase and write function. >>> I explained that above and you can see the code here: >>> https://elixir.bootlin.com/linux/v6.6-rc4/source/drivers/mtd/nand/raw/nand_base.c#L4599 > > >>> I agree with your goal to remove this workaround and we have suggested >>> one possible fix but we are also open to any other solution. >>> >> We have integrated the destructive operation patch and are ready for the >> v3. If you don't think my proposal on the WP status fix is a good idea, >> can we get this exce_op conversion patch series going first? After all, >> we don't modify the WP status handling behavior in this patch. We can >> fix it in another patch whenever we agree on a solution. Please let me >> know and thanks a lot for all your comments and thoughts. > > The NAND core has been a playground for coding horrors sometimes, and > this ->exec_op() conversion is us the way to a cleaner and mastered > approach, I am not willing to let something that obvious get in, I'm > sorry. For you it's just a workaround, for me it means any change in > the core will just break with this controller. > > This is of course not against you or your work, perhaps I should > emphasize that I strongly appreciate your efforts and, besides this > workaround the code is clean. > > The problem is that the WP pin can be used in two different ways: > internally and externally. When it's used externally, you expect it > to be deasserted before you start a destructive operation. When you use > it internally, you expect it to be deasserted during the destructive > operation. > > The final solution needs to be approved by comparing with > similar drivers which perform this internal procedure themselves > as well. Maybe we could add a flag somewhere in the core's controller > structure to tell the core not to perform these checks as we master the > handling of the WP pin, telling the controller will handle it > correctly as long as the destructive flag is passed. > Thanks for the comments and I think we are on the same page. We will add the flag in the core controller's and provide the updated patches. > Thanks, Miquèl >
diff --git a/drivers/mtd/nand/raw/brcmnand/brcmnand.c b/drivers/mtd/nand/raw/brcmnand/brcmnand.c index 440bef477930..5fc83fa11ad3 100644 --- a/drivers/mtd/nand/raw/brcmnand/brcmnand.c +++ b/drivers/mtd/nand/raw/brcmnand/brcmnand.c @@ -282,6 +282,8 @@ struct brcmnand_controller { u32 flash_dma_mode; u32 flash_edu_mode; bool pio_poll_mode; + bool status_cmd; + bool wp_cmd; }; struct brcmnand_cfg { @@ -625,6 +627,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 +1026,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; @@ -1061,10 +1052,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; @@ -1073,6 +1065,9 @@ static int bcmnand_ctrl_poll_status(struct brcmnand_controller *ctrl, 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; @@ -1379,7 +1374,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); @@ -1387,9 +1382,10 @@ 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(ctrl, + ret = bcmnand_ctrl_poll_status(host, NAND_CTRL_RDY | NAND_STATUS_READY | NAND_STATUS_WP, @@ -1629,13 +1625,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 */ @@ -1643,16 +1639,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); @@ -1664,7 +1650,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 { @@ -1703,6 +1689,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), @@ -1752,190 +1749,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 */ @@ -2345,8 +2158,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); @@ -2359,8 +2173,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, @@ -2468,11 +2283,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, @@ -2481,13 +2296,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) @@ -2511,6 +2328,142 @@ static int brcmnand_write_oob_raw(struct nand_chip *chip, int page) return ret; } +static int brcmnand_exec_instr(struct brcmnand_host *host, + const struct nand_op_instr *instr, + bool last_op) +{ + struct brcmnand_controller *ctrl = host->ctrl; + unsigned int i; + const u8 *out; + u8 *in; + int ret = 0; + + bcmnand_ctrl_poll_status(host, NAND_CTRL_RDY, NAND_CTRL_RDY, 0); + + 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); + 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); + 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); + break; + + default: + dev_err(ctrl->dev, "unsupported instruction type: %d\n", + instr->type); + ret = -EINVAL; + break; + } + + return ret; +} + +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 */ + } 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); + } + } 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); + } + + 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; + } + + ret = brcmnand_exec_instr(host, instr, i == (subop->ninstrs - 1)); + } + } + + return ret; +} + +static const struct nand_op_parser brcmnand_op_parser = NAND_OP_PARSER( + NAND_OP_PARSER_PATTERN( + brcmnand_parser_exec_matched_op, + NAND_OP_PARSER_PAT_CMD_ELEM(true)), + NAND_OP_PARSER_PATTERN( + brcmnand_parser_exec_matched_op, + NAND_OP_PARSER_PAT_ADDR_ELEM(true, 8)), + NAND_OP_PARSER_PATTERN( + brcmnand_parser_exec_matched_op, + NAND_OP_PARSER_PAT_DATA_IN_ELEM(true, 8192)), + NAND_OP_PARSER_PATTERN( + brcmnand_parser_exec_matched_op, + NAND_OP_PARSER_PAT_DATA_OUT_ELEM(true, 8192)), + NAND_OP_PARSER_PATTERN( + brcmnand_parser_exec_matched_op, + NAND_OP_PARSER_PAT_WAITRDY_ELEM(true)), +); + +static int brcmnand_exec_op(struct nand_chip *chip, + const struct nand_operation *op, + bool check_only) +{ + return nand_op_parser_exec_op(chip, &brcmnand_op_parser, op, check_only); +} + /*********************************************************************** * Per-CS setup (1 NAND device) ***********************************************************************/ @@ -2821,6 +2774,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, @@ -2845,13 +2799,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; @@ -3071,6 +3018,9 @@ int brcmnand_probe(struct platform_device *pdev, struct brcmnand_soc *soc) if (brcmnand_soc_has_ops(ctrl->soc)) static_branch_enable(&brcmnand_soc_has_ops_key); + ctrl->status_cmd = 0; + ctrl->wp_cmd = 0; + init_completion(&ctrl->done); init_completion(&ctrl->dma_done); init_completion(&ctrl->edu_done);