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
Signed-off-by: Moritz Fischer <[email protected]>
---
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
Signed-off-by: Moritz Fischer <[email protected]>
---
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
This adds a reset controller driver to control the Xilinx Zynq
SoC's various resets.
Signed-off-by: Moritz Fischer <[email protected]>
---
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), ®);
+
+ 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 <[email protected]>");
+MODULE_DESCRIPTION("Zynq Reset Controller Driver");
--
2.4.3
On 07/25/2015 02:21 AM, Moritz Fischer wrote:
> Signed-off-by: Moritz Fischer <[email protected]>
> ---
> 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: http://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
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 <[email protected]>
> ---
> 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), ®);
> +
> + 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 <[email protected]>");
> +MODULE_DESCRIPTION("Zynq Reset Controller Driver");
>
The rest looks good - will test it.
Thanks,
Michal
--
Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91
w: http://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
Hi Moritz,
On 07/25/2015 02:21 AM, Moritz Fischer wrote:
> Signed-off-by: Moritz Fischer <[email protected]>
> ---
> 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: http://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
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 <[email protected]>
> ---
> 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), ®);
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 <[email protected]>");
> +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: http://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
Hi Moritz,
On Fri, 2015-07-24 at 05:21PM -0700, Moritz Fischer wrote:
> Signed-off-by: Moritz Fischer <[email protected]>
> ---
> 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
Hi Sören,
thanks for your feedback.
On Mon, Jul 27, 2015 at 7:58 PM, Sören Brinkmann
<[email protected]> wrote:
> Hi Moritz,
>
> On Fri, 2015-07-24 at 05:21PM -0700, Moritz Fischer wrote:
>> Signed-off-by: Moritz Fischer <[email protected]>
>> ---
>> 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
Hi Michal,
On Sun, Jul 26, 2015 at 10:09 PM, Michal Simek <[email protected]> wrote:
> On 07/25/2015 02:21 AM, Moritz Fischer wrote:
>> Signed-off-by: Moritz Fischer <[email protected]>
>> ---
>> 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: http://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
Hi Michal,
On Mon, Jul 27, 2015 at 12:12 AM, Michal Simek <[email protected]> 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 <[email protected]>
>> ---
>> 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), ®);
>
> 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 <[email protected]>");
>> +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: http://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
Hi Michal,
I agree we need to be careful with changing the bindings.
On Sun, Jul 26, 2015 at 11:56 PM, Michal Simek <[email protected]> wrote:
> Hi Moritz,
>
> On 07/25/2015 02:21 AM, Moritz Fischer wrote:
>> Signed-off-by: Moritz Fischer <[email protected]>
>> ---
>> 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: http://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
On 07/28/2015 06:55 AM, Moritz Fischer wrote:
> Hi Michal,
>
> On Sun, Jul 26, 2015 at 10:09 PM, Michal Simek <[email protected]> wrote:
>> On 07/25/2015 02:21 AM, Moritz Fischer wrote:
>>> Signed-off-by: Moritz Fischer <[email protected]>
>>> ---
>>> 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: http://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
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 <[email protected]> wrote:
>> Hi Moritz,
>>
>> On 07/25/2015 02:21 AM, Moritz Fischer wrote:
>>> Signed-off-by: Moritz Fischer <[email protected]>
>>> ---
>>> 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: http://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
On 07/28/2015 06:59 AM, Moritz Fischer wrote:
> Hi Michal,
>
> On Mon, Jul 27, 2015 at 12:12 AM, Michal Simek <[email protected]> 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 <[email protected]>
>>> ---
>>> 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: http://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
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 <[email protected]> wrote:
>> Hi Moritz,
>>
>> On 07/25/2015 02:21 AM, Moritz Fischer wrote:
>>> Signed-off-by: Moritz Fischer <[email protected]>
>>> ---
>>> 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
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 <[email protected]> wrote:
>>> Hi Moritz,
>>>
>>> On 07/25/2015 02:21 AM, Moritz Fischer wrote:
>>>> Signed-off-by: Moritz Fischer <[email protected]>
>>>> ---
>>>> 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
Am Freitag, den 24.07.2015, 17:21 -0700 schrieb Moritz Fischer:
> Signed-off-by: Moritz Fischer <[email protected]>
> ---
> 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
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 <[email protected]>
>> ---
>> 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
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 <[email protected]>
> ---
> 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), ®);
> +
> + 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 <[email protected]>");
> +MODULE_DESCRIPTION("Zynq Reset Controller Driver");
regards
Philipp
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 <[email protected]> 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 <[email protected]> wrote:
>>>> Hi Moritz,
>>>>
>>>> On 07/25/2015 02:21 AM, Moritz Fischer wrote:
>>>>> Signed-off-by: Moritz Fischer <[email protected]>
>>>>> ---
>>>>> 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
>
Philip, Michal,
thanks for your reviews.
On Tue, Jul 28, 2015 at 1:25 AM, Michal Simek <[email protected]> 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 <[email protected]>
>>> ---
>>> 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
Philip,
thanks for your review :)
On Tue, Jul 28, 2015 at 1:38 AM, Philipp Zabel <[email protected]> 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 <[email protected]>
>> ---
>> 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), ®);
>> +
>> + return !(reg & BIT(offset));
>> +}
Will change to:
ret = regmap_read(priv->slcr, SLCR_RST_CTRL_OFFSET + (bank * 4), ®);
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 <[email protected]>");
>> +MODULE_DESCRIPTION("Zynq Reset Controller Driver");
>
> regards
> Philipp
>
Thanks,
Moritz
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 <[email protected]> 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 <[email protected]>
[...]
> >> +
> >> +/* 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), ®);
> >> +
> >> + return !(reg & BIT(offset));
> >> +}
>
> Will change to:
> ret = regmap_read(priv->slcr, SLCR_RST_CTRL_OFFSET + (bank * 4), ®);
> 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
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
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
> <[email protected]> wrote:
> > Hi Moritz,
> >
> > On Fri, 2015-07-24 at 05:21PM -0700, Moritz Fischer wrote:
> >> Signed-off-by: Moritz Fischer <[email protected]>
> >> ---
> >> 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
Hi Sören,
On Tue, Jul 28, 2015 at 3:53 PM, Sören Brinkmann
<[email protected]> 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
>> <[email protected]> wrote:
>> > Hi Moritz,
>> >
>> > On Fri, 2015-07-24 at 05:21PM -0700, Moritz Fischer wrote:
>> >> Signed-off-by: Moritz Fischer <[email protected]>
>> >> ---
>> >> 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
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
> <[email protected]> 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
> >> <[email protected]> wrote:
> >> > Hi Moritz,
> >> >
> >> > On Fri, 2015-07-24 at 05:21PM -0700, Moritz Fischer wrote:
> >> >> Signed-off-by: Moritz Fischer <[email protected]>
> >> >> ---
> >> >> 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
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
>> <[email protected]> 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
>>>> <[email protected]> wrote:
>>>>> Hi Moritz,
>>>>>
>>>>> On Fri, 2015-07-24 at 05:21PM -0700, Moritz Fischer wrote:
>>>>>> Signed-off-by: Moritz Fischer <[email protected]>
>>>>>> ---
>>>>>> 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