[PATCH 0/2] reset: uniphier: add USB3 controller reset support

[PATCH 0/2] reset: uniphier: add USB3 controller reset support

[Kunihiko Hayashi]

This series add new USB3 reset control support for UniPhier SoCs.
This reset lines is included in USB3 glue layer.

Kunihiko Hayashi (2):
  dt-bindings: reset: uniphier: add USB3 controller reset support
  reset: uniphier: add USB3 controller reset control

 .../devicetree/bindings/reset/uniphier-reset.txt   |  45 +++++
 drivers/reset/Kconfig                              |  10 ++
 drivers/reset/Makefile                             |   1 +
 drivers/reset/reset-uniphier-usb3.c                | 183 +++++++++++++++++++++
 4 files changed, 239 insertions(+)
 create mode 100644 drivers/reset/reset-uniphier-usb3.c

-- 
2.7.4

[PATCH 1/2] dt-bindings: reset: uniphier: add USB3 controller reset support

[Kunihiko Hayashi]

Add DT bindings for reset control of USB3 controller implemented in
UniPhier SoCs.

Signed-off-by: Kunihiko Hayashi <hayashi.kunihiko@socionext.com>
---
 .../devicetree/bindings/reset/uniphier-reset.txt   | 45 ++++++++++++++++++++++
 1 file changed, 45 insertions(+)

diff --git a/Documentation/devicetree/bindings/reset/uniphier-reset.txt b/Documentation/devicetree/bindings/reset/uniphier-reset.txt
index 93efed6..f21d81c 100644
--- a/Documentation/devicetree/bindings/reset/uniphier-reset.txt
+++ b/Documentation/devicetree/bindings/reset/uniphier-reset.txt
@@ -118,3 +118,48 @@ Example:
 
 		other nodes ...
 	};
+
+
+USB3 controller reset
+---------------------
+
+Required properties:
+- compatible: Should be
+    "socionext,uniphier-pro4-usb3-reset" - for Pro4 SoC
+    "socionext,uniphier-pxs2-usb3-reset" - for PXs2 SoC
+    "socionext,uniphier-ld20-usb3-reset" - for LD20 SoC
+    "socionext,uniphier-pxs3-usb3-reset" - for PXs3 SoC
+- #reset-cells: Should be 1.
+- reg: Specifies offset and length of the register set for the device.
+- clocks: A list of phandles to the clock gate for USB3 glue layer.
+	According to the clock-names, appropriate clocks are required.
+- clock-names: Should contain
+    "gio", "link" - for Pro4 SoC
+    "link"        - for others
+- resets: A list of phandles to the reset control for USB3 glue layer.
+	According to the reset-names, appropriate resets are required.
+- reset-names: Should contain
+    "gio", "link" - for Pro4 SoC
+    "link"        - for others
+
+Example:
+
+	usb-glue@65b00000 {
+		compatible = "socionext,uniphier-ld20-dwc3-glue",
+			     "simple-mfd";
+		#address-cells = <1>;
+		#size-cells = <1>;
+		ranges = <0 0x65b00000 0x400>;
+
+		usb_rst: reset@0 {
+			compatible = "socionext,uniphier-ld20-usb3-reset";
+			reg = <0x0 0x4>;
+			#reset-cells = <1>;
+			clock-names = "link";
+			clocks = <&sys_clk 14>;
+			clock-names = "link";
+			resets = <&sys_rst 14>;
+		};
+
+		other nodes ...
+	};
-- 
2.7.4

Re: [PATCH 1/2] dt-bindings: reset: uniphier: add USB3 controller reset support

[Philipp Zabel]

On Fri, 2018-06-29 at 17:11 +0900, Kunihiko Hayashi wrote:
> Add DT bindings for reset control of USB3 controller implemented in
> UniPhier SoCs.
> 
> Signed-off-by: Kunihiko Hayashi <hayashi.kunihiko@socionext.com>
> ---
>  .../devicetree/bindings/reset/uniphier-reset.txt   | 45 ++++++++++++++++++++++
>  1 file changed, 45 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/reset/uniphier-reset.txt b/Documentation/devicetree/bindings/reset/uniphier-reset.txt
> index 93efed6..f21d81c 100644
> --- a/Documentation/devicetree/bindings/reset/uniphier-reset.txt
> +++ b/Documentation/devicetree/bindings/reset/uniphier-reset.txt
> @@ -118,3 +118,48 @@ Example:
>  
>  		other nodes ...
>  	};
> +
> +
> +USB3 controller reset
> +---------------------
> +
> +Required properties:
> +- compatible: Should be
> +    "socionext,uniphier-pro4-usb3-reset" - for Pro4 SoC
> +    "socionext,uniphier-pxs2-usb3-reset" - for PXs2 SoC
> +    "socionext,uniphier-ld20-usb3-reset" - for LD20 SoC
> +    "socionext,uniphier-pxs3-usb3-reset" - for PXs3 SoC
> +- #reset-cells: Should be 1.
> +- reg: Specifies offset and length of the register set for the device.
> +- clocks: A list of phandles to the clock gate for USB3 glue layer.
> +	According to the clock-names, appropriate clocks are required.
> +- clock-names: Should contain
> +    "gio", "link" - for Pro4 SoC
> +    "link"        - for others
> +- resets: A list of phandles to the reset control for USB3 glue layer.
> +	According to the reset-names, appropriate resets are required.
> +- reset-names: Should contain
> +    "gio", "link" - for Pro4 SoC
> +    "link"        - for others
> +
> +Example:
> +
> +	usb-glue@65b00000 {
> +		compatible = "socionext,uniphier-ld20-dwc3-glue",
> +			     "simple-mfd";
> +		#address-cells = <1>;
> +		#size-cells = <1>;
> +		ranges = <0 0x65b00000 0x400>;
> +
> +		usb_rst: reset@0 {
> +			compatible = "socionext,uniphier-ld20-usb3-reset";
> +			reg = <0x0 0x4>;
> +			#reset-cells = <1>;
> +			clock-names = "link";
> +			clocks = <&sys_clk 14>;
> +			clock-names = "link";

s/clock/reset/

> +			resets = <&sys_rst 14>;
> +		};
> +
> +		other nodes ...
> +	};

