libnvdimm: Add of_node to region and bus descriptors

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 <[email protected]>

---
 drivers/nvdimm/region_devs.c | 6 ++++++
 include/linux/libnvdimm.h    | 1 +
 2 files changed, 7 insertions(+)

drivers/nvdimm/region_devs.c +0 -0

index 2f1d5771100e..76f46fd1fae0 100644

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 <[email protected]>

---
 drivers/nvdimm/region_devs.c | 6 ++++++
 include/linux/libnvdimm.h    | 1 +
 2 files changed, 7 insertions(+)

drivers/nvdimm/region_devs.c +0 -0

index 2f1d5771100e..76f46fd1fae0 100644

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 <[email protected]>

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

Quoted 1 line · Oliver O'Halloran

This patch adds peliminary device-tree bindings for the NVDIMM driver.

*preliminary

Quoted 10 lines

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 <[email protected]>

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

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 <[email protected]>

---

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

MAINTAINERS +8 -0

index 4e62756936fa..e3fc47fbfc7a 100644
@@ -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 <[email protected]>
+L:	[email protected]
+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 <[email protected]>
 L:	[email protected]

drivers/nvdimm/Kconfig +0 -0

index a65f2e1d9f53..505a9bbbe49f 100644

drivers/nvdimm/Kconfig +3 -0

+
+	  Select Y if unsure.
+
 endif

drivers/nvdimm/Makefile +1 -0

index 70d5f3ad9909..fd6a5838aa25 100644
@@ -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
 

drivers/nvdimm/of_nvdimm.c new file +6 -0

new file mode 100644
index 000000000000..79c28291f420
@@ -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>

drivers/nvdimm/of_nvdimm.c new file +30 -0

+ */
+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);
+

drivers/nvdimm/of_nvdimm.c new file +64 -0

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

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

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

Cc [email protected]

Signed-off-by Oliver O'Halloran <[email protected]>

---
 .../devicetree/bindings/nvdimm/nvdimm-region.txt   | 45 ++++++++++++++++++++++
 1 file changed, 45 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/nvdimm/nvdimm-region.txt

Documentation/devicetree/bindings/nvdimm/nvdimm-region.txt new file +0 -0

new file mode 100644
index 000000000000..02091117ff16

Documentation/devicetree/bindings/nvdimm/nvdimm-region.txt new file +18 -0

+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

Documentation/devicetree/bindings/nvdimm/nvdimm-region.txt new file +4 -0

+			volatile;
+		};
+	};
+};

Signature · 6 lines

2.9.5

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Quoted 15 lines · Oliver O'Halloran

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

Cc [email protected]

Signed-off-by Oliver O'Halloran <[email protected]>

---
 .../devicetree/bindings/nvdimm/nvdimm-region.txt   | 45 ++++++++++++++++++++++
 1 file changed, 45 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/nvdimm/nvdimm-region.txt

Documentation/devicetree/bindings/nvdimm/nvdimm-region.txt new file +0 -0

new file mode 100644
index 000000000000..02091117ff16

Documentation/devicetree/bindings/nvdimm/nvdimm-region.txt line 4

Quoted 4 lines

Documentation/devicetree/bindings/nvdimm/nvdimm-region.txt new file +4 -0

@@ -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?

Quoted 23 lines

Documentation/devicetree/bindings/nvdimm/nvdimm-region.txt new file +23 -0

+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:
+--------------------
+

Documentation/devicetree/bindings/nvdimm/nvdimm-region.txt line 32

Quoted 5 lines

Documentation/devicetree/bindings/nvdimm/nvdimm-region.txt new file +5 -0

