[PATCH v3 00/10] ClearFog Base static variant support

[PATCH v3 00/10] ClearFog Base static variant support

[Joel Johnson]

This patch series adds support for ClearFog Base static configuration,
as well as updating and fixing the ClearFog support for MMC and SPI
booting.

Stefan - I think everything is straight forward, with the only known
unresolved question being handling of the duplicated Kconfig entries for
the board-specific overrides. I've laid out the description in that
particular commit, if you have guidance on a preferred approach I'm all
ears.

V2 changes:
  - updated against, and dependent on, https://patchwork.ozlabs.org/cover/1200324
V3 changes:
  - rebased against ClearFog runtime TLV EEPROM changes merged into mvebu/master


Joel Johnson (10):
  arm: mvebu: fix SerDes table alignment
  arm: mvebu: solidrun: remove hardcoded DTS MAC address
  arm: mvebu: clearfog: initial ClearFog Base variant
  arm: mvebu: clearfog: Use Pro DT by default
  arm: mvebu: clearfog: Add SATA mode flags
  arm: mvebu: clearfog: Add option for 2.5 Gbps SFP
  arm: mvebu: clearfog: add SPI offsets
  arm: mvebu: enable working default boot support
  arm: mvebu: clearfog: move ENV params to Kconfig
  arm: mvebu: clearfog: reduce MMC boot assumptions

 .../arm/dts/armada-38x-solidrun-microsom.dtsi |  1 -
 arch/arm/mach-mvebu/Kconfig                   | 13 ++++
 .../serdes/a38x/high_speed_env_spec.c         |  6 +-
 board/solidrun/clearfog/Kconfig               | 62 +++++++++++++++++++
 board/solidrun/clearfog/clearfog.c            | 26 +++++++-
 configs/clearfog_defconfig                    |  5 --
 include/configs/clearfog.h                    |  1 -
 7 files changed, 103 insertions(+), 11 deletions(-)
 create mode 100644 board/solidrun/clearfog/Kconfig

-- 
2.20.1

[PATCH v3 01/10] arm: mvebu: fix SerDes table alignment

[Joel Johnson]

Tested on Solidrun ClearFog Base. Table alignment was:
 | Lane #  | Speed |  Type       |
 --------------------------------
 |   0    |  3   |  SATA0       |
 |   1    |  0   |  SGMII1      |
 |   2    |  3   |  SATA1       |
 |   3    |  5   |  USB3 HOST1  |
 |   4    |  5   |  USB3 HOST0  |
 |   5    |  4   |  SGMII2      |
 --------------------------------

After the change, it's correctly aligned as:
 | Lane # | Speed |  Type       |
 --------------------------------
 |   0    |   3   | SATA0       |
 |   1    |   0   | SGMII1      |
 |   2    |   5   | PCIe1       |
 |   3    |   5   | USB3 HOST1  |
 |   4    |   5   | PCIe2       |
 |   5    |   0   | SGMII2      |
 --------------------------------

Signed-off-by: Joel Johnson <mrjoel@lixil.net>

---

v2 changes
  - none
v3 changes
  - none

---
 arch/arm/mach-mvebu/serdes/a38x/high_speed_env_spec.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/arch/arm/mach-mvebu/serdes/a38x/high_speed_env_spec.c b/arch/arm/mach-mvebu/serdes/a38x/high_speed_env_spec.c
