Re: [PATCH] spi-nor: add support for is25wp256d

[Palmer Dabbelt]

On Mon, 06 Aug 2018 14:05:11 PDT (-0700), marek.vasut@xxxxxxxxx wrote:
> On 08/06/2018 10:58 PM, Palmer Dabbelt wrote:
> > On Sat, 04 Aug 2018 02:27:54 PDT (-0700), marek.vasut@xxxxxxxxx wrote:
> > > On 08/04/2018 03:49 AM, Palmer Dabbelt wrote:
> > > > From: "Wesley W. Terpstra" <wesley@xxxxxxxxxx>
> > > >
> > > > This is used of the HiFive Unleashed development board.
> > > >
> > > > Signed-off-by: Wesley W. Terpstra <wesley@xxxxxxxxxx>
> > > > Signed-off-by: Palmer Dabbelt <palmer@xxxxxxxxxx>
> > > > ---
> > > > Âdrivers/mtd/spi-nor/spi-nor.c | 47
> > > > ++++++++++++++++++++++++++++++++++++++++++-
> > > > Âinclude/linux/mtd/spi-nor.hÂÂ |Â 2 ++
> > > > Â2 files changed, 48 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/mtd/spi-nor/spi-nor.c
> > > > b/drivers/mtd/spi-nor/spi-nor.c
> > > > index d9c368c44194..e9a3557a3c23 100644
> > > > --- a/drivers/mtd/spi-nor/spi-nor.c
> > > > +++ b/drivers/mtd/spi-nor/spi-nor.c
> > > > @@ -1072,6 +1072,9 @@ static const struct flash_info spi_nor_ids[] = {
> > > > ÂÂÂÂÂÂÂÂÂÂÂÂ SECT_4K | SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ) },
> > > >  { "is25wp128", INFO(0x9d7018, 0, 64 * 1024, 256,
> > > > ÂÂÂÂÂÂÂÂÂÂÂÂ SECT_4K | SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ) },
> > > > +ÂÂÂ { "is25wp256d", INFO(0x9d7019, 0, 32 * 1024, 1024,
> > >
> > > Is there a reason for the trailing 'd' in is25wp256d ? I'd drop it.
> >
> > I'm honestly not sure. There are data sheets for both of them, but I
> > don't see much of a difference
> >
> > ÂÂ http://www.issi.com/WW/pdf/IS25LP(WP)256D.pdf
> > ÂÂ http://www.issi.com/WW/pdf/25LP-WP256.pdf
> >
> > Following the pattern, I'd expect to see
> >
> >  { "is25wp256", INFO(0x9d7019, 0, 64 * 1024, 512,
> > ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ SECT_4K | SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ) },
> >
> > versus
> >
> > ÂÂÂÂÂÂ { "is25wp256d", INFO(0x9d7019, 0, 32 * 1024, 1024,
> > ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ SECT_4K | SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ |
> > SPI_NOR_4B_OPCODES)
> > ÂÂÂÂÂÂ },
>
> They have the same ID ? Do we support the variant without the d already?

Sorry for being a bit vague there.  There is no is25wp256 in the list already, 
but if I follow the pattern from the other similar chips I get a different 
value for is25wp256 than what was proposed in the patch for an is25wp256d.  
> From my understanding the different values are just because we picked a 
different block size, which seems possible because the original version of this 
patch was written before the other is25wp devices were added to the list.

What I'm proposing is adding an is25wp256 with the same block size as the other 
similar entries in the list.  Looking at the data sheets they appear to have 
the same values for the "Read Product Identification" instruction.

The data sheets are shared with the is25lp256, which there is an entry for and 
matches my expected ID and block sizes.

> > So in other words: the d less sections that are larger, and also has the
> > 4B opcodes flag set. From the documentation in looks like the non-d
> > version supports 3 and 4 byte opcodes, so I guess it's just a different
> > physical layout?
> >
> > In the data sheet for both I see
> >
> > ÂÂ "Pages can be erased in groups of 4Kbyte sectors, 32Kbyte blocks,
> > 64Kbyte ÂÂ blocks, and/or the entire chip"
> >
> > which indicates to me that maybe we've just selected the larger section
> > size? If so then I'll change it to the first one in the new patch.
> >
> > > > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ SECT_4K | SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ
> > > > | SPI_NOR_4B_OPCODES)
> > > > +ÂÂÂ },
> > > >
> > > > ÂÂÂÂ /* Macronix */
> > > > ÂÂÂÂ { "mx25l512e",ÂÂ INFO(0xc22010, 0, 64 * 1024,ÂÂ 1, SECT_4K) },
> > > > @@ -1515,6 +1518,45 @@ static int macronix_quad_enable(struct spi_nor
> > > > *nor)
> > > > ÂÂÂÂ return 0;
> > > > Â}
> > > >
> > > > +/**
> > > > + * issi_unlock() - clear BP[0123] write-protection.
> > > > + * @nor:ÂÂÂ pointer to a 'struct spi_nor'
> > > > + *
> > > > + * Bits [2345] of the Status Register are BP[0123].
> > > > + * ISSI chips use a different block protection scheme than other chips.
> > > > + * Just disable the write-protect unilaterally.
> > > > + *
> > > > + * Return: 0 on success, -errno otherwise.
> > > > + */
> > > > +static int issi_unlock(struct spi_nor *nor)
> > > > +{
> > > > +ÂÂÂ int ret, val;
> > > > +ÂÂÂ u8 mask = SR_BP0 | SR_BP1 | SR_BP2 | SR_BP3;
> > > > +
> > > > +ÂÂÂ val = read_sr(nor);
> > > > +ÂÂÂ if (val < 0)
> > > > +ÂÂÂÂÂÂÂ return val;
> > > > +ÂÂÂ if (!(val & mask))
> > > > +ÂÂÂÂÂÂÂ return 0;
> > > > +
> > > > +ÂÂÂ write_enable(nor);
> > > > +
> > > > +ÂÂÂ write_sr(nor, val & ~mask);
> > > > +
> > > > +ÂÂÂ ret = spi_nor_wait_till_ready(nor);
> > > > +ÂÂÂ if (ret)
> > > > +ÂÂÂÂÂÂÂ return ret;
> > > > +
> > > > +ÂÂÂ ret = read_sr(nor);
> > > > +ÂÂÂ if (ret > 0 && !(ret & mask)) {
> > > > +ÂÂÂÂÂÂÂ dev_info(nor->dev, "ISSI Block Protection Bits cleared\n");
> > > > +ÂÂÂÂÂÂÂ return 0;
> > >
> > > Is the dev_info() really needed ?
> >
> > Nope. I'll spin a v2 pending the above discussion.
> >
> > > > +ÂÂÂ } else {
> > > > +ÂÂÂÂÂÂÂ dev_err(nor->dev, "ISSI Block Protection Bits not cleared\n");
> > > > +ÂÂÂÂÂÂÂ return -EINVAL;
> > > > +ÂÂÂ }
> > > > +}
> > >
> > > [...]
> >
> > Thanks!