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

[Simon Glass <sjg@chromium.org>]

Hi Sean,

On Fri, 29 Apr 2022 at 13:40, Sean Anderson <sean.anderson@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@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@700000 {
> >>>                 #address-cells = <1>;
> >>>                 #size-cells = <1>;
> >>>                 reg = <0x00700000 0x100000>;
> >>>
> >>>                 /* ... */
> >>>
> >>>                 tsens_calibration: calib@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].

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

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

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.

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?

Regards,
Simon

[1] https://lore.kernel.org/all/20220327202622.3438333-1-sjg@chromium.org/