[PATCH 00/10] board: sl28: add sl28cpld support and board cleanups

[PATCH 00/10] board: sl28: add sl28cpld support and board cleanups

[Michael Walle]

Add support for the sl28cpld management controller found on this board.
This is especially useful, because of the integrated watchdog in this
controller. It is used to supervise the bootup of the bootloader and will
automatically switch to the failsafe bootloader if u-boot didn't start. Up
until now, we had to disable this feature, leaving a gap in the failsafe
boot concept of this board. Now, with this driver in place, this watchdog
can be enabled at reset and u-boot will disable it late during its startup.

Additionally, add support for the GPIOs which are connected to this
controller.

Besides this support, there are various cleanups, like DRAM output fix,
printing the version of the management controller and disabling the random
MAC address generation.

Implementationwise, the MFD driver is implemented as UCLASS_NOP which
provides basic access functions and will enumerate its child nodes. The
device tree binding is the same as already acked by the linux kernel.

Michael Walle (10):
  misc: add sl28cpld base driver
  watchdog: add sl28cpld watchdog driver
  gpio: add sl28cpld driver
  board: sl28: fix DRAM pretty print
  board: sl28: print CPLD version on bootup
  board: sl28: enable sl28cpld support
  board: sl28: enable SoC watchdog support
  board: sl28: disable recovery watchdog
  board: sl28: remove "Useful I2C tricks" section from docs
  board: sl28: disable random MAC address generation

 MAINTAINERS                     |   7 ++
 board/kontron/sl28/sl28.c       |  59 +++++++++++-
 configs/kontron_sl28_defconfig  |  11 ++-
 doc/board/kontron/sl28.rst      |  66 +++++++------
 drivers/gpio/Kconfig            |   6 ++
 drivers/gpio/Makefile           |   1 +
 drivers/gpio/sl28cpld-gpio.c    | 165 ++++++++++++++++++++++++++++++++
 drivers/misc/Kconfig            |   8 ++
 drivers/misc/Makefile           |   1 +
 drivers/misc/sl28cpld.c         | 105 ++++++++++++++++++++
 drivers/watchdog/Kconfig        |   7 ++
 drivers/watchdog/Makefile       |   1 +
 drivers/watchdog/sl28cpld-wdt.c | 109 +++++++++++++++++++++
 include/sl28cpld.h              |  17 ++++
 14 files changed, 528 insertions(+), 35 deletions(-)
 create mode 100644 drivers/gpio/sl28cpld-gpio.c
 create mode 100644 drivers/misc/sl28cpld.c
 create mode 100644 drivers/watchdog/sl28cpld-wdt.c
 create mode 100644 include/sl28cpld.h

-- 
2.30.2

[PATCH 01/10] misc: add sl28cpld base driver

[Michael Walle]

Add a multi-function device driver which will probe its children and
provides methods to access the device.

Signed-off-by: Michael Walle <michael@walle.cc>
---
 MAINTAINERS             |   5 ++
 drivers/misc/Kconfig    |   8 +++
 drivers/misc/Makefile   |   1 +
 drivers/misc/sl28cpld.c | 105 ++++++++++++++++++++++++++++++++++++++++
 include/sl28cpld.h      |  14 ++++++
 5 files changed, 133 insertions(+)
 create mode 100644 drivers/misc/sl28cpld.c
 create mode 100644 include/sl28cpld.h

diff --git a/MAINTAINERS b/MAINTAINERS
index 6db5354322..e9d0bd0818 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1100,6 +1100,11 @@ S:	Maintained
 T:	git https://source.denx.de/u-boot/custodians/u-boot-sh.git
 F:	arch/sh/
 
+SL28CLPD
+M:	Michael Walle <michael@walle.cc>
+S:	Maintained
+F:	drivers/misc/sl28cpld.c
+
 SPI
 M:	Jagan Teki <jagan@amarulasolutions.com>
 S:	Maintained
diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
index 3bae072005..b2cc4fc920 100644
--- a/drivers/misc/Kconfig
+++ b/drivers/misc/Kconfig
@@ -500,4 +500,12 @@ config ESM_PMIC
 	  Support ESM (Error Signal Monitor) on PMIC devices. ESM is used
 	  typically to reboot the board in error condition.
 
+config SL28CPLD
+	bool "Enable Kontron sl28cpld multi-function driver"
+	depends on DM_I2C
+	help
+	  Support for the Kontron sl28cpld management controller. This is
+	  the base driver which provides common access methods for the
+	  sub-drivers.
+
 endmenu
diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
index f9826d2462..cfb4cbff79 100644
--- a/drivers/misc/Makefile
+++ b/drivers/misc/Makefile
@@ -82,3 +82,4 @@ obj-$(CONFIG_MICROCHIP_FLEXCOM) += microchip_flexcom.o
 obj-$(CONFIG_K3_AVS0) += k3_avs.o
 obj-$(CONFIG_ESM_K3) += k3_esm.o
 obj-$(CONFIG_ESM_PMIC) += esm_pmic.o
+obj-$(CONFIG_SL28CPLD) += sl28cpld.o
diff --git a/drivers/misc/sl28cpld.c b/drivers/misc/sl28cpld.c
new file mode 100644
index 0000000000..01ef1c6178
--- /dev/null
+++ b/drivers/misc/sl28cpld.c
@@ -0,0 +1,105 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Copyright (c) 2021 Michael Walle <michael@walle.cc>
+ */
+
+#include <common.h>
+#include <dm.h>
+#include <i2c.h>
+
+struct sl28cpld_child_plat {
+	uint offset;
+};
+
+/*
+ * The access methods works either with the first argument being a child
+ * device or with the MFD device itself.
+ */
+static int sl28cpld_read_child(struct udevice *dev, uint offset)
+{
+	struct sl28cpld_child_plat *plat = dev_get_parent_plat(dev);
+	struct udevice *mfd = dev_get_parent(dev);
+
+	return dm_i2c_reg_read(mfd, offset + plat->offset);
+}
+
+int sl28cpld_read(struct udevice *dev, uint offset)
+{
+	if (dev->driver == DM_DRIVER_GET(sl28cpld))
+		return dm_i2c_reg_read(dev, offset);
+	else
+		return sl28cpld_read_child(dev, offset);
+}
+
+static int sl28cpld_write_child(struct udevice *dev, uint offset,
+				uint8_t value)
+{
+	struct sl28cpld_child_plat *plat = dev_get_parent_plat(dev);
+	struct udevice *mfd = dev_get_parent(dev);
+
+	return dm_i2c_reg_write(mfd, offset + plat->offset, value);
+}
+
+int sl28cpld_write(struct udevice *dev, uint offset, uint8_t value)
+{
+	if (dev->driver == DM_DRIVER_GET(sl28cpld))
+		return dm_i2c_reg_write(dev, offset, value);
+	else
+		return sl28cpld_write_child(dev, offset, value);
+}
+
+int sl28cpld_update(struct udevice *dev, uint offset, uint8_t clear,
+		    uint8_t set)
+{
+	int val;
+
+	val = sl28cpld_read(dev, offset);
+	if (val < 0)
+		return val;
+
+	val &= ~clear;
+	val |= set;
+
+	return sl28cpld_write(dev, offset, val);
+}
+
+static int sl28cpld_probe(struct udevice *dev)
+{
+	i2c_set_chip_flags(dev, DM_I2C_CHIP_RD_ADDRESS |
+			   DM_I2C_CHIP_WR_ADDRESS);
+
+	return 0;
+}
+
+static int sl28cpld_child_post_bind(struct udevice *dev)
+{
+	struct sl28cpld_child_plat *plat = dev_get_parent_plat(dev);
+	int offset;
+
+	if (!dev_has_ofnode(dev))
+		return 0;
+
+	offset = dev_read_u32_default(dev, "reg", -1);
+	if (offset == -1)
+		return -EINVAL;
+
+	plat->offset = offset;
+
+	return 0;
+}
+
+static const struct udevice_id sl28cpld_ids[] = {
+	{ .compatible = "kontron,sl28cpld" },
+	{}
+};
+
+U_BOOT_DRIVER(sl28cpld) = {
+	.name		= "sl28cpld",
+	.id		= UCLASS_NOP,
+	.of_match	= sl28cpld_ids,
+	.probe		= sl28cpld_probe,
+	.bind		= dm_scan_fdt_dev,
+	.flags		= DM_FLAG_PRE_RELOC,
+	.per_child_plat_auto = sizeof(struct sl28cpld_child_plat),
+	.child_post_bind = sl28cpld_child_post_bind,
+};
diff --git a/include/sl28cpld.h b/include/sl28cpld.h
new file mode 100644
index 0000000000..d116607cfb
--- /dev/null
+++ b/include/sl28cpld.h
@@ -0,0 +1,14 @@
+/* SPDX-License-Identifier: GPL-2.0+ */
+/*
+ * Copyright (c) 2021 Michael Walle <michael@walle.cc>
+ */
+
+#ifndef __SL28CPLD_H
+#define __SL28CPLD_H
+
+int sl28cpld_read(struct udevice *dev, uint offset);
+int sl28cpld_write(struct udevice *dev, uint offset, uint8_t value);
+int sl28cpld_update(struct udevice *dev, uint offset, uint8_t clear,
+		    uint8_t set);
+
+#endif
-- 
2.30.2

[PATCH 02/10] watchdog: add sl28cpld watchdog driver

[Michael Walle]

The watchdog timer is part of the sl28cpld management controller. The
watchdog timer usually supervises the bootloader boot-up and if it bites
the failsafe bootloader will be activated. Apart from that it supports
the usual board level reset and one SMARC speciality: driving the
WDT_TIMEOUT# signal.

Signed-off-by: Michael Walle <michael@walle.cc>
---
 MAINTAINERS                     |   1 +
 doc/board/kontron/sl28.rst      |  53 +++++++++++-----
 drivers/watchdog/Kconfig        |   7 ++
 drivers/watchdog/Makefile       |   1 +
 drivers/watchdog/sl28cpld-wdt.c | 109 ++++++++++++++++++++++++++++++++
 5 files changed, 154 insertions(+), 17 deletions(-)
 create mode 100644 drivers/watchdog/sl28cpld-wdt.c

diff --git a/MAINTAINERS b/MAINTAINERS
index e9d0bd0818..d7f09e1c81 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1104,6 +1104,7 @@ SL28CLPD
 M:	Michael Walle <michael@walle.cc>
 S:	Maintained
 F:	drivers/misc/sl28cpld.c
+F:	drivers/watchdog/sl28cpld-wdt.c
 
 SPI
 M:	Jagan Teki <jagan@amarulasolutions.com>
diff --git a/doc/board/kontron/sl28.rst b/doc/board/kontron/sl28.rst
index c7b18bed10..c2cdc5e424 100644
--- a/doc/board/kontron/sl28.rst
+++ b/doc/board/kontron/sl28.rst
@@ -35,23 +35,6 @@ The board is fully failsafe, you can't break anything. But because you've
 disabled the builtin watchdog you might have to manually enter failsafe
 mode by asserting the ``FORCE_RECOV#`` line during board reset.
 
-Disable the builtin watchdog
-^^^^^^^^^^^^^^^^^^^^^^^^^^^^
-
-- boot into the failsafe bootloader, either by asserting the
-  ``FORCE_RECOV#`` line or if you still have the original bootloader
-  installed you can use the command::
-
-  > wdt dev cpld_watchdog@4a; wdt expire 1
-
-- in the failsafe bootloader use the "sl28 nvm" command to disable
-  the automatic start of the builtin watchdog::
-
-  > sl28 nvm 0008
-
-- power-cycle the board
-
-
 Update image
 ------------
 
@@ -82,6 +65,42 @@ u-boot (yet). But you can use the i2c command to access it.
   > i2c md 4a 3.1 1
 
 
+Builtin watchdog
+----------------
+
+The builtin watchdog will supervise the bootloader startup. If anything
+goes wrong it will reset the board and boot into the failsafe bootloader.
+
+Once the bootloader is started successfully, it will disable the watchdog
+timer.
+
+wdt command flags
+^^^^^^^^^^^^^^^^^
+
+The `wdt start` as well as the `wdt expire` command take a flags argument.
+The supported bitmask is as follows.
+
+| Bit | Description                   |
+| --- | ----------------------------- |
+|   0 | Enable failsafe mode          |
+|   1 | Lock the control register     |
+|   2 | Disable board reset           |
+|   3 | Enable WDT_TIME_OUT# line     |
+
+For example, you can use `wdt expire 1` to issue a reset and boot into the
+failsafe bootloader.
+
+Disable the builtin watchdog
+^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+If for some reason, this isn't a desired behavior, the watchdog can also
+be configured to not be enabled on board reset. It's configuration is saved
+in the non-volatile board configuration bits. To change these you can use
+the `sl28 nvm` command.
+
+For more information on the non-volatile board configuration bits, see the
+following section.
+
 Non-volatile Board Configuration Bits
 -------------------------------------
 
diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index 1177f17fd8..a232b595ba 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -257,6 +257,13 @@ config WDT_SBSA
 	   In the single stage mode, when the timeout is reached, your system
 	   will be reset by WS1. The first signal (WS0) is ignored.
 
+config WDT_SL28CPLD
+	bool "sl28cpld watchdog timer support"
+	depends on WDT && SL28CPLD
+	help
+	  Enable support for the watchdog timer in the Kontron sl28cpld
+	  management controller.
+
 config WDT_SP805
 	bool "SP805 watchdog timer support"
 	depends on WDT
diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
index fa7ce583ce..8c6e5326c0 100644
--- a/drivers/watchdog/Makefile
+++ b/drivers/watchdog/Makefile
@@ -34,6 +34,7 @@ obj-$(CONFIG_WDT_OCTEONTX) += octeontx_wdt.o
 obj-$(CONFIG_WDT_OMAP3) += omap_wdt.o
 obj-$(CONFIG_WDT_SBSA) += sbsa_gwdt.o
 obj-$(CONFIG_WDT_K3_RTI) += rti_wdt.o
+obj-$(CONFIG_WDT_SL28CPLD) += sl28cpld-wdt.o
 obj-$(CONFIG_WDT_SP805) += sp805_wdt.o
 obj-$(CONFIG_WDT_STM32MP) += stm32mp_wdt.o
 obj-$(CONFIG_WDT_SUNXI) += sunxi_wdt.o
