lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20150829094815.GD5594@opentech.at>
Date:	Sat, 29 Aug 2015 11:48:15 +0200
From:	Nicholas Mc Guire <der.herr@...r.at>
To:	Han Xu <b45815@...escale.com>
Cc:	shijie.huang@....com, dwmw2@...radead.org,
	computersforpeace@...il.com, boris.brezillon@...e-electrons.com,
	fabio.estevam@...escale.com, hofrat@...dl.org,
	linux-mtd@...ts.infradead.org, linux-kernel@...r.kernel.org,
	vinod.koul@...el.com, dan.j.williams@...el.com,
	dmaengine@...r.kernel.org
Subject: Re: [PATCH v3 4/6] mtd: nand: gpmi: add GPMI NAND support for
	i.MX7D

On Fri, 28 Aug 2015, Han Xu wrote:

> support GPMI NAND on i.MX7D
>

minor coding style fixup suggestion in 
gpmi_extra_init()
gpmi_nand_init()
gpmi_init_last()
see below.
 
> Signed-off-by: Han Xu <b45815@...escale.com>
> ---
>  drivers/mtd/nand/gpmi-nand/bch-regs.h  | 14 +++++++-------
>  drivers/mtd/nand/gpmi-nand/gpmi-lib.c  | 10 ++++++----
>  drivers/mtd/nand/gpmi-nand/gpmi-nand.c | 24 ++++++++++++++++++++----
>  drivers/mtd/nand/gpmi-nand/gpmi-nand.h |  7 +++++--
>  4 files changed, 38 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/mtd/nand/gpmi-nand/bch-regs.h b/drivers/mtd/nand/gpmi-nand/bch-regs.h
> index 05bb91f..53e58bc 100644
> --- a/drivers/mtd/nand/gpmi-nand/bch-regs.h
> +++ b/drivers/mtd/nand/gpmi-nand/bch-regs.h
> @@ -1,7 +1,7 @@
>  /*
>   * Freescale GPMI NAND Flash Driver
>   *
> - * Copyright 2008-2011 Freescale Semiconductor, Inc.
> + * Copyright 2008-2015 Freescale Semiconductor, Inc.
>   * Copyright 2008 Embedded Alley Solutions, Inc.
>   *
>   * This program is free software; you can redistribute it and/or modify
> @@ -54,7 +54,7 @@
>  #define MX6Q_BP_BCH_FLASH0LAYOUT0_ECC0		11
>  #define MX6Q_BM_BCH_FLASH0LAYOUT0_ECC0	(0x1f << MX6Q_BP_BCH_FLASH0LAYOUT0_ECC0)
>  #define BF_BCH_FLASH0LAYOUT0_ECC0(v, x)				\
> -	(GPMI_IS_MX6(x)					\
> +	((GPMI_IS_MX6(x) || GPMI_IS_MX7(x))				\
>  		? (((v) << MX6Q_BP_BCH_FLASH0LAYOUT0_ECC0)	\
>  			& MX6Q_BM_BCH_FLASH0LAYOUT0_ECC0)	\
>  		: (((v) << BP_BCH_FLASH0LAYOUT0_ECC0)		\
> @@ -65,7 +65,7 @@
>  #define MX6Q_BM_BCH_FLASH0LAYOUT0_GF_13_14			\
>  				(0x1 << MX6Q_BP_BCH_FLASH0LAYOUT0_GF_13_14)
>  #define BF_BCH_FLASH0LAYOUT0_GF(v, x)				\
> -	((GPMI_IS_MX6(x) && ((v) == 14))			\
> +	(((GPMI_IS_MX6(x) || GPMI_IS_MX7(x)) && ((v) == 14))\
>  		? (((1) << MX6Q_BP_BCH_FLASH0LAYOUT0_GF_13_14)	\
>  			& MX6Q_BM_BCH_FLASH0LAYOUT0_GF_13_14)	\
>  		: 0						\
> @@ -77,7 +77,7 @@
>  #define MX6Q_BM_BCH_FLASH0LAYOUT0_DATA0_SIZE	\
>  			(0x3ff << BP_BCH_FLASH0LAYOUT0_DATA0_SIZE)
>  #define BF_BCH_FLASH0LAYOUT0_DATA0_SIZE(v, x)				\
> -	(GPMI_IS_MX6(x)						\
> +	((GPMI_IS_MX6(x) || GPMI_IS_MX7(x))	\
>  		? (((v) >> 2) & MX6Q_BM_BCH_FLASH0LAYOUT0_DATA0_SIZE)	\
>  		: ((v) & BM_BCH_FLASH0LAYOUT0_DATA0_SIZE)		\
>  	)
> @@ -96,7 +96,7 @@
>  #define MX6Q_BP_BCH_FLASH0LAYOUT1_ECCN		11
>  #define MX6Q_BM_BCH_FLASH0LAYOUT1_ECCN	(0x1f << MX6Q_BP_BCH_FLASH0LAYOUT1_ECCN)
>  #define BF_BCH_FLASH0LAYOUT1_ECCN(v, x)				\
> -	(GPMI_IS_MX6(x)					\
> +	((GPMI_IS_MX6(x) || GPMI_IS_MX7(x))				\
>  		? (((v) << MX6Q_BP_BCH_FLASH0LAYOUT1_ECCN)	\
>  			& MX6Q_BM_BCH_FLASH0LAYOUT1_ECCN)	\
>  		: (((v) << BP_BCH_FLASH0LAYOUT1_ECCN)		\
> @@ -107,7 +107,7 @@
>  #define MX6Q_BM_BCH_FLASH0LAYOUT1_GF_13_14			\
>  				(0x1 << MX6Q_BP_BCH_FLASH0LAYOUT1_GF_13_14)
>  #define BF_BCH_FLASH0LAYOUT1_GF(v, x)				\
> -	((GPMI_IS_MX6(x) && ((v) == 14))			\
> +	(((GPMI_IS_MX6(x) || GPMI_IS_MX7(x)) && ((v) == 14))\
>  		? (((1) << MX6Q_BP_BCH_FLASH0LAYOUT1_GF_13_14)	\
>  			& MX6Q_BM_BCH_FLASH0LAYOUT1_GF_13_14)	\
>  		: 0						\
> @@ -119,7 +119,7 @@
>  #define MX6Q_BM_BCH_FLASH0LAYOUT1_DATAN_SIZE	\
>  			(0x3ff << BP_BCH_FLASH0LAYOUT1_DATAN_SIZE)
>  #define BF_BCH_FLASH0LAYOUT1_DATAN_SIZE(v, x)				\
> -	(GPMI_IS_MX6(x)						\
> +	((GPMI_IS_MX6(x) || GPMI_IS_MX7(x))	\
>  		? (((v) >> 2) & MX6Q_BM_BCH_FLASH0LAYOUT1_DATAN_SIZE)	\
>  		: ((v) & BM_BCH_FLASH0LAYOUT1_DATAN_SIZE)		\
>  	)
> diff --git a/drivers/mtd/nand/gpmi-nand/gpmi-lib.c b/drivers/mtd/nand/gpmi-nand/gpmi-lib.c
> index 43fa16b..6b37414 100644
> --- a/drivers/mtd/nand/gpmi-nand/gpmi-lib.c
> +++ b/drivers/mtd/nand/gpmi-nand/gpmi-lib.c
> @@ -1,7 +1,7 @@
>  /*
>   * Freescale GPMI NAND Flash Driver
>   *
> - * Copyright (C) 2008-2011 Freescale Semiconductor, Inc.
> + * Copyright (C) 2008-2015 Freescale Semiconductor, Inc.
>   * Copyright (C) 2008 Embedded Alley Solutions, Inc.
>   *
>   * This program is free software; you can redistribute it and/or modify
> @@ -971,7 +971,8 @@ int gpmi_extra_init(struct gpmi_nand_data *this)
>  	struct nand_chip *chip = &this->nand;
>  
>  	/* Enable the asynchronous EDO feature. */
> -	if (GPMI_IS_MX6(this) && chip->onfi_version) {
> +	if ((GPMI_IS_MX6(this) || GPMI_IS_MX7(this))
> +			&& chip->onfi_version) {

This probably should be:
	if ((GPMI_IS_MX6(this) || GPMI_IS_MX7(this)) &&
			 chip->onfi_version) {

>  		int mode = onfi_get_async_timing_mode(chip);
>  
>  		/* We only support the timing mode 4 and mode 5. */
> @@ -1093,12 +1094,13 @@ int gpmi_is_ready(struct gpmi_nand_data *this, unsigned chip)
>  	if (GPMI_IS_MX23(this)) {
>  		mask = MX23_BM_GPMI_DEBUG_READY0 << chip;
>  		reg = readl(r->gpmi_regs + HW_GPMI_DEBUG);
> -	} else if (GPMI_IS_MX28(this) || GPMI_IS_MX6(this)) {
> +	} else if (GPMI_IS_MX28(this) || GPMI_IS_MX6(this) ||
> +			GPMI_IS_MX7(this)) {
>  		/*
>  		 * In the imx6, all the ready/busy pins are bound
>  		 * together. So we only need to check chip 0.
>  		 */
> -		if (GPMI_IS_MX6(this))
> +		if (GPMI_IS_MX6(this) || GPMI_IS_MX7(this))
>  			chip = 0;
>  
>  		/* MX28 shares the same R/B register as MX6Q. */
> diff --git a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
> index 632876c..4e52885 100644
> --- a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
> +++ b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
> @@ -77,6 +77,12 @@ static const struct gpmi_devdata gpmi_devdata_imx6sx = {
>  	.max_chain_delay = 12,
>  };
>  
> +static const struct gpmi_devdata gpmi_devdata_imx7d = {
> +	.type = IS_MX7D,
> +	.bch_max_ecc_strength = 62,
> +	.max_chain_delay = 12,
> +};
> +
>  static irqreturn_t bch_irq(int irq, void *cookie)
>  {
>  	struct gpmi_nand_data *this = cookie;
> @@ -575,6 +581,10 @@ static char *extra_clks_for_mx6q[GPMI_CLK_MAX] = {
>  	"gpmi_apb", "gpmi_bch", "gpmi_bch_apb", "per1_bch",
>  };
>  
> +static char *extra_clks_for_mx7d[GPMI_CLK_MAX] = {
> +	"gpmi_bch_apb",
> +};
> +
>  static int gpmi_get_clks(struct gpmi_nand_data *this)
>  {
>  	struct resources *r = &this->resources;
> @@ -592,6 +602,8 @@ static int gpmi_get_clks(struct gpmi_nand_data *this)
>  	/* Get extra clocks */
>  	if (GPMI_IS_MX6(this))
>  		extra_clks = extra_clks_for_mx6q;
> +	if (GPMI_IS_MX7(this))
> +		extra_clks = extra_clks_for_mx7d;
>  	if (!extra_clks)
>  		return 0;
>  
> @@ -608,7 +620,7 @@ static int gpmi_get_clks(struct gpmi_nand_data *this)
>  		r->clock[i] = clk;
>  	}
>  
> -	if (GPMI_IS_MX6(this))
> +	if (GPMI_IS_MX6(this) || GPMI_IS_MX7(this))
>  		/*
>  		 * Set the default value for the gpmi clock.
>  		 *
> @@ -1869,7 +1881,7 @@ static int gpmi_init_last(struct gpmi_nand_data *this)
>  	 *  (1) the chip is imx6, and
>  	 *  (2) the size of the ECC parity is byte aligned.
>  	 */
> -	if (GPMI_IS_MX6(this) &&
> +	if ((GPMI_IS_MX6(this) || GPMI_IS_MX7(this)) &&
>  		((bch_geo->gf_len * bch_geo->ecc_strength) % 8) == 0) {

looks like that indentation needs fixing 
	if ((GPMI_IS_MX6(this) || GPMI_IS_MX7(this)) &&
  	    ((bch_geo->gf_len * bch_geo->ecc_strength) % 8) == 0) {
or
	if ((GPMI_IS_MX6(this) || GPMI_IS_MX7(this)) &&
 			((bch_geo->gf_len * bch_geo->ecc_strength) % 8) == 0) {

>  		ecc->read_subpage = gpmi_ecc_read_subpage;
>  		chip->options |= NAND_SUBPAGE_READ;
> @@ -1936,7 +1948,8 @@ static int gpmi_nand_init(struct gpmi_nand_data *this)
>  	if (ret)
>  		goto err_out;
>  
> -	ret = nand_scan_ident(mtd, GPMI_IS_MX6(this) ? 2 : 1, NULL);
> +	ret = nand_scan_ident(mtd, GPMI_IS_MX6(this) || GPMI_IS_MX7(this)\
> +				? 2 : 1, NULL);

I think this should preferably be:

	ret = nand_scan_ident(mtd, GPMI_IS_MX6(this) ||
			      GPMI_IS_MX7(this) ? 2 : 1, NULL);

>  	if (ret)
>  		goto err_out;
>  
> @@ -1980,7 +1993,10 @@ static const struct of_device_id gpmi_nand_id_table[] = {
>  	}, {
>  		.compatible = "fsl,imx6sx-gpmi-nand",
>  		.data = &gpmi_devdata_imx6sx,
> -	}, {}
> +	}, {
> +		.compatible = "fsl,imx7d-gpmi-nand",
> +		.data = (void *)&gpmi_devdata_imx7d,
> +	}
>  };
>  MODULE_DEVICE_TABLE(of, gpmi_nand_id_table);
>  
> diff --git a/drivers/mtd/nand/gpmi-nand/gpmi-nand.h b/drivers/mtd/nand/gpmi-nand/gpmi-nand.h
> index 544062f..58b3d69 100644
> --- a/drivers/mtd/nand/gpmi-nand/gpmi-nand.h
> +++ b/drivers/mtd/nand/gpmi-nand/gpmi-nand.h
> @@ -1,7 +1,7 @@
>  /*
>   * Freescale GPMI NAND Flash Driver
>   *
> - * Copyright (C) 2010-2011 Freescale Semiconductor, Inc.
> + * Copyright (C) 2010-2015 Freescale Semiconductor, Inc.
>   * Copyright (C) 2008 Embedded Alley Solutions, Inc.
>   *
>   * This program is free software; you can redistribute it and/or modify
> @@ -123,7 +123,8 @@ enum gpmi_type {
>  	IS_MX23,
>  	IS_MX28,
>  	IS_MX6Q,
> -	IS_MX6SX
> +	IS_MX6SX,
> +	IS_MX7D,
>  };
>  
>  struct gpmi_devdata {
> @@ -306,6 +307,8 @@ void gpmi_copy_bits(u8 *dst, size_t dst_bit_off,
>  #define GPMI_IS_MX28(x)		((x)->devdata->type == IS_MX28)
>  #define GPMI_IS_MX6Q(x)		((x)->devdata->type == IS_MX6Q)
>  #define GPMI_IS_MX6SX(x)	((x)->devdata->type == IS_MX6SX)
> +#define GPMI_IS_MX7D(x)		((x)->devdata->type == IS_MX7D)
>  
>  #define GPMI_IS_MX6(x)		(GPMI_IS_MX6Q(x) || GPMI_IS_MX6SX(x))
> +#define GPMI_IS_MX7(x)		(GPMI_IS_MX7D(x))
>  #endif
> -- 
> 1.9.1
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