x86: Clean up SPI flash drivers in defconfig

PATCH New Bin Meng 3 participants 24 messages Nov 28, 2015 – Feb 12, 2016

lore

Share & tools

lore

Bin Meng

author

2015-11-28 12:45 UTC #0 lore raw

PATCH 1/1 x86: Clean up SPI flash drivers in defconfig

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 <[email protected]>

---

 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(-)

configs/bayleybay_defconfig +0 -2

index 0a5a56f..c1ed990 100644
@@ -21,8 +21,6 @@ CONFIG_CMD_BOOTSTAGE=y
 CONFIG_OF_CONTROL=y
 CONFIG_CPU=y
 CONFIG_SPI_FLASH=y
-CONFIG_SPI_FLASH_GIGADEVICE=y
-CONFIG_SPI_FLASH_MACRONIX=y
 CONFIG_SPI_FLASH_WINBOND=y
 CONFIG_DM_ETH=y
 CONFIG_E1000=y

configs/chromebook_link_defconfig +0 -2

index ac64877..26b1d13 100644
@@ -21,8 +21,6 @@ CONFIG_CMD_CROS_EC=y
 CONFIG_CROS_EC=y
 CONFIG_CROS_EC_LPC=y
 CONFIG_SPI_FLASH=y
-CONFIG_SPI_FLASH_GIGADEVICE=y
-CONFIG_SPI_FLASH_MACRONIX=y
 CONFIG_SPI_FLASH_WINBOND=y
 CONFIG_DM_PCI=y
 CONFIG_DM_RTC=y

configs/chromebox_panther_defconfig +0 -2

index 0f3a9af..7fb25b4 100644
@@ -20,8 +20,6 @@ CONFIG_CMD_CROS_EC=y
 CONFIG_CROS_EC=y
 CONFIG_CROS_EC_LPC=y
 CONFIG_SPI_FLASH=y
-CONFIG_SPI_FLASH_GIGADEVICE=y
-CONFIG_SPI_FLASH_MACRONIX=y
 CONFIG_SPI_FLASH_WINBOND=y
 CONFIG_DM_PCI=y
 CONFIG_DM_RTC=y

configs/coreboot-x86_defconfig +0 -4

index 8903cdd..96b6d0d 100644
@@ -12,10 +12,6 @@ CONFIG_CMD_BOOTSTAGE=y
 CONFIG_CMD_TPM=y
 CONFIG_CMD_TPM_TEST=y
 CONFIG_OF_CONTROL=y
-CONFIG_SPI_FLASH=y
-CONFIG_SPI_FLASH_GIGADEVICE=y
-CONFIG_SPI_FLASH_MACRONIX=y
-CONFIG_SPI_FLASH_WINBOND=y
 CONFIG_DM_ETH=y
 CONFIG_E1000=y
 CONFIG_DM_PCI=y

configs/crownbay_defconfig +0 -3

index f4592c5..c96e800 100644
@@ -19,10 +19,7 @@ CONFIG_CMD_BOOTSTAGE=y
 CONFIG_OF_CONTROL=y
 CONFIG_CPU=y
 CONFIG_SPI_FLASH=y
-CONFIG_SPI_FLASH_GIGADEVICE=y
-CONFIG_SPI_FLASH_MACRONIX=y
 CONFIG_SPI_FLASH_SST=y
-CONFIG_SPI_FLASH_WINBOND=y
 CONFIG_DM_ETH=y
 CONFIG_E1000=y
 CONFIG_PCH_GBE=y

configs/galileo_defconfig +0 -2

index 3612350..0dfc5f2 100644
@@ -15,8 +15,6 @@ CONFIG_CMD_BOOTSTAGE=y
 CONFIG_OF_CONTROL=y
 CONFIG_NET_RANDOM_ETHADDR=y
 CONFIG_SPI_FLASH=y
-CONFIG_SPI_FLASH_GIGADEVICE=y
-CONFIG_SPI_FLASH_MACRONIX=y
 CONFIG_SPI_FLASH_WINBOND=y
 CONFIG_DM_ETH=y
 CONFIG_ETH_DESIGNWARE=y

configs/minnowmax_defconfig +0 -3

index 37c07c1..44fb426 100644
@@ -20,10 +20,7 @@ CONFIG_CMD_BOOTSTAGE=y
 CONFIG_OF_CONTROL=y
 CONFIG_CPU=y
 CONFIG_SPI_FLASH=y
-CONFIG_SPI_FLASH_GIGADEVICE=y
-CONFIG_SPI_FLASH_MACRONIX=y
 CONFIG_SPI_FLASH_STMICRO=y
-CONFIG_SPI_FLASH_WINBOND=y
 CONFIG_DM_ETH=y
 CONFIG_DM_PCI=y
 CONFIG_DM_RTC=y

configs/qemu-x86_defconfig +0 -4

index ebdb892..6cd6bfb 100644
@@ -14,10 +14,6 @@ CONFIG_BOOTSTAGE_REPORT=y
 CONFIG_CMD_BOOTSTAGE=y
 CONFIG_OF_CONTROL=y
 CONFIG_CPU=y
-CONFIG_SPI_FLASH=y
-CONFIG_SPI_FLASH_GIGADEVICE=y
-CONFIG_SPI_FLASH_MACRONIX=y
-CONFIG_SPI_FLASH_WINBOND=y
 CONFIG_DM_ETH=y
 CONFIG_E1000=y
 CONFIG_DM_PCI=y

Signature · 1 line

1.8.2.1

Simon Glass

2015-12-01 16:32 UTC #1 lore raw

PATCH 1/1 x86: Clean up SPI flash drivers in defconfig

Hi Bin,

Quoted 9 lines · Bin Meng

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 <[email protected]>
---

Quoted 9 lines

 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?

For all of these platforms we can use the dediprog em100 which I typically set to use winbond as the manufacturer, regardless of which chip is actually on the board.

For U-Boot on coreboot, why is SPI flash not supported? It certainly works with link.

Regards,
Simon

Bin Meng

author

2015-12-02 01:41 UTC #2 lore raw

PATCH 1/1 x86: Clean up SPI flash drivers in defconfig

Hi Simon,

Quoted 22 lines · Simon Glass

Hi Bin,
Quoted 9 lines · Bin Meng
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 <[email protected]>
---
Quoted 9 lines
 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(-)

Quoted 2 lines

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.

Quoted 5 lines

For all of these platforms we can use the dediprog em100 which I typically set to use winbond as the manufacturer, regardless of which chip is actually on the board.

I think that's because emulator can emulate flash from various vendors.

