[resend V2:PATCH 0/2] add reset-hi3660

[resend V2:PATCH 0/2] add reset-hi3660

[Zhangfei Gao]

Considering Arnd and Philipp suggestions, 
move reset register to dts as table instead of dts header in case of ABI issue

Zhangfei Gao (2):
  dt-bindings: Document the hi3660 reset bindings
  reset: hisilicon: add reset-hi3660

 .../bindings/reset/hisilicon,hi3660-reset.txt      |  51 ++++++++
 drivers/reset/hisilicon/Kconfig                    |   7 +
 drivers/reset/hisilicon/Makefile                   |   1 +
 drivers/reset/hisilicon/reset-hi3660.c             | 144 +++++++++++++++++++++
 4 files changed, 203 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/reset/hisilicon,hi3660-reset.txt
 create mode 100644 drivers/reset/hisilicon/reset-hi3660.c

-- 
2.7.4

[resend v2: PATCH 1/2] dt-bindings: Document the hi3660 reset bindings

[Zhangfei Gao]

Add DT bindings documentation for hi3660 SoC reset controller.

Signed-off-by: Zhangfei Gao <zhangfei.gao@linaro.org>
---
 .../bindings/reset/hisilicon,hi3660-reset.txt      | 51 ++++++++++++++++++++++
 1 file changed, 51 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/reset/hisilicon,hi3660-reset.txt

diff --git a/Documentation/devicetree/bindings/reset/hisilicon,hi3660-reset.txt b/Documentation/devicetree/bindings/reset/hisilicon,hi3660-reset.txt
new file mode 100644
index 0000000..250daf2
--- /dev/null
+++ b/Documentation/devicetree/bindings/reset/hisilicon,hi3660-reset.txt
@@ -0,0 +1,51 @@
+Hisilicon System Reset Controller
+======================================
+
+Please also refer to reset.txt in this directory for common reset
+controller binding usage.
+
+The reset controller registers are part of the system-ctl block on
+hi3660 SoC.
+
+Required properties:
+- compatible: should be
+		 "hisilicon,hi3660-reset"
+- #reset-cells: 1, see below
+- hisi,rst-syscon: phandle of the reset's syscon.
+- hisi,reset-bits: Contains the reset control register information
+		  Should contain 2 cells for each reset exposed to
+		  consumers, defined as:
+			Cell #1 : offset from the syscon register base
+			Cell #2 : bits position of the control register
+
+Example:
+	iomcu: iomcu at ffd7e000 {
+		compatible = "hisilicon,hi3660-iomcu", "syscon";
+		reg = <0x0 0xffd7e000 0x0 0x1000>;
+	};
+
+	iomcu_rst: iomcu_rst_controller {
+		compatible = "hisilicon,hi3660-reset";
+		#reset-cells = <1>;
+		hisi,rst-syscon = <&iomcu>;
+		hisi,reset-bits = <0x20 0x8		/* 0: i2c0 */
+				   0x20 0x10		/* 1: i2c1 */
+				   0x20 0x20		/* 2: i2c2 */
+				   0x20 0x8000000>;	/* 3: i2c6 */
+	};
+
+Specifying reset lines connected to IP modules
+==============================================
+example:
+
+        i2c0: i2c at ..... {
+                ...
+		resets = <&iomcu_rst 0>;
+                ...
+        };
+
+	i2c1: i2c at ..... {
+                ...
+		resets = <&iomcu_rst 1>;
+                ...
+        };
-- 
2.7.4

[resend V2: PATCH 2/2] reset: hisilicon: add reset-hi3660

[Zhangfei Gao]

Add hi3660 reset driver
Reset offset & bits should be listed in dts
Like:
	iomcu_rst: iomcu_rst_controller {
		compatible = "hisilicon,hi3660-reset";
		#reset-cells = <1>;
		hisi,rst-syscon = <&iomcu>;
		hisi,reset-bits = <0x20 0x8		/* 0: i2c0 */
				   0x20 0x10		/* 1: i2c1 */
				   0x20 0x20		/* 2: i2c2 */
				   0x20 0x8000000>;	/* 3: i2c6 */
	};

