[U-Boot] [PATCH] x86: Clean up SPI flash drivers in defconfig

[Jagan Teki]

Hi Bin,

On 3 December 2015 at 10:14, Bin Meng <bmeng.cn at gmail.com> wrote:
> Hi Simon,
>
> On Thu, Dec 3, 2015 at 5:05 AM, Simon Glass <sjg at chromium.org> wrote:
>> +Jagan
>>
>> Hi Bin,
>>
>> On 1 December 2015 at 18:41, Bin Meng <bmeng.cn at gmail.com> wrote:
>>>
>>> Hi Simon,
>>>
>>> On Wed, Dec 2, 2015 at 12:32 AM, Simon Glass <sjg at chromium.org> wrote:
>>> > Hi Bin,
>>> >
>>> > On 28 November 2015 at 05:45, Bin Meng <bmeng.cn at 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 at 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?

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

>
>>>
>>> >
>>> > For all of these platforms we can use the dediprog em100 which I
>>> > typically set to use winbond as the manufacturer, regardless of which
>>> > chip is actually on the board.
>>> >
>>>
>>> I think that's because emulator can emulate flash from various vendors.
>>
>> Yes, and also for convenience.
>>
>>>
>>> > For U-Boot on coreboot, why is SPI flash not supported? It certainly
>>> > works with link.
>>>
>>> Yes, booting from coreboot does support SPI flash. However since we
>>> decided to use QEMU as the default build target for coreboot, and QEMU
>>> does not support SPI flash yet, these config options are removed. One
>>> can certainly adjust these Kconfig options via 'make menuconfig', eg:
>>> adding SD/MMC support which is not in coreboot's defconfig either.
>>
>> Well this breaks booting on link, since the SPI flash stops working.
>> I'm really not keen on having to specially select the SPI flash when
>> you want to use link.
>>
>
> Do you mean booting U-Boot on link from coreboot? Without SPI flash it
> should still boot. It looks to me your preference is to include all
> the available drivers into coreboot's defconfig, yes? If so, we may
> add some other drivers Kconfig in coreboot-x86_defconfig too, like
> SD/MMC drivers, all the available ethernet drivers even they only
> exist on some boards.

thanks!
-- 
Jagan | openedev.