Hi Jagan,
On 12 February 2016 at 09:31, Jagan Teki jteki@openedev.com wrote:
Hi Simon,
On 12 February 2016 at 21:24, Simon Glass sjg@chromium.org wrote:
Hi,
On 6 February 2016 at 02:57, Jagan Teki jteki@openedev.com wrote:
Hi Bin,
On 6 February 2016 at 09:46, Bin Meng bmeng.cn@gmail.com wrote:
Hi Jagan, Simon,
On Thu, Jan 21, 2016 at 1:53 PM, Bin Meng bmeng.cn@gmail.com wrote:
Hi,
On Tue, Dec 15, 2015 at 2:27 PM, Jagan Teki jteki@openedev.com
wrote:
On 15 December 2015 at 11:48, Bin Meng bmeng.cn@gmail.com wrote: > Hi Jagan, > > On Tue, Dec 15, 2015 at 2:15 PM, Jagan Teki jteki@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@openedev.com >>> wrote: >>>> >>>> Hi Bin, >>>> >>>> On 15 December 2015 at 10:48, Bin Meng bmeng.cn@gmail.com
wrote:
>>>>> >>>>> Hi Jagan, Simon, >>>>> >>>>> On Tue, Dec 8, 2015 at 11:44 PM, Jagan Teki <jteki@openedev.com
>>>>> wrote: >>>>>> >>>>>> On 8 December 2015 at 17:27, Bin Meng bmeng.cn@gmail.com
wrote:
>>>>>>> >>>>>>> Hi Jagan, >>>>>>> >>>>>>> On Fri, Dec 4, 2015 at 2:57 AM, Simon Glass <sjg@chromium.org
>>>>>>> wrote: >>>>>>>> >>>>>>>> Hi, >>>>>>>> >>>>>>>> On 3 December 2015 at 06:27, Bin Meng bmeng.cn@gmail.com
wrote:
>>>>>>>>> >>>>>>>>> Hi Jagan, >>>>>>>>> >>>>>>>>> On Thu, Dec 3, 2015 at 6:24 PM, Jagan Teki <
jteki@openedev.com>
>>>>>>>>> wrote: >>>>>>>>>> >>>>>>>>>> Hi Bin, >>>>>>>>>> >>>>>>>>>> On 3 December 2015 at 10:14, Bin Meng bmeng.cn@gmail.com >>>>>>>>>> wrote: >>>>>>>>>>> >>>>>>>>>>> Hi Simon, >>>>>>>>>>> >>>>>>>>>>> On Thu, Dec 3, 2015 at 5:05 AM, Simon Glass <
sjg@chromium.org>
>>>>>>>>>>> wrote: >>>>>>>>>>>> >>>>>>>>>>>> +Jagan >>>>>>>>>>>> >>>>>>>>>>>> Hi Bin, >>>>>>>>>>>> >>>>>>>>>>>> On 1 December 2015 at 18:41, Bin Meng <
bmeng.cn@gmail.com>
>>>>>>>>>>>> wrote: >>>>>>>>>>>>> >>>>>>>>>>>>> >>>>>>>>>>>>> Hi Simon, >>>>>>>>>>>>> >>>>>>>>>>>>> On Wed, Dec 2, 2015 at 12:32 AM, Simon Glass >>>>>>>>>>>>> sjg@chromium.org >>>>>>>>>>>>> wrote: >>>>>>>>>>>>>> >>>>>>>>>>>>>> Hi Bin, >>>>>>>>>>>>>> >>>>>>>>>>>>>> On 28 November 2015 at 05:45, Bin Meng <
bmeng.cn@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@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