[PATCH 0/2] bootcount: Replace I2C legacy implementation by driver model

[PATCH 0/2] bootcount: Replace I2C legacy implementation by driver model

[Philip Richard Oberfichtner]

The generic I2C bootcounter driver does not yet adhere to driver model.
This patchset intends to replace the legacy implementation.

There are currently no upstream boards using the driver, so it should be
safe to just remove it. For downstream users it should be straighforward to
switch to the new implementation.

Philip Richard Oberfichtner (2):
  bootcount: Remove legacy I2C driver
  bootcount: Add driver model I2C driver

 drivers/bootcount/Kconfig            |  34 +++-----
 drivers/bootcount/Makefile           |   2 +-
 drivers/bootcount/bootcount_dm_i2c.c | 126 +++++++++++++++++++++++++++
 drivers/bootcount/bootcount_i2c.c    |  43 ---------
 4 files changed, 140 insertions(+), 65 deletions(-)
 create mode 100644 drivers/bootcount/bootcount_dm_i2c.c
 delete mode 100644 drivers/bootcount/bootcount_i2c.c

-- 
2.42.0

[PATCH 1/2] bootcount: Remove legacy I2C driver

[Philip Richard Oberfichtner]

The legacy I2C bootcounter will hereby be removed and eventually
be replaced by a driver model implementation in the follow-up commit.

The legacy driver has the following drawbacks:
	- It's not adhering to the driver model
	- Settings are grabbed from Kconfig rather than device tree
	- i2c_{read,write} are being used instead of dm_i2c_{read,write}

Signed-off-by: Philip Richard Oberfichtner <pro@denx.de>
---
 drivers/bootcount/Kconfig         | 24 +++--------------
 drivers/bootcount/Makefile        |  1 -
 drivers/bootcount/bootcount_i2c.c | 43 -------------------------------
 3 files changed, 3 insertions(+), 65 deletions(-)
 delete mode 100644 drivers/bootcount/bootcount_i2c.c

diff --git a/drivers/bootcount/Kconfig b/drivers/bootcount/Kconfig
index 570252d186..7a2548ace2 100644
--- a/drivers/bootcount/Kconfig
+++ b/drivers/bootcount/Kconfig
@@ -79,14 +79,6 @@ config BOOTCOUNT_RAM
 	  Store the bootcount in DRAM protected against bit errors
 	  due to short power loss or holding a system in RESET.
 
-config BOOTCOUNT_I2C
-	bool "Boot counter on I2C device"
-	help
-	  Enable support for the bootcounter on an i2c (like RTC) device.
-	  CFG_SYS_I2C_RTC_ADDR = i2c chip address
-	  CONFIG_SYS_BOOTCOUNT_ADDR = i2c addr which is used for
-	                              the bootcounter.
-
 config BOOTCOUNT_AT91
 	bool "Boot counter for Atmel AT91SAM9XE"
 	depends on AT91SAM9XE
@@ -175,14 +167,6 @@ config BOOTCOUNT_BOOTLIMIT
 	  counter being cleared.
 	  If set to 0, do not set a boot limit in the environment.
 
-config BOOTCOUNT_ALEN
-	int "I2C address length"
-	default 1
-	depends on BOOTCOUNT_I2C
-	help
-	  Length of the the I2C address at SYS_BOOTCOUNT_ADDR for storing
-	  the boot counter.
-
 config SYS_BOOTCOUNT_SINGLEWORD
 	bool "Use single word to pack boot count and magic value"
 	depends on BOOTCOUNT_GENERIC
@@ -218,7 +202,7 @@ config SYS_BOOTCOUNT_ADDR
 	default 0x44E3E000 if BOOTCOUNT_AM33XX || BOOTCOUNT_AM33XX_NVMEM
 	default 0xE0115FF8 if ARCH_LS1043A || ARCH_LS1021A
 	depends on BOOTCOUNT_AM33XX || BOOTCOUNT_GENERIC || BOOTCOUNT_EXT || \
-		   BOOTCOUNT_I2C || BOOTCOUNT_AM33XX_NVMEM
+		   BOOTCOUNT_AM33XX_NVMEM
 	help
 	  Set the address used for reading and writing the boot counter.
 
@@ -226,13 +210,11 @@ config SYS_BOOTCOUNT_MAGIC
 	hex "Magic value for the boot counter"
 	default 0xB001C041 if BOOTCOUNT_GENERIC || BOOTCOUNT_EXT || \
 			      BOOTCOUNT_AM33XX || BOOTCOUNT_ENV || \
-			      BOOTCOUNT_RAM || BOOTCOUNT_I2C || \
-			      BOOTCOUNT_AT91 || DM_BOOTCOUNT
+			      BOOTCOUNT_RAM || BOOTCOUNT_AT91 || DM_BOOTCOUNT
 	default 0xB0 if BOOTCOUNT_AM33XX_NVMEM
 	depends on BOOTCOUNT_GENERIC || BOOTCOUNT_EXT || \
 		   BOOTCOUNT_AM33XX || BOOTCOUNT_ENV || \
-		   BOOTCOUNT_RAM || BOOTCOUNT_I2C || \
-		   BOOTCOUNT_AT91 || DM_BOOTCOUNT || \
+		   BOOTCOUNT_RAM || BOOTCOUNT_AT91 || DM_BOOTCOUNT || \
 		   BOOTCOUNT_AM33XX_NVMEM
 	help
 	  Set the magic value used for the boot counter.
