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

Tue Dec 15 07:18:24 CET 2015

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.

Regards,
Bin