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

[PATCH 1/3] 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@gmail.com>
---
 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 0a5e6cd758fe..d6f94303ae44 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 abaf38c61220..4d756d8c8386 100644
--- a/drivers/nvdimm/region_devs.c
+++ b/drivers/nvdimm/region_devs.c
@@ -992,6 +992,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 f8109ddb5ef1..fcd9f521e481 100644
--- a/include/linux/libnvdimm.h
+++ b/include/linux/libnvdimm.h
@@ -65,12 +65,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,
@@ -112,6 +114,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/3] libnvdimm: Add a device-tree interface

[Oliver O'Halloran]

A fairly bare-bones set of device-tree bindings so libnvdimm can be used
on powerpc and other device-tree based platforms.

Cc: devicetree@vger.kernel.org
Signed-off-by: Oliver O'Halloran <oohall@gmail.com>
---
 .../devicetree/bindings/nvdimm/nvdimm-bus.txt      |  69 +++++++
 MAINTAINERS                                        |   8 +
 drivers/nvdimm/Kconfig                             |  10 +
 drivers/nvdimm/Makefile                            |   1 +
 drivers/nvdimm/of_nvdimm.c                         | 202 +++++++++++++++++++++
 5 files changed, 290 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/nvdimm/nvdimm-bus.txt
 create mode 100644 drivers/nvdimm/of_nvdimm.c

diff --git a/Documentation/devicetree/bindings/nvdimm/nvdimm-bus.txt b/Documentation/devicetree/bindings/nvdimm/nvdimm-bus.txt
new file mode 100644
index 000000000000..491e7c4900ed
--- /dev/null
+++ b/Documentation/devicetree/bindings/nvdimm/nvdimm-bus.txt
@@ -0,0 +1,69 @@
+Device-tree bindings for nonvolatile-memory / NVDIMMs
+-----------------------------------------------------
+
+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. This can provide
+substantial performance improvements for applications designed to take
+advantage of in-memory storage.
+
+This binding provides a way to describe memory regions that should be managed
+by an NVDIMM storage driver (libNVDIMM in Linux) and some of the associated
+metadata. The binding itself is split into two main parts: A container bus and
+its sub-nodes which describe which memory address ranges corresponding to
+NVDIMM backed memory.
+
+Bindings for the container bus:
+------------------------------
+
+Required properties:
+ - compatible = "nvdimm-bus";
+ - ranges;
+	A blank ranges property is required because the sub-nodes have
+	addresses in the system's physical address space.
+
+The use of a container bus is mainly to handle future expansion of the binding. For
+comparison the ACPI equivalent of this binding (NFIT) describes: Memory regions, DIMM
+control structures, Block mode DIMM control structures, interleave sets, and more. Some
+of these structures cross reference each other so everyone should be happier if we keep
+it relatively self contained.
+
+Bindings for the region nodes:
+-----------------------------
+
+Required properties:
+	- compatible = "nvdimm-persistent" or "nvdimm-volatile"
+
+		The "nvdimm-persistent" region type indicates that this memory region
+		is actually a persistent region. The volatile type is mainly useful
+		for testing and RAM disks that can persist across kexec.
+
+	- reg = <base, size>;
+		The reg property should only contain one address range.
+
+Optional properties:
+	- Any relevant NUMA assocativity properties for the target platform.
+
+A complete example:
+--------------------
+
+/ {
+	#size-cells = <1>;
+	#address-cells = <1>;
+
+	nonvolatile-memory {
+		compatible = "nonvolatile-memory";
+		ranges;
+
+		region@5000 {
+			compatible = "nvdimm-persistent";
+			reg = <0x5000 0x1000>;
+		};
+
+		region@6000 {
+			compatible = "nvdimm-volatile";
+			reg = <0x6000 0x1000>;
+		};
+	};
+};
diff --git a/MAINTAINERS b/MAINTAINERS
index 65eff7857ec3..0350bf5a94d2 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -7875,6 +7875,14 @@ Q:	https://patchwork.kernel.org/project/linux-nvdimm/list/
 S:	Supported
 F:	drivers/nvdimm/pmem*
 
+LIBNVDIMM: DEVICETREE BINDINGS
+M:	Oliver O'Halloran <oohall@gmail.com>
+L:	linux-nvdimm@lists.01.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@intel.com>
 L:	linux-nvdimm@lists.01.org
diff --git a/drivers/nvdimm/Kconfig b/drivers/nvdimm/Kconfig
index 5bdd499b5f4f..72d147b55596 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 ca6d325438a3..9029511a8486 100644
--- a/drivers/nvdimm/Makefile
+++ b/drivers/nvdimm/Makefile
@@ -3,6 +3,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..765cbbae8bcb
--- /dev/null
+++ b/drivers/nvdimm/of_nvdimm.c
@@ -0,0 +1,202 @@
+/*
+ * Copyright 2017, IBM Corporation
+ *
+ * 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.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, you can access it online at
+ * http://www.gnu.org/licenses/gpl-2.0.html.
+ */
+
+#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>
+
+static const struct attribute_group *region_attr_groups[] = {
+	&nd_region_attribute_group,
+	&nd_device_attribute_group,
+	NULL,
+};
+
+static int of_nvdimm_add_byte(struct nvdimm_bus *bus, struct device_node *np)
+{
+	struct nd_region_desc ndr_desc;
+	struct resource temp_res;
+	struct nd_region *region;
+
+	/*
+	 * byte regions should only have one address range
+	 */
+	if (of_address_to_resource(np, 0, &temp_res)) {
+		pr_warn("Unable to parse reg[0] for %pOF\n", np);
+		return -ENXIO;
+	}
+
+	pr_debug("Found %pR for %pOF\n", &temp_res, np);
+
+	memset(&ndr_desc, 0, sizeof(ndr_desc));
+	ndr_desc.res = &temp_res;
+	ndr_desc.of_node = np;
+	ndr_desc.attr_groups = region_attr_groups;
+#ifdef CONFIG_NUMA
+	ndr_desc.numa_node = of_node_to_nid(np);
+#endif
+	set_bit(ND_REGION_PAGEMAP, &ndr_desc.flags);
+
+	if (of_device_is_compatible(np, "nvdimm-volatile"))
+		region = nvdimm_volatile_region_create(bus, &ndr_desc);
+	else
+		region = nvdimm_pmem_region_create(bus, &ndr_desc);
+
+	if (!region)
+		return -ENXIO;
+
+	return 0;
+}
+
+static const struct of_device_id of_nvdimm_dev_types[] = {
+	{ .compatible = "nvdimm-persistent", .data = of_nvdimm_add_byte },
+	{ .compatible = "nvdimm-volatile", .data = of_nvdimm_add_byte },
+	{ },
+};
+
+static void of_nvdimm_parse_one(struct nvdimm_bus *bus,
+		struct device_node *node)
+{
+	int (*parse_node)(struct nvdimm_bus *, struct device_node *);
+	const struct of_device_id *match;
+	int rc;
+
+	if (of_node_test_and_set_flag(node, OF_POPULATED)) {
+		pr_debug("%pOF already parsed, skipping\n", node);
+		return;
+	}
+
+	match = of_match_node(of_nvdimm_dev_types, node);
+	if (!match) {
+		pr_info("No compatible match for '%pOF'\n", node);
+		of_node_clear_flag(node, OF_POPULATED);
+		return;
+	}
+
+	of_node_get(node);
+	parse_node = match->data;
+	rc = parse_node(bus, node);
+
+	if (rc) {
+		of_node_clear_flag(node, OF_POPULATED);
+		of_node_put(node);
+	}
+
+	pr_debug("Parsed %pOF, rc = %d\n", node, rc);
+
+	return;
+}
+
+/*
+ * The nvdimm core refers to the bus descriptor structure at runtime
+ * so we need to keep it around. Note that that this is different to
+ * the other nd_*_desc types which are essentially containers for extra
+ * function parameters.
+ */
+struct of_nd_private {
+	struct nvdimm_bus_descriptor desc;
+	struct nvdimm_bus *bus;
+};
+
+static const struct attribute_group *bus_attr_groups[] = {
+	&nvdimm_bus_attribute_group,
+	NULL,
+};
+
+static int of_nvdimm_probe(struct platform_device *pdev)
+{
+	struct device_node *node, *child;
+	struct of_nd_private *priv;
+
+	node = dev_of_node(&pdev->dev);
+	if (!node)
+		return -ENXIO;
+
+	priv = kzalloc(sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	priv->desc.attr_groups = bus_attr_groups;
+	priv->desc.provider_name = "of_nvdimm";
+	priv->desc.module = THIS_MODULE;
+	priv->desc.of_node = node;
+
+	priv->bus = nvdimm_bus_register(&pdev->dev, &priv->desc);
+	if (!priv->bus)
+		goto err;
+
+	platform_set_drvdata(pdev, priv);
+
+	/* now walk the node bus and setup regions, etc */
+	for_each_available_child_of_node(node, child)
+		of_nvdimm_parse_one(priv->bus, child);
+
+	return 0;
+
+err:
+	nvdimm_bus_unregister(priv->bus);
+	kfree(priv);
+	return -ENXIO;
+}
+
+static int of_nvdimm_remove(struct platform_device *pdev)
+{
+	struct of_nd_private *priv = platform_get_drvdata(pdev);
+	struct device_node *node;
+
+	if (!priv)
+		return 0; /* possible? */
+
+	for_each_available_child_of_node(pdev->dev.of_node, node) {
+		if (!of_node_check_flag(node, OF_POPULATED))
+			continue;
+
+		of_node_clear_flag(node, OF_POPULATED);
+		of_node_put(node);
+		pr_debug("de-populating %s\n", node->full_name);
+	}
+
+	nvdimm_bus_unregister(priv->bus);
+	kfree(priv);
+
+	return 0;
+}
+
+static const struct of_device_id of_nvdimm_bus_match[] = {
+	{ .compatible = "nvdimm-bus" },
+	{ },
+};
+
+static const struct platform_driver of_nvdimm_driver = {
+	.probe = of_nvdimm_probe,
+	.remove = of_nvdimm_remove,
+	.driver = {
+		.name = "of_nvdimm",
+		.owner = THIS_MODULE,
+		.of_match_table = of_nvdimm_bus_match,
+	},
+};
+
+module_platform_driver(of_nvdimm_driver);
+MODULE_DEVICE_TABLE(of, of_nvdimm_bus_match);
+MODULE_LICENSE("GPL v2");
+MODULE_AUTHOR("IBM Corporation");
-- 
2.9.5

Re: [PATCH 2/3] libnvdimm: Add a device-tree interface

[Rob Herring]

On Thu, Nov 16, 2017 at 04:51:30AM +1100, Oliver O'Halloran wrote:
> A fairly bare-bones set of device-tree bindings so libnvdimm can be used
> on powerpc and other device-tree based platforms.
> 
> Cc: devicetree@vger.kernel.org
> Signed-off-by: Oliver O'Halloran <oohall@gmail.com>
> ---
>  .../devicetree/bindings/nvdimm/nvdimm-bus.txt      |  69 +++++++
>  MAINTAINERS                                        |   8 +
>  drivers/nvdimm/Kconfig                             |  10 +
>  drivers/nvdimm/Makefile                            |   1 +
>  drivers/nvdimm/of_nvdimm.c                         | 202 +++++++++++++++++++++
>  5 files changed, 290 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/nvdimm/nvdimm-bus.txt
>  create mode 100644 drivers/nvdimm/of_nvdimm.c
> 
> diff --git a/Documentation/devicetree/bindings/nvdimm/nvdimm-bus.txt b/Documentation/devicetree/bindings/nvdimm/nvdimm-bus.txt
> new file mode 100644
> index 000000000000..491e7c4900ed
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/nvdimm/nvdimm-bus.txt
> @@ -0,0 +1,69 @@
> +Device-tree bindings for nonvolatile-memory / NVDIMMs
> +-----------------------------------------------------
> +
> +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. This can provide
> +substantial performance improvements for applications designed to take
> +advantage of in-memory storage.
> +
> +This binding provides a way to describe memory regions that should be managed
> +by an NVDIMM storage driver (libNVDIMM in Linux) and some of the associated
> +metadata. The binding itself is split into two main parts: A container bus and
> +its sub-nodes which describe which memory address ranges corresponding to
> +NVDIMM backed memory.
> +
> +Bindings for the container bus:
> +------------------------------
> +
> +Required properties:
> + - compatible = "nvdimm-bus";
> + - ranges;
> +	A blank ranges property is required because the sub-nodes have
> +	addresses in the system's physical address space.
> +
> +The use of a container bus is mainly to handle future expansion of the binding. For
> +comparison the ACPI equivalent of this binding (NFIT) describes: Memory regions, DIMM
> +control structures, Block mode DIMM control structures, interleave sets, and more. Some
> +of these structures cross reference each other so everyone should be happier if we keep
> +it relatively self contained.

Will adding any of these things need unit addresses and colide with the 
existing nodes below? IOW, at one level there's only 1 number space.

> +
> +Bindings for the region nodes:
> +-----------------------------
> +
> +Required properties:
> +	- compatible = "nvdimm-persistent" or "nvdimm-volatile"
> +
> +		The "nvdimm-persistent" region type indicates that this memory region
> +		is actually a persistent region. The volatile type is mainly useful
> +		for testing and RAM disks that can persist across kexec.
> +
> +	- reg = <base, size>;
> +		The reg property should only contain one address range.
> +
> +Optional properties:
> +	- Any relevant NUMA assocativity properties for the target platform.
> +
> +A complete example:
> +--------------------
> +
> +/ {
> +	#size-cells = <1>;
> +	#address-cells = <1>;
> +
> +	nonvolatile-memory {
> +		compatible = "nonvolatile-memory";
> +		ranges;
> +
> +		region@5000 {
> +			compatible = "nvdimm-persistent";
> +			reg = <0x5000 0x1000>;
> +		};
> +
> +		region@6000 {
> +			compatible = "nvdimm-volatile";
> +			reg = <0x6000 0x1000>;
> +		};
> +	};
> +};
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 65eff7857ec3..0350bf5a94d2 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -7875,6 +7875,14 @@ Q:	https://patchwork.kernel.org/project/linux-nvdimm/list/
>  S:	Supported
>  F:	drivers/nvdimm/pmem*
>  
> +LIBNVDIMM: DEVICETREE BINDINGS
> +M:	Oliver O'Halloran <oohall@gmail.com>
> +L:	linux-nvdimm@lists.01.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@intel.com>
>  L:	linux-nvdimm@lists.01.org
> diff --git a/drivers/nvdimm/Kconfig b/drivers/nvdimm/Kconfig
> index 5bdd499b5f4f..72d147b55596 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 ca6d325438a3..9029511a8486 100644
> --- a/drivers/nvdimm/Makefile
> +++ b/drivers/nvdimm/Makefile
> @@ -3,6 +3,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..765cbbae8bcb
> --- /dev/null
> +++ b/drivers/nvdimm/of_nvdimm.c
> @@ -0,0 +1,202 @@
> +/*
> + * Copyright 2017, IBM Corporation
> + *
> + * 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.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, you can access it online at
> + * http://www.gnu.org/licenses/gpl-2.0.html.

This can be SPDX tag.

> + */
> +
> +#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>
> +
> +static const struct attribute_group *region_attr_groups[] = {
> +	&nd_region_attribute_group,
> +	&nd_device_attribute_group,
> +	NULL,
> +};
> +
> +static int of_nvdimm_add_byte(struct nvdimm_bus *bus, struct device_node *np)

_add_byte_region() would be more clear.

> +{
> +	struct nd_region_desc ndr_desc;
> +	struct resource temp_res;
> +	struct nd_region *region;
> +
> +	/*
> +	 * byte regions should only have one address range
> +	 */
> +	if (of_address_to_resource(np, 0, &temp_res)) {
> +		pr_warn("Unable to parse reg[0] for %pOF\n", np);
> +		return -ENXIO;
> +	}
> +
> +	pr_debug("Found %pR for %pOF\n", &temp_res, np);
> +
> +	memset(&ndr_desc, 0, sizeof(ndr_desc));
> +	ndr_desc.res = &temp_res;
> +	ndr_desc.of_node = np;
> +	ndr_desc.attr_groups = region_attr_groups;
> +#ifdef CONFIG_NUMA

I believe of_node_to_nid will do the right thing for !CONFIG_NUMA and 
this can be dropped.

> +	ndr_desc.numa_node = of_node_to_nid(np);
> +#endif
> +	set_bit(ND_REGION_PAGEMAP, &ndr_desc.flags);
> +
> +	if (of_device_is_compatible(np, "nvdimm-volatile"))

You've already matched the node once with of_match_node, don't do it 
again.

> +		region = nvdimm_volatile_region_create(bus, &ndr_desc);
> +	else
> +		region = nvdimm_pmem_region_create(bus, &ndr_desc);
> +
> +	if (!region)
> +		return -ENXIO;
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id of_nvdimm_dev_types[] = {
> +	{ .compatible = "nvdimm-persistent", .data = of_nvdimm_add_byte },
> +	{ .compatible = "nvdimm-volatile", .data = of_nvdimm_add_byte },
> +	{ },
> +};
> +
> +static void of_nvdimm_parse_one(struct nvdimm_bus *bus,
> +		struct device_node *node)
> +{
> +	int (*parse_node)(struct nvdimm_bus *, struct device_node *);
> +	const struct of_device_id *match;
> +	int rc;
> +
> +	if (of_node_test_and_set_flag(node, OF_POPULATED)) {

Why not make the regions platform devices too? Then you don't need your 
own populate code.

> +		pr_debug("%pOF already parsed, skipping\n", node);
> +		return;
> +	}
> +
> +	match = of_match_node(of_nvdimm_dev_types, node);
> +	if (!match) {
> +		pr_info("No compatible match for '%pOF'\n", node);
> +		of_node_clear_flag(node, OF_POPULATED);
> +		return;
> +	}
> +
> +	of_node_get(node);
> +	parse_node = match->data;

This makes no sense. I assume you plan to add some other region type 
later?

> +	rc = parse_node(bus, node);
> +
> +	if (rc) {
> +		of_node_clear_flag(node, OF_POPULATED);
> +		of_node_put(node);
> +	}
> +
> +	pr_debug("Parsed %pOF, rc = %d\n", node, rc);
> +
> +	return;
> +}
> +
> +/*
> + * The nvdimm core refers to the bus descriptor structure at runtime
> + * so we need to keep it around. Note that that this is different to
> + * the other nd_*_desc types which are essentially containers for extra
> + * function parameters.
> + */
> +struct of_nd_private {
> +	struct nvdimm_bus_descriptor desc;
> +	struct nvdimm_bus *bus;
> +};
> +
> +static const struct attribute_group *bus_attr_groups[] = {
> +	&nvdimm_bus_attribute_group,
> +	NULL,
> +};
> +
> +static int of_nvdimm_probe(struct platform_device *pdev)
> +{
> +	struct device_node *node, *child;
> +	struct of_nd_private *priv;
> +
> +	node = dev_of_node(&pdev->dev);
> +	if (!node)
> +		return -ENXIO;
> +
> +	priv = kzalloc(sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	priv->desc.attr_groups = bus_attr_groups;
> +	priv->desc.provider_name = "of_nvdimm";
> +	priv->desc.module = THIS_MODULE;
> +	priv->desc.of_node = node;
> +
> +	priv->bus = nvdimm_bus_register(&pdev->dev, &priv->desc);
> +	if (!priv->bus)
> +		goto err;
> +
> +	platform_set_drvdata(pdev, priv);
> +
> +	/* now walk the node bus and setup regions, etc */
> +	for_each_available_child_of_node(node, child)
> +		of_nvdimm_parse_one(priv->bus, child);
> +
> +	return 0;
> +
> +err:
> +	nvdimm_bus_unregister(priv->bus);
> +	kfree(priv);
> +	return -ENXIO;
> +}
> +
> +static int of_nvdimm_remove(struct platform_device *pdev)
> +{
> +	struct of_nd_private *priv = platform_get_drvdata(pdev);
> +	struct device_node *node;
> +
> +	if (!priv)
> +		return 0; /* possible? */
> +
> +	for_each_available_child_of_node(pdev->dev.of_node, node) {
> +		if (!of_node_check_flag(node, OF_POPULATED))
> +			continue;
> +
> +		of_node_clear_flag(node, OF_POPULATED);
> +		of_node_put(node);
> +		pr_debug("de-populating %s\n", node->full_name);
> +	}
> +
> +	nvdimm_bus_unregister(priv->bus);
> +	kfree(priv);
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id of_nvdimm_bus_match[] = {
> +	{ .compatible = "nvdimm-bus" },
> +	{ },
> +};
> +
> +static const struct platform_driver of_nvdimm_driver = {
> +	.probe = of_nvdimm_probe,
> +	.remove = of_nvdimm_remove,
> +	.driver = {
> +		.name = "of_nvdimm",
> +		.owner = THIS_MODULE,
> +		.of_match_table = of_nvdimm_bus_match,
> +	},
> +};
> +
> +module_platform_driver(of_nvdimm_driver);
> +MODULE_DEVICE_TABLE(of, of_nvdimm_bus_match);
> +MODULE_LICENSE("GPL v2");
> +MODULE_AUTHOR("IBM Corporation");
> -- 
> 2.9.5
> 
> --
> To unsubscribe from this list: send the line "unsubscribe devicetree" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/3] libnvdimm: Add a device-tree interface

[Michael Ellerman]

Rob Herring <robh@kernel.org> writes:
> On Thu, Nov 16, 2017 at 04:51:30AM +1100, Oliver O'Halloran wrote:
...
>> diff --git a/drivers/nvdimm/of_nvdimm.c b/drivers/nvdimm/of_nvdimm.c
>> new file mode 100644
>> index 000000000000..765cbbae8bcb
>> --- /dev/null
>> +++ b/drivers/nvdimm/of_nvdimm.c
>> @@ -0,0 +1,202 @@
>> +/*
>> + * Copyright 2017, IBM Corporation
>> + *
>> + * 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.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> + * GNU General Public License for more details.
>> + *
>> + * You should have received a copy of the GNU General Public License
>> + * along with this program; if not, you can access it online at
>> + * http://www.gnu.org/licenses/gpl-2.0.html.
>
> This can be SPDX tag.

Specifically:

// SPDX-License-Identifier: GPL-2.0+
/*
 * Copyright 2017, IBM Corporation
 */

...

cheers

Re: [PATCH 2/3] libnvdimm: Add a device-tree interface

[Rob Herring]

On Thu, Nov 16, 2017 at 04:51:30AM +1100, Oliver O'Halloran wrote:
> A fairly bare-bones set of device-tree bindings so libnvdimm can be used
> on powerpc and other device-tree based platforms.
> 
> Cc: devicetree@vger.kernel.org
> Signed-off-by: Oliver O'Halloran <oohall@gmail.com>
> ---
>  .../devicetree/bindings/nvdimm/nvdimm-bus.txt      |  69 +++++++

Also, please split bindings to a separate patch.

>  MAINTAINERS                                        |   8 +
>  drivers/nvdimm/Kconfig                             |  10 +
>  drivers/nvdimm/Makefile                            |   1 +
>  drivers/nvdimm/of_nvdimm.c                         | 202 +++++++++++++++++++++
>  5 files changed, 290 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/nvdimm/nvdimm-bus.txt
>  create mode 100644 drivers/nvdimm/of_nvdimm.c

[PATCH 3/3] 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@gmail.com>
---
 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 65c79ecf5a4d..32df0d44f086 100644
--- a/arch/powerpc/platforms/powernv/opal.c
+++ b/arch/powerpc/platforms/powernv/opal.c
@@ -831,6 +831,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-bus");
+
 	/* Setup a heatbeat thread if requested by OPAL */
 	opal_init_heartbeat();
 
-- 
2.9.5

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

[Dan Williams]

On Wed, Nov 15, 2017 at 9:51 AM, Oliver O'Halloran <oohall@gmail.com> 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@gmail.com>

Looks ok to me,

Acked-by: Dan Williams <dan.j.williams@intel.com>

...this is late if you were expecting a 4.15 merge.

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

[Oliver]

On Thu, Nov 16, 2017 at 5:05 AM, Dan Williams <dan.j.williams@gmail.com> wrote:
> On Wed, Nov 15, 2017 at 9:51 AM, Oliver O'Halloran <oohall@gmail.com> 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@gmail.com>
>
> Looks ok to me,
>
> Acked-by: Dan Williams <dan.j.williams@intel.com>
>
> ...this is late if you were expecting a 4.15 merge.

I'm not that optimistic, but I wouldn't complain.