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

[Jagan Teki]

On 15 December 2015 at 11:48, Bin Meng <bmeng.cn at gmail.com> wrote:
> Hi Jagan,
>
> On Tue, Dec 15, 2015 at 2:15 PM, Jagan Teki <jteki at openedev.com> wrote:
>> Hi Bin,
>>
>>
>> On Tuesday 15 December 2015 11:37 AM, Bin Meng wrote:
>>>
>>> Hi Jagan,
>>>
>>> On Tue, Dec 15, 2015 at 1:51 PM, Jagan Teki <jteki at openedev.com> wrote:
>>>>
>>>> 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.
>>>>
>>>
>>> Yes, your understanding is the same as mine. I wasn't clear in my
>>> previous question.
>>>
>>> Right now this patch is doing exactly as what you and I understand,
>>> that we just want to select the specific flash macro for a specific
>>> x86 board. But Simon wanted to enable all of the flash macros for one
>>> board for convenience. Thus I came to ask for what's our direction.
>>
>>
>> So, does this board supports or connected all flash variants? in that case
>> it is true right? "for convenience" here means for testing then ie also true
>> because this particular board is meant for testing all flash devices, and
>> also it is up to this particular board config over-head rather than the
>> generic spi-flash.
>>
>
> No, each board only connects one specific type of SPI flash, as
> described in the board device tree file.

So your patch did that change? let me look at it what Simon commenting.

thanks!
-- 
Jagan.