Signed-off-by: Zhangfei Gao <zhangfei.gao@linaro.org>
---
 drivers/reset/hisilicon/Kconfig        |   7 ++
 drivers/reset/hisilicon/Makefile       |   1 +
 drivers/reset/hisilicon/reset-hi3660.c | 144 +++++++++++++++++++++++++++++++++
 3 files changed, 152 insertions(+)
 create mode 100644 drivers/reset/hisilicon/reset-hi3660.c

diff --git a/drivers/reset/hisilicon/Kconfig b/drivers/reset/hisilicon/Kconfig
index 1ff8b0c..10134dc 100644
--- a/drivers/reset/hisilicon/Kconfig
+++ b/drivers/reset/hisilicon/Kconfig
@@ -1,3 +1,10 @@
+config COMMON_RESET_HI3660
+	tristate "Hi3660 Reset Driver"
+	depends on ARCH_HISI || COMPILE_TEST
+	default ARCH_HISI
+	help
+	  Build the Hisilicon Hi3660 reset driver.
+
 config COMMON_RESET_HI6220
 	tristate "Hi6220 Reset Driver"
 	depends on ARCH_HISI || COMPILE_TEST
diff --git a/drivers/reset/hisilicon/Makefile b/drivers/reset/hisilicon/Makefile
index c932f86..ab8a7bf 100644
--- a/drivers/reset/hisilicon/Makefile
+++ b/drivers/reset/hisilicon/Makefile
@@ -1 +1,2 @@
 obj-$(CONFIG_COMMON_RESET_HI6220) += hi6220_reset.o
