Re: [PATCH v4 7/9] mtd: rawnand: ingenic: Add support for the JZ4740

[Paul Cercueil]

On Mon, Mar 4, 2019 at 11:34 AM, Miquel Raynal 
<miquel.raynal@xxxxxxxxxxx> wrote:
> Hi Paul,
>
> Paul Cercueil <paul@xxxxxxxxxxxxxxx <mailto:paul@xxxxxxxxxxxxxxx>> 
> 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@xxxxxxxxxxxxxxx 
> > <mailto:paul@xxxxxxxxxxxxxxx>>
> >  ---
> >
> >  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@xxxxxxxxxxxxxxx 
> > <mailto:paul@xxxxxxxxxxxxxxx>>
> >  + *
> >  + * 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