| Message ID | 1402579245-13377-5-git-send-email-LW@KARO-electronics.de |
|---|---|
| State | Changes Requested |
| Headers | show |
(BTW, that's a mighty CC list you have! I'm not sure all CC'd parties are interested in this series; e.g., Russel and the ARM list seem unrelated) Hi Lothar, Sorry for the delay on this. I get busy enough that I can't/don't reply to everything quickly... On Thu, Jun 12, 2014 at 03:20:44PM +0200, Lothar Waßmann wrote: > add a boolean property 'nand-no-oob-bbm' and helper function to be > able to set the NAND_BBT_NO_OOB_BBM flag in DT capable NAND drivers > and use it for i.MX and MXS nand drivers. If I'm understanding your previous conversations with Huang correctly, you *must* use NAND_BBT_NO_OOB_BBM if you're going to use the fsl,no-blockmark-swap option. Correct? If so, then you might not need a separate 'nand-no-oob-bbm' binding; your driver should imply from 'fsl,no-blockmark-swap' that it must also enable NAND_BBT_NO_OOB_BBM. Also, as I noted in [1], I don't really like exposing a ton of individual boolean DT properties like this. (At least this property is orthogonal to the bad block table; I was a little off-base in [1].) Brian [1] http://lists.infradead.org/pipermail/linux-mtd/2014-July/054764.html > Signed-off-by: Lothar Waßmann <LW@KARO-electronics.de> > --- > Documentation/devicetree/bindings/mtd/nand.txt | 1 + > drivers/mtd/nand/gpmi-nand/gpmi-nand.c | 3 +++ > drivers/mtd/nand/mxc_nand.c | 2 ++ > drivers/of/of_mtd.c | 12 ++++++++++++ > include/linux/of_mtd.h | 6 ++++++ > 5 files changed, 24 insertions(+) > > diff --git a/Documentation/devicetree/bindings/mtd/nand.txt b/Documentation/devicetree/bindings/mtd/nand.txt > index b53f92e..e46bfbe 100644 > --- a/Documentation/devicetree/bindings/mtd/nand.txt > +++ b/Documentation/devicetree/bindings/mtd/nand.txt > @@ -5,6 +5,7 @@ > "soft_bch". > - nand-bus-width : 8 or 16 bus width if not present 8 > - nand-on-flash-bbt: boolean to enable on flash bbt option if not present false > +- nand-no-oob-bbm: boolean to disable writing bad block markers to flash > > - nand-ecc-strength: integer representing the number of bits to correct > per ECC step. > diff --git a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c > index 959cb9b..37537b4 100644 > --- a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c > +++ b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c > @@ -1724,6 +1724,9 @@ static int gpmi_nand_init(struct gpmi_nand_data *this) > if (of_get_nand_on_flash_bbt(this->dev->of_node)) { > chip->bbt_options |= NAND_BBT_USE_FLASH | NAND_BBT_NO_OOB; > > + if (of_get_nand_no_oob_bbm(this->dev->of_node)) > + chip->bbt_options |= NAND_BBT_NO_OOB_BBM; > + > if (of_property_read_bool(this->dev->of_node, > "fsl,no-blockmark-swap")) > this->swap_block_mark = false; > diff --git a/drivers/mtd/nand/mxc_nand.c b/drivers/mtd/nand/mxc_nand.c > index dba262b..bb54a2a 100644 > --- a/drivers/mtd/nand/mxc_nand.c > +++ b/drivers/mtd/nand/mxc_nand.c > @@ -1496,6 +1496,8 @@ static int mxcnd_probe(struct platform_device *pdev) > this->bbt_md = &bbt_mirror_descr; > /* update flash based bbt */ > this->bbt_options |= NAND_BBT_USE_FLASH; > + if (of_get_nand_no_oob_bbm(pdev->dev.of_node)) > + this->bbt_options |= NAND_BBT_NO_OOB_BBM; > } > > init_completion(&host->op_completion); > diff --git a/drivers/of/of_mtd.c b/drivers/of/of_mtd.c > index b7361ed..d947acc 100644 > --- a/drivers/of/of_mtd.c > +++ b/drivers/of/of_mtd.c > @@ -117,3 +117,15 @@ bool of_get_nand_on_flash_bbt(struct device_node *np) > return of_property_read_bool(np, "nand-on-flash-bbt"); > } > EXPORT_SYMBOL_GPL(of_get_nand_on_flash_bbt); > + > +/** > + * of_get_nand_no_oob_bbm - Get nand no oob bbm for given device_node > + * @np: Pointer to the given device_node > + * > + * return true if present, false otherwise > + */ > +bool of_get_nand_no_oob_bbm(struct device_node *np) > +{ > + return of_property_read_bool(np, "nand-no-oob-bbm"); > +} > +EXPORT_SYMBOL_GPL(of_get_nand_no_oob_bbm); > diff --git a/include/linux/of_mtd.h b/include/linux/of_mtd.h > index e266caa..6ece1a9 100644 > --- a/include/linux/of_mtd.h > +++ b/include/linux/of_mtd.h > @@ -17,6 +17,7 @@ int of_get_nand_ecc_step_size(struct device_node *np); > int of_get_nand_ecc_strength(struct device_node *np); > int of_get_nand_bus_width(struct device_node *np); > bool of_get_nand_on_flash_bbt(struct device_node *np); > +bool of_get_nand_no_oob_bbm(struct device_node *np); > > #else /* CONFIG_OF_MTD */ > > @@ -45,6 +46,11 @@ static inline bool of_get_nand_on_flash_bbt(struct device_node *np) > return false; > } > > +static inline bool of_get_nand_no_oob_bbm(struct device_node *np) > +{ > + return false; > +} > + > #endif /* CONFIG_OF_MTD */ > > #endif /* __LINUX_OF_MTD_H */
Hi, Brian Norris wrote: > (BTW, that's a mighty CC list you have! I'm not sure all CC'd parties > are interested in this series; e.g., Russel and the ARM list seem > unrelated) > > Hi Lothar, > > Sorry for the delay on this. I get busy enough that I can't/don't reply > to everything quickly... > > On Thu, Jun 12, 2014 at 03:20:44PM +0200, Lothar Waßmann wrote: > > add a boolean property 'nand-no-oob-bbm' and helper function to be > > able to set the NAND_BBT_NO_OOB_BBM flag in DT capable NAND drivers > > and use it for i.MX and MXS nand drivers. > > If I'm understanding your previous conversations with Huang correctly, > you *must* use NAND_BBT_NO_OOB_BBM if you're going to use the > fsl,no-blockmark-swap option. Correct? If so, then you might not need > a separate 'nand-no-oob-bbm' binding; your driver should imply from > 'fsl,no-blockmark-swap' that it must also enable NAND_BBT_NO_OOB_BBM. > no-blockmark-swap implies NO_OOB_BBM but NO_OOB_BBM may also be used independent from no-blockmark-swap. IMO writing a bad block marker to flash (which is prevented by the NAND_BBT_NO_OOB_BBM flag) is a misinterpretation of the purpose of the BB mark in the first place. The manufacturer guarantees that blocks which are initially bad will have at least one zero bit in the position of the BB mark. That's all to it. There is no guarantee, that you will even be able to write any deterministic data to a block that has turned bad due to wearout or other flash defects. It is rather bogus to rely on data written to a known bad block to reflect the state of the block. > Also, as I noted in [1], I don't really like exposing a ton of > individual boolean DT properties like this. (At least this property is > orthogonal to the bad block table; I was a little off-base in [1].) > How else should this information be conveyed to the flash drivers? Lothar Waßmann
On Thu, Jul 24, 2014 at 08:49:15AM +0200, Lothar Waßmann wrote: > Brian Norris wrote: > > On Thu, Jun 12, 2014 at 03:20:44PM +0200, Lothar Waßmann wrote: > > > add a boolean property 'nand-no-oob-bbm' and helper function to be > > > able to set the NAND_BBT_NO_OOB_BBM flag in DT capable NAND drivers > > > and use it for i.MX and MXS nand drivers. > > > > If I'm understanding your previous conversations with Huang correctly, > > you *must* use NAND_BBT_NO_OOB_BBM if you're going to use the > > fsl,no-blockmark-swap option. Correct? If so, then you might not need > > a separate 'nand-no-oob-bbm' binding; your driver should imply from > > 'fsl,no-blockmark-swap' that it must also enable NAND_BBT_NO_OOB_BBM. > > > no-blockmark-swap implies NO_OOB_BBM but NO_OOB_BBM may also be used > independent from no-blockmark-swap. Why would you want NO_OOB_BBM without no-blockmark-swap? If the block is bad, why do you care what's written to it? (For that matter, why is it ever important to use NO_OOB_BBM? At worst, the extra BB marks are useless / written to the wrong place.) > IMO writing a bad block marker to flash (which is prevented by > the NAND_BBT_NO_OOB_BBM flag) is a misinterpretation of the purpose of > the BB mark in the first place. The manufacturer guarantees that blocks > which are initially bad will have at least one zero bit in the position > of the BB mark. That's all to it. Yes, it is a misinterpretation, and it's really irrelevant in many cases whether or not the BB mark is written to each block's OOB. But it does still provide some resilience in case the on-flash table ever gets completely corrupted -- nand_bbt will rescan the flash for BB marks on the next boot (and this will be totally broken--with or without NO_OOB_BBM--for hardware like yours). Or to put it another way, it supports some legacy scenarios without (AFAICT) any real negative effects. > There is no guarantee, that you will even be able to write any > deterministic data to a block that has turned bad due to wearout or > other flash defects. Certainly. But that's not an argument against attempting. > It is rather bogus to rely on data written to a > known bad block to reflect the state of the block. We don't "rely" on this. If the BBT (and its mirrors) never completely fails, these marks are never used. > > Also, as I noted in [1], I don't really like exposing a ton of > > individual boolean DT properties like this. (At least this property is > > orthogonal to the bad block table; I was a little off-base in [1].) > > > How else should this information be conveyed to the flash drivers? I'm not convinced the NO_OOB_BBM DT property is actually necessary at all. I was more concerned about bad block *table* properties, where I see that at least some users (e.g. ST Micro's BCH NAND driver) expect a different BBT format than the default, and we might begin to see extra boolean flags for random bits of differentiation. This is apparently still just a theoretical concern, though. Brian
diff --git a/Documentation/devicetree/bindings/mtd/nand.txt b/Documentation/devicetree/bindings/mtd/nand.txt index b53f92e..e46bfbe 100644 --- a/Documentation/devicetree/bindings/mtd/nand.txt +++ b/Documentation/devicetree/bindings/mtd/nand.txt @@ -5,6 +5,7 @@ "soft_bch". - nand-bus-width : 8 or 16 bus width if not present 8 - nand-on-flash-bbt: boolean to enable on flash bbt option if not present false +- nand-no-oob-bbm: boolean to disable writing bad block markers to flash - nand-ecc-strength: integer representing the number of bits to correct per ECC step. diff --git a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c index 959cb9b..37537b4 100644 --- a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c +++ b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c @@ -1724,6 +1724,9 @@ static int gpmi_nand_init(struct gpmi_nand_data *this) if (of_get_nand_on_flash_bbt(this->dev->of_node)) { chip->bbt_options |= NAND_BBT_USE_FLASH | NAND_BBT_NO_OOB; + if (of_get_nand_no_oob_bbm(this->dev->of_node)) + chip->bbt_options |= NAND_BBT_NO_OOB_BBM; + if (of_property_read_bool(this->dev->of_node, "fsl,no-blockmark-swap")) this->swap_block_mark = false; diff --git a/drivers/mtd/nand/mxc_nand.c b/drivers/mtd/nand/mxc_nand.c index dba262b..bb54a2a 100644 --- a/drivers/mtd/nand/mxc_nand.c +++ b/drivers/mtd/nand/mxc_nand.c @@ -1496,6 +1496,8 @@ static int mxcnd_probe(struct platform_device *pdev) this->bbt_md = &bbt_mirror_descr; /* update flash based bbt */ this->bbt_options |= NAND_BBT_USE_FLASH; + if (of_get_nand_no_oob_bbm(pdev->dev.of_node)) + this->bbt_options |= NAND_BBT_NO_OOB_BBM; } init_completion(&host->op_completion); diff --git a/drivers/of/of_mtd.c b/drivers/of/of_mtd.c index b7361ed..d947acc 100644 --- a/drivers/of/of_mtd.c +++ b/drivers/of/of_mtd.c @@ -117,3 +117,15 @@ bool of_get_nand_on_flash_bbt(struct device_node *np) return of_property_read_bool(np, "nand-on-flash-bbt"); } EXPORT_SYMBOL_GPL(of_get_nand_on_flash_bbt); + +/** + * of_get_nand_no_oob_bbm - Get nand no oob bbm for given device_node + * @np: Pointer to the given device_node + * + * return true if present, false otherwise + */ +bool of_get_nand_no_oob_bbm(struct device_node *np) +{ + return of_property_read_bool(np, "nand-no-oob-bbm"); +} +EXPORT_SYMBOL_GPL(of_get_nand_no_oob_bbm); diff --git a/include/linux/of_mtd.h b/include/linux/of_mtd.h index e266caa..6ece1a9 100644 --- a/include/linux/of_mtd.h +++ b/include/linux/of_mtd.h @@ -17,6 +17,7 @@ int of_get_nand_ecc_step_size(struct device_node *np); int of_get_nand_ecc_strength(struct device_node *np); int of_get_nand_bus_width(struct device_node *np); bool of_get_nand_on_flash_bbt(struct device_node *np); +bool of_get_nand_no_oob_bbm(struct device_node *np); #else /* CONFIG_OF_MTD */ @@ -45,6 +46,11 @@ static inline bool of_get_nand_on_flash_bbt(struct device_node *np) return false; } +static inline bool of_get_nand_no_oob_bbm(struct device_node *np) +{ + return false; +} + #endif /* CONFIG_OF_MTD */ #endif /* __LINUX_OF_MTD_H */
add a boolean property 'nand-no-oob-bbm' and helper function to be able to set the NAND_BBT_NO_OOB_BBM flag in DT capable NAND drivers and use it for i.MX and MXS nand drivers. Signed-off-by: Lothar Waßmann <LW@KARO-electronics.de> --- Documentation/devicetree/bindings/mtd/nand.txt | 1 + drivers/mtd/nand/gpmi-nand/gpmi-nand.c | 3 +++ drivers/mtd/nand/mxc_nand.c | 2 ++ drivers/of/of_mtd.c | 12 ++++++++++++ include/linux/of_mtd.h | 6 ++++++ 5 files changed, 24 insertions(+)