Quoted 2 lines

For U-Boot on coreboot, why is SPI flash not supported? It certainly works with link.

Yes, booting from coreboot does support SPI flash. However since we decided to use QEMU as the default build target for coreboot, and QEMU does not support SPI flash yet, these config options are removed. One can certainly adjust these Kconfig options via 'make menuconfig', eg:
adding SD/MMC support which is not in coreboot's defconfig either.

Regards,
Bin

Simon Glass

2015-12-02 21:05 UTC #3 lore raw

PATCH 1/1 x86: Clean up SPI flash drivers in defconfig

+Jagan

Hi Bin,

Quoted 26 lines · Bin Meng

Hi Simon,
Quoted 22 lines · Simon Glass
Hi Bin,
Quoted 18 lines · Bin Meng
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 <[email protected]>
---
 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(-)

Quoted 8 lines

Quoted 2 lines
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.

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?

Quoted 8 lines

Quoted 6 lines
For all of these platforms we can use the dediprog em100 which I typically set to use winbond as the manufacturer, regardless of which chip is actually on the board.
I think that's because emulator can emulate flash from various vendors.

Yes, and also for convenience.

Quoted 9 lines

Quoted 3 lines
For U-Boot on coreboot, why is SPI flash not supported? It certainly works with link.
Yes, booting from coreboot does support SPI flash. However since we decided to use QEMU as the default build target for coreboot, and QEMU does not support SPI flash yet, these config options are removed. One can certainly adjust these Kconfig options via 'make menuconfig', eg:
adding SD/MMC support which is not in coreboot's defconfig either.

Well this breaks booting on link, since the SPI flash stops working. I'm really not keen on having to specially select the SPI flash when you want to use link.

Regards,
Simon

Bin Meng

author

2015-12-03 04:44 UTC #4 lore raw

PATCH 1/1 x86: Clean up SPI flash drivers in defconfig

Hi Simon,

Quoted 40 lines · Simon Glass

+Jagan
Hi Bin,
Quoted 26 lines · Bin Meng
Hi Simon,
Quoted 22 lines · Simon Glass
Hi Bin,
Quoted 18 lines · Bin Meng
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 <[email protected]>
---
 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(-)
Quoted 8 lines
Quoted 2 lines
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.

Quoted 5 lines

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.

Quoted 21 lines

Quoted 8 lines
Quoted 6 lines
For all of these platforms we can use the dediprog em100 which I typically set to use winbond as the manufacturer, regardless of which chip is actually on the board.
I think that's because emulator can emulate flash from various vendors.
Yes, and also for convenience.
Quoted 9 lines
Quoted 3 lines
For U-Boot on coreboot, why is SPI flash not supported? It certainly works with link.
Yes, booting from coreboot does support SPI flash. However since we decided to use QEMU as the default build target for coreboot, and QEMU does not support SPI flash yet, these config options are removed. One can certainly adjust these Kconfig options via 'make menuconfig', eg:
adding SD/MMC support which is not in coreboot's defconfig either.

Quoted 4 lines

Well this breaks booting on link, since the SPI flash stops working. I'm really not keen on having to specially select the SPI flash when you want to use link.

Do you mean booting U-Boot on link from coreboot? Without SPI flash it should still boot. It looks to me your preference is to include all the available drivers into coreboot's defconfig, yes? If so, we may add some other drivers Kconfig in coreboot-x86_defconfig too, like SD/MMC drivers, all the available ethernet drivers even they only exist on some boards.

Regards,
Bin

Jagan Teki

2015-12-03 10:24 UTC #5 lore raw

PATCH 1/1 x86: Clean up SPI flash drivers in defconfig

Hi Bin,

Quoted 42 lines · Bin Meng

Hi Simon,
Quoted 39 lines · Simon Glass
+Jagan
Hi Bin,
Quoted 34 lines · Bin Meng
Hi Simon,
Quoted 22 lines · Simon Glass
Hi Bin,
Quoted 9 lines · Bin Meng
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 <[email protected]>
---
Quoted 9 lines
 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(-)
Quoted 2 lines
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?

Quoted 11 lines

Quoted 6 lines
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.

Quoted 7 lines

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.

Quoted 27 lines

Quoted 22 lines
Quoted 9 lines
Quoted 7 lines
For all of these platforms we can use the dediprog em100 which I typically set to use winbond as the manufacturer, regardless of which chip is actually on the board.
I think that's because emulator can emulate flash from various vendors.
Yes, and also for convenience.
Quoted 9 lines
Quoted 3 lines
For U-Boot on coreboot, why is SPI flash not supported? It certainly works with link.
Yes, booting from coreboot does support SPI flash. However since we decided to use QEMU as the default build target for coreboot, and QEMU does not support SPI flash yet, these config options are removed. One can certainly adjust these Kconfig options via 'make menuconfig', eg:
adding SD/MMC support which is not in coreboot's defconfig either.
Quoted 4 lines
Well this breaks booting on link, since the SPI flash stops working. I'm really not keen on having to specially select the SPI flash when you want to use link.

Quoted 6 lines

Do you mean booting U-Boot on link from coreboot? Without SPI flash it should still boot. It looks to me your preference is to include all the available drivers into coreboot's defconfig, yes? If so, we may add some other drivers Kconfig in coreboot-x86_defconfig too, like SD/MMC drivers, all the available ethernet drivers even they only exist on some boards.

thanks!

Signature · 1 line

Jagan | openedev.

Bin Meng

author

2015-12-03 13:27 UTC #6 lore raw

PATCH 1/1 x86: Clean up SPI flash drivers in defconfig

Hi Jagan,

Quoted 40 lines · Jagan Teki

Hi Bin,
Quoted 37 lines · Bin Meng
Hi Simon,
Quoted 34 lines · Simon Glass
+Jagan
Hi Bin,
Quoted 29 lines · Bin Meng
Hi Simon,
Quoted 24 lines · Simon Glass
Hi Bin,
Quoted 9 lines · Bin Meng
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 <[email protected]>
---
Quoted 9 lines
 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?

Quoted 8 lines

Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
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.

Quoted 14 lines

Quoted 14 lines
Quoted 6 lines
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"

Quoted 8 lines

Quoted 4 lines
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.

Quoted 34 lines

