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

[Jagan Teki]

Hi Tom/Simon/Febio,

On 17 November 2015 at 02:37, Simon Glass <sjg at chromium.org> wrote:
> 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.

Idea of making lock ops to common for both dm and non-dm(w/o adding
them to dm_spi_ops) is that all generic spi_flash ops are going to use
mtd ops similar to nand and Linux spi-nor. I did that mtd conversion
in this [1] series and going forward spi_flash{} doesn't have generic
ops all are inherited from mtd_info this is my initial plan for moving
to spi-nor core addition.

[1] http://u-boot.10912.n7.nabble.com/PATCH-v6-00-23-sf-MTD-support-td233769.html

thanks!
-- 
Jagan | openedev.