Hi Simon, On 12 February 2016 at 21:24, Simon Glass <s...@chromium.org> wrote: > 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?
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? -- Jagan. _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot