[U-Boot] [PATCH 3/3] bootcount: add DM-based backing store for bootcount
Dr. Philipp Tomsich
philipp.tomsich at theobroma-systems.com
Thu Aug 23 13:48:51 UTC 2018
Simon,
> On 23 Aug 2018, at 12:45, Simon Glass <sjg at chromium.org> wrote:
>
> Hi Philipp,
>
> On 17 August 2018 at 06:56, Dr. Philipp Tomsich
> <philipp.tomsich at theobroma-systems.com> wrote:
>> Simon,
>>
>> On 17 Aug 2018, at 14:49, Simon Glass <sjg at chromium.org> wrote:
>>
>> Hi Philipp,
>>
>> On 14 August 2018 at 05:39, Dr. Philipp Tomsich
>> <philipp.tomsich at theobroma-systems.com> wrote:
>>
>> Lukasz,
>>
>> On 14 Aug 2018, at 13:10, Lukasz Majewski <lukma at 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 at theobroma-systems.com>
>> Tested-by: Klaus Goger <klaus.goger at 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
More information about the U-Boot
mailing list