| Message ID | CAOMZO5AY-sVgwkAkZ=HLc6sfOQriY-gd0bL_Tge+Fw9H6xi3OA@mail.gmail.com |
|---|---|
| State | RFC |
| Headers | show |
Hi, On Wed, Nov 11, 2015 at 10:04 PM, Fabio Estevam <festevam@gmail.com> wrote: > On Wed, Nov 11, 2015 at 12:56 AM, Simon Glass <sjg@chromium.org> wrote: >> Hi Fabio, >> >> On 10 November 2015 at 16:51, Fabio Estevam <festevam@gmail.com> wrote: >>> >>> Hi Simon, >>> >>> On Tue, Nov 10, 2015 at 10:09 PM, Simon Glass <sjg@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! Regards, Bin
Hi, On 13 November 2015 at 03:41, Bin Meng <bmeng.cn@gmail.com> wrote: > Hi, > > On Wed, Nov 11, 2015 at 10:04 PM, Fabio Estevam <festevam@gmail.com> wrote: >> On Wed, Nov 11, 2015 at 12:56 AM, Simon Glass <sjg@chromium.org> wrote: >>> Hi Fabio, >>> >>> On 10 November 2015 at 16:51, Fabio Estevam <festevam@gmail.com> wrote: >>>> >>>> Hi Simon, >>>> >>>> On Tue, Nov 10, 2015 at 10:09 PM, Simon Glass <sjg@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? Regards, Simon
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@gmail.com> wrote: > > Hi, > > > > On Wed, Nov 11, 2015 at 10:04 PM, Fabio Estevam <festevam@gmail.com> wrote: > >> On Wed, Nov 11, 2015 at 12:56 AM, Simon Glass <sjg@chromium.org> wrote: > >>> Hi Fabio, > >>> > >>> On 10 November 2015 at 16:51, Fabio Estevam <festevam@gmail.com> wrote: > >>>> > >>>> Hi Simon, > >>>> > >>>> On Tue, Nov 10, 2015 at 10:09 PM, Simon Glass <sjg@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.
Hi Simon, On Sun, Nov 15, 2015 at 11:34 PM, Simon Glass <sjg@chromium.org> wrote: > 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? Ok, I will give it a try in converting imx spi driver to DM as a first step, and then moving the lock functions into dm_spi_flash_ops. Actually I should start with converting imx serial driver to DM as we now have a deadline approaching :-)
Hi Tom, On 15 November 2015 at 18:58, Tom Rini <trini@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@gmail.com> wrote: >> > Hi, >> > >> > On Wed, Nov 11, 2015 at 10:04 PM, Fabio Estevam <festevam@gmail.com> wrote: >> >> On Wed, Nov 11, 2015 at 12:56 AM, Simon Glass <sjg@chromium.org> wrote: >> >>> Hi Fabio, >> >>> >> >>> On 10 November 2015 at 16:51, Fabio Estevam <festevam@gmail.com> wrote: >> >>>> >> >>>> Hi Simon, >> >>>> >> >>>> On Tue, Nov 10, 2015 at 10:09 PM, Simon Glass <sjg@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
Hi Tom/Simon/Febio, On 17 November 2015 at 02:37, Simon Glass <sjg@chromium.org> wrote: > Hi Tom, > > On 15 November 2015 at 18:58, Tom Rini <trini@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@gmail.com> wrote: >>> > Hi, >>> > >>> > On Wed, Nov 11, 2015 at 10:04 PM, Fabio Estevam <festevam@gmail.com> wrote: >>> >> On Wed, Nov 11, 2015 at 12:56 AM, Simon Glass <sjg@chromium.org> wrote: >>> >>> Hi Fabio, >>> >>> >>> >>> On 10 November 2015 at 16:51, Fabio Estevam <festevam@gmail.com> wrote: >>> >>>> >>> >>>> Hi Simon, >>> >>>> >>> >>>> On Tue, Nov 10, 2015 at 10:09 PM, Simon Glass <sjg@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!
--- 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; }