[V2] nvmem: add explicit config option to read OF fixed cells

  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).

This behaviour has actually caused a problem already with the MTD
subsystem. MTD subpartitions were incorrectly treated as NVMEM cells.

Also with upcoming support for NVMEM layouts no new binding or driver
should support fixed cells defined in device node.

Solve this by modifying drivers for bindings that support specifying
fixed NVMEM cells in DT. Make them explicitly tell NVMEM subsystem to
read cells from DT.

It wasn't clear (to me) if rtc and w1 code actually uses fixed cells. I
enabled them to don't risk any breakage.

Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
[for drivers/nvmem/meson-{efuse,mx-efuse}.c]
Acked-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
---
V2: Fix stm32-romem.c typo breaking its compilation
    Pick Martin's Acked-by
    Add paragraph about layouts deprecating use_fixed_of_cells
---
 drivers/mtd/mtdcore.c          | 2 ++
 drivers/nvmem/apple-efuses.c   | 1 +
 drivers/nvmem/core.c           | 8 +++++---
 drivers/nvmem/imx-ocotp-scu.c  | 1 +
 drivers/nvmem/imx-ocotp.c      | 1 +
 drivers/nvmem/meson-efuse.c    | 1 +
 drivers/nvmem/meson-mx-efuse.c | 1 +
 drivers/nvmem/microchip-otpc.c | 1 +
 drivers/nvmem/mtk-efuse.c      | 1 +
 drivers/nvmem/qcom-spmi-sdam.c | 1 +
 drivers/nvmem/qfprom.c         | 1 +
 drivers/nvmem/rave-sp-eeprom.c | 1 +
 drivers/nvmem/rockchip-efuse.c | 1 +
 drivers/nvmem/sc27xx-efuse.c   | 1 +
 drivers/nvmem/sprd-efuse.c     | 1 +
 drivers/nvmem/stm32-romem.c    | 1 +
 drivers/nvmem/sunplus-ocotp.c  | 1 +
 drivers/nvmem/sunxi_sid.c      | 1 +
 drivers/nvmem/uniphier-efuse.c | 1 +
 drivers/nvmem/zynqmp_nvmem.c   | 1 +
 drivers/rtc/nvmem.c            | 1 +
 drivers/w1/slaves/w1_ds250x.c  | 1 +
 include/linux/nvmem-provider.h | 2 ++
 23 files changed, 29 insertions(+), 3 deletions(-)
  

Il 24/02/23 08:29, Rafał Miłecki ha scritto:
> 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).
> 
> This behaviour has actually caused a problem already with the MTD
> subsystem. MTD subpartitions were incorrectly treated as NVMEM cells.
> 
> Also with upcoming support for NVMEM layouts no new binding or driver
> should support fixed cells defined in device node.
> 
> Solve this by modifying drivers for bindings that support specifying
> fixed NVMEM cells in DT. Make them explicitly tell NVMEM subsystem to
> read cells from DT.
> 
> It wasn't clear (to me) if rtc and w1 code actually uses fixed cells. I
> enabled them to don't risk any breakage.
> 
> Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
> [for drivers/nvmem/meson-{efuse,mx-efuse}.c]
> Acked-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>

[for mtk-efuse.c, nvmem/core.c, nvmem-provider.h]
Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>

[MT8192, MT8195 Chromebooks]
Tested-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
  

On 24.02.2023 09:29, Rafał Miłecki wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> 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).
> 
> This behaviour has actually caused a problem already with the MTD
> subsystem. MTD subpartitions were incorrectly treated as NVMEM cells.
> 
> Also with upcoming support for NVMEM layouts no new binding or driver
> should support fixed cells defined in device node.
> 
> Solve this by modifying drivers for bindings that support specifying
> fixed NVMEM cells in DT. Make them explicitly tell NVMEM subsystem to
> read cells from DT.
> 
> It wasn't clear (to me) if rtc and w1 code actually uses fixed cells. I
> enabled them to don't risk any breakage.
> 
> Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
> [for drivers/nvmem/meson-{efuse,mx-efuse}.c]
> Acked-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>

Reviewed-by: Claudiu Beznea <claudiu.beznea@microchip.com> # for microchip-otpc
Tested-by: Claudiu Beznea <claudiu.beznea@microchip.com> # on SAMA7G5-EK

> ---
> V2: Fix stm32-romem.c typo breaking its compilation
>     Pick Martin's Acked-by
>     Add paragraph about layouts deprecating use_fixed_of_cells
> ---
>  drivers/mtd/mtdcore.c          | 2 ++
>  drivers/nvmem/apple-efuses.c   | 1 +
>  drivers/nvmem/core.c           | 8 +++++---
>  drivers/nvmem/imx-ocotp-scu.c  | 1 +
>  drivers/nvmem/imx-ocotp.c      | 1 +
>  drivers/nvmem/meson-efuse.c    | 1 +
>  drivers/nvmem/meson-mx-efuse.c | 1 +
>  drivers/nvmem/microchip-otpc.c | 1 +
>  drivers/nvmem/mtk-efuse.c      | 1 +
>  drivers/nvmem/qcom-spmi-sdam.c | 1 +
>  drivers/nvmem/qfprom.c         | 1 +
>  drivers/nvmem/rave-sp-eeprom.c | 1 +
>  drivers/nvmem/rockchip-efuse.c | 1 +
>  drivers/nvmem/sc27xx-efuse.c   | 1 +
>  drivers/nvmem/sprd-efuse.c     | 1 +
>  drivers/nvmem/stm32-romem.c    | 1 +
>  drivers/nvmem/sunplus-ocotp.c  | 1 +
>  drivers/nvmem/sunxi_sid.c      | 1 +
>  drivers/nvmem/uniphier-efuse.c | 1 +
>  drivers/nvmem/zynqmp_nvmem.c   | 1 +
>  drivers/rtc/nvmem.c            | 1 +
>  drivers/w1/slaves/w1_ds250x.c  | 1 +
>  include/linux/nvmem-provider.h | 2 ++
>  23 files changed, 29 insertions(+), 3 deletions(-)
> 
> 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..a6fc43acb797 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->cfg.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;
> --
> 2.34.1
>
  

Hi Rafał,

zajec5@gmail.com wrote on Fri, 24 Feb 2023 08:29:03 +0100:

> 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).
> 
> This behaviour has actually caused a problem already with the MTD
> subsystem. MTD subpartitions were incorrectly treated as NVMEM cells.

That's true, but I expect this to be really MTD specific.

A concrete proposal below.

> Also with upcoming support for NVMEM layouts no new binding or driver
> should support fixed cells defined in device node.

I'm not sure I agree with this statement. We are not preventing new
binding/driver to use fixed cells, or...? We offer a new way to expose
nvmem cells with another way than "fixed-offset" and "fixed-size" OF
nodes.

> Solve this by modifying drivers for bindings that support specifying
> fixed NVMEM cells in DT. Make them explicitly tell NVMEM subsystem to
> read cells from DT.
> 
> It wasn't clear (to me) if rtc and w1 code actually uses fixed cells. I
> enabled them to don't risk any breakage.
> 
> Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
> [for drivers/nvmem/meson-{efuse,mx-efuse}.c]
> Acked-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
> ---
> V2: Fix stm32-romem.c typo breaking its compilation
>     Pick Martin's Acked-by
>     Add paragraph about layouts deprecating use_fixed_of_cells
> ---
>  drivers/mtd/mtdcore.c          | 2 ++
>  drivers/nvmem/apple-efuses.c   | 1 +
>  drivers/nvmem/core.c           | 8 +++++---
>  drivers/nvmem/imx-ocotp-scu.c  | 1 +
>  drivers/nvmem/imx-ocotp.c      | 1 +
>  drivers/nvmem/meson-efuse.c    | 1 +
>  drivers/nvmem/meson-mx-efuse.c | 1 +
>  drivers/nvmem/microchip-otpc.c | 1 +
>  drivers/nvmem/mtk-efuse.c      | 1 +
>  drivers/nvmem/qcom-spmi-sdam.c | 1 +
>  drivers/nvmem/qfprom.c         | 1 +
>  drivers/nvmem/rave-sp-eeprom.c | 1 +
>  drivers/nvmem/rockchip-efuse.c | 1 +
>  drivers/nvmem/sc27xx-efuse.c   | 1 +
>  drivers/nvmem/sprd-efuse.c     | 1 +
>  drivers/nvmem/stm32-romem.c    | 1 +
>  drivers/nvmem/sunplus-ocotp.c  | 1 +
>  drivers/nvmem/sunxi_sid.c      | 1 +
>  drivers/nvmem/uniphier-efuse.c | 1 +
>  drivers/nvmem/zynqmp_nvmem.c   | 1 +
>  drivers/rtc/nvmem.c            | 1 +
>  drivers/w1/slaves/w1_ds250x.c  | 1 +
>  include/linux/nvmem-provider.h | 2 ++
>  23 files changed, 29 insertions(+), 3 deletions(-)
> 
> 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");

