| Message ID | 1448714709-30460-1-git-send-email-bmeng.cn@gmail.com |
|---|---|
| State | Deferred |
| Delegated to: | Bin Meng |
| Headers | show |
Hi Bin, On 28 November 2015 at 05:45, Bin Meng <bmeng.cn@gmail.com> wrote: > Every board has one dedicated type of SPI flash, hence it is > unnecessary to include multiple SPI flash drivers. > > For QEMU and coreboot (default build of coreboot is also QEMU), > SPI flash is not supported. Remove those SPI flash drivers. > > Signed-off-by: Bin Meng <bmeng.cn@gmail.com> > --- > > configs/bayleybay_defconfig | 2 -- > configs/chromebook_link_defconfig | 2 -- > configs/chromebox_panther_defconfig | 2 -- > configs/coreboot-x86_defconfig | 4 ---- > configs/crownbay_defconfig | 3 --- > configs/galileo_defconfig | 2 -- > configs/minnowmax_defconfig | 3 --- > configs/qemu-x86_defconfig | 4 ---- > 8 files changed, 22 deletions(-) What is the benefit of this? I see it removes a few lines in a data table. Does it matter? For all of these platforms we can use the dediprog em100 which I typically set to use winbond as the manufacturer, regardless of which chip is actually on the board. For U-Boot on coreboot, why is SPI flash not supported? It certainly works with link. Regards, Simon
Hi Simon, On Wed, Dec 2, 2015 at 12:32 AM, Simon Glass <sjg@chromium.org> wrote: > Hi Bin, > > On 28 November 2015 at 05:45, Bin Meng <bmeng.cn@gmail.com> wrote: >> Every board has one dedicated type of SPI flash, hence it is >> unnecessary to include multiple SPI flash drivers. >> >> For QEMU and coreboot (default build of coreboot is also QEMU), >> SPI flash is not supported. Remove those SPI flash drivers. >> >> Signed-off-by: Bin Meng <bmeng.cn@gmail.com> >> --- >> >> configs/bayleybay_defconfig | 2 -- >> configs/chromebook_link_defconfig | 2 -- >> configs/chromebox_panther_defconfig | 2 -- >> configs/coreboot-x86_defconfig | 4 ---- >> configs/crownbay_defconfig | 3 --- >> configs/galileo_defconfig | 2 -- >> configs/minnowmax_defconfig | 3 --- >> configs/qemu-x86_defconfig | 4 ---- >> 8 files changed, 22 deletions(-) > > What is the benefit of this? I see it removes a few lines in a data > table. Does it matter? Maybe we should ask the other way around, why do we create so many flash driver Kconfig option? I believe the intention was footprint. Besides the footprint issue, having just one flash driver in each board makes it very clear instead of causing confusion. Looks other board defconfig files only select one. > > For all of these platforms we can use the dediprog em100 which I > typically set to use winbond as the manufacturer, regardless of which > chip is actually on the board. > I think that's because emulator can emulate flash from various vendors. > For U-Boot on coreboot, why is SPI flash not supported? It certainly > works with link. Yes, booting from coreboot does support SPI flash. However since we decided to use QEMU as the default build target for coreboot, and QEMU does not support SPI flash yet, these config options are removed. One can certainly adjust these Kconfig options via 'make menuconfig', eg: adding SD/MMC support which is not in coreboot's defconfig either. Regards, Bin
+Jagan Hi Bin, On 1 December 2015 at 18:41, Bin Meng <bmeng.cn@gmail.com> wrote: > > Hi Simon, > > On Wed, Dec 2, 2015 at 12:32 AM, Simon Glass <sjg@chromium.org> wrote: > > Hi Bin, > > > > On 28 November 2015 at 05:45, Bin Meng <bmeng.cn@gmail.com> wrote: > >> Every board has one dedicated type of SPI flash, hence it is > >> unnecessary to include multiple SPI flash drivers. > >> > >> For QEMU and coreboot (default build of coreboot is also QEMU), > >> SPI flash is not supported. Remove those SPI flash drivers. > >> > >> Signed-off-by: Bin Meng <bmeng.cn@gmail.com> > >> --- > >> > >> configs/bayleybay_defconfig | 2 -- > >> configs/chromebook_link_defconfig | 2 -- > >> configs/chromebox_panther_defconfig | 2 -- > >> configs/coreboot-x86_defconfig | 4 ---- > >> configs/crownbay_defconfig | 3 --- > >> configs/galileo_defconfig | 2 -- > >> configs/minnowmax_defconfig | 3 --- > >> configs/qemu-x86_defconfig | 4 ---- > >> 8 files changed, 22 deletions(-) > > > > What is the benefit of this? I see it removes a few lines in a data > > table. Does it matter? > > Maybe we should ask the other way around, why do we create so many > flash driver Kconfig option? I believe the intention was footprint. > Besides the footprint issue, having just one flash driver in each > board makes it very clear instead of causing confusion. Looks other > board defconfig files only select one. They are a hangover from when we had a separate driver for each one. Jagan put a lot of effort into removing all the semi-duplicated code. Maybe we should prune down these options? > > > > > For all of these platforms we can use the dediprog em100 which I > > typically set to use winbond as the manufacturer, regardless of which > > chip is actually on the board. > > > > I think that's because emulator can emulate flash from various vendors. Yes, and also for convenience. > > > For U-Boot on coreboot, why is SPI flash not supported? It certainly > > works with link. > > Yes, booting from coreboot does support SPI flash. However since we > decided to use QEMU as the default build target for coreboot, and QEMU > does not support SPI flash yet, these config options are removed. One > can certainly adjust these Kconfig options via 'make menuconfig', eg: > adding SD/MMC support which is not in coreboot's defconfig either. Well this breaks booting on link, since the SPI flash stops working. I'm really not keen on having to specially select the SPI flash when you want to use link. Regards, Simon
Hi Simon, On Thu, Dec 3, 2015 at 5:05 AM, Simon Glass <sjg@chromium.org> wrote: > +Jagan > > Hi Bin, > > On 1 December 2015 at 18:41, Bin Meng <bmeng.cn@gmail.com> wrote: >> >> Hi Simon, >> >> On Wed, Dec 2, 2015 at 12:32 AM, Simon Glass <sjg@chromium.org> wrote: >> > Hi Bin, >> > >> > On 28 November 2015 at 05:45, Bin Meng <bmeng.cn@gmail.com> wrote: >> >> Every board has one dedicated type of SPI flash, hence it is >> >> unnecessary to include multiple SPI flash drivers. >> >> >> >> For QEMU and coreboot (default build of coreboot is also QEMU), >> >> SPI flash is not supported. Remove those SPI flash drivers. >> >> >> >> Signed-off-by: Bin Meng <bmeng.cn@gmail.com> >> >> --- >> >> >> >> configs/bayleybay_defconfig | 2 -- >> >> configs/chromebook_link_defconfig | 2 -- >> >> configs/chromebox_panther_defconfig | 2 -- >> >> configs/coreboot-x86_defconfig | 4 ---- >> >> configs/crownbay_defconfig | 3 --- >> >> configs/galileo_defconfig | 2 -- >> >> configs/minnowmax_defconfig | 3 --- >> >> configs/qemu-x86_defconfig | 4 ---- >> >> 8 files changed, 22 deletions(-) >> > >> > What is the benefit of this? I see it removes a few lines in a data >> > table. Does it matter? >> >> Maybe we should ask the other way around, why do we create so many >> flash driver Kconfig option? I believe the intention was footprint. >> Besides the footprint issue, having just one flash driver in each >> board makes it very clear instead of causing confusion. Looks other >> board defconfig files only select one. > > They are a hangover from when we had a separate driver for each one. > Jagan put a lot of effort into removing all the semi-duplicated code. > > Maybe we should prune down these options? > But if we already spent a lot of effort into removing all the semi-duplicated code, we should not have converted those flash driver to Kconfig options before. See commit d5af92315bb48740f16bf8817f38e227d3076905 "sf: kconfig: add kconfig options for spi flashes" I suspect we may remove most of these SPI flash macros, but at least SST flash macro should be kept since right now it is mixed in the generic driver with a special byte program and word program which is incompatible with other vendors' flashes. >> >> > >> > For all of these platforms we can use the dediprog em100 which I >> > typically set to use winbond as the manufacturer, regardless of which >> > chip is actually on the board. >> > >> >> I think that's because emulator can emulate flash from various vendors. > > Yes, and also for convenience. > >> >> > For U-Boot on coreboot, why is SPI flash not supported? It certainly >> > works with link. >> >> Yes, booting from coreboot does support SPI flash. However since we >> decided to use QEMU as the default build target for coreboot, and QEMU >> does not support SPI flash yet, these config options are removed. One >> can certainly adjust these Kconfig options via 'make menuconfig', eg: >> adding SD/MMC support which is not in coreboot's defconfig either. > > Well this breaks booting on link, since the SPI flash stops working. > I'm really not keen on having to specially select the SPI flash when > you want to use link. > Do you mean booting U-Boot on link from coreboot? Without SPI flash it should still boot. It looks to me your preference is to include all the available drivers into coreboot's defconfig, yes? If so, we may add some other drivers Kconfig in coreboot-x86_defconfig too, like SD/MMC drivers, all the available ethernet drivers even they only exist on some boards. Regards, Bin
Hi Bin, On 3 December 2015 at 10:14, Bin Meng <bmeng.cn@gmail.com> wrote: > Hi Simon, > > On Thu, Dec 3, 2015 at 5:05 AM, Simon Glass <sjg@chromium.org> wrote: >> +Jagan >> >> Hi Bin, >> >> On 1 December 2015 at 18:41, Bin Meng <bmeng.cn@gmail.com> wrote: >>> >>> Hi Simon, >>> >>> On Wed, Dec 2, 2015 at 12:32 AM, Simon Glass <sjg@chromium.org> wrote: >>> > Hi Bin, >>> > >>> > On 28 November 2015 at 05:45, Bin Meng <bmeng.cn@gmail.com> wrote: >>> >> Every board has one dedicated type of SPI flash, hence it is >>> >> unnecessary to include multiple SPI flash drivers. >>> >> >>> >> For QEMU and coreboot (default build of coreboot is also QEMU), >>> >> SPI flash is not supported. Remove those SPI flash drivers. >>> >> >>> >> Signed-off-by: Bin Meng <bmeng.cn@gmail.com> >>> >> --- >>> >> >>> >> configs/bayleybay_defconfig | 2 -- >>> >> configs/chromebook_link_defconfig | 2 -- >>> >> configs/chromebox_panther_defconfig | 2 -- >>> >> configs/coreboot-x86_defconfig | 4 ---- >>> >> configs/crownbay_defconfig | 3 --- >>> >> configs/galileo_defconfig | 2 -- >>> >> configs/minnowmax_defconfig | 3 --- >>> >> configs/qemu-x86_defconfig | 4 ---- >>> >> 8 files changed, 22 deletions(-) >>> > >>> > What is the benefit of this? I see it removes a few lines in a data >>> > table. Does it matter? >>> >>> Maybe we should ask the other way around, why do we create so many >>> flash driver Kconfig option? I believe the intention was footprint. >>> Besides the footprint issue, having just one flash driver in each >>> board makes it very clear instead of causing confusion. Looks other >>> board defconfig files only select one. Are you talking about flash vendor config or CONFIG_SPI_FLASH? >> >> They are a hangover from when we had a separate driver for each one. >> Jagan put a lot of effort into removing all the semi-duplicated code. >> >> Maybe we should prune down these options? >> > > But if we already spent a lot of effort into removing all the > semi-duplicated code, we should not have converted those flash driver > to Kconfig options before. > > See commit d5af92315bb48740f16bf8817f38e227d3076905 "sf: kconfig: add > kconfig options for spi flashes" > > I suspect we may remove most of these SPI flash macros, but at least > SST flash macro should be kept since right now it is mixed in the > generic driver with a special byte program and word program which is > incompatible with other vendors' flashes. But there is some flash vendor specific code like quad enable bit, locking ops and finally about spi_flash_params table. > >>> >>> > >>> > For all of these platforms we can use the dediprog em100 which I >>> > typically set to use winbond as the manufacturer, regardless of which >>> > chip is actually on the board. >>> > >>> >>> I think that's because emulator can emulate flash from various vendors. >> >> Yes, and also for convenience. >> >>> >>> > For U-Boot on coreboot, why is SPI flash not supported? It certainly >>> > works with link. >>> >>> Yes, booting from coreboot does support SPI flash. However since we >>> decided to use QEMU as the default build target for coreboot, and QEMU >>> does not support SPI flash yet, these config options are removed. One >>> can certainly adjust these Kconfig options via 'make menuconfig', eg: >>> adding SD/MMC support which is not in coreboot's defconfig either. >> >> Well this breaks booting on link, since the SPI flash stops working. >> I'm really not keen on having to specially select the SPI flash when >> you want to use link. >> > > Do you mean booting U-Boot on link from coreboot? Without SPI flash it > should still boot. It looks to me your preference is to include all > the available drivers into coreboot's defconfig, yes? If so, we may > add some other drivers Kconfig in coreboot-x86_defconfig too, like > SD/MMC drivers, all the available ethernet drivers even they only > exist on some boards. thanks!
Hi Jagan, On Thu, Dec 3, 2015 at 6:24 PM, Jagan Teki <jteki@openedev.com> wrote: > Hi Bin, > > On 3 December 2015 at 10:14, Bin Meng <bmeng.cn@gmail.com> wrote: >> Hi Simon, >> >> On Thu, Dec 3, 2015 at 5:05 AM, Simon Glass <sjg@chromium.org> wrote: >>> +Jagan >>> >>> Hi Bin, >>> >>> On 1 December 2015 at 18:41, Bin Meng <bmeng.cn@gmail.com> wrote: >>>> >>>> Hi Simon, >>>> >>>> On Wed, Dec 2, 2015 at 12:32 AM, Simon Glass <sjg@chromium.org> wrote: >>>> > Hi Bin, >>>> > >>>> > On 28 November 2015 at 05:45, Bin Meng <bmeng.cn@gmail.com> wrote: >>>> >> Every board has one dedicated type of SPI flash, hence it is >>>> >> unnecessary to include multiple SPI flash drivers. >>>> >> >>>> >> For QEMU and coreboot (default build of coreboot is also QEMU), >>>> >> SPI flash is not supported. Remove those SPI flash drivers. >>>> >> >>>> >> Signed-off-by: Bin Meng <bmeng.cn@gmail.com> >>>> >> --- >>>> >> >>>> >> configs/bayleybay_defconfig | 2 -- >>>> >> configs/chromebook_link_defconfig | 2 -- >>>> >> configs/chromebox_panther_defconfig | 2 -- >>>> >> configs/coreboot-x86_defconfig | 4 ---- >>>> >> configs/crownbay_defconfig | 3 --- >>>> >> configs/galileo_defconfig | 2 -- >>>> >> configs/minnowmax_defconfig | 3 --- >>>> >> configs/qemu-x86_defconfig | 4 ---- >>>> >> 8 files changed, 22 deletions(-) >>>> > >>>> > What is the benefit of this? I see it removes a few lines in a data >>>> > table. Does it matter? >>>> >>>> Maybe we should ask the other way around, why do we create so many >>>> flash driver Kconfig option? I believe the intention was footprint. >>>> Besides the footprint issue, having just one flash driver in each >>>> board makes it very clear instead of causing confusion. Looks other >>>> board defconfig files only select one. > > Are you talking about flash vendor config or CONFIG_SPI_FLASH? > Flash vendor config, as you see in this patch. >>> >>> They are a hangover from when we had a separate driver for each one. >>> Jagan put a lot of effort into removing all the semi-duplicated code. >>> >>> Maybe we should prune down these options? >>> >> >> But if we already spent a lot of effort into removing all the >> semi-duplicated code, we should not have converted those flash driver >> to Kconfig options before. >> >> See commit d5af92315bb48740f16bf8817f38e227d3076905 "sf: kconfig: add >> kconfig options for spi flashes" >> >> I suspect we may remove most of these SPI flash macros, but at least >> SST flash macro should be kept since right now it is mixed in the >> generic driver with a special byte program and word program which is >> incompatible with other vendors' flashes. > > But there is some flash vendor specific code like quad enable bit, > locking ops and finally about spi_flash_params table. > I know. That's probably why adding all these SPI flash drivers don't help at all because only one code path will take effect. And what I did in this patch is to select one type of flash per board. >> >>>> >>>> > >>>> > For all of these platforms we can use the dediprog em100 which I >>>> > typically set to use winbond as the manufacturer, regardless of which >>>> > chip is actually on the board. >>>> > >>>> >>>> I think that's because emulator can emulate flash from various vendors. >>> >>> Yes, and also for convenience. >>> >>>> >>>> > For U-Boot on coreboot, why is SPI flash not supported? It certainly >>>> > works with link. >>>> >>>> Yes, booting from coreboot does support SPI flash. However since we >>>> decided to use QEMU as the default build target for coreboot, and QEMU >>>> does not support SPI flash yet, these config options are removed. One >>>> can certainly adjust these Kconfig options via 'make menuconfig', eg: >>>> adding SD/MMC support which is not in coreboot's defconfig either. >>> >>> Well this breaks booting on link, since the SPI flash stops working. >>> I'm really not keen on having to specially select the SPI flash when >>> you want to use link. >>> >> >> Do you mean booting U-Boot on link from coreboot? Without SPI flash it >> should still boot. It looks to me your preference is to include all >> the available drivers into coreboot's defconfig, yes? If so, we may >> add some other drivers Kconfig in coreboot-x86_defconfig too, like >> SD/MMC drivers, all the available ethernet drivers even they only >> exist on some boards. > > thanks! > -- Regards, Bin
Hi, On 3 December 2015 at 06:27, Bin Meng <bmeng.cn@gmail.com> wrote: > Hi Jagan, > > On Thu, Dec 3, 2015 at 6:24 PM, Jagan Teki <jteki@openedev.com> wrote: >> Hi Bin, >> >> On 3 December 2015 at 10:14, Bin Meng <bmeng.cn@gmail.com> wrote: >>> Hi Simon, >>> >>> On Thu, Dec 3, 2015 at 5:05 AM, Simon Glass <sjg@chromium.org> wrote: >>>> +Jagan >>>> >>>> Hi Bin, >>>> >>>> On 1 December 2015 at 18:41, Bin Meng <bmeng.cn@gmail.com> wrote: >>>>> >>>>> Hi Simon, >>>>> >>>>> On Wed, Dec 2, 2015 at 12:32 AM, Simon Glass <sjg@chromium.org> wrote: >>>>> > Hi Bin, >>>>> > >>>>> > On 28 November 2015 at 05:45, Bin Meng <bmeng.cn@gmail.com> wrote: >>>>> >> Every board has one dedicated type of SPI flash, hence it is >>>>> >> unnecessary to include multiple SPI flash drivers. >>>>> >> >>>>> >> For QEMU and coreboot (default build of coreboot is also QEMU), >>>>> >> SPI flash is not supported. Remove those SPI flash drivers. >>>>> >> >>>>> >> Signed-off-by: Bin Meng <bmeng.cn@gmail.com> >>>>> >> --- >>>>> >> >>>>> >> configs/bayleybay_defconfig | 2 -- >>>>> >> configs/chromebook_link_defconfig | 2 -- >>>>> >> configs/chromebox_panther_defconfig | 2 -- >>>>> >> configs/coreboot-x86_defconfig | 4 ---- >>>>> >> configs/crownbay_defconfig | 3 --- >>>>> >> configs/galileo_defconfig | 2 -- >>>>> >> configs/minnowmax_defconfig | 3 --- >>>>> >> configs/qemu-x86_defconfig | 4 ---- >>>>> >> 8 files changed, 22 deletions(-) >>>>> > >>>>> > What is the benefit of this? I see it removes a few lines in a data >>>>> > table. Does it matter? >>>>> >>>>> Maybe we should ask the other way around, why do we create so many >>>>> flash driver Kconfig option? I believe the intention was footprint. >>>>> Besides the footprint issue, having just one flash driver in each >>>>> board makes it very clear instead of causing confusion. Looks other >>>>> board defconfig files only select one. >> >> Are you talking about flash vendor config or CONFIG_SPI_FLASH? >> > > Flash vendor config, as you see in this patch. > >>>> >>>> They are a hangover from when we had a separate driver for each one. >>>> Jagan put a lot of effort into removing all the semi-duplicated code. >>>> >>>> Maybe we should prune down these options? >>>> >>> >>> But if we already spent a lot of effort into removing all the >>> semi-duplicated code, we should not have converted those flash driver >>> to Kconfig options before. >>> >>> See commit d5af92315bb48740f16bf8817f38e227d3076905 "sf: kconfig: add >>> kconfig options for spi flashes" >>> >>> I suspect we may remove most of these SPI flash macros, but at least >>> SST flash macro should be kept since right now it is mixed in the >>> generic driver with a special byte program and word program which is >>> incompatible with other vendors' flashes. >> >> But there is some flash vendor specific code like quad enable bit, >> locking ops and finally about spi_flash_params table. >> > > I know. That's probably why adding all these SPI flash drivers don't > help at all because only one code path will take effect. And what I > did in this patch is to select one type of flash per board. So how about we group together 3-4 of the common ones, with no special features, into a 'CONFIG_SPI_FLASH_GENERIC'? > >>> >>>>> >>>>> > >>>>> > For all of these platforms we can use the dediprog em100 which I >>>>> > typically set to use winbond as the manufacturer, regardless of which >>>>> > chip is actually on the board. >>>>> > >>>>> >>>>> I think that's because emulator can emulate flash from various vendors. >>>> >>>> Yes, and also for convenience. >>>> >>>>> >>>>> > For U-Boot on coreboot, why is SPI flash not supported? It certainly >>>>> > works with link. >>>>> >>>>> Yes, booting from coreboot does support SPI flash. However since we >>>>> decided to use QEMU as the default build target for coreboot, and QEMU >>>>> does not support SPI flash yet, these config options are removed. One >>>>> can certainly adjust these Kconfig options via 'make menuconfig', eg: >>>>> adding SD/MMC support which is not in coreboot's defconfig either. >>>> >>>> Well this breaks booting on link, since the SPI flash stops working. >>>> I'm really not keen on having to specially select the SPI flash when >>>> you want to use link. >>>> >>> >>> Do you mean booting U-Boot on link from coreboot? Without SPI flash it >>> should still boot. It looks to me your preference is to include all >>> the available drivers into coreboot's defconfig, yes? If so, we may >>> add some other drivers Kconfig in coreboot-x86_defconfig too, like >>> SD/MMC drivers, all the available ethernet drivers even they only >>> exist on some boards. >> >> thanks! >> -- > > Regards, > Bin Regards, Simon
Hi Jagan, On Fri, Dec 4, 2015 at 2:57 AM, Simon Glass <sjg@chromium.org> wrote: > Hi, > > On 3 December 2015 at 06:27, Bin Meng <bmeng.cn@gmail.com> wrote: >> Hi Jagan, >> >> On Thu, Dec 3, 2015 at 6:24 PM, Jagan Teki <jteki@openedev.com> wrote: >>> Hi Bin, >>> >>> On 3 December 2015 at 10:14, Bin Meng <bmeng.cn@gmail.com> wrote: >>>> Hi Simon, >>>> >>>> On Thu, Dec 3, 2015 at 5:05 AM, Simon Glass <sjg@chromium.org> wrote: >>>>> +Jagan >>>>> >>>>> Hi Bin, >>>>> >>>>> On 1 December 2015 at 18:41, Bin Meng <bmeng.cn@gmail.com> wrote: >>>>>> >>>>>> Hi Simon, >>>>>> >>>>>> On Wed, Dec 2, 2015 at 12:32 AM, Simon Glass <sjg@chromium.org> wrote: >>>>>> > Hi Bin, >>>>>> > >>>>>> > On 28 November 2015 at 05:45, Bin Meng <bmeng.cn@gmail.com> wrote: >>>>>> >> Every board has one dedicated type of SPI flash, hence it is >>>>>> >> unnecessary to include multiple SPI flash drivers. >>>>>> >> >>>>>> >> For QEMU and coreboot (default build of coreboot is also QEMU), >>>>>> >> SPI flash is not supported. Remove those SPI flash drivers. >>>>>> >> >>>>>> >> Signed-off-by: Bin Meng <bmeng.cn@gmail.com> >>>>>> >> --- >>>>>> >> >>>>>> >> configs/bayleybay_defconfig | 2 -- >>>>>> >> configs/chromebook_link_defconfig | 2 -- >>>>>> >> configs/chromebox_panther_defconfig | 2 -- >>>>>> >> configs/coreboot-x86_defconfig | 4 ---- >>>>>> >> configs/crownbay_defconfig | 3 --- >>>>>> >> configs/galileo_defconfig | 2 -- >>>>>> >> configs/minnowmax_defconfig | 3 --- >>>>>> >> configs/qemu-x86_defconfig | 4 ---- >>>>>> >> 8 files changed, 22 deletions(-) >>>>>> > >>>>>> > What is the benefit of this? I see it removes a few lines in a data >>>>>> > table. Does it matter? >>>>>> >>>>>> Maybe we should ask the other way around, why do we create so many >>>>>> flash driver Kconfig option? I believe the intention was footprint. >>>>>> Besides the footprint issue, having just one flash driver in each >>>>>> board makes it very clear instead of causing confusion. Looks other >>>>>> board defconfig files only select one. >>> >>> Are you talking about flash vendor config or CONFIG_SPI_FLASH? >>> >> >> Flash vendor config, as you see in this patch. >> >>>>> >>>>> They are a hangover from when we had a separate driver for each one. >>>>> Jagan put a lot of effort into removing all the semi-duplicated code. >>>>> >>>>> Maybe we should prune down these options? >>>>> >>>> >>>> But if we already spent a lot of effort into removing all the >>>> semi-duplicated code, we should not have converted those flash driver >>>> to Kconfig options before. >>>> >>>> See commit d5af92315bb48740f16bf8817f38e227d3076905 "sf: kconfig: add >>>> kconfig options for spi flashes" >>>> >>>> I suspect we may remove most of these SPI flash macros, but at least >>>> SST flash macro should be kept since right now it is mixed in the >>>> generic driver with a special byte program and word program which is >>>> incompatible with other vendors' flashes. >>> >>> But there is some flash vendor specific code like quad enable bit, >>> locking ops and finally about spi_flash_params table. >>> >> >> I know. That's probably why adding all these SPI flash drivers don't >> help at all because only one code path will take effect. And what I >> did in this patch is to select one type of flash per board. > > So how about we group together 3-4 of the common ones, with no special > features, into a 'CONFIG_SPI_FLASH_GENERIC'? > Can you comment on this CONFIG_SPI_FLASH_GENERIC as Simon suggested? [snip] Regards, Bin
On 8 December 2015 at 17:27, Bin Meng <bmeng.cn@gmail.com> wrote: > Hi Jagan, > > On Fri, Dec 4, 2015 at 2:57 AM, Simon Glass <sjg@chromium.org> wrote: >> Hi, >> >> On 3 December 2015 at 06:27, Bin Meng <bmeng.cn@gmail.com> wrote: >>> Hi Jagan, >>> >>> On Thu, Dec 3, 2015 at 6:24 PM, Jagan Teki <jteki@openedev.com> wrote: >>>> Hi Bin, >>>> >>>> On 3 December 2015 at 10:14, Bin Meng <bmeng.cn@gmail.com> wrote: >>>>> Hi Simon, >>>>> >>>>> On Thu, Dec 3, 2015 at 5:05 AM, Simon Glass <sjg@chromium.org> wrote: >>>>>> +Jagan >>>>>> >>>>>> Hi Bin, >>>>>> >>>>>> On 1 December 2015 at 18:41, Bin Meng <bmeng.cn@gmail.com> wrote: >>>>>>> >>>>>>> Hi Simon, >>>>>>> >>>>>>> On Wed, Dec 2, 2015 at 12:32 AM, Simon Glass <sjg@chromium.org> wrote: >>>>>>> > Hi Bin, >>>>>>> > >>>>>>> > On 28 November 2015 at 05:45, Bin Meng <bmeng.cn@gmail.com> wrote: >>>>>>> >> Every board has one dedicated type of SPI flash, hence it is >>>>>>> >> unnecessary to include multiple SPI flash drivers. >>>>>>> >> >>>>>>> >> For QEMU and coreboot (default build of coreboot is also QEMU), >>>>>>> >> SPI flash is not supported. Remove those SPI flash drivers. >>>>>>> >> >>>>>>> >> Signed-off-by: Bin Meng <bmeng.cn@gmail.com> >>>>>>> >> --- >>>>>>> >> >>>>>>> >> configs/bayleybay_defconfig | 2 -- >>>>>>> >> configs/chromebook_link_defconfig | 2 -- >>>>>>> >> configs/chromebox_panther_defconfig | 2 -- >>>>>>> >> configs/coreboot-x86_defconfig | 4 ---- >>>>>>> >> configs/crownbay_defconfig | 3 --- >>>>>>> >> configs/galileo_defconfig | 2 -- >>>>>>> >> configs/minnowmax_defconfig | 3 --- >>>>>>> >> configs/qemu-x86_defconfig | 4 ---- >>>>>>> >> 8 files changed, 22 deletions(-) >>>>>>> > >>>>>>> > What is the benefit of this? I see it removes a few lines in a data >>>>>>> > table. Does it matter? >>>>>>> >>>>>>> Maybe we should ask the other way around, why do we create so many >>>>>>> flash driver Kconfig option? I believe the intention was footprint. >>>>>>> Besides the footprint issue, having just one flash driver in each >>>>>>> board makes it very clear instead of causing confusion. Looks other >>>>>>> board defconfig files only select one. >>>> >>>> Are you talking about flash vendor config or CONFIG_SPI_FLASH? >>>> >>> >>> Flash vendor config, as you see in this patch. >>> >>>>>> >>>>>> They are a hangover from when we had a separate driver for each one. >>>>>> Jagan put a lot of effort into removing all the semi-duplicated code. >>>>>> >>>>>> Maybe we should prune down these options? >>>>>> >>>>> >>>>> But if we already spent a lot of effort into removing all the >>>>> semi-duplicated code, we should not have converted those flash driver >>>>> to Kconfig options before. >>>>> >>>>> See commit d5af92315bb48740f16bf8817f38e227d3076905 "sf: kconfig: add >>>>> kconfig options for spi flashes" >>>>> >>>>> I suspect we may remove most of these SPI flash macros, but at least >>>>> SST flash macro should be kept since right now it is mixed in the >>>>> generic driver with a special byte program and word program which is >>>>> incompatible with other vendors' flashes. >>>> >>>> But there is some flash vendor specific code like quad enable bit, >>>> locking ops and finally about spi_flash_params table. >>>> >>> >>> I know. That's probably why adding all these SPI flash drivers don't >>> help at all because only one code path will take effect. And what I >>> did in this patch is to select one type of flash per board. >> >> So how about we group together 3-4 of the common ones, with no special >> features, into a 'CONFIG_SPI_FLASH_GENERIC'? >> > > Can you comment on this CONFIG_SPI_FLASH_GENERIC as Simon suggested? Good idea, but if we don't find enough foot-print difference on no feature flags may be we can remove those config items and I have a plan to re-arrange the sf_param_table which suits Linux may be I will come back about these things. thanks!
Hi Jagan, Simon, On Tue, Dec 8, 2015 at 11:44 PM, Jagan Teki <jteki@openedev.com> wrote: > On 8 December 2015 at 17:27, Bin Meng <bmeng.cn@gmail.com> wrote: >> Hi Jagan, >> >> On Fri, Dec 4, 2015 at 2:57 AM, Simon Glass <sjg@chromium.org> wrote: >>> Hi, >>> >>> On 3 December 2015 at 06:27, Bin Meng <bmeng.cn@gmail.com> wrote: >>>> Hi Jagan, >>>> >>>> On Thu, Dec 3, 2015 at 6:24 PM, Jagan Teki <jteki@openedev.com> wrote: >>>>> Hi Bin, >>>>> >>>>> On 3 December 2015 at 10:14, Bin Meng <bmeng.cn@gmail.com> wrote: >>>>>> Hi Simon, >>>>>> >>>>>> On Thu, Dec 3, 2015 at 5:05 AM, Simon Glass <sjg@chromium.org> wrote: >>>>>>> +Jagan >>>>>>> >>>>>>> Hi Bin, >>>>>>> >>>>>>> On 1 December 2015 at 18:41, Bin Meng <bmeng.cn@gmail.com> wrote: >>>>>>>> >>>>>>>> Hi Simon, >>>>>>>> >>>>>>>> On Wed, Dec 2, 2015 at 12:32 AM, Simon Glass <sjg@chromium.org> wrote: >>>>>>>> > Hi Bin, >>>>>>>> > >>>>>>>> > On 28 November 2015 at 05:45, Bin Meng <bmeng.cn@gmail.com> wrote: >>>>>>>> >> Every board has one dedicated type of SPI flash, hence it is >>>>>>>> >> unnecessary to include multiple SPI flash drivers. >>>>>>>> >> >>>>>>>> >> For QEMU and coreboot (default build of coreboot is also QEMU), >>>>>>>> >> SPI flash is not supported. Remove those SPI flash drivers. >>>>>>>> >> >>>>>>>> >> Signed-off-by: Bin Meng <bmeng.cn@gmail.com> >>>>>>>> >> --- >>>>>>>> >> >>>>>>>> >> configs/bayleybay_defconfig | 2 -- >>>>>>>> >> configs/chromebook_link_defconfig | 2 -- >>>>>>>> >> configs/chromebox_panther_defconfig | 2 -- >>>>>>>> >> configs/coreboot-x86_defconfig | 4 ---- >>>>>>>> >> configs/crownbay_defconfig | 3 --- >>>>>>>> >> configs/galileo_defconfig | 2 -- >>>>>>>> >> configs/minnowmax_defconfig | 3 --- >>>>>>>> >> configs/qemu-x86_defconfig | 4 ---- >>>>>>>> >> 8 files changed, 22 deletions(-) >>>>>>>> > >>>>>>>> > What is the benefit of this? I see it removes a few lines in a data >>>>>>>> > table. Does it matter? >>>>>>>> >>>>>>>> Maybe we should ask the other way around, why do we create so many >>>>>>>> flash driver Kconfig option? I believe the intention was footprint. >>>>>>>> Besides the footprint issue, having just one flash driver in each >>>>>>>> board makes it very clear instead of causing confusion. Looks other >>>>>>>> board defconfig files only select one. >>>>> >>>>> Are you talking about flash vendor config or CONFIG_SPI_FLASH? >>>>> >>>> >>>> Flash vendor config, as you see in this patch. >>>> >>>>>>> >>>>>>> They are a hangover from when we had a separate driver for each one. >>>>>>> Jagan put a lot of effort into removing all the semi-duplicated code. >>>>>>> >>>>>>> Maybe we should prune down these options? >>>>>>> >>>>>> >>>>>> But if we already spent a lot of effort into removing all the >>>>>> semi-duplicated code, we should not have converted those flash driver >>>>>> to Kconfig options before. >>>>>> >>>>>> See commit d5af92315bb48740f16bf8817f38e227d3076905 "sf: kconfig: add >>>>>> kconfig options for spi flashes" >>>>>> >>>>>> I suspect we may remove most of these SPI flash macros, but at least >>>>>> SST flash macro should be kept since right now it is mixed in the >>>>>> generic driver with a special byte program and word program which is >>>>>> incompatible with other vendors' flashes. >>>>> >>>>> But there is some flash vendor specific code like quad enable bit, >>>>> locking ops and finally about spi_flash_params table. >>>>> >>>> >>>> I know. That's probably why adding all these SPI flash drivers don't >>>> help at all because only one code path will take effect. And what I >>>> did in this patch is to select one type of flash per board. >>> >>> So how about we group together 3-4 of the common ones, with no special >>> features, into a 'CONFIG_SPI_FLASH_GENERIC'? >>> >> >> Can you comment on this CONFIG_SPI_FLASH_GENERIC as Simon suggested? > > Good idea, but if we don't find enough foot-print difference on no > feature flags may be we can remove those config items and I have a > plan to re-arrange the sf_param_table which suits Linux may be I will > come back about these things. > Can you please suggest which way should we go for this patch? I still prefer one board with one SPI flash macro. Regards, Bin
Hi Bin, On 15 December 2015 at 10:48, Bin Meng <bmeng.cn@gmail.com> wrote: > Hi Jagan, Simon, > > On Tue, Dec 8, 2015 at 11:44 PM, Jagan Teki <jteki@openedev.com> wrote: >> On 8 December 2015 at 17:27, Bin Meng <bmeng.cn@gmail.com> wrote: >>> Hi Jagan, >>> >>> On Fri, Dec 4, 2015 at 2:57 AM, Simon Glass <sjg@chromium.org> wrote: >>>> Hi, >>>> >>>> On 3 December 2015 at 06:27, Bin Meng <bmeng.cn@gmail.com> wrote: >>>>> Hi Jagan, >>>>> >>>>> On Thu, Dec 3, 2015 at 6:24 PM, Jagan Teki <jteki@openedev.com> wrote: >>>>>> Hi Bin, >>>>>> >>>>>> On 3 December 2015 at 10:14, Bin Meng <bmeng.cn@gmail.com> wrote: >>>>>>> Hi Simon, >>>>>>> >>>>>>> On Thu, Dec 3, 2015 at 5:05 AM, Simon Glass <sjg@chromium.org> wrote: >>>>>>>> +Jagan >>>>>>>> >>>>>>>> Hi Bin, >>>>>>>> >>>>>>>> On 1 December 2015 at 18:41, Bin Meng <bmeng.cn@gmail.com> wrote: >>>>>>>>> >>>>>>>>> Hi Simon, >>>>>>>>> >>>>>>>>> On Wed, Dec 2, 2015 at 12:32 AM, Simon Glass <sjg@chromium.org> wrote: >>>>>>>>> > Hi Bin, >>>>>>>>> > >>>>>>>>> > On 28 November 2015 at 05:45, Bin Meng <bmeng.cn@gmail.com> wrote: >>>>>>>>> >> Every board has one dedicated type of SPI flash, hence it is >>>>>>>>> >> unnecessary to include multiple SPI flash drivers. >>>>>>>>> >> >>>>>>>>> >> For QEMU and coreboot (default build of coreboot is also QEMU), >>>>>>>>> >> SPI flash is not supported. Remove those SPI flash drivers. >>>>>>>>> >> >>>>>>>>> >> Signed-off-by: Bin Meng <bmeng.cn@gmail.com> >>>>>>>>> >> --- >>>>>>>>> >> >>>>>>>>> >> configs/bayleybay_defconfig | 2 -- >>>>>>>>> >> configs/chromebook_link_defconfig | 2 -- >>>>>>>>> >> configs/chromebox_panther_defconfig | 2 -- >>>>>>>>> >> configs/coreboot-x86_defconfig | 4 ---- >>>>>>>>> >> configs/crownbay_defconfig | 3 --- >>>>>>>>> >> configs/galileo_defconfig | 2 -- >>>>>>>>> >> configs/minnowmax_defconfig | 3 --- >>>>>>>>> >> configs/qemu-x86_defconfig | 4 ---- >>>>>>>>> >> 8 files changed, 22 deletions(-) >>>>>>>>> > >>>>>>>>> > What is the benefit of this? I see it removes a few lines in a data >>>>>>>>> > table. Does it matter? >>>>>>>>> >>>>>>>>> Maybe we should ask the other way around, why do we create so many >>>>>>>>> flash driver Kconfig option? I believe the intention was footprint. >>>>>>>>> Besides the footprint issue, having just one flash driver in each >>>>>>>>> board makes it very clear instead of causing confusion. Looks other >>>>>>>>> board defconfig files only select one. >>>>>> >>>>>> Are you talking about flash vendor config or CONFIG_SPI_FLASH? >>>>>> >>>>> >>>>> Flash vendor config, as you see in this patch. >>>>> >>>>>>>> >>>>>>>> They are a hangover from when we had a separate driver for each one. >>>>>>>> Jagan put a lot of effort into removing all the semi-duplicated code. >>>>>>>> >>>>>>>> Maybe we should prune down these options? >>>>>>>> >>>>>>> >>>>>>> But if we already spent a lot of effort into removing all the >>>>>>> semi-duplicated code, we should not have converted those flash driver >>>>>>> to Kconfig options before. >>>>>>> >>>>>>> See commit d5af92315bb48740f16bf8817f38e227d3076905 "sf: kconfig: add >>>>>>> kconfig options for spi flashes" >>>>>>> >>>>>>> I suspect we may remove most of these SPI flash macros, but at least >>>>>>> SST flash macro should be kept since right now it is mixed in the >>>>>>> generic driver with a special byte program and word program which is >>>>>>> incompatible with other vendors' flashes. >>>>>> >>>>>> But there is some flash vendor specific code like quad enable bit, >>>>>> locking ops and finally about spi_flash_params table. >>>>>> >>>>> >>>>> I know. That's probably why adding all these SPI flash drivers don't >>>>> help at all because only one code path will take effect. And what I >>>>> did in this patch is to select one type of flash per board. >>>> >>>> So how about we group together 3-4 of the common ones, with no special >>>> features, into a 'CONFIG_SPI_FLASH_GENERIC'? >>>> >>> >>> Can you comment on this CONFIG_SPI_FLASH_GENERIC as Simon suggested? >> >> Good idea, but if we don't find enough foot-print difference on no >> feature flags may be we can remove those config items and I have a >> plan to re-arrange the sf_param_table which suits Linux may be I will >> come back about these things. >> > > Can you please suggest which way should we go for this patch? I still > prefer one board with one SPI flash macro. Sorry, I didn't get you what do you mean by one board with one SPI flash macro? Suppose if board have one controller connected with micro flash then the board file include CONFIG_SPI_FLASH_STMICRO and if another board having two controllers one connected with spansion and other connected with micro then the board file include CONFIG_SPI_FLASH_STMICRO, CONFIG_SPI_FLASH_SPANSION. It's entirely up to board that connected flash devices. thanks!
Hi Jagan, On Tue, Dec 15, 2015 at 1:51 PM, Jagan Teki <jteki@openedev.com> wrote: > Hi Bin, > > On 15 December 2015 at 10:48, Bin Meng <bmeng.cn@gmail.com> wrote: >> Hi Jagan, Simon, >> >> On Tue, Dec 8, 2015 at 11:44 PM, Jagan Teki <jteki@openedev.com> wrote: >>> On 8 December 2015 at 17:27, Bin Meng <bmeng.cn@gmail.com> wrote: >>>> Hi Jagan, >>>> >>>> On Fri, Dec 4, 2015 at 2:57 AM, Simon Glass <sjg@chromium.org> wrote: >>>>> Hi, >>>>> >>>>> On 3 December 2015 at 06:27, Bin Meng <bmeng.cn@gmail.com> wrote: >>>>>> Hi Jagan, >>>>>> >>>>>> On Thu, Dec 3, 2015 at 6:24 PM, Jagan Teki <jteki@openedev.com> wrote: >>>>>>> Hi Bin, >>>>>>> >>>>>>> On 3 December 2015 at 10:14, Bin Meng <bmeng.cn@gmail.com> wrote: >>>>>>>> Hi Simon, >>>>>>>> >>>>>>>> On Thu, Dec 3, 2015 at 5:05 AM, Simon Glass <sjg@chromium.org> wrote: >>>>>>>>> +Jagan >>>>>>>>> >>>>>>>>> Hi Bin, >>>>>>>>> >>>>>>>>> On 1 December 2015 at 18:41, Bin Meng <bmeng.cn@gmail.com> wrote: >>>>>>>>>> >>>>>>>>>> Hi Simon, >>>>>>>>>> >>>>>>>>>> On Wed, Dec 2, 2015 at 12:32 AM, Simon Glass <sjg@chromium.org> wrote: >>>>>>>>>> > Hi Bin, >>>>>>>>>> > >>>>>>>>>> > On 28 November 2015 at 05:45, Bin Meng <bmeng.cn@gmail.com> wrote: >>>>>>>>>> >> Every board has one dedicated type of SPI flash, hence it is >>>>>>>>>> >> unnecessary to include multiple SPI flash drivers. >>>>>>>>>> >> >>>>>>>>>> >> For QEMU and coreboot (default build of coreboot is also QEMU), >>>>>>>>>> >> SPI flash is not supported. Remove those SPI flash drivers. >>>>>>>>>> >> >>>>>>>>>> >> Signed-off-by: Bin Meng <bmeng.cn@gmail.com> >>>>>>>>>> >> --- >>>>>>>>>> >> >>>>>>>>>> >> configs/bayleybay_defconfig | 2 -- >>>>>>>>>> >> configs/chromebook_link_defconfig | 2 -- >>>>>>>>>> >> configs/chromebox_panther_defconfig | 2 -- >>>>>>>>>> >> configs/coreboot-x86_defconfig | 4 ---- >>>>>>>>>> >> configs/crownbay_defconfig | 3 --- >>>>>>>>>> >> configs/galileo_defconfig | 2 -- >>>>>>>>>> >> configs/minnowmax_defconfig | 3 --- >>>>>>>>>> >> configs/qemu-x86_defconfig | 4 ---- >>>>>>>>>> >> 8 files changed, 22 deletions(-) >>>>>>>>>> > >>>>>>>>>> > What is the benefit of this? I see it removes a few lines in a data >>>>>>>>>> > table. Does it matter? >>>>>>>>>> >>>>>>>>>> Maybe we should ask the other way around, why do we create so many >>>>>>>>>> flash driver Kconfig option? I believe the intention was footprint. >>>>>>>>>> Besides the footprint issue, having just one flash driver in each >>>>>>>>>> board makes it very clear instead of causing confusion. Looks other >>>>>>>>>> board defconfig files only select one. >>>>>>> >>>>>>> Are you talking about flash vendor config or CONFIG_SPI_FLASH? >>>>>>> >>>>>> >>>>>> Flash vendor config, as you see in this patch. >>>>>> >>>>>>>>> >>>>>>>>> They are a hangover from when we had a separate driver for each one. >>>>>>>>> Jagan put a lot of effort into removing all the semi-duplicated code. >>>>>>>>> >>>>>>>>> Maybe we should prune down these options? >>>>>>>>> >>>>>>>> >>>>>>>> But if we already spent a lot of effort into removing all the >>>>>>>> semi-duplicated code, we should not have converted those flash driver >>>>>>>> to Kconfig options before. >>>>>>>> >>>>>>>> See commit d5af92315bb48740f16bf8817f38e227d3076905 "sf: kconfig: add >>>>>>>> kconfig options for spi flashes" >>>>>>>> >>>>>>>> I suspect we may remove most of these SPI flash macros, but at least >>>>>>>> SST flash macro should be kept since right now it is mixed in the >>>>>>>> generic driver with a special byte program and word program which is >>>>>>>> incompatible with other vendors' flashes. >>>>>>> >>>>>>> But there is some flash vendor specific code like quad enable bit, >>>>>>> locking ops and finally about spi_flash_params table. >>>>>>> >>>>>> >>>>>> I know. That's probably why adding all these SPI flash drivers don't >>>>>> help at all because only one code path will take effect. And what I >>>>>> did in this patch is to select one type of flash per board. >>>>> >>>>> So how about we group together 3-4 of the common ones, with no special >>>>> features, into a 'CONFIG_SPI_FLASH_GENERIC'? >>>>> >>>> >>>> Can you comment on this CONFIG_SPI_FLASH_GENERIC as Simon suggested? >>> >>> Good idea, but if we don't find enough foot-print difference on no >>> feature flags may be we can remove those config items and I have a >>> plan to re-arrange the sf_param_table which suits Linux may be I will >>> come back about these things. >>> >> >> Can you please suggest which way should we go for this patch? I still >> prefer one board with one SPI flash macro. > > Sorry, I didn't get you what do you mean by one board with one SPI > flash macro? Suppose if board have one controller connected with micro > flash then the board file include CONFIG_SPI_FLASH_STMICRO and if > another board having two controllers one connected with spansion and > other connected with micro then the board file include > CONFIG_SPI_FLASH_STMICRO, CONFIG_SPI_FLASH_SPANSION. It's entirely up > to board that connected flash devices. > Yes, your understanding is the same as mine. I wasn't clear in my previous question. Right now this patch is doing exactly as what you and I understand, that we just want to select the specific flash macro for a specific x86 board. But Simon wanted to enable all of the flash macros for one board for convenience. Thus I came to ask for what's our direction. > thanks! > -- > Jagan. Regards, Bin
Hi Bin, On Tuesday 15 December 2015 11:37 AM, Bin Meng wrote: > Hi Jagan, > > On Tue, Dec 15, 2015 at 1:51 PM, Jagan Teki <jteki@openedev.com> wrote: >> Hi Bin, >> >> On 15 December 2015 at 10:48, Bin Meng <bmeng.cn@gmail.com> wrote: >>> Hi Jagan, Simon, >>> >>> On Tue, Dec 8, 2015 at 11:44 PM, Jagan Teki <jteki@openedev.com> wrote: >>>> On 8 December 2015 at 17:27, Bin Meng <bmeng.cn@gmail.com> wrote: >>>>> Hi Jagan, >>>>> >>>>> On Fri, Dec 4, 2015 at 2:57 AM, Simon Glass <sjg@chromium.org> wrote: >>>>>> Hi, >>>>>> >>>>>> On 3 December 2015 at 06:27, Bin Meng <bmeng.cn@gmail.com> wrote: >>>>>>> Hi Jagan, >>>>>>> >>>>>>> On Thu, Dec 3, 2015 at 6:24 PM, Jagan Teki <jteki@openedev.com> wrote: >>>>>>>> Hi Bin, >>>>>>>> >>>>>>>> On 3 December 2015 at 10:14, Bin Meng <bmeng.cn@gmail.com> wrote: >>>>>>>>> Hi Simon, >>>>>>>>> >>>>>>>>> On Thu, Dec 3, 2015 at 5:05 AM, Simon Glass <sjg@chromium.org> wrote: >>>>>>>>>> +Jagan >>>>>>>>>> >>>>>>>>>> Hi Bin, >>>>>>>>>> >>>>>>>>>> On 1 December 2015 at 18:41, Bin Meng <bmeng.cn@gmail.com> wrote: >>>>>>>>>>> >>>>>>>>>>> Hi Simon, >>>>>>>>>>> >>>>>>>>>>> On Wed, Dec 2, 2015 at 12:32 AM, Simon Glass <sjg@chromium.org> wrote: >>>>>>>>>>>> Hi Bin, >>>>>>>>>>>> >>>>>>>>>>>> On 28 November 2015 at 05:45, Bin Meng <bmeng.cn@gmail.com> wrote: >>>>>>>>>>>>> Every board has one dedicated type of SPI flash, hence it is >>>>>>>>>>>>> unnecessary to include multiple SPI flash drivers. >>>>>>>>>>>>> >>>>>>>>>>>>> For QEMU and coreboot (default build of coreboot is also QEMU), >>>>>>>>>>>>> SPI flash is not supported. Remove those SPI flash drivers. >>>>>>>>>>>>> >>>>>>>>>>>>> Signed-off-by: Bin Meng <bmeng.cn@gmail.com> >>>>>>>>>>>>> --- >>>>>>>>>>>>> >>>>>>>>>>>>> configs/bayleybay_defconfig | 2 -- >>>>>>>>>>>>> configs/chromebook_link_defconfig | 2 -- >>>>>>>>>>>>> configs/chromebox_panther_defconfig | 2 -- >>>>>>>>>>>>> configs/coreboot-x86_defconfig | 4 ---- >>>>>>>>>>>>> configs/crownbay_defconfig | 3 --- >>>>>>>>>>>>> configs/galileo_defconfig | 2 -- >>>>>>>>>>>>> configs/minnowmax_defconfig | 3 --- >>>>>>>>>>>>> configs/qemu-x86_defconfig | 4 ---- >>>>>>>>>>>>> 8 files changed, 22 deletions(-) >>>>>>>>>>>> >>>>>>>>>>>> What is the benefit of this? I see it removes a few lines in a data >>>>>>>>>>>> table. Does it matter? >>>>>>>>>>> >>>>>>>>>>> Maybe we should ask the other way around, why do we create so many >>>>>>>>>>> flash driver Kconfig option? I believe the intention was footprint. >>>>>>>>>>> Besides the footprint issue, having just one flash driver in each >>>>>>>>>>> board makes it very clear instead of causing confusion. Looks other >>>>>>>>>>> board defconfig files only select one. >>>>>>>> >>>>>>>> Are you talking about flash vendor config or CONFIG_SPI_FLASH? >>>>>>>> >>>>>>> >>>>>>> Flash vendor config, as you see in this patch. >>>>>>> >>>>>>>>>> >>>>>>>>>> They are a hangover from when we had a separate driver for each one. >>>>>>>>>> Jagan put a lot of effort into removing all the semi-duplicated code. >>>>>>>>>> >>>>>>>>>> Maybe we should prune down these options? >>>>>>>>>> >>>>>>>>> >>>>>>>>> But if we already spent a lot of effort into removing all the >>>>>>>>> semi-duplicated code, we should not have converted those flash driver >>>>>>>>> to Kconfig options before. >>>>>>>>> >>>>>>>>> See commit d5af92315bb48740f16bf8817f38e227d3076905 "sf: kconfig: add >>>>>>>>> kconfig options for spi flashes" >>>>>>>>> >>>>>>>>> I suspect we may remove most of these SPI flash macros, but at least >>>>>>>>> SST flash macro should be kept since right now it is mixed in the >>>>>>>>> generic driver with a special byte program and word program which is >>>>>>>>> incompatible with other vendors' flashes. >>>>>>>> >>>>>>>> But there is some flash vendor specific code like quad enable bit, >>>>>>>> locking ops and finally about spi_flash_params table. >>>>>>>> >>>>>>> >>>>>>> I know. That's probably why adding all these SPI flash drivers don't >>>>>>> help at all because only one code path will take effect. And what I >>>>>>> did in this patch is to select one type of flash per board. >>>>>> >>>>>> So how about we group together 3-4 of the common ones, with no special >>>>>> features, into a 'CONFIG_SPI_FLASH_GENERIC'? >>>>>> >>>>> >>>>> Can you comment on this CONFIG_SPI_FLASH_GENERIC as Simon suggested? >>>> >>>> Good idea, but if we don't find enough foot-print difference on no >>>> feature flags may be we can remove those config items and I have a >>>> plan to re-arrange the sf_param_table which suits Linux may be I will >>>> come back about these things. >>>> >>> >>> Can you please suggest which way should we go for this patch? I still >>> prefer one board with one SPI flash macro. >> >> Sorry, I didn't get you what do you mean by one board with one SPI >> flash macro? Suppose if board have one controller connected with micro >> flash then the board file include CONFIG_SPI_FLASH_STMICRO and if >> another board having two controllers one connected with spansion and >> other connected with micro then the board file include >> CONFIG_SPI_FLASH_STMICRO, CONFIG_SPI_FLASH_SPANSION. It's entirely up >> to board that connected flash devices. >> > > Yes, your understanding is the same as mine. I wasn't clear in my > previous question. > > Right now this patch is doing exactly as what you and I understand, > that we just want to select the specific flash macro for a specific > x86 board. But Simon wanted to enable all of the flash macros for one > board for convenience. Thus I came to ask for what's our direction. So, does this board supports or connected all flash variants? in that case it is true right? "for convenience" here means for testing then ie also true because this particular board is meant for testing all flash devices, and also it is up to this particular board config over-head rather than the generic spi-flash. thanks!
Hi Jagan, On Tue, Dec 15, 2015 at 2:15 PM, Jagan Teki <jteki@openedev.com> wrote: > Hi Bin, > > > On Tuesday 15 December 2015 11:37 AM, Bin Meng wrote: >> >> Hi Jagan, >> >> On Tue, Dec 15, 2015 at 1:51 PM, Jagan Teki <jteki@openedev.com> wrote: >>> >>> Hi Bin, >>> >>> On 15 December 2015 at 10:48, Bin Meng <bmeng.cn@gmail.com> wrote: >>>> >>>> Hi Jagan, Simon, >>>> >>>> On Tue, Dec 8, 2015 at 11:44 PM, Jagan Teki <jteki@openedev.com> wrote: >>>>> >>>>> On 8 December 2015 at 17:27, Bin Meng <bmeng.cn@gmail.com> wrote: >>>>>> >>>>>> Hi Jagan, >>>>>> >>>>>> On Fri, Dec 4, 2015 at 2:57 AM, Simon Glass <sjg@chromium.org> wrote: >>>>>>> >>>>>>> Hi, >>>>>>> >>>>>>> On 3 December 2015 at 06:27, Bin Meng <bmeng.cn@gmail.com> wrote: >>>>>>>> >>>>>>>> Hi Jagan, >>>>>>>> >>>>>>>> On Thu, Dec 3, 2015 at 6:24 PM, Jagan Teki <jteki@openedev.com> >>>>>>>> wrote: >>>>>>>>> >>>>>>>>> Hi Bin, >>>>>>>>> >>>>>>>>> On 3 December 2015 at 10:14, Bin Meng <bmeng.cn@gmail.com> wrote: >>>>>>>>>> >>>>>>>>>> Hi Simon, >>>>>>>>>> >>>>>>>>>> On Thu, Dec 3, 2015 at 5:05 AM, Simon Glass <sjg@chromium.org> >>>>>>>>>> wrote: >>>>>>>>>>> >>>>>>>>>>> +Jagan >>>>>>>>>>> >>>>>>>>>>> Hi Bin, >>>>>>>>>>> >>>>>>>>>>> On 1 December 2015 at 18:41, Bin Meng <bmeng.cn@gmail.com> wrote: >>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>> Hi Simon, >>>>>>>>>>>> >>>>>>>>>>>> On Wed, Dec 2, 2015 at 12:32 AM, Simon Glass <sjg@chromium.org> >>>>>>>>>>>> wrote: >>>>>>>>>>>>> >>>>>>>>>>>>> Hi Bin, >>>>>>>>>>>>> >>>>>>>>>>>>> On 28 November 2015 at 05:45, Bin Meng <bmeng.cn@gmail.com> >>>>>>>>>>>>> wrote: >>>>>>>>>>>>>> >>>>>>>>>>>>>> Every board has one dedicated type of SPI flash, hence it is >>>>>>>>>>>>>> unnecessary to include multiple SPI flash drivers. >>>>>>>>>>>>>> >>>>>>>>>>>>>> For QEMU and coreboot (default build of coreboot is also >>>>>>>>>>>>>> QEMU), >>>>>>>>>>>>>> SPI flash is not supported. Remove those SPI flash drivers. >>>>>>>>>>>>>> >>>>>>>>>>>>>> Signed-off-by: Bin Meng <bmeng.cn@gmail.com> >>>>>>>>>>>>>> --- >>>>>>>>>>>>>> >>>>>>>>>>>>>> configs/bayleybay_defconfig | 2 -- >>>>>>>>>>>>>> configs/chromebook_link_defconfig | 2 -- >>>>>>>>>>>>>> configs/chromebox_panther_defconfig | 2 -- >>>>>>>>>>>>>> configs/coreboot-x86_defconfig | 4 ---- >>>>>>>>>>>>>> configs/crownbay_defconfig | 3 --- >>>>>>>>>>>>>> configs/galileo_defconfig | 2 -- >>>>>>>>>>>>>> configs/minnowmax_defconfig | 3 --- >>>>>>>>>>>>>> configs/qemu-x86_defconfig | 4 ---- >>>>>>>>>>>>>> 8 files changed, 22 deletions(-) >>>>>>>>>>>>> >>>>>>>>>>>>> >>>>>>>>>>>>> What is the benefit of this? I see it removes a few lines in a >>>>>>>>>>>>> data >>>>>>>>>>>>> table. Does it matter? >>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>> Maybe we should ask the other way around, why do we create so >>>>>>>>>>>> many >>>>>>>>>>>> flash driver Kconfig option? I believe the intention was >>>>>>>>>>>> footprint. >>>>>>>>>>>> Besides the footprint issue, having just one flash driver in >>>>>>>>>>>> each >>>>>>>>>>>> board makes it very clear instead of causing confusion. Looks >>>>>>>>>>>> other >>>>>>>>>>>> board defconfig files only select one. >>>>>>>>> >>>>>>>>> >>>>>>>>> Are you talking about flash vendor config or CONFIG_SPI_FLASH? >>>>>>>>> >>>>>>>> >>>>>>>> Flash vendor config, as you see in this patch. >>>>>>>> >>>>>>>>>>> >>>>>>>>>>> They are a hangover from when we had a separate driver for each >>>>>>>>>>> one. >>>>>>>>>>> Jagan put a lot of effort into removing all the semi-duplicated >>>>>>>>>>> code. >>>>>>>>>>> >>>>>>>>>>> Maybe we should prune down these options? >>>>>>>>>>> >>>>>>>>>> >>>>>>>>>> But if we already spent a lot of effort into removing all the >>>>>>>>>> semi-duplicated code, we should not have converted those flash >>>>>>>>>> driver >>>>>>>>>> to Kconfig options before. >>>>>>>>>> >>>>>>>>>> See commit d5af92315bb48740f16bf8817f38e227d3076905 "sf: kconfig: >>>>>>>>>> add >>>>>>>>>> kconfig options for spi flashes" >>>>>>>>>> >>>>>>>>>> I suspect we may remove most of these SPI flash macros, but at >>>>>>>>>> least >>>>>>>>>> SST flash macro should be kept since right now it is mixed in the >>>>>>>>>> generic driver with a special byte program and word program which >>>>>>>>>> is >>>>>>>>>> incompatible with other vendors' flashes. >>>>>>>>> >>>>>>>>> >>>>>>>>> But there is some flash vendor specific code like quad enable bit, >>>>>>>>> locking ops and finally about spi_flash_params table. >>>>>>>>> >>>>>>>> >>>>>>>> I know. That's probably why adding all these SPI flash drivers don't >>>>>>>> help at all because only one code path will take effect. And what I >>>>>>>> did in this patch is to select one type of flash per board. >>>>>>> >>>>>>> >>>>>>> So how about we group together 3-4 of the common ones, with no >>>>>>> special >>>>>>> features, into a 'CONFIG_SPI_FLASH_GENERIC'? >>>>>>> >>>>>> >>>>>> Can you comment on this CONFIG_SPI_FLASH_GENERIC as Simon suggested? >>>>> >>>>> >>>>> Good idea, but if we don't find enough foot-print difference on no >>>>> feature flags may be we can remove those config items and I have a >>>>> plan to re-arrange the sf_param_table which suits Linux may be I will >>>>> come back about these things. >>>>> >>>> >>>> Can you please suggest which way should we go for this patch? I still >>>> prefer one board with one SPI flash macro. >>> >>> >>> Sorry, I didn't get you what do you mean by one board with one SPI >>> flash macro? Suppose if board have one controller connected with micro >>> flash then the board file include CONFIG_SPI_FLASH_STMICRO and if >>> another board having two controllers one connected with spansion and >>> other connected with micro then the board file include >>> CONFIG_SPI_FLASH_STMICRO, CONFIG_SPI_FLASH_SPANSION. It's entirely up >>> to board that connected flash devices. >>> >> >> Yes, your understanding is the same as mine. I wasn't clear in my >> previous question. >> >> Right now this patch is doing exactly as what you and I understand, >> that we just want to select the specific flash macro for a specific >> x86 board. But Simon wanted to enable all of the flash macros for one >> board for convenience. Thus I came to ask for what's our direction. > > > So, does this board supports or connected all flash variants? in that case > it is true right? "for convenience" here means for testing then ie also true > because this particular board is meant for testing all flash devices, and > also it is up to this particular board config over-head rather than the > generic spi-flash. > No, each board only connects one specific type of SPI flash, as described in the board device tree file. Regards, Bin
On 15 December 2015 at 11:48, Bin Meng <bmeng.cn@gmail.com> wrote: > Hi Jagan, > > On Tue, Dec 15, 2015 at 2:15 PM, Jagan Teki <jteki@openedev.com> wrote: >> Hi Bin, >> >> >> On Tuesday 15 December 2015 11:37 AM, Bin Meng wrote: >>> >>> Hi Jagan, >>> >>> On Tue, Dec 15, 2015 at 1:51 PM, Jagan Teki <jteki@openedev.com> wrote: >>>> >>>> Hi Bin, >>>> >>>> On 15 December 2015 at 10:48, Bin Meng <bmeng.cn@gmail.com> wrote: >>>>> >>>>> Hi Jagan, Simon, >>>>> >>>>> On Tue, Dec 8, 2015 at 11:44 PM, Jagan Teki <jteki@openedev.com> wrote: >>>>>> >>>>>> On 8 December 2015 at 17:27, Bin Meng <bmeng.cn@gmail.com> wrote: >>>>>>> >>>>>>> Hi Jagan, >>>>>>> >>>>>>> On Fri, Dec 4, 2015 at 2:57 AM, Simon Glass <sjg@chromium.org> wrote: >>>>>>>> >>>>>>>> Hi, >>>>>>>> >>>>>>>> On 3 December 2015 at 06:27, Bin Meng <bmeng.cn@gmail.com> wrote: >>>>>>>>> >>>>>>>>> Hi Jagan, >>>>>>>>> >>>>>>>>> On Thu, Dec 3, 2015 at 6:24 PM, Jagan Teki <jteki@openedev.com> >>>>>>>>> wrote: >>>>>>>>>> >>>>>>>>>> Hi Bin, >>>>>>>>>> >>>>>>>>>> On 3 December 2015 at 10:14, Bin Meng <bmeng.cn@gmail.com> wrote: >>>>>>>>>>> >>>>>>>>>>> Hi Simon, >>>>>>>>>>> >>>>>>>>>>> On Thu, Dec 3, 2015 at 5:05 AM, Simon Glass <sjg@chromium.org> >>>>>>>>>>> wrote: >>>>>>>>>>>> >>>>>>>>>>>> +Jagan >>>>>>>>>>>> >>>>>>>>>>>> Hi Bin, >>>>>>>>>>>> >>>>>>>>>>>> On 1 December 2015 at 18:41, Bin Meng <bmeng.cn@gmail.com> wrote: >>>>>>>>>>>>> >>>>>>>>>>>>> >>>>>>>>>>>>> Hi Simon, >>>>>>>>>>>>> >>>>>>>>>>>>> On Wed, Dec 2, 2015 at 12:32 AM, Simon Glass <sjg@chromium.org> >>>>>>>>>>>>> wrote: >>>>>>>>>>>>>> >>>>>>>>>>>>>> Hi Bin, >>>>>>>>>>>>>> >>>>>>>>>>>>>> On 28 November 2015 at 05:45, Bin Meng <bmeng.cn@gmail.com> >>>>>>>>>>>>>> wrote: >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> Every board has one dedicated type of SPI flash, hence it is >>>>>>>>>>>>>>> unnecessary to include multiple SPI flash drivers. >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> For QEMU and coreboot (default build of coreboot is also >>>>>>>>>>>>>>> QEMU), >>>>>>>>>>>>>>> SPI flash is not supported. Remove those SPI flash drivers. >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> Signed-off-by: Bin Meng <bmeng.cn@gmail.com> >>>>>>>>>>>>>>> --- >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> configs/bayleybay_defconfig | 2 -- >>>>>>>>>>>>>>> configs/chromebook_link_defconfig | 2 -- >>>>>>>>>>>>>>> configs/chromebox_panther_defconfig | 2 -- >>>>>>>>>>>>>>> configs/coreboot-x86_defconfig | 4 ---- >>>>>>>>>>>>>>> configs/crownbay_defconfig | 3 --- >>>>>>>>>>>>>>> configs/galileo_defconfig | 2 -- >>>>>>>>>>>>>>> configs/minnowmax_defconfig | 3 --- >>>>>>>>>>>>>>> configs/qemu-x86_defconfig | 4 ---- >>>>>>>>>>>>>>> 8 files changed, 22 deletions(-) >>>>>>>>>>>>>> >>>>>>>>>>>>>> >>>>>>>>>>>>>> What is the benefit of this? I see it removes a few lines in a >>>>>>>>>>>>>> data >>>>>>>>>>>>>> table. Does it matter? >>>>>>>>>>>>> >>>>>>>>>>>>> >>>>>>>>>>>>> Maybe we should ask the other way around, why do we create so >>>>>>>>>>>>> many >>>>>>>>>>>>> flash driver Kconfig option? I believe the intention was >>>>>>>>>>>>> footprint. >>>>>>>>>>>>> Besides the footprint issue, having just one flash driver in >>>>>>>>>>>>> each >>>>>>>>>>>>> board makes it very clear instead of causing confusion. Looks >>>>>>>>>>>>> other >>>>>>>>>>>>> board defconfig files only select one. >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> Are you talking about flash vendor config or CONFIG_SPI_FLASH? >>>>>>>>>> >>>>>>>>> >>>>>>>>> Flash vendor config, as you see in this patch. >>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>> They are a hangover from when we had a separate driver for each >>>>>>>>>>>> one. >>>>>>>>>>>> Jagan put a lot of effort into removing all the semi-duplicated >>>>>>>>>>>> code. >>>>>>>>>>>> >>>>>>>>>>>> Maybe we should prune down these options? >>>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> But if we already spent a lot of effort into removing all the >>>>>>>>>>> semi-duplicated code, we should not have converted those flash >>>>>>>>>>> driver >>>>>>>>>>> to Kconfig options before. >>>>>>>>>>> >>>>>>>>>>> See commit d5af92315bb48740f16bf8817f38e227d3076905 "sf: kconfig: >>>>>>>>>>> add >>>>>>>>>>> kconfig options for spi flashes" >>>>>>>>>>> >>>>>>>>>>> I suspect we may remove most of these SPI flash macros, but at >>>>>>>>>>> least >>>>>>>>>>> SST flash macro should be kept since right now it is mixed in the >>>>>>>>>>> generic driver with a special byte program and word program which >>>>>>>>>>> is >>>>>>>>>>> incompatible with other vendors' flashes. >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> But there is some flash vendor specific code like quad enable bit, >>>>>>>>>> locking ops and finally about spi_flash_params table. >>>>>>>>>> >>>>>>>>> >>>>>>>>> I know. That's probably why adding all these SPI flash drivers don't >>>>>>>>> help at all because only one code path will take effect. And what I >>>>>>>>> did in this patch is to select one type of flash per board. >>>>>>>> >>>>>>>> >>>>>>>> So how about we group together 3-4 of the common ones, with no >>>>>>>> special >>>>>>>> features, into a 'CONFIG_SPI_FLASH_GENERIC'? >>>>>>>> >>>>>>> >>>>>>> Can you comment on this CONFIG_SPI_FLASH_GENERIC as Simon suggested? >>>>>> >>>>>> >>>>>> Good idea, but if we don't find enough foot-print difference on no >>>>>> feature flags may be we can remove those config items and I have a >>>>>> plan to re-arrange the sf_param_table which suits Linux may be I will >>>>>> come back about these things. >>>>>> >>>>> >>>>> Can you please suggest which way should we go for this patch? I still >>>>> prefer one board with one SPI flash macro. >>>> >>>> >>>> Sorry, I didn't get you what do you mean by one board with one SPI >>>> flash macro? Suppose if board have one controller connected with micro >>>> flash then the board file include CONFIG_SPI_FLASH_STMICRO and if >>>> another board having two controllers one connected with spansion and >>>> other connected with micro then the board file include >>>> CONFIG_SPI_FLASH_STMICRO, CONFIG_SPI_FLASH_SPANSION. It's entirely up >>>> to board that connected flash devices. >>>> >>> >>> Yes, your understanding is the same as mine. I wasn't clear in my >>> previous question. >>> >>> Right now this patch is doing exactly as what you and I understand, >>> that we just want to select the specific flash macro for a specific >>> x86 board. But Simon wanted to enable all of the flash macros for one >>> board for convenience. Thus I came to ask for what's our direction. >> >> >> So, does this board supports or connected all flash variants? in that case >> it is true right? "for convenience" here means for testing then ie also true >> because this particular board is meant for testing all flash devices, and >> also it is up to this particular board config over-head rather than the >> generic spi-flash. >> > > No, each board only connects one specific type of SPI flash, as > described in the board device tree file. So your patch did that change? let me look at it what Simon commenting. thanks!
Hi, On Tue, Dec 15, 2015 at 2:27 PM, Jagan Teki <jteki@openedev.com> wrote: > On 15 December 2015 at 11:48, Bin Meng <bmeng.cn@gmail.com> wrote: > > Hi Jagan, > > > > On Tue, Dec 15, 2015 at 2:15 PM, Jagan Teki <jteki@openedev.com> wrote: > >> Hi Bin, > >> > >> > >> On Tuesday 15 December 2015 11:37 AM, Bin Meng wrote: > >>> > >>> Hi Jagan, > >>> > >>> On Tue, Dec 15, 2015 at 1:51 PM, Jagan Teki <jteki@openedev.com> > wrote: > >>>> > >>>> Hi Bin, > >>>> > >>>> On 15 December 2015 at 10:48, Bin Meng <bmeng.cn@gmail.com> wrote: > >>>>> > >>>>> Hi Jagan, Simon, > >>>>> > >>>>> On Tue, Dec 8, 2015 at 11:44 PM, Jagan Teki <jteki@openedev.com> > wrote: > >>>>>> > >>>>>> On 8 December 2015 at 17:27, Bin Meng <bmeng.cn@gmail.com> wrote: > >>>>>>> > >>>>>>> Hi Jagan, > >>>>>>> > >>>>>>> On Fri, Dec 4, 2015 at 2:57 AM, Simon Glass <sjg@chromium.org> > wrote: > >>>>>>>> > >>>>>>>> Hi, > >>>>>>>> > >>>>>>>> On 3 December 2015 at 06:27, Bin Meng <bmeng.cn@gmail.com> wrote: > >>>>>>>>> > >>>>>>>>> Hi Jagan, > >>>>>>>>> > >>>>>>>>> On Thu, Dec 3, 2015 at 6:24 PM, Jagan Teki <jteki@openedev.com> > >>>>>>>>> wrote: > >>>>>>>>>> > >>>>>>>>>> Hi Bin, > >>>>>>>>>> > >>>>>>>>>> On 3 December 2015 at 10:14, Bin Meng <bmeng.cn@gmail.com> > wrote: > >>>>>>>>>>> > >>>>>>>>>>> Hi Simon, > >>>>>>>>>>> > >>>>>>>>>>> On Thu, Dec 3, 2015 at 5:05 AM, Simon Glass <sjg@chromium.org> > >>>>>>>>>>> wrote: > >>>>>>>>>>>> > >>>>>>>>>>>> +Jagan > >>>>>>>>>>>> > >>>>>>>>>>>> Hi Bin, > >>>>>>>>>>>> > >>>>>>>>>>>> On 1 December 2015 at 18:41, Bin Meng <bmeng.cn@gmail.com> > wrote: > >>>>>>>>>>>>> > >>>>>>>>>>>>> > >>>>>>>>>>>>> Hi Simon, > >>>>>>>>>>>>> > >>>>>>>>>>>>> On Wed, Dec 2, 2015 at 12:32 AM, Simon Glass < > sjg@chromium.org> > >>>>>>>>>>>>> wrote: > >>>>>>>>>>>>>> > >>>>>>>>>>>>>> Hi Bin, > >>>>>>>>>>>>>> > >>>>>>>>>>>>>> On 28 November 2015 at 05:45, Bin Meng <bmeng.cn@gmail.com> > >>>>>>>>>>>>>> wrote: > >>>>>>>>>>>>>>> > >>>>>>>>>>>>>>> Every board has one dedicated type of SPI flash, hence it > is > >>>>>>>>>>>>>>> unnecessary to include multiple SPI flash drivers. > >>>>>>>>>>>>>>> > >>>>>>>>>>>>>>> For QEMU and coreboot (default build of coreboot is also > >>>>>>>>>>>>>>> QEMU), > >>>>>>>>>>>>>>> SPI flash is not supported. Remove those SPI flash drivers. > >>>>>>>>>>>>>>> > >>>>>>>>>>>>>>> Signed-off-by: Bin Meng <bmeng.cn@gmail.com> > >>>>>>>>>>>>>>> --- > >>>>>>>>>>>>>>> > >>>>>>>>>>>>>>> configs/bayleybay_defconfig | 2 -- > >>>>>>>>>>>>>>> configs/chromebook_link_defconfig | 2 -- > >>>>>>>>>>>>>>> configs/chromebox_panther_defconfig | 2 -- > >>>>>>>>>>>>>>> configs/coreboot-x86_defconfig | 4 ---- > >>>>>>>>>>>>>>> configs/crownbay_defconfig | 3 --- > >>>>>>>>>>>>>>> configs/galileo_defconfig | 2 -- > >>>>>>>>>>>>>>> configs/minnowmax_defconfig | 3 --- > >>>>>>>>>>>>>>> configs/qemu-x86_defconfig | 4 ---- > >>>>>>>>>>>>>>> 8 files changed, 22 deletions(-) > >>>>>>>>>>>>>> > >>>>>>>>>>>>>> > >>>>>>>>>>>>>> What is the benefit of this? I see it removes a few lines > in a > >>>>>>>>>>>>>> data > >>>>>>>>>>>>>> table. Does it matter? > >>>>>>>>>>>>> > >>>>>>>>>>>>> > >>>>>>>>>>>>> Maybe we should ask the other way around, why do we create so > >>>>>>>>>>>>> many > >>>>>>>>>>>>> flash driver Kconfig option? I believe the intention was > >>>>>>>>>>>>> footprint. > >>>>>>>>>>>>> Besides the footprint issue, having just one flash driver in > >>>>>>>>>>>>> each > >>>>>>>>>>>>> board makes it very clear instead of causing confusion. Looks > >>>>>>>>>>>>> other > >>>>>>>>>>>>> board defconfig files only select one. > >>>>>>>>>> > >>>>>>>>>> > >>>>>>>>>> Are you talking about flash vendor config or CONFIG_SPI_FLASH? > >>>>>>>>>> > >>>>>>>>> > >>>>>>>>> Flash vendor config, as you see in this patch. > >>>>>>>>> > >>>>>>>>>>>> > >>>>>>>>>>>> They are a hangover from when we had a separate driver for > each > >>>>>>>>>>>> one. > >>>>>>>>>>>> Jagan put a lot of effort into removing all the > semi-duplicated > >>>>>>>>>>>> code. > >>>>>>>>>>>> > >>>>>>>>>>>> Maybe we should prune down these options? > >>>>>>>>>>>> > >>>>>>>>>>> > >>>>>>>>>>> But if we already spent a lot of effort into removing all the > >>>>>>>>>>> semi-duplicated code, we should not have converted those flash > >>>>>>>>>>> driver > >>>>>>>>>>> to Kconfig options before. > >>>>>>>>>>> > >>>>>>>>>>> See commit d5af92315bb48740f16bf8817f38e227d3076905 "sf: > kconfig: > >>>>>>>>>>> add > >>>>>>>>>>> kconfig options for spi flashes" > >>>>>>>>>>> > >>>>>>>>>>> I suspect we may remove most of these SPI flash macros, but at > >>>>>>>>>>> least > >>>>>>>>>>> SST flash macro should be kept since right now it is mixed in > the > >>>>>>>>>>> generic driver with a special byte program and word program > which > >>>>>>>>>>> is > >>>>>>>>>>> incompatible with other vendors' flashes. > >>>>>>>>>> > >>>>>>>>>> > >>>>>>>>>> But there is some flash vendor specific code like quad enable > bit, > >>>>>>>>>> locking ops and finally about spi_flash_params table. > >>>>>>>>>> > >>>>>>>>> > >>>>>>>>> I know. That's probably why adding all these SPI flash drivers > don't > >>>>>>>>> help at all because only one code path will take effect. And > what I > >>>>>>>>> did in this patch is to select one type of flash per board. > >>>>>>>> > >>>>>>>> > >>>>>>>> So how about we group together 3-4 of the common ones, with no > >>>>>>>> special > >>>>>>>> features, into a 'CONFIG_SPI_FLASH_GENERIC'? > >>>>>>>> > >>>>>>> > >>>>>>> Can you comment on this CONFIG_SPI_FLASH_GENERIC as Simon > suggested? > >>>>>> > >>>>>> > >>>>>> Good idea, but if we don't find enough foot-print difference on no > >>>>>> feature flags may be we can remove those config items and I have a > >>>>>> plan to re-arrange the sf_param_table which suits Linux may be I > will > >>>>>> come back about these things. > >>>>>> > >>>>> > >>>>> Can you please suggest which way should we go for this patch? I still > >>>>> prefer one board with one SPI flash macro. > >>>> > >>>> > >>>> Sorry, I didn't get you what do you mean by one board with one SPI > >>>> flash macro? Suppose if board have one controller connected with micro > >>>> flash then the board file include CONFIG_SPI_FLASH_STMICRO and if > >>>> another board having two controllers one connected with spansion and > >>>> other connected with micro then the board file include > >>>> CONFIG_SPI_FLASH_STMICRO, CONFIG_SPI_FLASH_SPANSION. It's entirely up > >>>> to board that connected flash devices. > >>>> > >>> > >>> Yes, your understanding is the same as mine. I wasn't clear in my > >>> previous question. > >>> > >>> Right now this patch is doing exactly as what you and I understand, > >>> that we just want to select the specific flash macro for a specific > >>> x86 board. But Simon wanted to enable all of the flash macros for one > >>> board for convenience. Thus I came to ask for what's our direction. > >> > >> > >> So, does this board supports or connected all flash variants? in that > case > >> it is true right? "for convenience" here means for testing then ie also > true > >> because this particular board is meant for testing all flash devices, > and > >> also it is up to this particular board config over-head rather than the > >> generic spi-flash. > >> > > > > No, each board only connects one specific type of SPI flash, as > > described in the board device tree file. > > So your patch did that change? let me look at it what Simon commenting. > > I don't see any further comments on this patch. Can we close this? Regards, Bin
Hi Jagan, Simon, On Thu, Jan 21, 2016 at 1:53 PM, Bin Meng <bmeng.cn@gmail.com> wrote: > Hi, > > On Tue, Dec 15, 2015 at 2:27 PM, Jagan Teki <jteki@openedev.com> wrote: >> >> On 15 December 2015 at 11:48, Bin Meng <bmeng.cn@gmail.com> wrote: >> > Hi Jagan, >> > >> > On Tue, Dec 15, 2015 at 2:15 PM, Jagan Teki <jteki@openedev.com> wrote: >> >> Hi Bin, >> >> >> >> >> >> On Tuesday 15 December 2015 11:37 AM, Bin Meng wrote: >> >>> >> >>> Hi Jagan, >> >>> >> >>> On Tue, Dec 15, 2015 at 1:51 PM, Jagan Teki <jteki@openedev.com> >> >>> wrote: >> >>>> >> >>>> Hi Bin, >> >>>> >> >>>> On 15 December 2015 at 10:48, Bin Meng <bmeng.cn@gmail.com> wrote: >> >>>>> >> >>>>> Hi Jagan, Simon, >> >>>>> >> >>>>> On Tue, Dec 8, 2015 at 11:44 PM, Jagan Teki <jteki@openedev.com> >> >>>>> wrote: >> >>>>>> >> >>>>>> On 8 December 2015 at 17:27, Bin Meng <bmeng.cn@gmail.com> wrote: >> >>>>>>> >> >>>>>>> Hi Jagan, >> >>>>>>> >> >>>>>>> On Fri, Dec 4, 2015 at 2:57 AM, Simon Glass <sjg@chromium.org> >> >>>>>>> wrote: >> >>>>>>>> >> >>>>>>>> Hi, >> >>>>>>>> >> >>>>>>>> On 3 December 2015 at 06:27, Bin Meng <bmeng.cn@gmail.com> wrote: >> >>>>>>>>> >> >>>>>>>>> Hi Jagan, >> >>>>>>>>> >> >>>>>>>>> On Thu, Dec 3, 2015 at 6:24 PM, Jagan Teki <jteki@openedev.com> >> >>>>>>>>> wrote: >> >>>>>>>>>> >> >>>>>>>>>> Hi Bin, >> >>>>>>>>>> >> >>>>>>>>>> On 3 December 2015 at 10:14, Bin Meng <bmeng.cn@gmail.com> >> >>>>>>>>>> wrote: >> >>>>>>>>>>> >> >>>>>>>>>>> Hi Simon, >> >>>>>>>>>>> >> >>>>>>>>>>> On Thu, Dec 3, 2015 at 5:05 AM, Simon Glass <sjg@chromium.org> >> >>>>>>>>>>> wrote: >> >>>>>>>>>>>> >> >>>>>>>>>>>> +Jagan >> >>>>>>>>>>>> >> >>>>>>>>>>>> Hi Bin, >> >>>>>>>>>>>> >> >>>>>>>>>>>> On 1 December 2015 at 18:41, Bin Meng <bmeng.cn@gmail.com> >> >>>>>>>>>>>> wrote: >> >>>>>>>>>>>>> >> >>>>>>>>>>>>> >> >>>>>>>>>>>>> Hi Simon, >> >>>>>>>>>>>>> >> >>>>>>>>>>>>> On Wed, Dec 2, 2015 at 12:32 AM, Simon Glass >> >>>>>>>>>>>>> <sjg@chromium.org> >> >>>>>>>>>>>>> wrote: >> >>>>>>>>>>>>>> >> >>>>>>>>>>>>>> Hi Bin, >> >>>>>>>>>>>>>> >> >>>>>>>>>>>>>> On 28 November 2015 at 05:45, Bin Meng <bmeng.cn@gmail.com> >> >>>>>>>>>>>>>> wrote: >> >>>>>>>>>>>>>>> >> >>>>>>>>>>>>>>> Every board has one dedicated type of SPI flash, hence it >> >>>>>>>>>>>>>>> is >> >>>>>>>>>>>>>>> unnecessary to include multiple SPI flash drivers. >> >>>>>>>>>>>>>>> >> >>>>>>>>>>>>>>> For QEMU and coreboot (default build of coreboot is also >> >>>>>>>>>>>>>>> QEMU), >> >>>>>>>>>>>>>>> SPI flash is not supported. Remove those SPI flash >> >>>>>>>>>>>>>>> drivers. >> >>>>>>>>>>>>>>> >> >>>>>>>>>>>>>>> Signed-off-by: Bin Meng <bmeng.cn@gmail.com> >> >>>>>>>>>>>>>>> --- >> >>>>>>>>>>>>>>> >> >>>>>>>>>>>>>>> configs/bayleybay_defconfig | 2 -- >> >>>>>>>>>>>>>>> configs/chromebook_link_defconfig | 2 -- >> >>>>>>>>>>>>>>> configs/chromebox_panther_defconfig | 2 -- >> >>>>>>>>>>>>>>> configs/coreboot-x86_defconfig | 4 ---- >> >>>>>>>>>>>>>>> configs/crownbay_defconfig | 3 --- >> >>>>>>>>>>>>>>> configs/galileo_defconfig | 2 -- >> >>>>>>>>>>>>>>> configs/minnowmax_defconfig | 3 --- >> >>>>>>>>>>>>>>> configs/qemu-x86_defconfig | 4 ---- >> >>>>>>>>>>>>>>> 8 files changed, 22 deletions(-) >> >>>>>>>>>>>>>> >> >>>>>>>>>>>>>> >> >>>>>>>>>>>>>> What is the benefit of this? I see it removes a few lines >> >>>>>>>>>>>>>> in a >> >>>>>>>>>>>>>> data >> >>>>>>>>>>>>>> table. Does it matter? >> >>>>>>>>>>>>> >> >>>>>>>>>>>>> >> >>>>>>>>>>>>> Maybe we should ask the other way around, why do we create >> >>>>>>>>>>>>> so >> >>>>>>>>>>>>> many >> >>>>>>>>>>>>> flash driver Kconfig option? I believe the intention was >> >>>>>>>>>>>>> footprint. >> >>>>>>>>>>>>> Besides the footprint issue, having just one flash driver in >> >>>>>>>>>>>>> each >> >>>>>>>>>>>>> board makes it very clear instead of causing confusion. >> >>>>>>>>>>>>> Looks >> >>>>>>>>>>>>> other >> >>>>>>>>>>>>> board defconfig files only select one. >> >>>>>>>>>> >> >>>>>>>>>> >> >>>>>>>>>> Are you talking about flash vendor config or CONFIG_SPI_FLASH? >> >>>>>>>>>> >> >>>>>>>>> >> >>>>>>>>> Flash vendor config, as you see in this patch. >> >>>>>>>>> >> >>>>>>>>>>>> >> >>>>>>>>>>>> They are a hangover from when we had a separate driver for >> >>>>>>>>>>>> each >> >>>>>>>>>>>> one. >> >>>>>>>>>>>> Jagan put a lot of effort into removing all the >> >>>>>>>>>>>> semi-duplicated >> >>>>>>>>>>>> code. >> >>>>>>>>>>>> >> >>>>>>>>>>>> Maybe we should prune down these options? >> >>>>>>>>>>>> >> >>>>>>>>>>> >> >>>>>>>>>>> But if we already spent a lot of effort into removing all the >> >>>>>>>>>>> semi-duplicated code, we should not have converted those flash >> >>>>>>>>>>> driver >> >>>>>>>>>>> to Kconfig options before. >> >>>>>>>>>>> >> >>>>>>>>>>> See commit d5af92315bb48740f16bf8817f38e227d3076905 "sf: >> >>>>>>>>>>> kconfig: >> >>>>>>>>>>> add >> >>>>>>>>>>> kconfig options for spi flashes" >> >>>>>>>>>>> >> >>>>>>>>>>> I suspect we may remove most of these SPI flash macros, but at >> >>>>>>>>>>> least >> >>>>>>>>>>> SST flash macro should be kept since right now it is mixed in >> >>>>>>>>>>> the >> >>>>>>>>>>> generic driver with a special byte program and word program >> >>>>>>>>>>> which >> >>>>>>>>>>> is >> >>>>>>>>>>> incompatible with other vendors' flashes. >> >>>>>>>>>> >> >>>>>>>>>> >> >>>>>>>>>> But there is some flash vendor specific code like quad enable >> >>>>>>>>>> bit, >> >>>>>>>>>> locking ops and finally about spi_flash_params table. >> >>>>>>>>>> >> >>>>>>>>> >> >>>>>>>>> I know. That's probably why adding all these SPI flash drivers >> >>>>>>>>> don't >> >>>>>>>>> help at all because only one code path will take effect. And >> >>>>>>>>> what I >> >>>>>>>>> did in this patch is to select one type of flash per board. >> >>>>>>>> >> >>>>>>>> >> >>>>>>>> So how about we group together 3-4 of the common ones, with no >> >>>>>>>> special >> >>>>>>>> features, into a 'CONFIG_SPI_FLASH_GENERIC'? >> >>>>>>>> >> >>>>>>> >> >>>>>>> Can you comment on this CONFIG_SPI_FLASH_GENERIC as Simon >> >>>>>>> suggested? >> >>>>>> >> >>>>>> >> >>>>>> Good idea, but if we don't find enough foot-print difference on no >> >>>>>> feature flags may be we can remove those config items and I have a >> >>>>>> plan to re-arrange the sf_param_table which suits Linux may be I >> >>>>>> will >> >>>>>> come back about these things. >> >>>>>> >> >>>>> >> >>>>> Can you please suggest which way should we go for this patch? I >> >>>>> still >> >>>>> prefer one board with one SPI flash macro. >> >>>> >> >>>> >> >>>> Sorry, I didn't get you what do you mean by one board with one SPI >> >>>> flash macro? Suppose if board have one controller connected with >> >>>> micro >> >>>> flash then the board file include CONFIG_SPI_FLASH_STMICRO and if >> >>>> another board having two controllers one connected with spansion and >> >>>> other connected with micro then the board file include >> >>>> CONFIG_SPI_FLASH_STMICRO, CONFIG_SPI_FLASH_SPANSION. It's entirely up >> >>>> to board that connected flash devices. >> >>>> >> >>> >> >>> Yes, your understanding is the same as mine. I wasn't clear in my >> >>> previous question. >> >>> >> >>> Right now this patch is doing exactly as what you and I understand, >> >>> that we just want to select the specific flash macro for a specific >> >>> x86 board. But Simon wanted to enable all of the flash macros for one >> >>> board for convenience. Thus I came to ask for what's our direction. >> >> >> >> >> >> So, does this board supports or connected all flash variants? in that >> >> case >> >> it is true right? "for convenience" here means for testing then ie also >> >> true >> >> because this particular board is meant for testing all flash devices, >> >> and >> >> also it is up to this particular board config over-head rather than the >> >> generic spi-flash. >> >> >> > >> > No, each board only connects one specific type of SPI flash, as >> > described in the board device tree file. >> >> So your patch did that change? let me look at it what Simon commenting. >> > > I don't see any further comments on this patch. Can we close this? > A gentle ping .. I still would like to clean these up unless there is something changes in the SF (as Simon proposed) to be planned. Regards, Bin
Hi Bin, On 6 February 2016 at 09:46, Bin Meng <bmeng.cn@gmail.com> wrote: > Hi Jagan, Simon, > > On Thu, Jan 21, 2016 at 1:53 PM, Bin Meng <bmeng.cn@gmail.com> wrote: >> Hi, >> >> On Tue, Dec 15, 2015 at 2:27 PM, Jagan Teki <jteki@openedev.com> wrote: >>> >>> On 15 December 2015 at 11:48, Bin Meng <bmeng.cn@gmail.com> wrote: >>> > Hi Jagan, >>> > >>> > On Tue, Dec 15, 2015 at 2:15 PM, Jagan Teki <jteki@openedev.com> wrote: >>> >> Hi Bin, >>> >> >>> >> >>> >> On Tuesday 15 December 2015 11:37 AM, Bin Meng wrote: >>> >>> >>> >>> Hi Jagan, >>> >>> >>> >>> On Tue, Dec 15, 2015 at 1:51 PM, Jagan Teki <jteki@openedev.com> >>> >>> wrote: >>> >>>> >>> >>>> Hi Bin, >>> >>>> >>> >>>> On 15 December 2015 at 10:48, Bin Meng <bmeng.cn@gmail.com> wrote: >>> >>>>> >>> >>>>> Hi Jagan, Simon, >>> >>>>> >>> >>>>> On Tue, Dec 8, 2015 at 11:44 PM, Jagan Teki <jteki@openedev.com> >>> >>>>> wrote: >>> >>>>>> >>> >>>>>> On 8 December 2015 at 17:27, Bin Meng <bmeng.cn@gmail.com> wrote: >>> >>>>>>> >>> >>>>>>> Hi Jagan, >>> >>>>>>> >>> >>>>>>> On Fri, Dec 4, 2015 at 2:57 AM, Simon Glass <sjg@chromium.org> >>> >>>>>>> wrote: >>> >>>>>>>> >>> >>>>>>>> Hi, >>> >>>>>>>> >>> >>>>>>>> On 3 December 2015 at 06:27, Bin Meng <bmeng.cn@gmail.com> wrote: >>> >>>>>>>>> >>> >>>>>>>>> Hi Jagan, >>> >>>>>>>>> >>> >>>>>>>>> On Thu, Dec 3, 2015 at 6:24 PM, Jagan Teki <jteki@openedev.com> >>> >>>>>>>>> wrote: >>> >>>>>>>>>> >>> >>>>>>>>>> Hi Bin, >>> >>>>>>>>>> >>> >>>>>>>>>> On 3 December 2015 at 10:14, Bin Meng <bmeng.cn@gmail.com> >>> >>>>>>>>>> wrote: >>> >>>>>>>>>>> >>> >>>>>>>>>>> Hi Simon, >>> >>>>>>>>>>> >>> >>>>>>>>>>> On Thu, Dec 3, 2015 at 5:05 AM, Simon Glass <sjg@chromium.org> >>> >>>>>>>>>>> wrote: >>> >>>>>>>>>>>> >>> >>>>>>>>>>>> +Jagan >>> >>>>>>>>>>>> >>> >>>>>>>>>>>> Hi Bin, >>> >>>>>>>>>>>> >>> >>>>>>>>>>>> On 1 December 2015 at 18:41, Bin Meng <bmeng.cn@gmail.com> >>> >>>>>>>>>>>> wrote: >>> >>>>>>>>>>>>> >>> >>>>>>>>>>>>> >>> >>>>>>>>>>>>> Hi Simon, >>> >>>>>>>>>>>>> >>> >>>>>>>>>>>>> On Wed, Dec 2, 2015 at 12:32 AM, Simon Glass >>> >>>>>>>>>>>>> <sjg@chromium.org> >>> >>>>>>>>>>>>> wrote: >>> >>>>>>>>>>>>>> >>> >>>>>>>>>>>>>> Hi Bin, >>> >>>>>>>>>>>>>> >>> >>>>>>>>>>>>>> On 28 November 2015 at 05:45, Bin Meng <bmeng.cn@gmail.com> >>> >>>>>>>>>>>>>> wrote: >>> >>>>>>>>>>>>>>> >>> >>>>>>>>>>>>>>> Every board has one dedicated type of SPI flash, hence it >>> >>>>>>>>>>>>>>> is >>> >>>>>>>>>>>>>>> unnecessary to include multiple SPI flash drivers. >>> >>>>>>>>>>>>>>> >>> >>>>>>>>>>>>>>> For QEMU and coreboot (default build of coreboot is also >>> >>>>>>>>>>>>>>> QEMU), >>> >>>>>>>>>>>>>>> SPI flash is not supported. Remove those SPI flash >>> >>>>>>>>>>>>>>> drivers. >>> >>>>>>>>>>>>>>> >>> >>>>>>>>>>>>>>> Signed-off-by: Bin Meng <bmeng.cn@gmail.com> >>> >>>>>>>>>>>>>>> --- >>> >>>>>>>>>>>>>>> >>> >>>>>>>>>>>>>>> configs/bayleybay_defconfig | 2 -- >>> >>>>>>>>>>>>>>> configs/chromebook_link_defconfig | 2 -- >>> >>>>>>>>>>>>>>> configs/chromebox_panther_defconfig | 2 -- >>> >>>>>>>>>>>>>>> configs/coreboot-x86_defconfig | 4 ---- >>> >>>>>>>>>>>>>>> configs/crownbay_defconfig | 3 --- >>> >>>>>>>>>>>>>>> configs/galileo_defconfig | 2 -- >>> >>>>>>>>>>>>>>> configs/minnowmax_defconfig | 3 --- >>> >>>>>>>>>>>>>>> configs/qemu-x86_defconfig | 4 ---- >>> >>>>>>>>>>>>>>> 8 files changed, 22 deletions(-) >>> >>>>>>>>>>>>>> >>> >>>>>>>>>>>>>> >>> >>>>>>>>>>>>>> What is the benefit of this? I see it removes a few lines >>> >>>>>>>>>>>>>> in a >>> >>>>>>>>>>>>>> data >>> >>>>>>>>>>>>>> table. Does it matter? >>> >>>>>>>>>>>>> >>> >>>>>>>>>>>>> >>> >>>>>>>>>>>>> Maybe we should ask the other way around, why do we create >>> >>>>>>>>>>>>> so >>> >>>>>>>>>>>>> many >>> >>>>>>>>>>>>> flash driver Kconfig option? I believe the intention was >>> >>>>>>>>>>>>> footprint. >>> >>>>>>>>>>>>> Besides the footprint issue, having just one flash driver in >>> >>>>>>>>>>>>> each >>> >>>>>>>>>>>>> board makes it very clear instead of causing confusion. >>> >>>>>>>>>>>>> Looks >>> >>>>>>>>>>>>> other >>> >>>>>>>>>>>>> board defconfig files only select one. >>> >>>>>>>>>> >>> >>>>>>>>>> >>> >>>>>>>>>> Are you talking about flash vendor config or CONFIG_SPI_FLASH? >>> >>>>>>>>>> >>> >>>>>>>>> >>> >>>>>>>>> Flash vendor config, as you see in this patch. >>> >>>>>>>>> >>> >>>>>>>>>>>> >>> >>>>>>>>>>>> They are a hangover from when we had a separate driver for >>> >>>>>>>>>>>> each >>> >>>>>>>>>>>> one. >>> >>>>>>>>>>>> Jagan put a lot of effort into removing all the >>> >>>>>>>>>>>> semi-duplicated >>> >>>>>>>>>>>> code. >>> >>>>>>>>>>>> >>> >>>>>>>>>>>> Maybe we should prune down these options? >>> >>>>>>>>>>>> >>> >>>>>>>>>>> >>> >>>>>>>>>>> But if we already spent a lot of effort into removing all the >>> >>>>>>>>>>> semi-duplicated code, we should not have converted those flash >>> >>>>>>>>>>> driver >>> >>>>>>>>>>> to Kconfig options before. >>> >>>>>>>>>>> >>> >>>>>>>>>>> See commit d5af92315bb48740f16bf8817f38e227d3076905 "sf: >>> >>>>>>>>>>> kconfig: >>> >>>>>>>>>>> add >>> >>>>>>>>>>> kconfig options for spi flashes" >>> >>>>>>>>>>> >>> >>>>>>>>>>> I suspect we may remove most of these SPI flash macros, but at >>> >>>>>>>>>>> least >>> >>>>>>>>>>> SST flash macro should be kept since right now it is mixed in >>> >>>>>>>>>>> the >>> >>>>>>>>>>> generic driver with a special byte program and word program >>> >>>>>>>>>>> which >>> >>>>>>>>>>> is >>> >>>>>>>>>>> incompatible with other vendors' flashes. >>> >>>>>>>>>> >>> >>>>>>>>>> >>> >>>>>>>>>> But there is some flash vendor specific code like quad enable >>> >>>>>>>>>> bit, >>> >>>>>>>>>> locking ops and finally about spi_flash_params table. >>> >>>>>>>>>> >>> >>>>>>>>> >>> >>>>>>>>> I know. That's probably why adding all these SPI flash drivers >>> >>>>>>>>> don't >>> >>>>>>>>> help at all because only one code path will take effect. And >>> >>>>>>>>> what I >>> >>>>>>>>> did in this patch is to select one type of flash per board. >>> >>>>>>>> >>> >>>>>>>> >>> >>>>>>>> So how about we group together 3-4 of the common ones, with no >>> >>>>>>>> special >>> >>>>>>>> features, into a 'CONFIG_SPI_FLASH_GENERIC'? >>> >>>>>>>> >>> >>>>>>> >>> >>>>>>> Can you comment on this CONFIG_SPI_FLASH_GENERIC as Simon >>> >>>>>>> suggested? >>> >>>>>> >>> >>>>>> >>> >>>>>> Good idea, but if we don't find enough foot-print difference on no >>> >>>>>> feature flags may be we can remove those config items and I have a >>> >>>>>> plan to re-arrange the sf_param_table which suits Linux may be I >>> >>>>>> will >>> >>>>>> come back about these things. >>> >>>>>> >>> >>>>> >>> >>>>> Can you please suggest which way should we go for this patch? I >>> >>>>> still >>> >>>>> prefer one board with one SPI flash macro. >>> >>>> >>> >>>> >>> >>>> Sorry, I didn't get you what do you mean by one board with one SPI >>> >>>> flash macro? Suppose if board have one controller connected with >>> >>>> micro >>> >>>> flash then the board file include CONFIG_SPI_FLASH_STMICRO and if >>> >>>> another board having two controllers one connected with spansion and >>> >>>> other connected with micro then the board file include >>> >>>> CONFIG_SPI_FLASH_STMICRO, CONFIG_SPI_FLASH_SPANSION. It's entirely up >>> >>>> to board that connected flash devices. >>> >>>> >>> >>> >>> >>> Yes, your understanding is the same as mine. I wasn't clear in my >>> >>> previous question. >>> >>> >>> >>> Right now this patch is doing exactly as what you and I understand, >>> >>> that we just want to select the specific flash macro for a specific >>> >>> x86 board. But Simon wanted to enable all of the flash macros for one >>> >>> board for convenience. Thus I came to ask for what's our direction. >>> >> >>> >> >>> >> So, does this board supports or connected all flash variants? in that >>> >> case >>> >> it is true right? "for convenience" here means for testing then ie also >>> >> true >>> >> because this particular board is meant for testing all flash devices, >>> >> and >>> >> also it is up to this particular board config over-head rather than the >>> >> generic spi-flash. >>> >> >>> > >>> > No, each board only connects one specific type of SPI flash, as >>> > described in the board device tree file. >>> >>> So your patch did that change? let me look at it what Simon commenting. >>> >> >> I don't see any further comments on this patch. Can we close this? >> > > A gentle ping .. I still would like to clean these up unless there is > something changes in the SF (as Simon proposed) to be planned. Except - macronix, spansion, stmicro, sst, winbond configs we can remove remaining flash vendor configs for now, because later configs using common code.
Hi, On 6 February 2016 at 02:57, Jagan Teki <jteki@openedev.com> wrote: > Hi Bin, > > On 6 February 2016 at 09:46, Bin Meng <bmeng.cn@gmail.com> wrote: >> Hi Jagan, Simon, >> >> On Thu, Jan 21, 2016 at 1:53 PM, Bin Meng <bmeng.cn@gmail.com> wrote: >>> Hi, >>> >>> On Tue, Dec 15, 2015 at 2:27 PM, Jagan Teki <jteki@openedev.com> wrote: >>>> >>>> On 15 December 2015 at 11:48, Bin Meng <bmeng.cn@gmail.com> wrote: >>>> > Hi Jagan, >>>> > >>>> > On Tue, Dec 15, 2015 at 2:15 PM, Jagan Teki <jteki@openedev.com> wrote: >>>> >> Hi Bin, >>>> >> >>>> >> >>>> >> On Tuesday 15 December 2015 11:37 AM, Bin Meng wrote: >>>> >>> >>>> >>> Hi Jagan, >>>> >>> >>>> >>> On Tue, Dec 15, 2015 at 1:51 PM, Jagan Teki <jteki@openedev.com> >>>> >>> wrote: >>>> >>>> >>>> >>>> Hi Bin, >>>> >>>> >>>> >>>> On 15 December 2015 at 10:48, Bin Meng <bmeng.cn@gmail.com> wrote: >>>> >>>>> >>>> >>>>> Hi Jagan, Simon, >>>> >>>>> >>>> >>>>> On Tue, Dec 8, 2015 at 11:44 PM, Jagan Teki <jteki@openedev.com> >>>> >>>>> wrote: >>>> >>>>>> >>>> >>>>>> On 8 December 2015 at 17:27, Bin Meng <bmeng.cn@gmail.com> wrote: >>>> >>>>>>> >>>> >>>>>>> Hi Jagan, >>>> >>>>>>> >>>> >>>>>>> On Fri, Dec 4, 2015 at 2:57 AM, Simon Glass <sjg@chromium.org> >>>> >>>>>>> wrote: >>>> >>>>>>>> >>>> >>>>>>>> Hi, >>>> >>>>>>>> >>>> >>>>>>>> On 3 December 2015 at 06:27, Bin Meng <bmeng.cn@gmail.com> wrote: >>>> >>>>>>>>> >>>> >>>>>>>>> Hi Jagan, >>>> >>>>>>>>> >>>> >>>>>>>>> On Thu, Dec 3, 2015 at 6:24 PM, Jagan Teki <jteki@openedev.com> >>>> >>>>>>>>> wrote: >>>> >>>>>>>>>> >>>> >>>>>>>>>> Hi Bin, >>>> >>>>>>>>>> >>>> >>>>>>>>>> On 3 December 2015 at 10:14, Bin Meng <bmeng.cn@gmail.com> >>>> >>>>>>>>>> wrote: >>>> >>>>>>>>>>> >>>> >>>>>>>>>>> Hi Simon, >>>> >>>>>>>>>>> >>>> >>>>>>>>>>> On Thu, Dec 3, 2015 at 5:05 AM, Simon Glass <sjg@chromium.org> >>>> >>>>>>>>>>> wrote: >>>> >>>>>>>>>>>> >>>> >>>>>>>>>>>> +Jagan >>>> >>>>>>>>>>>> >>>> >>>>>>>>>>>> Hi Bin, >>>> >>>>>>>>>>>> >>>> >>>>>>>>>>>> On 1 December 2015 at 18:41, Bin Meng <bmeng.cn@gmail.com> >>>> >>>>>>>>>>>> wrote: >>>> >>>>>>>>>>>>> >>>> >>>>>>>>>>>>> >>>> >>>>>>>>>>>>> Hi Simon, >>>> >>>>>>>>>>>>> >>>> >>>>>>>>>>>>> On Wed, Dec 2, 2015 at 12:32 AM, Simon Glass >>>> >>>>>>>>>>>>> <sjg@chromium.org> >>>> >>>>>>>>>>>>> wrote: >>>> >>>>>>>>>>>>>> >>>> >>>>>>>>>>>>>> Hi Bin, >>>> >>>>>>>>>>>>>> >>>> >>>>>>>>>>>>>> On 28 November 2015 at 05:45, Bin Meng <bmeng.cn@gmail.com> >>>> >>>>>>>>>>>>>> wrote: >>>> >>>>>>>>>>>>>>> >>>> >>>>>>>>>>>>>>> Every board has one dedicated type of SPI flash, hence it >>>> >>>>>>>>>>>>>>> is >>>> >>>>>>>>>>>>>>> unnecessary to include multiple SPI flash drivers. >>>> >>>>>>>>>>>>>>> >>>> >>>>>>>>>>>>>>> For QEMU and coreboot (default build of coreboot is also >>>> >>>>>>>>>>>>>>> QEMU), >>>> >>>>>>>>>>>>>>> SPI flash is not supported. Remove those SPI flash >>>> >>>>>>>>>>>>>>> drivers. >>>> >>>>>>>>>>>>>>> >>>> >>>>>>>>>>>>>>> Signed-off-by: Bin Meng <bmeng.cn@gmail.com> >>>> >>>>>>>>>>>>>>> --- >>>> >>>>>>>>>>>>>>> >>>> >>>>>>>>>>>>>>> configs/bayleybay_defconfig | 2 -- >>>> >>>>>>>>>>>>>>> configs/chromebook_link_defconfig | 2 -- >>>> >>>>>>>>>>>>>>> configs/chromebox_panther_defconfig | 2 -- >>>> >>>>>>>>>>>>>>> configs/coreboot-x86_defconfig | 4 ---- >>>> >>>>>>>>>>>>>>> configs/crownbay_defconfig | 3 --- >>>> >>>>>>>>>>>>>>> configs/galileo_defconfig | 2 -- >>>> >>>>>>>>>>>>>>> configs/minnowmax_defconfig | 3 --- >>>> >>>>>>>>>>>>>>> configs/qemu-x86_defconfig | 4 ---- >>>> >>>>>>>>>>>>>>> 8 files changed, 22 deletions(-) >>>> >>>>>>>>>>>>>> >>>> >>>>>>>>>>>>>> >>>> >>>>>>>>>>>>>> What is the benefit of this? I see it removes a few lines >>>> >>>>>>>>>>>>>> in a >>>> >>>>>>>>>>>>>> data >>>> >>>>>>>>>>>>>> table. Does it matter? >>>> >>>>>>>>>>>>> >>>> >>>>>>>>>>>>> >>>> >>>>>>>>>>>>> Maybe we should ask the other way around, why do we create >>>> >>>>>>>>>>>>> so >>>> >>>>>>>>>>>>> many >>>> >>>>>>>>>>>>> flash driver Kconfig option? I believe the intention was >>>> >>>>>>>>>>>>> footprint. >>>> >>>>>>>>>>>>> Besides the footprint issue, having just one flash driver in >>>> >>>>>>>>>>>>> each >>>> >>>>>>>>>>>>> board makes it very clear instead of causing confusion. >>>> >>>>>>>>>>>>> Looks >>>> >>>>>>>>>>>>> other >>>> >>>>>>>>>>>>> board defconfig files only select one. >>>> >>>>>>>>>> >>>> >>>>>>>>>> >>>> >>>>>>>>>> Are you talking about flash vendor config or CONFIG_SPI_FLASH? >>>> >>>>>>>>>> >>>> >>>>>>>>> >>>> >>>>>>>>> Flash vendor config, as you see in this patch. >>>> >>>>>>>>> >>>> >>>>>>>>>>>> >>>> >>>>>>>>>>>> They are a hangover from when we had a separate driver for >>>> >>>>>>>>>>>> each >>>> >>>>>>>>>>>> one. >>>> >>>>>>>>>>>> Jagan put a lot of effort into removing all the >>>> >>>>>>>>>>>> semi-duplicated >>>> >>>>>>>>>>>> code. >>>> >>>>>>>>>>>> >>>> >>>>>>>>>>>> Maybe we should prune down these options? >>>> >>>>>>>>>>>> >>>> >>>>>>>>>>> >>>> >>>>>>>>>>> But if we already spent a lot of effort into removing all the >>>> >>>>>>>>>>> semi-duplicated code, we should not have converted those flash >>>> >>>>>>>>>>> driver >>>> >>>>>>>>>>> to Kconfig options before. >>>> >>>>>>>>>>> >>>> >>>>>>>>>>> See commit d5af92315bb48740f16bf8817f38e227d3076905 "sf: >>>> >>>>>>>>>>> kconfig: >>>> >>>>>>>>>>> add >>>> >>>>>>>>>>> kconfig options for spi flashes" >>>> >>>>>>>>>>> >>>> >>>>>>>>>>> I suspect we may remove most of these SPI flash macros, but at >>>> >>>>>>>>>>> least >>>> >>>>>>>>>>> SST flash macro should be kept since right now it is mixed in >>>> >>>>>>>>>>> the >>>> >>>>>>>>>>> generic driver with a special byte program and word program >>>> >>>>>>>>>>> which >>>> >>>>>>>>>>> is >>>> >>>>>>>>>>> incompatible with other vendors' flashes. >>>> >>>>>>>>>> >>>> >>>>>>>>>> >>>> >>>>>>>>>> But there is some flash vendor specific code like quad enable >>>> >>>>>>>>>> bit, >>>> >>>>>>>>>> locking ops and finally about spi_flash_params table. >>>> >>>>>>>>>> >>>> >>>>>>>>> >>>> >>>>>>>>> I know. That's probably why adding all these SPI flash drivers >>>> >>>>>>>>> don't >>>> >>>>>>>>> help at all because only one code path will take effect. And >>>> >>>>>>>>> what I >>>> >>>>>>>>> did in this patch is to select one type of flash per board. >>>> >>>>>>>> >>>> >>>>>>>> >>>> >>>>>>>> So how about we group together 3-4 of the common ones, with no >>>> >>>>>>>> special >>>> >>>>>>>> features, into a 'CONFIG_SPI_FLASH_GENERIC'? >>>> >>>>>>>> >>>> >>>>>>> >>>> >>>>>>> Can you comment on this CONFIG_SPI_FLASH_GENERIC as Simon >>>> >>>>>>> suggested? >>>> >>>>>> >>>> >>>>>> >>>> >>>>>> Good idea, but if we don't find enough foot-print difference on no >>>> >>>>>> feature flags may be we can remove those config items and I have a >>>> >>>>>> plan to re-arrange the sf_param_table which suits Linux may be I >>>> >>>>>> will >>>> >>>>>> come back about these things. >>>> >>>>>> >>>> >>>>> >>>> >>>>> Can you please suggest which way should we go for this patch? I >>>> >>>>> still >>>> >>>>> prefer one board with one SPI flash macro. >>>> >>>> >>>> >>>> >>>> >>>> Sorry, I didn't get you what do you mean by one board with one SPI >>>> >>>> flash macro? Suppose if board have one controller connected with >>>> >>>> micro >>>> >>>> flash then the board file include CONFIG_SPI_FLASH_STMICRO and if >>>> >>>> another board having two controllers one connected with spansion and >>>> >>>> other connected with micro then the board file include >>>> >>>> CONFIG_SPI_FLASH_STMICRO, CONFIG_SPI_FLASH_SPANSION. It's entirely up >>>> >>>> to board that connected flash devices. >>>> >>>> >>>> >>> >>>> >>> Yes, your understanding is the same as mine. I wasn't clear in my >>>> >>> previous question. >>>> >>> >>>> >>> Right now this patch is doing exactly as what you and I understand, >>>> >>> that we just want to select the specific flash macro for a specific >>>> >>> x86 board. But Simon wanted to enable all of the flash macros for one >>>> >>> board for convenience. Thus I came to ask for what's our direction. >>>> >> >>>> >> >>>> >> So, does this board supports or connected all flash variants? in that >>>> >> case >>>> >> it is true right? "for convenience" here means for testing then ie also >>>> >> true >>>> >> because this particular board is meant for testing all flash devices, >>>> >> and >>>> >> also it is up to this particular board config over-head rather than the >>>> >> generic spi-flash. >>>> >> >>>> > >>>> > No, each board only connects one specific type of SPI flash, as >>>> > described in the board device tree file. >>>> >>>> So your patch did that change? let me look at it what Simon commenting. >>>> >>> >>> I don't see any further comments on this patch. Can we close this? >>> >> >> A gentle ping .. I still would like to clean these up unless there is >> something changes in the SF (as Simon proposed) to be planned. > > Except - macronix, spansion, stmicro, sst, winbond configs we can > remove remaining flash vendor configs for now, because later configs > using common code. Does this mean the CONFIG_SPI_FLASH_GENERIC idea is good, or not? Regards, Simon
Hi Simon, On 12 February 2016 at 21:24, Simon Glass <sjg@chromium.org> wrote: > Hi, > > On 6 February 2016 at 02:57, Jagan Teki <jteki@openedev.com> wrote: >> Hi Bin, >> >> On 6 February 2016 at 09:46, Bin Meng <bmeng.cn@gmail.com> wrote: >>> Hi Jagan, Simon, >>> >>> On Thu, Jan 21, 2016 at 1:53 PM, Bin Meng <bmeng.cn@gmail.com> wrote: >>>> Hi, >>>> >>>> On Tue, Dec 15, 2015 at 2:27 PM, Jagan Teki <jteki@openedev.com> wrote: >>>>> >>>>> On 15 December 2015 at 11:48, Bin Meng <bmeng.cn@gmail.com> wrote: >>>>> > Hi Jagan, >>>>> > >>>>> > On Tue, Dec 15, 2015 at 2:15 PM, Jagan Teki <jteki@openedev.com> wrote: >>>>> >> Hi Bin, >>>>> >> >>>>> >> >>>>> >> On Tuesday 15 December 2015 11:37 AM, Bin Meng wrote: >>>>> >>> >>>>> >>> Hi Jagan, >>>>> >>> >>>>> >>> On Tue, Dec 15, 2015 at 1:51 PM, Jagan Teki <jteki@openedev.com> >>>>> >>> wrote: >>>>> >>>> >>>>> >>>> Hi Bin, >>>>> >>>> >>>>> >>>> On 15 December 2015 at 10:48, Bin Meng <bmeng.cn@gmail.com> wrote: >>>>> >>>>> >>>>> >>>>> Hi Jagan, Simon, >>>>> >>>>> >>>>> >>>>> On Tue, Dec 8, 2015 at 11:44 PM, Jagan Teki <jteki@openedev.com> >>>>> >>>>> wrote: >>>>> >>>>>> >>>>> >>>>>> On 8 December 2015 at 17:27, Bin Meng <bmeng.cn@gmail.com> wrote: >>>>> >>>>>>> >>>>> >>>>>>> Hi Jagan, >>>>> >>>>>>> >>>>> >>>>>>> On Fri, Dec 4, 2015 at 2:57 AM, Simon Glass <sjg@chromium.org> >>>>> >>>>>>> wrote: >>>>> >>>>>>>> >>>>> >>>>>>>> Hi, >>>>> >>>>>>>> >>>>> >>>>>>>> On 3 December 2015 at 06:27, Bin Meng <bmeng.cn@gmail.com> wrote: >>>>> >>>>>>>>> >>>>> >>>>>>>>> Hi Jagan, >>>>> >>>>>>>>> >>>>> >>>>>>>>> On Thu, Dec 3, 2015 at 6:24 PM, Jagan Teki <jteki@openedev.com> >>>>> >>>>>>>>> wrote: >>>>> >>>>>>>>>> >>>>> >>>>>>>>>> Hi Bin, >>>>> >>>>>>>>>> >>>>> >>>>>>>>>> On 3 December 2015 at 10:14, Bin Meng <bmeng.cn@gmail.com> >>>>> >>>>>>>>>> wrote: >>>>> >>>>>>>>>>> >>>>> >>>>>>>>>>> Hi Simon, >>>>> >>>>>>>>>>> >>>>> >>>>>>>>>>> On Thu, Dec 3, 2015 at 5:05 AM, Simon Glass <sjg@chromium.org> >>>>> >>>>>>>>>>> wrote: >>>>> >>>>>>>>>>>> >>>>> >>>>>>>>>>>> +Jagan >>>>> >>>>>>>>>>>> >>>>> >>>>>>>>>>>> Hi Bin, >>>>> >>>>>>>>>>>> >>>>> >>>>>>>>>>>> On 1 December 2015 at 18:41, Bin Meng <bmeng.cn@gmail.com> >>>>> >>>>>>>>>>>> wrote: >>>>> >>>>>>>>>>>>> >>>>> >>>>>>>>>>>>> >>>>> >>>>>>>>>>>>> Hi Simon, >>>>> >>>>>>>>>>>>> >>>>> >>>>>>>>>>>>> On Wed, Dec 2, 2015 at 12:32 AM, Simon Glass >>>>> >>>>>>>>>>>>> <sjg@chromium.org> >>>>> >>>>>>>>>>>>> wrote: >>>>> >>>>>>>>>>>>>> >>>>> >>>>>>>>>>>>>> Hi Bin, >>>>> >>>>>>>>>>>>>> >>>>> >>>>>>>>>>>>>> On 28 November 2015 at 05:45, Bin Meng <bmeng.cn@gmail.com> >>>>> >>>>>>>>>>>>>> wrote: >>>>> >>>>>>>>>>>>>>> >>>>> >>>>>>>>>>>>>>> Every board has one dedicated type of SPI flash, hence it >>>>> >>>>>>>>>>>>>>> is >>>>> >>>>>>>>>>>>>>> unnecessary to include multiple SPI flash drivers. >>>>> >>>>>>>>>>>>>>> >>>>> >>>>>>>>>>>>>>> For QEMU and coreboot (default build of coreboot is also >>>>> >>>>>>>>>>>>>>> QEMU), >>>>> >>>>>>>>>>>>>>> SPI flash is not supported. Remove those SPI flash >>>>> >>>>>>>>>>>>>>> drivers. >>>>> >>>>>>>>>>>>>>> >>>>> >>>>>>>>>>>>>>> Signed-off-by: Bin Meng <bmeng.cn@gmail.com> >>>>> >>>>>>>>>>>>>>> --- >>>>> >>>>>>>>>>>>>>> >>>>> >>>>>>>>>>>>>>> configs/bayleybay_defconfig | 2 -- >>>>> >>>>>>>>>>>>>>> configs/chromebook_link_defconfig | 2 -- >>>>> >>>>>>>>>>>>>>> configs/chromebox_panther_defconfig | 2 -- >>>>> >>>>>>>>>>>>>>> configs/coreboot-x86_defconfig | 4 ---- >>>>> >>>>>>>>>>>>>>> configs/crownbay_defconfig | 3 --- >>>>> >>>>>>>>>>>>>>> configs/galileo_defconfig | 2 -- >>>>> >>>>>>>>>>>>>>> configs/minnowmax_defconfig | 3 --- >>>>> >>>>>>>>>>>>>>> configs/qemu-x86_defconfig | 4 ---- >>>>> >>>>>>>>>>>>>>> 8 files changed, 22 deletions(-) >>>>> >>>>>>>>>>>>>> >>>>> >>>>>>>>>>>>>> >>>>> >>>>>>>>>>>>>> What is the benefit of this? I see it removes a few lines >>>>> >>>>>>>>>>>>>> in a >>>>> >>>>>>>>>>>>>> data >>>>> >>>>>>>>>>>>>> table. Does it matter? >>>>> >>>>>>>>>>>>> >>>>> >>>>>>>>>>>>> >>>>> >>>>>>>>>>>>> Maybe we should ask the other way around, why do we create >>>>> >>>>>>>>>>>>> so >>>>> >>>>>>>>>>>>> many >>>>> >>>>>>>>>>>>> flash driver Kconfig option? I believe the intention was >>>>> >>>>>>>>>>>>> footprint. >>>>> >>>>>>>>>>>>> Besides the footprint issue, having just one flash driver in >>>>> >>>>>>>>>>>>> each >>>>> >>>>>>>>>>>>> board makes it very clear instead of causing confusion. >>>>> >>>>>>>>>>>>> Looks >>>>> >>>>>>>>>>>>> other >>>>> >>>>>>>>>>>>> board defconfig files only select one. >>>>> >>>>>>>>>> >>>>> >>>>>>>>>> >>>>> >>>>>>>>>> Are you talking about flash vendor config or CONFIG_SPI_FLASH? >>>>> >>>>>>>>>> >>>>> >>>>>>>>> >>>>> >>>>>>>>> Flash vendor config, as you see in this patch. >>>>> >>>>>>>>> >>>>> >>>>>>>>>>>> >>>>> >>>>>>>>>>>> They are a hangover from when we had a separate driver for >>>>> >>>>>>>>>>>> each >>>>> >>>>>>>>>>>> one. >>>>> >>>>>>>>>>>> Jagan put a lot of effort into removing all the >>>>> >>>>>>>>>>>> semi-duplicated >>>>> >>>>>>>>>>>> code. >>>>> >>>>>>>>>>>> >>>>> >>>>>>>>>>>> Maybe we should prune down these options? >>>>> >>>>>>>>>>>> >>>>> >>>>>>>>>>> >>>>> >>>>>>>>>>> But if we already spent a lot of effort into removing all the >>>>> >>>>>>>>>>> semi-duplicated code, we should not have converted those flash >>>>> >>>>>>>>>>> driver >>>>> >>>>>>>>>>> to Kconfig options before. >>>>> >>>>>>>>>>> >>>>> >>>>>>>>>>> See commit d5af92315bb48740f16bf8817f38e227d3076905 "sf: >>>>> >>>>>>>>>>> kconfig: >>>>> >>>>>>>>>>> add >>>>> >>>>>>>>>>> kconfig options for spi flashes" >>>>> >>>>>>>>>>> >>>>> >>>>>>>>>>> I suspect we may remove most of these SPI flash macros, but at >>>>> >>>>>>>>>>> least >>>>> >>>>>>>>>>> SST flash macro should be kept since right now it is mixed in >>>>> >>>>>>>>>>> the >>>>> >>>>>>>>>>> generic driver with a special byte program and word program >>>>> >>>>>>>>>>> which >>>>> >>>>>>>>>>> is >>>>> >>>>>>>>>>> incompatible with other vendors' flashes. >>>>> >>>>>>>>>> >>>>> >>>>>>>>>> >>>>> >>>>>>>>>> But there is some flash vendor specific code like quad enable >>>>> >>>>>>>>>> bit, >>>>> >>>>>>>>>> locking ops and finally about spi_flash_params table. >>>>> >>>>>>>>>> >>>>> >>>>>>>>> >>>>> >>>>>>>>> I know. That's probably why adding all these SPI flash drivers >>>>> >>>>>>>>> don't >>>>> >>>>>>>>> help at all because only one code path will take effect. And >>>>> >>>>>>>>> what I >>>>> >>>>>>>>> did in this patch is to select one type of flash per board. >>>>> >>>>>>>> >>>>> >>>>>>>> >>>>> >>>>>>>> So how about we group together 3-4 of the common ones, with no >>>>> >>>>>>>> special >>>>> >>>>>>>> features, into a 'CONFIG_SPI_FLASH_GENERIC'? >>>>> >>>>>>>> >>>>> >>>>>>> >>>>> >>>>>>> Can you comment on this CONFIG_SPI_FLASH_GENERIC as Simon >>>>> >>>>>>> suggested? >>>>> >>>>>> >>>>> >>>>>> >>>>> >>>>>> Good idea, but if we don't find enough foot-print difference on no >>>>> >>>>>> feature flags may be we can remove those config items and I have a >>>>> >>>>>> plan to re-arrange the sf_param_table which suits Linux may be I >>>>> >>>>>> will >>>>> >>>>>> come back about these things. >>>>> >>>>>> >>>>> >>>>> >>>>> >>>>> Can you please suggest which way should we go for this patch? I >>>>> >>>>> still >>>>> >>>>> prefer one board with one SPI flash macro. >>>>> >>>> >>>>> >>>> >>>>> >>>> Sorry, I didn't get you what do you mean by one board with one SPI >>>>> >>>> flash macro? Suppose if board have one controller connected with >>>>> >>>> micro >>>>> >>>> flash then the board file include CONFIG_SPI_FLASH_STMICRO and if >>>>> >>>> another board having two controllers one connected with spansion and >>>>> >>>> other connected with micro then the board file include >>>>> >>>> CONFIG_SPI_FLASH_STMICRO, CONFIG_SPI_FLASH_SPANSION. It's entirely up >>>>> >>>> to board that connected flash devices. >>>>> >>>> >>>>> >>> >>>>> >>> Yes, your understanding is the same as mine. I wasn't clear in my >>>>> >>> previous question. >>>>> >>> >>>>> >>> Right now this patch is doing exactly as what you and I understand, >>>>> >>> that we just want to select the specific flash macro for a specific >>>>> >>> x86 board. But Simon wanted to enable all of the flash macros for one >>>>> >>> board for convenience. Thus I came to ask for what's our direction. >>>>> >> >>>>> >> >>>>> >> So, does this board supports or connected all flash variants? in that >>>>> >> case >>>>> >> it is true right? "for convenience" here means for testing then ie also >>>>> >> true >>>>> >> because this particular board is meant for testing all flash devices, >>>>> >> and >>>>> >> also it is up to this particular board config over-head rather than the >>>>> >> generic spi-flash. >>>>> >> >>>>> > >>>>> > No, each board only connects one specific type of SPI flash, as >>>>> > described in the board device tree file. >>>>> >>>>> So your patch did that change? let me look at it what Simon commenting. >>>>> >>>> >>>> I don't see any further comments on this patch. Can we close this? >>>> >>> >>> A gentle ping .. I still would like to clean these up unless there is >>> something changes in the SF (as Simon proposed) to be planned. >> >> Except - macronix, spansion, stmicro, sst, winbond configs we can >> remove remaining flash vendor configs for now, because later configs >> using common code. > > Does this mean the CONFIG_SPI_FLASH_GENERIC idea is good, or not? If foot-print is not so huge with those configs I think it's better not to add an extra config - what do you think?
Hi Jagan, On 12 February 2016 at 09:31, Jagan Teki <jteki@openedev.com> wrote: > Hi Simon, > > On 12 February 2016 at 21:24, Simon Glass <sjg@chromium.org> wrote: >> Hi, >> >> On 6 February 2016 at 02:57, Jagan Teki <jteki@openedev.com> wrote: >>> Hi Bin, >>> >>> On 6 February 2016 at 09:46, Bin Meng <bmeng.cn@gmail.com> wrote: >>>> Hi Jagan, Simon, >>>> >>>> On Thu, Jan 21, 2016 at 1:53 PM, Bin Meng <bmeng.cn@gmail.com> wrote: >>>>> Hi, >>>>> >>>>> On Tue, Dec 15, 2015 at 2:27 PM, Jagan Teki <jteki@openedev.com> wrote: >>>>>> >>>>>> On 15 December 2015 at 11:48, Bin Meng <bmeng.cn@gmail.com> wrote: >>>>>> > Hi Jagan, >>>>>> > >>>>>> > On Tue, Dec 15, 2015 at 2:15 PM, Jagan Teki <jteki@openedev.com> wrote: >>>>>> >> Hi Bin, >>>>>> >> >>>>>> >> >>>>>> >> On Tuesday 15 December 2015 11:37 AM, Bin Meng wrote: >>>>>> >>> >>>>>> >>> Hi Jagan, >>>>>> >>> >>>>>> >>> On Tue, Dec 15, 2015 at 1:51 PM, Jagan Teki <jteki@openedev.com> >>>>>> >>> wrote: >>>>>> >>>> >>>>>> >>>> Hi Bin, >>>>>> >>>> >>>>>> >>>> On 15 December 2015 at 10:48, Bin Meng <bmeng.cn@gmail.com> wrote: >>>>>> >>>>> >>>>>> >>>>> Hi Jagan, Simon, >>>>>> >>>>> >>>>>> >>>>> On Tue, Dec 8, 2015 at 11:44 PM, Jagan Teki <jteki@openedev.com> >>>>>> >>>>> wrote: >>>>>> >>>>>> >>>>>> >>>>>> On 8 December 2015 at 17:27, Bin Meng <bmeng.cn@gmail.com> wrote: >>>>>> >>>>>>> >>>>>> >>>>>>> Hi Jagan, >>>>>> >>>>>>> >>>>>> >>>>>>> On Fri, Dec 4, 2015 at 2:57 AM, Simon Glass <sjg@chromium.org> >>>>>> >>>>>>> wrote: >>>>>> >>>>>>>> >>>>>> >>>>>>>> Hi, >>>>>> >>>>>>>> >>>>>> >>>>>>>> On 3 December 2015 at 06:27, Bin Meng <bmeng.cn@gmail.com> wrote: >>>>>> >>>>>>>>> >>>>>> >>>>>>>>> Hi Jagan, >>>>>> >>>>>>>>> >>>>>> >>>>>>>>> On Thu, Dec 3, 2015 at 6:24 PM, Jagan Teki <jteki@openedev.com> >>>>>> >>>>>>>>> wrote: >>>>>> >>>>>>>>>> >>>>>> >>>>>>>>>> Hi Bin, >>>>>> >>>>>>>>>> >>>>>> >>>>>>>>>> On 3 December 2015 at 10:14, Bin Meng <bmeng.cn@gmail.com> >>>>>> >>>>>>>>>> wrote: >>>>>> >>>>>>>>>>> >>>>>> >>>>>>>>>>> Hi Simon, >>>>>> >>>>>>>>>>> >>>>>> >>>>>>>>>>> On Thu, Dec 3, 2015 at 5:05 AM, Simon Glass <sjg@chromium.org> >>>>>> >>>>>>>>>>> wrote: >>>>>> >>>>>>>>>>>> >>>>>> >>>>>>>>>>>> +Jagan >>>>>> >>>>>>>>>>>> >>>>>> >>>>>>>>>>>> Hi Bin, >>>>>> >>>>>>>>>>>> >>>>>> >>>>>>>>>>>> On 1 December 2015 at 18:41, Bin Meng <bmeng.cn@gmail.com> >>>>>> >>>>>>>>>>>> wrote: >>>>>> >>>>>>>>>>>>> >>>>>> >>>>>>>>>>>>> >>>>>> >>>>>>>>>>>>> Hi Simon, >>>>>> >>>>>>>>>>>>> >>>>>> >>>>>>>>>>>>> On Wed, Dec 2, 2015 at 12:32 AM, Simon Glass >>>>>> >>>>>>>>>>>>> <sjg@chromium.org> >>>>>> >>>>>>>>>>>>> wrote: >>>>>> >>>>>>>>>>>>>> >>>>>> >>>>>>>>>>>>>> Hi Bin, >>>>>> >>>>>>>>>>>>>> >>>>>> >>>>>>>>>>>>>> On 28 November 2015 at 05:45, Bin Meng <bmeng.cn@gmail.com> >>>>>> >>>>>>>>>>>>>> wrote: >>>>>> >>>>>>>>>>>>>>> >>>>>> >>>>>>>>>>>>>>> Every board has one dedicated type of SPI flash, hence it >>>>>> >>>>>>>>>>>>>>> is >>>>>> >>>>>>>>>>>>>>> unnecessary to include multiple SPI flash drivers. >>>>>> >>>>>>>>>>>>>>> >>>>>> >>>>>>>>>>>>>>> For QEMU and coreboot (default build of coreboot is also >>>>>> >>>>>>>>>>>>>>> QEMU), >>>>>> >>>>>>>>>>>>>>> SPI flash is not supported. Remove those SPI flash >>>>>> >>>>>>>>>>>>>>> drivers. >>>>>> >>>>>>>>>>>>>>> >>>>>> >>>>>>>>>>>>>>> Signed-off-by: Bin Meng <bmeng.cn@gmail.com> >>>>>> >>>>>>>>>>>>>>> --- >>>>>> >>>>>>>>>>>>>>> >>>>>> >>>>>>>>>>>>>>> configs/bayleybay_defconfig | 2 -- >>>>>> >>>>>>>>>>>>>>> configs/chromebook_link_defconfig | 2 -- >>>>>> >>>>>>>>>>>>>>> configs/chromebox_panther_defconfig | 2 -- >>>>>> >>>>>>>>>>>>>>> configs/coreboot-x86_defconfig | 4 ---- >>>>>> >>>>>>>>>>>>>>> configs/crownbay_defconfig | 3 --- >>>>>> >>>>>>>>>>>>>>> configs/galileo_defconfig | 2 -- >>>>>> >>>>>>>>>>>>>>> configs/minnowmax_defconfig | 3 --- >>>>>> >>>>>>>>>>>>>>> configs/qemu-x86_defconfig | 4 ---- >>>>>> >>>>>>>>>>>>>>> 8 files changed, 22 deletions(-) >>>>>> >>>>>>>>>>>>>> >>>>>> >>>>>>>>>>>>>> >>>>>> >>>>>>>>>>>>>> What is the benefit of this? I see it removes a few lines >>>>>> >>>>>>>>>>>>>> in a >>>>>> >>>>>>>>>>>>>> data >>>>>> >>>>>>>>>>>>>> table. Does it matter? >>>>>> >>>>>>>>>>>>> >>>>>> >>>>>>>>>>>>> >>>>>> >>>>>>>>>>>>> Maybe we should ask the other way around, why do we create >>>>>> >>>>>>>>>>>>> so >>>>>> >>>>>>>>>>>>> many >>>>>> >>>>>>>>>>>>> flash driver Kconfig option? I believe the intention was >>>>>> >>>>>>>>>>>>> footprint. >>>>>> >>>>>>>>>>>>> Besides the footprint issue, having just one flash driver in >>>>>> >>>>>>>>>>>>> each >>>>>> >>>>>>>>>>>>> board makes it very clear instead of causing confusion. >>>>>> >>>>>>>>>>>>> Looks >>>>>> >>>>>>>>>>>>> other >>>>>> >>>>>>>>>>>>> board defconfig files only select one. >>>>>> >>>>>>>>>> >>>>>> >>>>>>>>>> >>>>>> >>>>>>>>>> Are you talking about flash vendor config or CONFIG_SPI_FLASH? >>>>>> >>>>>>>>>> >>>>>> >>>>>>>>> >>>>>> >>>>>>>>> Flash vendor config, as you see in this patch. >>>>>> >>>>>>>>> >>>>>> >>>>>>>>>>>> >>>>>> >>>>>>>>>>>> They are a hangover from when we had a separate driver for >>>>>> >>>>>>>>>>>> each >>>>>> >>>>>>>>>>>> one. >>>>>> >>>>>>>>>>>> Jagan put a lot of effort into removing all the >>>>>> >>>>>>>>>>>> semi-duplicated >>>>>> >>>>>>>>>>>> code. >>>>>> >>>>>>>>>>>> >>>>>> >>>>>>>>>>>> Maybe we should prune down these options? >>>>>> >>>>>>>>>>>> >>>>>> >>>>>>>>>>> >>>>>> >>>>>>>>>>> But if we already spent a lot of effort into removing all the >>>>>> >>>>>>>>>>> semi-duplicated code, we should not have converted those flash >>>>>> >>>>>>>>>>> driver >>>>>> >>>>>>>>>>> to Kconfig options before. >>>>>> >>>>>>>>>>> >>>>>> >>>>>>>>>>> See commit d5af92315bb48740f16bf8817f38e227d3076905 "sf: >>>>>> >>>>>>>>>>> kconfig: >>>>>> >>>>>>>>>>> add >>>>>> >>>>>>>>>>> kconfig options for spi flashes" >>>>>> >>>>>>>>>>> >>>>>> >>>>>>>>>>> I suspect we may remove most of these SPI flash macros, but at >>>>>> >>>>>>>>>>> least >>>>>> >>>>>>>>>>> SST flash macro should be kept since right now it is mixed in >>>>>> >>>>>>>>>>> the >>>>>> >>>>>>>>>>> generic driver with a special byte program and word program >>>>>> >>>>>>>>>>> which >>>>>> >>>>>>>>>>> is >>>>>> >>>>>>>>>>> incompatible with other vendors' flashes. >>>>>> >>>>>>>>>> >>>>>> >>>>>>>>>> >>>>>> >>>>>>>>>> But there is some flash vendor specific code like quad enable >>>>>> >>>>>>>>>> bit, >>>>>> >>>>>>>>>> locking ops and finally about spi_flash_params table. >>>>>> >>>>>>>>>> >>>>>> >>>>>>>>> >>>>>> >>>>>>>>> I know. That's probably why adding all these SPI flash drivers >>>>>> >>>>>>>>> don't >>>>>> >>>>>>>>> help at all because only one code path will take effect. And >>>>>> >>>>>>>>> what I >>>>>> >>>>>>>>> did in this patch is to select one type of flash per board. >>>>>> >>>>>>>> >>>>>> >>>>>>>> >>>>>> >>>>>>>> So how about we group together 3-4 of the common ones, with no >>>>>> >>>>>>>> special >>>>>> >>>>>>>> features, into a 'CONFIG_SPI_FLASH_GENERIC'? >>>>>> >>>>>>>> >>>>>> >>>>>>> >>>>>> >>>>>>> Can you comment on this CONFIG_SPI_FLASH_GENERIC as Simon >>>>>> >>>>>>> suggested? >>>>>> >>>>>> >>>>>> >>>>>> >>>>>> >>>>>> Good idea, but if we don't find enough foot-print difference on no >>>>>> >>>>>> feature flags may be we can remove those config items and I have a >>>>>> >>>>>> plan to re-arrange the sf_param_table which suits Linux may be I >>>>>> >>>>>> will >>>>>> >>>>>> come back about these things. >>>>>> >>>>>> >>>>>> >>>>> >>>>>> >>>>> Can you please suggest which way should we go for this patch? I >>>>>> >>>>> still >>>>>> >>>>> prefer one board with one SPI flash macro. >>>>>> >>>> >>>>>> >>>> >>>>>> >>>> Sorry, I didn't get you what do you mean by one board with one SPI >>>>>> >>>> flash macro? Suppose if board have one controller connected with >>>>>> >>>> micro >>>>>> >>>> flash then the board file include CONFIG_SPI_FLASH_STMICRO and if >>>>>> >>>> another board having two controllers one connected with spansion and >>>>>> >>>> other connected with micro then the board file include >>>>>> >>>> CONFIG_SPI_FLASH_STMICRO, CONFIG_SPI_FLASH_SPANSION. It's entirely up >>>>>> >>>> to board that connected flash devices. >>>>>> >>>> >>>>>> >>> >>>>>> >>> Yes, your understanding is the same as mine. I wasn't clear in my >>>>>> >>> previous question. >>>>>> >>> >>>>>> >>> Right now this patch is doing exactly as what you and I understand, >>>>>> >>> that we just want to select the specific flash macro for a specific >>>>>> >>> x86 board. But Simon wanted to enable all of the flash macros for one >>>>>> >>> board for convenience. Thus I came to ask for what's our direction. >>>>>> >> >>>>>> >> >>>>>> >> So, does this board supports or connected all flash variants? in that >>>>>> >> case >>>>>> >> it is true right? "for convenience" here means for testing then ie also >>>>>> >> true >>>>>> >> because this particular board is meant for testing all flash devices, >>>>>> >> and >>>>>> >> also it is up to this particular board config over-head rather than the >>>>>> >> generic spi-flash. >>>>>> >> >>>>>> > >>>>>> > No, each board only connects one specific type of SPI flash, as >>>>>> > described in the board device tree file. >>>>>> >>>>>> So your patch did that change? let me look at it what Simon commenting. >>>>>> >>>>> >>>>> I don't see any further comments on this patch. Can we close this? >>>>> >>>> >>>> A gentle ping .. I still would like to clean these up unless there is >>>> something changes in the SF (as Simon proposed) to be planned. >>> >>> Except - macronix, spansion, stmicro, sst, winbond configs we can >>> remove remaining flash vendor configs for now, because later configs >>> using common code. >> >> Does this mean the CONFIG_SPI_FLASH_GENERIC idea is good, or not? > > If foot-print is not so huge with those configs I think it's better > not to add an extra config - what do you think? CONFIG_SPI_FLASH_GENERIC could cover the common cases and it should just be a small amount of entries in the SPI flash table listing supported chips. Regards, Simon
Hi Jagan, On 12 February 2016 at 09:31, Jagan Teki <jteki@openedev.com> wrote: > Hi Simon, > > On 12 February 2016 at 21:24, Simon Glass <sjg@chromium.org> wrote: >> Hi, >> >> On 6 February 2016 at 02:57, Jagan Teki <jteki@openedev.com> wrote: >>> Hi Bin, >>> >>> On 6 February 2016 at 09:46, Bin Meng <bmeng.cn@gmail.com> wrote: >>>> Hi Jagan, Simon, >>>> >>>> On Thu, Jan 21, 2016 at 1:53 PM, Bin Meng <bmeng.cn@gmail.com> wrote: >>>>> Hi, >>>>> >>>>> On Tue, Dec 15, 2015 at 2:27 PM, Jagan Teki <jteki@openedev.com> wrote: >>>>>> >>>>>> On 15 December 2015 at 11:48, Bin Meng <bmeng.cn@gmail.com> wrote: >>>>>> > Hi Jagan, >>>>>> > >>>>>> > On Tue, Dec 15, 2015 at 2:15 PM, Jagan Teki <jteki@openedev.com> wrote: >>>>>> >> Hi Bin, >>>>>> >> >>>>>> >> >>>>>> >> On Tuesday 15 December 2015 11:37 AM, Bin Meng wrote: >>>>>> >>> >>>>>> >>> Hi Jagan, >>>>>> >>> >>>>>> >>> On Tue, Dec 15, 2015 at 1:51 PM, Jagan Teki <jteki@openedev.com> >>>>>> >>> wrote: >>>>>> >>>> >>>>>> >>>> Hi Bin, >>>>>> >>>> >>>>>> >>>> On 15 December 2015 at 10:48, Bin Meng <bmeng.cn@gmail.com> wrote: >>>>>> >>>>> >>>>>> >>>>> Hi Jagan, Simon, >>>>>> >>>>> >>>>>> >>>>> On Tue, Dec 8, 2015 at 11:44 PM, Jagan Teki <jteki@openedev.com > >>>>>> >>>>> wrote: >>>>>> >>>>>> >>>>>> >>>>>> On 8 December 2015 at 17:27, Bin Meng <bmeng.cn@gmail.com> wrote: >>>>>> >>>>>>> >>>>>> >>>>>>> Hi Jagan, >>>>>> >>>>>>> >>>>>> >>>>>>> On Fri, Dec 4, 2015 at 2:57 AM, Simon Glass <sjg@chromium.org > >>>>>> >>>>>>> wrote: >>>>>> >>>>>>>> >>>>>> >>>>>>>> Hi, >>>>>> >>>>>>>> >>>>>> >>>>>>>> On 3 December 2015 at 06:27, Bin Meng <bmeng.cn@gmail.com> wrote: >>>>>> >>>>>>>>> >>>>>> >>>>>>>>> Hi Jagan, >>>>>> >>>>>>>>> >>>>>> >>>>>>>>> On Thu, Dec 3, 2015 at 6:24 PM, Jagan Teki < jteki@openedev.com> >>>>>> >>>>>>>>> wrote: >>>>>> >>>>>>>>>> >>>>>> >>>>>>>>>> Hi Bin, >>>>>> >>>>>>>>>> >>>>>> >>>>>>>>>> On 3 December 2015 at 10:14, Bin Meng <bmeng.cn@gmail.com> >>>>>> >>>>>>>>>> wrote: >>>>>> >>>>>>>>>>> >>>>>> >>>>>>>>>>> Hi Simon, >>>>>> >>>>>>>>>>> >>>>>> >>>>>>>>>>> On Thu, Dec 3, 2015 at 5:05 AM, Simon Glass < sjg@chromium.org> >>>>>> >>>>>>>>>>> wrote: >>>>>> >>>>>>>>>>>> >>>>>> >>>>>>>>>>>> +Jagan >>>>>> >>>>>>>>>>>> >>>>>> >>>>>>>>>>>> Hi Bin, >>>>>> >>>>>>>>>>>> >>>>>> >>>>>>>>>>>> On 1 December 2015 at 18:41, Bin Meng < bmeng.cn@gmail.com> >>>>>> >>>>>>>>>>>> wrote: >>>>>> >>>>>>>>>>>>> >>>>>> >>>>>>>>>>>>> >>>>>> >>>>>>>>>>>>> Hi Simon, >>>>>> >>>>>>>>>>>>> >>>>>> >>>>>>>>>>>>> On Wed, Dec 2, 2015 at 12:32 AM, Simon Glass >>>>>> >>>>>>>>>>>>> <sjg@chromium.org> >>>>>> >>>>>>>>>>>>> wrote: >>>>>> >>>>>>>>>>>>>> >>>>>> >>>>>>>>>>>>>> Hi Bin, >>>>>> >>>>>>>>>>>>>> >>>>>> >>>>>>>>>>>>>> On 28 November 2015 at 05:45, Bin Meng < bmeng.cn@gmail.com> >>>>>> >>>>>>>>>>>>>> wrote: >>>>>> >>>>>>>>>>>>>>> >>>>>> >>>>>>>>>>>>>>> Every board has one dedicated type of SPI flash, hence it >>>>>> >>>>>>>>>>>>>>> is >>>>>> >>>>>>>>>>>>>>> unnecessary to include multiple SPI flash drivers. >>>>>> >>>>>>>>>>>>>>> >>>>>> >>>>>>>>>>>>>>> For QEMU and coreboot (default build of coreboot is also >>>>>> >>>>>>>>>>>>>>> QEMU), >>>>>> >>>>>>>>>>>>>>> SPI flash is not supported. Remove those SPI flash >>>>>> >>>>>>>>>>>>>>> drivers. >>>>>> >>>>>>>>>>>>>>> >>>>>> >>>>>>>>>>>>>>> Signed-off-by: Bin Meng <bmeng.cn@gmail.com> >>>>>> >>>>>>>>>>>>>>> --- >>>>>> >>>>>>>>>>>>>>> >>>>>> >>>>>>>>>>>>>>> configs/bayleybay_defconfig | 2 -- >>>>>> >>>>>>>>>>>>>>> configs/chromebook_link_defconfig | 2 -- >>>>>> >>>>>>>>>>>>>>> configs/chromebox_panther_defconfig | 2 -- >>>>>> >>>>>>>>>>>>>>> configs/coreboot-x86_defconfig | 4 ---- >>>>>> >>>>>>>>>>>>>>> configs/crownbay_defconfig | 3 --- >>>>>> >>>>>>>>>>>>>>> configs/galileo_defconfig | 2 -- >>>>>> >>>>>>>>>>>>>>> configs/minnowmax_defconfig | 3 --- >>>>>> >>>>>>>>>>>>>>> configs/qemu-x86_defconfig | 4 ---- >>>>>> >>>>>>>>>>>>>>> 8 files changed, 22 deletions(-) >>>>>> >>>>>>>>>>>>>> >>>>>> >>>>>>>>>>>>>> >>>>>> >>>>>>>>>>>>>> What is the benefit of this? I see it removes a few lines >>>>>> >>>>>>>>>>>>>> in a >>>>>> >>>>>>>>>>>>>> data >>>>>> >>>>>>>>>>>>>> table. Does it matter? >>>>>> >>>>>>>>>>>>> >>>>>> >>>>>>>>>>>>> >>>>>> >>>>>>>>>>>>> Maybe we should ask the other way around, why do we create >>>>>> >>>>>>>>>>>>> so >>>>>> >>>>>>>>>>>>> many >>>>>> >>>>>>>>>>>>> flash driver Kconfig option? I believe the intention was >>>>>> >>>>>>>>>>>>> footprint. >>>>>> >>>>>>>>>>>>> Besides the footprint issue, having just one flash driver in >>>>>> >>>>>>>>>>>>> each >>>>>> >>>>>>>>>>>>> board makes it very clear instead of causing confusion. >>>>>> >>>>>>>>>>>>> Looks >>>>>> >>>>>>>>>>>>> other >>>>>> >>>>>>>>>>>>> board defconfig files only select one. >>>>>> >>>>>>>>>> >>>>>> >>>>>>>>>> >>>>>> >>>>>>>>>> Are you talking about flash vendor config or CONFIG_SPI_FLASH? >>>>>> >>>>>>>>>> >>>>>> >>>>>>>>> >>>>>> >>>>>>>>> Flash vendor config, as you see in this patch. >>>>>> >>>>>>>>> >>>>>> >>>>>>>>>>>> >>>>>> >>>>>>>>>>>> They are a hangover from when we had a separate driver for >>>>>> >>>>>>>>>>>> each >>>>>> >>>>>>>>>>>> one. >>>>>> >>>>>>>>>>>> Jagan put a lot of effort into removing all the >>>>>> >>>>>>>>>>>> semi-duplicated >>>>>> >>>>>>>>>>>> code. >>>>>> >>>>>>>>>>>> >>>>>> >>>>>>>>>>>> Maybe we should prune down these options? >>>>>> >>>>>>>>>>>> >>>>>> >>>>>>>>>>> >>>>>> >>>>>>>>>>> But if we already spent a lot of effort into removing all the >>>>>> >>>>>>>>>>> semi-duplicated code, we should not have converted those flash >>>>>> >>>>>>>>>>> driver >>>>>> >>>>>>>>>>> to Kconfig options before. >>>>>> >>>>>>>>>>> >>>>>> >>>>>>>>>>> See commit d5af92315bb48740f16bf8817f38e227d3076905 "sf: >>>>>> >>>>>>>>>>> kconfig: >>>>>> >>>>>>>>>>> add >>>>>> >>>>>>>>>>> kconfig options for spi flashes" >>>>>> >>>>>>>>>>> >>>>>> >>>>>>>>>>> I suspect we may remove most of these SPI flash macros, but at >>>>>> >>>>>>>>>>> least >>>>>> >>>>>>>>>>> SST flash macro should be kept since right now it is mixed in >>>>>> >>>>>>>>>>> the >>>>>> >>>>>>>>>>> generic driver with a special byte program and word program >>>>>> >>>>>>>>>>> which >>>>>> >>>>>>>>>>> is >>>>>> >>>>>>>>>>> incompatible with other vendors' flashes. >>>>>> >>>>>>>>>> >>>>>> >>>>>>>>>> >>>>>> >>>>>>>>>> But there is some flash vendor specific code like quad enable >>>>>> >>>>>>>>>> bit, >>>>>> >>>>>>>>>> locking ops and finally about spi_flash_params table. >>>>>> >>>>>>>>>> >>>>>> >>>>>>>>> >>>>>> >>>>>>>>> I know. That's probably why adding all these SPI flash drivers >>>>>> >>>>>>>>> don't >>>>>> >>>>>>>>> help at all because only one code path will take effect. And >>>>>> >>>>>>>>> what I >>>>>> >>>>>>>>> did in this patch is to select one type of flash per board. >>>>>> >>>>>>>> >>>>>> >>>>>>>> >>>>>> >>>>>>>> So how about we group together 3-4 of the common ones, with no >>>>>> >>>>>>>> special >>>>>> >>>>>>>> features, into a 'CONFIG_SPI_FLASH_GENERIC'? >>>>>> >>>>>>>> >>>>>> >>>>>>> >>>>>> >>>>>>> Can you comment on this CONFIG_SPI_FLASH_GENERIC as Simon >>>>>> >>>>>>> suggested? >>>>>> >>>>>> >>>>>> >>>>>> >>>>>> >>>>>> Good idea, but if we don't find enough foot-print difference on no >>>>>> >>>>>> feature flags may be we can remove those config items and I have a >>>>>> >>>>>> plan to re-arrange the sf_param_table which suits Linux may be I >>>>>> >>>>>> will >>>>>> >>>>>> come back about these things. >>>>>> >>>>>> >>>>>> >>>>> >>>>>> >>>>> Can you please suggest which way should we go for this patch? I >>>>>> >>>>> still >>>>>> >>>>> prefer one board with one SPI flash macro. >>>>>> >>>> >>>>>> >>>> >>>>>> >>>> Sorry, I didn't get you what do you mean by one board with one SPI >>>>>> >>>> flash macro? Suppose if board have one controller connected with >>>>>> >>>> micro >>>>>> >>>> flash then the board file include CONFIG_SPI_FLASH_STMICRO and if >>>>>> >>>> another board having two controllers one connected with spansion and >>>>>> >>>> other connected with micro then the board file include >>>>>> >>>> CONFIG_SPI_FLASH_STMICRO, CONFIG_SPI_FLASH_SPANSION. It's entirely up >>>>>> >>>> to board that connected flash devices. >>>>>> >>>> >>>>>> >>> >>>>>> >>> Yes, your understanding is the same as mine. I wasn't clear in my >>>>>> >>> previous question. >>>>>> >>> >>>>>> >>> Right now this patch is doing exactly as what you and I understand, >>>>>> >>> that we just want to select the specific flash macro for a specific >>>>>> >>> x86 board. But Simon wanted to enable all of the flash macros for one >>>>>> >>> board for convenience. Thus I came to ask for what's our direction. >>>>>> >> >>>>>> >> >>>>>> >> So, does this board supports or connected all flash variants? in that >>>>>> >> case >>>>>> >> it is true right? "for convenience" here means for testing then ie also >>>>>> >> true >>>>>> >> because this particular board is meant for testing all flash devices, >>>>>> >> and >>>>>> >> also it is up to this particular board config over-head rather than the >>>>>> >> generic spi-flash. >>>>>> >> >>>>>> > >>>>>> > No, each board only connects one specific type of SPI flash, as >>>>>> > described in the board device tree file. >>>>>> >>>>>> So your patch did that change? let me look at it what Simon commenting. >>>>>> >>>>> >>>>> I don't see any further comments on this patch. Can we close this? >>>>> >>>> >>>> A gentle ping .. I still would like to clean these up unless there is >>>> something changes in the SF (as Simon proposed) to be planned. >>> >>> Except - macronix, spansion, stmicro, sst, winbond configs we can >>> remove remaining flash vendor configs for now, because later configs >>> using common code. >> >> Does this mean the CONFIG_SPI_FLASH_GENERIC idea is good, or not? > > If foot-print is not so huge with those configs I think it's better > not to add an extra config - what do you think? CONFIG_SPI_FLASH_GENERIC could cover the common cases and it should just be a small amount of entries in the SPI flash table listing supported chips. Regards, Simon
Hi Simon, On 13 February 2016 at 00:58, Simon Glass <sjg@chromium.org> wrote: > Hi Jagan, > > On 12 February 2016 at 09:31, Jagan Teki <jteki@openedev.com> wrote: >> Hi Simon, >> >> On 12 February 2016 at 21:24, Simon Glass <sjg@chromium.org> wrote: >>> Hi, >>> >>> On 6 February 2016 at 02:57, Jagan Teki <jteki@openedev.com> wrote: >>>> Hi Bin, >>>> >>>> On 6 February 2016 at 09:46, Bin Meng <bmeng.cn@gmail.com> wrote: >>>>> Hi Jagan, Simon, >>>>> >>>>> On Thu, Jan 21, 2016 at 1:53 PM, Bin Meng <bmeng.cn@gmail.com> wrote: >>>>>> Hi, >>>>>> >>>>>> On Tue, Dec 15, 2015 at 2:27 PM, Jagan Teki <jteki@openedev.com> wrote: >>>>>>> >>>>>>> On 15 December 2015 at 11:48, Bin Meng <bmeng.cn@gmail.com> wrote: >>>>>>> > Hi Jagan, >>>>>>> > >>>>>>> > On Tue, Dec 15, 2015 at 2:15 PM, Jagan Teki <jteki@openedev.com> wrote: >>>>>>> >> Hi Bin, >>>>>>> >> >>>>>>> >> >>>>>>> >> On Tuesday 15 December 2015 11:37 AM, Bin Meng wrote: >>>>>>> >>> >>>>>>> >>> Hi Jagan, >>>>>>> >>> >>>>>>> >>> On Tue, Dec 15, 2015 at 1:51 PM, Jagan Teki <jteki@openedev.com> >>>>>>> >>> wrote: >>>>>>> >>>> >>>>>>> >>>> Hi Bin, >>>>>>> >>>> >>>>>>> >>>> On 15 December 2015 at 10:48, Bin Meng <bmeng.cn@gmail.com> wrote: >>>>>>> >>>>> >>>>>>> >>>>> Hi Jagan, Simon, >>>>>>> >>>>> >>>>>>> >>>>> On Tue, Dec 8, 2015 at 11:44 PM, Jagan Teki <jteki@openedev.com> >>>>>>> >>>>> wrote: >>>>>>> >>>>>> >>>>>>> >>>>>> On 8 December 2015 at 17:27, Bin Meng <bmeng.cn@gmail.com> wrote: >>>>>>> >>>>>>> >>>>>>> >>>>>>> Hi Jagan, >>>>>>> >>>>>>> >>>>>>> >>>>>>> On Fri, Dec 4, 2015 at 2:57 AM, Simon Glass <sjg@chromium.org> >>>>>>> >>>>>>> wrote: >>>>>>> >>>>>>>> >>>>>>> >>>>>>>> Hi, >>>>>>> >>>>>>>> >>>>>>> >>>>>>>> On 3 December 2015 at 06:27, Bin Meng <bmeng.cn@gmail.com> wrote: >>>>>>> >>>>>>>>> >>>>>>> >>>>>>>>> Hi Jagan, >>>>>>> >>>>>>>>> >>>>>>> >>>>>>>>> On Thu, Dec 3, 2015 at 6:24 PM, Jagan Teki <jteki@openedev.com> >>>>>>> >>>>>>>>> wrote: >>>>>>> >>>>>>>>>> >>>>>>> >>>>>>>>>> Hi Bin, >>>>>>> >>>>>>>>>> >>>>>>> >>>>>>>>>> On 3 December 2015 at 10:14, Bin Meng <bmeng.cn@gmail.com> >>>>>>> >>>>>>>>>> wrote: >>>>>>> >>>>>>>>>>> >>>>>>> >>>>>>>>>>> Hi Simon, >>>>>>> >>>>>>>>>>> >>>>>>> >>>>>>>>>>> On Thu, Dec 3, 2015 at 5:05 AM, Simon Glass <sjg@chromium.org> >>>>>>> >>>>>>>>>>> wrote: >>>>>>> >>>>>>>>>>>> >>>>>>> >>>>>>>>>>>> +Jagan >>>>>>> >>>>>>>>>>>> >>>>>>> >>>>>>>>>>>> Hi Bin, >>>>>>> >>>>>>>>>>>> >>>>>>> >>>>>>>>>>>> On 1 December 2015 at 18:41, Bin Meng <bmeng.cn@gmail.com> >>>>>>> >>>>>>>>>>>> wrote: >>>>>>> >>>>>>>>>>>>> >>>>>>> >>>>>>>>>>>>> >>>>>>> >>>>>>>>>>>>> Hi Simon, >>>>>>> >>>>>>>>>>>>> >>>>>>> >>>>>>>>>>>>> On Wed, Dec 2, 2015 at 12:32 AM, Simon Glass >>>>>>> >>>>>>>>>>>>> <sjg@chromium.org> >>>>>>> >>>>>>>>>>>>> wrote: >>>>>>> >>>>>>>>>>>>>> >>>>>>> >>>>>>>>>>>>>> Hi Bin, >>>>>>> >>>>>>>>>>>>>> >>>>>>> >>>>>>>>>>>>>> On 28 November 2015 at 05:45, Bin Meng <bmeng.cn@gmail.com> >>>>>>> >>>>>>>>>>>>>> wrote: >>>>>>> >>>>>>>>>>>>>>> >>>>>>> >>>>>>>>>>>>>>> Every board has one dedicated type of SPI flash, hence it >>>>>>> >>>>>>>>>>>>>>> is >>>>>>> >>>>>>>>>>>>>>> unnecessary to include multiple SPI flash drivers. >>>>>>> >>>>>>>>>>>>>>> >>>>>>> >>>>>>>>>>>>>>> For QEMU and coreboot (default build of coreboot is also >>>>>>> >>>>>>>>>>>>>>> QEMU), >>>>>>> >>>>>>>>>>>>>>> SPI flash is not supported. Remove those SPI flash >>>>>>> >>>>>>>>>>>>>>> drivers. >>>>>>> >>>>>>>>>>>>>>> >>>>>>> >>>>>>>>>>>>>>> Signed-off-by: Bin Meng <bmeng.cn@gmail.com> >>>>>>> >>>>>>>>>>>>>>> --- >>>>>>> >>>>>>>>>>>>>>> >>>>>>> >>>>>>>>>>>>>>> configs/bayleybay_defconfig | 2 -- >>>>>>> >>>>>>>>>>>>>>> configs/chromebook_link_defconfig | 2 -- >>>>>>> >>>>>>>>>>>>>>> configs/chromebox_panther_defconfig | 2 -- >>>>>>> >>>>>>>>>>>>>>> configs/coreboot-x86_defconfig | 4 ---- >>>>>>> >>>>>>>>>>>>>>> configs/crownbay_defconfig | 3 --- >>>>>>> >>>>>>>>>>>>>>> configs/galileo_defconfig | 2 -- >>>>>>> >>>>>>>>>>>>>>> configs/minnowmax_defconfig | 3 --- >>>>>>> >>>>>>>>>>>>>>> configs/qemu-x86_defconfig | 4 ---- >>>>>>> >>>>>>>>>>>>>>> 8 files changed, 22 deletions(-) >>>>>>> >>>>>>>>>>>>>> >>>>>>> >>>>>>>>>>>>>> >>>>>>> >>>>>>>>>>>>>> What is the benefit of this? I see it removes a few lines >>>>>>> >>>>>>>>>>>>>> in a >>>>>>> >>>>>>>>>>>>>> data >>>>>>> >>>>>>>>>>>>>> table. Does it matter? >>>>>>> >>>>>>>>>>>>> >>>>>>> >>>>>>>>>>>>> >>>>>>> >>>>>>>>>>>>> Maybe we should ask the other way around, why do we create >>>>>>> >>>>>>>>>>>>> so >>>>>>> >>>>>>>>>>>>> many >>>>>>> >>>>>>>>>>>>> flash driver Kconfig option? I believe the intention was >>>>>>> >>>>>>>>>>>>> footprint. >>>>>>> >>>>>>>>>>>>> Besides the footprint issue, having just one flash driver in >>>>>>> >>>>>>>>>>>>> each >>>>>>> >>>>>>>>>>>>> board makes it very clear instead of causing confusion. >>>>>>> >>>>>>>>>>>>> Looks >>>>>>> >>>>>>>>>>>>> other >>>>>>> >>>>>>>>>>>>> board defconfig files only select one. >>>>>>> >>>>>>>>>> >>>>>>> >>>>>>>>>> >>>>>>> >>>>>>>>>> Are you talking about flash vendor config or CONFIG_SPI_FLASH? >>>>>>> >>>>>>>>>> >>>>>>> >>>>>>>>> >>>>>>> >>>>>>>>> Flash vendor config, as you see in this patch. >>>>>>> >>>>>>>>> >>>>>>> >>>>>>>>>>>> >>>>>>> >>>>>>>>>>>> They are a hangover from when we had a separate driver for >>>>>>> >>>>>>>>>>>> each >>>>>>> >>>>>>>>>>>> one. >>>>>>> >>>>>>>>>>>> Jagan put a lot of effort into removing all the >>>>>>> >>>>>>>>>>>> semi-duplicated >>>>>>> >>>>>>>>>>>> code. >>>>>>> >>>>>>>>>>>> >>>>>>> >>>>>>>>>>>> Maybe we should prune down these options? >>>>>>> >>>>>>>>>>>> >>>>>>> >>>>>>>>>>> >>>>>>> >>>>>>>>>>> But if we already spent a lot of effort into removing all the >>>>>>> >>>>>>>>>>> semi-duplicated code, we should not have converted those flash >>>>>>> >>>>>>>>>>> driver >>>>>>> >>>>>>>>>>> to Kconfig options before. >>>>>>> >>>>>>>>>>> >>>>>>> >>>>>>>>>>> See commit d5af92315bb48740f16bf8817f38e227d3076905 "sf: >>>>>>> >>>>>>>>>>> kconfig: >>>>>>> >>>>>>>>>>> add >>>>>>> >>>>>>>>>>> kconfig options for spi flashes" >>>>>>> >>>>>>>>>>> >>>>>>> >>>>>>>>>>> I suspect we may remove most of these SPI flash macros, but at >>>>>>> >>>>>>>>>>> least >>>>>>> >>>>>>>>>>> SST flash macro should be kept since right now it is mixed in >>>>>>> >>>>>>>>>>> the >>>>>>> >>>>>>>>>>> generic driver with a special byte program and word program >>>>>>> >>>>>>>>>>> which >>>>>>> >>>>>>>>>>> is >>>>>>> >>>>>>>>>>> incompatible with other vendors' flashes. >>>>>>> >>>>>>>>>> >>>>>>> >>>>>>>>>> >>>>>>> >>>>>>>>>> But there is some flash vendor specific code like quad enable >>>>>>> >>>>>>>>>> bit, >>>>>>> >>>>>>>>>> locking ops and finally about spi_flash_params table. >>>>>>> >>>>>>>>>> >>>>>>> >>>>>>>>> >>>>>>> >>>>>>>>> I know. That's probably why adding all these SPI flash drivers >>>>>>> >>>>>>>>> don't >>>>>>> >>>>>>>>> help at all because only one code path will take effect. And >>>>>>> >>>>>>>>> what I >>>>>>> >>>>>>>>> did in this patch is to select one type of flash per board. >>>>>>> >>>>>>>> >>>>>>> >>>>>>>> >>>>>>> >>>>>>>> So how about we group together 3-4 of the common ones, with no >>>>>>> >>>>>>>> special >>>>>>> >>>>>>>> features, into a 'CONFIG_SPI_FLASH_GENERIC'? >>>>>>> >>>>>>>> >>>>>>> >>>>>>> >>>>>>> >>>>>>> Can you comment on this CONFIG_SPI_FLASH_GENERIC as Simon >>>>>>> >>>>>>> suggested? >>>>>>> >>>>>> >>>>>>> >>>>>> >>>>>>> >>>>>> Good idea, but if we don't find enough foot-print difference on no >>>>>>> >>>>>> feature flags may be we can remove those config items and I have a >>>>>>> >>>>>> plan to re-arrange the sf_param_table which suits Linux may be I >>>>>>> >>>>>> will >>>>>>> >>>>>> come back about these things. >>>>>>> >>>>>> >>>>>>> >>>>> >>>>>>> >>>>> Can you please suggest which way should we go for this patch? I >>>>>>> >>>>> still >>>>>>> >>>>> prefer one board with one SPI flash macro. >>>>>>> >>>> >>>>>>> >>>> >>>>>>> >>>> Sorry, I didn't get you what do you mean by one board with one SPI >>>>>>> >>>> flash macro? Suppose if board have one controller connected with >>>>>>> >>>> micro >>>>>>> >>>> flash then the board file include CONFIG_SPI_FLASH_STMICRO and if >>>>>>> >>>> another board having two controllers one connected with spansion and >>>>>>> >>>> other connected with micro then the board file include >>>>>>> >>>> CONFIG_SPI_FLASH_STMICRO, CONFIG_SPI_FLASH_SPANSION. It's entirely up >>>>>>> >>>> to board that connected flash devices. >>>>>>> >>>> >>>>>>> >>> >>>>>>> >>> Yes, your understanding is the same as mine. I wasn't clear in my >>>>>>> >>> previous question. >>>>>>> >>> >>>>>>> >>> Right now this patch is doing exactly as what you and I understand, >>>>>>> >>> that we just want to select the specific flash macro for a specific >>>>>>> >>> x86 board. But Simon wanted to enable all of the flash macros for one >>>>>>> >>> board for convenience. Thus I came to ask for what's our direction. >>>>>>> >> >>>>>>> >> >>>>>>> >> So, does this board supports or connected all flash variants? in that >>>>>>> >> case >>>>>>> >> it is true right? "for convenience" here means for testing then ie also >>>>>>> >> true >>>>>>> >> because this particular board is meant for testing all flash devices, >>>>>>> >> and >>>>>>> >> also it is up to this particular board config over-head rather than the >>>>>>> >> generic spi-flash. >>>>>>> >> >>>>>>> > >>>>>>> > No, each board only connects one specific type of SPI flash, as >>>>>>> > described in the board device tree file. >>>>>>> >>>>>>> So your patch did that change? let me look at it what Simon commenting. >>>>>>> >>>>>> >>>>>> I don't see any further comments on this patch. Can we close this? >>>>>> >>>>> >>>>> A gentle ping .. I still would like to clean these up unless there is >>>>> something changes in the SF (as Simon proposed) to be planned. >>>> >>>> Except - macronix, spansion, stmicro, sst, winbond configs we can >>>> remove remaining flash vendor configs for now, because later configs >>>> using common code. >>> >>> Does this mean the CONFIG_SPI_FLASH_GENERIC idea is good, or not? >> >> If foot-print is not so huge with those configs I think it's better >> not to add an extra config - what do you think? > > CONFIG_SPI_FLASH_GENERIC could cover the common cases and it should > just be a small amount of entries in the SPI flash table listing > supported chips. OK, then will do this change on top of spi-nor. thanks!
diff --git a/configs/bayleybay_defconfig b/configs/bayleybay_defconfig index 0a5a56f..c1ed990 100644 --- a/configs/bayleybay_defconfig +++ b/configs/bayleybay_defconfig @@ -21,8 +21,6 @@ CONFIG_CMD_BOOTSTAGE=y CONFIG_OF_CONTROL=y CONFIG_CPU=y CONFIG_SPI_FLASH=y -CONFIG_SPI_FLASH_GIGADEVICE=y -CONFIG_SPI_FLASH_MACRONIX=y CONFIG_SPI_FLASH_WINBOND=y CONFIG_DM_ETH=y CONFIG_E1000=y diff --git a/configs/chromebook_link_defconfig b/configs/chromebook_link_defconfig index ac64877..26b1d13 100644 --- a/configs/chromebook_link_defconfig +++ b/configs/chromebook_link_defconfig @@ -21,8 +21,6 @@ CONFIG_CMD_CROS_EC=y CONFIG_CROS_EC=y CONFIG_CROS_EC_LPC=y CONFIG_SPI_FLASH=y -CONFIG_SPI_FLASH_GIGADEVICE=y -CONFIG_SPI_FLASH_MACRONIX=y CONFIG_SPI_FLASH_WINBOND=y CONFIG_DM_PCI=y CONFIG_DM_RTC=y diff --git a/configs/chromebox_panther_defconfig b/configs/chromebox_panther_defconfig index 0f3a9af..7fb25b4 100644 --- a/configs/chromebox_panther_defconfig +++ b/configs/chromebox_panther_defconfig @@ -20,8 +20,6 @@ CONFIG_CMD_CROS_EC=y CONFIG_CROS_EC=y CONFIG_CROS_EC_LPC=y CONFIG_SPI_FLASH=y -CONFIG_SPI_FLASH_GIGADEVICE=y -CONFIG_SPI_FLASH_MACRONIX=y CONFIG_SPI_FLASH_WINBOND=y CONFIG_DM_PCI=y CONFIG_DM_RTC=y diff --git a/configs/coreboot-x86_defconfig b/configs/coreboot-x86_defconfig index 8903cdd..96b6d0d 100644 --- a/configs/coreboot-x86_defconfig +++ b/configs/coreboot-x86_defconfig @@ -12,10 +12,6 @@ CONFIG_CMD_BOOTSTAGE=y CONFIG_CMD_TPM=y CONFIG_CMD_TPM_TEST=y CONFIG_OF_CONTROL=y -CONFIG_SPI_FLASH=y -CONFIG_SPI_FLASH_GIGADEVICE=y -CONFIG_SPI_FLASH_MACRONIX=y -CONFIG_SPI_FLASH_WINBOND=y CONFIG_DM_ETH=y CONFIG_E1000=y CONFIG_DM_PCI=y diff --git a/configs/crownbay_defconfig b/configs/crownbay_defconfig index f4592c5..c96e800 100644 --- a/configs/crownbay_defconfig +++ b/configs/crownbay_defconfig @@ -19,10 +19,7 @@ CONFIG_CMD_BOOTSTAGE=y CONFIG_OF_CONTROL=y CONFIG_CPU=y CONFIG_SPI_FLASH=y -CONFIG_SPI_FLASH_GIGADEVICE=y -CONFIG_SPI_FLASH_MACRONIX=y CONFIG_SPI_FLASH_SST=y -CONFIG_SPI_FLASH_WINBOND=y CONFIG_DM_ETH=y CONFIG_E1000=y CONFIG_PCH_GBE=y diff --git a/configs/galileo_defconfig b/configs/galileo_defconfig index 3612350..0dfc5f2 100644 --- a/configs/galileo_defconfig +++ b/configs/galileo_defconfig @@ -15,8 +15,6 @@ CONFIG_CMD_BOOTSTAGE=y CONFIG_OF_CONTROL=y CONFIG_NET_RANDOM_ETHADDR=y CONFIG_SPI_FLASH=y -CONFIG_SPI_FLASH_GIGADEVICE=y -CONFIG_SPI_FLASH_MACRONIX=y CONFIG_SPI_FLASH_WINBOND=y CONFIG_DM_ETH=y CONFIG_ETH_DESIGNWARE=y diff --git a/configs/minnowmax_defconfig b/configs/minnowmax_defconfig index 37c07c1..44fb426 100644 --- a/configs/minnowmax_defconfig +++ b/configs/minnowmax_defconfig @@ -20,10 +20,7 @@ CONFIG_CMD_BOOTSTAGE=y CONFIG_OF_CONTROL=y CONFIG_CPU=y CONFIG_SPI_FLASH=y -CONFIG_SPI_FLASH_GIGADEVICE=y -CONFIG_SPI_FLASH_MACRONIX=y CONFIG_SPI_FLASH_STMICRO=y -CONFIG_SPI_FLASH_WINBOND=y CONFIG_DM_ETH=y CONFIG_DM_PCI=y CONFIG_DM_RTC=y diff --git a/configs/qemu-x86_defconfig b/configs/qemu-x86_defconfig index ebdb892..6cd6bfb 100644 --- a/configs/qemu-x86_defconfig +++ b/configs/qemu-x86_defconfig @@ -14,10 +14,6 @@ CONFIG_BOOTSTAGE_REPORT=y CONFIG_CMD_BOOTSTAGE=y CONFIG_OF_CONTROL=y CONFIG_CPU=y -CONFIG_SPI_FLASH=y -CONFIG_SPI_FLASH_GIGADEVICE=y -CONFIG_SPI_FLASH_MACRONIX=y -CONFIG_SPI_FLASH_WINBOND=y CONFIG_DM_ETH=y CONFIG_E1000=y CONFIG_DM_PCI=y
Every board has one dedicated type of SPI flash, hence it is unnecessary to include multiple SPI flash drivers. For QEMU and coreboot (default build of coreboot is also QEMU), SPI flash is not supported. Remove those SPI flash drivers. Signed-off-by: Bin Meng <bmeng.cn@gmail.com> --- configs/bayleybay_defconfig | 2 -- configs/chromebook_link_defconfig | 2 -- configs/chromebox_panther_defconfig | 2 -- configs/coreboot-x86_defconfig | 4 ---- configs/crownbay_defconfig | 3 --- configs/galileo_defconfig | 2 -- configs/minnowmax_defconfig | 3 --- configs/qemu-x86_defconfig | 4 ---- 8 files changed, 22 deletions(-)