diff --git a/drivers/bootcount/Makefile b/drivers/bootcount/Makefile
index b65959a384..d6d2389c16 100644
--- a/drivers/bootcount/Makefile
+++ b/drivers/bootcount/Makefile
@@ -6,7 +6,6 @@ obj-$(CONFIG_BOOTCOUNT_AT91)	+= bootcount_at91.o
 obj-$(CONFIG_BOOTCOUNT_AM33XX)	+= bootcount_davinci.o
 obj-$(CONFIG_BOOTCOUNT_RAM)	+= bootcount_ram.o
 obj-$(CONFIG_BOOTCOUNT_ENV)	+= bootcount_env.o
-obj-$(CONFIG_BOOTCOUNT_I2C)	+= bootcount_i2c.o
 obj-$(CONFIG_BOOTCOUNT_EXT)	+= bootcount_ext.o
 obj-$(CONFIG_BOOTCOUNT_AM33XX_NVMEM)	+= bootcount_nvmem.o
 
diff --git a/drivers/bootcount/bootcount_i2c.c b/drivers/bootcount/bootcount_i2c.c
deleted file mode 100644
index b3ac67ea35..0000000000
--- a/drivers/bootcount/bootcount_i2c.c
+++ /dev/null
@@ -1,43 +0,0 @@
-// SPDX-License-Identifier: GPL-2.0+
-/*
- * (C) Copyright 2013
- * Heiko Schocher, DENX Software Engineering, hs@denx.de.
- */
-
-#include <bootcount.h>
-#include <linux/compiler.h>
-#include <i2c.h>
-
-#define BC_MAGIC	0xbc
-
-void bootcount_store(ulong a)
-{
-	unsigned char buf[3];
-	int ret;
-
-	buf[0] = BC_MAGIC;
-	buf[1] = (a & 0xff);
-	ret = i2c_write(CFG_SYS_I2C_RTC_ADDR, CONFIG_SYS_BOOTCOUNT_ADDR,
-		  CONFIG_BOOTCOUNT_ALEN, buf, 2);
-	if (ret != 0)
-		puts("Error writing bootcount\n");
-}
-
-ulong bootcount_load(void)
-{
-	unsigned char buf[3];
-	int ret;
-
-	ret = i2c_read(CFG_SYS_I2C_RTC_ADDR, CONFIG_SYS_BOOTCOUNT_ADDR,
-		       CONFIG_BOOTCOUNT_ALEN, buf, 2);
-	if (ret != 0) {
-		puts("Error loading bootcount\n");
-		return 0;
-	}
-	if (buf[0] == BC_MAGIC)
-		return buf[1];
-
-	bootcount_store(0);
-
-	return 0;
-}
-- 
2.42.0

[PATCH 2/2] bootcount: Add driver model I2C driver

[Philip Richard Oberfichtner]

This adds a generic I2C bootcounter adhering to driver model to replace
the previously removed legacy implementation.

There is no change in functionality, it can be used on any I2C device.
The device tree configuration may look like this for example:

	bootcount {
		compatible = "u-boot,bootcount-i2c";
		i2c-bus = <&i2c1>;
		address = <0x52>;
		offset = <0x11>;
	};

Signed-off-by: Philip Richard Oberfichtner <pro@denx.de>
---
 drivers/bootcount/Kconfig            |  10 +++
 drivers/bootcount/Makefile           |   1 +
 drivers/bootcount/bootcount_dm_i2c.c | 126 +++++++++++++++++++++++++++
 3 files changed, 137 insertions(+)
 create mode 100644 drivers/bootcount/bootcount_dm_i2c.c

diff --git a/drivers/bootcount/Kconfig b/drivers/bootcount/Kconfig
index 7a2548ace2..1cf1de2b6d 100644
--- a/drivers/bootcount/Kconfig
+++ b/drivers/bootcount/Kconfig
@@ -109,6 +109,16 @@ config DM_BOOTCOUNT_RTC
 	  Accesses to the backing store are performed using the write16
 	  and read16 ops of DM RTC devices.
 
+config DM_BOOTCOUNT_I2C
+	bool "Driver Model boot counter on I2C device"
+	depends on DM_I2C
+	help
+	  Enable support for the bootcounter on a generic i2c device, like a
+	  RTC or PMIC. This requires an 'i2c-bus', the i2c chip 'address' and
+	  the 'offset' to read to and write from. All of the three settings are
+	  defined as device tree properties using the "u-boot,bootcount-i2c"
+	  compatible string.
+
 config DM_BOOTCOUNT_I2C_EEPROM
 	bool "Support i2c eeprom devices as a backing store for bootcount"
 	depends on I2C_EEPROM
diff --git a/drivers/bootcount/Makefile b/drivers/bootcount/Makefile
index d6d2389c16..e7771f5b36 100644
--- a/drivers/bootcount/Makefile
+++ b/drivers/bootcount/Makefile
@@ -13,5 +13,6 @@ obj-$(CONFIG_DM_BOOTCOUNT)      += bootcount-uclass.o
 obj-$(CONFIG_DM_BOOTCOUNT_PMIC_PFUZE100) += pmic_pfuze100.o
 obj-$(CONFIG_DM_BOOTCOUNT_RTC)  += rtc.o
 obj-$(CONFIG_DM_BOOTCOUNT_I2C_EEPROM)	+= i2c-eeprom.o
+obj-$(CONFIG_DM_BOOTCOUNT_I2C)	+= bootcount_dm_i2c.o
 obj-$(CONFIG_DM_BOOTCOUNT_SPI_FLASH)	+= spi-flash.o
 obj-$(CONFIG_DM_BOOTCOUNT_SYSCON) += bootcount_syscon.o
