Hi Simon, On 13 February 2016 at 00:58, Simon Glass <s...@chromium.org> wrote: > 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.
OK, then will do this change on top of spi-nor. thanks! -- Jagan. _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot