Re: [PATCH v2] mtd: atmel_nand: make PMECC lookup table and offset property optional

[Brian Norris <computersforpeace-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>]

On Mon, Aug 18, 2014 at 03:08:40PM +0800, Josh Wu wrote:
> If there is no PMECC lookup table stored in ROM, or lookup table offset is
> not specified, PMECC driver should build it in DDR by itself.
> 
> That make the PMECC driver work for some board which doesn't has PMECC
> lookup table in ROM.
> 
> Signed-off-by: Josh Wu <josh.wu-AIFe0yeh4nAAvxtiuMwx3w@public.gmane.org>
> Cc: devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> ---
> This patch is based on mtd-l2/next branch
> 
> v1 -> v2:
>   make create_lookup_table() static.

sparse gives me several new complaints:

+drivers/mtd/nand/atmel_nand.c:1219:50: warning: incorrect type in argument 2 (different signedness) [sparse]
+drivers/mtd/nand/atmel_nand.c:1219:50:    expected signed short [usertype] *index_of [sparse]
+drivers/mtd/nand/atmel_nand.c:1219:50:    got unsigned short [usertype] *addr [sparse]
+drivers/mtd/nand/atmel_nand.c:1219:61: warning: incorrect type in argument 3 (different signedness) [sparse]
+drivers/mtd/nand/atmel_nand.c:1219:61:    expected signed short [usertype] *alpha_to [sparse]
+drivers/mtd/nand/atmel_nand.c:1219:61:    got unsigned short [usertype] * [sparse]
+drivers/mtd/nand/atmel_nand.c:1292:38: warning: incorrect type in assignment (different address spaces) [sparse]
+drivers/mtd/nand/atmel_nand.c:1292:38:    expected void [noderef] <asn:2>*pmecc_rom_base [sparse]
+drivers/mtd/nand/atmel_nand.c:1292:38:    got unsigned short [usertype] *[assigned] galois_table [sparse]

The third one might be a false positive. I suppose it's safe to use
regular memory with __iomem accessors (but not vice versa).