+/ {
+     #size-cells = <2>;
+     #address-cells = <2>;
+
+     platform {

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

Quoted 4 lines

Documentation/devicetree/bindings/nvdimm/nvdimm-region.txt new file +4 -0

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

Documentation/devicetree/bindings/nvdimm/nvdimm-region.txt line 41

Quoted 5 lines

Documentation/devicetree/bindings/nvdimm/nvdimm-region.txt new file +5 -0

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

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

Quoted 11 lines

Documentation/devicetree/bindings/nvdimm/nvdimm-region.txt new file +4 -1

+                     volatile;
+             };
+     };
+};
--

2.9.5

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 23 lines · Rob Herring

Quoted 15 lines · Oliver O'Halloran

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

Cc [email protected]

Signed-off-by Oliver O'Halloran <[email protected]>

---
 .../devicetree/bindings/nvdimm/nvdimm-region.txt   | 45 ++++++++++++++++++++++
 1 file changed, 45 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/nvdimm/nvdimm-region.txt

Documentation/devicetree/bindings/nvdimm/nvdimm-region.txt new file +0 -0

new file mode 100644
index 000000000000..02091117ff16

Documentation/devicetree/bindings/nvdimm/nvdimm-region.txt line 4

Quoted 4 lines

Documentation/devicetree/bindings/nvdimm/nvdimm-region.txt new file +4 -0

@@ -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?

Quoted 1 line

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.

Quoted 1 line

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.

Quoted 23 lines

Quoted 23 lines

Documentation/devicetree/bindings/nvdimm/nvdimm-region.txt new file +23 -0

+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:
+--------------------
+

Quoted 8 lines

Documentation/devicetree/bindings/nvdimm/nvdimm-region.txt line 32

Quoted 5 lines

Documentation/devicetree/bindings/nvdimm/nvdimm-region.txt new file +5 -0

+/ {
+     #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.

Quoted 9 lines

Documentation/devicetree/bindings/nvdimm/nvdimm-region.txt line 41

Quoted 9 lines

Documentation/devicetree/bindings/nvdimm/nvdimm-region.txt new file +9 -0

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

Quoted 4 lines

Quoted 3 lines

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?

Quoted 8 lines

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

Quoted 4 lines

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.

Quoted 26 lines · Oliver

Quoted 23 lines · Rob Herring

Quoted 15 lines · Oliver O'Halloran

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

Cc [email protected]

Signed-off-by Oliver O'Halloran <[email protected]>

---
 .../devicetree/bindings/nvdimm/nvdimm-region.txt   | 45 ++++++++++++++++++++++
 1 file changed, 45 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/nvdimm/nvdimm-region.txt

Documentation/devicetree/bindings/nvdimm/nvdimm-region.txt new file +0 -0

new file mode 100644
index 000000000000..02091117ff16

Documentation/devicetree/bindings/nvdimm/nvdimm-region.txt line 4

Quoted 4 lines

Documentation/devicetree/bindings/nvdimm/nvdimm-region.txt new file +4 -0

@@ -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?

Quoted 1 line

And if you have multiple DIMMs, does each DT node correspond to a DIMM?

Quoted 8 lines

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

Quoted 41 lines

Quoted 1 line

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.

Quoted 23 lines

Quoted 23 lines

Documentation/devicetree/bindings/nvdimm/nvdimm-region.txt new file +23 -0

+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:
+--------------------
+

Quoted 8 lines

Documentation/devicetree/bindings/nvdimm/nvdimm-region.txt line 32

Quoted 5 lines

Documentation/devicetree/bindings/nvdimm/nvdimm-region.txt new file +5 -0

+/ {
+     #size-cells = <2>;
+     #address-cells = <2>;
+
+     platform {

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

Quoted 1 line

I think treating it as a platform device is fine. Memory nodes are

Platform device is a Linux term...

Quoted 4 lines

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.

Quoted 12 lines

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

Quoted 9 lines

Documentation/devicetree/bindings/nvdimm/nvdimm-region.txt line 41

Quoted 9 lines

Documentation/devicetree/bindings/nvdimm/nvdimm-region.txt new file +9 -0

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

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 <[email protected]>

Acked-by Dan Williams <[email protected]>

---