I am wondering how mtd specific this is? For me all OF nodes containing
the nvmem-cells compatible should be treated as cells providers and
populate nvmem cells as for each children.

Why don't we just check for this compatible to be present? in
nvmem_add_cells_from_of() ? And if not we just skip the operation.

This way we still follow the bindings (even though using nvmem-cells in
the compatible property to require cells population was a mistake in
the first place, as discussed in the devlink thread recently) but there
is no need for a per-driver config option?

>  	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);
>  

Thanks,
Miquèl
  

On 2023-03-08 17:34, Miquel Raynal wrote:
> Hi Rafał,
> 
> zajec5@gmail.com wrote on Fri, 24 Feb 2023 08:29:03 +0100:
> 
>> 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).
>> 
>> This behaviour has actually caused a problem already with the MTD
>> subsystem. MTD subpartitions were incorrectly treated as NVMEM cells.
> 
> That's true, but I expect this to be really MTD specific.
> 
> A concrete proposal below.
> 
>> Also with upcoming support for NVMEM layouts no new binding or driver
>> should support fixed cells defined in device node.
> 
> I'm not sure I agree with this statement. We are not preventing new
> binding/driver to use fixed cells, or...? We offer a new way to expose
> nvmem cells with another way than "fixed-offset" and "fixed-size" OF
> nodes.

 From what I understood all new NVMEM bindings should have cells defined
in the nvmem-layout { } node. That's what I mean by saying they should
not be defined in device node (but its "nvmem-layout" instead).


>> Solve this by modifying drivers for bindings that support specifying
>> fixed NVMEM cells in DT. Make them explicitly tell NVMEM subsystem to
>> read cells from DT.
>> 
>> It wasn't clear (to me) if rtc and w1 code actually uses fixed cells. 
>> I
>> enabled them to don't risk any breakage.
>> 
>> Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
>> [for drivers/nvmem/meson-{efuse,mx-efuse}.c]
>> Acked-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
>> ---
>> V2: Fix stm32-romem.c typo breaking its compilation
>>     Pick Martin's Acked-by
>>     Add paragraph about layouts deprecating use_fixed_of_cells
>> ---
>>  drivers/mtd/mtdcore.c          | 2 ++
>>  drivers/nvmem/apple-efuses.c   | 1 +
>>  drivers/nvmem/core.c           | 8 +++++---
>>  drivers/nvmem/imx-ocotp-scu.c  | 1 +
>>  drivers/nvmem/imx-ocotp.c      | 1 +
>>  drivers/nvmem/meson-efuse.c    | 1 +
>>  drivers/nvmem/meson-mx-efuse.c | 1 +
>>  drivers/nvmem/microchip-otpc.c | 1 +
>>  drivers/nvmem/mtk-efuse.c      | 1 +
>>  drivers/nvmem/qcom-spmi-sdam.c | 1 +
>>  drivers/nvmem/qfprom.c         | 1 +
>>  drivers/nvmem/rave-sp-eeprom.c | 1 +
>>  drivers/nvmem/rockchip-efuse.c | 1 +
>>  drivers/nvmem/sc27xx-efuse.c   | 1 +
>>  drivers/nvmem/sprd-efuse.c     | 1 +
>>  drivers/nvmem/stm32-romem.c    | 1 +
>>  drivers/nvmem/sunplus-ocotp.c  | 1 +
>>  drivers/nvmem/sunxi_sid.c      | 1 +
>>  drivers/nvmem/uniphier-efuse.c | 1 +
>>  drivers/nvmem/zynqmp_nvmem.c   | 1 +
>>  drivers/rtc/nvmem.c            | 1 +
>>  drivers/w1/slaves/w1_ds250x.c  | 1 +
>>  include/linux/nvmem-provider.h | 2 ++
>>  23 files changed, 29 insertions(+), 3 deletions(-)
>> 
>> 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");
> 
> I am wondering how mtd specific this is? For me all OF nodes containing
> the nvmem-cells compatible should be treated as cells providers and
> populate nvmem cells as for each children.
> 
> Why don't we just check for this compatible to be present? in
> nvmem_add_cells_from_of() ? And if not we just skip the operation.
> 
> This way we still follow the bindings (even though using nvmem-cells in
> the compatible property to require cells population was a mistake in
> the first place, as discussed in the devlink thread recently) but there
> is no need for a per-driver config option?

This isn't mtd specific. Please check this patch for all occurrences of
the:
use_fixed_of_cells = true

The very first one: drivers/nvmem/apple-efuses.c driver for the
"apple,efuses" binding. That binding supports fixed OF cells, see:
Documentation/devicetree/bindings/nvmem/apple,efuses.yaml


>>  	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);
>> 
> 
> Thanks,
> Miquèl
  

Hi Rafał,

rafal@milecki.pl wrote on Wed, 08 Mar 2023 17:55:46 +0100:

> On 2023-03-08 17:34, Miquel Raynal wrote:
> > Hi Rafał,
> > 
> > zajec5@gmail.com wrote on Fri, 24 Feb 2023 08:29:03 +0100:
> >   
> >> 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).  
> >> >> This behaviour has actually caused a problem already with the MTD  
> >> subsystem. MTD subpartitions were incorrectly treated as NVMEM cells.  
> > 
> > That's true, but I expect this to be really MTD specific.
> > 
> > A concrete proposal below.
> >   
> >> Also with upcoming support for NVMEM layouts no new binding or driver
> >> should support fixed cells defined in device node.  
> > 
> > I'm not sure I agree with this statement. We are not preventing new
> > binding/driver to use fixed cells, or...? We offer a new way to expose
> > nvmem cells with another way than "fixed-offset" and "fixed-size" OF
> > nodes.  
> 
>  From what I understood all new NVMEM bindings should have cells defined
> in the nvmem-layout { } node. That's what I mean by saying they should
> not be defined in device node (but its "nvmem-layout" instead).

Layouts are just another possibility, either you user the nvmem-cells
compatible and produce nvmem cells with fixed OF nodes, or you use the
nvmem-layout container. I don't think all new bindings should have
cells in layouts. It depends if the content is static or not.

> >> Solve this by modifying drivers for bindings that support specifying
> >> fixed NVMEM cells in DT. Make them explicitly tell NVMEM subsystem to
> >> read cells from DT.  
> >> >> It wasn't clear (to me) if rtc and w1 code actually uses fixed cells. >> I  
> >> enabled them to don't risk any breakage.  
> >> >> Signed-off-by: Rafał Miłecki <rafal@milecki.pl>  
> >> [for drivers/nvmem/meson-{efuse,mx-efuse}.c]
> >> Acked-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
> >> ---
> >> V2: Fix stm32-romem.c typo breaking its compilation
> >>     Pick Martin's Acked-by
> >>     Add paragraph about layouts deprecating use_fixed_of_cells
> >> ---
> >>  drivers/mtd/mtdcore.c          | 2 ++
> >>  drivers/nvmem/apple-efuses.c   | 1 +
> >>  drivers/nvmem/core.c           | 8 +++++---
> >>  drivers/nvmem/imx-ocotp-scu.c  | 1 +
> >>  drivers/nvmem/imx-ocotp.c      | 1 +
> >>  drivers/nvmem/meson-efuse.c    | 1 +
> >>  drivers/nvmem/meson-mx-efuse.c | 1 +
> >>  drivers/nvmem/microchip-otpc.c | 1 +
> >>  drivers/nvmem/mtk-efuse.c      | 1 +
> >>  drivers/nvmem/qcom-spmi-sdam.c | 1 +
> >>  drivers/nvmem/qfprom.c         | 1 +
> >>  drivers/nvmem/rave-sp-eeprom.c | 1 +
> >>  drivers/nvmem/rockchip-efuse.c | 1 +
> >>  drivers/nvmem/sc27xx-efuse.c   | 1 +
> >>  drivers/nvmem/sprd-efuse.c     | 1 +
> >>  drivers/nvmem/stm32-romem.c    | 1 +
> >>  drivers/nvmem/sunplus-ocotp.c  | 1 +
> >>  drivers/nvmem/sunxi_sid.c      | 1 +
> >>  drivers/nvmem/uniphier-efuse.c | 1 +
> >>  drivers/nvmem/zynqmp_nvmem.c   | 1 +
> >>  drivers/rtc/nvmem.c            | 1 +
> >>  drivers/w1/slaves/w1_ds250x.c  | 1 +
> >>  include/linux/nvmem-provider.h | 2 ++
> >>  23 files changed, 29 insertions(+), 3 deletions(-)  
> >> >> 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");  
> > 
> > I am wondering how mtd specific this is? For me all OF nodes containing
> > the nvmem-cells compatible should be treated as cells providers and
> > populate nvmem cells as for each children.
> > 
> > Why don't we just check for this compatible to be present? in
> > nvmem_add_cells_from_of() ? And if not we just skip the operation.
> > 
> > This way we still follow the bindings (even though using nvmem-cells in
> > the compatible property to require cells population was a mistake in
> > the first place, as discussed in the devlink thread recently) but there
> > is no need for a per-driver config option?  
> 
> This isn't mtd specific. Please check this patch for all occurrences of
> the:
> use_fixed_of_cells = true
> 
> The very first one: drivers/nvmem/apple-efuses.c driver for the
> "apple,efuses" binding. That binding supports fixed OF cells, see:
> Documentation/devicetree/bindings/nvmem/apple,efuses.yaml

