| Message ID | 1483693931-22249-1-git-send-email-linshunquan1@hisilicon.com |
|---|---|
| State | Not Applicable |
| Delegated to: | Cyrille Pitchen |
| Headers | show |
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
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 > . >
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:
(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(+)