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

[Simon Glass]

Hi,

On 3 December 2015 at 06:27, Bin Meng <bmeng.cn at gmail.com> wrote:
> 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.

So how about we group together 3-4 of the common ones, with no special
features, into a 'CONFIG_SPI_FLASH_GENERIC'?

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

Regards,
Simon