Hi Jagan, On 12 February 2016 at 09:31, Jagan Teki <jt...@openedev.com> wrote: > 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?
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 _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
