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@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].
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