Hi Simon, On 5/3/22 4:50 AM, Simon Glass wrote: > Hi Sean, > > On Fri, 29 Apr 2022 at 13:40, Sean Anderson <[email protected]> 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 <[email protected]> >> >> 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
