[PATCH v3 08/13] misc: Add support for nvmem cells

Sean Anderson sean.anderson at seco.com
Thu May 5 17:26:51 CEST 2022


Hi Simon,

On 5/3/22 4:50 AM, Simon Glass wrote:
> Hi Sean,
> 
> On Fri, 29 Apr 2022 at 13:40, Sean Anderson <sean.anderson at seco.com> wrote:
>>
>> Hi Simon,
>>
>> On 4/25/22 11:24 AM, Sean Anderson wrote:
>> >
>> >
>> > On 4/25/22 1:48 AM, Simon Glass wrote:
>> >> Hi Sean,
>> >>
>> >> On Mon, 18 Apr 2022 at 13:37, Sean Anderson <sean.anderson at seco.com> wrote:
>> >>>
>> >>> This adds support for "nvmem cells" as seen in Linux. The nvmem device
>> >>> class in Linux is used for various assorted ROMs and EEPROMs. In this
>> >>> sense, it is similar to UCLASS_MISC, but also includes
>> >>> UCLASS_I2C_EEPROM, UCLASS_RTC, and UCLASS_MTD. New drivers corresponding
>> >>> to a Linux-style nvmem device should be implemented as one of the
>> >>> previously-mentioned uclasses. The nvmem API acts as a compatibility
>> >>> layer to adapt the (slightly different) APIs of these uclasses. It also
>> >>> handles the lookup of nvmem cells.
>> >>>
>> >>> While nvmem devices can be accessed directly, they are most often used
>> >>> by reading/writing contiguous values called "cells". Cells typically
>> >>> hold information like calibration, versions, or configuration (such as
>> >>> mac addresses).
>> >>>
>> >>> nvmem devices can specify "cells" in their device tree:
>> >>>
>> >>>         qfprom: eeprom at 700000 {
>> >>>                 #address-cells = <1>;
>> >>>                 #size-cells = <1>;
>> >>>                 reg = <0x00700000 0x100000>;
>> >>>
>> >>>                 /* ... */
>> >>>
>> >>>                 tsens_calibration: calib at 404 {
>> >>>                         reg = <0x404 0x10>;
>> >>>                 };
>> >>>         };
>> >>>
>> >>> which can then be referenced like:
>> >>>
>> >>>         tsens {
>> >>>                 /* ... */
>> >>>                 nvmem-cells = <&tsens_calibration>;
>> >>>                 nvmem-cell-names = "calibration";
>> >>>         };
>> >>>
>> >>> The tsens driver could then read the calibration value like:
>> >>>
>> >>>         struct nvmem_cell cal_cell;
>> >>>         u8 cal[16];
>> >>>         nvmem_cell_get_by_name(dev, "calibration", &cal_cell);
>> >>>         nvmem_cell_read(&cal_cell, cal, sizeof(cal));
>> >>>
>> >>> Because nvmem devices are not all of the same uclass, supported uclasses
>> >>> must register a nvmem_interface struct. This allows CONFIG_NVMEM to be
>> >>> enabled without depending on specific uclasses. At the moment,
>> >>> nvmem_interface is very bare-bones, and assumes that no initialization
>> >>> is necessary. However, this could be amended in the future.
>> >>>
>> >>> Although I2C_EEPROM and MISC are quite similar (and could likely be
>> >>> unified), they present different read/write function signatures. To
>> >>> abstract over this, NVMEM uses the same read/write signature as Linux.
>> >>> In particular, short read/writes are not allowed, which is allowed by
>> >>> MISC.
>> >>>
>> >>> The functionality implemented by nvmem cells is very similar to that
>> >>> provided by i2c_eeprom_partition. "fixed-partition"s for eeproms does
>> >>> not seem to have made its way into Linux or into any device tree other
>> >>> than sandbox. It is possible that with the introduction of this API it
>> >>> would be possible to remove it.
>> >>
>> >> I still think this would be better as a separate uclass, with child
>> >> devices created at bind time in each of the respective uclasses, like
>> >> mmc_bind() does. Then you will see the nvmem devices in the DM tree.
>> >> Wouldn't we want to add a command to access the nvmem devices?
>> >
>> > We already do. E.g. the misc/rtc/eeprom commands. The problem is that
>> > for software to access them, they would have to use misc_read/dm_rtc_read/
>> > i2c_eeprom_read.
>> >
>> >> This patch feels like a shortcut to me and I'm not sure of the
>> >> benefit of that shortcut.
>> > Well, I suppose it's because "nvmem" devices are strict subsets of
>> > existing devices. There is no new functionality here (except adapting
>> > between semantics like for misc). We should always be able to use the
>> > existing API to implement support for a new underlying uclass. There
>> > should never be device-specific read/write methods, because we can
>> > use the existing read/write uclass methods.
>> >
>> > What I'm trying to get at is that we sort of already have an nvmem
>> > uclass with nvmem devices, they're just not accessible in a uniform
>> > way. This series is trying to address the uniformity aspect. But I
>> > don't think we need new devices for each nvmem interface, because
>> > all they would do would take up ram/rom.
>> >
>> > --Sean
>> >
>> > PS. In an ideal world we'd have something like
>> >
>> > struct nvmem_ops {
>> >       read();
>> >       write();
>> > };
>> >
>> > struct dm_rtc_ops {
>> >       nvmem_ops nvmem;
>> >       /* the other ops minus read/write */
>> > };
>> >
>> > int nvmem_read (...) {
>> >       struct nvmem_ops *ops = cell->nvmem->ops;
>> >       /* ... */
>> >
>> >       return ops->read(...);
>> > }
>> >
>> > but unfortunately, we already have fragmented implementations.
> 
> I don't see that as ideal as it involves a 'nested' API, something we
> have avoided so far.
> 
>> >
>>
>> To follow up on this, I've conducted some size experiments. The
>> following is the bloat caused by applying the current series on
>> sandbox64_defconfig:
>>
>> add/remove: 8/0 grow/shrink: 7/2 up/down: 1069/-170 (899)
>> Function                                     old     new   delta
>> nvmem_cell_get_by_index                        -     216    +216
>> dm_test_ethaddr                                -     192    +192
>> nvmem_cell_write                               -     125    +125
>> nvmem_cell_read                                -     125    +125
>> nvmem_cell_get_by_name                         -      65     +65
>> addr                                           -      64     +64
>> sandbox_i2c_rtc_probe                          -      54     +54
>> sb_eth_write_hwaddr                           14      57     +43
>> sandbox_i2c_eeprom_probe                      70     112     +42
>> misc_sandbox_probe                            21      61     +40
>> eth_post_probe                               444     484     +40
>> _u_boot_list_2_ut_dm_test_2_dm_test_ethaddr       -      32     +32
>> __func__                                   15147   15163     +16
>> data_gz                                    18327   18338     +11
>> dsa_pre_probe                                181     185      +4
>> sb_eth_of_to_plat                            126      64     -62
>> default_environment                          553     445    -108
>> Total: Before=1765267, After=1766166, chg +0.05%
>>
>> And here is the difference (from baseline) when using your
>> suggested approach:
>>
>> add/remove: 26/0 grow/shrink: 8/2 up/down: 2030/-170 (1860)
>> Function                                     old     new   delta
>> dm_test_ethaddr                                -     192    +192
>> nvmem_cell_get_by_index                        -     152    +152
>> nvmem_register                                 -     137    +137
>> _u_boot_list_2_driver_2_rtc_nvmem              -     128    +128
>> _u_boot_list_2_driver_2_misc_nvmem             -     128    +128
>> _u_boot_list_2_driver_2_i2c_eeprom_nvmem       -     128    +128
>> _u_boot_list_2_uclass_driver_2_nvmem           -     120    +120
>> misc_nvmem_write                               -      68     +68
>> misc_nvmem_read                                -      68     +68
>> nvmem_cell_write                               -      66     +66
>> nvmem_cell_read                                -      65     +65
>> nvmem_cell_get_by_name                         -      65     +65
>> addr                                           -      64     +64
>> sandbox_i2c_rtc_probe                          -      54     +54
>> rtc_post_bind                                  -      48     +48
>> nvmem_rtc_write                                -      48     +48
>> nvmem_rtc_read                                 -      48     +48
>> misc_post_bind                                 -      48     +48
>> i2c_eeprom_nvmem_write                         -      48     +48
>> i2c_eeprom_nvmem_read                          -      48     +48
>> sb_eth_write_hwaddr                           14      57     +43
>> sandbox_i2c_eeprom_probe                      70     112     +42
>> misc_sandbox_probe                            21      61     +40
>> eth_post_probe                               444     484     +40
>> _u_boot_list_2_ut_dm_test_2_dm_test_ethaddr       -      32     +32
>> rtc_nvmem_ops                                  -      16     +16
>> misc_nvmem_ops                                 -      16     +16
>> i2c_eeprom_post_bind                           -      16     +16
>> i2c_eeprom_nvmem_ops                           -      16     +16
>> __func__                                   15147   15163     +16
>> data_gz                                    18327   18338     +11
>> fmt                                            -       9      +9
>> version_string                                68      74      +6
>> dsa_pre_probe                                181     185      +4
>> sb_eth_of_to_plat                            126      64     -62
>> default_environment                          553     445    -108
>> Total: Before=1765267, After=1767127, chg +0.11%
>>
>> As you can see, adding a second driver for each nvmem device
>> doubles the size of this feature. The patch I used for this follows
>> (it does not apply cleanly to v3 because the base contains some
>> changes fixing bugs pointed out by Tom).
> 
> Thanks for the analysis and patch. This is what buildman reports for me:
> 
> 01: test: Load mac address using misc device
>    sandbox:  w+   sandbox
> 02: misc: nvmem: Convert to using udevices
>    sandbox: (for 1/1 boards) all +1584.0 data +576.0 rodata +32.0 text +976.0
> [..]
> 
> So we have text growth of about 1KB on 64-bit x86. The data size is
> not that important IMO since this is most likely to be used in U-Boot
> proper which doesn't have tight constraints. For that we should
> instead focus on reducing the cost of driver model overall, e.g. with
> the ideas mentioned at [1].

Yes, I agree. One of the big problems is that each driver struct takes
128 bytes each. With 3 drivers added (and a uclass driver), that's 512
bytes right from the start. I don't think this is covered by your series.
The current design is very ergonomic, but I don't know if it's the best
space-wise.

Another problem is that each function has to move registers around to set
up the parameters for its two calls:

00000000000a56c4 <i2c_eeprom_nvmem_write>:
   a56c4:	f3 0f 1e fa          	endbr64 
   a56c8:	53                   	push   %rbx
   a56c9:	48 89 cb             	mov    %rcx,%rbx
   a56cc:	48 83 ec 10          	sub    $0x10,%rsp
   a56d0:	89 74 24 0c          	mov    %esi,0xc(%rsp)
   a56d4:	48 89 14 24          	mov    %rdx,(%rsp)
   a56d8:	e8 49 00 fd ff       	callq  75726 <dev_get_parent>
   a56dd:	48 8b 14 24          	mov    (%rsp),%rdx
   a56e1:	8b 74 24 0c          	mov    0xc(%rsp),%esi
   a56e5:	89 d9                	mov    %ebx,%ecx
   a56e7:	48 83 c4 10          	add    $0x10,%rsp
   a56eb:	48 89 c7             	mov    %rax,%rdi
   a56ee:	5b                   	pop    %rbx
   a56ef:	e9 4c ff ff ff       	jmpq   a5640 <i2c_eeprom_write>


