Re: [PATCH V2] bcma: add basic NAND flash driver

[Florian Fainelli <florian@openwrt.org>]

Hello Rafal,

Some minor comments below.

On Sunday 05 August 2012 22:03:07 Rafał Miłecki wrote:
> This is basic driver for NAND flash memory that allows reading it's
> content. It was succesfully tested on Netgear WNDR4500 which is BCM4706
> based router.
> 
> This driver has been written using specs at:
> http://bcm-v4.sipsolutions.net/NAND_Flash
> 
> Signed-off-by: Rafał Miłecki <zajec5@gmail.com>
> CC: Larry Finger <Larry.Finger@lwfinger.net>
> CC: Hauke Mehrtens <hauke@hauke-m.de>
> ---
> V2: Split bcma_nflash_read into three functions as suggested by Hauke
>     Use bcma_* for printing
> ---
>  drivers/bcma/Kconfig                        |    4 +-
>  drivers/bcma/bcma_private.h                 |    2 +-
>  drivers/bcma/driver_chipcommon_nflash.c     |  402 
++++++++++++++++++++++++++-
>  drivers/bcma/driver_chipcommon_pmu.c        |    2 +-
>  include/linux/bcma/bcma_driver_chipcommon.h |   88 ++++++
>  5 files changed, 490 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/bcma/Kconfig b/drivers/bcma/Kconfig
> index 8d1f777..a58d7a9 100644
> --- a/drivers/bcma/Kconfig
> +++ b/drivers/bcma/Kconfig
> @@ -53,8 +53,8 @@ config BCMA_SFLASH
>  	default y
>  
>  config BCMA_NFLASH
> -	bool
> -	depends on BCMA_DRIVER_MIPS && BROKEN
> +	bool "Support for NAND flash"
> +	depends on BCMA_DRIVER_MIPS
>  	default y
>  
>  config BCMA_DRIVER_GMAC_CMN
> diff --git a/drivers/bcma/bcma_private.h b/drivers/bcma/bcma_private.h
> index 3cf9cc9..9895c7e 100644
> --- a/drivers/bcma/bcma_private.h
> +++ b/drivers/bcma/bcma_private.h
> @@ -68,7 +68,7 @@ int bcma_nflash_init(struct bcma_drv_cc *cc);
>  #else
>  static inline int bcma_nflash_init(struct bcma_drv_cc *cc)
>  {
> -	bcma_err(cc->core->bus, "NAND flash not supported\n");
> +	bcma_err(cc->core->bus, "NAND flash support not enabled\n");
>  	return 0;
>  }
>  #endif /* CONFIG_BCMA_NFLASH */
> diff --git a/drivers/bcma/driver_chipcommon_nflash.c 
b/drivers/bcma/driver_chipcommon_nflash.c
> index 574d624..0d11e6f 100644
> --- a/drivers/bcma/driver_chipcommon_nflash.c
> +++ b/drivers/bcma/driver_chipcommon_nflash.c
> @@ -2,18 +2,412 @@
>   * Broadcom specific AMBA
>   * ChipCommon NAND flash interface
>   *
> + * Copyright 2012, Rafał Miłecki <zajec5@gmail.com>
> + *
>   * Licensed under the GNU/GPL. See COPYING for details.
>   */
>  
> +#include "bcma_private.h"
>  #include <linux/bcma/bcma.h>
>  #include <linux/bcma/bcma_driver_chipcommon.h>
>  #include <linux/delay.h>
>  
> -#include "bcma_private.h"
> +/* Broadcom uses 1'000'000 but it seems to be too many. Tests on WNDR4500 
has
> + * shown 6 retries were enough. */
> +#define NFLASH_READY_RETRIES		100
>  
> -/* Initialize NAND flash access */
> -int bcma_nflash_init(struct bcma_drv_cc *cc)
> +/**************************************************
> + * Various helpers
> + **************************************************/
> +
> +static inline u8 bcma_nflash_ns_to_cycle(u16 ns, u16 clock)
> +{
> +	return ((ns * 1000 * clock) / 1000000) + 1;
> +}
> +
> +static void bcma_nflash_enable(struct bcma_drv_cc *cc, bool enable)
> +{
> +	if (cc->core->id.rev == 38) {
> +		if (cc->status & BCMA_CC_CHIPST_REV38_NAND_BOOT)
> +			return;
> +		if (enable)
> +			bcma_chipco_chipctl_maskset(cc, 1, ~0, 0x10000);
> +		else
> +			bcma_chipco_chipctl_maskset(cc, 1, ~0x10000, 0);
> +	}
> +}

Can you define a constant for this bit?

> +
> +static int bcma_nflash_ctl_cmd(struct bcma_drv_cc *cc, u32 code)
> +{
> +	int i = 0;
> +
> +	bcma_cc_write32(cc, BCMA_CC_NFLASH_CTL, 0x80000000 | code);
> +	for (i = 0; i < NFLASH_READY_RETRIES; i++) {
> +		if (!(bcma_cc_read32(cc, BCMA_CC_NFLASH_CTL) & 0x80000000)) {
> +			i = 0;
> +			break;
> +		}
> +	}
> +	if (i) {
> +		bcma_err(cc->core->bus, "NFLASH control command not ready!\n");
> +		return -EBUSY;
> +	}
> +	return 0;

Would not it be simple to check for i == NFLASH_READY_RETRIES instead of 
setting the value of i in the for loop? That sounds more natural. You might 
also want to add a cpu_relax() right after reading the NFLASH_CTL register.

> +}
> +
> +int bcma_nflash_poll(struct bcma_drv_cc *cc)
> +{
> +	int i;
> +	u32 tmp;
> +
> +	if (cc->core->bus->chipinfo.id == BCMA_CHIP_ID_BCM4706) {
> +		for (i = 0; i < NFLASH_READY_RETRIES; i++) {
> +			if (bcma_cc_read32(cc, BCMA_CC_NFLASH_CTL) &
> +			    0x04000000) {
> +				if (bcma_cc_read32(cc, BCMA_CC_NFLASH_CTL) &
> +				    BCMA_CC_NFLASH_CTL_ERR) {
> +					bcma_err(cc->core->bus, "Error on polling\n");
> +					return -EBUSY;
> +				} else {
> +					return 0;
> +				}
> +			}
> +		}
> +	} else {
> +		for (i = 0; i < NFLASH_READY_RETRIES; i++) {
> +			tmp = bcma_cc_read32(cc, BCMA_CC_NAND_INTFC_STATUS);
> +			if ((tmp & 0xC0000000) == 0xC0000000)
> +				return 0;
> +		}
> +	}

Use constants here too.