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

[Jagan Teki]

Hi Bin,

On 15 December 2015 at 10:48, Bin Meng <bmeng.cn at gmail.com> wrote:
> Hi Jagan, Simon,
>
> On Tue, Dec 8, 2015 at 11:44 PM, Jagan Teki <jteki at openedev.com> wrote:
>> On 8 December 2015 at 17:27, Bin Meng <bmeng.cn at gmail.com> wrote:
>>> 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?
>>
>> 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!
-- 
Jagan.