[PATCH v5 09/10] mtd: spi-nor-core: Add fixups for Cypress s25hl-t/s25hs-t

[Pratyush Yadav]

On 08/03/21 05:47PM, Takahiro Kuwano wrote:
> On 2/24/2021 9:40 PM, Pratyush Yadav wrote:
> > On 19/02/21 10:56AM, tkuw584924 at gmail.com wrote:
> >> From: Takahiro Kuwano <Takahiro.Kuwano at infineon.com>
> >>
> >> This patch adds Flash specific fixups and hooks for Cypress
> >> S25HL-T/S25HS-T family, based on the features introduced in [0][1][2].
> > 
> > Instead of linking the patches like this, it would be a better idea to 
> > simply include them in your series. This will make your series 
> > independent from mine and will make it easier for the maintainer to 
> > apply it.
> > 
> Noted with thanks.
> 
> >>
> >> The nor->ready() and spansion_sr_ready() introduced in #5 and #6 in this
> >> series are used for multi-die package parts.
> > 
> > Nitpick: Once this patch makes it into the Git history there is no patch 
> > #5 or #6. Probably a better idea to just say "introduced earlier" or 
> > something similar. 
> > 
> OK.
> 
> >>
> >> The nor->quad_enable() sets the volatile QE bit on each die.
> >>
> >> The mtd._erase() is hooked if the device is single-die package and not
> >> configured to uniform sectors, assuming it is in factory default
> >> configuration that has 32 x 4KB sectors overlaid on bottom address.
> >> Other configurations, top and split, are not supported at this point.
> >> Will submit additional patches to support it as needed.
> > 
> > Ah, this patch does check for uniform sector map. Nice. I assume you 
> > have tested with both configurations.
> > 
> Yes. I tested both.
> 
> >>
> >> The post_bfpt/sfdp() fixes the params wrongly advertised in SFDP.
> >>
> >> [0] https://patchwork.ozlabs.org/project/uboot/patch/20200904153500.3569-7-p.yadav@ti.com/
> >> [1] https://patchwork.ozlabs.org/project/uboot/patch/20200904153500.3569-8-p.yadav@ti.com/
> >> [2] https://patchwork.ozlabs.org/project/uboot/patch/20200904153500.3569-9-p.yadav@ti.com/
> >>
> >> Signed-off-by: Takahiro Kuwano <Takahiro.Kuwano at infineon.com>
> >> ---
> >> Changes in v5:
> >>   - Add s25hx_t_erase_non_uniform()
> >>   - Change mtd.writesize and mtd.flags in s25hx_t_setup()
> >>   - Fix page size and erase size issues in s25hx_t_post_bfpt_fixup()
> >>
> >>  drivers/mtd/spi/spi-nor-core.c | 155 +++++++++++++++++++++++++++++++++
> >>  include/linux/mtd/spi-nor.h    |   3 +
> >>  2 files changed, 158 insertions(+)
> >>
> >> diff --git a/drivers/mtd/spi/spi-nor-core.c b/drivers/mtd/spi/spi-nor-core.c
> >> index 8d63681cb3..315e26ab27 100644
> >> --- a/drivers/mtd/spi/spi-nor-core.c
> >> +++ b/drivers/mtd/spi/spi-nor-core.c
[...]
> >> +static int s25hx_t_post_bfpt_fixup(struct spi_nor *nor,
> >> +				   const struct sfdp_parameter_header *header,
> >> +				   const struct sfdp_bfpt *bfpt,
> >> +				   struct spi_nor_flash_parameter *params)
> >> +{
> >> +	int ret;
> >> +	u32 addr;
> >> +	u8 cfr3v;
> >> +
> >> +	/* erase size in case it is set to 4K from BFPT */
> >> +	nor->erase_opcode = SPINOR_OP_SE_4B;
> >> +	nor->mtd.erasesize = nor->info->sector_size;
> >> +
> >> +	/* Enter 4-byte addressing mode for Read Any Register */
> >> +	ret = set_4byte(nor, nor->info, 1);
> >> +	if (ret)
> >> +		return ret;
> > 
> > You enter 4byte addressing but don't exit it before returning. Is this 
> > intentional?
> > 
> Yes. The nor->addr_width must be 4 for read/write/erase to work correctly.
> I think device's addressing mode should be in sync with nor->addr_width.

Right. But the comment above implies that 4-byte mode is only enabled 
for the "Read Any Register" command. Either remove the comment 
completely or make it clear that you are changing to 4-byte for all 
further operations, not just the Read Any Register command.

> 
> >> +	nor->addr_width = 4;
> >> +
> >> +	/*
> >> +	 * The page_size is set to 512B from BFPT, but it actually depends on
> >> +	 * the configuration register. Look up the CFR3V and determine the
> >> +	 * page_size. For multi-die package parts, use 512B only when the all
> >> +	 * dies are configured to 512B buffer.
> >> +	 */
> >> +	for (addr = 0; addr < params->size; addr += SZ_128M) {
> >> +		ret = spansion_read_any_reg(nor, addr + SPINOR_REG_ADDR_CFR3V,
> >> +					    0, &cfr3v);
> >> +		if (ret)
> >> +			return ret;
> >> +
> >> +		if (!(cfr3v & CFR3V_PGMBUF)) {
> >> +			params->page_size = 256;
> >> +			return 0;
> >> +		}
> >> +	}
> >> +	params->page_size = 512;
> > 
> > Ok.
> > 
> >> +
> >> +	return 0;
> >> +}
> >> +

-- 
Regards,
Pratyush Yadav
Texas Instruments Inc.