| Message ID | 1568793387-25199-2-git-send-email-masonccyang@mxic.com.tw |
|---|---|
| State | Changes Requested |
| Delegated to: | Miquel Raynal |
| Headers | show |
| Series | [RFC,1/3] mtd: rawnand: Add support manufacturer postponed initialization | expand |
Hi Mason, Mason Yang <masonccyang@mxic.com.tw> wrote on Wed, 18 Sep 2019 15:56:25 +0800: > Macronix AC series support using SET/GET_FEATURES to change > Block Protection and Unprotection. > > MTD default _lock/_unlock function replacement by manufacturer > postponed initialization. Why would we do that? Anyway your solution looks overkill, if we ever decide to implement these hooks for raw nand, it is better just to not overwrite them in nand_scan_tail() if they have been filled previously (ie. by the manufacturer code). > Signed-off-by: Mason Yang <masonccyang@mxic.com.tw> > --- > drivers/mtd/nand/raw/nand_macronix.c | 80 +++++++++++++++++++++++++++++++++--- > 1 file changed, 75 insertions(+), 5 deletions(-) > > diff --git a/drivers/mtd/nand/raw/nand_macronix.c b/drivers/mtd/nand/raw/nand_macronix.c > index 58511ae..991c636 100644 > --- a/drivers/mtd/nand/raw/nand_macronix.c > +++ b/drivers/mtd/nand/raw/nand_macronix.c > @@ -11,6 +11,10 @@ > #define MACRONIX_READ_RETRY_BIT BIT(0) > #define MACRONIX_NUM_READ_RETRY_MODES 6 > > +#define ONFI_FEATURE_ADDR_MXIC_PROTECTION 0xA0 > +#define MXIC_BLOCK_PROTECTION_ALL_LOCK 0x38 > +#define MXIC_BLOCK_PROTECTION_ALL_UNLOCK 0x0 > + > struct nand_onfi_vendor_macronix { > u8 reserved; > u8 reliability_func; > @@ -57,10 +61,7 @@ static void macronix_nand_onfi_init(struct nand_chip *chip) > * the timings unlike what is declared in the parameter page. Unflag > * this feature to avoid unnecessary downturns. > */ > -static void macronix_nand_fix_broken_get_timings(struct nand_chip *chip) > -{ > - unsigned int i; > - static const char * const broken_get_timings[] = { > +static const char * const broken_get_timings[] = { > "MX30LF1G18AC", > "MX30LF1G28AC", > "MX30LF2G18AC", > @@ -75,7 +76,11 @@ static void macronix_nand_fix_broken_get_timings(struct nand_chip *chip) > "MX30UF4G18AC", > "MX30UF4G16AC", > "MX30UF4G28AC", > - }; > +}; > + > +static void macronix_nand_fix_broken_get_timings(struct nand_chip *chip) > +{ > + unsigned int i; > > if (!chip->parameters.supports_set_get_features) > return; > @@ -105,6 +110,71 @@ static int macronix_nand_init(struct nand_chip *chip) > return 0; > } > > +static int mxic_nand_lock(struct mtd_info *mtd, loff_t ofs, uint64_t len) > +{ > + struct nand_chip *chip = mtd_to_nand(mtd); > + u8 feature[ONFI_SUBFEATURE_PARAM_LEN]; > + int ret; > + > + feature[0] = MXIC_BLOCK_PROTECTION_ALL_LOCK; > + nand_select_target(chip, 0); > + ret = nand_set_features(chip, ONFI_FEATURE_ADDR_MXIC_PROTECTION, > + feature); > + nand_deselect_target(chip); > + if (ret) > + pr_err("%s all blocks failed\n", __func__); > + > + return ret; > +} > + > +static int mxic_nand_unlock(struct mtd_info *mtd, loff_t ofs, uint64_t len) > +{ > + struct nand_chip *chip = mtd_to_nand(mtd); > + u8 feature[ONFI_SUBFEATURE_PARAM_LEN]; > + int ret; > + > + feature[0] = MXIC_BLOCK_PROTECTION_ALL_UNLOCK; > + nand_select_target(chip, 0); > + ret = nand_set_features(chip, ONFI_FEATURE_ADDR_MXIC_PROTECTION, > + feature); > + nand_deselect_target(chip); > + if (ret) > + pr_err("%s all blocks failed\n", __func__); > + > + return ret; > +} > + > +/* > + * Macronix AC series support using SET/GET_FEATURES to change > + * Block Protection and Unprotection. > + * > + * MTD call-back function replacement by manufacturer postponed > + * initialization. > + */ > +static void macronix_nand_post_init(struct nand_chip *chip) > +{ > + unsigned int i, blockprotected = 0; > + struct mtd_info *mtd = nand_to_mtd(chip); > + > + for (i = 0; i < ARRAY_SIZE(broken_get_timings); i++) { > + if (!strcmp(broken_get_timings[i], chip->parameters.model)) { > + blockprotected = 1; > + break; > + } > + } > + > + if (blockprotected && chip->parameters.supports_set_get_features) { > + bitmap_set(chip->parameters.set_feature_list, > + ONFI_FEATURE_ADDR_MXIC_PROTECTION, 1); > + bitmap_set(chip->parameters.get_feature_list, > + ONFI_FEATURE_ADDR_MXIC_PROTECTION, 1); > + > + mtd->_lock = mxic_nand_lock; > + mtd->_unlock = mxic_nand_unlock; > + } > +} > + > const struct nand_manufacturer_ops macronix_nand_manuf_ops = { > .init = macronix_nand_init, > + .post_init = macronix_nand_post_init, > }; Thanks, Miquèl
Hi Mason, Miquel Raynal <miquel.raynal@bootlin.com> wrote on Mon, 7 Oct 2019 10:45:11 +0200: > Hi Mason, > > Mason Yang <masonccyang@mxic.com.tw> wrote on Wed, 18 Sep 2019 15:56:25 > +0800: > > > Macronix AC series support using SET/GET_FEATURES to change > > Block Protection and Unprotection. > > > > MTD default _lock/_unlock function replacement by manufacturer > > postponed initialization. > > Why would we do that? > > Anyway your solution looks overkill, if we ever decide to > implement these hooks for raw nand, it is better just to not > overwrite them in nand_scan_tail() if they have been filled > previously (ie. by the manufacturer code). Actually you should add two NAND hooks that do the interface with the MTD hooks. In the NAND hooks, check that the request is to lock all the device, otherwise return -ENOTSUPP. Then fill-in these two hooks from the manufacturer code, without the postponed init. > > > Signed-off-by: Mason Yang <masonccyang@mxic.com.tw> > > --- > > drivers/mtd/nand/raw/nand_macronix.c | 80 +++++++++++++++++++++++++++++++++--- > > 1 file changed, 75 insertions(+), 5 deletions(-) > > > > diff --git a/drivers/mtd/nand/raw/nand_macronix.c b/drivers/mtd/nand/raw/nand_macronix.c > > index 58511ae..991c636 100644 > > --- a/drivers/mtd/nand/raw/nand_macronix.c > > +++ b/drivers/mtd/nand/raw/nand_macronix.c > > @@ -11,6 +11,10 @@ > > #define MACRONIX_READ_RETRY_BIT BIT(0) > > #define MACRONIX_NUM_READ_RETRY_MODES 6 > > > > +#define ONFI_FEATURE_ADDR_MXIC_PROTECTION 0xA0 > > +#define MXIC_BLOCK_PROTECTION_ALL_LOCK 0x38 > > +#define MXIC_BLOCK_PROTECTION_ALL_UNLOCK 0x0 > > + > > struct nand_onfi_vendor_macronix { > > u8 reserved; > > u8 reliability_func; > > @@ -57,10 +61,7 @@ static void macronix_nand_onfi_init(struct nand_chip *chip) > > * the timings unlike what is declared in the parameter page. Unflag > > * this feature to avoid unnecessary downturns. > > */ > > -static void macronix_nand_fix_broken_get_timings(struct nand_chip *chip) > > -{ > > - unsigned int i; > > - static const char * const broken_get_timings[] = { > > +static const char * const broken_get_timings[] = { > > "MX30LF1G18AC", > > "MX30LF1G28AC", > > "MX30LF2G18AC", > > @@ -75,7 +76,11 @@ static void macronix_nand_fix_broken_get_timings(struct nand_chip *chip) > > "MX30UF4G18AC", > > "MX30UF4G16AC", > > "MX30UF4G28AC", > > - }; > > +}; > > + > > +static void macronix_nand_fix_broken_get_timings(struct nand_chip *chip) > > +{ > > + unsigned int i; > > > > if (!chip->parameters.supports_set_get_features) > > return; > > @@ -105,6 +110,71 @@ static int macronix_nand_init(struct nand_chip *chip) > > return 0; > > } > > > > +static int mxic_nand_lock(struct mtd_info *mtd, loff_t ofs, uint64_t len) > > +{ > > + struct nand_chip *chip = mtd_to_nand(mtd); > > + u8 feature[ONFI_SUBFEATURE_PARAM_LEN]; > > + int ret; > > + > > + feature[0] = MXIC_BLOCK_PROTECTION_ALL_LOCK; > > + nand_select_target(chip, 0); > > + ret = nand_set_features(chip, ONFI_FEATURE_ADDR_MXIC_PROTECTION, > > + feature); > > + nand_deselect_target(chip); > > + if (ret) > > + pr_err("%s all blocks failed\n", __func__); > > + > > + return ret; > > +} > > + > > +static int mxic_nand_unlock(struct mtd_info *mtd, loff_t ofs, uint64_t len) > > +{ > > + struct nand_chip *chip = mtd_to_nand(mtd); > > + u8 feature[ONFI_SUBFEATURE_PARAM_LEN]; > > + int ret; > > + > > + feature[0] = MXIC_BLOCK_PROTECTION_ALL_UNLOCK; > > + nand_select_target(chip, 0); > > + ret = nand_set_features(chip, ONFI_FEATURE_ADDR_MXIC_PROTECTION, > > + feature); > > + nand_deselect_target(chip); > > + if (ret) > > + pr_err("%s all blocks failed\n", __func__); > > + > > + return ret; > > +} > > + > > +/* > > + * Macronix AC series support using SET/GET_FEATURES to change > > + * Block Protection and Unprotection. > > + * > > + * MTD call-back function replacement by manufacturer postponed > > + * initialization. > > + */ > > +static void macronix_nand_post_init(struct nand_chip *chip) > > +{ > > + unsigned int i, blockprotected = 0; > > + struct mtd_info *mtd = nand_to_mtd(chip); > > + > > + for (i = 0; i < ARRAY_SIZE(broken_get_timings); i++) { > > + if (!strcmp(broken_get_timings[i], chip->parameters.model)) { > > + blockprotected = 1; > > + break; > > + } > > + } > > + > > + if (blockprotected && chip->parameters.supports_set_get_features) { > > + bitmap_set(chip->parameters.set_feature_list, > > + ONFI_FEATURE_ADDR_MXIC_PROTECTION, 1); > > + bitmap_set(chip->parameters.get_feature_list, > > + ONFI_FEATURE_ADDR_MXIC_PROTECTION, 1); > > + > > + mtd->_lock = mxic_nand_lock; > > + mtd->_unlock = mxic_nand_unlock; > > + } > > +} > > + > > const struct nand_manufacturer_ops macronix_nand_manuf_ops = { > > .init = macronix_nand_init, > > + .post_init = macronix_nand_post_init, > > }; > > > Thanks, > Miquèl Thanks, Miquèl
Hi Miquel, > > > > > Macronix AC series support using SET/GET_FEATURES to change > > > Block Protection and Unprotection. > > > > > > MTD default _lock/_unlock function replacement by manufacturer > > > postponed initialization. > > > > Why would we do that? > > > > Anyway your solution looks overkill, if we ever decide to > > implement these hooks for raw nand, it is better just to not > > overwrite them in nand_scan_tail() if they have been filled > > previously (ie. by the manufacturer code). > > Actually you should add two NAND hooks that do the interface with the > MTD hooks. In the NAND hooks, check that the request is to lock all the > device, otherwise return -ENOTSUPP. sorry, can't get your point. Because the NAND entire chip will be protected if PT(protection) pin is active high at power-on. > > Then fill-in these two hooks from the manufacturer code, without the > postponed init. > But in the final of nand_scan_tail(), mtd->_lock/_unlock will be filled by NULL, right ? thanks & best regards, 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. =====================================================================
Hi Mason, masonccyang@mxic.com.tw wrote on Tue, 8 Oct 2019 10:33:11 +0800: > Hi Miquel, > > > > > > > > Macronix AC series support using SET/GET_FEATURES to change > > > > Block Protection and Unprotection. > > > > > > > > MTD default _lock/_unlock function replacement by manufacturer > > > > postponed initialization. > > > > > > Why would we do that? > > > > > > Anyway your solution looks overkill, if we ever decide to > > > implement these hooks for raw nand, it is better just to not > > > overwrite them in nand_scan_tail() if they have been filled > > > previously (ie. by the manufacturer code). > > > > Actually you should add two NAND hooks that do the interface with the > > MTD hooks. In the NAND hooks, check that the request is to lock all the > > device, otherwise return -ENOTSUPP. > > sorry, can't get your point. > > Because the NAND entire chip will be protected if PT(protection) pin > is active high at power-on. In your implementation of the locking, you should check that the locking request is over the entire device, unless you can lock a smaller portion of course. > > > > > Then fill-in these two hooks from the manufacturer code, without the > > postponed init. > > > > But in the final of nand_scan_tail(), mtd->_lock/_unlock will be > filled by NULL, right ? The NAND core should set mtd->_lock/_unlock() to NAND specific hooks so that the MTD layer is abstracted and and drivers do not see it. Then, in the NAND helper, either there is no specific hook defined by a manufacturer driver and you return -ENOTSUPP, or you execute the defined hook. Thanks, Miquèl
Hi Miquel, > > > > > > > > > Macronix AC series support using SET/GET_FEATURES to change > > > > > Block Protection and Unprotection. > > > > > > > > > > MTD default _lock/_unlock function replacement by manufacturer > > > > > postponed initialization. > > > > > > > > Why would we do that? > > > > > > > > Anyway your solution looks overkill, if we ever decide to > > > > implement these hooks for raw nand, it is better just to not > > > > overwrite them in nand_scan_tail() if they have been filled > > > > previously (ie. by the manufacturer code). > > > > > > Actually you should add two NAND hooks that do the interface with the > > > MTD hooks. In the NAND hooks, check that the request is to lock all the > > > device, otherwise return -ENOTSUPP. > > > > sorry, can't get your point. > > > > Because the NAND entire chip will be protected if PT(protection) pin > > is active high at power-on. > > In your implementation of the locking, you should check that the > locking request is over the entire device, unless you can lock a > smaller portion of course. yes, I can lock a smaller portion. And at the power-on of device with PT pin at high voltage, all blocks are locked. They have to be unlocked by set feature command. > > > > > > > > > Then fill-in these two hooks from the manufacturer code, without the > > > postponed init. > > > > > > > But in the final of nand_scan_tail(), mtd->_lock/_unlock will be > > filled by NULL, right ? > > The NAND core should set mtd->_lock/_unlock() to NAND specific hooks so > that the MTD layer is abstracted and and drivers do not see it. Then, > in the NAND helper, either there is no specific hook defined by a > manufacturer driver and you return -ENOTSUPP, or you execute the > defined hook. okay, patch specific manufacturer _lock/_unlock driver in nand_manufacturer_init(); and in the final of nand_scan_tail() if (!mtd->_lock) mtd->_lock = NULL; if (!mtd->_unlock) mtd->_unlock = NULL; thanks for your time and 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. =====================================================================
Hi Miquel, > > > > Then fill-in these two hooks from the manufacturer code, without the > > > > postponed init. > > > > > > > > > > But in the final of nand_scan_tail(), mtd->_lock/_unlock will be > > > filled by NULL, right ? > > > > The NAND core should set mtd->_lock/_unlock() to NAND specific hooks so > > that the MTD layer is abstracted and and drivers do not see it. Then, > > in the NAND helper, either there is no specific hook defined by a > > manufacturer driver and you return -ENOTSUPP, or you execute the > > defined hook. > > okay, patch specific manufacturer _lock/_unlock driver > in nand_manufacturer_init(); > > and in the final of nand_scan_tail() > if (!mtd->_lock) > mtd->_lock = NULL; > if (!mtd->_unlock) > mtd->_unlock = NULL; I'm still considering of post_init() in nand_scan_tail() for MTD layer default call-back function replacement because there would be more call-back functions need it. i.e., MTD->_lock/_unlokc MTD->_suspend/_resume NTD->_point/_unpoint ... actually, my patch series are including MTD->_locl/_unlock and MTD->_suspend/_resume. how do you think ? 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. =====================================================================
On Mon, 21 Oct 2019 15:23:57 +0800 masonccyang@mxic.com.tw wrote: > Hi Miquel, > > > > > > > Then fill-in these two hooks from the manufacturer code, without > the > > > > > postponed init. > > > > > > > > > > > > > But in the final of nand_scan_tail(), mtd->_lock/_unlock will be > > > > filled by NULL, right ? > > > > > > The NAND core should set mtd->_lock/_unlock() to NAND specific hooks > so > > > that the MTD layer is abstracted and and drivers do not see it. Then, > > > in the NAND helper, either there is no specific hook defined by a > > > manufacturer driver and you return -ENOTSUPP, or you execute the > > > defined hook. > > > > okay, patch specific manufacturer _lock/_unlock driver > > in nand_manufacturer_init(); > > > > and in the final of nand_scan_tail() > > if (!mtd->_lock) > > mtd->_lock = NULL; > > if (!mtd->_unlock) > > mtd->_unlock = NULL; > > > I'm still considering of post_init() in nand_scan_tail() for > MTD layer default call-back function replacement because > there would be more call-back functions need it. > i.e., > MTD->_lock/_unlokc > MTD->_suspend/_resume Again, that's something that needs to be abstracted so that both the NAND manufacturer driver and the NAND controller driver can take appropriate actions on suspend/resume operations. > NTD->_point/_unpoint ->_point/_unpoint() are irrelevant for a NAND chip. > ... > > > actually, my patch series are including MTD->_locl/_unlock and > MTD->_suspend/_resume. how do you think ? Miquel was suggesting to add nand_chip->{lock,unlock,is_locked}() methods that would be implemented by the NAND manufacturer drivers, and have generic wrappers implemented in nand_base.c: static int nand_lock(struct mtd_info *mtd, loff_t ofs, uint64_t len) { struct nand_chip *chip = mtd_to_nand(mtd); if (!chip->lock) return -ENOTSUPP; return chip->lock(chip, ofs, len); } ... If you do that, you won't need this post_init() hook.
Hi Boris, > > > > > > Then fill-in these two hooks from the manufacturer code, without > > the > > > > > > postponed init. > > > > > > > > > > > > > > > > But in the final of nand_scan_tail(), mtd->_lock/_unlock will be > > > > > filled by NULL, right ? > > > > > > > > The NAND core should set mtd->_lock/_unlock() to NAND specific hooks > > so > > > > that the MTD layer is abstracted and and drivers do not see it. Then, > > > > in the NAND helper, either there is no specific hook defined by a > > > > manufacturer driver and you return -ENOTSUPP, or you execute the > > > > defined hook. > > > > > > okay, patch specific manufacturer _lock/_unlock driver > > > in nand_manufacturer_init(); > > > > > > and in the final of nand_scan_tail() > > > if (!mtd->_lock) > > > mtd->_lock = NULL; > > > if (!mtd->_unlock) > > > mtd->_unlock = NULL; > > > > > > I'm still considering of post_init() in nand_scan_tail() for > > MTD layer default call-back function replacement because > > there would be more call-back functions need it. > > i.e., > > MTD->_lock/_unlokc > > MTD->_suspend/_resume > > Again, that's something that needs to be abstracted so that both the > NAND manufacturer driver and the NAND controller driver can take > appropriate actions on suspend/resume operations. > > > NTD->_point/_unpoint > > ->_point/_unpoint() are irrelevant for a NAND chip. > > > ... > > > > > > actually, my patch series are including MTD->_locl/_unlock and > > MTD->_suspend/_resume. how do you think ? > > Miquel was suggesting to add nand_chip->{lock,unlock,is_locked}() > methods that would be implemented by the NAND manufacturer drivers, and > have generic wrappers implemented in nand_base.c: > > static int nand_lock(struct mtd_info *mtd, loff_t ofs, uint64_t len) > { > struct nand_chip *chip = mtd_to_nand(mtd); > > if (!chip->lock) > return -ENOTSUPP; > > return chip->lock(chip, ofs, len); > } > > ... > > If you do that, you won't need this post_init() hook. got it, but ... user space program flash_lock/flash_unlock are calling mtd_lock() & mtd_unlock(). i.e., int mtd_lock(struct mtd_info *mtd, loff_t ofs, uint64_t len) { if (!mtd->_lock) return -EOPNOTSUPP; if (ofs < 0 || ofs >= mtd->size || len > mtd->size - ofs) return -EINVAL; if (!len) return 0; return mtd->_lock(mtd, ofs, len); } 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. =====================================================================
On Mon, 21 Oct 2019 16:40:57 +0800 masonccyang@mxic.com.tw wrote: > Hi Boris, > > > > > > > > > Then fill-in these two hooks from the manufacturer code, > without > > > the > > > > > > > postponed init. > > > > > > > > > > > > > > > > > > > But in the final of nand_scan_tail(), mtd->_lock/_unlock will be > > > > > > filled by NULL, right ? > > > > > > > > > > The NAND core should set mtd->_lock/_unlock() to NAND specific > hooks > > > so > > > > > that the MTD layer is abstracted and and drivers do not see it. > Then, > > > > > in the NAND helper, either there is no specific hook defined by a > > > > > manufacturer driver and you return -ENOTSUPP, or you execute the > > > > > defined hook. > > > > > > > > okay, patch specific manufacturer _lock/_unlock driver > > > > in nand_manufacturer_init(); > > > > > > > > and in the final of nand_scan_tail() > > > > if (!mtd->_lock) > > > > mtd->_lock = NULL; > > > > if (!mtd->_unlock) > > > > mtd->_unlock = NULL; > > > > > > > > > I'm still considering of post_init() in nand_scan_tail() for > > > MTD layer default call-back function replacement because > > > there would be more call-back functions need it. > > > i.e., > > > MTD->_lock/_unlokc > > > MTD->_suspend/_resume > > > > Again, that's something that needs to be abstracted so that both the > > NAND manufacturer driver and the NAND controller driver can take > > appropriate actions on suspend/resume operations. > > > > > NTD->_point/_unpoint > > > > ->_point/_unpoint() are irrelevant for a NAND chip. > > > > > ... > > > > > > > > > actually, my patch series are including MTD->_locl/_unlock and > > > MTD->_suspend/_resume. how do you think ? > > > > Miquel was suggesting to add nand_chip->{lock,unlock,is_locked}() > > methods that would be implemented by the NAND manufacturer drivers, and > > have generic wrappers implemented in nand_base.c: > > > > static int nand_lock(struct mtd_info *mtd, loff_t ofs, uint64_t len) > > { > > struct nand_chip *chip = mtd_to_nand(mtd); > > > > if (!chip->lock) > > return -ENOTSUPP; > > > > return chip->lock(chip, ofs, len); > > } > > > > ... > > > > If you do that, you won't need this post_init() hook. > > got it, but ... > user space program flash_lock/flash_unlock are calling > mtd_lock() & mtd_unlock(). > i.e., > int mtd_lock(struct mtd_info *mtd, loff_t ofs, uint64_t len) > { > if (!mtd->_lock) > return -EOPNOTSUPP; > if (ofs < 0 || ofs >= mtd->size || len > mtd->size - ofs) > return -EINVAL; > if (!len) > return 0; > return mtd->_lock(mtd, ofs, len); > } > Assign mtd lock/unlock/is_locked hooks to the generic wrappers in nand_scan_tail(): mtd->_lock = nand_lock; mtd->_unlock = nand_unlock; mtd->_is_locked = nand_is_locked; Seriously, we've almost implemented the thing for you with all the details we've given. At some point you have to look more closely at how things are done/designed in the NAND framework if you want to contribute core changes. I'm fine giving hints but we're far beyond that point here.
Hi Boris, > > Assign mtd lock/unlock/is_locked hooks to the generic wrappers in > nand_scan_tail(): > > mtd->_lock = nand_lock; > mtd->_unlock = nand_unlock; > mtd->_is_locked = nand_is_locked; > > Seriously, we've almost implemented the thing for you with all the > details we've given. At some point you have to look more closely at how > things are done/designed in the NAND framework if you want to > contribute core changes. I'm fine giving hints but we're far beyond > that point here. got your point & idea. thanks a lot for your time & opinions. 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 58511ae..991c636 100644 --- a/drivers/mtd/nand/raw/nand_macronix.c +++ b/drivers/mtd/nand/raw/nand_macronix.c @@ -11,6 +11,10 @@ #define MACRONIX_READ_RETRY_BIT BIT(0) #define MACRONIX_NUM_READ_RETRY_MODES 6 +#define ONFI_FEATURE_ADDR_MXIC_PROTECTION 0xA0 +#define MXIC_BLOCK_PROTECTION_ALL_LOCK 0x38 +#define MXIC_BLOCK_PROTECTION_ALL_UNLOCK 0x0 + struct nand_onfi_vendor_macronix { u8 reserved; u8 reliability_func; @@ -57,10 +61,7 @@ static void macronix_nand_onfi_init(struct nand_chip *chip) * the timings unlike what is declared in the parameter page. Unflag * this feature to avoid unnecessary downturns. */ -static void macronix_nand_fix_broken_get_timings(struct nand_chip *chip) -{ - unsigned int i; - static const char * const broken_get_timings[] = { +static const char * const broken_get_timings[] = { "MX30LF1G18AC", "MX30LF1G28AC", "MX30LF2G18AC", @@ -75,7 +76,11 @@ static void macronix_nand_fix_broken_get_timings(struct nand_chip *chip) "MX30UF4G18AC", "MX30UF4G16AC", "MX30UF4G28AC", - }; +}; + +static void macronix_nand_fix_broken_get_timings(struct nand_chip *chip) +{ + unsigned int i; if (!chip->parameters.supports_set_get_features) return; @@ -105,6 +110,71 @@ static int macronix_nand_init(struct nand_chip *chip) return 0; } +static int mxic_nand_lock(struct mtd_info *mtd, loff_t ofs, uint64_t len) +{ + struct nand_chip *chip = mtd_to_nand(mtd); + u8 feature[ONFI_SUBFEATURE_PARAM_LEN]; + int ret; + + feature[0] = MXIC_BLOCK_PROTECTION_ALL_LOCK; + nand_select_target(chip, 0); + ret = nand_set_features(chip, ONFI_FEATURE_ADDR_MXIC_PROTECTION, + feature); + nand_deselect_target(chip); + if (ret) + pr_err("%s all blocks failed\n", __func__); + + return ret; +} + +static int mxic_nand_unlock(struct mtd_info *mtd, loff_t ofs, uint64_t len) +{ + struct nand_chip *chip = mtd_to_nand(mtd); + u8 feature[ONFI_SUBFEATURE_PARAM_LEN]; + int ret; + + feature[0] = MXIC_BLOCK_PROTECTION_ALL_UNLOCK; + nand_select_target(chip, 0); + ret = nand_set_features(chip, ONFI_FEATURE_ADDR_MXIC_PROTECTION, + feature); + nand_deselect_target(chip); + if (ret) + pr_err("%s all blocks failed\n", __func__); + + return ret; +} + +/* + * Macronix AC series support using SET/GET_FEATURES to change + * Block Protection and Unprotection. + * + * MTD call-back function replacement by manufacturer postponed + * initialization. + */ +static void macronix_nand_post_init(struct nand_chip *chip) +{ + unsigned int i, blockprotected = 0; + struct mtd_info *mtd = nand_to_mtd(chip); + + for (i = 0; i < ARRAY_SIZE(broken_get_timings); i++) { + if (!strcmp(broken_get_timings[i], chip->parameters.model)) { + blockprotected = 1; + break; + } + } + + if (blockprotected && chip->parameters.supports_set_get_features) { + bitmap_set(chip->parameters.set_feature_list, + ONFI_FEATURE_ADDR_MXIC_PROTECTION, 1); + bitmap_set(chip->parameters.get_feature_list, + ONFI_FEATURE_ADDR_MXIC_PROTECTION, 1); + + mtd->_lock = mxic_nand_lock; + mtd->_unlock = mxic_nand_unlock; + } +} + const struct nand_manufacturer_ops macronix_nand_manuf_ops = { .init = macronix_nand_init, + .post_init = macronix_nand_post_init, };
Macronix AC series support using SET/GET_FEATURES to change Block Protection and Unprotection. MTD default _lock/_unlock function replacement by manufacturer postponed initialization. Signed-off-by: Mason Yang <masonccyang@mxic.com.tw> --- drivers/mtd/nand/raw/nand_macronix.c | 80 +++++++++++++++++++++++++++++++++--- 1 file changed, 75 insertions(+), 5 deletions(-)