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

[Bin Meng]

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.

Regards,
Bin