[PATCH 1/6] libnvdimm: Add of_node to region and bus descriptors

[PATCH 1/6] libnvdimm: Add of_node to region and bus descriptors

[Oliver O'Halloran]

We want to be able to cross reference the region and bus devices
with the device tree node that they were spawned from. libNVDIMM
handles creating the actual devices for these internally, so we
need to pass in a pointer to the relevant node in the descriptor.

Signed-off-by: Oliver O'Halloran <oohall-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Acked-by: Dan Williams <dan.j.williams-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
---
 drivers/nvdimm/bus.c         | 1 +
 drivers/nvdimm/region_devs.c | 1 +
 include/linux/libnvdimm.h    | 3 +++
 3 files changed, 5 insertions(+)

diff --git a/drivers/nvdimm/bus.c b/drivers/nvdimm/bus.c
index 78eabc3a1ab1..c6106914f396 100644
--- a/drivers/nvdimm/bus.c
+++ b/drivers/nvdimm/bus.c
@@ -358,6 +358,7 @@ struct nvdimm_bus *nvdimm_bus_register(struct device *parent,
 	nvdimm_bus->dev.release = nvdimm_bus_release;
 	nvdimm_bus->dev.groups = nd_desc->attr_groups;
 	nvdimm_bus->dev.bus = &nvdimm_bus_type;
+	nvdimm_bus->dev.of_node = nd_desc->of_node;
 	dev_set_name(&nvdimm_bus->dev, "ndbus%d", nvdimm_bus->id);
 	rc = device_register(&nvdimm_bus->dev);
 	if (rc) {
diff --git a/drivers/nvdimm/region_devs.c b/drivers/nvdimm/region_devs.c
index e6d01911e092..2f1d5771100e 100644
--- a/drivers/nvdimm/region_devs.c
+++ b/drivers/nvdimm/region_devs.c
@@ -1005,6 +1005,7 @@ static struct nd_region *nd_region_create(struct nvdimm_bus *nvdimm_bus,
 	dev->parent = &nvdimm_bus->dev;
 	dev->type = dev_type;
 	dev->groups = ndr_desc->attr_groups;
+	dev->of_node = ndr_desc->of_node;
 	nd_region->ndr_size = resource_size(ndr_desc->res);
 	nd_region->ndr_start = ndr_desc->res->start;
 	nd_device_register(dev);
diff --git a/include/linux/libnvdimm.h b/include/linux/libnvdimm.h
index ff855ed965fb..f61cb5050297 100644
--- a/include/linux/libnvdimm.h
+++ b/include/linux/libnvdimm.h
@@ -76,12 +76,14 @@ typedef int (*ndctl_fn)(struct nvdimm_bus_descriptor *nd_desc,
 		struct nvdimm *nvdimm, unsigned int cmd, void *buf,
 		unsigned int buf_len, int *cmd_rc);
 
+struct device_node;
 struct nvdimm_bus_descriptor {
 	const struct attribute_group **attr_groups;
 	unsigned long bus_dsm_mask;
 	unsigned long cmd_mask;
 	struct module *module;
 	char *provider_name;
+	struct device_node *of_node;
 	ndctl_fn ndctl;
 	int (*flush_probe)(struct nvdimm_bus_descriptor *nd_desc);
 	int (*clear_to_send)(struct nvdimm_bus_descriptor *nd_desc,
@@ -123,6 +125,7 @@ struct nd_region_desc {
 	int num_lanes;
 	int numa_node;
 	unsigned long flags;
+	struct device_node *of_node;
 };
 
 struct device;
-- 
2.9.5

[PATCH 2/6] libnvdimm: Add nd_region_destroy()

[Oliver O'Halloran]

Currently there's no way to remove a region from and nvdimm_bus without
tearing down the whole bus. This patch adds an API for removing a single
region from the bus so that we can implement a sensible unbind operation
for the of_nd_region platform driver.

Signed-off-by: Oliver O'Halloran <oohall-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 drivers/nvdimm/region_devs.c | 6 ++++++
 include/linux/libnvdimm.h    | 1 +
 2 files changed, 7 insertions(+)

diff --git a/drivers/nvdimm/region_devs.c b/drivers/nvdimm/region_devs.c
index 2f1d5771100e..76f46fd1fae0 100644
--- a/drivers/nvdimm/region_devs.c
+++ b/drivers/nvdimm/region_devs.c
@@ -1039,6 +1039,12 @@ struct nd_region *nvdimm_blk_region_create(struct nvdimm_bus *nvdimm_bus,
 }
 EXPORT_SYMBOL_GPL(nvdimm_blk_region_create);
 
+void nd_region_destroy(struct nd_region *region)
+{
+	nd_device_unregister(&region->dev, ND_SYNC);
+}
+EXPORT_SYMBOL_GPL(nd_region_destroy);
+
 struct nd_region *nvdimm_volatile_region_create(struct nvdimm_bus *nvdimm_bus,
 		struct nd_region_desc *ndr_desc)
 {
diff --git a/include/linux/libnvdimm.h b/include/linux/libnvdimm.h
index f61cb5050297..df21ca176e98 100644
--- a/include/linux/libnvdimm.h
+++ b/include/linux/libnvdimm.h
@@ -192,6 +192,7 @@ struct nd_region *nvdimm_blk_region_create(struct nvdimm_bus *nvdimm_bus,
 		struct nd_region_desc *ndr_desc);
 struct nd_region *nvdimm_volatile_region_create(struct nvdimm_bus *nvdimm_bus,
 		struct nd_region_desc *ndr_desc);
+void nd_region_destroy(struct nd_region *region);
 void *nd_region_provider_data(struct nd_region *nd_region);
 void *nd_blk_region_provider_data(struct nd_blk_region *ndbr);
 void nd_blk_region_set_provider_data(struct nd_blk_region *ndbr, void *data);
-- 
2.9.5

Re: [PATCH 2/6] libnvdimm: Add nd_region_destroy()

[Dan Williams]

On Fri, Mar 23, 2018 at 1:12 AM, Oliver O'Halloran <oohall-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> Currently there's no way to remove a region from and nvdimm_bus without
> tearing down the whole bus. This patch adds an API for removing a single
> region from the bus so that we can implement a sensible unbind operation
> for the of_nd_region platform driver.
>
> Signed-off-by: Oliver O'Halloran <oohall-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---
>  drivers/nvdimm/region_devs.c | 6 ++++++
>  include/linux/libnvdimm.h    | 1 +
>  2 files changed, 7 insertions(+)
>
> diff --git a/drivers/nvdimm/region_devs.c b/drivers/nvdimm/region_devs.c
> index 2f1d5771100e..76f46fd1fae0 100644
> --- a/drivers/nvdimm/region_devs.c
> +++ b/drivers/nvdimm/region_devs.c
> @@ -1039,6 +1039,12 @@ struct nd_region *nvdimm_blk_region_create(struct nvdimm_bus *nvdimm_bus,
>  }
>  EXPORT_SYMBOL_GPL(nvdimm_blk_region_create);
>
> +void nd_region_destroy(struct nd_region *region)

Let's put this in the "nvdimm_" namespace so it pairs with the
nvdimm_*_region_create() apis.

Re: [PATCH 2/6] libnvdimm: Add nd_region_destroy()

[Balbir Singh]

On Fri, 23 Mar 2018 19:12:05 +1100
Oliver O'Halloran <oohall-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:

> Currently there's no way to remove a region from and nvdimm_bus without
> tearing down the whole bus. This patch adds an API for removing a single
> region from the bus so that we can implement a sensible unbind operation
> for the of_nd_region platform driver.
> 
> Signed-off-by: Oliver O'Halloran <oohall-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---
>  drivers/nvdimm/region_devs.c | 6 ++++++
>  include/linux/libnvdimm.h    | 1 +
>  2 files changed, 7 insertions(+)
> 
> diff --git a/drivers/nvdimm/region_devs.c b/drivers/nvdimm/region_devs.c
> index 2f1d5771100e..76f46fd1fae0 100644
> --- a/drivers/nvdimm/region_devs.c
> +++ b/drivers/nvdimm/region_devs.c
> @@ -1039,6 +1039,12 @@ struct nd_region *nvdimm_blk_region_create(struct nvdimm_bus *nvdimm_bus,
>  }
>  EXPORT_SYMBOL_GPL(nvdimm_blk_region_create);
>  
> +void nd_region_destroy(struct nd_region *region)
> +{
> +	nd_device_unregister(&region->dev, ND_SYNC);

child_unregister seems to do the same thing, but is expected to be used
as a callback from device_for_each_child()

I'd suggest we merge the two and rename child_unregister and nd_region_unregister

Balbir Singh.

[PATCH 3/6] libnvdimm: Add device-tree based driver

[Oliver O'Halloran]

This patch adds peliminary device-tree bindings for the NVDIMM driver.
Currently this only supports one bus (created at probe time) which all
regions are added to with individual regions being created by a platform
device driver.

Signed-off-by: Oliver O'Halloran <oohall-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
I suspect the platform driver should be holding a reference to the
created region. I left that out here since previously Dan has said
he'd rather keep the struct device internal to libnvdimm and the only
other way a region device can disappear is when the bus is unregistered.
---
 MAINTAINERS                |   8 +++
 drivers/nvdimm/Kconfig     |  10 ++++
 drivers/nvdimm/Makefile    |   1 +
 drivers/nvdimm/of_nvdimm.c | 130 +++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 149 insertions(+)
 create mode 100644 drivers/nvdimm/of_nvdimm.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 4e62756936fa..e3fc47fbfc7a 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -8035,6 +8035,14 @@ Q:	https://patchwork.kernel.org/project/linux-nvdimm/list/
 S:	Supported
 F:	drivers/nvdimm/pmem*
 
+LIBNVDIMM: DEVICETREE BINDINGS
+M:	Oliver O'Halloran <oohall-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
+L:	linux-nvdimm-hn68Rpc1hR1g9hUCZPvPmw@public.gmane.org
+Q:	https://patchwork.kernel.org/project/linux-nvdimm/list/
+S:	Supported
+F:	drivers/nvdimm/of_nvdimm.c
+F:	Documentation/devicetree/bindings/nvdimm/nvdimm-bus.txt
+
 LIBNVDIMM: NON-VOLATILE MEMORY DEVICE SUBSYSTEM
 M:	Dan Williams <dan.j.williams-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
 L:	linux-nvdimm-hn68Rpc1hR1g9hUCZPvPmw@public.gmane.org
diff --git a/drivers/nvdimm/Kconfig b/drivers/nvdimm/Kconfig
index a65f2e1d9f53..505a9bbbe49f 100644
--- a/drivers/nvdimm/Kconfig
+++ b/drivers/nvdimm/Kconfig
@@ -102,4 +102,14 @@ config NVDIMM_DAX
 
 	  Select Y if unsure
 
+config OF_NVDIMM
+	tristate "Device-tree support for NVDIMMs"
+	depends on OF
+	default LIBNVDIMM
+	help
+	  Allows byte addressable persistent memory regions to be described in the
+	  device-tree.
+
+	  Select Y if unsure.
+
 endif
diff --git a/drivers/nvdimm/Makefile b/drivers/nvdimm/Makefile
index 70d5f3ad9909..fd6a5838aa25 100644
--- a/drivers/nvdimm/Makefile
+++ b/drivers/nvdimm/Makefile
@@ -4,6 +4,7 @@ obj-$(CONFIG_BLK_DEV_PMEM) += nd_pmem.o
 obj-$(CONFIG_ND_BTT) += nd_btt.o
 obj-$(CONFIG_ND_BLK) += nd_blk.o
 obj-$(CONFIG_X86_PMEM_LEGACY) += nd_e820.o
+obj-$(CONFIG_OF_NVDIMM) += of_nvdimm.o
 
 nd_pmem-y := pmem.o
 
diff --git a/drivers/nvdimm/of_nvdimm.c b/drivers/nvdimm/of_nvdimm.c
new file mode 100644
index 000000000000..79c28291f420
--- /dev/null
+++ b/drivers/nvdimm/of_nvdimm.c
@@ -0,0 +1,130 @@
+// SPDX-License-Identifier: GPL-2.0+
+
+#define pr_fmt(fmt) "of_nvdimm: " fmt
+
+#include <linux/of_platform.h>
+#include <linux/of_address.h>
+#include <linux/libnvdimm.h>
+#include <linux/module.h>
+#include <linux/ioport.h>
+#include <linux/slab.h>
+
+/*
+ * Container bus stuff.  For now we just chunk regions into a default
+ * bus with no ndctl support. In the future we'll add some mechanism
+ * for dispatching regions into the correct bus type, but this is useful
+ * for now.
+ */
+struct nvdimm_bus_descriptor bus_desc;
+struct nvdimm_bus *bus;
+
+/* region driver */
+
+static const struct attribute_group *region_attr_groups[] = {
+	&nd_region_attribute_group,
+	&nd_device_attribute_group,
+	NULL,
+};
+
+static const struct attribute_group *bus_attr_groups[] = {
+	&nvdimm_bus_attribute_group,
+	NULL,
+};
+
+static int of_nd_region_probe(struct platform_device *pdev)
+{
+	struct nd_region_desc ndr_desc;
+	struct resource temp_res;
+	struct nd_region *region;
+	struct device_node *np;
+
+	np = dev_of_node(&pdev->dev);
+	if (!np)
+		return -ENXIO;
+
+	pr_err("registering region for %pOF\n", np);
+
+	if (of_address_to_resource(np, 0, &temp_res)) {
+		pr_warn("Unable to parse reg[0] for %pOF\n", np);
+		return -ENXIO;
+	}
+
+	memset(&ndr_desc, 0, sizeof(ndr_desc));
+	ndr_desc.res = &temp_res;
+	ndr_desc.of_node = np;
+	ndr_desc.attr_groups = region_attr_groups;
+	ndr_desc.numa_node = of_node_to_nid(np);
+	set_bit(ND_REGION_PAGEMAP, &ndr_desc.flags);
+
+	/*
+	 * NB: libnvdimm copies the data from ndr_desc into it's own structures
+	 * so passing stack pointers is fine.
+	 */
+	if (of_get_property(np, "volatile", NULL))
+		region = nvdimm_volatile_region_create(bus, &ndr_desc);
+	else
+		region = nvdimm_pmem_region_create(bus, &ndr_desc);
+
+	pr_warn("registered pmem region %px\n", region);
+	if (!region)
+		return -ENXIO;
+
+	platform_set_drvdata(pdev, region);
+
+	return 0;
+}
+
+static int of_nd_region_remove(struct platform_device *pdev)
+{
+	struct nd_region *r = platform_get_drvdata(pdev);
+
+	nd_region_destroy(r);
+
+	return 0;
+}
+
+static const struct of_device_id of_nd_region_match[] = {
+	{ .compatible = "nvdimm-region" },
+	{ },
+};
+
+static struct platform_driver of_nd_region_driver = {
+	.probe = of_nd_region_probe,
+	.remove = of_nd_region_remove,
+	.driver = {
+		.name = "of_nd_region",
+		.owner = THIS_MODULE,
+		.of_match_table = of_nd_region_match,
+	},
+};
+
+/* bus wrangling */
+
+static int __init of_nvdimm_init(void)
+{
+	/* register  */
+	bus_desc.attr_groups = bus_attr_groups;
+	bus_desc.provider_name = "of_nvdimm";
+	bus_desc.module = THIS_MODULE;
+
+	/* does parent == NULL work? */
+	bus = nvdimm_bus_register(NULL, &bus_desc);
+	if (!bus)
+		return -ENODEV;
+
+	platform_driver_register(&of_nd_region_driver);
+
+	return 0;
+}
+module_init(of_nvdimm_init);
+
+static void __init of_nvdimm_exit(void)
+{
+	nvdimm_bus_unregister(bus);
+	platform_driver_unregister(&of_nd_region_driver);
+}
+module_exit(of_nvdimm_exit);
+
+MODULE_DEVICE_TABLE(of, of_nd_region_match);
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("IBM Corporation");
-- 
2.9.5

Re: [PATCH 3/6] libnvdimm: Add device-tree based driver

[Dan Williams]

On Fri, Mar 23, 2018 at 1:12 AM, Oliver O'Halloran <oohall-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> This patch adds peliminary device-tree bindings for the NVDIMM driver.

*preliminary

> Currently this only supports one bus (created at probe time) which all
> regions are added to with individual regions being created by a platform
> device driver.
>
> Signed-off-by: Oliver O'Halloran <oohall-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---
> I suspect the platform driver should be holding a reference to the
> created region. I left that out here since previously Dan has said
> he'd rather keep the struct device internal to libnvdimm and the only
> other way a region device can disappear is when the bus is unregistered.

Hmm, but this still seems broken if bus teardown races region
teardown. I think the more natural model, given the way libnvdimm is
structured, to have each of these of_nd_region instances create their
own nvdimm bus. Thoughts?

Re: [PATCH 3/6] libnvdimm: Add device-tree based driver

[Oliver]

On Sat, Mar 24, 2018 at 4:07 AM, Dan Williams <dan.j.williams-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org> wrote:
> On Fri, Mar 23, 2018 at 1:12 AM, Oliver O'Halloran <oohall-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>> This patch adds peliminary device-tree bindings for the NVDIMM driver.
>
> *preliminary
>
>> Currently this only supports one bus (created at probe time) which all
>> regions are added to with individual regions being created by a platform
>> device driver.
>>
>> Signed-off-by: Oliver O'Halloran <oohall-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>> ---
>> I suspect the platform driver should be holding a reference to the
>> created region. I left that out here since previously Dan has said
>> he'd rather keep the struct device internal to libnvdimm and the only
>> other way a region device can disappear is when the bus is unregistered.

> Hmm, but this still seems broken if bus teardown races region
> teardown.

Yeah looks like it.

>I think the more natural model, given the way libnvdimm is
> structured, to have each of these of_nd_region instances create their
> own nvdimm bus. Thoughts?

I'd prefer this too tbh. The main reason I'm looking at this approach
is that we plan on doing all interleave set configuration, etc from
Linux. So we'd like to have "similar" region/dimm devices end up in
the same bus to simplify validating managment ops, etc. That said, the
real enablement work for that is blocked on teams so I'm ok with
ignoring all of that for now.

Oliver

Re: [PATCH 3/6] libnvdimm: Add device-tree based driver

[kbuild test robot]

Hi Oliver,

I love your patch! Yet something to improve:

[auto build test ERROR on linux-nvdimm/libnvdimm-for-next]
[also build test ERROR on v4.16-rc6 next-20180323]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Oliver-O-Halloran/libnvdimm-Add-of_node-to-region-and-bus-descriptors/20180325-082036
base:   https://git.kernel.org/pub/scm/linux/kernel/git/nvdimm/nvdimm.git libnvdimm-for-next
config: ia64-allmodconfig (attached as .config)
compiler: ia64-linux-gcc (GCC) 7.2.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=ia64 

All errors (new ones prefixed by >>):

   ERROR: "ia64_delay_loop" [drivers/spi/spi-thunderx.ko] undefined!
>> ERROR: "of_node_to_nid" [drivers/nvdimm/of_nvdimm.ko] undefined!
   ERROR: "ia64_delay_loop" [drivers/net/phy/mdio-cavium.ko] undefined!

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

Re: [PATCH 3/6] libnvdimm: Add device-tree based driver

[kbuild test robot]

Hi Oliver,

I love your patch! Perhaps something to improve:

[auto build test WARNING on linux-nvdimm/libnvdimm-for-next]
[also build test WARNING on v4.16-rc6 next-20180323]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Oliver-O-Halloran/libnvdimm-Add-of_node-to-region-and-bus-descriptors/20180325-082036
base:   https://git.kernel.org/pub/scm/linux/kernel/git/nvdimm/nvdimm.git libnvdimm-for-next
reproduce:
        # apt-get install sparse
        make ARCH=x86_64 allmodconfig
        make C=1 CF=-D__CHECK_ENDIAN__


sparse warnings: (new ones prefixed by >>)

>> drivers/nvdimm/of_nvdimm.c:18:30: sparse: symbol 'bus_desc' was not declared. Should it be static?
   drivers/nvdimm/of_nvdimm.c:19:19: sparse: symbol 'bus' was not declared. Should it be static?

Please review and possibly fold the followup patch.

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[RFC PATCH] libnvdimm: bus_desc can be static

[kbuild test robot]

Fixes: 6cb2b51b9b76 ("libnvdimm: Add device-tree based driver")
Signed-off-by: Fengguang Wu <fengguang.wu-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
---
 of_nvdimm.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/nvdimm/of_nvdimm.c b/drivers/nvdimm/of_nvdimm.c
index 79c2829..4ed373d 100644
--- a/drivers/nvdimm/of_nvdimm.c
+++ b/drivers/nvdimm/of_nvdimm.c
@@ -15,7 +15,7 @@
  * for dispatching regions into the correct bus type, but this is useful
  * for now.
  */
-struct nvdimm_bus_descriptor bus_desc;
+static struct nvdimm_bus_descriptor bus_desc;
 struct nvdimm_bus *bus;
 
 /* region driver */

Re: [PATCH 3/6] libnvdimm: Add device-tree based driver

[Balbir Singh]

On Fri, 23 Mar 2018 19:12:06 +1100
Oliver O'Halloran <oohall-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:

> This patch adds peliminary device-tree bindings for the NVDIMM driver.
> Currently this only supports one bus (created at probe time) which all
> regions are added to with individual regions being created by a platform
> device driver.
> 
> Signed-off-by: Oliver O'Halloran <oohall-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---

It's a good idea to keep a changelog from the previous revisions, but I'm
just nit-picking. The reason I want to bring that up is because this revision
has different bindings from what was proposed earlier.

> I suspect the platform driver should be holding a reference to the
> created region. I left that out here since previously Dan has said
> he'd rather keep the struct device internal to libnvdimm and the only
> other way a region device can disappear is when the bus is unregistered.

I think the previous bindings did not have this issue, but the suggestion
was to move regions to being platform devices so as to avoid custom
code for tracking what is populated and what is not.

Personally I liked the previous binding better, but then I am not a device
tree expert.

> ---
>  MAINTAINERS                |   8 +++
>  drivers/nvdimm/Kconfig     |  10 ++++
>  drivers/nvdimm/Makefile    |   1 +
>  drivers/nvdimm/of_nvdimm.c | 130 +++++++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 149 insertions(+)
>  create mode 100644 drivers/nvdimm/of_nvdimm.c
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 4e62756936fa..e3fc47fbfc7a 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -8035,6 +8035,14 @@ Q:	https://patchwork.kernel.org/project/linux-nvdimm/list/
>  S:	Supported
>  F:	drivers/nvdimm/pmem*
>  
> +LIBNVDIMM: DEVICETREE BINDINGS
> +M:	Oliver O'Halloran <oohall-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> +L:	linux-nvdimm-hn68Rpc1hR1g9hUCZPvPmw@public.gmane.org
> +Q:	https://patchwork.kernel.org/project/linux-nvdimm/list/
> +S:	Supported
> +F:	drivers/nvdimm/of_nvdimm.c
> +F:	Documentation/devicetree/bindings/nvdimm/nvdimm-bus.txt
> +
>  LIBNVDIMM: NON-VOLATILE MEMORY DEVICE SUBSYSTEM
>  M:	Dan Williams <dan.j.williams-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
>  L:	linux-nvdimm-hn68Rpc1hR1g9hUCZPvPmw@public.gmane.org
> diff --git a/drivers/nvdimm/Kconfig b/drivers/nvdimm/Kconfig
> index a65f2e1d9f53..505a9bbbe49f 100644
> --- a/drivers/nvdimm/Kconfig
> +++ b/drivers/nvdimm/Kconfig
> @@ -102,4 +102,14 @@ config NVDIMM_DAX
>  
>  	  Select Y if unsure
>  
> +config OF_NVDIMM
> +	tristate "Device-tree support for NVDIMMs"
> +	depends on OF
> +	default LIBNVDIMM
> +	help
> +	  Allows byte addressable persistent memory regions to be described in the
> +	  device-tree.

Again nit-picking, but I thought we supported volatile mappings too.

> +
> +	  Select Y if unsure.
> +
>  endif
> diff --git a/drivers/nvdimm/Makefile b/drivers/nvdimm/Makefile
> index 70d5f3ad9909..fd6a5838aa25 100644
> --- a/drivers/nvdimm/Makefile
> +++ b/drivers/nvdimm/Makefile
> @@ -4,6 +4,7 @@ obj-$(CONFIG_BLK_DEV_PMEM) += nd_pmem.o
>  obj-$(CONFIG_ND_BTT) += nd_btt.o
>  obj-$(CONFIG_ND_BLK) += nd_blk.o
>  obj-$(CONFIG_X86_PMEM_LEGACY) += nd_e820.o
> +obj-$(CONFIG_OF_NVDIMM) += of_nvdimm.o
>  
>  nd_pmem-y := pmem.o
>  
> diff --git a/drivers/nvdimm/of_nvdimm.c b/drivers/nvdimm/of_nvdimm.c
> new file mode 100644
> index 000000000000..79c28291f420
> --- /dev/null
> +++ b/drivers/nvdimm/of_nvdimm.c
> @@ -0,0 +1,130 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +
> +#define pr_fmt(fmt) "of_nvdimm: " fmt
> +
> +#include <linux/of_platform.h>
> +#include <linux/of_address.h>
> +#include <linux/libnvdimm.h>
> +#include <linux/module.h>
> +#include <linux/ioport.h>
> +#include <linux/slab.h>
> +
> +/*
> + * Container bus stuff.  For now we just chunk regions into a default
> + * bus with no ndctl support. In the future we'll add some mechanism
> + * for dispatching regions into the correct bus type, but this is useful
> + * for now.

I liked that the previous binding handled this better, it looked very
similar to the e820 binding in terms of simplicity.

> + */
> +struct nvdimm_bus_descriptor bus_desc;
> +struct nvdimm_bus *bus;
> +
> +/* region driver */
> +
> +static const struct attribute_group *region_attr_groups[] = {
> +	&nd_region_attribute_group,
> +	&nd_device_attribute_group,
> +	NULL,
> +};
> +
> +static const struct attribute_group *bus_attr_groups[] = {
> +	&nvdimm_bus_attribute_group,
> +	NULL,
> +};
> +
> +static int of_nd_region_probe(struct platform_device *pdev)
> +{
> +	struct nd_region_desc ndr_desc;
> +	struct resource temp_res;
> +	struct nd_region *region;
> +	struct device_node *np;
> +
> +	np = dev_of_node(&pdev->dev);
> +	if (!np)
> +		return -ENXIO;
> +
> +	pr_err("registering region for %pOF\n", np);
> +
> +	if (of_address_to_resource(np, 0, &temp_res)) {
> +		pr_warn("Unable to parse reg[0] for %pOF\n", np);
> +		return -ENXIO;
> +	}
> +
> +	memset(&ndr_desc, 0, sizeof(ndr_desc));
> +	ndr_desc.res = &temp_res;
> +	ndr_desc.of_node = np;
> +	ndr_desc.attr_groups = region_attr_groups;
> +	ndr_desc.numa_node = of_node_to_nid(np);

We probably need an ndr_desc.provider

> +	set_bit(ND_REGION_PAGEMAP, &ndr_desc.flags);
> +
> +	/*
> +	 * NB: libnvdimm copies the data from ndr_desc into it's own structures
> +	 * so passing stack pointers is fine.
> +	 */
> +	if (of_get_property(np, "volatile", NULL))
> +		region = nvdimm_volatile_region_create(bus, &ndr_desc);
> +	else
> +		region = nvdimm_pmem_region_create(bus, &ndr_desc);
> +
> +	pr_warn("registered pmem region %px\n", region);
> +	if (!region)
> +		return -ENXIO;
> +
> +	platform_set_drvdata(pdev, region);
> +
> +	return 0;
> +}
> +
> +static int of_nd_region_remove(struct platform_device *pdev)
> +{
> +	struct nd_region *r = platform_get_drvdata(pdev);
> +
> +	nd_region_destroy(r);
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id of_nd_region_match[] = {
> +	{ .compatible = "nvdimm-region" },
> +	{ },
> +};
> +
> +static struct platform_driver of_nd_region_driver = {
> +	.probe = of_nd_region_probe,
> +	.remove = of_nd_region_remove,
> +	.driver = {
> +		.name = "of_nd_region",
> +		.owner = THIS_MODULE,
> +		.of_match_table = of_nd_region_match,
> +	},
> +};
> +
> +/* bus wrangling */
> +
> +static int __init of_nvdimm_init(void)
> +{
> +	/* register  */
> +	bus_desc.attr_groups = bus_attr_groups;
> +	bus_desc.provider_name = "of_nvdimm";
> +	bus_desc.module = THIS_MODULE;
> +
> +	/* does parent == NULL work? */
> +	bus = nvdimm_bus_register(NULL, &bus_desc);
> +	if (!bus)
> +		return -ENODEV;
> +
> +	platform_driver_register(&of_nd_region_driver);
> +
> +	return 0;
> +}
> +module_init(of_nvdimm_init);
> +
> +static void __init of_nvdimm_exit(void)
> +{
> +	nvdimm_bus_unregister(bus);
> +	platform_driver_unregister(&of_nd_region_driver);
> +}
> +module_exit(of_nvdimm_exit);
> +
> +MODULE_DEVICE_TABLE(of, of_nd_region_match);
> +MODULE_LICENSE("GPL");
> +MODULE_AUTHOR("IBM Corporation");

Balbir Singh.

[PATCH 4/6] libnvdimm/of: Symlink platform and region devices

[Oliver O'Halloran]

Add a way direct link between the region and the platform device that
creates the region.

Signed-off-by: Oliver O'Halloran <oohall-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 drivers/nvdimm/of_nvdimm.c   | 11 +++++++++++
 drivers/nvdimm/region_devs.c | 13 +++++++++++++
 include/linux/libnvdimm.h    |  1 +
 3 files changed, 25 insertions(+)

diff --git a/drivers/nvdimm/of_nvdimm.c b/drivers/nvdimm/of_nvdimm.c
index 79c28291f420..28f4ca23a690 100644
--- a/drivers/nvdimm/of_nvdimm.c
+++ b/drivers/nvdimm/of_nvdimm.c
@@ -37,6 +37,7 @@ static int of_nd_region_probe(struct platform_device *pdev)
 	struct resource temp_res;
 	struct nd_region *region;
 	struct device_node *np;
+	int rc;
 
 	np = dev_of_node(&pdev->dev);
 	if (!np)
@@ -71,6 +72,15 @@ static int of_nd_region_probe(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, region);
 
+	/*
+	 * Add a symlink to the ndbus region object. Without this there's no
+	 * simple way to go from the platform device to the region it spawned.
+	 */
+	rc = sysfs_create_link(&pdev->dev.kobj,
+				nd_region_kobj(region), "region");
+	if (rc)
+		pr_warn("Failed to create symlink to region (rc = %d)!\n", rc);
+
 	return 0;
 }
 
@@ -78,6 +88,7 @@ static int of_nd_region_remove(struct platform_device *pdev)
 {
 	struct nd_region *r = platform_get_drvdata(pdev);
 
+	sysfs_delete_link(&pdev->dev.kobj, nd_region_kobj(r), "region");
 	nd_region_destroy(r);
 
 	return 0;
diff --git a/drivers/nvdimm/region_devs.c b/drivers/nvdimm/region_devs.c
index 76f46fd1fae0..af09acc1d93b 100644
--- a/drivers/nvdimm/region_devs.c
+++ b/drivers/nvdimm/region_devs.c
@@ -1054,6 +1054,19 @@ struct nd_region *nvdimm_volatile_region_create(struct nvdimm_bus *nvdimm_bus,
 }
 EXPORT_SYMBOL_GPL(nvdimm_volatile_region_create);
 
+struct kobject *nd_region_kobj(struct nd_region *region)
+{
+	/*
+	 * region init is async so we need to explicitly synchronise
+	 * to prevent handing out a kobj reference before device_add()
+	 * has been run
+	 */
+	nd_synchronize();
+
+	return &region->dev.kobj;
+}
+EXPORT_SYMBOL_GPL(nd_region_kobj);
+
 /**
  * nvdimm_flush - flush any posted write queues between the cpu and pmem media
  * @nd_region: blk or interleaved pmem region
diff --git a/include/linux/libnvdimm.h b/include/linux/libnvdimm.h
index df21ca176e98..a4b3663bac38 100644
--- a/include/linux/libnvdimm.h
+++ b/include/linux/libnvdimm.h
@@ -172,6 +172,7 @@ struct nvdimm_bus_descriptor *to_nd_desc(struct nvdimm_bus *nvdimm_bus);
 struct device *to_nvdimm_bus_dev(struct nvdimm_bus *nvdimm_bus);
 const char *nvdimm_name(struct nvdimm *nvdimm);
 struct kobject *nvdimm_kobj(struct nvdimm *nvdimm);
+struct kobject *nd_region_kobj(struct nd_region *region);
 unsigned long nvdimm_cmd_mask(struct nvdimm *nvdimm);
 void *nvdimm_provider_data(struct nvdimm *nvdimm);
 struct nvdimm *nvdimm_create(struct nvdimm_bus *nvdimm_bus, void *provider_data,
-- 
2.9.5

Re: [PATCH 4/6] libnvdimm/of: Symlink platform and region devices

[Dan Williams]

On Fri, Mar 23, 2018 at 1:12 AM, Oliver O'Halloran <oohall-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> Add a way direct link between the region and the platform device that
> creates the region.
>

This linking would not be needed if of_nd_regions each lived on their own bus.

[PATCH 5/6] powerpc/powernv: Create platform devs for nvdimm buses

[Oliver O'Halloran]

Scan the devicetree for an nvdimm-bus compatible and create
a platform device for them.

Signed-off-by: Oliver O'Halloran <oohall-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 arch/powerpc/platforms/powernv/opal.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/arch/powerpc/platforms/powernv/opal.c b/arch/powerpc/platforms/powernv/opal.c
index c15182765ff5..a16f4b63ccf2 100644
--- a/arch/powerpc/platforms/powernv/opal.c
+++ b/arch/powerpc/platforms/powernv/opal.c
@@ -821,6 +821,9 @@ static int __init opal_init(void)
 	/* Create i2c platform devices */
 	opal_pdev_init("ibm,opal-i2c");
 
+	/* Handle non-volatile memory devices */
+	opal_pdev_init("nvdimm-region");
+
 	/* Setup a heatbeat thread if requested by OPAL */
 	opal_init_heartbeat();
 
-- 
2.9.5

[PATCH 6/6] doc/devicetree: NVDIMM region documentation

[Oliver O'Halloran]

Add device-tree binding documentation for the nvdimm region driver.

Cc: devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
Signed-off-by: Oliver O'Halloran <oohall-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 .../devicetree/bindings/nvdimm/nvdimm-region.txt   | 45 ++++++++++++++++++++++
 1 file changed, 45 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/nvdimm/nvdimm-region.txt

diff --git a/Documentation/devicetree/bindings/nvdimm/nvdimm-region.txt b/Documentation/devicetree/bindings/nvdimm/nvdimm-region.txt
new file mode 100644
index 000000000000..02091117ff16
--- /dev/null
+++ b/Documentation/devicetree/bindings/nvdimm/nvdimm-region.txt
@@ -0,0 +1,45 @@
+Device-tree bindings for NVDIMM memory regions
+-----------------------------------------------------
+
+Non-volatile DIMMs are memory modules used to provide (cacheable) main memory
+that retains its contents across power cycles. In more practical terms, they
+are kind of storage device where the contents can be accessed by the CPU
+directly, rather than indirectly via a storage controller or similar. The an
+nvdimm-region specifies a physical address range that is hosted on an NVDIMM
+device.
+
+Bindings for the region nodes:
+-----------------------------
+
+Required properties:
+	- compatible = "nvdimm-region"
+
+	- reg = <base, size>;
+		The system physical address range of this nvdimm region.
+
+Optional properties:
+	- Any relevant NUMA assocativity properties for the target platform.
+	- A "volatile" property indicating that this region is actually in
+	  normal DRAM and does not require cache flushes after each write.
+
+A complete example:
+--------------------
+
+/ {
+	#size-cells = <2>;
+	#address-cells = <2>;
+
+	platform {
+		region@5000 {
+			compatible = "nvdimm-region;
+			reg = <0x00000001 0x00000000 0x00000000 0x40000000>
+
+		};
+
+		region@6000 {
+			compatible = "nvdimm-region";
+			reg = <0x00000001 0x00000000 0x00000000 0x40000000>
+			volatile;
+		};
+	};
+};
-- 
2.9.5

Re: [PATCH 6/6] doc/devicetree: NVDIMM region documentation

[Rob Herring]

On Fri, Mar 23, 2018 at 07:12:09PM +1100, Oliver O'Halloran wrote:
> Add device-tree binding documentation for the nvdimm region driver.
> 
> Cc: devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> Signed-off-by: Oliver O'Halloran <oohall-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---
>  .../devicetree/bindings/nvdimm/nvdimm-region.txt   | 45 ++++++++++++++++++++++
>  1 file changed, 45 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/nvdimm/nvdimm-region.txt
> 
> diff --git a/Documentation/devicetree/bindings/nvdimm/nvdimm-region.txt b/Documentation/devicetree/bindings/nvdimm/nvdimm-region.txt
> new file mode 100644
> index 000000000000..02091117ff16
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/nvdimm/nvdimm-region.txt
> @@ -0,0 +1,45 @@
> +Device-tree bindings for NVDIMM memory regions
> +-----------------------------------------------------
> +
> +Non-volatile DIMMs are memory modules used to provide (cacheable) main memory

Are DIMMs always going to be the only form factor for NV memory?

And if you have multiple DIMMs, does each DT node correspond to a DIMM? 
If not, then what if we want/need to provide power control to a DIMM?

> +that retains its contents across power cycles. In more practical terms, they
> +are kind of storage device where the contents can be accessed by the CPU
> +directly, rather than indirectly via a storage controller or similar. The an
> +nvdimm-region specifies a physical address range that is hosted on an NVDIMM
> +device.
> +
> +Bindings for the region nodes:
> +-----------------------------
> +
> +Required properties:
> +	- compatible = "nvdimm-region"
> +
> +	- reg = <base, size>;
> +		The system physical address range of this nvdimm region.
> +
> +Optional properties:
> +	- Any relevant NUMA assocativity properties for the target platform.
> +	- A "volatile" property indicating that this region is actually in
> +	  normal DRAM and does not require cache flushes after each write.
> +
> +A complete example:
> +--------------------
> +
> +/ {
> +	#size-cells = <2>;
> +	#address-cells = <2>;
> +
> +	platform {

Perhaps we need a more well defined node here. Like we have 'memory' for 
memory nodes.

> +		region@5000 {
> +			compatible = "nvdimm-region;
> +			reg = <0x00000001 0x00000000 0x00000000 0x40000000>
> +
> +		};
> +
> +		region@6000 {
> +			compatible = "nvdimm-region";
> +			reg = <0x00000001 0x00000000 0x00000000 0x40000000>

Your reg property and unit-address don't match and you have overlapping 
regions.

> +			volatile;
> +		};
> +	};
> +};
> -- 
> 2.9.5
> 
> --
> To unsubscribe from this list: send the line "unsubscribe devicetree" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 6/6] doc/devicetree: NVDIMM region documentation

[Oliver]

On Tue, Mar 27, 2018 at 9:24 AM, Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> On Fri, Mar 23, 2018 at 07:12:09PM +1100, Oliver O'Halloran wrote:
>> Add device-tree binding documentation for the nvdimm region driver.
>>
>> Cc: devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
>> Signed-off-by: Oliver O'Halloran <oohall-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>> ---
>>  .../devicetree/bindings/nvdimm/nvdimm-region.txt   | 45 ++++++++++++++++++++++
>>  1 file changed, 45 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/nvdimm/nvdimm-region.txt
>>
>> diff --git a/Documentation/devicetree/bindings/nvdimm/nvdimm-region.txt b/Documentation/devicetree/bindings/nvdimm/nvdimm-region.txt
>> new file mode 100644
>> index 000000000000..02091117ff16
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/nvdimm/nvdimm-region.txt
>> @@ -0,0 +1,45 @@
>> +Device-tree bindings for NVDIMM memory regions
>> +-----------------------------------------------------
>> +
>> +Non-volatile DIMMs are memory modules used to provide (cacheable) main memory
>
> Are DIMMs always going to be the only form factor for NV memory?
>
> And if you have multiple DIMMs, does each DT node correspond to a DIMM?

A nvdimm-region might correspond to a single NVDIMM, a set of
interleaved NVDIMMs, or it might just be a chunk of normal memory that
you want treated as a NVDIMM for some reason. The last case is useful
for provisioning install media on servers since it allows you do
download a DVD image, turn it into an nvdimm-region, and kexec into
the installer which can use it as a root disk. That may seem a little
esoteric, but it's handy and we're using a full linux environment for
our boot loader so it's easy to make use of.

> If not, then what if we want/need to provide power control to a DIMM?

That would require a DIMM (and probably memory controller) specific
driver. I've deliberately left out how regions are mapped back to
DIMMs from the binding since it's not really clear to me how that
should work. A phandle array pointing to each DIMM device (which could
be anything) would do the trick, but I've found that a bit awkward to
plumb into the model that libnvdimm expects.

>> +that retains its contents across power cycles. In more practical terms, they
>> +are kind of storage device where the contents can be accessed by the CPU
>> +directly, rather than indirectly via a storage controller or similar. The an
>> +nvdimm-region specifies a physical address range that is hosted on an NVDIMM
>> +device.
>> +
>> +Bindings for the region nodes:
>> +-----------------------------
>> +
>> +Required properties:
>> +     - compatible = "nvdimm-region"
>> +
>> +     - reg = <base, size>;
>> +             The system physical address range of this nvdimm region.
>> +
>> +Optional properties:
>> +     - Any relevant NUMA assocativity properties for the target platform.
>> +     - A "volatile" property indicating that this region is actually in
>> +       normal DRAM and does not require cache flushes after each write.
>> +
>> +A complete example:
>> +--------------------
>> +
>> +/ {
>> +     #size-cells = <2>;
>> +     #address-cells = <2>;
>> +
>> +     platform {
>
> Perhaps we need a more well defined node here. Like we have 'memory' for
> memory nodes.

I think treating it as a platform device is fine. Memory nodes are
special since the OS needs to know where it can allocate early in boot
and I don't see non-volatile memory as being similarly significant.
Fundamentally an NVDIMM is just a memory mapped storage device so we
should be able to defer looking at them until later in boot.

That said you might have problems with XIP kernels and what not. I
think that problem is better solved through other means though.

>> +             region@5000 {
>> +                     compatible = "nvdimm-region;
>> +                     reg = <0x00000001 0x00000000 0x00000000 0x40000000>
>> +
>> +             };
>> +
>> +             region@6000 {
>> +                     compatible = "nvdimm-region";
>> +                     reg = <0x00000001 0x00000000 0x00000000 0x40000000>
>
> Your reg property and unit-address don't match and you have overlapping
> regions.

Yep, those are completely screwed up.

>> +                     volatile;
>> +             };
>> +     };
>> +};
>> --
>> 2.9.5
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe devicetree" in
>> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 6/6] doc/devicetree: NVDIMM region documentation

[Rob Herring]

On Tue, Mar 27, 2018 at 9:53 AM, Oliver <oohall-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> On Tue, Mar 27, 2018 at 9:24 AM, Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
>> On Fri, Mar 23, 2018 at 07:12:09PM +1100, Oliver O'Halloran wrote:
>>> Add device-tree binding documentation for the nvdimm region driver.
>>>
>>> Cc: devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
>>> Signed-off-by: Oliver O'Halloran <oohall-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>>> ---
>>>  .../devicetree/bindings/nvdimm/nvdimm-region.txt   | 45 ++++++++++++++++++++++
>>>  1 file changed, 45 insertions(+)
>>>  create mode 100644 Documentation/devicetree/bindings/nvdimm/nvdimm-region.txt
>>>
>>> diff --git a/Documentation/devicetree/bindings/nvdimm/nvdimm-region.txt b/Documentation/devicetree/bindings/nvdimm/nvdimm-region.txt
>>> new file mode 100644
>>> index 000000000000..02091117ff16
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/nvdimm/nvdimm-region.txt
>>> @@ -0,0 +1,45 @@
>>> +Device-tree bindings for NVDIMM memory regions
>>> +-----------------------------------------------------
>>> +
>>> +Non-volatile DIMMs are memory modules used to provide (cacheable) main memory
>>
>> Are DIMMs always going to be the only form factor for NV memory?
>>
>> And if you have multiple DIMMs, does each DT node correspond to a DIMM?
>
> A nvdimm-region might correspond to a single NVDIMM, a set of
> interleaved NVDIMMs, or it might just be a chunk of normal memory that
> you want treated as a NVDIMM for some reason. The last case is useful
> for provisioning install media on servers since it allows you do
> download a DVD image, turn it into an nvdimm-region, and kexec into
> the installer which can use it as a root disk. That may seem a little
> esoteric, but it's handy and we're using a full linux environment for
> our boot loader so it's easy to make use of.

I'm really just asking if we should drop the "dimm" name because it is
not always a DIMM. Maybe pmem instead? I don't know, naming is
hard(TM).

>> If not, then what if we want/need to provide power control to a DIMM?
>
> That would require a DIMM (and probably memory controller) specific
> driver. I've deliberately left out how regions are mapped back to
> DIMMs from the binding since it's not really clear to me how that
> should work. A phandle array pointing to each DIMM device (which could
> be anything) would do the trick, but I've found that a bit awkward to
> plumb into the model that libnvdimm expects.
>
>>> +that retains its contents across power cycles. In more practical terms, they
>>> +are kind of storage device where the contents can be accessed by the CPU
>>> +directly, rather than indirectly via a storage controller or similar. The an
>>> +nvdimm-region specifies a physical address range that is hosted on an NVDIMM
>>> +device.
>>> +
>>> +Bindings for the region nodes:
>>> +-----------------------------
>>> +
>>> +Required properties:
>>> +     - compatible = "nvdimm-region"
>>> +
>>> +     - reg = <base, size>;
>>> +             The system physical address range of this nvdimm region.
>>> +
>>> +Optional properties:
>>> +     - Any relevant NUMA assocativity properties for the target platform.
>>> +     - A "volatile" property indicating that this region is actually in
>>> +       normal DRAM and does not require cache flushes after each write.
>>> +
>>> +A complete example:
>>> +--------------------
>>> +
>>> +/ {
>>> +     #size-cells = <2>;
>>> +     #address-cells = <2>;
>>> +
>>> +     platform {
>>
>> Perhaps we need a more well defined node here. Like we have 'memory' for
>> memory nodes.
>
> I think treating it as a platform device is fine. Memory nodes are

Platform device is a Linux term...

> special since the OS needs to know where it can allocate early in boot
> and I don't see non-volatile memory as being similarly significant.
> Fundamentally an NVDIMM is just a memory mapped storage device so we
> should be able to defer looking at them until later in boot.

It's not clear if 'platform' is just an example or random name or what
the node is required to be called. In the latter case, we should be
much more specific because 'platform' could be anything. In the former
case, then we have no way to find or validate the node because the
name could be anything and there's no compatible property either.

"region" is pretty generic too.

>
> That said you might have problems with XIP kernels and what not. I
> think that problem is better solved through other means though.
>
>>> +             region@5000 {
>>> +                     compatible = "nvdimm-region;
>>> +                     reg = <0x00000001 0x00000000 0x00000000 0x40000000>
>>> +
>>> +             };
>>> +
>>> +             region@6000 {
>>> +                     compatible = "nvdimm-region";
>>> +                     reg = <0x00000001 0x00000000 0x00000000 0x40000000>

Thinking about this some more, the 2 levels of nodes is pointless.
Just follow memory nodes structure.

nv-memory@100000000 {
  compatible = "nvdimm-region";
  reg = <0x00000001 0x00000000 0x00000000 0x40000000>;
};

nv-memory@200000000 {
  compatible = "nvdimm-region";
  reg = <0x00000002 0x00000000 0x00000000 0x40000000>;
};

or:

nv-memory@100000000 {
  compatible = "nvdimm-region";
  reg = <0x00000001 0x00000000 0x00000000 0x40000000>
    <0x00000002 0x00000000 0x00000000 0x40000000>;
};

Both forms should be allowed.

Rob

Re: [PATCH 6/6] doc/devicetree: NVDIMM region documentation

[Dan Williams]

On Wed, Mar 28, 2018 at 10:06 AM, Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
[..]
> >> Are DIMMs always going to be the only form factor for NV memory?
> >>
> >> And if you have multiple DIMMs, does each DT node correspond to a DIMM?
> >
> > A nvdimm-region might correspond to a single NVDIMM, a set of
> > interleaved NVDIMMs, or it might just be a chunk of normal memory that
> > you want treated as a NVDIMM for some reason. The last case is useful
> > for provisioning install media on servers since it allows you do
> > download a DVD image, turn it into an nvdimm-region, and kexec into
> > the installer which can use it as a root disk. That may seem a little
> > esoteric, but it's handy and we're using a full linux environment for
> > our boot loader so it's easy to make use of.
>
> I'm really just asking if we should drop the "dimm" name because it is
> not always a DIMM. Maybe pmem instead? I don't know, naming is
> hard(TM).

The Linux enabling uses the term "memory device". The Linux device
object name for memory devices is "nmem".

[..]
> > special since the OS needs to know where it can allocate early in boot
> > and I don't see non-volatile memory as being similarly significant.
> > Fundamentally an NVDIMM is just a memory mapped storage device so we
> > should be able to defer looking at them until later in boot.
>
> It's not clear if 'platform' is just an example or random name or what
> the node is required to be called. In the latter case, we should be
> much more specific because 'platform' could be anything. In the former
> case, then we have no way to find or validate the node because the
> name could be anything and there's no compatible property either.
>
> "region" is pretty generic too.
>

The term "nvdimm-region" has specific meaning to the libnvdimm
sub-system. It is a contiguous physical address range backed by one or
more memory devices, DIMMs, in an interleaved configuration
(interleave set).

One feature that is currently missing from libnvdimm is a management
interface to change an interleave configuration. To date, Linux only
reads a static region configuration published by platform firmware.
Linux can provide dynamic provisioning of namespaces out of those
regions, but interleave configuration has been left to vendor specific
tooling to date. It would be great to start incorporating generic
Linux support for that capability across platform firmware
implementations.

Re: [PATCH 6/6] doc/devicetree: NVDIMM region documentation

[Oliver]

On Thu, Mar 29, 2018 at 4:06 AM, Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> On Tue, Mar 27, 2018 at 9:53 AM, Oliver <oohall-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>> On Tue, Mar 27, 2018 at 9:24 AM, Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
>>> On Fri, Mar 23, 2018 at 07:12:09PM +1100, Oliver O'Halloran wrote:
>>>> Add device-tree binding documentation for the nvdimm region driver.
>>>>
>>>> Cc: devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
>>>> Signed-off-by: Oliver O'Halloran <oohall-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>>>> ---
>>>>  .../devicetree/bindings/nvdimm/nvdimm-region.txt   | 45 ++++++++++++++++++++++
>>>>  1 file changed, 45 insertions(+)
>>>>  create mode 100644 Documentation/devicetree/bindings/nvdimm/nvdimm-region.txt
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/nvdimm/nvdimm-region.txt b/Documentation/devicetree/bindings/nvdimm/nvdimm-region.txt
>>>> new file mode 100644
>>>> index 000000000000..02091117ff16
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/nvdimm/nvdimm-region.txt
>>>> @@ -0,0 +1,45 @@
>>>> +Device-tree bindings for NVDIMM memory regions
>>>> +-----------------------------------------------------
>>>> +
>>>> +Non-volatile DIMMs are memory modules used to provide (cacheable) main memory
>>>
>>> Are DIMMs always going to be the only form factor for NV memory?
>>>
>>> And if you have multiple DIMMs, does each DT node correspond to a DIMM?
>>
>> A nvdimm-region might correspond to a single NVDIMM, a set of
>> interleaved NVDIMMs, or it might just be a chunk of normal memory that
>> you want treated as a NVDIMM for some reason. The last case is useful
>> for provisioning install media on servers since it allows you do
>> download a DVD image, turn it into an nvdimm-region, and kexec into
>> the installer which can use it as a root disk. That may seem a little
>> esoteric, but it's handy and we're using a full linux environment for
>> our boot loader so it's easy to make use of.
>
> I'm really just asking if we should drop the "dimm" name because it is
> not always a DIMM. Maybe pmem instead? I don't know, naming is
> hard(TM).

pmem is probably a better name. I'll fix that up.

>>> If not, then what if we want/need to provide power control to a DIMM?
>>
>> That would require a DIMM (and probably memory controller) specific
>> driver. I've deliberately left out how regions are mapped back to
>> DIMMs from the binding since it's not really clear to me how that
>> should work. A phandle array pointing to each DIMM device (which could
>> be anything) would do the trick, but I've found that a bit awkward to
>> plumb into the model that libnvdimm expects.
>>
>>>> +that retains its contents across power cycles. In more practical terms, they
>>>> +are kind of storage device where the contents can be accessed by the CPU
>>>> +directly, rather than indirectly via a storage controller or similar. The an
>>>> +nvdimm-region specifies a physical address range that is hosted on an NVDIMM
>>>> +device.
>>>> +
>>>> +Bindings for the region nodes:
>>>> +-----------------------------
>>>> +
>>>> +Required properties:
>>>> +     - compatible = "nvdimm-region"
>>>> +
>>>> +     - reg = <base, size>;
>>>> +             The system physical address range of this nvdimm region.
>>>> +
>>>> +Optional properties:
>>>> +     - Any relevant NUMA assocativity properties for the target platform.
>>>> +     - A "volatile" property indicating that this region is actually in
>>>> +       normal DRAM and does not require cache flushes after each write.
>>>> +
>>>> +A complete example:
>>>> +--------------------
>>>> +
>>>> +/ {
>>>> +     #size-cells = <2>;
>>>> +     #address-cells = <2>;
>>>> +
>>>> +     platform {
>>>
>>> Perhaps we need a more well defined node here. Like we have 'memory' for
>>> memory nodes.
>>
>> I think treating it as a platform device is fine. Memory nodes are
>
> Platform device is a Linux term...
>
>> special since the OS needs to know where it can allocate early in boot
>> and I don't see non-volatile memory as being similarly significant.
>> Fundamentally an NVDIMM is just a memory mapped storage device so we
>> should be able to defer looking at them until later in boot.
>
> It's not clear if 'platform' is just an example or random name or what
> the node is required to be called. In the latter case, we should be
> much more specific because 'platform' could be anything. In the former
> case, then we have no way to find or validate the node because the
> name could be anything and there's no compatible property either.

Sorry, the platform node is just there as an example. I'll remove it.

> "region" is pretty generic too.

It is, but I didn't see a compelling reason to call it something else.

>> That said you might have problems with XIP kernels and what not. I
>> think that problem is better solved through other means though.
>>
>>>> +             region@5000 {
>>>> +                     compatible = "nvdimm-region;
>>>> +                     reg = <0x00000001 0x00000000 0x00000000 0x40000000>
>>>> +
>>>> +             };
>>>> +
>>>> +             region@6000 {
>>>> +                     compatible = "nvdimm-region";
>>>> +                     reg = <0x00000001 0x00000000 0x00000000 0x40000000>
>
> Thinking about this some more, the 2 levels of nodes is pointless.
> Just follow memory nodes structure.
>
> nv-memory@100000000 {
>   compatible = "nvdimm-region";
>   reg = <0x00000001 0x00000000 0x00000000 0x40000000>;
> };
>
> nv-memory@200000000 {
>   compatible = "nvdimm-region";
>   reg = <0x00000002 0x00000000 0x00000000 0x40000000>;
> };
>
> or:
>
> nv-memory@100000000 {
>   compatible = "nvdimm-region";
>   reg = <0x00000001 0x00000000 0x00000000 0x40000000>
>     <0x00000002 0x00000000 0x00000000 0x40000000>;
> };
>
> Both forms should be allowed.

In the example you need two separate nodes since one has the
"volatile" property to indicate it's backed by normal memory while the
other doesn't. That detail is important since the OS can skip doing
cache flushes when writing to a region that it knows is volatile.

Anyway, the usefulness of having multiple ranges in the reg is a bit
dubious since you should never see dis-contiguous ranges of memory
backed by the same devices. Keep in mind that this binding here is
deliberately skeletal and leaves out the parts required to map the
region to the backing devices, once that is added there's not going to
be a whole lot of room for coalescing nodes. That said, I'll add
support for it anyway since it might be nice to have for hand-written
DTs (ours are mostly generated by FW).

Thanks,
Oliver

Re: [PATCH 1/6] libnvdimm: Add of_node to region and bus descriptors

[Balbir Singh]

On Fri, 23 Mar 2018 19:12:04 +1100
Oliver O'Halloran <oohall-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:

> We want to be able to cross reference the region and bus devices
> with the device tree node that they were spawned from. libNVDIMM
> handles creating the actual devices for these internally, so we
> need to pass in a pointer to the relevant node in the descriptor.
> 
> Signed-off-by: Oliver O'Halloran <oohall-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> Acked-by: Dan Williams <dan.j.williams-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> ---

Acked-by: Balbir Singh <bsingharora-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>