diff --git a/drivers/watchdog/sl28cpld-wdt.c b/drivers/watchdog/sl28cpld-wdt.c
new file mode 100644
index 0000000000..af5a6b1a28
--- /dev/null
+++ b/drivers/watchdog/sl28cpld-wdt.c
@@ -0,0 +1,109 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Watchdog driver for the sl28cpld
+ *
+ * Copyright (c) 2021 Michael Walle <michael@walle.cc>
+ */
+
+#include <common.h>
+#include <dm.h>
+#include <wdt.h>
+#include <sl28cpld.h>
+#include <div64.h>
+
+#define SL28CPLD_WDT_CTRL		0x00
+#define  WDT_CTRL_EN0			BIT(0)
+#define  WDT_CTRL_EN1			BIT(1)
+#define  WDT_CTRL_EN_MASK		GENMASK(1, 0)
+#define  WDT_CTRL_LOCK			BIT(2)
+#define  WDT_CTRL_ASSERT_SYS_RESET	BIT(6)
+#define  WDT_CTRL_ASSERT_WDT_TIMEOUT	BIT(7)
+#define SL28CPLD_WDT_TIMEOUT		0x01
+#define SL28CPLD_WDT_KICK		0x02
+#define  WDT_KICK_VALUE			0x6b
+
+static int sl28cpld_wdt_reset(struct udevice *dev)
+{
+	return sl28cpld_write(dev, SL28CPLD_WDT_KICK, WDT_KICK_VALUE);
+}
+
+static int sl28cpld_wdt_start(struct udevice *dev, u64 timeout, ulong flags)
+{
+	int ret, val;
+
+	val = sl28cpld_read(dev, SL28CPLD_WDT_CTRL);
+	if (val < 0)
+		return val;
+
+	/* (1) disable watchdog */
+	val &= ~WDT_CTRL_EN_MASK;
+	ret = sl28cpld_write(dev, SL28CPLD_WDT_CTRL, val);
+	if (ret)
+		return ret;
+
+	/* (2) set timeout */
+	ret = sl28cpld_write(dev, SL28CPLD_WDT_TIMEOUT, lldiv(timeout, 1000));
+	if (ret)
+		return ret;
+
+	/* (3) kick it, will reset timer to the timeout value */
+	ret = sl28cpld_wdt_reset(dev);
+	if (ret)
+		return ret;
+
+	/* (4) enable either recovery or normal one */
+	if (flags & BIT(0))
+		val |= WDT_CTRL_EN1;
+	else
+		val |= WDT_CTRL_EN0;
+
+	if (flags & BIT(1))
+		val |= WDT_CTRL_LOCK;
+
+	if (flags & BIT(2))
+		val &= ~WDT_CTRL_ASSERT_SYS_RESET;
+	else
+		val |= WDT_CTRL_ASSERT_SYS_RESET;
+
+	if (flags & BIT(3))
+		val |= WDT_CTRL_ASSERT_WDT_TIMEOUT;
+	else
+		val &= ~WDT_CTRL_ASSERT_WDT_TIMEOUT;
+
+	return sl28cpld_write(dev, SL28CPLD_WDT_CTRL, val);
+}
+
+static int sl28cpld_wdt_stop(struct udevice *dev)
+{
+	int val;
+
+	val = sl28cpld_read(dev, SL28CPLD_WDT_CTRL);
+	if (val < 0)
+		return val;
+
+	return sl28cpld_write(dev, SL28CPLD_WDT_CTRL, val & ~WDT_CTRL_EN_MASK);
+}
+
+static int sl28cpld_wdt_expire_now(struct udevice *dev, ulong flags)
+{
+	return sl28cpld_wdt_start(dev, 0, flags);
+}
+
+static const struct wdt_ops sl28cpld_wdt_ops = {
+	.start = sl28cpld_wdt_start,
+	.reset = sl28cpld_wdt_reset,
+	.stop = sl28cpld_wdt_stop,
+	.expire_now = sl28cpld_wdt_expire_now,
+};
+
+static const struct udevice_id sl28cpld_wdt_ids[] = {
+	{ .compatible = "kontron,sl28cpld-wdt", },
+	{}
+};
+
+U_BOOT_DRIVER(sl28cpld_wdt) = {
+	.name = "sl28cpld-wdt",
+	.id = UCLASS_WDT,
+	.of_match = sl28cpld_wdt_ids,
+	.ops = &sl28cpld_wdt_ops,
+};
-- 
2.30.2

[PATCH 03/10] gpio: add sl28cpld driver

[Michael Walle]

The gpio block is part of the sl28cpld sl28cpld management controller.
There are three different flavors: the usual input and output where the
direction is configurable, but also input only and output only variants.

Signed-off-by: Michael Walle <michael@walle.cc>
---
 MAINTAINERS                  |   1 +
 drivers/gpio/Kconfig         |   6 ++
 drivers/gpio/Makefile        |   1 +
 drivers/gpio/sl28cpld-gpio.c | 165 +++++++++++++++++++++++++++++++++++
 4 files changed, 173 insertions(+)
 create mode 100644 drivers/gpio/sl28cpld-gpio.c

diff --git a/MAINTAINERS b/MAINTAINERS
index d7f09e1c81..f2dd8d067a 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1103,6 +1103,7 @@ F:	arch/sh/
 SL28CLPD
 M:	Michael Walle <michael@walle.cc>
 S:	Maintained
+F:	drivers/gpio/sl28cpld-gpio.c
 F:	drivers/misc/sl28cpld.c
 F:	drivers/watchdog/sl28cpld-wdt.c
 
diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index 40abc33772..bb89babc0b 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -523,4 +523,10 @@ config NOMADIK_GPIO
 	  into a number of banks each with 32 GPIOs. The GPIOs for a device are
 	  defined in the device tree with one node for each bank.
 
+config SL28CPLD_GPIO
+	bool "Kontron sl28cpld GPIO driver"
+	depends on DM_GPIO && SL28CPLD
+	help
+	  Support GPIO access on Kontron sl28cpld board management controllers.
+
 endif
diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
index 3c851b38c7..e9dab62745 100644
--- a/drivers/gpio/Makefile
+++ b/drivers/gpio/Makefile
@@ -69,3 +69,4 @@ obj-$(CONFIG_NX_GPIO)		+= nx_gpio.o
 obj-$(CONFIG_SIFIVE_GPIO)	+= sifive-gpio.o
 obj-$(CONFIG_NOMADIK_GPIO)	+= nmk_gpio.o
 obj-$(CONFIG_MAX7320_GPIO)	+= max7320_gpio.o
