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

[Jagan Teki]

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.

-- 
Jagan.