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

2018-03-25 Thread Balbir Singh 
On Fri, 23 Mar 2018 19:12:06 +1100
Oliver O'Halloran  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 
> ---

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 
> +L:   linux-nvd...@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 
>  L:   linux-nvd...@lists.01.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 ..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 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +/*
> + * 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[] = {
> + _region_attribute_group,
> + _device_attribute_group,
> + NULL,
> +};
> +
> +static const struct attribute_group *bus_attr_groups[] = {
> + _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(>dev);
> + if (!np)
> + return -ENXIO;
> +
> + pr_err("registering region for %pOF\n", np);
> +
> + if (of_address_to_resource(np, 0, _res)) {
> + pr_warn("Unable to parse reg[0] for %pOF\n", np);
> + return -ENXIO;
> + }
> +
> + memset(_desc, 0, sizeof(ndr_desc));
> + ndr_desc.res = _res;
> + ndr_desc.of_node = np;
> + ndr_desc.attr_groups = region_attr_groups;
> + ndr_desc.numa_node =

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

2018-03-25 Thread Oliver 
On Sat, Mar 24, 2018 at 4:07 AM, Dan Williams  wrote:
> On Fri, Mar 23, 2018 at 1:12 AM, Oliver O'Halloran  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 
>> ---
>> 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

2018-03-24 Thread 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 infrastructureOpen Source Technology Center
https://lists.01.org/pipermail/kbuild-all   Intel Corporation

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

2018-03-24 Thread 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 infrastructureOpen Source Technology Center
https://lists.01.org/pipermail/kbuild-all   Intel Corporation


.config.gz
Description: application/gzip

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

2018-03-23 Thread Dan Williams 
On Fri, Mar 23, 2018 at 1:12 AM, Oliver O'Halloran  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 
> ---
> 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?