From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from phobos.denx.de (phobos.denx.de [85.214.62.61]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 5F99AC433EF for ; Tue, 3 May 2022 08:51:15 +0000 (UTC) Received: from h2850616.stratoserver.net (localhost [IPv6:::1]) by phobos.denx.de (Postfix) with ESMTP id 864E683D07; Tue, 3 May 2022 10:51:12 +0200 (CEST) Authentication-Results: phobos.denx.de; dmarc=pass (p=none dis=none) header.from=chromium.org Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=u-boot-bounces@lists.denx.de Authentication-Results: phobos.denx.de; dkim=pass (1024-bit key; unprotected) header.d=chromium.org header.i=@chromium.org header.b="flWzdvDf"; dkim-atps=neutral Received: by phobos.denx.de (Postfix, from userid 109) id 7054083D7E; Tue, 3 May 2022 10:51:10 +0200 (CEST) Received: from mail-vs1-xe2b.google.com (mail-vs1-xe2b.google.com [IPv6:2607:f8b0:4864:20::e2b]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits)) (No client certificate requested) by phobos.denx.de (Postfix) with ESMTPS id 10A2C83040 for ; Tue, 3 May 2022 10:51:05 +0200 (CEST) Authentication-Results: phobos.denx.de; dmarc=pass (p=none dis=none) header.from=chromium.org Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=sjg@google.com Received: by mail-vs1-xe2b.google.com with SMTP id a127so15795328vsa.3 for ; Tue, 03 May 2022 01:51:04 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org; s=google; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=c8IEyQGehlQ1RCWeGJNj3BCRQBlZNb5HEn1KvLe9EYI=; b=flWzdvDfZSmjyABybIKOTXYtCxZiPmcHkEt6JurRmLcktUIcD5YmQ7RxAU7FWrlLUm vdponSghcRb6+t+USI8eiWtZZ46G8zG/JeyYRVvwuiC0L/mTCYAPOZRiZm4JhssV/cp0 JIetHSimZAjArKIGBpX9zVznJjgjpua2+1XuE= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=c8IEyQGehlQ1RCWeGJNj3BCRQBlZNb5HEn1KvLe9EYI=; b=b+ErnKlTBSOPnaGa/RP6rJaG9laNfIQ4o5UEZ+YSYZtlZKPGXmtkJQ+otUG5Iza+ON Up+JFU7zGbNS1BlVikkROpsyHcSHoqv1FU/14d/AXPMzViOKQZtqdYZ3FNqYI9HdXc0T 2Fsxx9q99QGqbW5e6CvaIrAX3eqPoAC4MLF8V+tlUI3FA8Kv/G/Cp1UgUt72/c7eMKzp +fH+akCnY1K2QQfG4cQn+CDDyL2j4GkjnQDBRctOIByEuZRVFHyWZ0S2VNBrekdfCcSK mb6xYYdBO1KjtV8rKeRw0XYfybe3lCk9Df7CW7quenWf1ld/4hhbBWiucJ6ib6uFTuax NH/A== X-Gm-Message-State: AOAM531uQ1JMWlTWZfLi6/X+PqbgYgR7azR/7pEK9w1K3TVPfbTbzdu6 N2r+jaxhOgMyVYCqbA5chkq0dC/54+Cjclud6YzFfw== X-Google-Smtp-Source: ABdhPJz+pJPSVTcksBmuUNb82q/g9Geet3RhtmPWJe6BanuyWPdKovi65/xRTBojPepwHlEYKN4yrAq1d6KbF5aYTEY= X-Received: by 2002:a67:dd96:0:b0:32d:33db:cefc with SMTP id i22-20020a67dd96000000b0032d33dbcefcmr3351540vsk.61.1651567863434; Tue, 03 May 2022 01:51:03 -0700 (PDT) MIME-Version: 1.0 References: <20220418193659.3677824-1-sean.anderson@seco.com> <20220418193659.3677824-9-sean.anderson@seco.com> <32a295f1-f2c8-080e-b3f6-30f0dee6e25d@seco.com> <86ab39e4-f463-418e-a337-3173d019f624@seco.com> In-Reply-To: <86ab39e4-f463-418e-a337-3173d019f624@seco.com> From: Simon Glass Date: Tue, 3 May 2022 02:50:52 -0600 Message-ID: Subject: Re: [PATCH v3 08/13] misc: Add support for nvmem cells To: Sean Anderson Cc: U-Boot Mailing List , Mario Six , Ramon Fried , Heinrich Schuchardt , Tom Rini , Joe Hershberger Content-Type: text/plain; charset="UTF-8" X-BeenThere: u-boot@lists.denx.de X-Mailman-Version: 2.1.39 Precedence: list List-Id: U-Boot discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: u-boot-bounces@lists.denx.de Sender: "U-Boot" X-Virus-Scanned: clamav-milter 0.103.5 at phobos.denx.de X-Virus-Status: Clean Hi Sean, On Fri, 29 Apr 2022 at 13:40, Sean Anderson 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 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/