Hi Philipp, On 14 August 2018 at 05:39, Dr. Philipp Tomsich <philipp.toms...@theobroma-systems.com> wrote: > Lukasz, > >> On 14 Aug 2018, at 13:10, Lukasz Majewski <lu...@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.toms...@theobroma-systems.com> >>> Tested-by: Klaus Goger <klaus.go...@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@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 _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot