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
