| Message ID | 1452702857-2240-4-git-send-email-romain.izard.pro@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
On Wed, Jan 13, 2016 at 05:34:15PM +0100, Romain Izard 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@gmail.com> > --- > .../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. What compatible string would AT91SAM9x5 be? > - 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
> -----Original Message----- > From: Rob Herring [mailto:robh@kernel.org] > Sent: 2016年1月14日 9:13 > To: Romain Izard <romain.izard.pro@gmail.com> > Cc: linux-mtd@lists.infradead.org; devicetree@vger.kernel.org; Yang, Wenyou > <Wenyou.Yang@atmel.com>; Josh Wu <rainyfeeling@outlook.com>; Ferre, > Nicolas <Nicolas.FERRE@atmel.com> > Subject: Re: [PATCH v1 3/5] mtd: atmel_nand: Support PMECC on SAMA5D2 > > On Wed, Jan 13, 2016 at 05:34:15PM +0100, Romain Izard 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@gmail.com> > > --- > > .../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. > > What compatible string would AT91SAM9x5 be? "atmel,at91rm9200-nand". > > > - 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 Best Regards, Wenyou Yang
On Wed, 13 Jan 2016 17:34:15 +0100 Romain Izard <romain.izard.pro@gmail.com> 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@gmail.com> > --- > .../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. */ > + 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. > > /* Galois field dimension */ > #define PMECC_GF_DIMENSION_13 13
On Wed, Jan 13, 2016 at 7:17 PM, Yang, Wenyou <Wenyou.Yang@atmel.com> wrote: > > >> -----Original Message----- >> From: Rob Herring [mailto:robh@kernel.org] >> Sent: 2016年1月14日 9:13 >> To: Romain Izard <romain.izard.pro@gmail.com> >> Cc: linux-mtd@lists.infradead.org; devicetree@vger.kernel.org; Yang, Wenyou >> <Wenyou.Yang@atmel.com>; Josh Wu <rainyfeeling@outlook.com>; Ferre, >> Nicolas <Nicolas.FERRE@atmel.com> >> Subject: Re: [PATCH v1 3/5] mtd: atmel_nand: Support PMECC on SAMA5D2 >> >> On Wed, Jan 13, 2016 at 05:34:15PM +0100, Romain Izard 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@gmail.com> >> > --- >> > .../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. >> >> What compatible string would AT91SAM9x5 be? > > "atmel,at91rm9200-nand". Answer the question in the binding doc by saying which compatible strings a property is valid for. Rob
Hi Rob, > -----Original Message----- > From: Rob Herring [mailto:robh@kernel.org] > Sent: 2016年1月14日 21:15 > To: Yang, Wenyou <Wenyou.Yang@atmel.com> > Cc: Romain Izard <romain.izard.pro@gmail.com>; linux-mtd@lists.infradead.org; > devicetree@vger.kernel.org; Josh Wu <rainyfeeling@outlook.com>; Ferre, > Nicolas <Nicolas.FERRE@atmel.com> > Subject: Re: [PATCH v1 3/5] mtd: atmel_nand: Support PMECC on SAMA5D2 > > On Wed, Jan 13, 2016 at 7:17 PM, Yang, Wenyou <Wenyou.Yang@atmel.com> > wrote: > > > > > >> -----Original Message----- > >> From: Rob Herring [mailto:robh@kernel.org] > >> Sent: 2016年1月14日 9:13 > >> To: Romain Izard <romain.izard.pro@gmail.com> > >> Cc: linux-mtd@lists.infradead.org; devicetree@vger.kernel.org; Yang, > >> Wenyou <Wenyou.Yang@atmel.com>; Josh Wu <rainyfeeling@outlook.com>; > >> Ferre, Nicolas <Nicolas.FERRE@atmel.com> > >> Subject: Re: [PATCH v1 3/5] mtd: atmel_nand: Support PMECC on SAMA5D2 > >> > >> On Wed, Jan 13, 2016 at 05:34:15PM +0100, Romain Izard 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@gmail.com> > >> > --- > >> > .../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. > >> > >> What compatible string would AT91SAM9x5 be? > > > > "atmel,at91rm9200-nand". > > Answer the question in the binding doc by saying which compatible strings a > property is valid for. > Thank you for your advice. I will keep in mind next time. Best Regards, Wenyou Yang
Hi Rob, Wenyou, 2016-01-14 14:14 GMT+01:00 Rob Herring <robh@kernel.org>: > On Wed, Jan 13, 2016 at 7:17 PM, Yang, Wenyou <Wenyou.Yang@atmel.com> wrote: >> >> >>> -----Original Message----- >>> From: Rob Herring [mailto:robh@kernel.org] >>> Sent: 2016年1月14日 9:13 >>> To: Romain Izard <romain.izard.pro@gmail.com> >>> Cc: linux-mtd@lists.infradead.org; devicetree@vger.kernel.org; Yang, Wenyou >>> <Wenyou.Yang@atmel.com>; Josh Wu <rainyfeeling@outlook.com>; Ferre, >>> Nicolas <Nicolas.FERRE@atmel.com> >>> Subject: Re: [PATCH v1 3/5] mtd: atmel_nand: Support PMECC on SAMA5D2 >>> >>> On Wed, Jan 13, 2016 at 05:34:15PM +0100, Romain Izard 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@gmail.com> >>> > --- >>> > .../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. >>> >>> What compatible string would AT91SAM9x5 be? >> >> "atmel,at91rm9200-nand". > > Answer the question in the binding doc by saying which compatible > strings a property is valid for. I'm trying to rewrite the documentation for this point, and in the end it seems to me that the line should be removed instead. Adding the 'atmel,has-pmecc' property to a SoC device tree is the normal way to describe that it supports this type of controller. If we need to write it in the documentation as well, it's a duplicate information that can become stale quite fast, as it was before. In the end, I believe the best path is to describe better what PMECC is, instead of describing as "the controller found in this kind of chip". Best regards,
Hi Boris, 2016-01-14 10:19 GMT+01:00 Boris Brezillon <boris.brezillon@free-electrons.com>: > On Wed, 13 Jan 2016 17:34:15 +0100 > Romain Izard <romain.izard.pro@gmail.com> 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@gmail.com> >> --- >> .../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,
On Fri, Jan 15, 2016 at 09:54:07AM +0100, Romain Izard wrote: > Hi Rob, Wenyou, > > 2016-01-14 14:14 GMT+01:00 Rob Herring <robh@kernel.org>: > > On Wed, Jan 13, 2016 at 7:17 PM, Yang, Wenyou <Wenyou.Yang@atmel.com> wrote: > >> > >> > >>> -----Original Message----- > >>> From: Rob Herring [mailto:robh@kernel.org] > >>> Sent: 2016年1月14日 9:13 > >>> To: Romain Izard <romain.izard.pro@gmail.com> > >>> Cc: linux-mtd@lists.infradead.org; devicetree@vger.kernel.org; Yang, Wenyou > >>> <Wenyou.Yang@atmel.com>; Josh Wu <rainyfeeling@outlook.com>; Ferre, > >>> Nicolas <Nicolas.FERRE@atmel.com> > >>> Subject: Re: [PATCH v1 3/5] mtd: atmel_nand: Support PMECC on SAMA5D2 > >>> > >>> On Wed, Jan 13, 2016 at 05:34:15PM +0100, Romain Izard 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@gmail.com> > >>> > --- > >>> > .../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. > >>> > >>> What compatible string would AT91SAM9x5 be? > >> > >> "atmel,at91rm9200-nand". > > > > Answer the question in the binding doc by saying which compatible > > strings a property is valid for. > > I'm trying to rewrite the documentation for this point, and in the end it > seems to me that the line should be removed instead. > > Adding the 'atmel,has-pmecc' property to a SoC device tree is the normal way > to describe that it supports this type of controller. If we need to write it > in the documentation as well, it's a duplicate information that can become > stale quite fast, as it was before. > > In the end, I believe the best path is to describe better what PMECC is, > instead of describing as "the controller found in this kind of chip". Okay, that is important too. We are wanting to move binding documentation to something more structured where we can do some amount of validation of dts files against binding documentation. So that is why I am thinking in terms of having the information to validate a dts. But we are a ways off from doing such a thing. Rob
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; } + 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)) /* Galois field dimension */ #define PMECC_GF_DIMENSION_13 13
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@gmail.com> --- .../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(-)