| Message ID | 1572256527-5074-5-git-send-email-masonccyang@mxic.com.tw |
|---|---|
| State | Changes Requested |
| Delegated to: | Miquel Raynal |
| Headers | show |
| Series | mtd: rawnand: Add support Macronix Block Protection & deep power down mode | expand |
Hi Mason, Mason Yang <masonccyang@mxic.com.tw> wrote on Mon, 28 Oct 2019 17:55:27 +0800: > Macronix AD series support deep power down mode for a minimum > power consumption state. > > Patch nand_suspend() & nand_resume() by Macronix specific > deep power down mode command and exit it. > > Signed-off-by: Mason Yang <masonccyang@mxic.com.tw> > --- > drivers/mtd/nand/raw/nand_macronix.c | 72 +++++++++++++++++++++++++++++++++++- > 1 file changed, 70 insertions(+), 2 deletions(-) > > diff --git a/drivers/mtd/nand/raw/nand_macronix.c b/drivers/mtd/nand/raw/nand_macronix.c > index 13929bf..3098bc0 100644 > --- a/drivers/mtd/nand/raw/nand_macronix.c > +++ b/drivers/mtd/nand/raw/nand_macronix.c > @@ -15,6 +15,8 @@ > #define MXIC_BLOCK_PROTECTION_ALL_LOCK 0x38 > #define MXIC_BLOCK_PROTECTION_ALL_UNLOCK 0x0 > > +#define NAND_CMD_POWER_DOWN 0xB9 I suppose this value is Macronix specific, and hence should have a MACRONIX_ or MXIC_ prefix instead of NAND_. > + > struct nand_onfi_vendor_macronix { > u8 reserved; > u8 reliability_func; > @@ -137,13 +139,66 @@ static int mxic_nand_unlock(struct nand_chip *chip, loff_t ofs, uint64_t len) > return ret; > } > > +int nand_power_down_op(struct nand_chip *chip) > +{ > + int ret; > + > + if (nand_has_exec_op(chip)) { > + struct nand_op_instr instrs[] = { > + NAND_OP_CMD(NAND_CMD_POWER_DOWN, 0), > + }; > + > + struct nand_operation op = NAND_OPERATION(chip->cur_cs, instrs); > + > + ret = nand_exec_op(chip, &op); > + if (ret) > + return ret; > + > + } else { > + chip->legacy.cmdfunc(chip, NAND_CMD_POWER_DOWN, -1, -1); > + } > + > + return 0; > +} > + > +static int mxic_nand_suspend(struct nand_chip *chip) > +{ > + int ret; > + > + nand_select_target(chip, 0); > + ret = nand_power_down_op(chip); > + if (ret < 0) > + pr_err("%s called for chip into suspend failed\n", __func__); What about something more specific? "Suspending MXIC NAND chip failed (%)\n", ret > + nand_deselect_target(chip); > + > + return ret; > +} > + > +static void mxic_nand_resume(struct nand_chip *chip) > +{ > + /* > + * Toggle #CS pin to resume NAND device and don't care > + * of the others CLE, #WE, #RE pins status. > + * Here sending power down command to toggle #CS line. The first sentence seems right, the second could be upgraded: The purpose of doing a power down operation is just to ensure some bytes will be sent over the NAND bus so that #CS gets toggled because this is why the chip is woken up. The content of the bytes sent on the NAND bus are not relevant at this time. Sending bytes on the bus is mandatory for a lot of NAND controllers otherwise they are not able to just assert/de-assert #CS. > + */ > + nand_select_target(chip, 0); > + nand_power_down_op(chip); Are you sure sending a power_down_op will not be interpreted by the chip? I would expect a sleeping delay here, even small. > + nand_deselect_target(chip); > +} > + > /* > - * Macronix NAND AC series support Block Protection by SET_FEATURES > + * Macronix NAND AC & AD series support Block Protection by SET_FEATURES > * to lock/unlock blocks. > */ > static int macronix_nand_init(struct nand_chip *chip) > { > - bool blockprotected = false; > + unsigned int i; > + bool blockprotected = false, powerdown = false; > + static const char * const power_down_dev[] = { > + "MX30LF1G28AD", > + "MX30LF2G28AD", > + "MX30LF4G28AD", > + }; > > if (nand_is_slc(chip)) > chip->options |= NAND_BBM_FIRSTPAGE | NAND_BBM_SECONDPAGE; > @@ -153,6 +208,14 @@ static int macronix_nand_init(struct nand_chip *chip) > > macronix_nand_onfi_init(chip); > > + for (i = 0; i < ARRAY_SIZE(power_down_dev); i++) { > + if (!strcmp(power_down_dev[i], chip->parameters.model)) { > + blockprotected = true; > + powerdown = true; > + break; > + } > + } > + > if (blockprotected) { > bitmap_set(chip->parameters.set_feature_list, > ONFI_FEATURE_ADDR_MXIC_PROTECTION, 1); > @@ -163,6 +226,11 @@ static int macronix_nand_init(struct nand_chip *chip) > chip->_unlock = mxic_nand_unlock; > } > > + if (powerdown) { > + chip->_suspend = mxic_nand_suspend; > + chip->_resume = mxic_nand_resume; > + } See my comment on patch 2. > + > return 0; > } > Thanks, Miquèl
Hi Miquel, > > Macronix AD series support deep power down mode for a minimum > > power consumption state. > > > > Patch nand_suspend() & nand_resume() by Macronix specific > > deep power down mode command and exit it. > > > > Signed-off-by: Mason Yang <masonccyang@mxic.com.tw> > > --- > > drivers/mtd/nand/raw/nand_macronix.c | 72 +++++++++++++++++++++++++++++++++++- > > 1 file changed, 70 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/mtd/nand/raw/nand_macronix.c b/drivers/mtd/nand/raw/ > nand_macronix.c > > index 13929bf..3098bc0 100644 > > --- a/drivers/mtd/nand/raw/nand_macronix.c > > +++ b/drivers/mtd/nand/raw/nand_macronix.c > > @@ -15,6 +15,8 @@ > > #define MXIC_BLOCK_PROTECTION_ALL_LOCK 0x38 > > #define MXIC_BLOCK_PROTECTION_ALL_UNLOCK 0x0 > > > > +#define NAND_CMD_POWER_DOWN 0xB9 > > I suppose this value is Macronix specific, and hence should have a > MACRONIX_ or MXIC_ prefix instead of NAND_. okay, will patch it to #define MXIC_CMD_POWER_DOWN 0xB9 > > > + > > struct nand_onfi_vendor_macronix { > > u8 reserved; > > u8 reliability_func; > > @@ -137,13 +139,66 @@ static int mxic_nand_unlock(struct nand_chip *chip, > loff_t ofs, uint64_t len) > > return ret; > > } > > > > +int nand_power_down_op(struct nand_chip *chip) > > +{ > > + int ret; > > + > > + if (nand_has_exec_op(chip)) { > > + struct nand_op_instr instrs[] = { > > + NAND_OP_CMD(NAND_CMD_POWER_DOWN, 0), > > + }; > > + > > + struct nand_operation op = NAND_OPERATION(chip->cur_cs, instrs); > > + > > + ret = nand_exec_op(chip, &op); > > + if (ret) > > + return ret; > > + > > + } else { > > + chip->legacy.cmdfunc(chip, NAND_CMD_POWER_DOWN, -1, -1); > > + } > > + > > + return 0; > > +} > > + > > +static int mxic_nand_suspend(struct nand_chip *chip) > > +{ > > + int ret; > > + > > + nand_select_target(chip, 0); > > + ret = nand_power_down_op(chip); > > + if (ret < 0) > > + pr_err("%s called for chip into suspend failed\n", __func__); > > What about something more specific? > > "Suspending MXIC NAND chip failed (%)\n", ret okay, patch it with this sentence, thanks. > > > + nand_deselect_target(chip); > > + > > + return ret; > > +} > > + > > +static void mxic_nand_resume(struct nand_chip *chip) > > +{ > > + /* > > + * Toggle #CS pin to resume NAND device and don't care > > + * of the others CLE, #WE, #RE pins status. > > + * Here sending power down command to toggle #CS line. > > The first sentence seems right, the second could be upgraded: > > The purpose of doing a power down operation is just to > ensure some bytes will be sent over the NAND bus so that #CS > gets toggled because this is why the chip is woken up. > The content of the bytes sent on the NAND bus are not > relevant at this time. Sending bytes on the bus is mandatory > for a lot of NAND controllers otherwise they are not able to > just assert/de-assert #CS. okay, will patch the second sentence based on your comments. i.e,. /* * Toggle #CS pin to resume NAND device and don't care * of the others CLE, #WE, #RE pins status. * A NAND controller ensure it is able to assert/de-assert #CS * by sending any byte over the NAND bus. * i.e., * NAND power down command or reset command. */ > > > + */ > > + nand_select_target(chip, 0); > > + nand_power_down_op(chip); > > Are you sure sending a power_down_op will not be interpreted by the > chip? yes, sure ! > > I would expect a sleeping delay here, even small. okay, will patch it. > > > + nand_deselect_target(chip); > > +} > > + > > /* > > - * Macronix NAND AC series support Block Protection by SET_FEATURES > > + * Macronix NAND AC & AD series support Block Protection by SET_FEATURES > > * to lock/unlock blocks. > > */ > > static int macronix_nand_init(struct nand_chip *chip) > > { > > - bool blockprotected = false; > > + unsigned int i; > > + bool blockprotected = false, powerdown = false; > > + static const char * const power_down_dev[] = { > > + "MX30LF1G28AD", > > + "MX30LF2G28AD", > > + "MX30LF4G28AD", > > + }; > > > > if (nand_is_slc(chip)) > > chip->options |= NAND_BBM_FIRSTPAGE | NAND_BBM_SECONDPAGE; > > @@ -153,6 +208,14 @@ static int macronix_nand_init(struct nand_chip *chip) > > > > macronix_nand_onfi_init(chip); > > > > + for (i = 0; i < ARRAY_SIZE(power_down_dev); i++) { > > + if (!strcmp(power_down_dev[i], chip->parameters.model)) { > > + blockprotected = true; > > + powerdown = true; > > + break; > > + } > > + } > > + > > if (blockprotected) { > > bitmap_set(chip->parameters.set_feature_list, > > ONFI_FEATURE_ADDR_MXIC_PROTECTION, 1); > > @@ -163,6 +226,11 @@ static int macronix_nand_init(struct nand_chip *chip) > > chip->_unlock = mxic_nand_unlock; > > } > > > > + if (powerdown) { > > + chip->_suspend = mxic_nand_suspend; > > + chip->_resume = mxic_nand_resume; > > + } > > See my comment on patch 2. ok, got it. thanks for your time & comments. Mason CONFIDENTIALITY NOTE: This e-mail and any attachments may contain confidential information and/or personal data, which is protected by applicable laws. Please be reminded that duplication, disclosure, distribution, or use of this e-mail (and/or its attachments) or any part thereof is prohibited. If you receive this e-mail in error, please notify us immediately and delete this mail as well as its attachment(s) from your system. In addition, please be informed that collection, processing, and/or use of personal data is prohibited unless expressly permitted by personal data protection laws. Thank you for your attention and cooperation. Macronix International Co., Ltd. ===================================================================== ============================================================================ CONFIDENTIALITY NOTE: This e-mail and any attachments may contain confidential information and/or personal data, which is protected by applicable laws. Please be reminded that duplication, disclosure, distribution, or use of this e-mail (and/or its attachments) or any part thereof is prohibited. If you receive this e-mail in error, please notify us immediately and delete this mail as well as its attachment(s) from your system. In addition, please be informed that collection, processing, and/or use of personal data is prohibited unless expressly permitted by personal data protection laws. Thank you for your attention and cooperation. Macronix International Co., Ltd. =====================================================================
diff --git a/drivers/mtd/nand/raw/nand_macronix.c b/drivers/mtd/nand/raw/nand_macronix.c index 13929bf..3098bc0 100644 --- a/drivers/mtd/nand/raw/nand_macronix.c +++ b/drivers/mtd/nand/raw/nand_macronix.c @@ -15,6 +15,8 @@ #define MXIC_BLOCK_PROTECTION_ALL_LOCK 0x38 #define MXIC_BLOCK_PROTECTION_ALL_UNLOCK 0x0 +#define NAND_CMD_POWER_DOWN 0xB9 + struct nand_onfi_vendor_macronix { u8 reserved; u8 reliability_func; @@ -137,13 +139,66 @@ static int mxic_nand_unlock(struct nand_chip *chip, loff_t ofs, uint64_t len) return ret; } +int nand_power_down_op(struct nand_chip *chip) +{ + int ret; + + if (nand_has_exec_op(chip)) { + struct nand_op_instr instrs[] = { + NAND_OP_CMD(NAND_CMD_POWER_DOWN, 0), + }; + + struct nand_operation op = NAND_OPERATION(chip->cur_cs, instrs); + + ret = nand_exec_op(chip, &op); + if (ret) + return ret; + + } else { + chip->legacy.cmdfunc(chip, NAND_CMD_POWER_DOWN, -1, -1); + } + + return 0; +} + +static int mxic_nand_suspend(struct nand_chip *chip) +{ + int ret; + + nand_select_target(chip, 0); + ret = nand_power_down_op(chip); + if (ret < 0) + pr_err("%s called for chip into suspend failed\n", __func__); + nand_deselect_target(chip); + + return ret; +} + +static void mxic_nand_resume(struct nand_chip *chip) +{ + /* + * Toggle #CS pin to resume NAND device and don't care + * of the others CLE, #WE, #RE pins status. + * Here sending power down command to toggle #CS line. + */ + nand_select_target(chip, 0); + nand_power_down_op(chip); + nand_deselect_target(chip); +} + /* - * Macronix NAND AC series support Block Protection by SET_FEATURES + * Macronix NAND AC & AD series support Block Protection by SET_FEATURES * to lock/unlock blocks. */ static int macronix_nand_init(struct nand_chip *chip) { - bool blockprotected = false; + unsigned int i; + bool blockprotected = false, powerdown = false; + static const char * const power_down_dev[] = { + "MX30LF1G28AD", + "MX30LF2G28AD", + "MX30LF4G28AD", + }; if (nand_is_slc(chip)) chip->options |= NAND_BBM_FIRSTPAGE | NAND_BBM_SECONDPAGE; @@ -153,6 +208,14 @@ static int macronix_nand_init(struct nand_chip *chip) macronix_nand_onfi_init(chip); + for (i = 0; i < ARRAY_SIZE(power_down_dev); i++) { + if (!strcmp(power_down_dev[i], chip->parameters.model)) { + blockprotected = true; + powerdown = true; + break; + } + } + if (blockprotected) { bitmap_set(chip->parameters.set_feature_list, ONFI_FEATURE_ADDR_MXIC_PROTECTION, 1); @@ -163,6 +226,11 @@ static int macronix_nand_init(struct nand_chip *chip) chip->_unlock = mxic_nand_unlock; } + if (powerdown) { + chip->_suspend = mxic_nand_suspend; + chip->_resume = mxic_nand_resume; + } + return 0; }
Macronix AD series support deep power down mode for a minimum power consumption state. Patch nand_suspend() & nand_resume() by Macronix specific deep power down mode command and exit it. Signed-off-by: Mason Yang <masonccyang@mxic.com.tw> --- drivers/mtd/nand/raw/nand_macronix.c | 72 +++++++++++++++++++++++++++++++++++- 1 file changed, 70 insertions(+), 2 deletions(-)