Quoted 27 lines
Quoted 22 lines
Quoted 9 lines
Quoted 7 lines
For all of these platforms we can use the dediprog em100 which I typically set to use winbond as the manufacturer, regardless of which chip is actually on the board.
I think that's because emulator can emulate flash from various vendors.
Yes, and also for convenience.
Quoted 9 lines
Quoted 3 lines
For U-Boot on coreboot, why is SPI flash not supported? It certainly works with link.
Yes, booting from coreboot does support SPI flash. However since we decided to use QEMU as the default build target for coreboot, and QEMU does not support SPI flash yet, these config options are removed. One can certainly adjust these Kconfig options via 'make menuconfig', eg:
adding SD/MMC support which is not in coreboot's defconfig either.
Quoted 4 lines
Well this breaks booting on link, since the SPI flash stops working. I'm really not keen on having to specially select the SPI flash when you want to use link.
Quoted 6 lines
Do you mean booting U-Boot on link from coreboot? Without SPI flash it should still boot. It looks to me your preference is to include all the available drivers into coreboot's defconfig, yes? If so, we may add some other drivers Kconfig in coreboot-x86_defconfig too, like SD/MMC drivers, all the available ethernet drivers even they only exist on some boards.

Quoted 2 lines

thanks!
--

Regards,
Bin

Simon Glass

2015-12-03 18:57 UTC #7 lore raw

PATCH 1/1 x86: Clean up SPI flash drivers in defconfig

Hi,

Quoted 73 lines · Bin Meng

Hi Jagan,
Quoted 40 lines · Jagan Teki
Hi Bin,
Quoted 37 lines · Bin Meng
Hi Simon,
Quoted 34 lines · Simon Glass
+Jagan
Hi Bin,
Quoted 29 lines · Bin Meng
Hi Simon,
Quoted 24 lines · Simon Glass
Hi Bin,
Quoted 9 lines · Bin Meng
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 <[email protected]>
---
Quoted 9 lines
 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?
Quoted 8 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
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.
Quoted 19 lines
Quoted 18 lines
Quoted 6 lines
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.

Quoted 7 lines

Quoted 3 lines
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'?

Quoted 35 lines

Quoted 35 lines
Quoted 34 lines
Quoted 23 lines
Quoted 10 lines
Quoted 8 lines
For all of these platforms we can use the dediprog em100 which I typically set to use winbond as the manufacturer, regardless of which chip is actually on the board.
I think that's because emulator can emulate flash from various vendors.
Yes, and also for convenience.
Quoted 9 lines
Quoted 3 lines
For U-Boot on coreboot, why is SPI flash not supported? It certainly works with link.
Yes, booting from coreboot does support SPI flash. However since we decided to use QEMU as the default build target for coreboot, and QEMU does not support SPI flash yet, these config options are removed. One can certainly adjust these Kconfig options via 'make menuconfig', eg:
adding SD/MMC support which is not in coreboot's defconfig either.
Quoted 4 lines
Well this breaks booting on link, since the SPI flash stops working. I'm really not keen on having to specially select the SPI flash when you want to use link.
Do you mean booting U-Boot on link from coreboot? Without SPI flash it should still boot. It looks to me your preference is to include all the available drivers into coreboot's defconfig, yes? If so, we may add some other drivers Kconfig in coreboot-x86_defconfig too, like SD/MMC drivers, all the available ethernet drivers even they only exist on some boards.

Quoted 5 lines

Quoted 2 lines
thanks!
--
Regards,
Bin

Regards,
Simon

Bin Meng

author

2015-12-08 11:57 UTC #8 lore raw

PATCH 1/1 x86: Clean up SPI flash drivers in defconfig

Hi Jagan,

Quoted 80 lines · Simon Glass

Hi,
Quoted 77 lines · Bin Meng
Hi Jagan,
Quoted 40 lines · Jagan Teki
Hi Bin,
Quoted 37 lines · Bin Meng
Hi Simon,
Quoted 34 lines · Simon Glass
+Jagan
Hi Bin,
Quoted 29 lines · Bin Meng
Hi Simon,
Quoted 24 lines · Simon Glass
Hi Bin,
Quoted 9 lines · Bin Meng
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 <[email protected]>
---
Quoted 9 lines
 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?
Quoted 8 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
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.
Quoted 22 lines
Quoted 11 lines
Quoted 6 lines
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.
Quoted 7 lines
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.

Quoted 7 lines

Quoted 3 lines
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?

[snip]

Regards,
Bin

Jagan Teki

2015-12-08 15:44 UTC #9 lore raw

PATCH 1/1 x86: Clean up SPI flash drivers in defconfig

Quoted 87 lines · Bin Meng

Hi Jagan,
Quoted 84 lines · Simon Glass
Hi,
Quoted 80 lines · Bin Meng
Hi Jagan,
Quoted 40 lines · Jagan Teki
Hi Bin,
Quoted 37 lines · Bin Meng
Hi Simon,
Quoted 34 lines · Simon Glass
+Jagan
Hi Bin,
Quoted 29 lines · Bin Meng
Hi Simon,
Quoted 24 lines · Simon Glass
Hi Bin,
Quoted 9 lines · Bin Meng
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 <[email protected]>
---
Quoted 9 lines
 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?
Quoted 8 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
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.
Quoted 14 lines
Quoted 14 lines
Quoted 6 lines
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"
Quoted 8 lines
Quoted 4 lines
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.

Quoted 5 lines

Quoted 3 lines
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.

thanks!

Signature · 1 line

Jagan.

Bin Meng

author

2015-12-15 05:18 UTC #10 lore raw

PATCH 1/1 x86: Clean up SPI flash drivers in defconfig

Hi Jagan, Simon,

Quoted 92 lines · Jagan Teki

Quoted 91 lines · Bin Meng
Hi Jagan,
Quoted 87 lines · Simon Glass
Hi,
Quoted 73 lines · Bin Meng
Hi Jagan,
Quoted 40 lines · Jagan Teki
Hi Bin,
Quoted 37 lines · Bin Meng
Hi Simon,
Quoted 34 lines · Simon Glass
+Jagan
Hi Bin,
Quoted 29 lines · Bin Meng
Hi Simon,
Quoted 24 lines · Simon Glass
Hi Bin,
Quoted 9 lines · Bin Meng
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 <[email protected]>
---
Quoted 9 lines
 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?
Quoted 8 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
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.
Quoted 19 lines
Quoted 18 lines
Quoted 6 lines
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.
Quoted 7 lines
Quoted 3 lines
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'?

Quoted 7 lines

Quoted 1 line
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.

Regards,
Bin

Jagan Teki

2015-12-15 05:51 UTC #11 lore raw

PATCH 1/1 x86: Clean up SPI flash drivers in defconfig

Hi Bin,

Quoted 97 lines · Bin Meng