+obj-$(CONFIG_COMMON_RESET_HI3660) += reset-hi3660.o
diff --git a/drivers/reset/hisilicon/reset-hi3660.c b/drivers/reset/hisilicon/reset-hi3660.c
new file mode 100644
index 0000000..3307252
--- /dev/null
+++ b/drivers/reset/hisilicon/reset-hi3660.c
@@ -0,0 +1,144 @@
+/*
+ * Copyright (c) 2016-2017 Linaro Ltd.
+ * Copyright (c) 2016-2017 HiSilicon Technologies Co., Ltd.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ */
+#include <linux/kernel.h>
+#include <linux/mfd/syscon.h>
+#include <linux/module.h>
+#include <linux/of_device.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+#include <linux/reset-controller.h>
+
+struct hi3660_reset_data {
+	unsigned int off;
+	unsigned int bits;
+};
+
+struct hi3660_reset_controller {
+	struct reset_controller_dev rst;
+	struct regmap *map;
+	const struct hi3660_reset_data *datas;
+};
+
+#define to_hi3660_reset_controller(_rst) \
+	container_of(_rst, struct hi3660_reset_controller, rst)
+
+static int hi3660_reset_program_hw(struct reset_controller_dev *rcdev,
+				   unsigned long idx, bool assert)
+{
+	struct hi3660_reset_controller *rc = to_hi3660_reset_controller(rcdev);
+	const struct hi3660_reset_data *d;
+
+	if (idx >= rcdev->nr_resets)
+		return -EINVAL;
+
+	d = &rc->datas[idx];
+
+	if (assert)
+		return regmap_write(rc->map, d->off, d->bits);
+	else
+		return regmap_write(rc->map, d->off + 4, d->bits);
+}
+
+static int hi3660_reset_assert(struct reset_controller_dev *rcdev,
+			       unsigned long idx)
+{
+	return hi3660_reset_program_hw(rcdev, idx, true);
+}
+
+static int hi3660_reset_deassert(struct reset_controller_dev *rcdev,
+				 unsigned long idx)
+{
+	return hi3660_reset_program_hw(rcdev, idx, false);
+}
+
+static int hi3660_reset_dev(struct reset_controller_dev *rcdev,
+			    unsigned long idx)
+{
+	int err;
+
+	err = hi3660_reset_assert(rcdev, idx);
+	if (err)
+		return err;
+
+	return hi3660_reset_deassert(rcdev, idx);
+}
+
+static struct reset_control_ops hi3660_reset_ops = {
+	.reset    = hi3660_reset_dev,
+	.assert   = hi3660_reset_assert,
+	.deassert = hi3660_reset_deassert,
+};
+
+static int hi3660_reset_probe(struct platform_device *pdev)
+{
+	struct hi3660_reset_controller *rc;
+	struct device_node *np = pdev->dev.of_node;
+	struct device *dev = &pdev->dev;
+	struct hi3660_reset_data *datas;
+	const __be32 *list;
+	int size, nr, i;
+
+	rc = devm_kzalloc(dev, sizeof(*rc), GFP_KERNEL);
+	if (!rc)
+		return -ENOMEM;
+
+	rc->map = syscon_regmap_lookup_by_phandle(np, "hisi,rst-syscon");
+	if (IS_ERR(rc->map)) {
+		dev_err(dev, "failed to get hi3660,rst-syscon\n");
+		return PTR_ERR(rc->map);
+	}
+
+	list = of_get_property(np, "hisi,reset-bits", &size);
+	if (!list || (size / sizeof(*list)) % 2 != 0) {
+		dev_err(dev, "invalid DT reset description\n");
+		return -EINVAL;
+	}
+
+	nr = (size / sizeof(*list)) / 2;
+	datas = devm_kzalloc(dev, nr * sizeof(*datas), GFP_KERNEL);
+	if (!datas)
+		return -ENOMEM;
+
+	for (i = 0; i < nr; i++) {
+		datas[i].off = be32_to_cpup(list++);
+		datas[i].bits = be32_to_cpup(list++);
+	}
+
+	rc->rst.ops = &hi3660_reset_ops,
+	rc->rst.of_node = np;
+	rc->rst.nr_resets = nr;
+	rc->datas = datas;
+
+	return reset_controller_register(&rc->rst);
+}
+
+static const struct of_device_id hi3660_reset_match[] = {
+	{ .compatible = "hisilicon,hi3660-reset", },
+	{},
+};
+MODULE_DEVICE_TABLE(of, hi3660_reset_match);
+
+static struct platform_driver hi3660_reset_driver = {
+	.probe = hi3660_reset_probe,
+	.driver = {
+		.name = "hi3660-reset",
+		.of_match_table = hi3660_reset_match,
+	},
+};
+
+static int __init hi3660_reset_init(void)
+{
+	return platform_driver_register(&hi3660_reset_driver);
+}
+arch_initcall(hi3660_reset_init);
+
+MODULE_LICENSE("GPL");
+MODULE_ALIAS("platform:hi3660-reset");
+MODULE_DESCRIPTION("HiSilicon Hi3660 Reset Driver");
-- 
2.7.4

[resend v2: PATCH 1/2] dt-bindings: Document the hi3660 reset bindings

[Arnd Bergmann]

On Thursday, December 1, 2016 8:48:40 AM CET Zhangfei Gao wrote:
> +               hisi,reset-bits = <0x20 0x8             /* 0: i2c0 */
> +                                  0x20 0x10            /* 1: i2c1 */
> +                                  0x20 0x20            /* 2: i2c2 */
> +                                  0x20 0x8000000>;     /* 3: i2c6 */
> +       };
> +
> +Specifying reset lines connected to IP modules
> +==============================================
> +example:
> +
> +        i2c0: i2c at ..... {
> +                ...
> +               resets = <&iomcu_rst 0>;
> +                ...
> +        };

I don't really like this approach, since now the information is
in two places. Why not put the data into the reset specifier
directly when it is used?

Also the format seems a little too close to the actual register
layout and could be a little more abstract, using bit numbers instead
of a bitmask and register numbers instead of offsets.

	Arnd

[resend v2: PATCH 1/2] dt-bindings: Document the hi3660 reset bindings

[zhangfei]

Hi, Arnd

