[U-Boot] [PATCH v7 21/21] sf: Add SPI NOR protection mechanism

[Simon Glass]

Hi Tom,

On 15 November 2015 at 18:58, Tom Rini <trini at konsulko.com> wrote:
> On Sun, Nov 15, 2015 at 06:34:51PM -0700, Simon Glass wrote:
>> Hi,
>>
>> On 13 November 2015 at 03:41, Bin Meng <bmeng.cn at gmail.com> wrote:
>> > Hi,
>> >
>> > On Wed, Nov 11, 2015 at 10:04 PM, Fabio Estevam <festevam at gmail.com> wrote:
>> >> On Wed, Nov 11, 2015 at 12:56 AM, Simon Glass <sjg at chromium.org> wrote:
>> >>> Hi Fabio,
>> >>>
>> >>> On 10 November 2015 at 16:51, Fabio Estevam <festevam at gmail.com> wrote:
>> >>>>
>> >>>> Hi Simon,
>> >>>>
>> >>>> On Tue, Nov 10, 2015 at 10:09 PM, Simon Glass <sjg at chromium.org> wrote:
>> >>>>
>> >>>> > This patch breaks chromebook_link - I think because it adds a new
>> >>>> > operation which is not supported by all flash chips. Those that are
>> >>>> > not supported (i.e that don't have the 'flash_is_locked' method)
>> >>>> > should still work.
>> >>>>
>> >>>> What is the symptom you are seeing? Which SPI NOR flash does this board have?
>> >>>
>> >>> It crashes reading the environment:
>> >>>
>> >>> U-Boot 2015.10-00544-gcad0499 (Nov 10 2015 - 17:06:00 -0700)
>> >>>
>> >>> CPU:   Intel(R) Core(TM) i5-3427U CPU @ 1.80GHz
>> >>> DRAM:  2.7 GiB
>> >>> SF: Detected W25Q64CV with page size 256 Bytes, erase size 4 KiB, total 8 MiB
>> >>> *** Warning - bad CRC, using default environment
>> >>>
>> >>> Video: 1280x1024x16
>> >>> Model: Google Link
>> >>> SF: Detected W25Q64CV with page size 256 Bytes, erase size 4 KiB, total 8 MiB
>> >>> Invalid Opcode (Undefined Opcode)
>> >>
>> >> I am wondering if this invalid opcode is caused by 6c2f758cee266f7648.
>> >>
>> >> Could you please try this?
>> >>
>> >
>> > No, this does not resolve this issue.
>> >
>> >> --- a/arch/x86/include/asm/bitops.h
>> >> +++ b/arch/x86/include/asm/bitops.h
>> >> @@ -364,7 +364,7 @@ static __inline__ int ffs(int x)
>> >>         __asm__("bsfl %1,%0\n\t"
>> >>                 "jnz 1f\n\t"
>> >>                 "movl $-1,%0\n"
>> >> -               "1:" : "=r" (r) : "rm" (x));
>> >> +               "1:" : "=r" (r) : "g" (x));
>> >>
>> >>         return r+1;
>> >>  }
>> >
>> > It turns out it is a NULL pointer exception! Fixing this NULL pointer
>> > makes the crash disappear, but 'saveenv' does not actually work on the
>> > SST flash. Something is broken again, gosh!
>>
>> Bin thank you for fixing this.
>>
>> We still have a big problem with this patch though - it adds features
>> to the pre-driver-model SPI flash implementation and not the driver
>> model implementation! If I somehow have the wrong end of the stick
>> please let me know.
>>
>> If we accept this sort of patch we will never be done with driver
>> model conversions, as we make it impossible for boards to move
>> forward.
>>
>> Fabio can you please rework this to remove the pre-driver-model
>> support, and add your new functions to struct dm_spi_flash_ops
>> instead, then convert the affected boards to driver model?
>
> Hang on, didn't we have this discussion on one of the earlier threads
> about this?  Part of the problem here is that everything else required
> to do this on DM isn't quite there and Fabio has agreed to (under pain
> of feature removal later) do the conversion when possible but to not
> otherwise block getting this feature (and thus some platform(s))
> integrated already.

OK that sounds good, thanks.

Regards,
Simon