Hi Jagan, Simon,
Quoted 94 lines · Jagan Teki
Quoted 92 lines · Bin Meng
Hi Jagan,
Quoted 80 lines · Simon Glass
Hi,
Quoted 77 lines · Bin Meng
Hi Jagan,
Quoted 40 lines · Jagan Teki
Hi Bin,
Quoted 37 lines · Bin Meng
Hi Simon,
Quoted 34 lines · Simon Glass
+Jagan
Hi Bin,
Quoted 29 lines · Bin Meng
Hi Simon,
Quoted 24 lines · Simon Glass
Hi Bin,
Quoted 9 lines · Bin Meng
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 <[email protected]>
---
Quoted 9 lines
 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?
Quoted 8 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
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.
Quoted 22 lines
Quoted 11 lines
Quoted 6 lines
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.
Quoted 7 lines
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.
Quoted 7 lines
Quoted 3 lines
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?

Quoted 8 lines

Quoted 5 lines
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.

thanks!

Signature · 1 line

Jagan.

Bin Meng

author

2015-12-15 06:07 UTC #12 lore raw

PATCH 1/1 x86: Clean up SPI flash drivers in defconfig

Hi Jagan,

Quoted 109 lines · Jagan Teki

Hi Bin,
Quoted 97 lines · Bin Meng
Hi Jagan, Simon,
Quoted 94 lines · Jagan Teki
Quoted 92 lines · Bin Meng
Hi Jagan,
Quoted 80 lines · Simon Glass
Hi,
Quoted 77 lines · Bin Meng
Hi Jagan,
Quoted 40 lines · Jagan Teki
Hi Bin,
Quoted 37 lines · Bin Meng
Hi Simon,
Quoted 34 lines · Simon Glass
+Jagan
Hi Bin,
Quoted 29 lines · Bin Meng
Hi Simon,
Quoted 24 lines · Simon Glass
Hi Bin,
Quoted 9 lines · Bin Meng
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 <[email protected]>
---
Quoted 9 lines
 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?
Quoted 8 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
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.
Quoted 22 lines
Quoted 11 lines
Quoted 6 lines
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.
Quoted 7 lines
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.
Quoted 7 lines
Quoted 3 lines
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?
Quoted 8 lines
Quoted 5 lines
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.

Quoted 8 lines

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.

Quoted 3 lines

thanks!
--
Jagan.

Regards,
Bin

Jagan Teki

2015-12-15 06:15 UTC #13 lore raw

PATCH 1/1 x86: Clean up SPI flash drivers in defconfig

Hi Bin,

Quoted 121 lines · Bin Meng

Hi Jagan,
Quoted 109 lines · Jagan Teki
Hi Bin,
Quoted 97 lines · Bin Meng
Hi Jagan, Simon,
Quoted 94 lines · Jagan Teki
Quoted 92 lines · Bin Meng
Hi Jagan,
Quoted 80 lines · Simon Glass
Hi,
Quoted 77 lines · Bin Meng
Hi Jagan,
Quoted 40 lines · Jagan Teki
Hi Bin,
Quoted 37 lines · Bin Meng
Hi Simon,
Quoted 34 lines · Simon Glass
+Jagan
Hi Bin,
Quoted 29 lines · Bin Meng
Hi Simon,
Quoted 24 lines · Simon Glass
Hi Bin,
Quoted 18 lines · Bin Meng
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 <[email protected]>
---
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?
Quoted 8 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
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.
Quoted 22 lines
Quoted 11 lines
Quoted 6 lines
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.
Quoted 7 lines
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.
Quoted 7 lines
Quoted 3 lines
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?
Quoted 8 lines
Quoted 5 lines
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.
Quoted 8 lines
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.

Quoted 7 lines

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.

thanks!

Signature · 1 line

Jagan

Bin Meng

author

2015-12-15 06:18 UTC #14 lore raw

PATCH 1/1 x86: Clean up SPI flash drivers in defconfig

Hi Jagan,

Quoted 171 lines · Jagan Teki

Hi Bin,
Quoted 158 lines · Bin Meng
Hi Jagan,
Quoted 145 lines · Jagan Teki
Hi Bin,
Quoted 131 lines · Bin Meng
Hi Jagan, Simon,
Quoted 127 lines · Jagan Teki
Quoted 123 lines · Bin Meng
Hi Jagan,
Quoted 108 lines · Simon Glass
Hi,
Quoted 104 lines · Bin Meng
Hi Jagan,
Quoted 64 lines · Jagan Teki
Hi Bin,
Quoted 58 lines · Bin Meng
Hi Simon,
Quoted 53 lines · Simon Glass
+Jagan
Hi Bin,
Quoted 47 lines · Bin Meng
Hi Simon,
Quoted 30 lines · Simon Glass
Hi Bin,
On 28 November 2015 at 05:45, Bin Meng <[email protected]>
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 <[email protected]>
---
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.
Quoted 2 lines
Are you talking about flash vendor config or CONFIG_SPI_FLASH?
Flash vendor config, as you see in this patch.
Quoted 29 lines
Quoted 18 lines
Quoted 8 lines
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"
Quoted 6 lines
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.
Quoted 9 lines
Quoted 3 lines
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?
Quoted 8 lines
Quoted 5 lines
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.
Quoted 8 lines
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.
Quoted 7 lines
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.

Quoted 6 lines

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.

Regards,
Bin

Jagan Teki

2015-12-15 06:27 UTC #15 lore raw

PATCH 1/1 x86: Clean up SPI flash drivers in defconfig

Quoted 181 lines · Bin Meng

Hi Jagan,
Quoted 171 lines · Jagan Teki
Hi Bin,
Quoted 158 lines · Bin Meng
Hi Jagan,
Quoted 145 lines · Jagan Teki
Hi Bin,
Quoted 131 lines · Bin Meng
Hi Jagan, Simon,
Quoted 127 lines · Jagan Teki
Quoted 123 lines · Bin Meng
Hi Jagan,
Quoted 108 lines · Simon Glass
Hi,
Quoted 104 lines · Bin Meng
Hi Jagan,
Quoted 64 lines · Jagan Teki
Hi Bin,
Quoted 58 lines · Bin Meng
Hi Simon,
Quoted 53 lines · Simon Glass
+Jagan
Hi Bin,
Quoted 47 lines · Bin Meng
Hi Simon,
On Wed, Dec 2, 2015 at 12:32 AM, Simon Glass <[email protected]>
wrote:
Hi Bin,
On 28 November 2015 at 05:45, Bin Meng <[email protected]>
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 <[email protected]>
---
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.
Quoted 2 lines
Are you talking about flash vendor config or CONFIG_SPI_FLASH?
Flash vendor config, as you see in this patch.
Quoted 29 lines
Quoted 18 lines
Quoted 8 lines
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"
Quoted 6 lines
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.
Quoted 9 lines
Quoted 3 lines
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?
Quoted 8 lines
Quoted 5 lines
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.
Quoted 8 lines
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.
Quoted 7 lines
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.
Quoted 6 lines
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.