+obj-$(CONFIG_SL28CPLD_GPIO)	+= sl28cpld-gpio.o
diff --git a/drivers/gpio/sl28cpld-gpio.c b/drivers/gpio/sl28cpld-gpio.c
new file mode 100644
index 0000000000..700fc3df29
--- /dev/null
+++ b/drivers/gpio/sl28cpld-gpio.c
@@ -0,0 +1,165 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * GPIO driver for the sl28cpld
+ *
+ * Copyright (c) 2021 Michael Walle <michael@walle.cc>
+ */
+
+#include <common.h>
+#include <dm.h>
+#include <asm/gpio.h>
+#include <sl28cpld.h>
+
+/* GPIO flavor */
+#define SL28CPLD_GPIO_DIR	0x00
+#define SL28CPLD_GPIO_OUT	0x01
+#define SL28CPLD_GPIO_IN	0x02
+
+/* input-only flavor */
+#define SL28CPLD_GPI_IN		0x00
+
+/* output-only flavor */
+#define SL28CPLD_GPO_OUT	0x00
+
+enum {
+	SL28CPLD_GPIO,
+	SL28CPLD_GPI,
+	SL28CPLD_GPO,
+};
+
+static int sl28cpld_gpio_get_value(struct udevice *dev, unsigned int gpio)
+{
+	ulong type = dev_get_driver_data(dev);
+	int val, reg;
+
+	switch (type) {
+	case SL28CPLD_GPIO:
+		reg = SL28CPLD_GPIO_IN;
+		break;
+	case SL28CPLD_GPI:
+		reg = SL28CPLD_GPI_IN;
+		break;
+	case SL28CPLD_GPO:
+		/* we are output only, thus just return the output value */
+		reg = SL28CPLD_GPO_OUT;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	val = sl28cpld_read(dev, reg);
+
+	return val < 0 ? val : !!(val & BIT(gpio));
+}
+
+static int sl28cpld_gpio_set_value(struct udevice *dev, unsigned int gpio,
+				   int value)
+{
+	ulong type = dev_get_driver_data(dev);
+	uint reg;
+
+	switch (type) {
+	case SL28CPLD_GPIO:
+		reg = SL28CPLD_GPIO_OUT;
+		break;
+	case SL28CPLD_GPO:
+		reg = SL28CPLD_GPO_OUT;
+		break;
+	case SL28CPLD_GPI:
+	default:
+		return -EINVAL;
+	}
+
+	if (value)
+		return sl28cpld_update(dev, reg, 0, BIT(gpio));
+	else
+		return sl28cpld_update(dev, reg, BIT(gpio), 0);
+}
+
+static int sl28cpld_gpio_direction_input(struct udevice *dev, unsigned int gpio)
+{
+	ulong type = dev_get_driver_data(dev);
+
+	switch (type) {
+	case SL28CPLD_GPI:
+		return 0;
+	case SL28CPLD_GPIO:
+		return sl28cpld_update(dev, SL28CPLD_GPIO_DIR, BIT(gpio), 0);
+	case SL28CPLD_GPO:
+	default:
+		return -EINVAL;
+	}
+}
+
+static int sl28cpld_gpio_direction_output(struct udevice *dev,
+					  unsigned int gpio, int value)
+{
+	ulong type = dev_get_driver_data(dev);
+	int ret;
+
+	/* set_value() will report an error if we are input-only */
+	ret = sl28cpld_gpio_set_value(dev, gpio, value);
+	if (ret)
+		return ret;
+
+	if (type == SL28CPLD_GPIO)
+		return sl28cpld_update(dev, SL28CPLD_GPIO_DIR, 0, BIT(gpio));
+
+	return 0;
+}
+
+static int sl28cpld_gpio_get_function(struct udevice *dev, unsigned int gpio)
+{
+	ulong type = dev_get_driver_data(dev);
+	int val;
+
+	switch (type) {
+	case SL28CPLD_GPIO:
+		val = sl28cpld_read(dev, SL28CPLD_GPIO_DIR);
+		if (val < 0)
+			return val;
+		if (val & BIT(gpio))
+			return GPIOF_OUTPUT;
+		else
+			return GPIOF_INPUT;
+	case SL28CPLD_GPI:
+		return GPIOF_INPUT;
+	case SL28CPLD_GPO:
+		return GPIOF_OUTPUT;
+	default:
+		return -EINVAL;
+	}
+}
+
+static const struct dm_gpio_ops sl28cpld_gpio_ops = {
+	.direction_input = sl28cpld_gpio_direction_input,
+	.direction_output = sl28cpld_gpio_direction_output,
+	.get_value = sl28cpld_gpio_get_value,
+	.set_value = sl28cpld_gpio_set_value,
+	.get_function = sl28cpld_gpio_get_function,
+};
+
+static int sl28cpld_gpio_probe(struct udevice *dev)
+{
+	struct gpio_dev_priv *uc_priv = dev_get_uclass_priv(dev);
+
+	uc_priv->gpio_count = 8;
+	uc_priv->bank_name = dev_read_name(dev);
+
+	return 0;
+}
+
+static const struct udevice_id sl28cpld_gpio_ids[] = {
+	{ .compatible = "kontron,sl28cpld-gpio", .data = SL28CPLD_GPIO},
+	{ .compatible = "kontron,sl28cpld-gpo", .data = SL28CPLD_GPO},
+	{ .compatible = "kontron,sl28cpld-gpi", .data = SL28CPLD_GPI},
+	{ }
+};
+
+U_BOOT_DRIVER(sl28cpld_gpio) = {
+	.name	= "sl28cpld_gpio",
+	.id	= UCLASS_GPIO,
+	.of_match = sl28cpld_gpio_ids,
+	.probe	= sl28cpld_gpio_probe,
+	.ops	= &sl28cpld_gpio_ops,
+};
-- 
2.30.2

[PATCH 04/10] board: sl28: fix DRAM pretty print

[Michael Walle]

The current console output is:

DRAM:  4 GiB
DDR    4 GiB (DDR3, 32-bit, CL=11, ECC on)

The size is printed twice and we can save one line of console output if
we join both lines. The new output is as follows:

DRAM:  4 GiB (DDR3, 32-bit, CL=11, ECC on)

Signed-off-by: Michael Walle <michael@walle.cc>
---
 board/kontron/sl28/sl28.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/board/kontron/sl28/sl28.c b/board/kontron/sl28/sl28.c
index c8ed7ac81a..4701eaad8d 100644
--- a/board/kontron/sl28/sl28.c
+++ b/board/kontron/sl28/sl28.c
@@ -47,8 +47,6 @@ int checkboard(void)
 
 void detail_board_ddr_info(void)
 {
-	puts("\nDDR    ");
-	print_size(gd->bd->bi_dram[0].size + gd->bd->bi_dram[1].size, "");
 	print_ddr_info(0);
 }
 
-- 
2.30.2

[PATCH 05/10] board: sl28: print CPLD version on bootup

[Michael Walle]

Most of the time it is very useful to have the version of the board
management controller. Now that we have a driver, print it during
startup.

Signed-off-by: Michael Walle <michael@walle.cc>
---
 board/kontron/sl28/sl28.c | 28 ++++++++++++++++++++++++++++
 include/sl28cpld.h        |  2 ++
 2 files changed, 30 insertions(+)

diff --git a/board/kontron/sl28/sl28.c b/board/kontron/sl28/sl28.c
index 4701eaad8d..e5c9f90c7a 100644
--- a/board/kontron/sl28/sl28.c
+++ b/board/kontron/sl28/sl28.c
@@ -1,6 +1,7 @@
 // SPDX-License-Identifier: GPL-2.0+
 
 #include <common.h>
+#include <dm.h>
 #include <malloc.h>
 #include <errno.h>
 #include <fsl_ddr.h>
@@ -15,6 +16,7 @@
 #include <fsl_immap.h>
 #include <netdev.h>
 
+#include <sl28cpld.h>
 #include <fdtdec.h>
 #include <miiphy.h>
 
@@ -39,9 +41,35 @@ int board_eth_init(struct bd_info *bis)
 	return pci_eth_init(bis);
 }
 
+static int __sl28cpld_read(uint reg)
+{
+	struct udevice *dev;
+	int ret;
+
+	ret = uclass_get_device_by_driver(UCLASS_NOP,
+					  DM_DRIVER_GET(sl28cpld), &dev);
+	if (ret)
+		return ret;
+
+	return sl28cpld_read(dev, reg);
+}
+
+static void print_cpld_version(void)
+{
+	int version = __sl28cpld_read(SL28CPLD_VERSION);
+
+	if (version < 0)
+		printf("CPLD:  error reading version (%d)\n", version);
+	else
+		printf("CPLD:  v%d\n", version);
+}
+
 int checkboard(void)
 {
 	printf("EL:    %d\n", current_el());
+	if (CONFIG_IS_ENABLED(SL28CPLD))
+		print_cpld_version();
+
 	return 0;
 }
 
diff --git a/include/sl28cpld.h b/include/sl28cpld.h
index d116607cfb..9a7c6de31f 100644
--- a/include/sl28cpld.h
+++ b/include/sl28cpld.h
@@ -6,6 +6,8 @@
 #ifndef __SL28CPLD_H
 #define __SL28CPLD_H
 
