[U-Boot] [PATCH 3/3] bootcount: add DM-based backing store for bootcount
Dr. Philipp Tomsich
philipp.tomsich at theobroma-systems.com
Fri Aug 17 12:56:49 UTC 2018
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 <mailto: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 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.
More information about the U-Boot
mailing list