Hi, On 6 February 2016 at 02:57, Jagan Teki <jt...@openedev.com> wrote: > Hi Bin, > > On 6 February 2016 at 09:46, Bin Meng <bmeng...@gmail.com> wrote: >> Hi Jagan, Simon, >> >> On Thu, Jan 21, 2016 at 1:53 PM, Bin Meng <bmeng...@gmail.com> wrote: >>> Hi, >>> >>> On Tue, Dec 15, 2015 at 2:27 PM, Jagan Teki <jt...@openedev.com> wrote: >>>> >>>> On 15 December 2015 at 11:48, Bin Meng <bmeng...@gmail.com> wrote: >>>> > Hi Jagan, >>>> > >>>> > On Tue, Dec 15, 2015 at 2:15 PM, Jagan Teki <jt...@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 <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. >>>> >> >>>> >> >>>> >> 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 _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
