[v4,1/9] dt-bindings: mtd: ingenic: Add compatible strings for JZ4740 and JZ4725B

Message ID 20190209192305.4434-1-paul@crapouillou.net
State Not Applicable
Headers show
Series
  • [v4,1/9] dt-bindings: mtd: ingenic: Add compatible strings for JZ4740 and JZ4725B
Related show

Checks

Context Check Description
robh/checkpatch success

Commit Message

Paul Cercueil Feb. 9, 2019, 7:22 p.m.
Add compatible strings to probe the jz4780-nand and jz4780-bch drivers
from devicetree on the JZ4725B and JZ4740 SoCs from Ingenic.

Signed-off-by: Paul Cercueil <paul@crapouillou.net>
---

Changes:

v2: - Change 'ingenic,jz4725b-nand' compatible string to
      'ingenic,jz4740-nand' to reflect driver change
    - Add 'ingenic,jz4740-bch' compatible string
    - Document 'ingenic,oob-layout' property

v3: - Removed 'ingenic,oob-layout' property
    - Update compatible strings to what the driver supports

v4: No change

 Documentation/devicetree/bindings/mtd/ingenic,jz4780-nand.txt | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

Comments

Rob Herring Feb. 13, 2019, 9:45 p.m. | #1
On Sat,  9 Feb 2019 16:22:57 -0300, Paul Cercueil wrote:
> Add compatible strings to probe the jz4780-nand and jz4780-bch drivers
> from devicetree on the JZ4725B and JZ4740 SoCs from Ingenic.
> 
> Signed-off-by: Paul Cercueil <paul@crapouillou.net>
> ---
> 
> Changes:
> 
> v2: - Change 'ingenic,jz4725b-nand' compatible string to
>       'ingenic,jz4740-nand' to reflect driver change
>     - Add 'ingenic,jz4740-bch' compatible string
>     - Document 'ingenic,oob-layout' property
> 
> v3: - Removed 'ingenic,oob-layout' property
>     - Update compatible strings to what the driver supports
> 
> v4: No change
> 
>  Documentation/devicetree/bindings/mtd/ingenic,jz4780-nand.txt | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 

Reviewed-by: Rob Herring <robh@kernel.org>
Miquel Raynal March 4, 2019, 9:45 a.m. | #2
Hi Paul,

Paul Cercueil <paul@crapouillou.net> wrote on Sat,  9 Feb 2019 16:22:57
-0300:

> Add compatible strings to probe the jz4780-nand and jz4780-bch drivers
> from devicetree on the JZ4725B and JZ4740 SoCs from Ingenic.
> 
> Signed-off-by: Paul Cercueil <paul@crapouillou.net>
> ---
> 
> Changes:
> 
> v2: - Change 'ingenic,jz4725b-nand' compatible string to
>       'ingenic,jz4740-nand' to reflect driver change
>     - Add 'ingenic,jz4740-bch' compatible string
>     - Document 'ingenic,oob-layout' property
> 
> v3: - Removed 'ingenic,oob-layout' property
>     - Update compatible strings to what the driver supports
> 
> v4: No change
> 
>  Documentation/devicetree/bindings/mtd/ingenic,jz4780-nand.txt | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/mtd/ingenic,jz4780-nand.txt b/Documentation/devicetree/bindings/mtd/ingenic,jz4780-nand.txt
> index 29ea5853ca91..a5b940f18bf6 100644
> --- a/Documentation/devicetree/bindings/mtd/ingenic,jz4780-nand.txt
> +++ b/Documentation/devicetree/bindings/mtd/ingenic,jz4780-nand.txt
> @@ -6,7 +6,10 @@ memory-controllers/ingenic,jz4780-nemc.txt), and thus NAND device nodes must
>  be children of the NEMC node.
>  
>  Required NAND controller device properties:
> -- compatible: Should be set to "ingenic,jz4780-nand".
> +- compatible: Should be one of:
> +  * ingenic,jz4740-nand
> +  * ingenic,jz4725b-nand
> +  * ingenic,jz4780-nand

Wouldn't "-nand-controller" suffix be better? Of course in the driver
you should still check for jz4780-nand.

>  - reg: For each bank with a NAND chip attached, should specify a bank number,
>    an offset of 0 and a size of 0x1000000 (i.e. the whole NEMC bank).
>  
> @@ -72,7 +75,10 @@ NAND devices. The following is a description of the device properties for a
>  BCH controller.
>  
>  Required BCH properties:
> -- compatible: Should be set to "ingenic,jz4780-bch".
> +- compatible: Should be one of:
> +  * ingenic,jz4740-ecc
> +  * ingenic,jz4725b-bch
> +  * ingenic,jz4780-bch
>  - reg: Should specify the BCH controller registers location and length.
>  - clocks: Clock for the BCH controller.
>  

Thanks,
Miquèl
Miquel Raynal March 4, 2019, 10:20 a.m. | #3
Hi Paul,

Paul Cercueil <paul@crapouillou.net> wrote on Sat,  9 Feb 2019 16:23:02
-0300:

> The ingenic-nand driver uses an API provided by the jz4780-bch driver.
> This makes it difficult to support other SoCs in the jz4780-bch driver.
> To work around this, we separate the API functions from the SoC-specific
> code, so that these API functions are SoC-agnostic.
> 

I like the idea, actually I am working on this separation (see
[1]) and I would really appreciate that you try to implement the
interface when it will be available (v2 is coming this week, I think v3
will be the one to test when raw NAND devices will be properly
supported). I will add you in Cc: if you want to follow/review.

[1] http://lists.infradead.org/pipermail/linux-mtd/2019-February/087815.html

> Signed-off-by: Paul Cercueil <paul@crapouillou.net>
> ---
> 
> v2: Add an optional .probe() callback. It is used for instance to set
>     the clock rate in the JZ4780 backend.
> 
> v3: The common code is now inside the ingenic-ecc module. Each
>     SoC-specific ECC code is now in its own module, which leaves to the
>     user the choice of which (if any) ECC code should be supported.
> 
> v4: No change
> 
>  drivers/mtd/nand/raw/ingenic/Kconfig        |  17 +++
>  drivers/mtd/nand/raw/ingenic/Makefile       |   5 +-
>  drivers/mtd/nand/raw/ingenic/ingenic_ecc.c  | 157 +++++++++++++++++++++++++
>  drivers/mtd/nand/raw/ingenic/ingenic_ecc.h  |  84 ++++++++++++++
>  drivers/mtd/nand/raw/ingenic/ingenic_nand.c |  38 +++----
>  drivers/mtd/nand/raw/ingenic/jz4780_bch.c   | 170 +++++-----------------------
>  drivers/mtd/nand/raw/ingenic/jz4780_bch.h   |  40 -------
>  7 files changed, 308 insertions(+), 203 deletions(-)
>  create mode 100644 drivers/mtd/nand/raw/ingenic/ingenic_ecc.c
>  create mode 100644 drivers/mtd/nand/raw/ingenic/ingenic_ecc.h
>  delete mode 100644 drivers/mtd/nand/raw/ingenic/jz4780_bch.h
> 

[...]

> diff --git a/drivers/mtd/nand/raw/ingenic/ingenic_nand.c b/drivers/mtd/nand/raw/ingenic/ingenic_nand.c
> index 8c73f7c5be9a..0f51fd15fe79 100644
> --- a/drivers/mtd/nand/raw/ingenic/ingenic_nand.c
> +++ b/drivers/mtd/nand/raw/ingenic/ingenic_nand.c
> @@ -22,7 +22,7 @@
>  
>  #include <linux/jz4780-nemc.h>
>  
> -#include "jz4780_bch.h"
> +#include "ingenic_ecc.h"
>  
>  #define DRV_NAME	"ingenic-nand"
>  
> @@ -40,7 +40,7 @@ struct ingenic_nand_cs {
>  
>  struct ingenic_nfc {
>  	struct device *dev;
> -	struct jz4780_bch *bch;
> +	struct ingenic_ecc *ecc;
>  	struct nand_controller controller;
>  	unsigned int num_banks;
>  	struct list_head chips;
> @@ -124,10 +124,10 @@ static int ingenic_nand_ecc_calculate(struct nand_chip *chip, const u8 *dat,
>  {
>  	struct ingenic_nand *nand = to_ingenic_nand(nand_to_mtd(chip));
>  	struct ingenic_nfc *nfc = to_ingenic_nfc(nand->chip.controller);
> -	struct jz4780_bch_params params;
> +	struct ingenic_ecc_params params;
>  
>  	/*
> -	 * Don't need to generate the ECC when reading, BCH does it for us as
> +	 * Don't need to generate the ECC when reading, ECC does it for us as

"the ECC engine does it for us" would be more meaningful.

>  	 * part of decoding/correction.
>  	 */
>  	if (nand->reading)
> @@ -137,7 +137,7 @@ static int ingenic_nand_ecc_calculate(struct nand_chip *chip, const u8 *dat,
>  	params.bytes = nand->chip.ecc.bytes;
>  	params.strength = nand->chip.ecc.strength;
>  
> -	return jz4780_bch_calculate(nfc->bch, &params, dat, ecc_code);
> +	return ingenic_ecc_calculate(nfc->ecc, &params, dat, ecc_code);
>  }
>  

Thanks,
Miquèl
Miquel Raynal March 4, 2019, 10:34 a.m. | #4
Hi Paul,

Paul Cercueil <paul@crapouillou.net> wrote on Sat,  9 Feb 2019 16:23:03
-0300:

> Add support for probing the ingenic-nand driver on the JZ4740 SoC from
> Ingenic, and the jz4740-ecc driver to support the JZ4740-specific
> ECC hardware.
> 
> Signed-off-by: Paul Cercueil <paul@crapouillou.net>
> ---
> 
> Changes:
> 
> v2: New patch
> 
> v3: Also add support for the hardware ECC of the JZ4740 in this patch
> 
> v4: - Fix formatting issues
>     - Add MODULE_* macros
> 
>  drivers/mtd/nand/raw/ingenic/Kconfig        |  10 ++
>  drivers/mtd/nand/raw/ingenic/Makefile       |   1 +
>  drivers/mtd/nand/raw/ingenic/ingenic_nand.c |  48 +++++--
>  drivers/mtd/nand/raw/ingenic/jz4740_ecc.c   | 196 ++++++++++++++++++++++++++++
>  4 files changed, 244 insertions(+), 11 deletions(-)
>  create mode 100644 drivers/mtd/nand/raw/ingenic/jz4740_ecc.c
> 

[...]

>  	switch (chip->ecc.mode) {
>  	case NAND_ECC_HW:
> @@ -270,8 +279,8 @@ static int ingenic_nand_init_chip(struct platform_device *pdev,
>  		return -ENOMEM;
>  	mtd->dev.parent = dev;
>  
> -	chip->legacy.IO_ADDR_R = cs->base + OFFSET_DATA;
> -	chip->legacy.IO_ADDR_W = cs->base + OFFSET_DATA;
> +	chip->legacy.IO_ADDR_R = cs->base + nfc->soc_info->data_offset;
> +	chip->legacy.IO_ADDR_W = cs->base + nfc->soc_info->data_offset;
>  	chip->legacy.chip_delay = RB_DELAY_US;
>  	chip->options = NAND_NO_SUBPAGE_WRITE;
>  	chip->legacy.select_chip = ingenic_nand_select_chip;

I think Boris already asked for it, but it would be really great that
you update this driver to not use any legacy interface anymore.

> diff --git a/drivers/mtd/nand/raw/ingenic/jz4740_ecc.c b/drivers/mtd/nand/raw/ingenic/jz4740_ecc.c
> new file mode 100644
> index 000000000000..83b42881720e
> --- /dev/null
> +++ b/drivers/mtd/nand/raw/ingenic/jz4740_ecc.c
> @@ -0,0 +1,196 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * JZ4740 ECC controller driver
> + *
> + * Copyright (c) 2019 Paul Cercueil <paul@crapouillou.net>
> + *
> + * based on jz4740-nand.c
> + */
> +
> +#include <linux/bitops.h>
> +#include <linux/device.h>
> +#include <linux/io.h>
> +#include <linux/module.h>
> +#include <linux/of_platform.h>
> +#include <linux/platform_device.h>
> +
> +#include "ingenic_ecc.h"
> +
> +#define JZ_REG_NAND_ECC_CTRL	0x00
> +#define JZ_REG_NAND_DATA	0x04
> +#define JZ_REG_NAND_PAR0	0x08
> +#define JZ_REG_NAND_PAR1	0x0C
> +#define JZ_REG_NAND_PAR2	0x10
> +#define JZ_REG_NAND_IRQ_STAT	0x14
> +#define JZ_REG_NAND_IRQ_CTRL	0x18
> +#define JZ_REG_NAND_ERR(x)	(0x1C + ((x) << 2))
> +
> +#define JZ_NAND_ECC_CTRL_PAR_READY	BIT(4)
> +#define JZ_NAND_ECC_CTRL_ENCODING	BIT(3)
> +#define JZ_NAND_ECC_CTRL_RS		BIT(2)
> +#define JZ_NAND_ECC_CTRL_RESET		BIT(1)
> +#define JZ_NAND_ECC_CTRL_ENABLE		BIT(0)
> +
> +#define JZ_NAND_STATUS_ERR_COUNT	(BIT(31) | BIT(30) | BIT(29))
> +#define JZ_NAND_STATUS_PAD_FINISH	BIT(4)
> +#define JZ_NAND_STATUS_DEC_FINISH	BIT(3)
> +#define JZ_NAND_STATUS_ENC_FINISH	BIT(2)
> +#define JZ_NAND_STATUS_UNCOR_ERROR	BIT(1)
> +#define JZ_NAND_STATUS_ERROR		BIT(0)
> +
> +static const uint8_t empty_block_ecc[] = {
> +	0xcd, 0x9d, 0x90, 0x58, 0xf4, 0x8b, 0xff, 0xb7, 0x6f
> +};
> +
> +static void jz4740_ecc_init(struct ingenic_ecc *ecc, bool encode)
> +{
> +	uint32_t reg;
> +
> +	/* Clear interrupt status */
> +	writel(0, ecc->base + JZ_REG_NAND_IRQ_STAT);
> +
> +	/* Initialize and enable ECC hardware */
> +	reg = readl(ecc->base + JZ_REG_NAND_ECC_CTRL);
> +	reg |= JZ_NAND_ECC_CTRL_RESET;
> +	reg |= JZ_NAND_ECC_CTRL_ENABLE;
> +	reg |= JZ_NAND_ECC_CTRL_RS;
> +	if (encode)
> +		reg |= JZ_NAND_ECC_CTRL_ENCODING;
> +	else
> +		reg &= ~JZ_NAND_ECC_CTRL_ENCODING;
> +
> +	writel(reg, ecc->base + JZ_REG_NAND_ECC_CTRL);
> +}
> +
> +static int jz4740_ecc_calculate(struct ingenic_ecc *ecc,
> +				struct ingenic_ecc_params *params,
> +				const u8 *buf, u8 *ecc_code)
> +{
> +	uint32_t reg, status;
> +	unsigned int timeout = 1000;
> +	int i;
> +
> +	jz4740_ecc_init(ecc, true);
> +
> +	do {
> +		status = readl(ecc->base + JZ_REG_NAND_IRQ_STAT);
> +	} while (!(status & JZ_NAND_STATUS_ENC_FINISH) && --timeout);
> +
> +	if (timeout == 0)
> +		return -ETIMEDOUT;
> +
> +	reg = readl(ecc->base + JZ_REG_NAND_ECC_CTRL);
> +	reg &= ~JZ_NAND_ECC_CTRL_ENABLE;
> +	writel(reg, ecc->base + JZ_REG_NAND_ECC_CTRL);
> +
> +	for (i = 0; i < params->bytes; ++i)
> +		ecc_code[i] = readb(ecc->base + JZ_REG_NAND_PAR0 + i);
> +
> +	/* If the written data is completely 0xff, we also want to write 0xff as
> +	 * ecc, otherwise we will get in trouble when doing subpage writes.
> +	 */

Comment formatting

s/ecc/ECC/ in plain English

> +	if (memcmp(ecc_code, empty_block_ecc, ARRAY_SIZE(empty_block_ecc)) == 0)
> +		memset(ecc_code, 0xff, ARRAY_SIZE(empty_block_ecc));
> +
> +	return 0;
> +}
> +


Thanks,
Miquèl
Miquel Raynal March 4, 2019, 10:35 a.m. | #5
Hi Paul,

Paul Cercueil <paul@crapouillou.net> wrote on Sat,  9 Feb 2019 16:23:04
-0300:

> The boot ROM of the JZ4725B SoC expects a specific OOB layout on the
> NAND, so we use it unconditionally in the ingenic-nand driver.
> 
> Also add the jz4725b-bch driver to support the JZ4725B-specific BCH
> hardware.
> 
> Signed-off-by: Paul Cercueil <paul@crapouillou.net>
> ---
> 
> Changes:
> 
> v2: Instead of forcing the OOB layout, leave it to the board code or
>     devicetree to decide if the jz4725b-specific layout should be used
>     or not.
> 
> v3: - Revert the change in v2, as the previous behaviour was correct.
>     - Also add support for the hardware BCH of the JZ4725B in this
>       patch.
> 
> v4: - Add MODULE_* macros
>     - Add tweaks suggested by upstream feedback
> 
>  drivers/mtd/nand/raw/ingenic/Kconfig        |  10 +
>  drivers/mtd/nand/raw/ingenic/Makefile       |   1 +
>  drivers/mtd/nand/raw/ingenic/ingenic_nand.c |  48 ++++-
>  drivers/mtd/nand/raw/ingenic/jz4725b_bch.c  | 292 ++++++++++++++++++++++++++++
>  4 files changed, 350 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/mtd/nand/raw/ingenic/jz4725b_bch.c
> 

[...]

> +static int jz4725b_calculate(struct ingenic_ecc *bch,
> +			     struct ingenic_ecc_params *params,
> +			     const u8 *buf, u8 *ecc_code)
> +{
> +	int ret;
> +
> +	mutex_lock(&bch->lock);
> +	ret = jz4725b_bch_init(bch, params, true);

I really don't like this bch_init name. A BCH initialization is what is
supposed to be done only once (probably at boot time), can you find a
better name or a better organization of the correct/calculate path?

> +	if (ret) {
> +		dev_err(bch->dev, "Unable to init BCH with given parameters\n");
> +		goto out_disable;
> +	}
> +
> +	jz4725b_bch_write_data(bch, buf, params->size);
> +
> +	ret = jz4725b_bch_wait_complete(bch, BCH_BHINT_ENCF, NULL);
> +	if (ret) {
> +		dev_err(bch->dev, "timed out while calculating ECC\n");
> +		goto out_disable;
> +	}
> +
> +	jz4725b_bch_read_parity(bch, ecc_code, params->bytes);
> +
> +out_disable:
> +	jz4725b_bch_disable(bch);
> +	mutex_unlock(&bch->lock);
> +
> +	return ret;
> +}
> +

[...]

Thanks,
Miquèl
Paul Cercueil March 4, 2019, 6:22 p.m. | #6
Hi Miquel,

On Mon, Mar 4, 2019 at 10:45 AM, Miquel Raynal 
<miquel.raynal@bootlin.com> wrote:
> Hi Paul,
> 
> Paul Cercueil <paul@crapouillou.net <mailto:paul@crapouillou.net>> 
> wrote on Sat,  9 Feb 2019 16:22:57
> -0300:
> 
>>  Add compatible strings to probe the jz4780-nand and jz4780-bch 
>> drivers
>>  from devicetree on the JZ4725B and JZ4740 SoCs from Ingenic.
>> 
>>  Signed-off-by: Paul Cercueil <paul@crapouillou.net 
>> <mailto:paul@crapouillou.net>>
>>  ---
>> 
>>  Changes:
>> 
>>  v2: - Change 'ingenic,jz4725b-nand' compatible string to
>>        'ingenic,jz4740-nand' to reflect driver change
>>      - Add 'ingenic,jz4740-bch' compatible string
>>      - Document 'ingenic,oob-layout' property
>> 
>>  v3: - Removed 'ingenic,oob-layout' property
>>      - Update compatible strings to what the driver supports
>> 
>>  v4: No change
>> 
>>   Documentation/devicetree/bindings/mtd/ingenic,jz4780-nand.txt | 10 
>> ++++++++--
>>   1 file changed, 8 insertions(+), 2 deletions(-)
>> 
>>  diff --git 
>> a/Documentation/devicetree/bindings/mtd/ingenic,jz4780-nand.txt 
>> b/Documentation/devicetree/bindings/mtd/ingenic,jz4780-nand.txt
>>  index 29ea5853ca91..a5b940f18bf6 100644
>>  --- a/Documentation/devicetree/bindings/mtd/ingenic,jz4780-nand.txt
>>  +++ b/Documentation/devicetree/bindings/mtd/ingenic,jz4780-nand.txt
>>  @@ -6,7 +6,10 @@ memory-controllers/ingenic,jz4780-nemc.txt), and 
>> thus NAND device nodes must
>>   be children of the NEMC node.
>> 
>>   Required NAND controller device properties:
>>  -- compatible: Should be set to "ingenic,jz4780-nand".
>>  +- compatible: Should be one of:
>>  +  * ingenic,jz4740-nand
>>  +  * ingenic,jz4725b-nand
>>  +  * ingenic,jz4780-nand
> 
> Wouldn't "-nand-controller" suffix be better? Of course in the driver
> you should still check for jz4780-nand.

So I would be compatible with:
* ingenic,jz4740-nand-controller
* ingenic,jz4725b-nand-controller
* ingenic,jz4780-nand
?

>>   - reg: For each bank with a NAND chip attached, should specify a 
>> bank number,
>>     an offset of 0 and a size of 0x1000000 (i.e. the whole NEMC 
>> bank).
>> 
>>  @@ -72,7 +75,10 @@ NAND devices. The following is a description of 
>> the device properties for a
>>   BCH controller.
>> 
>>   Required BCH properties:
>>  -- compatible: Should be set to "ingenic,jz4780-bch".
>>  +- compatible: Should be one of:
>>  +  * ingenic,jz4740-ecc
>>  +  * ingenic,jz4725b-bch
>>  +  * ingenic,jz4780-bch
>>   - reg: Should specify the BCH controller registers location and 
>> length.
>>   - clocks: Clock for the BCH controller.
>> 
> 
> Thanks,
> Miquèl
Paul Cercueil March 4, 2019, 6:26 p.m. | #7
On Mon, Mar 4, 2019 at 11:20 AM, Miquel Raynal 
<miquel.raynal@bootlin.com> wrote:
> Hi Paul,
> 
> Paul Cercueil <paul@crapouillou.net <mailto:paul@crapouillou.net>> 
> wrote on Sat,  9 Feb 2019 16:23:02
> -0300:
> 
>>  The ingenic-nand driver uses an API provided by the jz4780-bch 
>> driver.
>>  This makes it difficult to support other SoCs in the jz4780-bch 
>> driver.
>>  To work around this, we separate the API functions from the 
>> SoC-specific
>>  code, so that these API functions are SoC-agnostic.
>> 
> 
> I like the idea, actually I am working on this separation (see
> [1]) and I would really appreciate that you try to implement the
> interface when it will be available (v2 is coming this week, I think 
> v3
> will be the one to test when raw NAND devices will be properly
> supported). I will add you in Cc: if you want to follow/review.
> 
> [1] 
> <http://lists.infradead.org/pipermail/linux-mtd/2019-February/087815.html>

Do you think this will be ready for 5.2?

You can add me in Cc:.

>>  Signed-off-by: Paul Cercueil <paul@crapouillou.net 
>> <mailto:paul@crapouillou.net>>
>>  ---
>> 
>>  v2: Add an optional .probe() callback. It is used for instance to 
>> set
>>      the clock rate in the JZ4780 backend.
>> 
>>  v3: The common code is now inside the ingenic-ecc module. Each
>>      SoC-specific ECC code is now in its own module, which leaves to 
>> the
>>      user the choice of which (if any) ECC code should be supported.
>> 
>>  v4: No change
>> 
>>   drivers/mtd/nand/raw/ingenic/Kconfig        |  17 +++
>>   drivers/mtd/nand/raw/ingenic/Makefile       |   5 +-
>>   drivers/mtd/nand/raw/ingenic/ingenic_ecc.c  | 157 
>> +++++++++++++++++++++++++
>>   drivers/mtd/nand/raw/ingenic/ingenic_ecc.h  |  84 ++++++++++++++
>>   drivers/mtd/nand/raw/ingenic/ingenic_nand.c |  38 +++----
>>   drivers/mtd/nand/raw/ingenic/jz4780_bch.c   | 170 
>> +++++-----------------------
>>   drivers/mtd/nand/raw/ingenic/jz4780_bch.h   |  40 -------
>>   7 files changed, 308 insertions(+), 203 deletions(-)
>>   create mode 100644 drivers/mtd/nand/raw/ingenic/ingenic_ecc.c
>>   create mode 100644 drivers/mtd/nand/raw/ingenic/ingenic_ecc.h
>>   delete mode 100644 drivers/mtd/nand/raw/ingenic/jz4780_bch.h
>> 
> 
> [...]
> 
>>  diff --git a/drivers/mtd/nand/raw/ingenic/ingenic_nand.c 
>> b/drivers/mtd/nand/raw/ingenic/ingenic_nand.c
>>  index 8c73f7c5be9a..0f51fd15fe79 100644
>>  --- a/drivers/mtd/nand/raw/ingenic/ingenic_nand.c
>>  +++ b/drivers/mtd/nand/raw/ingenic/ingenic_nand.c
>>  @@ -22,7 +22,7 @@
>> 
>>   #include <linux/jz4780-nemc.h>
>> 
>>  -#include "jz4780_bch.h"
>>  +#include "ingenic_ecc.h"
>> 
>>   #define DRV_NAME	"ingenic-nand"
>> 
>>  @@ -40,7 +40,7 @@ struct ingenic_nand_cs {
>> 
>>   struct ingenic_nfc {
>>   	struct device *dev;
>>  -	struct jz4780_bch *bch;
>>  +	struct ingenic_ecc *ecc;
>>   	struct nand_controller controller;
>>   	unsigned int num_banks;
>>   	struct list_head chips;
>>  @@ -124,10 +124,10 @@ static int ingenic_nand_ecc_calculate(struct 
>> nand_chip *chip, const u8 *dat,
>>   {
>>   	struct ingenic_nand *nand = to_ingenic_nand(nand_to_mtd(chip));
>>   	struct ingenic_nfc *nfc = to_ingenic_nfc(nand->chip.controller);
>>  -	struct jz4780_bch_params params;
>>  +	struct ingenic_ecc_params params;
>> 
>>   	/*
>>  -	 * Don't need to generate the ECC when reading, BCH does it for 
>> us as
>>  +	 * Don't need to generate the ECC when reading, ECC does it for 
>> us as
> 
> "the ECC engine does it for us" would be more meaningful.

OK.

>>   	 * part of decoding/correction.
>>   	 */
>>   	if (nand->reading)
>>  @@ -137,7 +137,7 @@ static int ingenic_nand_ecc_calculate(struct 
>> nand_chip *chip, const u8 *dat,
>>   	params.bytes = nand->chip.ecc.bytes;
>>   	params.strength = nand->chip.ecc.strength;
>> 
>>  -	return jz4780_bch_calculate(nfc->bch, &params, dat, ecc_code);
>>  +	return ingenic_ecc_calculate(nfc->ecc, &params, dat, ecc_code);
>>   }
>> 
> 
> Thanks,
> Miquèl
Paul Cercueil March 4, 2019, 6:28 p.m. | #8
On Mon, Mar 4, 2019 at 11:34 AM, Miquel Raynal 
<miquel.raynal@bootlin.com> wrote:
> Hi Paul,
> 
> Paul Cercueil <paul@crapouillou.net <mailto:paul@crapouillou.net>> 
> wrote on Sat,  9 Feb 2019 16:23:03
> -0300:
> 
>>  Add support for probing the ingenic-nand driver on the JZ4740 SoC 
>> from
>>  Ingenic, and the jz4740-ecc driver to support the JZ4740-specific
>>  ECC hardware.
>> 
>>  Signed-off-by: Paul Cercueil <paul@crapouillou.net 
>> <mailto:paul@crapouillou.net>>
>>  ---
>> 
>>  Changes:
>> 
>>  v2: New patch
>> 
>>  v3: Also add support for the hardware ECC of the JZ4740 in this 
>> patch
>> 
>>  v4: - Fix formatting issues
>>      - Add MODULE_* macros
>> 
>>   drivers/mtd/nand/raw/ingenic/Kconfig        |  10 ++
>>   drivers/mtd/nand/raw/ingenic/Makefile       |   1 +
>>   drivers/mtd/nand/raw/ingenic/ingenic_nand.c |  48 +++++--
>>   drivers/mtd/nand/raw/ingenic/jz4740_ecc.c   | 196 
>> ++++++++++++++++++++++++++++
>>   4 files changed, 244 insertions(+), 11 deletions(-)
>>   create mode 100644 drivers/mtd/nand/raw/ingenic/jz4740_ecc.c
>> 
> 
> [...]
> 
>>   	switch (chip->ecc.mode) {
>>   	case NAND_ECC_HW:
>>  @@ -270,8 +279,8 @@ static int ingenic_nand_init_chip(struct 
>> platform_device *pdev,
>>   		return -ENOMEM;
>>   	mtd->dev.parent = dev;
>> 
>>  -	chip->legacy.IO_ADDR_R = cs->base + OFFSET_DATA;
>>  -	chip->legacy.IO_ADDR_W = cs->base + OFFSET_DATA;
>>  +	chip->legacy.IO_ADDR_R = cs->base + nfc->soc_info->data_offset;
>>  +	chip->legacy.IO_ADDR_W = cs->base + nfc->soc_info->data_offset;
>>   	chip->legacy.chip_delay = RB_DELAY_US;
>>   	chip->options = NAND_NO_SUBPAGE_WRITE;
>>   	chip->legacy.select_chip = ingenic_nand_select_chip;
> 
> I think Boris already asked for it, but it would be really great that
> you update this driver to not use any legacy interface anymore.

I thought I'd send a patch later. But I don't mind doing the update in
this patchset.

>>  diff --git a/drivers/mtd/nand/raw/ingenic/jz4740_ecc.c 
>> b/drivers/mtd/nand/raw/ingenic/jz4740_ecc.c
>>  new file mode 100644
>>  index 000000000000..83b42881720e
>>  --- /dev/null
>>  +++ b/drivers/mtd/nand/raw/ingenic/jz4740_ecc.c
>>  @@ -0,0 +1,196 @@
>>  +// SPDX-License-Identifier: GPL-2.0
>>  +/*
>>  + * JZ4740 ECC controller driver
>>  + *
>>  + * Copyright (c) 2019 Paul Cercueil <paul@crapouillou.net 
>> <mailto:paul@crapouillou.net>>
>>  + *
>>  + * based on jz4740-nand.c
>>  + */
>>  +
>>  +#include <linux/bitops.h>
>>  +#include <linux/device.h>
>>  +#include <linux/io.h>
>>  +#include <linux/module.h>
>>  +#include <linux/of_platform.h>
>>  +#include <linux/platform_device.h>
>>  +
>>  +#include "ingenic_ecc.h"
>>  +
>>  +#define JZ_REG_NAND_ECC_CTRL	0x00
>>  +#define JZ_REG_NAND_DATA	0x04
>>  +#define JZ_REG_NAND_PAR0	0x08
>>  +#define JZ_REG_NAND_PAR1	0x0C
>>  +#define JZ_REG_NAND_PAR2	0x10
>>  +#define JZ_REG_NAND_IRQ_STAT	0x14
>>  +#define JZ_REG_NAND_IRQ_CTRL	0x18
>>  +#define JZ_REG_NAND_ERR(x)	(0x1C + ((x) << 2))
>>  +
>>  +#define JZ_NAND_ECC_CTRL_PAR_READY	BIT(4)
>>  +#define JZ_NAND_ECC_CTRL_ENCODING	BIT(3)
>>  +#define JZ_NAND_ECC_CTRL_RS		BIT(2)
>>  +#define JZ_NAND_ECC_CTRL_RESET		BIT(1)
>>  +#define JZ_NAND_ECC_CTRL_ENABLE		BIT(0)
>>  +
>>  +#define JZ_NAND_STATUS_ERR_COUNT	(BIT(31) | BIT(30) | BIT(29))
>>  +#define JZ_NAND_STATUS_PAD_FINISH	BIT(4)
>>  +#define JZ_NAND_STATUS_DEC_FINISH	BIT(3)
>>  +#define JZ_NAND_STATUS_ENC_FINISH	BIT(2)
>>  +#define JZ_NAND_STATUS_UNCOR_ERROR	BIT(1)
>>  +#define JZ_NAND_STATUS_ERROR		BIT(0)
>>  +
>>  +static const uint8_t empty_block_ecc[] = {
>>  +	0xcd, 0x9d, 0x90, 0x58, 0xf4, 0x8b, 0xff, 0xb7, 0x6f
>>  +};
>>  +
>>  +static void jz4740_ecc_init(struct ingenic_ecc *ecc, bool encode)
>>  +{
>>  +	uint32_t reg;
>>  +
>>  +	/* Clear interrupt status */
>>  +	writel(0, ecc->base + JZ_REG_NAND_IRQ_STAT);
>>  +
>>  +	/* Initialize and enable ECC hardware */
>>  +	reg = readl(ecc->base + JZ_REG_NAND_ECC_CTRL);
>>  +	reg |= JZ_NAND_ECC_CTRL_RESET;
>>  +	reg |= JZ_NAND_ECC_CTRL_ENABLE;
>>  +	reg |= JZ_NAND_ECC_CTRL_RS;
>>  +	if (encode)
>>  +		reg |= JZ_NAND_ECC_CTRL_ENCODING;
>>  +	else
>>  +		reg &= ~JZ_NAND_ECC_CTRL_ENCODING;
>>  +
>>  +	writel(reg, ecc->base + JZ_REG_NAND_ECC_CTRL);
>>  +}
>>  +
>>  +static int jz4740_ecc_calculate(struct ingenic_ecc *ecc,
>>  +				struct ingenic_ecc_params *params,
>>  +				const u8 *buf, u8 *ecc_code)
>>  +{
>>  +	uint32_t reg, status;
>>  +	unsigned int timeout = 1000;
>>  +	int i;
>>  +
>>  +	jz4740_ecc_init(ecc, true);
>>  +
>>  +	do {
>>  +		status = readl(ecc->base + JZ_REG_NAND_IRQ_STAT);
>>  +	} while (!(status & JZ_NAND_STATUS_ENC_FINISH) && --timeout);
>>  +
>>  +	if (timeout == 0)
>>  +		return -ETIMEDOUT;
>>  +
>>  +	reg = readl(ecc->base + JZ_REG_NAND_ECC_CTRL);
>>  +	reg &= ~JZ_NAND_ECC_CTRL_ENABLE;
>>  +	writel(reg, ecc->base + JZ_REG_NAND_ECC_CTRL);
>>  +
>>  +	for (i = 0; i < params->bytes; ++i)
>>  +		ecc_code[i] = readb(ecc->base + JZ_REG_NAND_PAR0 + i);
>>  +
>>  +	/* If the written data is completely 0xff, we also want to write 
>> 0xff as
>>  +	 * ecc, otherwise we will get in trouble when doing subpage 
>> writes.
>>  +	 */
> 
> Comment formatting
> 
> s/ecc/ECC/ in plain English
> 
>>  +	if (memcmp(ecc_code, empty_block_ecc, 
>> ARRAY_SIZE(empty_block_ecc)) == 0)
>>  +		memset(ecc_code, 0xff, ARRAY_SIZE(empty_block_ecc));
>>  +
>>  +	return 0;
>>  +}
>>  +
> 
> 
> Thanks,
> Miquèl
Paul Cercueil March 4, 2019, 6:30 p.m. | #9
On Mon, Mar 4, 2019 at 11:35 AM, Miquel Raynal 
<miquel.raynal@bootlin.com> wrote:
> Hi Paul,
> 
> Paul Cercueil <paul@crapouillou.net <mailto:paul@crapouillou.net>> 
> wrote on Sat,  9 Feb 2019 16:23:04
> -0300:
> 
>>  The boot ROM of the JZ4725B SoC expects a specific OOB layout on the
>>  NAND, so we use it unconditionally in the ingenic-nand driver.
>> 
>>  Also add the jz4725b-bch driver to support the JZ4725B-specific BCH
>>  hardware.
>> 
>>  Signed-off-by: Paul Cercueil <paul@crapouillou.net 
>> <mailto:paul@crapouillou.net>>
>>  ---
>> 
>>  Changes:
>> 
>>  v2: Instead of forcing the OOB layout, leave it to the board code or
>>      devicetree to decide if the jz4725b-specific layout should be 
>> used
>>      or not.
>> 
>>  v3: - Revert the change in v2, as the previous behaviour was 
>> correct.
>>      - Also add support for the hardware BCH of the JZ4725B in this
>>        patch.
>> 
>>  v4: - Add MODULE_* macros
>>      - Add tweaks suggested by upstream feedback
>> 
>>   drivers/mtd/nand/raw/ingenic/Kconfig        |  10 +
>>   drivers/mtd/nand/raw/ingenic/Makefile       |   1 +
>>   drivers/mtd/nand/raw/ingenic/ingenic_nand.c |  48 ++++-
>>   drivers/mtd/nand/raw/ingenic/jz4725b_bch.c  | 292 
>> ++++++++++++++++++++++++++++
>>   4 files changed, 350 insertions(+), 1 deletion(-)
>>   create mode 100644 drivers/mtd/nand/raw/ingenic/jz4725b_bch.c
>> 
> 
> [...]
> 
>>  +static int jz4725b_calculate(struct ingenic_ecc *bch,
>>  +			     struct ingenic_ecc_params *params,
>>  +			     const u8 *buf, u8 *ecc_code)
>>  +{
>>  +	int ret;
>>  +
>>  +	mutex_lock(&bch->lock);
>>  +	ret = jz4725b_bch_init(bch, params, true);
> 
> I really don't like this bch_init name. A BCH initialization is what 
> is
> supposed to be done only once (probably at boot time), can you find a
> better name or a better organization of the correct/calculate path?

jz4725b_bch_setup() maybe?

>>  +	if (ret) {
>>  +		dev_err(bch->dev, "Unable to init BCH with given parameters\n");
>>  +		goto out_disable;
>>  +	}
>>  +
>>  +	jz4725b_bch_write_data(bch, buf, params->size);
>>  +
>>  +	ret = jz4725b_bch_wait_complete(bch, BCH_BHINT_ENCF, NULL);
>>  +	if (ret) {
>>  +		dev_err(bch->dev, "timed out while calculating ECC\n");
>>  +		goto out_disable;
>>  +	}
>>  +
>>  +	jz4725b_bch_read_parity(bch, ecc_code, params->bytes);
>>  +
>>  +out_disable:
>>  +	jz4725b_bch_disable(bch);
>>  +	mutex_unlock(&bch->lock);
>>  +
>>  +	return ret;
>>  +}
>>  +
> 
> [...]
> 
> Thanks,
> Miquèl
Miquel Raynal March 4, 2019, 6:51 p.m. | #10
Hi Paul,

> >>  --- a/Documentation/devicetree/bindings/mtd/ingenic,jz4780-nand.txt
> >>  +++ b/Documentation/devicetree/bindings/mtd/ingenic,jz4780-nand.txt
> >>  @@ -6,7 +6,10 @@ memory-controllers/ingenic,jz4780-nemc.txt), and >> thus NAND device nodes must
> >>   be children of the NEMC node.  
> >> >>   Required NAND controller device properties:  
> >>  -- compatible: Should be set to "ingenic,jz4780-nand".
> >>  +- compatible: Should be one of:
> >>  +  * ingenic,jz4740-nand
> >>  +  * ingenic,jz4725b-nand
> >>  +  * ingenic,jz4780-nand  
> > 
> > Wouldn't "-nand-controller" suffix be better? Of course in the driver
> > you should still check for jz4780-nand.  
> 
> So I would be compatible with:
> * ingenic,jz4740-nand-controller
> * ingenic,jz4725b-nand-controller
> * ingenic,jz4780-nand
> ?

>From a driver POV I would even prefer ingenic,jz4780-nand-controller. I
don't know what's best here. Maybe Boris or Rob can help?


Thanks,
Miquèl
Miquel Raynal March 4, 2019, 7:04 p.m. | #11
Hi Paul,

Paul Cercueil <paul@crapouillou.net> wrote on Mon, 04 Mar 2019 19:26:08
+0100:

> On Mon, Mar 4, 2019 at 11:20 AM, Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> > Hi Paul,
> > 
> > Paul Cercueil <paul@crapouillou.net <mailto:paul@crapouillou.net>> > wrote on Sat,  9 Feb 2019 16:23:02
> > -0300:
> >   
> >>  The ingenic-nand driver uses an API provided by the jz4780-bch >> driver.
> >>  This makes it difficult to support other SoCs in the jz4780-bch >> driver.
> >>  To work around this, we separate the API functions from the >> SoC-specific
> >>  code, so that these API functions are SoC-agnostic.  
> >> >   
> > I like the idea, actually I am working on this separation (see
> > [1]) and I would really appreciate that you try to implement the
> > interface when it will be available (v2 is coming this week, I think > v3
> > will be the one to test when raw NAND devices will be properly
> > supported). I will add you in Cc: if you want to follow/review.
> > 
> > [1] > <http://lists.infradead.org/pipermail/linux-mtd/2019-February/087815.html
> >  
> 
> Do you think this will be ready for 5.2?

Maybe, but I can't tell for sure. It will depend on how invasive the
raw NAND conversion is. I don't want to delay your work but maybe once
the interface will be ready to be implemented it would be a great
opportunity to do it with the Ingenic driver.

> 
> You can add me in Cc:.

Thanks!
Miquèl
Miquel Raynal March 4, 2019, 7:07 p.m. | #12
Hi Paul,

Paul Cercueil <paul@crapouillou.net> wrote on Mon, 04 Mar 2019 19:28:49
+0100:

> On Mon, Mar 4, 2019 at 11:34 AM, Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> > Hi Paul,
> > 
> > Paul Cercueil <paul@crapouillou.net <mailto:paul@crapouillou.net>> > wrote on Sat,  9 Feb 2019 16:23:03
> > -0300:
> >   
> >>  Add support for probing the ingenic-nand driver on the JZ4740 SoC >> from
> >>  Ingenic, and the jz4740-ecc driver to support the JZ4740-specific
> >>  ECC hardware.  
> >> >>  Signed-off-by: Paul Cercueil <paul@crapouillou.net >> <mailto:paul@crapouillou.net>>  
> >>  ---  
> >> >>  Changes:
> >> >>  v2: New patch
> >> >>  v3: Also add support for the hardware ECC of the JZ4740 in this >> patch
> >> >>  v4: - Fix formatting issues  
> >>      - Add MODULE_* macros  
> >> >>   drivers/mtd/nand/raw/ingenic/Kconfig        |  10 ++  
> >>   drivers/mtd/nand/raw/ingenic/Makefile       |   1 +
> >>   drivers/mtd/nand/raw/ingenic/ingenic_nand.c |  48 +++++--
> >>   drivers/mtd/nand/raw/ingenic/jz4740_ecc.c   | 196 >> ++++++++++++++++++++++++++++
> >>   4 files changed, 244 insertions(+), 11 deletions(-)
> >>   create mode 100644 drivers/mtd/nand/raw/ingenic/jz4740_ecc.c  
> >> >   
> > [...]
> >   
> >>   	switch (chip->ecc.mode) {
> >>   	case NAND_ECC_HW:
> >>  @@ -270,8 +279,8 @@ static int ingenic_nand_init_chip(struct >> platform_device *pdev,
> >>   		return -ENOMEM;
> >>   	mtd->dev.parent = dev;  
> >> >>  -	chip->legacy.IO_ADDR_R = cs->base + OFFSET_DATA;  
> >>  -	chip->legacy.IO_ADDR_W = cs->base + OFFSET_DATA;
> >>  +	chip->legacy.IO_ADDR_R = cs->base + nfc->soc_info->data_offset;
> >>  +	chip->legacy.IO_ADDR_W = cs->base + nfc->soc_info->data_offset;
> >>   	chip->legacy.chip_delay = RB_DELAY_US;
> >>   	chip->options = NAND_NO_SUBPAGE_WRITE;
> >>   	chip->legacy.select_chip = ingenic_nand_select_chip;  
> > 
> > I think Boris already asked for it, but it would be really great that
> > you update this driver to not use any legacy interface anymore.  
> 
> I thought I'd send a patch later. But I don't mind doing the update in
> this patchset.

Great! No it's okay, don't delay this patch set, doing it in another
thread is fine.
Miquel Raynal March 4, 2019, 7:09 p.m. | #13
Hi Paul,

Paul Cercueil <paul@crapouillou.net> wrote on Mon, 04 Mar 2019 19:30:04
+0100:

> On Mon, Mar 4, 2019 at 11:35 AM, Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> > Hi Paul,
> > 
> > Paul Cercueil <paul@crapouillou.net <mailto:paul@crapouillou.net>> > wrote on Sat,  9 Feb 2019 16:23:04
> > -0300:
> >   
> >>  The boot ROM of the JZ4725B SoC expects a specific OOB layout on the
> >>  NAND, so we use it unconditionally in the ingenic-nand driver.  
> >> >>  Also add the jz4725b-bch driver to support the JZ4725B-specific BCH  
> >>  hardware.  
> >> >>  Signed-off-by: Paul Cercueil <paul@crapouillou.net >> <mailto:paul@crapouillou.net>>  
> >>  ---  
> >> >>  Changes:
> >> >>  v2: Instead of forcing the OOB layout, leave it to the board code or  
> >>      devicetree to decide if the jz4725b-specific layout should be >> used
> >>      or not.  
> >> >>  v3: - Revert the change in v2, as the previous behaviour was >> correct.  
> >>      - Also add support for the hardware BCH of the JZ4725B in this
> >>        patch.  
> >> >>  v4: - Add MODULE_* macros  
> >>      - Add tweaks suggested by upstream feedback  
> >> >>   drivers/mtd/nand/raw/ingenic/Kconfig        |  10 +  
> >>   drivers/mtd/nand/raw/ingenic/Makefile       |   1 +
> >>   drivers/mtd/nand/raw/ingenic/ingenic_nand.c |  48 ++++-
> >>   drivers/mtd/nand/raw/ingenic/jz4725b_bch.c  | 292 >> ++++++++++++++++++++++++++++
> >>   4 files changed, 350 insertions(+), 1 deletion(-)
> >>   create mode 100644 drivers/mtd/nand/raw/ingenic/jz4725b_bch.c  
> >> >   
> > [...]
> >   
> >>  +static int jz4725b_calculate(struct ingenic_ecc *bch,
> >>  +			     struct ingenic_ecc_params *params,
> >>  +			     const u8 *buf, u8 *ecc_code)
> >>  +{
> >>  +	int ret;
> >>  +
> >>  +	mutex_lock(&bch->lock);
> >>  +	ret = jz4725b_bch_init(bch, params, true);  
> > 
> > I really don't like this bch_init name. A BCH initialization is what > is
> > supposed to be done only once (probably at boot time), can you find a
> > better name or a better organization of the correct/calculate path?  
> 
> jz4725b_bch_setup() maybe?

Unless I am not understanding what this does, I don't get why you would
need to do this setup everytime you want to use the ECC engine. Are you
sure this is needed?
Paul Cercueil March 13, 2019, 12:46 p.m. | #14
Hi Miquel,

Le lun. 4 mars 2019 à 16:09, Miquel Raynal <miquel.raynal@bootlin.com> 
a écrit :
> Hi Paul,
> 
> Paul Cercueil <paul@crapouillou.net> wrote on Mon, 04 Mar 2019 
> 19:30:04
> +0100:
> 
>>  On Mon, Mar 4, 2019 at 11:35 AM, Miquel Raynal 
>> <miquel.raynal@bootlin.com> wrote:
>>  > Hi Paul,
>>  >
>>  > Paul Cercueil <paul@crapouillou.net 
>> <mailto:paul@crapouillou.net>> > wrote on Sat,  9 Feb 2019 16:23:04
>>  > -0300:
>>  >
>>  >>  The boot ROM of the JZ4725B SoC expects a specific OOB layout 
>> on the
>>  >>  NAND, so we use it unconditionally in the ingenic-nand driver.
>>  >> >>  Also add the jz4725b-bch driver to support the 
>> JZ4725B-specific BCH
>>  >>  hardware.
>>  >> >>  Signed-off-by: Paul Cercueil <paul@crapouillou.net >> 
>> <mailto:paul@crapouillou.net>>
>>  >>  ---
>>  >> >>  Changes:
>>  >> >>  v2: Instead of forcing the OOB layout, leave it to the board 
>> code or
>>  >>      devicetree to decide if the jz4725b-specific layout should 
>> be >> used
>>  >>      or not.
>>  >> >>  v3: - Revert the change in v2, as the previous behaviour was 
>> >> correct.
>>  >>      - Also add support for the hardware BCH of the JZ4725B in 
>> this
>>  >>        patch.
>>  >> >>  v4: - Add MODULE_* macros
>>  >>      - Add tweaks suggested by upstream feedback
>>  >> >>   drivers/mtd/nand/raw/ingenic/Kconfig        |  10 +
>>  >>   drivers/mtd/nand/raw/ingenic/Makefile       |   1 +
>>  >>   drivers/mtd/nand/raw/ingenic/ingenic_nand.c |  48 ++++-
>>  >>   drivers/mtd/nand/raw/ingenic/jz4725b_bch.c  | 292 >> 
>> ++++++++++++++++++++++++++++
>>  >>   4 files changed, 350 insertions(+), 1 deletion(-)
>>  >>   create mode 100644 drivers/mtd/nand/raw/ingenic/jz4725b_bch.c
>>  >> >
>>  > [...]
>>  >
>>  >>  +static int jz4725b_calculate(struct ingenic_ecc *bch,
>>  >>  +			     struct ingenic_ecc_params *params,
>>  >>  +			     const u8 *buf, u8 *ecc_code)
>>  >>  +{
>>  >>  +	int ret;
>>  >>  +
>>  >>  +	mutex_lock(&bch->lock);
>>  >>  +	ret = jz4725b_bch_init(bch, params, true);
>>  >
>>  > I really don't like this bch_init name. A BCH initialization is 
>> what > is
>>  > supposed to be done only once (probably at boot time), can you 
>> find a
>>  > better name or a better organization of the correct/calculate 
>> path?
>> 
>>  jz4725b_bch_setup() maybe?
> 
> Unless I am not understanding what this does, I don't get why you 
> would
> need to do this setup everytime you want to use the ECC engine. Are 
> you
> sure this is needed?

It configures the hardware for a new ECC encoding or decoding sequence, 
so
yes, it has to be done everytime.

-Paul
Paul Cercueil March 13, 2019, 12:55 p.m. | #15
Hi,

Le lun. 4 mars 2019 à 15:51, Miquel Raynal <miquel.raynal@bootlin.com> 
a écrit :
> Hi Paul,
> 
>>  >>  --- 
>> a/Documentation/devicetree/bindings/mtd/ingenic,jz4780-nand.txt
>>  >>  +++ 
>> b/Documentation/devicetree/bindings/mtd/ingenic,jz4780-nand.txt
>>  >>  @@ -6,7 +6,10 @@ memory-controllers/ingenic,jz4780-nemc.txt), 
>> and >> thus NAND device nodes must
>>  >>   be children of the NEMC node.
>>  >> >>   Required NAND controller device properties:
>>  >>  -- compatible: Should be set to "ingenic,jz4780-nand".
>>  >>  +- compatible: Should be one of:
>>  >>  +  * ingenic,jz4740-nand
>>  >>  +  * ingenic,jz4725b-nand
>>  >>  +  * ingenic,jz4780-nand
>>  >
>>  > Wouldn't "-nand-controller" suffix be better? Of course in the 
>> driver
>>  > you should still check for jz4780-nand.
>> 
>>  So I would be compatible with:
>>  * ingenic,jz4740-nand-controller
>>  * ingenic,jz4725b-nand-controller
>>  * ingenic,jz4780-nand
>>  ?
> 
> From a driver POV I would even prefer ingenic,jz4780-nand-controller. 
> I
> don't know what's best here. Maybe Boris or Rob can help?

The "ingenic,jz4780-nand" compatible string is already out there and 
used
in devicetree files, so I wouldn't change it just for the sake of it.

-Paul
Boris Brezillon March 13, 2019, 1:09 p.m. | #16
On Wed, 13 Mar 2019 09:55:34 -0300
Paul Cercueil <paul@crapouillou.net> wrote:

> Hi,
> 
> Le lun. 4 mars 2019 à 15:51, Miquel Raynal <miquel.raynal@bootlin.com> 
> a écrit :
> > Hi Paul,
> >   
> >>  >>  ---   
> >> a/Documentation/devicetree/bindings/mtd/ingenic,jz4780-nand.txt  
> >>  >>  +++   
> >> b/Documentation/devicetree/bindings/mtd/ingenic,jz4780-nand.txt  
> >>  >>  @@ -6,7 +6,10 @@ memory-controllers/ingenic,jz4780-nemc.txt),   
> >> and >> thus NAND device nodes must  
> >>  >>   be children of the NEMC node.  
> >>  >> >>   Required NAND controller device properties:  
> >>  >>  -- compatible: Should be set to "ingenic,jz4780-nand".
> >>  >>  +- compatible: Should be one of:
> >>  >>  +  * ingenic,jz4740-nand
> >>  >>  +  * ingenic,jz4725b-nand
> >>  >>  +  * ingenic,jz4780-nand  
> >>  >
> >>  > Wouldn't "-nand-controller" suffix be better? Of course in the   
> >> driver  
> >>  > you should still check for jz4780-nand.  
> >> 
> >>  So I would be compatible with:
> >>  * ingenic,jz4740-nand-controller
> >>  * ingenic,jz4725b-nand-controller
> >>  * ingenic,jz4780-nand
> >>  ?  
> > 
> > From a driver POV I would even prefer ingenic,jz4780-nand-controller. 
> > I
> > don't know what's best here. Maybe Boris or Rob can help?

Let's keep it consistent and have all compatibles follow the old
naming scheme (ingenic,<soc>-nand). But yes, for new drivers, I agree
that -nand-controller is better than just -nand.

Patch

diff --git a/Documentation/devicetree/bindings/mtd/ingenic,jz4780-nand.txt b/Documentation/devicetree/bindings/mtd/ingenic,jz4780-nand.txt
index 29ea5853ca91..a5b940f18bf6 100644
--- a/Documentation/devicetree/bindings/mtd/ingenic,jz4780-nand.txt
+++ b/Documentation/devicetree/bindings/mtd/ingenic,jz4780-nand.txt
@@ -6,7 +6,10 @@  memory-controllers/ingenic,jz4780-nemc.txt), and thus NAND device nodes must
 be children of the NEMC node.
 
 Required NAND controller device properties:
-- compatible: Should be set to "ingenic,jz4780-nand".
+- compatible: Should be one of:
+  * ingenic,jz4740-nand
+  * ingenic,jz4725b-nand
+  * ingenic,jz4780-nand
 - reg: For each bank with a NAND chip attached, should specify a bank number,
   an offset of 0 and a size of 0x1000000 (i.e. the whole NEMC bank).
 
@@ -72,7 +75,10 @@  NAND devices. The following is a description of the device properties for a
 BCH controller.
 
 Required BCH properties:
-- compatible: Should be set to "ingenic,jz4780-bch".
+- compatible: Should be one of:
+  * ingenic,jz4740-ecc
+  * ingenic,jz4725b-bch
+  * ingenic,jz4780-bch
 - reg: Should specify the BCH controller registers location and length.
 - clocks: Clock for the BCH controller.