Hi Pratyush, Thank you for the feedback. I will address this in v5.
On 1/30/2021 3:49 AM, Pratyush Yadav wrote: > Hi, > > On 28/01/21 01:36PM, tkuw584...@gmail.com wrote: >> From: Takahiro Kuwano <takahiro.kuw...@infineon.com> >> >> For dual/quad die package devices from Spansion/Cypress, the device's >> status needs to be checked by reading status registers in all dies, by >> using Read Any Register command. To support this, a Flash specific hook >> that can overwrite the legacy status check is needed. >> >> The spansion_sr_ready() reads status register 1 by Read Any Register >> commnad. This function is called from Flash specific hook with die address >> and dummy cycles. >> >> Signed-off-by: Takahiro Kuwano <takahiro.kuw...@infineon.com> >> --- >> drivers/mtd/spi/spi-nor-core.c | 32 ++++++++++++++++++++++++++++++++ >> include/linux/mtd/spi-nor.h | 4 +++- >> 2 files changed, 35 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/mtd/spi/spi-nor-core.c b/drivers/mtd/spi/spi-nor-core.c >> index 624e730524..1c0ba5abf9 100644 >> --- a/drivers/mtd/spi/spi-nor-core.c >> +++ b/drivers/mtd/spi/spi-nor-core.c >> @@ -522,6 +522,35 @@ static int set_4byte(struct spi_nor *nor, const struct >> flash_info *info, >> } >> } >> >> +#ifdef CONFIG_SPI_FLASH_SPANSION >> +/* >> + * Read status register 1 by using Read Any Register command to support >> multi >> + * die package parts. >> + */ >> +static int spansion_sr_ready(struct spi_nor *nor, u32 addr_base, u8 dummy) >> +{ >> + u32 reg_addr = addr_base + SPINOR_REG_ADDR_STR1V; >> + u8 sr; >> + int ret; >> + >> + ret = spansion_read_any_reg(nor, reg_addr, dummy, &sr); >> + if (ret < 0) >> + return ret; >> + >> + if (sr & (SR_E_ERR | SR_P_ERR)) { >> + if (sr & SR_E_ERR) >> + dev_dbg(nor->dev, "Erase Error occurred\n"); >> + else >> + dev_dbg(nor->dev, "Programming Error occurred\n"); >> + >> + nor->write_reg(nor, SPINOR_OP_CLSR, NULL, 0); >> + return -EIO; >> + } >> + >> + return !(sr & SR_WIP); >> +} >> +#endif >> + > > Do not add this function in this patch. Just add the hook here. Add it > in the patch that actually uses it. > >> static int spi_nor_sr_ready(struct spi_nor *nor) >> { >> int sr = read_sr(nor); >> @@ -570,6 +599,9 @@ static int spi_nor_ready(struct spi_nor *nor) >> { >> int sr, fsr; >> >> + if (nor->ready) >> + return nor->ready(nor); >> + > > Move the code below in a separate function and call it something like > spi_nor_default_ready(). Then call that function from here. > >> sr = spi_nor_sr_ready(nor); >> if (sr < 0) >> return sr; >> diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h >> index e31073eb24..25234177de 100644 >> --- a/include/linux/mtd/spi-nor.h >> +++ b/include/linux/mtd/spi-nor.h >> @@ -434,8 +434,9 @@ struct flash_info; >> * @flash_lock: [FLASH-SPECIFIC] lock a region of the SPI NOR >> * @flash_unlock: [FLASH-SPECIFIC] unlock a region of the SPI NOR >> * @flash_is_locked: [FLASH-SPECIFIC] check if a region of the SPI >> NOR is >> - * @quad_enable: [FLASH-SPECIFIC] enables SPI NOR quad mode >> * completely locked >> + * @quad_enable: [FLASH-SPECIFIC] enables SPI NOR quad mode >> + * @ready: [FLASH-SPECIFIC] check if the flash is ready >> * @priv: the private data >> */ >> struct spi_nor { >> @@ -481,6 +482,7 @@ struct spi_nor { >> int (*flash_unlock)(struct spi_nor *nor, loff_t ofs, uint64_t len); >> int (*flash_is_locked)(struct spi_nor *nor, loff_t ofs, uint64_t len); >> int (*quad_enable)(struct spi_nor *nor); >> + int (*ready)(struct spi_nor *nor); >> >> void *priv; >> /* Compatibility for spi_flash, remove once sf layer is merged with mtd */ >> -- >> 2.25.1 >> > Best Regards, Takahiro