[PATCH 0/2] bootcount: Replace I2C legacy implementation by driver model
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
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; -}
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
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
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
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, +};
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 aRTC or PMIC. This requires an 'i2c-bus', the i2c chip 'address' andthe 'offset' to read to and write from. All of the three settings aredefined 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
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
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
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
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
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
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
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
participants (4)
-
Heiko Schocher -
Philip Oberfichtner -
Philip Richard Oberfichtner
-
Simon Glass