RE: [PATCH 2/4] mtd: devices: elm: Add support for ELM error correction
On Tue, Oct 16, 2012 at 01:10:47, Peter Korsgaard wrote: > > Philip, Avinash writes: > > > Platforms containing the ELM module can be used to correct errors > > reported by BCH 4, 8 & 16 bit ECC scheme. For now only 4 & 8 bit > > support is added. > > This sounds odd to me. What about something like: > > The ELM hardware module can be used to speedup BCH 4/8/16 ECC scheme > error correction. > > For now only 4 & 8 bit support is added. Ok I will correct it. > > > > +++ b/drivers/mtd/devices/Makefile > > @@ -17,8 +17,10 @@ obj-$(CONFIG_MTD_LART) += lart.o > > obj-$(CONFIG_MTD_BLOCK2MTD) += block2mtd.o > > obj-$(CONFIG_MTD_DATAFLASH) += mtd_dataflash.o > > obj-$(CONFIG_MTD_M25P80) += m25p80.o > > +obj-$(CONFIG_MTD_NAND_OMAP2) += elm.o > > You seem to only use it in 4/4 if CONFIG_MTD_NAND_OMAP_BCH is set, so it > probably makes more sense to use that symbol to not needlessly include > it if it won't be used. Ok. This been good. > > > > +static void elm_write_reg(void *offset, u32 val) > > +{ > > + writel(val, offset); > > +} > > + > > +static u32 elm_read_reg(void *offset) > > +{ > > + return readl(offset); > > +} > > As written these read/write wrappers don't add anything. How about > passing struct elm_info and offset as an integer so you can drop > elm_base from all call sites, E.G.: > > static void elm_write_reg(struct elm_info *info, int offset, u32 val) > { > writel(val, info->elm_base + offset); > } > Ok, this helps to reduce some indentation levels also. > > > +void elm_decode_bch_error_page(struct device *dev, u8 *ecc_calc, > > + struct elm_errorvec *err_vec) > > +{ > > + int i; > > + u8 syndrome[BCH_MAX_ECC_BYTES_PER_SECTOR] = {0}, *syn_p; > > > Why do you need to keep the entire syndrome around? You seem to only use > it between elm_reverse_eccdata() and elm_load_syndrome(), so it could as > well be BCH8_ECC_OOB_BYTES long (or rather a multiple of sizeof(u32). > > It would also be good to do the shuffeling directly in elm_load_syndrome > so you don't need the extra copy. I will check. > > > > + */ > > + > > +#ifndef __ELM_H > > +#define __ELM_H > > + > > +enum bch_ecc { > > + BCH4_ECC = 0, > > + BCH8_ECC, > > + BCH16_ECC, > > It probably makes more sense to not provide the enum value for BCH16 as > you don't support it. > Ok I will remove. > > > +}; > > + Thanks Avinash -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/4] mtd: devices: elm: Add support for ELM error correction
> Philip, Avinash writes: > Platforms containing the ELM module can be used to correct errors > reported by BCH 4, 8 & 16 bit ECC scheme. For now only 4 & 8 bit > support is added. This sounds odd to me. What about something like: The ELM hardware module can be used to speedup BCH 4/8/16 ECC scheme error correction. For now only 4 & 8 bit support is added. > +++ b/drivers/mtd/devices/Makefile > @@ -17,8 +17,10 @@ obj-$(CONFIG_MTD_LART)+= lart.o > obj-$(CONFIG_MTD_BLOCK2MTD) += block2mtd.o > obj-$(CONFIG_MTD_DATAFLASH) += mtd_dataflash.o > obj-$(CONFIG_MTD_M25P80)+= m25p80.o > +obj-$(CONFIG_MTD_NAND_OMAP2)+= elm.o You seem to only use it in 4/4 if CONFIG_MTD_NAND_OMAP_BCH is set, so it probably makes more sense to use that symbol to not needlessly include it if it won't be used. > +++ b/drivers/mtd/devices/elm.c > @@ -0,0 +1,440 @@ > +/* > + * Error Location Module > + * > + * Copyright (C) 2012 Texas Instruments Incorporated - http://www.ti.com/ > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License as published by > + * the Free Software Foundation; either version 2 of the License, or > + * (at your option) any later version. > + * > + * This program is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. > + * > + */ > + > +#include > +#include > +#include > +#include > +#include > +#include > +#include > + > +#define ELM_IRQSTATUS 0x018 > +#define ELM_IRQENABLE 0x01c > +#define ELM_LOCATION_CONFIG 0x020 > +#define ELM_PAGE_CTRL 0x080 > +#define ELM_SYNDROME_FRAGMENT_0 0x400 > +#define ELM_SYNDROME_FRAGMENT_6 0x418 > +#define ELM_LOCATION_STATUS 0x800 > +#define ELM_ERROR_LOCATION_00x880 > + > +/* ELM Interrupt Status Register */ > +#define INTR_STATUS_PAGE_VALID BIT(8) > + > +/* ELM Interrupt Enable Register */ > +#define INTR_EN_PAGE_MASK BIT(8) > + > +/* ELM Location Configuration Register */ > +#define ECC_BCH_LEVEL_MASK 0x3 > + > +/* ELM syndrome */ > +#define ELM_SYNDROME_VALID BIT(16) > + > +/* ELM_LOCATION_STATUS Register */ > +#define ECC_CORRECTABLE_MASKBIT(8) > +#define ECC_NB_ERRORS_MASK 0x1f > + > +/* ELM_ERROR_LOCATION_0-15 Registers */ > +#define ECC_ERROR_LOCATION_MASK 0x1fff > + > +#define ELM_ECC_SIZE0x7ff > + > +#define SYNDROME_FRAGMENT_REG_SIZE 0x40 > +#define ERROR_LOCATION_SIZE 0x100 > +#define MAX_BCH_ELM_ERROR 16 > +#define ELM_FRAGMENT_REG7 > + > +typedef u32 syn_t[ELM_FRAGMENT_REG]; > +typedef u32 elm_error_t[MAX_BCH_ELM_ERROR]; > + > +struct elm_info { > +struct device *dev; > +void __iomem *elm_base; > +struct completion elm_completion; > +struct list_head list; > +enum bch_ecc bch_type; > +}; > + > +static LIST_HEAD(elm_devices); > + > +static void elm_write_reg(void *offset, u32 val) > +{ > +writel(val, offset); > +} > + > +static u32 elm_read_reg(void *offset) > +{ > +return readl(offset); > +} As written these read/write wrappers don't add anything. How about passing struct elm_info and offset as an integer so you can drop elm_base from all call sites, E.G.: static void elm_write_reg(struct elm_info *info, int offset, u32 val) { writel(val, info->elm_base + offset); } > + > +/** > + * elm_config - Configure ELM module > + * @info: elm info > + */ > +static void elm_config(struct elm_info *info) > +{ > +u32 reg_val; > + > +reg_val = (info->bch_type & ECC_BCH_LEVEL_MASK) | (ELM_ECC_SIZE << 16); > +elm_write_reg(info->elm_base + ELM_LOCATION_CONFIG, reg_val); > +} > + > +/** > + * elm_configure_page_mode - Enable/Disable page mode > + * @info: elm info > + * @index: index number of syndrome fragment vector > + * @enable: enable/disable flag for page mode > + * > + * Enable page mode for syndrome fragment index > + */ > +static void elm_configure_page_mode(struct elm_info *info, int index, > +bool enable) > +{ > +u32 reg_val; > + > +reg_val = elm_read_reg(info->elm_base + ELM_PAGE_CTRL); > +if (enable) > +reg_val |= BIT(index); /* enable page mode */ > +else > +reg_val &= ~BIT(index); /* disable page mode */ > + > +elm_write_reg(info->elm_base + ELM_PAGE_CTRL, reg_val); > +} > + > +static void rearrange_bch4(u8 *syndrome) > +{ > +/* > + * BCH4 has 52 bit used for ecc, but OOB stored with > + * nibble 0 appended, removes appended 0 nibble > + */ > +u64 *
RE: [PATCH 2/4] mtd: devices: elm: Add support for ELM error correction
On Wed, Oct 03, 2012 at 20:40:46, Peter Meerwald wrote: > > some minor nitpicks below > > > + * > > + * On completion of processing by elm module, error location status > > + * register updated with correctable/uncorrectable error information. > > + * In case of correctable errors, number of errors located from > > + * elm location status register & read the these many positions from > > should probably be: "... & read the positions from ..."? Ok I will correct it. > > > + * elm error location register. > > + */ > > +static void elm_error_correction(struct elm_info *info, > > + struct elm_errorvec *err_vec) > > +{ > > + int i, j, errors = 0; > > + void *err_loc_base = info->elm_base + ELM_ERROR_LOCATION_0; > > + elm_error_t *err_loc; > > + void *offset; > > + u32 reg_val; > > + > > + for (i = 0; i < ERROR_VECTOR_MAX; i++) { > > + /* check error reported */ > > + if (err_vec[i].error_reported) { > > + offset = info->elm_base + ELM_LOCATION_STATUS + > > + i * ERROR_LOCATION_SIZE; > > + reg_val = elm_read_reg(offset); > > + /* check correctable error or not */ > > + if (reg_val & ECC_CORRECTABLE_MASK) { > > + err_loc = err_loc_base + > > + i * ERROR_LOCATION_SIZE; > > + /* read count of correctable errors */ > > + err_vec[i].error_count = reg_val & > > + ECC_NB_ERRORS_MASK; > > + > > + /* update the error locations in error vector */ > > + for (j = 0; j < err_vec[i].error_count; j++) { > > + > > + reg_val = elm_read_reg(*err_loc + j); > > + err_vec[i].error_loc[j] = reg_val & > > + ECC_ERROR_LOCATION_MASK; > > + } > > + > > + errors += err_vec[i].error_count; > > + } else { > > + err_vec[i].error_uncorrectable++; > > + } > > extra braces above As per coding style This does not apply if only one branch of a conditional statement is a single statement; in the latter case use braces in both branches: > > > + > > + /* clearing interrupts for processed error vectors */ > > + elm_write_reg(info->elm_base + ELM_IRQSTATUS, BIT(i)); > > + > > + /* disable page mode */ > > + elm_configure_page_mode(info, i, false); > > + } > > + } > > + > > + return; > > +} > > + > > +/** > > + * elm_decode_bch_error_page - Locate error position > > + * @info: elm info > > should be dev, not info Ok I will correct it. > > > + * @ecc_calc: calculated ECC bytes from GPMC > > + * @err_vec: elm error vectors > > + * > > + * Called with one or greater reported and is vectors with error reported > > + * is updated in err_vec[].error_reported > > + */ > > I do not understand the statement "Called with one ..." elm_decode_bch_error_page() API will called from nand driver if and only if errors are present while reading page. Errors can be reported in one or multiple error vectors. I will reword it as. Called with one or more error reported vectors & vectors with error reported is updated in err_vec[].error_reported > [snip] > > + > > +#define BCH8_ECC_OOB_BYTES 13 > > +/* RBL requires 14 byte even though BCH8 uses only 13 byte */ > > not sure what RBL is? RBL stands for Rom boot Loader > > > +#define BCH8_SIZE (BCH8_ECC_OOB_BYTES + 1) > > +#define BCH4_SIZE 7 > > + > > +#defineBCH8_SYNDROME_SIZE 4 /* 13 bytes of ecc */ > > +#defineBCH4_SYNDROME_SIZE 2 /* 7 bytes of ecc */ > > + > > +/** > > + * struct elm_errorvec - error vector for elm > > + * @error_reported:set true for vectors error is reported > > + * > > + * @error_count: number of correctable errors in the sector > > + * @error_uncorrectable: number of uncorrectable errors > > + * @error_loc: buffer for error location > > + * > > + */ > > +struct elm_errorvec { > > + bool error_reported; > > + int error_count; > > + int error_uncorrectable; > > + int error_loc[ERROR_VECTOR_MAX]; > > +}; > > + > > +void elm_decode_bch_error_page(struct device *dev, u8 *ecc_calc, > > + struct elm_errorvec *err_vec); > > +struct device *elm_request(enum bch_ecc bch_type); > > +#endif /* __ELM_H */ > > > > -- > > Peter Meerwald > +43-664-218 (mobile) > -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/4] mtd: devices: elm: Add support for ELM error correction
some minor nitpicks below > Platforms containing the ELM module can be used to correct errors > reported by BCH 4, 8 & 16 bit ECC scheme. For now only 4 & 8 bit > support is added. > > Signed-off-by: Philip, Avinash > Cc: Grant Likely > Cc: Rob Herring > Cc: Rob Landley > --- > :00 100644 000... b88ee83... A > Documentation/devicetree/bindings/mtd/elm.txt > :100644 100644 395733a... 0f6a94b... Mdrivers/mtd/devices/Makefile > :00 100644 000... 802b572... Adrivers/mtd/devices/elm.c > :00 100644 000... eb53163... A > include/linux/platform_data/elm.h > Documentation/devicetree/bindings/mtd/elm.txt | 18 + > drivers/mtd/devices/Makefile |4 +- > drivers/mtd/devices/elm.c | 440 > + > include/linux/platform_data/elm.h | 60 > 4 files changed, 521 insertions(+), 1 deletions(-) > > diff --git a/Documentation/devicetree/bindings/mtd/elm.txt > b/Documentation/devicetree/bindings/mtd/elm.txt > new file mode 100644 > index 000..b88ee83 > --- /dev/null > +++ b/Documentation/devicetree/bindings/mtd/elm.txt > @@ -0,0 +1,18 @@ > +Error location module > + > +Required properties: > +- compatible: Must be "ti,elm" > +- reg: physical base address and size of the registers map. > +- interrupts: Interrupt number for the elm. > +- interrupt-parent: The parent interrupt controller > + > +Optional properties: > +- ti,hwmods: Name of the hwmod associated to the elm > + > +Example: > +elm: elm@0 { > + compatible = "ti,elm"; > + reg = <0x4808 0x2000>; > + interrupt-parent = <&intc>; > + interrupts = <4>; > +}; > diff --git a/drivers/mtd/devices/Makefile b/drivers/mtd/devices/Makefile > index 395733a..0f6a94b 100644 > --- a/drivers/mtd/devices/Makefile > +++ b/drivers/mtd/devices/Makefile > @@ -17,8 +17,10 @@ obj-$(CONFIG_MTD_LART) += lart.o > obj-$(CONFIG_MTD_BLOCK2MTD) += block2mtd.o > obj-$(CONFIG_MTD_DATAFLASH) += mtd_dataflash.o > obj-$(CONFIG_MTD_M25P80) += m25p80.o > +obj-$(CONFIG_MTD_NAND_OMAP2) += elm.o > obj-$(CONFIG_MTD_SPEAR_SMI) += spear_smi.o > obj-$(CONFIG_MTD_SST25L) += sst25l.o > obj-$(CONFIG_MTD_BCM47XXSFLASH) += bcm47xxsflash.o > > -CFLAGS_docg3.o += -I$(src) > \ No newline at end of file > + > +CFLAGS_docg3.o += -I$(src) > diff --git a/drivers/mtd/devices/elm.c b/drivers/mtd/devices/elm.c > new file mode 100644 > index 000..802b572 > --- /dev/null > +++ b/drivers/mtd/devices/elm.c > @@ -0,0 +1,440 @@ > +/* > + * Error Location Module > + * > + * Copyright (C) 2012 Texas Instruments Incorporated - http://www.ti.com/ > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License as published by > + * the Free Software Foundation; either version 2 of the License, or > + * (at your option) any later version. > + * > + * This program is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. > + * > + */ > + > +#include > +#include > +#include > +#include > +#include > +#include > +#include > + > +#define ELM_IRQSTATUS0x018 > +#define ELM_IRQENABLE0x01c > +#define ELM_LOCATION_CONFIG 0x020 > +#define ELM_PAGE_CTRL0x080 > +#define ELM_SYNDROME_FRAGMENT_0 0x400 > +#define ELM_SYNDROME_FRAGMENT_6 0x418 > +#define ELM_LOCATION_STATUS 0x800 > +#define ELM_ERROR_LOCATION_0 0x880 > + > +/* ELM Interrupt Status Register */ > +#define INTR_STATUS_PAGE_VALID BIT(8) > + > +/* ELM Interrupt Enable Register */ > +#define INTR_EN_PAGE_MASKBIT(8) > + > +/* ELM Location Configuration Register */ > +#define ECC_BCH_LEVEL_MASK 0x3 > + > +/* ELM syndrome */ > +#define ELM_SYNDROME_VALID BIT(16) > + > +/* ELM_LOCATION_STATUS Register */ > +#define ECC_CORRECTABLE_MASK BIT(8) > +#define ECC_NB_ERRORS_MASK 0x1f > + > +/* ELM_ERROR_LOCATION_0-15 Registers */ > +#define ECC_ERROR_LOCATION_MASK 0x1fff > + > +#define ELM_ECC_SIZE 0x7ff > + > +#define SYNDROME_FRAGMENT_REG_SIZE 0x40 > +#define ERROR_LOCATION_SIZE 0x100 > +#define MAX_BCH_ELM_ERROR16 > +#define ELM_FRAGMENT_REG 7 > + > +typedef u32 syn_t[ELM_FRAGMENT_REG]; > +typedef u32 elm_error_t[MAX_BCH_ELM_ERROR]; > + > +struct elm_info { > + struct device *dev; > + void __iomem *elm_base; > + struct completion elm_completion; > + struct list_head list; > + enum bch_ecc bch_type; > +}; > + > +static LIST_HEAD(elm_devices); > + > +static void elm_write_reg(void *offset, u32 val) > +{ > + writel(val, o
[PATCH 2/4] mtd: devices: elm: Add support for ELM error correction
Platforms containing the ELM module can be used to correct errors reported by BCH 4, 8 & 16 bit ECC scheme. For now only 4 & 8 bit support is added. Signed-off-by: Philip, Avinash Cc: Grant Likely Cc: Rob Herring Cc: Rob Landley --- :00 100644 000... b88ee83... A Documentation/devicetree/bindings/mtd/elm.txt :100644 100644 395733a... 0f6a94b... M drivers/mtd/devices/Makefile :00 100644 000... 802b572... A drivers/mtd/devices/elm.c :00 100644 000... eb53163... A include/linux/platform_data/elm.h Documentation/devicetree/bindings/mtd/elm.txt | 18 + drivers/mtd/devices/Makefile |4 +- drivers/mtd/devices/elm.c | 440 + include/linux/platform_data/elm.h | 60 4 files changed, 521 insertions(+), 1 deletions(-) diff --git a/Documentation/devicetree/bindings/mtd/elm.txt b/Documentation/devicetree/bindings/mtd/elm.txt new file mode 100644 index 000..b88ee83 --- /dev/null +++ b/Documentation/devicetree/bindings/mtd/elm.txt @@ -0,0 +1,18 @@ +Error location module + +Required properties: +- compatible: Must be "ti,elm" +- reg: physical base address and size of the registers map. +- interrupts: Interrupt number for the elm. +- interrupt-parent: The parent interrupt controller + +Optional properties: +- ti,hwmods: Name of the hwmod associated to the elm + +Example: +elm: elm@0 { + compatible = "ti,elm"; + reg = <0x4808 0x2000>; + interrupt-parent = <&intc>; + interrupts = <4>; +}; diff --git a/drivers/mtd/devices/Makefile b/drivers/mtd/devices/Makefile index 395733a..0f6a94b 100644 --- a/drivers/mtd/devices/Makefile +++ b/drivers/mtd/devices/Makefile @@ -17,8 +17,10 @@ obj-$(CONFIG_MTD_LART) += lart.o obj-$(CONFIG_MTD_BLOCK2MTD)+= block2mtd.o obj-$(CONFIG_MTD_DATAFLASH)+= mtd_dataflash.o obj-$(CONFIG_MTD_M25P80) += m25p80.o +obj-$(CONFIG_MTD_NAND_OMAP2) += elm.o obj-$(CONFIG_MTD_SPEAR_SMI)+= spear_smi.o obj-$(CONFIG_MTD_SST25L) += sst25l.o obj-$(CONFIG_MTD_BCM47XXSFLASH)+= bcm47xxsflash.o -CFLAGS_docg3.o += -I$(src) \ No newline at end of file + +CFLAGS_docg3.o += -I$(src) diff --git a/drivers/mtd/devices/elm.c b/drivers/mtd/devices/elm.c new file mode 100644 index 000..802b572 --- /dev/null +++ b/drivers/mtd/devices/elm.c @@ -0,0 +1,440 @@ +/* + * Error Location Module + * + * Copyright (C) 2012 Texas Instruments Incorporated - http://www.ti.com/ + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; either version 2 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + */ + +#include +#include +#include +#include +#include +#include +#include + +#define ELM_IRQSTATUS 0x018 +#define ELM_IRQENABLE 0x01c +#define ELM_LOCATION_CONFIG0x020 +#define ELM_PAGE_CTRL 0x080 +#define ELM_SYNDROME_FRAGMENT_00x400 +#define ELM_SYNDROME_FRAGMENT_60x418 +#define ELM_LOCATION_STATUS0x800 +#define ELM_ERROR_LOCATION_0 0x880 + +/* ELM Interrupt Status Register */ +#define INTR_STATUS_PAGE_VALID BIT(8) + +/* ELM Interrupt Enable Register */ +#define INTR_EN_PAGE_MASK BIT(8) + +/* ELM Location Configuration Register */ +#define ECC_BCH_LEVEL_MASK 0x3 + +/* ELM syndrome */ +#define ELM_SYNDROME_VALID BIT(16) + +/* ELM_LOCATION_STATUS Register */ +#define ECC_CORRECTABLE_MASK BIT(8) +#define ECC_NB_ERRORS_MASK 0x1f + +/* ELM_ERROR_LOCATION_0-15 Registers */ +#define ECC_ERROR_LOCATION_MASK0x1fff + +#define ELM_ECC_SIZE 0x7ff + +#define SYNDROME_FRAGMENT_REG_SIZE 0x40 +#define ERROR_LOCATION_SIZE0x100 +#define MAX_BCH_ELM_ERROR 16 +#define ELM_FRAGMENT_REG 7 + +typedef u32 syn_t[ELM_FRAGMENT_REG]; +typedef u32 elm_error_t[MAX_BCH_ELM_ERROR]; + +struct elm_info { + struct device *dev; + void __iomem *elm_base; + struct completion elm_completion; + struct list_head list; + enum bch_ecc bch_type; +}; + +static LIST_HEAD(elm_devices); + +static void elm_write_reg(void *offset, u32 val) +{ + writel(val, offset); +} + +static u32 elm_read_reg(void *offset) +{ + return readl(offset); +} + +/** + * elm_config - Configure ELM module + * @info: elm info + */ +static void elm_config(struct elm_info *info) +{ + u32 reg_val; + + reg_val = (info->bch_type & ECC_BCH_LEVEL_MASK) | (ELM_ECC_SIZE << 16)