regards
Philipp

Re: [PATCH 1/2] dt-bindings: reset: uniphier: add USB3 controller reset support

[Rob Herring]

On Fri, Jun 29, 2018 at 05:11:30PM +0900, Kunihiko Hayashi wrote:
> Add DT bindings for reset control of USB3 controller implemented in
> UniPhier SoCs.
> 
> Signed-off-by: Kunihiko Hayashi <hayashi.kunihiko@socionext.com>
> ---
>  .../devicetree/bindings/reset/uniphier-reset.txt   | 45 ++++++++++++++++++++++
>  1 file changed, 45 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/reset/uniphier-reset.txt b/Documentation/devicetree/bindings/reset/uniphier-reset.txt
> index 93efed6..f21d81c 100644
> --- a/Documentation/devicetree/bindings/reset/uniphier-reset.txt
> +++ b/Documentation/devicetree/bindings/reset/uniphier-reset.txt
> @@ -118,3 +118,48 @@ Example:
>  
>  		other nodes ...
>  	};
> +
> +
> +USB3 controller reset
> +---------------------
> +
> +Required properties:
> +- compatible: Should be
> +    "socionext,uniphier-pro4-usb3-reset" - for Pro4 SoC
> +    "socionext,uniphier-pxs2-usb3-reset" - for PXs2 SoC
> +    "socionext,uniphier-ld20-usb3-reset" - for LD20 SoC
> +    "socionext,uniphier-pxs3-usb3-reset" - for PXs3 SoC
> +- #reset-cells: Should be 1.
> +- reg: Specifies offset and length of the register set for the device.
> +- clocks: A list of phandles to the clock gate for USB3 glue layer.
> +	According to the clock-names, appropriate clocks are required.
> +- clock-names: Should contain
> +    "gio", "link" - for Pro4 SoC
> +    "link"        - for others
> +- resets: A list of phandles to the reset control for USB3 glue layer.
> +	According to the reset-names, appropriate resets are required.
> +- reset-names: Should contain
> +    "gio", "link" - for Pro4 SoC
> +    "link"        - for others
> +
> +Example:
> +
> +	usb-glue@65b00000 {
> +		compatible = "socionext,uniphier-ld20-dwc3-glue",
> +			     "simple-mfd";
> +		#address-cells = <1>;
> +		#size-cells = <1>;
> +		ranges = <0 0x65b00000 0x400>;
> +
> +		usb_rst: reset@0 {
> +			compatible = "socionext,uniphier-ld20-usb3-reset";

This looks weird. You have a reset controller within the USB block? And 
then a parent reset controller too?

> +			reg = <0x0 0x4>;
> +			#reset-cells = <1>;
> +			clock-names = "link";
> +			clocks = <&sys_clk 14>;
> +			clock-names = "link";
> +			resets = <&sys_rst 14>;
> +		};
> +
> +		other nodes ...

What other nodes?

> +	};
> -- 
> 2.7.4
> 

Re: [PATCH 1/2] dt-bindings: reset: uniphier: add USB3 controller reset support

[Kunihiko Hayashi]

Hi Rob,

On Tue, 3 Jul 2018 17:37:47 -0600 <robh@kernel.org> wrote:

> On Fri, Jun 29, 2018 at 05:11:30PM +0900, Kunihiko Hayashi wrote:
> > Add DT bindings for reset control of USB3 controller implemented in
> > UniPhier SoCs.
> > 
> > Signed-off-by: Kunihiko Hayashi <hayashi.kunihiko@socionext.com>
> > ---
> >  .../devicetree/bindings/reset/uniphier-reset.txt   | 45 ++++++++++++++++++++++
> >  1 file changed, 45 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/reset/uniphier-reset.txt b/Documentation/devicetree/bindings/reset/uniphier-reset.txt
> > index 93efed6..f21d81c 100644
> > --- a/Documentation/devicetree/bindings/reset/uniphier-reset.txt
> > +++ b/Documentation/devicetree/bindings/reset/uniphier-reset.txt
> > @@ -118,3 +118,48 @@ Example:
> >  
> >  		other nodes ...
> >  	};
> > +
> > +
> > +USB3 controller reset
> > +---------------------
> > +
> > +Required properties:
> > +- compatible: Should be
> > +    "socionext,uniphier-pro4-usb3-reset" - for Pro4 SoC
> > +    "socionext,uniphier-pxs2-usb3-reset" - for PXs2 SoC
> > +    "socionext,uniphier-ld20-usb3-reset" - for LD20 SoC
> > +    "socionext,uniphier-pxs3-usb3-reset" - for PXs3 SoC
> > +- #reset-cells: Should be 1.
> > +- reg: Specifies offset and length of the register set for the device.
> > +- clocks: A list of phandles to the clock gate for USB3 glue layer.
> > +	According to the clock-names, appropriate clocks are required.
> > +- clock-names: Should contain
> > +    "gio", "link" - for Pro4 SoC
> > +    "link"        - for others
> > +- resets: A list of phandles to the reset control for USB3 glue layer.
> > +	According to the reset-names, appropriate resets are required.
> > +- reset-names: Should contain
> > +    "gio", "link" - for Pro4 SoC
> > +    "link"        - for others
> > +
> > +Example:
> > +
> > +	usb-glue@65b00000 {
> > +		compatible = "socionext,uniphier-ld20-dwc3-glue",
> > +			     "simple-mfd";
> > +		#address-cells = <1>;
> > +		#size-cells = <1>;
> > +		ranges = <0 0x65b00000 0x400>;
> > +
> > +		usb_rst: reset@0 {
> > +			compatible = "socionext,uniphier-ld20-usb3-reset";
> 
> This looks weird. You have a reset controller within the USB block? And 
> then a parent reset controller too?

Yes, this reset control is included in USB3 glue layer, and this is necessary 
to enable USB3 core. The following diagram shows those relationships.

	USB3 block
	|
	+---USB3 glue layer
	|   |
	|   +--- usb3-reset
	|   |
	|   +--- usb3-regluator
	|   |
	|   +--- usb3-phy
	|
	+---USB3 core

The system reset, as parent reset controller, is necessary to enable
the entire USB3 block including the glue layer.

> > +			reg = <0x0 0x4>;
> > +			#reset-cells = <1>;
> > +			clock-names = "link";
> > +			clocks = <&sys_clk 14>;
> > +			clock-names = "link";
> > +			resets = <&sys_rst 14>;
> > +		};
> > +
> > +		other nodes ...
> 
> What other nodes?

As mentioned above, the glue layer consists of reset, regulator, and phy.
I assume that the "other nodes" mean that these nodes are placed.

Thank you,

---
Best Regards,
Kunihiko Hayashi

[PATCH 2/2] reset: uniphier: add USB3 controller reset control

[Kunihiko Hayashi]

Add reset lines for USB3 controller implemented in UniPhier SoCs.

This reuses only the reset operations in reset-simple, because
the reset-simple doesn't handle any SoC-dependent clocks and resets.
This reset lines is included in the USB3 glue layer, and it's necessary
to enable clocks and deassert resets of the layer before using this
reset lines.

Signed-off-by: Kunihiko Hayashi <hayashi.kunihiko@socionext.com>
---
 drivers/reset/Kconfig               |  10 ++
 drivers/reset/Makefile              |   1 +
 drivers/reset/reset-uniphier-usb3.c | 183 ++++++++++++++++++++++++++++++++++++
 3 files changed, 194 insertions(+)
 create mode 100644 drivers/reset/reset-uniphier-usb3.c

diff --git a/drivers/reset/Kconfig b/drivers/reset/Kconfig
index c0b292b..851c9c3 100644
--- a/drivers/reset/Kconfig
+++ b/drivers/reset/Kconfig
@@ -138,6 +138,16 @@ config RESET_UNIPHIER
 	  Say Y if you want to control reset signals provided by System Control
 	  block, Media I/O block, Peripheral Block.
 
+config RESET_UNIPHIER_USB3
+	tristate "USB3 reset driver for UniPhier SoCs"
+	depends on (ARCH_UNIPHIER || COMPILE_TEST) && OF
+	default ARCH_UNIPHIER
+	select RESET_SIMPLE
+	help
+	  Support for USB3 reset controllers on UniPhier SoCs.
+	  Say Y if you want to control reset signals provided by
+	  USB3 controller block.
+
 config RESET_ZYNQ
 	bool "ZYNQ Reset Driver" if COMPILE_TEST
 	default ARCH_ZYNQ
diff --git a/drivers/reset/Makefile b/drivers/reset/Makefile
index c1261dc..2638ac4 100644
--- a/drivers/reset/Makefile
+++ b/drivers/reset/Makefile
@@ -20,5 +20,6 @@ obj-$(CONFIG_RESET_SUNXI) += reset-sunxi.o
 obj-$(CONFIG_RESET_TI_SCI) += reset-ti-sci.o
 obj-$(CONFIG_RESET_TI_SYSCON) += reset-ti-syscon.o
 obj-$(CONFIG_RESET_UNIPHIER) += reset-uniphier.o
+obj-$(CONFIG_RESET_UNIPHIER_USB3) += reset-uniphier-usb3.o
 obj-$(CONFIG_RESET_ZYNQ) += reset-zynq.o
 
diff --git a/drivers/reset/reset-uniphier-usb3.c b/drivers/reset/reset-uniphier-usb3.c
new file mode 100644
index 0000000..397e6c9
--- /dev/null
+++ b/drivers/reset/reset-uniphier-usb3.c
@@ -0,0 +1,183 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * reset-uniphier-usb3.c - USB3 reset driver for UniPhier
+ * Copyright 2018 Socionext Inc.
+ * Author: Kunihiko Hayashi <hayashi.kunihiko@socionext.com>
+ */
+
+#include <linux/clk.h>
+#include <linux/module.h>
+#include <linux/of_device.h>
+#include <linux/platform_device.h>
+#include <linux/reset.h>
+
+#include "reset-simple.h"
+
+#define MAX_CLKS	2
+#define MAX_RSTS	2
+
+struct uniphier_usb3_reset_soc_data {
+	const char *clock_names[MAX_CLKS];
+	const char *reset_names[MAX_RSTS];
+};
+
+struct uniphier_usb3_reset_priv {
+	int nclks;
+	struct clk *clk[MAX_CLKS];
+	int nrsts;
+	struct reset_control *rst[MAX_RSTS];
+	const struct uniphier_usb3_reset_soc_data *data;
+};
+
+static int uniphier_usb3_reset_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct uniphier_usb3_reset_priv *priv;
+	struct reset_simple_data *rst_data;
+	struct resource *res;
+	resource_size_t size;
+	const char *name;
+	int i, ret, nc, nr;
+
+	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	priv->data = of_device_get_match_data(dev);
+	if (WARN_ON(!priv->data))
+		return -EINVAL;
+
+	rst_data = devm_kzalloc(dev, sizeof(*rst_data), GFP_KERNEL);
+	if (!rst_data)
+		return -ENOMEM;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	size = resource_size(res);
+	rst_data->membase = devm_ioremap_resource(dev, res);
+	if (IS_ERR(rst_data->membase))
+		return PTR_ERR(rst_data->membase);
+
+	for (i = 0; i < MAX_CLKS; i++) {
+		name = priv->data->clock_names[i];
+		if (!name)
+			break;
+		priv->clk[i] = devm_clk_get(dev, name);
+		if (IS_ERR(priv->clk[i]))
+			return PTR_ERR(priv->clk[i]);
+		priv->nclks++;
+	}
+
+	for (i = 0; i < MAX_RSTS; i++) {
+		name = priv->data->reset_names[i];
+		if (!name)
+			break;
+		priv->rst[i] = devm_reset_control_get_shared(dev, name);
+		if (IS_ERR(priv->rst[i]))
+			return PTR_ERR(priv->rst[i]);
+		priv->nrsts++;
+	}
+
+	for (nc = 0; nc < priv->nclks; nc++) {
+		ret = clk_prepare_enable(priv->clk[nc]);
+		if (ret)
+			goto out_clk_disable;
+	}
+
+	for (nr = 0; nr < priv->nrsts; nr++) {
+		ret = reset_control_deassert(priv->rst[nr]);
+		if (ret)
+			goto out_rst_assert;
+	}
+
+	spin_lock_init(&rst_data->lock);
+	rst_data->rcdev.owner = THIS_MODULE;
+	rst_data->rcdev.nr_resets = size * BITS_PER_BYTE;
+	rst_data->rcdev.ops = &reset_simple_ops;
+	rst_data->rcdev.of_node = dev->of_node;
+	rst_data->active_low = true;
+
+	platform_set_drvdata(pdev, priv);
+
+	ret = devm_reset_controller_register(dev, &rst_data->rcdev);
+	if (ret)
+		goto out_rst_assert;
+
+	return 0;
+
+out_rst_assert:
+	while (nr--)
+		reset_control_assert(priv->rst[nr]);
+out_clk_disable:
+	while (nc--)
+		clk_disable_unprepare(priv->clk[nc]);
+
+	return ret;
+}
+
+static int uniphier_usb3_reset_remove(struct platform_device *pdev)
+{
+	struct uniphier_usb3_reset_priv *priv = platform_get_drvdata(pdev);
+	int i;
+
+	for (i = 0; i < priv->nrsts; i++)
+		reset_control_assert(priv->rst[i]);
+	for (i = 0; i < priv->nclks; i++)
+		clk_disable_unprepare(priv->clk[i]);
+
+	return 0;
+}
+
+static const struct uniphier_usb3_reset_soc_data uniphier_pro4_data = {
+	.clock_names = { "gio", "link", },
+	.reset_names = { "gio", "link", },
+};
+
+static const struct uniphier_usb3_reset_soc_data uniphier_pxs2_data = {
+	.clock_names = { "link", },
+	.reset_names = { "link", },
+};
+
+static const struct uniphier_usb3_reset_soc_data uniphier_ld20_data = {
+	.clock_names = { "link", },
+	.reset_names = { "link", },
+};
+
+static const struct uniphier_usb3_reset_soc_data uniphier_pxs3_data = {
+	.clock_names = { "link", },
+	.reset_names = { "link", },
+};
+
+static const struct of_device_id uniphier_usb3_reset_match[] = {
+	{
+		.compatible = "socionext,uniphier-pro4-usb3-reset",
+		.data = &uniphier_pro4_data,
+	},
+	{
+		.compatible = "socionext,uniphier-pxs2-usb3-reset",
+		.data = &uniphier_pxs2_data,
+	},
+	{
+		.compatible = "socionext,uniphier-ld20-usb3-reset",
+		.data = &uniphier_ld20_data,
+	},
+	{
+		.compatible = "socionext,uniphier-pxs3-usb3-reset",
+		.data = &uniphier_pxs3_data,
+	},
+	{ /* Sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, uniphier_usb3_reset_match);
+
+static struct platform_driver uniphier_usb3_reset_driver = {
+	.probe = uniphier_usb3_reset_probe,
+	.remove = uniphier_usb3_reset_remove,
+	.driver = {
+		.name = "uniphier-usb3-reset",
+		.of_match_table = uniphier_usb3_reset_match,
+	},
+};
+module_platform_driver(uniphier_usb3_reset_driver);
+
+MODULE_AUTHOR("Kunihiko Hayashi <hayashi.kunihiko@socionext.com>");
+MODULE_DESCRIPTION("UniPhier USB3 Reset Driver");
+MODULE_LICENSE("GPL");
-- 
2.7.4

Re: [PATCH 2/2] reset: uniphier: add USB3 controller reset control

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 1291 bytes --]

Hi Kunihiko,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on pza/reset/next]
[also build test ERROR on v4.18-rc2 next-20180628]
[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/Kunihiko-Hayashi/reset-uniphier-add-USB3-controller-reset-support/20180629-162836
base:   git://git.pengutronix.de/git/pza/linux reset/next
config: ia64-allmodconfig (attached as .config)
compiler: ia64-linux-gcc (GCC) 8.1.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
        GCC_VERSION=8.1.0 make.cross ARCH=ia64 

All errors (new ones prefixed by >>):

   ERROR: "ia64_delay_loop" [drivers/spi/spi-thunderx.ko] undefined!
>> ERROR: "reset_simple_ops" [drivers/reset/reset-uniphier-usb3.ko] undefined!
   ERROR: "__sw_hweight8" [drivers/net/wireless/mediatek/mt76/mt76.ko] undefined!
   ERROR: "ia64_delay_loop" [drivers/net/phy/mdio-cavium.ko] undefined!

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

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 49877 bytes --]

Re: [PATCH 2/2] reset: uniphier: add USB3 controller reset control

[Philipp Zabel]

Hi Kunihiko,

thank you for the patch. I just have a few small comments below:

On Fri, 2018-06-29 at 17:11 +0900, Kunihiko Hayashi wrote:
> Add reset lines for USB3 controller implemented in UniPhier SoCs.
> 
> This reuses only the reset operations in reset-simple, because
> the reset-simple doesn't handle any SoC-dependent clocks and resets.
> This reset lines is included in the USB3 glue layer, and it's necessary
> to enable clocks and deassert resets of the layer before using this
> reset lines.
> 
> Signed-off-by: Kunihiko Hayashi <hayashi.kunihiko@socionext.com>
> ---
>  drivers/reset/Kconfig               |  10 ++
>  drivers/reset/Makefile              |   1 +
>  drivers/reset/reset-uniphier-usb3.c | 183 ++++++++++++++++++++++++++++++++++++
>  3 files changed, 194 insertions(+)
>  create mode 100644 drivers/reset/reset-uniphier-usb3.c
> 
> diff --git a/drivers/reset/Kconfig b/drivers/reset/Kconfig
> index c0b292b..851c9c3 100644
> --- a/drivers/reset/Kconfig
> +++ b/drivers/reset/Kconfig
> @@ -138,6 +138,16 @@ config RESET_UNIPHIER
>  	  Say Y if you want to control reset signals provided by System Control
>  	  block, Media I/O block, Peripheral Block.
>  
> +config RESET_UNIPHIER_USB3
> +	tristate "USB3 reset driver for UniPhier SoCs"
> +	depends on (ARCH_UNIPHIER || COMPILE_TEST) && OF
> +	default ARCH_UNIPHIER
> +	select RESET_SIMPLE
> +	help
> +	  Support for USB3 reset controllers on UniPhier SoCs.
> +	  Say Y if you want to control reset signals provided by
> +	  USB3 controller block.
> +
>  config RESET_ZYNQ
>  	bool "ZYNQ Reset Driver" if COMPILE_TEST
>  	default ARCH_ZYNQ
> diff --git a/drivers/reset/Makefile b/drivers/reset/Makefile
> index c1261dc..2638ac4 100644
> --- a/drivers/reset/Makefile
> +++ b/drivers/reset/Makefile
> @@ -20,5 +20,6 @@ obj-$(CONFIG_RESET_SUNXI) += reset-sunxi.o
>  obj-$(CONFIG_RESET_TI_SCI) += reset-ti-sci.o
>  obj-$(CONFIG_RESET_TI_SYSCON) += reset-ti-syscon.o
>  obj-$(CONFIG_RESET_UNIPHIER) += reset-uniphier.o
> +obj-$(CONFIG_RESET_UNIPHIER_USB3) += reset-uniphier-usb3.o
>  obj-$(CONFIG_RESET_ZYNQ) += reset-zynq.o
>  
> diff --git a/drivers/reset/reset-uniphier-usb3.c b/drivers/reset/reset-uniphier-usb3.c
> new file mode 100644
> index 0000000..397e6c9
> --- /dev/null
> +++ b/drivers/reset/reset-uniphier-usb3.c
> @@ -0,0 +1,183 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * reset-uniphier-usb3.c - USB3 reset driver for UniPhier
> + * Copyright 2018 Socionext Inc.
> + * Author: Kunihiko Hayashi <hayashi.kunihiko@socionext.com>
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/module.h>
> +#include <linux/of_device.h>
> +#include <linux/platform_device.h>
> +#include <linux/reset.h>
> +
> +#include "reset-simple.h"
> +
> +#define MAX_CLKS	2
> +#define MAX_RSTS	2
> +
> +struct uniphier_usb3_reset_soc_data {
> +	const char *clock_names[MAX_CLKS];
> +	const char *reset_names[MAX_RSTS];
> +};
> +
> +struct uniphier_usb3_reset_priv {

I'd embed a struct reset_simple_data here ...

> +	int nclks;
> +	struct clk *clk[MAX_CLKS];
> +	int nrsts;
> +	struct reset_control *rst[MAX_RSTS];
> +	const struct uniphier_usb3_reset_soc_data *data;
> +};
> +
> +static int uniphier_usb3_reset_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct uniphier_usb3_reset_priv *priv;
> +	struct reset_simple_data *rst_data;
> +	struct resource *res;
> +	resource_size_t size;
> +	const char *name;
> +	int i, ret, nc, nr;
> +
> +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	priv->data = of_device_get_match_data(dev);
> +	if (WARN_ON(!priv->data))
> +		return -EINVAL;
> +
> +	rst_data = devm_kzalloc(dev, sizeof(*rst_data), GFP_KERNEL);
> +	if (!rst_data)
> +		return -ENOMEM;

... that way there's no need for two separate allocations.

> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	size = resource_size(res);
> +	rst_data->membase = devm_ioremap_resource(dev, res);
> +	if (IS_ERR(rst_data->membase))
> +		return PTR_ERR(rst_data->membase);
> +
> +	for (i = 0; i < MAX_CLKS; i++) {
> +		name = priv->data->clock_names[i];
> +		if (!name)
> +			break;
> +		priv->clk[i] = devm_clk_get(dev, name);
> +		if (IS_ERR(priv->clk[i]))
> +			return PTR_ERR(priv->clk[i]);
> +		priv->nclks++;
> +	}
> +
> +	for (i = 0; i < MAX_RSTS; i++) {
> +		name = priv->data->reset_names[i];
> +		if (!name)
> +			break;
> +		priv->rst[i] = devm_reset_control_get_shared(dev, name);
> +		if (IS_ERR(priv->rst[i]))
> +			return PTR_ERR(priv->rst[i]);
> +		priv->nrsts++;
> +	}
> +
> +	for (nc = 0; nc < priv->nclks; nc++) {
> +		ret = clk_prepare_enable(priv->clk[nc]);
> +		if (ret)
> +			goto out_clk_disable;
> +	}
> +
> +	for (nr = 0; nr < priv->nrsts; nr++) {
> +		ret = reset_control_deassert(priv->rst[nr]);
> +		if (ret)
> +			goto out_rst_assert;
> +	}

Have you seen the clk_bulk_* APIs? Those could possibly simplify the
code a bit.
Unfortunately we don't have the equivalent reset bulk API.

> +
> +	spin_lock_init(&rst_data->lock);
> +	rst_data->rcdev.owner = THIS_MODULE;
> +	rst_data->rcdev.nr_resets = size * BITS_PER_BYTE;
> +	rst_data->rcdev.ops = &reset_simple_ops;
> +	rst_data->rcdev.of_node = dev->of_node;
> +	rst_data->active_low = true;
> +
> +	platform_set_drvdata(pdev, priv);
> +
> +	ret = devm_reset_controller_register(dev, &rst_data->rcdev);
> +	if (ret)
> +		goto out_rst_assert;
> +
> +	return 0;
> +
> +out_rst_assert:
> +	while (nr--)
> +		reset_control_assert(priv->rst[nr]);
> +out_clk_disable:
> +	while (nc--)
> +		clk_disable_unprepare(priv->clk[nc]);
> +
> +	return ret;
> +}
> +
> +static int uniphier_usb3_reset_remove(struct platform_device *pdev)
> +{
> +	struct uniphier_usb3_reset_priv *priv = platform_get_drvdata(pdev);
> +	int i;
> +
> +	for (i = 0; i < priv->nrsts; i++)
> +		reset_control_assert(priv->rst[i]);
> +	for (i = 0; i < priv->nclks; i++)
> +		clk_disable_unprepare(priv->clk[i]);
> +
> +	return 0;
> +}
> +
> +static const struct uniphier_usb3_reset_soc_data uniphier_pro4_data = {
> +	.clock_names = { "gio", "link", },
> +	.reset_names = { "gio", "link", },
> +};
> +
> +static const struct uniphier_usb3_reset_soc_data uniphier_pxs2_data = {
> +	.clock_names = { "link", },
> +	.reset_names = { "link", },
> +};
> +
> +static const struct uniphier_usb3_reset_soc_data uniphier_ld20_data = {
> +	.clock_names = { "link", },
> +	.reset_names = { "link", },
> +};
> +
> +static const struct uniphier_usb3_reset_soc_data uniphier_pxs3_data = {
> +	.clock_names = { "link", },
> +	.reset_names = { "link", },
> +};

Could you reuse the same SoC data for pxs2, ld20 and pxs3?

> +static const struct of_device_id uniphier_usb3_reset_match[] = {
> +	{
> +		.compatible = "socionext,uniphier-pro4-usb3-reset",
> +		.data = &uniphier_pro4_data,
> +	},
> +	{
> +		.compatible = "socionext,uniphier-pxs2-usb3-reset",
> +		.data = &uniphier_pxs2_data,
> +	},
> +	{
> +		.compatible = "socionext,uniphier-ld20-usb3-reset",
> +		.data = &uniphier_ld20_data,
> +	},
> +	{
> +		.compatible = "socionext,uniphier-pxs3-usb3-reset",
> +		.data = &uniphier_pxs3_data,
> +	},
> +	{ /* Sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, uniphier_usb3_reset_match);
> +
> +static struct platform_driver uniphier_usb3_reset_driver = {
> +	.probe = uniphier_usb3_reset_probe,
> +	.remove = uniphier_usb3_reset_remove,
> +	.driver = {
> +		.name = "uniphier-usb3-reset",
> +		.of_match_table = uniphier_usb3_reset_match,
> +	},
> +};
> +module_platform_driver(uniphier_usb3_reset_driver);
> +
> +MODULE_AUTHOR("Kunihiko Hayashi <hayashi.kunihiko@socionext.com>");
> +MODULE_DESCRIPTION("UniPhier USB3 Reset Driver");
> +MODULE_LICENSE("GPL");

regards
Philipp

Re: [PATCH 2/2] reset: uniphier: add USB3 controller reset control

[Kunihiko Hayashi]

Hi Philipp,
Thank you for your comments.

On Fri, 29 Jun 2018 11:55:24 +0200 <p.zabel@pengutronix.de> wrote:

> Hi Kunihiko,
> 
> thank you for the patch. I just have a few small comments below:
> 
> On Fri, 2018-06-29 at 17:11 +0900, Kunihiko Hayashi wrote:
> > Add reset lines for USB3 controller implemented in UniPhier SoCs.
> > 
> > This reuses only the reset operations in reset-simple, because
> > the reset-simple doesn't handle any SoC-dependent clocks and resets.
> > This reset lines is included in the USB3 glue layer, and it's necessary
> > to enable clocks and deassert resets of the layer before using this
> > reset lines.
> > 
> > Signed-off-by: Kunihiko Hayashi <hayashi.kunihiko@socionext.com>
> > ---
> >  drivers/reset/Kconfig               |  10 ++
> >  drivers/reset/Makefile              |   1 +
> >  drivers/reset/reset-uniphier-usb3.c | 183 ++++++++++++++++++++++++++++++++++++
> >  3 files changed, 194 insertions(+)
> >  create mode 100644 drivers/reset/reset-uniphier-usb3.c
> > 
> > diff --git a/drivers/reset/Kconfig b/drivers/reset/Kconfig
> > index c0b292b..851c9c3 100644
> > --- a/drivers/reset/Kconfig
> > +++ b/drivers/reset/Kconfig
> > @@ -138,6 +138,16 @@ config RESET_UNIPHIER
> >  	  Say Y if you want to control reset signals provided by System Control
> >  	  block, Media I/O block, Peripheral Block.
> >  
> > +config RESET_UNIPHIER_USB3
> > +	tristate "USB3 reset driver for UniPhier SoCs"
> > +	depends on (ARCH_UNIPHIER || COMPILE_TEST) && OF
> > +	default ARCH_UNIPHIER
> > +	select RESET_SIMPLE
> > +	help
> > +	  Support for USB3 reset controllers on UniPhier SoCs.
> > +	  Say Y if you want to control reset signals provided by
> > +	  USB3 controller block.
> > +
> >  config RESET_ZYNQ
> >  	bool "ZYNQ Reset Driver" if COMPILE_TEST
> >  	default ARCH_ZYNQ
> > diff --git a/drivers/reset/Makefile b/drivers/reset/Makefile
> > index c1261dc..2638ac4 100644
> > --- a/drivers/reset/Makefile
> > +++ b/drivers/reset/Makefile
> > @@ -20,5 +20,6 @@ obj-$(CONFIG_RESET_SUNXI) += reset-sunxi.o
> >  obj-$(CONFIG_RESET_TI_SCI) += reset-ti-sci.o
> >  obj-$(CONFIG_RESET_TI_SYSCON) += reset-ti-syscon.o
> >  obj-$(CONFIG_RESET_UNIPHIER) += reset-uniphier.o
> > +obj-$(CONFIG_RESET_UNIPHIER_USB3) += reset-uniphier-usb3.o
> >  obj-$(CONFIG_RESET_ZYNQ) += reset-zynq.o
> >  
> > diff --git a/drivers/reset/reset-uniphier-usb3.c b/drivers/reset/reset-uniphier-usb3.c
> > new file mode 100644
> > index 0000000..397e6c9
> > --- /dev/null
> > +++ b/drivers/reset/reset-uniphier-usb3.c
> > @@ -0,0 +1,183 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * reset-uniphier-usb3.c - USB3 reset driver for UniPhier
> > + * Copyright 2018 Socionext Inc.
> > + * Author: Kunihiko Hayashi <hayashi.kunihiko@socionext.com>
> > + */
> > +
> > +#include <linux/clk.h>
> > +#include <linux/module.h>
> > +#include <linux/of_device.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/reset.h>
> > +
> > +#include "reset-simple.h"
> > +
> > +#define MAX_CLKS	2
> > +#define MAX_RSTS	2
> > +
> > +struct uniphier_usb3_reset_soc_data {
> > +	const char *clock_names[MAX_CLKS];
> > +	const char *reset_names[MAX_RSTS];
> > +};
> > +
> > +struct uniphier_usb3_reset_priv {
> 
> I'd embed a struct reset_simple_data here ...
> 
> > +	int nclks;
> > +	struct clk *clk[MAX_CLKS];
> > +	int nrsts;
> > +	struct reset_control *rst[MAX_RSTS];
> > +	const struct uniphier_usb3_reset_soc_data *data;
> > +};
> > +
> > +static int uniphier_usb3_reset_probe(struct platform_device *pdev)
> > +{
> > +	struct device *dev = &pdev->dev;
> > +	struct uniphier_usb3_reset_priv *priv;
> > +	struct reset_simple_data *rst_data;
> > +	struct resource *res;
> > +	resource_size_t size;
> > +	const char *name;
> > +	int i, ret, nc, nr;
> > +
> > +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> > +	if (!priv)
> > +		return -ENOMEM;
> > +
> > +	priv->data = of_device_get_match_data(dev);
> > +	if (WARN_ON(!priv->data))
> > +		return -EINVAL;
> > +
> > +	rst_data = devm_kzalloc(dev, sizeof(*rst_data), GFP_KERNEL);
> > +	if (!rst_data)
> > +		return -ENOMEM;
> 
> ... that way there's no need for two separate allocations.

Indeed. I'll embed reset_simple_data in uniphier_usb3_reset_priv,
and remove this.

> > +
> > +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > +	size = resource_size(res);
> > +	rst_data->membase = devm_ioremap_resource(dev, res);
> > +	if (IS_ERR(rst_data->membase))
> > +		return PTR_ERR(rst_data->membase);
> > +
> > +	for (i = 0; i < MAX_CLKS; i++) {
> > +		name = priv->data->clock_names[i];
> > +		if (!name)
> > +			break;
> > +		priv->clk[i] = devm_clk_get(dev, name);
> > +		if (IS_ERR(priv->clk[i]))
> > +			return PTR_ERR(priv->clk[i]);
> > +		priv->nclks++;
> > +	}
> > +
> > +	for (i = 0; i < MAX_RSTS; i++) {
> > +		name = priv->data->reset_names[i];
> > +		if (!name)
> > +			break;
> > +		priv->rst[i] = devm_reset_control_get_shared(dev, name);
> > +		if (IS_ERR(priv->rst[i]))
> > +			return PTR_ERR(priv->rst[i]);
> > +		priv->nrsts++;
> > +	}
> > +
> > +	for (nc = 0; nc < priv->nclks; nc++) {
> > +		ret = clk_prepare_enable(priv->clk[nc]);
> > +		if (ret)
> > +			goto out_clk_disable;
> > +	}
> > +
> > +	for (nr = 0; nr < priv->nrsts; nr++) {
> > +		ret = reset_control_deassert(priv->rst[nr]);
> > +		if (ret)
> > +			goto out_rst_assert;
> > +	}
> 
> Have you seen the clk_bulk_* APIs? Those could possibly simplify the
> code a bit.

Okay. I'll try to apply it.

> Unfortunately we don't have the equivalent reset bulk API.

Yes. And although the reset array API can get multiple resets,
it can't specify each reset with its name.

> > +
> > +	spin_lock_init(&rst_data->lock);
> > +	rst_data->rcdev.owner = THIS_MODULE;
> > +	rst_data->rcdev.nr_resets = size * BITS_PER_BYTE;
> > +	rst_data->rcdev.ops = &reset_simple_ops;
> > +	rst_data->rcdev.of_node = dev->of_node;
> > +	rst_data->active_low = true;
> > +
> > +	platform_set_drvdata(pdev, priv);
> > +
> > +	ret = devm_reset_controller_register(dev, &rst_data->rcdev);
> > +	if (ret)
> > +		goto out_rst_assert;
> > +
> > +	return 0;
> > +
> > +out_rst_assert:
> > +	while (nr--)
> > +		reset_control_assert(priv->rst[nr]);
> > +out_clk_disable:
> > +	while (nc--)
> > +		clk_disable_unprepare(priv->clk[nc]);
> > +
> > +	return ret;
> > +}
> > +
> > +static int uniphier_usb3_reset_remove(struct platform_device *pdev)
> > +{
> > +	struct uniphier_usb3_reset_priv *priv = platform_get_drvdata(pdev);
> > +	int i;
> > +
> > +	for (i = 0; i < priv->nrsts; i++)
> > +		reset_control_assert(priv->rst[i]);
> > +	for (i = 0; i < priv->nclks; i++)
> > +		clk_disable_unprepare(priv->clk[i]);
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct uniphier_usb3_reset_soc_data uniphier_pro4_data = {
> > +	.clock_names = { "gio", "link", },
> > +	.reset_names = { "gio", "link", },
> > +};
> > +
> > +static const struct uniphier_usb3_reset_soc_data uniphier_pxs2_data = {
> > +	.clock_names = { "link", },
> > +	.reset_names = { "link", },
> > +};
> > +
> > +static const struct uniphier_usb3_reset_soc_data uniphier_ld20_data = {
> > +	.clock_names = { "link", },
> > +	.reset_names = { "link", },
> > +};
> > +
> > +static const struct uniphier_usb3_reset_soc_data uniphier_pxs3_data = {
> > +	.clock_names = { "link", },
> > +	.reset_names = { "link", },
> > +};
> 
> Could you reuse the same SoC data for pxs2, ld20 and pxs3?

Sure, I'll rewrite it.

Thank you,

> > +static const struct of_device_id uniphier_usb3_reset_match[] = {
> > +	{
> > +		.compatible = "socionext,uniphier-pro4-usb3-reset",
> > +		.data = &uniphier_pro4_data,
> > +	},
> > +	{
> > +		.compatible = "socionext,uniphier-pxs2-usb3-reset",
> > +		.data = &uniphier_pxs2_data,
> > +	},
> > +	{
> > +		.compatible = "socionext,uniphier-ld20-usb3-reset",
> > +		.data = &uniphier_ld20_data,
> > +	},
> > +	{
> > +		.compatible = "socionext,uniphier-pxs3-usb3-reset",
> > +		.data = &uniphier_pxs3_data,
> > +	},
> > +	{ /* Sentinel */ }
> > +};
> > +MODULE_DEVICE_TABLE(of, uniphier_usb3_reset_match);
> > +
> > +static struct platform_driver uniphier_usb3_reset_driver = {
> > +	.probe = uniphier_usb3_reset_probe,
> > +	.remove = uniphier_usb3_reset_remove,
> > +	.driver = {
> > +		.name = "uniphier-usb3-reset",
> > +		.of_match_table = uniphier_usb3_reset_match,
> > +	},
> > +};
> > +module_platform_driver(uniphier_usb3_reset_driver);
> > +
> > +MODULE_AUTHOR("Kunihiko Hayashi <hayashi.kunihiko@socionext.com>");
> > +MODULE_DESCRIPTION("UniPhier USB3 Reset Driver");
> > +MODULE_LICENSE("GPL");
> 
> regards
> Philipp

---
Best Regards,
Kunihiko Hayashi