I'm saying: based on what has been enforced so far, I would expect all
fixed cell providers to come with nvmem-cells as compatible, no?

If that's the case we could use that as a common denominator?

> 
> 
> >>  	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);
> >> > > Thanks,  
> > Miquèl  


Thanks,
Miquèl
  

On 2023-03-08 19:06, Miquel Raynal wrote:
> Hi Rafał,
> 
> rafal@milecki.pl wrote on Wed, 08 Mar 2023 17:55:46 +0100:
> 
>> On 2023-03-08 17:34, Miquel Raynal wrote:
>> > Hi Rafał,
>> >
>> > zajec5@gmail.com wrote on Fri, 24 Feb 2023 08:29:03 +0100:
>> >
>> >> 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).
>> >> >> This behaviour has actually caused a problem already with the MTD
>> >> subsystem. MTD subpartitions were incorrectly treated as NVMEM cells.
>> >
>> > That's true, but I expect this to be really MTD specific.
>> >
>> > A concrete proposal below.
>> >
>> >> Also with upcoming support for NVMEM layouts no new binding or driver
>> >> should support fixed cells defined in device node.
>> >
>> > I'm not sure I agree with this statement. We are not preventing new
>> > binding/driver to use fixed cells, or...? We offer a new way to expose
>> > nvmem cells with another way than "fixed-offset" and "fixed-size" OF
>> > nodes.
>> 
>>  From what I understood all new NVMEM bindings should have cells 
>> defined
>> in the nvmem-layout { } node. That's what I mean by saying they should
>> not be defined in device node (but its "nvmem-layout" instead).
> 
> Layouts are just another possibility, either you user the nvmem-cells
> compatible and produce nvmem cells with fixed OF nodes, or you use the
> nvmem-layout container. I don't think all new bindings should have
> cells in layouts. It depends if the content is static or not.
> 
>> >> Solve this by modifying drivers for bindings that support specifying
>> >> fixed NVMEM cells in DT. Make them explicitly tell NVMEM subsystem to
>> >> read cells from DT.
>> >> >> It wasn't clear (to me) if rtc and w1 code actually uses fixed cells. >> I
>> >> enabled them to don't risk any breakage.
>> >> >> Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
>> >> [for drivers/nvmem/meson-{efuse,mx-efuse}.c]
>> >> Acked-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
>> >> ---
>> >> V2: Fix stm32-romem.c typo breaking its compilation
>> >>     Pick Martin's Acked-by
>> >>     Add paragraph about layouts deprecating use_fixed_of_cells
>> >> ---
>> >>  drivers/mtd/mtdcore.c          | 2 ++
>> >>  drivers/nvmem/apple-efuses.c   | 1 +
>> >>  drivers/nvmem/core.c           | 8 +++++---
>> >>  drivers/nvmem/imx-ocotp-scu.c  | 1 +
>> >>  drivers/nvmem/imx-ocotp.c      | 1 +
>> >>  drivers/nvmem/meson-efuse.c    | 1 +
>> >>  drivers/nvmem/meson-mx-efuse.c | 1 +
>> >>  drivers/nvmem/microchip-otpc.c | 1 +
>> >>  drivers/nvmem/mtk-efuse.c      | 1 +
>> >>  drivers/nvmem/qcom-spmi-sdam.c | 1 +
>> >>  drivers/nvmem/qfprom.c         | 1 +
>> >>  drivers/nvmem/rave-sp-eeprom.c | 1 +
>> >>  drivers/nvmem/rockchip-efuse.c | 1 +
>> >>  drivers/nvmem/sc27xx-efuse.c   | 1 +
>> >>  drivers/nvmem/sprd-efuse.c     | 1 +
>> >>  drivers/nvmem/stm32-romem.c    | 1 +
>> >>  drivers/nvmem/sunplus-ocotp.c  | 1 +
>> >>  drivers/nvmem/sunxi_sid.c      | 1 +
>> >>  drivers/nvmem/uniphier-efuse.c | 1 +
>> >>  drivers/nvmem/zynqmp_nvmem.c   | 1 +
>> >>  drivers/rtc/nvmem.c            | 1 +
>> >>  drivers/w1/slaves/w1_ds250x.c  | 1 +
>> >>  include/linux/nvmem-provider.h | 2 ++
>> >>  23 files changed, 29 insertions(+), 3 deletions(-)
>> >> >> 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");
>> >
>> > I am wondering how mtd specific this is? For me all OF nodes containing
>> > the nvmem-cells compatible should be treated as cells providers and
>> > populate nvmem cells as for each children.
>> >
>> > Why don't we just check for this compatible to be present? in
>> > nvmem_add_cells_from_of() ? And if not we just skip the operation.
>> >
>> > This way we still follow the bindings (even though using nvmem-cells in
>> > the compatible property to require cells population was a mistake in
>> > the first place, as discussed in the devlink thread recently) but there
>> > is no need for a per-driver config option?
>> 
>> This isn't mtd specific. Please check this patch for all occurrences 
>> of
>> the:
>> use_fixed_of_cells = true
>> 
>> The very first one: drivers/nvmem/apple-efuses.c driver for the
>> "apple,efuses" binding. That binding supports fixed OF cells, see:
>> Documentation/devicetree/bindings/nvmem/apple,efuses.yaml
> 
> I'm saying: based on what has been enforced so far, I would expect all
> fixed cell providers to come with nvmem-cells as compatible, no?
> 
> If that's the case we could use that as a common denominator?

Sorry, I don't get it. Have you checked
Documentation/devicetree/bindings/nvmem/apple,efuses.yaml
?

It's a NVMEM provied binding with fixed cells that doesn't use
nvmem-cells as compatible. There are many more.
  

Hi Rafał,

rafal@milecki.pl wrote on Wed, 08 Mar 2023 19:12:32 +0100:

> On 2023-03-08 19:06, Miquel Raynal wrote:
> > Hi Rafał,
> > 
> > rafal@milecki.pl wrote on Wed, 08 Mar 2023 17:55:46 +0100:
> >   
> >> On 2023-03-08 17:34, Miquel Raynal wrote:  
> >> > Hi Rafał,
> >> >
> >> > zajec5@gmail.com wrote on Fri, 24 Feb 2023 08:29:03 +0100:
> >> >  
> >> >> 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).  
> >> >> >> This behaviour has actually caused a problem already with the MTD  
> >> >> subsystem. MTD subpartitions were incorrectly treated as NVMEM cells.  
> >> >
> >> > That's true, but I expect this to be really MTD specific.
> >> >
> >> > A concrete proposal below.
> >> >  
> >> >> Also with upcoming support for NVMEM layouts no new binding or driver
> >> >> should support fixed cells defined in device node.  
> >> >
> >> > I'm not sure I agree with this statement. We are not preventing new
> >> > binding/driver to use fixed cells, or...? We offer a new way to expose
> >> > nvmem cells with another way than "fixed-offset" and "fixed-size" OF
> >> > nodes.  
> >> >>  From what I understood all new NVMEM bindings should have cells >> defined  
> >> in the nvmem-layout { } node. That's what I mean by saying they should
> >> not be defined in device node (but its "nvmem-layout" instead).  
> > 
> > Layouts are just another possibility, either you user the nvmem-cells
> > compatible and produce nvmem cells with fixed OF nodes, or you use the
> > nvmem-layout container. I don't think all new bindings should have
> > cells in layouts. It depends if the content is static or not.
> >   
> >> >> Solve this by modifying drivers for bindings that support specifying
> >> >> fixed NVMEM cells in DT. Make them explicitly tell NVMEM subsystem to
> >> >> read cells from DT.  
> >> >> >> It wasn't clear (to me) if rtc and w1 code actually uses fixed cells. >> I  
> >> >> enabled them to don't risk any breakage.  
> >> >> >> Signed-off-by: Rafał Miłecki <rafal@milecki.pl>  
> >> >> [for drivers/nvmem/meson-{efuse,mx-efuse}.c]
> >> >> Acked-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
> >> >> ---
> >> >> V2: Fix stm32-romem.c typo breaking its compilation
> >> >>     Pick Martin's Acked-by
> >> >>     Add paragraph about layouts deprecating use_fixed_of_cells
> >> >> ---
> >> >>  drivers/mtd/mtdcore.c          | 2 ++
> >> >>  drivers/nvmem/apple-efuses.c   | 1 +
> >> >>  drivers/nvmem/core.c           | 8 +++++---
> >> >>  drivers/nvmem/imx-ocotp-scu.c  | 1 +
> >> >>  drivers/nvmem/imx-ocotp.c      | 1 +
> >> >>  drivers/nvmem/meson-efuse.c    | 1 +
> >> >>  drivers/nvmem/meson-mx-efuse.c | 1 +
> >> >>  drivers/nvmem/microchip-otpc.c | 1 +
> >> >>  drivers/nvmem/mtk-efuse.c      | 1 +
> >> >>  drivers/nvmem/qcom-spmi-sdam.c | 1 +
> >> >>  drivers/nvmem/qfprom.c         | 1 +
> >> >>  drivers/nvmem/rave-sp-eeprom.c | 1 +
> >> >>  drivers/nvmem/rockchip-efuse.c | 1 +
> >> >>  drivers/nvmem/sc27xx-efuse.c   | 1 +
> >> >>  drivers/nvmem/sprd-efuse.c     | 1 +
> >> >>  drivers/nvmem/stm32-romem.c    | 1 +
> >> >>  drivers/nvmem/sunplus-ocotp.c  | 1 +
> >> >>  drivers/nvmem/sunxi_sid.c      | 1 +
> >> >>  drivers/nvmem/uniphier-efuse.c | 1 +
> >> >>  drivers/nvmem/zynqmp_nvmem.c   | 1 +
> >> >>  drivers/rtc/nvmem.c            | 1 +
> >> >>  drivers/w1/slaves/w1_ds250x.c  | 1 +
> >> >>  include/linux/nvmem-provider.h | 2 ++
> >> >>  23 files changed, 29 insertions(+), 3 deletions(-)  
> >> >> >> 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");  
> >> >
> >> > I am wondering how mtd specific this is? For me all OF nodes containing
> >> > the nvmem-cells compatible should be treated as cells providers and
> >> > populate nvmem cells as for each children.
> >> >
> >> > Why don't we just check for this compatible to be present? in
> >> > nvmem_add_cells_from_of() ? And if not we just skip the operation.
> >> >
> >> > This way we still follow the bindings (even though using nvmem-cells in
> >> > the compatible property to require cells population was a mistake in
> >> > the first place, as discussed in the devlink thread recently) but there
> >> > is no need for a per-driver config option?  
> >> >> This isn't mtd specific. Please check this patch for all occurrences >> of  
> >> the:
> >> use_fixed_of_cells = true  
> >> >> The very first one: drivers/nvmem/apple-efuses.c driver for the  
> >> "apple,efuses" binding. That binding supports fixed OF cells, see:
> >> Documentation/devicetree/bindings/nvmem/apple,efuses.yaml  
> > 
> > I'm saying: based on what has been enforced so far, I would expect all
> > fixed cell providers to come with nvmem-cells as compatible, no?
> > 
> > If that's the case we could use that as a common denominator?  
> 
> Sorry, I don't get it. Have you checked
> Documentation/devicetree/bindings/nvmem/apple,efuses.yaml
> ?
> 
> It's a NVMEM provied binding with fixed cells that doesn't use
> nvmem-cells as compatible. There are many more.

Oh yeah you're right, I'm mixing things. Well I guess you're right
then, it's such a mess, we have to tell the core the parsing method.

So maybe another question: do we have other situations than mtd which
sometimes expect the nvmem core to parse the OF nodes to populate cells,
and sometimes not?

Also, what about "of_children_are_cells" ? Because actually in most
cases it's a "fixed of cell", so I don't find the current naming
descriptive enough for something so touchy.

Thanks, Miquèl
  

On 8.03.2023 19:31, Miquel Raynal wrote:
> Hi Rafał,
> 
> rafal@milecki.pl wrote on Wed, 08 Mar 2023 19:12:32 +0100:
> 
>> On 2023-03-08 19:06, Miquel Raynal wrote:
>>> Hi Rafał,
>>>
>>> rafal@milecki.pl wrote on Wed, 08 Mar 2023 17:55:46 +0100:
>>>    
>>>> On 2023-03-08 17:34, Miquel Raynal wrote:
>>>>> Hi Rafał,
>>>>>
>>>>> zajec5@gmail.com wrote on Fri, 24 Feb 2023 08:29:03 +0100:
>>>>>   
>>>>>> 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).
>>>>>>>> This behaviour has actually caused a problem already with the MTD
>>>>>> subsystem. MTD subpartitions were incorrectly treated as NVMEM cells.
>>>>>
>>>>> That's true, but I expect this to be really MTD specific.
>>>>>
>>>>> A concrete proposal below.
>>>>>   
>>>>>> Also with upcoming support for NVMEM layouts no new binding or driver
>>>>>> should support fixed cells defined in device node.
>>>>>
>>>>> I'm not sure I agree with this statement. We are not preventing new
>>>>> binding/driver to use fixed cells, or...? We offer a new way to expose
>>>>> nvmem cells with another way than "fixed-offset" and "fixed-size" OF
>>>>> nodes.
>>>>>>   From what I understood all new NVMEM bindings should have cells >> defined
>>>> in the nvmem-layout { } node. That's what I mean by saying they should
>>>> not be defined in device node (but its "nvmem-layout" instead).
>>>
>>> Layouts are just another possibility, either you user the nvmem-cells
>>> compatible and produce nvmem cells with fixed OF nodes, or you use the
>>> nvmem-layout container. I don't think all new bindings should have
>>> cells in layouts. It depends if the content is static or not.
>>>    
>>>>>> Solve this by modifying drivers for bindings that support specifying
>>>>>> fixed NVMEM cells in DT. Make them explicitly tell NVMEM subsystem to
>>>>>> read cells from DT.
>>>>>>>> It wasn't clear (to me) if rtc and w1 code actually uses fixed cells. >> I
>>>>>> enabled them to don't risk any breakage.
>>>>>>>> Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
>>>>>> [for drivers/nvmem/meson-{efuse,mx-efuse}.c]
>>>>>> Acked-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
>>>>>> ---
>>>>>> V2: Fix stm32-romem.c typo breaking its compilation
>>>>>>      Pick Martin's Acked-by
>>>>>>      Add paragraph about layouts deprecating use_fixed_of_cells
>>>>>> ---
>>>>>>   drivers/mtd/mtdcore.c          | 2 ++
>>>>>>   drivers/nvmem/apple-efuses.c   | 1 +
>>>>>>   drivers/nvmem/core.c           | 8 +++++---
>>>>>>   drivers/nvmem/imx-ocotp-scu.c  | 1 +
>>>>>>   drivers/nvmem/imx-ocotp.c      | 1 +
>>>>>>   drivers/nvmem/meson-efuse.c    | 1 +
>>>>>>   drivers/nvmem/meson-mx-efuse.c | 1 +
>>>>>>   drivers/nvmem/microchip-otpc.c | 1 +
>>>>>>   drivers/nvmem/mtk-efuse.c      | 1 +
>>>>>>   drivers/nvmem/qcom-spmi-sdam.c | 1 +
>>>>>>   drivers/nvmem/qfprom.c         | 1 +
>>>>>>   drivers/nvmem/rave-sp-eeprom.c | 1 +
>>>>>>   drivers/nvmem/rockchip-efuse.c | 1 +
>>>>>>   drivers/nvmem/sc27xx-efuse.c   | 1 +
>>>>>>   drivers/nvmem/sprd-efuse.c     | 1 +
>>>>>>   drivers/nvmem/stm32-romem.c    | 1 +
>>>>>>   drivers/nvmem/sunplus-ocotp.c  | 1 +
>>>>>>   drivers/nvmem/sunxi_sid.c      | 1 +
>>>>>>   drivers/nvmem/uniphier-efuse.c | 1 +
>>>>>>   drivers/nvmem/zynqmp_nvmem.c   | 1 +
>>>>>>   drivers/rtc/nvmem.c            | 1 +
>>>>>>   drivers/w1/slaves/w1_ds250x.c  | 1 +
>>>>>>   include/linux/nvmem-provider.h | 2 ++
>>>>>>   23 files changed, 29 insertions(+), 3 deletions(-)
>>>>>>>> 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");
>>>>>
>>>>> I am wondering how mtd specific this is? For me all OF nodes containing
>>>>> the nvmem-cells compatible should be treated as cells providers and
>>>>> populate nvmem cells as for each children.
>>>>>
>>>>> Why don't we just check for this compatible to be present? in
>>>>> nvmem_add_cells_from_of() ? And if not we just skip the operation.
>>>>>
>>>>> This way we still follow the bindings (even though using nvmem-cells in
>>>>> the compatible property to require cells population was a mistake in
>>>>> the first place, as discussed in the devlink thread recently) but there
>>>>> is no need for a per-driver config option?
>>>>>> This isn't mtd specific. Please check this patch for all occurrences >> of
>>>> the:
>>>> use_fixed_of_cells = true
>>>>>> The very first one: drivers/nvmem/apple-efuses.c driver for the
>>>> "apple,efuses" binding. That binding supports fixed OF cells, see:
>>>> Documentation/devicetree/bindings/nvmem/apple,efuses.yaml
>>>
>>> I'm saying: based on what has been enforced so far, I would expect all
>>> fixed cell providers to come with nvmem-cells as compatible, no?
>>>
>>> If that's the case we could use that as a common denominator?
>>
>> Sorry, I don't get it. Have you checked
>> Documentation/devicetree/bindings/nvmem/apple,efuses.yaml
>> ?
>>
>> It's a NVMEM provied binding with fixed cells that doesn't use
>> nvmem-cells as compatible. There are many more.
> 
> Oh yeah you're right, I'm mixing things. Well I guess you're right
> then, it's such a mess, we have to tell the core the parsing method.
> 
> So maybe another question: do we have other situations than mtd which
> sometimes expect the nvmem core to parse the OF nodes to populate cells,
> and sometimes not?

I'm not aware of that. Please also check my patch. The only case I set
"use_fixed_of_cells" conditionally is mtd code. In other cases it's
hardcoded to "true".


> Also, what about "of_children_are_cells" ? Because actually in most
> cases it's a "fixed of cell", so I don't find the current naming
> descriptive enough for something so touchy.

That would be just incorrect because this new config property
("use_fixed_of_cells") is only about FIXED cells.

There are cases of OF children being cells but NOT being fixed cells.
They should NOT be parsed by the nvmem_add_cells_from_of().

Example:
a607a850ba1f ("dt-bindings: nvmem: u-boot,env: add basic NVMEM cells")
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=a607a850ba1fa966cbb035544c1588e24a6307df

So that would result in U-Boot env:
1. Having OF children nodes being cells
2. Setting "of_children_are_cells" to false (counter-intuitive) to avoid nvmem_add_cells_from_of()
  

Hi Rafał,

zajec5@gmail.com wrote on Thu, 9 Mar 2023 07:56:05 +0100:

> On 8.03.2023 19:31, Miquel Raynal wrote:
> > Hi Rafał,
> > 
> > rafal@milecki.pl wrote on Wed, 08 Mar 2023 19:12:32 +0100:
> >   
> >> On 2023-03-08 19:06, Miquel Raynal wrote:  
> >>> Hi Rafał,
> >>>
> >>> rafal@milecki.pl wrote on Wed, 08 Mar 2023 17:55:46 +0100:  
> >>>    >>>> On 2023-03-08 17:34, Miquel Raynal wrote:  
> >>>>> Hi Rafał,
> >>>>>
> >>>>> zajec5@gmail.com wrote on Fri, 24 Feb 2023 08:29:03 +0100:  
> >>>>>   >>>>>> 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).  
> >>>>>>>> This behaviour has actually caused a problem already with the MTD  
> >>>>>> subsystem. MTD subpartitions were incorrectly treated as NVMEM cells.  
> >>>>>
> >>>>> That's true, but I expect this to be really MTD specific.
> >>>>>
> >>>>> A concrete proposal below.  
> >>>>>   >>>>>> Also with upcoming support for NVMEM layouts no new binding or driver  
> >>>>>> should support fixed cells defined in device node.  
> >>>>>
> >>>>> I'm not sure I agree with this statement. We are not preventing new
> >>>>> binding/driver to use fixed cells, or...? We offer a new way to expose
> >>>>> nvmem cells with another way than "fixed-offset" and "fixed-size" OF
> >>>>> nodes.  
> >>>>>>   From what I understood all new NVMEM bindings should have cells >> defined  
> >>>> in the nvmem-layout { } node. That's what I mean by saying they should
> >>>> not be defined in device node (but its "nvmem-layout" instead).  
> >>>
> >>> Layouts are just another possibility, either you user the nvmem-cells
> >>> compatible and produce nvmem cells with fixed OF nodes, or you use the
> >>> nvmem-layout container. I don't think all new bindings should have
> >>> cells in layouts. It depends if the content is static or not.  
> >>>    >>>>>> Solve this by modifying drivers for bindings that support specifying  
> >>>>>> fixed NVMEM cells in DT. Make them explicitly tell NVMEM subsystem to
> >>>>>> read cells from DT.  
> >>>>>>>> It wasn't clear (to me) if rtc and w1 code actually uses fixed cells. >> I  
> >>>>>> enabled them to don't risk any breakage.  
> >>>>>>>> Signed-off-by: Rafał Miłecki <rafal@milecki.pl>  
> >>>>>> [for drivers/nvmem/meson-{efuse,mx-efuse}.c]
> >>>>>> Acked-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
> >>>>>> ---
> >>>>>> V2: Fix stm32-romem.c typo breaking its compilation
> >>>>>>      Pick Martin's Acked-by
> >>>>>>      Add paragraph about layouts deprecating use_fixed_of_cells
> >>>>>> ---
> >>>>>>   drivers/mtd/mtdcore.c          | 2 ++
> >>>>>>   drivers/nvmem/apple-efuses.c   | 1 +
> >>>>>>   drivers/nvmem/core.c           | 8 +++++---
> >>>>>>   drivers/nvmem/imx-ocotp-scu.c  | 1 +
> >>>>>>   drivers/nvmem/imx-ocotp.c      | 1 +
> >>>>>>   drivers/nvmem/meson-efuse.c    | 1 +
> >>>>>>   drivers/nvmem/meson-mx-efuse.c | 1 +
> >>>>>>   drivers/nvmem/microchip-otpc.c | 1 +
> >>>>>>   drivers/nvmem/mtk-efuse.c      | 1 +
> >>>>>>   drivers/nvmem/qcom-spmi-sdam.c | 1 +
> >>>>>>   drivers/nvmem/qfprom.c         | 1 +
> >>>>>>   drivers/nvmem/rave-sp-eeprom.c | 1 +
> >>>>>>   drivers/nvmem/rockchip-efuse.c | 1 +
> >>>>>>   drivers/nvmem/sc27xx-efuse.c   | 1 +
> >>>>>>   drivers/nvmem/sprd-efuse.c     | 1 +
> >>>>>>   drivers/nvmem/stm32-romem.c    | 1 +
> >>>>>>   drivers/nvmem/sunplus-ocotp.c  | 1 +
> >>>>>>   drivers/nvmem/sunxi_sid.c      | 1 +
> >>>>>>   drivers/nvmem/uniphier-efuse.c | 1 +
> >>>>>>   drivers/nvmem/zynqmp_nvmem.c   | 1 +
> >>>>>>   drivers/rtc/nvmem.c            | 1 +
> >>>>>>   drivers/w1/slaves/w1_ds250x.c  | 1 +
> >>>>>>   include/linux/nvmem-provider.h | 2 ++
> >>>>>>   23 files changed, 29 insertions(+), 3 deletions(-)  
> >>>>>>>> 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");  
> >>>>>
> >>>>> I am wondering how mtd specific this is? For me all OF nodes containing
> >>>>> the nvmem-cells compatible should be treated as cells providers and
> >>>>> populate nvmem cells as for each children.
> >>>>>
> >>>>> Why don't we just check for this compatible to be present? in
> >>>>> nvmem_add_cells_from_of() ? And if not we just skip the operation.
> >>>>>
> >>>>> This way we still follow the bindings (even though using nvmem-cells in
> >>>>> the compatible property to require cells population was a mistake in
> >>>>> the first place, as discussed in the devlink thread recently) but there
> >>>>> is no need for a per-driver config option?  
> >>>>>> This isn't mtd specific. Please check this patch for all occurrences >> of  
> >>>> the:
> >>>> use_fixed_of_cells = true  
> >>>>>> The very first one: drivers/nvmem/apple-efuses.c driver for the  
> >>>> "apple,efuses" binding. That binding supports fixed OF cells, see:
> >>>> Documentation/devicetree/bindings/nvmem/apple,efuses.yaml  
> >>>
> >>> I'm saying: based on what has been enforced so far, I would expect all
> >>> fixed cell providers to come with nvmem-cells as compatible, no?
> >>>
> >>> If that's the case we could use that as a common denominator?  
> >>
> >> Sorry, I don't get it. Have you checked
> >> Documentation/devicetree/bindings/nvmem/apple,efuses.yaml
> >> ?
> >>
> >> It's a NVMEM provied binding with fixed cells that doesn't use
> >> nvmem-cells as compatible. There are many more.  
> > 
> > Oh yeah you're right, I'm mixing things. Well I guess you're right
> > then, it's such a mess, we have to tell the core the parsing method.
> > 
> > So maybe another question: do we have other situations than mtd which
> > sometimes expect the nvmem core to parse the OF nodes to populate cells,
> > and sometimes not?  
> 
> I'm not aware of that. Please also check my patch. The only case I set
> "use_fixed_of_cells" conditionally is mtd code. In other cases it's
> hardcoded to "true".
> 
> 
> > Also, what about "of_children_are_cells" ? Because actually in most
> > cases it's a "fixed of cell", so I don't find the current naming
> > descriptive enough for something so touchy.  
> 
> That would be just incorrect because this new config property
> ("use_fixed_of_cells") is only about FIXED cells.
> 
> There are cases of OF children being cells but NOT being fixed cells.
> They should NOT be parsed by the nvmem_add_cells_from_of().
> 
> Example:
> a607a850ba1f ("dt-bindings: nvmem: u-boot,env: add basic NVMEM cells")
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=a607a850ba1fa966cbb035544c1588e24a6307df

This is backwards. That's why layouts have been proposed: having
a clear framework were the nvmem core should or should not play with
the OF children nodes. If each binding is different, you end up with
the mess we have today, where nobody knows how to properly
populate the cells.

Anyway, it's not a big deal either, if the cells are empty we can
easily check that and have *yet another* specific case in the core.

> So that would result in U-Boot env:
> 1. Having OF children nodes being cells
> 2. Setting "of_children_are_cells" to false (counter-intuitive) to avoid nvmem_add_cells_from_of()

Agreed. So what about config.ignore_of_children?
- mtd sets this to !is_compatible(nvmem-cells)
- nobody else touches it (the core don't try to parse the cell if it's
  empty so U-Boot env driver works)

Thanks,
Miquèl
  

On 2023-03-09 09:34, Miquel Raynal wrote:
> Hi Rafał,
> 
> zajec5@gmail.com wrote on Thu, 9 Mar 2023 07:56:05 +0100:
> 
>> On 8.03.2023 19:31, Miquel Raynal wrote:
>> > Hi Rafał,
>> >
>> > rafal@milecki.pl wrote on Wed, 08 Mar 2023 19:12:32 +0100:
>> >
>> >> On 2023-03-08 19:06, Miquel Raynal wrote:
>> >>> Hi Rafał,
>> >>>
>> >>> rafal@milecki.pl wrote on Wed, 08 Mar 2023 17:55:46 +0100:
>> >>>    >>>> On 2023-03-08 17:34, Miquel Raynal wrote:
>> >>>>> Hi Rafał,
>> >>>>>
>> >>>>> zajec5@gmail.com wrote on Fri, 24 Feb 2023 08:29:03 +0100:
>> >>>>>   >>>>>> 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).
>> >>>>>>>> This behaviour has actually caused a problem already with the MTD
>> >>>>>> subsystem. MTD subpartitions were incorrectly treated as NVMEM cells.
>> >>>>>
>> >>>>> That's true, but I expect this to be really MTD specific.
>> >>>>>
>> >>>>> A concrete proposal below.
>> >>>>>   >>>>>> Also with upcoming support for NVMEM layouts no new binding or driver
>> >>>>>> should support fixed cells defined in device node.
>> >>>>>
>> >>>>> I'm not sure I agree with this statement. We are not preventing new
>> >>>>> binding/driver to use fixed cells, or...? We offer a new way to expose
>> >>>>> nvmem cells with another way than "fixed-offset" and "fixed-size" OF
>> >>>>> nodes.
>> >>>>>>   From what I understood all new NVMEM bindings should have cells >> defined
>> >>>> in the nvmem-layout { } node. That's what I mean by saying they should
>> >>>> not be defined in device node (but its "nvmem-layout" instead).
>> >>>
>> >>> Layouts are just another possibility, either you user the nvmem-cells
>> >>> compatible and produce nvmem cells with fixed OF nodes, or you use the
>> >>> nvmem-layout container. I don't think all new bindings should have
>> >>> cells in layouts. It depends if the content is static or not.
>> >>>    >>>>>> Solve this by modifying drivers for bindings that support specifying
>> >>>>>> fixed NVMEM cells in DT. Make them explicitly tell NVMEM subsystem to
>> >>>>>> read cells from DT.
>> >>>>>>>> It wasn't clear (to me) if rtc and w1 code actually uses fixed cells. >> I
>> >>>>>> enabled them to don't risk any breakage.
>> >>>>>>>> Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
>> >>>>>> [for drivers/nvmem/meson-{efuse,mx-efuse}.c]
>> >>>>>> Acked-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
>> >>>>>> ---
>> >>>>>> V2: Fix stm32-romem.c typo breaking its compilation
>> >>>>>>      Pick Martin's Acked-by
>> >>>>>>      Add paragraph about layouts deprecating use_fixed_of_cells
>> >>>>>> ---
>> >>>>>>   drivers/mtd/mtdcore.c          | 2 ++
>> >>>>>>   drivers/nvmem/apple-efuses.c   | 1 +
>> >>>>>>   drivers/nvmem/core.c           | 8 +++++---
>> >>>>>>   drivers/nvmem/imx-ocotp-scu.c  | 1 +
>> >>>>>>   drivers/nvmem/imx-ocotp.c      | 1 +
>> >>>>>>   drivers/nvmem/meson-efuse.c    | 1 +
>> >>>>>>   drivers/nvmem/meson-mx-efuse.c | 1 +
>> >>>>>>   drivers/nvmem/microchip-otpc.c | 1 +
>> >>>>>>   drivers/nvmem/mtk-efuse.c      | 1 +
>> >>>>>>   drivers/nvmem/qcom-spmi-sdam.c | 1 +
>> >>>>>>   drivers/nvmem/qfprom.c         | 1 +
>> >>>>>>   drivers/nvmem/rave-sp-eeprom.c | 1 +
>> >>>>>>   drivers/nvmem/rockchip-efuse.c | 1 +
>> >>>>>>   drivers/nvmem/sc27xx-efuse.c   | 1 +
>> >>>>>>   drivers/nvmem/sprd-efuse.c     | 1 +
>> >>>>>>   drivers/nvmem/stm32-romem.c    | 1 +
>> >>>>>>   drivers/nvmem/sunplus-ocotp.c  | 1 +
>> >>>>>>   drivers/nvmem/sunxi_sid.c      | 1 +
>> >>>>>>   drivers/nvmem/uniphier-efuse.c | 1 +
>> >>>>>>   drivers/nvmem/zynqmp_nvmem.c   | 1 +
>> >>>>>>   drivers/rtc/nvmem.c            | 1 +
>> >>>>>>   drivers/w1/slaves/w1_ds250x.c  | 1 +
>> >>>>>>   include/linux/nvmem-provider.h | 2 ++
>> >>>>>>   23 files changed, 29 insertions(+), 3 deletions(-)
>> >>>>>>>> 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");
>> >>>>>
>> >>>>> I am wondering how mtd specific this is? For me all OF nodes containing
>> >>>>> the nvmem-cells compatible should be treated as cells providers and
>> >>>>> populate nvmem cells as for each children.
>> >>>>>
>> >>>>> Why don't we just check for this compatible to be present? in
>> >>>>> nvmem_add_cells_from_of() ? And if not we just skip the operation.
>> >>>>>
>> >>>>> This way we still follow the bindings (even though using nvmem-cells in
>> >>>>> the compatible property to require cells population was a mistake in
>> >>>>> the first place, as discussed in the devlink thread recently) but there
>> >>>>> is no need for a per-driver config option?
>> >>>>>> This isn't mtd specific. Please check this patch for all occurrences >> of
>> >>>> the:
>> >>>> use_fixed_of_cells = true
>> >>>>>> The very first one: drivers/nvmem/apple-efuses.c driver for the
>> >>>> "apple,efuses" binding. That binding supports fixed OF cells, see:
>> >>>> Documentation/devicetree/bindings/nvmem/apple,efuses.yaml
>> >>>
>> >>> I'm saying: based on what has been enforced so far, I would expect all
>> >>> fixed cell providers to come with nvmem-cells as compatible, no?
>> >>>
>> >>> If that's the case we could use that as a common denominator?
>> >>
>> >> Sorry, I don't get it. Have you checked
>> >> Documentation/devicetree/bindings/nvmem/apple,efuses.yaml
>> >> ?
>> >>
>> >> It's a NVMEM provied binding with fixed cells that doesn't use
>> >> nvmem-cells as compatible. There are many more.
>> >
>> > Oh yeah you're right, I'm mixing things. Well I guess you're right
>> > then, it's such a mess, we have to tell the core the parsing method.
>> >
>> > So maybe another question: do we have other situations than mtd which
>> > sometimes expect the nvmem core to parse the OF nodes to populate cells,
>> > and sometimes not?
>> 
>> I'm not aware of that. Please also check my patch. The only case I set
>> "use_fixed_of_cells" conditionally is mtd code. In other cases it's
>> hardcoded to "true".
>> 
>> 
>> > Also, what about "of_children_are_cells" ? Because actually in most
>> > cases it's a "fixed of cell", so I don't find the current naming
>> > descriptive enough for something so touchy.
>> 
>> That would be just incorrect because this new config property
>> ("use_fixed_of_cells") is only about FIXED cells.
>> 
>> There are cases of OF children being cells but NOT being fixed cells.
>> They should NOT be parsed by the nvmem_add_cells_from_of().
>> 
>> Example:
>> a607a850ba1f ("dt-bindings: nvmem: u-boot,env: add basic NVMEM cells")
>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=a607a850ba1fa966cbb035544c1588e24a6307df
> 
> This is backwards. That's why layouts have been proposed: having
> a clear framework were the nvmem core should or should not play with
> the OF children nodes. If each binding is different, you end up with
> the mess we have today, where nobody knows how to properly
> populate the cells.
> 
> Anyway, it's not a big deal either, if the cells are empty we can
> easily check that and have *yet another* specific case in the core.
> 
>> So that would result in U-Boot env:
>> 1. Having OF children nodes being cells
>> 2. Setting "of_children_are_cells" to false (counter-intuitive) to 
>> avoid nvmem_add_cells_from_of()
> 
> Agreed. So what about config.ignore_of_children?
> - mtd sets this to !is_compatible(nvmem-cells)
> - nobody else touches it (the core don't try to parse the cell if it's
>   empty so U-Boot env driver works)

