diff mbox series

[v2,11/12] arm: mvebu: Espressobin: Use new API for setting default env at runtime

Message ID 20211103232332.2737-12-kabel@kernel.org
State Changes Requested
Delegated to: Simon Glass
Headers show
Series Board specific runtime determined default env | expand

Commit Message

Marek Behún Nov. 3, 2021, 11:23 p.m. UTC
From: Marek Behún <marek.behun@nic.cz>

ESPRESSObin's board code uses an ad-hoc solution for ensuring that
ethaddrs are not overwritten by `env default -a` command and that the
`fdtfile` is set to correct value when `env default -a` is called.

This ad-hoc solution is overwriting the default_environment[] buffer in
board_late_init().

Since now we have a specific API for overwriting default environment,
convert this ad-hoc code to this new API.

Signed-off-by: Marek Behún <marek.behun@nic.cz>
---
 board/Marvell/mvebu_armada-37xx/board.c     | 135 +++++++++++++-------
 configs/mvebu_espressobin-88f3720_defconfig |   1 +
 include/configs/mvebu_armada-37xx.h         |  17 +--
 3 files changed, 90 insertions(+), 63 deletions(-)

Comments

Simon Glass Nov. 5, 2021, 2:02 a.m. UTC | #1
On Wed, 3 Nov 2021 at 17:23, Marek Behún <kabel@kernel.org> wrote:
>
> From: Marek Behún <marek.behun@nic.cz>
>
> ESPRESSObin's board code uses an ad-hoc solution for ensuring that
> ethaddrs are not overwritten by `env default -a` command and that the
> `fdtfile` is set to correct value when `env default -a` is called.
>
> This ad-hoc solution is overwriting the default_environment[] buffer in
> board_late_init().
>
> Since now we have a specific API for overwriting default environment,
> convert this ad-hoc code to this new API.
>
> Signed-off-by: Marek Behún <marek.behun@nic.cz>
> ---
>  board/Marvell/mvebu_armada-37xx/board.c     | 135 +++++++++++++-------
>  configs/mvebu_espressobin-88f3720_defconfig |   1 +
>  include/configs/mvebu_armada-37xx.h         |  17 +--
>  3 files changed, 90 insertions(+), 63 deletions(-)

Reviewed-by: Simon Glass <sjg@chromium.org>
Pali Rohár Nov. 5, 2021, 8:50 a.m. UTC | #2
On Thursday 04 November 2021 20:02:33 Simon Glass wrote:
> On Wed, 3 Nov 2021 at 17:23, Marek Behún <kabel@kernel.org> wrote:
> >
> > From: Marek Behún <marek.behun@nic.cz>
> >
> > ESPRESSObin's board code uses an ad-hoc solution for ensuring that
> > ethaddrs are not overwritten by `env default -a` command and that the
> > `fdtfile` is set to correct value when `env default -a` is called.
> >
> > This ad-hoc solution is overwriting the default_environment[] buffer in
> > board_late_init().
> >
> > Since now we have a specific API for overwriting default environment,
> > convert this ad-hoc code to this new API.
> >
> > Signed-off-by: Marek Behún <marek.behun@nic.cz>
> > ---
> >  board/Marvell/mvebu_armada-37xx/board.c     | 135 +++++++++++++-------
> >  configs/mvebu_espressobin-88f3720_defconfig |   1 +
> >  include/configs/mvebu_armada-37xx.h         |  17 +--
> >  3 files changed, 90 insertions(+), 63 deletions(-)
> 
> Reviewed-by: Simon Glass <sjg@chromium.org>