+#define SL28CPLD_VERSION	0x03
+
 int sl28cpld_read(struct udevice *dev, uint offset);
 int sl28cpld_write(struct udevice *dev, uint offset, uint8_t value);
 int sl28cpld_update(struct udevice *dev, uint offset, uint8_t clear,
-- 
2.30.2

[PATCH 06/10] board: sl28: enable sl28cpld support

[Michael Walle]

Enable the GPIO and watchdog driver. Don't start the watchdog
automatically, though.

Signed-off-by: Michael Walle <michael@walle.cc>
---
 configs/kontron_sl28_defconfig | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/configs/kontron_sl28_defconfig b/configs/kontron_sl28_defconfig
index 31a1083b0a..48cf3cf5e6 100644
--- a/configs/kontron_sl28_defconfig
+++ b/configs/kontron_sl28_defconfig
@@ -41,12 +41,14 @@ CONFIG_CMD_GREPENV=y
 CONFIG_CMD_NVEDIT_EFI=y
 CONFIG_CMD_DFU=y
 CONFIG_CMD_DM=y
+CONFIG_CMD_GPIO=y
 CONFIG_CMD_GPT=y
 CONFIG_CMD_I2C=y
 CONFIG_CMD_MMC=y
 CONFIG_CMD_PCI=y
 CONFIG_CMD_USB=y
 CONFIG_CMD_USB_MASS_STORAGE=y
+CONFIG_CMD_WDT=y
 CONFIG_CMD_CACHE=y
 CONFIG_CMD_EFIDEBUG=y
 CONFIG_CMD_RNG=y
@@ -68,8 +70,10 @@ CONFIG_DDR_ECC=y
 CONFIG_ECC_INIT_VIA_DDRCONTROLLER=y
 CONFIG_DFU_MMC=y
 CONFIG_DFU_SF=y
+CONFIG_SL28CPLD_GPIO=y
 CONFIG_I2C_SET_DEFAULT_BUS_NUM=y
 CONFIG_I2C_MUX=y
+CONFIG_SL28CPLD=y
 CONFIG_MMC_HS400_SUPPORT=y
 CONFIG_FSL_ESDHC=y
 CONFIG_FSL_ESDHC_SUPPORT_ADMA2=y
@@ -101,6 +105,10 @@ CONFIG_USB_DWC3=y
 CONFIG_USB_DWC3_LAYERSCAPE=y
 CONFIG_USB_GADGET=y
 CONFIG_USB_GADGET_DOWNLOAD=y
+# CONFIG_WATCHDOG is not set
+# CONFIG_WATCHDOG_AUTOSTART is not set
+CONFIG_WDT=y
+CONFIG_WDT_SL28CPLD=y
 CONFIG_OF_LIBFDT_ASSUME_MASK=0x0
 CONFIG_OF_LIBFDT_OVERLAY=y
 CONFIG_EFI_SET_TIME=y
-- 
2.30.2

[PATCH 07/10] board: sl28: enable SoC watchdog support

[Michael Walle]

The SoC provides two additional watchdogs integrated in the SoC. Enable
support for these.

Signed-off-by: Michael Walle <michael@walle.cc>
---
 configs/kontron_sl28_defconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/configs/kontron_sl28_defconfig b/configs/kontron_sl28_defconfig
index 48cf3cf5e6..af907175f1 100644
--- a/configs/kontron_sl28_defconfig
+++ b/configs/kontron_sl28_defconfig
@@ -109,6 +109,7 @@ CONFIG_USB_GADGET_DOWNLOAD=y
 # CONFIG_WATCHDOG_AUTOSTART is not set
 CONFIG_WDT=y
 CONFIG_WDT_SL28CPLD=y
+CONFIG_WDT_SP805=y
 CONFIG_OF_LIBFDT_ASSUME_MASK=0x0
 CONFIG_OF_LIBFDT_OVERLAY=y
 CONFIG_EFI_SET_TIME=y
-- 
2.30.2

[PATCH 08/10] board: sl28: disable recovery watchdog

[Michael Walle]

This board has an internal watchdog which supervises the board startup.
Although, the initial state of the watchdog is configurable, it is
enabled by default. In board_late_init(), which means almost everything
worked as expected, disable the watchdog.

Signed-off-by: Michael Walle <michael@walle.cc>
---
 board/kontron/sl28/sl28.c  | 29 +++++++++++++++++++++++++++++
 doc/board/kontron/sl28.rst | 12 ++++++------
 2 files changed, 35 insertions(+), 6 deletions(-)

diff --git a/board/kontron/sl28/sl28.c b/board/kontron/sl28/sl28.c
index e5c9f90c7a..9572502499 100644
--- a/board/kontron/sl28/sl28.c
+++ b/board/kontron/sl28/sl28.c
@@ -15,6 +15,7 @@
 #include <asm/arch/soc.h>
 #include <fsl_immap.h>
 #include <netdev.h>
+#include <wdt.h>
 
 #include <sl28cpld.h>
 #include <fdtdec.h>
@@ -73,6 +74,34 @@ int checkboard(void)
 	return 0;
 }
 
