[RFCv2 0/3] Adding support for Zynq Reset Controller

[RFCv2 0/3] Adding support for Zynq Reset Controller

[Moritz Fischer]

Hi all,

I went ahead and generalized it to support all the resets I could
find in the TRM. I don't know if all of them are sensible, so we 
need to carfully double check that. I also tried to add in the stuff
that was pointed out in v1.

If this looks good enough for a patch let me know.

Thanks for your feedback,

Moritz

Moritz Fischer (3):
  docs: dts: Added documentation for Xilinx Zynq Reset Controller
    bindings.
  dts: zynq: Add devicetree entry for Xilinx Zynq reset controller.
  reset: reset-zynq: Adding support for Xilinx Zynq reset controller.

 .../devicetree/bindings/reset/zynq-reset-pl.txt    |  13 ++
 arch/arm/boot/dts/zynq-7000.dtsi                   |  43 ++++++-
 arch/arm/boot/dts/zynq-parallella.dts              |   2 +-
 arch/arm/boot/dts/zynq-zc702.dts                   |   2 +-
 arch/arm/boot/dts/zynq-zc706.dts                   |   2 +-
 arch/arm/boot/dts/zynq-zed.dts                     |   2 +-
 arch/arm/boot/dts/zynq-zybo.dts                    |   2 +-
 drivers/reset/Makefile                             |   1 +
 drivers/reset/reset-zynq.c                         | 142 +++++++++++++++++++++
 include/dt-bindings/reset/xlnx,zynq-reset.h        |  94 ++++++++++++++
 10 files changed, 297 insertions(+), 6 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/reset/zynq-reset-pl.txt
 create mode 100644 drivers/reset/reset-zynq.c
 create mode 100644 include/dt-bindings/reset/xlnx,zynq-reset.h

-- 
2.4.3

[RFCv2 1/3] docs: dts: Added documentation for Xilinx Zynq Reset Controller bindings.

[Moritz Fischer]

Signed-off-by: Moritz Fischer <moritz.fischer@ettus.com>
---
 Documentation/devicetree/bindings/reset/zynq-reset-pl.txt | 13 +++++++++++++
 1 file changed, 13 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/reset/zynq-reset-pl.txt

diff --git a/Documentation/devicetree/bindings/reset/zynq-reset-pl.txt b/Documentation/devicetree/bindings/reset/zynq-reset-pl.txt
new file mode 100644
index 0000000..ac4499e
--- /dev/null
+++ b/Documentation/devicetree/bindings/reset/zynq-reset-pl.txt
@@ -0,0 +1,13 @@
+Xilinx Zynq PL Reset Manager
+
+Required properties:
+- compatible: "xlnx,zynq-reset-pl"
+- syscon <&slcr>;
+- #reset-cells: 1
+
+Example:
+	rstc: rstc@240 {
+		#reset-cells = <1>;
+		compatible = "xlnx,zynq-reset-pl";
+		syscon = <&slcr>;
+	};
-- 
2.4.3

[RFCv2 2/3] dts: zynq: Add devicetree entry for Xilinx Zynq reset controller.

[Moritz Fischer]

Signed-off-by: Moritz Fischer <moritz.fischer@ettus.com>
---
 arch/arm/boot/dts/zynq-7000.dtsi            | 43 ++++++++++++-
 arch/arm/boot/dts/zynq-parallella.dts       |  2 +-
 arch/arm/boot/dts/zynq-zc702.dts            |  2 +-
 arch/arm/boot/dts/zynq-zc706.dts            |  2 +-
 arch/arm/boot/dts/zynq-zed.dts              |  2 +-
 arch/arm/boot/dts/zynq-zybo.dts             |  2 +-
 include/dt-bindings/reset/xlnx,zynq-reset.h | 94 +++++++++++++++++++++++++++++
 7 files changed, 141 insertions(+), 6 deletions(-)
 create mode 100644 include/dt-bindings/reset/xlnx,zynq-reset.h

diff --git a/arch/arm/boot/dts/zynq-7000.dtsi b/arch/arm/boot/dts/zynq-7000.dtsi
index b429e1d..1d4faa2 100644
--- a/arch/arm/boot/dts/zynq-7000.dtsi
+++ b/arch/arm/boot/dts/zynq-7000.dtsi
@@ -1,5 +1,6 @@
 /*
  *  Copyright (C) 2011 - 2014 Xilinx
+ *  Copyright (C) 2015 National Instruments Corp.
  *
  * This software is licensed under the terms of the GNU General Public
  * License version 2, as published by the Free Software Foundation, and
@@ -10,7 +11,8 @@
  * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
  * GNU General Public License for more details.
  */
-/include/ "skeleton.dtsi"
+#include "skeleton.dtsi"
+#include <dt-bindings/reset/xlnx,zynq-reset.h>
 
 / {
 	compatible = "xlnx,zynq-7000";
@@ -77,6 +79,8 @@
 			status = "disabled";
 			clocks = <&clkc 19>, <&clkc 36>;
 			clock-names = "can_clk", "pclk";
+			resets = <&rstc CAN0_RESET>, <&rstc CAN0_REF_RESET>;
+			reset-names = "reset", "ref_reset";
 			reg = <0xe0008000 0x1000>;
 			interrupts = <0 28 4>;
 			interrupt-parent = <&intc>;
@@ -89,6 +93,8 @@
 			status = "disabled";
 			clocks = <&clkc 20>, <&clkc 37>;
 			clock-names = "can_clk", "pclk";
+			resets = <&rstc CAN1_RESET>, <&rstc CAN1_REF_RESET>;
+			reset-names = "reset", "ref_reset";
 			reg = <0xe0009000 0x1000>;
 			interrupts = <0 51 4>;
 			interrupt-parent = <&intc>;
@@ -100,6 +106,8 @@
 			compatible = "xlnx,zynq-gpio-1.0";
 			#gpio-cells = <2>;
 			clocks = <&clkc 42>;
+			resets = <&rstc GPIO_RESET>;
+			reset-names = "reset";
 			gpio-controller;
 			interrupt-parent = <&intc>;
 			interrupts = <0 20 4>;
@@ -110,6 +118,8 @@
 			compatible = "cdns,i2c-r1p10";
 			status = "disabled";
 			clocks = <&clkc 38>;
+			reset = <&rstc I2C0_RESET>;
+			reset-names = "reset";
 			interrupt-parent = <&intc>;
 			interrupts = <0 25 4>;
 			reg = <0xe0004000 0x1000>;
@@ -121,6 +131,8 @@
 			compatible = "cdns,i2c-r1p10";
 			status = "disabled";
 			clocks = <&clkc 39>;
+			resets = <&rstc I2C1_RESET>;
+			reset-names = "reset";
 			interrupt-parent = <&intc>;
 			interrupts = <0 48 4>;
 			reg = <0xe0005000 0x1000>;
@@ -148,6 +160,8 @@
 		mc: memory-controller@f8006000 {
 			compatible = "xlnx,zynq-ddrc-a05";
 			reg = <0xf8006000 0x1000>;
+			resets = <&rstc DDR_RESET>;
+			reset-names = "reset";
 		};
 
 		uart0: serial@e0000000 {
@@ -155,6 +169,8 @@
 			status = "disabled";
 			clocks = <&clkc 23>, <&clkc 40>;
 			clock-names = "uart_clk", "pclk";
+			resets = <&rstc UART0_RESET>, <&rstc UART0_REF_RESET>;
+			reset-names = "reset", "ref_reset";
 			reg = <0xE0000000 0x1000>;
 			interrupts = <0 27 4>;
 		};
@@ -164,6 +180,8 @@
 			status = "disabled";
 			clocks = <&clkc 24>, <&clkc 41>;
 			clock-names = "uart_clk", "pclk";
+			resets = <&rstc UART1_RESET>, <&rstc UART1_REF_RESET>;
+			reset-names = "reset", "ref_reset";
 			reg = <0xE0001000 0x1000>;
 			interrupts = <0 50 4>;
 		};
@@ -176,6 +194,8 @@
 			interrupts = <0 26 4>;
 			clocks = <&clkc 25>, <&clkc 34>;
 			clock-names = "ref_clk", "pclk";
+			resets = <&rstc SPI0_RESET>, <&rstc SPI0_REF_RESET>;
+			reset-names = "reset", "ref_reset";
 			#address-cells = <1>;
 			#size-cells = <0>;
 		};
@@ -188,6 +208,8 @@
 			interrupts = <0 49 4>;
 			clocks = <&clkc 26>, <&clkc 35>;
 			clock-names = "ref_clk", "pclk";
+			resets = <&rstc SPI1_RESET>, <&rstc SPI1_REF_RESET>;
+			reset-names = "reset", "ref_reset";
 			#address-cells = <1>;
 			#size-cells = <0>;
 		};
@@ -199,6 +221,9 @@
 			interrupts = <0 22 4>;
 			clocks = <&clkc 30>, <&clkc 30>, <&clkc 13>;
 			clock-names = "pclk", "hclk", "tx_clk";
+			resets = <&rstc GEM0_RESET>, <&rstc GEM0_REF_RESET>,
+				<&rstc GEM0_RX_RESET>;
+			reset-names = "reset", "ref_reset", "rx_reset";
 			#address-cells = <1>;
 			#size-cells = <0>;
 		};
@@ -210,6 +235,9 @@
 			interrupts = <0 45 4>;
 			clocks = <&clkc 31>, <&clkc 31>, <&clkc 14>;
 			clock-names = "pclk", "hclk", "tx_clk";
+			resets = <&rstc GEM1_RESET>, <&rstc GEM1_REF_RESET>,
+				<&rstc GEM1_RX_RESET>;
+			reset-names = "reset", "ref_reset", "rx_reset";
 			#address-cells = <1>;
 			#size-cells = <0>;
 		};
@@ -258,6 +286,13 @@
 				reg = <0x100 0x100>;
 			};
 
