Linux-ARM-Kernel Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH 0/8] soc: ti: Add OMAP PRM driver
@ 2019-08-07  7:48 Tero Kristo
  2019-08-07  7:48 ` [PATCH 1/8] dt-bindings: omap: add new binding for PRM instances Tero Kristo
                   ` (8 more replies)
  0 siblings, 9 replies; 41+ messages in thread
From: Tero Kristo @ 2019-08-07  7:48 UTC (permalink / raw)
  To: ssantosh, linux-arm-kernel, linux-omap, robh+dt; +Cc: tony, devicetree

Hi,

This series adds OMAP PRM driver which initially supports only reset
handling. Later on, power domain support can be added to this to get
rid of the current OMAP power domain handling code which resides
under the mach-omap2 platform directory. Initially, reset data is
added for AM3, OMAP4 and DRA7 SoCs.

I've been testing the reset handling logic with OMAP remoteproc
driver which has been converted to use generic reset framework. This
part is a work in progress, so will be posting patches from that part
later on.

-Tero

--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 41+ messages in thread

* [PATCH 1/8] dt-bindings: omap: add new binding for PRM instances
  2019-08-07  7:48 [PATCH 0/8] soc: ti: Add OMAP PRM driver Tero Kristo
@ 2019-08-07  7:48 ` Tero Kristo
  2019-08-08  4:35   ` Keerthy
  2019-08-07  7:48 ` [PATCH 2/8] soc: ti: add initial PRM driver with reset control support Tero Kristo
                   ` (7 subsequent siblings)
  8 siblings, 1 reply; 41+ messages in thread
From: Tero Kristo @ 2019-08-07  7:48 UTC (permalink / raw)
  To: ssantosh, linux-arm-kernel, linux-omap, robh+dt; +Cc: tony, devicetree

Add new binding for OMAP PRM (Power and Reset Manager) instances. Each
of these will act as a power domain controller and potentially as a reset
provider.

Signed-off-by: Tero Kristo <t-kristo@ti.com>
---
 .../devicetree/bindings/arm/omap/prm-inst.txt      | 24 ++++++++++++++++++++++
 1 file changed, 24 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/arm/omap/prm-inst.txt

diff --git a/Documentation/devicetree/bindings/arm/omap/prm-inst.txt b/Documentation/devicetree/bindings/arm/omap/prm-inst.txt
new file mode 100644
index 0000000..e0ae87b
--- /dev/null
+++ b/Documentation/devicetree/bindings/arm/omap/prm-inst.txt
@@ -0,0 +1,24 @@
+OMAP PRM instance bindings
+
+Power and Reset Manager is an IP block on OMAP family of devices which
+handle the power domains and their current state, and provide reset
+handling for the domains and/or separate IP blocks under the power domain
+hierarchy.
+
+Required properties:
+- compatible:	Must be one of:
+		"ti,am3-prm-inst"
+		"ti,am4-prm-inst"
+		"ti,omap4-prm-inst"
+		"ti,omap5-prm-inst"
+		"ti,dra7-prm-inst"
+- reg:		Contains PRM instance register address range
+		(base address and length)
+
+Example:
+
+prm_dsp2: prm@1b00 {
+	compatible = "ti,dra7-prm-inst";
+	reg = <0x1b00 0x40>;
+	#reset-cells = <1>;
+};
-- 
1.9.1

--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 41+ messages in thread

* [PATCH 2/8] soc: ti: add initial PRM driver with reset control support
  2019-08-07  7:48 [PATCH 0/8] soc: ti: Add OMAP PRM driver Tero Kristo
  2019-08-07  7:48 ` [PATCH 1/8] dt-bindings: omap: add new binding for PRM instances Tero Kristo
@ 2019-08-07  7:48 ` Tero Kristo
  2019-08-08  5:26   ` Keerthy
  2019-08-07  7:48 ` [PATCH 3/8] soc: ti: omap-prm: poll for reset complete during de-assert Tero Kristo
                   ` (6 subsequent siblings)
  8 siblings, 1 reply; 41+ messages in thread
From: Tero Kristo @ 2019-08-07  7:48 UTC (permalink / raw)
  To: ssantosh, linux-arm-kernel, linux-omap, robh+dt; +Cc: tony, devicetree

Add initial PRM (Power and Reset Management) driver for TI OMAP class
SoCs. Initially this driver only supports reset control, but can be
extended to support rest of the functionality, like powerdomain
control, PRCM irq support etc.

Signed-off-by: Tero Kristo <t-kristo@ti.com>
---
 arch/arm/mach-omap2/Kconfig |   1 +
 drivers/soc/ti/Makefile     |   1 +
 drivers/soc/ti/omap_prm.c   | 216 ++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 218 insertions(+)
 create mode 100644 drivers/soc/ti/omap_prm.c

diff --git a/arch/arm/mach-omap2/Kconfig b/arch/arm/mach-omap2/Kconfig
index fdb6743..42ad063 100644
--- a/arch/arm/mach-omap2/Kconfig
+++ b/arch/arm/mach-omap2/Kconfig
@@ -109,6 +109,7 @@ config ARCH_OMAP2PLUS
 	select TI_SYSC
 	select OMAP_IRQCHIP
 	select CLKSRC_TI_32K
+	select RESET_CONTROLLER
 	help
 	  Systems based on OMAP2, OMAP3, OMAP4 or OMAP5
 
diff --git a/drivers/soc/ti/Makefile b/drivers/soc/ti/Makefile
index b3868d3..788b5cd 100644
--- a/drivers/soc/ti/Makefile
+++ b/drivers/soc/ti/Makefile
@@ -6,6 +6,7 @@ obj-$(CONFIG_KEYSTONE_NAVIGATOR_QMSS)	+= knav_qmss.o
 knav_qmss-y := knav_qmss_queue.o knav_qmss_acc.o
 obj-$(CONFIG_KEYSTONE_NAVIGATOR_DMA)	+= knav_dma.o
 obj-$(CONFIG_AMX3_PM)			+= pm33xx.o
+obj-$(CONFIG_ARCH_OMAP2PLUS)		+= omap_prm.o
 obj-$(CONFIG_WKUP_M3_IPC)		+= wkup_m3_ipc.o
 obj-$(CONFIG_TI_SCI_PM_DOMAINS)		+= ti_sci_pm_domains.o
 obj-$(CONFIG_TI_SCI_INTA_MSI_DOMAIN)	+= ti_sci_inta_msi.o
diff --git a/drivers/soc/ti/omap_prm.c b/drivers/soc/ti/omap_prm.c
new file mode 100644
index 0000000..7c89eb8
--- /dev/null
+++ b/drivers/soc/ti/omap_prm.c
@@ -0,0 +1,216 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * OMAP2+ PRM driver
+ *
+ * Copyright (C) 2019 Texas Instruments Incorporated - http://www.ti.com/
+ *	Tero Kristo <t-kristo@ti.com>
+ */
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/device.h>
+#include <linux/io.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/platform_device.h>
+#include <linux/reset-controller.h>
+#include <linux/delay.h>
+
+struct omap_rst_map {
+	s8 rst;
+	s8 st;
+};
+
+struct omap_prm_data {
+	u32 base;
+	const char *name;
+	u16 pwstctrl;
+	u16 pwstst;
+	u16 rstctl;
+	u16 rstst;
+	struct omap_rst_map *rstmap;
+	u8 flags;
+};
+
+struct omap_prm {
+	const struct omap_prm_data *data;
+	void __iomem *base;
+};
+
+struct omap_reset_data {
+	struct reset_controller_dev rcdev;
+	struct omap_prm *prm;
+};
+
+#define to_omap_reset_data(p) container_of((p), struct omap_reset_data, rcdev)
+
+#define OMAP_MAX_RESETS		8
+#define OMAP_RESET_MAX_WAIT	10000
+
+#define OMAP_PRM_NO_RSTST	BIT(0)
+
+static const struct of_device_id omap_prm_id_table[] = {
+	{ },
+};
+
+static int omap_reset_status(struct reset_controller_dev *rcdev,
+			     unsigned long id)
+{
+	struct omap_reset_data *reset = to_omap_reset_data(rcdev);
+	u32 v;
+
+	v = readl_relaxed(reset->prm->base + reset->prm->data->rstst);
+	v &= 1 << id;
+	v >>= id;
+
+	return v;
+}
+
+static int omap_reset_assert(struct reset_controller_dev *rcdev,
+			     unsigned long id)
+{
+	struct omap_reset_data *reset = to_omap_reset_data(rcdev);
+	u32 v;
+
+	/* assert the reset control line */
+	v = readl_relaxed(reset->prm->base + reset->prm->data->rstctl);
+	v |= 1 << id;
+	writel_relaxed(v, reset->prm->base + reset->prm->data->rstctl);
+
+	return 0;
+}
+
+static int omap_reset_get_st_bit(struct omap_reset_data *reset,
+				 unsigned long id)
+{
+	struct omap_rst_map *map = reset->prm->data->rstmap;
+
+	while (map && map->rst >= 0) {
+		if (map->rst == id)
+			return map->st;
+
+		map++;
+	}
+
+	return id;
+}
+
+/*
+ * Note that status will not change until clocks are on, and clocks cannot be
+ * enabled until reset is deasserted. Consumer drivers must check status
+ * separately after enabling clocks.
+ */
+static int omap_reset_deassert(struct reset_controller_dev *rcdev,
+			       unsigned long id)
+{
+	struct omap_reset_data *reset = to_omap_reset_data(rcdev);
+	u32 v;
+	int st_bit = id;
+	bool has_rstst;
+
+	/* check the current status to avoid de-asserting the line twice */
+	v = readl_relaxed(reset->prm->base + reset->prm->data->rstctl);
+	if (!(v & BIT(id)))
+		return -EEXIST;
+
+	has_rstst = !(reset->prm->data->flags & OMAP_PRM_NO_RSTST);
+
+	if (has_rstst) {
+		st_bit = omap_reset_get_st_bit(reset, id);
+
+		/* Clear the reset status by writing 1 to the status bit */
+		v = readl_relaxed(reset->prm->base + reset->prm->data->rstst);
+		v |= 1 << st_bit;
+		writel_relaxed(v, reset->prm->base + reset->prm->data->rstst);
+	}
+
+	/* de-assert the reset control line */
+	v = readl_relaxed(reset->prm->base + reset->prm->data->rstctl);
+	v &= ~(1 << id);
+	writel_relaxed(v, reset->prm->base + reset->prm->data->rstctl);
+
+	return 0;
+}
+
+static const struct reset_control_ops omap_reset_ops = {
+	.assert		= omap_reset_assert,
+	.deassert	= omap_reset_deassert,
+	.status		= omap_reset_status,
+};
+
+static int omap_prm_reset_probe(struct platform_device *pdev,
+				struct omap_prm *prm)
+{
+	struct omap_reset_data *reset;
+
+	/*
+	 * Check if we have resets. If either rstctl or rstst is
+	 * non-zero, we have reset registers in place. Additionally
+	 * the flag OMAP_PRM_NO_RSTST implies that we have resets.
+	 */
+	if (!prm->data->rstctl && !prm->data->rstst &&
+	    !(prm->data->flags & OMAP_PRM_NO_RSTST))
+		return 0;
+
+	reset = devm_kzalloc(&pdev->dev, sizeof(*reset), GFP_KERNEL);
+	if (!reset)
+		return -ENOMEM;
+
+	reset->rcdev.owner = THIS_MODULE;
+	reset->rcdev.ops = &omap_reset_ops;
+	reset->rcdev.of_node = pdev->dev.of_node;
+	reset->rcdev.nr_resets = OMAP_MAX_RESETS;
+
+	reset->prm = prm;
+
+	return devm_reset_controller_register(&pdev->dev, &reset->rcdev);
+}
+
+static int omap_prm_probe(struct platform_device *pdev)
+{
+	struct resource *res;
+	const struct omap_prm_data *data;
+	struct omap_prm *prm;
+	const struct of_device_id *match;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (!res)
+		return -ENODEV;
+
+	match = of_match_device(omap_prm_id_table, &pdev->dev);
+	if (!match)
+		return -ENOTSUPP;
+
+	prm = devm_kzalloc(&pdev->dev, sizeof(*prm), GFP_KERNEL);
+	if (!prm)
+		return -ENOMEM;
+
+	data = match->data;
+
+	while (data->base != res->start) {
+		if (!data->base)
+			return -EINVAL;
+		data++;
+	}
+
+	prm->data = data;
+
+	prm->base = devm_ioremap_resource(&pdev->dev, res);
+	if (!prm->base)
+		return -ENOMEM;
+
+	return omap_prm_reset_probe(pdev, prm);
+}
+
+static struct platform_driver omap_prm_driver = {
+	.probe = omap_prm_probe,
+	.driver = {
+		.name		= KBUILD_MODNAME,
+		.of_match_table	= omap_prm_id_table,
+	},
+};
+builtin_platform_driver(omap_prm_driver);
+
+MODULE_ALIAS("platform:prm");
+MODULE_LICENSE("GPL v2");
+MODULE_DESCRIPTION("omap2+ prm driver");
-- 
1.9.1

--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 41+ messages in thread

* [PATCH 3/8] soc: ti: omap-prm: poll for reset complete during de-assert
  2019-08-07  7:48 [PATCH 0/8] soc: ti: Add OMAP PRM driver Tero Kristo
  2019-08-07  7:48 ` [PATCH 1/8] dt-bindings: omap: add new binding for PRM instances Tero Kristo
  2019-08-07  7:48 ` [PATCH 2/8] soc: ti: add initial PRM driver with reset control support Tero Kristo
@ 2019-08-07  7:48 ` Tero Kristo
  2019-08-07  7:48 ` [PATCH 4/8] soc: ti: omap-prm: add support for denying idle for reset clockdomain Tero Kristo
                   ` (5 subsequent siblings)
  8 siblings, 0 replies; 41+ messages in thread
From: Tero Kristo @ 2019-08-07  7:48 UTC (permalink / raw)
  To: ssantosh, linux-arm-kernel, linux-omap, robh+dt; +Cc: tony, devicetree

Poll for reset completion status during de-assertion of reset, otherwise
the IP in question might be accessed before it has left reset properly.

Signed-off-by: Tero Kristo <t-kristo@ti.com>
---
 drivers/soc/ti/omap_prm.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/drivers/soc/ti/omap_prm.c b/drivers/soc/ti/omap_prm.c
