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.
thanks!