index 33e70569bc..66409a50c0 100644
--- a/arch/arm/mach-mvebu/serdes/a38x/high_speed_env_spec.c
+++ b/arch/arm/mach-mvebu/serdes/a38x/high_speed_env_spec.c
@@ -1366,16 +1366,16 @@ static void print_topology_details(const struct serdes_map *serdes_map,
 
 	DEBUG_INIT_S("board SerDes lanes topology details:\n");
 
-	DEBUG_INIT_S(" | Lane #  | Speed |  Type       |\n");
+	DEBUG_INIT_S(" | Lane # | Speed |  Type       |\n");
 	DEBUG_INIT_S(" --------------------------------\n");
 	for (lane_num = 0; lane_num < count; lane_num++) {
 		if (serdes_map[lane_num].serdes_type == DEFAULT_SERDES)
 			continue;
 		DEBUG_INIT_S(" |   ");
 		DEBUG_INIT_D(hws_get_physical_serdes_num(lane_num), 1);
-		DEBUG_INIT_S("    |  ");
+		DEBUG_INIT_S("    |   ");
 		DEBUG_INIT_D(serdes_map[lane_num].serdes_speed, 2);
-		DEBUG_INIT_S("   |  ");
+		DEBUG_INIT_S("   | ");
 		DEBUG_INIT_S((char *)
 			     serdes_type_to_string[serdes_map[lane_num].
 						   serdes_type]);
-- 
2.20.1

[PATCH v3 02/10] arm: mvebu: solidrun: remove hardcoded DTS MAC address

[Joel Johnson]

Using a consistent hardcoded MAC address from the DTS file causes
issues when using multiple devices on the same network segment.
Instead rely on environment configuration or random generation.

Signed-off-by: Joel Johnson <mrjoel@lixil.net>

---

v2 changes:
  - none
v3 changes:
  - none

---
 arch/arm/dts/armada-38x-solidrun-microsom.dtsi | 1 -
 1 file changed, 1 deletion(-)

diff --git a/arch/arm/dts/armada-38x-solidrun-microsom.dtsi b/arch/arm/dts/armada-38x-solidrun-microsom.dtsi
index a322a28c21..9bbeafc53b 100644
--- a/arch/arm/dts/armada-38x-solidrun-microsom.dtsi
+++ b/arch/arm/dts/armada-38x-solidrun-microsom.dtsi
@@ -39,7 +39,6 @@
 
 &eth0 {
 	/* ethernet at 70000 */
-	mac-address = [00 50 43 02 02 01];
 	pinctrl-0 = <&ge0_rgmii_pins>;
 	pinctrl-names = "default";
 	phy = <&phy_dedicated>;
-- 
2.20.1

[PATCH v3 03/10] arm: mvebu: clearfog: initial ClearFog Base variant

[Joel Johnson]

Add a unique entry for ClearFog Base variant, reflected in the board
name and adjusted SerDes topology.

Signed-off-by: Joel Johnson <mrjoel@lixil.net>

---

v2 changes:
  - reworked based on Baruch's run-time TLV EEPROM detection series
v3 changes:
  - rebased on mvebu merged run-time TLV EEPROM detection series
  - minor update to help test regarding runtime detection failures

---
 arch/arm/mach-mvebu/Kconfig        |  2 ++
 board/solidrun/clearfog/Kconfig    | 18 ++++++++++++++++++
 board/solidrun/clearfog/clearfog.c | 12 ++++++++++++
 3 files changed, 32 insertions(+)
 create mode 100644 board/solidrun/clearfog/Kconfig

diff --git a/arch/arm/mach-mvebu/Kconfig b/arch/arm/mach-mvebu/Kconfig
index bc5eaa5a76..161dee937f 100644
--- a/arch/arm/mach-mvebu/Kconfig
+++ b/arch/arm/mach-mvebu/Kconfig
@@ -280,4 +280,6 @@ config SECURED_MODE_CSK_INDEX
 	default 0
 	depends on SECURED_MODE_IMAGE
 
+source "board/solidrun/clearfog/Kconfig"
+
 endif
diff --git a/board/solidrun/clearfog/Kconfig b/board/solidrun/clearfog/Kconfig
new file mode 100644
index 0000000000..936d5918f8
--- /dev/null
+++ b/board/solidrun/clearfog/Kconfig
@@ -0,0 +1,18 @@
+menu "ClearFog configuration"
+	depends on TARGET_CLEARFOG
+
+config TARGET_CLEARFOG_BASE
+	bool "Use ClearFog Base static configuration"
+	help
+	  Use the ClearFog Base as the static configuration instead of the
+	  default which uses the ClearFog Pro.
+
+	  Runtime board detection is always attempted and used if available. The
+	  static configuration is used as a fallback in cases where runtime
+	  detection is disabled, is not available in hardware, or otherwise fails.
+
+	  Only newer revisions of the ClearFog product line support runtime
+	  detection via additional EEPROM hardware. This option enables selecting
+	  the Base variant for older hardware revisions.
+
+endmenu
diff --git a/board/solidrun/clearfog/clearfog.c b/board/solidrun/clearfog/clearfog.c
index e268ef55a2..e77b9465d4 100644
--- a/board/solidrun/clearfog/clearfog.c
+++ b/board/solidrun/clearfog/clearfog.c
@@ -47,7 +47,11 @@ static struct serdes_map board_serdes_map[] = {
 	{SGMII1, SERDES_SPEED_1_25_GBPS, SERDES_DEFAULT_MODE, 0, 0},
 	{PEX1, SERDES_SPEED_5_GBPS, PEX_ROOT_COMPLEX_X1, 0, 0},
 	{USB3_HOST1, SERDES_SPEED_5_GBPS, SERDES_DEFAULT_MODE, 0, 0},
+#if defined (CONFIG_TARGET_CLEARFOG_BASE)
+	{USB3_HOST0, SERDES_SPEED_5_GBPS, SERDES_DEFAULT_MODE, 0, 0},
+#else
 	{PEX2, SERDES_SPEED_5_GBPS, PEX_ROOT_COMPLEX_X1, 0, 0},
+#endif
 	{SGMII2, SERDES_SPEED_1_25_GBPS, SERDES_DEFAULT_MODE, 0, 0},
 };
 
@@ -170,7 +174,11 @@ int board_init(void)
 
 int checkboard(void)
 {
+#if defined (CONFIG_TARGET_CLEARFOG_BASE)
+	char *board = "ClearFog Base";
+#else
 	char *board = "ClearFog";
+#endif
 
 	cf_read_tlv_data();
 	if (strlen(cf_tlv_data.tlv_product_name[0]) > 0)
@@ -200,6 +208,10 @@ int board_late_init(void)
 		env_set("fdtfile", "armada-385-clearfog-gtr-s4.dtb");
 	else if (sr_product_is(&cf_tlv_data, "Clearfog GTR L8"))
 		env_set("fdtfile", "armada-385-clearfog-gtr-l8.dtb");
+#if defined (CONFIG_TARGET_CLEARFOG_BASE)
+	else
+		 env_set("fdtfile", "armada-388-clearfog-base.dtb");
+#endif
 
 	return 0;
 }
-- 
2.20.1

[PATCH v3 04/10] arm: mvebu: clearfog: Use Pro DT by default

[Joel Johnson]

Switch to explicitly using the Pro variant DT, which has been
available since Linux 4.11. Also unify the location of DT selection
in board_late_init instead of split between detection and static
configuration paths.

---

v2 changes
  - newly added in V2 series based on run-time rebasing
v2 changes
  - none

Signed-off-by: Joel Johnson <mrjoel@lixil.net>
---
 board/solidrun/clearfog/clearfog.c | 6 ++++--
 include/configs/clearfog.h         | 1 -
 2 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/board/solidrun/clearfog/clearfog.c b/board/solidrun/clearfog/clearfog.c
index e77b9465d4..086912e400 100644
--- a/board/solidrun/clearfog/clearfog.c
+++ b/board/solidrun/clearfog/clearfog.c
@@ -177,7 +177,7 @@ int checkboard(void)
 #if defined (CONFIG_TARGET_CLEARFOG_BASE)
 	char *board = "ClearFog Base";
 #else
-	char *board = "ClearFog";
+	char *board = "ClearFog Pro";
 #endif
 
 	cf_read_tlv_data();
@@ -208,9 +208,11 @@ int board_late_init(void)
 		env_set("fdtfile", "armada-385-clearfog-gtr-s4.dtb");
 	else if (sr_product_is(&cf_tlv_data, "Clearfog GTR L8"))
 		env_set("fdtfile", "armada-385-clearfog-gtr-l8.dtb");
-#if defined (CONFIG_TARGET_CLEARFOG_BASE)
 	else
+#if defined (CONFIG_TARGET_CLEARFOG_BASE)
 		 env_set("fdtfile", "armada-388-clearfog-base.dtb");
+#else
+		 env_set("fdtfile", "armada-388-clearfog-pro.dtb");
 #endif
 
 	return 0;
diff --git a/include/configs/clearfog.h b/include/configs/clearfog.h
index 633187d86f..6ca0474461 100644
--- a/include/configs/clearfog.h
+++ b/include/configs/clearfog.h
@@ -134,7 +134,6 @@
 #define CONFIG_EXTRA_ENV_SETTINGS \
 	RELOCATION_LIMITS_ENV_SETTINGS \
 	LOAD_ADDRESS_ENV_SETTINGS \
-	"fdtfile=" CONFIG_DEFAULT_DEVICE_TREE ".dtb\0" \
 	"console=ttyS0,115200\0" \
 	BOOTENV
 
-- 
2.20.1

[PATCH v3 05/10] arm: mvebu: clearfog: Add SATA mode flags

[Joel Johnson]

The mPCIe slots on ClearFog Pro and ClearFog Base may be alternately
configured for SATA usage.

Signed-off-by: Joel Johnson <mrjoel@lixil.net>

---

v2 changes:
  - fixed help indentation
v3 changes:
  - none

---
 board/solidrun/clearfog/Kconfig    | 17 +++++++++++++++++
 board/solidrun/clearfog/clearfog.c |  6 ++++++
 2 files changed, 23 insertions(+)

diff --git a/board/solidrun/clearfog/Kconfig b/board/solidrun/clearfog/Kconfig
index 936d5918f8..4e189b13e0 100644
--- a/board/solidrun/clearfog/Kconfig
+++ b/board/solidrun/clearfog/Kconfig
@@ -15,4 +15,21 @@ config TARGET_CLEARFOG_BASE
 	  detection via additional EEPROM hardware. This option enables selecting
 	  the Base variant for older hardware revisions.
 
+config CLEARFOG_CON3_SATA
+	bool "Use CON3 slot in SATA mode"
+	help
+	  Use the CON3 port with SATA protocol instead of the default PCIe.
+	  The ClearFog port allows usage of either mSATA or miniPCIe
+	  modules, but the desired protocol must be configured at build
+	  time since it affects the SerDes topology layout.
+
+config CLEARFOG_CON2_SATA
+	bool "Use CON2 slot in SATA mode"
+	depends on !TARGET_CLEARFOG_BASE
+	help
+	  Use the CON2 port with SATA protocol instead of the default PCIe.
+	  The ClearFog port allows usage of either mSATA or miniPCIe
+	  modules, but the desired protocol must be configured at build
+	  time since it affects the SerDes topology layout.
+
 endmenu
diff --git a/board/solidrun/clearfog/clearfog.c b/board/solidrun/clearfog/clearfog.c
index 086912e400..7046665d6c 100644
--- a/board/solidrun/clearfog/clearfog.c
+++ b/board/solidrun/clearfog/clearfog.c
@@ -45,10 +45,16 @@ static void cf_read_tlv_data(void)
 static struct serdes_map board_serdes_map[] = {
 	{SATA0, SERDES_SPEED_3_GBPS, SERDES_DEFAULT_MODE, 0, 0},
 	{SGMII1, SERDES_SPEED_1_25_GBPS, SERDES_DEFAULT_MODE, 0, 0},
+#if defined (CONFIG_CLEARFOG_CON3_SATA)
+	{SATA1, SERDES_SPEED_3_GBPS, SERDES_DEFAULT_MODE, 0, 0},
+#else
 	{PEX1, SERDES_SPEED_5_GBPS, PEX_ROOT_COMPLEX_X1, 0, 0},
+#endif
 	{USB3_HOST1, SERDES_SPEED_5_GBPS, SERDES_DEFAULT_MODE, 0, 0},
 #if defined (CONFIG_TARGET_CLEARFOG_BASE)
 	{USB3_HOST0, SERDES_SPEED_5_GBPS, SERDES_DEFAULT_MODE, 0, 0},
+#elif defined(CONFIG_CLEARFOG_CON2_SATA)
+	{SATA2, SERDES_SPEED_3_GBPS, SERDES_DEFAULT_MODE, 0, 0},
 #else
 	{PEX2, SERDES_SPEED_5_GBPS, PEX_ROOT_COMPLEX_X1, 0, 0},
 #endif
-- 
2.20.1

[PATCH v3 06/10] arm: mvebu: clearfog: Add option for 2.5 Gbps SFP

[Joel Johnson]

Signed-off-by: Joel Johnson <mrjoel@lixil.net>

---

v2 changes:
  - fixed help indentation
v3 changes:
  - none

---
 board/solidrun/clearfog/Kconfig    | 7 +++++++
 board/solidrun/clearfog/clearfog.c | 4 ++++
 2 files changed, 11 insertions(+)

diff --git a/board/solidrun/clearfog/Kconfig b/board/solidrun/clearfog/Kconfig
index 4e189b13e0..44224d903d 100644
--- a/board/solidrun/clearfog/Kconfig
+++ b/board/solidrun/clearfog/Kconfig
@@ -32,4 +32,11 @@ config CLEARFOG_CON2_SATA
 	  modules, but the desired protocol must be configured at build
 	  time since it affects the SerDes topology layout.
 
+config CLEARFOG_SFP_25GB
+	bool "Enable 2.5 Gbps mode for SFP"
+	help
+	  Set the SFP module connection to support 2.5 Gbps transfer speed for the
+	  SGMII connection (requires a supporting SFP). By default, transfer speed
+	  of 1.25 Gbps is used, suitable for a more common 1 Gbps SFP module.
+
 endmenu
diff --git a/board/solidrun/clearfog/clearfog.c b/board/solidrun/clearfog/clearfog.c
index 7046665d6c..5064bde378 100644
--- a/board/solidrun/clearfog/clearfog.c
+++ b/board/solidrun/clearfog/clearfog.c
@@ -58,7 +58,11 @@ static struct serdes_map board_serdes_map[] = {
 #else
 	{PEX2, SERDES_SPEED_5_GBPS, PEX_ROOT_COMPLEX_X1, 0, 0},
 #endif
+#if defined (CONFIG_CLEARFOG_SFP_25GB)
+	{SGMII2, SERDES_SPEED_3_125_GBPS, SERDES_DEFAULT_MODE, 0, 0},
+#else
 	{SGMII2, SERDES_SPEED_1_25_GBPS, SERDES_DEFAULT_MODE, 0, 0},
+#endif
 };
 
 int hws_board_topology_load(struct serdes_map **serdes_map_array, u8 *count)
-- 
2.20.1

[PATCH v3 07/10] arm: mvebu: clearfog: add SPI offsets

[Joel Johnson]

Add reasonable default SPI offsets and ENV size when configured to
boot from SPI flash.

Signed-off-by: Joel Johnson <mrjoel@lixil.net>

---

v2 changes:
  - none
v3 changes:
  - none

There was some reasonable concern raised about duplicating config
entries within a board specific config file rather than making
board specific configurations within defconfig. This seems to offer
a more board localized mechanism to provide platform defaults for
core values. As mentioned in the review, this usage seems to match
the Kconfig documented intended usage. As noted at
https://www.kernel.org/doc/html/latest/kbuild/kconfig-language.html:
    "Default values are not limited to the menu entry where
     they are defined. This means the default can be defined
     somewhere else or be overridden by an earlier definition."

Given that there is some dependency variability with these values I
prefer keeping them as Kconfig values, but defer to maintainers.
Notably, for the ENV values in this and a later commit, I'm using a
pattern already in used several other board configs.

---
 board/solidrun/clearfog/Kconfig | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/board/solidrun/clearfog/Kconfig b/board/solidrun/clearfog/Kconfig
index 44224d903d..ea9c419472 100644
--- a/board/solidrun/clearfog/Kconfig
+++ b/board/solidrun/clearfog/Kconfig
@@ -39,4 +39,16 @@ config CLEARFOG_SFP_25GB
 	  SGMII connection (requires a supporting SFP). By default, transfer speed
 	  of 1.25 Gbps is used, suitable for a more common 1 Gbps SFP module.
 
+config ENV_SECT_SIZE
+	hex "Environment Sector-Size"
+	# Use SPI flash erase block size of 4 KiB
+	default 0x1000 if MVEBU_SPL_BOOT_DEVICE_SPI
+	# Use optimistic 64 KiB erase block, will vary between actual media
+	default 0x10000 if MVEBU_SPL_BOOT_DEVICE_MMC
+
+config SYS_SPI_U_BOOT_OFFS
+	hex "address of u-boot payload in SPI flash"
+	default 0x20000
+	depends on MVEBU_SPL_BOOT_DEVICE_SPI
+
 endmenu
-- 
2.20.1

[PATCH v3 08/10] arm: mvebu: enable working default boot support

[Joel Johnson]

With the move to driver model usage, ensure that the required driver
support for SPI and MMC booting is available in SPL.

Tested on SolidRun ClearFog devices.

Signed-off-by: Joel Johnson <mrjoel@lixil.net>
---

v2 changes:
  - change "select" for ENV_IS_IN_X to "imply" to allow disabling the
    default env location and configuring a different one if desired
  - remove SPL_DM_GPIO from defconfig, only include if needed for
    MMC booting
v2 changes:
  - none

---
 arch/arm/mach-mvebu/Kconfig | 10 ++++++++++
 configs/clearfog_defconfig  |  1 -
 2 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/arch/arm/mach-mvebu/Kconfig b/arch/arm/mach-mvebu/Kconfig
index 161dee937f..32191e7157 100644
--- a/arch/arm/mach-mvebu/Kconfig
+++ b/arch/arm/mach-mvebu/Kconfig
@@ -235,9 +235,19 @@ choice
 
 config MVEBU_SPL_BOOT_DEVICE_SPI
 	bool "SPI NOR flash"
+	imply ENV_IS_IN_SPI_FLASH
+	select SPL_DM_SPI
+	select SPL_MTD_SUPPORT
+	select SPL_SPI_FLASH_SUPPORT
+	select SPL_SPI_LOAD
+	select SPL_SPI_SUPPORT
 
 config MVEBU_SPL_BOOT_DEVICE_MMC
 	bool "SDIO/MMC card"
+	imply ENV_IS_IN_MMC
+	# GPIO required for SD card presence detection line
+	select SPL_DM_GPIO
+	select SPL_DM_MMC
 	select SPL_LIBDISK_SUPPORT
 
 config MVEBU_SPL_BOOT_DEVICE_SATA
diff --git a/configs/clearfog_defconfig b/configs/clearfog_defconfig
index 53a51794f0..e2399a9739 100644
--- a/configs/clearfog_defconfig
+++ b/configs/clearfog_defconfig
@@ -27,7 +27,6 @@ CONFIG_SYS_CONSOLE_INFO_QUIET=y
 # CONFIG_DISPLAY_BOARDINFO is not set
 CONFIG_DISPLAY_BOARDINFO_LATE=y
 CONFIG_SYS_MMCSD_RAW_MODE_U_BOOT_DATA_PART_OFFSET=0x1
-CONFIG_SPL_DM_GPIO=y
 CONFIG_SPL_I2C_SUPPORT=y
 CONFIG_CMD_TLV_EEPROM=y
 CONFIG_SPL_CMD_TLV_EEPROM=y
-- 
2.20.1

[PATCH v3 09/10] arm: mvebu: clearfog: move ENV params to Kconfig

[Joel Johnson]

Migrate the values for ENV_SIZE and ENV_OFFSET into board specific
Kconfig defaults so they're more accessible for configuration.

---

v2 changes:
  - none
v3 changes:
  - none

Signed-off-by: Joel Johnson <mrjoel@lixil.net>
---
 board/solidrun/clearfog/Kconfig | 8 ++++++++
 configs/clearfog_defconfig      | 2 --
 2 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/board/solidrun/clearfog/Kconfig b/board/solidrun/clearfog/Kconfig
index ea9c419472..e8c3f53d84 100644
--- a/board/solidrun/clearfog/Kconfig
+++ b/board/solidrun/clearfog/Kconfig
@@ -39,6 +39,14 @@ config CLEARFOG_SFP_25GB
 	  SGMII connection (requires a supporting SFP). By default, transfer speed
 	  of 1.25 Gbps is used, suitable for a more common 1 Gbps SFP module.
 
+config ENV_SIZE
+	hex "Environment Size"
+	default 0x10000
+
+config ENV_OFFSET
+	hex "Environment offset"
+	default 0xF0000
+
 config ENV_SECT_SIZE
 	hex "Environment Sector-Size"
 	# Use SPI flash erase block size of 4 KiB
diff --git a/configs/clearfog_defconfig b/configs/clearfog_defconfig
index e2399a9739..37091319be 100644
--- a/configs/clearfog_defconfig
+++ b/configs/clearfog_defconfig
@@ -11,8 +11,6 @@ CONFIG_TARGET_CLEARFOG=y
 CONFIG_MVEBU_SPL_BOOT_DEVICE_MMC=y
 CONFIG_SPL_MMC_SUPPORT=y
 CONFIG_SPL_SERIAL_SUPPORT=y
-CONFIG_ENV_SIZE=0x10000
-CONFIG_ENV_OFFSET=0xF0000
 CONFIG_NR_DRAM_BANKS=2
 CONFIG_SPL=y
 CONFIG_DEBUG_UART_BASE=0xd0012000
-- 
2.20.1

[PATCH v3 10/10] arm: mvebu: clearfog: reduce MMC boot assumptions

[Joel Johnson]

Reduce those MMC booting assumptions from clearfog_defconfig which
are already selected by dependent options via Kconfig.

Signed-off-by: Joel Johnson <mrjoel@lixil.net>
---

v2 changes:
  - rebased on master to use Baruch's dynamic MMC/SD offset logic
  - update description, will revisit removal of
    CONFIG_MVEBU_SPL_BOOT_DEVICE_MMC in separate future path if a
    more viable option is identified
v3 changes:
  - none

---
 arch/arm/mach-mvebu/Kconfig | 1 +
 configs/clearfog_defconfig  | 2 --
 2 files changed, 1 insertion(+), 2 deletions(-)

diff --git a/arch/arm/mach-mvebu/Kconfig b/arch/arm/mach-mvebu/Kconfig
index 32191e7157..4b381a2936 100644
--- a/arch/arm/mach-mvebu/Kconfig
+++ b/arch/arm/mach-mvebu/Kconfig
@@ -249,6 +249,7 @@ config MVEBU_SPL_BOOT_DEVICE_MMC
 	select SPL_DM_GPIO
 	select SPL_DM_MMC
 	select SPL_LIBDISK_SUPPORT
+	select SPL_MMC_SUPPORT
 
 config MVEBU_SPL_BOOT_DEVICE_SATA
 	bool "SATA"
diff --git a/configs/clearfog_defconfig b/configs/clearfog_defconfig
index 37091319be..4a0df00007 100644
--- a/configs/clearfog_defconfig
+++ b/configs/clearfog_defconfig
@@ -9,7 +9,6 @@ CONFIG_SPL_LIBGENERIC_SUPPORT=y
 CONFIG_SYS_MALLOC_F_LEN=0x2000
 CONFIG_TARGET_CLEARFOG=y
 CONFIG_MVEBU_SPL_BOOT_DEVICE_MMC=y
-CONFIG_SPL_MMC_SUPPORT=y
 CONFIG_SPL_SERIAL_SUPPORT=y
 CONFIG_NR_DRAM_BANKS=2
 CONFIG_SPL=y
@@ -41,7 +40,6 @@ CONFIG_CMD_CACHE=y
 CONFIG_CMD_TIME=y
 # CONFIG_SPL_PARTITION_UUIDS is not set
 CONFIG_DEFAULT_DEVICE_TREE="armada-388-clearfog"
-CONFIG_ENV_IS_IN_MMC=y
 CONFIG_NET_RANDOM_ETHADDR=y
 CONFIG_SPL_OF_TRANSLATE=y
 CONFIG_AHCI_MVEBU=y
-- 
2.20.1

[PATCH v3 03/10] arm: mvebu: clearfog: initial ClearFog Base variant

[Baruch Siach]

Hi Joel,

On Tue, Jan 21, 2020 at 10:32:17AM -0700, Joel Johnson wrote:
> Add a unique entry for ClearFog Base variant, reflected in the board
> name and adjusted SerDes topology.
> 
> Signed-off-by: Joel Johnson <mrjoel@lixil.net>
> 
> ---
> 
> v2 changes:
>   - reworked based on Baruch's run-time TLV EEPROM detection series
> v3 changes:
>   - rebased on mvebu merged run-time TLV EEPROM detection series
>   - minor update to help test regarding runtime detection failures
> 
> ---
>  arch/arm/mach-mvebu/Kconfig        |  2 ++
>  board/solidrun/clearfog/Kconfig    | 18 ++++++++++++++++++
>  board/solidrun/clearfog/clearfog.c | 12 ++++++++++++
>  3 files changed, 32 insertions(+)
>  create mode 100644 board/solidrun/clearfog/Kconfig
> 
> diff --git a/arch/arm/mach-mvebu/Kconfig b/arch/arm/mach-mvebu/Kconfig
> index bc5eaa5a76..161dee937f 100644
> --- a/arch/arm/mach-mvebu/Kconfig
> +++ b/arch/arm/mach-mvebu/Kconfig
> @@ -280,4 +280,6 @@ config SECURED_MODE_CSK_INDEX
>  	default 0
>  	depends on SECURED_MODE_IMAGE
>  
> +source "board/solidrun/clearfog/Kconfig"
> +
>  endif
> diff --git a/board/solidrun/clearfog/Kconfig b/board/solidrun/clearfog/Kconfig
> new file mode 100644
> index 0000000000..936d5918f8
> --- /dev/null
> +++ b/board/solidrun/clearfog/Kconfig
> @@ -0,0 +1,18 @@
> +menu "ClearFog configuration"
> +	depends on TARGET_CLEARFOG
> +
> +config TARGET_CLEARFOG_BASE
> +	bool "Use ClearFog Base static configuration"
> +	help
> +	  Use the ClearFog Base as the static configuration instead of the
> +	  default which uses the ClearFog Pro.
> +
> +	  Runtime board detection is always attempted and used if available. The
> +	  static configuration is used as a fallback in cases where runtime
> +	  detection is disabled, is not available in hardware, or otherwise fails.
> +
> +	  Only newer revisions of the ClearFog product line support runtime
> +	  detection via additional EEPROM hardware. This option enables selecting
> +	  the Base variant for older hardware revisions.
> +
> +endmenu
> diff --git a/board/solidrun/clearfog/clearfog.c b/board/solidrun/clearfog/clearfog.c
> index e268ef55a2..e77b9465d4 100644
> --- a/board/solidrun/clearfog/clearfog.c
> +++ b/board/solidrun/clearfog/clearfog.c
> @@ -47,7 +47,11 @@ static struct serdes_map board_serdes_map[] = {
>  	{SGMII1, SERDES_SPEED_1_25_GBPS, SERDES_DEFAULT_MODE, 0, 0},
>  	{PEX1, SERDES_SPEED_5_GBPS, PEX_ROOT_COMPLEX_X1, 0, 0},
>  	{USB3_HOST1, SERDES_SPEED_5_GBPS, SERDES_DEFAULT_MODE, 0, 0},
> +#if defined (CONFIG_TARGET_CLEARFOG_BASE)
> +	{USB3_HOST0, SERDES_SPEED_5_GBPS, SERDES_DEFAULT_MODE, 0, 0},
> +#else
>  	{PEX2, SERDES_SPEED_5_GBPS, PEX_ROOT_COMPLEX_X1, 0, 0},
> +#endif

I'd prefer run-time test instead of '#ifdefs' that IMO make the code harder to 
read. Something like this (build tested only):

diff --git a/board/solidrun/clearfog/clearfog.c b/board/solidrun/clearfog/clearfog.c
index e268ef55a2a0..202c60cb7841 100644
--- a/board/solidrun/clearfog/clearfog.c
+++ b/board/solidrun/clearfog/clearfog.c
@@ -55,7 +55,8 @@ int hws_board_topology_load(struct serdes_map **serdes_map_array, u8 *count)
 {
 	cf_read_tlv_data();
 
-	if (sr_product_is(&cf_tlv_data, "Clearfog GTR")) {
+	if (sr_product_is(&cf_tlv_data, "Clearfog GTR") ||
+			CONFIG_IS_ENABLED(TARGET_CLEARFOG_BASE)) {
 		board_serdes_map[0].serdes_type = PEX0;
 		board_serdes_map[0].serdes_speed = SERDES_SPEED_5_GBPS;
 		board_serdes_map[0].serdes_mode = PEX_ROOT_COMPLEX_X1;
@@ -172,6 +173,9 @@ int checkboard(void)
 {
 	char *board = "ClearFog";
 
+	if (CONFIG_IS_ENABLED(TARGET_CLEARFOG_BASE))
+		board = "ClearFog Base";
+
 	cf_read_tlv_data();
 	if (strlen(cf_tlv_data.tlv_product_name[0]) > 0)
 		board = cf_tlv_data.tlv_product_name[0];
@@ -194,7 +198,8 @@ int board_late_init(void)
 {
 	cf_read_tlv_data();
 
-	if (sr_product_is(&cf_tlv_data, "Clearfog Base"))
+	if (sr_product_is(&cf_tlv_data, "Clearfog Base") ||
+			CONFIG_IS_ENABLED(TARGET_CLEARFOG_BASE))
 		env_set("fdtfile", "armada-388-clearfog-base.dtb");
 	else if (sr_product_is(&cf_tlv_data, "Clearfog GTR S4"))
 		env_set("fdtfile", "armada-385-clearfog-gtr-s4.dtb");

What do you think?

baruch

-- 
     http://baruch.siach.name/blog/                  ~. .~   Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
   - baruch at tkos.co.il - tel: +972.2.679.5364, http://www.tkos.co.il -

[PATCH v3 03/10] arm: mvebu: clearfog: initial ClearFog Base variant

[Joel Johnson]

On 2020-01-21 10:49, Baruch Siach wrote:
> Hi Joel,
> 
> On Tue, Jan 21, 2020 at 10:32:17AM -0700, Joel Johnson wrote:
>> Add a unique entry for ClearFog Base variant, reflected in the board
>> name and adjusted SerDes topology.
>> 
>> Signed-off-by: Joel Johnson <mrjoel@lixil.net>
>> 
>> ---
>> 
>> v2 changes:
>>   - reworked based on Baruch's run-time TLV EEPROM detection series
>> v3 changes:
>>   - rebased on mvebu merged run-time TLV EEPROM detection series
>>   - minor update to help test regarding runtime detection failures
>> 
>> ---
>>  arch/arm/mach-mvebu/Kconfig        |  2 ++
>>  board/solidrun/clearfog/Kconfig    | 18 ++++++++++++++++++
>>  board/solidrun/clearfog/clearfog.c | 12 ++++++++++++
>>  3 files changed, 32 insertions(+)
>>  create mode 100644 board/solidrun/clearfog/Kconfig
>> 
>> diff --git a/arch/arm/mach-mvebu/Kconfig b/arch/arm/mach-mvebu/Kconfig
>> index bc5eaa5a76..161dee937f 100644
>> --- a/arch/arm/mach-mvebu/Kconfig
>> +++ b/arch/arm/mach-mvebu/Kconfig
>> @@ -280,4 +280,6 @@ config SECURED_MODE_CSK_INDEX
>>  	default 0
>>  	depends on SECURED_MODE_IMAGE
>> 
>> +source "board/solidrun/clearfog/Kconfig"
>> +
>>  endif
>> diff --git a/board/solidrun/clearfog/Kconfig 
>> b/board/solidrun/clearfog/Kconfig
>> new file mode 100644
>> index 0000000000..936d5918f8
>> --- /dev/null
>> +++ b/board/solidrun/clearfog/Kconfig
>> @@ -0,0 +1,18 @@
>> +menu "ClearFog configuration"
>> +	depends on TARGET_CLEARFOG
>> +
>> +config TARGET_CLEARFOG_BASE
>> +	bool "Use ClearFog Base static configuration"
>> +	help
>> +	  Use the ClearFog Base as the static configuration instead of the
>> +	  default which uses the ClearFog Pro.
>> +
>> +	  Runtime board detection is always attempted and used if available. 
>> The
>> +	  static configuration is used as a fallback in cases where runtime
>> +	  detection is disabled, is not available in hardware, or otherwise 
>> fails.
>> +
>> +	  Only newer revisions of the ClearFog product line support runtime
>> +	  detection via additional EEPROM hardware. This option enables 
>> selecting
>> +	  the Base variant for older hardware revisions.
>> +
>> +endmenu
>> diff --git a/board/solidrun/clearfog/clearfog.c 
>> b/board/solidrun/clearfog/clearfog.c
>> index e268ef55a2..e77b9465d4 100644
>> --- a/board/solidrun/clearfog/clearfog.c
>> +++ b/board/solidrun/clearfog/clearfog.c
>> @@ -47,7 +47,11 @@ static struct serdes_map board_serdes_map[] = {
>>  	{SGMII1, SERDES_SPEED_1_25_GBPS, SERDES_DEFAULT_MODE, 0, 0},
>>  	{PEX1, SERDES_SPEED_5_GBPS, PEX_ROOT_COMPLEX_X1, 0, 0},
>>  	{USB3_HOST1, SERDES_SPEED_5_GBPS, SERDES_DEFAULT_MODE, 0, 0},
>> +#if defined (CONFIG_TARGET_CLEARFOG_BASE)
>> +	{USB3_HOST0, SERDES_SPEED_5_GBPS, SERDES_DEFAULT_MODE, 0, 0},
>> +#else
>>  	{PEX2, SERDES_SPEED_5_GBPS, PEX_ROOT_COMPLEX_X1, 0, 0},
>> +#endif
> 
> I'd prefer run-time test instead of '#ifdefs' that IMO make the code 
> harder to
> read. Something like this (build tested only):
> 
> diff --git a/board/solidrun/clearfog/clearfog.c
> b/board/solidrun/clearfog/clearfog.c
> index e268ef55a2a0..202c60cb7841 100644
> --- a/board/solidrun/clearfog/clearfog.c
> +++ b/board/solidrun/clearfog/clearfog.c
> @@ -55,7 +55,8 @@ int hws_board_topology_load(struct serdes_map
> **serdes_map_array, u8 *count)
>  {
>  	cf_read_tlv_data();
> 
> -	if (sr_product_is(&cf_tlv_data, "Clearfog GTR")) {
> +	if (sr_product_is(&cf_tlv_data, "Clearfog GTR") ||
> +			CONFIG_IS_ENABLED(TARGET_CLEARFOG_BASE)) {
>  		board_serdes_map[0].serdes_type = PEX0;
>  		board_serdes_map[0].serdes_speed = SERDES_SPEED_5_GBPS;
>  		board_serdes_map[0].serdes_mode = PEX_ROOT_COMPLEX_X1;
> @@ -172,6 +173,9 @@ int checkboard(void)
>  {
>  	char *board = "ClearFog";
> 
> +	if (CONFIG_IS_ENABLED(TARGET_CLEARFOG_BASE))
> +		board = "ClearFog Base";
> +
>  	cf_read_tlv_data();
>  	if (strlen(cf_tlv_data.tlv_product_name[0]) > 0)
>  		board = cf_tlv_data.tlv_product_name[0];
> @@ -194,7 +198,8 @@ int board_late_init(void)
>  {
>  	cf_read_tlv_data();
> 
> -	if (sr_product_is(&cf_tlv_data, "Clearfog Base"))
> +	if (sr_product_is(&cf_tlv_data, "Clearfog Base") ||
> +			CONFIG_IS_ENABLED(TARGET_CLEARFOG_BASE))
>  		env_set("fdtfile", "armada-388-clearfog-base.dtb");
>  	else if (sr_product_is(&cf_tlv_data, "Clearfog GTR S4"))
>  		env_set("fdtfile", "armada-385-clearfog-gtr-s4.dtb");
> 
> What do you think?
> 
> baruch

I'll have to revisit and test to be sure, but I believe the reason I 
didn't go that route is because I wasn't able to get the 
CONFIG_IS_ENABLED macro version to trigger when building SPL, although 
it worked for the main path. I know that was the case for something I 
was working up, but I don't recall whether it was this patch 
specifically or not.

A bit of a nit, but just using the macro isn't really runtime detection, 
however adding the separate code path for naming and serdes manipulation 
dynamically tends that way. I'm already running into several cases (not 
the default build options) where the SPL doesn't fit in the default 128K 
offset size, so I'm leery of adding paths that add actual binary size 
increase. I'm all for switching to CONFIG_IS_ENABLED if I test and can 
get it to work, but still would lean towards just replacing the ifdef in 
place; after all it is meant to be the static configuration, not 
dynamic.

Joel

[PATCH v3 03/10] arm: mvebu: clearfog: initial ClearFog Base variant

[Baruch Siach]

Hi Joel,

On Wed, Jan 22 2020, Joel Johnson wrote:
> On 2020-01-21 10:49, Baruch Siach wrote:
>> On Tue, Jan 21, 2020 at 10:32:17AM -0700, Joel Johnson wrote:
>>> Add a unique entry for ClearFog Base variant, reflected in the board
>>> name and adjusted SerDes topology.
>>>
>>> Signed-off-by: Joel Johnson <mrjoel@lixil.net>
>>>
>>> ---
>>>
>>> v2 changes:
>>>   - reworked based on Baruch's run-time TLV EEPROM detection series
>>> v3 changes:
>>>   - rebased on mvebu merged run-time TLV EEPROM detection series
>>>   - minor update to help test regarding runtime detection failures
>>>
>>> ---
>>>  arch/arm/mach-mvebu/Kconfig        |  2 ++
>>>  board/solidrun/clearfog/Kconfig    | 18 ++++++++++++++++++
>>>  board/solidrun/clearfog/clearfog.c | 12 ++++++++++++
>>>  3 files changed, 32 insertions(+)
>>>  create mode 100644 board/solidrun/clearfog/Kconfig
>>>
>>> diff --git a/arch/arm/mach-mvebu/Kconfig b/arch/arm/mach-mvebu/Kconfig
>>> index bc5eaa5a76..161dee937f 100644
>>> --- a/arch/arm/mach-mvebu/Kconfig
>>> +++ b/arch/arm/mach-mvebu/Kconfig
>>> @@ -280,4 +280,6 @@ config SECURED_MODE_CSK_INDEX
>>>  	default 0
>>>  	depends on SECURED_MODE_IMAGE
>>>
>>> +source "board/solidrun/clearfog/Kconfig"
>>> +
>>>  endif
>>> diff --git a/board/solidrun/clearfog/Kconfig
>>> b/board/solidrun/clearfog/Kconfig
>>> new file mode 100644
>>> index 0000000000..936d5918f8
>>> --- /dev/null
>>> +++ b/board/solidrun/clearfog/Kconfig
>>> @@ -0,0 +1,18 @@
>>> +menu "ClearFog configuration"
>>> +	depends on TARGET_CLEARFOG
>>> +
>>> +config TARGET_CLEARFOG_BASE
>>> +	bool "Use ClearFog Base static configuration"
>>> +	help
>>> +	  Use the ClearFog Base as the static configuration instead of the
>>> +	  default which uses the ClearFog Pro.
>>> +
>>> +	  Runtime board detection is always attempted and used if
>>> available. The
>>> +	  static configuration is used as a fallback in cases where runtime
>>> +	  detection is disabled, is not available in hardware, or otherwise
>>> fails.
>>> +
>>> +	  Only newer revisions of the ClearFog product line support runtime
>>> +	  detection via additional EEPROM hardware. This option enables
>>> selecting
>>> +	  the Base variant for older hardware revisions.
>>> +
>>> +endmenu
>>> diff --git a/board/solidrun/clearfog/clearfog.c
>>> b/board/solidrun/clearfog/clearfog.c
>>> index e268ef55a2..e77b9465d4 100644
>>> --- a/board/solidrun/clearfog/clearfog.c
>>> +++ b/board/solidrun/clearfog/clearfog.c
>>> @@ -47,7 +47,11 @@ static struct serdes_map board_serdes_map[] = {
>>>  	{SGMII1, SERDES_SPEED_1_25_GBPS, SERDES_DEFAULT_MODE, 0, 0},
>>>  	{PEX1, SERDES_SPEED_5_GBPS, PEX_ROOT_COMPLEX_X1, 0, 0},
>>>  	{USB3_HOST1, SERDES_SPEED_5_GBPS, SERDES_DEFAULT_MODE, 0, 0},
>>> +#if defined (CONFIG_TARGET_CLEARFOG_BASE)
>>> +	{USB3_HOST0, SERDES_SPEED_5_GBPS, SERDES_DEFAULT_MODE, 0, 0},
>>> +#else
>>>  	{PEX2, SERDES_SPEED_5_GBPS, PEX_ROOT_COMPLEX_X1, 0, 0},
>>> +#endif
>>
>> I'd prefer run-time test instead of '#ifdefs' that IMO make the code harder
>> to
>> read. Something like this (build tested only):
>>
>> diff --git a/board/solidrun/clearfog/clearfog.c
>> b/board/solidrun/clearfog/clearfog.c
>> index e268ef55a2a0..202c60cb7841 100644
>> --- a/board/solidrun/clearfog/clearfog.c
>> +++ b/board/solidrun/clearfog/clearfog.c
>> @@ -55,7 +55,8 @@ int hws_board_topology_load(struct serdes_map
>> **serdes_map_array, u8 *count)
>>  {
>>  	cf_read_tlv_data();
>>
>> -	if (sr_product_is(&cf_tlv_data, "Clearfog GTR")) {
>> +	if (sr_product_is(&cf_tlv_data, "Clearfog GTR") ||
>> +			CONFIG_IS_ENABLED(TARGET_CLEARFOG_BASE)) {
>>  		board_serdes_map[0].serdes_type = PEX0;
>>  		board_serdes_map[0].serdes_speed = SERDES_SPEED_5_GBPS;
>>  		board_serdes_map[0].serdes_mode = PEX_ROOT_COMPLEX_X1;
>> @@ -172,6 +173,9 @@ int checkboard(void)
>>  {
>>  	char *board = "ClearFog";
>>
>> +	if (CONFIG_IS_ENABLED(TARGET_CLEARFOG_BASE))
>> +		board = "ClearFog Base";
>> +
>>  	cf_read_tlv_data();
>>  	if (strlen(cf_tlv_data.tlv_product_name[0]) > 0)
>>  		board = cf_tlv_data.tlv_product_name[0];
>> @@ -194,7 +198,8 @@ int board_late_init(void)
>>  {
>>  	cf_read_tlv_data();
>>
>> -	if (sr_product_is(&cf_tlv_data, "Clearfog Base"))
>> +	if (sr_product_is(&cf_tlv_data, "Clearfog Base") ||
>> +			CONFIG_IS_ENABLED(TARGET_CLEARFOG_BASE))
>>  		env_set("fdtfile", "armada-388-clearfog-base.dtb");
>>  	else if (sr_product_is(&cf_tlv_data, "Clearfog GTR S4"))
>>  		env_set("fdtfile", "armada-385-clearfog-gtr-s4.dtb");
>>
>> What do you think?
>>
>> baruch
>
> I'll have to revisit and test to be sure, but I believe the reason I didn't go
> that route is because I wasn't able to get the CONFIG_IS_ENABLED macro version
> to trigger when building SPL, although it worked for the main path. I know
> that was the case for something I was working up, but I don't recall whether
> it was this patch specifically or not.

Right. We need the IS_ENABLED() macro here. For CONFIG_IS_ENABLED() we
would need another SPL_FOO config symbol. See include/linux/kconfig.h.

> A bit of a nit, but just using the macro isn't really runtime detection,
> however adding the separate code path for naming and serdes manipulation
> dynamically tends that way. I'm already running into several cases (not the
> default build options) where the SPL doesn't fit in the default 128K offset
> size, so I'm leery of adding paths that add actual binary size increase. I'm
> all for switching to CONFIG_IS_ENABLED if I test and can get it to work, but
> still would lean towards just replacing the ifdef in place; after all it is
> meant to be the static configuration, not dynamic.

It probably makes more sense to reverse the order of ORed conditions:

	if (IS_ENABLED(TARGET_CLEARFOG_BASE) ||
        		sr_product_is(&cf_tlv_data, "Clearfog Base"))

IS_ENABLED() is evaluated at build time. When it is true, the compiler
drops all other 'if' branches, thus saving space. That also means that
the build time configuration overrides the EEPROM set value, which is a
Good Thing I believe.

I would really like to avoid additional #ifdefs if possible.

baruch

--
     http://baruch.siach.name/blog/                  ~. .~   Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
   - baruch at tkos.co.il - tel: +972.52.368.4656, http://www.tkos.co.il -

[PATCH v3 03/10] arm: mvebu: clearfog: initial ClearFog Base variant

[Joel Johnson]

On January 22, 2020 10:32:58 AM UTC, Baruch Siach <baruch@tkos.co.il> wrote:
>Hi Joel,
>
>On Wed, Jan 22 2020, Joel Johnson wrote:
>> On 2020-01-21 10:49, Baruch Siach wrote:
>>> On Tue, Jan 21, 2020 at 10:32:17AM -0700, Joel Johnson wrote:
>>>> Add a unique entry for ClearFog Base variant, reflected in the
>board
>>>> name and adjusted SerDes topology.
>>>>
>>>> Signed-off-by: Joel Johnson <mrjoel@lixil.net>
>>>>
>>>> ---
>>>>
>>>> v2 changes:
>>>>   - reworked based on Baruch's run-time TLV EEPROM detection series
>>>> v3 changes:
>>>>   - rebased on mvebu merged run-time TLV EEPROM detection series
>>>>   - minor update to help test regarding runtime detection failures
>>>>
>>>> ---
>>>>  arch/arm/mach-mvebu/Kconfig        |  2 ++
>>>>  board/solidrun/clearfog/Kconfig    | 18 ++++++++++++++++++
>>>>  board/solidrun/clearfog/clearfog.c | 12 ++++++++++++
>>>>  3 files changed, 32 insertions(+)
>>>>  create mode 100644 board/solidrun/clearfog/Kconfig
>>>>
>>>> diff --git a/arch/arm/mach-mvebu/Kconfig
>b/arch/arm/mach-mvebu/Kconfig
>>>> index bc5eaa5a76..161dee937f 100644
>>>> --- a/arch/arm/mach-mvebu/Kconfig
>>>> +++ b/arch/arm/mach-mvebu/Kconfig
>>>> @@ -280,4 +280,6 @@ config SECURED_MODE_CSK_INDEX
>>>>  	default 0
>>>>  	depends on SECURED_MODE_IMAGE
>>>>
>>>> +source "board/solidrun/clearfog/Kconfig"
>>>> +
>>>>  endif
>>>> diff --git a/board/solidrun/clearfog/Kconfig
>>>> b/board/solidrun/clearfog/Kconfig
>>>> new file mode 100644
>>>> index 0000000000..936d5918f8
>>>> --- /dev/null
>>>> +++ b/board/solidrun/clearfog/Kconfig
>>>> @@ -0,0 +1,18 @@
>>>> +menu "ClearFog configuration"
>>>> +	depends on TARGET_CLEARFOG
>>>> +
>>>> +config TARGET_CLEARFOG_BASE
>>>> +	bool "Use ClearFog Base static configuration"
>>>> +	help
>>>> +	  Use the ClearFog Base as the static configuration instead of
>the
>>>> +	  default which uses the ClearFog Pro.
>>>> +
>>>> +	  Runtime board detection is always attempted and used if
>>>> available. The
>>>> +	  static configuration is used as a fallback in cases where
>runtime
>>>> +	  detection is disabled, is not available in hardware, or
>otherwise
>>>> fails.
>>>> +
>>>> +	  Only newer revisions of the ClearFog product line support
>runtime
>>>> +	  detection via additional EEPROM hardware. This option enables
>>>> selecting
>>>> +	  the Base variant for older hardware revisions.
>>>> +
>>>> +endmenu
>>>> diff --git a/board/solidrun/clearfog/clearfog.c
>>>> b/board/solidrun/clearfog/clearfog.c
>>>> index e268ef55a2..e77b9465d4 100644
>>>> --- a/board/solidrun/clearfog/clearfog.c
>>>> +++ b/board/solidrun/clearfog/clearfog.c
>>>> @@ -47,7 +47,11 @@ static struct serdes_map board_serdes_map[] = {
>>>>  	{SGMII1, SERDES_SPEED_1_25_GBPS, SERDES_DEFAULT_MODE, 0, 0},
>>>>  	{PEX1, SERDES_SPEED_5_GBPS, PEX_ROOT_COMPLEX_X1, 0, 0},
>>>>  	{USB3_HOST1, SERDES_SPEED_5_GBPS, SERDES_DEFAULT_MODE, 0, 0},
>>>> +#if defined (CONFIG_TARGET_CLEARFOG_BASE)
>>>> +	{USB3_HOST0, SERDES_SPEED_5_GBPS, SERDES_DEFAULT_MODE, 0, 0},
>>>> +#else
>>>>  	{PEX2, SERDES_SPEED_5_GBPS, PEX_ROOT_COMPLEX_X1, 0, 0},
>>>> +#endif
>>>
>>> I'd prefer run-time test instead of '#ifdefs' that IMO make the code
>harder
>>> to
>>> read. Something like this (build tested only):
>>>
>>> diff --git a/board/solidrun/clearfog/clearfog.c
>>> b/board/solidrun/clearfog/clearfog.c
>>> index e268ef55a2a0..202c60cb7841 100644
>>> --- a/board/solidrun/clearfog/clearfog.c
>>> +++ b/board/solidrun/clearfog/clearfog.c
>>> @@ -55,7 +55,8 @@ int hws_board_topology_load(struct serdes_map
>>> **serdes_map_array, u8 *count)
>>>  {
>>>  	cf_read_tlv_data();
>>>
>>> -	if (sr_product_is(&cf_tlv_data, "Clearfog GTR")) {
>>> +	if (sr_product_is(&cf_tlv_data, "Clearfog GTR") ||
>>> +			CONFIG_IS_ENABLED(TARGET_CLEARFOG_BASE)) {
>>>  		board_serdes_map[0].serdes_type = PEX0;
>>>  		board_serdes_map[0].serdes_speed = SERDES_SPEED_5_GBPS;
>>>  		board_serdes_map[0].serdes_mode = PEX_ROOT_COMPLEX_X1;
>>> @@ -172,6 +173,9 @@ int checkboard(void)
>>>  {
>>>  	char *board = "ClearFog";
>>>
>>> +	if (CONFIG_IS_ENABLED(TARGET_CLEARFOG_BASE))
>>> +		board = "ClearFog Base";
>>> +
>>>  	cf_read_tlv_data();
>>>  	if (strlen(cf_tlv_data.tlv_product_name[0]) > 0)
>>>  		board = cf_tlv_data.tlv_product_name[0];
>>> @@ -194,7 +198,8 @@ int board_late_init(void)
>>>  {
>>>  	cf_read_tlv_data();
>>>
>>> -	if (sr_product_is(&cf_tlv_data, "Clearfog Base"))
>>> +	if (sr_product_is(&cf_tlv_data, "Clearfog Base") ||
>>> +			CONFIG_IS_ENABLED(TARGET_CLEARFOG_BASE))
>>>  		env_set("fdtfile", "armada-388-clearfog-base.dtb");
>>>  	else if (sr_product_is(&cf_tlv_data, "Clearfog GTR S4"))
>>>  		env_set("fdtfile", "armada-385-clearfog-gtr-s4.dtb");
>>>
>>> What do you think?
>>>
>>> baruch
>>
>> I'll have to revisit and test to be sure, but I believe the reason I
>didn't go
>> that route is because I wasn't able to get the CONFIG_IS_ENABLED
>macro version
>> to trigger when building SPL, although it worked for the main path. I
>know
>> that was the case for something I was working up, but I don't recall
>whether
>> it was this patch specifically or not.
>
>Right. We need the IS_ENABLED() macro here. For CONFIG_IS_ENABLED() we
>would need another SPL_FOO config symbol. See include/linux/kconfig.h.
>
>> A bit of a nit, but just using the macro isn't really runtime
>detection,
>> however adding the separate code path for naming and serdes
>manipulation
>> dynamically tends that way. I'm already running into several cases
>(not the
>> default build options) where the SPL doesn't fit in the default 128K
>offset
>> size, so I'm leery of adding paths that add actual binary size
>increase. I'm
>> all for switching to CONFIG_IS_ENABLED if I test and can get it to
>work, but
>> still would lean towards just replacing the ifdef in place; after all
>it is
>> meant to be the static configuration, not dynamic.
>
>It probably makes more sense to reverse the order of ORed conditions:
>
>	if (IS_ENABLED(TARGET_CLEARFOG_BASE) ||
>        		sr_product_is(&cf_tlv_data, "Clearfog Base"))
>
>IS_ENABLED() is evaluated at build time. When it is true, the compiler
>drops all other 'if' branches, thus saving space. That also means that
>the build time configuration overrides the EEPROM set value, which is a
>Good Thing I believe.

I'll take a look and do testing and size comparison in the next day or two.

Note that I intended (and wrote the Base target help docs accordingly) that runtime detection, if both enabled and supported in hardware, should be preferred to the static configuration, with static config being only a fallback. This seems to be different from what your thought about it being good for build configuration to override EEPROM detected values, and I'm curious as to your reasoning.

There are a few gaps here related to what you point out (e.g. booting on a Pro with EEPROM and Base static config won't give the expected results). Relocating the static adjustment may be fine and help with that case as well.

I'll want to add support in such handling for the EEPROM Pro devices but don't have one available. You seem to have them available, can you confirm that "Clearfog Pro" would match those devices using sr_product_is?

Thanks,
Joel

[PATCH v3 03/10] arm: mvebu: clearfog: initial ClearFog Base variant

[Stefan Roese]

On 22.01.20 20:28, Joel Johnson wrote:

<snip>

>> It probably makes more sense to reverse the order of ORed conditions:
>>
>> 	if (IS_ENABLED(TARGET_CLEARFOG_BASE) ||
>>         		sr_product_is(&cf_tlv_data, "Clearfog Base"))
>>
>> IS_ENABLED() is evaluated at build time. When it is true, the compiler
>> drops all other 'if' branches, thus saving space. That also means that
>> the build time configuration overrides the EEPROM set value, which is a
>> Good Thing I believe.
> 
> I'll take a look and do testing and size comparison in the next day or
> two.
> 
> Note that I intended (and wrote the Base target help docs accordingly)
> that runtime detection, if both enabled and supported in hardware,
> should be preferred to the static configuration, with static config
> being only a fallback.

This sounds reasonable.

> This seems to be different from what your
> thought about it being good for build configuration to override
> EEPROM detected values, and I'm curious as to your reasoning.
> 
> There are a few gaps here related to what you point out (e.g.
> booting on a Pro with EEPROM and Base static config won't give the
> expected results). Relocating the static adjustment may be fine and
> help with that case as well.
> 
> I'll want to add support in such handling for the EEPROM Pro
> devices but don't have one available. You seem to have them
> available, can you confirm that "Clearfog Pro" would match those
> devices using sr_product_is?

I currently don't have any of these boards available, so I also would
like to see some reviewed-by and even better tested-by comments from
Baruch (or someone else at SolidRun).

Thanks,
Stefan

[PATCH v3 04/10] arm: mvebu: clearfog: Use Pro DT by default

[Baruch Siach]

Hi Joel,

On Tue, Jan 21, 2020 at 10:32:18AM -0700, Joel Johnson wrote:
> Switch to explicitly using the Pro variant DT, which has been
> available since Linux 4.11. Also unify the location of DT selection
> in board_late_init instead of split between detection and static
> configuration paths.
> 
> ---
> 
> v2 changes
>   - newly added in V2 series based on run-time rebasing
> v2 changes
>   - none
> 
> Signed-off-by: Joel Johnson <mrjoel@lixil.net>
> ---
>  board/solidrun/clearfog/clearfog.c | 6 ++++--
>  include/configs/clearfog.h         | 1 -
>  2 files changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/board/solidrun/clearfog/clearfog.c b/board/solidrun/clearfog/clearfog.c
> index e77b9465d4..086912e400 100644
> --- a/board/solidrun/clearfog/clearfog.c
> +++ b/board/solidrun/clearfog/clearfog.c
> @@ -177,7 +177,7 @@ int checkboard(void)
>  #if defined (CONFIG_TARGET_CLEARFOG_BASE)
>  	char *board = "ClearFog Base";
>  #else
> -	char *board = "ClearFog";
> +	char *board = "ClearFog Pro";
>  #endif
>  
>  	cf_read_tlv_data();
> @@ -208,9 +208,11 @@ int board_late_init(void)
>  		env_set("fdtfile", "armada-385-clearfog-gtr-s4.dtb");
>  	else if (sr_product_is(&cf_tlv_data, "Clearfog GTR L8"))
>  		env_set("fdtfile", "armada-385-clearfog-gtr-l8.dtb");
> -#if defined (CONFIG_TARGET_CLEARFOG_BASE)
>  	else
> +#if defined (CONFIG_TARGET_CLEARFOG_BASE)
>  		 env_set("fdtfile", "armada-388-clearfog-base.dtb");
> +#else
> +		 env_set("fdtfile", "armada-388-clearfog-pro.dtb");

I'd prefer to keep the default here for backwards compatibility. The kernel 
also keeps armada-388-clearfog.dtb. The -pro variant only updates the 'model' 
and 'compatible' properties.

Unrelated to that here is another reason to avoid #ifdef. Syntax aware text 
editor automatic indentation might fail to see that the second env_set() is in 
the 'else' block. We can workaround that by adding braces around the block, 
but it's not nice.

baruch

>  #endif
>  
>  	return 0;
> diff --git a/include/configs/clearfog.h b/include/configs/clearfog.h
> index 633187d86f..6ca0474461 100644
> --- a/include/configs/clearfog.h
> +++ b/include/configs/clearfog.h
> @@ -134,7 +134,6 @@
>  #define CONFIG_EXTRA_ENV_SETTINGS \
>  	RELOCATION_LIMITS_ENV_SETTINGS \
>  	LOAD_ADDRESS_ENV_SETTINGS \
> -	"fdtfile=" CONFIG_DEFAULT_DEVICE_TREE ".dtb\0" \
>  	"console=ttyS0,115200\0" \
>  	BOOTENV

-- 
     http://baruch.siach.name/blog/                  ~. .~   Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
   - baruch at tkos.co.il - tel: +972.2.679.5364, http://www.tkos.co.il -

[PATCH v3 05/10] arm: mvebu: clearfog: Add SATA mode flags

[Baruch Siach]

Hi Joel,

On Tue, Jan 21, 2020 at 10:32:19AM -0700, Joel Johnson wrote:
> The mPCIe slots on ClearFog Pro and ClearFog Base may be alternately
> configured for SATA usage.
> 
> Signed-off-by: Joel Johnson <mrjoel@lixil.net>
> 
> ---
> 
> v2 changes:
>   - fixed help indentation
> v3 changes:
>   - none
> 
> ---
>  board/solidrun/clearfog/Kconfig    | 17 +++++++++++++++++
>  board/solidrun/clearfog/clearfog.c |  6 ++++++
>  2 files changed, 23 insertions(+)
> 
> diff --git a/board/solidrun/clearfog/Kconfig b/board/solidrun/clearfog/Kconfig
> index 936d5918f8..4e189b13e0 100644
> --- a/board/solidrun/clearfog/Kconfig
> +++ b/board/solidrun/clearfog/Kconfig
> @@ -15,4 +15,21 @@ config TARGET_CLEARFOG_BASE
>  	  detection via additional EEPROM hardware. This option enables selecting
>  	  the Base variant for older hardware revisions.
>  
> +config CLEARFOG_CON3_SATA
> +	bool "Use CON3 slot in SATA mode"
> +	help
> +	  Use the CON3 port with SATA protocol instead of the default PCIe.
> +	  The ClearFog port allows usage of either mSATA or miniPCIe
> +	  modules, but the desired protocol must be configured at build
> +	  time since it affects the SerDes topology layout.
> +
> +config CLEARFOG_CON2_SATA
> +	bool "Use CON2 slot in SATA mode"
> +	depends on !TARGET_CLEARFOG_BASE
> +	help
> +	  Use the CON2 port with SATA protocol instead of the default PCIe.
> +	  The ClearFog port allows usage of either mSATA or miniPCIe
> +	  modules, but the desired protocol must be configured at build
> +	  time since it affects the SerDes topology layout.
> +
>  endmenu
> diff --git a/board/solidrun/clearfog/clearfog.c b/board/solidrun/clearfog/clearfog.c
> index 086912e400..7046665d6c 100644
> --- a/board/solidrun/clearfog/clearfog.c
> +++ b/board/solidrun/clearfog/clearfog.c
> @@ -45,10 +45,16 @@ static void cf_read_tlv_data(void)
>  static struct serdes_map board_serdes_map[] = {
>  	{SATA0, SERDES_SPEED_3_GBPS, SERDES_DEFAULT_MODE, 0, 0},
>  	{SGMII1, SERDES_SPEED_1_25_GBPS, SERDES_DEFAULT_MODE, 0, 0},
> +#if defined (CONFIG_CLEARFOG_CON3_SATA)
> +	{SATA1, SERDES_SPEED_3_GBPS, SERDES_DEFAULT_MODE, 0, 0},

Have you tested mSATA with this patch?

My testing showed that swap_rx must be set for mSATA. See this pull request:

  https://github.com/SolidRun/u-boot/pull/3

To make #ifdef less annoying I would prefer something like this instead (build 
tested only):

diff --git a/board/solidrun/clearfog/clearfog.c b/board/solidrun/clearfog/clearfog.c
index e268ef55a2a0..5bbb7906b681 100644
--- a/board/solidrun/clearfog/clearfog.c
+++ b/board/solidrun/clearfog/clearfog.c
@@ -29,6 +29,12 @@ DECLARE_GLOBAL_DATA_PTR;
 #define BOARD_GPP_POL_LOW	0x0
 #define BOARD_GPP_POL_MID	0x0
 
+#if defined (CONFIG_CLEARFOG_CON3_SATA)
+#define SERDES2_CONFIG {SATA1, SERDES_SPEED_3_GBPS, SERDES_DEFAULT_MODE, 1, 0}
+#else
+#define SERDES2_CONFIG {PEX1, SERDES_SPEED_5_GBPS, PEX_ROOT_COMPLEX_X1, 0, 0}
+#endif
+
 static struct tlv_data cf_tlv_data;
 
 static void cf_read_tlv_data(void)
@@ -45,7 +51,7 @@ static void cf_read_tlv_data(void)
 static struct serdes_map board_serdes_map[] = {
 	{SATA0, SERDES_SPEED_3_GBPS, SERDES_DEFAULT_MODE, 0, 0},
 	{SGMII1, SERDES_SPEED_1_25_GBPS, SERDES_DEFAULT_MODE, 0, 0},
-	{PEX1, SERDES_SPEED_5_GBPS, PEX_ROOT_COMPLEX_X1, 0, 0},
+	SERDES2_CONFIG,
 	{USB3_HOST1, SERDES_SPEED_5_GBPS, SERDES_DEFAULT_MODE, 0, 0},
 	{PEX2, SERDES_SPEED_5_GBPS, PEX_ROOT_COMPLEX_X1, 0, 0},
 	{SGMII2, SERDES_SPEED_1_25_GBPS, SERDES_DEFAULT_MODE, 0, 0},

baruch

> +#else
>  	{PEX1, SERDES_SPEED_5_GBPS, PEX_ROOT_COMPLEX_X1, 0, 0},
> +#endif
>  	{USB3_HOST1, SERDES_SPEED_5_GBPS, SERDES_DEFAULT_MODE, 0, 0},
>  #if defined (CONFIG_TARGET_CLEARFOG_BASE)
>  	{USB3_HOST0, SERDES_SPEED_5_GBPS, SERDES_DEFAULT_MODE, 0, 0},
> +#elif defined(CONFIG_CLEARFOG_CON2_SATA)
> +	{SATA2, SERDES_SPEED_3_GBPS, SERDES_DEFAULT_MODE, 0, 0},
>  #else
>  	{PEX2, SERDES_SPEED_5_GBPS, PEX_ROOT_COMPLEX_X1, 0, 0},
>  #endif

-- 
     http://baruch.siach.name/blog/                  ~. .~   Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
   - baruch at tkos.co.il - tel: +972.2.679.5364, http://www.tkos.co.il -

[PATCH v3 03/10] arm: mvebu: clearfog: initial ClearFog Base variant

[Baruch Siach]

Hi Joel,

On Wed, Jan 22, 2020 at 07:28:51PM +0000, Joel Johnson wrote:
> On January 22, 2020 10:32:58 AM UTC, Baruch Siach <baruch@tkos.co.il> wrote:
> >On Wed, Jan 22 2020, Joel Johnson wrote:
> >> On 2020-01-21 10:49, Baruch Siach wrote:
> >>> On Tue, Jan 21, 2020 at 10:32:17AM -0700, Joel Johnson wrote:
> >>>> Add a unique entry for ClearFog Base variant, reflected in the
> >board
> >>>> name and adjusted SerDes topology.
> >>>>
> >>>> Signed-off-by: Joel Johnson <mrjoel@lixil.net>
> >>>>
> >>>> ---
> >>>>
> >>>> v2 changes:
> >>>>   - reworked based on Baruch's run-time TLV EEPROM detection series
> >>>> v3 changes:
> >>>>   - rebased on mvebu merged run-time TLV EEPROM detection series
> >>>>   - minor update to help test regarding runtime detection failures
> >>>>
> >>>> ---
> >>>>  arch/arm/mach-mvebu/Kconfig        |  2 ++
> >>>>  board/solidrun/clearfog/Kconfig    | 18 ++++++++++++++++++
> >>>>  board/solidrun/clearfog/clearfog.c | 12 ++++++++++++
> >>>>  3 files changed, 32 insertions(+)
> >>>>  create mode 100644 board/solidrun/clearfog/Kconfig
> >>>>
> >>>> diff --git a/arch/arm/mach-mvebu/Kconfig
> >b/arch/arm/mach-mvebu/Kconfig
> >>>> index bc5eaa5a76..161dee937f 100644
> >>>> --- a/arch/arm/mach-mvebu/Kconfig
> >>>> +++ b/arch/arm/mach-mvebu/Kconfig
> >>>> @@ -280,4 +280,6 @@ config SECURED_MODE_CSK_INDEX
> >>>>  	default 0
> >>>>  	depends on SECURED_MODE_IMAGE
> >>>>
> >>>> +source "board/solidrun/clearfog/Kconfig"
> >>>> +
> >>>>  endif
> >>>> diff --git a/board/solidrun/clearfog/Kconfig
> >>>> b/board/solidrun/clearfog/Kconfig
> >>>> new file mode 100644
> >>>> index 0000000000..936d5918f8
> >>>> --- /dev/null
> >>>> +++ b/board/solidrun/clearfog/Kconfig
> >>>> @@ -0,0 +1,18 @@
> >>>> +menu "ClearFog configuration"
> >>>> +	depends on TARGET_CLEARFOG
> >>>> +
> >>>> +config TARGET_CLEARFOG_BASE
> >>>> +	bool "Use ClearFog Base static configuration"
> >>>> +	help
> >>>> +	  Use the ClearFog Base as the static configuration instead of
> >the
> >>>> +	  default which uses the ClearFog Pro.
> >>>> +
> >>>> +	  Runtime board detection is always attempted and used if
> >>>> available. The
> >>>> +	  static configuration is used as a fallback in cases where
> >runtime
> >>>> +	  detection is disabled, is not available in hardware, or
> >otherwise
> >>>> fails.
> >>>> +
> >>>> +	  Only newer revisions of the ClearFog product line support
> >runtime
> >>>> +	  detection via additional EEPROM hardware. This option enables
> >>>> selecting
> >>>> +	  the Base variant for older hardware revisions.
> >>>> +
> >>>> +endmenu
> >>>> diff --git a/board/solidrun/clearfog/clearfog.c
> >>>> b/board/solidrun/clearfog/clearfog.c
> >>>> index e268ef55a2..e77b9465d4 100644
> >>>> --- a/board/solidrun/clearfog/clearfog.c
> >>>> +++ b/board/solidrun/clearfog/clearfog.c
> >>>> @@ -47,7 +47,11 @@ static struct serdes_map board_serdes_map[] = {
> >>>>  	{SGMII1, SERDES_SPEED_1_25_GBPS, SERDES_DEFAULT_MODE, 0, 0},
> >>>>  	{PEX1, SERDES_SPEED_5_GBPS, PEX_ROOT_COMPLEX_X1, 0, 0},
> >>>>  	{USB3_HOST1, SERDES_SPEED_5_GBPS, SERDES_DEFAULT_MODE, 0, 0},
> >>>> +#if defined (CONFIG_TARGET_CLEARFOG_BASE)
> >>>> +	{USB3_HOST0, SERDES_SPEED_5_GBPS, SERDES_DEFAULT_MODE, 0, 0},
> >>>> +#else
> >>>>  	{PEX2, SERDES_SPEED_5_GBPS, PEX_ROOT_COMPLEX_X1, 0, 0},
> >>>> +#endif
> >>>
> >>> I'd prefer run-time test instead of '#ifdefs' that IMO make the code
> >harder
> >>> to
> >>> read. Something like this (build tested only):
> >>>
> >>> diff --git a/board/solidrun/clearfog/clearfog.c
> >>> b/board/solidrun/clearfog/clearfog.c
> >>> index e268ef55a2a0..202c60cb7841 100644
> >>> --- a/board/solidrun/clearfog/clearfog.c
> >>> +++ b/board/solidrun/clearfog/clearfog.c
> >>> @@ -55,7 +55,8 @@ int hws_board_topology_load(struct serdes_map
> >>> **serdes_map_array, u8 *count)
> >>>  {
> >>>  	cf_read_tlv_data();
> >>>
> >>> -	if (sr_product_is(&cf_tlv_data, "Clearfog GTR")) {
> >>> +	if (sr_product_is(&cf_tlv_data, "Clearfog GTR") ||
> >>> +			CONFIG_IS_ENABLED(TARGET_CLEARFOG_BASE)) {
> >>>  		board_serdes_map[0].serdes_type = PEX0;
> >>>  		board_serdes_map[0].serdes_speed = SERDES_SPEED_5_GBPS;
> >>>  		board_serdes_map[0].serdes_mode = PEX_ROOT_COMPLEX_X1;
> >>> @@ -172,6 +173,9 @@ int checkboard(void)
> >>>  {
> >>>  	char *board = "ClearFog";
> >>>
> >>> +	if (CONFIG_IS_ENABLED(TARGET_CLEARFOG_BASE))
> >>> +		board = "ClearFog Base";
> >>> +
> >>>  	cf_read_tlv_data();
> >>>  	if (strlen(cf_tlv_data.tlv_product_name[0]) > 0)
> >>>  		board = cf_tlv_data.tlv_product_name[0];
> >>> @@ -194,7 +198,8 @@ int board_late_init(void)
> >>>  {
> >>>  	cf_read_tlv_data();
> >>>
> >>> -	if (sr_product_is(&cf_tlv_data, "Clearfog Base"))
> >>> +	if (sr_product_is(&cf_tlv_data, "Clearfog Base") ||
> >>> +			CONFIG_IS_ENABLED(TARGET_CLEARFOG_BASE))
> >>>  		env_set("fdtfile", "armada-388-clearfog-base.dtb");
> >>>  	else if (sr_product_is(&cf_tlv_data, "Clearfog GTR S4"))
> >>>  		env_set("fdtfile", "armada-385-clearfog-gtr-s4.dtb");
> >>>
> >>> What do you think?
> >>>
> >>> baruch
> >>
> >> I'll have to revisit and test to be sure, but I believe the reason I
> >didn't go
> >> that route is because I wasn't able to get the CONFIG_IS_ENABLED
> >macro version
> >> to trigger when building SPL, although it worked for the main path. I
> >know
> >> that was the case for something I was working up, but I don't recall
> >whether
> >> it was this patch specifically or not.
> >
> >Right. We need the IS_ENABLED() macro here. For CONFIG_IS_ENABLED() we
> >would need another SPL_FOO config symbol. See include/linux/kconfig.h.
> >
> >> A bit of a nit, but just using the macro isn't really runtime
> >detection,
> >> however adding the separate code path for naming and serdes
> >manipulation
> >> dynamically tends that way. I'm already running into several cases
> >(not the
> >> default build options) where the SPL doesn't fit in the default 128K
> >offset
> >> size, so I'm leery of adding paths that add actual binary size
> >increase. I'm
> >> all for switching to CONFIG_IS_ENABLED if I test and can get it to
> >work, but
> >> still would lean towards just replacing the ifdef in place; after all
> >it is
> >> meant to be the static configuration, not dynamic.
> >
> >It probably makes more sense to reverse the order of ORed conditions:
> >
> >	if (IS_ENABLED(TARGET_CLEARFOG_BASE) ||
> >        		sr_product_is(&cf_tlv_data, "Clearfog Base"))
> >
> >IS_ENABLED() is evaluated at build time. When it is true, the compiler
> >drops all other 'if' branches, thus saving space. That also means that
> >the build time configuration overrides the EEPROM set value, which is a
> >Good Thing I believe.
> 
> I'll take a look and do testing and size comparison in the next day or two.
> 
> Note that I intended (and wrote the Base target help docs accordingly) that 
> runtime detection, if both enabled and supported in hardware, should be 
> preferred to the static configuration, with static config being only a 
> fallback. This seems to be different from what your thought about it being 
> good for build configuration to override EEPROM detected values, and I'm 
> curious as to your reasoning.

I was thinking about forcing board configuration for the manufacturing use 
case, when the EEPROM is not initialized. But I understand that this use case 
is not generally useful, unless the EEPROM data is damaged.

But if build time configuration serves as fallback, then you have to keep the 
run-time detection code anyway. How would '#ifdef' make your code smaller?

> There are a few gaps here related to what you point out (e.g. booting on a 
> Pro with EEPROM and Base static config won't give the expected results). 
> Relocating the static adjustment may be fine and help with that case as 
> well.
> 
> I'll want to add support in such handling for the EEPROM Pro devices but 
> don't have one available. You seem to have them available, can you confirm 
> that "Clearfog Pro" would match those devices using sr_product_is?

I considered Clearfog Pro a compatibility fallback. That is why it is not 
explicitly tested in the code, but assumed.

Clearfog Pro rev 2.2 is not in mass production yet to the best of my 
knowledge. I testing I used the "Clearfog Pro" string, which is a good match 
to "Clearfog Base" in current code.

baruch

-- 
     http://baruch.siach.name/blog/                  ~. .~   Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
   - baruch at tkos.co.il - tel: +972.2.679.5364, http://www.tkos.co.il -