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@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
> .
>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
