RE: [PATCH v1 2/2] spi: nor: update page program settings for is25wp256 using post bfpt fixup

[Sagar Kadam <sagar.kadam@sifive.com>]

Hi Pratyush,

> -----Original Message-----
> From: Pratyush Yadav <me@yadavpratyush.com>
> Sent: Friday, May 15, 2020 12:35 PM
> To: Sagar Kadam <sagar.kadam@sifive.com>
> Cc: linux-riscv@lists.infradead.org; linux-kernel@vger.kernel.org; linux-
> mtd@lists.infradead.org; tudor.ambarus@microchip.com;
> miquel.raynal@bootlin.com; richard@nod.at; vigneshr@ti.com;
> anup.patel@wdc.com; aou@eecs.berkeley.edu; palmer@dabbelt.com; Paul
> Walmsley <paul.walmsley@sifive.com>
> Subject: Re: [PATCH v1 2/2] spi: nor: update page program settings for
> is25wp256 using post bfpt fixup
> 
> [External Email] Do not click links or attachments unless you recognize the
> sender and know the content is safe
> 
> Hi Sagar,
> 
> On 14/05/20 04:50AM, Sagar Shrikant Kadam wrote:
> > During SFDP parsing it is seen that the IS25WP256d device is missing 4BAIT
> > (4-Byte address instruction table), due to which it's page program
> > capacity doesn't get correctly populated and the device gets configured
> > with 4-byte Address Serial Input Page Program i.e. SNOR_PROTO_1_1_1
> > even though it can work with SNOR_PROTO_1_1_4.
> >
> > Here using the post bfpt fixup hooks we update the page program
> > settings to 4-byte QUAD Input Page program operations.
> >
> > The patch is tested on HiFive Unleashed A00 board and it benefits
> > few seconds of average write time for entire flash write.
> >
> > QUAD Input Page Program operations:
> > > time mtd_debug write /dev/mtd0 0 33554432 rd32M
> > Copied 33554432 bytes from rd32M to address 0x00000000 in flash
> > real    0m 32.85s
> > user    0m 0.00s
> > sys     0m 31.79s
> >
> > Serial Input Page Program operations:
> > > time mtd_debug write /dev/mtd0 0 33554432 rd32M
> > Copied 33554432 bytes from rd32M to address 0x00000000 in flash
> > real    0m 35.87s
> > user    0m 0.00s
> > sys     0m 35.42s
> >
> > Signed-off-by: Sagar Shrikant Kadam <sagar.kadam@sifive.com>
> > ---
> >  drivers/mtd/spi-nor/issi.c | 16 ++++++++++++++++
> >  1 file changed, 16 insertions(+)
> >
> > diff --git a/drivers/mtd/spi-nor/issi.c b/drivers/mtd/spi-nor/issi.c
> > index ffcb60e..9eb6e82 100644
> > --- a/drivers/mtd/spi-nor/issi.c
> > +++ b/drivers/mtd/spi-nor/issi.c
> > @@ -23,6 +23,22 @@ is25lp256_post_bfpt_fixups(struct spi_nor *nor,
> >               BFPT_DWORD1_ADDRESS_BYTES_3_ONLY)
> >               nor->addr_width = 4;
> >
> > +     /*
> > +      * On IS25WP256d device 4-Byte address instruction table doesn't
> > +      * get populated and so the device get's configured with 4-byte
> > +      * Address Serial Input Page Program i.e. SNOR_PROTO_1_1_1 even
> > +      * though it supports SNOR_PROTO_1_1_4, so priorotize QUAD write
> > +      * over SINGLE write if device id table holds SPI_NOR_QUAD_READ.
> > +      */
> > +     if (strcmp(nor->info->name, "is25wp256") == 0) {
> 
> Instead of doing this, wouldn't it make more sense to have a separate
> fixup hook for is25wp256? Does this device also need the above address
> width fixup? If it does, maybe that can be split into a separate
> function, and used by both the fixups?
> 
Thanks for suggestion. Yes this device requires the above address width fixup.
I suspect that this QUAD mode fix might also be required for "is25lp256" device.
But since I don't have it on my board, I couldn't validate it. If someone could give it a try
on "is25lp256" device and confirm this, then I guess we can remove this check from 
here "if (strcmp(nor->info->name, "is25wp256") == 0)" and rename the is25lp256_post_bfpt_fixups
to is25lpwp256_post_bfpt_fixups to use command fixup for both flash devices,
else I am also ok to split it into separate function as suggested that can be used by both fixup's

Thanks & BR,
Sagar Kadam

> > +             if (nor->info->flags & SPI_NOR_QUAD_READ) {
> > +                     params->hwcaps.mask |= SNOR_HWCAPS_PP_1_1_4;
> > +                     spi_nor_set_pp_settings
> > +                             (&params->page_programs[SNOR_CMD_PP_1_1_4],
> > +                              SPINOR_OP_PP_1_1_4,
> > +                              SNOR_PROTO_1_1_4);
> > +             }
> > +     }
> >       return 0;
> >  }
> 
> --
> Regards,
> Pratyush Yadav