[PATCH v2 0/3] mtd:nor:ppb_unlock fixes

[PATCH v2 0/3] mtd:nor:ppb_unlock fixes

[Honza Petrouš]

Hi Boris,
as you advised I come back with v2 patch, now it is the series
of three fixes I found them during the testing of the original fix.

>From the fixes it is evident that PPB unlocking is not so much
widely used. What is understandable as usually the flashing
is done in bootloader. At least in projects I was involved before.

Anyway, I can say I tested the code only on one-flashchip
configuration, so I'm not 100% sure if all will be ok with multichip
setting. All my old embedded boards have unfortunatelly only
one nor chip.

As I noted already, the original issue was very slow unlocking
on Spansion S29GL01GS, what led in really horrible timing,
sometimes in minutes! With the fix (when only one chip unlock
cmd is used) the operation returns to the "standard" behaviour
whats make the whole system happy again.

BR.
/Honza

Re: [PATCH v2 0/3] mtd:nor:ppb_unlock fixes

[Boris Brezillon]

Hi Honza,

On Tue, 30 May 2017 11:51:07 +0200
Honza Petrouš <jpetrous@gmail.com> wrote:

> Hi Boris,
> as you advised I come back with v2 patch, now it is the series
> of three fixes I found them during the testing of the original fix.

Just a few things before I start reviewing the patches:

1/ commit title prefix should be "mtd: cfi: " not "mtd:nor:"
2/ Don't know how you send your patches, but patches 1 to 3 should be
   "In-Reply-To" your cover letter, which is not the case here.
   Normally, git send-email does that automatically.

> 
> From the fixes it is evident that PPB unlocking is not so much
> widely used. What is understandable as usually the flashing
> is done in bootloader. At least in projects I was involved before.
> 
> Anyway, I can say I tested the code only on one-flashchip
> configuration, so I'm not 100% sure if all will be ok with multichip
> setting. All my old embedded boards have unfortunatelly only
> one nor chip.
> 
> As I noted already, the original issue was very slow unlocking
> on Spansion S29GL01GS, what led in really horrible timing,
> sometimes in minutes! With the fix (when only one chip unlock
> cmd is used) the operation returns to the "standard" behaviour
> whats make the whole system happy again.
> 
> BR.
> /Honza

Re: [PATCH v2 0/3] mtd:nor:ppb_unlock fixes

[Honza Petrouš]

Hi Boris

2017-05-30 11:59 GMT+02:00 Boris Brezillon <boris.brezillon@free-electrons.com>:
> Hi Honza,
>
> On Tue, 30 May 2017 11:51:07 +0200
> Honza Petrouš <jpetrous@gmail.com> wrote:
>
>> Hi Boris,
>> as you advised I come back with v2 patch, now it is the series
>> of three fixes I found them during the testing of the original fix.
>
> Just a few things before I start reviewing the patches:
>
> 1/ commit title prefix should be "mtd: cfi: " not "mtd:nor:"
> 2/ Don't know how you send your patches, but patches 1 to 3 should be
>    "In-Reply-To" your cover letter, which is not the case here.
>    Normally, git send-email does that automatically.
>

OK, let me make v3 to fix that.

/Honza

Re: [PATCH v2 0/3] mtd:nor:ppb_unlock fixes

[Boris Brezillon]

On Tue, 30 May 2017 13:01:48 +0200
Honza Petrouš <jpetrous@gmail.com> wrote:

> Hi Boris
> 
> 2017-05-30 11:59 GMT+02:00 Boris Brezillon <boris.brezillon@free-electrons.com>:
> > Hi Honza,
> >
> > On Tue, 30 May 2017 11:51:07 +0200
> > Honza Petrouš <jpetrous@gmail.com> wrote:
> >  
> >> Hi Boris,
> >> as you advised I come back with v2 patch, now it is the series
> >> of three fixes I found them during the testing of the original fix.  
> >
> > Just a few things before I start reviewing the patches:
> >
> > 1/ commit title prefix should be "mtd: cfi: " not "mtd:nor:"
> > 2/ Don't know how you send your patches, but patches 1 to 3 should be
> >    "In-Reply-To" your cover letter, which is not the case here.
> >    Normally, git send-email does that automatically.
> >  
> 
> OK, let me make v3 to fix that.

Can you wait a bit. I'll try to review your v2 first.

Re: [PATCH v2 0/3] mtd:nor:ppb_unlock fixes

[Boris Brezillon]

On Tue, 30 May 2017 11:51:07 +0200
Honza Petrouš <jpetrous@gmail.com> wrote:

> Hi Boris,
> as you advised I come back with v2 patch, now it is the series
> of three fixes I found them during the testing of the original fix.
> 
> From the fixes it is evident that PPB unlocking is not so much
> widely used. What is understandable as usually the flashing
> is done in bootloader. At least in projects I was involved before.
> 
> Anyway, I can say I tested the code only on one-flashchip
> configuration, so I'm not 100% sure if all will be ok with multichip
> setting. All my old embedded boards have unfortunatelly only
> one nor chip.

I think I found another bug here [1]. The test does not work for
multichip flashes because adr is set back to 0 when you cross a chip
boundary.

If you want my opinion, you'd better re-code the whole logic (you can
probably do better, but here is an example [2]).

[1]http://elixir.free-electrons.com/linux/v4.12-rc3/source/drivers/mtd/chips/cfi_cmdset_0002.c#L2668
[2]http://code.bulix.org/35oaxp-140047

Re: [PATCH v2 0/3] mtd:nor:ppb_unlock fixes

[Honza Petrouš]

Hi Boris.

2017-05-30 15:24 GMT+02:00 Boris Brezillon <boris.brezillon@free-electrons.com>:
> On Tue, 30 May 2017 11:51:07 +0200
> Honza Petrouš <jpetrous@gmail.com> wrote:
>
>> Hi Boris,
>> as you advised I come back with v2 patch, now it is the series
>> of three fixes I found them during the testing of the original fix.
>>
>> From the fixes it is evident that PPB unlocking is not so much
>> widely used. What is understandable as usually the flashing
>> is done in bootloader. At least in projects I was involved before.
>>
>> Anyway, I can say I tested the code only on one-flashchip
>> configuration, so I'm not 100% sure if all will be ok with multichip
>> setting. All my old embedded boards have unfortunatelly only
>> one nor chip.
>
> I think I found another bug here [1]. The test does not work for
> multichip flashes because adr is set back to 0 when you cross a chip
> boundary.
>
> If you want my opinion, you'd better re-code the whole logic (you can
> probably do better, but here is an example [2]).

Yeh, it seems the code has much more issues.

I will try to recode it.

Thanks.

/Honza