I suppose we could shave off ~120 bytes by moving the dev_get_parent
calls to nvmem_cell_read.

> I didn't try on Thumb2 but I suppose it would be about 0.5KB.

add/remove: 20/0 grow/shrink: 0/3 up/down: 731/-100 (631)
Function                                     old     new   delta
dm_rtc_write                                   -      78     +78
nvmem_register                                 -      72     +72
_u_boot_list_2_uclass_driver_2_nvmem           -      72     +72
_u_boot_list_2_driver_2_rtc_nvmem              -      68     +68
_u_boot_list_2_driver_2_misc_nvmem             -      68     +68
_u_boot_list_2_driver_2_i2c_eeprom_nvmem       -      68     +68
misc_nvmem_write                               -      38     +38
misc_nvmem_read                                -      38     +38
rtc_post_bind                                  -      28     +28
misc_post_bind                                 -      28     +28
nvmem_rtc_write                                -      26     +26
nvmem_rtc_read                                 -      26     +26
i2c_eeprom_nvmem_write                         -      26     +26
i2c_eeprom_nvmem_read                          -      26     +26
misc_write                                     -      24     +24
i2c_eeprom_post_bind                           -      12     +12
fmt                                            -       9      +9
rtc_nvmem_ops                                  -       8      +8
misc_nvmem_ops                                 -       8      +8
i2c_eeprom_nvmem_ops                           -       8      +8
version_string                                74      68      -6
nvmem_cell_read                               96      50     -46
nvmem_cell_get_by_index                      144      96     -48
Total: Before=362129, After=362760, chg +0.17%

> It seems OK to pay this cost to keep things clean.

It's not terribly large, but I would like to try and keep the size of
new features down. I'd like to make it easy to choose to use NVMEM
instead of e.g. mac_read_from_eeprom

> If we do go ahead with this series (i.e. without the last patch), I
> don't think it should be a model for how to add new APIs in future.
> E.g. we could save space by making a special case for PMICs which
> support GPIOs, so that GPIO operations could accept a PMIC device or a
> GPIO device, but it would be quite confusing, We could make the
> random-number device disappear and just 'know' that a TPM and a MISC
> device can produce random numbers, but I have the same feeling about
> that.

I agree. I think this is a bit of a special case because of how we
already have several APIs all implementing the same idea.

> Given that you have done the analysis and you are still pretty keen on
> this, I am OK with it going in. I don't like it very much. but we can
> always review things later. Perhaps add a comment in the nvmem header
> files about how it works and why it doesn't use driver model in the
> normal way?

Sure. I will also include the patch from before as an RFC on the end of the series.

--Sean


More information about the U-Boot mailing list