Hi Philipp, On 23 August 2018 at 07:48, Dr. Philipp Tomsich <philipp.toms...@theobroma-systems.com> wrote: > Simon, > >> On 23 Aug 2018, at 12:45, Simon Glass <s...@chromium.org> wrote: >> >> Hi Philipp, >> >> On 17 August 2018 at 06:56, Dr. Philipp Tomsich >> <philipp.toms...@theobroma-systems.com> wrote: >>> Simon, >>> >>> On 17 Aug 2018, at 14:49, Simon Glass <s...@chromium.org> wrote: >>> >>> 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? >>> >>> >>> 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 _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot