[U-Boot] [PATCH 3/3] bootcount: add DM-based backing store for bootcount
Simon Glass
sjg at chromium.org
Thu Aug 30 00:29:20 UTC 2018
Hi Philipp,
On 23 August 2018 at 07:48, Dr. Philipp Tomsich
<philipp.tomsich at theobroma-systems.com> wrote:
> 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.
Yes that's possible. But I suppose this problem comes up in various
places and we can worry about it later?
Regards,
Simon
More information about the U-Boot
mailing list