Re: [RESEND PATCH v2 26/53] mtd: nand: denali: support 1024 byte ECC step size
From: Boris Brezillon
Date: Wed Mar 22 2017 - 17:35:05 EST
On Thu, 23 Mar 2017 05:07:25 +0900
Masahiro Yamada <yamada.masahiro@xxxxxxxxxxxxx> wrote:
> This driver was originally written for the Intel MRST platform with
> several platform specific parameters hard-coded. Another thing we
> need to fix is the hard-coded ECC step size. Currently, it is
> defined as follows:
>
> #define ECC_SECTOR_SIZE 512
>
> (somehow, it is defined in both denali.c and denali.h)
>
> This must be avoided because the Denali IP supports 1024B ECC size
> as well. The Denali User's Guide also says supporting both 512B and
> 1024B ECC sectors is possible, though it would require instantiation
> of two different ECC circuits. So, possible cases are:
>
> [1] only 512B ECC size is supported
> [2] only 1024B ECC size is supported
> [3] both 512B and 1024B ECC sizes are supported
>
> For [3], the actually used ECC size is specified by some registers.
>
> Newer versions of this IP have the following registers:
> CFG_DATA_BLOCK_SIZE (0x6b0)
> CFG_LAST_DATA_BLOCK_SIZE (0x6c0)
> CFG_NUM_DATA_BLOCKS (0x6d0)
>
> For those versions, the software should set ecc.size and ecc.steps
> to these registers. Old versions do not have such registers, but
> they are "reserved", so write accesses are safely ignored.
>
> This commit adds new flags DENALI_CAP_ECC_SIZE_{512,1024}.
>
> The DT property "nand-ecc-step-size" is still optional; a reasonable
> default will be chosen for [1] and [2]. For case [3], if exists, it
> is recommended to specify the desired ECC size explicitly.
Actually, the NAND chip gives some hints to help controller drivers
decide which ecc-block-size/strength is appropriate
(chip->ecc_strength_ds, chip->ecc_step_ds), so, in most cases
nand-ecc-step-size is unneeded (unless you want to force a specific
setting).
>
> Signed-off-by: Masahiro Yamada <yamada.masahiro@xxxxxxxxxxxxx>
> Acked-by: Rob Herring <robh@xxxxxxxxxx>
> ---
>
> Changes in v2:
> - Change the capability prefix DENALI_CAPS_ -> DENALI_CAP_
> - Make ECC 512 cap and ECC 1024 cap independent
> - Set up three CFG_... registers
>
> .../devicetree/bindings/mtd/denali-nand.txt | 5 +++
> drivers/mtd/nand/denali.c | 44 +++++++++++++++-------
> drivers/mtd/nand/denali.h | 12 +++++-
> drivers/mtd/nand/denali_dt.c | 3 +-
> drivers/mtd/nand/denali_pci.c | 2 +
> 5 files changed, 50 insertions(+), 16 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/mtd/denali-nand.txt b/Documentation/devicetree/bindings/mtd/denali-nand.txt
> index e593bbe..25313c7 100644
> --- a/Documentation/devicetree/bindings/mtd/denali-nand.txt
> +++ b/Documentation/devicetree/bindings/mtd/denali-nand.txt
> @@ -7,6 +7,11 @@ Required properties:
> - reg-names: Should contain the reg names "nand_data" and "denali_reg"
> - interrupts : The interrupt number.
>
> +Optional properties:
> + - nand-ecc-step-size: must be 512 or 1024. If not specified, default to:
> + 512 for "altr,socfpga-denali-nand"
> + see nand.txt for details.
> +
> The device tree may optionally contain sub-nodes describing partitions of the
> address space. See partition.txt for more detail.
>
> diff --git a/drivers/mtd/nand/denali.c b/drivers/mtd/nand/denali.c
> index a190cb2..cf8daba 100644
> --- a/drivers/mtd/nand/denali.c
> +++ b/drivers/mtd/nand/denali.c
> @@ -875,8 +875,6 @@ static int denali_hw_ecc_fixup(struct mtd_info *mtd,
> return max_bitflips;
> }
>
> -#define ECC_SECTOR_SIZE 512
> -
> #define ECC_SECTOR(x) (((x) & ECC_ERROR_ADDRESS__SECTOR_NR) >> 12)
> #define ECC_BYTE(x) (((x) & ECC_ERROR_ADDRESS__OFFSET))
> #define ECC_CORRECTION_VALUE(x) ((x) & ERR_CORRECTION_INFO__BYTEMASK)
> @@ -887,6 +885,7 @@ static int denali_hw_ecc_fixup(struct mtd_info *mtd,
> static int denali_sw_ecc_fixup(struct mtd_info *mtd,
> struct denali_nand_info *denali, uint8_t *buf)
> {
> + unsigned int ecc_size = denali->nand.ecc.size;
> unsigned int bitflips = 0;
> unsigned int max_bitflips = 0;
> unsigned int total_bitflips = 0;
> @@ -914,9 +913,9 @@ static int denali_sw_ecc_fixup(struct mtd_info *mtd,
>
> if (ECC_ERROR_UNCORRECTABLE(err_cor_info)) {
> ret = -EBADMSG;
> - } else if (err_byte < ECC_SECTOR_SIZE) {
> + } else if (err_byte < ecc_size) {
> /*
> - * If err_byte is larger than ECC_SECTOR_SIZE, means error
> + * If err_byte is larger than ecc_size, means error
> * happened in OOB, so we ignore it. It's no need for
> * us to correct it err_device is represented the NAND
> * error bits are happened in if there are more than
> @@ -925,7 +924,7 @@ static int denali_sw_ecc_fixup(struct mtd_info *mtd,
> int offset;
> unsigned int flips_in_byte;
>
> - offset = (err_sector * ECC_SECTOR_SIZE + err_byte) *
> + offset = (err_sector * ecc_size + err_byte) *
> denali->devnum + err_device;
>
> /* correct the ECC error */
> @@ -1579,22 +1578,37 @@ int denali_init(struct denali_nand_info *denali)
> /* no subpage writes on denali */
> chip->options |= NAND_NO_SUBPAGE_WRITE;
>
> + if (!chip->ecc.size) {
You should set it to chip->ecc_step_ds and pick a default value only if
it's still 0 after that. Same goes for ecc.strength.
> + if (denali->caps & DENALI_CAP_ECC_SIZE_512)
> + chip->ecc.size = 512;
> + if (denali->caps & DENALI_CAP_ECC_SIZE_1024)
> + chip->ecc.size = 1024;
> + if (WARN(!chip->ecc.size, "must support at least 512 or 1024 ECC size"))
> + goto failed_req_irq;
> + }
> +
> + if ((chip->ecc.size != 512 && chip->ecc.size != 1024) ||
> + (chip->ecc.size == 512 && !(denali->caps & DENALI_CAP_ECC_SIZE_512)) ||
> + (chip->ecc.size == 1024 && !(denali->caps & DENALI_CAP_ECC_SIZE_1024))) {
> + dev_err(denali->dev, "specified ECC size %d in not supported",
> + chip->ecc.size);
> + goto failed_req_irq;
> + }
> +
> /*
> * Denali Controller only support 15bit and 8bit ECC in MRST,
> * so just let controller do 15bit ECC for MLC and 8bit ECC for
> * SLC if possible.
Usually the NAND chips expose the ECC requirements, so basing our
decision only on the type of NAND sounds a bit weird.