* [U-Boot] [PATCH 0/3] Update RV3029 driver to DM and add DM-backed bootcount support
@ 2018-08-14 9:50 Philipp Tomsich
2018-08-14 9:51 ` [U-Boot] [PATCH 1/3] rtc: rv3029: add to Kconfig Philipp Tomsich
` (2 more replies)
0 siblings, 3 replies; 13+ messages in thread
From: Philipp Tomsich @ 2018-08-14 9:50 UTC (permalink / raw)
To: u-boot
On one of our application-specific carrier boards, a MicroCrystal
RV3029 acts as an off-module battery-backed RTC for the RK3399-Q7.
The RV3029 is intended both to provide RTC services (to Linux) and
to store the bootcount in its battery-backed SRAM section.
To support this use-case, this series adds the following:
* replaces the existing RV3029 driver by a DM-based one (note that the
existing driver appears unused and didn't even have a Kconfig entry)
that closely mirrors its incarnation in Linux 4.17
* adds a bootcount-method interfacing back into DM devices (implementing
support for the RTC uclass as of now).
Philipp Tomsich (3):
rtc: rv3029: add to Kconfig
rtc: rv3029: update to support DM and sync with Linux 4.17
bootcount: add DM-based backing store for bootcount
doc/device-tree-bindings/chosen.txt | 27 ++
drivers/bootcount/Kconfig | 12 +
drivers/bootcount/Makefile | 1 +
drivers/bootcount/bootcount_dm.c | 93 ++++++
drivers/rtc/Kconfig | 10 +
drivers/rtc/rv3029.c | 579 +++++++++++++++++++++++++++---------
scripts/config_whitelist.txt | 1 -
7 files changed, 585 insertions(+), 138 deletions(-)
create mode 100644 drivers/bootcount/bootcount_dm.c
--
2.1.4
^ permalink raw reply [flat|nested] 13+ messages in thread
* [U-Boot] [PATCH 1/3] rtc: rv3029: add to Kconfig
2018-08-14 9:50 [U-Boot] [PATCH 0/3] Update RV3029 driver to DM and add DM-backed bootcount support Philipp Tomsich
@ 2018-08-14 9:51 ` Philipp Tomsich
2018-08-14 16:46 ` Heinrich Schuchardt
2018-08-14 9:51 ` [U-Boot] [PATCH 2/3] rtc: rv3029: update to support DM and sync with Linux 4.17 Philipp Tomsich
2018-08-14 9:51 ` [U-Boot] [PATCH 3/3] bootcount: add DM-based backing store for bootcount Philipp Tomsich
2 siblings, 1 reply; 13+ messages in thread
From: Philipp Tomsich @ 2018-08-14 9:51 UTC (permalink / raw)
To: u-boot
The MicroCrystal RV3029 driver didn't have a Kconfig entry and was not used
anywhere. Add it to Kconfig to make it selectable.
Signed-off-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>
Tested-by: Klaus Goger <klaus.goger@theobroma-systems.com>
---
drivers/rtc/Kconfig | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig
index 5436509..8a6e796 100644
--- a/drivers/rtc/Kconfig
+++ b/drivers/rtc/Kconfig
@@ -42,6 +42,16 @@ config RTC_ISL1208
This driver supports reading and writing the RTC/calendar and detects
total power failures.
+config RTC_RV3029
+ bool "Enable RV3029 driver"
+ depends on DM_RTC
+ help
+ The MicroCrystal RV3029 is a I2C Real Time Clock (RTC) with 8-byte
+ battery-backed SRAM.
+
+ This driver supports reading and writing the RTC/calendar and the
+ battery-baced SRAM section.
+
config RTC_RX8010SJ
bool "Enable RX8010SJ driver"
depends on DM_RTC
--
2.1.4
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [U-Boot] [PATCH 2/3] rtc: rv3029: update to support DM and sync with Linux 4.17
2018-08-14 9:50 [U-Boot] [PATCH 0/3] Update RV3029 driver to DM and add DM-backed bootcount support Philipp Tomsich
2018-08-14 9:51 ` [U-Boot] [PATCH 1/3] rtc: rv3029: add to Kconfig Philipp Tomsich
@ 2018-08-14 9:51 ` Philipp Tomsich
2018-08-14 9:51 ` [U-Boot] [PATCH 3/3] bootcount: add DM-based backing store for bootcount Philipp Tomsich
2 siblings, 0 replies; 13+ messages in thread
From: Philipp Tomsich @ 2018-08-14 9:51 UTC (permalink / raw)
To: u-boot
The "Flamingo" carrier-board for the RK3399-Q7 has a RV3029 populated
and the application will use the off-module RV3029 RTC including the
battery backed SRAM.
To support this use case, this commit includes the following changes:
* updates the rv3029 driver to use DM
* implements the read8/write8 operations
This syncs the implementation with the Linux code (based on 4.17),
porting the trickle-charger support from there (with improvements to
avoid unnecessary EEPROM updates) and adheres to the Linux DTS
binding.
Signed-off-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>
Tested-by: Klaus Goger <klaus.goger@theobroma-systems.com>
---
drivers/rtc/rv3029.c | 579 +++++++++++++++++++++++++++++++++----------
scripts/config_whitelist.txt | 1 -
2 files changed, 442 insertions(+), 138 deletions(-)
diff --git a/drivers/rtc/rv3029.c b/drivers/rtc/rv3029.c
index dd4d060..4726f23 100644
--- a/drivers/rtc/rv3029.c
+++ b/drivers/rtc/rv3029.c
@@ -1,189 +1,494 @@
// SPDX-License-Identifier: GPL-2.0+
/*
- * (C) Copyright 2010
- * Heiko Schocher, DENX Software Engineering, hs at denx.de
+ * (C) Copyright 2018 Theobroma Systems Design und Consulting GmbH
+ *
+ * Based on a the Linux rtc-rv3029c2.c driver written by:
+ * Gregory Hermant <gregory.hermant@calao-systems.com>
+ * Michael Buesch <m@bues.ch>
*/
+
#include <common.h>
#include <command.h>
+#include <dm.h>
#include <i2c.h>
#include <rtc.h>
-#define RTC_RV3029_CTRL1 0x00
-#define RTC_RV3029_CTRL1_EERE (1 << 3)
-
-#define RTC_RV3029_CTRL_STATUS 0x03
-#define RTC_RV3029_CTRLS_EEBUSY (1 << 7)
+#define RTC_RV3029_PAGE_LEN 7
-#define RTC_RV3029_CTRL_RESET 0x04
-#define RTC_RV3029_CTRL_SYS_R (1 << 4)
+/* control section */
+#define RV3029_ONOFF_CTRL 0x00
+#define RV3029_ONOFF_CTRL_WE BIT(0)
+#define RV3029_ONOFF_CTRL_TE BIT(1)
+#define RV3029_ONOFF_CTRL_TAR BIT(2)
+#define RV3029_ONOFF_CTRL_EERE BIT(3)
+#define RV3029_ONOFF_CTRL_SRON BIT(4)
+#define RV3029_ONOFF_CTRL_TD0 BIT(5)
+#define RV3029_ONOFF_CTRL_TD1 BIT(6)
+#define RV3029_ONOFF_CTRL_CLKINT BIT(7)
+#define RV3029_IRQ_CTRL 0x01
+#define RV3029_IRQ_CTRL_AIE BIT(0)
+#define RV3029_IRQ_CTRL_TIE BIT(1)
+#define RV3029_IRQ_CTRL_V1IE BIT(2)
+#define RV3029_IRQ_CTRL_V2IE BIT(3)
+#define RV3029_IRQ_CTRL_SRIE BIT(4)
+#define RV3029_IRQ_FLAGS 0x02
+#define RV3029_IRQ_FLAGS_AF BIT(0)
+#define RV3029_IRQ_FLAGS_TF BIT(1)
+#define RV3029_IRQ_FLAGS_V1IF BIT(2)
+#define RV3029_IRQ_FLAGS_V2IF BIT(3)
+#define RV3029_IRQ_FLAGS_SRF BIT(4)
+#define RV3029_STATUS 0x03
+#define RV3029_STATUS_VLOW1 BIT(2)
+#define RV3029_STATUS_VLOW2 BIT(3)
+#define RV3029_STATUS_SR BIT(4)
+#define RV3029_STATUS_PON BIT(5)
+#define RV3029_STATUS_EEBUSY BIT(7)
+#define RV3029_RST_CTRL 0x04
+#define RV3029_RST_CTRL_SYSR BIT(4)
+#define RV3029_CONTROL_SECTION_LEN 0x05
-#define RTC_RV3029_CLOCK_PAGE 0x08
-#define RTC_RV3029_PAGE_LEN 7
+/* watch section */
+#define RV3029_W_SEC 0x08
+#define RV3029_W_MINUTES 0x09
+#define RV3029_W_HOURS 0x0A
+#define RV3029_REG_HR_12_24 BIT(6) /* 24h/12h mode */
+#define RV3029_REG_HR_PM BIT(5) /* PM/AM bit in 12h mode */
+#define RV3029_W_DATE 0x0B
+#define RV3029_W_DAYS 0x0C
+#define RV3029_W_MONTHS 0x0D
+#define RV3029_W_YEARS 0x0E
-#define RV3029C2_W_SECONDS 0x00
-#define RV3029C2_W_MINUTES 0x01
-#define RV3029C2_W_HOURS 0x02
-#define RV3029C2_W_DATE 0x03
-#define RV3029C2_W_DAYS 0x04
-#define RV3029C2_W_MONTHS 0x05
-#define RV3029C2_W_YEARS 0x06
+/* eeprom control section */
+#define RV3029_CONTROL_E2P_EECTRL 0x30
+#define RV3029_TRICKLE_1K BIT(4) /* 1.5K resistance */
+#define RV3029_TRICKLE_5K BIT(5) /* 5K resistance */
+#define RV3029_TRICKLE_20K BIT(6) /* 20K resistance */
+#define RV3029_TRICKLE_80K BIT(7) /* 80K resistance */
+#define RV3029_TRICKLE_MASK (RV3029_TRICKLE_1K |\
+ RV3029_TRICKLE_5K |\
+ RV3029_TRICKLE_20K |\
+ RV3029_TRICKLE_80K)
+#define RV3029_TRICKLE_SHIFT 4
-#define RV3029C2_REG_HR_12_24 (1 << 6) /* 24h/12h mode */
-#define RV3029C2_REG_HR_PM (1 << 5) /* PM/AM bit in 12h mode */
-#define RTC_RV3029_EEPROM_CTRL 0x30
-#define RTC_RV3029_TRICKLE_1K (1 << 4)
-#define RTC_RV3029_TRICKLE_5K (1 << 5)
-#define RTC_RV3029_TRICKLE_20K (1 << 6)
-#define RTC_RV3029_TRICKLE_80K (1 << 7)
-
-int rtc_get( struct rtc_time *tmp )
+static int rv3029_rtc_get(struct udevice *dev, struct rtc_time *tm)
{
- int ret;
- unsigned char buf[RTC_RV3029_PAGE_LEN];
+ u8 regs[RTC_RV3029_PAGE_LEN];
+ int ret;
- ret = i2c_read(CONFIG_SYS_I2C_RTC_ADDR, RTC_RV3029_CLOCK_PAGE, 1, buf, \
- RTC_RV3029_PAGE_LEN);
- if (ret) {
+ ret = dm_i2c_read(dev, RV3029_W_SEC, regs, sizeof(regs));
+ if (ret < 0) {
printf("%s: error reading RTC: %x\n", __func__, ret);
- return -1;
+ return -EIO;
+ }
+
+ tm->tm_sec = bcd2bin(regs[RV3029_W_SEC - RV3029_W_SEC]);
+ tm->tm_min = bcd2bin(regs[RV3029_W_MINUTES - RV3029_W_SEC]);
+
+ /* HR field has a more complex interpretation */
+ {
+ const u8 _hr = regs[RV3029_W_HOURS - RV3029_W_SEC];
+
+ if (_hr & RV3029_REG_HR_12_24) {
+ /* 12h format */
+ tm->tm_hour = bcd2bin(_hr & 0x1f);
+ if (_hr & RV3029_REG_HR_PM) /* PM flag set */
+ tm->tm_hour += 12;
+ } else {
+ /* 24h format */
+ tm->tm_hour = bcd2bin(_hr & 0x3f);
+ }
}
- tmp->tm_sec = bcd2bin( buf[RV3029C2_W_SECONDS] & 0x7f);
- tmp->tm_min = bcd2bin( buf[RV3029C2_W_MINUTES] & 0x7f);
- if (buf[RV3029C2_W_HOURS] & RV3029C2_REG_HR_12_24) {
- /* 12h format */
- tmp->tm_hour = bcd2bin(buf[RV3029C2_W_HOURS] & 0x1f);
- if (buf[RV3029C2_W_HOURS] & RV3029C2_REG_HR_PM)
- /* PM flag set */
- tmp->tm_hour += 12;
- } else
- tmp->tm_hour = bcd2bin(buf[RV3029C2_W_HOURS] & 0x3f);
-
- tmp->tm_mday = bcd2bin( buf[RV3029C2_W_DATE] & 0x3F );
- tmp->tm_mon = bcd2bin( buf[RV3029C2_W_MONTHS] & 0x1F );
- tmp->tm_wday = bcd2bin( buf[RV3029C2_W_DAYS] & 0x07 );
+
+ tm->tm_mday = bcd2bin(regs[RV3029_W_DATE - RV3029_W_SEC]);
+ tm->tm_mon = bcd2bin(regs[RV3029_W_MONTHS - RV3029_W_SEC]) - 1;
/* RTC supports only years > 1999 */
- tmp->tm_year = bcd2bin( buf[RV3029C2_W_YEARS]) + 2000;
- tmp->tm_yday = 0;
- tmp->tm_isdst = 0;
+ tm->tm_year = bcd2bin(regs[RV3029_W_YEARS - RV3029_W_SEC]) + 2000;
+ tm->tm_wday = bcd2bin(regs[RV3029_W_DAYS - RV3029_W_SEC]) - 1;
+
+ tm->tm_yday = 0;
+ tm->tm_isdst = 0;
- debug( "Get DATE: %4d-%02d-%02d (wday=%d) TIME: %2d:%02d:%02d\n",
- tmp->tm_year, tmp->tm_mon, tmp->tm_mday, tmp->tm_wday,
- tmp->tm_hour, tmp->tm_min, tmp->tm_sec );
+ debug("%s: %4d-%02d-%02d (wday=%d) %2d:%02d:%02d\n",
+ __func__, tm->tm_year, tm->tm_mon, tm->tm_mday,
+ tm->tm_wday, tm->tm_hour, tm->tm_min, tm->tm_sec);
return 0;
}
-int rtc_set( struct rtc_time *tmp )
+static int rv3029_rtc_set(struct udevice *dev, const struct rtc_time *tm)
{
- int ret;
- unsigned char buf[RTC_RV3029_PAGE_LEN];
+ u8 regs[RTC_RV3029_PAGE_LEN];
+
+ debug("%s: %4d-%02d-%02d (wday=%d( %2d:%02d:%02d\n",
+ __func__, tm->tm_year, tm->tm_mon, tm->tm_mday,
+ tm->tm_wday, tm->tm_hour, tm->tm_min, tm->tm_sec);
- debug( "Set DATE: %4d-%02d-%02d (wday=%d) TIME: %2d:%02d:%02d\n",
- tmp->tm_year, tmp->tm_mon, tmp->tm_mday, tmp->tm_wday,
- tmp->tm_hour, tmp->tm_min, tmp->tm_sec);
- if (tmp->tm_year < 2000) {
- printf("RTC: year %d < 2000 not possible\n", tmp->tm_year);
- return -1;
+ if (tm->tm_year < 2000) {
+ printf("%s: year %d (before 2000) not supported\n",
+ __func__, tm->tm_year);
+ return -EINVAL;
}
- buf[RV3029C2_W_SECONDS] = bin2bcd(tmp->tm_sec);
- buf[RV3029C2_W_MINUTES] = bin2bcd(tmp->tm_min);
- buf[RV3029C2_W_HOURS] = bin2bcd(tmp->tm_hour);
- /* set 24h format */
- buf[RV3029C2_W_HOURS] &= ~RV3029C2_REG_HR_12_24;
- buf[RV3029C2_W_DATE] = bin2bcd(tmp->tm_mday);
- buf[RV3029C2_W_DAYS] = bin2bcd(tmp->tm_wday);
- buf[RV3029C2_W_MONTHS] = bin2bcd(tmp->tm_mon);
- tmp->tm_year -= 2000;
- buf[RV3029C2_W_YEARS] = bin2bcd(tmp->tm_year);
- ret = i2c_write(CONFIG_SYS_I2C_RTC_ADDR, RTC_RV3029_CLOCK_PAGE, 1,
- buf, RTC_RV3029_PAGE_LEN);
-
- /* give the RTC some time to update */
- udelay(1000);
- return ret;
+
+ regs[RV3029_W_SEC - RV3029_W_SEC] = bin2bcd(tm->tm_sec);
+ regs[RV3029_W_MINUTES - RV3029_W_SEC] = bin2bcd(tm->tm_min);
+ regs[RV3029_W_HOURS - RV3029_W_SEC] = bin2bcd(tm->tm_hour);
+ regs[RV3029_W_DATE - RV3029_W_SEC] = bin2bcd(tm->tm_mday);
+ regs[RV3029_W_MONTHS - RV3029_W_SEC] = bin2bcd(tm->tm_mon + 1);
+ regs[RV3029_W_DAYS - RV3029_W_SEC] = bin2bcd(tm->tm_wday + 1) & 0x7;
+ regs[RV3029_W_YEARS - RV3029_W_SEC] = bin2bcd(tm->tm_year - 2000);
+
+ return dm_i2c_write(dev, RV3029_W_SEC, regs, sizeof(regs));
}
-/* sets EERE-Bit (automatic EEPROM refresh) */
-static void set_eere_bit(int state)
+static int rv3029_rtc_reset(struct udevice *dev)
{
- unsigned char reg_ctrl1;
+ u8 ctrl = RV3029_RST_CTRL_SYSR;
+ unsigned long start;
+ const unsigned long timeout_ms = 10000;
+ int ret;
- (void)i2c_read(CONFIG_SYS_I2C_RTC_ADDR, RTC_RV3029_CTRL1, 1,
- ®_ctrl1, 1);
+ /* trigger the system-reset */
+ ret = dm_i2c_write(dev, RV3029_RST_CTRL, &ctrl, 1);
+ if (ret < 0)
+ return -EIO;
- if (state)
- reg_ctrl1 |= RTC_RV3029_CTRL1_EERE;
- else
- reg_ctrl1 &= (~RTC_RV3029_CTRL1_EERE);
+ /* wait for the system-reset to complete */
+ start = get_timer(0);
+ do {
+ if (get_timer(start) > timeout_ms)
+ return -ETIMEDOUT;
- (void)i2c_write(CONFIG_SYS_I2C_RTC_ADDR, RTC_RV3029_CTRL1, 1,
- ®_ctrl1, 1);
+ ret = dm_i2c_read(dev, RV3029_RST_CTRL, &ctrl, 1);
+ if (ret < 0)
+ return -EIO;
+ } while (ctrl & RV3029_RST_CTRL_SYSR);
+
+ return 0;
}
-/* waits until EEPROM page is no longer busy (times out after 10ms*loops) */
-static int wait_eebusy(int loops)
+static int rv3029_rtc_read8(struct udevice *dev, unsigned int reg)
{
- int i;
- unsigned char ctrl_status;
+ u8 data;
+ int ret;
+
+ ret = dm_i2c_read(dev, reg, &data, sizeof(data));
+ return ret < 0 ? ret : data;
+}
- for (i = 0; i < loops; i++) {
- (void)i2c_read(CONFIG_SYS_I2C_RTC_ADDR, RTC_RV3029_CTRL_STATUS,
- 1, &ctrl_status, 1);
+static int rv3029_rtc_write8(struct udevice *dev, unsigned int reg, int val)
+{
+ u8 data = val;
+
+ return dm_i2c_write(dev, reg, &data, 1);
+}
+
+#if defined(OF_CONTROL)
+static int rv3029_get_sr(struct udevice *dev, u8 *buf)
+{
+ int ret = dm_i2c_read(dev, RV3029_STATUS, buf, 1);
+
+ if (ret < 0)
+ return -EIO;
+
+ dev_dbg(dev, "status = 0x%.2x (%d)\n", buf[0], buf[0]);
+ return 0;
+}
- if ((ctrl_status & RTC_RV3029_CTRLS_EEBUSY) == 0)
+static int rv3029_set_sr(struct udevice *dev, u8 val)
+{
+ int ret;
+
+ ret = dm_i2c_read(dev, RV3029_STATUS, &val, 1);
+ if (ret < 0)
+ return -EIO;
+
+ dev_dbg(dev, "status = 0x%.2x (%d)\n", val, val);
+ return 0;
+}
+
+static int rv3029_eeprom_busywait(struct udevice *dev)
+{
+ int i, ret;
+ u8 sr;
+
+ for (i = 100; i > 0; i--) {
+ ret = rv3029_get_sr(dev, &sr);
+ if (ret < 0)
+ break;
+ if (!(sr & RV3029_STATUS_EEBUSY))
break;
udelay(10000);
}
- return i;
+ if (i <= 0) {
+ dev_err(dev, "EEPROM busy wait timeout.\n");
+ return -ETIMEDOUT;
+ }
+
+ return ret;
}
-void rtc_reset (void)
+static int rv3029_update_bits(struct udevice *dev, u8 reg, u8 mask, u8 set)
{
- unsigned char buf[RTC_RV3029_PAGE_LEN];
+ u8 buf;
+ int ret;
- buf[0] = RTC_RV3029_CTRL_SYS_R;
- (void)i2c_write(CONFIG_SYS_I2C_RTC_ADDR, RTC_RV3029_CTRL_RESET, 1,
- buf, 1);
+ ret = dm_i2c_read(dev, reg, &buf, 1);
+ if (ret < 0)
+ return ret;
-#if defined(CONFIG_SYS_RV3029_TCR)
- /*
- * because EEPROM_CTRL register is in EEPROM page it is necessary to
- * disable automatic EEPROM refresh and check if EEPROM is busy
- * before EEPORM_CTRL register may be accessed
- */
- set_eere_bit(0);
- wait_eebusy(100);
- /* read current trickle charger setting */
- (void)i2c_read(CONFIG_SYS_I2C_RTC_ADDR, RTC_RV3029_EEPROM_CTRL,
- 1, buf, 1);
- /* enable automatic EEPROM refresh again */
- set_eere_bit(1);
+ if ((buf & mask) == (set && mask))
+ return 0;
- /*
- * to minimize EEPROM access write trickle charger setting only if it
- * differs from current value
- */
- if ((buf[0] & 0xF0) != CONFIG_SYS_RV3029_TCR) {
- buf[0] = (buf[0] & 0x0F) | CONFIG_SYS_RV3029_TCR;
- /*
- * write trickle charger setting (disable autom. EEPROM
- * refresh and wait until EEPROM is idle)
- */
- set_eere_bit(0);
- wait_eebusy(100);
- (void)i2c_write(CONFIG_SYS_I2C_RTC_ADDR,
- RTC_RV3029_EEPROM_CTRL, 1, buf, 1);
- /*
- * it is necessary to wait 10ms before EEBUSY-Bit may be read
- * (this is not documented in the data sheet yet, but the
- * manufacturer recommends it)
+ buf = (buf & ~mask) | (set & mask);
+ ret = dm_i2c_read(dev, reg, &buf, 1);
+ if (ret < 0)
+ return ret;
+
+ return 0;
+}
+
+static int rv3029_eeprom_exit(struct udevice *dev)
+{
+ /* Re-enable eeprom refresh */
+ return rv3029_update_bits(dev, RV3029_ONOFF_CTRL,
+ RV3029_ONOFF_CTRL_EERE,
+ RV3029_ONOFF_CTRL_EERE);
+}
+
+static int rv3029_eeprom_enter(struct udevice *dev)
+{
+ int ret;
+ u8 sr;
+
+ /* Check whether we are in the allowed voltage range. */
+ ret = rv3029_get_sr(dev, &sr);
+ if (ret < 0)
+ return ret;
+ if (sr & (RV3029_STATUS_VLOW1 | RV3029_STATUS_VLOW2)) {
+ /* We clear the bits and retry once just in case
+ * we had a brown out in early startup.
*/
+ sr &= ~RV3029_STATUS_VLOW1;
+ sr &= ~RV3029_STATUS_VLOW2;
+ ret = rv3029_set_sr(dev, sr);
+ if (ret < 0)
+ return ret;
udelay(10000);
- /* wait until EEPROM write access is finished */
- wait_eebusy(100);
- set_eere_bit(1);
+ ret = rv3029_get_sr(dev, &sr);
+ if (ret < 0)
+ return ret;
+ if (sr & (RV3029_STATUS_VLOW1 | RV3029_STATUS_VLOW2)) {
+ dev_err(dev, "Supply voltage is too low to safely access the EEPROM.\n");
+ return -ENODEV;
+ }
+ }
+
+ /* Disable eeprom refresh. */
+ ret = rv3029_update_bits(dev,
+ RV3029_ONOFF_CTRL, RV3029_ONOFF_CTRL_EERE, 0);
+ if (ret < 0)
+ return ret;
+
+ /* Wait for any previous eeprom accesses to finish. */
+ ret = rv3029_eeprom_busywait(dev);
+ if (ret < 0)
+ rv3029_eeprom_exit(dev);
+
+ return ret;
+}
+
+static int rv3029_eeprom_read(struct udevice *dev, u8 reg,
+ u8 buf[], size_t len)
+{
+ int ret, err;
+
+ err = rv3029_eeprom_enter(dev);
+ if (err < 0)
+ return err;
+
+ ret = dm_i2c_read(dev, reg, buf, len);
+
+ err = rv3029_eeprom_exit(dev);
+ if (err < 0)
+ return err;
+
+ return ret;
+}
+
+static int rv3029_eeprom_write(struct udevice *dev, u8 reg,
+ u8 const buf[], size_t len)
+{
+ int ret;
+ size_t i;
+ u8 tmp;
+
+ ret = rv3029_eeprom_enter(dev);
+ if (ret < 0)
+ return ret;
+
+ for (i = 0; i < len; i++, reg++) {
+ ret = dm_i2c_read(dev, reg, &tmp, 1);
+ if (ret < 0)
+ break;
+ if (tmp != buf[i]) {
+ ret = dm_i2c_write(dev, reg, &buf[i], 1);
+ if (ret < 0)
+ break;
+ }
+ ret = rv3029_eeprom_busywait(dev);
+ if (ret < 0)
+ break;
}
+
+ ret = rv3029_eeprom_exit(dev);
+ if (ret < 0)
+ return ret;
+
+ return 0;
+}
+
+static int rv3029_eeprom_update_bits(struct udevice *dev,
+ u8 reg, u8 mask, u8 set)
+{
+ u8 buf;
+ int ret;
+
+ ret = rv3029_eeprom_read(dev, reg, &buf, 1);
+ if (ret < 0)
+ return ret;
+
+ /*
+ * If the EEPROM already reads the correct bitpattern, we don't need
+ * to update it.
+ */
+ if ((buf & mask) == (set & mask))
+ return 0;
+
+ buf = (buf & ~mask) | (set & mask);
+ ret = rv3029_eeprom_write(dev, reg, &buf, 1);
+ if (ret < 0)
+ return ret;
+
+ return 0;
+}
+
+static void rv3029_trickle_config(struct udevice *dev)
+{
+ static const struct rv3029_trickle_tab_elem {
+ u32 r; /* resistance in ohms */
+ u8 conf; /* trickle config bits */
+ } rv3029_trickle_tab[] = {
+ {
+ .r = 1076,
+ .conf = RV3029_TRICKLE_1K | RV3029_TRICKLE_5K |
+ RV3029_TRICKLE_20K | RV3029_TRICKLE_80K,
+ }, {
+ .r = 1091,
+ .conf = RV3029_TRICKLE_1K | RV3029_TRICKLE_5K |
+ RV3029_TRICKLE_20K,
+ }, {
+ .r = 1137,
+ .conf = RV3029_TRICKLE_1K | RV3029_TRICKLE_5K |
+ RV3029_TRICKLE_80K,
+ }, {
+ .r = 1154,
+ .conf = RV3029_TRICKLE_1K | RV3029_TRICKLE_5K,
+ }, {
+ .r = 1371,
+ .conf = RV3029_TRICKLE_1K | RV3029_TRICKLE_20K |
+ RV3029_TRICKLE_80K,
+ }, {
+ .r = 1395,
+ .conf = RV3029_TRICKLE_1K | RV3029_TRICKLE_20K,
+ }, {
+ .r = 1472,
+ .conf = RV3029_TRICKLE_1K | RV3029_TRICKLE_80K,
+ }, {
+ .r = 1500,
+ .conf = RV3029_TRICKLE_1K,
+ }, {
+ .r = 3810,
+ .conf = RV3029_TRICKLE_5K | RV3029_TRICKLE_20K |
+ RV3029_TRICKLE_80K,
+ }, {
+ .r = 4000,
+ .conf = RV3029_TRICKLE_5K | RV3029_TRICKLE_20K,
+ }, {
+ .r = 4706,
+ .conf = RV3029_TRICKLE_5K | RV3029_TRICKLE_80K,
+ }, {
+ .r = 5000,
+ .conf = RV3029_TRICKLE_5K,
+ }, {
+ .r = 16000,
+ .conf = RV3029_TRICKLE_20K | RV3029_TRICKLE_80K,
+ }, {
+ .r = 20000,
+ .conf = RV3029_TRICKLE_20K,
+ }, {
+ .r = 80000,
+ .conf = RV3029_TRICKLE_80K,
+ },
+ };
+ int err;
+ u32 ohms;
+ u8 trickle_set_bits = 0;
+
+ /* Configure the trickle charger. */
+ err = dev_read_u32(dev, "trickle-resistor-ohms", &ohms);
+
+ if (!err) {
+ /* Find trickle-charger config */
+ for (int i = 0; i < ARRAY_SIZE(rv3029_trickle_tab); i++)
+ if (rv3029_trickle_tab[i].r >= ohms) {
+ dev_dbg(dev, "trickle charger at %d ohms\n",
+ rv3029_trickle_tab[i].r);
+ trickle_set_bits = rv3029_trickle_tab[i].conf;
+ break;
+ }
+ }
+
+ dev_dbg(dev, "trickle charger config 0x%x\n", trickle_set_bits);
+ err = rv3029_eeprom_update_bits(dev, RV3029_CONTROL_E2P_EECTRL,
+ RV3029_TRICKLE_MASK,
+ trickle_set_bits);
+ if (err < 0)
+ dev_dbg(dev, "failed to update trickle charger\n");
+}
+#else
+static inline void rv3029_trickle_config(struct udevice *dev)
+{
+}
#endif
+
+static int rv3029_probe(struct udevice *dev)
+{
+ i2c_set_chip_flags(dev, DM_I2C_CHIP_RD_ADDRESS |
+ DM_I2C_CHIP_WR_ADDRESS);
+
+ rv3029_trickle_config(dev);
+ return 0;
}
+
+static const struct rtc_ops rv3029_rtc_ops = {
+ .get = rv3029_rtc_get,
+ .set = rv3029_rtc_set,
+ .read8 = rv3029_rtc_read8,
+ .write8 = rv3029_rtc_write8,
+ .reset = rv3029_rtc_reset,
+};
+
+static const struct udevice_id rv3029_rtc_ids[] = {
+ { .compatible = "mc,rv3029" },
+ { }
+};
+
+U_BOOT_DRIVER(rtc_rv3029) = {
+ .name = "rtc-rv3029",
+ .id = UCLASS_RTC,
+ .probe = rv3029_probe,
+ .of_match = rv3029_rtc_ids,
+ .ops = &rv3029_rtc_ops,
+};
diff --git a/scripts/config_whitelist.txt b/scripts/config_whitelist.txt
index bd264d2..2597525 100644
--- a/scripts/config_whitelist.txt
+++ b/scripts/config_whitelist.txt
@@ -4100,7 +4100,6 @@ CONFIG_SYS_RTC_OSCILLATOR
CONFIG_SYS_RTC_PL031_BASE
CONFIG_SYS_RTC_REG_BASE_ADDR
CONFIG_SYS_RTC_SETUP
-CONFIG_SYS_RV3029_TCR
CONFIG_SYS_RX_ETH_BUFFER
CONFIG_SYS_SATA
CONFIG_SYS_SATA1
--
2.1.4
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [U-Boot] [PATCH 3/3] bootcount: add DM-based backing store for bootcount
2018-08-14 9:50 [U-Boot] [PATCH 0/3] Update RV3029 driver to DM and add DM-backed bootcount support Philipp Tomsich
2018-08-14 9:51 ` [U-Boot] [PATCH 1/3] rtc: rv3029: add to Kconfig Philipp Tomsich
2018-08-14 9:51 ` [U-Boot] [PATCH 2/3] rtc: rv3029: update to support DM and sync with Linux 4.17 Philipp Tomsich
@ 2018-08-14 9:51 ` Philipp Tomsich
2018-08-14 11:10 ` Lukasz Majewski
2 siblings, 1 reply; 13+ messages in thread
From: Philipp Tomsich @ 2018-08-14 9:51 UTC (permalink / raw)
To: u-boot
The original bootcount methods do not provide an interface to DM and
rely on a static configuration for I2C devices (e.g. bus, chip-addr,
etc. are configured through defines statically). On a modern system
that exposes multiple devices in a DTS-configurable way, this is less
than optimal and a interface to DM-based devices will be desirable.
This adds a simple driver that is DM-aware and configurable via DTS:
the /chosen/u-boot,bootcount-device property is used to detect the DM
device storing the bootcount and deviceclass-specific commands are
used to read/write the bootcount.
Initially, this provides support for the following DM devices:
* RTC devices implementing the read8/write8 ops
Signed-off-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>
Tested-by: Klaus Goger <klaus.goger@theobroma-systems.com>
---
doc/device-tree-bindings/chosen.txt | 27 +++++++++++
drivers/bootcount/Kconfig | 12 +++++
drivers/bootcount/Makefile | 1 +
drivers/bootcount/bootcount_dm.c | 93 +++++++++++++++++++++++++++++++++++++
4 files changed, 133 insertions(+)
create mode 100644 drivers/bootcount/bootcount_dm.c
diff --git a/doc/device-tree-bindings/chosen.txt b/doc/device-tree-bindings/chosen.txt
index da7b4e6..734fd15 100644
--- a/doc/device-tree-bindings/chosen.txt
+++ b/doc/device-tree-bindings/chosen.txt
@@ -42,6 +42,33 @@ Example
};
};
+u-boot,bootcount-device property
+--------------------------------
+In a DM-based system, the bootcount may be stored in a device known to
+the DM framework (e.g. in a battery-backed SRAM area within a RTC
+device). To identify the device to be used for the bootcount, the
+u-boot,bootcount-device property needs to point to the target device.
+
+Further configuration in the target device's node may be required
+(e.g. an offset into an I2C RTC's address space), depending on the
+UCLASS of the target device.
+
+Example
+-------
+/ {
+ chosen {
+ u-boot,bootcount-device = &rv3029;
+ };
+
+ i2c2 {
+ rv3029: rtc at 56 {
+ compatible = "mc,rv3029";
+ reg = <0x56>;
+ u-boot,bootcount-offset = <0x38>;
+ };
+ };
+};
+
u-boot,spl-boot-order property
------------------------------
diff --git a/drivers/bootcount/Kconfig b/drivers/bootcount/Kconfig
index d335ed1..cdde7b5 100644
--- a/drivers/bootcount/Kconfig
+++ b/drivers/bootcount/Kconfig
@@ -70,6 +70,18 @@ config BOOTCOUNT_AT91
bool "Boot counter for Atmel AT91SAM9XE"
depends on AT91SAM9XE
+config BOOTCOUNT_DM
+ bool "Boot counter in a device-model device"
+ help
+ Enables reading/writing the bootcount in a device-model
+ device referenced from the /chosen node. The type of the
+ device is detected from DM and any additional configuration
+ information (e.g. the offset into a RTC device that supports
+ read32/write32) is read from the device's node.
+
+ At this time the following DM device classes are supported:
+ * RTC (using read32/write32)
+
endchoice
config BOOTCOUNT_ALEN
diff --git a/drivers/bootcount/Makefile b/drivers/bootcount/Makefile
index 68bc006..e8ed729 100644
--- a/drivers/bootcount/Makefile
+++ b/drivers/bootcount/Makefile
@@ -7,3 +7,4 @@ 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_DM) += bootcount_dm.o
diff --git a/drivers/bootcount/bootcount_dm.c b/drivers/bootcount/bootcount_dm.c
new file mode 100644
index 0000000..99bdb88
--- /dev/null
+++ b/drivers/bootcount/bootcount_dm.c
@@ -0,0 +1,93 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * (C) Copyright 2018 Theobroma Systems Design und Consulting GmbH
+ */
+
+#include <common.h>
+#include <bootcount.h>
+#include <dm.h>
+#include <rtc.h>
+
+const u8 bootcount_magic = 0xbc;
+
+static void bootcount_store_rtc(struct udevice *dev, ulong a)
+{
+#if CONFIG_IS_ENABLED(DM_RTC)
+ u32 offset;
+ const char *offset_propname = "u-boot,bootcount-offset";
+ const u16 val = bootcount_magic << 8 | (a & 0xff);
+
+ if (dev_read_u32(dev, offset_propname, &offset) < 0) {
+ debug("%s: requires %s\n", __func__, offset_propname);
+ return;
+ }
+
+ if (rtc_write16(dev, offset, val) < 0) {
+ debug("%s: rtc_write16 failed\n", __func__);
+ return;
+ }
+#endif
+}
+
+static u32 bootcount_load_rtc(struct udevice *dev)
+{
+#if CONFIG_IS_ENABLED(DM_RTC)
+ u32 offset;
+ const char *offset_propname = "u-boot,bootcount-offset";
+ u16 val;
+
+ if (dev_read_u32(dev, offset_propname, &offset) < 0) {
+ debug("%s: requires %s\n", __func__, offset_propname);
+ goto fail;
+ }
+
+ if (rtc_read16(dev, offset, &val) < 0) {
+ debug("%s: rtc_write16 failed\n", __func__);
+ goto fail;
+ }
+
+ if (val >> 8 == bootcount_magic)
+ return val & 0xff;
+
+ debug("%s: bootcount magic does not match on %04x\n", __func__, val);
+ fail:
+#endif
+ return 0;
+}
+
+/* Now implement the generic default functions */
+void bootcount_store(ulong a)
+{
+ struct udevice *dev;
+ ofnode node;
+ const char *propname = "u-boot,bootcount-device";
+
+ node = ofnode_get_chosen_node(propname);
+ if (!ofnode_valid(node)) {
+ debug("%s: no '%s'\n", __func__, propname);
+ return;
+ }
+
+ /* RTC devices */
+ if (!uclass_get_device_by_ofnode(UCLASS_RTC, node, &dev))
+ return bootcount_store_rtc(dev, a);
+}
+
+ulong bootcount_load(void)
+{
+ struct udevice *dev;
+ ofnode node;
+ const char *propname = "u-boot,bootcount-device";
+
+ node = ofnode_get_chosen_node(propname);
+ if (!ofnode_valid(node)) {
+ debug("%s: no '%s'\n", __func__, propname);
+ return 0;
+ }
+
+ /* RTC devices */
+ if (!uclass_get_device_by_ofnode(UCLASS_RTC, node, &dev))
+ return bootcount_load_rtc(dev);
+
+ return 0;
+}
--
2.1.4
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [U-Boot] [PATCH 3/3] bootcount: add DM-based backing store for bootcount
2018-08-14 9:51 ` [U-Boot] [PATCH 3/3] bootcount: add DM-based backing store for bootcount Philipp Tomsich
@ 2018-08-14 11:10 ` Lukasz Majewski
2018-08-14 11:39 ` Dr. Philipp Tomsich
0 siblings, 1 reply; 13+ messages in thread
From: Lukasz Majewski @ 2018-08-14 11:10 UTC (permalink / raw)
To: u-boot
Hi Philipp,
> The original bootcount methods do not provide an interface to DM and
> rely on a static configuration for I2C devices (e.g. bus, chip-addr,
> etc. are configured through defines statically). On a modern system
> that exposes multiple devices in a DTS-configurable way, this is less
> than optimal and a interface to DM-based devices will be desirable.
>
> This adds a simple driver that is DM-aware and configurable via DTS:
> the /chosen/u-boot,bootcount-device property is used to detect the DM
> device storing the bootcount and deviceclass-specific commands are
> used to read/write the bootcount.
>
> Initially, this provides support for the following DM devices:
> * RTC devices implementing the read8/write8 ops
>
> Signed-off-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>
> Tested-by: Klaus Goger <klaus.goger@theobroma-systems.com>
> ---
>
> doc/device-tree-bindings/chosen.txt | 27 +++++++++++
> drivers/bootcount/Kconfig | 12 +++++
> drivers/bootcount/Makefile | 1 +
> drivers/bootcount/bootcount_dm.c | 93
> +++++++++++++++++++++++++++++++++++++ 4 files changed, 133
> insertions(+) create mode 100644 drivers/bootcount/bootcount_dm.c
>
> diff --git a/doc/device-tree-bindings/chosen.txt
> b/doc/device-tree-bindings/chosen.txt index da7b4e6..734fd15 100644
> --- a/doc/device-tree-bindings/chosen.txt
> +++ b/doc/device-tree-bindings/chosen.txt
> @@ -42,6 +42,33 @@ Example
> };
> };
>
> +u-boot,bootcount-device property
> +--------------------------------
> +In a DM-based system, the bootcount may be stored in a device known
> to +the DM framework (e.g. in a battery-backed SRAM area within a RTC
> +device). To identify the device to be used for the bootcount, the
> +u-boot,bootcount-device property needs to point to the target device.
> +
> +Further configuration in the target device's node may be required
> +(e.g. an offset into an I2C RTC's address space), depending on the
> +UCLASS of the target device.
> +
> +Example
> +-------
> +/ {
> + chosen {
> + u-boot,bootcount-device = &rv3029;
> + };
> +
> + i2c2 {
> + rv3029: rtc at 56 {
> + compatible = "mc,rv3029";
> + reg = <0x56>;
> + u-boot,bootcount-offset = <0x38>;
> + };
> + };
> +};
> +
> u-boot,spl-boot-order property
> ------------------------------
>
> diff --git a/drivers/bootcount/Kconfig b/drivers/bootcount/Kconfig
> index d335ed1..cdde7b5 100644
> --- a/drivers/bootcount/Kconfig
> +++ b/drivers/bootcount/Kconfig
> @@ -70,6 +70,18 @@ config BOOTCOUNT_AT91
> bool "Boot counter for Atmel AT91SAM9XE"
> depends on AT91SAM9XE
>
> +config BOOTCOUNT_DM
> + bool "Boot counter in a device-model device"
> + help
> + Enables reading/writing the bootcount in a device-model
> + device referenced from the /chosen node. The type of the
> + device is detected from DM and any additional configuration
> + information (e.g. the offset into a RTC device that
> supports
> + read32/write32) is read from the device's node.
> +
> + At this time the following DM device classes are supported:
> + * RTC (using read32/write32)
> +
> endchoice
>
> config BOOTCOUNT_ALEN
> diff --git a/drivers/bootcount/Makefile b/drivers/bootcount/Makefile
> index 68bc006..e8ed729 100644
> --- a/drivers/bootcount/Makefile
> +++ b/drivers/bootcount/Makefile
> @@ -7,3 +7,4 @@ 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_DM) += bootcount_dm.o
> diff --git a/drivers/bootcount/bootcount_dm.c
> b/drivers/bootcount/bootcount_dm.c new file mode 100644
> index 0000000..99bdb88
> --- /dev/null
> +++ b/drivers/bootcount/bootcount_dm.c
> @@ -0,0 +1,93 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * (C) Copyright 2018 Theobroma Systems Design und Consulting GmbH
> + */
> +
> +#include <common.h>
> +#include <bootcount.h>
> +#include <dm.h>
> +#include <rtc.h>
> +
> +const u8 bootcount_magic = 0xbc;
> +
> +static void bootcount_store_rtc(struct udevice *dev, ulong a)
> +{
> +#if CONFIG_IS_ENABLED(DM_RTC)
> + u32 offset;
> + const char *offset_propname = "u-boot,bootcount-offset";
> + const u16 val = bootcount_magic << 8 | (a & 0xff);
> +
> + if (dev_read_u32(dev, offset_propname, &offset) < 0) {
> + debug("%s: requires %s\n", __func__,
> offset_propname);
> + return;
> + }
> +
> + if (rtc_write16(dev, offset, val) < 0) {
> + debug("%s: rtc_write16 failed\n", __func__);
> + return;
> + }
> +#endif
> +}
> +
> +static u32 bootcount_load_rtc(struct udevice *dev)
> +{
> +#if CONFIG_IS_ENABLED(DM_RTC)
> + u32 offset;
> + const char *offset_propname = "u-boot,bootcount-offset";
> + u16 val;
> +
> + if (dev_read_u32(dev, offset_propname, &offset) < 0) {
> + debug("%s: requires %s\n", __func__,
> offset_propname);
> + goto fail;
> + }
> +
> + if (rtc_read16(dev, offset, &val) < 0) {
> + debug("%s: rtc_write16 failed\n", __func__);
> + goto fail;
> + }
> +
> + if (val >> 8 == bootcount_magic)
> + return val & 0xff;
> +
> + debug("%s: bootcount magic does not match on %04x\n",
> __func__, val);
> + fail:
> +#endif
> + return 0;
> +}
> +
> +/* Now implement the generic default functions */
> +void bootcount_store(ulong a)
> +{
> + struct udevice *dev;
> + ofnode node;
> + const char *propname = "u-boot,bootcount-device";
> +
> + node = ofnode_get_chosen_node(propname);
> + if (!ofnode_valid(node)) {
> + debug("%s: no '%s'\n", __func__, propname);
> + return;
> + }
> +
> + /* RTC devices */
> + if (!uclass_get_device_by_ofnode(UCLASS_RTC, node, &dev))
> + return bootcount_store_rtc(dev, a);
> +}
> +
> +ulong bootcount_load(void)
> +{
> + struct udevice *dev;
> + ofnode node;
> + const char *propname = "u-boot,bootcount-device";
> +
> + node = ofnode_get_chosen_node(propname);
> + if (!ofnode_valid(node)) {
> + debug("%s: no '%s'\n", __func__, propname);
> + return 0;
> + }
> +
> + /* RTC devices */
> + if (!uclass_get_device_by_ofnode(UCLASS_RTC, node, &dev))
> + return bootcount_load_rtc(dev);
> +
> + return 0;
> +}
Thanks for your patch.
However, if I may ask - would it be possible to add support for EEPROM
based bootcount in an easy way?
I mean - do you think that it would be feasible to have
bootcount-uclass, which would support generic load/store functions with
DM drivers for RTC, EEPROM or even simple write to SFR registers (as it
is just an offset in the end)?
Best regards,
Lukasz Majewski
--
DENX Software Engineering GmbH, Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 488 bytes
Desc: OpenPGP digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20180814/414cc4b5/attachment.sig>
^ permalink raw reply [flat|nested] 13+ messages in thread
* [U-Boot] [PATCH 3/3] bootcount: add DM-based backing store for bootcount
2018-08-14 11:10 ` Lukasz Majewski
@ 2018-08-14 11:39 ` Dr. Philipp Tomsich
2018-08-17 12:49 ` Simon Glass
0 siblings, 1 reply; 13+ messages in thread
From: Dr. Philipp Tomsich @ 2018-08-14 11:39 UTC (permalink / raw)
To: u-boot
Lukasz,
> On 14 Aug 2018, at 13:10, Lukasz Majewski <lukma@denx.de> wrote:
>
> Hi Philipp,
>
>> The original bootcount methods do not provide an interface to DM and
>> rely on a static configuration for I2C devices (e.g. bus, chip-addr,
>> etc. are configured through defines statically). On a modern system
>> that exposes multiple devices in a DTS-configurable way, this is less
>> than optimal and a interface to DM-based devices will be desirable.
>>
>> This adds a simple driver that is DM-aware and configurable via DTS:
>> the /chosen/u-boot,bootcount-device property is used to detect the DM
>> device storing the bootcount and deviceclass-specific commands are
>> used to read/write the bootcount.
>>
>> Initially, this provides support for the following DM devices:
>> * RTC devices implementing the read8/write8 ops
>>
>> Signed-off-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>
>> Tested-by: Klaus Goger <klaus.goger@theobroma-systems.com>
>> ---
>>
>> doc/device-tree-bindings/chosen.txt | 27 +++++++++++
>> drivers/bootcount/Kconfig | 12 +++++
>> drivers/bootcount/Makefile | 1 +
>> drivers/bootcount/bootcount_dm.c | 93
>> +++++++++++++++++++++++++++++++++++++ 4 files changed, 133
>> insertions(+) create mode 100644 drivers/bootcount/bootcount_dm.c
>>
>> diff --git a/doc/device-tree-bindings/chosen.txt
>> b/doc/device-tree-bindings/chosen.txt index da7b4e6..734fd15 100644
>> --- a/doc/device-tree-bindings/chosen.txt
>> +++ b/doc/device-tree-bindings/chosen.txt
>> @@ -42,6 +42,33 @@ Example
>> };
>> };
>>
>> +u-boot,bootcount-device property
>> +--------------------------------
>> +In a DM-based system, the bootcount may be stored in a device known
>> to +the DM framework (e.g. in a battery-backed SRAM area within a RTC
>> +device). To identify the device to be used for the bootcount, the
>> +u-boot,bootcount-device property needs to point to the target device.
>> +
>> +Further configuration in the target device's node may be required
>> +(e.g. an offset into an I2C RTC's address space), depending on the
>> +UCLASS of the target device.
>> +
>> +Example
>> +-------
>> +/ {
>> + chosen {
>> + u-boot,bootcount-device = &rv3029;
>> + };
>> +
>> + i2c2 {
>> + rv3029: rtc at 56 {
>> + compatible = "mc,rv3029";
>> + reg = <0x56>;
>> + u-boot,bootcount-offset = <0x38>;
>> + };
>> + };
>> +};
>> +
>> u-boot,spl-boot-order property
>> ------------------------------
>>
>> diff --git a/drivers/bootcount/Kconfig b/drivers/bootcount/Kconfig
>> index d335ed1..cdde7b5 100644
>> --- a/drivers/bootcount/Kconfig
>> +++ b/drivers/bootcount/Kconfig
>> @@ -70,6 +70,18 @@ config BOOTCOUNT_AT91
>> bool "Boot counter for Atmel AT91SAM9XE"
>> depends on AT91SAM9XE
>>
>> +config BOOTCOUNT_DM
>> + bool "Boot counter in a device-model device"
>> + help
>> + Enables reading/writing the bootcount in a device-model
>> + device referenced from the /chosen node. The type of the
>> + device is detected from DM and any additional configuration
>> + information (e.g. the offset into a RTC device that
>> supports
>> + read32/write32) is read from the device's node.
>> +
>> + At this time the following DM device classes are supported:
>> + * RTC (using read32/write32)
>> +
>> endchoice
>>
>> config BOOTCOUNT_ALEN
>> diff --git a/drivers/bootcount/Makefile b/drivers/bootcount/Makefile
>> index 68bc006..e8ed729 100644
>> --- a/drivers/bootcount/Makefile
>> +++ b/drivers/bootcount/Makefile
>> @@ -7,3 +7,4 @@ 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_DM) += bootcount_dm.o
>> diff --git a/drivers/bootcount/bootcount_dm.c
>> b/drivers/bootcount/bootcount_dm.c new file mode 100644
>> index 0000000..99bdb88
>> --- /dev/null
>> +++ b/drivers/bootcount/bootcount_dm.c
>> @@ -0,0 +1,93 @@
>> +// SPDX-License-Identifier: GPL-2.0+
>> +/*
>> + * (C) Copyright 2018 Theobroma Systems Design und Consulting GmbH
>> + */
>> +
>> +#include <common.h>
>> +#include <bootcount.h>
>> +#include <dm.h>
>> +#include <rtc.h>
>> +
>> +const u8 bootcount_magic = 0xbc;
>> +
>> +static void bootcount_store_rtc(struct udevice *dev, ulong a)
>> +{
>> +#if CONFIG_IS_ENABLED(DM_RTC)
>> + u32 offset;
>> + const char *offset_propname = "u-boot,bootcount-offset";
>> + const u16 val = bootcount_magic << 8 | (a & 0xff);
>> +
>> + if (dev_read_u32(dev, offset_propname, &offset) < 0) {
>> + debug("%s: requires %s\n", __func__,
>> offset_propname);
>> + return;
>> + }
>> +
>> + if (rtc_write16(dev, offset, val) < 0) {
>> + debug("%s: rtc_write16 failed\n", __func__);
>> + return;
>> + }
>> +#endif
>> +}
>> +
>> +static u32 bootcount_load_rtc(struct udevice *dev)
>> +{
>> +#if CONFIG_IS_ENABLED(DM_RTC)
>> + u32 offset;
>> + const char *offset_propname = "u-boot,bootcount-offset";
>> + u16 val;
>> +
>> + if (dev_read_u32(dev, offset_propname, &offset) < 0) {
>> + debug("%s: requires %s\n", __func__,
>> offset_propname);
>> + goto fail;
>> + }
>> +
>> + if (rtc_read16(dev, offset, &val) < 0) {
>> + debug("%s: rtc_write16 failed\n", __func__);
>> + goto fail;
>> + }
>> +
>> + if (val >> 8 == bootcount_magic)
>> + return val & 0xff;
>> +
>> + debug("%s: bootcount magic does not match on %04x\n",
>> __func__, val);
>> + fail:
>> +#endif
>> + return 0;
>> +}
>> +
>> +/* Now implement the generic default functions */
>> +void bootcount_store(ulong a)
>> +{
>> + struct udevice *dev;
>> + ofnode node;
>> + const char *propname = "u-boot,bootcount-device";
>> +
>> + node = ofnode_get_chosen_node(propname);
>> + if (!ofnode_valid(node)) {
>> + debug("%s: no '%s'\n", __func__, propname);
>> + return;
>> + }
>> +
>> + /* RTC devices */
>> + if (!uclass_get_device_by_ofnode(UCLASS_RTC, node, &dev))
>> + return bootcount_store_rtc(dev, a);
>> +}
>> +
>> +ulong bootcount_load(void)
>> +{
>> + struct udevice *dev;
>> + ofnode node;
>> + const char *propname = "u-boot,bootcount-device";
>> +
>> + node = ofnode_get_chosen_node(propname);
>> + if (!ofnode_valid(node)) {
>> + debug("%s: no '%s'\n", __func__, propname);
>> + return 0;
>> + }
>> +
>> + /* RTC devices */
>> + if (!uclass_get_device_by_ofnode(UCLASS_RTC, node, &dev))
>> + return bootcount_load_rtc(dev);
>> +
>> + return 0;
>> +}
>
> Thanks for your patch.
>
> However, if I may ask - would it be possible to add support for EEPROM
> based bootcount in an easy way?
This was always intended and is the reason why there’s a "RTC devices”
comment in bootcount_load.
If someone wants to store their bootcount in an I2C EEPROM, they just
need to add a “uclass_get_device_by_ofnode(UCLASS_I2C_EEPROM, …)”
with the appropriate logic in bootcount_load and bootcount_store.
> I mean - do you think that it would be feasible to have
> bootcount-uclass, which would support generic load/store functions with
> DM drivers for RTC, EEPROM or even simple write to SFR registers (as it
> is just an offset in the end)?
I thought about this, but didn’t go down that route as a bootcount will usually
be stored in an existing DM device (RTC, I2C-EEPROM, NOR flash). If we
add individual bootcount-devices wrapping these, then we’d be left with the
following in the -u-boot.dtsi:
bootcount {
compatible = “u-boot,bootcount-rtc”
rtc = &rv3029;
offset = <0x38>
}
While this nicely collects all the info together in one node, there’s the drawback
of users that might define multiple devices and set their status to “okay”… which
might cause inconsistent bootcount values across multiple devices.
For this reason, I went with the other design choice of viewing the various bootcount
implementations as “bootcount methods” (i.e. logic storing and retrieving a bootcount
somewhere). In the case of DM backends this means that the appropriate method
is to (a) identify the device by its uclass and (b) call the appropriate read/write method.
I briefly toyed with the idea of adding infrastructure to the DM core to get the device
for an ofnode (independent of its uclass) and adding a generic dm_read/dm_write
that would dispatch to the appropriate uclass’ read/write after looking at the uclass
of a udevice passed in. I didn’t go down that route as it seemed to contradict the
architecture of DM (i.e. devices are stored in per-uclass lists) in U-Boot.
One more thing that influenced the current modelling in the DTS: it prefer to have
all info regarding how the SRAM in a RTC—same for the memory in an EEPROM—
is subdivided in the RTC node. If this was in a separate node (as the bootcount
node above), someone might reuse the same SRAM addresses for different
purposes from different nodes causing inadvertent overwriting of live data.
>
>
> Best regards,
>
> Lukasz Majewski
>
> --
>
> DENX Software Engineering GmbH, Managing Director: Wolfgang Denk
> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
> Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
^ permalink raw reply [flat|nested] 13+ messages in thread
* [U-Boot] [PATCH 1/3] rtc: rv3029: add to Kconfig
2018-08-14 9:51 ` [U-Boot] [PATCH 1/3] rtc: rv3029: add to Kconfig Philipp Tomsich
@ 2018-08-14 16:46 ` Heinrich Schuchardt
2018-08-14 16:52 ` Dr. Philipp Tomsich
0 siblings, 1 reply; 13+ messages in thread
From: Heinrich Schuchardt @ 2018-08-14 16:46 UTC (permalink / raw)
To: u-boot
On 08/14/2018 11:51 AM, Philipp Tomsich wrote:
> The MicroCrystal RV3029 driver didn't have a Kconfig entry and was not used
> anywhere. Add it to Kconfig to make it selectable.
>
> Signed-off-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>
> Tested-by: Klaus Goger <klaus.goger@theobroma-systems.com>
> ---
>
> drivers/rtc/Kconfig | 10 ++++++++++
> 1 file changed, 10 insertions(+)
>
> diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig
> index 5436509..8a6e796 100644
> --- a/drivers/rtc/Kconfig
> +++ b/drivers/rtc/Kconfig
> @@ -42,6 +42,16 @@ config RTC_ISL1208
> This driver supports reading and writing the RTC/calendar and detects
> total power failures.
>
> +config RTC_RV3029
> + bool "Enable RV3029 driver"
> + depends on DM_RTC
> + help
> + The MicroCrystal RV3029 is a I2C Real Time Clock (RTC) with 8-byte
%s/a I2C/an I2C/
> + battery-backed SRAM.
> +
> + This driver supports reading and writing the RTC/calendar and the
What do you mean by calendar? If you just mean the RTC date register,
just drop the misleading word.
%s/RTC\/calendar/RTC/
> + battery-baced SRAM section.
%s/baced/backed/
Best regards
Heinrich
> +
> config RTC_RX8010SJ
> bool "Enable RX8010SJ driver"
> depends on DM_RTC
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* [U-Boot] [PATCH 1/3] rtc: rv3029: add to Kconfig
2018-08-14 16:46 ` Heinrich Schuchardt
@ 2018-08-14 16:52 ` Dr. Philipp Tomsich
0 siblings, 0 replies; 13+ messages in thread
From: Dr. Philipp Tomsich @ 2018-08-14 16:52 UTC (permalink / raw)
To: u-boot
> On 14 Aug 2018, at 18:46, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
> On 08/14/2018 11:51 AM, Philipp Tomsich wrote:
>> The MicroCrystal RV3029 driver didn't have a Kconfig entry and was not used
>> anywhere. Add it to Kconfig to make it selectable.
>>
>> Signed-off-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>
>> Tested-by: Klaus Goger <klaus.goger@theobroma-systems.com>
>> ---
>>
>> drivers/rtc/Kconfig | 10 ++++++++++
>> 1 file changed, 10 insertions(+)
>>
>> diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig
>> index 5436509..8a6e796 100644
>> --- a/drivers/rtc/Kconfig
>> +++ b/drivers/rtc/Kconfig
>> @@ -42,6 +42,16 @@ config RTC_ISL1208
>> This driver supports reading and writing the RTC/calendar and detects
>> total power failures.
>>
>> +config RTC_RV3029
>> + bool "Enable RV3029 driver"
>> + depends on DM_RTC
>> + help
>> + The MicroCrystal RV3029 is a I2C Real Time Clock (RTC) with 8-byte
>
> %s/a I2C/an I2C/
>
>> + battery-backed SRAM.
>> +
>> + This driver supports reading and writing the RTC/calendar and the
>
> What do you mean by calendar? If you just mean the RTC date register,
> just drop the misleading word.
>
> %s/RTC\/calendar/RTC/
The RV3029 datasheet also describes the devices as a RTC/calendar:
"The RV-3029 is a CMOS low power, real-time clock/calendar module…”
In the same Kconfig, there’s the same duality (i.e. RTC _and_ calendar) in the
help text for the
RTC_PCF2127
RTC_ISL1208
Happy to drop this here, if that’s the consensus...
>
>> + battery-baced SRAM section.
>
> %s/baced/backed/
>
> Best regards
>
> Heinrich
>
>> +
>> config RTC_RX8010SJ
>> bool "Enable RX8010SJ driver"
>> depends on DM_RTC
>>
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* [U-Boot] [PATCH 3/3] bootcount: add DM-based backing store for bootcount
2018-08-14 11:39 ` Dr. Philipp Tomsich
@ 2018-08-17 12:49 ` Simon Glass
2018-08-17 12:56 ` Dr. Philipp Tomsich
0 siblings, 1 reply; 13+ messages in thread
From: Simon Glass @ 2018-08-17 12:49 UTC (permalink / raw)
To: u-boot
Hi Philipp,
On 14 August 2018 at 05:39, Dr. Philipp Tomsich
<philipp.tomsich@theobroma-systems.com> wrote:
> Lukasz,
>
>> On 14 Aug 2018, at 13:10, Lukasz Majewski <lukma@denx.de> wrote:
>>
>> Hi Philipp,
>>
>>> The original bootcount methods do not provide an interface to DM and
>>> rely on a static configuration for I2C devices (e.g. bus, chip-addr,
>>> etc. are configured through defines statically). On a modern system
>>> that exposes multiple devices in a DTS-configurable way, this is less
>>> than optimal and a interface to DM-based devices will be desirable.
>>>
>>> This adds a simple driver that is DM-aware and configurable via DTS:
>>> the /chosen/u-boot,bootcount-device property is used to detect the DM
>>> device storing the bootcount and deviceclass-specific commands are
>>> used to read/write the bootcount.
>>>
>>> Initially, this provides support for the following DM devices:
>>> * RTC devices implementing the read8/write8 ops
>>>
>>> Signed-off-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>
>>> Tested-by: Klaus Goger <klaus.goger@theobroma-systems.com>
>>> ---
>>>
>>> doc/device-tree-bindings/chosen.txt | 27 +++++++++++
>>> drivers/bootcount/Kconfig | 12 +++++
>>> drivers/bootcount/Makefile | 1 +
>>> drivers/bootcount/bootcount_dm.c | 93
>>> +++++++++++++++++++++++++++++++++++++ 4 files changed, 133
>>> insertions(+) create mode 100644 drivers/bootcount/bootcount_dm.c
>>>
>>> diff --git a/doc/device-tree-bindings/chosen.txt
>>> b/doc/device-tree-bindings/chosen.txt index da7b4e6..734fd15 100644
>>> --- a/doc/device-tree-bindings/chosen.txt
>>> +++ b/doc/device-tree-bindings/chosen.txt
>>> @@ -42,6 +42,33 @@ Example
>>> };
>>> };
>>>
>>> +u-boot,bootcount-device property
>>> +--------------------------------
>>> +In a DM-based system, the bootcount may be stored in a device known
>>> to +the DM framework (e.g. in a battery-backed SRAM area within a RTC
>>> +device). To identify the device to be used for the bootcount, the
>>> +u-boot,bootcount-device property needs to point to the target device.
>>> +
>>> +Further configuration in the target device's node may be required
>>> +(e.g. an offset into an I2C RTC's address space), depending on the
>>> +UCLASS of the target device.
>>> +
>>> +Example
>>> +-------
>>> +/ {
>>> + chosen {
>>> + u-boot,bootcount-device = &rv3029;
>>> + };
>>> +
>>> + i2c2 {
>>> + rv3029: rtc at 56 {
>>> + compatible = "mc,rv3029";
>>> + reg = <0x56>;
>>> + u-boot,bootcount-offset = <0x38>;
>>> + };
>>> + };
>>> +};
>>> +
>>> u-boot,spl-boot-order property
>>> ------------------------------
>>>
>>> diff --git a/drivers/bootcount/Kconfig b/drivers/bootcount/Kconfig
>>> index d335ed1..cdde7b5 100644
>>> --- a/drivers/bootcount/Kconfig
>>> +++ b/drivers/bootcount/Kconfig
>>> @@ -70,6 +70,18 @@ config BOOTCOUNT_AT91
>>> bool "Boot counter for Atmel AT91SAM9XE"
>>> depends on AT91SAM9XE
>>>
>>> +config BOOTCOUNT_DM
>>> + bool "Boot counter in a device-model device"
>>> + help
>>> + Enables reading/writing the bootcount in a device-model
>>> + device referenced from the /chosen node. The type of the
>>> + device is detected from DM and any additional configuration
>>> + information (e.g. the offset into a RTC device that
>>> supports
>>> + read32/write32) is read from the device's node.
>>> +
>>> + At this time the following DM device classes are supported:
>>> + * RTC (using read32/write32)
>>> +
>>> endchoice
>>>
>>> config BOOTCOUNT_ALEN
>>> diff --git a/drivers/bootcount/Makefile b/drivers/bootcount/Makefile
>>> index 68bc006..e8ed729 100644
>>> --- a/drivers/bootcount/Makefile
>>> +++ b/drivers/bootcount/Makefile
>>> @@ -7,3 +7,4 @@ 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_DM) += bootcount_dm.o
>>> diff --git a/drivers/bootcount/bootcount_dm.c
>>> b/drivers/bootcount/bootcount_dm.c new file mode 100644
>>> index 0000000..99bdb88
>>> --- /dev/null
>>> +++ b/drivers/bootcount/bootcount_dm.c
>>> @@ -0,0 +1,93 @@
>>> +// SPDX-License-Identifier: GPL-2.0+
>>> +/*
>>> + * (C) Copyright 2018 Theobroma Systems Design und Consulting GmbH
>>> + */
>>> +
>>> +#include <common.h>
>>> +#include <bootcount.h>
>>> +#include <dm.h>
>>> +#include <rtc.h>
>>> +
>>> +const u8 bootcount_magic = 0xbc;
>>> +
>>> +static void bootcount_store_rtc(struct udevice *dev, ulong a)
>>> +{
>>> +#if CONFIG_IS_ENABLED(DM_RTC)
>>> + u32 offset;
>>> + const char *offset_propname = "u-boot,bootcount-offset";
>>> + const u16 val = bootcount_magic << 8 | (a & 0xff);
>>> +
>>> + if (dev_read_u32(dev, offset_propname, &offset) < 0) {
>>> + debug("%s: requires %s\n", __func__,
>>> offset_propname);
>>> + return;
>>> + }
>>> +
>>> + if (rtc_write16(dev, offset, val) < 0) {
>>> + debug("%s: rtc_write16 failed\n", __func__);
>>> + return;
>>> + }
>>> +#endif
>>> +}
>>> +
>>> +static u32 bootcount_load_rtc(struct udevice *dev)
>>> +{
>>> +#if CONFIG_IS_ENABLED(DM_RTC)
>>> + u32 offset;
>>> + const char *offset_propname = "u-boot,bootcount-offset";
>>> + u16 val;
>>> +
>>> + if (dev_read_u32(dev, offset_propname, &offset) < 0) {
>>> + debug("%s: requires %s\n", __func__,
>>> offset_propname);
>>> + goto fail;
>>> + }
>>> +
>>> + if (rtc_read16(dev, offset, &val) < 0) {
>>> + debug("%s: rtc_write16 failed\n", __func__);
>>> + goto fail;
>>> + }
>>> +
>>> + if (val >> 8 == bootcount_magic)
>>> + return val & 0xff;
>>> +
>>> + debug("%s: bootcount magic does not match on %04x\n",
>>> __func__, val);
>>> + fail:
>>> +#endif
>>> + return 0;
>>> +}
>>> +
>>> +/* Now implement the generic default functions */
>>> +void bootcount_store(ulong a)
>>> +{
>>> + struct udevice *dev;
>>> + ofnode node;
>>> + const char *propname = "u-boot,bootcount-device";
>>> +
>>> + node = ofnode_get_chosen_node(propname);
>>> + if (!ofnode_valid(node)) {
>>> + debug("%s: no '%s'\n", __func__, propname);
>>> + return;
>>> + }
>>> +
>>> + /* RTC devices */
>>> + if (!uclass_get_device_by_ofnode(UCLASS_RTC, node, &dev))
>>> + return bootcount_store_rtc(dev, a);
>>> +}
>>> +
>>> +ulong bootcount_load(void)
>>> +{
>>> + struct udevice *dev;
>>> + ofnode node;
>>> + const char *propname = "u-boot,bootcount-device";
>>> +
>>> + node = ofnode_get_chosen_node(propname);
>>> + if (!ofnode_valid(node)) {
>>> + debug("%s: no '%s'\n", __func__, propname);
>>> + return 0;
>>> + }
>>> +
>>> + /* RTC devices */
>>> + if (!uclass_get_device_by_ofnode(UCLASS_RTC, node, &dev))
>>> + return bootcount_load_rtc(dev);
>>> +
>>> + return 0;
>>> +}
>>
>> Thanks for your patch.
>>
>> However, if I may ask - would it be possible to add support for EEPROM
>> based bootcount in an easy way?
>
> This was always intended and is the reason why there’s a "RTC devices”
> comment in bootcount_load.
>
> If someone wants to store their bootcount in an I2C EEPROM, they just
> need to add a “uclass_get_device_by_ofnode(UCLASS_I2C_EEPROM, …)”
> with the appropriate logic in bootcount_load and bootcount_store.
>
>> I mean - do you think that it would be feasible to have
>> bootcount-uclass, which would support generic load/store functions with
>> DM drivers for RTC, EEPROM or even simple write to SFR registers (as it
>> is just an offset in the end)?
>
> I thought about this, but didn’t go down that route as a bootcount will usually
> be stored in an existing DM device (RTC, I2C-EEPROM, NOR flash). If we
> add individual bootcount-devices wrapping these, then we’d be left with the
> following in the -u-boot.dtsi:
>
> bootcount {
> compatible = “u-boot,bootcount-rtc”
> rtc = &rv3029;
> offset = <0x38>
> }
>
> While this nicely collects all the info together in one node, there’s the drawback
> of users that might define multiple devices and set their status to “okay”… which
> might cause inconsistent bootcount values across multiple devices.
>
> For this reason, I went with the other design choice of viewing the various bootcount
> implementations as “bootcount methods” (i.e. logic storing and retrieving a bootcount
> somewhere). In the case of DM backends this means that the appropriate method
> is to (a) identify the device by its uclass and (b) call the appropriate read/write method.
>
> I briefly toyed with the idea of adding infrastructure to the DM core to get the device
> for an ofnode (independent of its uclass) and adding a generic dm_read/dm_write
> that would dispatch to the appropriate uclass’ read/write after looking at the uclass
> of a udevice passed in. I didn’t go down that route as it seemed to contradict the
> architecture of DM (i.e. devices are stored in per-uclass lists) in U-Boot.
>
> One more thing that influenced the current modelling in the DTS: it prefer to have
> all info regarding how the SRAM in a RTC—same for the memory in an EEPROM—
> is subdivided in the RTC node. If this was in a separate node (as the bootcount
> node above), someone might reuse the same SRAM addresses for different
> purposes from different nodes causing inadvertent overwriting of live data.
I have to agree with Lukasz that this should really be a uclass with a
driver for RTC and perhaps one for EEPROM.
But we also have patches roaming around for a BOARD uclass, which
provides for reading settings from the board, which could of course
use any kind of storage. Would that be another option?
Regards,
Simon
^ permalink raw reply [flat|nested] 13+ messages in thread
* [U-Boot] [PATCH 3/3] bootcount: add DM-based backing store for bootcount
2018-08-17 12:49 ` Simon Glass
@ 2018-08-17 12:56 ` Dr. Philipp Tomsich
2018-08-23 10:45 ` Simon Glass
0 siblings, 1 reply; 13+ messages in thread
From: Dr. Philipp Tomsich @ 2018-08-17 12:56 UTC (permalink / raw)
To: u-boot
Simon,
> On 17 Aug 2018, at 14:49, Simon Glass <sjg@chromium.org> wrote:
>
> Hi Philipp,
>
> On 14 August 2018 at 05:39, Dr. Philipp Tomsich
> <philipp.tomsich at theobroma-systems.com <mailto:philipp.tomsich@theobroma-systems.com>> wrote:
>> Lukasz,
>>
>>> On 14 Aug 2018, at 13:10, Lukasz Majewski <lukma@denx.de> wrote:
>>>
>>> Hi Philipp,
>>>
>>>> The original bootcount methods do not provide an interface to DM and
>>>> rely on a static configuration for I2C devices (e.g. bus, chip-addr,
>>>> etc. are configured through defines statically). On a modern system
>>>> that exposes multiple devices in a DTS-configurable way, this is less
>>>> than optimal and a interface to DM-based devices will be desirable.
>>>>
>>>> This adds a simple driver that is DM-aware and configurable via DTS:
>>>> the /chosen/u-boot,bootcount-device property is used to detect the DM
>>>> device storing the bootcount and deviceclass-specific commands are
>>>> used to read/write the bootcount.
>>>>
>>>> Initially, this provides support for the following DM devices:
>>>> * RTC devices implementing the read8/write8 ops
>>>>
>>>> Signed-off-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>
>>>> Tested-by: Klaus Goger <klaus.goger@theobroma-systems.com>
>>>> ---
>>>>
>>>> doc/device-tree-bindings/chosen.txt | 27 +++++++++++
>>>> drivers/bootcount/Kconfig | 12 +++++
>>>> drivers/bootcount/Makefile | 1 +
>>>> drivers/bootcount/bootcount_dm.c | 93
>>>> +++++++++++++++++++++++++++++++++++++ 4 files changed, 133
>>>> insertions(+) create mode 100644 drivers/bootcount/bootcount_dm.c
>>>>
>>>> diff --git a/doc/device-tree-bindings/chosen.txt
>>>> b/doc/device-tree-bindings/chosen.txt index da7b4e6..734fd15 100644
>>>> --- a/doc/device-tree-bindings/chosen.txt
>>>> +++ b/doc/device-tree-bindings/chosen.txt
>>>> @@ -42,6 +42,33 @@ Example
>>>> };
>>>> };
>>>>
>>>> +u-boot,bootcount-device property
>>>> +--------------------------------
>>>> +In a DM-based system, the bootcount may be stored in a device known
>>>> to +the DM framework (e.g. in a battery-backed SRAM area within a RTC
>>>> +device). To identify the device to be used for the bootcount, the
>>>> +u-boot,bootcount-device property needs to point to the target device.
>>>> +
>>>> +Further configuration in the target device's node may be required
>>>> +(e.g. an offset into an I2C RTC's address space), depending on the
>>>> +UCLASS of the target device.
>>>> +
>>>> +Example
>>>> +-------
>>>> +/ {
>>>> + chosen {
>>>> + u-boot,bootcount-device = &rv3029;
>>>> + };
>>>> +
>>>> + i2c2 {
>>>> + rv3029: rtc at 56 {
>>>> + compatible = "mc,rv3029";
>>>> + reg = <0x56>;
>>>> + u-boot,bootcount-offset = <0x38>;
>>>> + };
>>>> + };
>>>> +};
>>>> +
>>>> u-boot,spl-boot-order property
>>>> ------------------------------
>>>>
>>>> diff --git a/drivers/bootcount/Kconfig b/drivers/bootcount/Kconfig
>>>> index d335ed1..cdde7b5 100644
>>>> --- a/drivers/bootcount/Kconfig
>>>> +++ b/drivers/bootcount/Kconfig
>>>> @@ -70,6 +70,18 @@ config BOOTCOUNT_AT91
>>>> bool "Boot counter for Atmel AT91SAM9XE"
>>>> depends on AT91SAM9XE
>>>>
>>>> +config BOOTCOUNT_DM
>>>> + bool "Boot counter in a device-model device"
>>>> + help
>>>> + Enables reading/writing the bootcount in a device-model
>>>> + device referenced from the /chosen node. The type of the
>>>> + device is detected from DM and any additional configuration
>>>> + information (e.g. the offset into a RTC device that
>>>> supports
>>>> + read32/write32) is read from the device's node.
>>>> +
>>>> + At this time the following DM device classes are supported:
>>>> + * RTC (using read32/write32)
>>>> +
>>>> endchoice
>>>>
>>>> config BOOTCOUNT_ALEN
>>>> diff --git a/drivers/bootcount/Makefile b/drivers/bootcount/Makefile
>>>> index 68bc006..e8ed729 100644
>>>> --- a/drivers/bootcount/Makefile
>>>> +++ b/drivers/bootcount/Makefile
>>>> @@ -7,3 +7,4 @@ 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_DM) += bootcount_dm.o
>>>> diff --git a/drivers/bootcount/bootcount_dm.c
>>>> b/drivers/bootcount/bootcount_dm.c new file mode 100644
>>>> index 0000000..99bdb88
>>>> --- /dev/null
>>>> +++ b/drivers/bootcount/bootcount_dm.c
>>>> @@ -0,0 +1,93 @@
>>>> +// SPDX-License-Identifier: GPL-2.0+
>>>> +/*
>>>> + * (C) Copyright 2018 Theobroma Systems Design und Consulting GmbH
>>>> + */
>>>> +
>>>> +#include <common.h>
>>>> +#include <bootcount.h>
>>>> +#include <dm.h>
>>>> +#include <rtc.h>
>>>> +
>>>> +const u8 bootcount_magic = 0xbc;
>>>> +
>>>> +static void bootcount_store_rtc(struct udevice *dev, ulong a)
>>>> +{
>>>> +#if CONFIG_IS_ENABLED(DM_RTC)
>>>> + u32 offset;
>>>> + const char *offset_propname = "u-boot,bootcount-offset";
>>>> + const u16 val = bootcount_magic << 8 | (a & 0xff);
>>>> +
>>>> + if (dev_read_u32(dev, offset_propname, &offset) < 0) {
>>>> + debug("%s: requires %s\n", __func__,
>>>> offset_propname);
>>>> + return;
>>>> + }
>>>> +
>>>> + if (rtc_write16(dev, offset, val) < 0) {
>>>> + debug("%s: rtc_write16 failed\n", __func__);
>>>> + return;
>>>> + }
>>>> +#endif
>>>> +}
>>>> +
>>>> +static u32 bootcount_load_rtc(struct udevice *dev)
>>>> +{
>>>> +#if CONFIG_IS_ENABLED(DM_RTC)
>>>> + u32 offset;
>>>> + const char *offset_propname = "u-boot,bootcount-offset";
>>>> + u16 val;
>>>> +
>>>> + if (dev_read_u32(dev, offset_propname, &offset) < 0) {
>>>> + debug("%s: requires %s\n", __func__,
>>>> offset_propname);
>>>> + goto fail;
>>>> + }
>>>> +
>>>> + if (rtc_read16(dev, offset, &val) < 0) {
>>>> + debug("%s: rtc_write16 failed\n", __func__);
>>>> + goto fail;
>>>> + }
>>>> +
>>>> + if (val >> 8 == bootcount_magic)
>>>> + return val & 0xff;
>>>> +
>>>> + debug("%s: bootcount magic does not match on %04x\n",
>>>> __func__, val);
>>>> + fail:
>>>> +#endif
>>>> + return 0;
>>>> +}
>>>> +
>>>> +/* Now implement the generic default functions */
>>>> +void bootcount_store(ulong a)
>>>> +{
>>>> + struct udevice *dev;
>>>> + ofnode node;
>>>> + const char *propname = "u-boot,bootcount-device";
>>>> +
>>>> + node = ofnode_get_chosen_node(propname);
>>>> + if (!ofnode_valid(node)) {
>>>> + debug("%s: no '%s'\n", __func__, propname);
>>>> + return;
>>>> + }
>>>> +
>>>> + /* RTC devices */
>>>> + if (!uclass_get_device_by_ofnode(UCLASS_RTC, node, &dev))
>>>> + return bootcount_store_rtc(dev, a);
>>>> +}
>>>> +
>>>> +ulong bootcount_load(void)
>>>> +{
>>>> + struct udevice *dev;
>>>> + ofnode node;
>>>> + const char *propname = "u-boot,bootcount-device";
>>>> +
>>>> + node = ofnode_get_chosen_node(propname);
>>>> + if (!ofnode_valid(node)) {
>>>> + debug("%s: no '%s'\n", __func__, propname);
>>>> + return 0;
>>>> + }
>>>> +
>>>> + /* RTC devices */
>>>> + if (!uclass_get_device_by_ofnode(UCLASS_RTC, node, &dev))
>>>> + return bootcount_load_rtc(dev);
>>>> +
>>>> + return 0;
>>>> +}
>>>
>>> Thanks for your patch.
>>>
>>> However, if I may ask - would it be possible to add support for EEPROM
>>> based bootcount in an easy way?
>>
>> This was always intended and is the reason why there’s a "RTC devices”
>> comment in bootcount_load.
>>
>> If someone wants to store their bootcount in an I2C EEPROM, they just
>> need to add a “uclass_get_device_by_ofnode(UCLASS_I2C_EEPROM, …)”
>> with the appropriate logic in bootcount_load and bootcount_store.
>>
>>> I mean - do you think that it would be feasible to have
>>> bootcount-uclass, which would support generic load/store functions with
>>> DM drivers for RTC, EEPROM or even simple write to SFR registers (as it
>>> is just an offset in the end)?
>>
>> I thought about this, but didn’t go down that route as a bootcount will usually
>> be stored in an existing DM device (RTC, I2C-EEPROM, NOR flash). If we
>> add individual bootcount-devices wrapping these, then we’d be left with the
>> following in the -u-boot.dtsi:
>>
>> bootcount {
>> compatible = “u-boot,bootcount-rtc”
>> rtc = &rv3029;
>> offset = <0x38>
>> }
>>
>> While this nicely collects all the info together in one node, there’s the drawback
>> of users that might define multiple devices and set their status to “okay”… which
>> might cause inconsistent bootcount values across multiple devices.
>>
>> For this reason, I went with the other design choice of viewing the various bootcount
>> implementations as “bootcount methods” (i.e. logic storing and retrieving a bootcount
>> somewhere). In the case of DM backends this means that the appropriate method
>> is to (a) identify the device by its uclass and (b) call the appropriate read/write method.
>>
>> I briefly toyed with the idea of adding infrastructure to the DM core to get the device
>> for an ofnode (independent of its uclass) and adding a generic dm_read/dm_write
>> that would dispatch to the appropriate uclass’ read/write after looking at the uclass
>> of a udevice passed in. I didn’t go down that route as it seemed to contradict the
>> architecture of DM (i.e. devices are stored in per-uclass lists) in U-Boot.
>>
>> One more thing that influenced the current modelling in the DTS: it prefer to have
>> all info regarding how the SRAM in a RTC—same for the memory in an EEPROM—
>> is subdivided in the RTC node. If this was in a separate node (as the bootcount
>> node above), someone might reuse the same SRAM addresses for different
>> purposes from different nodes causing inadvertent overwriting of live data.
>
> I have to agree with Lukasz that this should really be a uclass with a
> driver for RTC and perhaps one for EEPROM.
>
> But we also have patches roaming around for a BOARD uclass, which
> provides for reading settings from the board, which could of course
> use any kind of storage. Would that be another option?
I’ll change this over to be a new BOOTCOUNT uclass, although my reservation
regarding how this partitions a RTC’s or EEPROM’s address space remains.
I guess, we’ll have to live with that.
I don’t think this should be merged with the BOARD uclass, as the bootcount
is a standalone feature that could be configured differently even for the same
board (i.e. this is not similar enough to reading a board id to merge it into the
BOARD uclass).
Thanks,
Philipp.
^ permalink raw reply [flat|nested] 13+ messages in thread
* [U-Boot] [PATCH 3/3] bootcount: add DM-based backing store for bootcount
2018-08-17 12:56 ` Dr. Philipp Tomsich
@ 2018-08-23 10:45 ` Simon Glass
2018-08-23 13:48 ` Dr. Philipp Tomsich
0 siblings, 1 reply; 13+ messages in thread
From: Simon Glass @ 2018-08-23 10:45 UTC (permalink / raw)
To: u-boot
Hi Philipp,
On 17 August 2018 at 06:56, Dr. Philipp Tomsich
<philipp.tomsich@theobroma-systems.com> wrote:
> Simon,
>
> On 17 Aug 2018, at 14:49, Simon Glass <sjg@chromium.org> wrote:
>
> Hi Philipp,
>
> On 14 August 2018 at 05:39, Dr. Philipp Tomsich
> <philipp.tomsich@theobroma-systems.com> wrote:
>
> Lukasz,
>
> On 14 Aug 2018, at 13:10, Lukasz Majewski <lukma@denx.de> wrote:
>
> Hi Philipp,
>
> The original bootcount methods do not provide an interface to DM and
> rely on a static configuration for I2C devices (e.g. bus, chip-addr,
> etc. are configured through defines statically). On a modern system
> that exposes multiple devices in a DTS-configurable way, this is less
> than optimal and a interface to DM-based devices will be desirable.
>
> This adds a simple driver that is DM-aware and configurable via DTS:
> the /chosen/u-boot,bootcount-device property is used to detect the DM
> device storing the bootcount and deviceclass-specific commands are
> used to read/write the bootcount.
>
> Initially, this provides support for the following DM devices:
> * RTC devices implementing the read8/write8 ops
>
> Signed-off-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>
> Tested-by: Klaus Goger <klaus.goger@theobroma-systems.com>
> ---
>
> doc/device-tree-bindings/chosen.txt | 27 +++++++++++
> drivers/bootcount/Kconfig | 12 +++++
> drivers/bootcount/Makefile | 1 +
> drivers/bootcount/bootcount_dm.c | 93
> +++++++++++++++++++++++++++++++++++++ 4 files changed, 133
> insertions(+) create mode 100644 drivers/bootcount/bootcount_dm.c
>
> diff --git a/doc/device-tree-bindings/chosen.txt
> b/doc/device-tree-bindings/chosen.txt index da7b4e6..734fd15 100644
> --- a/doc/device-tree-bindings/chosen.txt
> +++ b/doc/device-tree-bindings/chosen.txt
> @@ -42,6 +42,33 @@ Example
> };
> };
>
> +u-boot,bootcount-device property
> +--------------------------------
> +In a DM-based system, the bootcount may be stored in a device known
> to +the DM framework (e.g. in a battery-backed SRAM area within a RTC
> +device). To identify the device to be used for the bootcount, the
> +u-boot,bootcount-device property needs to point to the target device.
> +
> +Further configuration in the target device's node may be required
> +(e.g. an offset into an I2C RTC's address space), depending on the
> +UCLASS of the target device.
> +
> +Example
> +-------
> +/ {
> + chosen {
> + u-boot,bootcount-device = &rv3029;
> + };
> +
> + i2c2 {
> + rv3029: rtc at 56 {
> + compatible = "mc,rv3029";
> + reg = <0x56>;
> + u-boot,bootcount-offset = <0x38>;
> + };
> + };
> +};
> +
> u-boot,spl-boot-order property
> ------------------------------
>
> diff --git a/drivers/bootcount/Kconfig b/drivers/bootcount/Kconfig
> index d335ed1..cdde7b5 100644
> --- a/drivers/bootcount/Kconfig
> +++ b/drivers/bootcount/Kconfig
> @@ -70,6 +70,18 @@ config BOOTCOUNT_AT91
> bool "Boot counter for Atmel AT91SAM9XE"
> depends on AT91SAM9XE
>
> +config BOOTCOUNT_DM
> + bool "Boot counter in a device-model device"
> + help
> + Enables reading/writing the bootcount in a device-model
> + device referenced from the /chosen node. The type of the
> + device is detected from DM and any additional configuration
> + information (e.g. the offset into a RTC device that
> supports
> + read32/write32) is read from the device's node.
> +
> + At this time the following DM device classes are supported:
> + * RTC (using read32/write32)
> +
> endchoice
>
> config BOOTCOUNT_ALEN
> diff --git a/drivers/bootcount/Makefile b/drivers/bootcount/Makefile
> index 68bc006..e8ed729 100644
> --- a/drivers/bootcount/Makefile
> +++ b/drivers/bootcount/Makefile
> @@ -7,3 +7,4 @@ 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_DM) += bootcount_dm.o
> diff --git a/drivers/bootcount/bootcount_dm.c
> b/drivers/bootcount/bootcount_dm.c new file mode 100644
> index 0000000..99bdb88
> --- /dev/null
> +++ b/drivers/bootcount/bootcount_dm.c
> @@ -0,0 +1,93 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * (C) Copyright 2018 Theobroma Systems Design und Consulting GmbH
> + */
> +
> +#include <common.h>
> +#include <bootcount.h>
> +#include <dm.h>
> +#include <rtc.h>
> +
> +const u8 bootcount_magic = 0xbc;
> +
> +static void bootcount_store_rtc(struct udevice *dev, ulong a)
> +{
> +#if CONFIG_IS_ENABLED(DM_RTC)
> + u32 offset;
> + const char *offset_propname = "u-boot,bootcount-offset";
> + const u16 val = bootcount_magic << 8 | (a & 0xff);
> +
> + if (dev_read_u32(dev, offset_propname, &offset) < 0) {
> + debug("%s: requires %s\n", __func__,
> offset_propname);
> + return;
> + }
> +
> + if (rtc_write16(dev, offset, val) < 0) {
> + debug("%s: rtc_write16 failed\n", __func__);
> + return;
> + }
> +#endif
> +}
> +
> +static u32 bootcount_load_rtc(struct udevice *dev)
> +{
> +#if CONFIG_IS_ENABLED(DM_RTC)
> + u32 offset;
> + const char *offset_propname = "u-boot,bootcount-offset";
> + u16 val;
> +
> + if (dev_read_u32(dev, offset_propname, &offset) < 0) {
> + debug("%s: requires %s\n", __func__,
> offset_propname);
> + goto fail;
> + }
> +
> + if (rtc_read16(dev, offset, &val) < 0) {
> + debug("%s: rtc_write16 failed\n", __func__);
> + goto fail;
> + }
> +
> + if (val >> 8 == bootcount_magic)
> + return val & 0xff;
> +
> + debug("%s: bootcount magic does not match on %04x\n",
> __func__, val);
> + fail:
> +#endif
> + return 0;
> +}
> +
> +/* Now implement the generic default functions */
> +void bootcount_store(ulong a)
> +{
> + struct udevice *dev;
> + ofnode node;
> + const char *propname = "u-boot,bootcount-device";
> +
> + node = ofnode_get_chosen_node(propname);
> + if (!ofnode_valid(node)) {
> + debug("%s: no '%s'\n", __func__, propname);
> + return;
> + }
> +
> + /* RTC devices */
> + if (!uclass_get_device_by_ofnode(UCLASS_RTC, node, &dev))
> + return bootcount_store_rtc(dev, a);
> +}
> +
> +ulong bootcount_load(void)
> +{
> + struct udevice *dev;
> + ofnode node;
> + const char *propname = "u-boot,bootcount-device";
> +
> + node = ofnode_get_chosen_node(propname);
> + if (!ofnode_valid(node)) {
> + debug("%s: no '%s'\n", __func__, propname);
> + return 0;
> + }
> +
> + /* RTC devices */
> + if (!uclass_get_device_by_ofnode(UCLASS_RTC, node, &dev))
> + return bootcount_load_rtc(dev);
> +
> + return 0;
> +}
>
>
> Thanks for your patch.
>
> However, if I may ask - would it be possible to add support for EEPROM
> based bootcount in an easy way?
>
>
> This was always intended and is the reason why there’s a "RTC devices”
> comment in bootcount_load.
>
> If someone wants to store their bootcount in an I2C EEPROM, they just
> need to add a “uclass_get_device_by_ofnode(UCLASS_I2C_EEPROM, …)”
> with the appropriate logic in bootcount_load and bootcount_store.
>
> I mean - do you think that it would be feasible to have
> bootcount-uclass, which would support generic load/store functions with
> DM drivers for RTC, EEPROM or even simple write to SFR registers (as it
> is just an offset in the end)?
>
>
> I thought about this, but didn’t go down that route as a bootcount will
> usually
> be stored in an existing DM device (RTC, I2C-EEPROM, NOR flash). If we
> add individual bootcount-devices wrapping these, then we’d be left with the
> following in the -u-boot.dtsi:
>
> bootcount {
> compatible = “u-boot,bootcount-rtc”
> rtc = &rv3029;
> offset = <0x38>
> }
>
> While this nicely collects all the info together in one node, there’s the
> drawback
> of users that might define multiple devices and set their status to “okay”…
> which
> might cause inconsistent bootcount values across multiple devices.
>
> For this reason, I went with the other design choice of viewing the various
> bootcount
> implementations as “bootcount methods” (i.e. logic storing and retrieving a
> bootcount
> somewhere). In the case of DM backends this means that the appropriate
> method
> is to (a) identify the device by its uclass and (b) call the appropriate
> read/write method.
>
> I briefly toyed with the idea of adding infrastructure to the DM core to get
> the device
> for an ofnode (independent of its uclass) and adding a generic
> dm_read/dm_write
> that would dispatch to the appropriate uclass’ read/write after looking at
> the uclass
> of a udevice passed in. I didn’t go down that route as it seemed to
> contradict the
> architecture of DM (i.e. devices are stored in per-uclass lists) in U-Boot.
>
> One more thing that influenced the current modelling in the DTS: it prefer
> to have
> all info regarding how the SRAM in a RTC—same for the memory in an EEPROM—
> is subdivided in the RTC node. If this was in a separate node (as the
> bootcount
> node above), someone might reuse the same SRAM addresses for different
> purposes from different nodes causing inadvertent overwriting of live data.
>
>
> I have to agree with Lukasz that this should really be a uclass with a
> driver for RTC and perhaps one for EEPROM.
>
> But we also have patches roaming around for a BOARD uclass, which
> provides for reading settings from the board, which could of course
> use any kind of storage. Would that be another option?
>
>
> I’ll change this over to be a new BOOTCOUNT uclass, although my reservation
> regarding how this partitions a RTC’s or EEPROM’s address space remains.
> I guess, we’ll have to live with that.
I don't fully understand this but will await the patch for this. I'm
assuming a DT property would be needed.
>
> I don’t think this should be merged with the BOARD uclass, as the bootcount
> is a standalone feature that could be configured differently even for the
> same
> board (i.e. this is not similar enough to reading a board id to merge it
> into the
> BOARD uclass).
>
OK. I suppose the BOARD uclass could make use of a BOOTCOUNT device if needed.
Regards,
Simon
^ permalink raw reply [flat|nested] 13+ messages in thread
* [U-Boot] [PATCH 3/3] bootcount: add DM-based backing store for bootcount
2018-08-23 10:45 ` Simon Glass
@ 2018-08-23 13:48 ` Dr. Philipp Tomsich
2018-08-30 0:29 ` Simon Glass
0 siblings, 1 reply; 13+ messages in thread
From: Dr. Philipp Tomsich @ 2018-08-23 13:48 UTC (permalink / raw)
To: u-boot
Simon,
> On 23 Aug 2018, at 12:45, Simon Glass <sjg@chromium.org> wrote:
>
> Hi Philipp,
>
> On 17 August 2018 at 06:56, Dr. Philipp Tomsich
> <philipp.tomsich@theobroma-systems.com> wrote:
>> Simon,
>>
>> On 17 Aug 2018, at 14:49, Simon Glass <sjg@chromium.org> wrote:
>>
>> Hi Philipp,
>>
>> On 14 August 2018 at 05:39, Dr. Philipp Tomsich
>> <philipp.tomsich@theobroma-systems.com> wrote:
>>
>> Lukasz,
>>
>> On 14 Aug 2018, at 13:10, Lukasz Majewski <lukma@denx.de> wrote:
>>
>> Hi Philipp,
>>
>> The original bootcount methods do not provide an interface to DM and
>> rely on a static configuration for I2C devices (e.g. bus, chip-addr,
>> etc. are configured through defines statically). On a modern system
>> that exposes multiple devices in a DTS-configurable way, this is less
>> than optimal and a interface to DM-based devices will be desirable.
>>
>> This adds a simple driver that is DM-aware and configurable via DTS:
>> the /chosen/u-boot,bootcount-device property is used to detect the DM
>> device storing the bootcount and deviceclass-specific commands are
>> used to read/write the bootcount.
>>
>> Initially, this provides support for the following DM devices:
>> * RTC devices implementing the read8/write8 ops
>>
>> Signed-off-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>
>> Tested-by: Klaus Goger <klaus.goger@theobroma-systems.com>
>> ---
>>
>> doc/device-tree-bindings/chosen.txt | 27 +++++++++++
>> drivers/bootcount/Kconfig | 12 +++++
>> drivers/bootcount/Makefile | 1 +
>> drivers/bootcount/bootcount_dm.c | 93
>> +++++++++++++++++++++++++++++++++++++ 4 files changed, 133
>> insertions(+) create mode 100644 drivers/bootcount/bootcount_dm.c
>>
>> diff --git a/doc/device-tree-bindings/chosen.txt
>> b/doc/device-tree-bindings/chosen.txt index da7b4e6..734fd15 100644
>> --- a/doc/device-tree-bindings/chosen.txt
>> +++ b/doc/device-tree-bindings/chosen.txt
>> @@ -42,6 +42,33 @@ Example
>> };
>> };
>>
>> +u-boot,bootcount-device property
>> +--------------------------------
>> +In a DM-based system, the bootcount may be stored in a device known
>> to +the DM framework (e.g. in a battery-backed SRAM area within a RTC
>> +device). To identify the device to be used for the bootcount, the
>> +u-boot,bootcount-device property needs to point to the target device.
>> +
>> +Further configuration in the target device's node may be required
>> +(e.g. an offset into an I2C RTC's address space), depending on the
>> +UCLASS of the target device.
>> +
>> +Example
>> +-------
>> +/ {
>> + chosen {
>> + u-boot,bootcount-device = &rv3029;
>> + };
>> +
>> + i2c2 {
>> + rv3029: rtc at 56 {
>> + compatible = "mc,rv3029";
>> + reg = <0x56>;
>> + u-boot,bootcount-offset = <0x38>;
>> + };
>> + };
>> +};
>> +
>> u-boot,spl-boot-order property
>> ------------------------------
>>
>> diff --git a/drivers/bootcount/Kconfig b/drivers/bootcount/Kconfig
>> index d335ed1..cdde7b5 100644
>> --- a/drivers/bootcount/Kconfig
>> +++ b/drivers/bootcount/Kconfig
>> @@ -70,6 +70,18 @@ config BOOTCOUNT_AT91
>> bool "Boot counter for Atmel AT91SAM9XE"
>> depends on AT91SAM9XE
>>
>> +config BOOTCOUNT_DM
>> + bool "Boot counter in a device-model device"
>> + help
>> + Enables reading/writing the bootcount in a device-model
>> + device referenced from the /chosen node. The type of the
>> + device is detected from DM and any additional configuration
>> + information (e.g. the offset into a RTC device that
>> supports
>> + read32/write32) is read from the device's node.
>> +
>> + At this time the following DM device classes are supported:
>> + * RTC (using read32/write32)
>> +
>> endchoice
>>
>> config BOOTCOUNT_ALEN
>> diff --git a/drivers/bootcount/Makefile b/drivers/bootcount/Makefile
>> index 68bc006..e8ed729 100644
>> --- a/drivers/bootcount/Makefile
>> +++ b/drivers/bootcount/Makefile
>> @@ -7,3 +7,4 @@ 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_DM) += bootcount_dm.o
>> diff --git a/drivers/bootcount/bootcount_dm.c
>> b/drivers/bootcount/bootcount_dm.c new file mode 100644
>> index 0000000..99bdb88
>> --- /dev/null
>> +++ b/drivers/bootcount/bootcount_dm.c
>> @@ -0,0 +1,93 @@
>> +// SPDX-License-Identifier: GPL-2.0+
>> +/*
>> + * (C) Copyright 2018 Theobroma Systems Design und Consulting GmbH
>> + */
>> +
>> +#include <common.h>
>> +#include <bootcount.h>
>> +#include <dm.h>
>> +#include <rtc.h>
>> +
>> +const u8 bootcount_magic = 0xbc;
>> +
>> +static void bootcount_store_rtc(struct udevice *dev, ulong a)
>> +{
>> +#if CONFIG_IS_ENABLED(DM_RTC)
>> + u32 offset;
>> + const char *offset_propname = "u-boot,bootcount-offset";
>> + const u16 val = bootcount_magic << 8 | (a & 0xff);
>> +
>> + if (dev_read_u32(dev, offset_propname, &offset) < 0) {
>> + debug("%s: requires %s\n", __func__,
>> offset_propname);
>> + return;
>> + }
>> +
>> + if (rtc_write16(dev, offset, val) < 0) {
>> + debug("%s: rtc_write16 failed\n", __func__);
>> + return;
>> + }
>> +#endif
>> +}
>> +
>> +static u32 bootcount_load_rtc(struct udevice *dev)
>> +{
>> +#if CONFIG_IS_ENABLED(DM_RTC)
>> + u32 offset;
>> + const char *offset_propname = "u-boot,bootcount-offset";
>> + u16 val;
>> +
>> + if (dev_read_u32(dev, offset_propname, &offset) < 0) {
>> + debug("%s: requires %s\n", __func__,
>> offset_propname);
>> + goto fail;
>> + }
>> +
>> + if (rtc_read16(dev, offset, &val) < 0) {
>> + debug("%s: rtc_write16 failed\n", __func__);
>> + goto fail;
>> + }
>> +
>> + if (val >> 8 == bootcount_magic)
>> + return val & 0xff;
>> +
>> + debug("%s: bootcount magic does not match on %04x\n",
>> __func__, val);
>> + fail:
>> +#endif
>> + return 0;
>> +}
>> +
>> +/* Now implement the generic default functions */
>> +void bootcount_store(ulong a)
>> +{
>> + struct udevice *dev;
>> + ofnode node;
>> + const char *propname = "u-boot,bootcount-device";
>> +
>> + node = ofnode_get_chosen_node(propname);
>> + if (!ofnode_valid(node)) {
>> + debug("%s: no '%s'\n", __func__, propname);
>> + return;
>> + }
>> +
>> + /* RTC devices */
>> + if (!uclass_get_device_by_ofnode(UCLASS_RTC, node, &dev))
>> + return bootcount_store_rtc(dev, a);
>> +}
>> +
>> +ulong bootcount_load(void)
>> +{
>> + struct udevice *dev;
>> + ofnode node;
>> + const char *propname = "u-boot,bootcount-device";
>> +
>> + node = ofnode_get_chosen_node(propname);
>> + if (!ofnode_valid(node)) {
>> + debug("%s: no '%s'\n", __func__, propname);
>> + return 0;
>> + }
>> +
>> + /* RTC devices */
>> + if (!uclass_get_device_by_ofnode(UCLASS_RTC, node, &dev))
>> + return bootcount_load_rtc(dev);
>> +
>> + return 0;
>> +}
>>
>>
>> Thanks for your patch.
>>
>> However, if I may ask - would it be possible to add support for EEPROM
>> based bootcount in an easy way?
>>
>>
>> This was always intended and is the reason why there’s a "RTC devices”
>> comment in bootcount_load.
>>
>> If someone wants to store their bootcount in an I2C EEPROM, they just
>> need to add a “uclass_get_device_by_ofnode(UCLASS_I2C_EEPROM, …)”
>> with the appropriate logic in bootcount_load and bootcount_store.
>>
>> I mean - do you think that it would be feasible to have
>> bootcount-uclass, which would support generic load/store functions with
>> DM drivers for RTC, EEPROM or even simple write to SFR registers (as it
>> is just an offset in the end)?
>>
>>
>> I thought about this, but didn’t go down that route as a bootcount will
>> usually
>> be stored in an existing DM device (RTC, I2C-EEPROM, NOR flash). If we
>> add individual bootcount-devices wrapping these, then we’d be left with the
>> following in the -u-boot.dtsi:
>>
>> bootcount {
>> compatible = “u-boot,bootcount-rtc”
>> rtc = &rv3029;
>> offset = <0x38>
>> }
>>
>> While this nicely collects all the info together in one node, there’s the
>> drawback
>> of users that might define multiple devices and set their status to “okay”…
>> which
>> might cause inconsistent bootcount values across multiple devices.
>>
>> For this reason, I went with the other design choice of viewing the various
>> bootcount
>> implementations as “bootcount methods” (i.e. logic storing and retrieving a
>> bootcount
>> somewhere). In the case of DM backends this means that the appropriate
>> method
>> is to (a) identify the device by its uclass and (b) call the appropriate
>> read/write method.
>>
>> I briefly toyed with the idea of adding infrastructure to the DM core to get
>> the device
>> for an ofnode (independent of its uclass) and adding a generic
>> dm_read/dm_write
>> that would dispatch to the appropriate uclass’ read/write after looking at
>> the uclass
>> of a udevice passed in. I didn’t go down that route as it seemed to
>> contradict the
>> architecture of DM (i.e. devices are stored in per-uclass lists) in U-Boot.
>>
>> One more thing that influenced the current modelling in the DTS: it prefer
>> to have
>> all info regarding how the SRAM in a RTC—same for the memory in an EEPROM—
>> is subdivided in the RTC node. If this was in a separate node (as the
>> bootcount
>> node above), someone might reuse the same SRAM addresses for different
>> purposes from different nodes causing inadvertent overwriting of live data.
>>
>>
>> I have to agree with Lukasz that this should really be a uclass with a
>> driver for RTC and perhaps one for EEPROM.
>>
>> But we also have patches roaming around for a BOARD uclass, which
>> provides for reading settings from the board, which could of course
>> use any kind of storage. Would that be another option?
>>
>>
>> I’ll change this over to be a new BOOTCOUNT uclass, although my reservation
>> regarding how this partitions a RTC’s or EEPROM’s address space remains.
>> I guess, we’ll have to live with that.
>
> I don't fully understand this but will await the patch for this. I'm
> assuming a DT property would be needed.
A RTC device may have more than the 2 bytes of SRAM, which might be subdivided
into multiple storage regions (e.g. first 2 bytes for bootcount, next 6 bytes for something
else). My concerns are about modelling this as part of the device-tree.
When we have a bootcount-uclass, this will only be visible in the -u-boot.dtsi and it
makes sense to require all info regarding bootcount-placement within the delegate
device to be in the u-boot only node.
Let’s assume the following (pseudo-DTS) example:
bootcount {
compatible = “u-boot,bootcount-rtc”;
rtc = <&my-rtc>;
offset = <0x38>;
}
my-rtc {
compatible = “…”;
vendor,some-other-info-offset-in-sram-offset = <0x38>;
}
My concern/reservation is that such a situation might be detected very late, due to
the offset not being in the rtc’s node… then again, this might not even ever become
an issue and my fear of it might only be informed by my own perception.
Thanks,
Philipp.
>> I don’t think this should be merged with the BOARD uclass, as the bootcount
>> is a standalone feature that could be configured differently even for the
>> same
>> board (i.e. this is not similar enough to reading a board id to merge it
>> into the
>> BOARD uclass).
>>
>
> OK. I suppose the BOARD uclass could make use of a BOOTCOUNT device if needed.
>
> Regards,
> Simon
^ permalink raw reply [flat|nested] 13+ messages in thread
* [U-Boot] [PATCH 3/3] bootcount: add DM-based backing store for bootcount
2018-08-23 13:48 ` Dr. Philipp Tomsich
@ 2018-08-30 0:29 ` Simon Glass
0 siblings, 0 replies; 13+ messages in thread
From: Simon Glass @ 2018-08-30 0:29 UTC (permalink / raw)
To: u-boot
Hi Philipp,
On 23 August 2018 at 07:48, Dr. Philipp Tomsich
<philipp.tomsich@theobroma-systems.com> wrote:
> Simon,
>
>> On 23 Aug 2018, at 12:45, Simon Glass <sjg@chromium.org> wrote:
>>
>> Hi Philipp,
>>
>> On 17 August 2018 at 06:56, Dr. Philipp Tomsich
>> <philipp.tomsich@theobroma-systems.com> wrote:
>>> Simon,
>>>
>>> On 17 Aug 2018, at 14:49, Simon Glass <sjg@chromium.org> wrote:
>>>
>>> Hi Philipp,
>>>
>>> On 14 August 2018 at 05:39, Dr. Philipp Tomsich
>>> <philipp.tomsich@theobroma-systems.com> wrote:
>>>
>>> Lukasz,
>>>
>>> On 14 Aug 2018, at 13:10, Lukasz Majewski <lukma@denx.de> wrote:
>>>
>>> Hi Philipp,
>>>
>>> The original bootcount methods do not provide an interface to DM and
>>> rely on a static configuration for I2C devices (e.g. bus, chip-addr,
>>> etc. are configured through defines statically). On a modern system
>>> that exposes multiple devices in a DTS-configurable way, this is less
>>> than optimal and a interface to DM-based devices will be desirable.
>>>
>>> This adds a simple driver that is DM-aware and configurable via DTS:
>>> the /chosen/u-boot,bootcount-device property is used to detect the DM
>>> device storing the bootcount and deviceclass-specific commands are
>>> used to read/write the bootcount.
>>>
>>> Initially, this provides support for the following DM devices:
>>> * RTC devices implementing the read8/write8 ops
>>>
>>> Signed-off-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>
>>> Tested-by: Klaus Goger <klaus.goger@theobroma-systems.com>
>>> ---
>>>
>>> doc/device-tree-bindings/chosen.txt | 27 +++++++++++
>>> drivers/bootcount/Kconfig | 12 +++++
>>> drivers/bootcount/Makefile | 1 +
>>> drivers/bootcount/bootcount_dm.c | 93
>>> +++++++++++++++++++++++++++++++++++++ 4 files changed, 133
>>> insertions(+) create mode 100644 drivers/bootcount/bootcount_dm.c
>>>
>>> diff --git a/doc/device-tree-bindings/chosen.txt
>>> b/doc/device-tree-bindings/chosen.txt index da7b4e6..734fd15 100644
>>> --- a/doc/device-tree-bindings/chosen.txt
>>> +++ b/doc/device-tree-bindings/chosen.txt
>>> @@ -42,6 +42,33 @@ Example
>>> };
>>> };
>>>
>>> +u-boot,bootcount-device property
>>> +--------------------------------
>>> +In a DM-based system, the bootcount may be stored in a device known
>>> to +the DM framework (e.g. in a battery-backed SRAM area within a RTC
>>> +device). To identify the device to be used for the bootcount, the
>>> +u-boot,bootcount-device property needs to point to the target device.
>>> +
>>> +Further configuration in the target device's node may be required
>>> +(e.g. an offset into an I2C RTC's address space), depending on the
>>> +UCLASS of the target device.
>>> +
>>> +Example
>>> +-------
>>> +/ {
>>> + chosen {
>>> + u-boot,bootcount-device = &rv3029;
>>> + };
>>> +
>>> + i2c2 {
>>> + rv3029: rtc at 56 {
>>> + compatible = "mc,rv3029";
>>> + reg = <0x56>;
>>> + u-boot,bootcount-offset = <0x38>;
>>> + };
>>> + };
>>> +};
>>> +
>>> u-boot,spl-boot-order property
>>> ------------------------------
>>>
>>> diff --git a/drivers/bootcount/Kconfig b/drivers/bootcount/Kconfig
>>> index d335ed1..cdde7b5 100644
>>> --- a/drivers/bootcount/Kconfig
>>> +++ b/drivers/bootcount/Kconfig
>>> @@ -70,6 +70,18 @@ config BOOTCOUNT_AT91
>>> bool "Boot counter for Atmel AT91SAM9XE"
>>> depends on AT91SAM9XE
>>>
>>> +config BOOTCOUNT_DM
>>> + bool "Boot counter in a device-model device"
>>> + help
>>> + Enables reading/writing the bootcount in a device-model
>>> + device referenced from the /chosen node. The type of the
>>> + device is detected from DM and any additional configuration
>>> + information (e.g. the offset into a RTC device that
>>> supports
>>> + read32/write32) is read from the device's node.
>>> +
>>> + At this time the following DM device classes are supported:
>>> + * RTC (using read32/write32)
>>> +
>>> endchoice
>>>
>>> config BOOTCOUNT_ALEN
>>> diff --git a/drivers/bootcount/Makefile b/drivers/bootcount/Makefile
>>> index 68bc006..e8ed729 100644
>>> --- a/drivers/bootcount/Makefile
>>> +++ b/drivers/bootcount/Makefile
>>> @@ -7,3 +7,4 @@ 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_DM) += bootcount_dm.o
>>> diff --git a/drivers/bootcount/bootcount_dm.c
>>> b/drivers/bootcount/bootcount_dm.c new file mode 100644
>>> index 0000000..99bdb88
>>> --- /dev/null
>>> +++ b/drivers/bootcount/bootcount_dm.c
>>> @@ -0,0 +1,93 @@
>>> +// SPDX-License-Identifier: GPL-2.0+
>>> +/*
>>> + * (C) Copyright 2018 Theobroma Systems Design und Consulting GmbH
>>> + */
>>> +
>>> +#include <common.h>
>>> +#include <bootcount.h>
>>> +#include <dm.h>
>>> +#include <rtc.h>
>>> +
>>> +const u8 bootcount_magic = 0xbc;
>>> +
>>> +static void bootcount_store_rtc(struct udevice *dev, ulong a)
>>> +{
>>> +#if CONFIG_IS_ENABLED(DM_RTC)
>>> + u32 offset;
>>> + const char *offset_propname = "u-boot,bootcount-offset";
>>> + const u16 val = bootcount_magic << 8 | (a & 0xff);
>>> +
>>> + if (dev_read_u32(dev, offset_propname, &offset) < 0) {
>>> + debug("%s: requires %s\n", __func__,
>>> offset_propname);
>>> + return;
>>> + }
>>> +
>>> + if (rtc_write16(dev, offset, val) < 0) {
>>> + debug("%s: rtc_write16 failed\n", __func__);
>>> + return;
>>> + }
>>> +#endif
>>> +}
>>> +
>>> +static u32 bootcount_load_rtc(struct udevice *dev)
>>> +{
>>> +#if CONFIG_IS_ENABLED(DM_RTC)
>>> + u32 offset;
>>> + const char *offset_propname = "u-boot,bootcount-offset";
>>> + u16 val;
>>> +
>>> + if (dev_read_u32(dev, offset_propname, &offset) < 0) {
>>> + debug("%s: requires %s\n", __func__,
>>> offset_propname);
>>> + goto fail;
>>> + }
>>> +
>>> + if (rtc_read16(dev, offset, &val) < 0) {
>>> + debug("%s: rtc_write16 failed\n", __func__);
>>> + goto fail;
>>> + }
>>> +
>>> + if (val >> 8 == bootcount_magic)
>>> + return val & 0xff;
>>> +
>>> + debug("%s: bootcount magic does not match on %04x\n",
>>> __func__, val);
>>> + fail:
>>> +#endif
>>> + return 0;
>>> +}
>>> +
>>> +/* Now implement the generic default functions */
>>> +void bootcount_store(ulong a)
>>> +{
>>> + struct udevice *dev;
>>> + ofnode node;
>>> + const char *propname = "u-boot,bootcount-device";
>>> +
>>> + node = ofnode_get_chosen_node(propname);
>>> + if (!ofnode_valid(node)) {
>>> + debug("%s: no '%s'\n", __func__, propname);
>>> + return;
>>> + }
>>> +
>>> + /* RTC devices */
>>> + if (!uclass_get_device_by_ofnode(UCLASS_RTC, node, &dev))
>>> + return bootcount_store_rtc(dev, a);
>>> +}
>>> +
>>> +ulong bootcount_load(void)
>>> +{
>>> + struct udevice *dev;
>>> + ofnode node;
>>> + const char *propname = "u-boot,bootcount-device";
>>> +
>>> + node = ofnode_get_chosen_node(propname);
>>> + if (!ofnode_valid(node)) {
>>> + debug("%s: no '%s'\n", __func__, propname);
>>> + return 0;
>>> + }
>>> +
>>> + /* RTC devices */
>>> + if (!uclass_get_device_by_ofnode(UCLASS_RTC, node, &dev))
>>> + return bootcount_load_rtc(dev);
>>> +
>>> + return 0;
>>> +}
>>>
>>>
>>> Thanks for your patch.
>>>
>>> However, if I may ask - would it be possible to add support for EEPROM
>>> based bootcount in an easy way?
>>>
>>>
>>> This was always intended and is the reason why there’s a "RTC devices”
>>> comment in bootcount_load.
>>>
>>> If someone wants to store their bootcount in an I2C EEPROM, they just
>>> need to add a “uclass_get_device_by_ofnode(UCLASS_I2C_EEPROM, …)”
>>> with the appropriate logic in bootcount_load and bootcount_store.
>>>
>>> I mean - do you think that it would be feasible to have
>>> bootcount-uclass, which would support generic load/store functions with
>>> DM drivers for RTC, EEPROM or even simple write to SFR registers (as it
>>> is just an offset in the end)?
>>>
>>>
>>> I thought about this, but didn’t go down that route as a bootcount will
>>> usually
>>> be stored in an existing DM device (RTC, I2C-EEPROM, NOR flash). If we
>>> add individual bootcount-devices wrapping these, then we’d be left with the
>>> following in the -u-boot.dtsi:
>>>
>>> bootcount {
>>> compatible = “u-boot,bootcount-rtc”
>>> rtc = &rv3029;
>>> offset = <0x38>
>>> }
>>>
>>> While this nicely collects all the info together in one node, there’s the
>>> drawback
>>> of users that might define multiple devices and set their status to “okay”…
>>> which
>>> might cause inconsistent bootcount values across multiple devices.
>>>
>>> For this reason, I went with the other design choice of viewing the various
>>> bootcount
>>> implementations as “bootcount methods” (i.e. logic storing and retrieving a
>>> bootcount
>>> somewhere). In the case of DM backends this means that the appropriate
>>> method
>>> is to (a) identify the device by its uclass and (b) call the appropriate
>>> read/write method.
>>>
>>> I briefly toyed with the idea of adding infrastructure to the DM core to get
>>> the device
>>> for an ofnode (independent of its uclass) and adding a generic
>>> dm_read/dm_write
>>> that would dispatch to the appropriate uclass’ read/write after looking at
>>> the uclass
>>> of a udevice passed in. I didn’t go down that route as it seemed to
>>> contradict the
>>> architecture of DM (i.e. devices are stored in per-uclass lists) in U-Boot.
>>>
>>> One more thing that influenced the current modelling in the DTS: it prefer
>>> to have
>>> all info regarding how the SRAM in a RTC—same for the memory in an EEPROM—
>>> is subdivided in the RTC node. If this was in a separate node (as the
>>> bootcount
>>> node above), someone might reuse the same SRAM addresses for different
>>> purposes from different nodes causing inadvertent overwriting of live data.
>>>
>>>
>>> I have to agree with Lukasz that this should really be a uclass with a
>>> driver for RTC and perhaps one for EEPROM.
>>>
>>> But we also have patches roaming around for a BOARD uclass, which
>>> provides for reading settings from the board, which could of course
>>> use any kind of storage. Would that be another option?
>>>
>>>
>>> I’ll change this over to be a new BOOTCOUNT uclass, although my reservation
>>> regarding how this partitions a RTC’s or EEPROM’s address space remains.
>>> I guess, we’ll have to live with that.
>>
>> I don't fully understand this but will await the patch for this. I'm
>> assuming a DT property would be needed.
>
> A RTC device may have more than the 2 bytes of SRAM, which might be subdivided
> into multiple storage regions (e.g. first 2 bytes for bootcount, next 6 bytes for something
> else). My concerns are about modelling this as part of the device-tree.
>
> When we have a bootcount-uclass, this will only be visible in the -u-boot.dtsi and it
> makes sense to require all info regarding bootcount-placement within the delegate
> device to be in the u-boot only node.
> Let’s assume the following (pseudo-DTS) example:
> bootcount {
> compatible = “u-boot,bootcount-rtc”;
> rtc = <&my-rtc>;
> offset = <0x38>;
> }
>
> my-rtc {
> compatible = “…”;
> vendor,some-other-info-offset-in-sram-offset = <0x38>;
> }
>
> My concern/reservation is that such a situation might be detected very late, due to
> the offset not being in the rtc’s node… then again, this might not even ever become
> an issue and my fear of it might only be informed by my own perception.
Yes that's possible. But I suppose this problem comes up in various
places and we can worry about it later?
Regards,
Simon
^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2018-08-30 0:29 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-08-14 9:50 [U-Boot] [PATCH 0/3] Update RV3029 driver to DM and add DM-backed bootcount support Philipp Tomsich
2018-08-14 9:51 ` [U-Boot] [PATCH 1/3] rtc: rv3029: add to Kconfig Philipp Tomsich
2018-08-14 16:46 ` Heinrich Schuchardt
2018-08-14 16:52 ` Dr. Philipp Tomsich
2018-08-14 9:51 ` [U-Boot] [PATCH 2/3] rtc: rv3029: update to support DM and sync with Linux 4.17 Philipp Tomsich
2018-08-14 9:51 ` [U-Boot] [PATCH 3/3] bootcount: add DM-based backing store for bootcount Philipp Tomsich
2018-08-14 11:10 ` Lukasz Majewski
2018-08-14 11:39 ` Dr. Philipp Tomsich
2018-08-17 12:49 ` Simon Glass
2018-08-17 12:56 ` Dr. Philipp Tomsich
2018-08-23 10:45 ` Simon Glass
2018-08-23 13:48 ` Dr. Philipp Tomsich
2018-08-30 0:29 ` Simon Glass
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.