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

[Jagan Teki]

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.

thanks!
-- 
Jagan