Quoted 2 lines

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.

thanks!

Signature · 1 line

Jagan.

Bin Meng

author

2016-01-21 05:53 UTC #16 lore raw

PATCH 1/1 x86: Clean up SPI flash drivers in defconfig

Hi,

Quoted 199 lines · Jagan Teki

Quoted 11 lines · Bin Meng
Hi Jagan,
Quoted 8 lines · Jagan Teki
Hi Bin,
Quoted 4 lines · Bin Meng
Hi Jagan,
On Tue, Dec 15, 2015 at 1:51 PM, Jagan Teki <[email protected]>
Quoted 8 lines · wrote:
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Hi Bin,
Quoted 4 lines · Bin Meng
Hi Jagan, Simon,
On Tue, Dec 8, 2015 at 11:44 PM, Jagan Teki <[email protected]>
Quoted 6 lines · wrote:
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines · Bin Meng
Quoted 4 lines
Hi Jagan,
On Fri, Dec 4, 2015 at 2:57 AM, Simon Glass <[email protected]>
Quoted 13 lines · wrote:
Quoted 13 lines
Quoted 13 lines
Quoted 13 lines
Quoted 13 lines
Quoted 13 lines
Quoted 13 lines
Quoted 13 lines
Hi,
Quoted 9 lines · Bin Meng
Hi Jagan,
Quoted 4 lines · Jagan Teki
Hi Bin,
On 3 December 2015 at 10:14, Bin Meng <[email protected]>
Quoted 11 lines · wrote:
Quoted 11 lines
Quoted 11 lines
Quoted 11 lines
Quoted 11 lines
Quoted 11 lines
Quoted 11 lines
Quoted 11 lines
Quoted 11 lines
Quoted 11 lines
Quoted 11 lines
Hi Simon,
Quoted 6 lines · Simon Glass
+Jagan
Hi Bin,
On 1 December 2015 at 18:41, Bin Meng <[email protected]>
Quoted 5 lines · wrote:
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Hi Simon,
On Wed, Dec 2, 2015 at 12:32 AM, Simon Glass <
sjg at chromium.org>
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
wrote:
Hi Bin,
On 28 November 2015 at 05:45, Bin Meng <[email protected]>
wrote:
Every board has one dedicated type of SPI flash, hence it
is
Quoted 20 lines
Quoted 20 lines
Quoted 20 lines
Quoted 20 lines
Quoted 20 lines
Quoted 20 lines
Quoted 20 lines
Quoted 20 lines
Quoted 20 lines
Quoted 20 lines
Quoted 20 lines
Quoted 20 lines
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 <[email protected]>
---
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(-)
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
What is the benefit of this? I see it removes a few lines
in a
Quoted 15 lines
Quoted 15 lines
Quoted 15 lines
Quoted 15 lines
Quoted 15 lines
Quoted 15 lines
Quoted 15 lines
Quoted 15 lines
Quoted 15 lines
Quoted 15 lines
Quoted 13 lines
Quoted 13 lines
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.
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 2 lines
Are you talking about flash vendor config or CONFIG_SPI_FLASH?
Flash vendor config, as you see in this patch.
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
They are a hangover from when we had a separate driver for
each
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
one.
Jagan put a lot of effort into removing all the
semi-duplicated
Quoted 11 lines
Quoted 11 lines
Quoted 11 lines
Quoted 11 lines
Quoted 11 lines
Quoted 11 lines
Quoted 11 lines
Quoted 11 lines
Quoted 11 lines
Quoted 11 lines
Quoted 11 lines
Quoted 4 lines
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:
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
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
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
generic driver with a special byte program and word program
which
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 2 lines
is
incompatible with other vendors' flashes.
But there is some flash vendor specific code like quad enable
bit,
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Quoted 2 lines
locking ops and finally about spi_flash_params table.
I know. That's probably why adding all these SPI flash drivers
don't
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
help at all because only one code path will take effect. And
what I
Quoted 9 lines
Quoted 9 lines
Quoted 9 lines
Quoted 9 lines
Quoted 9 lines
Quoted 9 lines
Quoted 9 lines
Quoted 7 lines
Quoted 1 line
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?
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
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
Quoted 19 lines
Quoted 19 lines
Quoted 19 lines
Quoted 7 lines
Quoted 5 lines
Quoted 2 lines
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.
Quoted 8 lines
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.
Quoted 7 lines
Quoted 7 lines
Quoted 4 lines
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
Quoted 1 line
Quoted 1 line
it is true right? "for convenience" here means for testing then ie also
true
Quoted 1 line
Quoted 1 line
because this particular board is meant for testing all flash devices,
and

Quoted 10 lines

Quoted 6 lines
Quoted 3 lines
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

Bin Meng

author

2016-02-06 04:16 UTC #17 lore raw

PATCH 1/1 x86: Clean up SPI flash drivers in defconfig

Hi Jagan, Simon,

Quoted 212 lines · Bin Meng

Hi,
Quoted 209 lines · Jagan Teki
Quoted 207 lines · Bin Meng
Hi Jagan,
Quoted 203 lines · Jagan Teki
Hi Bin,
Quoted 181 lines · Bin Meng
Hi Jagan,
Quoted 166 lines · Jagan Teki
Hi Bin,
Quoted 157 lines · Bin Meng
Hi Jagan, Simon,
Quoted 145 lines · Jagan Teki
Quoted 134 lines · Bin Meng
Hi Jagan,
Quoted 129 lines · Simon Glass
Hi,
Quoted 123 lines · Bin Meng
Hi Jagan,
Quoted 72 lines · Jagan Teki
Hi Bin,
Quoted 65 lines · Bin Meng
Hi Simon,
On Thu, Dec 3, 2015 at 5:05 AM, Simon Glass <[email protected]>
wrote:
+Jagan
Hi Bin,
On 1 December 2015 at 18:41, Bin Meng <[email protected]>
wrote:
Hi Simon,
On Wed, Dec 2, 2015 at 12:32 AM, Simon Glass
<[email protected]>
wrote:
Hi Bin,
On 28 November 2015 at 05:45, Bin Meng <[email protected]>
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 <[email protected]>
---
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.
Quoted 2 lines
Are you talking about flash vendor config or CONFIG_SPI_FLASH?
Flash vendor config, as you see in this patch.
Quoted 31 lines
Quoted 29 lines
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.
Quoted 4 lines
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.
Quoted 7 lines
Quoted 4 lines
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?
Quoted 6 lines
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.
Quoted 3 lines
Can you please suggest which way should we go for this patch? I still
prefer one board with one SPI flash macro.
Quoted 9 lines
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.
Quoted 7 lines
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.

