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

Simon Glass sjg at chromium.org
Fri Feb 12 20:31:31 CET 2016


Hi Jagan,

On 12 February 2016 at 09:31, Jagan Teki <jteki at openedev.com> wrote:
> 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?

CONFIG_SPI_FLASH_GENERIC could cover the common cases and it should just be
a small amount of entries in the SPI flash table listing supported chips.

Regards,
Simon


More information about the U-Boot mailing list