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

[Simon Glass]

Hi,

On 6 February 2016 at 02:57, Jagan Teki <jteki at openedev.com> wrote:
> Hi Bin,
>
> On 6 February 2016 at 09:46, Bin Meng <bmeng.cn at gmail.com> wrote:
>> Hi Jagan, Simon,
>>
>> On Thu, Jan 21, 2016 at 1:53 PM, Bin Meng <bmeng.cn at gmail.com> wrote:
>>> Hi,
>>>
>>> On Tue, Dec 15, 2015 at 2:27 PM, Jagan Teki <jteki at openedev.com> wrote:
>>>>
>>>> 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.
>>>>
>>>
>>> I don't see any further comments on this patch. Can we close this?
>>>
>>
>> A gentle ping .. I still would like to clean these up unless there is
>> something changes in the SF (as Simon proposed) to be planned.
>
> Except - macronix, spansion, stmicro, sst, winbond configs we can
> remove remaining flash vendor configs for now, because later configs
> using common code.

Does this mean the CONFIG_SPI_FLASH_GENERIC idea is good, or not?

Regards,
Simon