"ignore_of_children" would have opposite (reversed) meaning to the
"use_fixed_of_cells":
1. By default it would be 0 / false
2. By default NVMEM code would NOT ignore OF children nodes

That is what I actually *don't* want.

Having NVMEM core look for fixed cells in device node is undesirable.
I want that feature to be off by default. I want devices to have to
enable it explicitly only when it's needed.
  

Hi Rafał,

rafal@milecki.pl wrote on Thu, 09 Mar 2023 09:39:54 +0100:

> On 2023-03-09 09:34, Miquel Raynal wrote:
> > Hi Rafał,
> > 
> > zajec5@gmail.com wrote on Thu, 9 Mar 2023 07:56:05 +0100:
> >   
> >> On 8.03.2023 19:31, Miquel Raynal wrote:  
> >> > Hi Rafał,
> >> >
> >> > rafal@milecki.pl wrote on Wed, 08 Mar 2023 19:12:32 +0100:
> >> >  
> >> >> On 2023-03-08 19:06, Miquel Raynal wrote:  
> >> >>> Hi Rafał,
> >> >>>
> >> >>> rafal@milecki.pl wrote on Wed, 08 Mar 2023 17:55:46 +0100:  
> >> >>>    >>>> On 2023-03-08 17:34, Miquel Raynal wrote:  
> >> >>>>> Hi Rafał,
> >> >>>>>
> >> >>>>> zajec5@gmail.com wrote on Fri, 24 Feb 2023 08:29:03 +0100:  
> >> >>>>>   >>>>>> 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).  
> >> >>>>>>>> This behaviour has actually caused a problem already with the MTD  
> >> >>>>>> subsystem. MTD subpartitions were incorrectly treated as NVMEM cells.  
> >> >>>>>
> >> >>>>> That's true, but I expect this to be really MTD specific.
> >> >>>>>
> >> >>>>> A concrete proposal below.  
> >> >>>>>   >>>>>> Also with upcoming support for NVMEM layouts no new binding or driver  
> >> >>>>>> should support fixed cells defined in device node.  
> >> >>>>>
> >> >>>>> I'm not sure I agree with this statement. We are not preventing new
> >> >>>>> binding/driver to use fixed cells, or...? We offer a new way to expose
> >> >>>>> nvmem cells with another way than "fixed-offset" and "fixed-size" OF
> >> >>>>> nodes.  
> >> >>>>>>   From what I understood all new NVMEM bindings should have cells >> defined  
> >> >>>> in the nvmem-layout { } node. That's what I mean by saying they should
> >> >>>> not be defined in device node (but its "nvmem-layout" instead).  
> >> >>>
> >> >>> Layouts are just another possibility, either you user the nvmem-cells
> >> >>> compatible and produce nvmem cells with fixed OF nodes, or you use the
> >> >>> nvmem-layout container. I don't think all new bindings should have
> >> >>> cells in layouts. It depends if the content is static or not.  
> >> >>>    >>>>>> Solve this by modifying drivers for bindings that support specifying  
> >> >>>>>> fixed NVMEM cells in DT. Make them explicitly tell NVMEM subsystem to
> >> >>>>>> read cells from DT.  
> >> >>>>>>>> It wasn't clear (to me) if rtc and w1 code actually uses fixed cells. >> I  
> >> >>>>>> enabled them to don't risk any breakage.  
> >> >>>>>>>> Signed-off-by: Rafał Miłecki <rafal@milecki.pl>  
> >> >>>>>> [for drivers/nvmem/meson-{efuse,mx-efuse}.c]
> >> >>>>>> Acked-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
> >> >>>>>> ---
> >> >>>>>> V2: Fix stm32-romem.c typo breaking its compilation
> >> >>>>>>      Pick Martin's Acked-by
> >> >>>>>>      Add paragraph about layouts deprecating use_fixed_of_cells
> >> >>>>>> ---
> >> >>>>>>   drivers/mtd/mtdcore.c          | 2 ++
> >> >>>>>>   drivers/nvmem/apple-efuses.c   | 1 +
> >> >>>>>>   drivers/nvmem/core.c           | 8 +++++---
> >> >>>>>>   drivers/nvmem/imx-ocotp-scu.c  | 1 +
> >> >>>>>>   drivers/nvmem/imx-ocotp.c      | 1 +
> >> >>>>>>   drivers/nvmem/meson-efuse.c    | 1 +
> >> >>>>>>   drivers/nvmem/meson-mx-efuse.c | 1 +
> >> >>>>>>   drivers/nvmem/microchip-otpc.c | 1 +
> >> >>>>>>   drivers/nvmem/mtk-efuse.c      | 1 +
> >> >>>>>>   drivers/nvmem/qcom-spmi-sdam.c | 1 +
> >> >>>>>>   drivers/nvmem/qfprom.c         | 1 +
> >> >>>>>>   drivers/nvmem/rave-sp-eeprom.c | 1 +
> >> >>>>>>   drivers/nvmem/rockchip-efuse.c | 1 +
> >> >>>>>>   drivers/nvmem/sc27xx-efuse.c   | 1 +
> >> >>>>>>   drivers/nvmem/sprd-efuse.c     | 1 +
> >> >>>>>>   drivers/nvmem/stm32-romem.c    | 1 +
> >> >>>>>>   drivers/nvmem/sunplus-ocotp.c  | 1 +
> >> >>>>>>   drivers/nvmem/sunxi_sid.c      | 1 +
> >> >>>>>>   drivers/nvmem/uniphier-efuse.c | 1 +
> >> >>>>>>   drivers/nvmem/zynqmp_nvmem.c   | 1 +
> >> >>>>>>   drivers/rtc/nvmem.c            | 1 +
> >> >>>>>>   drivers/w1/slaves/w1_ds250x.c  | 1 +
> >> >>>>>>   include/linux/nvmem-provider.h | 2 ++
> >> >>>>>>   23 files changed, 29 insertions(+), 3 deletions(-)  
> >> >>>>>>>> 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");  
> >> >>>>>
> >> >>>>> I am wondering how mtd specific this is? For me all OF nodes containing
> >> >>>>> the nvmem-cells compatible should be treated as cells providers and
> >> >>>>> populate nvmem cells as for each children.
> >> >>>>>
> >> >>>>> Why don't we just check for this compatible to be present? in
> >> >>>>> nvmem_add_cells_from_of() ? And if not we just skip the operation.
> >> >>>>>
> >> >>>>> This way we still follow the bindings (even though using nvmem-cells in
> >> >>>>> the compatible property to require cells population was a mistake in
> >> >>>>> the first place, as discussed in the devlink thread recently) but there
> >> >>>>> is no need for a per-driver config option?  
> >> >>>>>> This isn't mtd specific. Please check this patch for all occurrences >> of  
> >> >>>> the:
> >> >>>> use_fixed_of_cells = true  
> >> >>>>>> The very first one: drivers/nvmem/apple-efuses.c driver for the  
> >> >>>> "apple,efuses" binding. That binding supports fixed OF cells, see:
> >> >>>> Documentation/devicetree/bindings/nvmem/apple,efuses.yaml  
> >> >>>
> >> >>> I'm saying: based on what has been enforced so far, I would expect all
> >> >>> fixed cell providers to come with nvmem-cells as compatible, no?
> >> >>>
> >> >>> If that's the case we could use that as a common denominator?  
> >> >>
> >> >> Sorry, I don't get it. Have you checked
> >> >> Documentation/devicetree/bindings/nvmem/apple,efuses.yaml
> >> >> ?
> >> >>
> >> >> It's a NVMEM provied binding with fixed cells that doesn't use
> >> >> nvmem-cells as compatible. There are many more.  
> >> >
> >> > Oh yeah you're right, I'm mixing things. Well I guess you're right
> >> > then, it's such a mess, we have to tell the core the parsing method.
> >> >
> >> > So maybe another question: do we have other situations than mtd which
> >> > sometimes expect the nvmem core to parse the OF nodes to populate cells,
> >> > and sometimes not?  
> >> >> I'm not aware of that. Please also check my patch. The only case I set  
> >> "use_fixed_of_cells" conditionally is mtd code. In other cases it's
> >> hardcoded to "true".  
> >> >> >> > Also, what about "of_children_are_cells" ? Because actually in most  
> >> > cases it's a "fixed of cell", so I don't find the current naming
> >> > descriptive enough for something so touchy.  
> >> >> That would be just incorrect because this new config property  
> >> ("use_fixed_of_cells") is only about FIXED cells.  
> >> >> There are cases of OF children being cells but NOT being fixed cells.  
> >> They should NOT be parsed by the nvmem_add_cells_from_of().  
> >> >> Example:  
> >> a607a850ba1f ("dt-bindings: nvmem: u-boot,env: add basic NVMEM cells")
> >> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=a607a850ba1fa966cbb035544c1588e24a6307df  
> > 
> > This is backwards. That's why layouts have been proposed: having
> > a clear framework were the nvmem core should or should not play with
> > the OF children nodes. If each binding is different, you end up with
> > the mess we have today, where nobody knows how to properly
> > populate the cells.
> > 
> > Anyway, it's not a big deal either, if the cells are empty we can
> > easily check that and have *yet another* specific case in the core.
> >   
> >> So that would result in U-Boot env:
> >> 1. Having OF children nodes being cells
> >> 2. Setting "of_children_are_cells" to false (counter-intuitive) to >> avoid nvmem_add_cells_from_of()  
> > 
> > Agreed. So what about config.ignore_of_children?
> > - mtd sets this to !is_compatible(nvmem-cells)
> > - nobody else touches it (the core don't try to parse the cell if it's
> >   empty so U-Boot env driver works)  
> 
> "ignore_of_children" would have opposite (reversed) meaning to the
> "use_fixed_of_cells":
> 1. By default it would be 0 / false
> 2. By default NVMEM code would NOT ignore OF children nodes
> 
> That is what I actually *don't* want.
> 
> Having NVMEM core look for fixed cells in device node is undesirable.