+static void stop_recovery_watchdog(void)
+{
+	struct udevice *dev;
+	int ret;
+
+	ret = uclass_get_device_by_driver(UCLASS_WDT,
+					  DM_DRIVER_GET(sl28cpld_wdt), &dev);
+	if (!ret)
+		wdt_stop(dev);
+}
+
+int fsl_board_late_init(void)
+{
+	/*
+	 * Usually, the after a board reset, the watchdog is enabled by
+	 * default. This is to supervise the bootloader boot-up. Therefore,
+	 * to prevent a watchdog reset if we don't actively kick it, we have
+	 * to disable it.
+	 *
+	 * If the watchdog isn't enabled at reset (which is a configuration
+	 * option) disabling it doesn't hurt either.
+	 */
+	if (!CONFIG_IS_ENABLED(WATCHDOG_AUTOSTART))
+		stop_recovery_watchdog();
+
+	return 0;
+}
+
 void detail_board_ddr_info(void)
 {
 	print_ddr_info(0);
diff --git a/doc/board/kontron/sl28.rst b/doc/board/kontron/sl28.rst
index c2cdc5e424..04483e1e57 100644
--- a/doc/board/kontron/sl28.rst
+++ b/doc/board/kontron/sl28.rst
@@ -23,17 +23,17 @@ Copy u-boot.rom to a TFTP server.
 Install the bootloader on the board
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 
-Please note, this bootloader doesn't support the builtin watchdog (yet),
-therefore you have to disable it, see below. Otherwise you'll end up in
-the failsafe bootloader on every reset::
+To install the bootloader binary use the following command::
 
  > tftp path/to/u-boot.rom
  > sf probe 0
  > sf update $fileaddr 0x210000 $filesize
 
-The board is fully failsafe, you can't break anything. But because you've
-disabled the builtin watchdog you might have to manually enter failsafe
-mode by asserting the ``FORCE_RECOV#`` line during board reset.
+The board is fully failsafe, you can't break anything. If builtin watchdog
+is enabled, you'll automatically end up in the failsafe bootloader if
+something goes wrong. If the watchdog is disabled, you have to manually
+enter failsafe mode by asserting the ``FORCE_RECOV#`` line during board
+reset.
 
 Update image
 ------------
-- 
2.30.2

[PATCH 09/10] board: sl28: remove "Useful I2C tricks" section from docs

[Michael Walle]

They are no longer needed, because we now have proper driver support for
the sl28cpld management controller.

Signed-off-by: Michael Walle <michael@walle.cc>
---
 doc/board/kontron/sl28.rst | 15 ---------------
 1 file changed, 15 deletions(-)

diff --git a/doc/board/kontron/sl28.rst b/doc/board/kontron/sl28.rst
index 04483e1e57..44435d90c6 100644
--- a/doc/board/kontron/sl28.rst
+++ b/doc/board/kontron/sl28.rst
@@ -50,21 +50,6 @@ Afterward you can copy this file to your ESP into the /EFI/UpdateCapsule/
 folder. On the next EFI boot this will automatically update your
 bootloader.
 
-Useful I2C tricks
------------------
-
-The board has a board management controller which is not supported in
-u-boot (yet). But you can use the i2c command to access it.
-
-- reset into failsafe bootloader::
-
-  > i2c mw 4a 5.1 0; i2c mw 4a 6.1 6b; i2c mw 4a 4.1 42
-
-- read board management controller version::
-
-  > i2c md 4a 3.1 1
-
-
 Builtin watchdog
 ----------------
 
-- 
2.30.2

[PATCH 10/10] board: sl28: disable random MAC address generation

[Michael Walle]

Nowadays, u-boot (when CONFIG_NET_RANDOM_ETHADDR is set) will set
enetaddr to a random value if not set and then pass the randomly
generated MAC address to linux.

This is bad for the following reasons:
 (1) it makes it impossible for linux to detect this error
 (2) linux won't trigger any fallback mechanism for the case where
     it didn't find any valid MAC address
 (3) a saveenv will store this randomly generated MAC address in the
     environment

Probably, the user will also be unaware that something is wrong. He will
just get different MAC addresses on each reboot, asking himself why this
is the case.

As this board usually have a serial port, the user can just fix this by
setting the MAC address manually in the environment. Also disable the
netconsole just in case, because it cannot be guaranteed that it will
work in any case. After all, this was just a convenience option, because
the bootloader - right now - doesn't have the ability to read the MAC
address, which is stored in the OTP. But it is far more important to
have a clear view of whats wrong with a board and that means we can no
longer use this Kconfig option.

Signed-off-by: Michael Walle <michael@walle.cc>
---
 configs/kontron_sl28_defconfig | 2 --
 1 file changed, 2 deletions(-)

diff --git a/configs/kontron_sl28_defconfig b/configs/kontron_sl28_defconfig
index af907175f1..7fb6bdbe82 100644
--- a/configs/kontron_sl28_defconfig
+++ b/configs/kontron_sl28_defconfig
@@ -59,8 +59,6 @@ CONFIG_OF_LIST=""
 CONFIG_ENV_OVERWRITE=y
 CONFIG_ENV_IS_IN_SPI_FLASH=y
 CONFIG_SYS_REDUNDAND_ENVIRONMENT=y
-CONFIG_NET_RANDOM_ETHADDR=y
-CONFIG_NETCONSOLE=y
 CONFIG_SPL_DM_SEQ_ALIAS=y
 CONFIG_SCSI_AHCI=y
 CONFIG_SATA_CEVA=y
-- 
2.30.2

Re: [PATCH 10/10] board: sl28: disable random MAC address generation

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 3173 bytes --]

On Mon, Nov 15, 2021 at 11:45:51PM +0100, Michael Walle wrote:

> Nowadays, u-boot (when CONFIG_NET_RANDOM_ETHADDR is set) will set
> enetaddr to a random value if not set and then pass the randomly
> generated MAC address to linux.

First, for clarity I'm not nak'ing this.  I kind of would like to see a
slight reword as I think some things aren't 100% correct, even if the
"save random MAC to ethaddr environment variable" change goes in.  For
example, it's quite long standing that (dev|pdata)->enetaddr populates
"mac-address" and "local-mac-address" and it seems in some older cases
we only set the "local-mac-address" property.

> This is bad for the following reasons:
>  (1) it makes it impossible for linux to detect this error
>  (2) linux won't trigger any fallback mechanism for the case where
>      it didn't find any valid MAC address

This feels in some ways to be a limitation of the binding:
https://www.kernel.org/doc/Documentation/devicetree/bindings/net/ethernet-controller.yaml

And it reads like we really must be populating "mac-address" with that
random one and while providing a blank "local-mac-address" would be a
way to say we don't know the true device one, it seems that wouldn't be
used / noticed?

>  (3) a saveenv will store this randomly generated MAC address in the
>      environment
> 
> Probably, the user will also be unaware that something is wrong. He will
> just get different MAC addresses on each reboot, asking himself why this
> is the case.
> 
> As this board usually have a serial port, the user can just fix this by
> setting the MAC address manually in the environment. Also disable the
> netconsole just in case, because it cannot be guaranteed that it will
> work in any case. After all, this was just a convenience option, because
> the bootloader - right now - doesn't have the ability to read the MAC
> address, which is stored in the OTP. But it is far more important to
> have a clear view of whats wrong with a board and that means we can no
> longer use this Kconfig option.
> 
> Signed-off-by: Michael Walle <michael@walle.cc>
> ---
>  configs/kontron_sl28_defconfig | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/configs/kontron_sl28_defconfig b/configs/kontron_sl28_defconfig
> index af907175f1..7fb6bdbe82 100644
> --- a/configs/kontron_sl28_defconfig
> +++ b/configs/kontron_sl28_defconfig
> @@ -59,8 +59,6 @@ CONFIG_OF_LIST=""
>  CONFIG_ENV_OVERWRITE=y
>  CONFIG_ENV_IS_IN_SPI_FLASH=y
>  CONFIG_SYS_REDUNDAND_ENVIRONMENT=y
> -CONFIG_NET_RANDOM_ETHADDR=y
> -CONFIG_NETCONSOLE=y
>  CONFIG_SPL_DM_SEQ_ALIAS=y
>  CONFIG_SCSI_AHCI=y
>  CONFIG_SATA_CEVA=y

Now, an alternate solution here would be to enable these two options
still and write an ft_board_setup() with:
... if ethaddr is in the locally administered pool then
        fdt_delprop(... "mac-address");
        fdt_delprop(... "local-mac-address");

And that should cause the kernel to fall through the cases to find out
where to get the real MAC from.  I'm not super happy with this at first,
but I also don't see anything clever in the binding we can do.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH 10/10] board: sl28: disable random MAC address generation

[Michael Walle]

Am 2021-11-16 22:14, schrieb Tom Rini:
> On Mon, Nov 15, 2021 at 11:45:51PM +0100, Michael Walle wrote:
> 
>> Nowadays, u-boot (when CONFIG_NET_RANDOM_ETHADDR is set) will set
>> enetaddr to a random value if not set and then pass the randomly
>> generated MAC address to linux.
> 
> First, for clarity I'm not nak'ing this.  I kind of would like to see a
> slight reword as I think some things aren't 100% correct, even if the
> "save random MAC to ethaddr environment variable" change goes in.  For
> example, it's quite long standing that (dev|pdata)->enetaddr populates
> "mac-address" and "local-mac-address" and it seems in some older cases
> we only set the "local-mac-address" property.

fdt_fixup_memory() in common/fdt_support.c does a env_get(mac).

I'm not even sure, if there is a connection between what is fixed up
in the kernel DT and the corresponding device in u-boot if
CONFIG_DM_SEQ_ALIAS is not set.

>> This is bad for the following reasons:
>>  (1) it makes it impossible for linux to detect this error
>>  (2) linux won't trigger any fallback mechanism for the case where
>>      it didn't find any valid MAC address
> 
> This feels in some ways to be a limitation of the binding:
> https://www.kernel.org/doc/Documentation/devicetree/bindings/net/ethernet-controller.yaml

