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

[Jagan Teki]

Hi Simon,

On 13 February 2016 at 00:58, Simon Glass <sjg at chromium.org> wrote:
> 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.

OK, then will do this change on top of spi-nor.

thanks!
-- 
Jagan.