Hi Bin,
On 6 February 2016 at 09:46, Bin Meng bmeng.cn@gmail.com wrote:
Hi Jagan, Simon,
On Thu, Jan 21, 2016 at 1:53 PM, Bin Meng bmeng.cn@gmail.com wrote:
Hi,
On Tue, Dec 15, 2015 at 2:27 PM, Jagan Teki jteki@openedev.com wrote:
On 15 December 2015 at 11:48, Bin Meng bmeng.cn@gmail.com wrote:
Hi Jagan,
On Tue, Dec 15, 2015 at 2:15 PM, Jagan Teki jteki@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 jteki@openedev.com wrote: > > Hi Bin, > > On 15 December 2015 at 10:48, Bin Meng bmeng.cn@gmail.com wrote: >> >> Hi Jagan, Simon, >> >> On Tue, Dec 8, 2015 at 11:44 PM, Jagan Teki jteki@openedev.com >> wrote: >>> >>> On 8 December 2015 at 17:27, Bin Meng bmeng.cn@gmail.com wrote: >>>> >>>> Hi Jagan, >>>> >>>> On Fri, Dec 4, 2015 at 2:57 AM, Simon Glass sjg@chromium.org >>>> wrote: >>>>> >>>>> Hi, >>>>> >>>>> On 3 December 2015 at 06:27, Bin Meng bmeng.cn@gmail.com wrote: >>>>>> >>>>>> Hi Jagan, >>>>>> >>>>>> On Thu, Dec 3, 2015 at 6:24 PM, Jagan Teki jteki@openedev.com >>>>>> wrote: >>>>>>> >>>>>>> Hi Bin, >>>>>>> >>>>>>> On 3 December 2015 at 10:14, Bin Meng bmeng.cn@gmail.com >>>>>>> wrote: >>>>>>>> >>>>>>>> Hi Simon, >>>>>>>> >>>>>>>> On Thu, Dec 3, 2015 at 5:05 AM, Simon Glass sjg@chromium.org >>>>>>>> wrote: >>>>>>>>> >>>>>>>>> +Jagan >>>>>>>>> >>>>>>>>> Hi Bin, >>>>>>>>> >>>>>>>>> On 1 December 2015 at 18:41, Bin Meng bmeng.cn@gmail.com >>>>>>>>> wrote: >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> Hi Simon, >>>>>>>>>> >>>>>>>>>> On Wed, Dec 2, 2015 at 12:32 AM, Simon Glass >>>>>>>>>> sjg@chromium.org >>>>>>>>>> wrote: >>>>>>>>>>> >>>>>>>>>>> Hi Bin, >>>>>>>>>>> >>>>>>>>>>> On 28 November 2015 at 05:45, Bin Meng bmeng.cn@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.cn@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.