Is it?

I think this is the standard case. Most of the time fixed cells are
the simplest and most direct approach. I don't get why we would like
to prevent it at some point? Only the more advanced stuff that does not
fit the purpose of fixed OF cells should go through layouts.

> I want that feature to be off by default. I want devices to have to
> enable it explicitly only when it's needed.

Well, that's exactly the opposite of what nvmem does right now, that's
maybe why reviewers don't get the interest of the current
implementation: it has many impacts on users which should not be
impacted just because we messed with mtd.

Thanks,
Miquèl
  

On 24/02/2023 07:29, Rafał Miłecki 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).

> 
> This behaviour has actually caused a problem already with the MTD
> subsystem. MTD subpartitions were incorrectly treated as NVMEM cells.
> 
> Also with upcoming support for NVMEM layouts no new binding or driver
> should support fixed cells defined in device node.

This is not very clear, are you saying that we should not support fixed 
cells? If that is the case then you are proabably taking this in wrong 
direction. nvmem was built based on the fact that drivers can read from 
a fixed offsets. Dynamic cells is something very new, that does not mean 
that we should ditch fixed cells support in dt.

> 
> Solve this by modifying drivers for bindings that support specifying
> fixed NVMEM cells in DT. Make them explicitly tell NVMEM subsystem to
> read cells from DT.

Shouldn't this be opposite, let the new providers tell that cells are 
created at runtime?

or even better if there is a way to detect if we can set this flag 
dynamically based on layout/post-processing configuration.

that should be much cleaner approch.

note, I have not read all the review discussions emails, so please 
ignore any duplicate comments.

--srini

> 
> It wasn't clear (to me) if rtc and w1 code actually uses fixed cells. I
> enabled them to don't risk any breakage.
> 
> Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
> [for drivers/nvmem/meson-{efuse,mx-efuse}.c]
> Acked-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
> ---
> V2: Fix stm32-romem.c typo breaking its compilation
>      Pick Martin's Acked-by
>      Add paragraph about layouts deprecating use_fixed_of_cells
> ---
>   drivers/mtd/mtdcore.c          | 2 ++
>   drivers/nvmem/apple-efuses.c   | 1 +
>   drivers/nvmem/core.c           | 8 +++++---
>   drivers/nvmem/imx-ocotp-scu.c  | 1 +
>   drivers/nvmem/imx-ocotp.c      | 1 +
>   drivers/nvmem/meson-efuse.c    | 1 +
>   drivers/nvmem/meson-mx-efuse.c | 1 +
>   drivers/nvmem/microchip-otpc.c | 1 +
>   drivers/nvmem/mtk-efuse.c      | 1 +
>   drivers/nvmem/qcom-spmi-sdam.c | 1 +
>   drivers/nvmem/qfprom.c         | 1 +
>   drivers/nvmem/rave-sp-eeprom.c | 1 +
>   drivers/nvmem/rockchip-efuse.c | 1 +
>   drivers/nvmem/sc27xx-efuse.c   | 1 +
>   drivers/nvmem/sprd-efuse.c     | 1 +
>   drivers/nvmem/stm32-romem.c    | 1 +
>   drivers/nvmem/sunplus-ocotp.c  | 1 +
>   drivers/nvmem/sunxi_sid.c      | 1 +
>   drivers/nvmem/uniphier-efuse.c | 1 +
>   drivers/nvmem/zynqmp_nvmem.c   | 1 +
>   drivers/rtc/nvmem.c            | 1 +
>   drivers/w1/slaves/w1_ds250x.c  | 1 +
>   include/linux/nvmem-provider.h | 2 ++
>   23 files changed, 29 insertions(+), 3 deletions(-)
> 
> 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..a6fc43acb797 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->cfg.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;
  

[as this mentions  nvmem layouts it would have been nice to include me]

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

Why is that? It still makes sense, doesn't it?

> 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).

What false discoveries?

> This behaviour has actually caused a problem already with the MTD
> subsystem. MTD subpartitions were incorrectly treated as NVMEM cells.

But this is solved, correct?

> Also with upcoming support for NVMEM layouts no new binding or driver
> should support fixed cells defined in device node.

How would you support older device trees with newer kernels or the
other way around? I'm not sure I get your motivation to drop handling
the fixed cells.

> Solve this by modifying drivers for bindings that support specifying
> fixed NVMEM cells in DT. Make them explicitly tell NVMEM subsystem to
> read cells from DT.

How can a driver know when there are fixed cells and when not? IOW.
only new ones can be affected.

If you want to get rid of the schema for *new* drivers then what
about having a new child node, something like "nvmem-fixed-cells",
similar to "nvmem-layout".

And then you'd tell the new drivers to use the new-style dt binding. But
there are no new drivers yet. So I'm still not sure I get your motivation.

-michael