On 2016?12?01? 20:05, Arnd Bergmann wrote:
> On Thursday, December 1, 2016 8:48:40 AM CET Zhangfei Gao wrote:
>> +               hisi,reset-bits = <0x20 0x8             /* 0: i2c0 */
>> +                                  0x20 0x10            /* 1: i2c1 */
>> +                                  0x20 0x20            /* 2: i2c2 */
>> +                                  0x20 0x8000000>;     /* 3: i2c6 */
>> +       };
>> +
>> +Specifying reset lines connected to IP modules
>> +==============================================
>> +example:
>> +
>> +        i2c0: i2c at ..... {
>> +                ...
>> +               resets = <&iomcu_rst 0>;
>> +                ...
>> +        };
> I don't really like this approach, since now the information is
> in two places. Why not put the data into the reset specifier
> directly when it is used?
Any example, still not understand.
They are consumer and provider.
>
> Also the format seems a little too close to the actual register
> layout and could be a little more abstract, using bit numbers instead
> of a bitmask and register numbers instead of offsets.
We use bit numbers first.
But in the developing process, we found several bits may be required for 
one driver.
And they may not be continuous as the bits may already be occupied.
Directly using offset, we can set several bits together for simple, to 
give more flexibility.
So after discussion, we directly use offset.

Thanks

[resend v2: PATCH 1/2] dt-bindings: Document the hi3660 reset bindings

[Arnd Bergmann]

On Friday, December 2, 2016 8:21:33 AM CET zhangfei wrote:
> Hi, Arnd
> 
> On 2016?12?01? 20:05, Arnd Bergmann wrote:
> > On Thursday, December 1, 2016 8:48:40 AM CET Zhangfei Gao wrote:
> >> +               hisi,reset-bits = <0x20 0x8             /* 0: i2c0 */
> >> +                                  0x20 0x10            /* 1: i2c1 */
> >> +                                  0x20 0x20            /* 2: i2c2 */
> >> +                                  0x20 0x8000000>;     /* 3: i2c6 */
> >> +       };
> >> +
> >> +Specifying reset lines connected to IP modules
> >> +==============================================
> >> +example:
> >> +
> >> +        i2c0: i2c at ..... {
> >> +                ...
> >> +               resets = <&iomcu_rst 0>;
> >> +                ...
> >> +        };
> > I don't really like this approach, since now the information is
> > in two places. Why not put the data into the reset specifier
> > directly when it is used?
> Any example, still not understand.
> They are consumer and provider.

I mean in the i2c node, have

	i2c0: i2c at ..... {
		...
		resets = <&iomcu_rst 0x20 0x8>;
		...
	}

> > Also the format seems a little too close to the actual register
> > layout and could be a little more abstract, using bit numbers instead
> > of a bitmask and register numbers instead of offsets.
> We use bit numbers first.
> But in the developing process, we found several bits may be required for 
> one driver.
> And they may not be continuous as the bits may already be occupied.
> Directly using offset, we can set several bits together for simple, to 
> give more flexibility.
> So after discussion, we directly use offset.

Can you give an example for why this is needed? Is this different
from a device that has multiple reset lines?

	Arnd

[resend v2: PATCH 1/2] dt-bindings: Document the hi3660 reset bindings

[Philipp Zabel]

Am Freitag, den 02.12.2016, 13:32 +0100 schrieb Arnd Bergmann:
> On Friday, December 2, 2016 8:21:33 AM CET zhangfei wrote:
> > Hi, Arnd
> > 
> > On 2016?12?01? 20:05, Arnd Bergmann wrote:
> > > On Thursday, December 1, 2016 8:48:40 AM CET Zhangfei Gao wrote:
> > >> +               hisi,reset-bits = <0x20 0x8             /* 0: i2c0 */
> > >> +                                  0x20 0x10            /* 1: i2c1 */
> > >> +                                  0x20 0x20            /* 2: i2c2 */
> > >> +                                  0x20 0x8000000>;     /* 3: i2c6 */
> > >> +       };
> > >> +
> > >> +Specifying reset lines connected to IP modules
> > >> +==============================================
> > >> +example:
> > >> +
> > >> +        i2c0: i2c at ..... {
> > >> +                ...
> > >> +               resets = <&iomcu_rst 0>;
> > >> +                ...
> > >> +        };
> > > I don't really like this approach, since now the information is
> > > in two places. Why not put the data into the reset specifier
> > > directly when it is used?