Quoted 8 lines

Quoted 5 lines
Quoted 2 lines
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.

Regards,
Bin

Jagan Teki

2016-02-06 09:57 UTC #18 lore raw

PATCH 1/1 x86: Clean up SPI flash drivers in defconfig

Hi Bin,

Quoted 218 lines · Bin Meng

Hi Jagan, Simon,
Quoted 215 lines · Bin Meng
Hi,
Quoted 212 lines · Jagan Teki
Quoted 209 lines · Bin Meng
Hi Jagan,
Quoted 194 lines · Jagan Teki
Hi Bin,
Quoted 181 lines · Bin Meng
Hi Jagan,
Quoted 166 lines · Jagan Teki
Hi Bin,
Quoted 157 lines · Bin Meng
Hi Jagan, Simon,
Quoted 145 lines · Jagan Teki
Quoted 134 lines · Bin Meng
Hi Jagan,
Quoted 129 lines · Simon Glass
Hi,
Quoted 123 lines · Bin Meng
Hi Jagan,
Quoted 74 lines · Jagan Teki
Hi Bin,
On 3 December 2015 at 10:14, Bin Meng <[email protected]>
wrote:
Hi Simon,
On Thu, Dec 3, 2015 at 5:05 AM, Simon Glass <[email protected]>
wrote:
+Jagan
Hi Bin,
On 1 December 2015 at 18:41, Bin Meng <[email protected]>
wrote:
Hi Simon,
On Wed, Dec 2, 2015 at 12:32 AM, Simon Glass
<[email protected]>
wrote:
Hi Bin,
On 28 November 2015 at 05:45, Bin Meng <[email protected]>
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 <[email protected]>
---
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.
Quoted 35 lines
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.
Quoted 7 lines
Quoted 4 lines
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?
Quoted 6 lines
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.
Quoted 3 lines
Can you please suggest which way should we go for this patch? I still
prefer one board with one SPI flash macro.
Quoted 9 lines
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.
Quoted 7 lines
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.
Quoted 9 lines
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.

Quoted 8 lines

Quoted 5 lines
Quoted 2 lines
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.

Signature · 1 line

Jagan.

Simon Glass

2016-02-12 15:54 UTC #19 lore raw

PATCH 1/1 x86: Clean up SPI flash drivers in defconfig

Hi,

Quoted 227 lines · Jagan Teki

Hi Bin,
Quoted 224 lines · Bin Meng
Hi Jagan, Simon,
Quoted 220 lines · Bin Meng
Hi,
Quoted 209 lines · Jagan Teki
Quoted 207 lines · Bin Meng
Hi Jagan,
Quoted 203 lines · Jagan Teki
Hi Bin,
Quoted 181 lines · Bin Meng
Hi Jagan,
Quoted 166 lines · Jagan Teki
Hi Bin,
Quoted 157 lines · Bin Meng
Hi Jagan, Simon,
Quoted 145 lines · Jagan Teki
Quoted 134 lines · Bin Meng
Hi Jagan,
Quoted 129 lines · Simon Glass
Hi,
Quoted 123 lines · Bin Meng
Hi Jagan,
On Thu, Dec 3, 2015 at 6:24 PM, Jagan Teki <[email protected]>
wrote:
Hi Bin,
On 3 December 2015 at 10:14, Bin Meng <[email protected]>
wrote:
Hi Simon,
On Thu, Dec 3, 2015 at 5:05 AM, Simon Glass <[email protected]>
wrote:
+Jagan
Hi Bin,
On 1 December 2015 at 18:41, Bin Meng <[email protected]>
wrote:
Hi Simon,
On Wed, Dec 2, 2015 at 12:32 AM, Simon Glass
<[email protected]>
wrote:
Hi Bin,
On 28 November 2015 at 05:45, Bin Meng <[email protected]>
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 <[email protected]>
---
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.
Quoted 7 lines
Quoted 4 lines
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?
Quoted 6 lines
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.
Quoted 3 lines
Can you please suggest which way should we go for this patch? I still
prefer one board with one SPI flash macro.
Quoted 9 lines
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.
Quoted 7 lines
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.
Quoted 5 lines
Quoted 2 lines
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?

Quoted 6 lines

Quoted 2 lines
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?

Regards,
Simon

Jagan Teki

2016-02-12 16:31 UTC #20 lore raw

PATCH 1/1 x86: Clean up SPI flash drivers in defconfig

Hi Simon,

Quoted 230 lines · Simon Glass

Hi,
Quoted 227 lines · Jagan Teki
Hi Bin,
Quoted 224 lines · Bin Meng
Hi Jagan, Simon,
Quoted 220 lines · Bin Meng
Hi,
Quoted 209 lines · Jagan Teki
Quoted 207 lines · Bin Meng
Hi Jagan,
Quoted 203 lines · Jagan Teki
Hi Bin,
Quoted 181 lines · Bin Meng
Hi Jagan,
Quoted 166 lines · Jagan Teki
Hi Bin,
Quoted 157 lines · Bin Meng
Hi Jagan, Simon,
Quoted 145 lines · Jagan Teki
Quoted 134 lines · Bin Meng
Hi Jagan,
Quoted 129 lines · Simon Glass
Hi,
On 3 December 2015 at 06:27, Bin Meng <[email protected]> wrote:
Hi Jagan,
On Thu, Dec 3, 2015 at 6:24 PM, Jagan Teki <[email protected]>
wrote:
Hi Bin,
On 3 December 2015 at 10:14, Bin Meng <[email protected]>
wrote:
Hi Simon,
On Thu, Dec 3, 2015 at 5:05 AM, Simon Glass <[email protected]>
wrote:
+Jagan
Hi Bin,
On 1 December 2015 at 18:41, Bin Meng <[email protected]>
wrote:
Hi Simon,
On Wed, Dec 2, 2015 at 12:32 AM, Simon Glass
<[email protected]>
wrote:
Hi Bin,
On 28 November 2015 at 05:45, Bin Meng <[email protected]>
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 <[email protected]>
---
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.
Quoted 7 lines
Quoted 4 lines
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?
Quoted 6 lines
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.
Quoted 3 lines
Can you please suggest which way should we go for this patch? I still
prefer one board with one SPI flash macro.
Quoted 9 lines
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.
Quoted 7 lines
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.
Quoted 5 lines
Quoted 2 lines
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?