Hello Simon! This change needs to be properly tested to ensure that
detection of eMMC is still correctly working and erase of MAC addresses
does not happen again. I will try to find some time during next week to
test this patch series on Espressobin board.
Pali Rohár Nov. 9, 2021, 3:37 p.m. UTC | #3
On Friday 05 November 2021 09:50:42 Pali Rohár wrote:
> On Thursday 04 November 2021 20:02:33 Simon Glass wrote:
> > On Wed, 3 Nov 2021 at 17:23, Marek Behún <kabel@kernel.org> wrote:
> > >
> > > From: Marek Behún <marek.behun@nic.cz>
> > >
> > > ESPRESSObin's board code uses an ad-hoc solution for ensuring that
> > > ethaddrs are not overwritten by `env default -a` command and that the
> > > `fdtfile` is set to correct value when `env default -a` is called.
> > >
> > > This ad-hoc solution is overwriting the default_environment[] buffer in
> > > board_late_init().
> > >
> > > Since now we have a specific API for overwriting default environment,
> > > convert this ad-hoc code to this new API.
> > >
> > > Signed-off-by: Marek Behún <marek.behun@nic.cz>
> > > ---
> > >  board/Marvell/mvebu_armada-37xx/board.c     | 135 +++++++++++++-------
> > >  configs/mvebu_espressobin-88f3720_defconfig |   1 +
> > >  include/configs/mvebu_armada-37xx.h         |  17 +--
> > >  3 files changed, 90 insertions(+), 63 deletions(-)
> > 
> > Reviewed-by: Simon Glass <sjg@chromium.org>
> 
> Hello Simon! This change needs to be properly tested to ensure that
> detection of eMMC is still correctly working and erase of MAC addresses
> does not happen again. I will try to find some time during next week to
> test this patch series on Espressobin board.

Hello! I have tested this change and it does NOT work.

Loading Environment from SPIFlash... SF: Detected w25q64dw with page size 256 Bytes, erase size 4 KiB, total 8 MiB
*** Warning - bad CRC, using default environment

=> echo $fdtfile

=>

Seems that $fdtfile is not set to default value when using default
environment. Calling 'env default -a' fixes it:

=> env default -a
## Resetting to default environment
=> echo $fdtfile 
marvell/armada-3720-espressobin.dtb
=>
diff mbox series

Patch

diff --git a/board/Marvell/mvebu_armada-37xx/board.c b/board/Marvell/mvebu_armada-37xx/board.c
index d7b6ecafbf..d36620c37a 100644
--- a/board/Marvell/mvebu_armada-37xx/board.c
+++ b/board/Marvell/mvebu_armada-37xx/board.c
@@ -6,17 +6,20 @@ 
 #include <common.h>
 #include <dm.h>
 #include <dm/device-internal.h>
+#include <dm/lists.h>
 #include <env.h>
 #include <env_internal.h>
 #include <i2c.h>
 #include <init.h>
 #include <mmc.h>
+#include <net.h>
 #include <phy.h>
 #include <asm/global_data.h>
 #include <asm/io.h>
 #include <asm/arch/cpu.h>
 #include <asm/arch/soc.h>
 #include <linux/delay.h>
+#include <sysinfo.h>
 
 DECLARE_GLOBAL_DATA_PTR;
 
@@ -85,66 +88,104 @@  int board_init(void)
 }
 
 #ifdef CONFIG_BOARD_LATE_INIT