>From my point of view, with the binding above, all reset controller
register/bit layout information is in a single place and can be easily
compared to a list in the reference manual, whereas with your suggestion
the description of the reset controller register layout is spread
throughout one or even several dtsi files.
Also, since no two reset controllers are exactly the same, we get a
proliferation of different slightly phandle argument meanings.

> > Any example, still not understand.
> > They are consumer and provider.
> 
> I mean in the i2c node, have
> 
> 	i2c0: i2c at ..... {
> 		...
> 		resets = <&iomcu_rst 0x20 0x8>;
> 		...
> 	}

There already are a few drivers that use this, and I fear people having
to change their bindings because new flags are needed that have not been
previously thought of.

regards
Philipp

[resend v2: PATCH 1/2] dt-bindings: Document the hi3660 reset bindings

[Arnd Bergmann]

On Friday, December 2, 2016 3:10:13 PM CET Philipp Zabel wrote:
> Am Freitag, den 02.12.2016, 13:32 +0100 schrieb Arnd Bergmann:
> > On Friday, December 2, 2016 8:21:33 AM CET zhangfei wrote:
> > > On 2016?12?01? 20:05, Arnd Bergmann wrote:
> > > > On Thursday, December 1, 2016 8:48:40 AM CET Zhangfei Gao wrote:
> > > >> +               hisi,reset-bits = <0x20 0x8             /* 0: i2c0 */
> > > >> +                                  0x20 0x10            /* 1: i2c1 */
> > > >> +                                  0x20 0x20            /* 2: i2c2 */
> > > >> +                                  0x20 0x8000000>;     /* 3: i2c6 */
> > > >> +       };
> > > >> +
> > > >> +Specifying reset lines connected to IP modules
> > > >> +==============================================
> > > >> +example:
> > > >> +
> > > >> +        i2c0: i2c at ..... {
> > > >> +                ...
> > > >> +               resets = <&iomcu_rst 0>;
> > > >> +                ...
> > > >> +        };
> > > > I don't really like this approach, since now the information is
> > > > in two places. Why not put the data into the reset specifier
> > > > directly when it is used?
> 
> From my point of view, with the binding above, all reset controller
> register/bit layout information is in a single place and can be easily
> compared to a list in the reference manual, whereas with your suggestion
> the description of the reset controller register layout is spread
> throughout one or even several dtsi files.
> Also, since no two reset controllers are exactly the same, we get a
> proliferation of different slightly phandle argument meanings.

There is no reason for this to be any different from other subsystems
that all do it the same way: interrupts, gpios, dma, clk, ... all
define #foo-cells to be used for addressing uniform things,
and the data is only in the reference, so that the node that
describes the controller needs no knowledge of what it's being
used for.

One exception is the case (often on clk bindings) where the register
layout is anything but uniform and every input line has a completely
different behavior. For that case, we define our own numbering
system in the driver and hardcode those tables there.

This reset driver does not seem to belong into that category though.
Even if it did, we putting information about the controller into
its own node is redundant as the driver already identifies the
register layout by the compatible string.

> > > Any example, still not understand.
> > > They are consumer and provider.
> > 
> > I mean in the i2c node, have
> > 
> > 	i2c0: i2c at ..... {
> > 		...
> > 		resets = <&iomcu_rst 0x20 0x8>;
> > 		...
> > 	}
> 
> There already are a few drivers that use this, and I fear people having
> to change their bindings because new flags are needed that have not been
> previously thought of.

It rarely happens on other subsystems, and the binding can
always specify different behavior depending on #reset-cells.

	Arnd

[resend v2: PATCH 1/2] dt-bindings: Document the hi3660 reset bindings

[Rob Herring]

On Fri, Dec 02, 2016 at 03:10:13PM +0100, Philipp Zabel wrote:
> Am Freitag, den 02.12.2016, 13:32 +0100 schrieb Arnd Bergmann:
> > On Friday, December 2, 2016 8:21:33 AM CET zhangfei wrote:
> > > Hi, Arnd
> > > 
> > > On 2016?12?01? 20:05, Arnd Bergmann wrote:
> > > > On Thursday, December 1, 2016 8:48:40 AM CET Zhangfei Gao wrote:
> > > >> +               hisi,reset-bits = <0x20 0x8             /* 0: i2c0 */
> > > >> +                                  0x20 0x10            /* 1: i2c1 */
> > > >> +                                  0x20 0x20            /* 2: i2c2 */
> > > >> +                                  0x20 0x8000000>;     /* 3: i2c6 */
> > > >> +       };
> > > >> +
> > > >> +Specifying reset lines connected to IP modules
> > > >> +==============================================
> > > >> +example:
> > > >> +
> > > >> +        i2c0: i2c at ..... {
> > > >> +                ...
> > > >> +               resets = <&iomcu_rst 0>;
> > > >> +                ...
> > > >> +        };
> > > > I don't really like this approach, since now the information is
> > > > in two places. Why not put the data into the reset specifier
> > > > directly when it is used?
> 
> From my point of view, with the binding above, all reset controller
> register/bit layout information is in a single place and can be easily
> compared to a list in the reference manual, whereas with your suggestion
> the description of the reset controller register layout is spread
> throughout one or even several dtsi files.

Which can be solved by tools.

> Also, since no two reset controllers are exactly the same, we get a
> proliferation of different slightly phandle argument meanings.

phandle args are supposed to be specific to the phandle it points to. 
Otherwise, we'd never need more than 1 cell and everything could be a 
lookup table.

> 
> > > Any example, still not understand.
> > > They are consumer and provider.
> > 
> > I mean in the i2c node, have
> > 
> > 	i2c0: i2c at ..... {
> > 		...
> > 		resets = <&iomcu_rst 0x20 0x8>;
> > 		...
> > 	}
> 
> There already are a few drivers that use this, and I fear people having
> to change their bindings because new flags are needed that have not been
> previously thought of.
> 

Drivers that use what?

Rob

[resend v2: PATCH 1/2] dt-bindings: Document the hi3660 reset bindings

[zhangfei]

Hi, Arnd


On 2016?12?02? 20:32, Arnd Bergmann wrote:
> On Friday, December 2, 2016 8:21:33 AM CET zhangfei wrote:
>> Hi, Arnd
>>
>> On 2016?12?01? 20:05, Arnd Bergmann wrote:
>>> On Thursday, December 1, 2016 8:48:40 AM CET Zhangfei Gao wrote:
>>>> +               hisi,reset-bits = <0x20 0x8             /* 0: i2c0 */
>>>> +                                  0x20 0x10            /* 1: i2c1 */
>>>> +                                  0x20 0x20            /* 2: i2c2 */
>>>> +                                  0x20 0x8000000>;     /* 3: i2c6 */
>>>> +       };
>>>> +
>>>> +Specifying reset lines connected to IP modules
>>>> +==============================================
>>>> +example:
>>>> +
>>>> +        i2c0: i2c at ..... {
>>>> +                ...
>>>> +               resets = <&iomcu_rst 0>;
>>>> +                ...
>>>> +        };
>>> I don't really like this approach, since now the information is
>>> in two places. Why not put the data into the reset specifier
>>> directly when it is used?
>> Any example, still not understand.
>> They are consumer and provider.
> I mean in the i2c node, have
>
> 	i2c0: i2c at ..... {
> 		...
> 		resets = <&iomcu_rst 0x20 0x8>;
> 		...
> }
Got it.
There is function of_xlate in reset_controller_dev can parse the dts
when devm_reset_control_get

* @of_xlate: translation function to translate from specifier as found 
in the
*            device tree to id as given to the reset control ops

Will use this instead.

>>> Also the format seems a little too close to the actual register
>>> layout and could be a little more abstract, using bit numbers instead
>>> of a bitmask and register numbers instead of offsets.
>> We use bit numbers first.
>> But in the developing process, we found several bits may be required for
>> one driver.
>> And they may not be continuous as the bits may already be occupied.
>> Directly using offset, we can set several bits together for simple, to
>> give more flexibility.
>> So after discussion, we directly use offset.
> Can you give an example for why this is needed? Is this different
> from a device that has multiple reset lines?
Yes, we can use multiple reset lines, which is also our original method.
But it may have too many reset lines, like pcie driver will have 5 resets.
So just thinking it can be optimized.

However, when using of_xlate, parsing offset & bit to rstc->id (unsigned 
int),
It only support u32, so will use bit numbers again.
rstc_id = rcdev->of_xlate(rcdev, &args);

Will update v3 patch, help take a look.

Thanks

[resend v2: PATCH 1/2] dt-bindings: Document the hi3660 reset bindings

[Philipp Zabel]

Am Freitag, den 02.12.2016, 23:11 +0100 schrieb Arnd Bergmann:
> On Friday, December 2, 2016 3:10:13 PM CET Philipp Zabel wrote:
> > Am Freitag, den 02.12.2016, 13:32 +0100 schrieb Arnd Bergmann:
> > > On Friday, December 2, 2016 8:21:33 AM CET zhangfei wrote:
> > > > On 2016?12?01? 20:05, Arnd Bergmann wrote:
> > > > > On Thursday, December 1, 2016 8:48:40 AM CET Zhangfei Gao wrote:
> > > > >> +               hisi,reset-bits = <0x20 0x8             /* 0: i2c0 */
> > > > >> +                                  0x20 0x10            /* 1: i2c1 */
> > > > >> +                                  0x20 0x20            /* 2: i2c2 */
> > > > >> +                                  0x20 0x8000000>;     /* 3: i2c6 */
> > > > >> +       };
> > > > >> +
> > > > >> +Specifying reset lines connected to IP modules
> > > > >> +==============================================
> > > > >> +example:
> > > > >> +
> > > > >> +        i2c0: i2c at ..... {
> > > > >> +                ...
> > > > >> +               resets = <&iomcu_rst 0>;
> > > > >> +                ...
> > > > >> +        };
> > > > > I don't really like this approach, since now the information is
> > > > > in two places. Why not put the data into the reset specifier
> > > > > directly when it is used?
> > 
> > From my point of view, with the binding above, all reset controller
> > register/bit layout information is in a single place and can be easily
> > compared to a list in the reference manual, whereas with your suggestion
> > the description of the reset controller register layout is spread
> > throughout one or even several dtsi files.
> > Also, since no two reset controllers are exactly the same, we get a
> > proliferation of different slightly phandle argument meanings.
> 
> There is no reason for this to be any different from other subsystems
> that all do it the same way: interrupts, gpios, dma, clk, ... all
> define #foo-cells to be used for addressing uniform things,
> and the data is only in the reference, so that the node that
> describes the controller needs no knowledge of what it's being
> used for.

For most of those bindings the knowledge about which register and bit
position(s) correspond to the handles resides in the driver.

> One exception is the case (often on clk bindings) where the register
> layout is anything but uniform 

The register layout is very non-uniform for many reset controllers. Some
share the same register space with some of those clock controllers.

> and every input line has a completely different behavior.

I can't argue that the behavior is non-uniform for the sane reset
controllers though, most of them have just a single bit, for most
of them all reset lines behave the same.

>  For that case, we define our own numbering
> system in the driver and hardcode those tables there.
>
> This reset driver does not seem to belong into that category though.

Yes. From what has been shown so far, it seems that in this case, while
the resets are distributed sparsely, the relative layout (set/clear
registers right next to each other) is uniform.