Agreed. But it doesn't render my argument invalid ;)

> And it reads like we really must be populating "mac-address" with that
> random one and while providing a blank "local-mac-address" would be a
> way to say we don't know the true device one, it seems that wouldn't be
> used / noticed?

I guess it will just use the one populated in mac-address, i.e. the
random one. Btw, the binding says "last used" what if u-boot never
actually used that ethernet controller, should it then be empty or
populated with the random mac address.

>>  (3) a saveenv will store this randomly generated MAC address in the
>>      environment
>> 
>> Probably, the user will also be unaware that something is wrong. He 
>> will
>> just get different MAC addresses on each reboot, asking himself why 
>> this
>> is the case.
>> 
>> As this board usually have a serial port, the user can just fix this 
>> by
>> setting the MAC address manually in the environment. Also disable the
>> netconsole just in case, because it cannot be guaranteed that it will
>> work in any case. After all, this was just a convenience option, 
>> because
>> the bootloader - right now - doesn't have the ability to read the MAC
>> address, which is stored in the OTP. But it is far more important to
>> have a clear view of whats wrong with a board and that means we can no
>> longer use this Kconfig option.
>> 
>> Signed-off-by: Michael Walle <michael@walle.cc>
>> ---
>>  configs/kontron_sl28_defconfig | 2 --
>>  1 file changed, 2 deletions(-)
>> 
>> diff --git a/configs/kontron_sl28_defconfig 
>> b/configs/kontron_sl28_defconfig
>> index af907175f1..7fb6bdbe82 100644
>> --- a/configs/kontron_sl28_defconfig
>> +++ b/configs/kontron_sl28_defconfig
>> @@ -59,8 +59,6 @@ CONFIG_OF_LIST=""
>>  CONFIG_ENV_OVERWRITE=y
>>  CONFIG_ENV_IS_IN_SPI_FLASH=y
>>  CONFIG_SYS_REDUNDAND_ENVIRONMENT=y
>> -CONFIG_NET_RANDOM_ETHADDR=y
>> -CONFIG_NETCONSOLE=y
>>  CONFIG_SPL_DM_SEQ_ALIAS=y
>>  CONFIG_SCSI_AHCI=y
>>  CONFIG_SATA_CEVA=y
> 
> Now, an alternate solution here would be to enable these two options
> still and write an ft_board_setup() with:
> ... if ethaddr is in the locally administered pool then
>         fdt_delprop(... "mac-address");
>         fdt_delprop(... "local-mac-address");

Which would also trigger if a user sets a "locally administered"
mac himself. Even if unlikely, I'm opposed to such magic and
unexpected behavior. Sooner or later it will bite you.

> And that should cause the kernel to fall through the cases to find out
> where to get the real MAC from.  I'm not super happy with this at 
> first,
> but I also don't see anything clever in the binding we can do.

If I'll need this one again, then I'll just add the random mac
generation in board_eth_init(), I think.

-michael

Re: [PATCH 10/10] board: sl28: disable random MAC address generation

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 5229 bytes --]

On Wed, Nov 17, 2021 at 05:45:58PM +0100, Michael Walle wrote:
> Am 2021-11-16 22:14, schrieb Tom Rini:
> > On Mon, Nov 15, 2021 at 11:45:51PM +0100, Michael Walle wrote:
> > 
> > > Nowadays, u-boot (when CONFIG_NET_RANDOM_ETHADDR is set) will set
> > > enetaddr to a random value if not set and then pass the randomly
> > > generated MAC address to linux.
> > 
> > First, for clarity I'm not nak'ing this.  I kind of would like to see a
> > slight reword as I think some things aren't 100% correct, even if the
> > "save random MAC to ethaddr environment variable" change goes in.  For
> > example, it's quite long standing that (dev|pdata)->enetaddr populates
> > "mac-address" and "local-mac-address" and it seems in some older cases
> > we only set the "local-mac-address" property.
> 
> fdt_fixup_memory() in common/fdt_support.c does a env_get(mac).

Whoops, yes, I misrecalled this.

> I'm not even sure, if there is a connection between what is fixed up
> in the kernel DT and the corresponding device in u-boot if
> CONFIG_DM_SEQ_ALIAS is not set.

And then yes, the logic here can be further fragile at times with
multiple interfaces.

> > > This is bad for the following reasons:
> > >  (1) it makes it impossible for linux to detect this error
> > >  (2) linux won't trigger any fallback mechanism for the case where
> > >      it didn't find any valid MAC address
> > 
> > This feels in some ways to be a limitation of the binding:
> > https://www.kernel.org/doc/Documentation/devicetree/bindings/net/ethernet-controller.yaml
> 
> Agreed. But it doesn't render my argument invalid ;)

"Bug" or "feature", but yes, OK.

> > And it reads like we really must be populating "mac-address" with that
> > random one and while providing a blank "local-mac-address" would be a
> > way to say we don't know the true device one, it seems that wouldn't be
> > used / noticed?
> 
> I guess it will just use the one populated in mac-address, i.e. the
> random one. Btw, the binding says "last used" what if u-boot never
> actually used that ethernet controller, should it then be empty or
> populated with the random mac address.

My gut feeling here is that we have an example of a binding that has
been formalized around what exists in the wild, and was not strictly
intentional and clear at all times.  What we do today in the case of
ethernet devices we haven't used is populate both, just like the one we
have used, based on the environment.  And popping over to current Linux
kernel git, the handful of boards that hard-code a non-zero mac-address
or local-mac-address in the dts just makes things even less-clear on
what to do.  Which probably leaves us with "populate mac-address with
whatever you can".

> > >  (3) a saveenv will store this randomly generated MAC address in the
> > >      environment
> > > 
> > > Probably, the user will also be unaware that something is wrong. He
> > > will
> > > just get different MAC addresses on each reboot, asking himself why
> > > this
> > > is the case.
> > > 
> > > As this board usually have a serial port, the user can just fix this
> > > by
> > > setting the MAC address manually in the environment. Also disable the
> > > netconsole just in case, because it cannot be guaranteed that it will
> > > work in any case. After all, this was just a convenience option,
> > > because
> > > the bootloader - right now - doesn't have the ability to read the MAC
> > > address, which is stored in the OTP. But it is far more important to
> > > have a clear view of whats wrong with a board and that means we can no
> > > longer use this Kconfig option.
> > > 
> > > Signed-off-by: Michael Walle <michael@walle.cc>
> > > ---
> > >  configs/kontron_sl28_defconfig | 2 --
> > >  1 file changed, 2 deletions(-)
> > > 
> > > diff --git a/configs/kontron_sl28_defconfig
> > > b/configs/kontron_sl28_defconfig
> > > index af907175f1..7fb6bdbe82 100644
> > > --- a/configs/kontron_sl28_defconfig
> > > +++ b/configs/kontron_sl28_defconfig
> > > @@ -59,8 +59,6 @@ CONFIG_OF_LIST=""
> > >  CONFIG_ENV_OVERWRITE=y
> > >  CONFIG_ENV_IS_IN_SPI_FLASH=y
> > >  CONFIG_SYS_REDUNDAND_ENVIRONMENT=y
> > > -CONFIG_NET_RANDOM_ETHADDR=y
> > > -CONFIG_NETCONSOLE=y
> > >  CONFIG_SPL_DM_SEQ_ALIAS=y
> > >  CONFIG_SCSI_AHCI=y
> > >  CONFIG_SATA_CEVA=y
> > 
> > Now, an alternate solution here would be to enable these two options
> > still and write an ft_board_setup() with:
> > ... if ethaddr is in the locally administered pool then
> >         fdt_delprop(... "mac-address");
> >         fdt_delprop(... "local-mac-address");
> 
> Which would also trigger if a user sets a "locally administered"
> mac himself. Even if unlikely, I'm opposed to such magic and
> unexpected behavior. Sooner or later it will bite you.

A fair point, yes.

> > And that should cause the kernel to fall through the cases to find out
> > where to get the real MAC from.  I'm not super happy with this at first,
> > but I also don't see anything clever in the binding we can do.
> 
> If I'll need this one again, then I'll just add the random mac
> generation in board_eth_init(), I think.

OK.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH 10/10] board: sl28: disable random MAC address generation

[Michael Walle]

Am 2021-11-17 19:24, schrieb Tom Rini:
> On Wed, Nov 17, 2021 at 05:45:58PM +0100, Michael Walle wrote:
>> Am 2021-11-16 22:14, schrieb Tom Rini:
>> > On Mon, Nov 15, 2021 at 11:45:51PM +0100, Michael Walle wrote:
>> >
>> > > Nowadays, u-boot (when CONFIG_NET_RANDOM_ETHADDR is set) will set
>> > > enetaddr to a random value if not set and then pass the randomly
>> > > generated MAC address to linux.
>> >
>> > First, for clarity I'm not nak'ing this.  I kind of would like to see a
>> > slight reword as I think some things aren't 100% correct, even if the
>> > "save random MAC to ethaddr environment variable" change goes in.  For
>> > example, it's quite long standing that (dev|pdata)->enetaddr populates
>> > "mac-address" and "local-mac-address" and it seems in some older cases
>> > we only set the "local-mac-address" property.
>> 
>> fdt_fixup_memory() in common/fdt_support.c does a env_get(mac).
> 
> Whoops, yes, I misrecalled this.

That means you are fine with this commit message?

-michael

Re: [PATCH 10/10] board: sl28: disable random MAC address generation

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 1279 bytes --]

On Thu, Nov 18, 2021 at 09:29:51AM +0100, Michael Walle wrote:
> Am 2021-11-17 19:24, schrieb Tom Rini:
> > On Wed, Nov 17, 2021 at 05:45:58PM +0100, Michael Walle wrote:
> > > Am 2021-11-16 22:14, schrieb Tom Rini:
> > > > On Mon, Nov 15, 2021 at 11:45:51PM +0100, Michael Walle wrote:
> > > >
> > > > > Nowadays, u-boot (when CONFIG_NET_RANDOM_ETHADDR is set) will set
> > > > > enetaddr to a random value if not set and then pass the randomly
> > > > > generated MAC address to linux.
> > > >
> > > > First, for clarity I'm not nak'ing this.  I kind of would like to see a
> > > > slight reword as I think some things aren't 100% correct, even if the
> > > > "save random MAC to ethaddr environment variable" change goes in.  For
> > > > example, it's quite long standing that (dev|pdata)->enetaddr populates
> > > > "mac-address" and "local-mac-address" and it seems in some older cases
> > > > we only set the "local-mac-address" property.
> > > 
> > > fdt_fixup_memory() in common/fdt_support.c does a env_get(mac).
> > 
> > Whoops, yes, I misrecalled this.
> 
> That means you are fine with this commit message?

Well, so long as it goes in after the change that makes
NET_RANDOM_ETHADDR update the environment, yes, it's all correct.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH 00/10] board: sl28: add sl28cpld support and board cleanups

[Michael Walle]

Am 2021-11-15 23:45, schrieb Michael Walle:
> Add support for the sl28cpld management controller found on this board.
> This is especially useful, because of the integrated watchdog in this
> controller. It is used to supervise the bootup of the bootloader and 
> will
> automatically switch to the failsafe bootloader if u-boot didn't start. 
> Up
> until now, we had to disable this feature, leaving a gap in the 
> failsafe
> boot concept of this board. Now, with this driver in place, this 
> watchdog
> can be enabled at reset and u-boot will disable it late during its 
> startup.
> 
> Additionally, add support for the GPIOs which are connected to this
> controller.
> 
> Besides this support, there are various cleanups, like DRAM output fix,
> printing the version of the management controller and disabling the 
> random
> MAC address generation.
> 
> Implementationwise, the MFD driver is implemented as UCLASS_NOP which
> provides basic access functions and will enumerate its child nodes. The
> device tree binding is the same as already acked by the linux kernel.

Ping. This is now 2.5 month old. It doesn't even touch anything outside
my own board.

-michael

Re: [PATCH 00/10] board: sl28: add sl28cpld support and board cleanups

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 1327 bytes --]

On Mon, Jan 31, 2022 at 08:51:39AM +0100, Michael Walle wrote:
> Am 2021-11-15 23:45, schrieb Michael Walle:
> > Add support for the sl28cpld management controller found on this board.
> > This is especially useful, because of the integrated watchdog in this
> > controller. It is used to supervise the bootup of the bootloader and
> > will
> > automatically switch to the failsafe bootloader if u-boot didn't start.
> > Up
> > until now, we had to disable this feature, leaving a gap in the failsafe
> > boot concept of this board. Now, with this driver in place, this
> > watchdog
> > can be enabled at reset and u-boot will disable it late during its
> > startup.
> > 
> > Additionally, add support for the GPIOs which are connected to this
> > controller.
> > 
> > Besides this support, there are various cleanups, like DRAM output fix,
> > printing the version of the management controller and disabling the
> > random
> > MAC address generation.
> > 
> > Implementationwise, the MFD driver is implemented as UCLASS_NOP which
> > provides basic access functions and will enumerate its child nodes. The
> > device tree binding is the same as already acked by the linux kernel.
> 
> Ping. This is now 2.5 month old. It doesn't even touch anything outside
> my own board.

Priyanka?

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

RE: [PATCH 00/10] board: sl28: add sl28cpld support and board cleanups

[Priyanka Jain]

>-----Original Message-----
>From: Tom Rini <trini@konsulko.com>
>Sent: Monday, January 31, 2022 8:56 PM
>To: Michael Walle <michael@walle.cc>; Priyanka Jain <priyanka.jain@nxp.com>
>Cc: u-boot@lists.denx.de
>Subject: Re: [PATCH 00/10] board: sl28: add sl28cpld support and board
>cleanups
>
>On Mon, Jan 31, 2022 at 08:51:39AM +0100, Michael Walle wrote:
>> Am 2021-11-15 23:45, schrieb Michael Walle:
>> > Add support for the sl28cpld management controller found on this board.
>> > This is especially useful, because of the integrated watchdog in
>> > this controller. It is used to supervise the bootup of the
>> > bootloader and will automatically switch to the failsafe bootloader
>> > if u-boot didn't start.
>> > Up
>> > until now, we had to disable this feature, leaving a gap in the
>> > failsafe boot concept of this board. Now, with this driver in place,
>> > this watchdog can be enabled at reset and u-boot will disable it
>> > late during its startup.
>> >
>> > Additionally, add support for the GPIOs which are connected to this
>> > controller.
>> >
>> > Besides this support, there are various cleanups, like DRAM output
>> > fix, printing the version of the management controller and disabling
>> > the random MAC address generation.
>> >
>> > Implementationwise, the MFD driver is implemented as UCLASS_NOP
>> > which provides basic access functions and will enumerate its child
>> > nodes. The device tree binding is the same as already acked by the linux
>kernel.
>>
>> Ping. This is now 2.5 month old. It doesn't even touch anything
>> outside my own board.
>
>Priyanka?
>
>--
>Tom

I saw the series delegated to me today.
I will review and merge if no comments in my next pull-request.

Regards
Priyanka