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

[Paul Cercueil <paul@crapouillou.net>]

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