[PATCH v4 0/3] Reset controller support for i.MX8MQ

[PATCH v4 0/3] Reset controller support for i.MX8MQ

[Andrey Smirnov]

Everyone:

This patch contains changes I made in order to add support for i.MX8MQ
to reset-imx7.c in order to enable support of PCIE IP block on i.MX8MQ
SoCs.

Feedback is welcome!

Thanks,
Andrey Smirnov

Changes since [v3]

 - Kconfig entry is converted to use ARCH_MXC

 - Collected Reviewed-by from Rob Herring

Changes since [v2]

 - Constified "variant" field in struct imx7_src

 - Moved dt-bindings/reset/imx8mq-reset.h to be a part of the patch
   documenting DT bindings

Changes since [v1]

 - Series re-written to use a per-variant LUT instead of using a
   single table

- Changed driver to use "imx8mq" insead of "imx8m" to match other
  drivers and CONFIG_ARCH_IMX8MQ

- Updated list of exported i.MX8MQ resets, add missing and remove
  bogus ones (hopefully nothing is missing this time)

[v3] lkml.kernel.org/r/20181217023827.7947-1-andrew.smirnov@gmail.com
[v2] lkml.kernel.org/r/20181128043738.12714-1-andrew.smirnov@gmail.com
[v1] lkml.kernel.org/r/20181117181131.9330-2-andrew.smirnov@gmail.com

Andrey Smirnov (3):
  reset: imx7: Add plubming to support multiple IP variants
  dt-bindings: reset: imx7: Document usage on i.MX8MQ SoCs
  reset: imx7: Add support for i.MX8MQ IP block variant

 .../bindings/reset/fsl,imx7-src.txt           |   7 +-
 drivers/reset/Kconfig                         |   4 +-
 drivers/reset/reset-imx7.c                    | 166 ++++++++++++++++--
 include/dt-bindings/reset/imx8mq-reset.h      |  64 +++++++
 4 files changed, 221 insertions(+), 20 deletions(-)
 create mode 100644 include/dt-bindings/reset/imx8mq-reset.h

-- 
2.19.1

[PATCH v4 1/3] reset: imx7: Add plubming to support multiple IP variants

[Andrey Smirnov]

In order to enable supporting i.MX8MQ with this driver, convert it to
expect variant specific bits to be passed via driver data.

Cc: p.zabel@pengutronix.de
Cc: Fabio Estevam <fabio.estevam@nxp.com>
Cc: cphealy@gmail.com
Cc: l.stach@pengutronix.de
Cc: Leonard Crestez <leonard.crestez@nxp.com>
Cc: "A.s. Dong" <aisheng.dong@nxp.com>
Cc: Richard Zhu <hongxing.zhu@nxp.com>
Cc: Rob Herring <robh@kernel.org>
Cc: devicetree@vger.kernel.org
Cc: linux-imx@nxp.com
Cc: linux-arm-kernel@lists.infradead.org
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
---
 drivers/reset/reset-imx7.c | 62 +++++++++++++++++++++++++++-----------
 1 file changed, 45 insertions(+), 17 deletions(-)

diff --git a/drivers/reset/reset-imx7.c b/drivers/reset/reset-imx7.c
index 77911fa8f31d..3a36d5863891 100644
--- a/drivers/reset/reset-imx7.c
+++ b/drivers/reset/reset-imx7.c
@@ -17,14 +17,29 @@
 
 #include <linux/mfd/syscon.h>
 #include <linux/mod_devicetable.h>
+#include <linux/of_device.h>
 #include <linux/platform_device.h>
 #include <linux/reset-controller.h>
 #include <linux/regmap.h>
 #include <dt-bindings/reset/imx7-reset.h>
 