>  .../devicetree/bindings/mtd/atmel-nand.txt         |   6 +-
>  drivers/mtd/nand/atmel_nand.c                      | 136 ++++++++++++++++++++-
>  2 files changed, 136 insertions(+), 6 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/mtd/atmel-nand.txt b/Documentation/devicetree/bindings/mtd/atmel-nand.txt
> index c472883..75d1847 100644
> --- a/Documentation/devicetree/bindings/mtd/atmel-nand.txt
> +++ b/Documentation/devicetree/bindings/mtd/atmel-nand.txt
> @@ -5,7 +5,9 @@ Required properties:
>  - 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
> -	PMECC, PMECC Error Location controller and ROM which has lookup tables.
> +	PMECC and PMECC Error Location controller.
> +	The PMECC lookup table address and size in ROM is optional. If not
> +	specified, driver will build it in runtime.
>  - atmel,nand-addr-offset : offset for the address latch.
>  - atmel,nand-cmd-offset : offset for the command latch.
>  - #address-cells, #size-cells : Must be present if the device has sub-nodes
> @@ -27,7 +29,7 @@ Optional properties:
>    are: 512, 1024.
>  - atmel,pmecc-lookup-table-offset : includes two offsets of lookup table in ROM
>    for different sector size. First one is for sector size 512, the next is for
> -  sector size 1024.
> +  sector size 1024. If not specified, driver will build the table in runtime.
>  - nand-bus-width : 8 or 16 bus width if not present 8
>  - nand-on-flash-bbt: boolean to enable on flash bbt option if not present false
>  - Nand Flash Controller(NFC) is a slave driver under Atmel nand flash
> diff --git a/drivers/mtd/nand/atmel_nand.c b/drivers/mtd/nand/atmel_nand.c
> index effa7a29..84af313 100644
> --- a/drivers/mtd/nand/atmel_nand.c
> +++ b/drivers/mtd/nand/atmel_nand.c
> @@ -124,6 +124,7 @@ struct atmel_nand_host {
>  	bool			has_pmecc;
>  	u8			pmecc_corr_cap;
>  	u16			pmecc_sector_size;
> +	bool			has_no_lookup_table;
>  	u32			pmecc_lookup_table_offset;
>  	u32			pmecc_lookup_table_offset_512;
>  	u32			pmecc_lookup_table_offset_1024;
> @@ -1109,12 +1110,121 @@ static int pmecc_choose_ecc(struct atmel_nand_host *host,
>  	return 0;
>  }
>  
> +static int pmecc_build_galois_table(int mm,
> +		int16_t *index_of, int16_t *alpha_to)
> +{
> +	unsigned int i, mask, nn;
> +	unsigned int p[PMECC_GF_DIMENSION_14 + 1];
> +
> +	nn = (1 << mm) - 1;
> +	/* set default value */
> +	for (i = 1; i < mm; i++)
> +		p[i] = 0;
> +
> +	/* 1 + X^mm */
> +	p[0]  = 1;
> +	p[mm] = 1;
> +
> +	/* others  */
> +	switch (mm) {
> +	case 3:
> +	case 4:
> +	case 6:
> +	case 15:
> +		p[1] = 1;
> +		break;
> +	case 5:
> +	case 11:
> +		p[2] = 1;
> +		break;
> +	case 7:
> +	case 10:
> +		p[3] = 1;
> +		break;
> +	case 8:
> +		p[2] = p[3] = p[4] = 1;
> +		break;
> +	case 9:
> +		p[4] = 1;
> +		break;
> +	case 12:
> +		p[1] = p[4] = p[6] = 1;
> +		break;
> +	case 13:
> +		p[1] = p[3] = p[4] = 1;
> +		break;
> +	case 14:
> +		p[1] = p[6] = p[10] = 1;
> +		break;
> +	default:
> +		/* Error */
> +		return -EINVAL;
> +	}
> +
> +	/* Build alpha ^ mm it will help to generate the field (primitiv) */
> +	alpha_to[mm] = 0;
> +	for (i = 0; i < mm; i++)
> +		if (p[i])
> +			alpha_to[mm] |= 1 << i;
> +
> +	/*
> +	 * Then build elements from 0 to mm - 1. As degree is less than mm
> +	 * so it is just a logical shift.
> +	 */
> +	mask = 1;
> +	for (i = 0; i < mm; i++) {
> +		alpha_to[i] = mask;
> +		index_of[alpha_to[i]] = i;
> +		mask <<= 1;
> +	}
> +
> +	index_of[alpha_to[mm]] = mm;
> +
> +	/* use a mask to select the MSB bit of the LFSR */
> +	mask >>= 1;
> +
> +	/* then finish the building */
> +	for (i = mm + 1; i <= nn; i++) {
> +		/* check if the msb bit of the lfsr is set */
> +		if (alpha_to[i - 1] & mask)
> +			alpha_to[i] = alpha_to[mm] ^
> +				((alpha_to[i - 1] ^ mask) << 1);
> +		else
> +			alpha_to[i] = alpha_to[i - 1] << 1;
> +
> +		index_of[alpha_to[i]] = i % nn;
> +	}
> +
> +	/* index of 0 is undefined in a multiplicative field */
> +	index_of[0] = -1;
> +
> +	return 0;
> +}

Is this algorithm documented? How did you come up with this?

> +
> +static uint16_t *create_lookup_table(struct device *dev, int sector_size)
> +{
> +	int table_size = (sector_size == 512) ?
> +			PMECC_LOOKUP_TABLE_SIZE_512 :
> +			PMECC_LOOKUP_TABLE_SIZE_1024;
> +	int degree = (sector_size == 512) ?
> +			PMECC_GF_DIMENSION_13 :
> +			PMECC_GF_DIMENSION_14;
> +	uint16_t *addr = devm_kzalloc(dev, 2 * table_size * sizeof(uint16_t),
> +			GFP_KERNEL);
> +
> +	if (addr)
> +		pmecc_build_galois_table(degree, addr, addr + table_size);
> +
> +	return addr;
> +}
> +
>  static int atmel_pmecc_nand_init_params(struct platform_device *pdev,
>  					 struct atmel_nand_host *host)
>  {
>  	struct mtd_info *mtd = &host->mtd;
>  	struct nand_chip *nand_chip = &host->nand_chip;
>  	struct resource *regs, *regs_pmerr, *regs_rom;
> +	uint16_t *galois_table;
>  	int cap, sector_size, err_no;
>  
>  	err_no = pmecc_choose_ecc(host, &cap, &sector_size);
> @@ -1160,8 +1270,24 @@ static int atmel_pmecc_nand_init_params(struct platform_device *pdev,
>  	regs_rom = platform_get_resource(pdev, IORESOURCE_MEM, 3);
>  	host->pmecc_rom_base = devm_ioremap_resource(&pdev->dev, regs_rom);
>  	if (IS_ERR(host->pmecc_rom_base)) {
> -		err_no = PTR_ERR(host->pmecc_rom_base);
> -		goto err;
> +		if (!host->has_no_lookup_table)
> +			/* Don't display the information again */
> +			dev_err(host->dev, "Can not get I/O resource for ROM, will build a lookup table in runtime!\n");
> +
> +		host->has_no_lookup_table = true;
> +	}
> +
> +	if (host->has_no_lookup_table) {
> +		/* Build the look-up table in runtime */
> +		galois_table = create_lookup_table(host->dev, sector_size);
> +		if (!galois_table) {
> +			dev_err(host->dev, "Failed to build a lookup table in runtime!\n");
> +			err_no = -ENOMEM;
> +			goto err;
> +		}
> +
> +		host->pmecc_rom_base = galois_table;
> +		host->pmecc_lookup_table_offset = 0;
>  	}
>  
>  	nand_chip->ecc.size = sector_size;
> @@ -1498,8 +1624,10 @@ static int atmel_of_init_port(struct atmel_nand_host *host,
>  
>  	if (of_property_read_u32_array(np, "atmel,pmecc-lookup-table-offset",
>  			offset, 2) != 0) {
> -		dev_err(host->dev, "Cannot get PMECC lookup table offset\n");
> -		return -EINVAL;
> +		dev_err(host->dev, "Cannot get PMECC lookup table offset, will build a lookup table in runtime.\n");
> +		host->has_no_lookup_table = true;
> +		/* Will build a lookup table and initialize the offset later */
> +		return 0;
>  	}
>  	if (!offset[0] && !offset[1]) {
>  		dev_err(host->dev, "Invalid PMECC lookup table offset\n");

Brian
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html