-int board_late_init(void)
-{
-	char *ptr = &default_environment[0];
-	struct udevice *dev;
-	struct mmc *mmc_dev;
+struct ebin_sysinfo {
 	bool ddr4, emmc;
-	const char *mac;
-	char eth[10];
-	int i;
-
-	if (!of_machine_is_compatible("globalscale,espressobin"))
-		return 0;
-
-	/* Find free buffer in default_environment[] for new variables */
-	while (*ptr != '\0' && *(ptr+1) != '\0') ptr++;
-	ptr += 2;
-
-	/*
-	 * Ensure that 'env default -a' does not erase permanent MAC addresses
-	 * stored in env variables: $ethaddr, $eth1addr, $eth2addr and $eth3addr
-	 */
-
-	mac = env_get("ethaddr");
-	if (mac && strlen(mac) <= 17)
-		ptr += sprintf(ptr, "ethaddr=%s", mac) + 1;
+	char vars[5][9];
+	u8 nvars;
+	u8 macs[4][6];
+};
 
-	for (i = 1; i <= 3; i++) {
-		sprintf(eth, "eth%daddr", i);
-		mac = env_get(eth);
-		if (mac && strlen(mac) <= 17)
-			ptr += sprintf(ptr, "%s=%s", eth, mac) + 1;
-	}
+/*
+ * We must do this in probe() instead of detect(), because we call
+ * eth_env_get_enetaddr_by_index(), which may try to read default environment,
+ * which may call detect() again from itself.
+ */
+static int ebin_probe(struct udevice *dev)
+{
+	struct ebin_sysinfo *priv = dev_get_priv(dev);
+	struct mmc *mmc;
+	int i;
 
 	/* If the memory controller has been configured for DDR4, we're running on v7 */
-	ddr4 = ((readl(A3700_CH0_MC_CTRL2_REG) >> A3700_MC_CTRL2_SDRAM_TYPE_OFFS)
-		& A3700_MC_CTRL2_SDRAM_TYPE_MASK) == A3700_MC_CTRL2_SDRAM_TYPE_DDR4;
+	priv->ddr4 = ((readl(A3700_CH0_MC_CTRL2_REG) >> A3700_MC_CTRL2_SDRAM_TYPE_OFFS)
+		      & A3700_MC_CTRL2_SDRAM_TYPE_MASK) == A3700_MC_CTRL2_SDRAM_TYPE_DDR4;
 
 	/* eMMC is mmc dev num 1 */
-	mmc_dev = find_mmc_device(1);
-	emmc = (mmc_dev && mmc_get_op_cond(mmc_dev, true) == 0);
+	mmc = find_mmc_device(1);
+	priv->emmc = (mmc && mmc_get_op_cond(mmc, true) == 0);
 
 	/* if eMMC is not present then remove it from DM */
-	if (!emmc && mmc_dev) {
-		dev = mmc_dev->dev;
-		device_remove(dev, DM_REMOVE_NORMAL);
-		device_unbind(dev);
+	if (!priv->emmc && mmc) {
+		struct udevice *mmc_dev = mmc->dev;
+		device_remove(mmc_dev, DM_REMOVE_NORMAL);
+		device_unbind(mmc_dev);
 	}
 
-	/* Ensure that 'env default -a' set correct value to $fdtfile */
-	if (ddr4 && emmc)
-		strcpy(ptr, "fdtfile=marvell/armada-3720-espressobin-v7-emmc.dtb");
-	else if (ddr4)
-		strcpy(ptr, "fdtfile=marvell/armada-3720-espressobin-v7.dtb");
-	else if (emmc)
-		strcpy(ptr, "fdtfile=marvell/armada-3720-espressobin-emmc.dtb");
-	else
-		strcpy(ptr, "fdtfile=marvell/armada-3720-espressobin.dtb");
+	strcpy(priv->vars[priv->nvars++], "fdtfile");
+
+	for (i = 0; i < 4; ++i)
+		if (eth_env_get_enetaddr_by_index("eth", i,
+						  priv->macs[priv->nvars - 1]))
+			sprintf(priv->vars[priv->nvars++],
+				i ? "eth%daddr" : "ethaddr", i);
 
 	return 0;
 }
+
+static int ebin_get_str_list(struct udevice *dev, int id, unsigned idx,
+			     size_t size, char *val)
+{
+	struct ebin_sysinfo *priv = dev_get_priv(dev);
+
+	if (id != SYSINFO_ID_DEF_ENV_NAMES && id != SYSINFO_ID_DEF_ENV_VALUES)
+		return -ENOENT;
+
+	if (idx >= priv->nvars)
+		return -ERANGE;
+
+	if (id == SYSINFO_ID_DEF_ENV_NAMES)
+		return snprintf(val, size, "%s", priv->vars[idx]);
+
+	if (idx == 0)
+		return snprintf(val, size,
+				"marvell/armada-3720-espressobin%s%s.dtb",
+				priv->ddr4 ? "-v7" : "",
+				priv->emmc ? "-emmc" : "");
+
+	return snprintf(val, size, "%pM", priv->macs[idx - 1]);
+}
+
+static struct sysinfo_ops ebin_sysinfo_ops = {
+	.get_str_list = ebin_get_str_list,
+};
+
+U_BOOT_DRIVER(ebin_sysinfo) = {
+	.name = "espressobin-sysinfo",
+	.id = UCLASS_SYSINFO,
+	.ops = &ebin_sysinfo_ops,
+	.priv_auto = sizeof(struct ebin_sysinfo),
+	.probe = ebin_probe,
+};
+
+int board_late_init(void)
+{
+	struct udevice *dev;
+	int res;
+
+	if (!of_machine_is_compatible("globalscale,espressobin"))
+		return 0;
+
+	res = device_bind_driver(gd->dm_root, "espressobin-sysinfo",
+				 "espressobin-sysinfo", &dev);
+	if (res < 0)
+		return res;
+
+	res = device_probe(dev);
+	if (res < 0)
+		return res;
+
+	return sysinfo_detect(dev);
+}
 #endif
 
 /* Board specific AHCI / SATA enable code */
diff --git a/configs/mvebu_espressobin-88f3720_defconfig b/configs/mvebu_espressobin-88f3720_defconfig
index 3a69954fcd..6e15d9b4b1 100644
--- a/configs/mvebu_espressobin-88f3720_defconfig
+++ b/configs/mvebu_espressobin-88f3720_defconfig
@@ -84,6 +84,7 @@  CONFIG_DM_REGULATOR_GPIO=y
 CONFIG_DEBUG_UART_ANNOUNCE=y
 CONFIG_MVEBU_A3700_UART=y
 CONFIG_MVEBU_A3700_SPI=y
+CONFIG_SYSINFO=y
 CONFIG_USB=y
 CONFIG_USB_XHCI_HCD=y
 CONFIG_USB_EHCI_HCD=y
diff --git a/include/configs/mvebu_armada-37xx.h b/include/configs/mvebu_armada-37xx.h
index e7f7e772fc..1669eaf715 100644
--- a/include/configs/mvebu_armada-37xx.h
+++ b/include/configs/mvebu_armada-37xx.h
@@ -35,11 +35,6 @@ 
 /* End of 16M scrubbed by training in bootrom */
 #define CONFIG_SYS_INIT_SP_ADDR         (CONFIG_SYS_TEXT_BASE + 0xFF0000)
 
-/*
- * Environment
- */
-#define DEFAULT_ENV_IS_RW		/* required for configuring default fdtfile= */
-
 /*
  * Ethernet Driver configuration
  */
@@ -70,15 +65,6 @@ 
 
 #include <config_distro_bootcmd.h>
 
-/* filler for default values filled by board_early_init_f() */
-#define ENV_RW_FILLER \
-	"\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0" /* for ethaddr= */ \
-	"\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0" /* for eth1addr= */ \
-	"\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0" /* for eth2addr= */ \
-	"\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0" /* for eth3addr= */ \
-	"\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0" /* for fdtfile= */ \
-	""
-
 /* fdt_addr and kernel_addr are needed for existing distribution boot scripts */
 #define CONFIG_EXTRA_ENV_SETTINGS	\
 	"scriptaddr=0x6d00000\0"	\
@@ -88,7 +74,6 @@ 
 	"kernel_addr=0x7000000\0"	\
 	"kernel_addr_r=0x7000000\0"	\
 	"ramdisk_addr_r=0xa000000\0"	\
-	BOOTENV \
-	ENV_RW_FILLER
+	BOOTENV
 
 #endif /* _CONFIG_MVEBU_ARMADA_37XX_H */