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

From: Sagar Kadam
Date: Fri May 15 2020 - 04:41:36 EST


Hi Pratyush,

> -----Original Message-----
> From: Pratyush Yadav <me@xxxxxxxxxxxxxxxxx>
> Sent: Friday, May 15, 2020 12:35 PM
> To: Sagar Kadam <sagar.kadam@xxxxxxxxxx>
> Cc: linux-riscv@xxxxxxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; linux-
> mtd@xxxxxxxxxxxxxxxxxxx; tudor.ambarus@xxxxxxxxxxxxx;
> miquel.raynal@xxxxxxxxxxx; richard@xxxxxx; vigneshr@xxxxxx;
> anup.patel@xxxxxxx; aou@xxxxxxxxxxxxxxxxx; palmer@xxxxxxxxxxx; Paul
> Walmsley <paul.walmsley@xxxxxxxxxx>
> 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@xxxxxxxxxx>
> > ---
> > 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