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

Message ID 1483693931-22249-1-git-send-email-linshunquan1@hisilicon.com
State New
Delegated to: Cyrille Pitchen
Headers show

Commit Message

linshunquan 00354166 Jan. 6, 2017, 9:12 a.m.
(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@hisilicon.com>
---
 drivers/mtd/spi-nor/hisi-sfc.c | 30 ++++++++++++++++++++++++++++++
 1 file changed, 30 insertions(+)

Comments

Cyrille Pitchen Jan. 6, 2017, 1:44 p.m. | #1
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@hisilicon.com>
> ---
>  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
linshunquan 00354166 Jan. 7, 2017, 7:33 a.m. | #2
Hi Cyrille,
 Thanks for your relay, I will update this patch soon.

On 2017/1/6 21:44, Cyrille Pitchen wrote:
> 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@hisilicon.com>
>> ---
>>  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
> .
>

Patch

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);
+}
+
 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);
+		writel(reg, host->regbase + FMC_CFG);
+	}
+
+	/* 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: