mtd:nor: unlock enhancement for cmdset0002

mtd:nor: unlock enhancement for cmdset0002

[Honza Petrouš]

Hi,

I'm not so much experienced in MTD area, so please correct me, if I'm wrong.

NOR flashes supported by cmdset0002 (AMD & Fujitsu Standard Vendor Command Set)
are not able to unlock one particular eraseblock, the unlocking is
done by unlock
the whole chip. Because of that the driver (cfi_cmdset_0002.c) do it
in free steps:
  1) remember all locked eraseblocks
  2) do chip unlock
  3) lock all remaining eraseblocks (minus, of course ones which have to be
enabled by current ioctl call).

Unfortunately, in step 2 (unlocking chip) there is used unlocking over
all eraseblocks
which belong to requested unlock area. It means that chip unlock operation
is doing multiple times, without any reason of doing so. It is enough to do
unlock only on one eraseblock (as it already means full chip unlock). This way
we can save programming time, on bigger parts even more dramatically
(one chip unlock per request vs. [number of eraseblocks] * chip unlock).

So I would like to ask, if my finding is correct.

Thanks
/Honza

BTW, in current project we have one NOR chip, so when I applied
the following patch (I know it has to be generalized, even now I
understand that it can not work correctly in case of multiple NOR chips
or with different eraseblock sizes, ... etc) I lowered unlocking for 20x
on 64MB filesystem.


diff --git a/drivers/mtd/chips/cfi_cmdset_0002.c
b/drivers/mtd/chips/cfi_cmdset_0002.c
index 9dca881..36e45d2 100644
--- a/drivers/mtd/chips/cfi_cmdset_0002.c
+++ b/drivers/mtd/chips/cfi_cmdset_0002.c
@@ -2694,8 +2694,8 @@ static int __maybe_unused cfi_ppb_unlock(struct
mtd_info *mtd, loff_t ofs,
        }

        /* Now unlock the whole chip */
-       ret = cfi_varsize_frob(mtd, do_ppb_xxlock, ofs, len,
-                              DO_XXLOCK_ONEBLOCK_UNLOCK);
+       ret = do_ppb_xxlock(map, &cfi->chips[0], 0, regions[0].erasesize,
+                                DO_XXLOCK_ONEBLOCK_UNLOCK);
        if (ret) {
                kfree(sect);
                return ret;

Re: mtd:nor: unlock enhancement for cmdset0002

[Honza Petrouš]

Sorry for self-answering, but I forgot to add mtd maintainers and
mtd-mailinglist.

/Honza

2017-05-11 12:11 GMT+02:00 Honza Petrouš <jpetrous@gmail.com>:
> Hi,
>
> I'm not so much experienced in MTD area, so please correct me, if I'm wrong.
>
> NOR flashes supported by cmdset0002 (AMD & Fujitsu Standard Vendor Command Set)
> are not able to unlock one particular eraseblock, the unlocking is
> done by unlock
> the whole chip. Because of that the driver (cfi_cmdset_0002.c) do it
> in free steps:
>   1) remember all locked eraseblocks
>   2) do chip unlock
>   3) lock all remaining eraseblocks (minus, of course ones which have to be
> enabled by current ioctl call).
>
> Unfortunately, in step 2 (unlocking chip) there is used unlocking over
> all eraseblocks
> which belong to requested unlock area. It means that chip unlock operation
> is doing multiple times, without any reason of doing so. It is enough to do
> unlock only on one eraseblock (as it already means full chip unlock). This way
> we can save programming time, on bigger parts even more dramatically
> (one chip unlock per request vs. [number of eraseblocks] * chip unlock).
>
> So I would like to ask, if my finding is correct.
>
> Thanks
> /Honza
>
> BTW, in current project we have one NOR chip, so when I applied
> the following patch (I know it has to be generalized, even now I
> understand that it can not work correctly in case of multiple NOR chips
> or with different eraseblock sizes, ... etc) I lowered unlocking for 20x
> on 64MB filesystem.
>
>
> diff --git a/drivers/mtd/chips/cfi_cmdset_0002.c
> b/drivers/mtd/chips/cfi_cmdset_0002.c
> index 9dca881..36e45d2 100644
> --- a/drivers/mtd/chips/cfi_cmdset_0002.c
> +++ b/drivers/mtd/chips/cfi_cmdset_0002.c
> @@ -2694,8 +2694,8 @@ static int __maybe_unused cfi_ppb_unlock(struct
> mtd_info *mtd, loff_t ofs,
>         }
>
>         /* Now unlock the whole chip */
> -       ret = cfi_varsize_frob(mtd, do_ppb_xxlock, ofs, len,
> -                              DO_XXLOCK_ONEBLOCK_UNLOCK);
> +       ret = do_ppb_xxlock(map, &cfi->chips[0], 0, regions[0].erasesize,
> +                                DO_XXLOCK_ONEBLOCK_UNLOCK);
>         if (ret) {
>                 kfree(sect);
>                 return ret;