| Message ID | 20230221145021.31993-1-zajec5@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | nvmem: add explicit config option to read OF fixed cells | expand |
Hello Rafał, On Tue, Feb 21, 2023 at 3:50 PM Rafał Miłecki <zajec5@gmail.com> wrote: > > From: Rafał Miłecki <rafal@milecki.pl> > > NVMEM subsystem looks for fixed NVMEM cells (specified in DT) by > default. This behaviour made sense in early days before adding support > for dynamic cells. > > With every new supported NVMEM device with dynamic cells current > behaviour becomes non-optimal. It results in unneeded iterating over DT > nodes and may result in false discovery of cells (depending on used DT > properties). I am not familiar with the recent changes around dynamic cells. Is there any discussion/summary that I can read to get up to speed? My main thought is: if there are many "fixed OF cells" implementations and only a few "dynamic" ones - does it make sense to flip the logic and introduce a new "use_dynamic_of_cells" flag instead? Best regards, Martin
On 21.02.2023 22:38, Martin Blumenstingl wrote: > On Tue, Feb 21, 2023 at 3:50 PM Rafał Miłecki <zajec5@gmail.com> wrote: >> >> From: Rafał Miłecki <rafal@milecki.pl> >> >> NVMEM subsystem looks for fixed NVMEM cells (specified in DT) by >> default. This behaviour made sense in early days before adding support >> for dynamic cells. >> >> With every new supported NVMEM device with dynamic cells current >> behaviour becomes non-optimal. It results in unneeded iterating over DT >> nodes and may result in false discovery of cells (depending on used DT >> properties). > I am not familiar with the recent changes around dynamic cells. > Is there any discussion/summary that I can read to get up to speed? Some NVMEM devices don't store specific data at hardcoded offsets. For such devices we have drivers (to become: layouts) that parse their magic content. They discover cells and register them and provide matching with proper DT nodes. For bindings see: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=084973e944bec21804f8afb0515b25434438699a https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=a607a850ba1fa966cbb035544c1588e24a6307df For example driver see: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=6e977eaa8280e957b87904b536661550f2a6b3e8 https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=207775f7e17b8fd0426a2ac4a5b81e4e1d71849e For usage see: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=c8442f0fb09ca3d842b9b23d1d0650f649fd10f8 > My main thought is: if there are many "fixed OF cells" implementations > and only a few "dynamic" ones - does it make sense to flip the logic > and introduce a new "use_dynamic_of_cells" flag instead? The problem is that there are more cases than just two. We can have: 1. No cells at all 2. Fixed cells in DT 3. Dynamic cells with references in DT 4. Driver specified cells (specified within config) 5. Cells defined in a global table So we need to reference DT cells explicitly (we can't just confirm / deny *dynamic* cells). Another solution would be to have "no_fixed_of_cells" but: 1. Personally I think negation is less clear / easy to follow 2. There may be actually more drivers with no fixed cells. I think I modified 18 drivers. It seems devm_nvmem_register() is referenced in 44 places. Few of them may be not actual users but it still seems to be about equal.
Hello Rafał, On Tue, Feb 21, 2023 at 11:29 PM Rafał Miłecki <zajec5@gmail.com> wrote: [...] > > I am not familiar with the recent changes around dynamic cells. > > Is there any discussion/summary that I can read to get up to speed? > > Some NVMEM devices don't store specific data at hardcoded offsets. For > such devices we have drivers (to become: layouts) that parse their magic > content. They discover cells and register them and provide matching with > proper DT nodes. > > For bindings see: > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=084973e944bec21804f8afb0515b25434438699a > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=a607a850ba1fa966cbb035544c1588e24a6307df > > For example driver see: > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=6e977eaa8280e957b87904b536661550f2a6b3e8 > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=207775f7e17b8fd0426a2ac4a5b81e4e1d71849e > > For usage see: > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=c8442f0fb09ca3d842b9b23d1d0650f649fd10f8 Thanks, that makes things clearer for me! > > My main thought is: if there are many "fixed OF cells" implementations > > and only a few "dynamic" ones - does it make sense to flip the logic > > and introduce a new "use_dynamic_of_cells" flag instead? > > The problem is that there are more cases than just two. We can have: > 1. No cells at all > 2. Fixed cells in DT > 3. Dynamic cells with references in DT > 4. Driver specified cells (specified within config) > 5. Cells defined in a global table > > So we need to reference DT cells explicitly (we can't just confirm / > deny *dynamic* cells). > > Another solution would be to have "no_fixed_of_cells" but: > > 1. Personally I think negation is less clear / easy to follow > > 2. There may be actually more drivers with no fixed cells. > I think I modified 18 drivers. It seems devm_nvmem_register() is > referenced in 44 places. Few of them may be not actual users but it > still seems to be about equal. Thanks for this overview of all the special cases. Please add my: Acked-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com> for the following two drivers: - drivers/nvmem/meson-efuse.c - drivers/nvmem/meson-mx-efuse.c Best regards, Martin
diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c index 0feacb9fbdac..1bb479c0f758 100644 --- a/drivers/mtd/mtdcore.c +++ b/drivers/mtd/mtdcore.c @@ -523,6 +523,7 @@ static int mtd_nvmem_add(struct mtd_info *mtd) config.dev = &mtd->dev; config.name = dev_name(&mtd->dev); config.owner = THIS_MODULE; + config.use_fixed_of_cells = of_device_is_compatible(node, "nvmem-cells"); config.reg_read = mtd_nvmem_reg_read; config.size = mtd->size; config.word_size = 1; @@ -891,6 +892,7 @@ static struct nvmem_device *mtd_otp_nvmem_register(struct mtd_info *mtd, config.name = kasprintf(GFP_KERNEL, "%s-%s", dev_name(&mtd->dev), compatible); config.id = NVMEM_DEVID_NONE; config.owner = THIS_MODULE; + config.use_fixed_of_cells = true; config.type = NVMEM_TYPE_OTP; config.root_only = true; config.ignore_wp = true; diff --git a/drivers/nvmem/apple-efuses.c b/drivers/nvmem/apple-efuses.c index 9b7c87102104..0119bac43b2c 100644 --- a/drivers/nvmem/apple-efuses.c +++ b/drivers/nvmem/apple-efuses.c @@ -36,6 +36,7 @@ static int apple_efuses_probe(struct platform_device *pdev) struct resource *res; struct nvmem_config config = { .dev = &pdev->dev, + .use_fixed_of_cells = true, .read_only = true, .reg_read = apple_efuses_read, .stride = sizeof(u32), diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c index 174ef3574e07..6783cd8478d7 100644 --- a/drivers/nvmem/core.c +++ b/drivers/nvmem/core.c @@ -844,9 +844,11 @@ struct nvmem_device *nvmem_register(const struct nvmem_config *config) if (rval) goto err_remove_cells; - rval = nvmem_add_cells_from_of(nvmem); - if (rval) - goto err_remove_cells; + if (config->use_fixed_of_cells) { + rval = nvmem_add_cells_from_of(nvmem); + if (rval) + goto err_remove_cells; + } dev_dbg(&nvmem->dev, "Registering nvmem device %s\n", config->name); diff --git a/drivers/nvmem/imx-ocotp-scu.c b/drivers/nvmem/imx-ocotp-scu.c index 399e1eb8b4c1..ec5cce7c6697 100644 --- a/drivers/nvmem/imx-ocotp-scu.c +++ b/drivers/nvmem/imx-ocotp-scu.c @@ -220,6 +220,7 @@ static int imx_scu_ocotp_write(void *context, unsigned int offset, static struct nvmem_config imx_scu_ocotp_nvmem_config = { .name = "imx-scu-ocotp", + .use_fixed_of_cells = true, .read_only = false, .word_size = 4, .stride = 1, diff --git a/drivers/nvmem/imx-ocotp.c b/drivers/nvmem/imx-ocotp.c index e9b52ecb3f72..e37a82f98ba6 100644 --- a/drivers/nvmem/imx-ocotp.c +++ b/drivers/nvmem/imx-ocotp.c @@ -616,6 +616,7 @@ static int imx_ocotp_probe(struct platform_device *pdev) return PTR_ERR(priv->clk); priv->params = of_device_get_match_data(&pdev->dev); + imx_ocotp_nvmem_config.use_fixed_of_cells = true; imx_ocotp_nvmem_config.size = 4 * priv->params->nregs; imx_ocotp_nvmem_config.dev = dev; imx_ocotp_nvmem_config.priv = priv; diff --git a/drivers/nvmem/meson-efuse.c b/drivers/nvmem/meson-efuse.c index d6b533497ce1..657e171d5af3 100644 --- a/drivers/nvmem/meson-efuse.c +++ b/drivers/nvmem/meson-efuse.c @@ -93,6 +93,7 @@ static int meson_efuse_probe(struct platform_device *pdev) econfig->dev = dev; econfig->name = dev_name(dev); + econfig->use_fixed_of_cells = true; econfig->stride = 1; econfig->word_size = 1; econfig->reg_read = meson_efuse_read; diff --git a/drivers/nvmem/meson-mx-efuse.c b/drivers/nvmem/meson-mx-efuse.c index 13eb14316f46..7cc51391ec9c 100644 --- a/drivers/nvmem/meson-mx-efuse.c +++ b/drivers/nvmem/meson-mx-efuse.c @@ -213,6 +213,7 @@ static int meson_mx_efuse_probe(struct platform_device *pdev) efuse->config.owner = THIS_MODULE; efuse->config.dev = &pdev->dev; efuse->config.priv = efuse; + efuse->config.use_fixed_of_cells = true; efuse->config.stride = drvdata->word_size; efuse->config.word_size = drvdata->word_size; efuse->config.size = SZ_512; diff --git a/drivers/nvmem/microchip-otpc.c b/drivers/nvmem/microchip-otpc.c index 436e0dc4f337..fb920fd7a8fc 100644 --- a/drivers/nvmem/microchip-otpc.c +++ b/drivers/nvmem/microchip-otpc.c @@ -261,6 +261,7 @@ static int mchp_otpc_probe(struct platform_device *pdev) return ret; mchp_nvmem_config.dev = otpc->dev; + mchp_nvmem_config.use_fixed_of_cells = true; mchp_nvmem_config.size = size; mchp_nvmem_config.priv = otpc; nvmem = devm_nvmem_register(&pdev->dev, &mchp_nvmem_config); diff --git a/drivers/nvmem/mtk-efuse.c b/drivers/nvmem/mtk-efuse.c index a08e0aedd21c..1947337a121f 100644 --- a/drivers/nvmem/mtk-efuse.c +++ b/drivers/nvmem/mtk-efuse.c @@ -45,6 +45,7 @@ static int mtk_efuse_probe(struct platform_device *pdev) if (IS_ERR(priv->base)) return PTR_ERR(priv->base); + econfig.use_fixed_of_cells = true; econfig.stride = 1; econfig.word_size = 1; econfig.reg_read = mtk_reg_read; diff --git a/drivers/nvmem/qcom-spmi-sdam.c b/drivers/nvmem/qcom-spmi-sdam.c index f822790db49e..b547def94b5b 100644 --- a/drivers/nvmem/qcom-spmi-sdam.c +++ b/drivers/nvmem/qcom-spmi-sdam.c @@ -142,6 +142,7 @@ static int sdam_probe(struct platform_device *pdev) sdam->sdam_config.name = "spmi_sdam"; sdam->sdam_config.id = NVMEM_DEVID_AUTO; sdam->sdam_config.owner = THIS_MODULE; + sdam->sdam_config.use_fixed_of_cells = true; sdam->sdam_config.stride = 1; sdam->sdam_config.word_size = 1; sdam->sdam_config.reg_read = sdam_read; diff --git a/drivers/nvmem/qfprom.c b/drivers/nvmem/qfprom.c index c1e893c8a247..eb126d507561 100644 --- a/drivers/nvmem/qfprom.c +++ b/drivers/nvmem/qfprom.c @@ -357,6 +357,7 @@ static int qfprom_probe(struct platform_device *pdev) { struct nvmem_config econfig = { .name = "qfprom", + .use_fixed_of_cells = true, .stride = 1, .word_size = 1, .id = NVMEM_DEVID_AUTO, diff --git a/drivers/nvmem/rave-sp-eeprom.c b/drivers/nvmem/rave-sp-eeprom.c index c456011b75e8..e9b4c7927e37 100644 --- a/drivers/nvmem/rave-sp-eeprom.c +++ b/drivers/nvmem/rave-sp-eeprom.c @@ -328,6 +328,7 @@ static int rave_sp_eeprom_probe(struct platform_device *pdev) of_property_read_string(np, "zii,eeprom-name", &config.name); config.priv = eeprom; config.dev = dev; + config.use_fixed_of_cells = true; config.size = size; config.reg_read = rave_sp_eeprom_reg_read; config.reg_write = rave_sp_eeprom_reg_write; diff --git a/drivers/nvmem/rockchip-efuse.c b/drivers/nvmem/rockchip-efuse.c index e4579de5d014..211f6e7401a9 100644 --- a/drivers/nvmem/rockchip-efuse.c +++ b/drivers/nvmem/rockchip-efuse.c @@ -205,6 +205,7 @@ static int rockchip_rk3399_efuse_read(void *context, unsigned int offset, static struct nvmem_config econfig = { .name = "rockchip-efuse", + .use_fixed_of_cells = true, .stride = 1, .word_size = 1, .read_only = true, diff --git a/drivers/nvmem/sc27xx-efuse.c b/drivers/nvmem/sc27xx-efuse.c index c825fc902d10..ed5cc4f3e2bc 100644 --- a/drivers/nvmem/sc27xx-efuse.c +++ b/drivers/nvmem/sc27xx-efuse.c @@ -248,6 +248,7 @@ static int sc27xx_efuse_probe(struct platform_device *pdev) econfig.reg_read = sc27xx_efuse_read; econfig.priv = efuse; econfig.dev = &pdev->dev; + econfig.use_fixed_of_cells = true; nvmem = devm_nvmem_register(&pdev->dev, &econfig); if (IS_ERR(nvmem)) { dev_err(&pdev->dev, "failed to register nvmem config\n"); diff --git a/drivers/nvmem/sprd-efuse.c b/drivers/nvmem/sprd-efuse.c index 4f1fcbfec394..ef3161645f60 100644 --- a/drivers/nvmem/sprd-efuse.c +++ b/drivers/nvmem/sprd-efuse.c @@ -408,6 +408,7 @@ static int sprd_efuse_probe(struct platform_device *pdev) econfig.read_only = false; econfig.name = "sprd-efuse"; econfig.size = efuse->data->blk_nums * SPRD_EFUSE_BLOCK_WIDTH; + econfig.use_fixed_of_cells = true; econfig.reg_read = sprd_efuse_read; econfig.reg_write = sprd_efuse_write; econfig.priv = efuse; diff --git a/drivers/nvmem/stm32-romem.c b/drivers/nvmem/stm32-romem.c index ba779e26937a..3d3e557cb88f 100644 --- a/drivers/nvmem/stm32-romem.c +++ b/drivers/nvmem/stm32-romem.c @@ -208,6 +208,7 @@ static int stm32_romem_probe(struct platform_device *pdev) priv->cfg.priv = priv; priv->cfg.owner = THIS_MODULE; priv->cfg.type = NVMEM_TYPE_OTP; + priv->cfe.use_fixed_of_cells = true; priv->lower = 0; diff --git a/drivers/nvmem/sunplus-ocotp.c b/drivers/nvmem/sunplus-ocotp.c index 52b928a7a6d5..57e3e0833b85 100644 --- a/drivers/nvmem/sunplus-ocotp.c +++ b/drivers/nvmem/sunplus-ocotp.c @@ -145,6 +145,7 @@ static int sp_ocotp_read(void *priv, unsigned int offset, void *value, size_t by static struct nvmem_config sp_ocotp_nvmem_config = { .name = "sp-ocotp", + .use_fixed_of_cells = true, .read_only = true, .word_size = 1, .size = QAC628_OTP_SIZE, diff --git a/drivers/nvmem/sunxi_sid.c b/drivers/nvmem/sunxi_sid.c index a970f1741cc6..6ab7aa3724a0 100644 --- a/drivers/nvmem/sunxi_sid.c +++ b/drivers/nvmem/sunxi_sid.c @@ -156,6 +156,7 @@ static int sunxi_sid_probe(struct platform_device *pdev) nvmem_cfg->dev = dev; nvmem_cfg->name = "sunxi-sid"; nvmem_cfg->type = NVMEM_TYPE_OTP; + nvmem_cfg->use_fixed_of_cells = true; nvmem_cfg->read_only = true; nvmem_cfg->size = cfg->size; nvmem_cfg->word_size = 1; diff --git a/drivers/nvmem/uniphier-efuse.c b/drivers/nvmem/uniphier-efuse.c index aca910b3b6f8..69b9dfe6ab6e 100644 --- a/drivers/nvmem/uniphier-efuse.c +++ b/drivers/nvmem/uniphier-efuse.c @@ -53,6 +53,7 @@ static int uniphier_efuse_probe(struct platform_device *pdev) econfig.size = resource_size(res); econfig.priv = priv; econfig.dev = dev; + econfig.use_fixed_of_cells = true; nvmem = devm_nvmem_register(dev, &econfig); return PTR_ERR_OR_ZERO(nvmem); diff --git a/drivers/nvmem/zynqmp_nvmem.c b/drivers/nvmem/zynqmp_nvmem.c index e28d7b133e11..d13750a4c112 100644 --- a/drivers/nvmem/zynqmp_nvmem.c +++ b/drivers/nvmem/zynqmp_nvmem.c @@ -58,6 +58,7 @@ static int zynqmp_nvmem_probe(struct platform_device *pdev) priv->dev = dev; econfig.dev = dev; + econfig.use_fixed_of_cells = true; econfig.reg_read = zynqmp_nvmem_read; econfig.priv = priv; diff --git a/drivers/rtc/nvmem.c b/drivers/rtc/nvmem.c index 07ede21cee34..5cc039d92257 100644 --- a/drivers/rtc/nvmem.c +++ b/drivers/rtc/nvmem.c @@ -21,6 +21,7 @@ int devm_rtc_nvmem_register(struct rtc_device *rtc, nvmem_config->dev = dev; nvmem_config->owner = rtc->owner; + nvmem_config->use_fixed_of_cells = true; nvmem = devm_nvmem_register(dev, nvmem_config); if (IS_ERR(nvmem)) dev_err(dev, "failed to register nvmem device for RTC\n"); diff --git a/drivers/w1/slaves/w1_ds250x.c b/drivers/w1/slaves/w1_ds250x.c index 7592c7050d1d..538722b41f87 100644 --- a/drivers/w1/slaves/w1_ds250x.c +++ b/drivers/w1/slaves/w1_ds250x.c @@ -168,6 +168,7 @@ static int w1_eprom_add_slave(struct w1_slave *sl) struct nvmem_device *nvmem; struct nvmem_config nvmem_cfg = { .dev = &sl->dev, + .use_fixed_of_cells = true, .reg_read = w1_nvmem_read, .type = NVMEM_TYPE_OTP, .read_only = true, diff --git a/include/linux/nvmem-provider.h b/include/linux/nvmem-provider.h index 0262b86194eb..b3c14ce87a65 100644 --- a/include/linux/nvmem-provider.h +++ b/include/linux/nvmem-provider.h @@ -73,6 +73,7 @@ struct nvmem_cell_info { * @owner: Pointer to exporter module. Used for refcounting. * @cells: Optional array of pre-defined NVMEM cells. * @ncells: Number of elements in cells. + * @use_fixed_of_cells: Read fixed NVMEM cells from OF. * @keepout: Optional array of keepout ranges (sorted ascending by start). * @nkeepout: Number of elements in the keepout array. * @type: Type of the nvmem storage @@ -103,6 +104,7 @@ struct nvmem_config { struct module *owner; const struct nvmem_cell_info *cells; int ncells; + bool use_fixed_of_cells; const struct nvmem_keepout *keepout; unsigned int nkeepout; enum nvmem_type type;