Quoted 8 lines

Quoted 6 lines
Quoted 2 lines
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?

Signature · 1 line

Jagan.

Simon Glass

2016-02-12 19:28 UTC #21 lore raw

PATCH 1/1 x86: Clean up SPI flash drivers in defconfig

Hi Jagan,

Quoted 236 lines · Jagan Teki

Hi Simon,
Quoted 233 lines · Simon Glass
Hi,
Quoted 230 lines · Jagan Teki
Hi Bin,
Quoted 226 lines · Bin Meng
Hi Jagan, Simon,
Quoted 212 lines · Bin Meng
Hi,
Quoted 209 lines · Jagan Teki
Quoted 207 lines · Bin Meng
Hi Jagan,
Quoted 203 lines · Jagan Teki
Hi Bin,
Quoted 181 lines · Bin Meng
Hi Jagan,
Quoted 166 lines · Jagan Teki
Hi Bin,
Quoted 157 lines · Bin Meng
Hi Jagan, Simon,
Quoted 145 lines · Jagan Teki
Quoted 141 lines · Bin Meng
Hi Jagan,
On Fri, Dec 4, 2015 at 2:57 AM, Simon Glass <[email protected]>
wrote:
Hi,
On 3 December 2015 at 06:27, Bin Meng <[email protected]> wrote:
Hi Jagan,
On Thu, Dec 3, 2015 at 6:24 PM, Jagan Teki <[email protected]>
wrote:
Hi Bin,
On 3 December 2015 at 10:14, Bin Meng <[email protected]>
wrote:
Hi Simon,
On Thu, Dec 3, 2015 at 5:05 AM, Simon Glass <[email protected]>
wrote:
+Jagan
Hi Bin,
On 1 December 2015 at 18:41, Bin Meng <[email protected]>
wrote:
Hi Simon,
On Wed, Dec 2, 2015 at 12:32 AM, Simon Glass
<[email protected]>
wrote:
Hi Bin,
On 28 November 2015 at 05:45, Bin Meng <[email protected]>
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 <[email protected]>
---
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?
Quoted 6 lines
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.
Quoted 3 lines
Can you please suggest which way should we go for this patch? I still
prefer one board with one SPI flash macro.
Quoted 9 lines
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.
Quoted 7 lines
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.
Quoted 8 lines
Quoted 5 lines
Quoted 2 lines
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.

Quoted 8 lines

Quoted 5 lines
Quoted 3 lines
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

Jagan Teki

2016-02-12 20:12 UTC #22 lore raw

PATCH 1/1 x86: Clean up SPI flash drivers in defconfig

Hi Simon,

Quoted 243 lines · Simon Glass

Hi Jagan,
Quoted 240 lines · Jagan Teki
Hi Simon,
Quoted 237 lines · Simon Glass
Hi,
Quoted 233 lines · Jagan Teki
Hi Bin,
Quoted 218 lines · Bin Meng
Hi Jagan, Simon,
Quoted 215 lines · Bin Meng
Hi,
Quoted 212 lines · Jagan Teki
Quoted 209 lines · Bin Meng
Hi Jagan,
Quoted 194 lines · Jagan Teki
Hi Bin,
Quoted 181 lines · Bin Meng
Hi Jagan,
Quoted 166 lines · Jagan Teki
Hi Bin,
Quoted 157 lines · Bin Meng
Hi Jagan, Simon,
Quoted 151 lines · Jagan Teki
On 8 December 2015 at 17:27, Bin Meng <[email protected]> wrote:
Hi Jagan,
On Fri, Dec 4, 2015 at 2:57 AM, Simon Glass <[email protected]>
wrote:
Hi,
On 3 December 2015 at 06:27, Bin Meng <[email protected]> wrote:
Hi Jagan,
On Thu, Dec 3, 2015 at 6:24 PM, Jagan Teki <[email protected]>
wrote:
Hi Bin,
On 3 December 2015 at 10:14, Bin Meng <[email protected]>
wrote:
Hi Simon,
On Thu, Dec 3, 2015 at 5:05 AM, Simon Glass <[email protected]>
wrote:
+Jagan
Hi Bin,
On 1 December 2015 at 18:41, Bin Meng <[email protected]>
wrote:
Hi Simon,
On Wed, Dec 2, 2015 at 12:32 AM, Simon Glass
<[email protected]>
wrote:
Hi Bin,
On 28 November 2015 at 05:45, Bin Meng <[email protected]>
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 <[email protected]>
---
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.
Quoted 3 lines
Can you please suggest which way should we go for this patch? I still
prefer one board with one SPI flash macro.
Quoted 9 lines
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.
Quoted 7 lines
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.
Quoted 9 lines
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.
Quoted 8 lines
Quoted 5 lines
Quoted 2 lines
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.

Quoted 8 lines

Quoted 4 lines
Quoted 1 line
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!

Signature · 1 line

Jagan.

Simon Glass

2016-02-12 19:31 UTC #23 lore raw

PATCH 1/1 x86: Clean up SPI flash drivers in defconfig

In reply to Jagan Teki

Hi Jagan,

Quoted 15 lines · Jagan Teki

Hi Simon,
Quoted 12 lines · Simon Glass
Hi,
Quoted 9 lines · Jagan Teki
Hi Bin,
Quoted 6 lines · Bin Meng
Hi Jagan, Simon,
Quoted 3 lines · Bin Meng
Hi,
On Tue, Dec 15, 2015 at 2:27 PM, Jagan Teki <[email protected]>

Quoted 5 lines · wrote:

Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines · Bin Meng
Quoted 3 lines
Hi Jagan,
On Tue, Dec 15, 2015 at 2:15 PM, Jagan Teki <[email protected]>

Quoted 13 lines · wrote:

Quoted 13 lines
Quoted 13 lines
Quoted 13 lines
Quoted 13 lines
Quoted 13 lines
Quoted 13 lines
Quoted 13 lines
Hi Bin,
Quoted 9 lines · Bin Meng
Hi Jagan,
Quoted 4 lines · Jagan Teki
Hi Bin,
On 15 December 2015 at 10:48, Bin Meng <[email protected]>

