Hi Jagan, On Tue, Dec 15, 2015 at 1:51 PM, Jagan Teki <jt...@openedev.com> wrote: > Hi Bin, > > On 15 December 2015 at 10:48, Bin Meng <bmeng...@gmail.com> wrote: >> Hi Jagan, Simon, >> >> On Tue, Dec 8, 2015 at 11:44 PM, Jagan Teki <jt...@openedev.com> wrote: >>> On 8 December 2015 at 17:27, Bin Meng <bmeng...@gmail.com> wrote: >>>> Hi Jagan, >>>> >>>> On Fri, Dec 4, 2015 at 2:57 AM, Simon Glass <s...@chromium.org> wrote: >>>>> Hi, >>>>> >>>>> On 3 December 2015 at 06:27, Bin Meng <bmeng...@gmail.com> wrote: >>>>>> Hi Jagan, >>>>>> >>>>>> On Thu, Dec 3, 2015 at 6:24 PM, Jagan Teki <jt...@openedev.com> wrote: >>>>>>> Hi Bin, >>>>>>> >>>>>>> On 3 December 2015 at 10:14, Bin Meng <bmeng...@gmail.com> wrote: >>>>>>>> Hi Simon, >>>>>>>> >>>>>>>> On Thu, Dec 3, 2015 at 5:05 AM, Simon Glass <s...@chromium.org> wrote: >>>>>>>>> +Jagan >>>>>>>>> >>>>>>>>> Hi Bin, >>>>>>>>> >>>>>>>>> On 1 December 2015 at 18:41, Bin Meng <bmeng...@gmail.com> wrote: >>>>>>>>>> >>>>>>>>>> Hi Simon, >>>>>>>>>> >>>>>>>>>> On Wed, Dec 2, 2015 at 12:32 AM, Simon Glass <s...@chromium.org> >>>>>>>>>> wrote: >>>>>>>>>> > Hi Bin, >>>>>>>>>> > >>>>>>>>>> > On 28 November 2015 at 05:45, Bin Meng <bmeng...@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...@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 _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
