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

[Bin Meng]

Hi Jagan,

On Thu, Dec 3, 2015 at 6:24 PM, Jagan Teki <jteki at openedev.com> wrote:
> 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?
>

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.

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

Regards,
Bin