index 7c89eb8..d412af3 100644
--- a/drivers/soc/ti/omap_prm.c
+++ b/drivers/soc/ti/omap_prm.c
@@ -107,6 +107,7 @@ static int omap_reset_deassert(struct reset_controller_dev *rcdev,
 	u32 v;
 	int st_bit = id;
 	bool has_rstst;
+	int timeout = 0;
 
 	/* check the current status to avoid de-asserting the line twice */
 	v = readl_relaxed(reset->prm->base + reset->prm->data->rstctl);
@@ -129,6 +130,22 @@ static int omap_reset_deassert(struct reset_controller_dev *rcdev,
 	v &= ~(1 << id);
 	writel_relaxed(v, reset->prm->base + reset->prm->data->rstctl);
 
+	if (!has_rstst)
+		return 0;
+
+	/* wait for the status to be set */
+	while (1) {
+		v = readl_relaxed(reset->prm->base + reset->prm->data->rstst);
+		v &= 1 << st_bit;
+		if (v)
+			break;
+		timeout++;
+		if (timeout > OMAP_RESET_MAX_WAIT)
+			return -EBUSY;
+
+		udelay(1);
+	}
+
 	return 0;
 }
 
-- 
1.9.1

--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 41+ messages in thread

* [PATCH 4/8] soc: ti: omap-prm: add support for denying idle for reset clockdomain
  2019-08-07  7:48 [PATCH 0/8] soc: ti: Add OMAP PRM driver Tero Kristo
                   ` (2 preceding siblings ...)
  2019-08-07  7:48 ` [PATCH 3/8] soc: ti: omap-prm: poll for reset complete during de-assert Tero Kristo
@ 2019-08-07  7:48 ` Tero Kristo
  2019-08-19 23:16   ` Suman Anna
  2019-08-07  7:48 ` [PATCH 5/8] soc: ti: omap-prm: add omap4 PRM data Tero Kristo
                   ` (4 subsequent siblings)
  8 siblings, 1 reply; 41+ messages in thread
From: Tero Kristo @ 2019-08-07  7:48 UTC (permalink / raw)
  To: ssantosh, linux-arm-kernel, linux-omap, robh+dt; +Cc: tony, devicetree

TI SoCs hardware reset signals require the parent clockdomain to be
in force wakeup mode while de-asserting the reset, otherwise it may
never complete. To support this, add pdata hooks to control the
clockdomain directly.

Signed-off-by: Tero Kristo <t-kristo@ti.com>
---
 drivers/soc/ti/omap_prm.c            | 32 ++++++++++++++++++++++++++++----
 include/linux/platform_data/ti-prm.h | 21 +++++++++++++++++++++
 2 files changed, 49 insertions(+), 4 deletions(-)
 create mode 100644 include/linux/platform_data/ti-prm.h

diff --git a/drivers/soc/ti/omap_prm.c b/drivers/soc/ti/omap_prm.c
index d412af3..870515e3 100644
--- a/drivers/soc/ti/omap_prm.c
+++ b/drivers/soc/ti/omap_prm.c
@@ -16,6 +16,8 @@
 #include <linux/reset-controller.h>
 #include <linux/delay.h>
 
+#include <linux/platform_data/ti-prm.h>
+
 struct omap_rst_map {
 	s8 rst;
 	s8 st;
@@ -24,6 +26,7 @@ struct omap_rst_map {
 struct omap_prm_data {
 	u32 base;
 	const char *name;
+	const char *clkdm_name;
 	u16 pwstctrl;
 	u16 pwstst;
 	u16 rstctl;
@@ -40,6 +43,8 @@ struct omap_prm {
 struct omap_reset_data {
 	struct reset_controller_dev rcdev;
 	struct omap_prm *prm;
+	struct clockdomain *clkdm;
+	struct device *dev;
 };
 
 #define to_omap_reset_data(p) container_of((p), struct omap_reset_data, rcdev)
@@ -108,6 +113,8 @@ static int omap_reset_deassert(struct reset_controller_dev *rcdev,
 	int st_bit = id;
 	bool has_rstst;
 	int timeout = 0;
+	struct ti_prm_platform_data *pdata = dev_get_platdata(reset->dev);
+	int ret = 0;
 
 	/* check the current status to avoid de-asserting the line twice */
 	v = readl_relaxed(reset->prm->base + reset->prm->data->rstctl);
@@ -125,13 +132,16 @@ static int omap_reset_deassert(struct reset_controller_dev *rcdev,
 		writel_relaxed(v, reset->prm->base + reset->prm->data->rstst);
 	}
 
+	if (pdata->clkdm_deny_idle && reset->clkdm)
+		pdata->clkdm_deny_idle(reset->clkdm);
+
 	/* de-assert the reset control line */
 	v = readl_relaxed(reset->prm->base + reset->prm->data->rstctl);
 	v &= ~(1 << id);
 	writel_relaxed(v, reset->prm->base + reset->prm->data->rstctl);
 
 	if (!has_rstst)
-		return 0;
+		goto exit;
 
 	/* wait for the status to be set */
 	while (1) {
@@ -140,13 +150,19 @@ static int omap_reset_deassert(struct reset_controller_dev *rcdev,
 		if (v)
 			break;
 		timeout++;
-		if (timeout > OMAP_RESET_MAX_WAIT)
-			return -EBUSY;
+		if (timeout > OMAP_RESET_MAX_WAIT) {
+			ret = -EBUSY;
+			goto exit;
+		}
 
 		udelay(1);
 	}
 
-	return 0;
+exit:
+	if (pdata->clkdm_allow_idle && reset->clkdm)
+		pdata->clkdm_allow_idle(reset->clkdm);
+
+	return ret;
 }
 
 static const struct reset_control_ops omap_reset_ops = {
@@ -159,6 +175,8 @@ static int omap_prm_reset_probe(struct platform_device *pdev,
 				struct omap_prm *prm)
 {
 	struct omap_reset_data *reset;
+	struct ti_prm_platform_data *pdata = dev_get_platdata(&pdev->dev);
+	char buf[32];
 
 	/*
 	 * Check if we have resets. If either rstctl or rstst is
@@ -177,9 +195,15 @@ static int omap_prm_reset_probe(struct platform_device *pdev,
 	reset->rcdev.ops = &omap_reset_ops;
 	reset->rcdev.of_node = pdev->dev.of_node;
 	reset->rcdev.nr_resets = OMAP_MAX_RESETS;
+	reset->dev = &pdev->dev;
 
 	reset->prm = prm;
 
+	sprintf(buf, "%s_clkdm", prm->data->clkdm_name ? prm->data->clkdm_name :
+		prm->data->name);
+
+	reset->clkdm = pdata->clkdm_lookup(buf);
+
 	return devm_reset_controller_register(&pdev->dev, &reset->rcdev);
 }
 
diff --git a/include/linux/platform_data/ti-prm.h b/include/linux/platform_data/ti-prm.h
new file mode 100644
index 0000000..28154c3
--- /dev/null
+++ b/include/linux/platform_data/ti-prm.h
@@ -0,0 +1,21 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * TI PRM (Power & Reset Manager) platform data
+ *
+ * Copyright (C) 2019 Texas Instruments, Inc.
+ *
+ * Tero Kristo <t-kristo@ti.com>
+ */
+
+#ifndef _LINUX_PLATFORM_DATA_TI_PRM_H
+#define _LINUX_PLATFORM_DATA_TI_PRM_H
+
+struct clockdomain;
+
+struct ti_prm_platform_data {
+	void (*clkdm_deny_idle)(struct clockdomain *clkdm);
+	void (*clkdm_allow_idle)(struct clockdomain *clkdm);
+	struct clockdomain * (*clkdm_lookup)(const char *name);
+};
+
+#endif /* _LINUX_PLATFORM_DATA_TI_PRM_H */
-- 
1.9.1

--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 41+ messages in thread

* [PATCH 5/8] soc: ti: omap-prm: add omap4 PRM data
  2019-08-07  7:48 [PATCH 0/8] soc: ti: Add OMAP PRM driver Tero Kristo
                   ` (3 preceding siblings ...)
  2019-08-07  7:48 ` [PATCH 4/8] soc: ti: omap-prm: add support for denying idle for reset clockdomain Tero Kristo
@ 2019-08-07  7:48 ` Tero Kristo
  2019-08-08  5:30   ` Keerthy
  2019-08-19 23:08   ` Suman Anna
  2019-08-07  7:48 ` [PATCH 6/8] soc: ti: omap_prm: add data for am33xx Tero Kristo
                   ` (3 subsequent siblings)
  8 siblings, 2 replies; 41+ messages in thread
From: Tero Kristo @ 2019-08-07  7:48 UTC (permalink / raw)
  To: ssantosh, linux-arm-kernel, linux-omap, robh+dt; +Cc: tony, devicetree

Add PRM data for omap4 family of SoCs.

Signed-off-by: Tero Kristo <t-kristo@ti.com>
---
 drivers/soc/ti/omap_prm.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/drivers/soc/ti/omap_prm.c b/drivers/soc/ti/omap_prm.c
index 870515e3..9b8d5945 100644
--- a/drivers/soc/ti/omap_prm.c
+++ b/drivers/soc/ti/omap_prm.c
@@ -54,7 +54,27 @@ struct omap_reset_data {
 
 #define OMAP_PRM_NO_RSTST	BIT(0)
 
+struct omap_prm_data omap4_prm_data[] = {
+	{ .name = "mpu", .base = 0x4a306300, .pwstst = 0x4 },
+	{ .name = "tesla", .base = 0x4a306400, .pwstst = 0x4, .rstctl = 0x10, .rstst = 0x14 },
+	{ .name = "abe", .base = 0x4a306500, .pwstst = 0x4 },
+	{ .name = "always_on_core", .base = 0x4a306600, .pwstst = 0x4 },
+	{ .name = "core", .base = 0x4a306700, .pwstst = 0x4, .rstctl = 0x210, .rstst = 0x214 },
+	{ .name = "ivahd", .base = 0x4a306f00, .pwstst = 0x4, .rstctl = 0x10, .rstst = 0x14 },
+	{ .name = "cam", .base = 0x4a307000, .pwstst = 0x4 },
+	{ .name = "dss", .base = 0x4a307100, .pwstst = 0x4 },
+	{ .name = "gfx", .base = 0x4a307200, .pwstst = 0x4 },
+	{ .name = "l3init", .base = 0x4a307300, .pwstst = 0x4 },
+	{ .name = "l4per", .base = 0x4a307400, .pwstst = 0x4 },
+	{ .name = "cefuse", .base = 0x4a307600, .pwstst = 0x4 },
+	{ .name = "wkup", .base = 0x4a307700, .pwstst = 0x4 },
+	{ .name = "emu", .base = 0x4a307900, .pwstst = 0x4 },
+	{ .name = "device", .base = 0x4a307b00, .rstctl = 0x0, .rstst = 0x4 },
+	{ },
+};
+
 static const struct of_device_id omap_prm_id_table[] = {
+	{ .compatible = "ti,omap4-prm-inst", .data = omap4_prm_data },
 	{ },
 };
 
-- 
1.9.1

--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 41+ messages in thread

* [PATCH 6/8] soc: ti: omap_prm: add data for am33xx
  2019-08-07  7:48 [PATCH 0/8] soc: ti: Add OMAP PRM driver Tero Kristo
                   ` (4 preceding siblings ...)
  2019-08-07  7:48 ` [PATCH 5/8] soc: ti: omap-prm: add omap4 PRM data Tero Kristo
@ 2019-08-07  7:48 ` Tero Kristo
  2019-08-19 23:11   ` Suman Anna
  2019-08-20 18:48   ` Suman Anna
  2019-08-07  7:48 ` [PATCH 7/8] soc: ti: omap-prm: add dra7 PRM data Tero Kristo
                   ` (2 subsequent siblings)
  8 siblings, 2 replies; 41+ messages in thread
From: Tero Kristo @ 2019-08-07  7:48 UTC (permalink / raw)
  To: ssantosh, linux-arm-kernel, linux-omap, robh+dt; +Cc: tony, devicetree

Add PRM instance data for AM33xx SoC. Includes some basic register
definitions and reset data for now.

Signed-off-by: Tero Kristo <t-kristo@ti.com>
---
 drivers/soc/ti/omap_prm.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/drivers/soc/ti/omap_prm.c b/drivers/soc/ti/omap_prm.c
index 9b8d5945..fadfc7f 100644
--- a/drivers/soc/ti/omap_prm.c
+++ b/drivers/soc/ti/omap_prm.c
@@ -73,8 +73,25 @@ struct omap_prm_data omap4_prm_data[] = {
 	{ },
 };
 
+struct omap_rst_map am3_wkup_rst_map[] = {
+	{ .rst = 3, .st = 5 },
+	{ .rst = -1 },
+};
+
+struct omap_prm_data am3_prm_data[] = {
+	{ .name = "per", .base = 0x44e00c00, .pwstctrl = 0xc, .pwstst = 0x8, .flags = OMAP_PRM_NO_RSTST },
+	{ .name = "wkup", .base = 0x44e00d00, .pwstctrl = 0x4, .pwstst = 0x8, .rstst = 0xc, .rstmap = am3_wkup_rst_map },
+	{ .name = "mpu", .base = 0x44e00e00, .pwstst = 0x4 },
+	{ .name = "device", .base = 0x44e00f00, .rstctl = 0x0, .rstst = 0x8 },
+	{ .name = "rtc", .base = 0x44e01000, .pwstst = 0x4 },
+	{ .name = "gfx", .base = 0x44e01100, .pwstst = 0x10, .rstctl = 0x4, .rstst = 0x14 },
+	{ .name = "cefuse", .base = 0x44e01200, .pwstst = 0x4 },
+	{ },
+};
+
 static const struct of_device_id omap_prm_id_table[] = {
 	{ .compatible = "ti,omap4-prm-inst", .data = omap4_prm_data },
+	{ .compatible = "ti,am3-prm-inst", .data = am3_prm_data },
 	{ },
 };
 
-- 
1.9.1

--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 41+ messages in thread

* [PATCH 7/8] soc: ti: omap-prm: add dra7 PRM data
  2019-08-07  7:48 [PATCH 0/8] soc: ti: Add OMAP PRM driver Tero Kristo
                   ` (5 preceding siblings ...)
  2019-08-07  7:48 ` [PATCH 6/8] soc: ti: omap_prm: add data for am33xx Tero Kristo
@ 2019-08-07  7:48 ` Tero Kristo
  2019-08-19 23:12   ` Suman Anna
  2019-08-20 19:03   ` Suman Anna
  2019-08-07  7:48 ` [PATCH 8/8] ARM: OMAP2+: pdata-quirks: add PRM data for reset support Tero Kristo
  2019-08-19 23:20 ` [PATCH 0/8] soc: ti: Add OMAP PRM driver Suman Anna
  8 siblings, 2 replies; 41+ messages in thread
From: Tero Kristo @ 2019-08-07  7:48 UTC (permalink / raw)
  To: ssantosh, linux-arm-kernel, linux-omap, robh+dt; +Cc: tony, devicetree

Add PRM data for dra7 family of SoCs.

Signed-off-by: Tero Kristo <t-kristo@ti.com>
---
 drivers/soc/ti/omap_prm.c | 26 ++++++++++++++++++++++++++
 1 file changed, 26 insertions(+)

diff --git a/drivers/soc/ti/omap_prm.c b/drivers/soc/ti/omap_prm.c
index fadfc7f..05b7749 100644
--- a/drivers/soc/ti/omap_prm.c
+++ b/drivers/soc/ti/omap_prm.c
@@ -73,6 +73,31 @@ struct omap_prm_data omap4_prm_data[] = {
 	{ },
 };
 
+static struct omap_prm_data dra7_prm_data[] = {
+	{ .name = "mpu", .base = 0x4ae06300, .pwstst = 0x4 },
+	{ .name = "dsp1", .base = 0x4ae06400, .pwstst = 0x4, .rstctl = 0x10, .rstst = 0x14 },
+	{ .name = "ipu", .base = 0x4ae06500, .pwstst = 0x4, .rstctl = 0x10, .rstst = 0x14, .clkdm_name = "ipu1" },
+	{ .name = "coreaon", .base = 0x4ae06628, .pwstst = 0x4 },
+	{ .name = "core", .base = 0x4ae06700, .pwstst = 0x4, .rstctl = 0x210, .rstst = 0x214, .clkdm_name = "ipu2" },
+	{ .name = "iva", .base = 0x4ae06f00, .pwstst = 0x4 },
+	{ .name = "cam", .base = 0x4ae07000, .pwstst = 0x4 },
+	{ .name = "dss", .base = 0x4ae07100, .pwstst = 0x4 },
+	{ .name = "gpu", .base = 0x4ae07200, .pwstst = 0x4 },
+	{ .name = "l3init", .base = 0x4ae07300, .pwstst = 0x4, .rstctl = 0x10, .rstst = 0x14 },
+	{ .name = "l4per", .base = 0x4ae07400, .pwstst = 0x4 },
+	{ .name = "custefuse", .base = 0x4ae07600, .pwstst = 0x4 },
+	{ .name = "wkupaon", .base = 0x4ae07724, .pwstst = 0x4 },
+	{ .name = "emu", .base = 0x4ae07900, .pwstst = 0x4 },
+	{ .name = "dsp2", .base = 0x4ae07b00, .pwstst = 0x4, .rstctl = 0x10, .rstst = 0x14 },
+	{ .name = "eve1", .base = 0x4ae07b40, .pwstst = 0x4 },
+	{ .name = "eve2", .base = 0x4ae07b80, .pwstst = 0x4 },
+	{ .name = "eve3", .base = 0x4ae07bc0, .pwstst = 0x4 },
+	{ .name = "eve4", .base = 0x4ae07c00, .pwstst = 0x4 },
+	{ .name = "rtc", .base = 0x4ae07c60, .pwstst = 0x4 },
+	{ .name = "vpe", .base = 0x4ae07c80, .pwstst = 0x4 },
+	{ },
+};
+
 struct omap_rst_map am3_wkup_rst_map[] = {
 	{ .rst = 3, .st = 5 },
 	{ .rst = -1 },
@@ -91,6 +116,7 @@ struct omap_prm_data am3_prm_data[] = {
 
 static const struct of_device_id omap_prm_id_table[] = {
 	{ .compatible = "ti,omap4-prm-inst", .data = omap4_prm_data },
+	{ .compatible = "ti,dra7-prm-inst", .data = dra7_prm_data },
 	{ .compatible = "ti,am3-prm-inst", .data = am3_prm_data },
 	{ },
 };
-- 
1.9.1

--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 41+ messages in thread

* [PATCH 8/8] ARM: OMAP2+: pdata-quirks: add PRM data for reset support
  2019-08-07  7:48 [PATCH 0/8] soc: ti: Add OMAP PRM driver Tero Kristo
                   ` (6 preceding siblings ...)
  2019-08-07  7:48 ` [PATCH 7/8] soc: ti: omap-prm: add dra7 PRM data Tero Kristo
@ 2019-08-07  7:48 ` Tero Kristo
  2019-08-19 23:20 ` [PATCH 0/8] soc: ti: Add OMAP PRM driver Suman Anna
  8 siblings, 0 replies; 41+ messages in thread
From: Tero Kristo @ 2019-08-07  7:48 UTC (permalink / raw)
  To: ssantosh, linux-arm-kernel, linux-omap, robh+dt; +Cc: tony, devicetree

The parent clockdomain for reset must be in force wakeup mode, otherwise
the reset may never complete. Add pdata quirks for this purpose for PRM
driver.

Signed-off-by: Tero Kristo <t-kristo@ti.com>
---
 arch/arm/mach-omap2/pdata-quirks.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/arch/arm/mach-omap2/pdata-quirks.c b/arch/arm/mach-omap2/pdata-quirks.c
index 6c6f8fc..87e2457 100644
--- a/arch/arm/mach-omap2/pdata-quirks.c
+++ b/arch/arm/mach-omap2/pdata-quirks.c
@@ -25,6 +25,7 @@
 #include <linux/platform_data/ti-sysc.h>
 #include <linux/platform_data/wkup_m3.h>
 #include <linux/platform_data/asoc-ti-mcbsp.h>
+#include <linux/platform_data/ti-prm.h>
 
 #include "clockdomain.h"
 #include "common.h"
@@ -565,6 +566,12 @@ void omap_pcs_legacy_init(int irq, void (*rearm)(void))
 	pcs_pdata.rearm = rearm;
 }
 
+static struct ti_prm_platform_data ti_prm_pdata = {
+	.clkdm_deny_idle = clkdm_deny_idle,
+	.clkdm_allow_idle = clkdm_allow_idle,
+	.clkdm_lookup = clkdm_lookup,
+};
+
 /*
  * GPIOs for TWL are initialized by the I2C bus and need custom
  * handing until DSS has device tree bindings.
@@ -664,6 +671,9 @@ static void __init omap3_mcbsp_init(void) {}
 	/* Common auxdata */
 	OF_DEV_AUXDATA("ti,sysc", 0, NULL, &ti_sysc_pdata),
 	OF_DEV_AUXDATA("pinctrl-single", 0, NULL, &pcs_pdata),
+	OF_DEV_AUXDATA("ti,omap4-prm-inst", 0, NULL, &ti_prm_pdata),
+	OF_DEV_AUXDATA("ti,dra7-prm-inst", 0, NULL, &ti_prm_pdata),
+	OF_DEV_AUXDATA("ti,am3-prm-inst", 0, NULL, &ti_prm_pdata),
 	{ /* sentinel */ },
 };
 
-- 
1.9.1

--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 41+ messages in thread

* Re: [PATCH 1/8] dt-bindings: omap: add new binding for PRM instances
  2019-08-07  7:48 ` [PATCH 1/8] dt-bindings: omap: add new binding for PRM instances Tero Kristo
@ 2019-08-08  4:35   ` Keerthy
  2019-08-19  9:28     ` Tero Kristo
  0 siblings, 1 reply; 41+ messages in thread
From: Keerthy @ 2019-08-08  4:35 UTC (permalink / raw)
  To: Tero Kristo, ssantosh, linux-arm-kernel, linux-omap, robh+dt
  Cc: tony, devicetree



On 07/08/19 1:18 PM, Tero Kristo wrote:
> Add new binding for OMAP PRM (Power and Reset Manager) instances. Each
> of these will act as a power domain controller and potentially as a reset
> provider.
> 
> Signed-off-by: Tero Kristo <t-kristo@ti.com>
> ---
>   .../devicetree/bindings/arm/omap/prm-inst.txt      | 24 ++++++++++++++++++++++
>   1 file changed, 24 insertions(+)
>   create mode 100644 Documentation/devicetree/bindings/arm/omap/prm-inst.txt
> 
> diff --git a/Documentation/devicetree/bindings/arm/omap/prm-inst.txt b/Documentation/devicetree/bindings/arm/omap/prm-inst.txt
> new file mode 100644
> index 0000000..e0ae87b
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/arm/omap/prm-inst.txt
> @@ -0,0 +1,24 @@
> +OMAP PRM instance bindings
> +
> +Power and Reset Manager is an IP block on OMAP family of devices which
> +handle the power domains and their current state, and provide reset
> +handling for the domains and/or separate IP blocks under the power domain
> +hierarchy.
> +
> +Required properties:
> +- compatible:	Must be one of:
> +		"ti,am3-prm-inst"
> +		"ti,am4-prm-inst"
> +		"ti,omap4-prm-inst"
> +		"ti,omap5-prm-inst"
> +		"ti,dra7-prm-inst"
> +- reg:		Contains PRM instance register address range
> +		(base address and length)

How about reset-cells property, Isn't that a mandatory property?

> +
> +Example:
> +
> +prm_dsp2: prm@1b00 {
> +	compatible = "ti,dra7-prm-inst";
> +	reg = <0x1b00 0x40>;
> +	#reset-cells = <1>;
> +};
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 41+ messages in thread

* Re: [PATCH 2/8] soc: ti: add initial PRM driver with reset control support
  2019-08-07  7:48 ` [PATCH 2/8] soc: ti: add initial PRM driver with reset control support Tero Kristo
@ 2019-08-08  5:26   ` Keerthy
  2019-08-19  9:32     ` Tero Kristo
  0 siblings, 1 reply; 41+ messages in thread
From: Keerthy @ 2019-08-08  5:26 UTC (permalink / raw)
  To: Tero Kristo, ssantosh, linux-arm-kernel, linux-omap, robh+dt
  Cc: tony, devicetree



On 07/08/19 1:18 PM, Tero Kristo wrote:
> Add initial PRM (Power and Reset Management) driver for TI OMAP class
> SoCs. Initially this driver only supports reset control, but can be
> extended to support rest of the functionality, like powerdomain
> control, PRCM irq support etc.
> 
> Signed-off-by: Tero Kristo <t-kristo@ti.com>
> ---
>   arch/arm/mach-omap2/Kconfig |   1 +
>   drivers/soc/ti/Makefile     |   1 +
>   drivers/soc/ti/omap_prm.c   | 216 ++++++++++++++++++++++++++++++++++++++++++++
>   3 files changed, 218 insertions(+)
>   create mode 100644 drivers/soc/ti/omap_prm.c
> 
> diff --git a/arch/arm/mach-omap2/Kconfig b/arch/arm/mach-omap2/Kconfig
> index fdb6743..42ad063 100644
> --- a/arch/arm/mach-omap2/Kconfig
> +++ b/arch/arm/mach-omap2/Kconfig
> @@ -109,6 +109,7 @@ config ARCH_OMAP2PLUS
>   	select TI_SYSC
>   	select OMAP_IRQCHIP
>   	select CLKSRC_TI_32K
> +	select RESET_CONTROLLER
>   	help
>   	  Systems based on OMAP2, OMAP3, OMAP4 or OMAP5
>   
> diff --git a/drivers/soc/ti/Makefile b/drivers/soc/ti/Makefile
> index b3868d3..788b5cd 100644
> --- a/drivers/soc/ti/Makefile
> +++ b/drivers/soc/ti/Makefile
> @@ -6,6 +6,7 @@ obj-$(CONFIG_KEYSTONE_NAVIGATOR_QMSS)	+= knav_qmss.o
>   knav_qmss-y := knav_qmss_queue.o knav_qmss_acc.o
>   obj-$(CONFIG_KEYSTONE_NAVIGATOR_DMA)	+= knav_dma.o
>   obj-$(CONFIG_AMX3_PM)			+= pm33xx.o
> +obj-$(CONFIG_ARCH_OMAP2PLUS)		+= omap_prm.o
>   obj-$(CONFIG_WKUP_M3_IPC)		+= wkup_m3_ipc.o
>   obj-$(CONFIG_TI_SCI_PM_DOMAINS)		+= ti_sci_pm_domains.o
>   obj-$(CONFIG_TI_SCI_INTA_MSI_DOMAIN)	+= ti_sci_inta_msi.o
> diff --git a/drivers/soc/ti/omap_prm.c b/drivers/soc/ti/omap_prm.c
> new file mode 100644
> index 0000000..7c89eb8
> --- /dev/null
> +++ b/drivers/soc/ti/omap_prm.c
> @@ -0,0 +1,216 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * OMAP2+ PRM driver
> + *
> + * Copyright (C) 2019 Texas Instruments Incorporated - http://www.ti.com/
> + *	Tero Kristo <t-kristo@ti.com>
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/device.h>
> +#include <linux/io.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
> +#include <linux/platform_device.h>
> +#include <linux/reset-controller.h>
> +#include <linux/delay.h>
> +
> +struct omap_rst_map {
> +	s8 rst;
> +	s8 st;
> +};
> +
> +struct omap_prm_data {
> +	u32 base;
> +	const char *name;
> +	u16 pwstctrl;
> +	u16 pwstst;
> +	u16 rstctl;
> +	u16 rstst;
> +	struct omap_rst_map *rstmap;
> +	u8 flags;
> +};
> +
> +struct omap_prm {
> +	const struct omap_prm_data *data;
> +	void __iomem *base;
> +};
> +
> +struct omap_reset_data {
> +	struct reset_controller_dev rcdev;
> +	struct omap_prm *prm;
> +};
> +
> +#define to_omap_reset_data(p) container_of((p), struct omap_reset_data, rcdev)
> +
> +#define OMAP_MAX_RESETS		8
> +#define OMAP_RESET_MAX_WAIT	10000
> +
> +#define OMAP_PRM_NO_RSTST	BIT(0)
> +
> +static const struct of_device_id omap_prm_id_table[] = {
> +	{ },
> +};

This table is blank and we are doing of_match_device against it.

> +
> +static int omap_reset_status(struct reset_controller_dev *rcdev,
> +			     unsigned long id)
> +{
> +	struct omap_reset_data *reset = to_omap_reset_data(rcdev);
> +	u32 v;
> +
> +	v = readl_relaxed(reset->prm->base + reset->prm->data->rstst);
> +	v &= 1 << id;
> +	v >>= id;
> +
> +	return v;
> +}
> +
> +static int omap_reset_assert(struct reset_controller_dev *rcdev,
> +			     unsigned long id)
> +{
> +	struct omap_reset_data *reset = to_omap_reset_data(rcdev);
> +	u32 v;
> +
> +	/* assert the reset control line */
> +	v = readl_relaxed(reset->prm->base + reset->prm->data->rstctl);
> +	v |= 1 << id;
> +	writel_relaxed(v, reset->prm->base + reset->prm->data->rstctl);
> +
> +	return 0;
> +}
> +
> +static int omap_reset_get_st_bit(struct omap_reset_data *reset,
> +				 unsigned long id)
> +{
> +	struct omap_rst_map *map = reset->prm->data->rstmap;
> +
> +	while (map && map->rst >= 0) {
> +		if (map->rst == id)
> +			return map->st;
> +
> +		map++;
> +	}
> +
> +	return id;
> +}
> +
> +/*
> + * Note that status will not change until clocks are on, and clocks cannot be
> + * enabled until reset is deasserted. Consumer drivers must check status
> + * separately after enabling clocks.
> + */
> +static int omap_reset_deassert(struct reset_controller_dev *rcdev,
> +			       unsigned long id)
> +{
> +	struct omap_reset_data *reset = to_omap_reset_data(rcdev);
> +	u32 v;
> +	int st_bit = id;
> +	bool has_rstst;
> +
> +	/* check the current status to avoid de-asserting the line twice */
> +	v = readl_relaxed(reset->prm->base + reset->prm->data->rstctl);
> +	if (!(v & BIT(id)))
> +		return -EEXIST;
> +
> +	has_rstst = !(reset->prm->data->flags & OMAP_PRM_NO_RSTST);
> +
> +	if (has_rstst) {
> +		st_bit = omap_reset_get_st_bit(reset, id);
> +
> +		/* Clear the reset status by writing 1 to the status bit */
> +		v = readl_relaxed(reset->prm->base + reset->prm->data->rstst);
> +		v |= 1 << st_bit;
> +		writel_relaxed(v, reset->prm->base + reset->prm->data->rstst);
> +	}
> +
> +	/* de-assert the reset control line */
> +	v = readl_relaxed(reset->prm->base + reset->prm->data->rstctl);
> +	v &= ~(1 << id);
> +	writel_relaxed(v, reset->prm->base + reset->prm->data->rstctl);
> +
> +	return 0;
> +}
> +
> +static const struct reset_control_ops omap_reset_ops = {
> +	.assert		= omap_reset_assert,
> +	.deassert	= omap_reset_deassert,
> +	.status		= omap_reset_status,
> +};
> +
> +static int omap_prm_reset_probe(struct platform_device *pdev,
> +				struct omap_prm *prm)
> +{
> +	struct omap_reset_data *reset;
> +
> +	/*
> +	 * Check if we have resets. If either rstctl or rstst is
> +	 * non-zero, we have reset registers in place. Additionally
> +	 * the flag OMAP_PRM_NO_RSTST implies that we have resets.
> +	 */
> +	if (!prm->data->rstctl && !prm->data->rstst &&
> +	    !(prm->data->flags & OMAP_PRM_NO_RSTST))
> +		return 0;
> +
> +	reset = devm_kzalloc(&pdev->dev, sizeof(*reset), GFP_KERNEL);
> +	if (!reset)
> +		return -ENOMEM;
> +
> +	reset->rcdev.owner = THIS_MODULE;
> +	reset->rcdev.ops = &omap_reset_ops;
> +	reset->rcdev.of_node = pdev->dev.of_node;
> +	reset->rcdev.nr_resets = OMAP_MAX_RESETS;
> +
> +	reset->prm = prm;
> +
> +	return devm_reset_controller_register(&pdev->dev, &reset->rcdev);
> +}
> +
> +static int omap_prm_probe(struct platform_device *pdev)
> +{
> +	struct resource *res;
> +	const struct omap_prm_data *data;
> +	struct omap_prm *prm;
> +	const struct of_device_id *match;
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	if (!res)
> +		return -ENODEV;
> +
> +	match = of_match_device(omap_prm_id_table, &pdev->dev);
> +	if (!match)
> +		return -ENOTSUPP;
> +
> +	prm = devm_kzalloc(&pdev->dev, sizeof(*prm), GFP_KERNEL);
> +	if (!prm)
> +		return -ENOMEM;
> +
> +	data = match->data;
> +
> +	while (data->base != res->start) {
> +		if (!data->base)
> +			return -EINVAL;
> +		data++;
> +	}
> +
> +	prm->data = data;
> +
> +	prm->base = devm_ioremap_resource(&pdev->dev, res);
> +	if (!prm->base)
> +		return -ENOMEM;
> +
> +	return omap_prm_reset_probe(pdev, prm);
> +}
> +
> +static struct platform_driver omap_prm_driver = {
> +	.probe = omap_prm_probe,
> +	.driver = {
> +		.name		= KBUILD_MODNAME,
> +		.of_match_table	= omap_prm_id_table,
> +	},
> +};
> +builtin_platform_driver(omap_prm_driver);
> +
> +MODULE_ALIAS("platform:prm");
> +MODULE_LICENSE("GPL v2");
> +MODULE_DESCRIPTION("omap2+ prm driver");

It is a builtin_platform_driver so do we need the MODULE*?

> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 41+ messages in thread

* Re: [PATCH 5/8] soc: ti: omap-prm: add omap4 PRM data
  2019-08-07  7:48 ` [PATCH 5/8] soc: ti: omap-prm: add omap4 PRM data Tero Kristo
@ 2019-08-08  5:30   ` Keerthy
  2019-08-19  9:32     ` Tero Kristo
  2019-08-19 23:08   ` Suman Anna
  1 sibling, 1 reply; 41+ messages in thread
From: Keerthy @ 2019-08-08  5:30 UTC (permalink / raw)
  To: Tero Kristo, ssantosh, linux-arm-kernel, linux-omap, robh+dt
  Cc: tony, devicetree



On 07/08/19 1:18 PM, Tero Kristo wrote:
> Add PRM data for omap4 family of SoCs.
> 
> Signed-off-by: Tero Kristo <t-kristo@ti.com>
> ---
>   drivers/soc/ti/omap_prm.c | 20 ++++++++++++++++++++
>   1 file changed, 20 insertions(+)
> 
> diff --git a/drivers/soc/ti/omap_prm.c b/drivers/soc/ti/omap_prm.c
> index 870515e3..9b8d5945 100644
> --- a/drivers/soc/ti/omap_prm.c
> +++ b/drivers/soc/ti/omap_prm.c
> @@ -54,7 +54,27 @@ struct omap_reset_data {
>   
>   #define OMAP_PRM_NO_RSTST	BIT(0)
>   
> +struct omap_prm_data omap4_prm_data[] = {
> +	{ .name = "mpu", .base = 0x4a306300, .pwstst = 0x4 },
> +	{ .name = "tesla", .base = 0x4a306400, .pwstst = 0x4, .rstctl = 0x10, .rstst = 0x14 },
> +	{ .name = "abe", .base = 0x4a306500, .pwstst = 0x4 },
> +	{ .name = "always_on_core", .base = 0x4a306600, .pwstst = 0x4 },
> +	{ .name = "core", .base = 0x4a306700, .pwstst = 0x4, .rstctl = 0x210, .rstst = 0x214 },
> +	{ .name = "ivahd", .base = 0x4a306f00, .pwstst = 0x4, .rstctl = 0x10, .rstst = 0x14 },
> +	{ .name = "cam", .base = 0x4a307000, .pwstst = 0x4 },
> +	{ .name = "dss", .base = 0x4a307100, .pwstst = 0x4 },
> +	{ .name = "gfx", .base = 0x4a307200, .pwstst = 0x4 },
> +	{ .name = "l3init", .base = 0x4a307300, .pwstst = 0x4 },
> +	{ .name = "l4per", .base = 0x4a307400, .pwstst = 0x4 },
> +	{ .name = "cefuse", .base = 0x4a307600, .pwstst = 0x4 },
> +	{ .name = "wkup", .base = 0x4a307700, .pwstst = 0x4 },
> +	{ .name = "emu", .base = 0x4a307900, .pwstst = 0x4 },
> +	{ .name = "device", .base = 0x4a307b00, .rstctl = 0x0, .rstst = 0x4 },
> +	{ },
> +};

So at some point arch/arm/mach-omap2/powerdomains44xx_data.c
duplicated data will be removed?

> +
>   static const struct of_device_id omap_prm_id_table[] = {
> +	{ .compatible = "ti,omap4-prm-inst", .data = omap4_prm_data },
>   	{ },
>   };
>   
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 41+ messages in thread

* Re: [PATCH 1/8] dt-bindings: omap: add new binding for PRM instances
  2019-08-08  4:35   ` Keerthy
@ 2019-08-19  9:28     ` Tero Kristo
  2019-08-19 21:28       ` Suman Anna
  0 siblings, 1 reply; 41+ messages in thread
From: Tero Kristo @ 2019-08-19  9:28 UTC (permalink / raw)
  To: Keerthy, ssantosh, linux-arm-kernel, linux-omap, robh+dt
  Cc: tony, devicetree, s-anna

On 08/08/2019 07:35, Keerthy wrote:
> 
> 
> On 07/08/19 1:18 PM, Tero Kristo wrote:
>> Add new binding for OMAP PRM (Power and Reset Manager) instances. Each
>> of these will act as a power domain controller and potentially as a reset
>> provider.
>>
>> Signed-off-by: Tero Kristo <t-kristo@ti.com>
>> ---
>>   .../devicetree/bindings/arm/omap/prm-inst.txt      | 24 
>> ++++++++++++++++++++++
>>   1 file changed, 24 insertions(+)
>>   create mode 100644 
>> Documentation/devicetree/bindings/arm/omap/prm-inst.txt
>>
>> diff --git a/Documentation/devicetree/bindings/arm/omap/prm-inst.txt 
>> b/Documentation/devicetree/bindings/arm/omap/prm-inst.txt
>> new file mode 100644
>> index 0000000..e0ae87b
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/arm/omap/prm-inst.txt
>> @@ -0,0 +1,24 @@
>> +OMAP PRM instance bindings
>> +
>> +Power and Reset Manager is an IP block on OMAP family of devices which
>> +handle the power domains and their current state, and provide reset
>> +handling for the domains and/or separate IP blocks under the power 
>> domain
>> +hierarchy.
>> +
>> +Required properties:
>> +- compatible:    Must be one of:
>> +        "ti,am3-prm-inst"
>> +        "ti,am4-prm-inst"
>> +        "ti,omap4-prm-inst"
>> +        "ti,omap5-prm-inst"
>> +        "ti,dra7-prm-inst"
>> +- reg:        Contains PRM instance register address range
>> +        (base address and length)
> 
> How about reset-cells property, Isn't that a mandatory property?

It is optional, but you are right, should be added to this.

-Tero

> 
>> +
>> +Example:
>> +
>> +prm_dsp2: prm@1b00 {
>> +    compatible = "ti,dra7-prm-inst";
>> +    reg = <0x1b00 0x40>;
>> +    #reset-cells = <1>;
>> +};
>>

--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 41+ messages in thread

* Re: [PATCH 2/8] soc: ti: add initial PRM driver with reset control support
  2019-08-08  5:26   ` Keerthy
@ 2019-08-19  9:32     ` Tero Kristo
  2019-08-19 23:01       ` Suman Anna
  0 siblings, 1 reply; 41+ messages in thread
From: Tero Kristo @ 2019-08-19  9:32 UTC (permalink / raw)
  To: Keerthy, ssantosh, linux-arm-kernel, linux-omap, robh+dt
  Cc: tony, devicetree, s-anna

On 08/08/2019 08:26, Keerthy wrote:
> 
> 
> On 07/08/19 1:18 PM, Tero Kristo wrote:
>> Add initial PRM (Power and Reset Management) driver for TI OMAP class
>> SoCs. Initially this driver only supports reset control, but can be
>> extended to support rest of the functionality, like powerdomain
>> control, PRCM irq support etc.
>>
>> Signed-off-by: Tero Kristo <t-kristo@ti.com>
>> ---
>>   arch/arm/mach-omap2/Kconfig |   1 +
>>   drivers/soc/ti/Makefile     |   1 +
>>   drivers/soc/ti/omap_prm.c   | 216 
>> ++++++++++++++++++++++++++++++++++++++++++++
>>   3 files changed, 218 insertions(+)
>>   create mode 100644 drivers/soc/ti/omap_prm.c
>>
>> diff --git a/arch/arm/mach-omap2/Kconfig b/arch/arm/mach-omap2/Kconfig
>> index fdb6743..42ad063 100644
>> --- a/arch/arm/mach-omap2/Kconfig
>> +++ b/arch/arm/mach-omap2/Kconfig
>> @@ -109,6 +109,7 @@ config ARCH_OMAP2PLUS
>>       select TI_SYSC
>>       select OMAP_IRQCHIP
>>       select CLKSRC_TI_32K
>> +    select RESET_CONTROLLER
>>       help
>>         Systems based on OMAP2, OMAP3, OMAP4 or OMAP5
>> diff --git a/drivers/soc/ti/Makefile b/drivers/soc/ti/Makefile
>> index b3868d3..788b5cd 100644
>> --- a/drivers/soc/ti/Makefile
>> +++ b/drivers/soc/ti/Makefile
>> @@ -6,6 +6,7 @@ obj-$(CONFIG_KEYSTONE_NAVIGATOR_QMSS)    += knav_qmss.o
>>   knav_qmss-y := knav_qmss_queue.o knav_qmss_acc.o
>>   obj-$(CONFIG_KEYSTONE_NAVIGATOR_DMA)    += knav_dma.o
>>   obj-$(CONFIG_AMX3_PM)            += pm33xx.o
>> +obj-$(CONFIG_ARCH_OMAP2PLUS)        += omap_prm.o
>>   obj-$(CONFIG_WKUP_M3_IPC)        += wkup_m3_ipc.o
>>   obj-$(CONFIG_TI_SCI_PM_DOMAINS)        += ti_sci_pm_domains.o
>>   obj-$(CONFIG_TI_SCI_INTA_MSI_DOMAIN)    += ti_sci_inta_msi.o
>> diff --git a/drivers/soc/ti/omap_prm.c b/drivers/soc/ti/omap_prm.c
>> new file mode 100644
>> index 0000000..7c89eb8
>> --- /dev/null
>> +++ b/drivers/soc/ti/omap_prm.c
>> @@ -0,0 +1,216 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * OMAP2+ PRM driver
>> + *
>> + * Copyright (C) 2019 Texas Instruments Incorporated - 
>> http://www.ti.com/
>> + *    Tero Kristo <t-kristo@ti.com>
>> + */
>> +
>> +#include <linux/kernel.h>
>> +#include <linux/module.h>
>> +#include <linux/device.h>
>> +#include <linux/io.h>
>> +#include <linux/of.h>
>> +#include <linux/of_device.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/reset-controller.h>
>> +#include <linux/delay.h>
>> +
>> +struct omap_rst_map {
>> +    s8 rst;
>> +    s8 st;
>> +};
>> +
>> +struct omap_prm_data {
>> +    u32 base;
>> +    const char *name;
>> +    u16 pwstctrl;
>> +    u16 pwstst;
>> +    u16 rstctl;
>> +    u16 rstst;
>> +    struct omap_rst_map *rstmap;
>> +    u8 flags;
>> +};
>> +
>> +struct omap_prm {
>> +    const struct omap_prm_data *data;
>> +    void __iomem *base;
>> +};
>> +
>> +struct omap_reset_data {
>> +    struct reset_controller_dev rcdev;
>> +    struct omap_prm *prm;
>> +};
>> +
>> +#define to_omap_reset_data(p) container_of((p), struct 
>> omap_reset_data, rcdev)
>> +
>> +#define OMAP_MAX_RESETS        8
>> +#define OMAP_RESET_MAX_WAIT    10000
>> +
>> +#define OMAP_PRM_NO_RSTST    BIT(0)
>> +
>> +static const struct of_device_id omap_prm_id_table[] = {
>> +    { },
>> +};
> 
> This table is blank and we are doing of_match_device against it.

Yes, it gets populated with other patches in series, one entry per soc.

> 
>> +
>> +static int omap_reset_status(struct reset_controller_dev *rcdev,
>> +                 unsigned long id)
>> +{
>> +    struct omap_reset_data *reset = to_omap_reset_data(rcdev);
>> +    u32 v;
>> +
>> +    v = readl_relaxed(reset->prm->base + reset->prm->data->rstst);
>> +    v &= 1 << id;
>> +    v >>= id;
>> +
>> +    return v;
>> +}
>> +
>> +static int omap_reset_assert(struct reset_controller_dev *rcdev,
>> +                 unsigned long id)
>> +{
>> +    struct omap_reset_data *reset = to_omap_reset_data(rcdev);
>> +    u32 v;
>> +
>> +    /* assert the reset control line */
>> +    v = readl_relaxed(reset->prm->base + reset->prm->data->rstctl);
>> +    v |= 1 << id;
>> +    writel_relaxed(v, reset->prm->base + reset->prm->data->rstctl);
>> +
>> +    return 0;
>> +}
>> +
>> +static int omap_reset_get_st_bit(struct omap_reset_data *reset,
>> +                 unsigned long id)
>> +{
>> +    struct omap_rst_map *map = reset->prm->data->rstmap;
>> +
>> +    while (map && map->rst >= 0) {
>> +        if (map->rst == id)
>> +            return map->st;
>> +
>> +        map++;
>> +    }
>> +
>> +    return id;
>> +}
>> +
>> +/*
>> + * Note that status will not change until clocks are on, and clocks 
>> cannot be
>> + * enabled until reset is deasserted. Consumer drivers must check status
>> + * separately after enabling clocks.
>> + */
>> +static int omap_reset_deassert(struct reset_controller_dev *rcdev,
>> +                   unsigned long id)
>> +{
>> +    struct omap_reset_data *reset = to_omap_reset_data(rcdev);
>> +    u32 v;
>> +    int st_bit = id;
>> +    bool has_rstst;
>> +
>> +    /* check the current status to avoid de-asserting the line twice */
>> +    v = readl_relaxed(reset->prm->base + reset->prm->data->rstctl);
>> +    if (!(v & BIT(id)))
>> +        return -EEXIST;
>> +
>> +    has_rstst = !(reset->prm->data->flags & OMAP_PRM_NO_RSTST);
>> +
>> +    if (has_rstst) {
>> +        st_bit = omap_reset_get_st_bit(reset, id);
>> +
>> +        /* Clear the reset status by writing 1 to the status bit */
>> +        v = readl_relaxed(reset->prm->base + reset->prm->data->rstst);
>> +        v |= 1 << st_bit;
>> +        writel_relaxed(v, reset->prm->base + reset->prm->data->rstst);
>> +    }
>> +
>> +    /* de-assert the reset control line */
>> +    v = readl_relaxed(reset->prm->base + reset->prm->data->rstctl);
>> +    v &= ~(1 << id);
>> +    writel_relaxed(v, reset->prm->base + reset->prm->data->rstctl);
>> +
>> +    return 0;
>> +}
>> +
>> +static const struct reset_control_ops omap_reset_ops = {
>> +    .assert        = omap_reset_assert,
>> +    .deassert    = omap_reset_deassert,
>> +    .status        = omap_reset_status,
>> +};
>> +
>> +static int omap_prm_reset_probe(struct platform_device *pdev,
>> +                struct omap_prm *prm)
>> +{
>> +    struct omap_reset_data *reset;
>> +
>> +    /*
>> +     * Check if we have resets. If either rstctl or rstst is
>> +     * non-zero, we have reset registers in place. Additionally
>> +     * the flag OMAP_PRM_NO_RSTST implies that we have resets.
>> +     */
>> +    if (!prm->data->rstctl && !prm->data->rstst &&
>> +        !(prm->data->flags & OMAP_PRM_NO_RSTST))
>> +        return 0;
>> +
>> +    reset = devm_kzalloc(&pdev->dev, sizeof(*reset), GFP_KERNEL);
>> +    if (!reset)
>> +        return -ENOMEM;
>> +
>> +    reset->rcdev.owner = THIS_MODULE;
>> +    reset->rcdev.ops = &omap_reset_ops;
>> +    reset->rcdev.of_node = pdev->dev.of_node;
>> +    reset->rcdev.nr_resets = OMAP_MAX_RESETS;
>> +
>> +    reset->prm = prm;
>> +
>> +    return devm_reset_controller_register(&pdev->dev, &reset->rcdev);
>> +}
>> +
>> +static int omap_prm_probe(struct platform_device *pdev)
>> +{
>> +    struct resource *res;
>> +    const struct omap_prm_data *data;
>> +    struct omap_prm *prm;
>> +    const struct of_device_id *match;
>> +
>> +    res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> +    if (!res)
>> +        return -ENODEV;
>> +
>> +    match = of_match_device(omap_prm_id_table, &pdev->dev);
>> +    if (!match)
>> +        return -ENOTSUPP;
>> +
>> +    prm = devm_kzalloc(&pdev->dev, sizeof(*prm), GFP_KERNEL);
>> +    if (!prm)
>> +        return -ENOMEM;
>> +
>> +    data = match->data;
>> +
>> +    while (data->base != res->start) {
>> +        if (!data->base)
>> +            return -EINVAL;
>> +        data++;
>> +    }
>> +
>> +    prm->data = data;
>> +
>> +    prm->base = devm_ioremap_resource(&pdev->dev, res);
>> +    if (!prm->base)
>> +        return -ENOMEM;
>> +
>> +    return omap_prm_reset_probe(pdev, prm);
>> +}
>> +
>> +static struct platform_driver omap_prm_driver = {
>> +    .probe = omap_prm_probe,
>> +    .driver = {
>> +        .name        = KBUILD_MODNAME,
>> +        .of_match_table    = omap_prm_id_table,
>> +    },
>> +};
>> +builtin_platform_driver(omap_prm_driver);
>> +
>> +MODULE_ALIAS("platform:prm");
>> +MODULE_LICENSE("GPL v2");
>> +MODULE_DESCRIPTION("omap2+ prm driver");
> 
> It is a builtin_platform_driver so do we need the MODULE*?

Well, thats debatable, however some existing drivers do introduce this 
even if they are builtin.

-Tero
--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 41+ messages in thread

* Re: [PATCH 5/8] soc: ti: omap-prm: add omap4 PRM data
  2019-08-08  5:30   ` Keerthy
@ 2019-08-19  9:32     ` Tero Kristo
  0 siblings, 0 replies; 41+ messages in thread
From: Tero Kristo @ 2019-08-19  9:32 UTC (permalink / raw)
  To: Keerthy, ssantosh, linux-arm-kernel, linux-omap, robh+dt
  Cc: tony, devicetree, s-anna

On 08/08/2019 08:30, Keerthy wrote:
> 
> 
> On 07/08/19 1:18 PM, Tero Kristo wrote:
>> Add PRM data for omap4 family of SoCs.
>>
>> Signed-off-by: Tero Kristo <t-kristo@ti.com>
>> ---
>>   drivers/soc/ti/omap_prm.c | 20 ++++++++++++++++++++
>>   1 file changed, 20 insertions(+)
>>
>> diff --git a/drivers/soc/ti/omap_prm.c b/drivers/soc/ti/omap_prm.c
>> index 870515e3..9b8d5945 100644
>> --- a/drivers/soc/ti/omap_prm.c
>> +++ b/drivers/soc/ti/omap_prm.c
>> @@ -54,7 +54,27 @@ struct omap_reset_data {
>>   #define OMAP_PRM_NO_RSTST    BIT(0)
>> +struct omap_prm_data omap4_prm_data[] = {
>> +    { .name = "mpu", .base = 0x4a306300, .pwstst = 0x4 },
>> +    { .name = "tesla", .base = 0x4a306400, .pwstst = 0x4, .rstctl = 
>> 0x10, .rstst = 0x14 },
>> +    { .name = "abe", .base = 0x4a306500, .pwstst = 0x4 },
>> +    { .name = "always_on_core", .base = 0x4a306600, .pwstst = 0x4 },
>> +    { .name = "core", .base = 0x4a306700, .pwstst = 0x4, .rstctl = 
>> 0x210, .rstst = 0x214 },
>> +    { .name = "ivahd", .base = 0x4a306f00, .pwstst = 0x4, .rstctl = 
>> 0x10, .rstst = 0x14 },
>> +    { .name = "cam", .base = 0x4a307000, .pwstst = 0x4 },
>> +    { .name = "dss", .base = 0x4a307100, .pwstst = 0x4 },
>> +    { .name = "gfx", .base = 0x4a307200, .pwstst = 0x4 },
>> +    { .name = "l3init", .base = 0x4a307300, .pwstst = 0x4 },
>> +    { .name = "l4per", .base = 0x4a307400, .pwstst = 0x4 },
>> +    { .name = "cefuse", .base = 0x4a307600, .pwstst = 0x4 },
>> +    { .name = "wkup", .base = 0x4a307700, .pwstst = 0x4 },
>> +    { .name = "emu", .base = 0x4a307900, .pwstst = 0x4 },
>> +    { .name = "device", .base = 0x4a307b00, .rstctl = 0x0, .rstst = 
>> 0x4 },
>> +    { },
>> +};
> 
> So at some point arch/arm/mach-omap2/powerdomains44xx_data.c
> duplicated data will be removed?

Yes, that would be the path forward eventually.

-Tero

> 
>> +
>>   static const struct of_device_id omap_prm_id_table[] = {
>> +    { .compatible = "ti,omap4-prm-inst", .data = omap4_prm_data },
>>       { },
>>   };
>>

--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 41+ messages in thread

* Re: [PATCH 1/8] dt-bindings: omap: add new binding for PRM instances
  2019-08-19  9:28     ` Tero Kristo
@ 2019-08-19 21:28       ` Suman Anna
  2019-08-20  7:45         ` Tero Kristo
  0 siblings, 1 reply; 41+ messages in thread
From: Suman Anna @ 2019-08-19 21:28 UTC (permalink / raw)
  To: Tero Kristo, Keerthy, ssantosh, linux-arm-kernel, linux-omap, robh+dt
  Cc: tony, devicetree

Hi Tero,

On 8/19/19 4:28 AM, Tero Kristo wrote:
> On 08/08/2019 07:35, Keerthy wrote:
>>
>>
>> On 07/08/19 1:18 PM, Tero Kristo wrote:
>>> Add new binding for OMAP PRM (Power and Reset Manager) instances. Each
>>> of these will act as a power domain controller and potentially as a
>>> reset
>>> provider.
>>>
>>> Signed-off-by: Tero Kristo <t-kristo@ti.com>
>>> ---
>>>   .../devicetree/bindings/arm/omap/prm-inst.txt      | 24
>>> ++++++++++++++++++++++
>>>   1 file changed, 24 insertions(+)
>>>   create mode 100644
>>> Documentation/devicetree/bindings/arm/omap/prm-inst.txt
>>>
>>> diff --git a/Documentation/devicetree/bindings/arm/omap/prm-inst.txt
>>> b/Documentation/devicetree/bindings/arm/omap/prm-inst.txt
>>> new file mode 100644
>>> index 0000000..e0ae87b
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/arm/omap/prm-inst.txt
>>> @@ -0,0 +1,24 @@
>>> +OMAP PRM instance bindings
>>> +
>>> +Power and Reset Manager is an IP block on OMAP family of devices which
>>> +handle the power domains and their current state, and provide reset
>>> +handling for the domains and/or separate IP blocks under the power
>>> domain
>>> +hierarchy.

So, I see that you are adding these as flat nodes directly under the prm
nodes where we have the clocks and clock_domains. Are you anticipating a
single DT node and/or driver managing both power-domains and resets?

>>> +
>>> +Required properties:
>>> +- compatible:    Must be one of:
>>> +        "ti,am3-prm-inst"
>>> +        "ti,am4-prm-inst"
>>> +        "ti,omap4-prm-inst"
>>> +        "ti,omap5-prm-inst"
>>> +        "ti,dra7-prm-inst"

What about OMAP2, OMAP3, DM814x, DM816x?

regards
Suman

>>> +- reg:        Contains PRM instance register address range
>>> +        (base address and length)
>>
>> How about reset-cells property, Isn't that a mandatory property?
> 
> It is optional, but you are right, should be added to this.
> 
> -Tero
> 
>>
>>> +
>>> +Example:
>>> +
>>> +prm_dsp2: prm@1b00 {
>>> +    compatible = "ti,dra7-prm-inst";
>>> +    reg = <0x1b00 0x40>;
>>> +    #reset-cells = <1>;
>>> +};
>>>
> 
> -- 
> Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
> Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 41+ messages in thread

* Re: [PATCH 2/8] soc: ti: add initial PRM driver with reset control support
  2019-08-19  9:32     ` Tero Kristo
@ 2019-08-19 23:01       ` Suman Anna
  2019-08-20  7:37         ` Tero Kristo
  0 siblings, 1 reply; 41+ messages in thread
From: Suman Anna @ 2019-08-19 23:01 UTC (permalink / raw)
  To: Tero Kristo, Keerthy, ssantosh, linux-arm-kernel, linux-omap, robh+dt
  Cc: tony, devicetree

Hi Tero,

On 8/19/19 4:32 AM, Tero Kristo wrote:
> On 08/08/2019 08:26, Keerthy wrote:
>>
>>
>> On 07/08/19 1:18 PM, Tero Kristo wrote:
>>> Add initial PRM (Power and Reset Management) driver for TI OMAP class
>>> SoCs. Initially this driver only supports reset control, but can be
>>> extended to support rest of the functionality, like powerdomain
>>> control, PRCM irq support etc.
>>>
>>> Signed-off-by: Tero Kristo <t-kristo@ti.com>
>>> ---
>>>   arch/arm/mach-omap2/Kconfig |   1 +
>>>   drivers/soc/ti/Makefile     |   1 +
>>>   drivers/soc/ti/omap_prm.c   | 216
>>> ++++++++++++++++++++++++++++++++++++++++++++
>>>   3 files changed, 218 insertions(+)
>>>   create mode 100644 drivers/soc/ti/omap_prm.c
>>>
>>> diff --git a/arch/arm/mach-omap2/Kconfig b/arch/arm/mach-omap2/Kconfig
>>> index fdb6743..42ad063 100644
>>> --- a/arch/arm/mach-omap2/Kconfig
>>> +++ b/arch/arm/mach-omap2/Kconfig
>>> @@ -109,6 +109,7 @@ config ARCH_OMAP2PLUS
>>>       select TI_SYSC
>>>       select OMAP_IRQCHIP
>>>       select CLKSRC_TI_32K
>>> +    select RESET_CONTROLLER

Use ARCH_HAS_RESET_CONTROLLER instead.

>>>       help
>>>         Systems based on OMAP2, OMAP3, OMAP4 or OMAP5
>>> diff --git a/drivers/soc/ti/Makefile b/drivers/soc/ti/Makefile
>>> index b3868d3..788b5cd 100644
>>> --- a/drivers/soc/ti/Makefile
>>> +++ b/drivers/soc/ti/Makefile
>>> @@ -6,6 +6,7 @@ obj-$(CONFIG_KEYSTONE_NAVIGATOR_QMSS)    += knav_qmss.o
>>>   knav_qmss-y := knav_qmss_queue.o knav_qmss_acc.o
>>>   obj-$(CONFIG_KEYSTONE_NAVIGATOR_DMA)    += knav_dma.o
>>>   obj-$(CONFIG_AMX3_PM)            += pm33xx.o
>>> +obj-$(CONFIG_ARCH_OMAP2PLUS)        += omap_prm.o
>>>   obj-$(CONFIG_WKUP_M3_IPC)        += wkup_m3_ipc.o
>>>   obj-$(CONFIG_TI_SCI_PM_DOMAINS)        += ti_sci_pm_domains.o
>>>   obj-$(CONFIG_TI_SCI_INTA_MSI_DOMAIN)    += ti_sci_inta_msi.o
>>> diff --git a/drivers/soc/ti/omap_prm.c b/drivers/soc/ti/omap_prm.c
>>> new file mode 100644
>>> index 0000000..7c89eb8
>>> --- /dev/null
>>> +++ b/drivers/soc/ti/omap_prm.c
>>> @@ -0,0 +1,216 @@
>>> +// SPDX-License-Identifier: GPL-2.0
>>> +/*
>>> + * OMAP2+ PRM driver
>>> + *
>>> + * Copyright (C) 2019 Texas Instruments Incorporated -
>>> http://www.ti.com/
>>> + *    Tero Kristo <t-kristo@ti.com>
>>> + */
>>> +
>>> +#include <linux/kernel.h>
>>> +#include <linux/module.h>
>>> +#include <linux/device.h>
>>> +#include <linux/io.h>
>>> +#include <linux/of.h>
>>> +#include <linux/of_device.h>
>>> +#include <linux/platform_device.h>
>>> +#include <linux/reset-controller.h>
>>> +#include <linux/delay.h>
>>> +
>>> +struct omap_rst_map {
>>> +    s8 rst;
>>> +    s8 st;
>>> +};
>>> +
>>> +struct omap_prm_data {
>>> +    u32 base;
>>> +    const char *name;
>>> +    u16 pwstctrl;
>>> +    u16 pwstst;
>>> +    u16 rstctl;

Minor nit, can you use rstctrl instead here so that it is in sync with
the other variables and with the register names used in TRM.

>>> +    u16 rstst;
>>> +    struct omap_rst_map *rstmap;
>>> +    u8 flags;
>>> +};
>>> +
>>> +struct omap_prm {
>>> +    const struct omap_prm_data *data;
>>> +    void __iomem *base;
>>> +};
>>> +
>>> +struct omap_reset_data {
>>> +    struct reset_controller_dev rcdev;
>>> +    struct omap_prm *prm;
>>> +};
>>> +
>>> +#define to_omap_reset_data(p) container_of((p), struct
>>> omap_reset_data, rcdev)
>>> +
>>> +#define OMAP_MAX_RESETS        8
>>> +#define OMAP_RESET_MAX_WAIT    10000
>>> +
>>> +#define OMAP_PRM_NO_RSTST    BIT(0)
>>> +
>>> +static const struct of_device_id omap_prm_id_table[] = {
>>> +    { },
>>> +};
>>
>> This table is blank and we are doing of_match_device against it.
> 
> Yes, it gets populated with other patches in series, one entry per soc.
> 
>>
>>> +
>>> +static int omap_reset_status(struct reset_controller_dev *rcdev,
>>> +                 unsigned long id)
>>> +{
>>> +    struct omap_reset_data *reset = to_omap_reset_data(rcdev);
>>> +    u32 v;
>>> +
>>> +    v = readl_relaxed(reset->prm->base + reset->prm->data->rstst);
>>> +    v &= 1 << id;
>>> +    v >>= id;
>>> +
>>> +    return v;
>>> +}
>>> +
>>> +static int omap_reset_assert(struct reset_controller_dev *rcdev,
>>> +                 unsigned long id)
>>> +{
>>> +    struct omap_reset_data *reset = to_omap_reset_data(rcdev);
>>> +    u32 v;
>>> +
>>> +    /* assert the reset control line */
>>> +    v = readl_relaxed(reset->prm->base + reset->prm->data->rstctl);
>>> +    v |= 1 << id;
>>> +    writel_relaxed(v, reset->prm->base + reset->prm->data->rstctl);
>>> +
>>> +    return 0;
>>> +}
>>> +
>>> +static int omap_reset_get_st_bit(struct omap_reset_data *reset,
>>> +                 unsigned long id)
>>> +{
>>> +    struct omap_rst_map *map = reset->prm->data->rstmap;
>>> +
>>> +    while (map && map->rst >= 0) {
>>> +        if (map->rst == id)
>>> +            return map->st;
>>> +
>>> +        map++;
>>> +    }
>>> +
>>> +    return id;
>>> +}
>>> +
>>> +/*
>>> + * Note that status will not change until clocks are on, and clocks
>>> cannot be
>>> + * enabled until reset is deasserted. Consumer drivers must check
>>> status
>>> + * separately after enabling clocks.
>>> + */
>>> +static int omap_reset_deassert(struct reset_controller_dev *rcdev,
>>> +                   unsigned long id)
>>> +{
>>> +    struct omap_reset_data *reset = to_omap_reset_data(rcdev);
>>> +    u32 v;
>>> +    int st_bit = id;

No need to initialize this, will always get overwritten below.

>>> +    bool has_rstst;
>>> +
>>> +    /* check the current status to avoid de-asserting the line twice */
>>> +    v = readl_relaxed(reset->prm->base + reset->prm->data->rstctl);
>>> +    if (!(v & BIT(id)))
>>> +        return -EEXIST;
>>> +
>>> +    has_rstst = !(reset->prm->data->flags & OMAP_PRM_NO_RSTST);
>>> +
>>> +    if (has_rstst) {
>>> +        st_bit = omap_reset_get_st_bit(reset, id);
>>> +
>>> +        /* Clear the reset status by writing 1 to the status bit */
>>> +        v = readl_relaxed(reset->prm->base + reset->prm->data->rstst);
>>> +        v |= 1 << st_bit;
>>> +        writel_relaxed(v, reset->prm->base + reset->prm->data->rstst);
>>> +    }
>>> +
>>> +    /* de-assert the reset control line */
>>> +    v = readl_relaxed(reset->prm->base + reset->prm->data->rstctl);
>>> +    v &= ~(1 << id);
>>> +    writel_relaxed(v, reset->prm->base + reset->prm->data->rstctl);
>>> +
>>> +    return 0;
>>> +}
>>> +
>>> +static const struct reset_control_ops omap_reset_ops = {
>>> +    .assert        = omap_reset_assert,
>>> +    .deassert    = omap_reset_deassert,
>>> +    .status        = omap_reset_status,
>>> +};
>>> +
>>> +static int omap_prm_reset_probe(struct platform_device *pdev,
>>> +                struct omap_prm *prm)

Call this omap_prm_reset_init or something similar to avoid confusion.

>>> +{
>>> +    struct omap_reset_data *reset;
>>> +
>>> +    /*
>>> +     * Check if we have resets. If either rstctl or rstst is
>>> +     * non-zero, we have reset registers in place. Additionally
>>> +     * the flag OMAP_PRM_NO_RSTST implies that we have resets.
>>> +     */
>>> +    if (!prm->data->rstctl && !prm->data->rstst &&
>>> +        !(prm->data->flags & OMAP_PRM_NO_RSTST))
>>> +        return 0;
>>> +
>>> +    reset = devm_kzalloc(&pdev->dev, sizeof(*reset), GFP_KERNEL);
>>> +    if (!reset)
>>> +        return -ENOMEM;
>>> +
>>> +    reset->rcdev.owner = THIS_MODULE;
>>> +    reset->rcdev.ops = &omap_reset_ops;
>>> +    reset->rcdev.of_node = pdev->dev.of_node;
>>> +    reset->rcdev.nr_resets = OMAP_MAX_RESETS;

Suggest adding a number of resets to prm->data, and using it so that we
don't even entertain any resets beyond the actual number of resets.

You actually seem to be using the bit-position directly in client data
instead of a reset number. I am not sure if this is accepted practice
with reset controllers, do you incur any memory wastage?

>>> +
>>> +    reset->prm = prm;
>>> +
>>> +    return devm_reset_controller_register(&pdev->dev, &reset->rcdev);
>>> +}
>>> +
>>> +static int omap_prm_probe(struct platform_device *pdev)
>>> +{
>>> +    struct resource *res;
>>> +    const struct omap_prm_data *data;
>>> +    struct omap_prm *prm;
>>> +    const struct of_device_id *match;
>>> +
>>> +    res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>>> +    if (!res)
>>> +        return -ENODEV;
>>> +
>>> +    match = of_match_device(omap_prm_id_table, &pdev->dev);
>>> +    if (!match)
>>> +        return -ENOTSUPP;

Use of_device_get_match_data() instead to do both match and get the
data. That can perhaps be the first block.

>>> +
>>> +    prm = devm_kzalloc(&pdev->dev, sizeof(*prm), GFP_KERNEL);
>>> +    if (!prm)
>>> +        return -ENOMEM;

Perhaps move the allocate after the match check to streamline.

>>> +
>>> +    data = match->data;
>>> +
>>> +    while (data->base != res->start) {
>>> +        if (!data->base)
>>> +            return -EINVAL;
>>> +        data++;
>>> +    }
>>> +
>>> +    prm->data = data;
>>> +
>>> +    prm->base = devm_ioremap_resource(&pdev->dev, res);
>>> +    if (!prm->base)
>>> +        return -ENOMEM;
>>> +
>>> +    return omap_prm_reset_probe(pdev, prm);
>>> +}
>>> +
>>> +static struct platform_driver omap_prm_driver = {
>>> +    .probe = omap_prm_probe,
>>> +    .driver = {
>>> +        .name        = KBUILD_MODNAME,
>>> +        .of_match_table    = omap_prm_id_table,
>>> +    },
>>> +};
>>> +builtin_platform_driver(omap_prm_driver);
>>> +
>>> +MODULE_ALIAS("platform:prm");

Drop this and use MODULE_DEVICE_TABLE instead on omap_prm_id_table if
retaining, but I don't think these need to be defined.

regards
Suman

>>> +MODULE_LICENSE("GPL v2");
>>> +MODULE_DESCRIPTION("omap2+ prm driver");
>>
>> It is a builtin_platform_driver so do we need the MODULE*?
> 
> Well, thats debatable, however some existing drivers do introduce this
> even if they are builtin.
> 
> -Tero
> -- 
> Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
> Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 41+ messages in thread

* Re: [PATCH 5/8] soc: ti: omap-prm: add omap4 PRM data
  2019-08-07  7:48 ` [PATCH 5/8] soc: ti: omap-prm: add omap4 PRM data Tero Kristo
  2019-08-08  5:30   ` Keerthy
@ 2019-08-19 23:08   ` Suman Anna
  2019-08-20  7:52     ` Tero Kristo
  1 sibling, 1 reply; 41+ messages in thread
From: Suman Anna @ 2019-08-19 23:08 UTC (permalink / raw)
  To: Tero Kristo, ssantosh, linux-arm-kernel, linux-omap, robh+dt
  Cc: tony, devicetree

On 8/7/19 2:48 AM, Tero Kristo wrote:
> Add PRM data for omap4 family of SoCs.
> 
> Signed-off-by: Tero Kristo <t-kristo@ti.com>
> ---
>  drivers/soc/ti/omap_prm.c | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
> 
> diff --git a/drivers/soc/ti/omap_prm.c b/drivers/soc/ti/omap_prm.c
> index 870515e3..9b8d5945 100644
> --- a/drivers/soc/ti/omap_prm.c
> +++ b/drivers/soc/ti/omap_prm.c
> @@ -54,7 +54,27 @@ struct omap_reset_data {
>  
>  #define OMAP_PRM_NO_RSTST	BIT(0)
>  
> +struct omap_prm_data omap4_prm_data[] = {

static const

regards
Suman

> +	{ .name = "mpu", .base = 0x4a306300, .pwstst = 0x4 },
> +	{ .name = "tesla", .base = 0x4a306400, .pwstst = 0x4, .rstctl = 0x10, .rstst = 0x14 },
> +	{ .name = "abe", .base = 0x4a306500, .pwstst = 0x4 },
> +	{ .name = "always_on_core", .base = 0x4a306600, .pwstst = 0x4 },
> +	{ .name = "core", .base = 0x4a306700, .pwstst = 0x4, .rstctl = 0x210, .rstst = 0x214 },
> +	{ .name = "ivahd", .base = 0x4a306f00, .pwstst = 0x4, .rstctl = 0x10, .rstst = 0x14 },
> +	{ .name = "cam", .base = 0x4a307000, .pwstst = 0x4 },
> +	{ .name = "dss", .base = 0x4a307100, .pwstst = 0x4 },
> +	{ .name = "gfx", .base = 0x4a307200, .pwstst = 0x4 },
> +	{ .name = "l3init", .base = 0x4a307300, .pwstst = 0x4 },
> +	{ .name = "l4per", .base = 0x4a307400, .pwstst = 0x4 },
> +	{ .name = "cefuse", .base = 0x4a307600, .pwstst = 0x4 },
> +	{ .name = "wkup", .base = 0x4a307700, .pwstst = 0x4 },
> +	{ .name = "emu", .base = 0x4a307900, .pwstst = 0x4 },
> +	{ .name = "device", .base = 0x4a307b00, .rstctl = 0x0, .rstst = 0x4 },
> +	{ },
> +};
> +
>  static const struct of_device_id omap_prm_id_table[] = {
> +	{ .compatible = "ti,omap4-prm-inst", .data = omap4_prm_data },
>  	{ },
>  };
>  
> 


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 41+ messages in thread

* Re: [PATCH 6/8] soc: ti: omap_prm: add data for am33xx
  2019-08-07  7:48 ` [PATCH 6/8] soc: ti: omap_prm: add data for am33xx Tero Kristo
@ 2019-08-19 23:11   ` Suman Anna
  2019-08-20 18:48   ` Suman Anna
  1 sibling, 0 replies; 41+ messages in thread
From: Suman Anna @ 2019-08-19 23:11 UTC (permalink / raw)
  To: Tero Kristo, ssantosh, linux-arm-kernel, linux-omap, robh+dt
  Cc: tony, devicetree

On 8/7/19 2:48 AM, Tero Kristo wrote:
> Add PRM instance data for AM33xx SoC. Includes some basic register
> definitions and reset data for now.
> 
> Signed-off-by: Tero Kristo <t-kristo@ti.com>
> ---
>  drivers/soc/ti/omap_prm.c | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
> 
> diff --git a/drivers/soc/ti/omap_prm.c b/drivers/soc/ti/omap_prm.c
> index 9b8d5945..fadfc7f 100644
> --- a/drivers/soc/ti/omap_prm.c
> +++ b/drivers/soc/ti/omap_prm.c
> @@ -73,8 +73,25 @@ struct omap_prm_data omap4_prm_data[] = {
>  	{ },
>  };
>  
> +struct omap_rst_map am3_wkup_rst_map[] = {

static const here and below.

regards
Suman

> +	{ .rst = 3, .st = 5 },
> +	{ .rst = -1 },
> +};
> +
> +struct omap_prm_data am3_prm_data[] = {
> +	{ .name = "per", .base = 0x44e00c00, .pwstctrl = 0xc, .pwstst = 0x8, .flags = OMAP_PRM_NO_RSTST },
> +	{ .name = "wkup", .base = 0x44e00d00, .pwstctrl = 0x4, .pwstst = 0x8, .rstst = 0xc, .rstmap = am3_wkup_rst_map },
> +	{ .name = "mpu", .base = 0x44e00e00, .pwstst = 0x4 },
> +	{ .name = "device", .base = 0x44e00f00, .rstctl = 0x0, .rstst = 0x8 },
> +	{ .name = "rtc", .base = 0x44e01000, .pwstst = 0x4 },
> +	{ .name = "gfx", .base = 0x44e01100, .pwstst = 0x10, .rstctl = 0x4, .rstst = 0x14 },
> +	{ .name = "cefuse", .base = 0x44e01200, .pwstst = 0x4 },
> +	{ },
> +};
> +
>  static const struct of_device_id omap_prm_id_table[] = {
>  	{ .compatible = "ti,omap4-prm-inst", .data = omap4_prm_data },
> +	{ .compatible = "ti,am3-prm-inst", .data = am3_prm_data },
>  	{ },
>  };
>  
> 


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 41+ messages in thread

* Re: [PATCH 7/8] soc: ti: omap-prm: add dra7 PRM data
  2019-08-07  7:48 ` [PATCH 7/8] soc: ti: omap-prm: add dra7 PRM data Tero Kristo
@ 2019-08-19 23:12   ` Suman Anna
  2019-08-20 19:03   ` Suman Anna
  1 sibling, 0 replies; 41+ messages in thread
From: Suman Anna @ 2019-08-19 23:12 UTC (permalink / raw)
  To: Tero Kristo, ssantosh, linux-arm-kernel, linux-omap, robh+dt
  Cc: tony, devicetree

On 8/7/19 2:48 AM, Tero Kristo wrote:
> Add PRM data for dra7 family of SoCs.
> 
> Signed-off-by: Tero Kristo <t-kristo@ti.com>
> ---
>  drivers/soc/ti/omap_prm.c | 26 ++++++++++++++++++++++++++
>  1 file changed, 26 insertions(+)
> 
> diff --git a/drivers/soc/ti/omap_prm.c b/drivers/soc/ti/omap_prm.c
> index fadfc7f..05b7749 100644
> --- a/drivers/soc/ti/omap_prm.c
> +++ b/drivers/soc/ti/omap_prm.c
> @@ -73,6 +73,31 @@ struct omap_prm_data omap4_prm_data[] = {
>  	{ },
>  };
>  
> +static struct omap_prm_data dra7_prm_data[] = {

Same comment as on other data, static const.

regards
Suman

> +	{ .name = "mpu", .base = 0x4ae06300, .pwstst = 0x4 },
> +	{ .name = "dsp1", .base = 0x4ae06400, .pwstst = 0x4, .rstctl = 0x10, .rstst = 0x14 },
> +	{ .name = "ipu", .base = 0x4ae06500, .pwstst = 0x4, .rstctl = 0x10, .rstst = 0x14, .clkdm_name = "ipu1" },
> +	{ .name = "coreaon", .base = 0x4ae06628, .pwstst = 0x4 },
> +	{ .name = "core", .base = 0x4ae06700, .pwstst = 0x4, .rstctl = 0x210, .rstst = 0x214, .clkdm_name = "ipu2" },
> +	{ .name = "iva", .base = 0x4ae06f00, .pwstst = 0x4 },
> +	{ .name = "cam", .base = 0x4ae07000, .pwstst = 0x4 },
> +	{ .name = "dss", .base = 0x4ae07100, .pwstst = 0x4 },
> +	{ .name = "gpu", .base = 0x4ae07200, .pwstst = 0x4 },
> +	{ .name = "l3init", .base = 0x4ae07300, .pwstst = 0x4, .rstctl = 0x10, .rstst = 0x14 },
> +	{ .name = "l4per", .base = 0x4ae07400, .pwstst = 0x4 },
> +	{ .name = "custefuse", .base = 0x4ae07600, .pwstst = 0x4 },
> +	{ .name = "wkupaon", .base = 0x4ae07724, .pwstst = 0x4 },
> +	{ .name = "emu", .base = 0x4ae07900, .pwstst = 0x4 },
> +	{ .name = "dsp2", .base = 0x4ae07b00, .pwstst = 0x4, .rstctl = 0x10, .rstst = 0x14 },
> +	{ .name = "eve1", .base = 0x4ae07b40, .pwstst = 0x4 },
> +	{ .name = "eve2", .base = 0x4ae07b80, .pwstst = 0x4 },
> +	{ .name = "eve3", .base = 0x4ae07bc0, .pwstst = 0x4 },
> +	{ .name = "eve4", .base = 0x4ae07c00, .pwstst = 0x4 },
> +	{ .name = "rtc", .base = 0x4ae07c60, .pwstst = 0x4 },
> +	{ .name = "vpe", .base = 0x4ae07c80, .pwstst = 0x4 },
> +	{ },
> +};
> +
>  struct omap_rst_map am3_wkup_rst_map[] = {
>  	{ .rst = 3, .st = 5 },
>  	{ .rst = -1 },
> @@ -91,6 +116,7 @@ struct omap_prm_data am3_prm_data[] = {
>  
>  static const struct of_device_id omap_prm_id_table[] = {
>  	{ .compatible = "ti,omap4-prm-inst", .data = omap4_prm_data },
> +	{ .compatible = "ti,dra7-prm-inst", .data = dra7_prm_data },
>  	{ .compatible = "ti,am3-prm-inst", .data = am3_prm_data },
>  	{ },
>  };
> 


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 41+ messages in thread

* Re: [PATCH 4/8] soc: ti: omap-prm: add support for denying idle for reset clockdomain
  2019-08-07  7:48 ` [PATCH 4/8] soc: ti: omap-prm: add support for denying idle for reset clockdomain Tero Kristo
@ 2019-08-19 23:16   ` Suman Anna
  2019-08-20  7:51     ` Tero Kristo
  0 siblings, 1 reply; 41+ messages in thread
From: Suman Anna @ 2019-08-19 23:16 UTC (permalink / raw)
  To: Tero Kristo, ssantosh, linux-arm-kernel, linux-omap, robh+dt
  Cc: tony, devicetree

Hi Tero,

On 8/7/19 2:48 AM, Tero Kristo wrote:
> TI SoCs hardware reset signals require the parent clockdomain to be
> in force wakeup mode while de-asserting the reset, otherwise it may
> never complete. To support this, add pdata hooks to control the
> clockdomain directly.
> 
> Signed-off-by: Tero Kristo <t-kristo@ti.com>
> ---
>  drivers/soc/ti/omap_prm.c            | 32 ++++++++++++++++++++++++++++----
>  include/linux/platform_data/ti-prm.h | 21 +++++++++++++++++++++
>  2 files changed, 49 insertions(+), 4 deletions(-)
>  create mode 100644 include/linux/platform_data/ti-prm.h
> 
> diff --git a/drivers/soc/ti/omap_prm.c b/drivers/soc/ti/omap_prm.c
> index d412af3..870515e3 100644
> --- a/drivers/soc/ti/omap_prm.c
> +++ b/drivers/soc/ti/omap_prm.c
> @@ -16,6 +16,8 @@
>  #include <linux/reset-controller.h>
>  #include <linux/delay.h>
>  
> +#include <linux/platform_data/ti-prm.h>
> +
>  struct omap_rst_map {
>  	s8 rst;
>  	s8 st;
> @@ -24,6 +26,7 @@ struct omap_rst_map {
>  struct omap_prm_data {
>  	u32 base;
>  	const char *name;
> +	const char *clkdm_name;
>  	u16 pwstctrl;
>  	u16 pwstst;
>  	u16 rstctl;
> @@ -40,6 +43,8 @@ struct omap_prm {
>  struct omap_reset_data {
>  	struct reset_controller_dev rcdev;
>  	struct omap_prm *prm;
> +	struct clockdomain *clkdm;
> +	struct device *dev;
>  };
>  
>  #define to_omap_reset_data(p) container_of((p), struct omap_reset_data, rcdev)
> @@ -108,6 +113,8 @@ static int omap_reset_deassert(struct reset_controller_dev *rcdev,
>  	int st_bit = id;
>  	bool has_rstst;
>  	int timeout = 0;
> +	struct ti_prm_platform_data *pdata = dev_get_platdata(reset->dev);
> +	int ret = 0;
>  
>  	/* check the current status to avoid de-asserting the line twice */
>  	v = readl_relaxed(reset->prm->base + reset->prm->data->rstctl);
> @@ -125,13 +132,16 @@ static int omap_reset_deassert(struct reset_controller_dev *rcdev,
>  		writel_relaxed(v, reset->prm->base + reset->prm->data->rstst);
>  	}
>  
> +	if (pdata->clkdm_deny_idle && reset->clkdm)
> +		pdata->clkdm_deny_idle(reset->clkdm);
> +
>  	/* de-assert the reset control line */
>  	v = readl_relaxed(reset->prm->base + reset->prm->data->rstctl);
>  	v &= ~(1 << id);
>  	writel_relaxed(v, reset->prm->base + reset->prm->data->rstctl);
>  
>  	if (!has_rstst)
> -		return 0;
> +		goto exit;
>  
>  	/* wait for the status to be set */
>  	while (1) {
> @@ -140,13 +150,19 @@ static int omap_reset_deassert(struct reset_controller_dev *rcdev,
>  		if (v)
>  			break;
>  		timeout++;
> -		if (timeout > OMAP_RESET_MAX_WAIT)
> -			return -EBUSY;
> +		if (timeout > OMAP_RESET_MAX_WAIT) {
> +			ret = -EBUSY;
> +			goto exit;
> +		}
>  
>  		udelay(1);
>  	}
>  
> -	return 0;
> +exit:
> +	if (pdata->clkdm_allow_idle && reset->clkdm)
> +		pdata->clkdm_allow_idle(reset->clkdm);
> +
> +	return ret;
>  }
>  
>  static const struct reset_control_ops omap_reset_ops = {
> @@ -159,6 +175,8 @@ static int omap_prm_reset_probe(struct platform_device *pdev,
>  				struct omap_prm *prm)
>  {
>  	struct omap_reset_data *reset;
> +	struct ti_prm_platform_data *pdata = dev_get_platdata(&pdev->dev);

Please add checks for NULL callbacks. I don't think these are optional
right, so better to check in init rather than during runtime. Granted
you will probably not run into this after patch 8, but would be good to
check and print an error in case pdata quirks is missed out.

> +	char buf[32];
>  
>  	/*
>  	 * Check if we have resets. If either rstctl or rstst is
> @@ -177,9 +195,15 @@ static int omap_prm_reset_probe(struct platform_device *pdev,
>  	reset->rcdev.ops = &omap_reset_ops;
>  	reset->rcdev.of_node = pdev->dev.of_node;
>  	reset->rcdev.nr_resets = OMAP_MAX_RESETS;
> +	reset->dev = &pdev->dev;
>  
>  	reset->prm = prm;
>  
> +	sprintf(buf, "%s_clkdm", prm->data->clkdm_name ? prm->data->clkdm_name :
> +		prm->data->name);
> +
> +	reset->clkdm = pdata->clkdm_lookup(buf);

Not checking return status?

regards
Suman

> +
>  	return devm_reset_controller_register(&pdev->dev, &reset->rcdev);
>  }
>  
> diff --git a/include/linux/platform_data/ti-prm.h b/include/linux/platform_data/ti-prm.h
> new file mode 100644
> index 0000000..28154c3
> --- /dev/null
> +++ b/include/linux/platform_data/ti-prm.h
> @@ -0,0 +1,21 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * TI PRM (Power & Reset Manager) platform data
> + *
> + * Copyright (C) 2019 Texas Instruments, Inc.
> + *
> + * Tero Kristo <t-kristo@ti.com>
> + */
> +
> +#ifndef _LINUX_PLATFORM_DATA_TI_PRM_H
> +#define _LINUX_PLATFORM_DATA_TI_PRM_H
> +
> +struct clockdomain;
> +
> +struct ti_prm_platform_data {
> +	void (*clkdm_deny_idle)(struct clockdomain *clkdm);
> +	void (*clkdm_allow_idle)(struct clockdomain *clkdm);
> +	struct clockdomain * (*clkdm_lookup)(const char *name);
> +};
> +
> +#endif /* _LINUX_PLATFORM_DATA_TI_PRM_H */
> 


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 41+ messages in thread

* Re: [PATCH 0/8] soc: ti: Add OMAP PRM driver
  2019-08-07  7:48 [PATCH 0/8] soc: ti: Add OMAP PRM driver Tero Kristo
                   ` (7 preceding siblings ...)
  2019-08-07  7:48 ` [PATCH 8/8] ARM: OMAP2+: pdata-quirks: add PRM data for reset support Tero Kristo
@ 2019-08-19 23:20 ` Suman Anna
  2019-08-20  7:54   ` Tero Kristo
  8 siblings, 1 reply; 41+ messages in thread
From: Suman Anna @ 2019-08-19 23:20 UTC (permalink / raw)
  To: Tero Kristo, ssantosh, linux-arm-kernel, linux-omap, robh+dt
  Cc: tony, devicetree

Hi Tero,

On 8/7/19 2:48 AM, Tero Kristo wrote:
> Hi,
> 
> This series adds OMAP PRM driver which initially supports only reset
> handling. Later on, power domain support can be added to this to get
> rid of the current OMAP power domain handling code which resides
> under the mach-omap2 platform directory. Initially, reset data is
> added for AM3, OMAP4 and DRA7 SoCs.

Wakeup M3 remoteproc driver is fully upstream, so we should be able to
test that driver as well if you can add the AM4 data. That will also
unblock my PRUSS.

If you can add the data to others as well, it will help in easier
migration of the individual drivers, otherwise the ti-sysc interconnect,
hwmod, and hwmod reset data combinations will all have to be supported
in code.

regards
Suman

> 
> I've been testing the reset handling logic with OMAP remoteproc
> driver which has been converted to use generic reset framework. This
> part is a work in progress, so will be posting patches from that part
> later on.
> 
> -Tero
> 
> --
> Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
> 


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 41+ messages in thread

* Re: [PATCH 2/8] soc: ti: add initial PRM driver with reset control support
  2019-08-19 23:01       ` Suman Anna
@ 2019-08-20  7:37         ` Tero Kristo
  2019-08-20 16:47           ` Suman Anna
  0 siblings, 1 reply; 41+ messages in thread
From: Tero Kristo @ 2019-08-20  7:37 UTC (permalink / raw)
  To: Suman Anna, Keerthy, ssantosh, linux-arm-kernel, linux-omap, robh+dt
  Cc: tony, devicetree

On 20.8.2019 2.01, Suman Anna wrote:
> Hi Tero,
> 
> On 8/19/19 4:32 AM, Tero Kristo wrote:
>> On 08/08/2019 08:26, Keerthy wrote:
>>>
>>>
>>> On 07/08/19 1:18 PM, Tero Kristo wrote:
>>>> Add initial PRM (Power and Reset Management) driver for TI OMAP class
>>>> SoCs. Initially this driver only supports reset control, but can be
>>>> extended to support rest of the functionality, like powerdomain
>>>> control, PRCM irq support etc.
>>>>
>>>> Signed-off-by: Tero Kristo <t-kristo@ti.com>
>>>> ---
>>>>    arch/arm/mach-omap2/Kconfig |   1 +
>>>>    drivers/soc/ti/Makefile     |   1 +
>>>>    drivers/soc/ti/omap_prm.c   | 216
>>>> ++++++++++++++++++++++++++++++++++++++++++++
>>>>    3 files changed, 218 insertions(+)
>>>>    create mode 100644 drivers/soc/ti/omap_prm.c
>>>>
>>>> diff --git a/arch/arm/mach-omap2/Kconfig b/arch/arm/mach-omap2/Kconfig
>>>> index fdb6743..42ad063 100644
>>>> --- a/arch/arm/mach-omap2/Kconfig
>>>> +++ b/arch/arm/mach-omap2/Kconfig
>>>> @@ -109,6 +109,7 @@ config ARCH_OMAP2PLUS
>>>>        select TI_SYSC
>>>>        select OMAP_IRQCHIP
>>>>        select CLKSRC_TI_32K
>>>> +    select RESET_CONTROLLER
> 
> Use ARCH_HAS_RESET_CONTROLLER instead.

Ok.

> 
>>>>        help
>>>>          Systems based on OMAP2, OMAP3, OMAP4 or OMAP5
>>>> diff --git a/drivers/soc/ti/Makefile b/drivers/soc/ti/Makefile
>>>> index b3868d3..788b5cd 100644
>>>> --- a/drivers/soc/ti/Makefile
>>>> +++ b/drivers/soc/ti/Makefile
>>>> @@ -6,6 +6,7 @@ obj-$(CONFIG_KEYSTONE_NAVIGATOR_QMSS)    += knav_qmss.o
>>>>    knav_qmss-y := knav_qmss_queue.o knav_qmss_acc.o
>>>>    obj-$(CONFIG_KEYSTONE_NAVIGATOR_DMA)    += knav_dma.o
>>>>    obj-$(CONFIG_AMX3_PM)            += pm33xx.o
>>>> +obj-$(CONFIG_ARCH_OMAP2PLUS)        += omap_prm.o
>>>>    obj-$(CONFIG_WKUP_M3_IPC)        += wkup_m3_ipc.o
>>>>    obj-$(CONFIG_TI_SCI_PM_DOMAINS)        += ti_sci_pm_domains.o
>>>>    obj-$(CONFIG_TI_SCI_INTA_MSI_DOMAIN)    += ti_sci_inta_msi.o
>>>> diff --git a/drivers/soc/ti/omap_prm.c b/drivers/soc/ti/omap_prm.c
>>>> new file mode 100644
>>>> index 0000000..7c89eb8
>>>> --- /dev/null
>>>> +++ b/drivers/soc/ti/omap_prm.c
>>>> @@ -0,0 +1,216 @@
>>>> +// SPDX-License-Identifier: GPL-2.0
>>>> +/*
>>>> + * OMAP2+ PRM driver
>>>> + *
>>>> + * Copyright (C) 2019 Texas Instruments Incorporated -
>>>> http://www.ti.com/
>>>> + *    Tero Kristo <t-kristo@ti.com>
>>>> + */
>>>> +
>>>> +#include <linux/kernel.h>
>>>> +#include <linux/module.h>
>>>> +#include <linux/device.h>
>>>> +#include <linux/io.h>
>>>> +#include <linux/of.h>
>>>> +#include <linux/of_device.h>
>>>> +#include <linux/platform_device.h>
>>>> +#include <linux/reset-controller.h>
>>>> +#include <linux/delay.h>
>>>> +
>>>> +struct omap_rst_map {
>>>> +    s8 rst;
>>>> +    s8 st;
>>>> +};
>>>> +
>>>> +struct omap_prm_data {
>>>> +    u32 base;
>>>> +    const char *name;
>>>> +    u16 pwstctrl;
>>>> +    u16 pwstst;
>>>> +    u16 rstctl;
> 
> Minor nit, can you use rstctrl instead here so that it is in sync with
> the other variables and with the register names used in TRM.

Ok.

> 
>>>> +    u16 rstst;
>>>> +    struct omap_rst_map *rstmap;
>>>> +    u8 flags;
>>>> +};
>>>> +
>>>> +struct omap_prm {
>>>> +    const struct omap_prm_data *data;
>>>> +    void __iomem *base;
>>>> +};
>>>> +
>>>> +struct omap_reset_data {
>>>> +    struct reset_controller_dev rcdev;
>>>> +    struct omap_prm *prm;
>>>> +};
>>>> +
>>>> +#define to_omap_reset_data(p) container_of((p), struct
>>>> omap_reset_data, rcdev)
>>>> +
>>>> +#define OMAP_MAX_RESETS        8
>>>> +#define OMAP_RESET_MAX_WAIT    10000
>>>> +
>>>> +#define OMAP_PRM_NO_RSTST    BIT(0)
>>>> +
>>>> +static const struct of_device_id omap_prm_id_table[] = {
>>>> +    { },
>>>> +};
>>>
>>> This table is blank and we are doing of_match_device against it.
>>
>> Yes, it gets populated with other patches in series, one entry per soc.
>>
>>>
>>>> +
>>>> +static int omap_reset_status(struct reset_controller_dev *rcdev,
>>>> +                 unsigned long id)
>>>> +{
>>>> +    struct omap_reset_data *reset = to_omap_reset_data(rcdev);
>>>> +    u32 v;
>>>> +
>>>> +    v = readl_relaxed(reset->prm->base + reset->prm->data->rstst);
>>>> +    v &= 1 << id;
>>>> +    v >>= id;
>>>> +
>>>> +    return v;
>>>> +}
>>>> +
>>>> +static int omap_reset_assert(struct reset_controller_dev *rcdev,
>>>> +                 unsigned long id)
>>>> +{
>>>> +    struct omap_reset_data *reset = to_omap_reset_data(rcdev);
>>>> +    u32 v;
>>>> +
>>>> +    /* assert the reset control line */
>>>> +    v = readl_relaxed(reset->prm->base + reset->prm->data->rstctl);
>>>> +    v |= 1 << id;
>>>> +    writel_relaxed(v, reset->prm->base + reset->prm->data->rstctl);
>>>> +
>>>> +    return 0;
>>>> +}
>>>> +
>>>> +static int omap_reset_get_st_bit(struct omap_reset_data *reset,
>>>> +                 unsigned long id)
>>>> +{
>>>> +    struct omap_rst_map *map = reset->prm->data->rstmap;
>>>> +
>>>> +    while (map && map->rst >= 0) {
>>>> +        if (map->rst == id)
>>>> +            return map->st;
>>>> +
>>>> +        map++;
>>>> +    }
>>>> +
>>>> +    return id;
>>>> +}
>>>> +
>>>> +/*
>>>> + * Note that status will not change until clocks are on, and clocks
>>>> cannot be
>>>> + * enabled until reset is deasserted. Consumer drivers must check
>>>> status
>>>> + * separately after enabling clocks.
>>>> + */
>>>> +static int omap_reset_deassert(struct reset_controller_dev *rcdev,
>>>> +                   unsigned long id)
>>>> +{
>>>> +    struct omap_reset_data *reset = to_omap_reset_data(rcdev);
>>>> +    u32 v;
>>>> +    int st_bit = id;
> 
> No need to initialize this, will always get overwritten below.

Hmm right, must be a leftover from some earlier code.

> 
>>>> +    bool has_rstst;
>>>> +
>>>> +    /* check the current status to avoid de-asserting the line twice */
>>>> +    v = readl_relaxed(reset->prm->base + reset->prm->data->rstctl);
>>>> +    if (!(v & BIT(id)))
>>>> +        return -EEXIST;
>>>> +
>>>> +    has_rstst = !(reset->prm->data->flags & OMAP_PRM_NO_RSTST);
>>>> +
>>>> +    if (has_rstst) {
>>>> +        st_bit = omap_reset_get_st_bit(reset, id);
>>>> +
>>>> +        /* Clear the reset status by writing 1 to the status bit */
>>>> +        v = readl_relaxed(reset->prm->base + reset->prm->data->rstst);
>>>> +        v |= 1 << st_bit;
>>>> +        writel_relaxed(v, reset->prm->base + reset->prm->data->rstst);
>>>> +    }
>>>> +
>>>> +    /* de-assert the reset control line */
>>>> +    v = readl_relaxed(reset->prm->base + reset->prm->data->rstctl);
>>>> +    v &= ~(1 << id);
>>>> +    writel_relaxed(v, reset->prm->base + reset->prm->data->rstctl);
>>>> +
>>>> +    return 0;
>>>> +}
>>>> +
>>>> +static const struct reset_control_ops omap_reset_ops = {
>>>> +    .assert        = omap_reset_assert,
>>>> +    .deassert    = omap_reset_deassert,
>>>> +    .status        = omap_reset_status,
>>>> +};
>>>> +
>>>> +static int omap_prm_reset_probe(struct platform_device *pdev,
>>>> +                struct omap_prm *prm)
> 
> Call this omap_prm_reset_init or something similar to avoid confusion.

Ok, can change this.

> 
>>>> +{
>>>> +    struct omap_reset_data *reset;
>>>> +
>>>> +    /*
>>>> +     * Check if we have resets. If either rstctl or rstst is
>>>> +     * non-zero, we have reset registers in place. Additionally
>>>> +     * the flag OMAP_PRM_NO_RSTST implies that we have resets.
>>>> +     */
>>>> +    if (!prm->data->rstctl && !prm->data->rstst &&
>>>> +        !(prm->data->flags & OMAP_PRM_NO_RSTST))
>>>> +        return 0;
>>>> +
>>>> +    reset = devm_kzalloc(&pdev->dev, sizeof(*reset), GFP_KERNEL);
>>>> +    if (!reset)
>>>> +        return -ENOMEM;
>>>> +
>>>> +    reset->rcdev.owner = THIS_MODULE;
>>>> +    reset->rcdev.ops = &omap_reset_ops;
>>>> +    reset->rcdev.of_node = pdev->dev.of_node;
>>>> +    reset->rcdev.nr_resets = OMAP_MAX_RESETS;
> 
> Suggest adding a number of resets to prm->data, and using it so that we
> don't even entertain any resets beyond the actual number of resets.

Hmm why bother? Accessing a stale reset bit will just cause access to a 
reserved bit in the reset register, doing basically nothing. Also, this 
would not work for am3/am4 wkup, as there is a single reset bit at an 
arbitrary position.

> 
> You actually seem to be using the bit-position directly in client data
> instead of a reset number. I am not sure if this is accepted practice
> with reset controllers, do you incur any memory wastage?

Reset numbering almost always seems to start from 0, I think the only 
exception to this is wkup_m3 on am3/am4. Introducing an additional 
arbitrary mapping for this doesn't seem to make any sense.

Also, resets are allocated on-need-basis, so it only allocates one 
instance for the reset control whatever the index is.

> 
>>>> +
>>>> +    reset->prm = prm;
>>>> +
>>>> +    return devm_reset_controller_register(&pdev->dev, &reset->rcdev);
>>>> +}
>>>> +
>>>> +static int omap_prm_probe(struct platform_device *pdev)
>>>> +{
>>>> +    struct resource *res;
>>>> +    const struct omap_prm_data *data;
>>>> +    struct omap_prm *prm;
>>>> +    const struct of_device_id *match;
>>>> +
>>>> +    res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>>>> +    if (!res)
>>>> +        return -ENODEV;
>>>> +
>>>> +    match = of_match_device(omap_prm_id_table, &pdev->dev);
>>>> +    if (!match)
>>>> +        return -ENOTSUPP;
> 
> Use of_device_get_match_data() instead to do both match and get the
> data. That can perhaps be the first block.
> 
>>>> +
>>>> +    prm = devm_kzalloc(&pdev->dev, sizeof(*prm), GFP_KERNEL);
>>>> +    if (!prm)
>>>> +        return -ENOMEM;
> 
> Perhaps move the allocate after the match check to streamline.

Ok, will check these two out.

> 
>>>> +
>>>> +    data = match->data;
>>>> +
>>>> +    while (data->base != res->start) {
>>>> +        if (!data->base)
>>>> +            return -EINVAL;
>>>> +        data++;
>>>> +    }
>>>> +
>>>> +    prm->data = data;
>>>> +
>>>> +    prm->base = devm_ioremap_resource(&pdev->dev, res);
>>>> +    if (!prm->base)
>>>> +        return -ENOMEM;
>>>> +
>>>> +    return omap_prm_reset_probe(pdev, prm);
>>>> +}
>>>> +
>>>> +static struct platform_driver omap_prm_driver = {
>>>> +    .probe = omap_prm_probe,
>>>> +    .driver = {
>>>> +        .name        = KBUILD_MODNAME,
>>>> +        .of_match_table    = omap_prm_id_table,
>>>> +    },
>>>> +};
>>>> +builtin_platform_driver(omap_prm_driver);
>>>> +
>>>> +MODULE_ALIAS("platform:prm");
> 
> Drop this and use MODULE_DEVICE_TABLE instead on omap_prm_id_table if
> retaining, but I don't think these need to be defined.

Ok, will ditch them.

-Tero

> 
> regards
> Suman
> 
>>>> +MODULE_LICENSE("GPL v2");
>>>> +MODULE_DESCRIPTION("omap2+ prm driver");
>>>
>>> It is a builtin_platform_driver so do we need the MODULE*?
>>
>> Well, thats debatable, however some existing drivers do introduce this
>> even if they are builtin.
>>
>> -Tero
>> -- 
> 

--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 41+ messages in thread

* Re: [PATCH 1/8] dt-bindings: omap: add new binding for PRM instances
  2019-08-19 21:28       ` Suman Anna
@ 2019-08-20  7:45         ` Tero Kristo
  0 siblings, 0 replies; 41+ messages in thread
From: Tero Kristo @ 2019-08-20  7:45 UTC (permalink / raw)
  To: Suman Anna, Keerthy, ssantosh, linux-arm-kernel, linux-omap, robh+dt
  Cc: tony, devicetree

On 20.8.2019 0.28, Suman Anna wrote:
> Hi Tero,
> 
> On 8/19/19 4:28 AM, Tero Kristo wrote:
>> On 08/08/2019 07:35, Keerthy wrote:
>>>
>>>
>>> On 07/08/19 1:18 PM, Tero Kristo wrote:
>>>> Add new binding for OMAP PRM (Power and Reset Manager) instances. Each
>>>> of these will act as a power domain controller and potentially as a
>>>> reset
>>>> provider.
>>>>
>>>> Signed-off-by: Tero Kristo <t-kristo@ti.com>
>>>> ---
>>>>    .../devicetree/bindings/arm/omap/prm-inst.txt      | 24
>>>> ++++++++++++++++++++++
>>>>    1 file changed, 24 insertions(+)
>>>>    create mode 100644
>>>> Documentation/devicetree/bindings/arm/omap/prm-inst.txt
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/arm/omap/prm-inst.txt
>>>> b/Documentation/devicetree/bindings/arm/omap/prm-inst.txt
>>>> new file mode 100644
>>>> index 0000000..e0ae87b
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/arm/omap/prm-inst.txt
>>>> @@ -0,0 +1,24 @@
>>>> +OMAP PRM instance bindings
>>>> +
>>>> +Power and Reset Manager is an IP block on OMAP family of devices which
>>>> +handle the power domains and their current state, and provide reset
>>>> +handling for the domains and/or separate IP blocks under the power
>>>> domain
>>>> +hierarchy.
> 
> So, I see that you are adding these as flat nodes directly under the prm
> nodes where we have the clocks and clock_domains. Are you anticipating a
> single DT node and/or driver managing both power-domains and resets?

Single DT node + driver for both power-domains / resets. You can 
probably see from the driver code that the reset probing is called 
separately from the main probe function; power domain support should be 
added in similar manner under it.

> 
>>>> +
>>>> +Required properties:
>>>> +- compatible:    Must be one of:
>>>> +        "ti,am3-prm-inst"
>>>> +        "ti,am4-prm-inst"
>>>> +        "ti,omap4-prm-inst"
>>>> +        "ti,omap5-prm-inst"
>>>> +        "ti,dra7-prm-inst"
> 
> What about OMAP2, OMAP3, DM814x, DM816x?

OMAP2/3 architectures are slightly different beast to tackle, so 
planning to work on those later on.

-Tero

> 
> regards
> Suman
> 
>>>> +- reg:        Contains PRM instance register address range
>>>> +        (base address and length)
>>>
>>> How about reset-cells property, Isn't that a mandatory property?
>>
>> It is optional, but you are right, should be added to this.
>>
>> -Tero
>>
>>>
>>>> +
>>>> +Example:
>>>> +
>>>> +prm_dsp2: prm@1b00 {
>>>> +    compatible = "ti,dra7-prm-inst";
>>>> +    reg = <0x1b00 0x40>;
>>>> +    #reset-cells = <1>;
>>>> +};
>>>>
>>
>> -- 
> 

--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 41+ messages in thread

* Re: [PATCH 4/8] soc: ti: omap-prm: add support for denying idle for reset clockdomain
  2019-08-19 23:16   ` Suman Anna
@ 2019-08-20  7:51     ` Tero Kristo
  0 siblings, 0 replies; 41+ messages in thread
From: Tero Kristo @ 2019-08-20  7:51 UTC (permalink / raw)
  To: Suman Anna, ssantosh, linux-arm-kernel, linux-omap, robh+dt
  Cc: tony, devicetree

On 20.8.2019 2.16, Suman Anna wrote:
> Hi Tero,
> 
> On 8/7/19 2:48 AM, Tero Kristo wrote:
>> TI SoCs hardware reset signals require the parent clockdomain to be
>> in force wakeup mode while de-asserting the reset, otherwise it may
>> never complete. To support this, add pdata hooks to control the
>> clockdomain directly.
>>
>> Signed-off-by: Tero Kristo <t-kristo@ti.com>
>> ---
>>   drivers/soc/ti/omap_prm.c            | 32 ++++++++++++++++++++++++++++----
>>   include/linux/platform_data/ti-prm.h | 21 +++++++++++++++++++++
>>   2 files changed, 49 insertions(+), 4 deletions(-)
>>   create mode 100644 include/linux/platform_data/ti-prm.h
>>
>> diff --git a/drivers/soc/ti/omap_prm.c b/drivers/soc/ti/omap_prm.c
>> index d412af3..870515e3 100644
>> --- a/drivers/soc/ti/omap_prm.c
>> +++ b/drivers/soc/ti/omap_prm.c
>> @@ -16,6 +16,8 @@
>>   #include <linux/reset-controller.h>
>>   #include <linux/delay.h>
>>   
>> +#include <linux/platform_data/ti-prm.h>
>> +
>>   struct omap_rst_map {
>>   	s8 rst;
>>   	s8 st;
>> @@ -24,6 +26,7 @@ struct omap_rst_map {
>>   struct omap_prm_data {
>>   	u32 base;
>>   	const char *name;
>> +	const char *clkdm_name;
>>   	u16 pwstctrl;
>>   	u16 pwstst;
>>   	u16 rstctl;
>> @@ -40,6 +43,8 @@ struct omap_prm {
>>   struct omap_reset_data {
>>   	struct reset_controller_dev rcdev;
>>   	struct omap_prm *prm;
>> +	struct clockdomain *clkdm;
>> +	struct device *dev;
>>   };
>>   
>>   #define to_omap_reset_data(p) container_of((p), struct omap_reset_data, rcdev)
>> @@ -108,6 +113,8 @@ static int omap_reset_deassert(struct reset_controller_dev *rcdev,
>>   	int st_bit = id;
>>   	bool has_rstst;
>>   	int timeout = 0;
>> +	struct ti_prm_platform_data *pdata = dev_get_platdata(reset->dev);
>> +	int ret = 0;
>>   
>>   	/* check the current status to avoid de-asserting the line twice */
>>   	v = readl_relaxed(reset->prm->base + reset->prm->data->rstctl);
>> @@ -125,13 +132,16 @@ static int omap_reset_deassert(struct reset_controller_dev *rcdev,
>>   		writel_relaxed(v, reset->prm->base + reset->prm->data->rstst);
>>   	}
>>   
>> +	if (pdata->clkdm_deny_idle && reset->clkdm)
>> +		pdata->clkdm_deny_idle(reset->clkdm);
>> +
>>   	/* de-assert the reset control line */
>>   	v = readl_relaxed(reset->prm->base + reset->prm->data->rstctl);
>>   	v &= ~(1 << id);
>>   	writel_relaxed(v, reset->prm->base + reset->prm->data->rstctl);
>>   
>>   	if (!has_rstst)
>> -		return 0;
>> +		goto exit;
>>   
>>   	/* wait for the status to be set */
>>   	while (1) {
>> @@ -140,13 +150,19 @@ static int omap_reset_deassert(struct reset_controller_dev *rcdev,
>>   		if (v)
>>   			break;
>>   		timeout++;
>> -		if (timeout > OMAP_RESET_MAX_WAIT)
>> -			return -EBUSY;
>> +		if (timeout > OMAP_RESET_MAX_WAIT) {
>> +			ret = -EBUSY;
>> +			goto exit;
>> +		}
>>   
>>   		udelay(1);
>>   	}
>>   
>> -	return 0;
>> +exit:
>> +	if (pdata->clkdm_allow_idle && reset->clkdm)
>> +		pdata->clkdm_allow_idle(reset->clkdm);
>> +
>> +	return ret;
>>   }
>>   
>>   static const struct reset_control_ops omap_reset_ops = {
>> @@ -159,6 +175,8 @@ static int omap_prm_reset_probe(struct platform_device *pdev,
>>   				struct omap_prm *prm)
>>   {
>>   	struct omap_reset_data *reset;
>> +	struct ti_prm_platform_data *pdata = dev_get_platdata(&pdev->dev);
> 
> Please add checks for NULL callbacks. I don't think these are optional
> right, so better to check in init rather than during runtime. Granted
> you will probably not run into this after patch 8, but would be good to
> check and print an error in case pdata quirks is missed out.

Hmm ok, this would get rid of the runtime checks which would be slightly 
cleaner.

> 
>> +	char buf[32];
>>   
>>   	/*
>>   	 * Check if we have resets. If either rstctl or rstst is
>> @@ -177,9 +195,15 @@ static int omap_prm_reset_probe(struct platform_device *pdev,
>>   	reset->rcdev.ops = &omap_reset_ops;
>>   	reset->rcdev.of_node = pdev->dev.of_node;
>>   	reset->rcdev.nr_resets = OMAP_MAX_RESETS;
>> +	reset->dev = &pdev->dev;
>>   
>>   	reset->prm = prm;
>>   
>> +	sprintf(buf, "%s_clkdm", prm->data->clkdm_name ? prm->data->clkdm_name :
>> +		prm->data->name);
>> +
>> +	reset->clkdm = pdata->clkdm_lookup(buf);
> 
> Not checking return status?

clkdm will just end up being NULL in failure, which is fine for rest of 
the code. I don't think it can be ever NULL though, so I can add a check 
here for sanity.

-Tero

> 
> regards
> Suman
> 
>> +
>>   	return devm_reset_controller_register(&pdev->dev, &reset->rcdev);
>>   }
>>   
>> diff --git a/include/linux/platform_data/ti-prm.h b/include/linux/platform_data/ti-prm.h
>> new file mode 100644
>> index 0000000..28154c3
>> --- /dev/null
>> +++ b/include/linux/platform_data/ti-prm.h
>> @@ -0,0 +1,21 @@
>> +/* SPDX-License-Identifier: GPL-2.0-only */
>> +/*
>> + * TI PRM (Power & Reset Manager) platform data
>> + *
>> + * Copyright (C) 2019 Texas Instruments, Inc.
>> + *
>> + * Tero Kristo <t-kristo@ti.com>
>> + */
>> +
>> +#ifndef _LINUX_PLATFORM_DATA_TI_PRM_H
>> +#define _LINUX_PLATFORM_DATA_TI_PRM_H
>> +
>> +struct clockdomain;
>> +
>> +struct ti_prm_platform_data {
>> +	void (*clkdm_deny_idle)(struct clockdomain *clkdm);
>> +	void (*clkdm_allow_idle)(struct clockdomain *clkdm);
>> +	struct clockdomain * (*clkdm_lookup)(const char *name);
>> +};
>> +
>> +#endif /* _LINUX_PLATFORM_DATA_TI_PRM_H */
>>
> 

--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 41+ messages in thread

* Re: [PATCH 5/8] soc: ti: omap-prm: add omap4 PRM data
  2019-08-19 23:08   ` Suman Anna
@ 2019-08-20  7:52     ` Tero Kristo
  2019-08-20 17:23       ` Suman Anna
  0 siblings, 1 reply; 41+ messages in thread
From: Tero Kristo @ 2019-08-20  7:52 UTC (permalink / raw)
  To: Suman Anna, ssantosh, linux-arm-kernel, linux-omap, robh+dt
  Cc: tony, devicetree

On 20.8.2019 2.08, Suman Anna wrote:
> On 8/7/19 2:48 AM, Tero Kristo wrote:
>> Add PRM data for omap4 family of SoCs.
>>
>> Signed-off-by: Tero Kristo <t-kristo@ti.com>
>> ---
>>   drivers/soc/ti/omap_prm.c | 20 ++++++++++++++++++++
>>   1 file changed, 20 insertions(+)
>>
>> diff --git a/drivers/soc/ti/omap_prm.c b/drivers/soc/ti/omap_prm.c
>> index 870515e3..9b8d5945 100644
>> --- a/drivers/soc/ti/omap_prm.c
>> +++ b/drivers/soc/ti/omap_prm.c
>> @@ -54,7 +54,27 @@ struct omap_reset_data {
>>   
>>   #define OMAP_PRM_NO_RSTST	BIT(0)
>>   
>> +struct omap_prm_data omap4_prm_data[] = {
> 
> static const

Will fix this and rest of the similar comments.

-Tero

> 
> regards
> Suman
> 
>> +	{ .name = "mpu", .base = 0x4a306300, .pwstst = 0x4 },
>> +	{ .name = "tesla", .base = 0x4a306400, .pwstst = 0x4, .rstctl = 0x10, .rstst = 0x14 },
>> +	{ .name = "abe", .base = 0x4a306500, .pwstst = 0x4 },
>> +	{ .name = "always_on_core", .base = 0x4a306600, .pwstst = 0x4 },
>> +	{ .name = "core", .base = 0x4a306700, .pwstst = 0x4, .rstctl = 0x210, .rstst = 0x214 },
>> +	{ .name = "ivahd", .base = 0x4a306f00, .pwstst = 0x4, .rstctl = 0x10, .rstst = 0x14 },
>> +	{ .name = "cam", .base = 0x4a307000, .pwstst = 0x4 },
>> +	{ .name = "dss", .base = 0x4a307100, .pwstst = 0x4 },
>> +	{ .name = "gfx", .base = 0x4a307200, .pwstst = 0x4 },
>> +	{ .name = "l3init", .base = 0x4a307300, .pwstst = 0x4 },
>> +	{ .name = "l4per", .base = 0x4a307400, .pwstst = 0x4 },
>> +	{ .name = "cefuse", .base = 0x4a307600, .pwstst = 0x4 },
>> +	{ .name = "wkup", .base = 0x4a307700, .pwstst = 0x4 },
>> +	{ .name = "emu", .base = 0x4a307900, .pwstst = 0x4 },
>> +	{ .name = "device", .base = 0x4a307b00, .rstctl = 0x0, .rstst = 0x4 },
>> +	{ },
>> +};
>> +
>>   static const struct of_device_id omap_prm_id_table[] = {
>> +	{ .compatible = "ti,omap4-prm-inst", .data = omap4_prm_data },
>>   	{ },
>>   };
>>   
>>
> 

--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 41+ messages in thread

* Re: [PATCH 0/8] soc: ti: Add OMAP PRM driver
  2019-08-19 23:20 ` [PATCH 0/8] soc: ti: Add OMAP PRM driver Suman Anna
@ 2019-08-20  7:54   ` Tero Kristo
  2019-08-20 16:51     ` Suman Anna
  0 siblings, 1 reply; 41+ messages in thread
From: Tero Kristo @ 2019-08-20  7:54 UTC (permalink / raw)
  To: Suman Anna, ssantosh, linux-arm-kernel, linux-omap, robh+dt
  Cc: tony, devicetree

On 20.8.2019 2.20, Suman Anna wrote:
> Hi Tero,
> 
> On 8/7/19 2:48 AM, Tero Kristo wrote:
>> Hi,
>>
>> This series adds OMAP PRM driver which initially supports only reset
>> handling. Later on, power domain support can be added to this to get
>> rid of the current OMAP power domain handling code which resides
>> under the mach-omap2 platform directory. Initially, reset data is
>> added for AM3, OMAP4 and DRA7 SoCs.
> 
> Wakeup M3 remoteproc driver is fully upstream, so we should be able to
> test that driver as well if you can add the AM4 data. That will also
> unblock my PRUSS.
> 
> If you can add the data to others as well, it will help in easier
> migration of the individual drivers, otherwise the ti-sysc interconnect,
> hwmod, and hwmod reset data combinations will all have to be supported
> in code.

Ok, so you are saying you would need the PRM data for am4 in addition? I 
can generate that one also.

-Tero

> 
> regards
> Suman
> 
>>
>> I've been testing the reset handling logic with OMAP remoteproc
>> driver which has been converted to use generic reset framework. This
>> part is a work in progress, so will be posting patches from that part
>> later on.
>>
>> -Tero
>>
>> --
>>
> 

--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 41+ messages in thread

* Re: [PATCH 2/8] soc: ti: add initial PRM driver with reset control support
  2019-08-20  7:37         ` Tero Kristo
@ 2019-08-20 16:47           ` Suman Anna
  2019-08-21 15:10             ` Philipp Zabel
  0 siblings, 1 reply; 41+ messages in thread
From: Suman Anna @ 2019-08-20 16:47 UTC (permalink / raw)
  To: Tero Kristo, Keerthy, ssantosh, linux-arm-kernel, linux-omap,
	robh+dt, Philipp Zabel
  Cc: tony, devicetree

On 8/20/19 2:37 AM, Tero Kristo wrote:
> On 20.8.2019 2.01, Suman Anna wrote:
>> Hi Tero,
>>
>> On 8/19/19 4:32 AM, Tero Kristo wrote:
>>> On 08/08/2019 08:26, Keerthy wrote:
>>>>
>>>>
>>>> On 07/08/19 1:18 PM, Tero Kristo wrote:
>>>>> Add initial PRM (Power and Reset Management) driver for TI OMAP class
>>>>> SoCs. Initially this driver only supports reset control, but can be
>>>>> extended to support rest of the functionality, like powerdomain
>>>>> control, PRCM irq support etc.
>>>>>
>>>>> Signed-off-by: Tero Kristo <t-kristo@ti.com>
>>>>> ---
>>>>>    arch/arm/mach-omap2/Kconfig |   1 +
>>>>>    drivers/soc/ti/Makefile     |   1 +
>>>>>    drivers/soc/ti/omap_prm.c   | 216
>>>>> ++++++++++++++++++++++++++++++++++++++++++++
>>>>>    3 files changed, 218 insertions(+)
>>>>>    create mode 100644 drivers/soc/ti/omap_prm.c
>>>>>
>>>>> diff --git a/arch/arm/mach-omap2/Kconfig b/arch/arm/mach-omap2/Kconfig
>>>>> index fdb6743..42ad063 100644
>>>>> --- a/arch/arm/mach-omap2/Kconfig
>>>>> +++ b/arch/arm/mach-omap2/Kconfig
>>>>> @@ -109,6 +109,7 @@ config ARCH_OMAP2PLUS
>>>>>        select TI_SYSC
>>>>>        select OMAP_IRQCHIP
>>>>>        select CLKSRC_TI_32K
>>>>> +    select RESET_CONTROLLER
>>
>> Use ARCH_HAS_RESET_CONTROLLER instead.
> 
> Ok.
> 
>>
>>>>>        help
>>>>>          Systems based on OMAP2, OMAP3, OMAP4 or OMAP5
>>>>> diff --git a/drivers/soc/ti/Makefile b/drivers/soc/ti/Makefile
>>>>> index b3868d3..788b5cd 100644
>>>>> --- a/drivers/soc/ti/Makefile
>>>>> +++ b/drivers/soc/ti/Makefile
>>>>> @@ -6,6 +6,7 @@ obj-$(CONFIG_KEYSTONE_NAVIGATOR_QMSS)    +=
>>>>> knav_qmss.o
>>>>>    knav_qmss-y := knav_qmss_queue.o knav_qmss_acc.o
>>>>>    obj-$(CONFIG_KEYSTONE_NAVIGATOR_DMA)    += knav_dma.o
>>>>>    obj-$(CONFIG_AMX3_PM)            += pm33xx.o
>>>>> +obj-$(CONFIG_ARCH_OMAP2PLUS)        += omap_prm.o
>>>>>    obj-$(CONFIG_WKUP_M3_IPC)        += wkup_m3_ipc.o
>>>>>    obj-$(CONFIG_TI_SCI_PM_DOMAINS)        += ti_sci_pm_domains.o
>>>>>    obj-$(CONFIG_TI_SCI_INTA_MSI_DOMAIN)    += ti_sci_inta_msi.o
>>>>> diff --git a/drivers/soc/ti/omap_prm.c b/drivers/soc/ti/omap_prm.c
>>>>> new file mode 100644
>>>>> index 0000000..7c89eb8
>>>>> --- /dev/null
>>>>> +++ b/drivers/soc/ti/omap_prm.c
>>>>> @@ -0,0 +1,216 @@
>>>>> +// SPDX-License-Identifier: GPL-2.0
>>>>> +/*
>>>>> + * OMAP2+ PRM driver
>>>>> + *
>>>>> + * Copyright (C) 2019 Texas Instruments Incorporated -
>>>>> http://www.ti.com/
>>>>> + *    Tero Kristo <t-kristo@ti.com>
>>>>> + */
>>>>> +
>>>>> +#include <linux/kernel.h>
>>>>> +#include <linux/module.h>
>>>>> +#include <linux/device.h>
>>>>> +#include <linux/io.h>
>>>>> +#include <linux/of.h>
>>>>> +#include <linux/of_device.h>
>>>>> +#include <linux/platform_device.h>
>>>>> +#include <linux/reset-controller.h>
>>>>> +#include <linux/delay.h>
>>>>> +
>>>>> +struct omap_rst_map {
>>>>> +    s8 rst;
>>>>> +    s8 st;
>>>>> +};
>>>>> +
>>>>> +struct omap_prm_data {
>>>>> +    u32 base;
>>>>> +    const char *name;
>>>>> +    u16 pwstctrl;
>>>>> +    u16 pwstst;
>>>>> +    u16 rstctl;
>>
>> Minor nit, can you use rstctrl instead here so that it is in sync with
>> the other variables and with the register names used in TRM.
> 
> Ok.
> 
>>
>>>>> +    u16 rstst;
>>>>> +    struct omap_rst_map *rstmap;
>>>>> +    u8 flags;
>>>>> +};
>>>>> +
>>>>> +struct omap_prm {
>>>>> +    const struct omap_prm_data *data;
>>>>> +    void __iomem *base;
>>>>> +};
>>>>> +
>>>>> +struct omap_reset_data {
>>>>> +    struct reset_controller_dev rcdev;
>>>>> +    struct omap_prm *prm;
>>>>> +};
>>>>> +
>>>>> +#define to_omap_reset_data(p) container_of((p), struct
>>>>> omap_reset_data, rcdev)
>>>>> +
>>>>> +#define OMAP_MAX_RESETS        8
>>>>> +#define OMAP_RESET_MAX_WAIT    10000
>>>>> +
>>>>> +#define OMAP_PRM_NO_RSTST    BIT(0)
>>>>> +
>>>>> +static const struct of_device_id omap_prm_id_table[] = {
>>>>> +    { },
>>>>> +};
>>>>
>>>> This table is blank and we are doing of_match_device against it.
>>>
>>> Yes, it gets populated with other patches in series, one entry per soc.
>>>
>>>>
>>>>> +
>>>>> +static int omap_reset_status(struct reset_controller_dev *rcdev,
>>>>> +                 unsigned long id)
>>>>> +{
>>>>> +    struct omap_reset_data *reset = to_omap_reset_data(rcdev);
>>>>> +    u32 v;
>>>>> +
>>>>> +    v = readl_relaxed(reset->prm->base + reset->prm->data->rstst);
>>>>> +    v &= 1 << id;
>>>>> +    v >>= id;
>>>>> +
>>>>> +    return v;
>>>>> +}
>>>>> +
>>>>> +static int omap_reset_assert(struct reset_controller_dev *rcdev,
>>>>> +                 unsigned long id)
>>>>> +{
>>>>> +    struct omap_reset_data *reset = to_omap_reset_data(rcdev);
>>>>> +    u32 v;
>>>>> +
>>>>> +    /* assert the reset control line */
>>>>> +    v = readl_relaxed(reset->prm->base + reset->prm->data->rstctl);
>>>>> +    v |= 1 << id;
>>>>> +    writel_relaxed(v, reset->prm->base + reset->prm->data->rstctl);
>>>>> +
>>>>> +    return 0;
>>>>> +}
>>>>> +
>>>>> +static int omap_reset_get_st_bit(struct omap_reset_data *reset,
>>>>> +                 unsigned long id)
>>>>> +{
>>>>> +    struct omap_rst_map *map = reset->prm->data->rstmap;
>>>>> +
>>>>> +    while (map && map->rst >= 0) {
>>>>> +        if (map->rst == id)
>>>>> +            return map->st;
>>>>> +
>>>>> +        map++;
>>>>> +    }
>>>>> +
>>>>> +    return id;
>>>>> +}
>>>>> +
>>>>> +/*
>>>>> + * Note that status will not change until clocks are on, and clocks
>>>>> cannot be
>>>>> + * enabled until reset is deasserted. Consumer drivers must check
>>>>> status
>>>>> + * separately after enabling clocks.
>>>>> + */
>>>>> +static int omap_reset_deassert(struct reset_controller_dev *rcdev,
>>>>> +                   unsigned long id)
>>>>> +{
>>>>> +    struct omap_reset_data *reset = to_omap_reset_data(rcdev);
>>>>> +    u32 v;
>>>>> +    int st_bit = id;
>>
>> No need to initialize this, will always get overwritten below.
> 
> Hmm right, must be a leftover from some earlier code.
> 
>>
>>>>> +    bool has_rstst;
>>>>> +
>>>>> +    /* check the current status to avoid de-asserting the line
>>>>> twice */
>>>>> +    v = readl_relaxed(reset->prm->base + reset->prm->data->rstctl);
>>>>> +    if (!(v & BIT(id)))
>>>>> +        return -EEXIST;
>>>>> +
>>>>> +    has_rstst = !(reset->prm->data->flags & OMAP_PRM_NO_RSTST);
>>>>> +
>>>>> +    if (has_rstst) {
>>>>> +        st_bit = omap_reset_get_st_bit(reset, id);
>>>>> +
>>>>> +        /* Clear the reset status by writing 1 to the status bit */
>>>>> +        v = readl_relaxed(reset->prm->base +
>>>>> reset->prm->data->rstst);
>>>>> +        v |= 1 << st_bit;
>>>>> +        writel_relaxed(v, reset->prm->base +
>>>>> reset->prm->data->rstst);
>>>>> +    }
>>>>> +
>>>>> +    /* de-assert the reset control line */
>>>>> +    v = readl_relaxed(reset->prm->base + reset->prm->data->rstctl);
>>>>> +    v &= ~(1 << id);
>>>>> +    writel_relaxed(v, reset->prm->base + reset->prm->data->rstctl);
>>>>> +
>>>>> +    return 0;
>>>>> +}
>>>>> +
>>>>> +static const struct reset_control_ops omap_reset_ops = {
>>>>> +    .assert        = omap_reset_assert,
>>>>> +    .deassert    = omap_reset_deassert,
>>>>> +    .status        = omap_reset_status,
>>>>> +};
>>>>> +
>>>>> +static int omap_prm_reset_probe(struct platform_device *pdev,
>>>>> +                struct omap_prm *prm)
>>
>> Call this omap_prm_reset_init or something similar to avoid confusion.
> 
> Ok, can change this.
> 
>>
>>>>> +{
>>>>> +    struct omap_reset_data *reset;
>>>>> +
>>>>> +    /*
>>>>> +     * Check if we have resets. If either rstctl or rstst is
>>>>> +     * non-zero, we have reset registers in place. Additionally
>>>>> +     * the flag OMAP_PRM_NO_RSTST implies that we have resets.
>>>>> +     */
>>>>> +    if (!prm->data->rstctl && !prm->data->rstst &&
>>>>> +        !(prm->data->flags & OMAP_PRM_NO_RSTST))
>>>>> +        return 0;
>>>>> +
>>>>> +    reset = devm_kzalloc(&pdev->dev, sizeof(*reset), GFP_KERNEL);
>>>>> +    if (!reset)
>>>>> +        return -ENOMEM;
>>>>> +
>>>>> +    reset->rcdev.owner = THIS_MODULE;
>>>>> +    reset->rcdev.ops = &omap_reset_ops;
>>>>> +    reset->rcdev.of_node = pdev->dev.of_node;
>>>>> +    reset->rcdev.nr_resets = OMAP_MAX_RESETS;
>>
>> Suggest adding a number of resets to prm->data, and using it so that we
>> don't even entertain any resets beyond the actual number of resets.
> 
> Hmm why bother? Accessing a stale reset bit will just cause access to a
> reserved bit in the reset register, doing basically nothing. Also, this
> would not work for am3/am4 wkup, as there is a single reset bit at an
> arbitrary position.

The generic convention seems to be defining a reset id value defined
from include/dt-bindings/reset/ that can be used to match between the
dt-nodes and the reset-controller driver.

Philipp,
Any comments?

regards
Suman

> 
>>
>> You actually seem to be using the bit-position directly in client data
>> instead of a reset number. I am not sure if this is accepted practice
>> with reset controllers, do you incur any memory wastage?
> 
> Reset numbering almost always seems to start from 0, I think the only
> exception to this is wkup_m3 on am3/am4. Introducing an additional
> arbitrary mapping for this doesn't seem to make any sense.
> 
> Also, resets are allocated on-need-basis, so it only allocates one
> instance for the reset control whatever the index is.
> 
>>
>>>>> +
>>>>> +    reset->prm = prm;
>>>>> +
>>>>> +    return devm_reset_controller_register(&pdev->dev, &reset->rcdev);
>>>>> +}
>>>>> +
>>>>> +static int omap_prm_probe(struct platform_device *pdev)
>>>>> +{
>>>>> +    struct resource *res;
>>>>> +    const struct omap_prm_data *data;
>>>>> +    struct omap_prm *prm;
>>>>> +    const struct of_device_id *match;
>>>>> +
>>>>> +    res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>>>>> +    if (!res)
>>>>> +        return -ENODEV;
>>>>> +
>>>>> +    match = of_match_device(omap_prm_id_table, &pdev->dev);
>>>>> +    if (!match)
>>>>> +        return -ENOTSUPP;
>>
>> Use of_device_get_match_data() instead to do both match and get the
>> data. That can perhaps be the first block.
>>
>>>>> +
>>>>> +    prm = devm_kzalloc(&pdev->dev, sizeof(*prm), GFP_KERNEL);
>>>>> +    if (!prm)
>>>>> +        return -ENOMEM;
>>
>> Perhaps move the allocate after the match check to streamline.
> 
> Ok, will check these two out.
> 
>>
>>>>> +
>>>>> +    data = match->data;
>>>>> +
>>>>> +    while (data->base != res->start) {
>>>>> +        if (!data->base)
>>>>> +            return -EINVAL;
>>>>> +        data++;
>>>>> +    }
>>>>> +
>>>>> +    prm->data = data;
>>>>> +
>>>>> +    prm->base = devm_ioremap_resource(&pdev->dev, res);
>>>>> +    if (!prm->base)
>>>>> +        return -ENOMEM;
>>>>> +
>>>>> +    return omap_prm_reset_probe(pdev, prm);
>>>>> +}
>>>>> +
>>>>> +static struct platform_driver omap_prm_driver = {
>>>>> +    .probe = omap_prm_probe,
>>>>> +    .driver = {
>>>>> +        .name        = KBUILD_MODNAME,
>>>>> +        .of_match_table    = omap_prm_id_table,
>>>>> +    },
>>>>> +};
>>>>> +builtin_platform_driver(omap_prm_driver);
>>>>> +
>>>>> +MODULE_ALIAS("platform:prm");
>>
>> Drop this and use MODULE_DEVICE_TABLE instead on omap_prm_id_table if
>> retaining, but I don't think these need to be defined.
> 
> Ok, will ditch them.
> 
> -Tero
> 
>>
>> regards
>> Suman
>>
>>>>> +MODULE_LICENSE("GPL v2");
>>>>> +MODULE_DESCRIPTION("omap2+ prm driver");
>>>>
>>>> It is a builtin_platform_driver so do we need the MODULE*?
>>>
>>> Well, thats debatable, however some existing drivers do introduce this
>>> even if they are builtin.
>>>
>>> -Tero
>>> -- 
>>
> 
> -- 
> Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
> Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 41+ messages in thread

* Re: [PATCH 0/8] soc: ti: Add OMAP PRM driver
  2019-08-20  7:54   ` Tero Kristo
@ 2019-08-20 16:51     ` Suman Anna
  0 siblings, 0 replies; 41+ messages in thread
From: Suman Anna @ 2019-08-20 16:51 UTC (permalink / raw)
  To: Tero Kristo, ssantosh, linux-arm-kernel, linux-omap, robh+dt
  Cc: tony, devicetree

On 8/20/19 2:54 AM, Tero Kristo wrote:
> On 20.8.2019 2.20, Suman Anna wrote:
>> Hi Tero,
>>
>> On 8/7/19 2:48 AM, Tero Kristo wrote:
>>> Hi,
>>>
>>> This series adds OMAP PRM driver which initially supports only reset
>>> handling. Later on, power domain support can be added to this to get
>>> rid of the current OMAP power domain handling code which resides
>>> under the mach-omap2 platform directory. Initially, reset data is
>>> added for AM3, OMAP4 and DRA7 SoCs.
>>
>> Wakeup M3 remoteproc driver is fully upstream, so we should be able to
>> test that driver as well if you can add the AM4 data. That will also
>> unblock my PRUSS.
>>
>> If you can add the data to others as well, it will help in easier
>> migration of the individual drivers, otherwise the ti-sysc interconnect,
>> hwmod, and hwmod reset data combinations will all have to be supported
>> in code.
> 
> Ok, so you are saying you would need the PRM data for am4 in addition? I
> can generate that one also.

Yes, if you can include the data for AM4 and OMAP5 as well, then we can
convert all the SoCs other than OMAP2/OMAP3 at the same time as per your
comment on bindings. Almost all of the active remoteprocs will be
covered by these.

OMAP3 ISP driver is also fully upstream, so we would have to manage its
legacy compatibility.

regards
Suman

> 
> -Tero
> 
>>
>> regards
>> Suman
>>
>>>
>>> I've been testing the reset handling logic with OMAP remoteproc
>>> driver which has been converted to use generic reset framework. This
>>> part is a work in progress, so will be posting patches from that part
>>> later on.
>>>
>>> -Tero
>>>
>>> -- 
>>>
>>
> 
> -- 
> Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
> Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 41+ messages in thread

* Re: [PATCH 5/8] soc: ti: omap-prm: add omap4 PRM data
  2019-08-20  7:52     ` Tero Kristo
@ 2019-08-20 17:23       ` Suman Anna
  2019-08-21  6:38         ` Tero Kristo
  0 siblings, 1 reply; 41+ messages in thread
From: Suman Anna @ 2019-08-20 17:23 UTC (permalink / raw)
  To: Tero Kristo, ssantosh, linux-arm-kernel, linux-omap, robh+dt
  Cc: tony, devicetree

On 8/20/19 2:52 AM, Tero Kristo wrote:
> On 20.8.2019 2.08, Suman Anna wrote:
>> On 8/7/19 2:48 AM, Tero Kristo wrote:
>>> Add PRM data for omap4 family of SoCs.
>>>
>>> Signed-off-by: Tero Kristo <t-kristo@ti.com>
>>> ---
>>>   drivers/soc/ti/omap_prm.c | 20 ++++++++++++++++++++
>>>   1 file changed, 20 insertions(+)
>>>
>>> diff --git a/drivers/soc/ti/omap_prm.c b/drivers/soc/ti/omap_prm.c
>>> index 870515e3..9b8d5945 100644
>>> --- a/drivers/soc/ti/omap_prm.c
>>> +++ b/drivers/soc/ti/omap_prm.c
>>> @@ -54,7 +54,27 @@ struct omap_reset_data {
>>>     #define OMAP_PRM_NO_RSTST    BIT(0)
>>>   +struct omap_prm_data omap4_prm_data[] = {
>>
>> static const
> 
> Will fix this and rest of the similar comments.
> 
> -Tero
> 
>>
>> regards
>> Suman
>>
>>> +    { .name = "mpu", .base = 0x4a306300, .pwstst = 0x4 },
>>> +    { .name = "tesla", .base = 0x4a306400, .pwstst = 0x4, .rstctl =
>>> 0x10, .rstst = 0x14 },
>>> +    { .name = "abe", .base = 0x4a306500, .pwstst = 0x4 },
>>> +    { .name = "always_on_core", .base = 0x4a306600, .pwstst = 0x4 },
>>> +    { .name = "core", .base = 0x4a306700, .pwstst = 0x4, .rstctl =
>>> 0x210, .rstst = 0x214 },
>>> +    { .name = "ivahd", .base = 0x4a306f00, .pwstst = 0x4, .rstctl =
>>> 0x10, .rstst = 0x14 },
>>> +    { .name = "cam", .base = 0x4a307000, .pwstst = 0x4 },
>>> +    { .name = "dss", .base = 0x4a307100, .pwstst = 0x4 },
>>> +    { .name = "gfx", .base = 0x4a307200, .pwstst = 0x4 },
>>> +    { .name = "l3init", .base = 0x4a307300, .pwstst = 0x4 },
>>> +    { .name = "l4per", .base = 0x4a307400, .pwstst = 0x4 },
>>> +    { .name = "cefuse", .base = 0x4a307600, .pwstst = 0x4 },
>>> +    { .name = "wkup", .base = 0x4a307700, .pwstst = 0x4 },
>>> +    { .name = "emu", .base = 0x4a307900, .pwstst = 0x4 },
>>> +    { .name = "device", .base = 0x4a307b00, .rstctl = 0x0, .rstst =
>>> 0x4 },

So, looks like you are using pwstctrl as 0 by default, but some of them
will neither have pwstctrl or pwstst like "device" PRM here. Is the plan
to use -1 for the fields, or a flags field?

regards
Suman

>>> +    { },
>>> +};
>>> +
>>>   static const struct of_device_id omap_prm_id_table[] = {
>>> +    { .compatible = "ti,omap4-prm-inst", .data = omap4_prm_data },
>>>       { },
>>>   };
>>>  
>>
> 
> -- 
> Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
> Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 41+ messages in thread

* Re: [PATCH 6/8] soc: ti: omap_prm: add data for am33xx
  2019-08-07  7:48 ` [PATCH 6/8] soc: ti: omap_prm: add data for am33xx Tero Kristo
  2019-08-19 23:11   ` Suman Anna
@ 2019-08-20 18:48   ` Suman Anna
  2019-08-21  7:23     ` Tero Kristo
  1 sibling, 1 reply; 41+ messages in thread
From: Suman Anna @ 2019-08-20 18:48 UTC (permalink / raw)
  To: Tero Kristo, ssantosh, linux-arm-kernel, linux-omap, robh+dt
  Cc: tony, devicetree

Hi Tero,

On 8/7/19 2:48 AM, Tero Kristo wrote:
> Add PRM instance data for AM33xx SoC. Includes some basic register
> definitions and reset data for now.
> 
> Signed-off-by: Tero Kristo <t-kristo@ti.com>
> ---
>  drivers/soc/ti/omap_prm.c | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
> 
> diff --git a/drivers/soc/ti/omap_prm.c b/drivers/soc/ti/omap_prm.c
> index 9b8d5945..fadfc7f 100644
> --- a/drivers/soc/ti/omap_prm.c
> +++ b/drivers/soc/ti/omap_prm.c
> @@ -73,8 +73,25 @@ struct omap_prm_data omap4_prm_data[] = {
>  	{ },
>  };
>  
> +struct omap_rst_map am3_wkup_rst_map[] = {
> +	{ .rst = 3, .st = 5 },
> +	{ .rst = -1 },
> +};
> +
> +struct omap_prm_data am3_prm_data[] = {
> +	{ .name = "per", .base = 0x44e00c00, .pwstctrl = 0xc, .pwstst = 0x8, .flags = OMAP_PRM_NO_RSTST },
> +	{ .name = "wkup", .base = 0x44e00d00, .pwstctrl = 0x4, .pwstst = 0x8, .rstst = 0xc, .rstmap = am3_wkup_rst_map },
> +	{ .name = "mpu", .base = 0x44e00e00, .pwstst = 0x4 },

Has a rstst but no rstctrl, but your registration logic takes care of
this. Somewhat confusing, when you just look at the data. Should you
limit the check to only rstctrl and OMAP_PRM_NO_RSTST?

> +	{ .name = "device", .base = 0x44e00f00, .rstctl = 0x0, .rstst = 0x8 },

No pwrstctrl and pwrstst registers, so same comment as on OMAP4 data.

> +	{ .name = "rtc", .base = 0x44e01000, .pwstst = 0x4 },
> +	{ .name = "gfx", .base = 0x44e01100, .pwstst = 0x10, .rstctl = 0x4, .rstst = 0x14 },
> +	{ .name = "cefuse", .base = 0x44e01200, .pwstst = 0x4 },

I am not sure if it is better to explicitly list the registers at 0
offset rather than using the implied value of 0, since there are some
registers that do not exist on some PRM instances which are also not
defined.

regards
Suman

> +	{ },
> +};
> +
>  static const struct of_device_id omap_prm_id_table[] = {
>  	{ .compatible = "ti,omap4-prm-inst", .data = omap4_prm_data },
> +	{ .compatible = "ti,am3-prm-inst", .data = am3_prm_data },
>  	{ },
>  };
>  
> 


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 41+ messages in thread

* Re: [PATCH 7/8] soc: ti: omap-prm: add dra7 PRM data
  2019-08-07  7:48 ` [PATCH 7/8] soc: ti: omap-prm: add dra7 PRM data Tero Kristo
  2019-08-19 23:12   ` Suman Anna
@ 2019-08-20 19:03   ` Suman Anna
  2019-08-21  7:36     ` Tero Kristo
  1 sibling, 1 reply; 41+ messages in thread
From: Suman Anna @ 2019-08-20 19:03 UTC (permalink / raw)
  To: Tero Kristo, ssantosh, linux-arm-kernel, linux-omap, robh+dt
  Cc: tony, devicetree

Hi Tero,

On 8/7/19 2:48 AM, Tero Kristo wrote:
> Add PRM data for dra7 family of SoCs.
> 
> Signed-off-by: Tero Kristo <t-kristo@ti.com>
> ---
>  drivers/soc/ti/omap_prm.c | 26 ++++++++++++++++++++++++++
>  1 file changed, 26 insertions(+)
> 
> diff --git a/drivers/soc/ti/omap_prm.c b/drivers/soc/ti/omap_prm.c
> index fadfc7f..05b7749 100644
> --- a/drivers/soc/ti/omap_prm.c
> +++ b/drivers/soc/ti/omap_prm.c
> @@ -73,6 +73,31 @@ struct omap_prm_data omap4_prm_data[] = {
>  	{ },
>  };
>  
> +static struct omap_prm_data dra7_prm_data[] = {
> +	{ .name = "mpu", .base = 0x4ae06300, .pwstst = 0x4 },
> +	{ .name = "dsp1", .base = 0x4ae06400, .pwstst = 0x4, .rstctl = 0x10, .rstst = 0x14 },
> +	{ .name = "ipu", .base = 0x4ae06500, .pwstst = 0x4, .rstctl = 0x10, .rstst = 0x14, .clkdm_name = "ipu1" },
> +	{ .name = "coreaon", .base = 0x4ae06628, .pwstst = 0x4 },

Public TRM marks this region Reserved. Do you need it for anything?

> +	{ .name = "core", .base = 0x4ae06700, .pwstst = 0x4, .rstctl = 0x210, .rstst = 0x214, .clkdm_name = "ipu2" },
> +	{ .name = "iva", .base = 0x4ae06f00, .pwstst = 0x4 },

Missing rstctrl and rstst offsets.

> +	{ .name = "cam", .base = 0x4ae07000, .pwstst = 0x4 },
> +	{ .name = "dss", .base = 0x4ae07100, .pwstst = 0x4 },
> +	{ .name = "gpu", .base = 0x4ae07200, .pwstst = 0x4 },
> +	{ .name = "l3init", .base = 0x4ae07300, .pwstst = 0x4, .rstctl = 0x10, .rstst = 0x14 },
> +	{ .name = "l4per", .base = 0x4ae07400, .pwstst = 0x4 },
> +	{ .name = "custefuse", .base = 0x4ae07600, .pwstst = 0x4 },
> +	{ .name = "wkupaon", .base = 0x4ae07724, .pwstst = 0x4 },

No pwstctrl and pwstst bits documented in TRM or are marked reserved.

> +	{ .name = "emu", .base = 0x4ae07900, .pwstst = 0x4 },
> +	{ .name = "dsp2", .base = 0x4ae07b00, .pwstst = 0x4, .rstctl = 0x10, .rstst = 0x14 },
> +	{ .name = "eve1", .base = 0x4ae07b40, .pwstst = 0x4 },
> +	{ .name = "eve2", .base = 0x4ae07b80, .pwstst = 0x4 },
> +	{ .name = "eve3", .base = 0x4ae07bc0, .pwstst = 0x4 },
> +	{ .name = "eve4", .base = 0x4ae07c00, .pwstst = 0x4 },

All EVEs are missing rstctrl and rstst fields.

> +	{ .name = "rtc", .base = 0x4ae07c60, .pwstst = 0x4 },

Undocumented pwstctrl and pwstst registers.

> +	{ .name = "vpe", .base = 0x4ae07c80, .pwstst = 0x4 },

Missing "device" and "instr" PRM. The latter doesn't have any pwrstctl
and pwrstst though.

regards
Suman

> +	{ },
> +};
> +
>  struct omap_rst_map am3_wkup_rst_map[] = {
>  	{ .rst = 3, .st = 5 },
>  	{ .rst = -1 },
> @@ -91,6 +116,7 @@ struct omap_prm_data am3_prm_data[] = {
>  
>  static const struct of_device_id omap_prm_id_table[] = {
>  	{ .compatible = "ti,omap4-prm-inst", .data = omap4_prm_data },
> +	{ .compatible = "ti,dra7-prm-inst", .data = dra7_prm_data },
>  	{ .compatible = "ti,am3-prm-inst", .data = am3_prm_data },
>  	{ },
>  };
> 


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 41+ messages in thread

* Re: [PATCH 5/8] soc: ti: omap-prm: add omap4 PRM data
  2019-08-20 17:23       ` Suman Anna
@ 2019-08-21  6:38         ` Tero Kristo
  0 siblings, 0 replies; 41+ messages in thread
From: Tero Kristo @ 2019-08-21  6:38 UTC (permalink / raw)
  To: Suman Anna, ssantosh, linux-arm-kernel, linux-omap, robh+dt
  Cc: tony, devicetree

On 20.8.2019 20.23, Suman Anna wrote:
> On 8/20/19 2:52 AM, Tero Kristo wrote:
>> On 20.8.2019 2.08, Suman Anna wrote:
>>> On 8/7/19 2:48 AM, Tero Kristo wrote:
>>>> Add PRM data for omap4 family of SoCs.
>>>>
>>>> Signed-off-by: Tero Kristo <t-kristo@ti.com>
>>>> ---
>>>>    drivers/soc/ti/omap_prm.c | 20 ++++++++++++++++++++
>>>>    1 file changed, 20 insertions(+)
>>>>
>>>> diff --git a/drivers/soc/ti/omap_prm.c b/drivers/soc/ti/omap_prm.c
>>>> index 870515e3..9b8d5945 100644
>>>> --- a/drivers/soc/ti/omap_prm.c
>>>> +++ b/drivers/soc/ti/omap_prm.c
>>>> @@ -54,7 +54,27 @@ struct omap_reset_data {
>>>>      #define OMAP_PRM_NO_RSTST    BIT(0)
>>>>    +struct omap_prm_data omap4_prm_data[] = {
>>>
>>> static const
>>
>> Will fix this and rest of the similar comments.
>>
>> -Tero
>>
>>>
>>> regards
>>> Suman
>>>
>>>> +    { .name = "mpu", .base = 0x4a306300, .pwstst = 0x4 },
>>>> +    { .name = "tesla", .base = 0x4a306400, .pwstst = 0x4, .rstctl =
>>>> 0x10, .rstst = 0x14 },
>>>> +    { .name = "abe", .base = 0x4a306500, .pwstst = 0x4 },
>>>> +    { .name = "always_on_core", .base = 0x4a306600, .pwstst = 0x4 },
>>>> +    { .name = "core", .base = 0x4a306700, .pwstst = 0x4, .rstctl =
>>>> 0x210, .rstst = 0x214 },
>>>> +    { .name = "ivahd", .base = 0x4a306f00, .pwstst = 0x4, .rstctl =
>>>> 0x10, .rstst = 0x14 },
>>>> +    { .name = "cam", .base = 0x4a307000, .pwstst = 0x4 },
>>>> +    { .name = "dss", .base = 0x4a307100, .pwstst = 0x4 },
>>>> +    { .name = "gfx", .base = 0x4a307200, .pwstst = 0x4 },
>>>> +    { .name = "l3init", .base = 0x4a307300, .pwstst = 0x4 },
>>>> +    { .name = "l4per", .base = 0x4a307400, .pwstst = 0x4 },
>>>> +    { .name = "cefuse", .base = 0x4a307600, .pwstst = 0x4 },
>>>> +    { .name = "wkup", .base = 0x4a307700, .pwstst = 0x4 },
>>>> +    { .name = "emu", .base = 0x4a307900, .pwstst = 0x4 },
>>>> +    { .name = "device", .base = 0x4a307b00, .rstctl = 0x0, .rstst =
>>>> 0x4 },
> 
> So, looks like you are using pwstctrl as 0 by default, but some of them
> will neither have pwstctrl or pwstst like "device" PRM here. Is the plan
> to use -1 for the fields, or a flags field?

Multiple paths are possible, I will see what makes most sense once I 
implement it.

-Tero

> 
> regards
> Suman
> 
>>>> +    { },
>>>> +};
>>>> +
>>>>    static const struct of_device_id omap_prm_id_table[] = {
>>>> +    { .compatible = "ti,omap4-prm-inst", .data = omap4_prm_data },
>>>>        { },
>>>>    };
>>>>   
>>>
>>
>> -- 
> 

--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 41+ messages in thread

* Re: [PATCH 6/8] soc: ti: omap_prm: add data for am33xx
  2019-08-20 18:48   ` Suman Anna
@ 2019-08-21  7:23     ` Tero Kristo
  2019-08-21 15:49       ` Suman Anna
  0 siblings, 1 reply; 41+ messages in thread
From: Tero Kristo @ 2019-08-21  7:23 UTC (permalink / raw)
  To: Suman Anna, ssantosh, linux-arm-kernel, linux-omap, robh+dt
  Cc: tony, devicetree

On 20.8.2019 21.48, Suman Anna wrote:
> Hi Tero,
> 
> On 8/7/19 2:48 AM, Tero Kristo wrote:
>> Add PRM instance data for AM33xx SoC. Includes some basic register
>> definitions and reset data for now.
>>
>> Signed-off-by: Tero Kristo <t-kristo@ti.com>
>> ---
>>   drivers/soc/ti/omap_prm.c | 17 +++++++++++++++++
>>   1 file changed, 17 insertions(+)
>>
>> diff --git a/drivers/soc/ti/omap_prm.c b/drivers/soc/ti/omap_prm.c
>> index 9b8d5945..fadfc7f 100644
>> --- a/drivers/soc/ti/omap_prm.c
>> +++ b/drivers/soc/ti/omap_prm.c
>> @@ -73,8 +73,25 @@ struct omap_prm_data omap4_prm_data[] = {
>>   	{ },
>>   };
>>   
>> +struct omap_rst_map am3_wkup_rst_map[] = {
>> +	{ .rst = 3, .st = 5 },
>> +	{ .rst = -1 },
>> +};
>> +
>> +struct omap_prm_data am3_prm_data[] = {
>> +	{ .name = "per", .base = 0x44e00c00, .pwstctrl = 0xc, .pwstst = 0x8, .flags = OMAP_PRM_NO_RSTST },
>> +	{ .name = "wkup", .base = 0x44e00d00, .pwstctrl = 0x4, .pwstst = 0x8, .rstst = 0xc, .rstmap = am3_wkup_rst_map },
>> +	{ .name = "mpu", .base = 0x44e00e00, .pwstst = 0x4 },
> 
> Has a rstst but no rstctrl, but your registration logic takes care of
> this. Somewhat confusing, when you just look at the data. Should you
> limit the check to only rstctrl and OMAP_PRM_NO_RSTST?

I think its probably better I invert the flags and explicitly state 
OMAP_PRM_HAS_RSTST | OMAP_PRM_HAS_RSTCTRL, in case any zero value is 
used for these.

> 
>> +	{ .name = "device", .base = 0x44e00f00, .rstctl = 0x0, .rstst = 0x8 },
> 
> No pwrstctrl and pwrstst registers, so same comment as on OMAP4 data.

I should probably add some flag for this in future once the support for 
power domains is added.

Anyway, I'll ditch all pwstctrl / pwstst data for now as it seems to 
bother you too much.

-Tero

> 
>> +	{ .name = "rtc", .base = 0x44e01000, .pwstst = 0x4 },
>> +	{ .name = "gfx", .base = 0x44e01100, .pwstst = 0x10, .rstctl = 0x4, .rstst = 0x14 },
>> +	{ .name = "cefuse", .base = 0x44e01200, .pwstst = 0x4 },
> 
> I am not sure if it is better to explicitly list the registers at 0
> offset rather than using the implied value of 0, since there are some
> registers that do not exist on some PRM instances which are also not
> defined.
> 
> regards
> Suman
> 
>> +	{ },
>> +};
>> +
>>   static const struct of_device_id omap_prm_id_table[] = {
>>   	{ .compatible = "ti,omap4-prm-inst", .data = omap4_prm_data },
>> +	{ .compatible = "ti,am3-prm-inst", .data = am3_prm_data },
>>   	{ },
>>   };
>>   
>>
> 

--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 41+ messages in thread

* Re: [PATCH 7/8] soc: ti: omap-prm: add dra7 PRM data
  2019-08-20 19:03   ` Suman Anna
@ 2019-08-21  7:36     ` Tero Kristo
  0 siblings, 0 replies; 41+ messages in thread
From: Tero Kristo @ 2019-08-21  7:36 UTC (permalink / raw)
  To: Suman Anna, ssantosh, linux-arm-kernel, linux-omap, robh+dt
  Cc: tony, devicetree

On 20.8.2019 22.03, Suman Anna wrote:
> Hi Tero,
> 
> On 8/7/19 2:48 AM, Tero Kristo wrote:
>> Add PRM data for dra7 family of SoCs.
>>
>> Signed-off-by: Tero Kristo <t-kristo@ti.com>
>> ---
>>   drivers/soc/ti/omap_prm.c | 26 ++++++++++++++++++++++++++
>>   1 file changed, 26 insertions(+)
>>
>> diff --git a/drivers/soc/ti/omap_prm.c b/drivers/soc/ti/omap_prm.c
>> index fadfc7f..05b7749 100644
>> --- a/drivers/soc/ti/omap_prm.c
>> +++ b/drivers/soc/ti/omap_prm.c
>> @@ -73,6 +73,31 @@ struct omap_prm_data omap4_prm_data[] = {
>>   	{ },
>>   };
>>   
>> +static struct omap_prm_data dra7_prm_data[] = {
>> +	{ .name = "mpu", .base = 0x4ae06300, .pwstst = 0x4 },
>> +	{ .name = "dsp1", .base = 0x4ae06400, .pwstst = 0x4, .rstctl = 0x10, .rstst = 0x14 },
>> +	{ .name = "ipu", .base = 0x4ae06500, .pwstst = 0x4, .rstctl = 0x10, .rstst = 0x14, .clkdm_name = "ipu1" },
>> +	{ .name = "coreaon", .base = 0x4ae06628, .pwstst = 0x4 },
> 
> Public TRM marks this region Reserved. Do you need it for anything?
This is copied from existing PRM data from kernel. However, I'll ditch 
these for now and only retain the reset enabled domains.

> 
>> +	{ .name = "core", .base = 0x4ae06700, .pwstst = 0x4, .rstctl = 0x210, .rstst = 0x214, .clkdm_name = "ipu2" },
>> +	{ .name = "iva", .base = 0x4ae06f00, .pwstst = 0x4 },
> 
> Missing rstctrl and rstst offsets.

Will add.

> 
>> +	{ .name = "cam", .base = 0x4ae07000, .pwstst = 0x4 },
>> +	{ .name = "dss", .base = 0x4ae07100, .pwstst = 0x4 },
>> +	{ .name = "gpu", .base = 0x4ae07200, .pwstst = 0x4 },
>> +	{ .name = "l3init", .base = 0x4ae07300, .pwstst = 0x4, .rstctl = 0x10, .rstst = 0x14 },
>> +	{ .name = "l4per", .base = 0x4ae07400, .pwstst = 0x4 },
>> +	{ .name = "custefuse", .base = 0x4ae07600, .pwstst = 0x4 },
>> +	{ .name = "wkupaon", .base = 0x4ae07724, .pwstst = 0x4 },
> 
> No pwstctrl and pwstst bits documented in TRM or are marked reserved.

Same as coreaon.

> 
>> +	{ .name = "emu", .base = 0x4ae07900, .pwstst = 0x4 },
>> +	{ .name = "dsp2", .base = 0x4ae07b00, .pwstst = 0x4, .rstctl = 0x10, .rstst = 0x14 },
>> +	{ .name = "eve1", .base = 0x4ae07b40, .pwstst = 0x4 },
>> +	{ .name = "eve2", .base = 0x4ae07b80, .pwstst = 0x4 },
>> +	{ .name = "eve3", .base = 0x4ae07bc0, .pwstst = 0x4 },
>> +	{ .name = "eve4", .base = 0x4ae07c00, .pwstst = 0x4 },
> 
> All EVEs are missing rstctrl and rstst fields.

Will add.

> 
>> +	{ .name = "rtc", .base = 0x4ae07c60, .pwstst = 0x4 },
> 
> Undocumented pwstctrl and pwstst registers.
> 
>> +	{ .name = "vpe", .base = 0x4ae07c80, .pwstst = 0x4 },
> 
> Missing "device" and "instr" PRM. The latter doesn't have any pwrstctl
> and pwrstst though.

Will ditch those.

-Tero

> 
> regards
> Suman
> 
>> +	{ },
>> +};
>> +
>>   struct omap_rst_map am3_wkup_rst_map[] = {
>>   	{ .rst = 3, .st = 5 },
>>   	{ .rst = -1 },
>> @@ -91,6 +116,7 @@ struct omap_prm_data am3_prm_data[] = {
>>   
>>   static const struct of_device_id omap_prm_id_table[] = {
>>   	{ .compatible = "ti,omap4-prm-inst", .data = omap4_prm_data },
>> +	{ .compatible = "ti,dra7-prm-inst", .data = dra7_prm_data },
>>   	{ .compatible = "ti,am3-prm-inst", .data = am3_prm_data },
>>   	{ },
>>   };
>>
> 

--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 41+ messages in thread

* Re: [PATCH 2/8] soc: ti: add initial PRM driver with reset control support
  2019-08-20 16:47           ` Suman Anna
@ 2019-08-21 15:10             ` Philipp Zabel
  2019-08-21 15:45               ` Suman Anna
  0 siblings, 1 reply; 41+ messages in thread
From: Philipp Zabel @ 2019-08-21 15:10 UTC (permalink / raw)
  To: Suman Anna, Tero Kristo, Keerthy, ssantosh, linux-arm-kernel,
	linux-omap, robh+dt
  Cc: tony, devicetree

On Tue, 2019-08-20 at 11:47 -0500, Suman Anna wrote:
> On 8/20/19 2:37 AM, Tero Kristo wrote:
> > On 20.8.2019 2.01, Suman Anna wrote:
> > > Hi Tero,
> > > 
> > > On 8/19/19 4:32 AM, Tero Kristo wrote:
[...]
> > > > > > +{
> > > > > > +    struct omap_reset_data *reset;
> > > > > > +
> > > > > > +    /*
> > > > > > +     * Check if we have resets. If either rstctl or rstst is
> > > > > > +     * non-zero, we have reset registers in place. Additionally
> > > > > > +     * the flag OMAP_PRM_NO_RSTST implies that we have resets.
> > > > > > +     */
> > > > > > +    if (!prm->data->rstctl && !prm->data->rstst &&
> > > > > > +        !(prm->data->flags & OMAP_PRM_NO_RSTST))
> > > > > > +        return 0;
> > > > > > +
> > > > > > +    reset = devm_kzalloc(&pdev->dev, sizeof(*reset), GFP_KERNEL);
> > > > > > +    if (!reset)
> > > > > > +        return -ENOMEM;
> > > > > > +
> > > > > > +    reset->rcdev.owner = THIS_MODULE;
> > > > > > +    reset->rcdev.ops = &omap_reset_ops;
> > > > > > +    reset->rcdev.of_node = pdev->dev.of_node;
> > > > > > +    reset->rcdev.nr_resets = OMAP_MAX_RESETS;
> > > 
> > > Suggest adding a number of resets to prm->data, and using it so that we
> > > don't even entertain any resets beyond the actual number of resets.
> > 
> > Hmm why bother? Accessing a stale reset bit will just cause access to a
> > reserved bit in the reset register, doing basically nothing. Also, this
> > would not work for am3/am4 wkup, as there is a single reset bit at an
> > arbitrary position.
> 
> The generic convention seems to be defining a reset id value defined
> from include/dt-bindings/reset/ that can be used to match between the
> dt-nodes and the reset-controller driver.
> 
> Philipp,
> Any comments?

Are there only reset bits and reserved bits in the range accessible by
[0..OMAP_MAX_RESETS] or are ther bits with another function as well?
If the latter is the case, I would prefer enumerating the resets in a
dt-bindings header, with the driver containing an enum -> reg/bit
position lookup table.

In general, assuming the device tree contains no errors, this should not
matter much, but I think it is nice if the reset driver, even with a
misconfigured device tree, can't write into arbitrary bit fields.

regards
Philipp

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 41+ messages in thread

* Re: [PATCH 2/8] soc: ti: add initial PRM driver with reset control support
  2019-08-21 15:10             ` Philipp Zabel
@ 2019-08-21 15:45               ` Suman Anna
  2019-08-21 18:15                 ` Tero Kristo
  0 siblings, 1 reply; 41+ messages in thread
From: Suman Anna @ 2019-08-21 15:45 UTC (permalink / raw)
  To: Philipp Zabel, Tero Kristo, Keerthy, ssantosh, linux-arm-kernel,
	linux-omap, robh+dt
  Cc: tony, devicetree

On 8/21/19 10:10 AM, Philipp Zabel wrote:
> On Tue, 2019-08-20 at 11:47 -0500, Suman Anna wrote:
>> On 8/20/19 2:37 AM, Tero Kristo wrote:
>>> On 20.8.2019 2.01, Suman Anna wrote:
>>>> Hi Tero,
>>>>
>>>> On 8/19/19 4:32 AM, Tero Kristo wrote:
> [...]
>>>>>>> +{
>>>>>>> +    struct omap_reset_data *reset;
>>>>>>> +
>>>>>>> +    /*
>>>>>>> +     * Check if we have resets. If either rstctl or rstst is
>>>>>>> +     * non-zero, we have reset registers in place. Additionally
>>>>>>> +     * the flag OMAP_PRM_NO_RSTST implies that we have resets.
>>>>>>> +     */
>>>>>>> +    if (!prm->data->rstctl && !prm->data->rstst &&
>>>>>>> +        !(prm->data->flags & OMAP_PRM_NO_RSTST))
>>>>>>> +        return 0;
>>>>>>> +
>>>>>>> +    reset = devm_kzalloc(&pdev->dev, sizeof(*reset), GFP_KERNEL);
>>>>>>> +    if (!reset)
>>>>>>> +        return -ENOMEM;
>>>>>>> +
>>>>>>> +    reset->rcdev.owner = THIS_MODULE;
>>>>>>> +    reset->rcdev.ops = &omap_reset_ops;
>>>>>>> +    reset->rcdev.of_node = pdev->dev.of_node;
>>>>>>> +    reset->rcdev.nr_resets = OMAP_MAX_RESETS;
>>>>
>>>> Suggest adding a number of resets to prm->data, and using it so that we
>>>> don't even entertain any resets beyond the actual number of resets.
>>>
>>> Hmm why bother? Accessing a stale reset bit will just cause access to a
>>> reserved bit in the reset register, doing basically nothing. Also, this
>>> would not work for am3/am4 wkup, as there is a single reset bit at an
>>> arbitrary position.
>>
>> The generic convention seems to be defining a reset id value defined
>> from include/dt-bindings/reset/ that can be used to match between the
>> dt-nodes and the reset-controller driver.
>>
>> Philipp,
>> Any comments?
> 
> Are there only reset bits and reserved bits in the range accessible by
> [0..OMAP_MAX_RESETS] or are ther bits with another function as well?

Thanks Philipp, these are just reset bits and reserved bits.

> If the latter is the case, I would prefer enumerating the resets in a
> dt-bindings header, with the driver containing an enum -> reg/bit
> position lookup table.
> 
> In general, assuming the device tree contains no errors, this should not
> matter much, but I think it is nice if the reset driver, even with a
> misconfigured device tree, can't write into arbitrary bit fields.

Tero,
Can you add a check for this if possible?

regards
Suman

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 41+ messages in thread

* Re: [PATCH 6/8] soc: ti: omap_prm: add data for am33xx
  2019-08-21  7:23     ` Tero Kristo
@ 2019-08-21 15:49       ` Suman Anna
  0 siblings, 0 replies; 41+ messages in thread
From: Suman Anna @ 2019-08-21 15:49 UTC (permalink / raw)
  To: Tero Kristo, ssantosh, linux-arm-kernel, linux-omap, robh+dt
  Cc: tony, devicetree

On 8/21/19 2:23 AM, Tero Kristo wrote:
> On 20.8.2019 21.48, Suman Anna wrote:
>> Hi Tero,
>>
>> On 8/7/19 2:48 AM, Tero Kristo wrote:
>>> Add PRM instance data for AM33xx SoC. Includes some basic register
>>> definitions and reset data for now.
>>>
>>> Signed-off-by: Tero Kristo <t-kristo@ti.com>
>>> ---
>>>   drivers/soc/ti/omap_prm.c | 17 +++++++++++++++++
>>>   1 file changed, 17 insertions(+)
>>>
>>> diff --git a/drivers/soc/ti/omap_prm.c b/drivers/soc/ti/omap_prm.c
>>> index 9b8d5945..fadfc7f 100644
>>> --- a/drivers/soc/ti/omap_prm.c
>>> +++ b/drivers/soc/ti/omap_prm.c
>>> @@ -73,8 +73,25 @@ struct omap_prm_data omap4_prm_data[] = {
>>>       { },
>>>   };
>>>   +struct omap_rst_map am3_wkup_rst_map[] = {
>>> +    { .rst = 3, .st = 5 },
>>> +    { .rst = -1 },
>>> +};
>>> +
>>> +struct omap_prm_data am3_prm_data[] = {
>>> +    { .name = "per", .base = 0x44e00c00, .pwstctrl = 0xc, .pwstst =
>>> 0x8, .flags = OMAP_PRM_NO_RSTST },
>>> +    { .name = "wkup", .base = 0x44e00d00, .pwstctrl = 0x4, .pwstst =
>>> 0x8, .rstst = 0xc, .rstmap = am3_wkup_rst_map },
>>> +    { .name = "mpu", .base = 0x44e00e00, .pwstst = 0x4 },
>>
>> Has a rstst but no rstctrl, but your registration logic takes care of
>> this. Somewhat confusing, when you just look at the data. Should you
>> limit the check to only rstctrl and OMAP_PRM_NO_RSTST?
> 
> I think its probably better I invert the flags and explicitly state
> OMAP_PRM_HAS_RSTST | OMAP_PRM_HAS_RSTCTRL, in case any zero value is
> used for these.

Yeah, something similar to HWMOD_OMAP4_ZERO_CLKCTRL_OFFSET in current
hwmod code.

> 
>>
>>> +    { .name = "device", .base = 0x44e00f00, .rstctl = 0x0, .rstst =
>>> 0x8 },
>>
>> No pwrstctrl and pwrstst registers, so same comment as on OMAP4 data.
> 
> I should probably add some flag for this in future once the support for
> power domains is added.
> 
> Anyway, I'll ditch all pwstctrl / pwstst data for now as it seems to
> bother you too much.

OK, that's probably cleaner, and the code and data can be handled when
you implement the power-domain pieces.

regards
Suman

> 
> -Tero
> 
>>
>>> +    { .name = "rtc", .base = 0x44e01000, .pwstst = 0x4 },
>>> +    { .name = "gfx", .base = 0x44e01100, .pwstst = 0x10, .rstctl =
>>> 0x4, .rstst = 0x14 },
>>> +    { .name = "cefuse", .base = 0x44e01200, .pwstst = 0x4 },
>>
>> I am not sure if it is better to explicitly list the registers at 0
>> offset rather than using the implied value of 0, since there are some
>> registers that do not exist on some PRM instances which are also not
>> defined.
>>
>> regards
>> Suman
>>
>>> +    { },
>>> +};
>>> +
>>>   static const struct of_device_id omap_prm_id_table[] = {
>>>       { .compatible = "ti,omap4-prm-inst", .data = omap4_prm_data },
>>> +    { .compatible = "ti,am3-prm-inst", .data = am3_prm_data },
>>>       { },
>>>   };
>>>  
>>
> 
> -- 
> Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
> Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 41+ messages in thread

* Re: [PATCH 2/8] soc: ti: add initial PRM driver with reset control support
  2019-08-21 15:45               ` Suman Anna
@ 2019-08-21 18:15                 ` Tero Kristo
  2019-08-23  8:50                   ` Philipp Zabel
  0 siblings, 1 reply; 41+ messages in thread
From: Tero Kristo @ 2019-08-21 18:15 UTC (permalink / raw)
  To: Suman Anna, Philipp Zabel, Keerthy, ssantosh, linux-arm-kernel,
	linux-omap, robh+dt
  Cc: tony, devicetree

On 21.8.2019 18.45, Suman Anna wrote:
> On 8/21/19 10:10 AM, Philipp Zabel wrote:
>> On Tue, 2019-08-20 at 11:47 -0500, Suman Anna wrote:
>>> On 8/20/19 2:37 AM, Tero Kristo wrote:
>>>> On 20.8.2019 2.01, Suman Anna wrote:
>>>>> Hi Tero,
>>>>>
>>>>> On 8/19/19 4:32 AM, Tero Kristo wrote:
>> [...]
>>>>>>>> +{
>>>>>>>> +    struct omap_reset_data *reset;
>>>>>>>> +
>>>>>>>> +    /*
>>>>>>>> +     * Check if we have resets. If either rstctl or rstst is
>>>>>>>> +     * non-zero, we have reset registers in place. Additionally
>>>>>>>> +     * the flag OMAP_PRM_NO_RSTST implies that we have resets.
>>>>>>>> +     */
>>>>>>>> +    if (!prm->data->rstctl && !prm->data->rstst &&
>>>>>>>> +        !(prm->data->flags & OMAP_PRM_NO_RSTST))
>>>>>>>> +        return 0;
>>>>>>>> +
>>>>>>>> +    reset = devm_kzalloc(&pdev->dev, sizeof(*reset), GFP_KERNEL);
>>>>>>>> +    if (!reset)
>>>>>>>> +        return -ENOMEM;
>>>>>>>> +
>>>>>>>> +    reset->rcdev.owner = THIS_MODULE;
>>>>>>>> +    reset->rcdev.ops = &omap_reset_ops;
>>>>>>>> +    reset->rcdev.of_node = pdev->dev.of_node;
>>>>>>>> +    reset->rcdev.nr_resets = OMAP_MAX_RESETS;
>>>>>
>>>>> Suggest adding a number of resets to prm->data, and using it so that we
>>>>> don't even entertain any resets beyond the actual number of resets.
>>>>
>>>> Hmm why bother? Accessing a stale reset bit will just cause access to a
>>>> reserved bit in the reset register, doing basically nothing. Also, this
>>>> would not work for am3/am4 wkup, as there is a single reset bit at an
>>>> arbitrary position.
>>>
>>> The generic convention seems to be defining a reset id value defined
>>> from include/dt-bindings/reset/ that can be used to match between the
>>> dt-nodes and the reset-controller driver.
>>>
>>> Philipp,
>>> Any comments?
>>
>> Are there only reset bits and reserved bits in the range accessible by
>> [0..OMAP_MAX_RESETS] or are ther bits with another function as well?
> 
> Thanks Philipp, these are just reset bits and reserved bits.
> 
>> If the latter is the case, I would prefer enumerating the resets in a
>> dt-bindings header, with the driver containing an enum -> reg/bit
>> position lookup table.
>>
>> In general, assuming the device tree contains no errors, this should not
>> matter much, but I think it is nice if the reset driver, even with a
>> misconfigured device tree, can't write into arbitrary bit fields.
> 
> Tero,
> Can you add a check for this if possible?

Well, I can enforce the usage of reset bit mapping, which I have already 
implemented for some SoCs like am33xx. If the specific ID is not found, 
I can bail out. So, basically in this example requesting reset at index 
3 would succeed, but it would fail for any other ID; this would be 
direct HW bit mapping.

-Tero
--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 41+ messages in thread

* Re: [PATCH 2/8] soc: ti: add initial PRM driver with reset control support
  2019-08-21 18:15                 ` Tero Kristo
@ 2019-08-23  8:50                   ` Philipp Zabel
  2019-08-23 11:27                     ` Tero Kristo
  0 siblings, 1 reply; 41+ messages in thread
From: Philipp Zabel @ 2019-08-23  8:50 UTC (permalink / raw)
  To: Tero Kristo, Suman Anna, Keerthy, ssantosh, linux-arm-kernel,
	linux-omap, robh+dt
  Cc: tony, devicetree

On Wed, 2019-08-21 at 21:15 +0300, Tero Kristo wrote:
> On 21.8.2019 18.45, Suman Anna wrote:
> > On 8/21/19 10:10 AM, Philipp Zabel wrote:
[...]
> > > In general, assuming the device tree contains no errors, this should not
> > > matter much, but I think it is nice if the reset driver, even with a
> > > misconfigured device tree, can't write into arbitrary bit fields.
> > 
> > Tero,
> > Can you add a check for this if possible?
> 
> Well, I can enforce the usage of reset bit mapping, which I have already 
> implemented for some SoCs like am33xx. If the specific ID is not found, 
> I can bail out. So, basically in this example requesting reset at index 
> 3 would succeed, but it would fail for any other ID; this would be 
> direct HW bit mapping.

That should be fine.

regards
Philipp

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 41+ messages in thread

* Re: [PATCH 2/8] soc: ti: add initial PRM driver with reset control support
  2019-08-23  8:50                   ` Philipp Zabel
@ 2019-08-23 11:27                     ` Tero Kristo
  0 siblings, 0 replies; 41+ messages in thread
From: Tero Kristo @ 2019-08-23 11:27 UTC (permalink / raw)
  To: Philipp Zabel, Suman Anna, Keerthy, ssantosh, linux-arm-kernel,
	linux-omap, robh+dt
  Cc: tony, devicetree

On 23.8.2019 11.50, Philipp Zabel wrote:
> On Wed, 2019-08-21 at 21:15 +0300, Tero Kristo wrote:
>> On 21.8.2019 18.45, Suman Anna wrote:
>>> On 8/21/19 10:10 AM, Philipp Zabel wrote:
> [...]
>>>> In general, assuming the device tree contains no errors, this should not
>>>> matter much, but I think it is nice if the reset driver, even with a
>>>> misconfigured device tree, can't write into arbitrary bit fields.
>>>
>>> Tero,
>>> Can you add a check for this if possible?
>>
>> Well, I can enforce the usage of reset bit mapping, which I have already
>> implemented for some SoCs like am33xx. If the specific ID is not found,
>> I can bail out. So, basically in this example requesting reset at index
>> 3 would succeed, but it would fail for any other ID; this would be
>> direct HW bit mapping.
> 
> That should be fine.

Ok, I am re-working it like this locally right now.

-Tero
--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 41+ messages in thread

end of thread, back to index

Thread overview: 41+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-08-07  7:48 [PATCH 0/8] soc: ti: Add OMAP PRM driver Tero Kristo
2019-08-07  7:48 ` [PATCH 1/8] dt-bindings: omap: add new binding for PRM instances Tero Kristo
2019-08-08  4:35   ` Keerthy
2019-08-19  9:28     ` Tero Kristo
2019-08-19 21:28       ` Suman Anna
2019-08-20  7:45         ` Tero Kristo
2019-08-07  7:48 ` [PATCH 2/8] soc: ti: add initial PRM driver with reset control support Tero Kristo
2019-08-08  5:26   ` Keerthy
2019-08-19  9:32     ` Tero Kristo
2019-08-19 23:01       ` Suman Anna
2019-08-20  7:37         ` Tero Kristo
2019-08-20 16:47           ` Suman Anna
2019-08-21 15:10             ` Philipp Zabel
2019-08-21 15:45               ` Suman Anna
2019-08-21 18:15                 ` Tero Kristo
2019-08-23  8:50                   ` Philipp Zabel
2019-08-23 11:27                     ` Tero Kristo
2019-08-07  7:48 ` [PATCH 3/8] soc: ti: omap-prm: poll for reset complete during de-assert Tero Kristo
2019-08-07  7:48 ` [PATCH 4/8] soc: ti: omap-prm: add support for denying idle for reset clockdomain Tero Kristo
2019-08-19 23:16   ` Suman Anna
2019-08-20  7:51     ` Tero Kristo
2019-08-07  7:48 ` [PATCH 5/8] soc: ti: omap-prm: add omap4 PRM data Tero Kristo
2019-08-08  5:30   ` Keerthy
2019-08-19  9:32     ` Tero Kristo
2019-08-19 23:08   ` Suman Anna
2019-08-20  7:52     ` Tero Kristo
2019-08-20 17:23       ` Suman Anna
2019-08-21  6:38         ` Tero Kristo
2019-08-07  7:48 ` [PATCH 6/8] soc: ti: omap_prm: add data for am33xx Tero Kristo
2019-08-19 23:11   ` Suman Anna
2019-08-20 18:48   ` Suman Anna
2019-08-21  7:23     ` Tero Kristo
2019-08-21 15:49       ` Suman Anna
2019-08-07  7:48 ` [PATCH 7/8] soc: ti: omap-prm: add dra7 PRM data Tero Kristo
2019-08-19 23:12   ` Suman Anna
2019-08-20 19:03   ` Suman Anna
2019-08-21  7:36     ` Tero Kristo
2019-08-07  7:48 ` [PATCH 8/8] ARM: OMAP2+: pdata-quirks: add PRM data for reset support Tero Kristo
2019-08-19 23:20 ` [PATCH 0/8] soc: ti: Add OMAP PRM driver Suman Anna
2019-08-20  7:54   ` Tero Kristo
2019-08-20 16:51     ` Suman Anna

Linux-ARM-Kernel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-arm-kernel/0 linux-arm-kernel/git/0.git
	git clone --mirror https://lore.kernel.org/linux-arm-kernel/1 linux-arm-kernel/git/1.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-arm-kernel linux-arm-kernel/ https://lore.kernel.org/linux-arm-kernel \
		linux-arm-kernel@lists.infradead.org infradead-linux-arm-kernel@archiver.kernel.org
	public-inbox-index linux-arm-kernel


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.infradead.lists.linux-arm-kernel


AGPL code for this site: git clone https://public-inbox.org/ public-inbox