* [PATCH 2/2 v3] mtd: fsl_upm: Support NAND ECC DTS properties [not found] <993430213.110699.1421109005544.JavaMail.zimbra@xes-inc.com> @ 2015-01-13 0:36 ` Aaron Sierra 0 siblings, 0 replies; 18+ messages in thread From: Aaron Sierra @ 2015-01-13 0:36 UTC (permalink / raw) To: Brian Norris, David Woodhouse, Ezequiel Garcia Cc: linux-mtd-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, devicetree-u79uwXL29TY76Z2rM5mHXA From: Jordan Friendshuh <jfriendshuh-AQeFf1F/bRxBDgjK7y7TUQ@public.gmane.org> Support the generic nand-ecc-mode, nand-ecc-strength, and nand-ecc-step-size device-tree properties with the Freescale UPM NAND driver. This patch preserves the default software ECC mode while adding the ability to use BCH ECC for larger NAND devices. Signed-off-by: Jordan Friendshuh <jfriendshuh-AQeFf1F/bRxBDgjK7y7TUQ@public.gmane.org> Signed-off-by: Aaron Sierra <asierra-AQeFf1F/bRxBDgjK7y7TUQ@public.gmane.org> --- v2: * Now using ECC mode and strength helpers from of_mtd.h * ECC mode and strength checking is more robust v3: * Require nand-ecc-step-size for soft_bch. * Simplify mode/strength/step parameter checking. .../devicetree/bindings/mtd/fsl-upm-nand.txt | 32 +++++++++++ drivers/mtd/nand/Kconfig | 1 + drivers/mtd/nand/fsl_upm.c | 66 ++++++++++++++++++++-- 3 files changed, 94 insertions(+), 5 deletions(-) diff --git a/Documentation/devicetree/bindings/mtd/fsl-upm-nand.txt b/Documentation/devicetree/bindings/mtd/fsl-upm-nand.txt index fce4894..3643ee1 100644 --- a/Documentation/devicetree/bindings/mtd/fsl-upm-nand.txt +++ b/Documentation/devicetree/bindings/mtd/fsl-upm-nand.txt @@ -18,6 +18,9 @@ Optional properties: - chip-delay : chip dependent delay for transferring data from array to read registers (tR). Required if property "gpios" is not used (R/B# pins not connected). +- nand-ecc-mode : as defined by nand.txt ("soft" and "soft_bch", only). +- nand-ecc-strength : as defined by nand.txt. +- nand-ecc-step-size : as defined by nand.txt. Each flash chip described may optionally contain additional sub-nodes describing partitions of the address space. See partition.txt for more @@ -65,3 +68,32 @@ upm@3,0 { }; }; }; + +/* + * Micron MT29F32G08AFABA (M62B) + * 32 Gb (4 GiB), 2 chipselect + */ +upm@2,0 { + #address-cells = <0>; + #size-cells = <0>; + compatible = "fsl,upm-nand"; + reg = <2 0x0 0x80000>; + fsl,upm-addr-line-cs-offsets = <0x0 0x10000>; + fsl,upm-addr-offset = <0x10>; + fsl,upm-cmd-offset = <0x08>; + fsl,upm-wait-flags = <0x1>; + chip-delay = <50>; + + nand@0 { + #address-cells = <1>; + #size-cells = <2>; + nand-ecc-mode = "soft_bch"; + nand-ecc-strength = <4>; + nand-ecc-step-size = <512>; + + partition@0 { + label = "NAND filesystem"; + reg = <0x0 0x1 0x00000000>; + }; + }; +}; diff --git a/drivers/mtd/nand/Kconfig b/drivers/mtd/nand/Kconfig index e5e3343..4c85daf 100644 --- a/drivers/mtd/nand/Kconfig +++ b/drivers/mtd/nand/Kconfig @@ -444,6 +444,7 @@ config MTD_NAND_FSL_UPM tristate "Support for NAND on Freescale UPM" depends on PPC_83xx || PPC_85xx select FSL_LBC + select MTD_NAND_ECC_BCH help Enables support for NAND Flash chips wired onto Freescale PowerPC processor localbus with User-Programmable Machine support. diff --git a/drivers/mtd/nand/fsl_upm.c b/drivers/mtd/nand/fsl_upm.c index 72755d7..053d8bf 100644 --- a/drivers/mtd/nand/fsl_upm.c +++ b/drivers/mtd/nand/fsl_upm.c @@ -18,6 +18,7 @@ #include <linux/mtd/nand_ecc.h> #include <linux/mtd/partitions.h> #include <linux/mtd/mtd.h> +#include <linux/of_mtd.h> #include <linux/of_address.h> #include <linux/of_platform.h> #include <linux/of_gpio.h> @@ -160,6 +161,11 @@ static int fun_chip_init(struct fsl_upm_nand *fun, int ret; struct device_node *flash_np; struct mtd_part_parser_data ppdata; + int mode, strength, step_size; + + flash_np = of_get_next_child(upm_np, NULL); + if (!flash_np) + return -ENODEV; fun->chip.IO_ADDR_R = fun->io_base; fun->chip.IO_ADDR_W = fun->io_base; @@ -168,7 +174,61 @@ static int fun_chip_init(struct fsl_upm_nand *fun, fun->chip.read_byte = fun_read_byte; fun->chip.read_buf = fun_read_buf; fun->chip.write_buf = fun_write_buf; - fun->chip.ecc.mode = NAND_ECC_SOFT; + + /* + * Support NAND_ECC_SOFT and NAND_ECC_SOFT_BCH, error otherwise. + */ + mode = of_get_nand_ecc_mode(flash_np); + strength = of_get_nand_ecc_strength(flash_np); + step_size = of_get_nand_ecc_step_size(flash_np); + if (mode < 0) { + dev_info(fun->dev, "ECC mode defaulting to 'soft'"); + mode = NAND_ECC_SOFT; + } else if (mode != NAND_ECC_SOFT && mode != NAND_ECC_SOFT_BCH) { + dev_err(fun->dev, "ECC mode in device tree is unsupported"); + ret = -EINVAL; + goto err; + } + + /* + * In NAND_ECC_SOFT_BCH, require strength >= 1 and step >= 1. + * In NAND_ECC_SOFT: + * a. ignore strength (1 implied) + * b. step < 0, step = 256, or step = 512. + */ + if (mode == NAND_ECC_SOFT_BCH) { + if (strength < 1) + dev_err(fun->dev, "invalid nand-ecc-strength for BCH"); + + if (step_size < 1) + dev_err(fun->dev, "invalid nand-ecc-step-size for BCH"); + + if (strength < 1 || step_size < 1) { + ret = -EINVAL; + goto err; + } + + fun->chip.ecc.mode = mode; + fun->chip.ecc.strength = strength; + fun->chip.ecc.size = step_size; + } else { + if (strength >= 0) + dev_warn(fun->dev, "soft ECC implies 1-bit strength"); + + if (step_size < 0) { + step_size = 256; + } else if (step_size != 256 && step_size != 512) { + dev_err(fun->dev, + "soft ECC needs 256 or 512 byte step"); + ret = -EINVAL; + goto err; + } + + fun->chip.ecc.mode = mode; + fun->chip.ecc.strength = 1; + fun->chip.ecc.size = step_size; + } + if (fun->mchip_count > 1) fun->chip.select_chip = fun_select_chip; @@ -178,10 +238,6 @@ static int fun_chip_init(struct fsl_upm_nand *fun, fun->mtd.priv = &fun->chip; fun->mtd.owner = THIS_MODULE; - flash_np = of_get_next_child(upm_np, NULL); - if (!flash_np) - return -ENODEV; - fun->mtd.name = kasprintf(GFP_KERNEL, "0x%llx.%s", (u64)io_res->start, flash_np->name); if (!fun->mtd.name) { -- 1.9.1 -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH 2/2 v3] mtd: fsl_upm: Support NAND ECC DTS properties @ 2015-01-13 0:36 ` Aaron Sierra 0 siblings, 0 replies; 18+ messages in thread From: Aaron Sierra @ 2015-01-13 0:36 UTC (permalink / raw) To: Brian Norris, David Woodhouse, Ezequiel Garcia; +Cc: devicetree, linux-mtd From: Jordan Friendshuh <jfriendshuh@xes-inc.com> Support the generic nand-ecc-mode, nand-ecc-strength, and nand-ecc-step-size device-tree properties with the Freescale UPM NAND driver. This patch preserves the default software ECC mode while adding the ability to use BCH ECC for larger NAND devices. Signed-off-by: Jordan Friendshuh <jfriendshuh@xes-inc.com> Signed-off-by: Aaron Sierra <asierra@xes-inc.com> --- v2: * Now using ECC mode and strength helpers from of_mtd.h * ECC mode and strength checking is more robust v3: * Require nand-ecc-step-size for soft_bch. * Simplify mode/strength/step parameter checking. .../devicetree/bindings/mtd/fsl-upm-nand.txt | 32 +++++++++++ drivers/mtd/nand/Kconfig | 1 + drivers/mtd/nand/fsl_upm.c | 66 ++++++++++++++++++++-- 3 files changed, 94 insertions(+), 5 deletions(-) diff --git a/Documentation/devicetree/bindings/mtd/fsl-upm-nand.txt b/Documentation/devicetree/bindings/mtd/fsl-upm-nand.txt index fce4894..3643ee1 100644 --- a/Documentation/devicetree/bindings/mtd/fsl-upm-nand.txt +++ b/Documentation/devicetree/bindings/mtd/fsl-upm-nand.txt @@ -18,6 +18,9 @@ Optional properties: - chip-delay : chip dependent delay for transferring data from array to read registers (tR). Required if property "gpios" is not used (R/B# pins not connected). +- nand-ecc-mode : as defined by nand.txt ("soft" and "soft_bch", only). +- nand-ecc-strength : as defined by nand.txt. +- nand-ecc-step-size : as defined by nand.txt. Each flash chip described may optionally contain additional sub-nodes describing partitions of the address space. See partition.txt for more @@ -65,3 +68,32 @@ upm@3,0 { }; }; }; + +/* + * Micron MT29F32G08AFABA (M62B) + * 32 Gb (4 GiB), 2 chipselect + */ +upm@2,0 { + #address-cells = <0>; + #size-cells = <0>; + compatible = "fsl,upm-nand"; + reg = <2 0x0 0x80000>; + fsl,upm-addr-line-cs-offsets = <0x0 0x10000>; + fsl,upm-addr-offset = <0x10>; + fsl,upm-cmd-offset = <0x08>; + fsl,upm-wait-flags = <0x1>; + chip-delay = <50>; + + nand@0 { + #address-cells = <1>; + #size-cells = <2>; + nand-ecc-mode = "soft_bch"; + nand-ecc-strength = <4>; + nand-ecc-step-size = <512>; + + partition@0 { + label = "NAND filesystem"; + reg = <0x0 0x1 0x00000000>; + }; + }; +}; diff --git a/drivers/mtd/nand/Kconfig b/drivers/mtd/nand/Kconfig index e5e3343..4c85daf 100644 --- a/drivers/mtd/nand/Kconfig +++ b/drivers/mtd/nand/Kconfig @@ -444,6 +444,7 @@ config MTD_NAND_FSL_UPM tristate "Support for NAND on Freescale UPM" depends on PPC_83xx || PPC_85xx select FSL_LBC + select MTD_NAND_ECC_BCH help Enables support for NAND Flash chips wired onto Freescale PowerPC processor localbus with User-Programmable Machine support. diff --git a/drivers/mtd/nand/fsl_upm.c b/drivers/mtd/nand/fsl_upm.c index 72755d7..053d8bf 100644 --- a/drivers/mtd/nand/fsl_upm.c +++ b/drivers/mtd/nand/fsl_upm.c @@ -18,6 +18,7 @@ #include <linux/mtd/nand_ecc.h> #include <linux/mtd/partitions.h> #include <linux/mtd/mtd.h> +#include <linux/of_mtd.h> #include <linux/of_address.h> #include <linux/of_platform.h> #include <linux/of_gpio.h> @@ -160,6 +161,11 @@ static int fun_chip_init(struct fsl_upm_nand *fun, int ret; struct device_node *flash_np; struct mtd_part_parser_data ppdata; + int mode, strength, step_size; + + flash_np = of_get_next_child(upm_np, NULL); + if (!flash_np) + return -ENODEV; fun->chip.IO_ADDR_R = fun->io_base; fun->chip.IO_ADDR_W = fun->io_base; @@ -168,7 +174,61 @@ static int fun_chip_init(struct fsl_upm_nand *fun, fun->chip.read_byte = fun_read_byte; fun->chip.read_buf = fun_read_buf; fun->chip.write_buf = fun_write_buf; - fun->chip.ecc.mode = NAND_ECC_SOFT; + + /* + * Support NAND_ECC_SOFT and NAND_ECC_SOFT_BCH, error otherwise. + */ + mode = of_get_nand_ecc_mode(flash_np); + strength = of_get_nand_ecc_strength(flash_np); + step_size = of_get_nand_ecc_step_size(flash_np); + if (mode < 0) { + dev_info(fun->dev, "ECC mode defaulting to 'soft'"); + mode = NAND_ECC_SOFT; + } else if (mode != NAND_ECC_SOFT && mode != NAND_ECC_SOFT_BCH) { + dev_err(fun->dev, "ECC mode in device tree is unsupported"); + ret = -EINVAL; + goto err; + } + + /* + * In NAND_ECC_SOFT_BCH, require strength >= 1 and step >= 1. + * In NAND_ECC_SOFT: + * a. ignore strength (1 implied) + * b. step < 0, step = 256, or step = 512. + */ + if (mode == NAND_ECC_SOFT_BCH) { + if (strength < 1) + dev_err(fun->dev, "invalid nand-ecc-strength for BCH"); + + if (step_size < 1) + dev_err(fun->dev, "invalid nand-ecc-step-size for BCH"); + + if (strength < 1 || step_size < 1) { + ret = -EINVAL; + goto err; + } + + fun->chip.ecc.mode = mode; + fun->chip.ecc.strength = strength; + fun->chip.ecc.size = step_size; + } else { + if (strength >= 0) + dev_warn(fun->dev, "soft ECC implies 1-bit strength"); + + if (step_size < 0) { + step_size = 256; + } else if (step_size != 256 && step_size != 512) { + dev_err(fun->dev, + "soft ECC needs 256 or 512 byte step"); + ret = -EINVAL; + goto err; + } + + fun->chip.ecc.mode = mode; + fun->chip.ecc.strength = 1; + fun->chip.ecc.size = step_size; + } + if (fun->mchip_count > 1) fun->chip.select_chip = fun_select_chip; @@ -178,10 +238,6 @@ static int fun_chip_init(struct fsl_upm_nand *fun, fun->mtd.priv = &fun->chip; fun->mtd.owner = THIS_MODULE; - flash_np = of_get_next_child(upm_np, NULL); - if (!flash_np) - return -ENODEV; - fun->mtd.name = kasprintf(GFP_KERNEL, "0x%llx.%s", (u64)io_res->start, flash_np->name); if (!fun->mtd.name) { -- 1.9.1 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH 2/2 v3 RESEND] mtd: fsl_upm: Support NAND ECC DTS properties 2015-01-13 0:36 ` Aaron Sierra @ 2015-01-14 23:41 ` Aaron Sierra -1 siblings, 0 replies; 18+ messages in thread From: Aaron Sierra @ 2015-01-14 23:41 UTC (permalink / raw) To: Brian Norris, David Woodhouse, Ezequiel Garcia; +Cc: devicetree, linux-mtd From: Jordan Friendshuh <jfriendshuh@xes-inc.com> Support the generic nand-ecc-mode, nand-ecc-strength, and nand-ecc-step-size device-tree properties with the Freescale UPM NAND driver. This patch preserves the default software ECC mode while adding the ability to use BCH ECC for larger NAND devices. Signed-off-by: Jordan Friendshuh <jfriendshuh@xes-inc.com> Signed-off-by: Aaron Sierra <asierra@xes-inc.com> --- v2: * Now using ECC mode and strength helpers from of_mtd.h * ECC mode and strength checking is more robust v3 (resent due to [PATCH 1/2] v2 update): * Require nand-ecc-step-size for soft_bch. * Simplify mode/strength/step parameter checking. .../devicetree/bindings/mtd/fsl-upm-nand.txt | 32 +++++++++++ drivers/mtd/nand/Kconfig | 1 + drivers/mtd/nand/fsl_upm.c | 66 ++++++++++++++++++++-- 3 files changed, 94 insertions(+), 5 deletions(-) diff --git a/Documentation/devicetree/bindings/mtd/fsl-upm-nand.txt b/Documentation/devicetree/bindings/mtd/fsl-upm-nand.txt index fce4894..3643ee1 100644 --- a/Documentation/devicetree/bindings/mtd/fsl-upm-nand.txt +++ b/Documentation/devicetree/bindings/mtd/fsl-upm-nand.txt @@ -18,6 +18,9 @@ Optional properties: - chip-delay : chip dependent delay for transferring data from array to read registers (tR). Required if property "gpios" is not used (R/B# pins not connected). +- nand-ecc-mode : as defined by nand.txt ("soft" and "soft_bch", only). +- nand-ecc-strength : as defined by nand.txt. +- nand-ecc-step-size : as defined by nand.txt. Each flash chip described may optionally contain additional sub-nodes describing partitions of the address space. See partition.txt for more @@ -65,3 +68,32 @@ upm@3,0 { }; }; }; + +/* + * Micron MT29F32G08AFABA (M62B) + * 32 Gb (4 GiB), 2 chipselect + */ +upm@2,0 { + #address-cells = <0>; + #size-cells = <0>; + compatible = "fsl,upm-nand"; + reg = <2 0x0 0x80000>; + fsl,upm-addr-line-cs-offsets = <0x0 0x10000>; + fsl,upm-addr-offset = <0x10>; + fsl,upm-cmd-offset = <0x08>; + fsl,upm-wait-flags = <0x1>; + chip-delay = <50>; + + nand@0 { + #address-cells = <1>; + #size-cells = <2>; + nand-ecc-mode = "soft_bch"; + nand-ecc-strength = <4>; + nand-ecc-step-size = <512>; + + partition@0 { + label = "NAND filesystem"; + reg = <0x0 0x1 0x00000000>; + }; + }; +}; diff --git a/drivers/mtd/nand/Kconfig b/drivers/mtd/nand/Kconfig index e5e3343..4c85daf 100644 --- a/drivers/mtd/nand/Kconfig +++ b/drivers/mtd/nand/Kconfig @@ -444,6 +444,7 @@ config MTD_NAND_FSL_UPM tristate "Support for NAND on Freescale UPM" depends on PPC_83xx || PPC_85xx select FSL_LBC + select MTD_NAND_ECC_BCH help Enables support for NAND Flash chips wired onto Freescale PowerPC processor localbus with User-Programmable Machine support. diff --git a/drivers/mtd/nand/fsl_upm.c b/drivers/mtd/nand/fsl_upm.c index 72755d7..053d8bf 100644 --- a/drivers/mtd/nand/fsl_upm.c +++ b/drivers/mtd/nand/fsl_upm.c @@ -18,6 +18,7 @@ #include <linux/mtd/nand_ecc.h> #include <linux/mtd/partitions.h> #include <linux/mtd/mtd.h> +#include <linux/of_mtd.h> #include <linux/of_address.h> #include <linux/of_platform.h> #include <linux/of_gpio.h> @@ -160,6 +161,11 @@ static int fun_chip_init(struct fsl_upm_nand *fun, int ret; struct device_node *flash_np; struct mtd_part_parser_data ppdata; + int mode, strength, step_size; + + flash_np = of_get_next_child(upm_np, NULL); + if (!flash_np) + return -ENODEV; fun->chip.IO_ADDR_R = fun->io_base; fun->chip.IO_ADDR_W = fun->io_base; @@ -168,7 +174,61 @@ static int fun_chip_init(struct fsl_upm_nand *fun, fun->chip.read_byte = fun_read_byte; fun->chip.read_buf = fun_read_buf; fun->chip.write_buf = fun_write_buf; - fun->chip.ecc.mode = NAND_ECC_SOFT; + + /* + * Support NAND_ECC_SOFT and NAND_ECC_SOFT_BCH, error otherwise. + */ + mode = of_get_nand_ecc_mode(flash_np); + strength = of_get_nand_ecc_strength(flash_np); + step_size = of_get_nand_ecc_step_size(flash_np); + if (mode < 0) { + dev_info(fun->dev, "ECC mode defaulting to 'soft'"); + mode = NAND_ECC_SOFT; + } else if (mode != NAND_ECC_SOFT && mode != NAND_ECC_SOFT_BCH) { + dev_err(fun->dev, "ECC mode in device tree is unsupported"); + ret = -EINVAL; + goto err; + } + + /* + * In NAND_ECC_SOFT_BCH, require strength >= 1 and step >= 1. + * In NAND_ECC_SOFT: + * a. ignore strength (1 implied) + * b. step < 0, step = 256, or step = 512. + */ + if (mode == NAND_ECC_SOFT_BCH) { + if (strength < 1) + dev_err(fun->dev, "invalid nand-ecc-strength for BCH"); + + if (step_size < 1) + dev_err(fun->dev, "invalid nand-ecc-step-size for BCH"); + + if (strength < 1 || step_size < 1) { + ret = -EINVAL; + goto err; + } + + fun->chip.ecc.mode = mode; + fun->chip.ecc.strength = strength; + fun->chip.ecc.size = step_size; + } else { + if (strength >= 0) + dev_warn(fun->dev, "soft ECC implies 1-bit strength"); + + if (step_size < 0) { + step_size = 256; + } else if (step_size != 256 && step_size != 512) { + dev_err(fun->dev, + "soft ECC needs 256 or 512 byte step"); + ret = -EINVAL; + goto err; + } + + fun->chip.ecc.mode = mode; + fun->chip.ecc.strength = 1; + fun->chip.ecc.size = step_size; + } + if (fun->mchip_count > 1) fun->chip.select_chip = fun_select_chip; @@ -178,10 +238,6 @@ static int fun_chip_init(struct fsl_upm_nand *fun, fun->mtd.priv = &fun->chip; fun->mtd.owner = THIS_MODULE; - flash_np = of_get_next_child(upm_np, NULL); - if (!flash_np) - return -ENODEV; - fun->mtd.name = kasprintf(GFP_KERNEL, "0x%llx.%s", (u64)io_res->start, flash_np->name); if (!fun->mtd.name) { -- 1.9.1 ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/ ^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH 2/2 v3 RESEND] mtd: fsl_upm: Support NAND ECC DTS properties @ 2015-01-14 23:41 ` Aaron Sierra 0 siblings, 0 replies; 18+ messages in thread From: Aaron Sierra @ 2015-01-14 23:41 UTC (permalink / raw) To: Brian Norris, David Woodhouse, Ezequiel Garcia; +Cc: devicetree, linux-mtd From: Jordan Friendshuh <jfriendshuh@xes-inc.com> Support the generic nand-ecc-mode, nand-ecc-strength, and nand-ecc-step-size device-tree properties with the Freescale UPM NAND driver. This patch preserves the default software ECC mode while adding the ability to use BCH ECC for larger NAND devices. Signed-off-by: Jordan Friendshuh <jfriendshuh@xes-inc.com> Signed-off-by: Aaron Sierra <asierra@xes-inc.com> --- v2: * Now using ECC mode and strength helpers from of_mtd.h * ECC mode and strength checking is more robust v3 (resent due to [PATCH 1/2] v2 update): * Require nand-ecc-step-size for soft_bch. * Simplify mode/strength/step parameter checking. .../devicetree/bindings/mtd/fsl-upm-nand.txt | 32 +++++++++++ drivers/mtd/nand/Kconfig | 1 + drivers/mtd/nand/fsl_upm.c | 66 ++++++++++++++++++++-- 3 files changed, 94 insertions(+), 5 deletions(-) diff --git a/Documentation/devicetree/bindings/mtd/fsl-upm-nand.txt b/Documentation/devicetree/bindings/mtd/fsl-upm-nand.txt index fce4894..3643ee1 100644 --- a/Documentation/devicetree/bindings/mtd/fsl-upm-nand.txt +++ b/Documentation/devicetree/bindings/mtd/fsl-upm-nand.txt @@ -18,6 +18,9 @@ Optional properties: - chip-delay : chip dependent delay for transferring data from array to read registers (tR). Required if property "gpios" is not used (R/B# pins not connected). +- nand-ecc-mode : as defined by nand.txt ("soft" and "soft_bch", only). +- nand-ecc-strength : as defined by nand.txt. +- nand-ecc-step-size : as defined by nand.txt. Each flash chip described may optionally contain additional sub-nodes describing partitions of the address space. See partition.txt for more @@ -65,3 +68,32 @@ upm@3,0 { }; }; }; + +/* + * Micron MT29F32G08AFABA (M62B) + * 32 Gb (4 GiB), 2 chipselect + */ +upm@2,0 { + #address-cells = <0>; + #size-cells = <0>; + compatible = "fsl,upm-nand"; + reg = <2 0x0 0x80000>; + fsl,upm-addr-line-cs-offsets = <0x0 0x10000>; + fsl,upm-addr-offset = <0x10>; + fsl,upm-cmd-offset = <0x08>; + fsl,upm-wait-flags = <0x1>; + chip-delay = <50>; + + nand@0 { + #address-cells = <1>; + #size-cells = <2>; + nand-ecc-mode = "soft_bch"; + nand-ecc-strength = <4>; + nand-ecc-step-size = <512>; + + partition@0 { + label = "NAND filesystem"; + reg = <0x0 0x1 0x00000000>; + }; + }; +}; diff --git a/drivers/mtd/nand/Kconfig b/drivers/mtd/nand/Kconfig index e5e3343..4c85daf 100644 --- a/drivers/mtd/nand/Kconfig +++ b/drivers/mtd/nand/Kconfig @@ -444,6 +444,7 @@ config MTD_NAND_FSL_UPM tristate "Support for NAND on Freescale UPM" depends on PPC_83xx || PPC_85xx select FSL_LBC + select MTD_NAND_ECC_BCH help Enables support for NAND Flash chips wired onto Freescale PowerPC processor localbus with User-Programmable Machine support. diff --git a/drivers/mtd/nand/fsl_upm.c b/drivers/mtd/nand/fsl_upm.c index 72755d7..053d8bf 100644 --- a/drivers/mtd/nand/fsl_upm.c +++ b/drivers/mtd/nand/fsl_upm.c @@ -18,6 +18,7 @@ #include <linux/mtd/nand_ecc.h> #include <linux/mtd/partitions.h> #include <linux/mtd/mtd.h> +#include <linux/of_mtd.h> #include <linux/of_address.h> #include <linux/of_platform.h> #include <linux/of_gpio.h> @@ -160,6 +161,11 @@ static int fun_chip_init(struct fsl_upm_nand *fun, int ret; struct device_node *flash_np; struct mtd_part_parser_data ppdata; + int mode, strength, step_size; + + flash_np = of_get_next_child(upm_np, NULL); + if (!flash_np) + return -ENODEV; fun->chip.IO_ADDR_R = fun->io_base; fun->chip.IO_ADDR_W = fun->io_base; @@ -168,7 +174,61 @@ static int fun_chip_init(struct fsl_upm_nand *fun, fun->chip.read_byte = fun_read_byte; fun->chip.read_buf = fun_read_buf; fun->chip.write_buf = fun_write_buf; - fun->chip.ecc.mode = NAND_ECC_SOFT; + + /* + * Support NAND_ECC_SOFT and NAND_ECC_SOFT_BCH, error otherwise. + */ + mode = of_get_nand_ecc_mode(flash_np); + strength = of_get_nand_ecc_strength(flash_np); + step_size = of_get_nand_ecc_step_size(flash_np); + if (mode < 0) { + dev_info(fun->dev, "ECC mode defaulting to 'soft'"); + mode = NAND_ECC_SOFT; + } else if (mode != NAND_ECC_SOFT && mode != NAND_ECC_SOFT_BCH) { + dev_err(fun->dev, "ECC mode in device tree is unsupported"); + ret = -EINVAL; + goto err; + } + + /* + * In NAND_ECC_SOFT_BCH, require strength >= 1 and step >= 1. + * In NAND_ECC_SOFT: + * a. ignore strength (1 implied) + * b. step < 0, step = 256, or step = 512. + */ + if (mode == NAND_ECC_SOFT_BCH) { + if (strength < 1) + dev_err(fun->dev, "invalid nand-ecc-strength for BCH"); + + if (step_size < 1) + dev_err(fun->dev, "invalid nand-ecc-step-size for BCH"); + + if (strength < 1 || step_size < 1) { + ret = -EINVAL; + goto err; + } + + fun->chip.ecc.mode = mode; + fun->chip.ecc.strength = strength; + fun->chip.ecc.size = step_size; + } else { + if (strength >= 0) + dev_warn(fun->dev, "soft ECC implies 1-bit strength"); + + if (step_size < 0) { + step_size = 256; + } else if (step_size != 256 && step_size != 512) { + dev_err(fun->dev, + "soft ECC needs 256 or 512 byte step"); + ret = -EINVAL; + goto err; + } + + fun->chip.ecc.mode = mode; + fun->chip.ecc.strength = 1; + fun->chip.ecc.size = step_size; + } + if (fun->mchip_count > 1) fun->chip.select_chip = fun_select_chip; @@ -178,10 +238,6 @@ static int fun_chip_init(struct fsl_upm_nand *fun, fun->mtd.priv = &fun->chip; fun->mtd.owner = THIS_MODULE; - flash_np = of_get_next_child(upm_np, NULL); - if (!flash_np) - return -ENODEV; - fun->mtd.name = kasprintf(GFP_KERNEL, "0x%llx.%s", (u64)io_res->start, flash_np->name); if (!fun->mtd.name) { -- 1.9.1 ^ permalink raw reply related [flat|nested] 18+ messages in thread
[parent not found: <447095612.147126.1421278909058.JavaMail.zimbra-AQeFf1F/bRxBDgjK7y7TUQ@public.gmane.org>]
* Re: [PATCH 2/2 v3 RESEND] mtd: fsl_upm: Support NAND ECC DTS properties 2015-01-14 23:41 ` Aaron Sierra @ 2015-01-23 7:43 ` Brian Norris -1 siblings, 0 replies; 18+ messages in thread From: Brian Norris @ 2015-01-23 7:43 UTC (permalink / raw) To: Aaron Sierra Cc: David Woodhouse, Ezequiel Garcia, linux-mtd-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, devicetree-u79uwXL29TY76Z2rM5mHXA Hi Jordan, On Wed, Jan 14, 2015 at 05:41:49PM -0600, Aaron Sierra wrote: > From: Jordan Friendshuh <jfriendshuh-AQeFf1F/bRxBDgjK7y7TUQ@public.gmane.org> > > Support the generic nand-ecc-mode, nand-ecc-strength, and > nand-ecc-step-size device-tree properties with the Freescale UPM NAND > driver. > > This patch preserves the default software ECC mode while adding the > ability to use BCH ECC for larger NAND devices. > > Signed-off-by: Jordan Friendshuh <jfriendshuh-AQeFf1F/bRxBDgjK7y7TUQ@public.gmane.org> > Signed-off-by: Aaron Sierra <asierra-AQeFf1F/bRxBDgjK7y7TUQ@public.gmane.org> > --- > v2: > * Now using ECC mode and strength helpers from of_mtd.h > * ECC mode and strength checking is more robust > v3 (resent due to [PATCH 1/2] v2 update): > * Require nand-ecc-step-size for soft_bch. > * Simplify mode/strength/step parameter checking. This mostly looks pretty good. > .../devicetree/bindings/mtd/fsl-upm-nand.txt | 32 +++++++++++ > drivers/mtd/nand/Kconfig | 1 + > drivers/mtd/nand/fsl_upm.c | 66 ++++++++++++++++++++-- > 3 files changed, 94 insertions(+), 5 deletions(-) > ... > diff --git a/drivers/mtd/nand/Kconfig b/drivers/mtd/nand/Kconfig > index e5e3343..4c85daf 100644 > --- a/drivers/mtd/nand/Kconfig > +++ b/drivers/mtd/nand/Kconfig > @@ -444,6 +444,7 @@ config MTD_NAND_FSL_UPM > tristate "Support for NAND on Freescale UPM" > depends on PPC_83xx || PPC_85xx > select FSL_LBC > + select MTD_NAND_ECC_BCH Hmm, do you really need to 'select' here? I think your driver compiles just fine without it, and nand_base gives you a BUG() if you try to use its soft BCH without building in the driver. It's normally bad form to 'select' an option that is normally user-configurable, unless you absolutely require it. > help > Enables support for NAND Flash chips wired onto Freescale PowerPC > processor localbus with User-Programmable Machine support. > diff --git a/drivers/mtd/nand/fsl_upm.c b/drivers/mtd/nand/fsl_upm.c > index 72755d7..053d8bf 100644 > --- a/drivers/mtd/nand/fsl_upm.c > +++ b/drivers/mtd/nand/fsl_upm.c ... > @@ -168,7 +174,61 @@ static int fun_chip_init(struct fsl_upm_nand *fun, > fun->chip.read_byte = fun_read_byte; > fun->chip.read_buf = fun_read_buf; > fun->chip.write_buf = fun_write_buf; > - fun->chip.ecc.mode = NAND_ECC_SOFT; > + > + /* > + * Support NAND_ECC_SOFT and NAND_ECC_SOFT_BCH, error otherwise. > + */ > + mode = of_get_nand_ecc_mode(flash_np); > + strength = of_get_nand_ecc_strength(flash_np); > + step_size = of_get_nand_ecc_step_size(flash_np); > + if (mode < 0) { > + dev_info(fun->dev, "ECC mode defaulting to 'soft'"); > + mode = NAND_ECC_SOFT; > + } else if (mode != NAND_ECC_SOFT && mode != NAND_ECC_SOFT_BCH) { > + dev_err(fun->dev, "ECC mode in device tree is unsupported"); > + ret = -EINVAL; > + goto err; > + } > + > + /* > + * In NAND_ECC_SOFT_BCH, require strength >= 1 and step >= 1. > + * In NAND_ECC_SOFT: > + * a. ignore strength (1 implied) > + * b. step < 0, step = 256, or step = 512. (I'm getting nitpicky here, but if you're going to change the Kconfig above, you might as well address this.) This comment is nice, but it's still slightly confusing, as it doesn't have very good parallel structure. There needs to be a verb in bullet point (b). Perhaps the following? * b. require step < 0 (default to 256), step = 256, or step = 512. > + */ > + if (mode == NAND_ECC_SOFT_BCH) { > + if (strength < 1) > + dev_err(fun->dev, "invalid nand-ecc-strength for BCH"); > + > + if (step_size < 1) > + dev_err(fun->dev, "invalid nand-ecc-step-size for BCH"); > + > + if (strength < 1 || step_size < 1) { > + ret = -EINVAL; > + goto err; > + } > + > + fun->chip.ecc.mode = mode; > + fun->chip.ecc.strength = strength; > + fun->chip.ecc.size = step_size; > + } else { > + if (strength >= 0) I don't see why we should complain about strength == 1. It's a perfectly descriptive value from DT. Maybe: if (strength != 1 && strength >= 0) > + dev_warn(fun->dev, "soft ECC implies 1-bit strength"); > + > + if (step_size < 0) { > + step_size = 256; > + } else if (step_size != 256 && step_size != 512) { > + dev_err(fun->dev, > + "soft ECC needs 256 or 512 byte step"); > + ret = -EINVAL; > + goto err; > + } > + > + fun->chip.ecc.mode = mode; > + fun->chip.ecc.strength = 1; > + fun->chip.ecc.size = step_size; > + } > + > if (fun->mchip_count > 1) > fun->chip.select_chip = fun_select_chip; > ... Now that I think about it, I can just apply all these changes and apply the patch myself, if you agree. Or send a new version yourself. Either way. Regards, Brian -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 2/2 v3 RESEND] mtd: fsl_upm: Support NAND ECC DTS properties @ 2015-01-23 7:43 ` Brian Norris 0 siblings, 0 replies; 18+ messages in thread From: Brian Norris @ 2015-01-23 7:43 UTC (permalink / raw) To: Aaron Sierra; +Cc: linux-mtd, David Woodhouse, Ezequiel Garcia, devicetree Hi Jordan, On Wed, Jan 14, 2015 at 05:41:49PM -0600, Aaron Sierra wrote: > From: Jordan Friendshuh <jfriendshuh@xes-inc.com> > > Support the generic nand-ecc-mode, nand-ecc-strength, and > nand-ecc-step-size device-tree properties with the Freescale UPM NAND > driver. > > This patch preserves the default software ECC mode while adding the > ability to use BCH ECC for larger NAND devices. > > Signed-off-by: Jordan Friendshuh <jfriendshuh@xes-inc.com> > Signed-off-by: Aaron Sierra <asierra@xes-inc.com> > --- > v2: > * Now using ECC mode and strength helpers from of_mtd.h > * ECC mode and strength checking is more robust > v3 (resent due to [PATCH 1/2] v2 update): > * Require nand-ecc-step-size for soft_bch. > * Simplify mode/strength/step parameter checking. This mostly looks pretty good. > .../devicetree/bindings/mtd/fsl-upm-nand.txt | 32 +++++++++++ > drivers/mtd/nand/Kconfig | 1 + > drivers/mtd/nand/fsl_upm.c | 66 ++++++++++++++++++++-- > 3 files changed, 94 insertions(+), 5 deletions(-) > ... > diff --git a/drivers/mtd/nand/Kconfig b/drivers/mtd/nand/Kconfig > index e5e3343..4c85daf 100644 > --- a/drivers/mtd/nand/Kconfig > +++ b/drivers/mtd/nand/Kconfig > @@ -444,6 +444,7 @@ config MTD_NAND_FSL_UPM > tristate "Support for NAND on Freescale UPM" > depends on PPC_83xx || PPC_85xx > select FSL_LBC > + select MTD_NAND_ECC_BCH Hmm, do you really need to 'select' here? I think your driver compiles just fine without it, and nand_base gives you a BUG() if you try to use its soft BCH without building in the driver. It's normally bad form to 'select' an option that is normally user-configurable, unless you absolutely require it. > help > Enables support for NAND Flash chips wired onto Freescale PowerPC > processor localbus with User-Programmable Machine support. > diff --git a/drivers/mtd/nand/fsl_upm.c b/drivers/mtd/nand/fsl_upm.c > index 72755d7..053d8bf 100644 > --- a/drivers/mtd/nand/fsl_upm.c > +++ b/drivers/mtd/nand/fsl_upm.c ... > @@ -168,7 +174,61 @@ static int fun_chip_init(struct fsl_upm_nand *fun, > fun->chip.read_byte = fun_read_byte; > fun->chip.read_buf = fun_read_buf; > fun->chip.write_buf = fun_write_buf; > - fun->chip.ecc.mode = NAND_ECC_SOFT; > + > + /* > + * Support NAND_ECC_SOFT and NAND_ECC_SOFT_BCH, error otherwise. > + */ > + mode = of_get_nand_ecc_mode(flash_np); > + strength = of_get_nand_ecc_strength(flash_np); > + step_size = of_get_nand_ecc_step_size(flash_np); > + if (mode < 0) { > + dev_info(fun->dev, "ECC mode defaulting to 'soft'"); > + mode = NAND_ECC_SOFT; > + } else if (mode != NAND_ECC_SOFT && mode != NAND_ECC_SOFT_BCH) { > + dev_err(fun->dev, "ECC mode in device tree is unsupported"); > + ret = -EINVAL; > + goto err; > + } > + > + /* > + * In NAND_ECC_SOFT_BCH, require strength >= 1 and step >= 1. > + * In NAND_ECC_SOFT: > + * a. ignore strength (1 implied) > + * b. step < 0, step = 256, or step = 512. (I'm getting nitpicky here, but if you're going to change the Kconfig above, you might as well address this.) This comment is nice, but it's still slightly confusing, as it doesn't have very good parallel structure. There needs to be a verb in bullet point (b). Perhaps the following? * b. require step < 0 (default to 256), step = 256, or step = 512. > + */ > + if (mode == NAND_ECC_SOFT_BCH) { > + if (strength < 1) > + dev_err(fun->dev, "invalid nand-ecc-strength for BCH"); > + > + if (step_size < 1) > + dev_err(fun->dev, "invalid nand-ecc-step-size for BCH"); > + > + if (strength < 1 || step_size < 1) { > + ret = -EINVAL; > + goto err; > + } > + > + fun->chip.ecc.mode = mode; > + fun->chip.ecc.strength = strength; > + fun->chip.ecc.size = step_size; > + } else { > + if (strength >= 0) I don't see why we should complain about strength == 1. It's a perfectly descriptive value from DT. Maybe: if (strength != 1 && strength >= 0) > + dev_warn(fun->dev, "soft ECC implies 1-bit strength"); > + > + if (step_size < 0) { > + step_size = 256; > + } else if (step_size != 256 && step_size != 512) { > + dev_err(fun->dev, > + "soft ECC needs 256 or 512 byte step"); > + ret = -EINVAL; > + goto err; > + } > + > + fun->chip.ecc.mode = mode; > + fun->chip.ecc.strength = 1; > + fun->chip.ecc.size = step_size; > + } > + > if (fun->mchip_count > 1) > fun->chip.select_chip = fun_select_chip; > ... Now that I think about it, I can just apply all these changes and apply the patch myself, if you agree. Or send a new version yourself. Either way. Regards, Brian ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 2/2 v3 RESEND] mtd: fsl_upm: Support NAND ECC DTS properties 2015-01-14 23:41 ` Aaron Sierra @ 2015-01-23 8:30 ` Brian Norris -1 siblings, 0 replies; 18+ messages in thread From: Brian Norris @ 2015-01-23 8:30 UTC (permalink / raw) To: Aaron Sierra Cc: David Woodhouse, Ezequiel Garcia, linux-mtd-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, devicetree-u79uwXL29TY76Z2rM5mHXA On Wed, Jan 14, 2015 at 05:41:49PM -0600, Aaron Sierra wrote: > From: Jordan Friendshuh <jfriendshuh-AQeFf1F/bRxBDgjK7y7TUQ@public.gmane.org> > > Support the generic nand-ecc-mode, nand-ecc-strength, and > nand-ecc-step-size device-tree properties with the Freescale UPM NAND > driver. > > This patch preserves the default software ECC mode while adding the > ability to use BCH ECC for larger NAND devices. > > Signed-off-by: Jordan Friendshuh <jfriendshuh-AQeFf1F/bRxBDgjK7y7TUQ@public.gmane.org> > Signed-off-by: Aaron Sierra <asierra-AQeFf1F/bRxBDgjK7y7TUQ@public.gmane.org> > --- > v2: > * Now using ECC mode and strength helpers from of_mtd.h > * ECC mode and strength checking is more robust > v3 (resent due to [PATCH 1/2] v2 update): > * Require nand-ecc-step-size for soft_bch. > * Simplify mode/strength/step parameter checking. > > .../devicetree/bindings/mtd/fsl-upm-nand.txt | 32 +++++++++++ > drivers/mtd/nand/Kconfig | 1 + > drivers/mtd/nand/fsl_upm.c | 66 ++++++++++++++++++++-- I was thinking about this a bit more, and it seems like we could really just factor this all into the core nand_base code with something like the following patch. It could possibly use some smarter logic to rule out certain combinations (but some of those are already caught in nand_scan_tail() anyway). What do you think? diff --git a/drivers/mtd/nand/fsl_upm.c b/drivers/mtd/nand/fsl_upm.c index 72755d7ec25d..fc4834946233 100644 --- a/drivers/mtd/nand/fsl_upm.c +++ b/drivers/mtd/nand/fsl_upm.c @@ -181,6 +181,7 @@ static int fun_chip_init(struct fsl_upm_nand *fun, flash_np = of_get_next_child(upm_np, NULL); if (!flash_np) return -ENODEV; + fun->chip.dn = flash_np; fun->mtd.name = kasprintf(GFP_KERNEL, "0x%llx.%s", (u64)io_res->start, flash_np->name); diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c index 3f24b587304f..b701c3f23da1 100644 --- a/drivers/mtd/nand/nand_base.c +++ b/drivers/mtd/nand/nand_base.c @@ -48,6 +48,7 @@ #include <linux/leds.h> #include <linux/io.h> #include <linux/mtd/partitions.h> +#include <linux/of_mtd.h> /* Define default oob placement schemes for large and small page devices */ static struct nand_ecclayout nand_oob_8 = { @@ -3780,6 +3781,33 @@ ident_done: return type; } +static int nand_dt_init(struct mtd_info *mtd, struct nand_chip *chip, + struct device_node *dn) +{ + int ecc_mode, ecc_strength, ecc_step; + + if (of_get_nand_bus_width(dn) == 16) + chip->options |= NAND_BUSWIDTH_16; + + if (of_get_nand_on_flash_bbt(dn)) + chip->bbt_options |= NAND_BBT_USE_FLASH; + + ecc_mode = of_get_nand_ecc_mode(dn); + ecc_strength = of_get_nand_ecc_strength(dn); + ecc_step = of_get_nand_ecc_step_size(dn); + + if (ecc_mode >= 0) + chip->ecc.mode = ecc_mode; + + if (ecc_strength >= 0) + chip->ecc.strength = ecc_strength; + + if (ecc_step > 0) + chip->ecc.size = ecc_step; + + return 0; +} + /** * nand_scan_ident - [NAND Interface] Scan for the NAND device * @mtd: MTD device structure @@ -3797,6 +3825,13 @@ int nand_scan_ident(struct mtd_info *mtd, int maxchips, int i, nand_maf_id, nand_dev_id; struct nand_chip *chip = mtd->priv; struct nand_flash_dev *type; + int ret; + + if (chip->dn) { + ret = nand_dt_init(mtd, chip, chip->dn); + if (ret) + return ret; + } /* Set the default functions */ nand_set_defaults(chip, chip->options & NAND_BUSWIDTH_16); diff --git a/include/linux/mtd/nand.h b/include/linux/mtd/nand.h index 3d4ea7eb2b68..e0f40e12a2c8 100644 --- a/include/linux/mtd/nand.h +++ b/include/linux/mtd/nand.h @@ -26,6 +26,8 @@ struct mtd_info; struct nand_flash_dev; +struct device_node; + /* Scan and identify a NAND device */ extern int nand_scan(struct mtd_info *mtd, int max_chips); /* @@ -542,6 +544,7 @@ struct nand_buffers { * flash device * @IO_ADDR_W: [BOARDSPECIFIC] address to write the 8 I/O lines of the * flash device. + * @dn: [BOARDSPECIFIC] device node describing this instance * @read_byte: [REPLACEABLE] read one byte from the chip * @read_word: [REPLACEABLE] read one word from the chip * @write_byte: [REPLACEABLE] write a single byte to the chip on the @@ -644,6 +647,8 @@ struct nand_chip { void __iomem *IO_ADDR_R; void __iomem *IO_ADDR_W; + struct device_node *dn; + uint8_t (*read_byte)(struct mtd_info *mtd); u16 (*read_word)(struct mtd_info *mtd); void (*write_byte)(struct mtd_info *mtd, uint8_t byte); -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH 2/2 v3 RESEND] mtd: fsl_upm: Support NAND ECC DTS properties @ 2015-01-23 8:30 ` Brian Norris 0 siblings, 0 replies; 18+ messages in thread From: Brian Norris @ 2015-01-23 8:30 UTC (permalink / raw) To: Aaron Sierra; +Cc: linux-mtd, David Woodhouse, Ezequiel Garcia, devicetree On Wed, Jan 14, 2015 at 05:41:49PM -0600, Aaron Sierra wrote: > From: Jordan Friendshuh <jfriendshuh@xes-inc.com> > > Support the generic nand-ecc-mode, nand-ecc-strength, and > nand-ecc-step-size device-tree properties with the Freescale UPM NAND > driver. > > This patch preserves the default software ECC mode while adding the > ability to use BCH ECC for larger NAND devices. > > Signed-off-by: Jordan Friendshuh <jfriendshuh@xes-inc.com> > Signed-off-by: Aaron Sierra <asierra@xes-inc.com> > --- > v2: > * Now using ECC mode and strength helpers from of_mtd.h > * ECC mode and strength checking is more robust > v3 (resent due to [PATCH 1/2] v2 update): > * Require nand-ecc-step-size for soft_bch. > * Simplify mode/strength/step parameter checking. > > .../devicetree/bindings/mtd/fsl-upm-nand.txt | 32 +++++++++++ > drivers/mtd/nand/Kconfig | 1 + > drivers/mtd/nand/fsl_upm.c | 66 ++++++++++++++++++++-- I was thinking about this a bit more, and it seems like we could really just factor this all into the core nand_base code with something like the following patch. It could possibly use some smarter logic to rule out certain combinations (but some of those are already caught in nand_scan_tail() anyway). What do you think? diff --git a/drivers/mtd/nand/fsl_upm.c b/drivers/mtd/nand/fsl_upm.c index 72755d7ec25d..fc4834946233 100644 --- a/drivers/mtd/nand/fsl_upm.c +++ b/drivers/mtd/nand/fsl_upm.c @@ -181,6 +181,7 @@ static int fun_chip_init(struct fsl_upm_nand *fun, flash_np = of_get_next_child(upm_np, NULL); if (!flash_np) return -ENODEV; + fun->chip.dn = flash_np; fun->mtd.name = kasprintf(GFP_KERNEL, "0x%llx.%s", (u64)io_res->start, flash_np->name); diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c index 3f24b587304f..b701c3f23da1 100644 --- a/drivers/mtd/nand/nand_base.c +++ b/drivers/mtd/nand/nand_base.c @@ -48,6 +48,7 @@ #include <linux/leds.h> #include <linux/io.h> #include <linux/mtd/partitions.h> +#include <linux/of_mtd.h> /* Define default oob placement schemes for large and small page devices */ static struct nand_ecclayout nand_oob_8 = { @@ -3780,6 +3781,33 @@ ident_done: return type; } +static int nand_dt_init(struct mtd_info *mtd, struct nand_chip *chip, + struct device_node *dn) +{ + int ecc_mode, ecc_strength, ecc_step; + + if (of_get_nand_bus_width(dn) == 16) + chip->options |= NAND_BUSWIDTH_16; + + if (of_get_nand_on_flash_bbt(dn)) + chip->bbt_options |= NAND_BBT_USE_FLASH; + + ecc_mode = of_get_nand_ecc_mode(dn); + ecc_strength = of_get_nand_ecc_strength(dn); + ecc_step = of_get_nand_ecc_step_size(dn); + + if (ecc_mode >= 0) + chip->ecc.mode = ecc_mode; + + if (ecc_strength >= 0) + chip->ecc.strength = ecc_strength; + + if (ecc_step > 0) + chip->ecc.size = ecc_step; + + return 0; +} + /** * nand_scan_ident - [NAND Interface] Scan for the NAND device * @mtd: MTD device structure @@ -3797,6 +3825,13 @@ int nand_scan_ident(struct mtd_info *mtd, int maxchips, int i, nand_maf_id, nand_dev_id; struct nand_chip *chip = mtd->priv; struct nand_flash_dev *type; + int ret; + + if (chip->dn) { + ret = nand_dt_init(mtd, chip, chip->dn); + if (ret) + return ret; + } /* Set the default functions */ nand_set_defaults(chip, chip->options & NAND_BUSWIDTH_16); diff --git a/include/linux/mtd/nand.h b/include/linux/mtd/nand.h index 3d4ea7eb2b68..e0f40e12a2c8 100644 --- a/include/linux/mtd/nand.h +++ b/include/linux/mtd/nand.h @@ -26,6 +26,8 @@ struct mtd_info; struct nand_flash_dev; +struct device_node; + /* Scan and identify a NAND device */ extern int nand_scan(struct mtd_info *mtd, int max_chips); /* @@ -542,6 +544,7 @@ struct nand_buffers { * flash device * @IO_ADDR_W: [BOARDSPECIFIC] address to write the 8 I/O lines of the * flash device. + * @dn: [BOARDSPECIFIC] device node describing this instance * @read_byte: [REPLACEABLE] read one byte from the chip * @read_word: [REPLACEABLE] read one word from the chip * @write_byte: [REPLACEABLE] write a single byte to the chip on the @@ -644,6 +647,8 @@ struct nand_chip { void __iomem *IO_ADDR_R; void __iomem *IO_ADDR_W; + struct device_node *dn; + uint8_t (*read_byte)(struct mtd_info *mtd); u16 (*read_word)(struct mtd_info *mtd); void (*write_byte)(struct mtd_info *mtd, uint8_t byte); ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH 2/2 v3 RESEND] mtd: fsl_upm: Support NAND ECC DTS properties 2015-01-23 8:30 ` Brian Norris @ 2015-01-29 0:37 ` Aaron Sierra -1 siblings, 0 replies; 18+ messages in thread From: Aaron Sierra @ 2015-01-29 0:37 UTC (permalink / raw) To: Brian Norris; +Cc: linux-mtd, David Woodhouse, Ezequiel Garcia, devicetree ----- Original Message ----- > From: "Brian Norris" <computersforpeace@gmail.com> > Sent: Friday, January 23, 2015 2:30:26 AM > > On Wed, Jan 14, 2015 at 05:41:49PM -0600, Aaron Sierra wrote: > > From: Jordan Friendshuh <jfriendshuh@xes-inc.com> > > > > Support the generic nand-ecc-mode, nand-ecc-strength, and > > nand-ecc-step-size device-tree properties with the Freescale UPM NAND > > driver. > > > > This patch preserves the default software ECC mode while adding the > > ability to use BCH ECC for larger NAND devices. > > > > Signed-off-by: Jordan Friendshuh <jfriendshuh@xes-inc.com> > > Signed-off-by: Aaron Sierra <asierra@xes-inc.com> > > --- > > v2: > > * Now using ECC mode and strength helpers from of_mtd.h > > * ECC mode and strength checking is more robust > > v3 (resent due to [PATCH 1/2] v2 update): > > * Require nand-ecc-step-size for soft_bch. > > * Simplify mode/strength/step parameter checking. > > > > .../devicetree/bindings/mtd/fsl-upm-nand.txt | 32 +++++++++++ > > drivers/mtd/nand/Kconfig | 1 + > > drivers/mtd/nand/fsl_upm.c | 66 > > ++++++++++++++++++++-- > > I was thinking about this a bit more, and it seems like we could really > just factor this all into the core nand_base code with something like > the following patch. It could possibly use some smarter logic to rule > out certain combinations (but some of those are already caught in > nand_scan_tail() anyway). What do you think? Brian, If the NAND device tree property fetching were moved out of fsl_upm, I think it should not be called within nand_scan(). I think that it's imperative that each driver be able to access these properties before handing off to nand_scan(), since there are hardware ECC modes that only drivers will know how to error check. Also, catching errors in nand_scan_tail() tends to result in BUG()s. That said, this could be useful as a publicly exported function that individual drivers are responsible for calling (maybe in of_mtd.c). I have a couple of additional comments inline below. > diff --git a/drivers/mtd/nand/fsl_upm.c b/drivers/mtd/nand/fsl_upm.c > index 72755d7ec25d..fc4834946233 100644 > --- a/drivers/mtd/nand/fsl_upm.c > +++ b/drivers/mtd/nand/fsl_upm.c > @@ -181,6 +181,7 @@ static int fun_chip_init(struct fsl_upm_nand *fun, > flash_np = of_get_next_child(upm_np, NULL); > if (!flash_np) > return -ENODEV; > + fun->chip.dn = flash_np; > > fun->mtd.name = kasprintf(GFP_KERNEL, "0x%llx.%s", (u64)io_res->start, > flash_np->name); > diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c > index 3f24b587304f..b701c3f23da1 100644 > --- a/drivers/mtd/nand/nand_base.c > +++ b/drivers/mtd/nand/nand_base.c > @@ -48,6 +48,7 @@ > #include <linux/leds.h> > #include <linux/io.h> > #include <linux/mtd/partitions.h> > +#include <linux/of_mtd.h> > > /* Define default oob placement schemes for large and small page devices */ > static struct nand_ecclayout nand_oob_8 = { > @@ -3780,6 +3781,33 @@ ident_done: > return type; > } > > +static int nand_dt_init(struct mtd_info *mtd, struct nand_chip *chip, > + struct device_node *dn) Assuming that of_mtd.c is the best place to put this, the prototype could be simplified a little: int of_nand_chip_init(struct device_node *dn, struct nand_chip *chip) > +{ > + int ecc_mode, ecc_strength, ecc_step; > + > + if (of_get_nand_bus_width(dn) == 16) > + chip->options |= NAND_BUSWIDTH_16; > + > + if (of_get_nand_on_flash_bbt(dn)) > + chip->bbt_options |= NAND_BBT_USE_FLASH; > + > + ecc_mode = of_get_nand_ecc_mode(dn); > + ecc_strength = of_get_nand_ecc_strength(dn); > + ecc_step = of_get_nand_ecc_step_size(dn); > + > + if (ecc_mode >= 0) > + chip->ecc.mode = ecc_mode; > + > + if (ecc_strength >= 0) > + chip->ecc.strength = ecc_strength; > + > + if (ecc_step > 0) > + chip->ecc.size = ecc_step; > + > + return 0; > +} > + > /** > * nand_scan_ident - [NAND Interface] Scan for the NAND device > * @mtd: MTD device structure > @@ -3797,6 +3825,13 @@ int nand_scan_ident(struct mtd_info *mtd, int > maxchips, > int i, nand_maf_id, nand_dev_id; > struct nand_chip *chip = mtd->priv; > struct nand_flash_dev *type; > + int ret; > + > + if (chip->dn) { > + ret = nand_dt_init(mtd, chip, chip->dn); > + if (ret) > + return ret; > + } > > /* Set the default functions */ > nand_set_defaults(chip, chip->options & NAND_BUSWIDTH_16); > diff --git a/include/linux/mtd/nand.h b/include/linux/mtd/nand.h > index 3d4ea7eb2b68..e0f40e12a2c8 100644 > --- a/include/linux/mtd/nand.h > +++ b/include/linux/mtd/nand.h > @@ -26,6 +26,8 @@ > > struct mtd_info; > struct nand_flash_dev; > +struct device_node; > + > /* Scan and identify a NAND device */ > extern int nand_scan(struct mtd_info *mtd, int max_chips); > /* > @@ -542,6 +544,7 @@ struct nand_buffers { > * flash device > * @IO_ADDR_W: [BOARDSPECIFIC] address to write the 8 I/O lines of the > * flash device. > + * @dn: [BOARDSPECIFIC] device node describing this instance > * @read_byte: [REPLACEABLE] read one byte from the chip > * @read_word: [REPLACEABLE] read one word from the chip > * @write_byte: [REPLACEABLE] write a single byte to the chip on the > @@ -644,6 +647,8 @@ struct nand_chip { > void __iomem *IO_ADDR_R; > void __iomem *IO_ADDR_W; > > + struct device_node *dn; > + This addition to struct nand_chip wouldn't be needed, then. > uint8_t (*read_byte)(struct mtd_info *mtd); > u16 (*read_word)(struct mtd_info *mtd); > void (*write_byte)(struct mtd_info *mtd, uint8_t byte); > You hinted at implementing stronger error checking. If we went this route, would it make sense to only error check the software ECC modes? -Aaron ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/ ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 2/2 v3 RESEND] mtd: fsl_upm: Support NAND ECC DTS properties @ 2015-01-29 0:37 ` Aaron Sierra 0 siblings, 0 replies; 18+ messages in thread From: Aaron Sierra @ 2015-01-29 0:37 UTC (permalink / raw) To: Brian Norris; +Cc: linux-mtd, David Woodhouse, Ezequiel Garcia, devicetree ----- Original Message ----- > From: "Brian Norris" <computersforpeace@gmail.com> > Sent: Friday, January 23, 2015 2:30:26 AM > > On Wed, Jan 14, 2015 at 05:41:49PM -0600, Aaron Sierra wrote: > > From: Jordan Friendshuh <jfriendshuh@xes-inc.com> > > > > Support the generic nand-ecc-mode, nand-ecc-strength, and > > nand-ecc-step-size device-tree properties with the Freescale UPM NAND > > driver. > > > > This patch preserves the default software ECC mode while adding the > > ability to use BCH ECC for larger NAND devices. > > > > Signed-off-by: Jordan Friendshuh <jfriendshuh@xes-inc.com> > > Signed-off-by: Aaron Sierra <asierra@xes-inc.com> > > --- > > v2: > > * Now using ECC mode and strength helpers from of_mtd.h > > * ECC mode and strength checking is more robust > > v3 (resent due to [PATCH 1/2] v2 update): > > * Require nand-ecc-step-size for soft_bch. > > * Simplify mode/strength/step parameter checking. > > > > .../devicetree/bindings/mtd/fsl-upm-nand.txt | 32 +++++++++++ > > drivers/mtd/nand/Kconfig | 1 + > > drivers/mtd/nand/fsl_upm.c | 66 > > ++++++++++++++++++++-- > > I was thinking about this a bit more, and it seems like we could really > just factor this all into the core nand_base code with something like > the following patch. It could possibly use some smarter logic to rule > out certain combinations (but some of those are already caught in > nand_scan_tail() anyway). What do you think? Brian, If the NAND device tree property fetching were moved out of fsl_upm, I think it should not be called within nand_scan(). I think that it's imperative that each driver be able to access these properties before handing off to nand_scan(), since there are hardware ECC modes that only drivers will know how to error check. Also, catching errors in nand_scan_tail() tends to result in BUG()s. That said, this could be useful as a publicly exported function that individual drivers are responsible for calling (maybe in of_mtd.c). I have a couple of additional comments inline below. > diff --git a/drivers/mtd/nand/fsl_upm.c b/drivers/mtd/nand/fsl_upm.c > index 72755d7ec25d..fc4834946233 100644 > --- a/drivers/mtd/nand/fsl_upm.c > +++ b/drivers/mtd/nand/fsl_upm.c > @@ -181,6 +181,7 @@ static int fun_chip_init(struct fsl_upm_nand *fun, > flash_np = of_get_next_child(upm_np, NULL); > if (!flash_np) > return -ENODEV; > + fun->chip.dn = flash_np; > > fun->mtd.name = kasprintf(GFP_KERNEL, "0x%llx.%s", (u64)io_res->start, > flash_np->name); > diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c > index 3f24b587304f..b701c3f23da1 100644 > --- a/drivers/mtd/nand/nand_base.c > +++ b/drivers/mtd/nand/nand_base.c > @@ -48,6 +48,7 @@ > #include <linux/leds.h> > #include <linux/io.h> > #include <linux/mtd/partitions.h> > +#include <linux/of_mtd.h> > > /* Define default oob placement schemes for large and small page devices */ > static struct nand_ecclayout nand_oob_8 = { > @@ -3780,6 +3781,33 @@ ident_done: > return type; > } > > +static int nand_dt_init(struct mtd_info *mtd, struct nand_chip *chip, > + struct device_node *dn) Assuming that of_mtd.c is the best place to put this, the prototype could be simplified a little: int of_nand_chip_init(struct device_node *dn, struct nand_chip *chip) > +{ > + int ecc_mode, ecc_strength, ecc_step; > + > + if (of_get_nand_bus_width(dn) == 16) > + chip->options |= NAND_BUSWIDTH_16; > + > + if (of_get_nand_on_flash_bbt(dn)) > + chip->bbt_options |= NAND_BBT_USE_FLASH; > + > + ecc_mode = of_get_nand_ecc_mode(dn); > + ecc_strength = of_get_nand_ecc_strength(dn); > + ecc_step = of_get_nand_ecc_step_size(dn); > + > + if (ecc_mode >= 0) > + chip->ecc.mode = ecc_mode; > + > + if (ecc_strength >= 0) > + chip->ecc.strength = ecc_strength; > + > + if (ecc_step > 0) > + chip->ecc.size = ecc_step; > + > + return 0; > +} > + > /** > * nand_scan_ident - [NAND Interface] Scan for the NAND device > * @mtd: MTD device structure > @@ -3797,6 +3825,13 @@ int nand_scan_ident(struct mtd_info *mtd, int > maxchips, > int i, nand_maf_id, nand_dev_id; > struct nand_chip *chip = mtd->priv; > struct nand_flash_dev *type; > + int ret; > + > + if (chip->dn) { > + ret = nand_dt_init(mtd, chip, chip->dn); > + if (ret) > + return ret; > + } > > /* Set the default functions */ > nand_set_defaults(chip, chip->options & NAND_BUSWIDTH_16); > diff --git a/include/linux/mtd/nand.h b/include/linux/mtd/nand.h > index 3d4ea7eb2b68..e0f40e12a2c8 100644 > --- a/include/linux/mtd/nand.h > +++ b/include/linux/mtd/nand.h > @@ -26,6 +26,8 @@ > > struct mtd_info; > struct nand_flash_dev; > +struct device_node; > + > /* Scan and identify a NAND device */ > extern int nand_scan(struct mtd_info *mtd, int max_chips); > /* > @@ -542,6 +544,7 @@ struct nand_buffers { > * flash device > * @IO_ADDR_W: [BOARDSPECIFIC] address to write the 8 I/O lines of the > * flash device. > + * @dn: [BOARDSPECIFIC] device node describing this instance > * @read_byte: [REPLACEABLE] read one byte from the chip > * @read_word: [REPLACEABLE] read one word from the chip > * @write_byte: [REPLACEABLE] write a single byte to the chip on the > @@ -644,6 +647,8 @@ struct nand_chip { > void __iomem *IO_ADDR_R; > void __iomem *IO_ADDR_W; > > + struct device_node *dn; > + This addition to struct nand_chip wouldn't be needed, then. > uint8_t (*read_byte)(struct mtd_info *mtd); > u16 (*read_word)(struct mtd_info *mtd); > void (*write_byte)(struct mtd_info *mtd, uint8_t byte); > You hinted at implementing stronger error checking. If we went this route, would it make sense to only error check the software ECC modes? -Aaron ^ permalink raw reply [flat|nested] 18+ messages in thread
[parent not found: <1309767342.94086.1422491856685.JavaMail.zimbra-AQeFf1F/bRxBDgjK7y7TUQ@public.gmane.org>]
* Re: [PATCH 2/2 v3 RESEND] mtd: fsl_upm: Support NAND ECC DTS properties 2015-01-29 0:37 ` Aaron Sierra @ 2015-01-29 1:20 ` Brian Norris -1 siblings, 0 replies; 18+ messages in thread From: Brian Norris @ 2015-01-29 1:20 UTC (permalink / raw) To: Aaron Sierra Cc: David Woodhouse, Ezequiel Garcia, linux-mtd-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, devicetree-u79uwXL29TY76Z2rM5mHXA On Wed, Jan 28, 2015 at 06:37:36PM -0600, Aaron Sierra wrote: > ----- Original Message ----- > > From: "Brian Norris" <computersforpeace-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> > > I was thinking about this a bit more, and it seems like we could really > > just factor this all into the core nand_base code with something like > > the following patch. It could possibly use some smarter logic to rule > > out certain combinations (but some of those are already caught in > > nand_scan_tail() anyway). What do you think? > > Brian, > If the NAND device tree property fetching were moved out of fsl_upm, > I think it should not be called within nand_scan(). I think that > it's imperative that each driver be able to access these properties > before handing off to nand_scan(), since there are hardware ECC > modes that only drivers will know how to error check. That's why nand_scan() is broken into nand_scan_ident() and nand_scan_tail() functions which can be called individually. This allows drivers to do the up-front initialization in nand_scan_ident(), do their own error checking and handling of these parameters, and then call nand_scan_tail(). See, for example, drivers/mtd/nand/omap2.c. > Also, catching errors in nand_scan_tail() tends to result in BUG()s. Well, some of those can be changed. Feel free to propose changes. I'd prefer to make nand_scan_tail() play nicer than to compensate in individual drivers. > That said, this could be useful as a publicly exported function that > individual drivers are responsible for calling (maybe in of_mtd.c). I don't think of_mtd.c should really contain a lot of mtd_info / nand_chip knowledge, if we can avoid it. I really do think that the nand_scan() option is a better idea, if we can work out the other details (BUG(), error checking, and keeping it flexible enough). I think it provides the best place to flesh out any other common DT handling for all NAND drivers. Related: I believe the question came up recently about how to support a generic DT binding for using a GPIO as a NAND write-protect line. This would be another candidate for handling transparently in nand_scan_ident() and would then immediately apply to all NAND drivers, not just those that were rewritten to call another specialized init function. I really don't want to encourage the anti-pattern of each driver reimplementing code that might as well be shared, if at all possible. Adding more decentralized helpers to of_mtd.c does not really help that cause. > I have a couple of additional comments inline below. > > > diff --git a/drivers/mtd/nand/fsl_upm.c b/drivers/mtd/nand/fsl_upm.c > > index 72755d7ec25d..fc4834946233 100644 > > --- a/drivers/mtd/nand/fsl_upm.c > > +++ b/drivers/mtd/nand/fsl_upm.c > > @@ -181,6 +181,7 @@ static int fun_chip_init(struct fsl_upm_nand *fun, > > flash_np = of_get_next_child(upm_np, NULL); > > if (!flash_np) > > return -ENODEV; > > + fun->chip.dn = flash_np; > > > > fun->mtd.name = kasprintf(GFP_KERNEL, "0x%llx.%s", (u64)io_res->start, > > flash_np->name); > > diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c > > index 3f24b587304f..b701c3f23da1 100644 > > --- a/drivers/mtd/nand/nand_base.c > > +++ b/drivers/mtd/nand/nand_base.c > > @@ -48,6 +48,7 @@ > > #include <linux/leds.h> > > #include <linux/io.h> > > #include <linux/mtd/partitions.h> > > +#include <linux/of_mtd.h> > > > > /* Define default oob placement schemes for large and small page devices */ > > static struct nand_ecclayout nand_oob_8 = { > > @@ -3780,6 +3781,33 @@ ident_done: > > return type; > > } > > > > +static int nand_dt_init(struct mtd_info *mtd, struct nand_chip *chip, > > + struct device_node *dn) > > Assuming that of_mtd.c is the best place to put this, the > prototype could be simplified a little: > > int of_nand_chip_init(struct device_node *dn, struct nand_chip *chip) Though I don't agree about of_mtd.c, dropping the 'mtd' argument is possible. (In fact, we can just use a single 'chip' argument if we want.) I was just borrowing the style of the rest of nand_base.c, which often includes both 'mtd' and 'chip'. > > +{ > > + int ecc_mode, ecc_strength, ecc_step; > > + > > + if (of_get_nand_bus_width(dn) == 16) > > + chip->options |= NAND_BUSWIDTH_16; > > + > > + if (of_get_nand_on_flash_bbt(dn)) > > + chip->bbt_options |= NAND_BBT_USE_FLASH; > > + > > + ecc_mode = of_get_nand_ecc_mode(dn); > > + ecc_strength = of_get_nand_ecc_strength(dn); > > + ecc_step = of_get_nand_ecc_step_size(dn); > > + > > + if (ecc_mode >= 0) > > + chip->ecc.mode = ecc_mode; > > + > > + if (ecc_strength >= 0) > > + chip->ecc.strength = ecc_strength; > > + > > + if (ecc_step > 0) > > + chip->ecc.size = ecc_step; > > + > > + return 0; > > +} > > + > > /** > > * nand_scan_ident - [NAND Interface] Scan for the NAND device > > * @mtd: MTD device structure > > @@ -3797,6 +3825,13 @@ int nand_scan_ident(struct mtd_info *mtd, int > > maxchips, > > int i, nand_maf_id, nand_dev_id; > > struct nand_chip *chip = mtd->priv; > > struct nand_flash_dev *type; > > + int ret; > > + > > + if (chip->dn) { > > + ret = nand_dt_init(mtd, chip, chip->dn); > > + if (ret) > > + return ret; > > + } > > > > /* Set the default functions */ > > nand_set_defaults(chip, chip->options & NAND_BUSWIDTH_16); > > diff --git a/include/linux/mtd/nand.h b/include/linux/mtd/nand.h > > index 3d4ea7eb2b68..e0f40e12a2c8 100644 > > --- a/include/linux/mtd/nand.h > > +++ b/include/linux/mtd/nand.h > > @@ -26,6 +26,8 @@ > > > > struct mtd_info; > > struct nand_flash_dev; > > +struct device_node; > > + > > /* Scan and identify a NAND device */ > > extern int nand_scan(struct mtd_info *mtd, int max_chips); > > /* > > @@ -542,6 +544,7 @@ struct nand_buffers { > > * flash device > > * @IO_ADDR_W: [BOARDSPECIFIC] address to write the 8 I/O lines of the > > * flash device. > > + * @dn: [BOARDSPECIFIC] device node describing this instance > > * @read_byte: [REPLACEABLE] read one byte from the chip > > * @read_word: [REPLACEABLE] read one word from the chip > > * @write_byte: [REPLACEABLE] write a single byte to the chip on the > > @@ -644,6 +647,8 @@ struct nand_chip { > > void __iomem *IO_ADDR_R; > > void __iomem *IO_ADDR_W; > > > > + struct device_node *dn; > > + > > This addition to struct nand_chip wouldn't be needed, then. > > > uint8_t (*read_byte)(struct mtd_info *mtd); > > u16 (*read_word)(struct mtd_info *mtd); > > void (*write_byte)(struct mtd_info *mtd, uint8_t byte); > > > > You hinted at implementing stronger error checking. If we went > this route, would it make sense to only error check the software > ECC modes? Yes. I just elided some of the details for now, since it's not actually necessary to do some of it (many other drivers can use SW ECC without the extra error checks). Thanks, Brian -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 2/2 v3 RESEND] mtd: fsl_upm: Support NAND ECC DTS properties @ 2015-01-29 1:20 ` Brian Norris 0 siblings, 0 replies; 18+ messages in thread From: Brian Norris @ 2015-01-29 1:20 UTC (permalink / raw) To: Aaron Sierra; +Cc: linux-mtd, David Woodhouse, Ezequiel Garcia, devicetree On Wed, Jan 28, 2015 at 06:37:36PM -0600, Aaron Sierra wrote: > ----- Original Message ----- > > From: "Brian Norris" <computersforpeace@gmail.com> > > I was thinking about this a bit more, and it seems like we could really > > just factor this all into the core nand_base code with something like > > the following patch. It could possibly use some smarter logic to rule > > out certain combinations (but some of those are already caught in > > nand_scan_tail() anyway). What do you think? > > Brian, > If the NAND device tree property fetching were moved out of fsl_upm, > I think it should not be called within nand_scan(). I think that > it's imperative that each driver be able to access these properties > before handing off to nand_scan(), since there are hardware ECC > modes that only drivers will know how to error check. That's why nand_scan() is broken into nand_scan_ident() and nand_scan_tail() functions which can be called individually. This allows drivers to do the up-front initialization in nand_scan_ident(), do their own error checking and handling of these parameters, and then call nand_scan_tail(). See, for example, drivers/mtd/nand/omap2.c. > Also, catching errors in nand_scan_tail() tends to result in BUG()s. Well, some of those can be changed. Feel free to propose changes. I'd prefer to make nand_scan_tail() play nicer than to compensate in individual drivers. > That said, this could be useful as a publicly exported function that > individual drivers are responsible for calling (maybe in of_mtd.c). I don't think of_mtd.c should really contain a lot of mtd_info / nand_chip knowledge, if we can avoid it. I really do think that the nand_scan() option is a better idea, if we can work out the other details (BUG(), error checking, and keeping it flexible enough). I think it provides the best place to flesh out any other common DT handling for all NAND drivers. Related: I believe the question came up recently about how to support a generic DT binding for using a GPIO as a NAND write-protect line. This would be another candidate for handling transparently in nand_scan_ident() and would then immediately apply to all NAND drivers, not just those that were rewritten to call another specialized init function. I really don't want to encourage the anti-pattern of each driver reimplementing code that might as well be shared, if at all possible. Adding more decentralized helpers to of_mtd.c does not really help that cause. > I have a couple of additional comments inline below. > > > diff --git a/drivers/mtd/nand/fsl_upm.c b/drivers/mtd/nand/fsl_upm.c > > index 72755d7ec25d..fc4834946233 100644 > > --- a/drivers/mtd/nand/fsl_upm.c > > +++ b/drivers/mtd/nand/fsl_upm.c > > @@ -181,6 +181,7 @@ static int fun_chip_init(struct fsl_upm_nand *fun, > > flash_np = of_get_next_child(upm_np, NULL); > > if (!flash_np) > > return -ENODEV; > > + fun->chip.dn = flash_np; > > > > fun->mtd.name = kasprintf(GFP_KERNEL, "0x%llx.%s", (u64)io_res->start, > > flash_np->name); > > diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c > > index 3f24b587304f..b701c3f23da1 100644 > > --- a/drivers/mtd/nand/nand_base.c > > +++ b/drivers/mtd/nand/nand_base.c > > @@ -48,6 +48,7 @@ > > #include <linux/leds.h> > > #include <linux/io.h> > > #include <linux/mtd/partitions.h> > > +#include <linux/of_mtd.h> > > > > /* Define default oob placement schemes for large and small page devices */ > > static struct nand_ecclayout nand_oob_8 = { > > @@ -3780,6 +3781,33 @@ ident_done: > > return type; > > } > > > > +static int nand_dt_init(struct mtd_info *mtd, struct nand_chip *chip, > > + struct device_node *dn) > > Assuming that of_mtd.c is the best place to put this, the > prototype could be simplified a little: > > int of_nand_chip_init(struct device_node *dn, struct nand_chip *chip) Though I don't agree about of_mtd.c, dropping the 'mtd' argument is possible. (In fact, we can just use a single 'chip' argument if we want.) I was just borrowing the style of the rest of nand_base.c, which often includes both 'mtd' and 'chip'. > > +{ > > + int ecc_mode, ecc_strength, ecc_step; > > + > > + if (of_get_nand_bus_width(dn) == 16) > > + chip->options |= NAND_BUSWIDTH_16; > > + > > + if (of_get_nand_on_flash_bbt(dn)) > > + chip->bbt_options |= NAND_BBT_USE_FLASH; > > + > > + ecc_mode = of_get_nand_ecc_mode(dn); > > + ecc_strength = of_get_nand_ecc_strength(dn); > > + ecc_step = of_get_nand_ecc_step_size(dn); > > + > > + if (ecc_mode >= 0) > > + chip->ecc.mode = ecc_mode; > > + > > + if (ecc_strength >= 0) > > + chip->ecc.strength = ecc_strength; > > + > > + if (ecc_step > 0) > > + chip->ecc.size = ecc_step; > > + > > + return 0; > > +} > > + > > /** > > * nand_scan_ident - [NAND Interface] Scan for the NAND device > > * @mtd: MTD device structure > > @@ -3797,6 +3825,13 @@ int nand_scan_ident(struct mtd_info *mtd, int > > maxchips, > > int i, nand_maf_id, nand_dev_id; > > struct nand_chip *chip = mtd->priv; > > struct nand_flash_dev *type; > > + int ret; > > + > > + if (chip->dn) { > > + ret = nand_dt_init(mtd, chip, chip->dn); > > + if (ret) > > + return ret; > > + } > > > > /* Set the default functions */ > > nand_set_defaults(chip, chip->options & NAND_BUSWIDTH_16); > > diff --git a/include/linux/mtd/nand.h b/include/linux/mtd/nand.h > > index 3d4ea7eb2b68..e0f40e12a2c8 100644 > > --- a/include/linux/mtd/nand.h > > +++ b/include/linux/mtd/nand.h > > @@ -26,6 +26,8 @@ > > > > struct mtd_info; > > struct nand_flash_dev; > > +struct device_node; > > + > > /* Scan and identify a NAND device */ > > extern int nand_scan(struct mtd_info *mtd, int max_chips); > > /* > > @@ -542,6 +544,7 @@ struct nand_buffers { > > * flash device > > * @IO_ADDR_W: [BOARDSPECIFIC] address to write the 8 I/O lines of the > > * flash device. > > + * @dn: [BOARDSPECIFIC] device node describing this instance > > * @read_byte: [REPLACEABLE] read one byte from the chip > > * @read_word: [REPLACEABLE] read one word from the chip > > * @write_byte: [REPLACEABLE] write a single byte to the chip on the > > @@ -644,6 +647,8 @@ struct nand_chip { > > void __iomem *IO_ADDR_R; > > void __iomem *IO_ADDR_W; > > > > + struct device_node *dn; > > + > > This addition to struct nand_chip wouldn't be needed, then. > > > uint8_t (*read_byte)(struct mtd_info *mtd); > > u16 (*read_word)(struct mtd_info *mtd); > > void (*write_byte)(struct mtd_info *mtd, uint8_t byte); > > > > You hinted at implementing stronger error checking. If we went > this route, would it make sense to only error check the software > ECC modes? Yes. I just elided some of the details for now, since it's not actually necessary to do some of it (many other drivers can use SW ECC without the extra error checks). Thanks, Brian ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 2/2 v3 RESEND] mtd: fsl_upm: Support NAND ECC DTS properties 2015-01-29 1:20 ` Brian Norris @ 2015-01-29 16:40 ` Aaron Sierra -1 siblings, 0 replies; 18+ messages in thread From: Aaron Sierra @ 2015-01-29 16:40 UTC (permalink / raw) To: Brian Norris Cc: David Woodhouse, Ezequiel Garcia, linux-mtd-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, devicetree-u79uwXL29TY76Z2rM5mHXA ----- Original Message ----- > From: "Brian Norris" <computersforpeace-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> > Sent: Wednesday, January 28, 2015 7:20:42 PM > > On Wed, Jan 28, 2015 at 06:37:36PM -0600, Aaron Sierra wrote: > > ----- Original Message ----- > > > From: "Brian Norris" <computersforpeace-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> > > > > I was thinking about this a bit more, and it seems like we could really > > > just factor this all into the core nand_base code with something like > > > the following patch. It could possibly use some smarter logic to rule > > > out certain combinations (but some of those are already caught in > > > nand_scan_tail() anyway). What do you think? > > > > Brian, > > If the NAND device tree property fetching were moved out of fsl_upm, > > I think it should not be called within nand_scan(). I think that > > it's imperative that each driver be able to access these properties > > before handing off to nand_scan(), since there are hardware ECC > > modes that only drivers will know how to error check. > > That's why nand_scan() is broken into nand_scan_ident() and > nand_scan_tail() functions which can be called individually. This allows > drivers to do the up-front initialization in nand_scan_ident(), do their > own error checking and handling of these parameters, and then call > nand_scan_tail(). See, for example, drivers/mtd/nand/omap2.c. Thanks for pointing that out; I'll take a look. > > Also, catching errors in nand_scan_tail() tends to result in BUG()s. > > Well, some of those can be changed. Feel free to propose changes. I'd > prefer to make nand_scan_tail() play nicer than to compensate in > individual drivers. > > > That said, this could be useful as a publicly exported function that > > individual drivers are responsible for calling (maybe in of_mtd.c). > > I don't think of_mtd.c should really contain a lot of mtd_info / > nand_chip knowledge, if we can avoid it. > > I really do think that the nand_scan() option is a better idea, if we > can work out the other details (BUG(), error checking, and keeping it > flexible enough). I think it provides the best place to flesh out any > other common DT handling for all NAND drivers. > > Related: I believe the question came up recently about how to support a > generic DT binding for using a GPIO as a NAND write-protect line. This > would be another candidate for handling transparently in > nand_scan_ident() and would then immediately apply to all NAND drivers, > not just those that were rewritten to call another specialized init > function. > > I really don't want to encourage the anti-pattern of each driver > reimplementing code that might as well be shared, if at all possible. > Adding more decentralized helpers to of_mtd.c does not really help that > cause. Understood. [ snipped function prototype discussion ] > > You hinted at implementing stronger error checking. If we went > > this route, would it make sense to only error check the software > > ECC modes? > > Yes. I just elided some of the details for now, since it's not actually > necessary to do some of it (many other drivers can use SW ECC without > the extra error checks). OK, I'll rework the fsl_upm patch to work with your proposed patch. -Aaron -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 2/2 v3 RESEND] mtd: fsl_upm: Support NAND ECC DTS properties @ 2015-01-29 16:40 ` Aaron Sierra 0 siblings, 0 replies; 18+ messages in thread From: Aaron Sierra @ 2015-01-29 16:40 UTC (permalink / raw) To: Brian Norris; +Cc: linux-mtd, David Woodhouse, Ezequiel Garcia, devicetree ----- Original Message ----- > From: "Brian Norris" <computersforpeace@gmail.com> > Sent: Wednesday, January 28, 2015 7:20:42 PM > > On Wed, Jan 28, 2015 at 06:37:36PM -0600, Aaron Sierra wrote: > > ----- Original Message ----- > > > From: "Brian Norris" <computersforpeace@gmail.com> > > > > I was thinking about this a bit more, and it seems like we could really > > > just factor this all into the core nand_base code with something like > > > the following patch. It could possibly use some smarter logic to rule > > > out certain combinations (but some of those are already caught in > > > nand_scan_tail() anyway). What do you think? > > > > Brian, > > If the NAND device tree property fetching were moved out of fsl_upm, > > I think it should not be called within nand_scan(). I think that > > it's imperative that each driver be able to access these properties > > before handing off to nand_scan(), since there are hardware ECC > > modes that only drivers will know how to error check. > > That's why nand_scan() is broken into nand_scan_ident() and > nand_scan_tail() functions which can be called individually. This allows > drivers to do the up-front initialization in nand_scan_ident(), do their > own error checking and handling of these parameters, and then call > nand_scan_tail(). See, for example, drivers/mtd/nand/omap2.c. Thanks for pointing that out; I'll take a look. > > Also, catching errors in nand_scan_tail() tends to result in BUG()s. > > Well, some of those can be changed. Feel free to propose changes. I'd > prefer to make nand_scan_tail() play nicer than to compensate in > individual drivers. > > > That said, this could be useful as a publicly exported function that > > individual drivers are responsible for calling (maybe in of_mtd.c). > > I don't think of_mtd.c should really contain a lot of mtd_info / > nand_chip knowledge, if we can avoid it. > > I really do think that the nand_scan() option is a better idea, if we > can work out the other details (BUG(), error checking, and keeping it > flexible enough). I think it provides the best place to flesh out any > other common DT handling for all NAND drivers. > > Related: I believe the question came up recently about how to support a > generic DT binding for using a GPIO as a NAND write-protect line. This > would be another candidate for handling transparently in > nand_scan_ident() and would then immediately apply to all NAND drivers, > not just those that were rewritten to call another specialized init > function. > > I really don't want to encourage the anti-pattern of each driver > reimplementing code that might as well be shared, if at all possible. > Adding more decentralized helpers to of_mtd.c does not really help that > cause. Understood. [ snipped function prototype discussion ] > > You hinted at implementing stronger error checking. If we went > > this route, would it make sense to only error check the software > > ECC modes? > > Yes. I just elided some of the details for now, since it's not actually > necessary to do some of it (many other drivers can use SW ECC without > the extra error checks). OK, I'll rework the fsl_upm patch to work with your proposed patch. -Aaron ^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH] mtd: fsl_upm: Enable software BCH ECC support 2015-01-14 23:41 ` Aaron Sierra (?) (?) @ 2015-08-04 17:52 ` Aaron Sierra 2015-08-25 21:34 ` Brian Norris -1 siblings, 1 reply; 18+ messages in thread From: Aaron Sierra @ 2015-08-04 17:52 UTC (permalink / raw) To: Brian Norris, David Woodhouse, Ezequiel Garcia; +Cc: linux-mtd This patch preserves the default software ECC mode while adding the ability to use BCH ECC (as required for larger NAND devices). The BCH-required strength and step size values are pulled from the device-tree by nand_scan_ident(), so we replace nand_scan() with explicit calls to nand_scan_ident() and nand_scan_tail() in order to sanity check ECC properties from the device-tree. Tested-by: Ryan Schaefer <rschaefer@xes-inc.com> Signed-off-by: Jordan Friendshuh <jfriendshuh@xes-inc.com> Signed-off-by: Aaron Sierra <asierra@xes-inc.com> --- .../devicetree/bindings/mtd/fsl-upm-nand.txt | 32 ++++++++++++++++++++ drivers/mtd/nand/fsl_upm.c | 35 +++++++++++++++++++++- 2 files changed, 66 insertions(+), 1 deletion(-) diff --git a/Documentation/devicetree/bindings/mtd/fsl-upm-nand.txt b/Documentation/devicetree/bindings/mtd/fsl-upm-nand.txt index fce4894..3643ee1 100644 --- a/Documentation/devicetree/bindings/mtd/fsl-upm-nand.txt +++ b/Documentation/devicetree/bindings/mtd/fsl-upm-nand.txt @@ -18,6 +18,9 @@ Optional properties: - chip-delay : chip dependent delay for transferring data from array to read registers (tR). Required if property "gpios" is not used (R/B# pins not connected). +- nand-ecc-mode : as defined by nand.txt ("soft" and "soft_bch", only). +- nand-ecc-strength : as defined by nand.txt. +- nand-ecc-step-size : as defined by nand.txt. Each flash chip described may optionally contain additional sub-nodes describing partitions of the address space. See partition.txt for more @@ -65,3 +68,32 @@ upm@3,0 { }; }; }; + +/* + * Micron MT29F32G08AFABA (M62B) + * 32 Gb (4 GiB), 2 chipselect + */ +upm@2,0 { + #address-cells = <0>; + #size-cells = <0>; + compatible = "fsl,upm-nand"; + reg = <2 0x0 0x80000>; + fsl,upm-addr-line-cs-offsets = <0x0 0x10000>; + fsl,upm-addr-offset = <0x10>; + fsl,upm-cmd-offset = <0x08>; + fsl,upm-wait-flags = <0x1>; + chip-delay = <50>; + + nand@0 { + #address-cells = <1>; + #size-cells = <2>; + nand-ecc-mode = "soft_bch"; + nand-ecc-strength = <4>; + nand-ecc-step-size = <512>; + + partition@0 { + label = "NAND filesystem"; + reg = <0x0 0x1 0x00000000>; + }; + }; +}; diff --git a/drivers/mtd/nand/fsl_upm.c b/drivers/mtd/nand/fsl_upm.c index 72755d7..0982d7a 100644 --- a/drivers/mtd/nand/fsl_upm.c +++ b/drivers/mtd/nand/fsl_upm.c @@ -182,6 +182,7 @@ static int fun_chip_init(struct fsl_upm_nand *fun, if (!flash_np) return -ENODEV; + fun->chip.dn = flash_np; fun->mtd.name = kasprintf(GFP_KERNEL, "0x%llx.%s", (u64)io_res->start, flash_np->name); if (!fun->mtd.name) { @@ -189,7 +190,39 @@ static int fun_chip_init(struct fsl_upm_nand *fun, goto err; } - ret = nand_scan(&fun->mtd, fun->mchip_count); + ret = nand_scan_ident(&fun->mtd, fun->mchip_count, NULL); + if (ret) + goto err; + + switch (fun->chip.ecc.mode) { + case NAND_ECC_NONE: + fun->chip.ecc.mode = NAND_ECC_SOFT; + break; + case NAND_ECC_SOFT: + if (fun->chip.ecc.strength && fun->chip.ecc.strength != 1) + dev_warn(fun->dev, "Ignoring %d-bit software ECC\n", + fun->chip.ecc.strength); + if (fun->chip.ecc.size && + (fun->chip.ecc.size != 256) && + (fun->chip.ecc.size != 512)) { + dev_err(fun->dev, "Invalid software ECC step: %d\n", + fun->chip.ecc.size); + goto err; + } + break; + case NAND_ECC_SOFT_BCH: + if (fun->chip.ecc.strength < 2) { + dev_err(fun->dev, "Invalid BCH ECC strength: %d\n", + fun->chip.ecc.strength); + goto err; + } + break; + default: + dev_err(fun->dev, "ECC mode unsupported"); + goto err; + } + + ret = nand_scan_tail(&fun->mtd); if (ret) goto err; -- 1.9.1 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH] mtd: fsl_upm: Enable software BCH ECC support 2015-08-04 17:52 ` [PATCH] mtd: fsl_upm: Enable software BCH ECC support Aaron Sierra @ 2015-08-25 21:34 ` Brian Norris 2015-08-26 16:22 ` Aaron Sierra 0 siblings, 1 reply; 18+ messages in thread From: Brian Norris @ 2015-08-25 21:34 UTC (permalink / raw) To: Aaron Sierra; +Cc: David Woodhouse, Ezequiel Garcia, linux-mtd On Tue, Aug 04, 2015 at 12:52:54PM -0500, Aaron Sierra wrote: > This patch preserves the default software ECC mode while adding the > ability to use BCH ECC (as required for larger NAND devices). > > The BCH-required strength and step size values are pulled from the > device-tree by nand_scan_ident(), so we replace nand_scan() with > explicit calls to nand_scan_ident() and nand_scan_tail() in order > to sanity check ECC properties from the device-tree. > > Tested-by: Ryan Schaefer <rschaefer@xes-inc.com> > Signed-off-by: Jordan Friendshuh <jfriendshuh@xes-inc.com> > Signed-off-by: Aaron Sierra <asierra@xes-inc.com> > --- > .../devicetree/bindings/mtd/fsl-upm-nand.txt | 32 ++++++++++++++++++++ > drivers/mtd/nand/fsl_upm.c | 35 +++++++++++++++++++++- > 2 files changed, 66 insertions(+), 1 deletion(-) > > diff --git a/Documentation/devicetree/bindings/mtd/fsl-upm-nand.txt b/Documentation/devicetree/bindings/mtd/fsl-upm-nand.txt > index fce4894..3643ee1 100644 > --- a/Documentation/devicetree/bindings/mtd/fsl-upm-nand.txt > +++ b/Documentation/devicetree/bindings/mtd/fsl-upm-nand.txt > @@ -18,6 +18,9 @@ Optional properties: > - chip-delay : chip dependent delay for transferring data from array to > read registers (tR). Required if property "gpios" is not used > (R/B# pins not connected). > +- nand-ecc-mode : as defined by nand.txt ("soft" and "soft_bch", only). > +- nand-ecc-strength : as defined by nand.txt. > +- nand-ecc-step-size : as defined by nand.txt. These three properties go under the flash node (which is not yet documented, though it's mentioned in example and implementation), not the controller node. Can you add a separate paragraph/section to describe the flash node, and put these properties under that? > > Each flash chip described may optionally contain additional sub-nodes > describing partitions of the address space. See partition.txt for more > @@ -65,3 +68,32 @@ upm@3,0 { > }; > }; > }; > + > +/* > + * Micron MT29F32G08AFABA (M62B) > + * 32 Gb (4 GiB), 2 chipselect > + */ > +upm@2,0 { > + #address-cells = <0>; > + #size-cells = <0>; > + compatible = "fsl,upm-nand"; > + reg = <2 0x0 0x80000>; > + fsl,upm-addr-line-cs-offsets = <0x0 0x10000>; > + fsl,upm-addr-offset = <0x10>; > + fsl,upm-cmd-offset = <0x08>; > + fsl,upm-wait-flags = <0x1>; > + chip-delay = <50>; > + > + nand@0 { > + #address-cells = <1>; > + #size-cells = <2>; > + nand-ecc-mode = "soft_bch"; > + nand-ecc-strength = <4>; > + nand-ecc-step-size = <512>; > + > + partition@0 { > + label = "NAND filesystem"; > + reg = <0x0 0x1 0x00000000>; > + }; > + }; > +}; > diff --git a/drivers/mtd/nand/fsl_upm.c b/drivers/mtd/nand/fsl_upm.c > index 72755d7..0982d7a 100644 > --- a/drivers/mtd/nand/fsl_upm.c > +++ b/drivers/mtd/nand/fsl_upm.c > @@ -182,6 +182,7 @@ static int fun_chip_init(struct fsl_upm_nand *fun, > if (!flash_np) > return -ENODEV; > > + fun->chip.dn = flash_np; > fun->mtd.name = kasprintf(GFP_KERNEL, "0x%llx.%s", (u64)io_res->start, > flash_np->name); > if (!fun->mtd.name) { > @@ -189,7 +190,39 @@ static int fun_chip_init(struct fsl_upm_nand *fun, > goto err; > } > > - ret = nand_scan(&fun->mtd, fun->mchip_count); > + ret = nand_scan_ident(&fun->mtd, fun->mchip_count, NULL); > + if (ret) > + goto err; > + > + switch (fun->chip.ecc.mode) { > + case NAND_ECC_NONE: Maybe a comment here, to say that we default to soft for legacy reasons? "NONE" is actually a potentially valid option (but not likely or useful). > + fun->chip.ecc.mode = NAND_ECC_SOFT; > + break; > + case NAND_ECC_SOFT: > + if (fun->chip.ecc.strength && fun->chip.ecc.strength != 1) > + dev_warn(fun->dev, "Ignoring %d-bit software ECC\n", > + fun->chip.ecc.strength); Do you really want to ignore this? I think it's fair to make this an error case in nand_scan_tail(). Like: case NAND_ECC_SOFT: ... if (chip->ecc.strength && chip->ecc.strength != 1) { // error out // we really need to kill the heavy usage of // BUG() in nand_scan_tail()... } > + if (fun->chip.ecc.size && > + (fun->chip.ecc.size != 256) && > + (fun->chip.ecc.size != 512)) { > + dev_err(fun->dev, "Invalid software ECC step: %d\n", > + fun->chip.ecc.size); Is that a generic soft ECC limitation? (Looks like it only supports 256 and 512 to me.) If so, then let's put this in nand_base.c. Either in nand_dt_init(), or possibly in nand_scan_tail(), under the case for NAND_ECC_SOFT. > + goto err; > + } > + break; > + case NAND_ECC_SOFT_BCH: > + if (fun->chip.ecc.strength < 2) { > + dev_err(fun->dev, "Invalid BCH ECC strength: %d\n", > + fun->chip.ecc.strength); Same here. Maybe in nand_scan_tail()? > + goto err; > + } > + break; > + default: > + dev_err(fun->dev, "ECC mode unsupported"); > + goto err; > + } > + > + ret = nand_scan_tail(&fun->mtd); > if (ret) > goto err; > Brian ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] mtd: fsl_upm: Enable software BCH ECC support 2015-08-25 21:34 ` Brian Norris @ 2015-08-26 16:22 ` Aaron Sierra 2015-08-26 17:59 ` Brian Norris 0 siblings, 1 reply; 18+ messages in thread From: Aaron Sierra @ 2015-08-26 16:22 UTC (permalink / raw) To: Brian Norris; +Cc: David Woodhouse, Ezequiel Garcia, linux-mtd ----- Original Message ----- > From: "Brian Norris" <computersforpeace@gmail.com> > Sent: Tuesday, August 25, 2015 4:34:08 PM [snip] > > --- a/Documentation/devicetree/bindings/mtd/fsl-upm-nand.txt > > +++ b/Documentation/devicetree/bindings/mtd/fsl-upm-nand.txt > > @@ -18,6 +18,9 @@ Optional properties: > > - chip-delay : chip dependent delay for transferring data from array to > > read registers (tR). Required if property "gpios" is not used > > (R/B# pins not connected). > > +- nand-ecc-mode : as defined by nand.txt ("soft" and "soft_bch", only). > > +- nand-ecc-strength : as defined by nand.txt. > > +- nand-ecc-step-size : as defined by nand.txt. > > These three properties go under the flash node (which is not yet > documented, though it's mentioned in example and implementation), not > the controller node. Can you add a separate paragraph/section to > describe the flash node, and put these properties under that? Sure thing. [snip] > > --- a/drivers/mtd/nand/fsl_upm.c > > +++ b/drivers/mtd/nand/fsl_upm.c > > @@ -182,6 +182,7 @@ static int fun_chip_init(struct fsl_upm_nand *fun, > > if (!flash_np) > > return -ENODEV; > > > > + fun->chip.dn = flash_np; > > fun->mtd.name = kasprintf(GFP_KERNEL, "0x%llx.%s", (u64)io_res->start, > > flash_np->name); > > if (!fun->mtd.name) { > > @@ -189,7 +190,39 @@ static int fun_chip_init(struct fsl_upm_nand *fun, > > goto err; > > } > > > > - ret = nand_scan(&fun->mtd, fun->mchip_count); > > + ret = nand_scan_ident(&fun->mtd, fun->mchip_count, NULL); > > + if (ret) > > + goto err; > > + > > + switch (fun->chip.ecc.mode) { > > + case NAND_ECC_NONE: > > Maybe a comment here, to say that we default to soft for legacy reasons? > "NONE" is actually a potentially valid option (but not likely or > useful). OK > > + fun->chip.ecc.mode = NAND_ECC_SOFT; > > + break; > > + case NAND_ECC_SOFT: > > + if (fun->chip.ecc.strength && fun->chip.ecc.strength != 1) > > + dev_warn(fun->dev, "Ignoring %d-bit software ECC\n", > > + fun->chip.ecc.strength); > > Do you really want to ignore this? I think it's fair to make this an > error case in nand_scan_tail(). Like: > > case NAND_ECC_SOFT: > ... > if (chip->ecc.strength && chip->ecc.strength != 1) { > // error out > // we really need to kill the heavy usage of > // BUG() in nand_scan_tail()... > } My rationale was that for "soft" ECC mode, ecc.strength is really a don't-care property; the driver doesn't use it since the algorithm only supports 1-bit. Therefore it could be OK to output a warning and carry on. I can see the other side, too, where we might want all of the user-supplied values to agree with the capabilities of the driver. If I rework this, I would intend to accept zeros for ecc.strength and ecc.size as valid for this mode (size defaults to 256 within nand_ecc.c). Does that sit well with you? > > > + if (fun->chip.ecc.size && > > + (fun->chip.ecc.size != 256) && > > + (fun->chip.ecc.size != 512)) { > > + dev_err(fun->dev, "Invalid software ECC step: %d\n", > > + fun->chip.ecc.size); > > Is that a generic soft ECC limitation? (Looks like it only supports 256 > and 512 to me.) If so, then let's put this in nand_base.c. Either in > nand_dt_init(), or possibly in nand_scan_tail(), under the case for > NAND_ECC_SOFT. Yes, it is. OK. > > + goto err; > > + } > > + break; > > + case NAND_ECC_SOFT_BCH: > > + if (fun->chip.ecc.strength < 2) { > > + dev_err(fun->dev, "Invalid BCH ECC strength: %d\n", > > + fun->chip.ecc.strength); > > Same here. Maybe in nand_scan_tail()? OK -Aaron ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] mtd: fsl_upm: Enable software BCH ECC support 2015-08-26 16:22 ` Aaron Sierra @ 2015-08-26 17:59 ` Brian Norris 0 siblings, 0 replies; 18+ messages in thread From: Brian Norris @ 2015-08-26 17:59 UTC (permalink / raw) To: Aaron Sierra; +Cc: David Woodhouse, Ezequiel Garcia, linux-mtd On Wed, Aug 26, 2015 at 11:22:18AM -0500, Aaron Sierra wrote: > ----- Original Message ----- > > From: "Brian Norris" <computersforpeace@gmail.com> > > Sent: Tuesday, August 25, 2015 4:34:08 PM > > > + case NAND_ECC_SOFT: > > > + if (fun->chip.ecc.strength && fun->chip.ecc.strength != 1) > > > + dev_warn(fun->dev, "Ignoring %d-bit software ECC\n", > > > + fun->chip.ecc.strength); > > > > Do you really want to ignore this? I think it's fair to make this an > > error case in nand_scan_tail(). Like: > > > > case NAND_ECC_SOFT: > > ... > > if (chip->ecc.strength && chip->ecc.strength != 1) { > > // error out > > // we really need to kill the heavy usage of > > // BUG() in nand_scan_tail()... > > } > > My rationale was that for "soft" ECC mode, ecc.strength is really a > don't-care property; the driver doesn't use it since the algorithm only > supports 1-bit. Therefore it could be OK to output a warning and carry on. Understood. > I can see the other side, too, where we might want all of the > user-supplied values to agree with the capabilities of the driver. Right. I especially would want to keep sanity on the device tree, as this is something closer to an ABI (depending on who you ask...) than internal driver-provided values. > If I rework this, I would intend to accept zeros for ecc.strength and > ecc.size as valid for this mode (size defaults to 256 within nand_ecc.c). > Does that sit well with you? Sure, that's fair. We've been allowing this in nand_scan_tail() already (we tend to fill in reasonable defaults if the driver doesn't specify), so I don't want to remove that fallback. I just don't want people specifying 4-bit hamming ECC in their DT and not noticing your little warning log message saying that we're ignoring them and using 1-bit. But the DT binding doc should not suggest that they can be left at 0. If you're up for it, perhaps you can add some details in Documentation/devicetree/bindings/mtd/nand.txt about the "soft" and "soft_bch" ECC modes while you're at it. Thanks, Brian ^ permalink raw reply [flat|nested] 18+ messages in thread
end of thread, other threads:[~2015-08-26 17:59 UTC | newest] Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- [not found] <993430213.110699.1421109005544.JavaMail.zimbra@xes-inc.com> [not found] ` <993430213.110699.1421109005544.JavaMail.zimbra-AQeFf1F/bRxBDgjK7y7TUQ@public.gmane.org> 2015-01-13 0:36 ` [PATCH 2/2 v3] mtd: fsl_upm: Support NAND ECC DTS properties Aaron Sierra 2015-01-13 0:36 ` Aaron Sierra 2015-01-14 23:41 ` [PATCH 2/2 v3 RESEND] " Aaron Sierra 2015-01-14 23:41 ` Aaron Sierra [not found] ` <447095612.147126.1421278909058.JavaMail.zimbra-AQeFf1F/bRxBDgjK7y7TUQ@public.gmane.org> 2015-01-23 7:43 ` Brian Norris 2015-01-23 7:43 ` Brian Norris 2015-01-23 8:30 ` Brian Norris 2015-01-23 8:30 ` Brian Norris 2015-01-29 0:37 ` Aaron Sierra 2015-01-29 0:37 ` Aaron Sierra [not found] ` <1309767342.94086.1422491856685.JavaMail.zimbra-AQeFf1F/bRxBDgjK7y7TUQ@public.gmane.org> 2015-01-29 1:20 ` Brian Norris 2015-01-29 1:20 ` Brian Norris 2015-01-29 16:40 ` Aaron Sierra 2015-01-29 16:40 ` Aaron Sierra 2015-08-04 17:52 ` [PATCH] mtd: fsl_upm: Enable software BCH ECC support Aaron Sierra 2015-08-25 21:34 ` Brian Norris 2015-08-26 16:22 ` Aaron Sierra 2015-08-26 17:59 ` Brian Norris
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.