+			rstc: rstc@200 {
+				compatible = "xlnx,zynq-reset";
+				reg = <0x200 0x50>;
+				#reset-cells = <1>;
+				syscon = <&slcr>;
+			};
+
 			pinctrl0: pinctrl@700 {
 				compatible = "xlnx,pinctrl-zynq";
 				reg = <0x700 0x200>;
@@ -281,6 +316,8 @@
 			#dma-requests = <4>;
 			clocks = <&clkc 27>;
 			clock-names = "apb_pclk";
+			resets = <&rstc DMAC_RESET>;
+			reset-names = "dmac_reset";
 		};
 
 		devcfg: devcfg@f8007000 {
@@ -333,6 +370,8 @@
 			interrupts = <0 21 4>;
 			reg = <0xe0002000 0x1000>;
 			phy_type = "ulpi";
+			resets = <&rstc USB0_RESET>;
+			reset-names = "usb_reset";
 		};
 
 		usb1: usb@e0003000 {
@@ -343,6 +382,8 @@
 			interrupts = <0 44 4>;
 			reg = <0xe0003000 0x1000>;
 			phy_type = "ulpi";
+			resets = <&rstc USB1_RESET>;
+			reset-names = "usb_reset";
 		};
 
 		watchdog0: watchdog@f8005000 {
diff --git a/arch/arm/boot/dts/zynq-parallella.dts b/arch/arm/boot/dts/zynq-parallella.dts
index 9efd16c..adb0f6e 100644
--- a/arch/arm/boot/dts/zynq-parallella.dts
+++ b/arch/arm/boot/dts/zynq-parallella.dts
@@ -17,7 +17,7 @@
  * GNU General Public License for more details.
  */
 /dts-v1/;
-/include/ "zynq-7000.dtsi"
+#include "zynq-7000.dtsi"
 
 / {
 	model = "Adapteva Parallella Board";
diff --git a/arch/arm/boot/dts/zynq-zc702.dts b/arch/arm/boot/dts/zynq-zc702.dts
index fb59d34..3a08b47 100644
--- a/arch/arm/boot/dts/zynq-zc702.dts
+++ b/arch/arm/boot/dts/zynq-zc702.dts
@@ -12,7 +12,7 @@
  * GNU General Public License for more details.
  */
 /dts-v1/;
-/include/ "zynq-7000.dtsi"
+#include "zynq-7000.dtsi"
 
 / {
 	model = "Zynq ZC702 Development Board";
diff --git a/arch/arm/boot/dts/zynq-zc706.dts b/arch/arm/boot/dts/zynq-zc706.dts
index abf5d23..b238be3 100644
--- a/arch/arm/boot/dts/zynq-zc706.dts
+++ b/arch/arm/boot/dts/zynq-zc706.dts
@@ -12,7 +12,7 @@
  * GNU General Public License for more details.
  */
 /dts-v1/;
-/include/ "zynq-7000.dtsi"
+#include "zynq-7000.dtsi"
 
 / {
 	model = "Zynq ZC706 Development Board";
diff --git a/arch/arm/boot/dts/zynq-zed.dts b/arch/arm/boot/dts/zynq-zed.dts
index b9f2522..38c15ca 100644
--- a/arch/arm/boot/dts/zynq-zed.dts
+++ b/arch/arm/boot/dts/zynq-zed.dts
@@ -12,7 +12,7 @@
  * GNU General Public License for more details.
  */
 /dts-v1/;
-/include/ "zynq-7000.dtsi"
+#include "zynq-7000.dtsi"
 
 / {
 	model = "Zynq Zed Development Board";
diff --git a/arch/arm/boot/dts/zynq-zybo.dts b/arch/arm/boot/dts/zynq-zybo.dts
index 16c9cac..5192e41 100644
--- a/arch/arm/boot/dts/zynq-zybo.dts
+++ b/arch/arm/boot/dts/zynq-zybo.dts
@@ -12,7 +12,7 @@
  * GNU General Public License for more details.
  */
 /dts-v1/;
-/include/ "zynq-7000.dtsi"
+#include "zynq-7000.dtsi"
 
 / {
 	model = "Zynq ZYBO Development Board";
diff --git a/include/dt-bindings/reset/xlnx,zynq-reset.h b/include/dt-bindings/reset/xlnx,zynq-reset.h
new file mode 100644
index 0000000..09bdcef
--- /dev/null
+++ b/include/dt-bindings/reset/xlnx,zynq-reset.h
@@ -0,0 +1,94 @@
+/*
+ * Copyright (c) 2015, National Instruments Corp.
+ *
+ * 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; version 2 of the License.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ */
+
+#ifndef _DT_BINDINGS_RESET_XLNX_RST_H
+#define _DT_BINDINGS_RESET_XLNX_RST_H
+
+/* PSS_RST_CTRL */
+#define SOFT_RESET		0
+
+/* DDR_RST_CTRL */
+#define DDR_RESET		32
+
+/* TOPSW_RST_CTRL */
+#define TOPSW_RESET		64
+
+/* DMAC_RST_CTRL */
+#define DMAC_RESET		96
+
+/* USB_RST_CTRL */
+#define USB0_RESET	128
+#define USB1_RESET	129
+
+/* GEM_RST_CTRL */
+#define GEM0_RESET	160
+#define GEM1_RESET	161
+#define GEM0_RX_RESET		164
+#define GEM1_RX_RESET		165
+#define GEM0_REF_RESET		166
+#define GEM1_REF_RESET		167
+
+/* SDIO_RST_CTRL */
+#define SDIO0_RESET	192
+#define SDIO1_RESET	193
+#define SDIO0_REF_RESET		196
+#define SDIO1_REF_RESET		197
+
+/* SPI_RST_CTRL */
+#define SPI0_RESET	224
+#define SPI1_RESET	225
+#define SPI0_REF_RESET		226
+#define SPI1_REF_RESET		227
+
+/* CAN_RST_CTRL */
+#define CAN0_RESET	256
+#define CAN1_RESET	257
+#define CAN0_REF_RESET		258
+#define CAN1_REF_RESET		259
+
+/* I2C_RST_CTRL */
+#define I2C0_RESET	288
+#define I2C1_RESET	289
+
+/* UART_RST_CTRL */
+#define UART0_RESET	320
+#define UART1_RESET	321
+#define UART0_REF_RESET		322
+#define UART1_REF_RESET		323
+
+/* GPIO_RST_CTRL */
+#define GPIO_RESET	352
+
+/* LQSPI_RST_CTRL */
+#define LQSPI_RESET	384
+#define QSPI_REF_RESET		385
+
+/* SMC_RST_CTRL */
+#define SMC_RESET		416
+#define SMC_REF_RESET		417
+
+/* OCM_RST_CTRL */
+#define OCM_RESET		448
+
+/* FPGA_RST_CTRL */
+#define FPGA0_OUT_RESET		512
+#define FPGA1_OUT_RESET		513
+#define FPGA2_OUT_RESET		514
+#define FPGA3_OUT_RESET		515
+
+/* A9_CPU_RST_CTRL */
+#define A9_RESET0		544
+#define A9_RESET1		545
+#define PERI_RESET		552
+
+#endif
-- 
2.4.3

[RFCv2 3/3] reset: reset-zynq: Adding support for Xilinx Zynq reset controller.

[Moritz Fischer]

This adds a reset controller driver to control the Xilinx Zynq
SoC's various resets.

Signed-off-by: Moritz Fischer <moritz.fischer@ettus.com>
---
 drivers/reset/Makefile     |   1 +
 drivers/reset/reset-zynq.c | 142 +++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 143 insertions(+)
 create mode 100644 drivers/reset/reset-zynq.c

diff --git a/drivers/reset/Makefile b/drivers/reset/Makefile
index 157d421..3fe50e7 100644
--- a/drivers/reset/Makefile
+++ b/drivers/reset/Makefile
@@ -3,3 +3,4 @@ obj-$(CONFIG_ARCH_SOCFPGA) += reset-socfpga.o
 obj-$(CONFIG_ARCH_BERLIN) += reset-berlin.o
 obj-$(CONFIG_ARCH_SUNXI) += reset-sunxi.o
 obj-$(CONFIG_ARCH_STI) += sti/
+obj-$(CONFIG_ARCH_ZYNQ) += reset-zynq.o
diff --git a/drivers/reset/reset-zynq.c b/drivers/reset/reset-zynq.c
new file mode 100644
index 0000000..05e37f8
--- /dev/null
+++ b/drivers/reset/reset-zynq.c
@@ -0,0 +1,142 @@
+/*
+ * Copyright (c) 2015, National Instruments Corp.
+ *
+ * Xilinx Zynq Reset controller driver
+ *
+ * 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; version 2 of the License.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ */
+
+#include <linux/err.h>
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/mfd/syscon.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/reset-controller.h>
+#include <linux/regmap.h>
+#include <linux/types.h>
+
+/* Offsets into SLCR regmap */
+#define SLCR_RST_CTRL_OFFSET	0x200 /* FPGA Software Reset Control */
+
+#define NBANKS	18
+
+struct zynq_reset_data {
+	struct regmap *slcr;
+	struct reset_controller_dev rcdev;
+};
+
+#define to_zynq_reset_data(p)		\
+	container_of((p), struct zynq_reset_data, rcdev)
+
+static int zynq_reset_assert(struct reset_controller_dev *rcdev,
+			     unsigned long id)
+{
+	struct zynq_reset_data *priv = to_zynq_reset_data(rcdev);
+
+	int bank = id / BITS_PER_LONG;
+	int offset = id % BITS_PER_LONG;
+
+	regmap_update_bits(priv->slcr,
+			   SLCR_RST_CTRL_OFFSET + (bank * 4),
+			   BIT(offset),
+			   BIT(offset));
+
+	return 0;
+}
+
+static int zynq_reset_deassert(struct reset_controller_dev *rcdev,
+			       unsigned long id)
+{
+	struct zynq_reset_data *priv = to_zynq_reset_data(rcdev);
+
+	int bank = id / BITS_PER_LONG;
+	int offset = id % BITS_PER_LONG;
+
+	regmap_update_bits(priv->slcr,
+			   SLCR_RST_CTRL_OFFSET + (bank * 4),
+			   BIT(offset),
+			   ~BIT(offset));
+
+	return 0;
+}
+
+static int zynq_reset_status(struct reset_controller_dev *rcdev,
+			     unsigned long id)
+{
+	struct zynq_reset_data *priv = to_zynq_reset_data(rcdev);
+
+	int bank = id / BITS_PER_LONG;
+	int offset = id % BITS_PER_LONG;
+	u32 reg;
+
+	regmap_read(priv->slcr, SLCR_RST_CTRL_OFFSET + (bank * 4), &reg);
+
+	return !(reg & BIT(offset));
+}
+
+static const struct reset_control_ops zynq_reset_ops = {
+	.assert		= zynq_reset_assert,
+	.deassert	= zynq_reset_deassert,
+	.status		= zynq_reset_status,
+};
+
+static int zynq_reset_probe(struct platform_device *pdev)
+{
+	struct zynq_reset_data *priv;
+
+	priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+	platform_set_drvdata(pdev, priv);
+
+	priv->slcr = syscon_regmap_lookup_by_phandle(pdev->dev.of_node,
+		"syscon");
+	if (IS_ERR(priv->slcr)) {
+		dev_err(&pdev->dev, "unable to get zynq-slcr regmap");
+		return PTR_ERR(priv->slcr);
+	}
+
+	priv->rcdev.owner = THIS_MODULE;
+	priv->rcdev.nr_resets = NBANKS * BITS_PER_LONG;
+	priv->rcdev.ops = &zynq_reset_ops;
+	priv->rcdev.of_node = pdev->dev.of_node;
+	reset_controller_register(&priv->rcdev);
+
+	return 0;
+}
+
+static int zynq_reset_remove(struct platform_device *pdev)
+{
+	struct zynq_reset_data *priv = platform_get_drvdata(pdev);
+
+	reset_controller_unregister(&priv->rcdev);
+
+	return 0;
+}
+
+static const struct of_device_id zynq_reset_dt_ids[] = {
+	{ .compatible = "xlnx,zynq-reset", },
+	{ /* sentinel */ },
+};
+
+static struct platform_driver zynq_reset_driver = {
+	.probe	= zynq_reset_probe,
+	.remove	= zynq_reset_remove,
+	.driver = {
+		.name		= "zynq-pl-reset",
+		.of_match_table	= zynq_reset_dt_ids,
+	},
+};
+module_platform_driver(zynq_reset_driver);
+
+MODULE_LICENSE("GPL v2");
+MODULE_AUTHOR("Moritz Fischer <moritz.fischer@ettus.com>");
+MODULE_DESCRIPTION("Zynq Reset Controller Driver");
-- 
2.4.3

Re: [RFCv2 1/3] docs: dts: Added documentation for Xilinx Zynq Reset Controller bindings.

[Michal Simek]

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

On 07/25/2015 02:21 AM, Moritz Fischer wrote:
> Signed-off-by: Moritz Fischer <moritz.fischer@ettus.com>
> ---
>  Documentation/devicetree/bindings/reset/zynq-reset-pl.txt | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/reset/zynq-reset-pl.txt
> 
> diff --git a/Documentation/devicetree/bindings/reset/zynq-reset-pl.txt b/Documentation/devicetree/bindings/reset/zynq-reset-pl.txt
> new file mode 100644
> index 0000000..ac4499e
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/reset/zynq-reset-pl.txt
> @@ -0,0 +1,13 @@
> +Xilinx Zynq PL Reset Manager

here

> +
> +Required properties:
> +- compatible: "xlnx,zynq-reset-pl"

Currently it is not just PL reset controller.

> +- syscon <&slcr>;


missing : and please be more descriptive here.

> +- #reset-cells: 1
> +
> +Example:
> +	rstc: rstc@240 {
> +		#reset-cells = <1>;
> +		compatible = "xlnx,zynq-reset-pl";

Compatible property should go first.

I am missing that reg property

> +		syscon = <&slcr>;
> +	};
> 

Thanks,
Michal


-- 
Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91
w: www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel - Microblaze cpu - http://www.monstr.eu/fdt/
Maintainer of Linux kernel - Xilinx Zynq ARM architecture
Microblaze U-BOOT custodian and responsible for u-boot arm zynq platform



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [RFCv2 3/3] reset: reset-zynq: Adding support for Xilinx Zynq reset controller.

[Michal Simek]

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

On 07/25/2015 02:21 AM, Moritz Fischer wrote:
> This adds a reset controller driver to control the Xilinx Zynq
> SoC's various resets.
> 
> Signed-off-by: Moritz Fischer <moritz.fischer@ettus.com>
> ---
>  drivers/reset/Makefile     |   1 +
>  drivers/reset/reset-zynq.c | 142 +++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 143 insertions(+)
>  create mode 100644 drivers/reset/reset-zynq.c
> 
> diff --git a/drivers/reset/Makefile b/drivers/reset/Makefile
> index 157d421..3fe50e7 100644
> --- a/drivers/reset/Makefile
> +++ b/drivers/reset/Makefile
> @@ -3,3 +3,4 @@ obj-$(CONFIG_ARCH_SOCFPGA) += reset-socfpga.o
>  obj-$(CONFIG_ARCH_BERLIN) += reset-berlin.o
>  obj-$(CONFIG_ARCH_SUNXI) += reset-sunxi.o
>  obj-$(CONFIG_ARCH_STI) += sti/
> +obj-$(CONFIG_ARCH_ZYNQ) += reset-zynq.o
> diff --git a/drivers/reset/reset-zynq.c b/drivers/reset/reset-zynq.c
> new file mode 100644
> index 0000000..05e37f8
> --- /dev/null
> +++ b/drivers/reset/reset-zynq.c
> @@ -0,0 +1,142 @@
> +/*
> + * Copyright (c) 2015, National Instruments Corp.
> + *
> + * Xilinx Zynq Reset controller driver
> + *
> + * 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; version 2 of the License.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + */
> +
> +#include <linux/err.h>
> +#include <linux/io.h>
> +#include <linux/module.h>
> +#include <linux/mfd/syscon.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/reset-controller.h>
> +#include <linux/regmap.h>
> +#include <linux/types.h>
> +
> +/* Offsets into SLCR regmap */
> +#define SLCR_RST_CTRL_OFFSET	0x200 /* FPGA Software Reset Control */

incorrect comment.

> +
> +#define NBANKS	18
> +
> +struct zynq_reset_data {
> +	struct regmap *slcr;
> +	struct reset_controller_dev rcdev;
> +};
> +
> +#define to_zynq_reset_data(p)		\
> +	container_of((p), struct zynq_reset_data, rcdev)
> +
> +static int zynq_reset_assert(struct reset_controller_dev *rcdev,
> +			     unsigned long id)
> +{
> +	struct zynq_reset_data *priv = to_zynq_reset_data(rcdev);
> +
> +	int bank = id / BITS_PER_LONG;
> +	int offset = id % BITS_PER_LONG;
> +
> +	regmap_update_bits(priv->slcr,
> +			   SLCR_RST_CTRL_OFFSET + (bank * 4),
> +			   BIT(offset),
> +			   BIT(offset));
> +
> +	return 0;
> +}
> +
> +static int zynq_reset_deassert(struct reset_controller_dev *rcdev,
> +			       unsigned long id)
> +{
> +	struct zynq_reset_data *priv = to_zynq_reset_data(rcdev);
> +
> +	int bank = id / BITS_PER_LONG;
> +	int offset = id % BITS_PER_LONG;
> +
> +	regmap_update_bits(priv->slcr,
> +			   SLCR_RST_CTRL_OFFSET + (bank * 4),
> +			   BIT(offset),
> +			   ~BIT(offset));
> +
> +	return 0;
> +}
> +
> +static int zynq_reset_status(struct reset_controller_dev *rcdev,
> +			     unsigned long id)
> +{
> +	struct zynq_reset_data *priv = to_zynq_reset_data(rcdev);
> +
> +	int bank = id / BITS_PER_LONG;
> +	int offset = id % BITS_PER_LONG;
> +	u32 reg;
> +
> +	regmap_read(priv->slcr, SLCR_RST_CTRL_OFFSET + (bank * 4), &reg);
> +
> +	return !(reg & BIT(offset));
> +}
> +
> +static const struct reset_control_ops zynq_reset_ops = {
> +	.assert		= zynq_reset_assert,
> +	.deassert	= zynq_reset_deassert,
> +	.status		= zynq_reset_status,
> +};
> +
> +static int zynq_reset_probe(struct platform_device *pdev)
> +{
> +	struct zynq_reset_data *priv;
> +
> +	priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +	platform_set_drvdata(pdev, priv);
> +
> +	priv->slcr = syscon_regmap_lookup_by_phandle(pdev->dev.of_node,
> +		"syscon");

NIT - incorrect indentation.

> +	if (IS_ERR(priv->slcr)) {
> +		dev_err(&pdev->dev, "unable to get zynq-slcr regmap");
> +		return PTR_ERR(priv->slcr);
> +	}
> +
> +	priv->rcdev.owner = THIS_MODULE;
> +	priv->rcdev.nr_resets = NBANKS * BITS_PER_LONG;
> +	priv->rcdev.ops = &zynq_reset_ops;
> +	priv->rcdev.of_node = pdev->dev.of_node;
> +	reset_controller_register(&priv->rcdev);
> +
> +	return 0;
> +}
> +
> +static int zynq_reset_remove(struct platform_device *pdev)
> +{
> +	struct zynq_reset_data *priv = platform_get_drvdata(pdev);
> +
> +	reset_controller_unregister(&priv->rcdev);
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id zynq_reset_dt_ids[] = {
> +	{ .compatible = "xlnx,zynq-reset", },
> +	{ /* sentinel */ },
> +};
> +
> +static struct platform_driver zynq_reset_driver = {
> +	.probe	= zynq_reset_probe,
> +	.remove	= zynq_reset_remove,
> +	.driver = {
> +		.name		= "zynq-pl-reset",

PL in name.
BTW: Don't you want to use KBUILD_MODNAME here?

> +		.of_match_table	= zynq_reset_dt_ids,
> +	},
> +};
> +module_platform_driver(zynq_reset_driver);
> +
> +MODULE_LICENSE("GPL v2");
> +MODULE_AUTHOR("Moritz Fischer <moritz.fischer@ettus.com>");
> +MODULE_DESCRIPTION("Zynq Reset Controller Driver");
> 

The rest looks good - will test it.

Thanks,
Michal

-- 
Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91
w: www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel - Microblaze cpu - http://www.monstr.eu/fdt/
Maintainer of Linux kernel - Xilinx Zynq ARM architecture
Microblaze U-BOOT custodian and responsible for u-boot arm zynq platform



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [RFCv2 2/3] dts: zynq: Add devicetree entry for Xilinx Zynq reset controller.

[Michal Simek]

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

Hi Moritz,

On 07/25/2015 02:21 AM, Moritz Fischer wrote:
> Signed-off-by: Moritz Fischer <moritz.fischer@ettus.com>
> ---
>  arch/arm/boot/dts/zynq-7000.dtsi            | 43 ++++++++++++-

This patch is nice in general but every change in binding should be
discussed separately. There is also necessary to wire them up in the
driver to do action. That's why I think that will be the best just to
add the code to slcr and keep others untouched.

For example MACB/GEM is one example. Adding names to this node and
extending driver to work properly with reset means that all others MACB
users will be affected. Definitely this patch should be ACKed by Nicolas.

Thanks,
Michal

-- 
Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91
w: www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel - Microblaze cpu - http://www.monstr.eu/fdt/
Maintainer of Linux kernel - Xilinx Zynq ARM architecture
Microblaze U-BOOT custodian and responsible for u-boot arm zynq platform



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [RFCv2 3/3] reset: reset-zynq: Adding support for Xilinx Zynq reset controller.

[Michal Simek]

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

On 07/25/2015 02:21 AM, Moritz Fischer wrote:
> This adds a reset controller driver to control the Xilinx Zynq
> SoC's various resets.
> 
> Signed-off-by: Moritz Fischer <moritz.fischer@ettus.com>
> ---
>  drivers/reset/Makefile     |   1 +
>  drivers/reset/reset-zynq.c | 142 +++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 143 insertions(+)
>  create mode 100644 drivers/reset/reset-zynq.c
> 
> diff --git a/drivers/reset/Makefile b/drivers/reset/Makefile
> index 157d421..3fe50e7 100644
> --- a/drivers/reset/Makefile
> +++ b/drivers/reset/Makefile
> @@ -3,3 +3,4 @@ obj-$(CONFIG_ARCH_SOCFPGA) += reset-socfpga.o
>  obj-$(CONFIG_ARCH_BERLIN) += reset-berlin.o
>  obj-$(CONFIG_ARCH_SUNXI) += reset-sunxi.o
>  obj-$(CONFIG_ARCH_STI) += sti/
> +obj-$(CONFIG_ARCH_ZYNQ) += reset-zynq.o
> diff --git a/drivers/reset/reset-zynq.c b/drivers/reset/reset-zynq.c
> new file mode 100644
> index 0000000..05e37f8
> --- /dev/null
> +++ b/drivers/reset/reset-zynq.c
> @@ -0,0 +1,142 @@
> +/*
> + * Copyright (c) 2015, National Instruments Corp.
> + *
> + * Xilinx Zynq Reset controller driver
> + *
> + * 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; version 2 of the License.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + */
> +
> +#include <linux/err.h>
> +#include <linux/io.h>
> +#include <linux/module.h>
> +#include <linux/mfd/syscon.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/reset-controller.h>
> +#include <linux/regmap.h>
> +#include <linux/types.h>
> +
> +/* Offsets into SLCR regmap */
> +#define SLCR_RST_CTRL_OFFSET	0x200 /* FPGA Software Reset Control */
> +
> +#define NBANKS	18
> +
> +struct zynq_reset_data {
> +	struct regmap *slcr;
> +	struct reset_controller_dev rcdev;
> +};
> +
> +#define to_zynq_reset_data(p)		\
> +	container_of((p), struct zynq_reset_data, rcdev)
> +
> +static int zynq_reset_assert(struct reset_controller_dev *rcdev,
> +			     unsigned long id)
> +{
> +	struct zynq_reset_data *priv = to_zynq_reset_data(rcdev);
> +
> +	int bank = id / BITS_PER_LONG;
> +	int offset = id % BITS_PER_LONG;
> +

Personally me I would also add debug message here to be simply enabled
for easier tracking.

> +	regmap_update_bits(priv->slcr,
> +			   SLCR_RST_CTRL_OFFSET + (bank * 4),
> +			   BIT(offset),
> +			   BIT(offset));
> +
> +	return 0;
> +}
> +
> +static int zynq_reset_deassert(struct reset_controller_dev *rcdev,
> +			       unsigned long id)
> +{
> +	struct zynq_reset_data *priv = to_zynq_reset_data(rcdev);
> +
> +	int bank = id / BITS_PER_LONG;
> +	int offset = id % BITS_PER_LONG;
> +

debug message here too.

> +	regmap_update_bits(priv->slcr,
> +			   SLCR_RST_CTRL_OFFSET + (bank * 4),
> +			   BIT(offset),
> +			   ~BIT(offset));
> +
> +	return 0;
> +}
> +
> +static int zynq_reset_status(struct reset_controller_dev *rcdev,
> +			     unsigned long id)
> +{
> +	struct zynq_reset_data *priv = to_zynq_reset_data(rcdev);
> +
> +	int bank = id / BITS_PER_LONG;
> +	int offset = id % BITS_PER_LONG;
> +	u32 reg;
> +
> +	regmap_read(priv->slcr, SLCR_RST_CTRL_OFFSET + (bank * 4), &reg);

debug message here too.

> +
> +	return !(reg & BIT(offset));
> +}
> +
> +static const struct reset_control_ops zynq_reset_ops = {

Remove const here - there is sparse warning.

> +	.assert		= zynq_reset_assert,
> +	.deassert	= zynq_reset_deassert,
> +	.status		= zynq_reset_status,
> +};
> +
> +static int zynq_reset_probe(struct platform_device *pdev)
> +{
> +	struct zynq_reset_data *priv;
> +
> +	priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +	platform_set_drvdata(pdev, priv);
> +
> +	priv->slcr = syscon_regmap_lookup_by_phandle(pdev->dev.of_node,
> +		"syscon");
> +	if (IS_ERR(priv->slcr)) {
> +		dev_err(&pdev->dev, "unable to get zynq-slcr regmap");
> +		return PTR_ERR(priv->slcr);
> +	}
> +
> +	priv->rcdev.owner = THIS_MODULE;
> +	priv->rcdev.nr_resets = NBANKS * BITS_PER_LONG;
> +	priv->rcdev.ops = &zynq_reset_ops;
> +	priv->rcdev.of_node = pdev->dev.of_node;
> +	reset_controller_register(&priv->rcdev);
> +
> +	return 0;
> +}
> +
> +static int zynq_reset_remove(struct platform_device *pdev)
> +{
> +	struct zynq_reset_data *priv = platform_get_drvdata(pdev);
> +
> +	reset_controller_unregister(&priv->rcdev);
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id zynq_reset_dt_ids[] = {
> +	{ .compatible = "xlnx,zynq-reset", },
> +	{ /* sentinel */ },
> +};
> +
> +static struct platform_driver zynq_reset_driver = {
> +	.probe	= zynq_reset_probe,
> +	.remove	= zynq_reset_remove,
> +	.driver = {
> +		.name		= "zynq-pl-reset",
> +		.of_match_table	= zynq_reset_dt_ids,
> +	},
> +};
> +module_platform_driver(zynq_reset_driver);
> +
> +MODULE_LICENSE("GPL v2");
> +MODULE_AUTHOR("Moritz Fischer <moritz.fischer@ettus.com>");
> +MODULE_DESCRIPTION("Zynq Reset Controller Driver");
> 

Also I am missing enabling reset controller in the arch.


diff --git a/arch/arm/mach-zynq/Kconfig b/arch/arm/mach-zynq/Kconfig
index 78e5e007f52d..02a84fdee1bd 100644
--- a/arch/arm/mach-zynq/Kconfig
+++ b/arch/arm/mach-zynq/Kconfig
@@ -1,6 +1,7 @@
 config ARCH_ZYNQ
        bool "Xilinx Zynq ARM Cortex A9 Platform" if ARCH_MULTI_V7
        select ARCH_SUPPORTS_BIG_ENDIAN
+       select ARCH_HAS_RESET_CONTROLLER
        select ARM_AMBA
        select ARM_GIC
        select ARM_GLOBAL_TIMER if !CPU_FREQ

Thanks,
Michal



-- 
Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91
w: www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel - Microblaze cpu - http://www.monstr.eu/fdt/
Maintainer of Linux kernel - Xilinx Zynq ARM architecture
Microblaze U-BOOT custodian and responsible for u-boot arm zynq platform



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [RFCv2 1/3] docs: dts: Added documentation for Xilinx Zynq Reset Controller bindings.

[Sören Brinkmann]

Hi Moritz,

On Fri, 2015-07-24 at 05:21PM -0700, Moritz Fischer wrote:
> Signed-off-by: Moritz Fischer <moritz.fischer@ettus.com>
> ---
>  Documentation/devicetree/bindings/reset/zynq-reset-pl.txt | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/reset/zynq-reset-pl.txt
> 
> diff --git a/Documentation/devicetree/bindings/reset/zynq-reset-pl.txt b/Documentation/devicetree/bindings/reset/zynq-reset-pl.txt
> new file mode 100644
> index 0000000..ac4499e
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/reset/zynq-reset-pl.txt
> @@ -0,0 +1,13 @@
> +Xilinx Zynq PL Reset Manager
> +
> +Required properties:
> +- compatible: "xlnx,zynq-reset-pl"
> +- syscon <&slcr>;
> +- #reset-cells: 1
> +
> +Example:
> +	rstc: rstc@240 {
> +		#reset-cells = <1>;
> +		compatible = "xlnx,zynq-reset-pl";
> +		syscon = <&slcr>;
> +	};

I think you also have to add the outputs and make them part of the
binding. Otherwise you'd have to read the implementation to find
out what device should be hooked up to which output of the reset-controller.

	Sören

Re: [RFCv2 1/3] docs: dts: Added documentation for Xilinx Zynq Reset Controller bindings.

[Moritz Fischer]

Hi Sören,

thanks for your feedback.

On Mon, Jul 27, 2015 at 7:58 PM, Sören Brinkmann
<soren.brinkmann@xilinx.com> wrote:
> Hi Moritz,
>
> On Fri, 2015-07-24 at 05:21PM -0700, Moritz Fischer wrote:
>> Signed-off-by: Moritz Fischer <moritz.fischer@ettus.com>
>> ---
>>  Documentation/devicetree/bindings/reset/zynq-reset-pl.txt | 13 +++++++++++++
>>  1 file changed, 13 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/reset/zynq-reset-pl.txt
>>
>> diff --git a/Documentation/devicetree/bindings/reset/zynq-reset-pl.txt b/Documentation/devicetree/bindings/reset/zynq-reset-pl.txt
>> new file mode 100644
>> index 0000000..ac4499e
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/reset/zynq-reset-pl.txt
>> @@ -0,0 +1,13 @@
>> +Xilinx Zynq PL Reset Manager
>> +
>> +Required properties:
>> +- compatible: "xlnx,zynq-reset-pl"
>> +- syscon <&slcr>;
>> +- #reset-cells: 1
>> +
>> +Example:
>> +     rstc: rstc@240 {
>> +             #reset-cells = <1>;
>> +             compatible = "xlnx,zynq-reset-pl";
>> +             syscon = <&slcr>;
>> +     };
>
> I think you also have to add the outputs and make them part of the
> binding. Otherwise you'd have to read the implementation to find
> out what device should be hooked up to which output of the reset-controller.

Is something like this what you had in mind? I had that prepared for
the next round of patches:

Reset outputs:
 0  : soft reset
 32 : ddr reset
 64 : topsw reset
 96 : dmac reset
 128: usb0 reset
 129: usb1 reset
 160: gem0 reset
 161: gem1 reset
 164: gem0 rx reset
 165: gem1 rx reset
 166: gem0 ref reset
 167: gem1 ref reset
 192: sdio0 reset
 193: sdio1 reset
 196: sdio0 ref reset
 197: sdio1 ref reset
 224: spi0 reset
 225: spi1 reset
 226: spi0 ref reset
 227: spi1 ref reset
 256: can0 reset
 257: can1 reset
 258: can0 ref reset
 259: can1 ref reset
 288: i2c0 reset
 289: i2c1 reset
 320: uart0 reset
 321: uart1 reset
 322: uart0 ref reset
 323: uart1 ref reset
 352: gpio reset
 384: lqspi reset
 385: qspi ref reset
 416: smc reset
 417: smc ref reset
 448: ocm reset
 512: fpga0 out reset
 513: fpga1 out reset
 514: fpga2 out reset
 515: fpga3 out reset
 544: a9 reset 0
 545: a9 reset 1
 552: peri reset

>         Sören

Re: [RFCv2 1/3] docs: dts: Added documentation for Xilinx Zynq Reset Controller bindings.

[Moritz Fischer]

Hi Michal,

On Sun, Jul 26, 2015 at 10:09 PM, Michal Simek <monstr@monstr.eu> wrote:
> On 07/25/2015 02:21 AM, Moritz Fischer wrote:
>> Signed-off-by: Moritz Fischer <moritz.fischer@ettus.com>
>> ---
>>  Documentation/devicetree/bindings/reset/zynq-reset-pl.txt | 13 +++++++++++++
>>  1 file changed, 13 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/reset/zynq-reset-pl.txt
>>
>> diff --git a/Documentation/devicetree/bindings/reset/zynq-reset-pl.txt b/Documentation/devicetree/bindings/reset/zynq-reset-pl.txt
>> new file mode 100644
>> index 0000000..ac4499e
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/reset/zynq-reset-pl.txt
>> @@ -0,0 +1,13 @@
>> +Xilinx Zynq PL Reset Manager
>
> here
>
>> +
>> +Required properties:
>> +- compatible: "xlnx,zynq-reset-pl"
>
> Currently it is not just PL reset controller.
>
>> +- syscon <&slcr>;
>
>
> missing : and please be more descriptive here.
>
>> +- #reset-cells: 1
>> +
>> +Example:
>> +     rstc: rstc@240 {
>> +             #reset-cells = <1>;
>> +             compatible = "xlnx,zynq-reset-pl";
>
> Compatible property should go first.
>
> I am missing that reg property
>
>> +             syscon = <&slcr>;
>> +     };
>>
>
Would something like this work:

Xilinx Zynq Reset Manager

The Zynq AP-SoC has several different resets.

See Chapter 26 of the Zynq TRM (UG585) for more information about Zynq resets.

Required properties:
- compatible: "xlnx,zynq-reset"
- reg: SLCR offset and size taken via syscon <0x200 0x50>
- syscon: <&slcr>
  This should be a phandle to the Zynq's SLCR register.
- #reset-cells: Must be 1

The Zynq Reset Manager needs to be a child node of the SLCR.

Example:
        rstc: rstc@200 {
                compatible = "xlnx,zynq-reset";
                reg = <0x200 0x50>;
                #reset-cells = <1>;
                syscon = <&slcr>;
        };

> Thanks,
> Michal
>
>
> --
> Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91
> w: www.monstr.eu p: +42-0-721842854
> Maintainer of Linux kernel - Microblaze cpu - http://www.monstr.eu/fdt/
> Maintainer of Linux kernel - Xilinx Zynq ARM architecture
> Microblaze U-BOOT custodian and responsible for u-boot arm zynq platform
>
>

Thanks for your feedback,

Moritz

Re: [RFCv2 3/3] reset: reset-zynq: Adding support for Xilinx Zynq reset controller.

[Moritz Fischer]

Hi Michal,

On Mon, Jul 27, 2015 at 12:12 AM, Michal Simek <monstr@monstr.eu> wrote:
> On 07/25/2015 02:21 AM, Moritz Fischer wrote:
>> This adds a reset controller driver to control the Xilinx Zynq
>> SoC's various resets.
>>
>> Signed-off-by: Moritz Fischer <moritz.fischer@ettus.com>
>> ---
>>  drivers/reset/Makefile     |   1 +
>>  drivers/reset/reset-zynq.c | 142 +++++++++++++++++++++++++++++++++++++++++++++
>>  2 files changed, 143 insertions(+)
>>  create mode 100644 drivers/reset/reset-zynq.c
>>
>> diff --git a/drivers/reset/Makefile b/drivers/reset/Makefile
>> index 157d421..3fe50e7 100644
>> --- a/drivers/reset/Makefile
>> +++ b/drivers/reset/Makefile
>> @@ -3,3 +3,4 @@ obj-$(CONFIG_ARCH_SOCFPGA) += reset-socfpga.o
>>  obj-$(CONFIG_ARCH_BERLIN) += reset-berlin.o
>>  obj-$(CONFIG_ARCH_SUNXI) += reset-sunxi.o
>>  obj-$(CONFIG_ARCH_STI) += sti/
>> +obj-$(CONFIG_ARCH_ZYNQ) += reset-zynq.o
>> diff --git a/drivers/reset/reset-zynq.c b/drivers/reset/reset-zynq.c
>> new file mode 100644
>> index 0000000..05e37f8
>> --- /dev/null
>> +++ b/drivers/reset/reset-zynq.c
>> @@ -0,0 +1,142 @@
>> +/*
>> + * Copyright (c) 2015, National Instruments Corp.
>> + *
>> + * Xilinx Zynq Reset controller driver
>> + *
>> + * 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; version 2 of the License.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
>> + * GNU General Public License for more details.
>> + */
>> +
>> +#include <linux/err.h>
>> +#include <linux/io.h>
>> +#include <linux/module.h>
>> +#include <linux/mfd/syscon.h>
>> +#include <linux/of.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/reset-controller.h>
>> +#include <linux/regmap.h>
>> +#include <linux/types.h>
>> +
>> +/* Offsets into SLCR regmap */
>> +#define SLCR_RST_CTRL_OFFSET 0x200 /* FPGA Software Reset Control */
>> +
>> +#define NBANKS       18
>> +
>> +struct zynq_reset_data {
>> +     struct regmap *slcr;
>> +     struct reset_controller_dev rcdev;
>> +};
>> +
>> +#define to_zynq_reset_data(p)                \
>> +     container_of((p), struct zynq_reset_data, rcdev)
>> +
>> +static int zynq_reset_assert(struct reset_controller_dev *rcdev,
>> +                          unsigned long id)
>> +{
>> +     struct zynq_reset_data *priv = to_zynq_reset_data(rcdev);
>> +
>> +     int bank = id / BITS_PER_LONG;
>> +     int offset = id % BITS_PER_LONG;
>> +
>
> Personally me I would also add debug message here to be simply enabled
> for easier tracking.
See below
>
>> +     regmap_update_bits(priv->slcr,
>> +                        SLCR_RST_CTRL_OFFSET + (bank * 4),
>> +                        BIT(offset),
>> +                        BIT(offset));
>> +
>> +     return 0;
>> +}
>> +
>> +static int zynq_reset_deassert(struct reset_controller_dev *rcdev,
>> +                            unsigned long id)
>> +{
>> +     struct zynq_reset_data *priv = to_zynq_reset_data(rcdev);
>> +
>> +     int bank = id / BITS_PER_LONG;
>> +     int offset = id % BITS_PER_LONG;
>> +
>
> debug message here too.
is:
pr_debug("%s: bank: %u offset %u\n", __func__, bank, offset);
accetable? Otherwise I'd have to carry around a struct dev* to use dev_dbg()

>
>> +     regmap_update_bits(priv->slcr,
>> +                        SLCR_RST_CTRL_OFFSET + (bank * 4),
>> +                        BIT(offset),
>> +                        ~BIT(offset));
>> +
>> +     return 0;
>> +}
>> +
>> +static int zynq_reset_status(struct reset_controller_dev *rcdev,
>> +                          unsigned long id)
>> +{
>> +     struct zynq_reset_data *priv = to_zynq_reset_data(rcdev);
>> +
>> +     int bank = id / BITS_PER_LONG;
>> +     int offset = id % BITS_PER_LONG;
>> +     u32 reg;
>> +
>> +     regmap_read(priv->slcr, SLCR_RST_CTRL_OFFSET + (bank * 4), &reg);
>
> debug message here too.
>
>> +
>> +     return !(reg & BIT(offset));
>> +}
>> +
>> +static const struct reset_control_ops zynq_reset_ops = {
>
> Remove const here - there is sparse warning.
>
>> +     .assert         = zynq_reset_assert,
>> +     .deassert       = zynq_reset_deassert,
>> +     .status         = zynq_reset_status,
>> +};
>> +
>> +static int zynq_reset_probe(struct platform_device *pdev)
>> +{
>> +     struct zynq_reset_data *priv;
>> +
>> +     priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
>> +     if (!priv)
>> +             return -ENOMEM;
>> +     platform_set_drvdata(pdev, priv);
>> +
>> +     priv->slcr = syscon_regmap_lookup_by_phandle(pdev->dev.of_node,
>> +             "syscon");
>> +     if (IS_ERR(priv->slcr)) {
>> +             dev_err(&pdev->dev, "unable to get zynq-slcr regmap");
>> +             return PTR_ERR(priv->slcr);
>> +     }
>> +
>> +     priv->rcdev.owner = THIS_MODULE;
>> +     priv->rcdev.nr_resets = NBANKS * BITS_PER_LONG;
>> +     priv->rcdev.ops = &zynq_reset_ops;
>> +     priv->rcdev.of_node = pdev->dev.of_node;
>> +     reset_controller_register(&priv->rcdev);
>> +
>> +     return 0;
>> +}
>> +
>> +static int zynq_reset_remove(struct platform_device *pdev)
>> +{
>> +     struct zynq_reset_data *priv = platform_get_drvdata(pdev);
>> +
>> +     reset_controller_unregister(&priv->rcdev);
>> +
>> +     return 0;
>> +}
>> +
>> +static const struct of_device_id zynq_reset_dt_ids[] = {
>> +     { .compatible = "xlnx,zynq-reset", },
>> +     { /* sentinel */ },
>> +};
>> +
>> +static struct platform_driver zynq_reset_driver = {
>> +     .probe  = zynq_reset_probe,
>> +     .remove = zynq_reset_remove,
>> +     .driver = {
>> +             .name           = "zynq-pl-reset",
>> +             .of_match_table = zynq_reset_dt_ids,
>> +     },
>> +};
>> +module_platform_driver(zynq_reset_driver);
>> +
>> +MODULE_LICENSE("GPL v2");
>> +MODULE_AUTHOR("Moritz Fischer <moritz.fischer@ettus.com>");
>> +MODULE_DESCRIPTION("Zynq Reset Controller Driver");
>>
>
> Also I am missing enabling reset controller in the arch.
Good catch, thanks for pointing that out!
>
>
> diff --git a/arch/arm/mach-zynq/Kconfig b/arch/arm/mach-zynq/Kconfig
> index 78e5e007f52d..02a84fdee1bd 100644
> --- a/arch/arm/mach-zynq/Kconfig
> +++ b/arch/arm/mach-zynq/Kconfig
> @@ -1,6 +1,7 @@
>  config ARCH_ZYNQ
>         bool "Xilinx Zynq ARM Cortex A9 Platform" if ARCH_MULTI_V7
>         select ARCH_SUPPORTS_BIG_ENDIAN
> +       select ARCH_HAS_RESET_CONTROLLER
>         select ARM_AMBA
>         select ARM_GIC
>         select ARM_GLOBAL_TIMER if !CPU_FREQ
>
> Thanks,
> Michal
>
>
>
> --
> Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91
> w: www.monstr.eu p: +42-0-721842854
> Maintainer of Linux kernel - Microblaze cpu - http://www.monstr.eu/fdt/
> Maintainer of Linux kernel - Xilinx Zynq ARM architecture
> Microblaze U-BOOT custodian and responsible for u-boot arm zynq platform
>
>
Thanks,

Moritz

Re: [RFCv2 2/3] dts: zynq: Add devicetree entry for Xilinx Zynq reset controller.

[Moritz Fischer]

Hi Michal,

I agree we need to be careful with changing the bindings.

On Sun, Jul 26, 2015 at 11:56 PM, Michal Simek <monstr@monstr.eu> wrote:
> Hi Moritz,
>
> On 07/25/2015 02:21 AM, Moritz Fischer wrote:
>> Signed-off-by: Moritz Fischer <moritz.fischer@ettus.com>
>> ---
>>  arch/arm/boot/dts/zynq-7000.dtsi            | 43 ++++++++++++-
>
> This patch is nice in general but every change in binding should be
> discussed separately. There is also necessary to wire them up in the
> driver to do action. That's why I think that will be the best just to
> add the code to slcr and keep others untouched.

Ok, just to clarify: You'd suggest to just add the rstc as child node
to the slcr,
and leave the other nodes untouched?

>
> For example MACB/GEM is one example. Adding names to this node and
> extending driver to work properly with reset means that all others MACB
> users will be affected. Definitely this patch should be ACKed by Nicolas.
>
> Thanks,
> Michal
>
> --
> Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91
> w: www.monstr.eu p: +42-0-721842854
> Maintainer of Linux kernel - Microblaze cpu - http://www.monstr.eu/fdt/
> Maintainer of Linux kernel - Xilinx Zynq ARM architecture
> Microblaze U-BOOT custodian and responsible for u-boot arm zynq platform
>
>
Cheers,

Moritz

Re: [RFCv2 1/3] docs: dts: Added documentation for Xilinx Zynq Reset Controller bindings.

[Michal Simek]

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

On 07/28/2015 06:55 AM, Moritz Fischer wrote:
> Hi Michal,
> 
> On Sun, Jul 26, 2015 at 10:09 PM, Michal Simek <monstr@monstr.eu> wrote:
>> On 07/25/2015 02:21 AM, Moritz Fischer wrote:
>>> Signed-off-by: Moritz Fischer <moritz.fischer@ettus.com>
>>> ---
>>>  Documentation/devicetree/bindings/reset/zynq-reset-pl.txt | 13 +++++++++++++
>>>  1 file changed, 13 insertions(+)
>>>  create mode 100644 Documentation/devicetree/bindings/reset/zynq-reset-pl.txt
>>>
>>> diff --git a/Documentation/devicetree/bindings/reset/zynq-reset-pl.txt b/Documentation/devicetree/bindings/reset/zynq-reset-pl.txt
>>> new file mode 100644
>>> index 0000000..ac4499e
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/reset/zynq-reset-pl.txt
>>> @@ -0,0 +1,13 @@
>>> +Xilinx Zynq PL Reset Manager
>>
>> here
>>
>>> +
>>> +Required properties:
>>> +- compatible: "xlnx,zynq-reset-pl"
>>
>> Currently it is not just PL reset controller.
>>
>>> +- syscon <&slcr>;
>>
>>
>> missing : and please be more descriptive here.
>>
>>> +- #reset-cells: 1
>>> +
>>> +Example:
>>> +     rstc: rstc@240 {
>>> +             #reset-cells = <1>;
>>> +             compatible = "xlnx,zynq-reset-pl";
>>
>> Compatible property should go first.
>>
>> I am missing that reg property
>>
>>> +             syscon = <&slcr>;
>>> +     };
>>>
>>
> Would something like this work:
> 
> Xilinx Zynq Reset Manager
> 
> The Zynq AP-SoC has several different resets.
> 
> See Chapter 26 of the Zynq TRM (UG585) for more information about Zynq resets.
> 
> Required properties:
> - compatible: "xlnx,zynq-reset"
> - reg: SLCR offset and size taken via syscon <0x200 0x50>
> - syscon: <&slcr>
>   This should be a phandle to the Zynq's SLCR register.
> - #reset-cells: Must be 1
> 
> The Zynq Reset Manager needs to be a child node of the SLCR.
> 
> Example:
>         rstc: rstc@200 {
>                 compatible = "xlnx,zynq-reset";
>                 reg = <0x200 0x50>;
>                 #reset-cells = <1>;
>                 syscon = <&slcr>;
>         };

Looks good to me.

Thanks,
Michal


-- 
Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91
w: www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel - Microblaze cpu - http://www.monstr.eu/fdt/
Maintainer of Linux kernel - Xilinx Zynq ARM architecture
Microblaze U-BOOT custodian and responsible for u-boot arm zynq platform



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [RFCv2 2/3] dts: zynq: Add devicetree entry for Xilinx Zynq reset controller.

[Michal Simek]

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

On 07/28/2015 07:03 AM, Moritz Fischer wrote:
> Hi Michal,
> 
> I agree we need to be careful with changing the bindings.
> 
> On Sun, Jul 26, 2015 at 11:56 PM, Michal Simek <monstr@monstr.eu> wrote:
>> Hi Moritz,
>>
>> On 07/25/2015 02:21 AM, Moritz Fischer wrote:
>>> Signed-off-by: Moritz Fischer <moritz.fischer@ettus.com>
>>> ---
>>>  arch/arm/boot/dts/zynq-7000.dtsi            | 43 ++++++++++++-
>>
>> This patch is nice in general but every change in binding should be
>> discussed separately. There is also necessary to wire them up in the
>> driver to do action. That's why I think that will be the best just to
>> add the code to slcr and keep others untouched.
> 
> Ok, just to clarify: You'd suggest to just add the rstc as child node
> to the slcr,
> and leave the other nodes untouched?

yes and then add it on case-by-case basis.

Thanks,
Michal


-- 
Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91
w: www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel - Microblaze cpu - http://www.monstr.eu/fdt/
Maintainer of Linux kernel - Xilinx Zynq ARM architecture
Microblaze U-BOOT custodian and responsible for u-boot arm zynq platform



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [RFCv2 3/3] reset: reset-zynq: Adding support for Xilinx Zynq reset controller.

[Michal Simek]

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

On 07/28/2015 06:59 AM, Moritz Fischer wrote:
> Hi Michal,
> 
> On Mon, Jul 27, 2015 at 12:12 AM, Michal Simek <monstr@monstr.eu> wrote:
>> On 07/25/2015 02:21 AM, Moritz Fischer wrote:
>>> This adds a reset controller driver to control the Xilinx Zynq
>>> SoC's various resets.
>>>
>>> Signed-off-by: Moritz Fischer <moritz.fischer@ettus.com>
>>> ---
>>>  drivers/reset/Makefile     |   1 +
>>>  drivers/reset/reset-zynq.c | 142 +++++++++++++++++++++++++++++++++++++++++++++
>>>  2 files changed, 143 insertions(+)
>>>  create mode 100644 drivers/reset/reset-zynq.c
>>>
>>> diff --git a/drivers/reset/Makefile b/drivers/reset/Makefile
>>> index 157d421..3fe50e7 100644
>>> --- a/drivers/reset/Makefile
>>> +++ b/drivers/reset/Makefile
>>> @@ -3,3 +3,4 @@ obj-$(CONFIG_ARCH_SOCFPGA) += reset-socfpga.o
>>>  obj-$(CONFIG_ARCH_BERLIN) += reset-berlin.o
>>>  obj-$(CONFIG_ARCH_SUNXI) += reset-sunxi.o
>>>  obj-$(CONFIG_ARCH_STI) += sti/
>>> +obj-$(CONFIG_ARCH_ZYNQ) += reset-zynq.o
>>> diff --git a/drivers/reset/reset-zynq.c b/drivers/reset/reset-zynq.c
>>> new file mode 100644
>>> index 0000000..05e37f8
>>> --- /dev/null
>>> +++ b/drivers/reset/reset-zynq.c
>>> @@ -0,0 +1,142 @@
>>> +/*
>>> + * Copyright (c) 2015, National Instruments Corp.
>>> + *
>>> + * Xilinx Zynq Reset controller driver
>>> + *
>>> + * 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; version 2 of the License.
>>> + *
>>> + * This program is distributed in the hope that it will be useful,
>>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
>>> + * GNU General Public License for more details.
>>> + */
>>> +
>>> +#include <linux/err.h>
>>> +#include <linux/io.h>
>>> +#include <linux/module.h>
>>> +#include <linux/mfd/syscon.h>
>>> +#include <linux/of.h>
>>> +#include <linux/platform_device.h>
>>> +#include <linux/reset-controller.h>
>>> +#include <linux/regmap.h>
>>> +#include <linux/types.h>
>>> +
>>> +/* Offsets into SLCR regmap */
>>> +#define SLCR_RST_CTRL_OFFSET 0x200 /* FPGA Software Reset Control */
>>> +
>>> +#define NBANKS       18
>>> +
>>> +struct zynq_reset_data {
>>> +     struct regmap *slcr;
>>> +     struct reset_controller_dev rcdev;
>>> +};
>>> +
>>> +#define to_zynq_reset_data(p)                \
>>> +     container_of((p), struct zynq_reset_data, rcdev)
>>> +
>>> +static int zynq_reset_assert(struct reset_controller_dev *rcdev,
>>> +                          unsigned long id)
>>> +{
>>> +     struct zynq_reset_data *priv = to_zynq_reset_data(rcdev);
>>> +
>>> +     int bank = id / BITS_PER_LONG;
>>> +     int offset = id % BITS_PER_LONG;
>>> +
>>
>> Personally me I would also add debug message here to be simply enabled
>> for easier tracking.
> See below
>>
>>> +     regmap_update_bits(priv->slcr,
>>> +                        SLCR_RST_CTRL_OFFSET + (bank * 4),
>>> +                        BIT(offset),
>>> +                        BIT(offset));
>>> +
>>> +     return 0;
>>> +}
>>> +
>>> +static int zynq_reset_deassert(struct reset_controller_dev *rcdev,
>>> +                            unsigned long id)
>>> +{
>>> +     struct zynq_reset_data *priv = to_zynq_reset_data(rcdev);
>>> +
>>> +     int bank = id / BITS_PER_LONG;
>>> +     int offset = id % BITS_PER_LONG;
>>> +
>>
>> debug message here too.
> is:
> pr_debug("%s: bank: %u offset %u\n", __func__, bank, offset);
> accetable? Otherwise I'd have to carry around a struct dev* to use dev_dbg()

It is fine for me.

Thanks,
Michal

-- 
Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91
w: www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel - Microblaze cpu - http://www.monstr.eu/fdt/
Maintainer of Linux kernel - Xilinx Zynq ARM architecture
Microblaze U-BOOT custodian and responsible for u-boot arm zynq platform



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [RFCv2 2/3] dts: zynq: Add devicetree entry for Xilinx Zynq reset controller.

[Nicolas Ferre]

Le 28/07/2015 07:03, Moritz Fischer a écrit :
> Hi Michal,
> 
> I agree we need to be careful with changing the bindings.
> 
> On Sun, Jul 26, 2015 at 11:56 PM, Michal Simek <monstr@monstr.eu> wrote:
>> Hi Moritz,
>>
>> On 07/25/2015 02:21 AM, Moritz Fischer wrote:
>>> Signed-off-by: Moritz Fischer <moritz.fischer@ettus.com>
>>> ---
>>>  arch/arm/boot/dts/zynq-7000.dtsi            | 43 ++++++++++++-
>>
>> This patch is nice in general but every change in binding should be
>> discussed separately. There is also necessary to wire them up in the
>> driver to do action. That's why I think that will be the best just to
>> add the code to slcr and keep others untouched.
> 
> Ok, just to clarify: You'd suggest to just add the rstc as child node
> to the slcr,
> and leave the other nodes untouched?
> 
>>
>> For example MACB/GEM is one example. Adding names to this node and
>> extending driver to work properly with reset means that all others MACB
>> users will be affected. Definitely this patch should be ACKed by Nicolas.

Actually, I don't know why a reset property should be added to the macb
driver...

Bye,
-- 
Nicolas Ferre

Re: [RFCv2 2/3] dts: zynq: Add devicetree entry for Xilinx Zynq reset controller.

[Michal Simek]

On 07/28/2015 08:59 AM, Nicolas Ferre wrote:
> Le 28/07/2015 07:03, Moritz Fischer a écrit :
>> Hi Michal,
>>
>> I agree we need to be careful with changing the bindings.
>>
>> On Sun, Jul 26, 2015 at 11:56 PM, Michal Simek <monstr@monstr.eu> wrote:
>>> Hi Moritz,
>>>
>>> On 07/25/2015 02:21 AM, Moritz Fischer wrote:
>>>> Signed-off-by: Moritz Fischer <moritz.fischer@ettus.com>
>>>> ---
>>>>  arch/arm/boot/dts/zynq-7000.dtsi            | 43 ++++++++++++-
>>>
>>> This patch is nice in general but every change in binding should be
>>> discussed separately. There is also necessary to wire them up in the
>>> driver to do action. That's why I think that will be the best just to
>>> add the code to slcr and keep others untouched.
>>
>> Ok, just to clarify: You'd suggest to just add the rstc as child node
>> to the slcr,
>> and leave the other nodes untouched?
>>
>>>
>>> For example MACB/GEM is one example. Adding names to this node and
>>> extending driver to work properly with reset means that all others MACB
>>> users will be affected. Definitely this patch should be ACKed by Nicolas.
> 
> Actually, I don't know why a reset property should be added to the macb
> driver...

I expect resetting IP core can solve something. But as I said it is
questionable if IP should be reset when driver is probed. Definitely on
Zynq there is a support for it. I am not aware about any problem which
requires IP to be reset.

Thanks,
Michal

Re: [RFCv2 1/3] docs: dts: Added documentation for Xilinx Zynq Reset Controller bindings.

[Philipp Zabel]

Am Freitag, den 24.07.2015, 17:21 -0700 schrieb Moritz Fischer:
> Signed-off-by: Moritz Fischer <moritz.fischer@ettus.com>
> ---
>  Documentation/devicetree/bindings/reset/zynq-reset-pl.txt | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/reset/zynq-reset-pl.txt
> 
> diff --git a/Documentation/devicetree/bindings/reset/zynq-reset-pl.txt b/Documentation/devicetree/bindings/reset/zynq-reset-pl.txt
> new file mode 100644
> index 0000000..ac4499e
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/reset/zynq-reset-pl.txt
> @@ -0,0 +1,13 @@
> +Xilinx Zynq PL Reset Manager
> +
> +Required properties:
> +- compatible: "xlnx,zynq-reset-pl"
> +- syscon <&slcr>;
> +- #reset-cells: 1
> +
> +Example:
> +	rstc: rstc@240 {
> +		#reset-cells = <1>;
> +		compatible = "xlnx,zynq-reset-pl";
> +		syscon = <&slcr>;

Why the syscon phandle if rstc always is the child of slcr? Why not just
request the syscon for the rstc's parent node.

> +	};

regards
Philipp

Re: [RFCv2 1/3] docs: dts: Added documentation for Xilinx Zynq Reset Controller bindings.

[Michal Simek]

On 07/28/2015 10:05 AM, Philipp Zabel wrote:
> Am Freitag, den 24.07.2015, 17:21 -0700 schrieb Moritz Fischer:
>> Signed-off-by: Moritz Fischer <moritz.fischer@ettus.com>
>> ---
>>  Documentation/devicetree/bindings/reset/zynq-reset-pl.txt | 13 +++++++++++++
>>  1 file changed, 13 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/reset/zynq-reset-pl.txt
>>
>> diff --git a/Documentation/devicetree/bindings/reset/zynq-reset-pl.txt b/Documentation/devicetree/bindings/reset/zynq-reset-pl.txt
>> new file mode 100644
>> index 0000000..ac4499e
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/reset/zynq-reset-pl.txt
>> @@ -0,0 +1,13 @@
>> +Xilinx Zynq PL Reset Manager
>> +
>> +Required properties:
>> +- compatible: "xlnx,zynq-reset-pl"
>> +- syscon <&slcr>;
>> +- #reset-cells: 1
>> +
>> +Example:
>> +	rstc: rstc@240 {
>> +		#reset-cells = <1>;
>> +		compatible = "xlnx,zynq-reset-pl";
>> +		syscon = <&slcr>;
> 
> Why the syscon phandle if rstc always is the child of slcr? Why not just
> request the syscon for the rstc's parent node.

We are using this description for pincntrl which was properly reviewed
that's why I expect Moritz just use the same style.
But yes also referencing parent should work.

TBH I don't have strong preference but having unified style is something
what I would prefer.

Also I see that using parent is used by others and it looks like that
having something like syscon_regmap_lookup_parent will be worth to have.

Thanks,
Michal

Re: [RFCv2 3/3] reset: reset-zynq: Adding support for Xilinx Zynq reset controller.

[Philipp Zabel]

Hi Moritz,

Am Freitag, den 24.07.2015, 17:21 -0700 schrieb Moritz Fischer:
> This adds a reset controller driver to control the Xilinx Zynq
> SoC's various resets.
> 
> Signed-off-by: Moritz Fischer <moritz.fischer@ettus.com>
> ---
>  drivers/reset/Makefile     |   1 +
>  drivers/reset/reset-zynq.c | 142 +++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 143 insertions(+)
>  create mode 100644 drivers/reset/reset-zynq.c
> 
> diff --git a/drivers/reset/Makefile b/drivers/reset/Makefile
> index 157d421..3fe50e7 100644
> --- a/drivers/reset/Makefile
> +++ b/drivers/reset/Makefile
> @@ -3,3 +3,4 @@ obj-$(CONFIG_ARCH_SOCFPGA) += reset-socfpga.o
>  obj-$(CONFIG_ARCH_BERLIN) += reset-berlin.o
>  obj-$(CONFIG_ARCH_SUNXI) += reset-sunxi.o
>  obj-$(CONFIG_ARCH_STI) += sti/
> +obj-$(CONFIG_ARCH_ZYNQ) += reset-zynq.o
> diff --git a/drivers/reset/reset-zynq.c b/drivers/reset/reset-zynq.c
> new file mode 100644
> index 0000000..05e37f8
> --- /dev/null
> +++ b/drivers/reset/reset-zynq.c
> @@ -0,0 +1,142 @@
> +/*
> + * Copyright (c) 2015, National Instruments Corp.
> + *
> + * Xilinx Zynq Reset controller driver
> + *
> + * 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; version 2 of the License.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + */
> +
> +#include <linux/err.h>
> +#include <linux/io.h>
> +#include <linux/module.h>
> +#include <linux/mfd/syscon.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/reset-controller.h>
> +#include <linux/regmap.h>
> +#include <linux/types.h>
> +
> +/* Offsets into SLCR regmap */
> +#define SLCR_RST_CTRL_OFFSET	0x200 /* FPGA Software Reset Control */

Maybe get this value from the reg property? I'm not sure if this is ever
expected to change.

> +#define NBANKS	18

reg = <0x200 0x50> says there are two more registers, are those not used?

> +struct zynq_reset_data {
> +	struct regmap *slcr;
> +	struct reset_controller_dev rcdev;
> +};
> +
> +#define to_zynq_reset_data(p)		\
> +	container_of((p), struct zynq_reset_data, rcdev)
> +
> +static int zynq_reset_assert(struct reset_controller_dev *rcdev,
> +			     unsigned long id)
> +{
> +	struct zynq_reset_data *priv = to_zynq_reset_data(rcdev);
> +
> +	int bank = id / BITS_PER_LONG;
> +	int offset = id % BITS_PER_LONG;
> +
> +	regmap_update_bits(priv->slcr,
> +			   SLCR_RST_CTRL_OFFSET + (bank * 4),
> +			   BIT(offset),
> +			   BIT(offset));
> +
> +	return 0;
> +}

Just "return regmap_update_bits(...)" here ...

> +static int zynq_reset_deassert(struct reset_controller_dev *rcdev,
> +			       unsigned long id)
> +{
> +	struct zynq_reset_data *priv = to_zynq_reset_data(rcdev);
> +
> +	int bank = id / BITS_PER_LONG;
> +	int offset = id % BITS_PER_LONG;
> +
> +	regmap_update_bits(priv->slcr,
> +			   SLCR_RST_CTRL_OFFSET + (bank * 4),
> +			   BIT(offset),
> +			   ~BIT(offset));
> +
> +	return 0;
> +}

... and here.

> +static int zynq_reset_status(struct reset_controller_dev *rcdev,
> +			     unsigned long id)
> +{
> +	struct zynq_reset_data *priv = to_zynq_reset_data(rcdev);
> +
> +	int bank = id / BITS_PER_LONG;
> +	int offset = id % BITS_PER_LONG;
> +	u32 reg;
> +
> +	regmap_read(priv->slcr, SLCR_RST_CTRL_OFFSET + (bank * 4), &reg);
> +
> +	return !(reg & BIT(offset));
> +}

Do I understand this correctly, you write 1 to assert the reset, but the
register reads 0 while the reset is asserted and 1 otherwise?
Also note that reset_status may return negative ERRNO, so for offset ==
31 you should not return (1<<31).

> +static const struct reset_control_ops zynq_reset_ops = {
> +	.assert		= zynq_reset_assert,
> +	.deassert	= zynq_reset_deassert,
> +	.status		= zynq_reset_status,
> +};
> +
> +static int zynq_reset_probe(struct platform_device *pdev)
> +{
> +	struct zynq_reset_data *priv;
> +
> +	priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +	platform_set_drvdata(pdev, priv);
> +
> +	priv->slcr = syscon_regmap_lookup_by_phandle(pdev->dev.of_node,
> +		"syscon");

I'd just use syscon_node_to_regmap(pdev->dev.of_node->parent) here,
which removes the need for the syscon phandle binding.

> +	if (IS_ERR(priv->slcr)) {
> +		dev_err(&pdev->dev, "unable to get zynq-slcr regmap");
> +		return PTR_ERR(priv->slcr);
> +	}
> +
> +	priv->rcdev.owner = THIS_MODULE;
> +	priv->rcdev.nr_resets = NBANKS * BITS_PER_LONG;
> +	priv->rcdev.ops = &zynq_reset_ops;
> +	priv->rcdev.of_node = pdev->dev.of_node;
> +	reset_controller_register(&priv->rcdev);
> +
> +	return 0;
> +}
> +
> +static int zynq_reset_remove(struct platform_device *pdev)
> +{
> +	struct zynq_reset_data *priv = platform_get_drvdata(pdev);
> +
> +	reset_controller_unregister(&priv->rcdev);
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id zynq_reset_dt_ids[] = {
> +	{ .compatible = "xlnx,zynq-reset", },
> +	{ /* sentinel */ },
> +};
> +
> +static struct platform_driver zynq_reset_driver = {
> +	.probe	= zynq_reset_probe,
> +	.remove	= zynq_reset_remove,
> +	.driver = {
> +		.name		= "zynq-pl-reset",

Is -pl- a leftover?

> +		.of_match_table	= zynq_reset_dt_ids,
> +	},
> +};
> +module_platform_driver(zynq_reset_driver);
> +
> +MODULE_LICENSE("GPL v2");
> +MODULE_AUTHOR("Moritz Fischer <moritz.fischer@ettus.com>");
> +MODULE_DESCRIPTION("Zynq Reset Controller Driver");

regards
Philipp

Re: [RFCv2 2/3] dts: zynq: Add devicetree entry for Xilinx Zynq reset controller.

[Moritz Fischer]

Nicolas, Michal,

if macb doesn't benefit from it, no need for the reset in there then.
I think Michal's suggestion of adding it on an as necessary basis works fine.
For the PATCH round I'll just have the SLCR in there and drivers can
add it to their nodes later on if required.

Thanks,
Moritz

On Tue, Jul 28, 2015 at 12:44 AM, Michal Simek <michal.simek@xilinx.com> wrote:
> On 07/28/2015 08:59 AM, Nicolas Ferre wrote:
>> Le 28/07/2015 07:03, Moritz Fischer a écrit :
>>> Hi Michal,
>>>
>>> I agree we need to be careful with changing the bindings.
>>>
>>> On Sun, Jul 26, 2015 at 11:56 PM, Michal Simek <monstr@monstr.eu> wrote:
>>>> Hi Moritz,
>>>>
>>>> On 07/25/2015 02:21 AM, Moritz Fischer wrote:
>>>>> Signed-off-by: Moritz Fischer <moritz.fischer@ettus.com>
>>>>> ---
>>>>>  arch/arm/boot/dts/zynq-7000.dtsi            | 43 ++++++++++++-
>>>>
>>>> This patch is nice in general but every change in binding should be
>>>> discussed separately. There is also necessary to wire them up in the
>>>> driver to do action. That's why I think that will be the best just to
>>>> add the code to slcr and keep others untouched.
>>>
>>> Ok, just to clarify: You'd suggest to just add the rstc as child node
>>> to the slcr,
>>> and leave the other nodes untouched?
>>>
>>>>
>>>> For example MACB/GEM is one example. Adding names to this node and
>>>> extending driver to work properly with reset means that all others MACB
>>>> users will be affected. Definitely this patch should be ACKed by Nicolas.
>>
>> Actually, I don't know why a reset property should be added to the macb
>> driver...
>
> I expect resetting IP core can solve something. But as I said it is
> questionable if IP should be reset when driver is probed. Definitely on
> Zynq there is a support for it. I am not aware about any problem which
> requires IP to be reset.
>
> Thanks,
> Michal
>

Re: [RFCv2 1/3] docs: dts: Added documentation for Xilinx Zynq Reset Controller bindings.

[Moritz Fischer]

Philip, Michal,

thanks for your reviews.

On Tue, Jul 28, 2015 at 1:25 AM, Michal Simek <michal.simek@xilinx.com> wrote:
> On 07/28/2015 10:05 AM, Philipp Zabel wrote:
>> Am Freitag, den 24.07.2015, 17:21 -0700 schrieb Moritz Fischer:
>>> Signed-off-by: Moritz Fischer <moritz.fischer@ettus.com>
>>> ---
>>>  Documentation/devicetree/bindings/reset/zynq-reset-pl.txt | 13 +++++++++++++
>>>  1 file changed, 13 insertions(+)
>>>  create mode 100644 Documentation/devicetree/bindings/reset/zynq-reset-pl.txt
>>>
>>> diff --git a/Documentation/devicetree/bindings/reset/zynq-reset-pl.txt b/Documentation/devicetree/bindings/reset/zynq-reset-pl.txt
>>> new file mode 100644
>>> index 0000000..ac4499e
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/reset/zynq-reset-pl.txt
>>> @@ -0,0 +1,13 @@
>>> +Xilinx Zynq PL Reset Manager
>>> +
>>> +Required properties:
>>> +- compatible: "xlnx,zynq-reset-pl"
>>> +- syscon <&slcr>;
>>> +- #reset-cells: 1
>>> +
>>> +Example:
>>> +    rstc: rstc@240 {
>>> +            #reset-cells = <1>;
>>> +            compatible = "xlnx,zynq-reset-pl";
>>> +            syscon = <&slcr>;
>>
>> Why the syscon phandle if rstc always is the child of slcr? Why not just
>> request the syscon for the rstc's parent node.
>
> We are using this description for pincntrl which was properly reviewed
> that's why I expect Moritz just use the same style.
> But yes also referencing parent should work.

Michal is right, I tried to be consistent with the pinctrl. Either one
is fine for me.
We'll just have to make a decision :-)
>
> TBH I don't have strong preference but having unified style is something
> what I would prefer.
>
> Also I see that using parent is used by others and it looks like that
> having something like syscon_regmap_lookup_parent will be worth to have.
>
> Thanks,
> Michal
>
>
Moritz

Re: [RFCv2 3/3] reset: reset-zynq: Adding support for Xilinx Zynq reset controller.

[Moritz Fischer]

Philip,

thanks for your review :)

On Tue, Jul 28, 2015 at 1:38 AM, Philipp Zabel <p.zabel@pengutronix.de> wrote:
> Hi Moritz,
>
> Am Freitag, den 24.07.2015, 17:21 -0700 schrieb Moritz Fischer:
>> This adds a reset controller driver to control the Xilinx Zynq
>> SoC's various resets.
>>
>> Signed-off-by: Moritz Fischer <moritz.fischer@ettus.com>
>> ---
>>  drivers/reset/Makefile     |   1 +
>>  drivers/reset/reset-zynq.c | 142 +++++++++++++++++++++++++++++++++++++++++++++
>>  2 files changed, 143 insertions(+)
>>  create mode 100644 drivers/reset/reset-zynq.c
>>
>> diff --git a/drivers/reset/Makefile b/drivers/reset/Makefile
>> index 157d421..3fe50e7 100644
>> --- a/drivers/reset/Makefile
>> +++ b/drivers/reset/Makefile
>> @@ -3,3 +3,4 @@ obj-$(CONFIG_ARCH_SOCFPGA) += reset-socfpga.o
>>  obj-$(CONFIG_ARCH_BERLIN) += reset-berlin.o
>>  obj-$(CONFIG_ARCH_SUNXI) += reset-sunxi.o
>>  obj-$(CONFIG_ARCH_STI) += sti/
>> +obj-$(CONFIG_ARCH_ZYNQ) += reset-zynq.o
>> diff --git a/drivers/reset/reset-zynq.c b/drivers/reset/reset-zynq.c
>> new file mode 100644
>> index 0000000..05e37f8
>> --- /dev/null
>> +++ b/drivers/reset/reset-zynq.c
>> @@ -0,0 +1,142 @@
>> +/*
>> + * Copyright (c) 2015, National Instruments Corp.
>> + *
>> + * Xilinx Zynq Reset controller driver
>> + *
>> + * 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; version 2 of the License.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
>> + * GNU General Public License for more details.
>> + */
>> +
>> +#include <linux/err.h>
>> +#include <linux/io.h>
>> +#include <linux/module.h>
>> +#include <linux/mfd/syscon.h>
>> +#include <linux/of.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/reset-controller.h>
>> +#include <linux/regmap.h>
>> +#include <linux/types.h>
>> +
>> +/* Offsets into SLCR regmap */
>> +#define SLCR_RST_CTRL_OFFSET 0x200 /* FPGA Software Reset Control */
>
> Maybe get this value from the reg property? I'm not sure if this is ever
> expected to change.
I don't think it's going to change. Is there a reason to only expose
part of the resets?
>
>> +#define NBANKS       18
>
> reg = <0x200 0x50> says there are two more registers, are those not used?

Should be 0x48, you're right. Michal had suggested 0x50, but the last
two regs are not really resets.
>
>> +struct zynq_reset_data {
>> +     struct regmap *slcr;
>> +     struct reset_controller_dev rcdev;
>> +};
>> +
>> +#define to_zynq_reset_data(p)                \
>> +     container_of((p), struct zynq_reset_data, rcdev)
>> +
>> +static int zynq_reset_assert(struct reset_controller_dev *rcdev,
>> +                          unsigned long id)
>> +{
>> +     struct zynq_reset_data *priv = to_zynq_reset_data(rcdev);
>> +
>> +     int bank = id / BITS_PER_LONG;
>> +     int offset = id % BITS_PER_LONG;
>> +
>> +     regmap_update_bits(priv->slcr,
>> +                        SLCR_RST_CTRL_OFFSET + (bank * 4),
>> +                        BIT(offset),
>> +                        BIT(offset));
>> +
>> +     return 0;
>> +}
>
> Just "return regmap_update_bits(...)" here ...
Yup, good point.
>
>> +static int zynq_reset_deassert(struct reset_controller_dev *rcdev,
>> +                            unsigned long id)
>> +{
>> +     struct zynq_reset_data *priv = to_zynq_reset_data(rcdev);
>> +
>> +     int bank = id / BITS_PER_LONG;
>> +     int offset = id % BITS_PER_LONG;
>> +
>> +     regmap_update_bits(priv->slcr,
>> +                        SLCR_RST_CTRL_OFFSET + (bank * 4),
>> +                        BIT(offset),
>> +                        ~BIT(offset));
>> +
>> +     return 0;
>> +}
>
> ... and here.
>
>> +static int zynq_reset_status(struct reset_controller_dev *rcdev,
>> +                          unsigned long id)
>> +{
>> +     struct zynq_reset_data *priv = to_zynq_reset_data(rcdev);
>> +
>> +     int bank = id / BITS_PER_LONG;
>> +     int offset = id % BITS_PER_LONG;
>> +     u32 reg;
>> +
>> +     regmap_read(priv->slcr, SLCR_RST_CTRL_OFFSET + (bank * 4), &reg);
>> +
>> +     return !(reg & BIT(offset));
>> +}

Will change to:
        ret = regmap_read(priv->slcr, SLCR_RST_CTRL_OFFSET + (bank * 4), &reg);
        if (ret)
                return ret;
        else
                return !!(reg & BIT(offset));

the single '!' was a typo ...

>
> Do I understand this correctly, you write 1 to assert the reset, but the
> register reads 0 while the reset is asserted and 1 otherwise?
> Also note that reset_status may return negative ERRNO, so for offset ==
> 31 you should not return (1<<31).
>
>> +static const struct reset_control_ops zynq_reset_ops = {
>> +     .assert         = zynq_reset_assert,
>> +     .deassert       = zynq_reset_deassert,
>> +     .status         = zynq_reset_status,
>> +};
>> +
>> +static int zynq_reset_probe(struct platform_device *pdev)
>> +{
>> +     struct zynq_reset_data *priv;
>> +
>> +     priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
>> +     if (!priv)
>> +             return -ENOMEM;
>> +     platform_set_drvdata(pdev, priv);
>> +
>> +     priv->slcr = syscon_regmap_lookup_by_phandle(pdev->dev.of_node,
>> +             "syscon");
>
> I'd just use syscon_node_to_regmap(pdev->dev.of_node->parent) here,
> which removes the need for the syscon phandle binding.

See binding doc discussion. I don't have a strong preference either way,
just tried to be consistent with the pinctrl node.
We just need a decision one way or the other :)
>
>> +     if (IS_ERR(priv->slcr)) {
>> +             dev_err(&pdev->dev, "unable to get zynq-slcr regmap");
>> +             return PTR_ERR(priv->slcr);
>> +     }
>> +
>> +     priv->rcdev.owner = THIS_MODULE;
>> +     priv->rcdev.nr_resets = NBANKS * BITS_PER_LONG;
>> +     priv->rcdev.ops = &zynq_reset_ops;
>> +     priv->rcdev.of_node = pdev->dev.of_node;
>> +     reset_controller_register(&priv->rcdev);
>> +
>> +     return 0;
>> +}
>> +
>> +static int zynq_reset_remove(struct platform_device *pdev)
>> +{
>> +     struct zynq_reset_data *priv = platform_get_drvdata(pdev);
>> +
>> +     reset_controller_unregister(&priv->rcdev);
>> +
>> +     return 0;
>> +}
>> +
>> +static const struct of_device_id zynq_reset_dt_ids[] = {
>> +     { .compatible = "xlnx,zynq-reset", },
>> +     { /* sentinel */ },
>> +};
>> +
>> +static struct platform_driver zynq_reset_driver = {
>> +     .probe  = zynq_reset_probe,
>> +     .remove = zynq_reset_remove,
>> +     .driver = {
>> +             .name           = "zynq-pl-reset",
>
> Is -pl- a leftover?
Yes ... will fix that.
>
>> +             .of_match_table = zynq_reset_dt_ids,
>> +     },
>> +};
>> +module_platform_driver(zynq_reset_driver);
>> +
>> +MODULE_LICENSE("GPL v2");
>> +MODULE_AUTHOR("Moritz Fischer <moritz.fischer@ettus.com>");
>> +MODULE_DESCRIPTION("Zynq Reset Controller Driver");
>
> regards
> Philipp
>
Thanks,

Moritz

Re: [RFCv2 3/3] reset: reset-zynq: Adding support for Xilinx Zynq reset controller.

[Sören Brinkmann]

On Tue, 2015-07-28 at 07:05AM -0700, Moritz Fischer wrote:
> Philip,
> 
> thanks for your review :)
> 
> On Tue, Jul 28, 2015 at 1:38 AM, Philipp Zabel <p.zabel@pengutronix.de> wrote:
> > Hi Moritz,
> >
> > Am Freitag, den 24.07.2015, 17:21 -0700 schrieb Moritz Fischer:
> >> This adds a reset controller driver to control the Xilinx Zynq
> >> SoC's various resets.
> >>
> >> Signed-off-by: Moritz Fischer <moritz.fischer@ettus.com>
[...]
> >> +
> >> +/* Offsets into SLCR regmap */
> >> +#define SLCR_RST_CTRL_OFFSET 0x200 /* FPGA Software Reset Control */
> >
> > Maybe get this value from the reg property? I'm not sure if this is ever
> > expected to change.
> I don't think it's going to change. Is there a reason to only expose
> part of the resets?

I think all other users of the syscon (in the Zynq DT) use the 'reg' property
to retrieve an offset into the SLCR. We should probably do that here too and
remove the #define. Who knows, maybe this driver is reusable with some
modifications for the Zynq MPSoC.

> >
> >> +#define NBANKS       18
> >
> > reg = <0x200 0x50> says there are two more registers, are those not used?
> 
> Should be 0x48, you're right. Michal had suggested 0x50, but the last
> two regs are not really resets.
> >
[...]
> >> +static int zynq_reset_status(struct reset_controller_dev *rcdev,
> >> +                          unsigned long id)
> >> +{
> >> +     struct zynq_reset_data *priv = to_zynq_reset_data(rcdev);
> >> +
> >> +     int bank = id / BITS_PER_LONG;
> >> +     int offset = id % BITS_PER_LONG;
> >> +     u32 reg;
> >> +
> >> +     regmap_read(priv->slcr, SLCR_RST_CTRL_OFFSET + (bank * 4), &reg);
> >> +
> >> +     return !(reg & BIT(offset));
> >> +}
> 
> Will change to:
>         ret = regmap_read(priv->slcr, SLCR_RST_CTRL_OFFSET + (bank * 4), &reg);
>         if (ret)
>                 return ret;
>         else
>                 return !!(reg & BIT(offset));
> 
> the single '!' was a typo ...

You have an early return on error in the if-branch. No need for the
else.

> 
> >
> > Do I understand this correctly, you write 1 to assert the reset, but the
> > register reads 0 while the reset is asserted and 1 otherwise?
> > Also note that reset_status may return negative ERRNO, so for offset ==
> > 31 you should not return (1<<31).
> >
> >> +static const struct reset_control_ops zynq_reset_ops = {
> >> +     .assert         = zynq_reset_assert,
> >> +     .deassert       = zynq_reset_deassert,
> >> +     .status         = zynq_reset_status,
> >> +};
> >> +
> >> +static int zynq_reset_probe(struct platform_device *pdev)
> >> +{
> >> +     struct zynq_reset_data *priv;
> >> +
> >> +     priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
> >> +     if (!priv)
> >> +             return -ENOMEM;
> >> +     platform_set_drvdata(pdev, priv);
> >> +
> >> +     priv->slcr = syscon_regmap_lookup_by_phandle(pdev->dev.of_node,
> >> +             "syscon");
> >
> > I'd just use syscon_node_to_regmap(pdev->dev.of_node->parent) here,
> > which removes the need for the syscon phandle binding.
> 
> See binding doc discussion. I don't have a strong preference either way,
> just tried to be consistent with the pinctrl node.
> We just need a decision one way or the other :)

I personally like the syscon handle better since it would make placing
the node more flexible, while this proposal forces some topology on the
DT. In both cases though, the syscon and this user are tightly coupled
and this driver depends on the syscon. I don't really mind either way -
I think.

	Sören

Re: [RFCv2 1/3] docs: dts: Added documentation for Xilinx Zynq Reset Controller bindings.

[Philipp Zabel]

Hi Moritz, Michal,

Am Dienstag, den 28.07.2015, 06:57 -0700 schrieb Moritz Fischer:
[...]
> >>> +Example:
> >>> +    rstc: rstc@240 {
> >>> +            #reset-cells = <1>;
> >>> +            compatible = "xlnx,zynq-reset-pl";
> >>> +            syscon = <&slcr>;
> >>
> >> Why the syscon phandle if rstc always is the child of slcr? Why not just
> >> request the syscon for the rstc's parent node.
> >
> > We are using this description for pincntrl which was properly reviewed
> > that's why I expect Moritz just use the same style.
> > But yes also referencing parent should work.
> 
> Michal is right, I tried to be consistent with the pinctrl. Either one
> is fine for me.
> We'll just have to make a decision :-)

Do you have a pointer to the pinctrl review? I'd like to know if
somebody had a good reason to use the phandle over the parent-child
relationship. I'd rather not add DT properties if they are not
necessary.
Regarding consistency, since the pinctrl node is also a child of the
slcr, you could just as well make the syscon phandle optional there and
remove it from the DT without breaking backwards compatibility.

> > TBH I don't have strong preference but having unified style is something
> > what I would prefer.
> >
> > Also I see that using parent is used by others and it looks like that
> > having something like syscon_regmap_lookup_parent will be worth to have.

That would be useful, yes.

regards
Philipp

Re: [RFCv2 1/3] docs: dts: Added documentation for Xilinx Zynq Reset Controller bindings.

[Sören Brinkmann]

On Mon, 2015-07-27 at 09:52PM -0700, Moritz Fischer wrote:
> Hi Sören,
> 
> thanks for your feedback.
> 
> On Mon, Jul 27, 2015 at 7:58 PM, Sören Brinkmann
> <soren.brinkmann@xilinx.com> wrote:
> > Hi Moritz,
> >
> > On Fri, 2015-07-24 at 05:21PM -0700, Moritz Fischer wrote:
> >> Signed-off-by: Moritz Fischer <moritz.fischer@ettus.com>
> >> ---
> >>  Documentation/devicetree/bindings/reset/zynq-reset-pl.txt | 13 +++++++++++++
> >>  1 file changed, 13 insertions(+)
> >>  create mode 100644 Documentation/devicetree/bindings/reset/zynq-reset-pl.txt
> >>
> >> diff --git a/Documentation/devicetree/bindings/reset/zynq-reset-pl.txt b/Documentation/devicetree/bindings/reset/zynq-reset-pl.txt
> >> new file mode 100644
> >> index 0000000..ac4499e
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/bindings/reset/zynq-reset-pl.txt
> >> @@ -0,0 +1,13 @@
> >> +Xilinx Zynq PL Reset Manager
> >> +
> >> +Required properties:
> >> +- compatible: "xlnx,zynq-reset-pl"
> >> +- syscon <&slcr>;
> >> +- #reset-cells: 1
> >> +
> >> +Example:
> >> +     rstc: rstc@240 {
> >> +             #reset-cells = <1>;
> >> +             compatible = "xlnx,zynq-reset-pl";
> >> +             syscon = <&slcr>;
> >> +     };
> >
> > I think you also have to add the outputs and make them part of the
> > binding. Otherwise you'd have to read the implementation to find
> > out what device should be hooked up to which output of the reset-controller.
> 
> Is something like this what you had in mind? I had that prepared for
> the next round of patches:
> 
> Reset outputs:
>  0  : soft reset
>  32 : ddr reset
>  64 : topsw reset
>  96 : dmac reset
>  128: usb0 reset
>  129: usb1 reset
>  160: gem0 reset
>  161: gem1 reset
>  164: gem0 rx reset
>  165: gem1 rx reset
>  166: gem0 ref reset
>  167: gem1 ref reset
>  192: sdio0 reset
>  193: sdio1 reset
>  196: sdio0 ref reset
>  197: sdio1 ref reset
>  224: spi0 reset
>  225: spi1 reset
>  226: spi0 ref reset
>  227: spi1 ref reset
>  256: can0 reset
>  257: can1 reset
>  258: can0 ref reset
>  259: can1 ref reset
>  288: i2c0 reset
>  289: i2c1 reset
>  320: uart0 reset
>  321: uart1 reset
>  322: uart0 ref reset
>  323: uart1 ref reset
>  352: gpio reset
>  384: lqspi reset
>  385: qspi ref reset
>  416: smc reset
>  417: smc ref reset
>  448: ocm reset
>  512: fpga0 out reset
>  513: fpga1 out reset
>  514: fpga2 out reset
>  515: fpga3 out reset
>  544: a9 reset 0
>  545: a9 reset 1
>  552: peri reset

Basically, yes. I guess the gaps are due to directly mapping this number
to bank and bit instead of doing some more complex mapping in between?
I'm not sure whether I like this :) I guess if a number is off the
driver would still toggle the addressed bit? I'm not sure, is it worth
to do some explicit mapping from logical outputs to a physical reset? It
seems it would be a little safer since it would be easy to check that
the addressed reset is valid and there wouldn't be any reserved/invalid
bits be toggled. Also, it would make the outputs in here a continuous
series of numbers without these gaps. Not sure though whether
it's worth the additional complexity in the implementation.

	Thanks,
	Sören

Re: [RFCv2 1/3] docs: dts: Added documentation for Xilinx Zynq Reset Controller bindings.

[Moritz Fischer]

Hi Sören,

On Tue, Jul 28, 2015 at 3:53 PM, Sören Brinkmann
<soren.brinkmann@xilinx.com> wrote:
> On Mon, 2015-07-27 at 09:52PM -0700, Moritz Fischer wrote:
>> Hi Sören,
>>
>> thanks for your feedback.
>>
>> On Mon, Jul 27, 2015 at 7:58 PM, Sören Brinkmann
>> <soren.brinkmann@xilinx.com> wrote:
>> > Hi Moritz,
>> >
>> > On Fri, 2015-07-24 at 05:21PM -0700, Moritz Fischer wrote:
>> >> Signed-off-by: Moritz Fischer <moritz.fischer@ettus.com>
>> >> ---
>> >>  Documentation/devicetree/bindings/reset/zynq-reset-pl.txt | 13 +++++++++++++
>> >>  1 file changed, 13 insertions(+)
>> >>  create mode 100644 Documentation/devicetree/bindings/reset/zynq-reset-pl.txt
>> >>
>> >> diff --git a/Documentation/devicetree/bindings/reset/zynq-reset-pl.txt b/Documentation/devicetree/bindings/reset/zynq-reset-pl.txt
>> >> new file mode 100644
>> >> index 0000000..ac4499e
>> >> --- /dev/null
>> >> +++ b/Documentation/devicetree/bindings/reset/zynq-reset-pl.txt
>> >> @@ -0,0 +1,13 @@
>> >> +Xilinx Zynq PL Reset Manager
>> >> +
>> >> +Required properties:
>> >> +- compatible: "xlnx,zynq-reset-pl"
>> >> +- syscon <&slcr>;
>> >> +- #reset-cells: 1
>> >> +
>> >> +Example:
>> >> +     rstc: rstc@240 {
>> >> +             #reset-cells = <1>;
>> >> +             compatible = "xlnx,zynq-reset-pl";
>> >> +             syscon = <&slcr>;
>> >> +     };
>> >
>> > I think you also have to add the outputs and make them part of the
>> > binding. Otherwise you'd have to read the implementation to find
>> > out what device should be hooked up to which output of the reset-controller.
>>
>> Is something like this what you had in mind? I had that prepared for
>> the next round of patches:
>>
>> Reset outputs:
>>  0  : soft reset
>>  32 : ddr reset
>>  64 : topsw reset
>>  96 : dmac reset
>>  128: usb0 reset
>>  129: usb1 reset
>>  160: gem0 reset
>>  161: gem1 reset
>>  164: gem0 rx reset
>>  165: gem1 rx reset
>>  166: gem0 ref reset
>>  167: gem1 ref reset
>>  192: sdio0 reset
>>  193: sdio1 reset
>>  196: sdio0 ref reset
>>  197: sdio1 ref reset
>>  224: spi0 reset
>>  225: spi1 reset
>>  226: spi0 ref reset
>>  227: spi1 ref reset
>>  256: can0 reset
>>  257: can1 reset
>>  258: can0 ref reset
>>  259: can1 ref reset
>>  288: i2c0 reset
>>  289: i2c1 reset
>>  320: uart0 reset
>>  321: uart1 reset
>>  322: uart0 ref reset
>>  323: uart1 ref reset
>>  352: gpio reset
>>  384: lqspi reset
>>  385: qspi ref reset
>>  416: smc reset
>>  417: smc ref reset
>>  448: ocm reset
>>  512: fpga0 out reset
>>  513: fpga1 out reset
>>  514: fpga2 out reset
>>  515: fpga3 out reset
>>  544: a9 reset 0
>>  545: a9 reset 1
>>  552: peri reset
>
> Basically, yes. I guess the gaps are due to directly mapping this number
> to bank and bit instead of doing some more complex mapping in between?
> I'm not sure whether I like this :) I guess if a number is off the
> driver would still toggle the addressed bit?

My assumption was that people would use a #include
<dt-bindings/xlnx,zynq-reset.h> in their dts. Assuming I got the
numbers in there right this makes it hard to misuse by accident.
I'm not saying it's perfect ...

> I'm not sure, is it worth
> to do some explicit mapping from logical outputs to a physical reset? It
> seems it would be a little safer since it would be easy to check that
> the addressed reset is valid and there wouldn't be any reserved/invalid
> bits be toggled. Also, it would make the outputs in here a continuous
> series of numbers without these gaps. Not sure though whether
> it's worth the additional complexity in the implementation.

So your suggestion is to have a large switch case kind of thingy? I
thought about it and it seemed complex as there's quite a bunch of
resets with gaps. Do you know of a driver that does something similar?
When I wrote this I looked at the other reset drivers and figured they
all had this kind of bank mapping of sorts so I assumed that's how
people usually do it.

>
>         Thanks,
>         Sören

Thanks again for your input,

Moritz

Re: [RFCv2 1/3] docs: dts: Added documentation for Xilinx Zynq Reset Controller bindings.

[Sören Brinkmann]

On Tue, 2015-07-28 at 11:14PM -0700, Moritz Fischer wrote:
> Hi Sören,
> 
> On Tue, Jul 28, 2015 at 3:53 PM, Sören Brinkmann
> <soren.brinkmann@xilinx.com> wrote:
> > On Mon, 2015-07-27 at 09:52PM -0700, Moritz Fischer wrote:
> >> Hi Sören,
> >>
> >> thanks for your feedback.
> >>
> >> On Mon, Jul 27, 2015 at 7:58 PM, Sören Brinkmann
> >> <soren.brinkmann@xilinx.com> wrote:
> >> > Hi Moritz,
> >> >
> >> > On Fri, 2015-07-24 at 05:21PM -0700, Moritz Fischer wrote:
> >> >> Signed-off-by: Moritz Fischer <moritz.fischer@ettus.com>
> >> >> ---
> >> >>  Documentation/devicetree/bindings/reset/zynq-reset-pl.txt | 13 +++++++++++++
> >> >>  1 file changed, 13 insertions(+)
> >> >>  create mode 100644 Documentation/devicetree/bindings/reset/zynq-reset-pl.txt
> >> >>
> >> >> diff --git a/Documentation/devicetree/bindings/reset/zynq-reset-pl.txt b/Documentation/devicetree/bindings/reset/zynq-reset-pl.txt
> >> >> new file mode 100644
> >> >> index 0000000..ac4499e
> >> >> --- /dev/null
> >> >> +++ b/Documentation/devicetree/bindings/reset/zynq-reset-pl.txt
> >> >> @@ -0,0 +1,13 @@
> >> >> +Xilinx Zynq PL Reset Manager
> >> >> +
> >> >> +Required properties:
> >> >> +- compatible: "xlnx,zynq-reset-pl"
> >> >> +- syscon <&slcr>;
> >> >> +- #reset-cells: 1
> >> >> +
> >> >> +Example:
> >> >> +     rstc: rstc@240 {
> >> >> +             #reset-cells = <1>;
> >> >> +             compatible = "xlnx,zynq-reset-pl";
> >> >> +             syscon = <&slcr>;
> >> >> +     };
> >> >
> >> > I think you also have to add the outputs and make them part of the
> >> > binding. Otherwise you'd have to read the implementation to find
> >> > out what device should be hooked up to which output of the reset-controller.
> >>
> >> Is something like this what you had in mind? I had that prepared for
> >> the next round of patches:
> >>
> >> Reset outputs:
> >>  0  : soft reset
> >>  32 : ddr reset
> >>  64 : topsw reset
> >>  96 : dmac reset
> >>  128: usb0 reset
> >>  129: usb1 reset
> >>  160: gem0 reset
> >>  161: gem1 reset
> >>  164: gem0 rx reset
> >>  165: gem1 rx reset
> >>  166: gem0 ref reset
> >>  167: gem1 ref reset
> >>  192: sdio0 reset
> >>  193: sdio1 reset
> >>  196: sdio0 ref reset
> >>  197: sdio1 ref reset
> >>  224: spi0 reset
> >>  225: spi1 reset
> >>  226: spi0 ref reset
> >>  227: spi1 ref reset
> >>  256: can0 reset
> >>  257: can1 reset
> >>  258: can0 ref reset
> >>  259: can1 ref reset
> >>  288: i2c0 reset
> >>  289: i2c1 reset
> >>  320: uart0 reset
> >>  321: uart1 reset
> >>  322: uart0 ref reset
> >>  323: uart1 ref reset
> >>  352: gpio reset
> >>  384: lqspi reset
> >>  385: qspi ref reset
> >>  416: smc reset
> >>  417: smc ref reset
> >>  448: ocm reset
> >>  512: fpga0 out reset
> >>  513: fpga1 out reset
> >>  514: fpga2 out reset
> >>  515: fpga3 out reset
> >>  544: a9 reset 0
> >>  545: a9 reset 1
> >>  552: peri reset
> >
> > Basically, yes. I guess the gaps are due to directly mapping this number
> > to bank and bit instead of doing some more complex mapping in between?
> > I'm not sure whether I like this :) I guess if a number is off the
> > driver would still toggle the addressed bit?
> 
> My assumption was that people would use a #include
> <dt-bindings/xlnx,zynq-reset.h> in their dts. Assuming I got the
> numbers in there right this makes it hard to misuse by accident.
> I'm not saying it's perfect ...

Michal always turned down the #include patches with the argument of
re-using the dts files outside of the Linux sources where those includes
etc may not be available in this form.

> 
> > I'm not sure, is it worth
> > to do some explicit mapping from logical outputs to a physical reset? It
> > seems it would be a little safer since it would be easy to check that
> > the addressed reset is valid and there wouldn't be any reserved/invalid
> > bits be toggled. Also, it would make the outputs in here a continuous
> > series of numbers without these gaps. Not sure though whether
> > it's worth the additional complexity in the implementation.
> 
> So your suggestion is to have a large switch case kind of thingy? I
> thought about it and it seemed complex as there's quite a bunch of
> resets with gaps. Do you know of a driver that does something similar?

Yeah, that was what I had in mind. Some big switch-case lookup that maps
a logical reset number from DT to the HW.
No, I haven't looked around what other drivers do. So, probably the
right thing is to just do what everybody else does.
I was more thinking about how easy it might be to re-use the driver for
the next-gen device.

> When I wrote this I looked at the other reset drivers and figured they
> all had this kind of bank mapping of sorts so I assumed that's how
> people usually do it.

Yeah, I don't think we should make things overly complicated without a
good reason. So, unless DT or reset folks have any objections, I'm fine
with it.

	Thanks,
	Sören

Re: [RFCv2 1/3] docs: dts: Added documentation for Xilinx Zynq Reset Controller bindings.

[Michal Simek]

On 07/29/2015 07:38 PM, Sören Brinkmann wrote:
> On Tue, 2015-07-28 at 11:14PM -0700, Moritz Fischer wrote:
>> Hi Sören,
>>
>> On Tue, Jul 28, 2015 at 3:53 PM, Sören Brinkmann
>> <soren.brinkmann@xilinx.com> wrote:
>>> On Mon, 2015-07-27 at 09:52PM -0700, Moritz Fischer wrote:
>>>> Hi Sören,
>>>>
>>>> thanks for your feedback.
>>>>
>>>> On Mon, Jul 27, 2015 at 7:58 PM, Sören Brinkmann
>>>> <soren.brinkmann@xilinx.com> wrote:
>>>>> Hi Moritz,
>>>>>
>>>>> On Fri, 2015-07-24 at 05:21PM -0700, Moritz Fischer wrote:
>>>>>> Signed-off-by: Moritz Fischer <moritz.fischer@ettus.com>
>>>>>> ---
>>>>>>  Documentation/devicetree/bindings/reset/zynq-reset-pl.txt | 13 +++++++++++++
>>>>>>  1 file changed, 13 insertions(+)
>>>>>>  create mode 100644 Documentation/devicetree/bindings/reset/zynq-reset-pl.txt
>>>>>>
>>>>>> diff --git a/Documentation/devicetree/bindings/reset/zynq-reset-pl.txt b/Documentation/devicetree/bindings/reset/zynq-reset-pl.txt
>>>>>> new file mode 100644
>>>>>> index 0000000..ac4499e
>>>>>> --- /dev/null
>>>>>> +++ b/Documentation/devicetree/bindings/reset/zynq-reset-pl.txt
>>>>>> @@ -0,0 +1,13 @@
>>>>>> +Xilinx Zynq PL Reset Manager
>>>>>> +
>>>>>> +Required properties:
>>>>>> +- compatible: "xlnx,zynq-reset-pl"
>>>>>> +- syscon <&slcr>;
>>>>>> +- #reset-cells: 1
>>>>>> +
>>>>>> +Example:
>>>>>> +     rstc: rstc@240 {
>>>>>> +             #reset-cells = <1>;
>>>>>> +             compatible = "xlnx,zynq-reset-pl";
>>>>>> +             syscon = <&slcr>;
>>>>>> +     };
>>>>>
>>>>> I think you also have to add the outputs and make them part of the
>>>>> binding. Otherwise you'd have to read the implementation to find
>>>>> out what device should be hooked up to which output of the reset-controller.
>>>>
>>>> Is something like this what you had in mind? I had that prepared for
>>>> the next round of patches:
>>>>
>>>> Reset outputs:
>>>>  0  : soft reset
>>>>  32 : ddr reset
>>>>  64 : topsw reset
>>>>  96 : dmac reset
>>>>  128: usb0 reset
>>>>  129: usb1 reset
>>>>  160: gem0 reset
>>>>  161: gem1 reset
>>>>  164: gem0 rx reset
>>>>  165: gem1 rx reset
>>>>  166: gem0 ref reset
>>>>  167: gem1 ref reset
>>>>  192: sdio0 reset
>>>>  193: sdio1 reset
>>>>  196: sdio0 ref reset
>>>>  197: sdio1 ref reset
>>>>  224: spi0 reset
>>>>  225: spi1 reset
>>>>  226: spi0 ref reset
>>>>  227: spi1 ref reset
>>>>  256: can0 reset
>>>>  257: can1 reset
>>>>  258: can0 ref reset
>>>>  259: can1 ref reset
>>>>  288: i2c0 reset
>>>>  289: i2c1 reset
>>>>  320: uart0 reset
>>>>  321: uart1 reset
>>>>  322: uart0 ref reset
>>>>  323: uart1 ref reset
>>>>  352: gpio reset
>>>>  384: lqspi reset
>>>>  385: qspi ref reset
>>>>  416: smc reset
>>>>  417: smc ref reset
>>>>  448: ocm reset
>>>>  512: fpga0 out reset
>>>>  513: fpga1 out reset
>>>>  514: fpga2 out reset
>>>>  515: fpga3 out reset
>>>>  544: a9 reset 0
>>>>  545: a9 reset 1
>>>>  552: peri reset
>>>
>>> Basically, yes. I guess the gaps are due to directly mapping this number
>>> to bank and bit instead of doing some more complex mapping in between?
>>> I'm not sure whether I like this :) I guess if a number is off the
>>> driver would still toggle the addressed bit?
>>
>> My assumption was that people would use a #include
>> <dt-bindings/xlnx,zynq-reset.h> in their dts. Assuming I got the
>> numbers in there right this makes it hard to misuse by accident.
>> I'm not saying it's perfect ...
> 
> Michal always turned down the #include patches with the argument of
> re-using the dts files outside of the Linux sources where those includes
> etc may not be available in this form.

yes. All these includes end up in more work when you want to generate
them or move to any other project.
I don't think that make sense to caused another problem just because of
that. Simple add these values to reset binding doc and then you <&rstc
number> in nodes.
Because we agreed that it has to be done on driver basis this is just
copy that macros from one file to another.

Thanks,
Michal