Quoted 8 lines · wrote:

Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Hi Jagan, Simon,
On Tue, Dec 8, 2015 at 11:44 PM, Jagan Teki <[email protected]
Quoted 2 lines · wrote:
On 8 December 2015 at 17:27, Bin Meng <[email protected]>

Quoted 10 lines · wrote:

Quoted 10 lines
Quoted 10 lines
Quoted 10 lines
Quoted 10 lines
Quoted 10 lines
Quoted 10 lines
Quoted 10 lines
Quoted 10 lines
Quoted 10 lines
Quoted 10 lines
Quoted 10 lines
Quoted 10 lines
Hi Jagan,
On Fri, Dec 4, 2015 at 2:57 AM, Simon Glass <[email protected]
wrote:
Hi,
On 3 December 2015 at 06:27, Bin Meng <[email protected]>

Quoted 4 lines · wrote:

Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Hi Jagan,
On Thu, Dec 3, 2015 at 6:24 PM, Jagan Teki <

[email protected]>

Quoted 10 lines

Quoted 10 lines
Quoted 10 lines
Quoted 10 lines
Quoted 10 lines
Quoted 10 lines
Quoted 10 lines
Quoted 10 lines
Quoted 10 lines
Quoted 10 lines
Quoted 10 lines
Quoted 10 lines
Quoted 10 lines
wrote:
Hi Bin,
On 3 December 2015 at 10:14, Bin Meng <[email protected]>
wrote:
Hi Simon,
On Thu, Dec 3, 2015 at 5:05 AM, Simon Glass <

[email protected]>

Quoted 7 lines

Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
wrote:
+Jagan
Hi Bin,
On 1 December 2015 at 18:41, Bin Meng <

[email protected]>

Quoted 5 lines

Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
wrote:
Hi Simon,

Quoted 7 lines

Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
On Wed, Dec 2, 2015 at 12:32 AM, Simon Glass
<[email protected]>
wrote:
Hi Bin,
On 28 November 2015 at 05:45, Bin Meng <

[email protected]>

Quoted 3 lines

Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
wrote:
Every board has one dedicated type of SPI flash,

hence it

Quoted 4 lines

Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
is
unnecessary to include multiple SPI flash drivers.
For QEMU and coreboot (default build of coreboot is

also

Quoted 18 lines

Quoted 18 lines
Quoted 18 lines
Quoted 18 lines
Quoted 18 lines
Quoted 18 lines
Quoted 18 lines
Quoted 18 lines
Quoted 18 lines
Quoted 18 lines
Quoted 18 lines
Quoted 18 lines
Quoted 18 lines
QEMU),
SPI flash is not supported. Remove those SPI flash
drivers.
Signed-off-by: Bin Meng <[email protected]>
---
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(-)

Quoted 1 line

Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
What is the benefit of this? I see it removes a few

lines

Quoted 6 lines

Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
in a
data
table. Does it matter?
Maybe we should ask the other way around, why do we

create

Quoted 3 lines

Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
so
many
flash driver Kconfig option? I believe the intention

was

Quoted 2 lines

Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
footprint.
Besides the footprint issue, having just one flash

driver in

Quoted 8 lines

Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
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?

Quoted 6 lines

Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Flash vendor config, as you see in this patch.
They are a hangover from when we had a separate driver

for

Quoted 10 lines

Quoted 10 lines
Quoted 10 lines
Quoted 10 lines
Quoted 10 lines
Quoted 10 lines
Quoted 10 lines
Quoted 10 lines
Quoted 10 lines
Quoted 10 lines
Quoted 10 lines
Quoted 10 lines
Quoted 10 lines
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

Quoted 1 line

Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
semi-duplicated code, we should not have converted those

flash

Quoted 9 lines

Quoted 9 lines
Quoted 9 lines
Quoted 9 lines
Quoted 9 lines
Quoted 9 lines
Quoted 9 lines
Quoted 9 lines
Quoted 9 lines
Quoted 9 lines
Quoted 9 lines
Quoted 9 lines
Quoted 9 lines
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

Quoted 2 lines

Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
least
SST flash macro should be kept since right now it is

mixed in

Quoted 2 lines

Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
the
generic driver with a special byte program and word

program

Quoted 6 lines

Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
which
is
incompatible with other vendors' flashes.
But there is some flash vendor specific code like quad

enable

Quoted 5 lines

Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
bit,
locking ops and finally about spi_flash_params table.
I know. That's probably why adding all these SPI flash

drivers

Quoted 2 lines

Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
don't
help at all because only one code path will take effect.

And

Quoted 5 lines

Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
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

Quoted 9 lines

Quoted 9 lines
Quoted 9 lines
Quoted 9 lines
Quoted 9 lines
Quoted 9 lines
Quoted 9 lines
Quoted 9 lines
Quoted 9 lines
Quoted 9 lines
Quoted 9 lines
Quoted 9 lines
Quoted 6 lines
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

Quoted 1 line

Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
feature flags may be we can remove those config items and I

have a

Quoted 1 line

Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
plan to re-arrange the sf_param_table which suits Linux may

be I

Quoted 10 lines

Quoted 10 lines
Quoted 10 lines
Quoted 10 lines
Quoted 10 lines
Quoted 10 lines
Quoted 10 lines
Quoted 10 lines
Quoted 10 lines
Quoted 10 lines
Quoted 7 lines
Quoted 3 lines
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

Quoted 3 lines

Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
flash macro? Suppose if board have one controller connected with micro
flash then the board file include CONFIG_SPI_FLASH_STMICRO and

Quoted 1 line

Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
another board having two controllers one connected with

spansion and

Quoted 2 lines

Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
other connected with micro then the board file include CONFIG_SPI_FLASH_STMICRO, CONFIG_SPI_FLASH_SPANSION. It's

entirely up

Quoted 7 lines

Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 2 lines
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,

Quoted 1 line

Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
that we just want to select the specific flash macro for a

specific

Quoted 1 line

Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
x86 board. But Simon wanted to enable all of the flash macros

for one

Quoted 1 line

Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
board for convenience. Thus I came to ask for what's our

direction.

Quoted 3 lines

Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
So, does this board supports or connected all flash variants? in

that

Quoted 2 lines

Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
case
it is true right? "for convenience" here means for testing then

ie also

Quoted 2 lines

Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
true
because this particular board is meant for testing all flash

devices,

Quoted 2 lines

Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
and
also it is up to this particular board config over-head rather

than the

Quoted 7 lines

Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 5 lines
Quoted 2 lines
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.

Quoted 8 lines

Quoted 8 lines
Quoted 8 lines
Quoted 7 lines
Quoted 4 lines
Quoted 1 line
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.

Quoted 8 lines

Quoted 5 lines
Quoted 3 lines
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