Hi all,
First thanks to everybody for the feedback on the first series, I implemented
most of the suggestions in this new version. The biggest change is to
directly integrate in the MTD core instead of using notifiers which allow us
to simplify the code quiet a bit.
During the discussion 2 points showed up, reworking the NVMEM binding and
improving the MTD notifiers. Both are good points but not relevant for
this series, so I left that out for another time.
The NVMEM device ID bug fix found in the first series has already been
applied so I didn't include it again.
Alban (2):
doc: bindings: Add bindings documentation for mtd nvmem
mtd: Add support for reading MTD devices via the nvmem API
.../devicetree/bindings/nvmem/mtd-nvmem.txt | 33 ++++++++++
drivers/mtd/Kconfig | 9 +++
drivers/mtd/Makefile | 1 +
drivers/mtd/mtdcore.c | 13 ++++
drivers/mtd/mtdnvmem.c | 72 ++++++++++++++++++++++
drivers/mtd/mtdnvmem.h | 25 ++++++++
include/linux/mtd/mtd.h | 4 ++
7 files changed, 157 insertions(+)
create mode 100644 Documentation/devicetree/bindings/nvmem/mtd-nvmem.txt
create mode 100644 drivers/mtd/mtdnvmem.c
create mode 100644 drivers/mtd/mtdnvmem.h
--
2.7.4
Config data for drivers, like MAC addresses, is often stored in MTD.
Add a binding that define how such data storage can be represented in
device tree.
Signed-off-by: Alban <[email protected]>
---
Changelog:
v2: * Added a "Required properties" section with the nvmem-provider
property
---
.../devicetree/bindings/nvmem/mtd-nvmem.txt | 33 ++++++++++++++++++++++
1 file changed, 33 insertions(+)
create mode 100644 Documentation/devicetree/bindings/nvmem/mtd-nvmem.txt
diff --git a/Documentation/devicetree/bindings/nvmem/mtd-nvmem.txt b/Documentation/devicetree/bindings/nvmem/mtd-nvmem.txt
new file mode 100644
index 0000000..8ed25e6
--- /dev/null
+++ b/Documentation/devicetree/bindings/nvmem/mtd-nvmem.txt
@@ -0,0 +1,33 @@
+= NVMEM in MTD =
+
+Config data for drivers, like MAC addresses, is often stored in MTD.
+This binding define how such data storage can be represented in device tree.
+
+An MTD can be defined as an NVMEM provider by adding the `nvmem-provider`
+property to their node. Data cells can then be defined as child nodes
+of the partition as defined in nvmem.txt.
+
+Required properties:
+nvmem-provider: Indicate that the device should be registered as
+ NVMEM provider
+
+Example:
+
+ flash@0 {
+ ...
+
+ partition@2 {
+ label = "art";
+ reg = <0x7F0000 0x010000>;
+ read-only;
+
+ nvmem-provider;
+ #address-cells = <1>;
+ #size-cells = <1>;
+
+ eeprom@1000 {
+ label = "wmac-eeprom";
+ reg = <0x1000 0x1000>;
+ };
+ };
+ };
--
2.7.4
Allow drivers that use the nvmem API to read data stored on MTD devices.
Add an option to the MTD core that allow registering the MTD as
read-only NVMEM providers.
Signed-off-by: Alban <[email protected]>
---
Changelog:
v2: * Moved to the MTD core instead of using notifiers
* Fixed the Kconfig description
---
drivers/mtd/Kconfig | 9 +++++++
drivers/mtd/Makefile | 1 +
drivers/mtd/mtdcore.c | 13 +++++++++
drivers/mtd/mtdnvmem.c | 72 +++++++++++++++++++++++++++++++++++++++++++++++++
drivers/mtd/mtdnvmem.h | 25 +++++++++++++++++
include/linux/mtd/mtd.h | 4 +++
6 files changed, 124 insertions(+)
create mode 100644 drivers/mtd/mtdnvmem.c
create mode 100644 drivers/mtd/mtdnvmem.h
diff --git a/drivers/mtd/Kconfig b/drivers/mtd/Kconfig
index e83a279..5a34c6a 100644
--- a/drivers/mtd/Kconfig
+++ b/drivers/mtd/Kconfig
@@ -322,6 +322,15 @@ config MTD_PARTITIONED_MASTER
the parent of the partition device be the master device, rather than
what lies behind the master.
+config MTD_NVMEM
+ bool "Register MTD devices as NVMEM providers"
+ default y
+ depends on NVMEM || COMPILE_TEST
+ help
+ Provides support for reading config data from MTD devices. This can
+ be used by drivers to read device specific data such as MAC addresses
+ or calibration results.
+
source "drivers/mtd/chips/Kconfig"
source "drivers/mtd/maps/Kconfig"
diff --git a/drivers/mtd/Makefile b/drivers/mtd/Makefile
index 99bb9a1..879a542 100644
--- a/drivers/mtd/Makefile
+++ b/drivers/mtd/Makefile
@@ -5,6 +5,7 @@
# Core functionality.
obj-$(CONFIG_MTD) += mtd.o
mtd-y := mtdcore.o mtdsuper.o mtdconcat.o mtdpart.o mtdchar.o
+mtd-$(CONFIG_MTD_NVMEM) += mtdnvmem.o
obj-$(CONFIG_MTD_OF_PARTS) += ofpart.o
obj-$(CONFIG_MTD_REDBOOT_PARTS) += redboot.o
diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
index 66a9ded..bb88997 100644
--- a/drivers/mtd/mtdcore.c
+++ b/drivers/mtd/mtdcore.c
@@ -45,6 +45,7 @@
#include <linux/mtd/partitions.h>
#include "mtdcore.h"
+#include "mtdnvmem.h"
static struct backing_dev_info *mtd_bdi;
@@ -554,6 +555,11 @@ int add_mtd_device(struct mtd_info *mtd)
if (error)
goto fail_added;
+ /* Add the nvmem provider */
+ error = mtd_nvmem_add(mtd);
+ if (error)
+ goto fail_nvmem_add;
+
device_create(&mtd_class, mtd->dev.parent, MTD_DEVT(i) + 1, NULL,
"mtd%dro", i);
@@ -571,6 +577,8 @@ int add_mtd_device(struct mtd_info *mtd)
__module_get(THIS_MODULE);
return 0;
+fail_nvmem_add:
+ device_unregister(&mtd->dev);
fail_added:
of_node_put(mtd_get_of_node(mtd));
idr_remove(&mtd_idr, i);
@@ -611,6 +619,11 @@ int del_mtd_device(struct mtd_info *mtd)
mtd->index, mtd->name, mtd->usecount);
ret = -EBUSY;
} else {
+ /* Try to remove the NVMEM provider */
+ ret = mtd_nvmem_remove(mtd);
+ if (ret)
+ goto out_error;
+
device_unregister(&mtd->dev);
idr_remove(&mtd_idr, mtd->index);
diff --git a/drivers/mtd/mtdnvmem.c b/drivers/mtd/mtdnvmem.c
new file mode 100644
index 0000000..d6bc402
--- /dev/null
+++ b/drivers/mtd/mtdnvmem.c
@@ -0,0 +1,72 @@
+/*
+ * Copyright (C) 2017 Alban Bedel <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ */
+
+#include <linux/module.h>
+#include <linux/mtd/mtd.h>
+#include <linux/nvmem-provider.h>
+#include <linux/of.h>
+
+static int mtd_nvmem_reg_read(void *priv, unsigned int offset,
+ void *val, size_t bytes)
+{
+ struct mtd_info *mtd = priv;
+ size_t retlen;
+ int err;
+
+ err = mtd_read(mtd, offset, bytes, &retlen, val);
+ if (err && err != -EUCLEAN)
+ return err;
+
+ return retlen == bytes ? 0 : -EIO;
+}
+
+int mtd_nvmem_add(struct mtd_info *mtd)
+{
+ struct device_node *np = dev_of_node(&mtd->dev);
+ struct nvmem_config config = {};
+
+ /* OF devices must have the nvmem-provider property */
+ if (np && !of_property_read_bool(np, "nvmem-provider"))
+ return 0;
+
+ config.dev = &mtd->dev;
+ config.owner = THIS_MODULE;
+ config.reg_read = mtd_nvmem_reg_read;
+ config.size = mtd->size;
+ config.word_size = 1;
+ config.stride = 1;
+ config.read_only = true;
+ config.priv = mtd;
+
+ mtd->nvmem = nvmem_register(&config);
+ if (IS_ERR(mtd->nvmem)) {
+ dev_err(&mtd->dev, "Failed to register NVMEM device\n");
+ return PTR_ERR(mtd->nvmem);
+ }
+
+ return 0;
+}
+
+int mtd_nvmem_remove(struct mtd_info *mtd)
+{
+ int ret;
+
+ if (!mtd->nvmem)
+ return 0;
+
+ ret = nvmem_unregister(mtd->nvmem);
+ if (ret)
+ dev_err(&mtd->dev, "Failed to unregister NVMEM device\n");
+
+ return ret;
+}
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Alban Bedel <[email protected]>");
+MODULE_DESCRIPTION("Driver to read config data from MTD devices");
diff --git a/drivers/mtd/mtdnvmem.h b/drivers/mtd/mtdnvmem.h
new file mode 100644
index 0000000..a49d8bd
--- /dev/null
+++ b/drivers/mtd/mtdnvmem.h
@@ -0,0 +1,25 @@
+/*
+ * Copyright (C) 2017 Alban Bedel <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ */
+#ifndef MTD_NVMEM_H
+#define MTD_NVMEM_H 1
+
+struct mtd_info;
+
+#ifdef CONFIG_MTD_NVMEM
+int mtd_nvmem_add(struct mtd_info *mtd);
+int mtd_nvmem_remove(struct mtd_info *mtd);
+#else
+static inline int mtd_nvmem_add(struct mtd_info *mtd)
+{ return 0; }
+
+static inline int mtd_nvmem_remove(struct mtd_info *mtd)
+{ return 0; }
+#endif
+
+#endif /* MTD_NVMEM_H */
diff --git a/include/linux/mtd/mtd.h b/include/linux/mtd/mtd.h
index eebdc63..e06c6e6 100644
--- a/include/linux/mtd/mtd.h
+++ b/include/linux/mtd/mtd.h
@@ -205,6 +205,7 @@ struct mtd_pairing_scheme {
};
struct module; /* only needed for owner field in mtd_info */
+struct nvmem_device;
struct mtd_info {
u_char type;
@@ -351,6 +352,9 @@ struct mtd_info {
struct module *owner;
struct device dev;
int usecount;
+#if IS_ENABLED(CONFIG_MTD_NVMEM)
+ struct nvmem_device *nvmem;
+#endif
};
int mtd_ooblayout_ecc(struct mtd_info *mtd, int section,
--
2.7.4
On Tue, 7 Mar 2017 09:26:04 +0100
Alban <[email protected]> wrote:
> Allow drivers that use the nvmem API to read data stored on MTD devices.
> Add an option to the MTD core that allow registering the MTD as
> read-only NVMEM providers.
>
> Signed-off-by: Alban <[email protected]>
> ---
> Changelog:
> v2: * Moved to the MTD core instead of using notifiers
> * Fixed the Kconfig description
> ---
> drivers/mtd/Kconfig | 9 +++++++
> drivers/mtd/Makefile | 1 +
> drivers/mtd/mtdcore.c | 13 +++++++++
> drivers/mtd/mtdnvmem.c | 72 +++++++++++++++++++++++++++++++++++++++++++++++++
> drivers/mtd/mtdnvmem.h | 25 +++++++++++++++++
> include/linux/mtd/mtd.h | 4 +++
> 6 files changed, 124 insertions(+)
> create mode 100644 drivers/mtd/mtdnvmem.c
> create mode 100644 drivers/mtd/mtdnvmem.h
>
> diff --git a/drivers/mtd/Kconfig b/drivers/mtd/Kconfig
> index e83a279..5a34c6a 100644
> --- a/drivers/mtd/Kconfig
> +++ b/drivers/mtd/Kconfig
> @@ -322,6 +322,15 @@ config MTD_PARTITIONED_MASTER
> the parent of the partition device be the master device, rather than
> what lies behind the master.
>
> +config MTD_NVMEM
> + bool "Register MTD devices as NVMEM providers"
> + default y
> + depends on NVMEM || COMPILE_TEST
> + help
> + Provides support for reading config data from MTD devices. This can
> + be used by drivers to read device specific data such as MAC addresses
> + or calibration results.
> +
> source "drivers/mtd/chips/Kconfig"
>
> source "drivers/mtd/maps/Kconfig"
> diff --git a/drivers/mtd/Makefile b/drivers/mtd/Makefile
> index 99bb9a1..879a542 100644
> --- a/drivers/mtd/Makefile
> +++ b/drivers/mtd/Makefile
> @@ -5,6 +5,7 @@
> # Core functionality.
> obj-$(CONFIG_MTD) += mtd.o
> mtd-y := mtdcore.o mtdsuper.o mtdconcat.o mtdpart.o mtdchar.o
> +mtd-$(CONFIG_MTD_NVMEM) += mtdnvmem.o
>
> obj-$(CONFIG_MTD_OF_PARTS) += ofpart.o
> obj-$(CONFIG_MTD_REDBOOT_PARTS) += redboot.o
> diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
> index 66a9ded..bb88997 100644
> --- a/drivers/mtd/mtdcore.c
> +++ b/drivers/mtd/mtdcore.c
> @@ -45,6 +45,7 @@
> #include <linux/mtd/partitions.h>
>
> #include "mtdcore.h"
> +#include "mtdnvmem.h"
>
> static struct backing_dev_info *mtd_bdi;
>
> @@ -554,6 +555,11 @@ int add_mtd_device(struct mtd_info *mtd)
> if (error)
> goto fail_added;
>
> + /* Add the nvmem provider */
> + error = mtd_nvmem_add(mtd);
> + if (error)
> + goto fail_nvmem_add;
> +
> device_create(&mtd_class, mtd->dev.parent, MTD_DEVT(i) + 1, NULL,
> "mtd%dro", i);
>
> @@ -571,6 +577,8 @@ int add_mtd_device(struct mtd_info *mtd)
> __module_get(THIS_MODULE);
> return 0;
>
> +fail_nvmem_add:
> + device_unregister(&mtd->dev);
> fail_added:
> of_node_put(mtd_get_of_node(mtd));
> idr_remove(&mtd_idr, i);
> @@ -611,6 +619,11 @@ int del_mtd_device(struct mtd_info *mtd)
> mtd->index, mtd->name, mtd->usecount);
> ret = -EBUSY;
> } else {
> + /* Try to remove the NVMEM provider */
> + ret = mtd_nvmem_remove(mtd);
> + if (ret)
> + goto out_error;
> +
> device_unregister(&mtd->dev);
>
> idr_remove(&mtd_idr, mtd->index);
> diff --git a/drivers/mtd/mtdnvmem.c b/drivers/mtd/mtdnvmem.c
> new file mode 100644
> index 0000000..d6bc402
> --- /dev/null
> +++ b/drivers/mtd/mtdnvmem.c
> @@ -0,0 +1,72 @@
> +/*
> + * Copyright (C) 2017 Alban Bedel <[email protected]>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + */
> +
> +#include <linux/module.h>
> +#include <linux/mtd/mtd.h>
> +#include <linux/nvmem-provider.h>
> +#include <linux/of.h>
> +
> +static int mtd_nvmem_reg_read(void *priv, unsigned int offset,
> + void *val, size_t bytes)
> +{
> + struct mtd_info *mtd = priv;
> + size_t retlen;
> + int err;
> +
> + err = mtd_read(mtd, offset, bytes, &retlen, val);
> + if (err && err != -EUCLEAN)
> + return err;
> +
> + return retlen == bytes ? 0 : -EIO;
> +}
> +
> +int mtd_nvmem_add(struct mtd_info *mtd)
> +{
> + struct device_node *np = dev_of_node(&mtd->dev);
> + struct nvmem_config config = {};
> +
> + /* OF devices must have the nvmem-provider property */
> + if (np && !of_property_read_bool(np, "nvmem-provider"))
> + return 0;
> +
> + config.dev = &mtd->dev;
> + config.owner = THIS_MODULE;
> + config.reg_read = mtd_nvmem_reg_read;
> + config.size = mtd->size;
> + config.word_size = 1;
> + config.stride = 1;
> + config.read_only = true;
> + config.priv = mtd;
> +
> + mtd->nvmem = nvmem_register(&config);
> + if (IS_ERR(mtd->nvmem)) {
> + dev_err(&mtd->dev, "Failed to register NVMEM device\n");
> + return PTR_ERR(mtd->nvmem);
> + }
> +
> + return 0;
> +}
> +
> +int mtd_nvmem_remove(struct mtd_info *mtd)
> +{
> + int ret;
> +
> + if (!mtd->nvmem)
> + return 0;
> +
> + ret = nvmem_unregister(mtd->nvmem);
> + if (ret)
> + dev_err(&mtd->dev, "Failed to unregister NVMEM device\n");
> +
> + return ret;
> +}
Given the amount of code I wonder if this shouldn't be integrated in
mtdcore.c and unconditionally compiled in.
> +
> +MODULE_LICENSE("GPL");
> +MODULE_AUTHOR("Alban Bedel <[email protected]>");
> +MODULE_DESCRIPTION("Driver to read config data from MTD devices");
> diff --git a/drivers/mtd/mtdnvmem.h b/drivers/mtd/mtdnvmem.h
> new file mode 100644
> index 0000000..a49d8bd
> --- /dev/null
> +++ b/drivers/mtd/mtdnvmem.h
> @@ -0,0 +1,25 @@
> +/*
> + * Copyright (C) 2017 Alban Bedel <[email protected]>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + */
> +#ifndef MTD_NVMEM_H
> +#define MTD_NVMEM_H 1
> +
> +struct mtd_info;
> +
> +#ifdef CONFIG_MTD_NVMEM
> +int mtd_nvmem_add(struct mtd_info *mtd);
> +int mtd_nvmem_remove(struct mtd_info *mtd);
> +#else
> +static inline int mtd_nvmem_add(struct mtd_info *mtd)
> +{ return 0; }
> +
> +static inline int mtd_nvmem_remove(struct mtd_info *mtd)
> +{ return 0; }
> +#endif
> +
> +#endif /* MTD_NVMEM_H */
> diff --git a/include/linux/mtd/mtd.h b/include/linux/mtd/mtd.h
> index eebdc63..e06c6e6 100644
> --- a/include/linux/mtd/mtd.h
> +++ b/include/linux/mtd/mtd.h
> @@ -205,6 +205,7 @@ struct mtd_pairing_scheme {
> };
>
> struct module; /* only needed for owner field in mtd_info */
> +struct nvmem_device;
Include nvmem-provider.h instead of re-defining struct nvmem_device
here.
>
> struct mtd_info {
> u_char type;
> @@ -351,6 +352,9 @@ struct mtd_info {
> struct module *owner;
> struct device dev;
> int usecount;
> +#if IS_ENABLED(CONFIG_MTD_NVMEM)
> + struct nvmem_device *nvmem;
> +#endif
Hm, I don't see any #if IS_ENABLED() or #ifdef CONFIG_ statements in
this file. I know it adds an extra 32/64 bits field for nothing if the
feature is disabled, but how about keeping the struct definition simple
by unconditionally adding the nvmem field.
> };
>
> int mtd_ooblayout_ecc(struct mtd_info *mtd, int section,
On Tue, 7 Mar 2017 09:26:03 +0100
Alban <[email protected]> wrote:
> Config data for drivers, like MAC addresses, is often stored in MTD.
> Add a binding that define how such data storage can be represented in
> device tree.
>
> Signed-off-by: Alban <[email protected]>
> ---
> Changelog:
> v2: * Added a "Required properties" section with the nvmem-provider
> property
> ---
> .../devicetree/bindings/nvmem/mtd-nvmem.txt | 33 ++++++++++++++++++++++
> 1 file changed, 33 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/nvmem/mtd-nvmem.txt
>
> diff --git a/Documentation/devicetree/bindings/nvmem/mtd-nvmem.txt b/Documentation/devicetree/bindings/nvmem/mtd-nvmem.txt
> new file mode 100644
> index 0000000..8ed25e6
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/nvmem/mtd-nvmem.txt
> @@ -0,0 +1,33 @@
> += NVMEM in MTD =
> +
> +Config data for drivers, like MAC addresses, is often stored in MTD.
> +This binding define how such data storage can be represented in device tree.
> +
> +An MTD can be defined as an NVMEM provider by adding the `nvmem-provider`
> +property to their node.
If everyone agrees that this is actually needed, then it should
definitely go in the nvmem binding doc, and we should patch all nvmem
providers to define this property (even if we keep supporting nodes
that are not defining it). I'm not fully convinced yet, but I might be
wrong.
I also think we should take the "nvmem under flash node without partitions"
into account now, or at least have a clear plan on how we want to represent
it.
Something like that?
flash {
partitions {
part@X {
nvmem {
#address-cells = <1>;
#size-cells = <1>;
cell@Y {
};
};
};
};
nvmem {
#address-cells = <1>;
#size-cells = <1>;
cell@X {
};
};
};
Note that patching nvmem core to support the subnode case should be
pretty easy (see below).
--->8---
diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
index 408b521ee520..507c6190505b 100644
--- a/drivers/nvmem/core.c
+++ b/drivers/nvmem/core.c
@@ -465,7 +465,7 @@ struct nvmem_device *nvmem_register(const struct nvmem_config *config)
nvmem->priv = config->priv;
nvmem->reg_read = config->reg_read;
nvmem->reg_write = config->reg_write;
- np = config->dev->of_node;
+ np = config->of_node ? : config->dev->of_node;
nvmem->dev.of_node = np;
dev_set_name(&nvmem->dev, "%s%d",
config->name ? : "nvmem", config->id);
diff --git a/include/linux/nvmem-provider.h b/include/linux/nvmem-provider.h
index cd93416d762e..ec2f5116d62d 100644
--- a/include/linux/nvmem-provider.h
+++ b/include/linux/nvmem-provider.h
@@ -21,6 +21,7 @@ typedef int (*nvmem_reg_write_t)(void *priv, unsigned int offset,
struct nvmem_config {
struct device *dev;
+ struct device_node *of_node;
const char *name;
int id;
struct module *owner;
On Tue, 7 Mar 2017 22:01:07 +0100
Boris Brezillon <[email protected]> wrote:
> On Tue, 7 Mar 2017 09:26:03 +0100
> Alban <[email protected]> wrote:
>
> > Config data for drivers, like MAC addresses, is often stored in MTD.
> > Add a binding that define how such data storage can be represented in
> > device tree.
> >
> > Signed-off-by: Alban <[email protected]>
> > ---
> > Changelog:
> > v2: * Added a "Required properties" section with the nvmem-provider
> > property
> > ---
> > .../devicetree/bindings/nvmem/mtd-nvmem.txt | 33 ++++++++++++++++++++++
> > 1 file changed, 33 insertions(+)
> > create mode 100644 Documentation/devicetree/bindings/nvmem/mtd-nvmem.txt
> >
> > diff --git a/Documentation/devicetree/bindings/nvmem/mtd-nvmem.txt b/Documentation/devicetree/bindings/nvmem/mtd-nvmem.txt
> > new file mode 100644
> > index 0000000..8ed25e6
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/nvmem/mtd-nvmem.txt
> > @@ -0,0 +1,33 @@
> > += NVMEM in MTD =
> > +
> > +Config data for drivers, like MAC addresses, is often stored in MTD.
> > +This binding define how such data storage can be represented in device tree.
> > +
> > +An MTD can be defined as an NVMEM provider by adding the `nvmem-provider`
> > +property to their node.
>
> If everyone agrees that this is actually needed, then it should
> definitely go in the nvmem binding doc, and we should patch all nvmem
> providers to define this property (even if we keep supporting nodes
> that are not defining it). I'm not fully convinced yet, but I might be
> wrong.
I really like to hear what the DT people think about this.
> I also think we should take the "nvmem under flash node without partitions"
> into account now, or at least have a clear plan on how we want to represent
> it.
>
> Something like that?
Yes, but with the following extras:
> flash {
nvmem-provider;
> partitions {
> part@X {
> nvmem {
compatible = "nvmem-cells";
> #address-cells = <1>;
> #size-cells = <1>;
>
> cell@Y {
> };
> };
> };
> };
>
> nvmem {
compatible = "nvmem-cells";
> #address-cells = <1>;
> #size-cells = <1>;
>
> cell@X {
> };
> };
> };
>
> Note that patching nvmem core to support the subnode case should be
> pretty easy (see below).
This shouldn't be needed as nothing would change for the NVMEM devices,
what could be added is a check for the "nvmem-provider" property.
To support the proposed binding we would only need a minor change to
of_nvmem_cell_get():
diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
index 408b521ee520..6231ea27c9f4 100644
--- a/drivers/nvmem/core.c
+++ b/drivers/nvmem/core.c
@@ -444,6 +444,10 @@ struct nvmem_device *nvmem_register(const struct nvmem_config *config)
if (!config->dev)
return ERR_PTR(-EINVAL);
+ if (config->dev->of_node &&
+ !of_property_read_bool(config->dev->of_node, "nvmem-provider"))
+ return ERR_PTR(-ENODEV);
+
nvmem = kzalloc(sizeof(*nvmem), GFP_KERNEL);
if (!nvmem)
return ERR_PTR(-ENOMEM);
@@ -777,6 +781,15 @@ struct nvmem_cell *of_nvmem_cell_get(struct device_node *np,
if (!nvmem_np)
return ERR_PTR(-EINVAL);
+ /* handle the new cell binding */
+ if (of_device_is_compatible(nvmem_np, "nvmem-cells")) {
+ nvmem_np = of_get_next_parent(cell_np);
+ if (!nvmem_np)
+ return ERR_PTR(-EINVAL);
+ if (!of_property_read_bool(nvmem_np, "nvmem-provider"))
+ return ERR_PTR(-ENODEV);
+ }
+
nvmem = __nvmem_device_get(nvmem_np, NULL, NULL);
if (IS_ERR(nvmem))
return ERR_CAST(nvmem);
> --->8---
> diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
> index 408b521ee520..507c6190505b 100644
> --- a/drivers/nvmem/core.c
> +++ b/drivers/nvmem/core.c
> @@ -465,7 +465,7 @@ struct nvmem_device *nvmem_register(const struct nvmem_config *config)
> nvmem->priv = config->priv;
> nvmem->reg_read = config->reg_read;
> nvmem->reg_write = config->reg_write;
> - np = config->dev->of_node;
> + np = config->of_node ? : config->dev->of_node;
> nvmem->dev.of_node = np;
> dev_set_name(&nvmem->dev, "%s%d",
> config->name ? : "nvmem", config->id);
> diff --git a/include/linux/nvmem-provider.h b/include/linux/nvmem-provider.h
> index cd93416d762e..ec2f5116d62d 100644
> --- a/include/linux/nvmem-provider.h
> +++ b/include/linux/nvmem-provider.h
> @@ -21,6 +21,7 @@ typedef int (*nvmem_reg_write_t)(void *priv, unsigned int offset,
>
> struct nvmem_config {
> struct device *dev;
> + struct device_node *of_node;
> const char *name;
> int id;
> struct module *owner;
Hi Alban,
On Wed, 8 Mar 2017 16:20:01 +0100
Alban <[email protected]> wrote:
> On Tue, 7 Mar 2017 22:01:07 +0100
> Boris Brezillon <[email protected]> wrote:
>
> > On Tue, 7 Mar 2017 09:26:03 +0100
> > Alban <[email protected]> wrote:
> >
> > > Config data for drivers, like MAC addresses, is often stored in MTD.
> > > Add a binding that define how such data storage can be represented in
> > > device tree.
> > >
> > > Signed-off-by: Alban <[email protected]>
> > > ---
> > > Changelog:
> > > v2: * Added a "Required properties" section with the nvmem-provider
> > > property
> > > ---
> > > .../devicetree/bindings/nvmem/mtd-nvmem.txt | 33 ++++++++++++++++++++++
> > > 1 file changed, 33 insertions(+)
> > > create mode 100644 Documentation/devicetree/bindings/nvmem/mtd-nvmem.txt
> > >
> > > diff --git a/Documentation/devicetree/bindings/nvmem/mtd-nvmem.txt b/Documentation/devicetree/bindings/nvmem/mtd-nvmem.txt
> > > new file mode 100644
> > > index 0000000..8ed25e6
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/nvmem/mtd-nvmem.txt
> > > @@ -0,0 +1,33 @@
> > > += NVMEM in MTD =
> > > +
> > > +Config data for drivers, like MAC addresses, is often stored in MTD.
> > > +This binding define how such data storage can be represented in device tree.
> > > +
> > > +An MTD can be defined as an NVMEM provider by adding the `nvmem-provider`
> > > +property to their node.
> >
> > If everyone agrees that this is actually needed, then it should
> > definitely go in the nvmem binding doc, and we should patch all nvmem
> > providers to define this property (even if we keep supporting nodes
> > that are not defining it). I'm not fully convinced yet, but I might be
> > wrong.
>
> I really like to hear what the DT people think about this.
That was the plan.
>
> > I also think we should take the "nvmem under flash node without partitions"
> > into account now, or at least have a clear plan on how we want to represent
> > it.
> >
> > Something like that?
>
> Yes, but with the following extras:
>
> > flash {
> nvmem-provider;
> > partitions {
> > part@X {
> > nvmem {
> compatible = "nvmem-cells";
> > #address-cells = <1>;
> > #size-cells = <1>;
> >
> > cell@Y {
> > };
> > };
> > };
> > };
> >
> > nvmem {
> compatible = "nvmem-cells";
> > #address-cells = <1>;
> > #size-cells = <1>;
> >
> > cell@X {
> > };
> > };
> > };
> >
> > Note that patching nvmem core to support the subnode case should be
> > pretty easy (see below).
>
> This shouldn't be needed as nothing would change for the NVMEM devices,
> what could be added is a check for the "nvmem-provider" property.
> To support the proposed binding we would only need a minor change to
> of_nvmem_cell_get():
>
> diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
> index 408b521ee520..6231ea27c9f4 100644
> --- a/drivers/nvmem/core.c
> +++ b/drivers/nvmem/core.c
> @@ -444,6 +444,10 @@ struct nvmem_device *nvmem_register(const struct nvmem_config *config)
> if (!config->dev)
> return ERR_PTR(-EINVAL);
>
> + if (config->dev->of_node &&
> + !of_property_read_bool(config->dev->of_node, "nvmem-provider"))
> + return ERR_PTR(-ENODEV);
> +
> nvmem = kzalloc(sizeof(*nvmem), GFP_KERNEL);
> if (!nvmem)
> return ERR_PTR(-ENOMEM);
> @@ -777,6 +781,15 @@ struct nvmem_cell *of_nvmem_cell_get(struct device_node *np,
> if (!nvmem_np)
> return ERR_PTR(-EINVAL);
>
> + /* handle the new cell binding */
> + if (of_device_is_compatible(nvmem_np, "nvmem-cells")) {
> + nvmem_np = of_get_next_parent(cell_np);
> + if (!nvmem_np)
> + return ERR_PTR(-EINVAL);
> + if (!of_property_read_bool(nvmem_np, "nvmem-provider"))
> + return ERR_PTR(-ENODEV);
> + }
> +
> nvmem = __nvmem_device_get(nvmem_np, NULL, NULL);
> if (IS_ERR(nvmem))
> return ERR_CAST(nvmem);
>
Yep, works too. Let's wait for a DT review, before taking a decision.
Thanks,
Boris
On 03/07/2017 09:26 AM, Alban wrote:
> Config data for drivers, like MAC addresses, is often stored in MTD.
> Add a binding that define how such data storage can be represented in
> device tree.
>
> Signed-off-by: Alban <[email protected]>
> ---
> Changelog:
> v2: * Added a "Required properties" section with the nvmem-provider
> property
> ---
> .../devicetree/bindings/nvmem/mtd-nvmem.txt | 33 ++++++++++++++++++++++
> 1 file changed, 33 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/nvmem/mtd-nvmem.txt
>
> diff --git a/Documentation/devicetree/bindings/nvmem/mtd-nvmem.txt b/Documentation/devicetree/bindings/nvmem/mtd-nvmem.txt
> new file mode 100644
> index 0000000..8ed25e6
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/nvmem/mtd-nvmem.txt
> @@ -0,0 +1,33 @@
> += NVMEM in MTD =
> +
> +Config data for drivers, like MAC addresses, is often stored in MTD.
> +This binding define how such data storage can be represented in device tree.
> +
> +An MTD can be defined as an NVMEM provider by adding the `nvmem-provider`
> +property to their node. Data cells can then be defined as child nodes
> +of the partition as defined in nvmem.txt.
Why don't we just read the data from MTD and be done with it ? What's
the benefit of complicating things by using nvmem ?
> +Required properties:
> +nvmem-provider: Indicate that the device should be registered as
> + NVMEM provider
> +
> +Example:
> +
> + flash@0 {
> + ...
> +
> + partition@2 {
> + label = "art";
> + reg = <0x7F0000 0x010000>;
> + read-only;
> +
> + nvmem-provider;
> + #address-cells = <1>;
> + #size-cells = <1>;
> +
> + eeprom@1000 {
> + label = "wmac-eeprom";
> + reg = <0x1000 0x1000>;
> + };
> + };
> + };
>
--
Best regards,
Marek Vasut
On Thu, Mar 9, 2017 at 7:17 PM, Marek Vasut <[email protected]> wrote:
> On 03/07/2017 09:26 AM, Alban wrote:
>> Config data for drivers, like MAC addresses, is often stored in MTD.
>> Add a binding that define how such data storage can be represented in
>> device tree.
>>
>> Signed-off-by: Alban <[email protected]>
>> ---
>> Changelog:
>> v2: * Added a "Required properties" section with the nvmem-provider
>> property
>> ---
>> .../devicetree/bindings/nvmem/mtd-nvmem.txt | 33 ++++++++++++++++++++++
>> 1 file changed, 33 insertions(+)
>> create mode 100644 Documentation/devicetree/bindings/nvmem/mtd-nvmem.txt
>>
>> diff --git a/Documentation/devicetree/bindings/nvmem/mtd-nvmem.txt b/Documentation/devicetree/bindings/nvmem/mtd-nvmem.txt
>> new file mode 100644
>> index 0000000..8ed25e6
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/nvmem/mtd-nvmem.txt
>> @@ -0,0 +1,33 @@
>> += NVMEM in MTD =
>> +
>> +Config data for drivers, like MAC addresses, is often stored in MTD.
>> +This binding define how such data storage can be represented in device tree.
>> +
>> +An MTD can be defined as an NVMEM provider by adding the `nvmem-provider`
>> +property to their node. Data cells can then be defined as child nodes
>> +of the partition as defined in nvmem.txt.
>
> Why don't we just read the data from MTD and be done with it ? What's
> the benefit of complicating things by using nvmem ?
Well because usually stuff like MAC addresses etc are stored in eeproms.
This gives a nice abstraction with making them both look like nvmem (that was my
reasoning back then when I submitted a patch to support the OTP part in a
SPI NOR part.
>> +Required properties:
>> +nvmem-provider: Indicate that the device should be registered as
>> + NVMEM provider
>> +
>> +Example:
>> +
>> + flash@0 {
>> + ...
>> +
>> + partition@2 {
>> + label = "art";
>> + reg = <0x7F0000 0x010000>;
>> + read-only;
>> +
>> + nvmem-provider;
>> + #address-cells = <1>;
>> + #size-cells = <1>;
>> +
>> + eeprom@1000 {
>> + label = "wmac-eeprom";
>> + reg = <0x1000 0x1000>;
>> + };
>> + };
>> + };
>>
>
>
> --
> Best regards,
> Marek Vasut
Cheers,
Moritz
On 03/10/2017 05:06 AM, Moritz Fischer wrote:
> On Thu, Mar 9, 2017 at 7:17 PM, Marek Vasut <[email protected]> wrote:
>> On 03/07/2017 09:26 AM, Alban wrote:
>>> Config data for drivers, like MAC addresses, is often stored in MTD.
>>> Add a binding that define how such data storage can be represented in
>>> device tree.
>>>
>>> Signed-off-by: Alban <[email protected]>
>>> ---
>>> Changelog:
>>> v2: * Added a "Required properties" section with the nvmem-provider
>>> property
>>> ---
>>> .../devicetree/bindings/nvmem/mtd-nvmem.txt | 33 ++++++++++++++++++++++
>>> 1 file changed, 33 insertions(+)
>>> create mode 100644 Documentation/devicetree/bindings/nvmem/mtd-nvmem.txt
>>>
>>> diff --git a/Documentation/devicetree/bindings/nvmem/mtd-nvmem.txt b/Documentation/devicetree/bindings/nvmem/mtd-nvmem.txt
>>> new file mode 100644
>>> index 0000000..8ed25e6
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/nvmem/mtd-nvmem.txt
>>> @@ -0,0 +1,33 @@
>>> += NVMEM in MTD =
>>> +
>>> +Config data for drivers, like MAC addresses, is often stored in MTD.
>>> +This binding define how such data storage can be represented in device tree.
>>> +
>>> +An MTD can be defined as an NVMEM provider by adding the `nvmem-provider`
>>> +property to their node. Data cells can then be defined as child nodes
>>> +of the partition as defined in nvmem.txt.
>>
>> Why don't we just read the data from MTD and be done with it ? What's
>> the benefit of complicating things by using nvmem ?
>
> Well because usually stuff like MAC addresses etc are stored in eeproms.
But eeproms are already supported, see drivers/misc/ .
> This gives a nice abstraction with making them both look like nvmem (that was my
> reasoning back then when I submitted a patch to support the OTP part in a
> SPI NOR part.
Hm, I am confused here, we're mixing SPI NOR, EEPROMs and OTP devices here.
--
Best regards,
Marek Vasut
Hi Marek,
On Fri, Mar 10, 2017 at 05:52:36AM +0100, Marek Vasut wrote:
> On 03/10/2017 05:06 AM, Moritz Fischer wrote:
> > On Thu, Mar 9, 2017 at 7:17 PM, Marek Vasut <[email protected]> wrote:
> >> On 03/07/2017 09:26 AM, Alban wrote:
> >>> Config data for drivers, like MAC addresses, is often stored in MTD.
> >>> Add a binding that define how such data storage can be represented in
> >>> device tree.
> >>>
> >>> Signed-off-by: Alban <[email protected]>
> >>> ---
> >>> Changelog:
> >>> v2: * Added a "Required properties" section with the nvmem-provider
> >>> property
> >>> ---
> >>> .../devicetree/bindings/nvmem/mtd-nvmem.txt | 33 ++++++++++++++++++++++
> >>> 1 file changed, 33 insertions(+)
> >>> create mode 100644 Documentation/devicetree/bindings/nvmem/mtd-nvmem.txt
> >>>
> >>> diff --git a/Documentation/devicetree/bindings/nvmem/mtd-nvmem.txt b/Documentation/devicetree/bindings/nvmem/mtd-nvmem.txt
> >>> new file mode 100644
> >>> index 0000000..8ed25e6
> >>> --- /dev/null
> >>> +++ b/Documentation/devicetree/bindings/nvmem/mtd-nvmem.txt
> >>> @@ -0,0 +1,33 @@
> >>> += NVMEM in MTD =
> >>> +
> >>> +Config data for drivers, like MAC addresses, is often stored in MTD.
> >>> +This binding define how such data storage can be represented in device tree.
> >>> +
> >>> +An MTD can be defined as an NVMEM provider by adding the `nvmem-provider`
> >>> +property to their node. Data cells can then be defined as child nodes
> >>> +of the partition as defined in nvmem.txt.
> >>
> >> Why don't we just read the data from MTD and be done with it ? What's
> >> the benefit of complicating things by using nvmem ?
> >
> > Well because usually stuff like MAC addresses etc are stored in eeproms.
>
> But eeproms are already supported, see drivers/misc/ .
This the old, free for all, way to support eeproms. We have a proper
framework for them now, and it's called nvmem.
Maxime
--
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
On 03/10/2017 07:38 AM, Maxime Ripard wrote:
> Hi Marek,
>
> On Fri, Mar 10, 2017 at 05:52:36AM +0100, Marek Vasut wrote:
>> On 03/10/2017 05:06 AM, Moritz Fischer wrote:
>>> On Thu, Mar 9, 2017 at 7:17 PM, Marek Vasut <[email protected]> wrote:
>>>> On 03/07/2017 09:26 AM, Alban wrote:
>>>>> Config data for drivers, like MAC addresses, is often stored in MTD.
>>>>> Add a binding that define how such data storage can be represented in
>>>>> device tree.
>>>>>
>>>>> Signed-off-by: Alban <[email protected]>
>>>>> ---
>>>>> Changelog:
>>>>> v2: * Added a "Required properties" section with the nvmem-provider
>>>>> property
>>>>> ---
>>>>> .../devicetree/bindings/nvmem/mtd-nvmem.txt | 33 ++++++++++++++++++++++
>>>>> 1 file changed, 33 insertions(+)
>>>>> create mode 100644 Documentation/devicetree/bindings/nvmem/mtd-nvmem.txt
>>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/nvmem/mtd-nvmem.txt b/Documentation/devicetree/bindings/nvmem/mtd-nvmem.txt
>>>>> new file mode 100644
>>>>> index 0000000..8ed25e6
>>>>> --- /dev/null
>>>>> +++ b/Documentation/devicetree/bindings/nvmem/mtd-nvmem.txt
>>>>> @@ -0,0 +1,33 @@
>>>>> += NVMEM in MTD =
>>>>> +
>>>>> +Config data for drivers, like MAC addresses, is often stored in MTD.
>>>>> +This binding define how such data storage can be represented in device tree.
>>>>> +
>>>>> +An MTD can be defined as an NVMEM provider by adding the `nvmem-provider`
>>>>> +property to their node. Data cells can then be defined as child nodes
>>>>> +of the partition as defined in nvmem.txt.
>>>>
>>>> Why don't we just read the data from MTD and be done with it ? What's
>>>> the benefit of complicating things by using nvmem ?
>>>
>>> Well because usually stuff like MAC addresses etc are stored in eeproms.
>>
>> But eeproms are already supported, see drivers/misc/ .
>
> This the old, free for all, way to support eeproms. We have a proper
> framework for them now, and it's called nvmem.
Ha, so that's why this patchset, I see. Thanks for clarifying.
--
Best regards,
Marek Vasut
FYI, we noticed the following commit:
commit: 88eb23fa5ebda607c8e78c57ab3aa3da4daf2780 ("mtd: Add support for reading MTD devices via the nvmem API")
url: https://github.com/0day-ci/linux/commits/Alban/mtd-Add-support-for-reading-MTD-devices-via-the-nvmem-API/20170309-221824
in testcase: boot
on test machine: qemu-system-x86_64 -enable-kvm -m 420M
caused below changes (please refer to attached dmesg/kmsg for entire log/backtrace):
+-------------------------------------------+------------+------------+
| | 5d63b41078 | 88eb23fa5e |
+-------------------------------------------+------------+------------+
| boot_successes | 10 | 0 |
| boot_failures | 5 | 20 |
| BUG:kernel_hang_in_test_stage | 5 | |
| WARNING:at_fs/sysfs/dir.c:#sysfs_warn_dup | 0 | 20 |
| kernel_BUG_at_fs/sysfs/file.c | 0 | 20 |
| invalid_opcode:#[##]SMP | 0 | 10 |
| Kernel_panic-not_syncing:Fatal_exception | 0 | 20 |
| invalid_opcode:#[##] | 0 | 10 |
+-------------------------------------------+------------+------------+
[ 6.250800] WARNING: CPU: 0 PID: 1 at fs/sysfs/dir.c:31 sysfs_warn_dup+0x62/0x71
[ 6.253413] sysfs: cannot create duplicate filename '/bus/nvmem/devices/nvmem0'
[ 6.255675] Modules linked in:
[ 6.256734] CPU: 0 PID: 1 Comm: swapper/0 Not tainted 4.11.0-rc1-next-20170308-00002-g88eb23f #23
[ 6.259294] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.9.3-20161025_171302-gandalf 04/01/2014
[ 6.262137] Call Trace:
[ 6.263062] dump_stack+0x82/0xb8
[ 6.264167] __warn+0xc2/0xdd
[ 6.265204] warn_slowpath_fmt+0x4b/0x53
[ 6.266447] ? kernfs_path_from_node+0x54/0x60
[ 6.267796] sysfs_warn_dup+0x62/0x71
[ 6.268980] sysfs_do_create_link_sd+0x97/0xa5
[ 6.270465] sysfs_create_link+0x33/0x35
[ 6.271704] bus_add_device+0xca/0x172
[ 6.272907] device_add+0x326/0x509
[ 6.274053] nvmem_register+0x150/0x368
[ 6.275274] ? __mutex_unlock_slowpath+0x3a/0x229
[ 6.276683] mtd_nvmem_add+0x59/0x8a
[ 6.277844] ? mtdchar_unlocked_ioctl+0x4b/0x4b
[ 6.279226] add_mtd_device+0x1cd/0x269
[ 6.280449] add_mtd_partitions+0xde/0x107
[ 6.281725] ? set_debug_rodata+0x12/0x12
[ 6.282978] mtd_device_parse_register+0xfc/0x174
[ 6.284389] ns_init_module+0x693/0x6ab
[ 6.285620] ? init_nandsim+0x57e/0x57e
[ 6.286834] ? set_debug_rodata+0x12/0x12
[ 6.288090] do_one_initcall+0x90/0x142
[ 6.289316] ? set_debug_rodata+0x12/0x12
[ 6.290568] kernel_init_freeable+0x1cb/0x253
[ 6.291892] ? rest_init+0x13b/0x13b
[ 6.293055] kernel_init+0xe/0xf5
[ 6.294159] ret_from_fork+0x31/0x40
[ 6.295396] ---[ end trace cf2e75dc8a036eed ]---
[ 6.296867] mtd mtd1: Failed to register NVMEM device
[ 6.298919] ------------[ cut here ]------------
[ 6.300280] kernel BUG at fs/sysfs/file.c:330!
[ 6.322389] invalid opcode: 0000 [#1] SMP
[ 6.323618] Modules linked in:
[ 6.324641] CPU: 0 PID: 1 Comm: swapper/0 Tainted: G W 4.11.0-rc1-next-20170308-00002-g88eb23f #23
[ 6.327411] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.9.3-20161025_171302-gandalf 04/01/2014
[ 6.330198] task: ffff880037894040 task.stack: ffffc90000194000
[ 6.331827] RIP: 0010:sysfs_create_file_ns+0x18/0x2e
[ 6.333259] RSP: 0018:ffffc90000197d80 EFLAGS: 00010246
[ 6.334747] RAX: 0000000000000000 RBX: 0000000000000001 RCX: 00000000000000ff
[ 6.336641] RDX: 0000000000000000 RSI: ffffffff84247ae0 RDI: 0000000000000000
[ 6.338532] RBP: ffffc90000197da8 R08: 00000002817900e8 R09: 0000000000000000
[ 6.340419] R10: ffff880032f48b19 R11: ffff880032f48af8 R12: ffffffff84247ad0
[ 6.342306] R13: ffff8800314639b0 R14: ffff880031e54000 R15: ffffffff84247bc0
[ 6.344197] FS: 0000000000000000(0000) GS:ffff880039c00000(0000) knlGS:0000000000000000
[ 6.346542] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 6.348144] CR2: 0000000000000000 CR3: 000000000402c000 CR4: 00000000000006f0
[ 6.350043] Call Trace:
[ 6.350944] ? sysfs_create_files+0x35/0x6e
[ 6.352209] mtd_add_partition_attrs+0x1d/0x36
[ 6.353534] add_mtd_partitions+0xe6/0x107
[ 6.354787] ? set_debug_rodata+0x12/0x12
[ 6.356019] mtd_device_parse_register+0xfc/0x174
[ 6.357393] ns_init_module+0x693/0x6ab
[ 6.358587] ? init_nandsim+0x57e/0x57e
[ 6.359801] ? set_debug_rodata+0x12/0x12
[ 6.361036] do_one_initcall+0x90/0x142
[ 6.362232] ? set_debug_rodata+0x12/0x12
[ 6.363463] kernel_init_freeable+0x1cb/0x253
[ 6.364765] ? rest_init+0x13b/0x13b
[ 6.365905] kernel_init+0xe/0xf5
[ 6.366991] ret_from_fork+0x31/0x40
[ 6.368139] Code: 0f 44 c2 e9 31 ff ff ff 48 8d 65 e8 5b 41 5c 41 5d 5d c3 0f 1f 44 00 00 48 85 ff 74 0e 48 8b 7f 30 48 85 ff 74 05 48 85 f6 75 02 <0f> 0b 0f b7 4e 08 55 49 89 d0 31 d2 48 89 e5 e8 5e fe ff ff 5d
[ 6.373104] RIP: sysfs_create_file_ns+0x18/0x2e RSP: ffffc90000197d80
[ 6.374881] ---[ end trace cf2e75dc8a036eee ]---
[ 6.376252] Kernel panic - not syncing: Fatal exception
[ 6.377918] Kernel Offset: disabled
To reproduce:
git clone https://github.com/01org/lkp-tests.git
cd lkp-tests
bin/lkp qemu -k <bzImage> job-script # job-script is attached in this email
Thanks,
Xiaolong
On Tue, Mar 07, 2017 at 09:26:03AM +0100, Alban wrote:
> Config data for drivers, like MAC addresses, is often stored in MTD.
> Add a binding that define how such data storage can be represented in
> device tree.
>
> Signed-off-by: Alban <[email protected]>
> ---
> Changelog:
> v2: * Added a "Required properties" section with the nvmem-provider
> property
> ---
> .../devicetree/bindings/nvmem/mtd-nvmem.txt | 33 ++++++++++++++++++++++
> 1 file changed, 33 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/nvmem/mtd-nvmem.txt
>
> diff --git a/Documentation/devicetree/bindings/nvmem/mtd-nvmem.txt b/Documentation/devicetree/bindings/nvmem/mtd-nvmem.txt
> new file mode 100644
> index 0000000..8ed25e6
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/nvmem/mtd-nvmem.txt
> @@ -0,0 +1,33 @@
> += NVMEM in MTD =
> +
> +Config data for drivers, like MAC addresses, is often stored in MTD.
> +This binding define how such data storage can be represented in device tree.
> +
> +An MTD can be defined as an NVMEM provider by adding the `nvmem-provider`
> +property to their node. Data cells can then be defined as child nodes
> +of the partition as defined in nvmem.txt.
> +
> +Required properties:
> +nvmem-provider: Indicate that the device should be registered as
> + NVMEM provider
I think we should use a compatible string here (perhaps with a
generic fallback), and that can imply it is an nvmem provider. The
reason is then the compatible can also imply other information that
isn't defined in DT.
Rob
On Wed, 15 Mar 2017 12:24:01 -0500
Rob Herring <[email protected]> wrote:
> On Tue, Mar 07, 2017 at 09:26:03AM +0100, Alban wrote:
> > Config data for drivers, like MAC addresses, is often stored in MTD.
> > Add a binding that define how such data storage can be represented in
> > device tree.
> >
> > Signed-off-by: Alban <[email protected]>
> > ---
> > Changelog:
> > v2: * Added a "Required properties" section with the nvmem-provider
> > property
> > ---
> > .../devicetree/bindings/nvmem/mtd-nvmem.txt | 33 ++++++++++++++++++++++
> > 1 file changed, 33 insertions(+)
> > create mode 100644 Documentation/devicetree/bindings/nvmem/mtd-nvmem.txt
> >
> > diff --git a/Documentation/devicetree/bindings/nvmem/mtd-nvmem.txt b/Documentation/devicetree/bindings/nvmem/mtd-nvmem.txt
> > new file mode 100644
> > index 0000000..8ed25e6
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/nvmem/mtd-nvmem.txt
> > @@ -0,0 +1,33 @@
> > += NVMEM in MTD =
> > +
> > +Config data for drivers, like MAC addresses, is often stored in MTD.
> > +This binding define how such data storage can be represented in device tree.
> > +
> > +An MTD can be defined as an NVMEM provider by adding the `nvmem-provider`
> > +property to their node. Data cells can then be defined as child nodes
> > +of the partition as defined in nvmem.txt.
> > +
> > +Required properties:
> > +nvmem-provider: Indicate that the device should be registered as
> > + NVMEM provider
>
> I think we should use a compatible string here (perhaps with a
> generic fallback), and that can imply it is an nvmem provider. The
> reason is then the compatible can also imply other information that
> isn't defined in DT.
That would work for partitions but not for unpartitioned MTD as these
will already have a compatible string for the MTD hardware. I was also
under the impression that capabilities/services provided by devices
were represented with such properties, like interrupt-controller or
gpio-controller, and not with compatible strings.
There is also another problem with unpartitioned MTD, earlier MTD
partitions binding allowed to have partitions as direct child nodes
without any compatible strings. The current nvmem binding do the same
for the nvmem cells, so it wouldn't be clear if a child node of the MTD
is a partition using the old binding or an nvmem cell.
As I think this problem could happen with some other device types I
suggested to re-work the nvmem binding to be more like the current MTD
partitions. See these threads[1][2], but a short example would look like
this:
flash {
compatible = "vendor,flash-device-model";
...
nvmem-provider;
nvmem-cells {
compatible = "nvmem-cells";
#address-cells = <1>;
#size-cells = <1>;
cell@100 {
label = "mac-address";
reg = <0x100 0x6>;
};
};
};
or like this if the device is partitioned:
flash {
compatible = "vendor,flash-device-model";
...
partitions {
compatible = "fixed-partitions"
...
partition@1000 {
...
nvmem-provider;
nvmem-cells {
compatible = "nvmem-cells";
#address-cells = <1>;
#size-cells = <1>;
cell@100 {
label = "mac-address";
reg = <0x100 0x6>;
};
};
};
};
};
Alban
[1]:https://www.mail-archive.com/[email protected]/msg1344899.html
[2]:https://www.mail-archive.com/[email protected]/msg1348295.html
On Wed, Mar 15, 2017 at 2:41 PM, Alban <[email protected]> wrote:
> On Wed, 15 Mar 2017 12:24:01 -0500
> Rob Herring <[email protected]> wrote:
>
>> On Tue, Mar 07, 2017 at 09:26:03AM +0100, Alban wrote:
>> > Config data for drivers, like MAC addresses, is often stored in MTD.
>> > Add a binding that define how such data storage can be represented in
>> > device tree.
>> >
>> > Signed-off-by: Alban <[email protected]>
>> > ---
>> > Changelog:
>> > v2: * Added a "Required properties" section with the nvmem-provider
>> > property
>> > ---
>> > .../devicetree/bindings/nvmem/mtd-nvmem.txt | 33 ++++++++++++++++++++++
>> > 1 file changed, 33 insertions(+)
>> > create mode 100644 Documentation/devicetree/bindings/nvmem/mtd-nvmem.txt
>> >
>> > diff --git a/Documentation/devicetree/bindings/nvmem/mtd-nvmem.txt b/Documentation/devicetree/bindings/nvmem/mtd-nvmem.txt
>> > new file mode 100644
>> > index 0000000..8ed25e6
>> > --- /dev/null
>> > +++ b/Documentation/devicetree/bindings/nvmem/mtd-nvmem.txt
>> > @@ -0,0 +1,33 @@
>> > += NVMEM in MTD =
>> > +
>> > +Config data for drivers, like MAC addresses, is often stored in MTD.
>> > +This binding define how such data storage can be represented in device tree.
>> > +
>> > +An MTD can be defined as an NVMEM provider by adding the `nvmem-provider`
>> > +property to their node. Data cells can then be defined as child nodes
>> > +of the partition as defined in nvmem.txt.
>> > +
>> > +Required properties:
>> > +nvmem-provider: Indicate that the device should be registered as
>> > + NVMEM provider
>>
>> I think we should use a compatible string here (perhaps with a
>> generic fallback), and that can imply it is an nvmem provider. The
>> reason is then the compatible can also imply other information that
>> isn't defined in DT.
>
> That would work for partitions but not for unpartitioned MTD as these
> will already have a compatible string for the MTD hardware. I was also
> under the impression that capabilities/services provided by devices
> were represented with such properties, like interrupt-controller or
> gpio-controller, and not with compatible strings.
>
> There is also another problem with unpartitioned MTD, earlier MTD
> partitions binding allowed to have partitions as direct child nodes
> without any compatible strings. The current nvmem binding do the same
> for the nvmem cells, so it wouldn't be clear if a child node of the MTD
> is a partition using the old binding or an nvmem cell.
Perhaps a sign we should not repeat that.
> As I think this problem could happen with some other device types I
> suggested to re-work the nvmem binding to be more like the current MTD
> partitions. See these threads[1][2], but a short example would look like
> this:
>
> flash {
> compatible = "vendor,flash-device-model";
> ...
> nvmem-provider;
> nvmem-cells {
> compatible = "nvmem-cells";
Isn't the node name or compatible here enough to imply this is a nvmem provider?
> #address-cells = <1>;
> #size-cells = <1>;
>
> cell@100 {
> label = "mac-address";
> reg = <0x100 0x6>;
> };
> };
> };
On Sat, 18 Mar 2017 15:58:11 -0500
Rob Herring <[email protected]> wrote:
> On Wed, Mar 15, 2017 at 2:41 PM, Alban <[email protected]> wrote:
> > On Wed, 15 Mar 2017 12:24:01 -0500
> > Rob Herring <[email protected]> wrote:
> >
> >> On Tue, Mar 07, 2017 at 09:26:03AM +0100, Alban wrote:
> >> > Config data for drivers, like MAC addresses, is often stored in MTD.
> >> > Add a binding that define how such data storage can be represented in
> >> > device tree.
> >> >
> >> > Signed-off-by: Alban <[email protected]>
> >> > ---
> >> > Changelog:
> >> > v2: * Added a "Required properties" section with the nvmem-provider
> >> > property
> >> > ---
> >> > .../devicetree/bindings/nvmem/mtd-nvmem.txt | 33 ++++++++++++++++++++++
> >> > 1 file changed, 33 insertions(+)
> >> > create mode 100644 Documentation/devicetree/bindings/nvmem/mtd-nvmem.txt
> >> >
> >> > diff --git a/Documentation/devicetree/bindings/nvmem/mtd-nvmem.txt b/Documentation/devicetree/bindings/nvmem/mtd-nvmem.txt
> >> > new file mode 100644
> >> > index 0000000..8ed25e6
> >> > --- /dev/null
> >> > +++ b/Documentation/devicetree/bindings/nvmem/mtd-nvmem.txt
> >> > @@ -0,0 +1,33 @@
> >> > += NVMEM in MTD =
> >> > +
> >> > +Config data for drivers, like MAC addresses, is often stored in MTD.
> >> > +This binding define how such data storage can be represented in device tree.
> >> > +
> >> > +An MTD can be defined as an NVMEM provider by adding the `nvmem-provider`
> >> > +property to their node. Data cells can then be defined as child nodes
> >> > +of the partition as defined in nvmem.txt.
> >> > +
> >> > +Required properties:
> >> > +nvmem-provider: Indicate that the device should be registered as
> >> > + NVMEM provider
> >>
> >> I think we should use a compatible string here (perhaps with a
> >> generic fallback), and that can imply it is an nvmem provider. The
> >> reason is then the compatible can also imply other information that
> >> isn't defined in DT.
> >
> > That would work for partitions but not for unpartitioned MTD as these
> > will already have a compatible string for the MTD hardware. I was also
> > under the impression that capabilities/services provided by devices
> > were represented with such properties, like interrupt-controller or
> > gpio-controller, and not with compatible strings.
> >
> > There is also another problem with unpartitioned MTD, earlier MTD
> > partitions binding allowed to have partitions as direct child nodes
> > without any compatible strings. The current nvmem binding do the same
> > for the nvmem cells, so it wouldn't be clear if a child node of the MTD
> > is a partition using the old binding or an nvmem cell.
>
> Perhaps a sign we should not repeat that.
Yes, that's why I think we should "upgrade" the nvmem binding as a whole
and not just limit this to the MTD case.
> > As I think this problem could happen with some other device types I
> > suggested to re-work the nvmem binding to be more like the current MTD
> > partitions. See these threads[1][2], but a short example would look like
> > this:
> >
> > flash {
> > compatible = "vendor,flash-device-model";
> > ...
> > nvmem-provider;
> > nvmem-cells {
> > compatible = "nvmem-cells";
>
> Isn't the node name or compatible here enough to imply this is a nvmem provider?
If there are cells defined yes, but nvmem also allow referencing the
provider as a whole, so cells are optional. But yes it could be
removed, in the case of MTD it is only used to filter the relevant
devices to avoid having too many "useless" nvmem device registered.
> > #address-cells = <1>;
> > #size-cells = <1>;
> >
> > cell@100 {
> > label = "mac-address";
> > reg = <0x100 0x6>;
> > };
> > };
> > };