+struct imx7_src_signal {
+	unsigned int offset, bit;
+};
+
+struct imx7_src;
+
+struct imx7_src_variant {
+	const struct imx7_src_signal *signals;
+	unsigned int signals_num;
+	unsigned int (*prepare)(struct imx7_src *imx7src, unsigned long id,
+				bool assert);
+};
+
 struct imx7_src {
 	struct reset_controller_dev rcdev;
 	struct regmap *regmap;
+	const struct imx7_src_variant *variant;
 };
 
 enum imx7_src_registers {
@@ -39,10 +54,6 @@ enum imx7_src_registers {
 	SRC_DDRC_RCR		= 0x1000,
 };
 
-struct imx7_src_signal {
-	unsigned int offset, bit;
-};
-
 static const struct imx7_src_signal imx7_src_signals[IMX7_RESET_NUM] = {
 	[IMX7_RESET_A7_CORE_POR_RESET0] = { SRC_A7RCR0, BIT(0) },
 	[IMX7_RESET_A7_CORE_POR_RESET1] = { SRC_A7RCR0, BIT(1) },
@@ -72,17 +83,11 @@ static const struct imx7_src_signal imx7_src_signals[IMX7_RESET_NUM] = {
 	[IMX7_RESET_DDRC_CORE_RST]	= { SRC_DDRC_RCR, BIT(1) },
 };
 
-static struct imx7_src *to_imx7_src(struct reset_controller_dev *rcdev)
+static unsigned int
+imx7_src_prepare(struct imx7_src *imx7src, unsigned long id, bool assert)
 {
-	return container_of(rcdev, struct imx7_src, rcdev);
-}
-
-static int imx7_reset_set(struct reset_controller_dev *rcdev,
-			  unsigned long id, bool assert)
-{
-	struct imx7_src *imx7src = to_imx7_src(rcdev);
-	const struct imx7_src_signal *signal = &imx7_src_signals[id];
-	unsigned int value = assert ? signal->bit : 0;
+	const unsigned int bit = imx7src->variant->signals[id].bit;
+	unsigned int value = assert ? bit : 0;
 
 	switch (id) {
 	case IMX7_RESET_PCIEPHY:
@@ -95,10 +100,32 @@ static int imx7_reset_set(struct reset_controller_dev *rcdev,
 		break;
 
 	case IMX7_RESET_PCIE_CTRL_APPS_EN:
-		value = (assert) ? 0 : signal->bit;
+		value = assert ? 0 : bit;
 		break;
 	}
 
+	return value;
+}
+
+static const struct imx7_src_variant variant_imx7 = {
+	.signals = imx7_src_signals,
+	.signals_num = ARRAY_SIZE(imx7_src_signals),
+	.prepare = imx7_src_prepare,
+};
+
+static struct imx7_src *to_imx7_src(struct reset_controller_dev *rcdev)
+{
+	return container_of(rcdev, struct imx7_src, rcdev);
+}
+
+static int imx7_reset_set(struct reset_controller_dev *rcdev,
+			  unsigned long id, bool assert)
+{
+	struct imx7_src *imx7src = to_imx7_src(rcdev);
+	const struct imx7_src_variant *variant = imx7src->variant;
+	const struct imx7_src_signal *signal = &variant->signals[id];
+	const unsigned int value = variant->prepare(imx7src, id, assert);
+
 	return regmap_update_bits(imx7src->regmap,
 				  signal->offset, signal->bit, value);
 }
@@ -130,6 +157,7 @@ static int imx7_reset_probe(struct platform_device *pdev)
 	if (!imx7src)
 		return -ENOMEM;
 
+	imx7src->variant = of_device_get_match_data(dev);
 	imx7src->regmap = syscon_node_to_regmap(dev->of_node);
 	if (IS_ERR(imx7src->regmap)) {
 		dev_err(dev, "Unable to get imx7-src regmap");
@@ -138,7 +166,7 @@ static int imx7_reset_probe(struct platform_device *pdev)
 	regmap_attach_dev(dev, imx7src->regmap, &config);
 
 	imx7src->rcdev.owner     = THIS_MODULE;
-	imx7src->rcdev.nr_resets = IMX7_RESET_NUM;
+	imx7src->rcdev.nr_resets = imx7src->variant->signals_num;
 	imx7src->rcdev.ops       = &imx7_reset_ops;
 	imx7src->rcdev.of_node   = dev->of_node;
 
@@ -146,7 +174,7 @@ static int imx7_reset_probe(struct platform_device *pdev)
 }
 
 static const struct of_device_id imx7_reset_dt_ids[] = {
-	{ .compatible = "fsl,imx7d-src", },
+	{ .compatible = "fsl,imx7d-src", .data = &variant_imx7 },
 	{ /* sentinel */ },
 };
 
-- 
2.19.1

[PATCH v4 2/3] dt-bindings: reset: imx7: Document usage on i.MX8MQ SoCs

[Andrey Smirnov]

The driver now supports i.MX8MQ, so update bindings accordingly.

Cc: p.zabel@pengutronix.de
Cc: Fabio Estevam <fabio.estevam@nxp.com>
Cc: cphealy@gmail.com
Cc: l.stach@pengutronix.de
Cc: Leonard Crestez <leonard.crestez@nxp.com>
Cc: "A.s. Dong" <aisheng.dong@nxp.com>
Cc: Richard Zhu <hongxing.zhu@nxp.com>
Cc: linux-imx@nxp.com
Cc: linux-arm-kernel@lists.infradead.org
Cc: linux-kernel@vger.kernel.org
Reviewed-by: Rob Herring <robh@kernel.org>
Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
---
 .../bindings/reset/fsl,imx7-src.txt           |  7 +-
 include/dt-bindings/reset/imx8mq-reset.h      | 64 +++++++++++++++++++
 2 files changed, 69 insertions(+), 2 deletions(-)
 create mode 100644 include/dt-bindings/reset/imx8mq-reset.h

diff --git a/Documentation/devicetree/bindings/reset/fsl,imx7-src.txt b/Documentation/devicetree/bindings/reset/fsl,imx7-src.txt
index 1ab1d109318e..2ecf33815d18 100644
--- a/Documentation/devicetree/bindings/reset/fsl,imx7-src.txt
+++ b/Documentation/devicetree/bindings/reset/fsl,imx7-src.txt
@@ -5,7 +5,9 @@ Please also refer to reset.txt in this directory for common reset
 controller binding usage.
 
 Required properties:
-- compatible: Should be "fsl,imx7d-src", "syscon"
+- compatible:
+	- For i.MX7 SoCs should be "fsl,imx7d-src", "syscon"
+	- For i.MX8MQ SoCs should be "fsl,imx8mq-src", "syscon"
 - reg: should be register base and length as documented in the
   datasheet
 - interrupts: Should contain SRC interrupt
@@ -44,4 +46,5 @@ Example:
 
 
 For list of all valid reset indicies see
-<dt-bindings/reset/imx7-reset.h>
+<dt-bindings/reset/imx7-reset.h> for i.MX7 and
+<dt-bindings/reset/imx8mq-reset.h> for i.MX8MQ
diff --git a/include/dt-bindings/reset/imx8mq-reset.h b/include/dt-bindings/reset/imx8mq-reset.h
new file mode 100644
index 000000000000..57c592498aa0
--- /dev/null
+++ b/include/dt-bindings/reset/imx8mq-reset.h
@@ -0,0 +1,64 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Copyright (C) 2018 Zodiac Inflight Innovations
+ *
+ * Author: Andrey Smirnov <andrew.smirnov@gmail.com>
+ */
+
+#ifndef DT_BINDING_RESET_IMX8MQ_H
+#define DT_BINDING_RESET_IMX8MQ_H
+
+#define IMX8MQ_RESET_A53_CORE_POR_RESET0	0
+#define IMX8MQ_RESET_A53_CORE_POR_RESET1	1
+#define IMX8MQ_RESET_A53_CORE_POR_RESET2	2
+#define IMX8MQ_RESET_A53_CORE_POR_RESET3	3
+#define IMX8MQ_RESET_A53_CORE_RESET0		4
+#define IMX8MQ_RESET_A53_CORE_RESET1		5
+#define IMX8MQ_RESET_A53_CORE_RESET2		6
+#define IMX8MQ_RESET_A53_CORE_RESET3		7
+#define IMX8MQ_RESET_A53_DBG_RESET0		8
+#define IMX8MQ_RESET_A53_DBG_RESET1		9
+#define IMX8MQ_RESET_A53_DBG_RESET2		10
+#define IMX8MQ_RESET_A53_DBG_RESET3		11
+#define IMX8MQ_RESET_A53_ETM_RESET0		12
+#define IMX8MQ_RESET_A53_ETM_RESET1		13
+#define IMX8MQ_RESET_A53_ETM_RESET2		14
+#define IMX8MQ_RESET_A53_ETM_RESET3		15
+#define IMX8MQ_RESET_A53_SOC_DBG_RESET		16
+#define IMX8MQ_RESET_A53_L2RESET		17
+#define IMX8MQ_RESET_SW_NON_SCLR_M4C_RST	18
+#define IMX8MQ_RESET_OTG1_PHY_RESET		19
+#define IMX8MQ_RESET_OTG2_PHY_RESET		20
+#define IMX8MQ_RESET_MIPI_DSI_RESET_BYTE_N	21
+#define IMX8MQ_RESET_MIPI_DSI_RESET_N		22
+#define IMX8MQ_RESET_MIPI_DIS_DPI_RESET_N	23
+#define IMX8MQ_RESET_MIPI_DIS_ESC_RESET_N	24
+#define IMX8MQ_RESET_MIPI_DIS_PCLK_RESET_N	25
+#define IMX8MQ_RESET_PCIEPHY			26
+#define IMX8MQ_RESET_PCIEPHY_PERST		27
+#define IMX8MQ_RESET_PCIE_CTRL_APPS_EN		28
+#define IMX8MQ_RESET_PCIE_CTRL_APPS_TURNOFF	29
+#define IMX8MQ_RESET_HDMI_PHY_APB_RESET		30
+#define IMX8MQ_RESET_DISP_RESET			31
+#define IMX8MQ_RESET_GPU_RESET			32
+#define IMX8MQ_RESET_VPU_RESET			33
+#define IMX8MQ_RESET_PCIEPHY2			34
+#define IMX8MQ_RESET_PCIEPHY2_PERST		35
+#define IMX8MQ_RESET_PCIE2_CTRL_APPS_EN		36
+#define IMX8MQ_RESET_PCIE2_CTRL_APPS_TURNOFF	37
+#define IMX8MQ_RESET_MIPI_CSI1_CORE_RESET	38
+#define IMX8MQ_RESET_MIPI_CSI1_PHY_REF_RESET	39
+#define IMX8MQ_RESET_MIPI_CSI1_ESC_RESET	40
+#define IMX8MQ_RESET_MIPI_CSI2_CORE_RESET	41
+#define IMX8MQ_RESET_MIPI_CSI2_PHY_REF_RESET	42
+#define IMX8MQ_RESET_MIPI_CSI2_ESC_RESET	43
+#define IMX8MQ_RESET_DDRC1_PRST			44
+#define IMX8MQ_RESET_DDRC1_CORE_RESET		45
+#define IMX8MQ_RESET_DDRC1_PHY_RESET		46
+#define IMX8MQ_RESET_DDRC2_PRST			47
+#define IMX8MQ_RESET_DDRC2_CORE_RESET		48
+#define IMX8MQ_RESET_DDRC2_PHY_RESET		49
+
+#define IMX8MQ_RESET_NUM			50
+
+#endif
-- 
2.19.1

[PATCH v4 3/3] reset: imx7: Add support for i.MX8MQ IP block variant

[Andrey Smirnov]

Add bits and pieces needed to support IP block variant found on
i.MX8MQ SoCs.

Cc: p.zabel@pengutronix.de
Cc: Fabio Estevam <fabio.estevam@nxp.com>
Cc: cphealy@gmail.com
Cc: l.stach@pengutronix.de
Cc: Leonard Crestez <leonard.crestez@nxp.com>
Cc: "A.s. Dong" <aisheng.dong@nxp.com>
Cc: Richard Zhu <hongxing.zhu@nxp.com>
Cc: Rob Herring <robh@kernel.org>
Cc: devicetree@vger.kernel.org
Cc: linux-imx@nxp.com
Cc: linux-arm-kernel@lists.infradead.org
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
---
 drivers/reset/Kconfig      |   4 +-
 drivers/reset/reset-imx7.c | 106 +++++++++++++++++++++++++++++++++++++
 2 files changed, 108 insertions(+), 2 deletions(-)

diff --git a/drivers/reset/Kconfig b/drivers/reset/Kconfig
index c21da9fe51ec..add84b523be1 100644
--- a/drivers/reset/Kconfig
+++ b/drivers/reset/Kconfig
@@ -48,9 +48,9 @@ config RESET_HSDK
 	  This enables the reset controller driver for HSDK board.
 
 config RESET_IMX7
-	bool "i.MX7 Reset Driver" if COMPILE_TEST
+	bool "i.MX7/8 Reset Driver" if COMPILE_TEST
 	depends on HAS_IOMEM
-	default SOC_IMX7D
+	default SOC_IMX7D || (ARM64 && ARCH_MXC)
 	select MFD_SYSCON
 	help
 	  This enables the reset controller driver for i.MX7 SoCs.
diff --git a/drivers/reset/reset-imx7.c b/drivers/reset/reset-imx7.c
index 3a36d5863891..bb826935db6d 100644
--- a/drivers/reset/reset-imx7.c
+++ b/drivers/reset/reset-imx7.c
@@ -22,6 +22,7 @@
 #include <linux/reset-controller.h>
 #include <linux/regmap.h>
 #include <dt-bindings/reset/imx7-reset.h>
+#include <dt-bindings/reset/imx8mq-reset.h>
 
 struct imx7_src_signal {
 	unsigned int offset, bit;
@@ -113,6 +114,110 @@ static const struct imx7_src_variant variant_imx7 = {
 	.prepare = imx7_src_prepare,
 };
 
+enum imx8mq_src_registers {
+	SRC_A53RCR0		= 0x0004,
+	SRC_HDMI_RCR		= 0x0030,
+	SRC_DISP_RCR		= 0x0034,
+	SRC_GPU_RCR		= 0x0040,
+	SRC_VPU_RCR		= 0x0044,
+	SRC_PCIE2_RCR		= 0x0048,
+	SRC_MIPIPHY1_RCR	= 0x004c,
+	SRC_MIPIPHY2_RCR	= 0x0050,
+	SRC_DDRC2_RCR		= 0x1004,
+};
+
+static const struct imx7_src_signal imx8mq_src_signals[IMX8MQ_RESET_NUM] = {
+	[IMX8MQ_RESET_A53_CORE_POR_RESET0]	= { SRC_A53RCR0, BIT(0) },
+	[IMX8MQ_RESET_A53_CORE_POR_RESET1]	= { SRC_A53RCR0, BIT(1) },
+	[IMX8MQ_RESET_A53_CORE_POR_RESET2]	= { SRC_A53RCR0, BIT(2) },
+	[IMX8MQ_RESET_A53_CORE_POR_RESET3]	= { SRC_A53RCR0, BIT(3) },
+	[IMX8MQ_RESET_A53_CORE_RESET0]		= { SRC_A53RCR0, BIT(4) },
+	[IMX8MQ_RESET_A53_CORE_RESET1]		= { SRC_A53RCR0, BIT(5) },
+	[IMX8MQ_RESET_A53_CORE_RESET2]		= { SRC_A53RCR0, BIT(6) },
+	[IMX8MQ_RESET_A53_CORE_RESET3]		= { SRC_A53RCR0, BIT(7) },
+	[IMX8MQ_RESET_A53_DBG_RESET0]		= { SRC_A53RCR0, BIT(8) },
+	[IMX8MQ_RESET_A53_DBG_RESET1]		= { SRC_A53RCR0, BIT(9) },
+	[IMX8MQ_RESET_A53_DBG_RESET2]		= { SRC_A53RCR0, BIT(10) },
+	[IMX8MQ_RESET_A53_DBG_RESET3]		= { SRC_A53RCR0, BIT(11) },
+	[IMX8MQ_RESET_A53_ETM_RESET0]		= { SRC_A53RCR0, BIT(12) },
+	[IMX8MQ_RESET_A53_ETM_RESET1]		= { SRC_A53RCR0, BIT(13) },
+	[IMX8MQ_RESET_A53_ETM_RESET2]		= { SRC_A53RCR0, BIT(14) },
+	[IMX8MQ_RESET_A53_ETM_RESET3]		= { SRC_A53RCR0, BIT(15) },
+	[IMX8MQ_RESET_A53_SOC_DBG_RESET]	= { SRC_A53RCR0, BIT(20) },
+	[IMX8MQ_RESET_A53_L2RESET]		= { SRC_A53RCR0, BIT(21) },
+	[IMX8MQ_RESET_SW_NON_SCLR_M4C_RST]	= { SRC_M4RCR, BIT(0) },
+	[IMX8MQ_RESET_OTG1_PHY_RESET]		= { SRC_USBOPHY1_RCR, BIT(0) },
+	[IMX8MQ_RESET_OTG2_PHY_RESET]		= { SRC_USBOPHY2_RCR, BIT(0) },
+	[IMX8MQ_RESET_MIPI_DSI_RESET_BYTE_N]	= { SRC_MIPIPHY_RCR, BIT(1) },
+	[IMX8MQ_RESET_MIPI_DSI_RESET_N]		= { SRC_MIPIPHY_RCR, BIT(2) },
+	[IMX8MQ_RESET_MIPI_DIS_DPI_RESET_N]	= { SRC_MIPIPHY_RCR, BIT(3) },
+	[IMX8MQ_RESET_MIPI_DIS_ESC_RESET_N]	= { SRC_MIPIPHY_RCR, BIT(4) },
+	[IMX8MQ_RESET_MIPI_DIS_PCLK_RESET_N]	= { SRC_MIPIPHY_RCR, BIT(5) },
+	[IMX8MQ_RESET_PCIEPHY]			= { SRC_PCIEPHY_RCR,
+						    BIT(2) | BIT(1) },
+	[IMX8MQ_RESET_PCIEPHY_PERST]		= { SRC_PCIEPHY_RCR, BIT(3) },
+	[IMX8MQ_RESET_PCIE_CTRL_APPS_EN]	= { SRC_PCIEPHY_RCR, BIT(6) },
+	[IMX8MQ_RESET_PCIE_CTRL_APPS_TURNOFF]	= { SRC_PCIEPHY_RCR, BIT(11) },
+	[IMX8MQ_RESET_HDMI_PHY_APB_RESET]	= { SRC_HDMI_RCR, BIT(0) },
+	[IMX8MQ_RESET_DISP_RESET]		= { SRC_DISP_RCR, BIT(0) },
+	[IMX8MQ_RESET_GPU_RESET]		= { SRC_GPU_RCR, BIT(0) },
+	[IMX8MQ_RESET_VPU_RESET]		= { SRC_VPU_RCR, BIT(0) },
+	[IMX8MQ_RESET_PCIEPHY2]			= { SRC_PCIE2_RCR,
+						    BIT(2) | BIT(1) },
+	[IMX8MQ_RESET_PCIEPHY2_PERST]		= { SRC_PCIE2_RCR, BIT(3) },
+	[IMX8MQ_RESET_PCIE2_CTRL_APPS_EN]	= { SRC_PCIE2_RCR, BIT(6) },
+	[IMX8MQ_RESET_PCIE2_CTRL_APPS_TURNOFF]	= { SRC_PCIE2_RCR, BIT(11) },
+	[IMX8MQ_RESET_MIPI_CSI1_CORE_RESET]	= { SRC_MIPIPHY1_RCR, BIT(0) },
+	[IMX8MQ_RESET_MIPI_CSI1_PHY_REF_RESET]	= { SRC_MIPIPHY1_RCR, BIT(1) },
+	[IMX8MQ_RESET_MIPI_CSI1_ESC_RESET]	= { SRC_MIPIPHY1_RCR, BIT(2) },
+	[IMX8MQ_RESET_MIPI_CSI2_CORE_RESET]	= { SRC_MIPIPHY2_RCR, BIT(0) },
+	[IMX8MQ_RESET_MIPI_CSI2_PHY_REF_RESET]	= { SRC_MIPIPHY2_RCR, BIT(1) },
+	[IMX8MQ_RESET_MIPI_CSI2_ESC_RESET]	= { SRC_MIPIPHY2_RCR, BIT(2) },
+	[IMX8MQ_RESET_DDRC1_PRST]		= { SRC_DDRC_RCR, BIT(0) },
+	[IMX8MQ_RESET_DDRC1_CORE_RESET]		= { SRC_DDRC_RCR, BIT(1) },
+	[IMX8MQ_RESET_DDRC1_PHY_RESET]		= { SRC_DDRC_RCR, BIT(2) },
+	[IMX8MQ_RESET_DDRC2_PHY_RESET]		= { SRC_DDRC2_RCR, BIT(0) },
+	[IMX8MQ_RESET_DDRC2_CORE_RESET]		= { SRC_DDRC2_RCR, BIT(1) },
+	[IMX8MQ_RESET_DDRC2_PRST]		= { SRC_DDRC2_RCR, BIT(2) },
+};
+
+static unsigned int
+imx8mq_src_prepare(struct imx7_src *imx7src, unsigned long id, bool assert)
+{
+	const unsigned int bit = imx7src->variant->signals[id].bit;
+	unsigned int value = assert ? bit : 0;
+
+	switch (id) {
+	case IMX8MQ_RESET_PCIEPHY:
+	case IMX8MQ_RESET_PCIEPHY2: /* fallthrough */
+		/*
+		 * wait for more than 10us to release phy g_rst and
+		 * btnrst
+		 */
+		if (!assert)
+			udelay(10);
+		break;
+
+	case IMX8MQ_RESET_PCIE_CTRL_APPS_EN:
+	case IMX8MQ_RESET_PCIE2_CTRL_APPS_EN:	/* fallthrough */
+	case IMX8MQ_RESET_MIPI_DIS_PCLK_RESET_N:	/* fallthrough */
+	case IMX8MQ_RESET_MIPI_DIS_ESC_RESET_N:	/* fallthrough */
+	case IMX8MQ_RESET_MIPI_DIS_DPI_RESET_N:	/* fallthrough */
+	case IMX8MQ_RESET_MIPI_DSI_RESET_N:	/* fallthrough */
+	case IMX8MQ_RESET_MIPI_DSI_RESET_BYTE_N:	/* fallthrough */
+		value = assert ? 0 : bit;
+		break;
+	}
+
+	return value;
+}
+
+static const struct imx7_src_variant variant_imx8mq = {
+	.signals = imx8mq_src_signals,
+	.signals_num = ARRAY_SIZE(imx8mq_src_signals),
+	.prepare = imx8mq_src_prepare,
+};
+
 static struct imx7_src *to_imx7_src(struct reset_controller_dev *rcdev)
 {
 	return container_of(rcdev, struct imx7_src, rcdev);
@@ -175,6 +280,7 @@ static int imx7_reset_probe(struct platform_device *pdev)
 
 static const struct of_device_id imx7_reset_dt_ids[] = {
 	{ .compatible = "fsl,imx7d-src", .data = &variant_imx7 },
+	{ .compatible = "fsl,imx8mq-src", .data = &variant_imx8mq },
 	{ /* sentinel */ },
 };
 
-- 
2.19.1

Re: [PATCH v4 0/3] Reset controller support for i.MX8MQ

[Andrey Smirnov]

On Wed, Dec 19, 2018 at 5:07 PM Andrey Smirnov <andrew.smirnov@gmail.com> wrote:
>
> Everyone:
>
> This patch contains changes I made in order to add support for i.MX8MQ
> to reset-imx7.c in order to enable support of PCIE IP block on i.MX8MQ
> SoCs.
>
> Feedback is welcome!
>
> Thanks,
> Andrey Smirnov
>

Philipp, are there any changes that needs to be made to this series,
or is it good enough to be accepted?

Thanks,
Andrey Smirnov

Re: [PATCH v4 2/3] dt-bindings: reset: imx7: Document usage on i.MX8MQ SoCs

[Philipp Zabel]

Hi Andrey,

sorry for the delay. Thank you for the update, apart from the comments
below, the list now looks to be complete.

On Wed, 2018-12-19 at 17:06 -0800, Andrey Smirnov wrote:
> The driver now supports i.MX8MQ, so update bindings accordingly.
> 
> Cc: p.zabel@pengutronix.de
> Cc: Fabio Estevam <fabio.estevam@nxp.com>
> Cc: cphealy@gmail.com
> Cc: l.stach@pengutronix.de
> Cc: Leonard Crestez <leonard.crestez@nxp.com>
> Cc: "A.s. Dong" <aisheng.dong@nxp.com>
> Cc: Richard Zhu <hongxing.zhu@nxp.com>
> Cc: linux-imx@nxp.com
> Cc: linux-arm-kernel@lists.infradead.org
> Cc: linux-kernel@vger.kernel.org
> Reviewed-by: Rob Herring <robh@kernel.org>
> Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
> ---
>  .../bindings/reset/fsl,imx7-src.txt           |  7 +-
>  include/dt-bindings/reset/imx8mq-reset.h      | 64 +++++++++++++++++++
>  2 files changed, 69 insertions(+), 2 deletions(-)
>  create mode 100644 include/dt-bindings/reset/imx8mq-reset.h
> 
> diff --git a/Documentation/devicetree/bindings/reset/fsl,imx7-src.txt b/Documentation/devicetree/bindings/reset/fsl,imx7-src.txt
> index 1ab1d109318e..2ecf33815d18 100644
> --- a/Documentation/devicetree/bindings/reset/fsl,imx7-src.txt
> +++ b/Documentation/devicetree/bindings/reset/fsl,imx7-src.txt
> @@ -5,7 +5,9 @@ Please also refer to reset.txt in this directory for common reset
>  controller binding usage.
>  
>  Required properties:
> -- compatible: Should be "fsl,imx7d-src", "syscon"
> +- compatible:
> +	- For i.MX7 SoCs should be "fsl,imx7d-src", "syscon"
> +	- For i.MX8MQ SoCs should be "fsl,imx8mq-src", "syscon"
>  - reg: should be register base and length as documented in the
>    datasheet
>  - interrupts: Should contain SRC interrupt
> @@ -44,4 +46,5 @@ Example:
>  
>  
>  For list of all valid reset indicies see
> -<dt-bindings/reset/imx7-reset.h>
> +<dt-bindings/reset/imx7-reset.h> for i.MX7 and
> +<dt-bindings/reset/imx8mq-reset.h> for i.MX8MQ
> diff --git a/include/dt-bindings/reset/imx8mq-reset.h b/include/dt-bindings/reset/imx8mq-reset.h
> new file mode 100644
> index 000000000000..57c592498aa0
> --- /dev/null
> +++ b/include/dt-bindings/reset/imx8mq-reset.h
> @@ -0,0 +1,64 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Copyright (C) 2018 Zodiac Inflight Innovations
> + *
> + * Author: Andrey Smirnov <andrew.smirnov@gmail.com>
> + */
> +
> +#ifndef DT_BINDING_RESET_IMX8MQ_H
> +#define DT_BINDING_RESET_IMX8MQ_H
> +
> +#define IMX8MQ_RESET_A53_CORE_POR_RESET0	0
> +#define IMX8MQ_RESET_A53_CORE_POR_RESET1	1
> +#define IMX8MQ_RESET_A53_CORE_POR_RESET2	2
> +#define IMX8MQ_RESET_A53_CORE_POR_RESET3	3
> +#define IMX8MQ_RESET_A53_CORE_RESET0		4
> +#define IMX8MQ_RESET_A53_CORE_RESET1		5
> +#define IMX8MQ_RESET_A53_CORE_RESET2		6
> +#define IMX8MQ_RESET_A53_CORE_RESET3		7
> +#define IMX8MQ_RESET_A53_DBG_RESET0		8
> +#define IMX8MQ_RESET_A53_DBG_RESET1		9
> +#define IMX8MQ_RESET_A53_DBG_RESET2		10
> +#define IMX8MQ_RESET_A53_DBG_RESET3		11
> +#define IMX8MQ_RESET_A53_ETM_RESET0		12
> +#define IMX8MQ_RESET_A53_ETM_RESET1		13
> +#define IMX8MQ_RESET_A53_ETM_RESET2		14
> +#define IMX8MQ_RESET_A53_ETM_RESET3		15
> +#define IMX8MQ_RESET_A53_SOC_DBG_RESET		16
> +#define IMX8MQ_RESET_A53_L2RESET		17
> +#define IMX8MQ_RESET_SW_NON_SCLR_M4C_RST	18
                           ^^^^^^^^

This might be an implementation detail. The reset line is
(SW_?)M4C_RST. I suppose the self-clearing SW_M4C_RST bit could be used
to implement .reset() functionality for the same line if needed.

What about the self-clearing SW_M4P_RST bit? Has that been left out on
purpose?

> +#define IMX8MQ_RESET_OTG1_PHY_RESET		19
> +#define IMX8MQ_RESET_OTG2_PHY_RESET		20
> +#define IMX8MQ_RESET_MIPI_DSI_RESET_BYTE_N	21
> +#define IMX8MQ_RESET_MIPI_DSI_RESET_N		22
> +#define IMX8MQ_RESET_MIPI_DIS_DPI_RESET_N	23
> +#define IMX8MQ_RESET_MIPI_DIS_ESC_RESET_N	24
> +#define IMX8MQ_RESET_MIPI_DIS_PCLK_RESET_N	25
> +#define IMX8MQ_RESET_PCIEPHY			26
> +#define IMX8MQ_RESET_PCIEPHY_PERST		27
> +#define IMX8MQ_RESET_PCIE_CTRL_APPS_EN		28
> +#define IMX8MQ_RESET_PCIE_CTRL_APPS_TURNOFF	29

To be honest, I don't like these two, I'm not convinced anymore that
they actually qualify as reset signals. To me it looks like this is
something that the PCIe glue code should handle via syscon like i.MX6.
Leonard, Lucas, what do you think?

> +#define IMX8MQ_RESET_HDMI_PHY_APB_RESET		30
> +#define IMX8MQ_RESET_DISP_RESET			31
> +#define IMX8MQ_RESET_GPU_RESET			32
> +#define IMX8MQ_RESET_VPU_RESET			33
> +#define IMX8MQ_RESET_PCIEPHY2			34
> +#define IMX8MQ_RESET_PCIEPHY2_PERST		35
> +#define IMX8MQ_RESET_PCIE2_CTRL_APPS_EN		36
> +#define IMX8MQ_RESET_PCIE2_CTRL_APPS_TURNOFF	37

Same issue as PCIe #1

> +#define IMX8MQ_RESET_MIPI_CSI1_CORE_RESET	38
> +#define IMX8MQ_RESET_MIPI_CSI1_PHY_REF_RESET	39
> +#define IMX8MQ_RESET_MIPI_CSI1_ESC_RESET	40
> +#define IMX8MQ_RESET_MIPI_CSI2_CORE_RESET	41
> +#define IMX8MQ_RESET_MIPI_CSI2_PHY_REF_RESET	42
> +#define IMX8MQ_RESET_MIPI_CSI2_ESC_RESET	43
> +#define IMX8MQ_RESET_DDRC1_PRST			44
> +#define IMX8MQ_RESET_DDRC1_CORE_RESET		45
> +#define IMX8MQ_RESET_DDRC1_PHY_RESET		46

Does anybody know what the DDRC1_PHY_PWROKIN bit is, right next to the
PHY reset?

> +#define IMX8MQ_RESET_DDRC2_PRST			47
> +#define IMX8MQ_RESET_DDRC2_CORE_RESET		48
> +#define IMX8MQ_RESET_DDRC2_PHY_RESET		49
> +
> +#define IMX8MQ_RESET_NUM			50
> +
> +#endif

regards
Philipp

Re: [PATCH v4 1/3] reset: imx7: Add plubming to support multiple IP variants

[Philipp Zabel]

On Wed, 2018-12-19 at 17:06 -0800, Andrey Smirnov wrote:
> In order to enable supporting i.MX8MQ with this driver, convert it to
> expect variant specific bits to be passed via driver data.
> 
> Cc: p.zabel@pengutronix.de
> Cc: Fabio Estevam <fabio.estevam@nxp.com>
> Cc: cphealy@gmail.com
> Cc: l.stach@pengutronix.de
> Cc: Leonard Crestez <leonard.crestez@nxp.com>
> Cc: "A.s. Dong" <aisheng.dong@nxp.com>
> Cc: Richard Zhu <hongxing.zhu@nxp.com>
> Cc: Rob Herring <robh@kernel.org>
> Cc: devicetree@vger.kernel.org
> Cc: linux-imx@nxp.com
> Cc: linux-arm-kernel@lists.infradead.org
> Cc: linux-kernel@vger.kernel.org
> Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
> ---
>  drivers/reset/reset-imx7.c | 62 +++++++++++++++++++++++++++-----------
>  1 file changed, 45 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/reset/reset-imx7.c b/drivers/reset/reset-imx7.c
> index 77911fa8f31d..3a36d5863891 100644
> --- a/drivers/reset/reset-imx7.c
> +++ b/drivers/reset/reset-imx7.c
> @@ -17,14 +17,29 @@
>  
>  #include <linux/mfd/syscon.h>
>  #include <linux/mod_devicetable.h>
> +#include <linux/of_device.h>
>  #include <linux/platform_device.h>
>  #include <linux/reset-controller.h>
>  #include <linux/regmap.h>
>  #include <dt-bindings/reset/imx7-reset.h>
>  
> +struct imx7_src_signal {
> +	unsigned int offset, bit;
> +};
> +
> +struct imx7_src;
> +
> +struct imx7_src_variant {
> +	const struct imx7_src_signal *signals;
> +	unsigned int signals_num;
> +	unsigned int (*prepare)(struct imx7_src *imx7src, unsigned long id,
> +				bool assert);

Instead of adding a function pointer indirection, I'd prefer separate
imx7_reset_ops and imx8m_reset_ops set by the variant, see below.

> +};
> +
>  struct imx7_src {
>  	struct reset_controller_dev rcdev;
>  	struct regmap *regmap;
> +	const struct imx7_src_variant *variant;

This could then replaced with a direct pointer to the respective signals
array.

>  };
>  
>  enum imx7_src_registers {
> @@ -39,10 +54,6 @@ enum imx7_src_registers {
>  	SRC_DDRC_RCR		= 0x1000,
>  };
>  
> -struct imx7_src_signal {
> -	unsigned int offset, bit;
> -};
> -
>  static const struct imx7_src_signal imx7_src_signals[IMX7_RESET_NUM] = {
>  	[IMX7_RESET_A7_CORE_POR_RESET0] = { SRC_A7RCR0, BIT(0) },
>  	[IMX7_RESET_A7_CORE_POR_RESET1] = { SRC_A7RCR0, BIT(1) },
> @@ -72,17 +83,11 @@ static const struct imx7_src_signal imx7_src_signals[IMX7_RESET_NUM] = {
>  	[IMX7_RESET_DDRC_CORE_RST]	= { SRC_DDRC_RCR, BIT(1) },
>  };
>  
> -static struct imx7_src *to_imx7_src(struct reset_controller_dev *rcdev)
> +static unsigned int
> +imx7_src_prepare(struct imx7_src *imx7src, unsigned long id, bool assert)
>  {
> -	return container_of(rcdev, struct imx7_src, rcdev);
> -}
> -
> -static int imx7_reset_set(struct reset_controller_dev *rcdev,
> -			  unsigned long id, bool assert)
> -{
> -	struct imx7_src *imx7src = to_imx7_src(rcdev);
> -	const struct imx7_src_signal *signal = &imx7_src_signals[id];
> -	unsigned int value = assert ? signal->bit : 0;
> +	const unsigned int bit = imx7src->variant->signals[id].bit;
> +	unsigned int value = assert ? bit : 0;
>  
>  	switch (id) {
>  	case IMX7_RESET_PCIEPHY:
> @@ -95,10 +100,32 @@ static int imx7_reset_set(struct reset_controller_dev *rcdev,
>  		break;
>  
>  	case IMX7_RESET_PCIE_CTRL_APPS_EN:
> -		value = (assert) ? 0 : signal->bit;
> +		value = assert ? 0 : bit;
>  		break;
>  	}
>  
> +	return value;
> +}

Instead of having a common imx7_reset_set and then calling the custom
.prepare() through a function pointer, I'd suggest to have custom
imx7_reset_set and imx8m_reset_set functions that contain the code from
.prepare() and then call a common function to do the actual register
access.

> +
> +static const struct imx7_src_variant variant_imx7 = {
> +	.signals = imx7_src_signals,
> +	.signals_num = ARRAY_SIZE(imx7_src_signals),
> +	.prepare = imx7_src_prepare,
> +};
> +
> +static struct imx7_src *to_imx7_src(struct reset_controller_dev *rcdev)
> +{
> +	return container_of(rcdev, struct imx7_src, rcdev);
> +}
> +
> +static int imx7_reset_set(struct reset_controller_dev *rcdev,
> +			  unsigned long id, bool assert)
> +{
> +	struct imx7_src *imx7src = to_imx7_src(rcdev);
> +	const struct imx7_src_variant *variant = imx7src->variant;
> +	const struct imx7_src_signal *signal = &variant->signals[id];
> +	const unsigned int value = variant->prepare(imx7src, id, assert);
> +
>  	return regmap_update_bits(imx7src->regmap,
>  				  signal->offset, signal->bit, value);
>  }
> @@ -130,6 +157,7 @@ static int imx7_reset_probe(struct platform_device *pdev)
>  	if (!imx7src)
>  		return -ENOMEM;
>  
> +	imx7src->variant = of_device_get_match_data(dev);
>  	imx7src->regmap = syscon_node_to_regmap(dev->of_node);
>  	if (IS_ERR(imx7src->regmap)) {
>  		dev_err(dev, "Unable to get imx7-src regmap");
> @@ -138,7 +166,7 @@ static int imx7_reset_probe(struct platform_device *pdev)
>  	regmap_attach_dev(dev, imx7src->regmap, &config);
>  
>  	imx7src->rcdev.owner     = THIS_MODULE;
> -	imx7src->rcdev.nr_resets = IMX7_RESET_NUM;
> +	imx7src->rcdev.nr_resets = imx7src->variant->signals_num;
>  	imx7src->rcdev.ops       = &imx7_reset_ops;
>  	imx7src->rcdev.of_node   = dev->of_node;
>  
> @@ -146,7 +174,7 @@ static int imx7_reset_probe(struct platform_device *pdev)
>  }
>  
>  static const struct of_device_id imx7_reset_dt_ids[] = {
> -	{ .compatible = "fsl,imx7d-src", },
> +	{ .compatible = "fsl,imx7d-src", .data = &variant_imx7 },
>  	{ /* sentinel */ },
>  };

regards
Phililipp

Re: [PATCH v4 1/3] reset: imx7: Add plubming to support multiple IP variants

[Andrey Smirnov]

On Thu, Jan 17, 2019 at 9:16 AM Philipp Zabel <p.zabel@pengutronix.de> wrote:
>
> On Wed, 2018-12-19 at 17:06 -0800, Andrey Smirnov wrote:
> > In order to enable supporting i.MX8MQ with this driver, convert it to
> > expect variant specific bits to be passed via driver data.
> >
> > Cc: p.zabel@pengutronix.de
> > Cc: Fabio Estevam <fabio.estevam@nxp.com>
> > Cc: cphealy@gmail.com
> > Cc: l.stach@pengutronix.de
> > Cc: Leonard Crestez <leonard.crestez@nxp.com>
> > Cc: "A.s. Dong" <aisheng.dong@nxp.com>
> > Cc: Richard Zhu <hongxing.zhu@nxp.com>
> > Cc: Rob Herring <robh@kernel.org>
> > Cc: devicetree@vger.kernel.org
> > Cc: linux-imx@nxp.com
> > Cc: linux-arm-kernel@lists.infradead.org
> > Cc: linux-kernel@vger.kernel.org
> > Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
> > ---
> >  drivers/reset/reset-imx7.c | 62 +++++++++++++++++++++++++++-----------
> >  1 file changed, 45 insertions(+), 17 deletions(-)
> >
> > diff --git a/drivers/reset/reset-imx7.c b/drivers/reset/reset-imx7.c
> > index 77911fa8f31d..3a36d5863891 100644
> > --- a/drivers/reset/reset-imx7.c
> > +++ b/drivers/reset/reset-imx7.c
> > @@ -17,14 +17,29 @@
> >
> >  #include <linux/mfd/syscon.h>
> >  #include <linux/mod_devicetable.h>
> > +#include <linux/of_device.h>
> >  #include <linux/platform_device.h>
> >  #include <linux/reset-controller.h>
> >  #include <linux/regmap.h>
> >  #include <dt-bindings/reset/imx7-reset.h>
> >
> > +struct imx7_src_signal {
> > +     unsigned int offset, bit;
> > +};
> > +
> > +struct imx7_src;
> > +
> > +struct imx7_src_variant {
> > +     const struct imx7_src_signal *signals;
> > +     unsigned int signals_num;
> > +     unsigned int (*prepare)(struct imx7_src *imx7src, unsigned long id,
> > +                             bool assert);
>
> Instead of adding a function pointer indirection, I'd prefer separate
> imx7_reset_ops and imx8m_reset_ops set by the variant, see below.
>
> > +};
> > +
> >  struct imx7_src {
> >       struct reset_controller_dev rcdev;
> >       struct regmap *regmap;
> > +     const struct imx7_src_variant *variant;
>
> This could then replaced with a direct pointer to the respective signals
> array.
>
> >  };
> >
> >  enum imx7_src_registers {
> > @@ -39,10 +54,6 @@ enum imx7_src_registers {
> >       SRC_DDRC_RCR            = 0x1000,
> >  };
> >
> > -struct imx7_src_signal {
> > -     unsigned int offset, bit;
> > -};
> > -
> >  static const struct imx7_src_signal imx7_src_signals[IMX7_RESET_NUM] = {
> >       [IMX7_RESET_A7_CORE_POR_RESET0] = { SRC_A7RCR0, BIT(0) },
> >       [IMX7_RESET_A7_CORE_POR_RESET1] = { SRC_A7RCR0, BIT(1) },
> > @@ -72,17 +83,11 @@ static const struct imx7_src_signal imx7_src_signals[IMX7_RESET_NUM] = {
> >       [IMX7_RESET_DDRC_CORE_RST]      = { SRC_DDRC_RCR, BIT(1) },
> >  };
> >
> > -static struct imx7_src *to_imx7_src(struct reset_controller_dev *rcdev)
> > +static unsigned int
> > +imx7_src_prepare(struct imx7_src *imx7src, unsigned long id, bool assert)
> >  {
> > -     return container_of(rcdev, struct imx7_src, rcdev);
> > -}
> > -
> > -static int imx7_reset_set(struct reset_controller_dev *rcdev,
> > -                       unsigned long id, bool assert)
> > -{
> > -     struct imx7_src *imx7src = to_imx7_src(rcdev);
> > -     const struct imx7_src_signal *signal = &imx7_src_signals[id];
> > -     unsigned int value = assert ? signal->bit : 0;
> > +     const unsigned int bit = imx7src->variant->signals[id].bit;
> > +     unsigned int value = assert ? bit : 0;
> >
> >       switch (id) {
> >       case IMX7_RESET_PCIEPHY:
> > @@ -95,10 +100,32 @@ static int imx7_reset_set(struct reset_controller_dev *rcdev,
> >               break;
> >
> >       case IMX7_RESET_PCIE_CTRL_APPS_EN:
> > -             value = (assert) ? 0 : signal->bit;
> > +             value = assert ? 0 : bit;
> >               break;
> >       }
> >
> > +     return value;
> > +}
>
> Instead of having a common imx7_reset_set and then calling the custom
> .prepare() through a function pointer, I'd suggest to have custom
> imx7_reset_set and imx8m_reset_set functions that contain the code from
> .prepare() and then call a common function to do the actual register
> access.
>

OK, makes sense, will give it a spin in v5.

Thanks,
Andrey Smirnov

Re: [PATCH v4 2/3] dt-bindings: reset: imx7: Document usage on i.MX8MQ SoCs

[Andrey Smirnov]

On Thu, Jan 17, 2019 at 8:45 AM Philipp Zabel <p.zabel@pengutronix.de> wrote:
>
> Hi Andrey,
>
> sorry for the delay. Thank you for the update, apart from the comments
> below, the list now looks to be complete.
>
> On Wed, 2018-12-19 at 17:06 -0800, Andrey Smirnov wrote:
> > The driver now supports i.MX8MQ, so update bindings accordingly.
> >
> > Cc: p.zabel@pengutronix.de
> > Cc: Fabio Estevam <fabio.estevam@nxp.com>
> > Cc: cphealy@gmail.com
> > Cc: l.stach@pengutronix.de
> > Cc: Leonard Crestez <leonard.crestez@nxp.com>
> > Cc: "A.s. Dong" <aisheng.dong@nxp.com>
> > Cc: Richard Zhu <hongxing.zhu@nxp.com>
> > Cc: linux-imx@nxp.com
> > Cc: linux-arm-kernel@lists.infradead.org
> > Cc: linux-kernel@vger.kernel.org
> > Reviewed-by: Rob Herring <robh@kernel.org>
> > Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
> > ---
> >  .../bindings/reset/fsl,imx7-src.txt           |  7 +-
> >  include/dt-bindings/reset/imx8mq-reset.h      | 64 +++++++++++++++++++
> >  2 files changed, 69 insertions(+), 2 deletions(-)
> >  create mode 100644 include/dt-bindings/reset/imx8mq-reset.h
> >
> > diff --git a/Documentation/devicetree/bindings/reset/fsl,imx7-src.txt b/Documentation/devicetree/bindings/reset/fsl,imx7-src.txt
> > index 1ab1d109318e..2ecf33815d18 100644
> > --- a/Documentation/devicetree/bindings/reset/fsl,imx7-src.txt
> > +++ b/Documentation/devicetree/bindings/reset/fsl,imx7-src.txt
> > @@ -5,7 +5,9 @@ Please also refer to reset.txt in this directory for common reset
> >  controller binding usage.
> >
> >  Required properties:
> > -- compatible: Should be "fsl,imx7d-src", "syscon"
> > +- compatible:
> > +     - For i.MX7 SoCs should be "fsl,imx7d-src", "syscon"
> > +     - For i.MX8MQ SoCs should be "fsl,imx8mq-src", "syscon"
> >  - reg: should be register base and length as documented in the
> >    datasheet
> >  - interrupts: Should contain SRC interrupt
> > @@ -44,4 +46,5 @@ Example:
> >
> >
> >  For list of all valid reset indicies see
> > -<dt-bindings/reset/imx7-reset.h>
> > +<dt-bindings/reset/imx7-reset.h> for i.MX7 and
> > +<dt-bindings/reset/imx8mq-reset.h> for i.MX8MQ
> > diff --git a/include/dt-bindings/reset/imx8mq-reset.h b/include/dt-bindings/reset/imx8mq-reset.h
> > new file mode 100644
> > index 000000000000..57c592498aa0
> > --- /dev/null
> > +++ b/include/dt-bindings/reset/imx8mq-reset.h
> > @@ -0,0 +1,64 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +/*
> > + * Copyright (C) 2018 Zodiac Inflight Innovations
> > + *
> > + * Author: Andrey Smirnov <andrew.smirnov@gmail.com>
> > + */
> > +
> > +#ifndef DT_BINDING_RESET_IMX8MQ_H
> > +#define DT_BINDING_RESET_IMX8MQ_H
> > +
> > +#define IMX8MQ_RESET_A53_CORE_POR_RESET0     0
> > +#define IMX8MQ_RESET_A53_CORE_POR_RESET1     1
> > +#define IMX8MQ_RESET_A53_CORE_POR_RESET2     2
> > +#define IMX8MQ_RESET_A53_CORE_POR_RESET3     3
> > +#define IMX8MQ_RESET_A53_CORE_RESET0         4
> > +#define IMX8MQ_RESET_A53_CORE_RESET1         5
> > +#define IMX8MQ_RESET_A53_CORE_RESET2         6
> > +#define IMX8MQ_RESET_A53_CORE_RESET3         7
> > +#define IMX8MQ_RESET_A53_DBG_RESET0          8
> > +#define IMX8MQ_RESET_A53_DBG_RESET1          9
> > +#define IMX8MQ_RESET_A53_DBG_RESET2          10
> > +#define IMX8MQ_RESET_A53_DBG_RESET3          11
> > +#define IMX8MQ_RESET_A53_ETM_RESET0          12
> > +#define IMX8MQ_RESET_A53_ETM_RESET1          13
> > +#define IMX8MQ_RESET_A53_ETM_RESET2          14
> > +#define IMX8MQ_RESET_A53_ETM_RESET3          15
> > +#define IMX8MQ_RESET_A53_SOC_DBG_RESET               16
> > +#define IMX8MQ_RESET_A53_L2RESET             17
> > +#define IMX8MQ_RESET_SW_NON_SCLR_M4C_RST     18
>                            ^^^^^^^^
>
> This might be an implementation detail. The reset line is
> (SW_?)M4C_RST. I suppose the self-clearing SW_M4C_RST bit could be used
> to implement .reset() functionality for the same line if needed.
>

I was using literal bit names for reset lines. In this case this is a
bit 0 of SRC_M4RCR.

> What about the self-clearing SW_M4P_RST bit? Has that been left out on
> purpose?

Since they are self-clearing they'd require a separate .reset
function. I have no use-case for them at this moment and no way to
test it, so I left them out.

>
> > +#define IMX8MQ_RESET_OTG1_PHY_RESET          19
> > +#define IMX8MQ_RESET_OTG2_PHY_RESET          20
> > +#define IMX8MQ_RESET_MIPI_DSI_RESET_BYTE_N   21
> > +#define IMX8MQ_RESET_MIPI_DSI_RESET_N                22
> > +#define IMX8MQ_RESET_MIPI_DIS_DPI_RESET_N    23
> > +#define IMX8MQ_RESET_MIPI_DIS_ESC_RESET_N    24
> > +#define IMX8MQ_RESET_MIPI_DIS_PCLK_RESET_N   25
> > +#define IMX8MQ_RESET_PCIEPHY                 26
> > +#define IMX8MQ_RESET_PCIEPHY_PERST           27
> > +#define IMX8MQ_RESET_PCIE_CTRL_APPS_EN               28
> > +#define IMX8MQ_RESET_PCIE_CTRL_APPS_TURNOFF  29
>
> To be honest, I don't like these two, I'm not convinced anymore that
> they actually qualify as reset signals. To me it looks like this is
> something that the PCIe glue code should handle via syscon like i.MX6.
> Leonard, Lucas, what do you think?

OK, one thing to keep in mind about this is that those bits are
already exposed for i.MX7D and I think (correct me if I am wrong)
there's no going back there. PCIe driver already has the code to use
those on i.MX7D and, due to high degree of similarity, i.MX8MQ
actually re-uses the same codepath (at least for
IMX8MQ_RESET_PCIE_CTRL_APPS_EN).

Thanks,
Andrey Smirnov

Re: [PATCH v4 2/3] dt-bindings: reset: imx7: Document usage on i.MX8MQ SoCs

[Philipp Zabel]

On Thu, 2019-01-17 at 14:38 -0800, Andrey Smirnov wrote:
[...]
> > To be honest, I don't like these two, I'm not convinced anymore that
> > they actually qualify as reset signals. To me it looks like this is
> > something that the PCIe glue code should handle via syscon like i.MX6.
> > Leonard, Lucas, what do you think?
> 
> OK, one thing to keep in mind about this is that those bits are
> already exposed for i.MX7D and I think (correct me if I am wrong)
> there's no going back there.

That's not a reason to repeat the same mistake for i.MX8QM, but at the
moment I'm still trying to figure out if it actually was a mistake.

> PCIe driver already has the code to use
> those on i.MX7D and, due to high degree of similarity, i.MX8MQ
> actually re-uses the same codepath (at least for
> IMX8MQ_RESET_PCIE_CTRL_APPS_EN).

We can always switch to i.MX6-like direct syscon/GPR manipulation and
just drop the resets from DT.
Since if this is done, it should be done for i.MX7 as well, I see no
reason for this issue to hold up your i.MX8M changes.

regards
Philipp

Re: [PATCH v4 2/3] dt-bindings: reset: imx7: Document usage on i.MX8MQ SoCs

[Andrey Smirnov]

On Wed, Jan 23, 2019 at 2:52 AM Philipp Zabel <p.zabel@pengutronix.de> wrote:
>
> On Thu, 2019-01-17 at 14:38 -0800, Andrey Smirnov wrote:
> [...]
> > > To be honest, I don't like these two, I'm not convinced anymore that
> > > they actually qualify as reset signals. To me it looks like this is
> > > something that the PCIe glue code should handle via syscon like i.MX6.
> > > Leonard, Lucas, what do you think?
> >
> > OK, one thing to keep in mind about this is that those bits are
> > already exposed for i.MX7D and I think (correct me if I am wrong)
> > there's no going back there.
>
> That's not a reason to repeat the same mistake for i.MX8QM, but at the
> moment I'm still trying to figure out if it actually was a mistake.
>

It absolutely is. Removing those resets will not meaningfully simplify
maintenance burden for this driver (a one line change), but it will
cause additional code churn on PCI side of things. You may not agree
with me that it is a _good_ reason to not to remove those resets, but
it is a reason nonetheless.

> > PCIe driver already has the code to use
> > those on i.MX7D and, due to high degree of similarity, i.MX8MQ
> > actually re-uses the same codepath (at least for
> > IMX8MQ_RESET_PCIE_CTRL_APPS_EN).
>
> We can always switch to i.MX6-like direct syscon/GPR manipulation and
> just drop the resets from DT.
> Since if this is done, it should be done for i.MX7 as well, I see no
> reason for this issue to hold up your i.MX8M changes.
>

Cool, thanks!

Thanks,
Andrey Smirnov

Re: [PATCH v4 2/3] dt-bindings: reset: imx7: Document usage on i.MX8MQ SoCs

[Philipp Zabel]

Hi Andrey,

On Wed, 2019-01-23 at 21:33 -0800, Andrey Smirnov wrote:
> On Wed, Jan 23, 2019 at 2:52 AM Philipp Zabel <p.zabel@pengutronix.de> wrote:
> > 
> > On Thu, 2019-01-17 at 14:38 -0800, Andrey Smirnov wrote:
> > [...]
> > > > To be honest, I don't like these two, I'm not convinced anymore that
> > > > they actually qualify as reset signals. To me it looks like this is
> > > > something that the PCIe glue code should handle via syscon like i.MX6.
> > > > Leonard, Lucas, what do you think?
> > > 
> > > OK, one thing to keep in mind about this is that those bits are
> > > already exposed for i.MX7D and I think (correct me if I am wrong)
> > > there's no going back there.
> > 
> > That's not a reason to repeat the same mistake for i.MX8QM, but at the
> > moment I'm still trying to figure out if it actually was a mistake.
> > 
> 
> It absolutely is. 

Ok, that was a sloppy expression. I'm glad you got my meaning anyway.
Of course it is a reason, but it's not a good one.

> Removing those resets will not meaningfully simplify
> maintenance burden for this driver (a one line change),

I'm less worried about the maintenance burden on this reset driver and
more about lying in the device tree description and possibly setting a
bad precedent.

> but it will cause additional code churn on PCI side of things.
> You may not agree with me that it is a _good_ reason to not to remove
> those resets, but it is a reason nonetheless.

Exactly. Code churn in one driver implementation should not stop us from
fixing device tree bindings (in a backwards compatible fashion).
Mind you, this is all under my assumption that the bits in question do
not control resets and should never have been described as resets in the
device tree.

regards
Philipp