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