diff --git a/drivers/bootcount/bootcount_dm_i2c.c b/drivers/bootcount/bootcount_dm_i2c.c
new file mode 100644
index 0000000000..227641f77e
--- /dev/null
+++ b/drivers/bootcount/bootcount_dm_i2c.c
@@ -0,0 +1,126 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * (C) Copyright 2023
+ * Philip Richard Oberfichtner <pro@denx.de>
+ *
+ * Based on previous work from Heiko Schocher (legacy bootcount_i2c.c driver)
+ */
+
+#include <bootcount.h>
+#include <common.h>
+#include <dm.h>
+#include <i2c.h>
+
+#define BC_MAGIC	0x55
+
+struct bootcount_i2c_priv {
+	struct udevice *chip;
+	unsigned int offset;
+};
+
+static int bootcount_i2c_set(struct udevice *dev, const u32 val)
+{
+	int ret;
+	struct bootcount_i2c_priv *priv = dev_get_priv(dev);
+
+	ret = dm_i2c_reg_write(priv->chip, priv->offset, BC_MAGIC);
+	if (ret < 0)
+		goto err_exit;
+
+	ret = dm_i2c_reg_write(priv->chip, priv->offset + 1, val & 0xff);
+	if (ret < 0)
+		goto err_exit;
+
+	return 0;
+
+err_exit:
+	log_debug("%s: Error writing to I2C device (%d)\n", __func__, ret);
+	return ret;
+}
+
+static int bootcount_i2c_get(struct udevice *dev, u32 *val)
+{
+	int ret;
+	struct bootcount_i2c_priv *priv = dev_get_priv(dev);
+
+	ret = dm_i2c_reg_read(priv->chip, priv->offset);
+	if (ret < 0)
+		goto err_exit;
+
+	if ((ret & 0xff) != BC_MAGIC) {
+		log_debug("%s: Invalid Magic, reset bootcounter.\n", __func__);
+		*val = 0;
+		return bootcount_i2c_set(dev, 0);
+	}
+
+	ret = dm_i2c_reg_read(priv->chip, priv->offset + 1);
+	if (ret < 0)
+		goto err_exit;
+
+	*val = ret;
+	return 0;
+
+err_exit:
+	log_debug("%s: Error reading from I2C device (%d)\n", __func__, ret);
+	return ret;
+}
+
+static int bootcount_i2c_probe(struct udevice *dev)
+{
+	struct bootcount_i2c_priv *priv = dev_get_priv(dev);
+	struct ofnode_phandle_args phandle_args;
+	struct udevice *bus;
+	unsigned int addr;
+	int ret;
+
+	ret = dev_read_u32(dev, "offset", &priv->offset);
+	if (ret) {
+		log_debug("%s: Unable to get offset\n", __func__);
+		return ret;
+	}
+
+	ret = dev_read_u32(dev, "address", &addr);
+	if (ret) {
+		log_debug("%s: Unable to get chip address\n", __func__);
+		return ret;
+	}
+
+	ret = dev_read_phandle_with_args(dev, "i2c-bus", NULL, 0, 0, &phandle_args);
+	if (ret) {
+		log_debug("%s: Unable to get phandle\n", __func__);
+		return ret;
+	}
+
+	ret = uclass_get_device_by_ofnode(UCLASS_I2C, phandle_args.node, &bus);
+	if (ret) {
+		log_debug("%s: Unable to get i2c bus\n", __func__);
+		return ret;
+	}
+
+	ret = i2c_get_chip(bus, addr, 1, &priv->chip);
+	if (ret) {
+		log_debug("%s: Unable to get i2c chip\n", __func__);
+		return ret;
+	}
+
+	return 0;
+}
+
+static const struct bootcount_ops bootcount_i2c_ops = {
+	.get = bootcount_i2c_get,
+	.set = bootcount_i2c_set,
+};
+
+static const struct udevice_id bootcount_i2c_ids[] = {
+	{ .compatible = "u-boot,bootcount-i2c" },
+	{ }
+};
+
+U_BOOT_DRIVER(bootcount_i2c) = {
+	.name		= "bootcount-i2c",
+	.id		= UCLASS_BOOTCOUNT,
+	.priv_auto	= sizeof(struct bootcount_i2c_priv),
+	.probe		= bootcount_i2c_probe,
+	.of_match	= bootcount_i2c_ids,
+	.ops		= &bootcount_i2c_ops,
+};
-- 
2.42.0

Re: [PATCH 1/2] bootcount: Remove legacy I2C driver

[Heiko Schocher]

Hello Philip,

On 13.10.23 11:43, Philip Richard Oberfichtner wrote:
> The legacy I2C bootcounter will hereby be removed and eventually
> be replaced by a driver model implementation in the follow-up commit.
> 
> The legacy driver has the following drawbacks:
> 	- It's not adhering to the driver model
> 	- Settings are grabbed from Kconfig rather than device tree
> 	- i2c_{read,write} are being used instead of dm_i2c_{read,write}
> 
> Signed-off-by: Philip Richard Oberfichtner <pro@denx.de>
> ---
>  drivers/bootcount/Kconfig         | 24 +++--------------
>  drivers/bootcount/Makefile        |  1 -
>  drivers/bootcount/bootcount_i2c.c | 43 -------------------------------
>  3 files changed, 3 insertions(+), 65 deletions(-)
>  delete mode 100644 drivers/bootcount/bootcount_i2c.c

