Re: [PATCH v1 3/5] mtd: atmel_nand: Support PMECC on SAMA5D2

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




Hi Boris,

2016-01-14 10:19 GMT+01:00 Boris Brezillon <boris.brezillon@xxxxxxxxxxxxxxxxxx>:
> On Wed, 13 Jan 2016 17:34:15 +0100
> Romain Izard <romain.izard.pro@xxxxxxxxx> wrote:
>
>> Starting with the SAMA5D2, there is a new revision of the Atmel PMECC
>> controller that can correct 32 bits in each sector. This controller is
>> not 100% compatible with the previous revision that corrected a maximum
>> of 24 bits by sector, as some register addresses overlap.
>>
>> Using information from the device tree, we can configure the driver to
>> work with both versions.
>>
>> Signed-off-by: Romain Izard <romain.izard.pro@xxxxxxxxx>
>> ---
>>  .../devicetree/bindings/mtd/atmel-nand.txt         |  7 +++++--
>>  drivers/mtd/nand/atmel_nand.c                      | 23 +++++++++++++++++++++-
>>  drivers/mtd/nand/atmel_nand_ecc.h                  |  8 ++++++--
>>  3 files changed, 33 insertions(+), 5 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/mtd/atmel-nand.txt b/Documentation/devicetree/bindings/mtd/atmel-nand.txt
>> index 89b0db9801b0..90887b430f03 100644
>> --- a/Documentation/devicetree/bindings/mtd/atmel-nand.txt
>> +++ b/Documentation/devicetree/bindings/mtd/atmel-nand.txt
>> @@ -1,7 +1,10 @@
>>  Atmel NAND flash
>>
>>  Required properties:
>> -- compatible : should be "atmel,at91rm9200-nand" or "atmel,sama5d4-nand".
>> +- compatible: The possible values are:
>> +     "atmel,at91rm9200-nand"
>> +     "atmel,sama5d2-nand"
>> +     "atmel,sama5d4-nand"
>>  - reg : should specify localbus address and size used for the chip,
>>       and hardware ECC controller if available.
>>       If the hardware ECC is PMECC, it should contain address and size for
>> @@ -22,7 +25,7 @@ Optional properties:
>>    Supported values are: "none", "soft", "hw", "hw_syndrome", "hw_oob_first",
>>    "soft_bch".
>>  - atmel,has-pmecc : boolean to enable Programmable Multibit ECC hardware.
>> -  Only supported by at91sam9x5 or later sam9 product.
>> +  Supported by AT91SAM9x5 or later SAM9 chips, and SAMA5 chips.
>>  - atmel,pmecc-cap : error correct capability for Programmable Multibit ECC
>>    Controller. Supported values are: 2, 4, 8, 12, 24.
>>  - atmel,pmecc-sector-size : sector size for ECC computation. Supported values
>> diff --git a/drivers/mtd/nand/atmel_nand.c b/drivers/mtd/nand/atmel_nand.c
>> index e5d7e7e63f49..6fe50e2d291f 100644
>> --- a/drivers/mtd/nand/atmel_nand.c
>> +++ b/drivers/mtd/nand/atmel_nand.c
>> @@ -65,6 +65,7 @@ module_param(on_flash_bbt, int, 0);
>>
>>  struct atmel_nand_caps {
>>       bool pmecc_correct_erase_page;
>> +     uint8_t pmecc_max_correction;
>>  };
>>
>>  struct atmel_nand_nfc_priv {
>> @@ -146,6 +147,7 @@ struct atmel_nand_host {
>>       int                     pmecc_cw_len;   /* Length of codeword */
>>
>>       void __iomem            *pmerrloc_base;
>> +     void __iomem            *pmerrloc_el_base;
>>       void __iomem            *pmecc_rom_base;
>>
>>       /* lookup table for alpha_to and index_of */
>> @@ -817,7 +819,7 @@ static void pmecc_correct_data(struct mtd_info *mtd, uint8_t *buf, uint8_t *ecc,
>>       sector_size = host->pmecc_sector_size;
>>
>>       while (err_nbr) {
>> -             tmp = pmerrloc_readl_el_relaxed(host->pmerrloc_base, i) - 1;
>> +             tmp = pmerrloc_readl_el_relaxed(host->pmerrloc_el_base, i) - 1;
>>               byte_pos = tmp / 8;
>>               bit_pos  = tmp % 8;
>>
>> @@ -1208,6 +1210,8 @@ static int atmel_pmecc_nand_init_params(struct platform_device *pdev,
>>               err_no = PTR_ERR(host->pmerrloc_base);
>>               goto err;
>>       }
>
> How about putting the comment you added below here:
>
> /*
>  * The ATMEL_PMERRLOC_ELx register location depends on the number of
>  * bits corrected by the PMECC controller. Pre-calculate the
>  * pmerrloc_el_base according to the PMECC capabilities.
>  */
>
Here, this comment is paraphrasing the code. That's not very useful.

>> +     host->pmerrloc_el_base = host->pmerrloc_base + ATMEL_PMERRLOC_SIGMAx +
>> +             (host->caps->pmecc_max_correction + 1) * 4;
>>
>>       if (!host->has_no_lookup_table) {
>>               regs_rom = platform_get_resource(pdev, IORESOURCE_MEM, 3);
>> @@ -2307,17 +2311,34 @@ static int atmel_nand_remove(struct platform_device *pdev)
>>       return 0;
>>  }
>>
>> +/*
>> + * AT91RM9200 does not have PMECC or PMECC Errloc peripherals for
>> + * BCH ECC. Combined with the "atmel,has-pmecc", it is used to describe
>> + * devices from the SAM9 family that have those.
>> + */
>>  static struct atmel_nand_caps at91rm9200_caps = {
>>       .pmecc_correct_erase_page = false,
>> +     .pmecc_max_correction = 24,
>>  };
>>
>>  static struct atmel_nand_caps sama5d4_caps = {
>>       .pmecc_correct_erase_page = true,
>> +     .pmecc_max_correction = 24,
>> +};
>> +
>> +/*
>> + * The PMECC Errloc controller starting in SAMA5D2 is not compatible,
>> + * as the increased correction strength requires more registers.
>> + */
>> +static struct atmel_nand_caps sama5d2_caps = {
>> +     .pmecc_correct_erase_page = true,
>> +     .pmecc_max_correction = 32,
>>  };
>>
>>  static const struct of_device_id atmel_nand_dt_ids[] = {
>>       { .compatible = "atmel,at91rm9200-nand", .data = &at91rm9200_caps },
>>       { .compatible = "atmel,sama5d4-nand", .data = &sama5d4_caps },
>> +     { .compatible = "atmel,sama5d2-nand", .data = &sama5d2_caps },
>>       { /* sentinel */ }
>>  };
>>
>> diff --git a/drivers/mtd/nand/atmel_nand_ecc.h b/drivers/mtd/nand/atmel_nand_ecc.h
>> index 668e7358f19b..ec964c43c932 100644
>> --- a/drivers/mtd/nand/atmel_nand_ecc.h
>> +++ b/drivers/mtd/nand/atmel_nand_ecc.h
>> @@ -108,7 +108,11 @@
>>  #define              PMERRLOC_ERR_NUM_MASK           (0x1f << 8)
>>  #define              PMERRLOC_CALC_DONE              (1 << 0)
>>  #define ATMEL_PMERRLOC_SIGMAx                0x028   /* Error location SIGMA x */
>> -#define ATMEL_PMERRLOC_ELx           0x08c   /* Error location x */
>> +
>> +/*
>> + * The ATMEL_PMERRLOC_ELx register location depends from the number of
>> + * bits corrected by the PMECC controller. Do not use it.
>> + */
>>
>>  /* Register access macros for PMECC */
>>  #define pmecc_readl_relaxed(addr, reg) \
>> @@ -136,7 +140,7 @@
>>       readl_relaxed((addr) + ATMEL_PMERRLOC_SIGMAx + ((n) * 4))
>>
>>  #define pmerrloc_readl_el_relaxed(addr, n) \
>> -     readl_relaxed((addr) + ATMEL_PMERRLOC_ELx + ((n) * 4))
>> +     readl_relaxed((addr) + ((n) * 4))
>
> Another option would have been to pass the maximum ECC strength here,
> and modify the ATMEL_PMERRLOC_ELx macro to take a max_strength argument
> and return the adjusted ELx offset.
> Anyway, both solutions work for me.
>
The current solution makes it easier to keep within the 80 character limit.
The memory/computing tradeoff is so small it won't change anything.

>>
>>  /* Galois field dimension */
>>  #define PMECC_GF_DIMENSION_13                        13

Best regards,
-- 
Romain Izard
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]
  Powered by Linux