Re: [PATCH v1] mtd: spi nor: modify the boot and flash type of FMC

[Cyrille Pitchen]

Hi,

Le 06/01/2017 à 10:12, linshunquan 00354166 a écrit :
> (1) The HiSilicon Flash Memory Controller(FMC) is a multi-functions
>  device which supports SPI Nor flash controller, SPI nand Flash
>  controller and parallel nand flash controller. So when we are prepare
>  to operation SPI Nor, we should make sure the flash type is SPI Nor.
> 
> (2) Make sure the boot type is Normal Type before initialize the SPI
>     Nor controller.
> 
> Signed-off-by: linshunquan 00354166 <linshunquan1@xxxxxxxxxxxxx>
> ---
>  drivers/mtd/spi-nor/hisi-sfc.c | 30 ++++++++++++++++++++++++++++++
>  1 file changed, 30 insertions(+)
> 
> diff --git a/drivers/mtd/spi-nor/hisi-sfc.c b/drivers/mtd/spi-nor/hisi-sfc.c
> index 20378b0..7855024 100644
> --- a/drivers/mtd/spi-nor/hisi-sfc.c
> +++ b/drivers/mtd/spi-nor/hisi-sfc.c
> @@ -32,6 +32,8 @@
>  #define FMC_CFG_OP_MODE_MASK		BIT_MASK(0)
>  #define FMC_CFG_OP_MODE_BOOT		0
>  #define FMC_CFG_OP_MODE_NORMAL		1
> +#define FMC_CFG_OP_MODE_SEL(mode)      ((mode) & 0x1)
> +#define FMC_CFG_FLASH_SEL_SPI_NOR	(0x0 << 1)
>  #define FMC_CFG_FLASH_SEL(type)		(((type) & 0x3) << 1)
>  #define FMC_CFG_FLASH_SEL_MASK		0x6
>  #define FMC_ECC_TYPE(type)		(((type) & 0x7) << 5)
> @@ -141,10 +143,36 @@ static int get_if_type(enum read_mode flash_read)
>  	return if_type;
>  }
>  
> +static void spi_nor_switch_spi_type(struct hifmc_host *host)
> +{
> +	unsigned int reg;
> +
> +	reg = readl(host->regbase + FMC_CFG);
> +	if ((reg & FMC_CFG_FLASH_SEL_MASK)
> +		   	== FMC_CFG_FLASH_SEL_SPI_NOR)
> +		return;
> +
> +	/* if the flash type isn't spi nor, change it */
> +	reg &= ~FMC_CFG_FLASH_SEL_MASK;
> +	reg |= FMC_CFG_FLASH_SEL(0);
> +	writel(reg, host->regbase + FMC_CFG);
> +}
> +

This is not consistent: we have to check the macro definitions to
understand that FMC_CFG_FLASH_SPI_NOR == FMC_CFG_FLASH_SEL(0)

In such a function, you should use the very same macro for both the test
and the update of reg; either FMC_CFG_FLASH_SEL_SPI_NOR or
FMC_CFG_FLASH_SEL(0). Please don't mix the use of those macros.

>  static void hisi_spi_nor_init(struct hifmc_host *host)
>  {
>  	u32 reg;
>  
> +	/* switch the flash type to spi nor */
> +	spi_nor_switch_spi_type(host);
> +
> +	/* set the boot mode to normal */
> +	reg = readl(host->regbase + FMC_CFG);
> +	if ((reg & FMC_CFG_OP_MODE_MASK) == FMC_CFG_OP_MODE_BOOT) {
> +		reg |= FMC_CFG_OP_MODE_SEL(FMC_CFG_OP_MODE_NORMAL);

This is not consistent: you test FMC_CFG_OP_MODE_BOOT, hence without
FMC_CFG_OP_MODE_SEL() but you set
FMC_CFG_OP_MODE_SEL(FMC_CFG_OP_MODE_NORMAL), with FMC_CFG_OP_MODE_SEL().

Of course, looking at the macro definitions, it works as is but once again
we have to check the macro definitions to understand why sometime you use
FMC_CFG_OP_MODE_SEL() whereas other times you don't.

> +		writel(reg, host->regbase + FMC_CFG);
> +	}

spi_nor_switch_spi_type() already updates the FMC_CFG register in the very
same manner: read, test, modify, write. Hence you should write a more
generic function and update both bitfields at once.

static void hisi_spi_nor_update_reg(struct hifmc_host *host,
				    unsigned int reg_offset,
				    unsigned int value,
				    unsigned int mask)
{
	unsigned int reg;

	reg = readl(host->regbase + reg_offset);
	if (((reg ^ value) & mask) == 0)
		return;

	reg = (reg & ~mask) | (value & mask);
	writel(reg, host->regbase + reg_offset);
}

...

	unsigned int value, mask;

	/*
	 * switch the flash type to spi nor and set the boot mode to
	 * normal.
	 */
	value = FMC_CFG_OP_MODE_NORMAL | FMC_CFG_FLASH_SEL_SPI_NOR;
	mask = FMC_CFG_OP_MODE_MASK | FMC_CFG_FLASH_SEL_MASK;
	hisi_spi_nor_update_reg(host, FMC_CFG, value, mask);

> +
> +	/* set timming */
>  	reg = TIMING_CFG_TCSH(CS_HOLD_TIME)
>  		| TIMING_CFG_TCSS(CS_SETUP_TIME)
>  		| TIMING_CFG_TSHSL(CS_DESELECT_TIME);
> @@ -167,6 +195,8 @@ static int hisi_spi_nor_prep(struct spi_nor *nor, enum spi_nor_ops ops)
>  	if (ret)
>  		goto out;
>  
> +	spi_nor_switch_spi_type(host);
> +
>  	return 0;
>  
>  out:
> 

Best regards,

Cyrille