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

[Bin Meng]

Hi Jagan,

On Fri, Dec 4, 2015 at 2:57 AM, Simon Glass <sjg at chromium.org> wrote:
> 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'?
>

Can you comment on this CONFIG_SPI_FLASH_GENERIC as Simon suggested?

[snip]

Regards,
Bin