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

[Bin Meng]

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?

Regards,
Bin