> Even if it did, we putting information about the controller into
> its own node is redundant as the driver already identifies the
> register layout by the compatible string.

Indeed I would prefer the driver to carry the register layout tables
instead of putting this information into the device tree at all.

> > > > Any example, still not understand.
> > > > They are consumer and provider.
> > > 
> > > I mean in the i2c node, have
> > > 
> > > 	i2c0: i2c at ..... {
> > > 		...
> > > 		resets = <&iomcu_rst 0x20 0x8>;
> > > 		...
> > > 	}
> > 
> > There already are a few drivers that use this, and I fear people having
> > to change their bindings because new flags are needed that have not been
> > previously thought of.
> 
> It rarely happens on other subsystems, and the binding can
> always specify different behavior depending on #reset-cells.

I recognize I am biased here. So feel free to ignore this point.

What I'd like to make sure is that we have thought about and are happy
to spread (partial) information about the reset controller register
layout throughout the device tree like this.

regards
Philipp

[resend v2: PATCH 1/2] dt-bindings: Document the hi3660 reset bindings

[Philipp Zabel]

Am Montag, den 05.12.2016, 17:40 -0600 schrieb Rob Herring:
> On Fri, Dec 02, 2016 at 03:10:13PM +0100, Philipp Zabel wrote:
> > Am Freitag, den 02.12.2016, 13:32 +0100 schrieb Arnd Bergmann:
> > > On Friday, December 2, 2016 8:21:33 AM CET zhangfei wrote:
> > > > Hi, Arnd
> > > > 
> > > > On 2016?12?01? 20:05, Arnd Bergmann wrote:
> > > > > On Thursday, December 1, 2016 8:48:40 AM CET Zhangfei Gao wrote:
> > > > >> +               hisi,reset-bits = <0x20 0x8             /* 0: i2c0 */
> > > > >> +                                  0x20 0x10            /* 1: i2c1 */
> > > > >> +                                  0x20 0x20            /* 2: i2c2 */
> > > > >> +                                  0x20 0x8000000>;     /* 3: i2c6 */
> > > > >> +       };
> > > > >> +
> > > > >> +Specifying reset lines connected to IP modules
> > > > >> +==============================================
> > > > >> +example:
> > > > >> +
> > > > >> +        i2c0: i2c at ..... {
> > > > >> +                ...
> > > > >> +               resets = <&iomcu_rst 0>;
> > > > >> +                ...
> > > > >> +        };
> > > > > I don't really like this approach, since now the information is
> > > > > in two places. Why not put the data into the reset specifier
> > > > > directly when it is used?
> > 
> > From my point of view, with the binding above, all reset controller
> > register/bit layout information is in a single place and can be easily
> > compared to a list in the reference manual, whereas with your suggestion
> > the description of the reset controller register layout is spread
> > throughout one or even several dtsi files.
> 
> Which can be solved by tools.

True.

> > Also, since no two reset controllers are exactly the same, we get a
> > proliferation of different slightly phandle argument meanings.
> 
> phandle args are supposed to be specific to the phandle it points to. 
> Otherwise, we'd never need more than 1 cell and everything could be a 
> lookup table.

What I mean is that the clk bindings mostly use <&label index> or
<&label type index> phandles, not <&label register-offset bit-offset>
phandles. Usually the bindings don't spread information about the
register layout of the clock controller throughout the device tree,
because that is contained in the driver, as determined by the compatible
property. I assumed the same should be true for reset controllers, if
possible.

> > > > Any example, still not understand.
> > > > They are consumer and provider.
> > > 
> > > I mean in the i2c node, have
> > > 
> > > 	i2c0: i2c at ..... {
> > > 		...
> > > 		resets = <&iomcu_rst 0x20 0x8>;
> > > 		...
> > > 	}
> > 
> > There already are a few drivers that use this, and I fear people having
> > to change their bindings because new flags are needed that have not been
> > previously thought of. 
> 
> Drivers that use what?

Drivers that use <&label register-offset bit-offset> phandles instead of
<&label index> phandles.

regards
Philipp