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 > <mailto: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 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. _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
