[PATCH] mtd: chips: AMD chip 0x2201 - write words not buffers

[Mauri Sandberg]

Apologies for the etiquette mistakes. It's the first patch I am submitting.
Answers to comments inline.

> ----------------------------------------
> From: Vignesh Raghavendra <vigneshr at ti.com>
> Sent: Mon Mar 08 06:47:57 CET 2021
> To: Mauri Sandberg <sandberg at mailfence.com>, <linux-mtd at lists.infradead.org>
> Subject: Re: [PATCH] mtd: chips: AMD chip 0x2201 - write words not buffers
> 
> 
> Hi,
> 
> On 2/23/21 10:30 PM, Mauri Sandberg wrote:
> > CFI flash memory driver cmdset_0002 uses buffers for write operations.
> > That does not work with AMD chip with id 0x2201 and we must resort to
> > writing word sized chunks only.
> > 
> 
> Which flash is this? Do you have datasheet for the same.
Appears to be a S29GL256N giving hits as Spansion and Cypress Semiconductor being the manufacturer.
Datasheet is available too. Will make a note of that info.

Here's the link: https://www.cypress.com/file/219941/download

> 
> Please add above information to the commit message for future reference.
> 
> > This patch creates fixup mechanism that renders a chip to use word sized
> > write operation and adds that as the last item in a fixup table, shadowing
> > the entry that causes use of buffer writes.
> > 
> > Without the patch kernel logs will be flooded with entries like below:
> > 
> > MTD do_erase_oneblock(): ERASE 0x01fa0000
> > MTD do_write_buffer(): WRITE 0x01fa0000(0x00001985)
> > MTD do_erase_oneblock(): ERASE 0x01f80000
> > MTD do_write_buffer(): WRITE 0x01f80000(0x00001985)
> > MTD do_write_buffer_wait(): software timeout, address:0x01f8000a.
> > jffs2: Write clean marker to block at 0x01a60000 failed: -5
> > MTD do_erase_oneblock(): ERASE 0x01f60000
> > MTD do_write_buffer(): WRITE 0x01f60000(0x00001985)
> > MTD do_write_buffer_wait(): software timeout, address:0x01f6000a.
> > jffs2: Write clean marker to block at 0x01a40000 failed: -5
> > 
> > Signed-off-by: Mauri Sandberg <sandberg at mailfence.com>
> > ---
> 
> 1. Please explicitly CC maintainers for quicker response. Given the
> amount of traffic on Mailing list, it'll take quite a while for me to
> get to a patch that is not address directly to me.
> 
> Run ./scripts/get_maintainer.pl  on the patch to get list of maintainers
> to be CC'd
> 
> 2. $subject should start with appropriate prefix:
> 
> 	"mtd: cfi_cmdset_0002: ...."
> 
> Just run git log --oneline on the file being patched to know the convention.
> 
> If you don't CC the maintainers and use the appropriate prefixes, it
> will be a while until I get to such patches.
> 
> 
> >  drivers/mtd/chips/cfi_cmdset_0002.c | 8 ++++++++
> >  1 file changed, 8 insertions(+)
> > 
> > diff --git a/drivers/mtd/chips/cfi_cmdset_0002.c b/drivers/mtd/chips/cfi_cmdset_0002.c
> > index a1f3e1031c3d..b8b564342062 100644
> > --- a/drivers/mtd/chips/cfi_cmdset_0002.c
> > +++ b/drivers/mtd/chips/cfi_cmdset_0002.c
> > @@ -279,6 +279,12 @@ static void fixup_use_write_buffers(struct mtd_info *mtd)
> >  }
> >  #endif /* !FORCE_WORD_WRITE */
> >  
> > +static void fixup_use_write_words(struct mtd_info *mtd)
> > +{
> > +	pr_debug("Using word write method\n");
> > +	mtd->_write = cfi_amdstd_write_words;
> 
> Default is cfi_amdstd_write_words(). fixup_use_write_buffers() forces
> this to cfi_amdstd_write_buffers() if 	cfi->cfiq->BufWriteTimeoutTyp is
> non zero.
> 
> What does this field read in case of your flash? Should be 0 in case
> buffered write is not supported.
It gives me '6'.  Datasheet does indicate that buffer writes are supported.

> 
> > +}
> > +
> >  /* Atmel chips don't use the same PRI format as AMD chips */
> >  static void fixup_convert_atmel_pri(struct mtd_info *mtd)
> >  {
> > @@ -470,8 +476,10 @@ static struct cfi_fixup cfi_fixup_table[] = {
> >  #if !FORCE_WORD_WRITE
> >  	{ CFI_MFR_ANY, CFI_ID_ANY, fixup_use_write_buffers },
> >  #endif
> > +	{ CFI_MFR_AMD, 0x2201, fixup_use_write_words },
> 
> 
> Should be worked around in fixup_use_write_buffers() instead. No point
> in overriding mtd->_write to cfi_amdstd_write_buffers() and then
> reverting it again.
> 
> >  	{ 0, 0, NULL }
> >  };
> > +
> 
> Please don't do white space fixes in the same patch adding
> code/feature/bug-fix
> 
> >  static struct cfi_fixup jedec_fixup_table[] = {
> >  	{ CFI_MFR_SST, SST49LF004B, fixup_use_fwh_lock },
> >  	{ CFI_MFR_SST, SST49LF040B, fixup_use_fwh_lock },
> > 
> 
> Regards
> Vignesh


-- 
Sent with https://mailfence.com
Secure and private email