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

Fri Feb 12 17:31:31 CET 2016

Hi Simon,

On 12 February 2016 at 21:24, Simon Glass <sjg at chromium.org> wrote:
> 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?

If foot-print is not so huge with those configs I think it's better
not to add an extra config - what do you think?

-- 
Jagan.