Hmm.. I find some boards in mainline which still use this driver:

u-boot  [master] $ grep -lr BOOTCOUNT_I2C .
./configs/sandbox_defconfig
./configs/mx53ppd_defconfig
./configs/ge_bx50v3_defconfig
[...]

So your remove patch will break them ... okay sandbox should be
easy to convert to your DM approach patch from this series.

bye,
Heiko
-- 
DENX Software Engineering GmbH,      Managing Director: Erika Unter
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-52   Fax: +49-8142-66989-80   Email: hs@denx.de

Re: [PATCH 2/2] bootcount: Add driver model I2C driver

[Heiko Schocher]

Hello Philip,

On 13.10.23 11:43, Philip Richard Oberfichtner wrote:
> This adds a generic I2C bootcounter adhering to driver model to replace
> the previously removed legacy implementation.
> 
> There is no change in functionality, it can be used on any I2C device.
> The device tree configuration may look like this for example:
> 
> 	bootcount {
> 		compatible = "u-boot,bootcount-i2c";
> 		i2c-bus = <&i2c1>;
> 		address = <0x52>;

Hmm.. do we really need this here with DTS. Why not using a phandle
to a real i2c device? Something like this for example:

		i2cbcdev = &i2c_rtc;

with

&i2c1 {
        i2c_rtc: rtc@68 {
[...]

and so there is no need for knowing the bus and address ...

> 		offset = <0x11>;
> 	};
> 
> Signed-off-by: Philip Richard Oberfichtner <pro@denx.de>
> ---
>  drivers/bootcount/Kconfig            |  10 +++
>  drivers/bootcount/Makefile           |   1 +
>  drivers/bootcount/bootcount_dm_i2c.c | 126 +++++++++++++++++++++++++++
>  3 files changed, 137 insertions(+)
>  create mode 100644 drivers/bootcount/bootcount_dm_i2c.c
> 
> diff --git a/drivers/bootcount/Kconfig b/drivers/bootcount/Kconfig
> index 7a2548ace2..1cf1de2b6d 100644
> --- a/drivers/bootcount/Kconfig
> +++ b/drivers/bootcount/Kconfig
> @@ -109,6 +109,16 @@ config DM_BOOTCOUNT_RTC
>  	  Accesses to the backing store are performed using the write16
>  	  and read16 ops of DM RTC devices.
>  
> +config DM_BOOTCOUNT_I2C
> +	bool "Driver Model boot counter on I2C device"
> +	depends on DM_I2C
> +	help
> +	  Enable support for the bootcounter on a generic i2c device, like a
> +	  RTC or PMIC. This requires an 'i2c-bus', the i2c chip 'address' and
> +	  the 'offset' to read to and write from. All of the three settings are
> +	  defined as device tree properties using the "u-boot,bootcount-i2c"
> +	  compatible string.
> +
>  config DM_BOOTCOUNT_I2C_EEPROM
>  	bool "Support i2c eeprom devices as a backing store for bootcount"
>  	depends on I2C_EEPROM
> diff --git a/drivers/bootcount/Makefile b/drivers/bootcount/Makefile
> index d6d2389c16..e7771f5b36 100644
> --- a/drivers/bootcount/Makefile
> +++ b/drivers/bootcount/Makefile
> @@ -13,5 +13,6 @@ obj-$(CONFIG_DM_BOOTCOUNT)      += bootcount-uclass.o
>  obj-$(CONFIG_DM_BOOTCOUNT_PMIC_PFUZE100) += pmic_pfuze100.o
>  obj-$(CONFIG_DM_BOOTCOUNT_RTC)  += rtc.o
>  obj-$(CONFIG_DM_BOOTCOUNT_I2C_EEPROM)	+= i2c-eeprom.o
> +obj-$(CONFIG_DM_BOOTCOUNT_I2C)	+= bootcount_dm_i2c.o
>  obj-$(CONFIG_DM_BOOTCOUNT_SPI_FLASH)	+= spi-flash.o
>  obj-$(CONFIG_DM_BOOTCOUNT_SYSCON) += bootcount_syscon.o
> diff --git a/drivers/bootcount/bootcount_dm_i2c.c b/drivers/bootcount/bootcount_dm_i2c.c
> new file mode 100644
> index 0000000000..227641f77e
> --- /dev/null
> +++ b/drivers/bootcount/bootcount_dm_i2c.c
> @@ -0,0 +1,126 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * (C) Copyright 2023
> + * Philip Richard Oberfichtner <pro@denx.de>
> + *
> + * Based on previous work from Heiko Schocher (legacy bootcount_i2c.c driver)
> + */
> +
> +#include <bootcount.h>
> +#include <common.h>
> +#include <dm.h>
> +#include <i2c.h>
> +
> +#define BC_MAGIC	0x55
> +
> +struct bootcount_i2c_priv {
> +	struct udevice *chip;

may rename chip to i2cbcdev or bcdev ?

> +	unsigned int offset;
> +};
> +
> +static int bootcount_i2c_set(struct udevice *dev, const u32 val)
> +{
> +	int ret;
> +	struct bootcount_i2c_priv *priv = dev_get_priv(dev);
> +
> +	ret = dm_i2c_reg_write(priv->chip, priv->offset, BC_MAGIC);
> +	if (ret < 0)
> +		goto err_exit;
> +
> +	ret = dm_i2c_reg_write(priv->chip, priv->offset + 1, val & 0xff);
> +	if (ret < 0)
> +		goto err_exit;
> +
> +	return 0;
> +
> +err_exit:
> +	log_debug("%s: Error writing to I2C device (%d)\n", __func__, ret);
> +	return ret;
> +}
> +
> +static int bootcount_i2c_get(struct udevice *dev, u32 *val)
> +{
> +	int ret;
> +	struct bootcount_i2c_priv *priv = dev_get_priv(dev);
> +
> +	ret = dm_i2c_reg_read(priv->chip, priv->offset);
> +	if (ret < 0)
> +		goto err_exit;
> +
> +	if ((ret & 0xff) != BC_MAGIC) {
> +		log_debug("%s: Invalid Magic, reset bootcounter.\n", __func__);
> +		*val = 0;
> +		return bootcount_i2c_set(dev, 0);
> +	}
> +
> +	ret = dm_i2c_reg_read(priv->chip, priv->offset + 1);
> +	if (ret < 0)
> +		goto err_exit;
> +
> +	*val = ret;
> +	return 0;
> +
> +err_exit:
> +	log_debug("%s: Error reading from I2C device (%d)\n", __func__, ret);
> +	return ret;
> +}
> +
> +static int bootcount_i2c_probe(struct udevice *dev)
> +{
> +	struct bootcount_i2c_priv *priv = dev_get_priv(dev);
> +	struct ofnode_phandle_args phandle_args;
> +	struct udevice *bus;
> +	unsigned int addr;
> +	int ret;
> +
> +	ret = dev_read_u32(dev, "offset", &priv->offset);
> +	if (ret) {
> +		log_debug("%s: Unable to get offset\n", __func__);
> +		return ret;
> +	}
> +
> +	ret = dev_read_u32(dev, "address", &addr);
> +	if (ret) {
> +		log_debug("%s: Unable to get chip address\n", __func__);
> +		return ret;
> +	}
> +
> +	ret = dev_read_phandle_with_args(dev, "i2c-bus", NULL, 0, 0, &phandle_args);
> +	if (ret) {
> +		log_debug("%s: Unable to get phandle\n", __func__);
> +		return ret;
> +	}
> +
> +	ret = uclass_get_device_by_ofnode(UCLASS_I2C, phandle_args.node, &bus);
> +	if (ret) {
> +		log_debug("%s: Unable to get i2c bus\n", __func__);
> +		return ret;
> +	}
> +
> +	ret = i2c_get_chip(bus, addr, 1, &priv->chip);
> +	if (ret) {
> +		log_debug("%s: Unable to get i2c chip\n", __func__);
> +		return ret;
> +	}

when you use a phandle, you can replace the part from reading "offset"
with this:

	uclass_get_device_by_phandle(UCLASS_I2C, dev, "i2cbcdev", &priv->bcdev);

of course plus error checking...

> +
> +	return 0;
> +}
> +
> +static const struct bootcount_ops bootcount_i2c_ops = {
> +	.get = bootcount_i2c_get,
> +	.set = bootcount_i2c_set,
> +};
> +
> +static const struct udevice_id bootcount_i2c_ids[] = {
> +	{ .compatible = "u-boot,bootcount-i2c" },
> +	{ }
> +};
> +
> +U_BOOT_DRIVER(bootcount_i2c) = {
> +	.name		= "bootcount-i2c",
> +	.id		= UCLASS_BOOTCOUNT,
> +	.priv_auto	= sizeof(struct bootcount_i2c_priv),
> +	.probe		= bootcount_i2c_probe,
> +	.of_match	= bootcount_i2c_ids,
> +	.ops		= &bootcount_i2c_ops,
> +};
> 

bye,
Heiko
-- 
DENX Software Engineering GmbH,      Managing Director: Erika Unter
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-52   Fax: +49-8142-66989-80   Email: hs@denx.de

Re: [PATCH 1/2] bootcount: Remove legacy I2C driver

[Philip Oberfichtner]

Hi Heiko,

Thank you for reviewing.

On Fri, Oct 13, 2023 at 01:21:41PM +0200, Heiko Schocher wrote:
> [...]
>
> Hmm.. I find some boards in mainline which still use this driver:
> 
> u-boot  [master] $ grep -lr BOOTCOUNT_I2C .
> ./configs/sandbox_defconfig
> ./configs/mx53ppd_defconfig
> ./configs/ge_bx50v3_defconfig
> [...]
> 
> So your remove patch will break them ... okay sandbox should be
> easy to convert to your DM approach patch from this series.
> 


$ git grep -r BOOTCOUNT_I2C
configs/ge_bx50v3_defconfig:CONFIG_DM_BOOTCOUNT_I2C_EEPROM=y
configs/mx53ppd_defconfig:CONFIG_DM_BOOTCOUNT_I2C_EEPROM=y
configs/sandbox_defconfig:CONFIG_DM_BOOTCOUNT_I2C_EEPROM=y

Those boards use CONFIG_DM_BOOTCOUNT_I2C_EEPROM, which is not touched by
this removal, is it?

Best regards,
Philip


> bye,
> Heiko
> -- 
> DENX Software Engineering GmbH,      Managing Director: Erika Unter
> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
> Phone: +49-8142-66989-52   Fax: +49-8142-66989-80   Email: hs@denx.de

Re: [PATCH 1/2] bootcount: Remove legacy I2C driver

[Heiko Schocher]

Hello Phillip,

On 13.10.23 13:35, Philip Oberfichtner wrote:
> Hi Heiko,
> 
> Thank you for reviewing.
> 
> On Fri, Oct 13, 2023 at 01:21:41PM +0200, Heiko Schocher wrote:
>> [...]
>>
>> Hmm.. I find some boards in mainline which still use this driver:
>>
>> u-boot  [master] $ grep -lr BOOTCOUNT_I2C .
>> ./configs/sandbox_defconfig
>> ./configs/mx53ppd_defconfig
>> ./configs/ge_bx50v3_defconfig
>> [...]
>>
>> So your remove patch will break them ... okay sandbox should be
>> easy to convert to your DM approach patch from this series.
>>
> 
> 
> $ git grep -r BOOTCOUNT_I2C
> configs/ge_bx50v3_defconfig:CONFIG_DM_BOOTCOUNT_I2C_EEPROM=y
> configs/mx53ppd_defconfig:CONFIG_DM_BOOTCOUNT_I2C_EEPROM=y
> configs/sandbox_defconfig:CONFIG_DM_BOOTCOUNT_I2C_EEPROM=y
> 
> Those boards use CONFIG_DM_BOOTCOUNT_I2C_EEPROM, which is not touched by
> this removal, is it?

Heh, yes, sorry!

bye,
Heiko
-- 
DENX Software Engineering GmbH,      Managing Director: Erika Unter
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-52   Fax: +49-8142-66989-80   Email: hs@denx.de

Re: [PATCH 2/2] bootcount: Add driver model I2C driver

[Philip Oberfichtner]

Hello Heiko,

On Fri, Oct 13, 2023 at 01:28:47PM +0200, Heiko Schocher wrote:
> [...]
> > 
> > 	bootcount {
> > 		compatible = "u-boot,bootcount-i2c";
> > 		i2c-bus = <&i2c1>;
> > 		address = <0x52>;
> 
> Hmm.. do we really need this here with DTS. Why not using a phandle
> to a real i2c device? Something like this for example:
> 
> 		i2cbcdev = &i2c_rtc;
> 
> with
> 
> &i2c1 {
>         i2c_rtc: rtc@68 {
> [...]
> 
> and so there is no need for knowing the bus and address ...
> 

Yeah I agree that would be much better, but ...

> [...]
> 
> when you use a phandle, you can replace the part from reading "offset"
> with this:
> 
> 	uclass_get_device_by_phandle(UCLASS_I2C, dev, "i2cbcdev", &priv->bcdev);
> 
> of course plus error checking...

This does not work, UCLASS_I2C is used for the *bus above* the device we
actually want.

Next thing I thougt about: Why not use the UCLASS_I2C_GENERIC. But this
expects a "i2c-chip" compatible string, which our rtc or whatever device
obviously does not have.

I think using UCLASS_I2C_GENERIC might indeed be the best approach.
Maybe using something like 'device_bind_driver()'. But again, this
expects the parent device to be there already as far as I can tell.

Any suggestions?

Best regards,
Philip


> 
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct bootcount_ops bootcount_i2c_ops = {
> > +	.get = bootcount_i2c_get,
> > +	.set = bootcount_i2c_set,
> > +};
> > +
> > +static const struct udevice_id bootcount_i2c_ids[] = {
> > +	{ .compatible = "u-boot,bootcount-i2c" },
> > +	{ }
> > +};
> > +
> > +U_BOOT_DRIVER(bootcount_i2c) = {
> > +	.name		= "bootcount-i2c",
> > +	.id		= UCLASS_BOOTCOUNT,
> > +	.priv_auto	= sizeof(struct bootcount_i2c_priv),
> > +	.probe		= bootcount_i2c_probe,
> > +	.of_match	= bootcount_i2c_ids,
> > +	.ops		= &bootcount_i2c_ops,
> > +};
> > 
> 
> bye,
> Heiko
> -- 
> DENX Software Engineering GmbH,      Managing Director: Erika Unter
> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
> Phone: +49-8142-66989-52   Fax: +49-8142-66989-80   Email: hs@denx.de

Re: [PATCH 2/2] bootcount: Add driver model I2C driver

[Philip Oberfichtner]

Hi Simon,

maybe you can give me a hint on how to implement this cleanly in driver
model?

To sum it up, I'd like to have a phandle pointing to *any* I2C device,
not knowing which UCLASS actually fits. Then the device and the parent
bus should be probed/prepared such that dm_i2c_read() can be used.

Any ideas on this?

Best regards,
Philip


--
=====================================================================
DENX Software Engineering GmbH,        Managing Director: Erika Unter
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-22  Fax: +49-8142-66989-80   Email: pro@denx.de
=====================================================================

On Fri, Oct 13, 2023 at 02:58:04PM +0200, Philip Oberfichtner wrote:
> Hello Heiko,
> 
> On Fri, Oct 13, 2023 at 01:28:47PM +0200, Heiko Schocher wrote:
> > [...]
> > > 
> > > 	bootcount {
> > > 		compatible = "u-boot,bootcount-i2c";
> > > 		i2c-bus = <&i2c1>;
> > > 		address = <0x52>;
> > 
> > Hmm.. do we really need this here with DTS. Why not using a phandle
> > to a real i2c device? Something like this for example:
> > 
> > 		i2cbcdev = &i2c_rtc;
> > 
> > with
> > 
> > &i2c1 {
> >         i2c_rtc: rtc@68 {
> > [...]
> > 
> > and so there is no need for knowing the bus and address ...
> > 
> 
> Yeah I agree that would be much better, but ...
> 
> > [...]
> > 
> > when you use a phandle, you can replace the part from reading "offset"
> > with this:
> > 
> > 	uclass_get_device_by_phandle(UCLASS_I2C, dev, "i2cbcdev", &priv->bcdev);
> > 
> > of course plus error checking...
> 
> This does not work, UCLASS_I2C is used for the *bus above* the device we
> actually want.
> 
> Next thing I thougt about: Why not use the UCLASS_I2C_GENERIC. But this
> expects a "i2c-chip" compatible string, which our rtc or whatever device
> obviously does not have.
> 
> I think using UCLASS_I2C_GENERIC might indeed be the best approach.
> Maybe using something like 'device_bind_driver()'. But again, this
> expects the parent device to be there already as far as I can tell.
> 
> Any suggestions?
> 
> Best regards,
> Philip
> 
> 
> > 
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static const struct bootcount_ops bootcount_i2c_ops = {
> > > +	.get = bootcount_i2c_get,
> > > +	.set = bootcount_i2c_set,
> > > +};
> > > +
> > > +static const struct udevice_id bootcount_i2c_ids[] = {
> > > +	{ .compatible = "u-boot,bootcount-i2c" },
> > > +	{ }
> > > +};
> > > +
> > > +U_BOOT_DRIVER(bootcount_i2c) = {
> > > +	.name		= "bootcount-i2c",
> > > +	.id		= UCLASS_BOOTCOUNT,
> > > +	.priv_auto	= sizeof(struct bootcount_i2c_priv),
> > > +	.probe		= bootcount_i2c_probe,
> > > +	.of_match	= bootcount_i2c_ids,
> > > +	.ops		= &bootcount_i2c_ops,
> > > +};
> > > 
> > 
> > bye,
> > Heiko
> > -- 
> > DENX Software Engineering GmbH,      Managing Director: Erika Unter
> > HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
> > Phone: +49-8142-66989-52   Fax: +49-8142-66989-80   Email: hs@denx.de

Re: [PATCH 2/2] bootcount: Add driver model I2C driver

[Simon Glass]

Hi Philip,

On Tue, 17 Oct 2023 at 06:57, Philip Oberfichtner <pro@denx.de> wrote:
>
> Hi Simon,
>
> maybe you can give me a hint on how to implement this cleanly in driver
> model?
>
> To sum it up, I'd like to have a phandle pointing to *any* I2C device,
> not knowing which UCLASS actually fits. Then the device and the parent
> bus should be probed/prepared such that dm_i2c_read() can be used.
>
> Any ideas on this?

I suggest a phandle to the i2c device.

You can use oftree_get_by_phandle() to get the node and then
device_find_global_by_ofnode() to get the device.

This is expensive, although eventually I suspect we will fix that with
OF_LIVE. I think it should be implemented as a new function in i2c.h
so we can change the impl later easily.

If you want to be more efficient you could do something like:

int phandle = ??

struct uclass *uc;
struct udevice *bus;

uclass_id_foreach_dev(UCLASS_I2C, bus, uc) {
   struct udevice *dev;

   device_foreach_child(dev, bus) {
      if (!dev_read_u32(dev, "phandle", &val) && val == phandle)
         return dev;
   }
}

but honestly now I look at it, that is awful. We try to avoid exposing
the internals of phandle because it allows us to (one day) maintain a
list of them.

[..]

Regards,
Simon

Re: [PATCH 2/2] bootcount: Add driver model I2C driver

[Heiko Schocher]

Hello Simon,

On 18.10.23 05:33, Simon Glass wrote:
> Hi Philip,
> 
> On Tue, 17 Oct 2023 at 06:57, Philip Oberfichtner <pro@denx.de> wrote:
>>
>> Hi Simon,
>>
>> maybe you can give me a hint on how to implement this cleanly in driver
>> model?
>>
>> To sum it up, I'd like to have a phandle pointing to *any* I2C device,
>> not knowing which UCLASS actually fits. Then the device and the parent
>> bus should be probed/prepared such that dm_i2c_read() can be used.
>>
>> Any ideas on this?
> 
> I suggest a phandle to the i2c device.

Yep!

> You can use oftree_get_by_phandle() to get the node and then
> device_find_global_by_ofnode() to get the device.
> 
> This is expensive, although eventually I suspect we will fix that with
> OF_LIVE. I think it should be implemented as a new function in i2c.h
> so we can change the impl later easily.

Yes, please.

> If you want to be more efficient you could do something like:
> 
> int phandle = ??
> 
> struct uclass *uc;
> struct udevice *bus;
> 
> uclass_id_foreach_dev(UCLASS_I2C, bus, uc) {
>    struct udevice *dev;
> 
>    device_foreach_child(dev, bus) {
>       if (!dev_read_u32(dev, "phandle", &val) && val == phandle)
>          return dev;
>    }
> }
> 
> but honestly now I look at it, that is awful. We try to avoid exposing
> the internals of phandle because it allows us to (one day) maintain a
> list of them.

Yes, please not ... a list of phandles would be great we can than
walk through, yes, may in future...

May Philip can use uclass_get_device_by_phandle and try a list of
possible UCLASS candidates, like UCLASS_RTC, UCLASS_I2C_EEPROM,
UCLASS_POWER,... and if found, check if parent is UCLASS_I2C...

may not so expensive ...

bye,
Heiko
-- 
DENX Software Engineering GmbH,      Managing Director: Erika Unter
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-52   Fax: +49-8142-66989-80   Email: hs@denx.de

Re: [PATCH 2/2] bootcount: Add driver model I2C driver

[Philip Oberfichtner]

Hi Heiko,

On Wed, Oct 18, 2023 at 06:31:57AM +0200, Heiko Schocher wrote:
> [...]
> 
> May Philip can use uclass_get_device_by_phandle and try a list of
> possible UCLASS candidates, like UCLASS_RTC, UCLASS_I2C_EEPROM,
> UCLASS_POWER,... and if found, check if parent is UCLASS_I2C...
> 
> may not so expensive ...

Looks more cheap and elegant than the other variants indeed. But I'm not
sure if we can maintain generality using this approach. 

What if the specific I2C driver is not included in the build, because
the devices "actual" functionality is not required? Or what if a driver
for the specific device does not even exist in U-Boot?

Wouldn't the device fail to probe then?

Please correct me if I'm wrong, I'd give it a shot in that case.
Otherwise I'll try it with the aforementioned
device_find_global_by_ofnode() followed by device_bind_driver() using
UCLASS_I2C_GENERIC (I hope that works).

Best regards,
Philip


> 
> bye,
> Heiko
> -- 
> DENX Software Engineering GmbH,      Managing Director: Erika Unter
> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
> Phone: +49-8142-66989-52   Fax: +49-8142-66989-80   Email: hs@denx.de

-- 
=====================================================================
DENX Software Engineering GmbH,        Managing Director: Erika Unter
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-22  Fax: +49-8142-66989-80   Email: pro@denx.de
=====================================================================

Re: [PATCH 2/2] bootcount: Add driver model I2C driver

[Simon Glass]

Hi Philip,

On Wed, 18 Oct 2023 at 05:00, Philip Oberfichtner <pro@denx.de> wrote:
>
> Hi Heiko,
>
> On Wed, Oct 18, 2023 at 06:31:57AM +0200, Heiko Schocher wrote:
> > [...]
> >
> > May Philip can use uclass_get_device_by_phandle and try a list of
> > possible UCLASS candidates, like UCLASS_RTC, UCLASS_I2C_EEPROM,
> > UCLASS_POWER,... and if found, check if parent is UCLASS_I2C...
> >
> > may not so expensive ...
>
> Looks more cheap and elegant than the other variants indeed. But I'm not
> sure if we can maintain generality using this approach.
>
> What if the specific I2C driver is not included in the build, because
> the devices "actual" functionality is not required?

Then the device will not be bound and the function will fail. There needs
to be a node and a bound device.

> Or what if a driver
> for the specific device does not even exist in U-Boot?
>
> Wouldn't the device fail to probe then?

The DT should specify the compatible string so the correct i2c driver is
used. If there isn't one,I believe it uses I2C_GENERIC but you'll need to
check it.

>
> Please correct me if I'm wrong, I'd give it a shot in that case.
> Otherwise I'll try it with the aforementioned
> device_find_global_by_ofnode() followed by device_bind_driver() using
> UCLASS_I2C_GENERIC (I hope that works).

That is the same approach, I think. But anyway I think Heiko knows more
about this than me.

Regards,
Simon

Re: [PATCH 2/2] bootcount: Add driver model I2C driver

[Heiko Schocher]

Hello Philip,

On 18.10.23 18:10, Simon Glass wrote:
> Hi Philip,
> 
> On Wed, 18 Oct 2023 at 05:00, Philip Oberfichtner <pro@denx.de <mailto:pro@denx.de>> wrote:
>>
>> Hi Heiko,
>>
>> On Wed, Oct 18, 2023 at 06:31:57AM +0200, Heiko Schocher wrote:
>> > [...]
>> >
>> > May Philip can use uclass_get_device_by_phandle and try a list of
>> > possible UCLASS candidates, like UCLASS_RTC, UCLASS_I2C_EEPROM,
>> > UCLASS_POWER,... and if found, check if parent is UCLASS_I2C...
>> >
>> > may not so expensive ...
>>
>> Looks more cheap and elegant than the other variants indeed. But I'm not
>> sure if we can maintain generality using this approach.
>>
>> What if the specific I2C driver is not included in the build, because
>> the devices "actual" functionality is not required?
> 
> Then the device will not be bound and the function will fail. There needs to be a node and a bound
> device.

Yep.

>> Or what if a driver
>> for the specific device does not even exist in U-Boot?
>>
>> Wouldn't the device fail to probe then?
> 
> The DT should specify the compatible string so the correct i2c driver is used. If there isn't one,I
> believe it uses I2C_GENERIC but you'll need to check it.

Yes, needs a check.

But... that is the real issue with that approach (more or less with
all bootcounter drivers?) ... if you assume that the device driver
is not there, may there is missing device init/probe stuff, so also
may the bootcounter will not work!

The i2c device should know, that some registers, mem (or whatever) is
used for bootcounter function, so that it does not use this space for
other device stuff ... to make bootcounters correct, we need something
like a MFD approach here ... and register a bootcounter within that...
so init/probe stuff is always done, and registers/mem is reserved ...

But unfortunately we do not have this...

>> Please correct me if I'm wrong, I'd give it a shot in that case.
>> Otherwise I'll try it with the aforementioned
>> device_find_global_by_ofnode() followed by device_bind_driver() using
>> UCLASS_I2C_GENERIC (I hope that works).
> 
> That is the same approach, I think. But anyway I think Heiko knows more about this than me.

Hmm... yes, with UCLASS_I2C_GENERIC it could also work, but see my
comment above ...

bye,
Heiko
-- 
DENX Software Engineering GmbH,      Managing Director: Erika Unter
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-52   Fax: +49-8142-66989-80   Email: hs@denx.de