[RFC PATCH 0/9] Add generic PM domain support for Tegra SoC

[RFC PATCH 0/9] Add generic PM domain support for Tegra SoC

[Vince Hsu]

Hi Thierry & Peter,

This is a RFC series for the previous discussion about generic PM domain.
Could you have a quick review and give some comments? So that I can know
whether I should continue this work and add more support for other Tegra
SoCs and boards then.

Patch 2 & 3 are moved from the series "Add suspend/resume support for GK20A"
and improved according to the review comments.

Patch 4 is based on Thierry's work and adds DT support, etc.

Patch 5 - 9 are the examples to demonstrate the generic PM domains on
Tegra124/JetsonTK1. We can add support for other Tegra SoCs later if you
guys have no objection to this proof of concept.

Thanks,
Vince

Thierry Reding (1):
  soc: tegra: pmc: Add generic PM domain support

Vince Hsu (8):
  reset: add of_reset_control_get_by_index()
  memory: tegra: add mc flush support
  memory: tegra: add flush operation for Tegra124 memory clients
  ARM: tegra: add PM domain device nodes to Tegra124 DT
  ARM: tegra: add GPU power supply to Jetson TK1 DT
  drm/tegra: dc: remove the power sequence from driver
  PCI: tegra: remove the power sequence from driver
  ata: ahci_tegra: remove power sequence from driver

 arch/arm/boot/dts/tegra124-jetson-tk1.dts   |   4 +
 arch/arm/boot/dts/tegra124.dtsi             |  73 +++-
 drivers/ata/ahci_tegra.c                    |  11 -
 drivers/gpu/drm/tegra/dc.c                  |  46 +--
 drivers/memory/tegra/mc.c                   | 132 +++++++
 drivers/memory/tegra/tegra114.c             |   2 +-
 drivers/memory/tegra/tegra124.c             | 108 ++++-
 drivers/memory/tegra/tegra30.c              |   2 +-
 drivers/pci/host/pci-tegra.c                |  20 -
 drivers/reset/core.c                        |  47 ++-
 drivers/soc/tegra/pmc.c                     | 589 ++++++++++++++++++++++++++++
 include/dt-bindings/power/tegra-powergate.h |  30 ++
 include/linux/reset.h                       |   9 +
 include/soc/tegra/mc.h                      |  41 +-
 14 files changed, 1027 insertions(+), 87 deletions(-)
 create mode 100644 include/dt-bindings/power/tegra-powergate.h

-- 
1.9.1

[RFC PATCH 1/9] reset: add of_reset_control_get_by_index()

[Vince Hsu]

Add of_reset_control_get_by_index() to allow the dirvers to get reset device
without knowing its name.

Signed-off-by: Vince Hsu <vinceh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
 drivers/reset/core.c  | 47 ++++++++++++++++++++++++++++++++---------------
 include/linux/reset.h |  9 +++++++++
 2 files changed, 41 insertions(+), 15 deletions(-)

diff --git a/drivers/reset/core.c b/drivers/reset/core.c
index 7955e00d04d4..25a0da1654f8 100644
--- a/drivers/reset/core.c
+++ b/drivers/reset/core.c
@@ -140,28 +140,15 @@ int reset_control_status(struct reset_control *rstc)
 }
 EXPORT_SYMBOL_GPL(reset_control_status);
 
-/**
- * of_reset_control_get - Lookup and obtain a reference to a reset controller.
- * @node: device to be reset by the controller
- * @id: reset line name
- *
- * Returns a struct reset_control or IS_ERR() condition containing errno.
- *
- * Use of id names is optional.
- */
-struct reset_control *of_reset_control_get(struct device_node *node,
-					   const char *id)
+struct reset_control *__of_reset_control_get(struct device_node *node,
+						int index)
 {
 	struct reset_control *rstc = ERR_PTR(-EPROBE_DEFER);
 	struct reset_controller_dev *r, *rcdev;
 	struct of_phandle_args args;
-	int index = 0;
 	int rstc_id;
 	int ret;
 
-	if (id)
-		index = of_property_match_string(node,
-						 "reset-names", id);
 	ret = of_parse_phandle_with_args(node, "resets", "#reset-cells",
 					 index, &args);
 	if (ret)
@@ -202,6 +189,36 @@ struct reset_control *of_reset_control_get(struct device_node *node,
 
 	return rstc;
 }
+
+struct reset_control *of_reset_control_get_by_index(struct device_node *node,
+					   int index)
+{
+	if (!node)
+		return ERR_PTR(-EINVAL);
+
+	return __of_reset_control_get(node, index);
+}
+EXPORT_SYMBOL_GPL(of_reset_control_get_by_index);
+
+/**
+ * of_reset_control_get - Lookup and obtain a reference to a reset controller.
+ * @node: device to be reset by the controller
+ * @id: reset line name
+ *
+ * Returns a struct reset_control or IS_ERR() condition containing errno.
+ *
+ * Use of id names is optional.
+ */
+struct reset_control *of_reset_control_get(struct device_node *node,
+					   const char *id)
+{
+	int index = 0;
+
+	if (id)
+		index = of_property_match_string(node,
+						 "reset-names", id);
+	return __of_reset_control_get(node, index);
+}
 EXPORT_SYMBOL_GPL(of_reset_control_get);
 
 /**
diff --git a/include/linux/reset.h b/include/linux/reset.h
index da5602bd77d7..f29fc88c4868 100644
--- a/include/linux/reset.h
+++ b/include/linux/reset.h
@@ -38,6 +38,9 @@ static inline struct reset_control *devm_reset_control_get_optional(
 struct reset_control *of_reset_control_get(struct device_node *node,
 					   const char *id);
 
+struct reset_control *of_reset_control_get_by_index(
+					struct device_node *node, int index);
+
 #else
 
 static inline int reset_control_reset(struct reset_control *rstc)
@@ -92,6 +95,12 @@ static inline struct reset_control *of_reset_control_get(
 	return ERR_PTR(-ENOSYS);
 }
 
+struct reset_control *of_reset_control_get_by_index(
+				struct device_node *node, int index)
+{
+	return ERR_PTR(-ENOSYS);
+}
+
 #endif /* CONFIG_RESET_CONTROLLER */
 
 #endif
-- 
1.9.1

[RFC PATCH 2/9] memory: tegra: add mc flush support

[Vince Hsu]

The flush operation of memory clients is needed for various IP blocks in
the Tegra SoCs to perform a clean reset.

Signed-off-by: Vince Hsu <vinceh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
 drivers/memory/tegra/mc.c       | 132 ++++++++++++++++++++++++++++++++++++++++
 drivers/memory/tegra/tegra114.c |   2 +-
 drivers/memory/tegra/tegra30.c  |   2 +-
 include/soc/tegra/mc.h          |  41 ++++++++++++-
 4 files changed, 173 insertions(+), 4 deletions(-)

diff --git a/drivers/memory/tegra/mc.c b/drivers/memory/tegra/mc.c
index fe3c44e7e1d1..35f769f9f1cd 100644
--- a/drivers/memory/tegra/mc.c
+++ b/drivers/memory/tegra/mc.c
@@ -11,6 +11,7 @@
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/of.h>
+#include <linux/of_platform.h>
 #include <linux/platform_device.h>
 #include <linux/slab.h>
 
@@ -62,6 +63,130 @@ static const struct of_device_id tegra_mc_of_match[] = {
 };
 MODULE_DEVICE_TABLE(of, tegra_mc_of_match);
 
+static struct tegra_mc_swgroup *find_swgroup(struct tegra_mc *mc,
+		unsigned int swgroup)
+{
+	struct tegra_mc_swgroup *sg;
+
+	list_for_each_entry(sg, &mc->swgroups, head) {
+		if (sg->id == swgroup)
+			return sg;
+	}
+
+	return NULL;
+}
+
+static struct tegra_mc_swgroup *add_swgroup(struct tegra_mc *mc,
+	unsigned int swgroup)
+{
+	struct tegra_mc_swgroup *sg;
+
+	sg = devm_kzalloc(mc->dev, sizeof(*sg), GFP_KERNEL);
+	if (!sg)
+		return ERR_PTR(-ENOMEM);
+
+	sg->id = swgroup;
+	sg->mc = mc;
+	list_add_tail(&sg->head, &mc->swgroups);
+	INIT_LIST_HEAD(&sg->clients);
+
+	return sg;
+}
+
+struct tegra_mc_swgroup *tegra_mc_find_swgroup(struct device_node *node,
+					int index)
+{
+	struct of_phandle_args args;
+	struct platform_device *pdev;
+	struct tegra_mc *mc;
+	int ret;
+
+	ret = of_parse_phandle_with_fixed_args(node, "nvidia,swgroup",
+				1, index, &args);
+	if (ret)
+		return ERR_PTR(ret);
+
+	pdev = of_find_device_by_node(args.np);
+	if (!pdev)
+		return NULL;
+
+	mc = platform_get_drvdata(pdev);
+	if (!mc)
+		return NULL;
+
+	return find_swgroup(mc, args.args[0]);
+}
+EXPORT_SYMBOL(tegra_mc_find_swgroup);
+
+int tegra_mc_flush(struct tegra_mc_swgroup *sg)
+{
+	struct tegra_mc *mc;
+	const struct tegra_mc_hotreset *client;
+	int i;
+
+	if (!sg || !sg->mc)
+		return -EINVAL;;
+
+	mc = sg->mc;
+	if (!mc->soc->ops || !mc->soc->ops->flush)
+		return -EINVAL;;
+
+	client = mc->soc->hotresets;
+
+	for (i = 0; i < mc->soc->num_hotresets; i++, client++) {
+		if (sg->id == client->swgroup)
+			return mc->soc->ops->flush(mc, client);
+	}
+
+	return -EINVAL;
+}
+EXPORT_SYMBOL(tegra_mc_flush);
+
+int tegra_mc_flush_done(struct tegra_mc_swgroup *sg)
+{
+	struct tegra_mc *mc;
+	const struct tegra_mc_hotreset *client;
+	int i;
+
+	if (!sg || !sg->mc)
+		return -EINVAL;;
+
+	mc = sg->mc;
+	if (!mc->soc->ops || !mc->soc->ops->flush)
+		return -EINVAL;;
+
+	client = mc->soc->hotresets;
+
+	for (i = 0; i < mc->soc->num_hotresets; i++, client++) {
+		if (sg->id == client->swgroup)
+			return mc->soc->ops->flush_done(mc, client);
+	}
+
+	return -EINVAL;
+}
+EXPORT_SYMBOL(tegra_mc_flush_done);
+
+static int tegra_mc_build_swgroup(struct tegra_mc *mc)
+{
+	int i;
+
+	for (i = 0; i < mc->soc->num_clients; i++) {
+		struct tegra_mc_swgroup *sg;
+
+		sg = find_swgroup(mc, mc->soc->clients[i].swgroup);
+
+		if (!sg) {
+			sg = add_swgroup(mc,  mc->soc->clients[i].swgroup);
+			if (IS_ERR(sg))
+				return PTR_ERR(sg);
+		}
+
+		list_add_tail(&mc->soc->clients[i].head, &sg->clients);
+	}
+
+	return 0;
+}
+
 static int tegra_mc_setup_latency_allowance(struct tegra_mc *mc)
 {
 	unsigned long long tick;
@@ -229,6 +354,13 @@ static int tegra_mc_probe(struct platform_device *pdev)
 	/* length of MC tick in nanoseconds */
 	mc->tick = 30;
 
+	INIT_LIST_HEAD(&mc->swgroups);
+	err = tegra_mc_build_swgroup(mc);
+	if (err) {
+		dev_err(&pdev->dev, "failed to build swgroup: %d\n", err);
+		return err;
+	}
+
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	mc->regs = devm_ioremap_resource(&pdev->dev, res);
 	if (IS_ERR(mc->regs))
diff --git a/drivers/memory/tegra/tegra114.c b/drivers/memory/tegra/tegra114.c
index 511e9a25c151..92ab5552fcee 100644
--- a/drivers/memory/tegra/tegra114.c
+++ b/drivers/memory/tegra/tegra114.c
@@ -15,7 +15,7 @@
 
 #include "mc.h"
 
-static const struct tegra_mc_client tegra114_mc_clients[] = {
+static struct tegra_mc_client tegra114_mc_clients[] = {
 	{
 		.id = 0x00,
 		.name = "ptcr",
diff --git a/drivers/memory/tegra/tegra30.c b/drivers/memory/tegra/tegra30.c
index 71fe9376fe53..3ed4bf409a72 100644
--- a/drivers/memory/tegra/tegra30.c
+++ b/drivers/memory/tegra/tegra30.c
@@ -15,7 +15,7 @@
 
 #include "mc.h"
 
-static const struct tegra_mc_client tegra30_mc_clients[] = {
+static struct tegra_mc_client tegra30_mc_clients[] = {
 	{
 		.id = 0x00,
 		.name = "ptcr",
diff --git a/include/soc/tegra/mc.h b/include/soc/tegra/mc.h
index 63deb8d9f82a..95db1ab1a8bd 100644
--- a/include/soc/tegra/mc.h
+++ b/include/soc/tegra/mc.h
@@ -37,6 +37,32 @@ struct tegra_mc_client {
 
 	struct tegra_smmu_enable smmu;
 	struct tegra_mc_la la;
+
+	struct list_head head;
+};
+
+struct tegra_mc;
+
+/* hot reset */
+struct tegra_mc_hotreset {
+	unsigned int swgroup;
+	unsigned int ctrl;
+	unsigned int status;
+	unsigned int bit;
+};
+
+struct tegra_mc_swgroup {
+	unsigned int id;
+	struct tegra_mc *mc;
+	struct list_head head;
+	struct list_head clients;
+};
+
+struct tegra_mc_ops {
+	int (*flush)(struct tegra_mc *mc,
+			const struct tegra_mc_hotreset *hotreset);
+	int (*flush_done)(struct tegra_mc *mc,
+			const struct tegra_mc_hotreset *hotreset);
 };
 
 struct tegra_smmu_swgroup {
@@ -64,7 +90,6 @@ struct tegra_smmu_soc {
 	const struct tegra_smmu_ops *ops;
 };
 
-struct tegra_mc;
 struct tegra_smmu;
 
 #ifdef CONFIG_TEGRA_IOMMU_SMMU
@@ -81,9 +106,14 @@ tegra_smmu_probe(struct device *dev, const struct tegra_smmu_soc *soc,
 #endif
 
 struct tegra_mc_soc {
-	const struct tegra_mc_client *clients;
+	struct tegra_mc_client *clients;
 	unsigned int num_clients;
 
+	const struct tegra_mc_hotreset *hotresets;
+	unsigned int num_hotresets;
+
+	const struct tegra_mc_ops *ops;
+
 	const unsigned int *emem_regs;
 	unsigned int num_emem_regs;
 
@@ -102,6 +132,13 @@ struct tegra_mc {
 
 	const struct tegra_mc_soc *soc;
 	unsigned long tick;
+
+	struct list_head swgroups;
 };
 
+struct tegra_mc_swgroup *tegra_mc_find_swgroup(struct device_node *node,
+					int index);
+int tegra_mc_flush(struct tegra_mc_swgroup *sg);
+int tegra_mc_flush_done(struct tegra_mc_swgroup *sg);
+
 #endif /* __SOC_TEGRA_MC_H__ */
-- 
1.9.1

[RFC PATCH 3/9] memory: tegra: add flush operation for Tegra124 memory clients

[Vince Hsu]

Signed-off-by: Vince Hsu <vinceh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
 drivers/memory/tegra/tegra124.c | 108 +++++++++++++++++++++++++++++++++++++++-
 1 file changed, 107 insertions(+), 1 deletion(-)

diff --git a/drivers/memory/tegra/tegra124.c b/drivers/memory/tegra/tegra124.c
index 278d40b854c1..cce255d3df5c 100644
--- a/drivers/memory/tegra/tegra124.c
+++ b/drivers/memory/tegra/tegra124.c
@@ -6,6 +6,8 @@
  * published by the Free Software Foundation.
  */
 
+#include <linux/delay.h>
+#include <linux/device.h>
 #include <linux/of.h>
 #include <linux/mm.h>
 
@@ -15,7 +17,7 @@
 
 #include "mc.h"
 
-static const struct tegra_mc_client tegra124_mc_clients[] = {
+static struct tegra_mc_client tegra124_mc_clients[] = {
 	{
 		.id = 0x00,
 		.name = "ptcr",
@@ -959,7 +961,108 @@ static const struct tegra_smmu_swgroup tegra124_swgroups[] = {
 	{ .swgroup = TEGRA_SWGROUP_VI,        .reg = 0x280 },
 };
 
+static struct tegra_mc_hotreset tegra124_mc_hotreset[] = {
+	{TEGRA_SWGROUP_AFI,        0x200, 0x204,  0},
+	{TEGRA_SWGROUP_AVPC,       0x200, 0x204,  1},
+	{TEGRA_SWGROUP_DC,         0x200, 0x204,  2},
+	{TEGRA_SWGROUP_DCB,        0x200, 0x204,  3},
+	{TEGRA_SWGROUP_HC,         0x200, 0x204,  6},
+	{TEGRA_SWGROUP_HDA,        0x200, 0x204,  7},
+	{TEGRA_SWGROUP_ISP2,       0x200, 0x204,  8},
+	{TEGRA_SWGROUP_MPCORE,     0x200, 0x204,  9},
+	{TEGRA_SWGROUP_MPCORELP,   0x200, 0x204, 10},
+	{TEGRA_SWGROUP_MSENC,      0x200, 0x204, 11},
+	{TEGRA_SWGROUP_PPCS,       0x200, 0x204, 14},
+	{TEGRA_SWGROUP_SATA,       0x200, 0x204, 15},
+	{TEGRA_SWGROUP_VDE,        0x200, 0x204, 16},
+	{TEGRA_SWGROUP_VI,         0x200, 0x204, 17},
+	{TEGRA_SWGROUP_VIC,        0x200, 0x204, 18},
+	{TEGRA_SWGROUP_XUSB_HOST,  0x200, 0x204, 19},
+	{TEGRA_SWGROUP_XUSB_DEV,   0x200, 0x204, 20},
+	{TEGRA_SWGROUP_TSEC,       0x200, 0x204, 22},
+	{TEGRA_SWGROUP_SDMMC1A,    0x200, 0x204, 29},
+	{TEGRA_SWGROUP_SDMMC2A,    0x200, 0x204, 30},
+	{TEGRA_SWGROUP_SDMMC3A,    0x200, 0x204, 31},
+	{TEGRA_SWGROUP_SDMMC4A,    0x970, 0x974,  0},
+	{TEGRA_SWGROUP_ISP2B,      0x970, 0x974,  1},
+	{TEGRA_SWGROUP_GPU,        0x970, 0x974,  2},
+};
+
 #ifdef CONFIG_ARCH_TEGRA_124_SOC
+
+static bool tegra124_stable_hotreset_check(struct tegra_mc *mc,
+		u32 reg, u32 *stat)
+{
+	int i;
+	u32 cur_stat;
+	u32 prv_stat;
+
+	/*
+	 * There might be a glitch seen with the status register if we program
+	 * the control register and then read the status register in a short
+	 * window due to a HW bug. So here we poll for a stable status read.
+	 */
+	prv_stat = mc_readl(mc, reg);
+	for (i = 0; i < 5; i++) {
+		cur_stat = mc_readl(mc, reg);
+		if (cur_stat != prv_stat)
+			return false;
+	}
+	*stat = cur_stat;
+	return true;
+}
+
+static int tegra124_mc_flush(struct tegra_mc *mc,
+		const struct tegra_mc_hotreset *hotreset)
+{
+	u32 val;
+
+	if (!mc || !hotreset)
+		return -EINVAL;
+
+	/* XXX add mutex */
+
+	val = mc_readl(mc, hotreset->ctrl);
+	val |= BIT(hotreset->bit);
+	mc_writel(mc, val, hotreset->ctrl);
+	mc_readl(mc, hotreset->ctrl);
+
+	/* poll till the flush is done */
+	do {
+		udelay(10);
+		val = 0;
+		if (!tegra124_stable_hotreset_check(mc, hotreset->status, &val))
+			continue;
+	} while (!(val & BIT(hotreset->bit)));
+
+	dev_dbg(mc->dev, "%s bit %d\n", __func__, hotreset->bit);
+	return 0;
+}
+
+static int tegra124_mc_flush_done(struct tegra_mc *mc,
+		const struct tegra_mc_hotreset *hotreset)
+{
+	u32 val;
+
+	if (!mc || !hotreset)
+		return -EINVAL;
+
+	/* XXX add mutex */
+
+	val = mc_readl(mc, hotreset->ctrl);
+	val &= ~BIT(hotreset->bit);
+	mc_writel(mc, val, hotreset->ctrl);
+	mc_readl(mc, hotreset->ctrl);
+
+	dev_dbg(mc->dev, "%s bit %d\n", __func__, hotreset->bit);
+	return 0;
+}
+
+static const struct tegra_mc_ops tegra124_mc_ops = {
+	.flush = tegra124_mc_flush,
+	.flush_done = tegra124_mc_flush_done,
+};
+
 static void tegra124_flush_dcache(struct page *page, unsigned long offset,
 				  size_t size)
 {
@@ -991,5 +1094,8 @@ const struct tegra_mc_soc tegra124_mc_soc = {
 	.num_address_bits = 34,
 	.atom_size = 32,
 	.smmu = &tegra124_smmu_soc,
+	.hotresets = tegra124_mc_hotreset,
+	.num_hotresets = ARRAY_SIZE(tegra124_mc_hotreset),
+	.ops = &tegra124_mc_ops,
 };
 #endif /* CONFIG_ARCH_TEGRA_124_SOC */
-- 
1.9.1

[RFC PATCH 4/9] soc: tegra: pmc: Add generic PM domain support

[Vince Hsu]

From: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>

The PM domains are populated from DT, and the PM domain consumer devices are
also bound to their relevant PM domains by DT.

Signed-off-by: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
[vinceh: make changes based on Thierry and Peter's suggestions]
Signed-off-by: Vince Hsu <vinceh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
 drivers/soc/tegra/pmc.c                     | 589 ++++++++++++++++++++++++++++
 include/dt-bindings/power/tegra-powergate.h |  30 ++
 2 files changed, 619 insertions(+)
 create mode 100644 include/dt-bindings/power/tegra-powergate.h

diff --git a/drivers/soc/tegra/pmc.c b/drivers/soc/tegra/pmc.c
index 4bdc654bd747..0779b0ba6d3d 100644
--- a/drivers/soc/tegra/pmc.c
+++ b/drivers/soc/tegra/pmc.c
@@ -17,6 +17,8 @@
  *
  */
 
+#define DEBUG
+
 #include <linux/kernel.h>
 #include <linux/clk.h>
 #include <linux/clk/tegra.h>
@@ -27,15 +29,20 @@
 #include <linux/init.h>
 #include <linux/io.h>
 #include <linux/of.h>
+#include <linux/of_platform.h>
 #include <linux/of_address.h>
 #include <linux/platform_device.h>
+#include <linux/pm_domain.h>
 #include <linux/reboot.h>
+#include <linux/regulator/consumer.h>
 #include <linux/reset.h>
+#include <linux/sched.h>
 #include <linux/seq_file.h>
 #include <linux/spinlock.h>
 
 #include <soc/tegra/common.h>
 #include <soc/tegra/fuse.h>
+#include <soc/tegra/mc.h>
 #include <soc/tegra/pmc.h>
 
 #define PMC_CNTRL			0x0
@@ -83,6 +90,30 @@
 
 #define GPU_RG_CNTRL			0x2d4
 
+#define MAX_CLK_NUM		5
+#define MAX_RESET_NUM		5
+#define MAX_SWGROUP_NUM		5
+
+struct tegra_powergate {
+	struct generic_pm_domain base;
+	struct tegra_pmc *pmc;
+	unsigned int id;
+	const char *name;
+	struct list_head head;
+	struct device_node *of_node;
+	struct clk *clk[MAX_CLK_NUM];
+	struct reset_control *reset[MAX_RESET_NUM];
+	struct tegra_mc_swgroup *swgroup[MAX_SWGROUP_NUM];
+	bool need_vdd;
+	struct regulator *vdd;
+};
+
+static inline struct tegra_powergate *
+to_powergate(struct generic_pm_domain *domain)
+{
+	return container_of(domain, struct tegra_powergate, base);
+}
+
 struct tegra_pmc_soc {
 	unsigned int num_powergates;
 	const char *const *powergates;
@@ -92,8 +123,10 @@ struct tegra_pmc_soc {
 
 /**
  * struct tegra_pmc - NVIDIA Tegra PMC
+ * @dev: pointer to parent device
  * @base: pointer to I/O remapped register region
  * @clk: pointer to pclk clock
+ * @soc: SoC-specific data
  * @rate: currently configured rate of pclk
  * @suspend_mode: lowest suspend mode available
  * @cpu_good_time: CPU power good time (in microseconds)
@@ -107,9 +140,12 @@ struct tegra_pmc_soc {
  * @cpu_pwr_good_en: CPU power good signal is enabled
  * @lp0_vec_phys: physical base address of the LP0 warm boot code
  * @lp0_vec_size: size of the LP0 warm boot code
+ * @powergates: list of power gates
  * @powergates_lock: mutex for power gate register access
+ * @nb: bus notifier for generic power domains
  */
 struct tegra_pmc {
+	struct device *dev;
 	void __iomem *base;
 	struct clk *clk;
 
@@ -130,7 +166,12 @@ struct tegra_pmc {
 	u32 lp0_vec_phys;
 	u32 lp0_vec_size;
 
+	struct tegra_powergate *powergates;
 	struct mutex powergates_lock;
+	struct notifier_block nb;
+
+	struct list_head powergate_list;
+	int power_domain_num;
 };
 
 static struct tegra_pmc *pmc = &(struct tegra_pmc) {
@@ -353,6 +394,8 @@ int tegra_pmc_cpu_remove_clamping(int cpuid)
 	if (id < 0)
 		return id;
 
+	usleep_range(10, 20);
+
 	return tegra_powergate_remove_clamping(id);
 }
 #endif /* CONFIG_SMP */
@@ -387,6 +430,317 @@ void tegra_pmc_restart(enum reboot_mode mode, const char *cmd)
 	tegra_pmc_writel(value, 0);
 }
 
+static bool tegra_pmc_powergate_is_powered(struct tegra_powergate *powergate)
+{
+	u32 status = tegra_pmc_readl(PWRGATE_STATUS);
+
+	if (!powergate->need_vdd)
+		return (status & BIT(powergate->id)) != 0;
+
+	if (IS_ERR(powergate->vdd))
+		return false;
+	else
+		return regulator_is_enabled(powergate->vdd);
+}
+
+static int tegra_pmc_powergate_set(struct tegra_powergate *powergate,
+				   bool new_state)
+{
+	u32 status, mask = new_state ? BIT(powergate->id) : 0;
+	bool state = false;
+
+	mutex_lock(&pmc->powergates_lock);
+
+	/* check the current state of the partition */
+	status = tegra_pmc_readl(PWRGATE_STATUS);
+	if (status & BIT(powergate->id))
+		state = true;
+
+	/* nothing to do */
+	if (new_state == state) {
+		mutex_unlock(&pmc->powergates_lock);
+		return 0;
+	}
+
+	/* toggle partition state and wait for state change to finish */
+	tegra_pmc_writel(PWRGATE_TOGGLE_START | powergate->id, PWRGATE_TOGGLE);
+
+	while (1) {
+		status = tegra_pmc_readl(PWRGATE_STATUS);
+		if ((status & BIT(powergate->id)) == mask)
+			break;
+
+		usleep_range(10, 20);
+	}
+
+	mutex_unlock(&pmc->powergates_lock);
+
+	return 0;
+}
+
+static int
+tegra_pmc_powergate_remove_clamping(struct tegra_powergate *powergate)
+{
+	u32 mask;
+
+	/*
+	 * The Tegra124 GPU has a separate register (with different semantics)
+	 * to remove clamps.
+	 */
+	if (tegra_get_chip_id() == TEGRA124) {
+		if (powergate->id == TEGRA_POWERGATE_3D) {
+			tegra_pmc_writel(0, GPU_RG_CNTRL);
+			return 0;
+		}
+	}
+
+	/*
+	 * Tegra 2 has a bug where PCIE and VDE clamping masks are
+	 * swapped relatively to the partition ids
+	 */
+	if (powergate->id == TEGRA_POWERGATE_VDEC)
+		mask = (1 << TEGRA_POWERGATE_PCIE);
+	else if (powergate->id == TEGRA_POWERGATE_PCIE)
+		mask = (1 << TEGRA_POWERGATE_VDEC);
+	else
+		mask = (1 << powergate->id);
+
+	tegra_pmc_writel(mask, REMOVE_CLAMPING);
+
+	return 0;
+}
+
+static int tegra_pmc_powergate_enable_clocks(
+		struct tegra_powergate *powergate)
+{
+	int i, err;
+
+	for (i = 0; i < MAX_CLK_NUM; i++) {
+		if (!powergate->clk[i])
+			break;
+
+		err = clk_prepare_enable(powergate->clk[i]);
+		if (err)
+			goto out;
+	}
+
+	return 0;
+
+out:
+	while(i--)
+		clk_disable_unprepare(powergate->clk[i]);
+	return err;
+}
+
+static void tegra_pmc_powergate_disable_clocks(
+		struct tegra_powergate *powergate)
+{
+	int i;
+
+	for (i = 0; i < MAX_CLK_NUM; i++) {
+		if (!powergate->clk[i])
+			break;
+
+		clk_disable_unprepare(powergate->clk[i]);
+	}
+}
+
+static int tegra_pmc_powergate_mc_flush(struct tegra_powergate *powergate)
+{
+	int i, err;
+
+	for (i = 0; i < MAX_SWGROUP_NUM; i++) {
+		if (!powergate->swgroup[i])
+			break;
+
+		err = tegra_mc_flush(powergate->swgroup[i]);
+		if (err)
+			return err;
+	}
+
+	return 0;
+}
+
+static int tegra_pmc_powergate_mc_flush_done(struct tegra_powergate *powergate)
+{
+	int i, err;
+
+	for (i = 0; i < MAX_SWGROUP_NUM; i++) {
+		if (!powergate->swgroup[i])
+			break;
+
+		err = tegra_mc_flush_done(powergate->swgroup[i]);
+		if (err)
+			return err;
+	}
+
+	return 0;
+
+}
+
+static int tegra_pmc_powergate_reset_assert(
+		struct tegra_powergate *powergate)
+{
+	int i, err;
+
+	for (i = 0; i < MAX_RESET_NUM; i++) {
+		if (!powergate->reset[i])
+			break;
+
+		err = reset_control_assert(powergate->reset[i]);
+		if (err)
+			return err;
+	}
+
+	return 0;
+}
+
+static int tegra_pmc_powergate_reset_deassert(
+		struct tegra_powergate *powergate)
+{
+	int i, err;
+
+	for (i = 0; i < MAX_RESET_NUM; i++) {
+		if (!powergate->reset[i])
+			break;
+
+		err = reset_control_deassert(powergate->reset[i]);
+		if (err)
+			return err;
+	}
+
+	return 0;
+}
+
+static int get_regulator(struct tegra_powergate *powergate)
+{
+	struct platform_device *pdev;
+
+	if (!powergate->need_vdd)
+		return -EINVAL;
+
+	if (powergate->vdd && !IS_ERR(powergate->vdd))
+		return 0;
+
+	pdev = of_find_device_by_node(powergate->of_node);
+	if (!pdev)
+		return -EINVAL;
+
+	powergate->vdd = devm_regulator_get_optional(&pdev->dev, "vdd");
+	if (IS_ERR(powergate->vdd))
+		return -EINVAL;
+
+	return 0;
+}
+
+static int tegra_pmc_powergate_power_on(struct generic_pm_domain *domain)
+{
+	struct tegra_powergate *powergate = to_powergate(domain);
+	struct tegra_pmc *pmc = powergate->pmc;
+	int err;
+
+	dev_dbg(pmc->dev, "> %s(domain=%p)\n", __func__, domain);
+	dev_dbg(pmc->dev, "  name: %s\n", domain->name);
+
+	if (powergate->need_vdd) {
+		err = get_regulator(powergate);
+		if (!err) {
+			err = regulator_enable(powergate->vdd);
+		}
+	} else {
+		err = tegra_pmc_powergate_set(powergate, true);
+	}
+	if (err < 0)
+		goto out;
+	udelay(10);
+
+	err = tegra_pmc_powergate_enable_clocks(powergate);
+	if (err)
+		goto out;
+	udelay(10);
+
+	err = tegra_pmc_powergate_remove_clamping(powergate);
+	if (err)
+		goto out;
+	udelay(10);
+
+	err = tegra_pmc_powergate_reset_deassert(powergate);
+	if (err)
+		goto out;
+	udelay(10);
+
+	err = tegra_pmc_powergate_mc_flush_done(powergate);
+	if (err)
+		goto out;
+	udelay(10);
+
+	tegra_pmc_powergate_disable_clocks(powergate);
+
+	return 0;
+
+/* XXX more error handing */
+out:
+	dev_dbg(pmc->dev, "< %s() = %d\n", __func__, err);
+	return err;
+}
+
+static int tegra_pmc_powergate_power_off(struct generic_pm_domain *domain)
+{
+	struct tegra_powergate *powergate = to_powergate(domain);
+	struct tegra_pmc *pmc = powergate->pmc;
+	int err;
+
+	dev_dbg(pmc->dev, "> %s(domain=%p)\n", __func__, domain);
+	dev_dbg(pmc->dev, "  name: %s\n", domain->name);
+
+	/* never turn off this partition */
+	switch (powergate->id) {
+	case TEGRA_POWERGATE_CPU:
+	case TEGRA_POWERGATE_CPU1:
+	case TEGRA_POWERGATE_CPU2:
+	case TEGRA_POWERGATE_CPU3:
+	case TEGRA_POWERGATE_CPU0:
+	case TEGRA_POWERGATE_C0NC:
+	case TEGRA_POWERGATE_IRAM:
+		dev_dbg(pmc->dev, "not disabling always-on partition %s\n",
+			domain->name);
+		err = -EINVAL;
+		goto out;
+	}
+
+	err = tegra_pmc_powergate_enable_clocks(powergate);
+	if (err)
+		goto out;
+	udelay(10);
+
+	err = tegra_pmc_powergate_mc_flush(powergate);
+	if (err)
+		goto out;
+	udelay(10);
+
+	err = tegra_pmc_powergate_reset_assert(powergate);
+	if (err)
+		goto out;
+	udelay(10);
+
+	tegra_pmc_powergate_disable_clocks(powergate);
+	udelay(10);
+
+	if (powergate->vdd)
+		err = regulator_disable(powergate->vdd);
+	else
+		err = tegra_pmc_powergate_set(powergate, false);
+	if (err)
+		goto out;
+
+	return 0;
+
+/* XXX more error handling */
+out:
+	dev_dbg(pmc->dev, "< %s() = %d\n", __func__, err);
+	return err;
+}
+
 static int powergate_show(struct seq_file *s, void *data)
 {
 	unsigned int i;
@@ -429,6 +783,231 @@ static int tegra_powergate_debugfs_init(void)
 	return 0;
 }
 
+static struct generic_pm_domain *
+tegra_powergate_of_xlate(struct of_phandle_args *args, void *data)
+{
+	struct tegra_pmc *pmc = data;
+	struct tegra_powergate *powergate;
+
+	dev_dbg(pmc->dev, "> %s(args=%p, data=%p)\n", __func__, args, data);
+
+	list_for_each_entry(powergate, &pmc->powergate_list, head) {
+		if (!powergate->base.name)
+			continue;
+
+		if (powergate->id == args->args[0]) {
+			dev_dbg(pmc->dev, "< %s() = %p\n", __func__, powergate);
+			return &powergate->base;
+		}
+	}
+
+	dev_dbg(pmc->dev, "< %s() = -ENOENT\n", __func__);
+	return ERR_PTR(-ENOENT);
+}
+
+static int of_get_clks(struct tegra_powergate *powergate)
+{
+	struct clk *clk;
+	int i;
+
+	for (i = 0; i < MAX_CLK_NUM; i++) {
+		clk = of_clk_get(powergate->of_node, i);
+		if (IS_ERR(clk)) {
+			if (PTR_ERR(clk) == -ENOENT)
+				break;
+			else
+				return PTR_ERR(clk);
+		}
+
+		powergate->clk[i] = clk;
+	}
+
+	return 0;
+}
+
+static int of_get_resets(struct tegra_powergate *powergate)
+{
+	struct reset_control *reset;
+	int i;
+
+	for (i = 0; i < MAX_RESET_NUM; i++) {
+		reset = of_reset_control_get_by_index(powergate->of_node, i);
+		if (IS_ERR(reset)) {
+			if (PTR_ERR(reset) == -ENOENT)
+				break;
+			else
+				return PTR_ERR(reset);
+		}
+
+		powergate->reset[i] = reset;
+	}
+
+	return 0;
+}
+
+static int of_get_swgroups(struct tegra_powergate *powergate)
+{
+	struct tegra_mc_swgroup *sg;
+	int i;
+
+	for (i = 0; i < MAX_SWGROUP_NUM; i++) {
+		sg = tegra_mc_find_swgroup(powergate->of_node, i);
+		if (IS_ERR_OR_NULL(sg)) {
+			if (PTR_ERR(sg) == -ENOENT)
+				break;
+			else
+				return -EINVAL;
+		}
+
+		powergate->swgroup[i] = sg;
+	}
+
+	return 0;
+}
+
+static int tegra_pmc_powergate_init_powerdomain(struct tegra_pmc *pmc)
+{
+	struct device_node *np;
+
+	for_each_compatible_node(np, NULL, "nvidia,power-domains") {
+		struct tegra_powergate *powergate;
+		const char *name;
+		int err;
+		u32 id;
+		bool off;
+
+		err = of_property_read_string(np, "name", &name);
+		if (err) {
+			dev_err(pmc->dev, "no significant name for domain\n");
+			return err;
+		}
+
+		err = of_property_read_u32(np, "domain", &id);
+		if (err) {
+			dev_err(pmc->dev, "no powergate ID for domain\n");
+			return err;
+		}
+
+		powergate = devm_kzalloc(pmc->dev, sizeof(*powergate), GFP_KERNEL);
+		if (!powergate) {
+			dev_err(pmc->dev, "failed to allocate memory for domain %s\n",
+					name);
+			return -ENOMEM;
+		}
+
+		if (of_property_read_bool(np, "external-power-rail")) {
+			powergate->need_vdd = true;
+			err = get_regulator(powergate);
+			if (err) {
+				/* The regulator might not be ready yet, so just give a
+				 * warning instead of failing the whole init.
+				 */
+				dev_warn(pmc->dev, "couldn't locate regulator\n");
+			}
+		}
+
+		powergate->of_node = np;
+		powergate->name = name;
+		powergate->id = id;
+		powergate->base.name = kstrdup(powergate->name, GFP_KERNEL);
+		powergate->base.power_off = tegra_pmc_powergate_power_off;
+		powergate->base.power_on = tegra_pmc_powergate_power_on;
+		powergate->pmc = pmc;
+
+		err = of_get_clks(powergate);
+		if (err)
+			return err;
+
+		err = of_get_resets(powergate);
+		if (err)
+			return err;
+
+		err = of_get_swgroups(powergate);
+		if (err)
+			return err;
+
+		list_add_tail(&powergate->head, &pmc->powergate_list);
+
+		/* XXX */
+		if ((powergate->need_vdd && !IS_ERR(powergate->vdd)) ||
+			!powergate->need_vdd)
+			tegra_pmc_powergate_power_off(&powergate->base);
+
+		off = !tegra_pmc_powergate_is_powered(powergate);
+		pm_genpd_init(&powergate->base, NULL, off);
+
+		pmc->power_domain_num++;
+
+		dev_info(pmc->dev, "added power domain %d\n", powergate->id);
+	}
+
+	dev_info(pmc->dev, "%d power domains added\n", pmc->power_domain_num);
+	return 0;
+}
+
+static int tegra_pmc_powergate_init_subdomain(struct tegra_pmc *pmc)
+{
+	struct tegra_powergate *powergate;
+
+	list_for_each_entry(powergate, &pmc->powergate_list, head) {
+		struct device_node *pdn;
+		struct tegra_powergate *parent = NULL;
+		struct tegra_powergate *temp;
+		int err;
+
+		/* XXX might depend-on more than one domain */
+		pdn = of_parse_phandle(powergate->of_node, "depend-on", 0);
+		if (!pdn)
+			continue;
+
+		list_for_each_entry(temp, &pmc->powergate_list, head) {
+			if (temp->of_node == pdn) {
+				parent = temp;
+				break;
+			}
+		}
+
+		if (!parent)
+			return -EINVAL;
+
+		err = pm_genpd_add_subdomain_names(parent->name, powergate->name);
+		if (err)
+			return err;
+	}
+
+	return 0;
+}
+
+static int tegra_powergate_init(struct tegra_pmc *pmc)
+{
+	struct device_node *np = pmc->dev->of_node;
+	int err = 0;
+
+	dev_dbg(pmc->dev, "> %s(pmc=%p)\n", __func__, pmc);
+
+	INIT_LIST_HEAD(&pmc->powergate_list);
+	err = tegra_pmc_powergate_init_powerdomain(pmc);
+	if (err)
+		goto out;
+
+	err = tegra_pmc_powergate_init_subdomain(pmc);
+	if (err < 0)
+		return err;
+
+	err = __of_genpd_add_provider(np, tegra_powergate_of_xlate, pmc);
+	if (err < 0)
+		return err;
+
+#if 0
+	pmc->nb.notifier_call = tegra_powergate_notifier_call;
+	bus_register_notifier(&platform_bus_type, &pmc->nb);
+#endif
+
+out:
+	dev_dbg(pmc->dev, "< %s() = %d\n", __func__, err);
+	return err;
+}
+
 static int tegra_io_rail_prepare(int id, unsigned long *request,
 				 unsigned long *status, unsigned int *bit)
 {
@@ -709,6 +1288,8 @@ static int tegra_pmc_probe(struct platform_device *pdev)
 	struct resource *res;
 	int err;
 
+	dev_dbg(&pdev->dev, "> %s(pdev=%p)\n", __func__, pdev);
+
 	err = tegra_pmc_parse_dt(pmc, pdev->dev.of_node);
 	if (err < 0)
 		return err;
@@ -728,14 +1309,22 @@ static int tegra_pmc_probe(struct platform_device *pdev)
 		return err;
 	}
 
+	pmc->dev = &pdev->dev;
 	tegra_pmc_init(pmc);
 
+	if (IS_ENABLED(CONFIG_PM_GENERIC_DOMAINS)) {
+		err = tegra_powergate_init(pmc);
+		if (err < 0)
+			return err;
+	}
+
 	if (IS_ENABLED(CONFIG_DEBUG_FS)) {
 		err = tegra_powergate_debugfs_init();
 		if (err < 0)
 			return err;
 	}
 
+	dev_dbg(&pdev->dev, "< %s()\n", __func__);
 	return 0;
 }
 
diff --git a/include/dt-bindings/power/tegra-powergate.h b/include/dt-bindings/power/tegra-powergate.h
new file mode 100644
index 000000000000..b8265167c20e
--- /dev/null
+++ b/include/dt-bindings/power/tegra-powergate.h
@@ -0,0 +1,30 @@
+#ifndef _DT_BINDINGS_POWER_TEGRA_POWERGATE_H
+#define _DT_BINDINGS_POWER_TEGRA_POWERGATE_H
+
+#define TEGRA_POWERGATE_CPU	0
+#define TEGRA_POWERGATE_3D	1
+#define TEGRA_POWERGATE_VENC	2
+#define TEGRA_POWERGATE_PCIE	3
+#define TEGRA_POWERGATE_VDEC	4
+#define TEGRA_POWERGATE_L2	5
+#define TEGRA_POWERGATE_MPE	6
+#define TEGRA_POWERGATE_HEG	7
+#define TEGRA_POWERGATE_SATA	8
+#define TEGRA_POWERGATE_CPU1	9
+#define TEGRA_POWERGATE_CPU2	10
+#define TEGRA_POWERGATE_CPU3	11
+#define TEGRA_POWERGATE_CELP	12
+#define TEGRA_POWERGATE_3D1	13
+#define TEGRA_POWERGATE_CPU0	14
+#define TEGRA_POWERGATE_C0NC	15
+#define TEGRA_POWERGATE_C1NC	16
+#define TEGRA_POWERGATE_SOR	17
+#define TEGRA_POWERGATE_DIS	18
+#define TEGRA_POWERGATE_DISB	19
+#define TEGRA_POWERGATE_XUSBA	20
+#define TEGRA_POWERGATE_XUSBB	21
+#define TEGRA_POWERGATE_XUSBC	22
+#define TEGRA_POWERGATE_VIC	23
+#define TEGRA_POWERGATE_IRAM	24
+
+#endif
-- 
1.9.1

[RFC PATCH 5/9] ARM: tegra: add PM domain device nodes to Tegra124 DT

[Vince Hsu]

Also bind the PM domain provider and consumer together.

Signed-off-by: Vince Hsu <vinceh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
 arch/arm/boot/dts/tegra124.dtsi | 73 ++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 72 insertions(+), 1 deletion(-)

diff --git a/arch/arm/boot/dts/tegra124.dtsi b/arch/arm/boot/dts/tegra124.dtsi
index 4be06c6ea0c8..584307b2953e 100644
--- a/arch/arm/boot/dts/tegra124.dtsi
+++ b/arch/arm/boot/dts/tegra124.dtsi
@@ -3,6 +3,7 @@
 #include <dt-bindings/memory/tegra124-mc.h>
 #include <dt-bindings/pinctrl/pinctrl-tegra.h>
 #include <dt-bindings/pinctrl/pinctrl-tegra-xusb.h>
+#include <dt-bindings/power/tegra-powergate.h>
 #include <dt-bindings/interrupt-controller/arm-gic.h>
 #include <dt-bindings/thermal/tegra124-soctherm.h>
 
@@ -39,6 +40,8 @@
 			  0x82000000 0 0x13000000 0x0 0x13000000 0 0x0d000000   /* non-prefetchable memory (208 MiB) */
 			  0xc2000000 0 0x20000000 0x0 0x20000000 0 0x20000000>; /* prefetchable memory (512 MiB) */
 
+		power-domains = <&pmc TEGRA_POWERGATE_PCIE>;
+
 		clocks = <&tegra_car TEGRA124_CLK_PCIE>,
 			 <&tegra_car TEGRA124_CLK_AFI>,
 			 <&tegra_car TEGRA124_CLK_PLL_E>,
@@ -98,6 +101,7 @@
 			compatible = "nvidia,tegra124-dc";
 			reg = <0x0 0x54200000 0x0 0x00040000>;
 			interrupts = <GIC_SPI 73 IRQ_TYPE_LEVEL_HIGH>;
+			power-domains = <&pmc TEGRA_POWERGATE_DIS>;
 			clocks = <&tegra_car TEGRA124_CLK_DISP1>,
 				 <&tegra_car TEGRA124_CLK_PLL_P>;
 			clock-names = "dc", "parent";
@@ -187,6 +191,7 @@
 		clock-names = "gpu", "pwr";
 		resets = <&tegra_car 184>;
 		reset-names = "gpu";
+		power-domains = <&pmc TEGRA_POWERGATE_3D>;
 		status = "disabled";
 	};
 
@@ -542,11 +547,75 @@
 		clocks = <&tegra_car TEGRA124_CLK_RTC>;
 	};
 
-	pmc@0,7000e400 {
+	pmc: pmc@0,7000e400 {
 		compatible = "nvidia,tegra124-pmc";
 		reg = <0x0 0x7000e400 0x0 0x400>;
 		clocks = <&tegra_car TEGRA124_CLK_PCLK>, <&clk32k_in>;
 		clock-names = "pclk", "clk32k_in";
+		#power-domain-cells = <1>;
+	};
+
+	dc-power-domain {
+		compatible = "nvidia,power-domains";
+		name = "dc-power-domain";
+		domain = <TEGRA_POWERGATE_DIS>;
+		clocks = <&tegra_car TEGRA124_CLK_DISP1>;
+		resets = <&tegra_car 27>;
+		nvidia,swgroup = <&mc TEGRA_SWGROUP_DC>;
+		depend-on = <&sorpd>;
+	};
+
+	pcie-power-domain {
+		compatible = "nvidia,power-domains";
+		name = "pcie-power-domain";
+		domain = <TEGRA_POWERGATE_PCIE>;
+		clocks = <&tegra_car TEGRA124_CLK_PCIE>,
+			 <&tegra_car TEGRA124_CLK_AFI>,
+			 <&tegra_car TEGRA124_CLK_PLL_E>,
+			 <&tegra_car TEGRA124_CLK_CML0>;
+		resets = <&tegra_car 70>,
+			 <&tegra_car 72>,
+			 <&tegra_car 74>;
+		nvidia,swgroup = <&mc TEGRA_SWGROUP_AFI>;
+	};
+
+	sorpd: sor-power-domain {
+		compatible = "nvidia,power-domains";
+		name = "sor-power-domain";
+		domain = <TEGRA_POWERGATE_SOR>;
+		clocks = <&tegra_car TEGRA124_CLK_SOR0>,
+			 <&tegra_car TEGRA124_CLK_DSIA>,
+			 <&tegra_car TEGRA124_CLK_DSIB>,
+			 <&tegra_car TEGRA124_CLK_HDMI>,
+			 <&tegra_car TEGRA124_CLK_MIPI_CAL>,
+			 <&tegra_car TEGRA124_CLK_DPAUX>;
+		resets = <&tegra_car 182>,
+			<&tegra_car 48>,
+			<&tegra_car 82>,
+			<&tegra_car 51>,
+			<&tegra_car 56>;
+	};
+
+	gpu-power-domain {
+		compatible = "nvidia,power-domains";
+		name = "gpu-power-domain";
+		domain = <TEGRA_POWERGATE_3D>;
+		clocks = <&tegra_car TEGRA124_CLK_GPU>,
+			 <&tegra_car TEGRA124_CLK_PLL_P_OUT5>;
+		resets = <&tegra_car 184>;
+		external-power-rail;
+	};
+
+	sata-power-domain {
+		compatible = "nvidia,power-domains";
+		name = "sata-power-domain";
+		domain = <TEGRA_POWERGATE_SATA>;
+		clocks = <&tegra_car TEGRA124_CLK_SATA>,
+			<&tegra_car TEGRA124_CLK_SATA_OOB>,
+			<&tegra_car TEGRA124_CLK_CML1>;
+		resets = <&tegra_car 124>,
+			<&tegra_car 123>,
+			<&tegra_car 129>;
 	};
 
 	fuse@0,7000f800 {
@@ -588,6 +657,8 @@
 			<&tegra_car 129>;
 		reset-names = "sata", "sata-oob", "sata-cold";
 
+		power-domains = <&pmc TEGRA_POWERGATE_SATA>;
+
 		phys = <&padctl TEGRA_XUSB_PADCTL_SATA>;
 		phy-names = "sata-phy";
 
-- 
1.9.1

[RFC PATCH 6/9] ARM: tegra: add GPU power supply to Jetson TK1 DT

[Vince Hsu]

Add power supply information which is board dependent for GK20A.

Signed-off-by: Vince Hsu <vinceh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
 arch/arm/boot/dts/tegra124-jetson-tk1.dts | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/arch/arm/boot/dts/tegra124-jetson-tk1.dts b/arch/arm/boot/dts/tegra124-jetson-tk1.dts
index 4eb540be368f..76fbb46784b3 100644
--- a/arch/arm/boot/dts/tegra124-jetson-tk1.dts
+++ b/arch/arm/boot/dts/tegra124-jetson-tk1.dts
@@ -1675,6 +1675,10 @@
 		nvidia,sys-clock-req-active-high;
 	};
 
+	gpu-power-domain {
+		vdd-supply = <&vdd_gpu>;
+	};
+
 	/* Serial ATA */
 	sata@0,70020000 {
 		status = "okay";
-- 
1.9.1

[RFC PATCH 7/9] drm/tegra: dc: remove the power sequence from driver

[Vince Hsu]

We have the generic PM domain support for Tegra SoCs now. So remove the
duplicated power sequence here.

Signed-off-by: Vince Hsu <vinceh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
 drivers/gpu/drm/tegra/dc.c | 46 +++++++++++-----------------------------------
 1 file changed, 11 insertions(+), 35 deletions(-)

diff --git a/drivers/gpu/drm/tegra/dc.c b/drivers/gpu/drm/tegra/dc.c
index ae26cc054fff..078373336977 100644
--- a/drivers/gpu/drm/tegra/dc.c
+++ b/drivers/gpu/drm/tegra/dc.c
@@ -25,7 +25,6 @@ struct tegra_dc_soc_info {
 	bool supports_cursor;
 	bool supports_block_linear;
 	unsigned int pitch_align;
-	bool has_powergate;
 };
 
 struct tegra_plane {
@@ -1575,7 +1574,6 @@ static const struct tegra_dc_soc_info tegra20_dc_soc_info = {
 	.supports_cursor = false,
 	.supports_block_linear = false,
 	.pitch_align = 8,
-	.has_powergate = false,
 };
 
 static const struct tegra_dc_soc_info tegra30_dc_soc_info = {
@@ -1583,7 +1581,6 @@ static const struct tegra_dc_soc_info tegra30_dc_soc_info = {
 	.supports_cursor = false,
 	.supports_block_linear = false,
 	.pitch_align = 8,
-	.has_powergate = false,
 };
 
 static const struct tegra_dc_soc_info tegra114_dc_soc_info = {
@@ -1591,7 +1588,6 @@ static const struct tegra_dc_soc_info tegra114_dc_soc_info = {
 	.supports_cursor = false,
 	.supports_block_linear = false,
 	.pitch_align = 64,
-	.has_powergate = true,
 };
 
 static const struct tegra_dc_soc_info tegra124_dc_soc_info = {
@@ -1599,7 +1595,6 @@ static const struct tegra_dc_soc_info tegra124_dc_soc_info = {
 	.supports_cursor = true,
 	.supports_block_linear = true,
 	.pitch_align = 64,
-	.has_powergate = true,
 };
 
 static const struct of_device_id tegra_dc_of_match[] = {
@@ -1692,33 +1687,18 @@ static int tegra_dc_probe(struct platform_device *pdev)
 		return PTR_ERR(dc->rst);
 	}
 
-	if (dc->soc->has_powergate) {
-		if (dc->pipe == 0)
-			dc->powergate = TEGRA_POWERGATE_DIS;
-		else
-			dc->powergate = TEGRA_POWERGATE_DISB;
-
-		err = tegra_powergate_sequence_power_up(dc->powergate, dc->clk,
-							dc->rst);
-		if (err < 0) {
-			dev_err(&pdev->dev, "failed to power partition: %d\n",
-				err);
-			return err;
-		}
-	} else {
-		err = clk_prepare_enable(dc->clk);
-		if (err < 0) {
-			dev_err(&pdev->dev, "failed to enable clock: %d\n",
-				err);
-			return err;
-		}
+	err = clk_prepare_enable(dc->clk);
+	if (err < 0) {
+		dev_err(&pdev->dev, "failed to enable clock: %d\n",
+			err);
+		return err;
+	}
 
-		err = reset_control_deassert(dc->rst);
-		if (err < 0) {
-			dev_err(&pdev->dev, "failed to deassert reset: %d\n",
-				err);
-			return err;
-		}
+	err = reset_control_deassert(dc->rst);
+	if (err < 0) {
+		dev_err(&pdev->dev, "failed to deassert reset: %d\n",
+			err);
+		return err;
 	}
 
 	regs = platform_get_resource(pdev, IORESOURCE_MEM, 0);
@@ -1773,10 +1753,6 @@ static int tegra_dc_remove(struct platform_device *pdev)
 	}
 
 	reset_control_assert(dc->rst);
-
-	if (dc->soc->has_powergate)
-		tegra_powergate_power_off(dc->powergate);
-
 	clk_disable_unprepare(dc->clk);
 
 	return 0;
-- 
1.9.1

[RFC PATCH 8/9] PCI: tegra: remove the power sequence from driver

[Vince Hsu]

We have the generic PM domain support for Tegra SoCs now. So remove the
duplicated power sequence here.

Signed-off-by: Vince Hsu <vinceh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
 drivers/pci/host/pci-tegra.c | 20 --------------------
 1 file changed, 20 deletions(-)

diff --git a/drivers/pci/host/pci-tegra.c b/drivers/pci/host/pci-tegra.c
index 6f9c29fa70e7..22ef3cd61aa8 100644
--- a/drivers/pci/host/pci-tegra.c
+++ b/drivers/pci/host/pci-tegra.c
@@ -980,12 +980,6 @@ static void tegra_pcie_power_off(struct tegra_pcie *pcie)
 	if (err < 0)
 		dev_warn(pcie->dev, "failed to power off PHY: %d\n", err);
 
-	reset_control_assert(pcie->pcie_xrst);
-	reset_control_assert(pcie->afi_rst);
-	reset_control_assert(pcie->pex_rst);
-
-	tegra_powergate_power_off(TEGRA_POWERGATE_PCIE);
-
 	err = regulator_bulk_disable(pcie->num_supplies, pcie->supplies);
 	if (err < 0)
 		dev_warn(pcie->dev, "failed to disable regulators: %d\n", err);
@@ -996,25 +990,11 @@ static int tegra_pcie_power_on(struct tegra_pcie *pcie)
 	const struct tegra_pcie_soc_data *soc = pcie->soc_data;
 	int err;
 
-	reset_control_assert(pcie->pcie_xrst);
-	reset_control_assert(pcie->afi_rst);
-	reset_control_assert(pcie->pex_rst);
-
-	tegra_powergate_power_off(TEGRA_POWERGATE_PCIE);
-
 	/* enable regulators */
 	err = regulator_bulk_enable(pcie->num_supplies, pcie->supplies);
 	if (err < 0)
 		dev_err(pcie->dev, "failed to enable regulators: %d\n", err);
 
-	err = tegra_powergate_sequence_power_up(TEGRA_POWERGATE_PCIE,
-						pcie->pex_clk,
-						pcie->pex_rst);
-	if (err) {
-		dev_err(pcie->dev, "powerup sequence failed: %d\n", err);
-		return err;
-	}
-
 	reset_control_deassert(pcie->afi_rst);
 
 	err = clk_prepare_enable(pcie->afi_clk);
-- 
1.9.1

[RFC PATCH 9/9] ata: ahci_tegra: remove power sequence from driver

[Vince Hsu]

We have the generic PM domain support for Tegra SoCs now. So remove the
duplicated sequence here.

Signed-off-by: Vince Hsu <vinceh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
 drivers/ata/ahci_tegra.c | 11 -----------
 1 file changed, 11 deletions(-)

diff --git a/drivers/ata/ahci_tegra.c b/drivers/ata/ahci_tegra.c
index 032904402c95..8db4aebcda4e 100644
--- a/drivers/ata/ahci_tegra.c
+++ b/drivers/ata/ahci_tegra.c
@@ -118,12 +118,6 @@ static int tegra_ahci_power_on(struct ahci_host_priv *hpriv)
 	if (ret)
 		return ret;
 
-	ret = tegra_powergate_sequence_power_up(TEGRA_POWERGATE_SATA,
-						tegra->sata_clk,
-						tegra->sata_rst);
-	if (ret)
-		goto disable_regulators;
-
 	reset_control_assert(tegra->sata_oob_rst);
 	reset_control_assert(tegra->sata_cold_rst);
 
@@ -138,10 +132,6 @@ static int tegra_ahci_power_on(struct ahci_host_priv *hpriv)
 
 disable_power:
 	clk_disable_unprepare(tegra->sata_clk);
-
-	tegra_powergate_power_off(TEGRA_POWERGATE_SATA);
-
-disable_regulators:
 	regulator_bulk_disable(ARRAY_SIZE(tegra->supplies), tegra->supplies);
 
 	return ret;
@@ -158,7 +148,6 @@ static void tegra_ahci_power_off(struct ahci_host_priv *hpriv)
 	reset_control_assert(tegra->sata_cold_rst);
 
 	clk_disable_unprepare(tegra->sata_clk);
-	tegra_powergate_power_off(TEGRA_POWERGATE_SATA);
 
 	regulator_bulk_disable(ARRAY_SIZE(tegra->supplies), tegra->supplies);
 }
-- 
1.9.1

Re: [RFC PATCH 1/9] reset: add of_reset_control_get_by_index()

[Alexandre Courbot]

On Wed, Jan 14, 2015 at 3:19 PM, Vince Hsu <vinceh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> wrote:
> Add of_reset_control_get_by_index() to allow the dirvers to get reset device

s/dirvers/drivers

> without knowing its name.
>
> Signed-off-by: Vince Hsu <vinceh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> ---
>  drivers/reset/core.c  | 47 ++++++++++++++++++++++++++++++++---------------
>  include/linux/reset.h |  9 +++++++++
>  2 files changed, 41 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/reset/core.c b/drivers/reset/core.c
> index 7955e00d04d4..25a0da1654f8 100644
> --- a/drivers/reset/core.c
> +++ b/drivers/reset/core.c
> @@ -140,28 +140,15 @@ int reset_control_status(struct reset_control *rstc)
>  }
>  EXPORT_SYMBOL_GPL(reset_control_status);
>
> -/**
> - * of_reset_control_get - Lookup and obtain a reference to a reset controller.
> - * @node: device to be reset by the controller
> - * @id: reset line name
> - *
> - * Returns a struct reset_control or IS_ERR() condition containing errno.
> - *
> - * Use of id names is optional.
> - */
> -struct reset_control *of_reset_control_get(struct device_node *node,
> -                                          const char *id)
> +struct reset_control *__of_reset_control_get(struct device_node *node,
> +                                               int index)
>  {
>         struct reset_control *rstc = ERR_PTR(-EPROBE_DEFER);
>         struct reset_controller_dev *r, *rcdev;
>         struct of_phandle_args args;
> -       int index = 0;
>         int rstc_id;
>         int ret;
>
> -       if (id)
> -               index = of_property_match_string(node,
> -                                                "reset-names", id);
>         ret = of_parse_phandle_with_args(node, "resets", "#reset-cells",
>                                          index, &args);
>         if (ret)
> @@ -202,6 +189,36 @@ struct reset_control *of_reset_control_get(struct device_node *node,
>
>         return rstc;
>  }
> +
> +struct reset_control *of_reset_control_get_by_index(struct device_node *node,
> +                                          int index)
> +{
> +       if (!node)
> +               return ERR_PTR(-EINVAL);

Here you are checking node...

> +
> +       return __of_reset_control_get(node, index);
> +}
> +EXPORT_SYMBOL_GPL(of_reset_control_get_by_index);
> +
> +/**
> + * of_reset_control_get - Lookup and obtain a reference to a reset controller.
> + * @node: device to be reset by the controller
> + * @id: reset line name
> + *
> + * Returns a struct reset_control or IS_ERR() condition containing errno.
> + *
> + * Use of id names is optional.
> + */
> +struct reset_control *of_reset_control_get(struct device_node *node,
> +                                          const char *id)
> +{
> +       int index = 0;
> +
> +       if (id)
> +               index = of_property_match_string(node,
> +                                                "reset-names", id);
> +       return __of_reset_control_get(node, index);

... but not here?

> +}
>  EXPORT_SYMBOL_GPL(of_reset_control_get);
>
>  /**
> diff --git a/include/linux/reset.h b/include/linux/reset.h
> index da5602bd77d7..f29fc88c4868 100644
> --- a/include/linux/reset.h
> +++ b/include/linux/reset.h
> @@ -38,6 +38,9 @@ static inline struct reset_control *devm_reset_control_get_optional(
>  struct reset_control *of_reset_control_get(struct device_node *node,
>                                            const char *id);
>
> +struct reset_control *of_reset_control_get_by_index(
> +                                       struct device_node *node, int index);
> +
>  #else
>
>  static inline int reset_control_reset(struct reset_control *rstc)
> @@ -92,6 +95,12 @@ static inline struct reset_control *of_reset_control_get(
>         return ERR_PTR(-ENOSYS);
>  }
>
> +struct reset_control *of_reset_control_get_by_index(
> +                               struct device_node *node, int index)

Seems like the node argument would fit on the previous line.

Re: [RFC PATCH 2/9] memory: tegra: add mc flush support

[Alexandre Courbot]

On Wed, Jan 14, 2015 at 3:19 PM, Vince Hsu <vinceh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> wrote:
> The flush operation of memory clients is needed for various IP blocks in
> the Tegra SoCs to perform a clean reset.
>
> Signed-off-by: Vince Hsu <vinceh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> ---
>  drivers/memory/tegra/mc.c       | 132 ++++++++++++++++++++++++++++++++++++++++
>  drivers/memory/tegra/tegra114.c |   2 +-
>  drivers/memory/tegra/tegra30.c  |   2 +-
>  include/soc/tegra/mc.h          |  41 ++++++++++++-
>  4 files changed, 173 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/memory/tegra/mc.c b/drivers/memory/tegra/mc.c
> index fe3c44e7e1d1..35f769f9f1cd 100644
> --- a/drivers/memory/tegra/mc.c
> +++ b/drivers/memory/tegra/mc.c
> @@ -11,6 +11,7 @@
>  #include <linux/kernel.h>
>  #include <linux/module.h>
>  #include <linux/of.h>
> +#include <linux/of_platform.h>
>  #include <linux/platform_device.h>
>  #include <linux/slab.h>
>
> @@ -62,6 +63,130 @@ static const struct of_device_id tegra_mc_of_match[] = {
>  };
>  MODULE_DEVICE_TABLE(of, tegra_mc_of_match);
>
> +static struct tegra_mc_swgroup *find_swgroup(struct tegra_mc *mc,
> +               unsigned int swgroup)

Indentation looks a little bit weird here.

> +{
> +       struct tegra_mc_swgroup *sg;
> +
> +       list_for_each_entry(sg, &mc->swgroups, head) {
> +               if (sg->id == swgroup)
> +                       return sg;
> +       }
> +
> +       return NULL;
> +}
> +
> +static struct tegra_mc_swgroup *add_swgroup(struct tegra_mc *mc,
> +       unsigned int swgroup)

Here too. Also even though they are static, these functions should
probably be prefixed with tegra_mc or something.

> +int tegra_mc_flush(struct tegra_mc_swgroup *sg)
> +{
> +       struct tegra_mc *mc;
> +       const struct tegra_mc_hotreset *client;
> +       int i;
> +
> +       if (!sg || !sg->mc)
> +               return -EINVAL;;
> +
> +       mc = sg->mc;
> +       if (!mc->soc->ops || !mc->soc->ops->flush)
> +               return -EINVAL;;
> +
> +       client = mc->soc->hotresets;
> +
> +       for (i = 0; i < mc->soc->num_hotresets; i++, client++) {
> +               if (sg->id == client->swgroup)
> +                       return mc->soc->ops->flush(mc, client);
> +       }
> +
> +       return -EINVAL;
> +}
> +EXPORT_SYMBOL(tegra_mc_flush);
> +
> +int tegra_mc_flush_done(struct tegra_mc_swgroup *sg)
> +{
> +       struct tegra_mc *mc;
> +       const struct tegra_mc_hotreset *client;
> +       int i;
> +
> +       if (!sg || !sg->mc)
> +               return -EINVAL;;
> +
> +       mc = sg->mc;
> +       if (!mc->soc->ops || !mc->soc->ops->flush)
> +               return -EINVAL;;
> +
> +       client = mc->soc->hotresets;
> +
> +       for (i = 0; i < mc->soc->num_hotresets; i++, client++) {
> +               if (sg->id == client->swgroup)
> +                       return mc->soc->ops->flush_done(mc, client);
> +       }
> +
> +       return -EINVAL;
> +}
> +EXPORT_SYMBOL(tegra_mc_flush_done);

These functions are identical, excepted for the callback they are
invoking. Could you merge the common part into a function that returns
the right client to call the callback on, or ERR_PTR(-EINVAL) in case
of failure?

> +
> +static int tegra_mc_build_swgroup(struct tegra_mc *mc)
> +{
> +       int i;
> +
> +       for (i = 0; i < mc->soc->num_clients; i++) {
> +               struct tegra_mc_swgroup *sg;
> +
> +               sg = find_swgroup(mc, mc->soc->clients[i].swgroup);
> +
> +               if (!sg) {
> +                       sg = add_swgroup(mc,  mc->soc->clients[i].swgroup);
> +                       if (IS_ERR(sg))
> +                               return PTR_ERR(sg);
> +               }
> +
> +               list_add_tail(&mc->soc->clients[i].head, &sg->clients);
> +       }
> +
> +       return 0;
> +}
> +
>  static int tegra_mc_setup_latency_allowance(struct tegra_mc *mc)
>  {
>         unsigned long long tick;
> @@ -229,6 +354,13 @@ static int tegra_mc_probe(struct platform_device *pdev)
>         /* length of MC tick in nanoseconds */
>         mc->tick = 30;
>
> +       INIT_LIST_HEAD(&mc->swgroups);
> +       err = tegra_mc_build_swgroup(mc);
> +       if (err) {
> +               dev_err(&pdev->dev, "failed to build swgroup: %d\n", err);
> +               return err;
> +       }
> +
>         res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>         mc->regs = devm_ioremap_resource(&pdev->dev, res);
>         if (IS_ERR(mc->regs))
> diff --git a/drivers/memory/tegra/tegra114.c b/drivers/memory/tegra/tegra114.c
> index 511e9a25c151..92ab5552fcee 100644
> --- a/drivers/memory/tegra/tegra114.c
> +++ b/drivers/memory/tegra/tegra114.c
> @@ -15,7 +15,7 @@
>
>  #include "mc.h"
>
> -static const struct tegra_mc_client tegra114_mc_clients[] = {
> +static struct tegra_mc_client tegra114_mc_clients[] = {

Why is this needed? I cannot see anything in this patch that would
require dropping the const here.

Re: [RFC PATCH 3/9] memory: tegra: add flush operation for Tegra124 memory clients

[Alexandre Courbot]

On Wed, Jan 14, 2015 at 3:19 PM, Vince Hsu <vinceh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> wrote:
> Signed-off-by: Vince Hsu <vinceh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>

Please have a short commit message even if the subject seems to be
self-explanatory.

> ---
>  drivers/memory/tegra/tegra124.c | 108 +++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 107 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/memory/tegra/tegra124.c b/drivers/memory/tegra/tegra124.c
> index 278d40b854c1..cce255d3df5c 100644
> --- a/drivers/memory/tegra/tegra124.c
> +++ b/drivers/memory/tegra/tegra124.c
> @@ -6,6 +6,8 @@
>   * published by the Free Software Foundation.
>   */
>
> +#include <linux/delay.h>
> +#include <linux/device.h>
>  #include <linux/of.h>
>  #include <linux/mm.h>
>
> @@ -15,7 +17,7 @@
>
>  #include "mc.h"
>
> -static const struct tegra_mc_client tegra124_mc_clients[] = {
> +static struct tegra_mc_client tegra124_mc_clients[] = {

This const drop also seems out-of-place. If they are needed at all,
please have them all done in the same patch, and only when they become
needed.

>         {
>                 .id = 0x00,
>                 .name = "ptcr",
> @@ -959,7 +961,108 @@ static const struct tegra_smmu_swgroup tegra124_swgroups[] = {
>         { .swgroup = TEGRA_SWGROUP_VI,        .reg = 0x280 },
>  };
>
> +static struct tegra_mc_hotreset tegra124_mc_hotreset[] = {
> +       {TEGRA_SWGROUP_AFI,        0x200, 0x204,  0},
> +       {TEGRA_SWGROUP_AVPC,       0x200, 0x204,  1},
> +       {TEGRA_SWGROUP_DC,         0x200, 0x204,  2},
> +       {TEGRA_SWGROUP_DCB,        0x200, 0x204,  3},
> +       {TEGRA_SWGROUP_HC,         0x200, 0x204,  6},
> +       {TEGRA_SWGROUP_HDA,        0x200, 0x204,  7},
> +       {TEGRA_SWGROUP_ISP2,       0x200, 0x204,  8},
> +       {TEGRA_SWGROUP_MPCORE,     0x200, 0x204,  9},
> +       {TEGRA_SWGROUP_MPCORELP,   0x200, 0x204, 10},
> +       {TEGRA_SWGROUP_MSENC,      0x200, 0x204, 11},
> +       {TEGRA_SWGROUP_PPCS,       0x200, 0x204, 14},
> +       {TEGRA_SWGROUP_SATA,       0x200, 0x204, 15},
> +       {TEGRA_SWGROUP_VDE,        0x200, 0x204, 16},
> +       {TEGRA_SWGROUP_VI,         0x200, 0x204, 17},
> +       {TEGRA_SWGROUP_VIC,        0x200, 0x204, 18},
> +       {TEGRA_SWGROUP_XUSB_HOST,  0x200, 0x204, 19},
> +       {TEGRA_SWGROUP_XUSB_DEV,   0x200, 0x204, 20},
> +       {TEGRA_SWGROUP_TSEC,       0x200, 0x204, 22},
> +       {TEGRA_SWGROUP_SDMMC1A,    0x200, 0x204, 29},
> +       {TEGRA_SWGROUP_SDMMC2A,    0x200, 0x204, 30},
> +       {TEGRA_SWGROUP_SDMMC3A,    0x200, 0x204, 31},
> +       {TEGRA_SWGROUP_SDMMC4A,    0x970, 0x974,  0},
> +       {TEGRA_SWGROUP_ISP2B,      0x970, 0x974,  1},
> +       {TEGRA_SWGROUP_GPU,        0x970, 0x974,  2},
> +};
> +
>  #ifdef CONFIG_ARCH_TEGRA_124_SOC
> +
> +static bool tegra124_stable_hotreset_check(struct tegra_mc *mc,
> +               u32 reg, u32 *stat)
> +{
> +       int i;
> +       u32 cur_stat;
> +       u32 prv_stat;
> +
> +       /*
> +        * There might be a glitch seen with the status register if we program
> +        * the control register and then read the status register in a short
> +        * window due to a HW bug. So here we poll for a stable status read.
> +        */
> +       prv_stat = mc_readl(mc, reg);
> +       for (i = 0; i < 5; i++) {

Why 5 times?

> +               cur_stat = mc_readl(mc, reg);
> +               if (cur_stat != prv_stat)
> +                       return false;
> +       }
> +       *stat = cur_stat;
> +       return true;
> +}
> +
> +static int tegra124_mc_flush(struct tegra_mc *mc,
> +               const struct tegra_mc_hotreset *hotreset)
> +{
> +       u32 val;
> +
> +       if (!mc || !hotreset)
> +               return -EINVAL;
> +
> +       /* XXX add mutex */

I guess this is a TODO for when this series exists the RFC state. :)

> +
> +       val = mc_readl(mc, hotreset->ctrl);
> +       val |= BIT(hotreset->bit);
> +       mc_writel(mc, val, hotreset->ctrl);
> +       mc_readl(mc, hotreset->ctrl);
> +
> +       /* poll till the flush is done */
> +       do {
> +               udelay(10);
> +               val = 0;
> +               if (!tegra124_stable_hotreset_check(mc, hotreset->status, &val))
> +                       continue;
> +       } while (!(val & BIT(hotreset->bit)));

So here you are waiting for the right bit in hotreset->status to turn
to 1. Why is the whole thing with tegra124_stable_hotreset_check()
needed? Could it switch from 1 to 0 to 1 again, with the first window
at 1 being invalid? In that case isn't there a risk that
tegra124_stable_hotreset_check() might do 5 consecutive reads on that
invalid state and let us continue before the flush is completed?

Re: [RFC PATCH 4/9] soc: tegra: pmc: Add generic PM domain support

[Alexandre Courbot]

On Wed, Jan 14, 2015 at 3:19 PM, Vince Hsu <vinceh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> wrote:
> From: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
>
> The PM domains are populated from DT, and the PM domain consumer devices are
> also bound to their relevant PM domains by DT.
>
> Signed-off-by: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> [vinceh: make changes based on Thierry and Peter's suggestions]
> Signed-off-by: Vince Hsu <vinceh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> ---
>  drivers/soc/tegra/pmc.c                     | 589 ++++++++++++++++++++++++++++
>  include/dt-bindings/power/tegra-powergate.h |  30 ++
>  2 files changed, 619 insertions(+)
>  create mode 100644 include/dt-bindings/power/tegra-powergate.h
>
> diff --git a/drivers/soc/tegra/pmc.c b/drivers/soc/tegra/pmc.c
> index 4bdc654bd747..0779b0ba6d3d 100644
> --- a/drivers/soc/tegra/pmc.c
> +++ b/drivers/soc/tegra/pmc.c
> @@ -17,6 +17,8 @@
>   *
>   */
>
> +#define DEBUG
> +
>  #include <linux/kernel.h>
>  #include <linux/clk.h>
>  #include <linux/clk/tegra.h>
> @@ -27,15 +29,20 @@
>  #include <linux/init.h>
>  #include <linux/io.h>
>  #include <linux/of.h>
> +#include <linux/of_platform.h>
>  #include <linux/of_address.h>
>  #include <linux/platform_device.h>
> +#include <linux/pm_domain.h>
>  #include <linux/reboot.h>
> +#include <linux/regulator/consumer.h>
>  #include <linux/reset.h>
> +#include <linux/sched.h>
>  #include <linux/seq_file.h>
>  #include <linux/spinlock.h>
>
>  #include <soc/tegra/common.h>
>  #include <soc/tegra/fuse.h>
> +#include <soc/tegra/mc.h>
>  #include <soc/tegra/pmc.h>
>
>  #define PMC_CNTRL                      0x0
> @@ -83,6 +90,30 @@
>
>  #define GPU_RG_CNTRL                   0x2d4
>
> +#define MAX_CLK_NUM            5
> +#define MAX_RESET_NUM          5
> +#define MAX_SWGROUP_NUM                5
> +
> +struct tegra_powergate {
> +       struct generic_pm_domain base;
> +       struct tegra_pmc *pmc;
> +       unsigned int id;
> +       const char *name;
> +       struct list_head head;
> +       struct device_node *of_node;
> +       struct clk *clk[MAX_CLK_NUM];
> +       struct reset_control *reset[MAX_RESET_NUM];
> +       struct tegra_mc_swgroup *swgroup[MAX_SWGROUP_NUM];
> +       bool need_vdd;
> +       struct regulator *vdd;
> +};
> +
> +static inline struct tegra_powergate *
> +to_powergate(struct generic_pm_domain *domain)
> +{
> +       return container_of(domain, struct tegra_powergate, base);
> +}
> +
>  struct tegra_pmc_soc {
>         unsigned int num_powergates;
>         const char *const *powergates;
> @@ -92,8 +123,10 @@ struct tegra_pmc_soc {
>
>  /**
>   * struct tegra_pmc - NVIDIA Tegra PMC
> + * @dev: pointer to parent device
>   * @base: pointer to I/O remapped register region
>   * @clk: pointer to pclk clock
> + * @soc: SoC-specific data
>   * @rate: currently configured rate of pclk
>   * @suspend_mode: lowest suspend mode available
>   * @cpu_good_time: CPU power good time (in microseconds)
> @@ -107,9 +140,12 @@ struct tegra_pmc_soc {
>   * @cpu_pwr_good_en: CPU power good signal is enabled
>   * @lp0_vec_phys: physical base address of the LP0 warm boot code
>   * @lp0_vec_size: size of the LP0 warm boot code
> + * @powergates: list of power gates
>   * @powergates_lock: mutex for power gate register access
> + * @nb: bus notifier for generic power domains
>   */
>  struct tegra_pmc {
> +       struct device *dev;
>         void __iomem *base;
>         struct clk *clk;
>
> @@ -130,7 +166,12 @@ struct tegra_pmc {
>         u32 lp0_vec_phys;
>         u32 lp0_vec_size;
>
> +       struct tegra_powergate *powergates;
>         struct mutex powergates_lock;
> +       struct notifier_block nb;
> +
> +       struct list_head powergate_list;
> +       int power_domain_num;
>  };
>
>  static struct tegra_pmc *pmc = &(struct tegra_pmc) {
> @@ -353,6 +394,8 @@ int tegra_pmc_cpu_remove_clamping(int cpuid)
>         if (id < 0)
>                 return id;
>
> +       usleep_range(10, 20);
> +
>         return tegra_powergate_remove_clamping(id);
>  }
>  #endif /* CONFIG_SMP */
> @@ -387,6 +430,317 @@ void tegra_pmc_restart(enum reboot_mode mode, const char *cmd)
>         tegra_pmc_writel(value, 0);
>  }
>
> +static bool tegra_pmc_powergate_is_powered(struct tegra_powergate *powergate)
> +{
> +       u32 status = tegra_pmc_readl(PWRGATE_STATUS);
> +
> +       if (!powergate->need_vdd)
> +               return (status & BIT(powergate->id)) != 0;
> +
> +       if (IS_ERR(powergate->vdd))
> +               return false;
> +       else
> +               return regulator_is_enabled(powergate->vdd);

status is only needed if !powervate->need_vdd. In the other case you
will have read PWRGATE_STATUS for nothing.

Actually, I would have (intuitively) expected that if need_vdd is
true, then both the right status bit must be set *and* the regulator
must be enabled, but reading the rest of this patch seems to confirm
that this is not the case. May I then suggest to rename "need_vdd"
into something more directly reflecting this, like "is_vdd".

> +}
> +
> +static int tegra_pmc_powergate_set(struct tegra_powergate *powergate,
> +                                  bool new_state)
> +{
> +       u32 status, mask = new_state ? BIT(powergate->id) : 0;
> +       bool state = false;
> +
> +       mutex_lock(&pmc->powergates_lock);
> +
> +       /* check the current state of the partition */
> +       status = tegra_pmc_readl(PWRGATE_STATUS);
> +       if (status & BIT(powergate->id))
> +               state = true;

state = !!(status & BIT(powergate->id)) ?

> +
> +       /* nothing to do */
> +       if (new_state == state) {
> +               mutex_unlock(&pmc->powergates_lock);
> +               return 0;
> +       }
> +
> +       /* toggle partition state and wait for state change to finish */
> +       tegra_pmc_writel(PWRGATE_TOGGLE_START | powergate->id, PWRGATE_TOGGLE);
> +
> +       while (1) {
> +               status = tegra_pmc_readl(PWRGATE_STATUS);
> +               if ((status & BIT(powergate->id)) == mask)
> +                       break;
> +
> +               usleep_range(10, 20);
> +       }

Might be nice to have a timeout and error report here, just in case...

> +
> +       mutex_unlock(&pmc->powergates_lock);
> +
> +       return 0;
> +}
> +
> +static int
> +tegra_pmc_powergate_remove_clamping(struct tegra_powergate *powergate)
> +{
> +       u32 mask;
> +
> +       /*
> +        * The Tegra124 GPU has a separate register (with different semantics)
> +        * to remove clamps.
> +        */
> +       if (tegra_get_chip_id() == TEGRA124) {
> +               if (powergate->id == TEGRA_POWERGATE_3D) {

if (tegra_get_chip_id() == TEGRA124 &&
    powergate->id == TEGRA_POWERGATE_3D) ?

> +                       tegra_pmc_writel(0, GPU_RG_CNTRL);
> +                       return 0;
> +               }
> +       }
> +
> +       /*
> +        * Tegra 2 has a bug where PCIE and VDE clamping masks are
> +        * swapped relatively to the partition ids
> +        */
> +       if (powergate->id == TEGRA_POWERGATE_VDEC)
> +               mask = (1 << TEGRA_POWERGATE_PCIE);
> +       else if (powergate->id == TEGRA_POWERGATE_PCIE)
> +               mask = (1 << TEGRA_POWERGATE_VDEC);
> +       else
> +               mask = (1 << powergate->id);

If this is only a Tegra 2 issue, shouldn't we test for the chip ID as well?

> +
> +       tegra_pmc_writel(mask, REMOVE_CLAMPING);
> +
> +       return 0;
> +}
> +
> +static int tegra_pmc_powergate_enable_clocks(
> +               struct tegra_powergate *powergate)
> +{
> +       int i, err;
> +
> +       for (i = 0; i < MAX_CLK_NUM; i++) {
> +               if (!powergate->clk[i])
> +                       break;
> +
> +               err = clk_prepare_enable(powergate->clk[i]);
> +               if (err)
> +                       goto out;
> +       }
> +
> +       return 0;
> +
> +out:
> +       while(i--)
> +               clk_disable_unprepare(powergate->clk[i]);
> +       return err;
> +}
> +
> +static void tegra_pmc_powergate_disable_clocks(
> +               struct tegra_powergate *powergate)
> +{
> +       int i;
> +
> +       for (i = 0; i < MAX_CLK_NUM; i++) {
> +               if (!powergate->clk[i])
> +                       break;

nit: shouldn't we disable the clocks in the reverse order of their enabling?

> +
> +               clk_disable_unprepare(powergate->clk[i]);
> +       }
> +}
> +
> +static int tegra_pmc_powergate_mc_flush(struct tegra_powergate *powergate)
> +{
> +       int i, err;
> +
> +       for (i = 0; i < MAX_SWGROUP_NUM; i++) {
> +               if (!powergate->swgroup[i])
> +                       break;
> +
> +               err = tegra_mc_flush(powergate->swgroup[i]);
> +               if (err)
> +                       return err;
> +       }
> +
> +       return 0;
> +}
> +
> +static int tegra_pmc_powergate_mc_flush_done(struct tegra_powergate *powergate)
> +{
> +       int i, err;
> +
> +       for (i = 0; i < MAX_SWGROUP_NUM; i++) {
> +               if (!powergate->swgroup[i])
> +                       break;
> +
> +               err = tegra_mc_flush_done(powergate->swgroup[i]);
> +               if (err)
> +                       return err;
> +       }
> +
> +       return 0;
> +
> +}
> +
> +static int tegra_pmc_powergate_reset_assert(
> +               struct tegra_powergate *powergate)
> +{
> +       int i, err;
> +
> +       for (i = 0; i < MAX_RESET_NUM; i++) {
> +               if (!powergate->reset[i])
> +                       break;
> +
> +               err = reset_control_assert(powergate->reset[i]);
> +               if (err)
> +                       return err;
> +       }
> +
> +       return 0;
> +}
> +
> +static int tegra_pmc_powergate_reset_deassert(
> +               struct tegra_powergate *powergate)
> +{
> +       int i, err;
> +
> +       for (i = 0; i < MAX_RESET_NUM; i++) {
> +               if (!powergate->reset[i])
> +                       break;
> +
> +               err = reset_control_deassert(powergate->reset[i]);
> +               if (err)
> +                       return err;
> +       }

Same remark for the reset assert/deassert order.

> +
> +       return 0;
> +}
> +
> +static int get_regulator(struct tegra_powergate *powergate)

Name confusingly close to regulator_get, please prefix.

> +{
> +       struct platform_device *pdev;
> +
> +       if (!powergate->need_vdd)
> +               return -EINVAL;
> +
> +       if (powergate->vdd && !IS_ERR(powergate->vdd))
> +               return 0;
> +
> +       pdev = of_find_device_by_node(powergate->of_node);
> +       if (!pdev)
> +               return -EINVAL;
> +
> +       powergate->vdd = devm_regulator_get_optional(&pdev->dev, "vdd");
> +       if (IS_ERR(powergate->vdd))
> +               return -EINVAL;
> +
> +       return 0;
> +}
> +
> +static int tegra_pmc_powergate_power_on(struct generic_pm_domain *domain)
> +{
> +       struct tegra_powergate *powergate = to_powergate(domain);
> +       struct tegra_pmc *pmc = powergate->pmc;
> +       int err;
> +
> +       dev_dbg(pmc->dev, "> %s(domain=%p)\n", __func__, domain);
> +       dev_dbg(pmc->dev, "  name: %s\n", domain->name);
> +
> +       if (powergate->need_vdd) {
> +               err = get_regulator(powergate);
> +               if (!err) {
> +                       err = regulator_enable(powergate->vdd);
> +               }
> +       } else {
> +               err = tegra_pmc_powergate_set(powergate, true);
> +       }
> +       if (err < 0)
> +               goto out;
> +       udelay(10);
> +
> +       err = tegra_pmc_powergate_enable_clocks(powergate);
> +       if (err)
> +               goto out;
> +       udelay(10);
> +
> +       err = tegra_pmc_powergate_remove_clamping(powergate);
> +       if (err)
> +               goto out;
> +       udelay(10);
> +
> +       err = tegra_pmc_powergate_reset_deassert(powergate);
> +       if (err)
> +               goto out;
> +       udelay(10);
> +
> +       err = tegra_pmc_powergate_mc_flush_done(powergate);
> +       if (err)
> +               goto out;
> +       udelay(10);
> +
> +       tegra_pmc_powergate_disable_clocks(powergate);
> +
> +       return 0;
> +
> +/* XXX more error handing */
> +out:
> +       dev_dbg(pmc->dev, "< %s() = %d\n", __func__, err);
> +       return err;
> +}
> +
> +static int tegra_pmc_powergate_power_off(struct generic_pm_domain *domain)
> +{
> +       struct tegra_powergate *powergate = to_powergate(domain);
> +       struct tegra_pmc *pmc = powergate->pmc;
> +       int err;
> +
> +       dev_dbg(pmc->dev, "> %s(domain=%p)\n", __func__, domain);
> +       dev_dbg(pmc->dev, "  name: %s\n", domain->name);
> +
> +       /* never turn off this partition */

s/this partition/these partitions

> +       switch (powergate->id) {
> +       case TEGRA_POWERGATE_CPU:
> +       case TEGRA_POWERGATE_CPU1:
> +       case TEGRA_POWERGATE_CPU2:
> +       case TEGRA_POWERGATE_CPU3:
> +       case TEGRA_POWERGATE_CPU0:
> +       case TEGRA_POWERGATE_C0NC:
> +       case TEGRA_POWERGATE_IRAM:
> +               dev_dbg(pmc->dev, "not disabling always-on partition %s\n",
> +                       domain->name);
> +               err = -EINVAL;
> +               goto out;
> +       }
> +
> +       err = tegra_pmc_powergate_enable_clocks(powergate);
> +       if (err)
> +               goto out;
> +       udelay(10);
> +
> +       err = tegra_pmc_powergate_mc_flush(powergate);
> +       if (err)
> +               goto out;
> +       udelay(10);
> +
> +       err = tegra_pmc_powergate_reset_assert(powergate);
> +       if (err)
> +               goto out;
> +       udelay(10);
> +
> +       tegra_pmc_powergate_disable_clocks(powergate);
> +       udelay(10);
> +
> +       if (powergate->vdd)
> +               err = regulator_disable(powergate->vdd);
> +       else
> +               err = tegra_pmc_powergate_set(powergate, false);
> +       if (err)
> +               goto out;
> +
> +       return 0;
> +
> +/* XXX more error handling */
> +out:
> +       dev_dbg(pmc->dev, "< %s() = %d\n", __func__, err);
> +       return err;
> +}
> +
>  static int powergate_show(struct seq_file *s, void *data)
>  {
>         unsigned int i;
> @@ -429,6 +783,231 @@ static int tegra_powergate_debugfs_init(void)
>         return 0;
>  }
>
> +static struct generic_pm_domain *
> +tegra_powergate_of_xlate(struct of_phandle_args *args, void *data)
> +{
> +       struct tegra_pmc *pmc = data;
> +       struct tegra_powergate *powergate;
> +
> +       dev_dbg(pmc->dev, "> %s(args=%p, data=%p)\n", __func__, args, data);
> +
> +       list_for_each_entry(powergate, &pmc->powergate_list, head) {
> +               if (!powergate->base.name)
> +                       continue;
> +
> +               if (powergate->id == args->args[0]) {
> +                       dev_dbg(pmc->dev, "< %s() = %p\n", __func__, powergate);
> +                       return &powergate->base;
> +               }
> +       }
> +
> +       dev_dbg(pmc->dev, "< %s() = -ENOENT\n", __func__);
> +       return ERR_PTR(-ENOENT);
> +}
> +
> +static int of_get_clks(struct tegra_powergate *powergate)

This function (and the few following ones) should also get a less
ambiguous name.

> +{
> +       struct clk *clk;
> +       int i;
> +
> +       for (i = 0; i < MAX_CLK_NUM; i++) {
> +               clk = of_clk_get(powergate->of_node, i);
> +               if (IS_ERR(clk)) {
> +                       if (PTR_ERR(clk) == -ENOENT)
> +                               break;
> +                       else
> +                               return PTR_ERR(clk);

... they should also probably free the resources they managed to
acquire in case of error.

I cannot comment on the DT bindings - Peter and Thierry are probably
the best persons for that. I just wonder if we should not define them
all under a global power-domains node that would be the only one to
match the compatible property whereas the childs would define the
individual domains (a bit like pinmux definitions). But globally the
idea seems sound to me.

Re: [RFC PATCH 1/9] reset: add of_reset_control_get_by_index()

[Vince Hsu]

Hi,

On 02/12/2015 04:56 PM, Alexandre Courbot wrote:
> On Wed, Jan 14, 2015 at 3:19 PM, Vince Hsu<vinceh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>  wrote:
>> Add of_reset_control_get_by_index() to allow the dirvers to get reset device
> s/dirvers/drivers
>
>> without knowing its name.
>>
>> Signed-off-by: Vince Hsu<vinceh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
>> ---
>>   drivers/reset/core.c  | 47 ++++++++++++++++++++++++++++++++---------------
>>   include/linux/reset.h |  9 +++++++++
>>   2 files changed, 41 insertions(+), 15 deletions(-)
>>
>> diff --git a/drivers/reset/core.c b/drivers/reset/core.c
>> index 7955e00d04d4..25a0da1654f8 100644
>> --- a/drivers/reset/core.c
>> +++ b/drivers/reset/core.c
>> @@ -140,28 +140,15 @@ int reset_control_status(struct reset_control *rstc)
>>   }
>>   EXPORT_SYMBOL_GPL(reset_control_status);
>>
>> -/**
>> - * of_reset_control_get - Lookup and obtain a reference to a reset controller.
>> - * @node: device to be reset by the controller
>> - * @id: reset line name
>> - *
>> - * Returns a struct reset_control or IS_ERR() condition containing errno.
>> - *
>> - * Use of id names is optional.
>> - */
>> -struct reset_control *of_reset_control_get(struct device_node *node,
>> -                                          const char *id)
>> +struct reset_control *__of_reset_control_get(struct device_node *node,
>> +                                               int index)
>>   {
>>          struct reset_control *rstc = ERR_PTR(-EPROBE_DEFER);
>>          struct reset_controller_dev *r, *rcdev;
>>          struct of_phandle_args args;
>> -       int index = 0;
>>          int rstc_id;
>>          int ret;
>>
>> -       if (id)
>> -               index = of_property_match_string(node,
>> -                                                "reset-names", id);
>>          ret = of_parse_phandle_with_args(node, "resets", "#reset-cells",
>>                                           index, &args);
>>          if (ret)
>> @@ -202,6 +189,36 @@ struct reset_control *of_reset_control_get(struct device_node *node,
>>
>>          return rstc;
>>   }
>> +
>> +struct reset_control *of_reset_control_get_by_index(struct device_node *node,
>> +                                          int index)
>> +{
>> +       if (!node)
>> +               return ERR_PTR(-EINVAL);
> Here you are checking node...
Will drop the check here. It's not needed actually.

>> +
>> +       return __of_reset_control_get(node, index);
>> +}
>> +EXPORT_SYMBOL_GPL(of_reset_control_get_by_index);
>> +
>> +/**
>> + * of_reset_control_get - Lookup and obtain a reference to a reset controller.
>> + * @node: device to be reset by the controller
>> + * @id: reset line name
>> + *
>> + * Returns a struct reset_control or IS_ERR() condition containing errno.
>> + *
>> + * Use of id names is optional.
>> + */
>> +struct reset_control *of_reset_control_get(struct device_node *node,
>> +                                          const char *id)
>> +{
>> +       int index = 0;
>> +
>> +       if (id)
>> +               index = of_property_match_string(node,
>> +                                                "reset-names", id);
>> +       return __of_reset_control_get(node, index);
> ... but not here?
>
>> +}
>>   EXPORT_SYMBOL_GPL(of_reset_control_get);
>>
>>   /**
>> diff --git a/include/linux/reset.h b/include/linux/reset.h
>> index da5602bd77d7..f29fc88c4868 100644
>> --- a/include/linux/reset.h
>> +++ b/include/linux/reset.h
>> @@ -38,6 +38,9 @@ static inline struct reset_control *devm_reset_control_get_optional(
>>   struct reset_control *of_reset_control_get(struct device_node *node,
>>                                             const char *id);
>>
>> +struct reset_control *of_reset_control_get_by_index(
>> +                                       struct device_node *node, int index);
>> +
>>   #else
>>
>>   static inline int reset_control_reset(struct reset_control *rstc)
>> @@ -92,6 +95,12 @@ static inline struct reset_control *of_reset_control_get(
>>          return ERR_PTR(-ENOSYS);
>>   }
>>
>> +struct reset_control *of_reset_control_get_by_index(
>> +                               struct device_node *node, int index)
> Seems like the node argument would fit on the previous line.
Indeed.

Thanks,
Vince

Re: [RFC PATCH 2/9] memory: tegra: add mc flush support

[Vince Hsu]

On 02/12/2015 04:56 PM, Alexandre Courbot wrote:
> On Wed, Jan 14, 2015 at 3:19 PM, Vince Hsu<vinceh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>  wrote:
>> The flush operation of memory clients is needed for various IP blocks in
>> the Tegra SoCs to perform a clean reset.
>>
>> Signed-off-by: Vince Hsu<vinceh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
>> ---
>>   drivers/memory/tegra/mc.c       | 132 ++++++++++++++++++++++++++++++++++++++++
>>   drivers/memory/tegra/tegra114.c |   2 +-
>>   drivers/memory/tegra/tegra30.c  |   2 +-
>>   include/soc/tegra/mc.h          |  41 ++++++++++++-
>>   4 files changed, 173 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/memory/tegra/mc.c b/drivers/memory/tegra/mc.c
>> index fe3c44e7e1d1..35f769f9f1cd 100644
>> --- a/drivers/memory/tegra/mc.c
>> +++ b/drivers/memory/tegra/mc.c
>> @@ -11,6 +11,7 @@
>>   #include <linux/kernel.h>
>>   #include <linux/module.h>
>>   #include <linux/of.h>
>> +#include <linux/of_platform.h>
>>   #include <linux/platform_device.h>
>>   #include <linux/slab.h>
>>
>> @@ -62,6 +63,130 @@ static const struct of_device_id tegra_mc_of_match[] = {
>>   };
>>   MODULE_DEVICE_TABLE(of, tegra_mc_of_match);
>>
>> +static struct tegra_mc_swgroup *find_swgroup(struct tegra_mc *mc,
>> +               unsigned int swgroup)
> Indentation looks a little bit weird here.
>
>> +{
>> +       struct tegra_mc_swgroup *sg;
>> +
>> +       list_for_each_entry(sg, &mc->swgroups, head) {
>> +               if (sg->id == swgroup)
>> +                       return sg;
>> +       }
>> +
>> +       return NULL;
>> +}
>> +
>> +static struct tegra_mc_swgroup *add_swgroup(struct tegra_mc *mc,
>> +       unsigned int swgroup)
> Here too. Also even though they are static, these functions should
> probably be prefixed with tegra_mc or something.
Will fix the indentation and function naming.

>> +int tegra_mc_flush(struct tegra_mc_swgroup *sg)
>> +{
>> +       struct tegra_mc *mc;
>> +       const struct tegra_mc_hotreset *client;
>> +       int i;
>> +
>> +       if (!sg || !sg->mc)
>> +               return -EINVAL;;
>> +
>> +       mc = sg->mc;
>> +       if (!mc->soc->ops || !mc->soc->ops->flush)
>> +               return -EINVAL;;
>> +
>> +       client = mc->soc->hotresets;
>> +
>> +       for (i = 0; i < mc->soc->num_hotresets; i++, client++) {
>> +               if (sg->id == client->swgroup)
>> +                       return mc->soc->ops->flush(mc, client);
>> +       }
>> +
>> +       return -EINVAL;
>> +}
>> +EXPORT_SYMBOL(tegra_mc_flush);
>> +
>> +int tegra_mc_flush_done(struct tegra_mc_swgroup *sg)
>> +{
>> +       struct tegra_mc *mc;
>> +       const struct tegra_mc_hotreset *client;
>> +       int i;
>> +
>> +       if (!sg || !sg->mc)
>> +               return -EINVAL;;
>> +
>> +       mc = sg->mc;
>> +       if (!mc->soc->ops || !mc->soc->ops->flush)
>> +               return -EINVAL;;
>> +
>> +       client = mc->soc->hotresets;
>> +
>> +       for (i = 0; i < mc->soc->num_hotresets; i++, client++) {
>> +               if (sg->id == client->swgroup)
>> +                       return mc->soc->ops->flush_done(mc, client);
>> +       }
>> +
>> +       return -EINVAL;
>> +}
>> +EXPORT_SYMBOL(tegra_mc_flush_done);
> These functions are identical, excepted for the callback they are
> invoking. Could you merge the common part into a function that returns
> the right client to call the callback on, or ERR_PTR(-EINVAL) in case
> of failure?
I couldn't think of a clever way to do this. Any ideas? :)

>> +
>> +static int tegra_mc_build_swgroup(struct tegra_mc *mc)
>> +{
>> +       int i;
>> +
>> +       for (i = 0; i < mc->soc->num_clients; i++) {
>> +               struct tegra_mc_swgroup *sg;
>> +
>> +               sg = find_swgroup(mc, mc->soc->clients[i].swgroup);
>> +
>> +               if (!sg) {
>> +                       sg = add_swgroup(mc,  mc->soc->clients[i].swgroup);
>> +                       if (IS_ERR(sg))
>> +                               return PTR_ERR(sg);
>> +               }
>> +
>> +               list_add_tail(&mc->soc->clients[i].head, &sg->clients);
>> +       }
>> +
>> +       return 0;
>> +}
>> +
>>   static int tegra_mc_setup_latency_allowance(struct tegra_mc *mc)
>>   {
>>          unsigned long long tick;
>> @@ -229,6 +354,13 @@ static int tegra_mc_probe(struct platform_device *pdev)
>>          /* length of MC tick in nanoseconds */
>>          mc->tick = 30;
>>
>> +       INIT_LIST_HEAD(&mc->swgroups);
>> +       err = tegra_mc_build_swgroup(mc);
>> +       if (err) {
>> +               dev_err(&pdev->dev, "failed to build swgroup: %d\n", err);
>> +               return err;
>> +       }
>> +
>>          res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>>          mc->regs = devm_ioremap_resource(&pdev->dev, res);
>>          if (IS_ERR(mc->regs))
>> diff --git a/drivers/memory/tegra/tegra114.c b/drivers/memory/tegra/tegra114.c
>> index 511e9a25c151..92ab5552fcee 100644
>> --- a/drivers/memory/tegra/tegra114.c
>> +++ b/drivers/memory/tegra/tegra114.c
>> @@ -15,7 +15,7 @@
>>
>>   #include "mc.h"
>>
>> -static const struct tegra_mc_client tegra114_mc_clients[] = {
>> +static struct tegra_mc_client tegra114_mc_clients[] = {
> Why is this needed? I cannot see anything in this patch that would
> require dropping the const here.
This patch uses a list in the struct tegra_mc_client to gather swgroup 
information from
DT when driver is probed. So the mc client array instance can't be constant.

Re: [RFC PATCH 3/9] memory: tegra: add flush operation for Tegra124 memory clients

[Vince Hsu]

Hi,

On 02/12/2015 04:56 PM, Alexandre Courbot wrote:
> On Wed, Jan 14, 2015 at 3:19 PM, Vince Hsu<vinceh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>  wrote:
>> Signed-off-by: Vince Hsu<vinceh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> Please have a short commit message even if the subject seems to be
> self-explanatory.
Will do.

>> ---
>>   drivers/memory/tegra/tegra124.c | 108 +++++++++++++++++++++++++++++++++++++++-
>>   1 file changed, 107 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/memory/tegra/tegra124.c b/drivers/memory/tegra/tegra124.c
>> index 278d40b854c1..cce255d3df5c 100644
>> --- a/drivers/memory/tegra/tegra124.c
>> +++ b/drivers/memory/tegra/tegra124.c
>> @@ -6,6 +6,8 @@
>>    * published by the Free Software Foundation.
>>    */
>>
>> +#include <linux/delay.h>
>> +#include <linux/device.h>
>>   #include <linux/of.h>
>>   #include <linux/mm.h>
>>
>> @@ -15,7 +17,7 @@
>>
>>   #include "mc.h"
>>
>> -static const struct tegra_mc_client tegra124_mc_clients[] = {
>> +static struct tegra_mc_client tegra124_mc_clients[] = {
> This const drop also seems out-of-place. If they are needed at all,
> please have them all done in the same patch, and only when they become
> needed.
Will move to the patch #2.

>>          {
>>                  .id = 0x00,
>>                  .name = "ptcr",
>> @@ -959,7 +961,108 @@ static const struct tegra_smmu_swgroup tegra124_swgroups[] = {
>>          { .swgroup = TEGRA_SWGROUP_VI,        .reg = 0x280 },
>>   };
>>
>> +static struct tegra_mc_hotreset tegra124_mc_hotreset[] = {
>> +       {TEGRA_SWGROUP_AFI,        0x200, 0x204,  0},
>> +       {TEGRA_SWGROUP_AVPC,       0x200, 0x204,  1},
>> +       {TEGRA_SWGROUP_DC,         0x200, 0x204,  2},
>> +       {TEGRA_SWGROUP_DCB,        0x200, 0x204,  3},
>> +       {TEGRA_SWGROUP_HC,         0x200, 0x204,  6},
>> +       {TEGRA_SWGROUP_HDA,        0x200, 0x204,  7},
>> +       {TEGRA_SWGROUP_ISP2,       0x200, 0x204,  8},
>> +       {TEGRA_SWGROUP_MPCORE,     0x200, 0x204,  9},
>> +       {TEGRA_SWGROUP_MPCORELP,   0x200, 0x204, 10},
>> +       {TEGRA_SWGROUP_MSENC,      0x200, 0x204, 11},
>> +       {TEGRA_SWGROUP_PPCS,       0x200, 0x204, 14},
>> +       {TEGRA_SWGROUP_SATA,       0x200, 0x204, 15},
>> +       {TEGRA_SWGROUP_VDE,        0x200, 0x204, 16},
>> +       {TEGRA_SWGROUP_VI,         0x200, 0x204, 17},
>> +       {TEGRA_SWGROUP_VIC,        0x200, 0x204, 18},
>> +       {TEGRA_SWGROUP_XUSB_HOST,  0x200, 0x204, 19},
>> +       {TEGRA_SWGROUP_XUSB_DEV,   0x200, 0x204, 20},
>> +       {TEGRA_SWGROUP_TSEC,       0x200, 0x204, 22},
>> +       {TEGRA_SWGROUP_SDMMC1A,    0x200, 0x204, 29},
>> +       {TEGRA_SWGROUP_SDMMC2A,    0x200, 0x204, 30},
>> +       {TEGRA_SWGROUP_SDMMC3A,    0x200, 0x204, 31},
>> +       {TEGRA_SWGROUP_SDMMC4A,    0x970, 0x974,  0},
>> +       {TEGRA_SWGROUP_ISP2B,      0x970, 0x974,  1},
>> +       {TEGRA_SWGROUP_GPU,        0x970, 0x974,  2},
>> +};
>> +
>>   #ifdef CONFIG_ARCH_TEGRA_124_SOC
>> +
>> +static bool tegra124_stable_hotreset_check(struct tegra_mc *mc,
>> +               u32 reg, u32 *stat)
>> +{
>> +       int i;
>> +       u32 cur_stat;
>> +       u32 prv_stat;
>> +
>> +       /*
>> +        * There might be a glitch seen with the status register if we program
>> +        * the control register and then read the status register in a short
>> +        * window due to a HW bug. So here we poll for a stable status read.
>> +        */
>> +       prv_stat = mc_readl(mc, reg);
>> +       for (i = 0; i < 5; i++) {
> Why 5 times?
This is a magic number from downstream. It should be enough to reach a 
stable state.

>> +               cur_stat = mc_readl(mc, reg);
>> +               if (cur_stat != prv_stat)
>> +                       return false;
>> +       }
>> +       *stat = cur_stat;
>> +       return true;
>> +}
>> +
>> +static int tegra124_mc_flush(struct tegra_mc *mc,
>> +               const struct tegra_mc_hotreset *hotreset)
>> +{
>> +       u32 val;
>> +
>> +       if (!mc || !hotreset)
>> +               return -EINVAL;
>> +
>> +       /* XXX add mutex */
> I guess this is a TODO for when this series exists the RFC state. :)
Oh, yes.

>> +
>> +       val = mc_readl(mc, hotreset->ctrl);
>> +       val |= BIT(hotreset->bit);
>> +       mc_writel(mc, val, hotreset->ctrl);
>> +       mc_readl(mc, hotreset->ctrl);
>> +
>> +       /* poll till the flush is done */
>> +       do {
>> +               udelay(10);
>> +               val = 0;
>> +               if (!tegra124_stable_hotreset_check(mc, hotreset->status, &val))
>> +                       continue;
>> +       } while (!(val & BIT(hotreset->bit)));
> So here you are waiting for the right bit in hotreset->status to turn
> to 1. Why is the whole thing with tegra124_stable_hotreset_check()
> needed? Could it switch from 1 to 0 to 1 again, with the first window
> at 1 being invalid?
There is a comment which describes why we have to check this in
tegra124_stable_hotreset_check().

> In that case isn't there a risk that
> tegra124_stable_hotreset_check() might do 5 consecutive reads on that
> invalid state and let us continue before the flush is completed?
So there is another check in while(). :)

Thanks,
Vince

Re: [RFC PATCH 4/9] soc: tegra: pmc: Add generic PM domain support

[Vince Hsu]

Hi,

On 02/12/2015 04:56 PM, Alexandre Courbot wrote:
> On Wed, Jan 14, 2015 at 3:19 PM, Vince Hsu<vinceh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>  wrote:
>> From: Thierry Reding<treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
>>
>> The PM domains are populated from DT, and the PM domain consumer devices are
>> also bound to their relevant PM domains by DT.
>>
>> Signed-off-by: Thierry Reding<treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
>> [vinceh: make changes based on Thierry and Peter's suggestions]
>> Signed-off-by: Vince Hsu<vinceh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
>> ---
>>   drivers/soc/tegra/pmc.c                     | 589 ++++++++++++++++++++++++++++
>>   include/dt-bindings/power/tegra-powergate.h |  30 ++
>>   2 files changed, 619 insertions(+)
>>   create mode 100644 include/dt-bindings/power/tegra-powergate.h
>>
>> diff --git a/drivers/soc/tegra/pmc.c b/drivers/soc/tegra/pmc.c
>> index 4bdc654bd747..0779b0ba6d3d 100644
>> --- a/drivers/soc/tegra/pmc.c
>> +++ b/drivers/soc/tegra/pmc.c
>> @@ -17,6 +17,8 @@
>>    *
>>    */
>>
>> +#define DEBUG
>> +
>>   #include <linux/kernel.h>
>>   #include <linux/clk.h>
>>   #include <linux/clk/tegra.h>
>> @@ -27,15 +29,20 @@
>>   #include <linux/init.h>
>>   #include <linux/io.h>
>>   #include <linux/of.h>
>> +#include <linux/of_platform.h>
>>   #include <linux/of_address.h>
>>   #include <linux/platform_device.h>
>> +#include <linux/pm_domain.h>
>>   #include <linux/reboot.h>
>> +#include <linux/regulator/consumer.h>
>>   #include <linux/reset.h>
>> +#include <linux/sched.h>
>>   #include <linux/seq_file.h>
>>   #include <linux/spinlock.h>
>>
>>   #include <soc/tegra/common.h>
>>   #include <soc/tegra/fuse.h>
>> +#include <soc/tegra/mc.h>
>>   #include <soc/tegra/pmc.h>
>>
>>   #define PMC_CNTRL                      0x0
>> @@ -83,6 +90,30 @@
>>
>>   #define GPU_RG_CNTRL                   0x2d4
>>
>> +#define MAX_CLK_NUM            5
>> +#define MAX_RESET_NUM          5
>> +#define MAX_SWGROUP_NUM                5
>> +
>> +struct tegra_powergate {
>> +       struct generic_pm_domain base;
>> +       struct tegra_pmc *pmc;
>> +       unsigned int id;
>> +       const char *name;
>> +       struct list_head head;
>> +       struct device_node *of_node;
>> +       struct clk *clk[MAX_CLK_NUM];
>> +       struct reset_control *reset[MAX_RESET_NUM];
>> +       struct tegra_mc_swgroup *swgroup[MAX_SWGROUP_NUM];
>> +       bool need_vdd;
>> +       struct regulator *vdd;
>> +};
>> +
>> +static inline struct tegra_powergate *
>> +to_powergate(struct generic_pm_domain *domain)
>> +{
>> +       return container_of(domain, struct tegra_powergate, base);
>> +}
>> +
>>   struct tegra_pmc_soc {
>>          unsigned int num_powergates;
>>          const char *const *powergates;
>> @@ -92,8 +123,10 @@ struct tegra_pmc_soc {
>>
>>   /**
>>    * struct tegra_pmc - NVIDIA Tegra PMC
>> + * @dev: pointer to parent device
>>    * @base: pointer to I/O remapped register region
>>    * @clk: pointer to pclk clock
>> + * @soc: SoC-specific data
>>    * @rate: currently configured rate of pclk
>>    * @suspend_mode: lowest suspend mode available
>>    * @cpu_good_time: CPU power good time (in microseconds)
>> @@ -107,9 +140,12 @@ struct tegra_pmc_soc {
>>    * @cpu_pwr_good_en: CPU power good signal is enabled
>>    * @lp0_vec_phys: physical base address of the LP0 warm boot code
>>    * @lp0_vec_size: size of the LP0 warm boot code
>> + * @powergates: list of power gates
>>    * @powergates_lock: mutex for power gate register access
>> + * @nb: bus notifier for generic power domains
>>    */
>>   struct tegra_pmc {
>> +       struct device *dev;
>>          void __iomem *base;
>>          struct clk *clk;
>>
>> @@ -130,7 +166,12 @@ struct tegra_pmc {
>>          u32 lp0_vec_phys;
>>          u32 lp0_vec_size;
>>
>> +       struct tegra_powergate *powergates;
>>          struct mutex powergates_lock;
>> +       struct notifier_block nb;
>> +
>> +       struct list_head powergate_list;
>> +       int power_domain_num;
>>   };
>>
>>   static struct tegra_pmc *pmc = &(struct tegra_pmc) {
>> @@ -353,6 +394,8 @@ int tegra_pmc_cpu_remove_clamping(int cpuid)
>>          if (id < 0)
>>                  return id;
>>
>> +       usleep_range(10, 20);
>> +
>>          return tegra_powergate_remove_clamping(id);
>>   }
>>   #endif /* CONFIG_SMP */
>> @@ -387,6 +430,317 @@ void tegra_pmc_restart(enum reboot_mode mode, const char *cmd)
>>          tegra_pmc_writel(value, 0);
>>   }
>>
>> +static bool tegra_pmc_powergate_is_powered(struct tegra_powergate *powergate)
>> +{
>> +       u32 status = tegra_pmc_readl(PWRGATE_STATUS);
>> +
>> +       if (!powergate->need_vdd)
>> +               return (status & BIT(powergate->id)) != 0;
>> +
>> +       if (IS_ERR(powergate->vdd))
>> +               return false;
>> +       else
>> +               return regulator_is_enabled(powergate->vdd);
> status is only needed if !powervate->need_vdd. In the other case you
> will have read PWRGATE_STATUS for nothing.
>
> Actually, I would have (intuitively) expected that if need_vdd is
> true, then both the right status bit must be set *and* the regulator
> must be enabled, but reading the rest of this patch seems to confirm
> that this is not the case. May I then suggest to rename "need_vdd"
> into something more directly reflecting this, like "is_vdd".
Yes, will rename to "is_vdd".

>
>> +}
>> +
>> +static int tegra_pmc_powergate_set(struct tegra_powergate *powergate,
>> +                                  bool new_state)
>> +{
>> +       u32 status, mask = new_state ? BIT(powergate->id) : 0;
>> +       bool state = false;
>> +
>> +       mutex_lock(&pmc->powergates_lock);
>> +
>> +       /* check the current state of the partition */
>> +       status = tegra_pmc_readl(PWRGATE_STATUS);
>> +       if (status & BIT(powergate->id))
>> +               state = true;
> state = !!(status & BIT(powergate->id)) ?
OK.

>
>> +
>> +       /* nothing to do */
>> +       if (new_state == state) {
>> +               mutex_unlock(&pmc->powergates_lock);
>> +               return 0;
>> +       }
>> +
>> +       /* toggle partition state and wait for state change to finish */
>> +       tegra_pmc_writel(PWRGATE_TOGGLE_START | powergate->id, PWRGATE_TOGGLE);
>> +
>> +       while (1) {
>> +               status = tegra_pmc_readl(PWRGATE_STATUS);
>> +               if ((status & BIT(powergate->id)) == mask)
>> +                       break;
>> +
>> +               usleep_range(10, 20);
>> +       }
> Might be nice to have a timeout and error report here, just in case...
OK.

>
>> +
>> +       mutex_unlock(&pmc->powergates_lock);
>> +
>> +       return 0;
>> +}
>> +
>> +static int
>> +tegra_pmc_powergate_remove_clamping(struct tegra_powergate *powergate)
>> +{
>> +       u32 mask;
>> +
>> +       /*
>> +        * The Tegra124 GPU has a separate register (with different semantics)
>> +        * to remove clamps.
>> +        */
>> +       if (tegra_get_chip_id() == TEGRA124) {
>> +               if (powergate->id == TEGRA_POWERGATE_3D) {
> if (tegra_get_chip_id() == TEGRA124 &&
>      powergate->id == TEGRA_POWERGATE_3D) ?
Yes, I should rebase this series onto the patch below that gets rid of 
the chip ID checking though.

http://patchwork.ozlabs.org/patch/426997/


>
>> +                       tegra_pmc_writel(0, GPU_RG_CNTRL);
>> +                       return 0;
>> +               }
>> +       }
>> +
>> +       /*
>> +        * Tegra 2 has a bug where PCIE and VDE clamping masks are
>> +        * swapped relatively to the partition ids
>> +        */
>> +       if (powergate->id == TEGRA_POWERGATE_VDEC)
>> +               mask = (1 << TEGRA_POWERGATE_PCIE);
>> +       else if (powergate->id == TEGRA_POWERGATE_PCIE)
>> +               mask = (1 << TEGRA_POWERGATE_VDEC);
>> +       else
>> +               mask = (1 << powergate->id);
> If this is only a Tegra 2 issue, shouldn't we test for the chip ID as well?
Actually according to TRM, the later Tegra SoCs all have this swapping. 
So we should
revise the comment instead.

>> +
>> +       tegra_pmc_writel(mask, REMOVE_CLAMPING);
>> +
>> +       return 0;
>> +}
>> +
>> +static int tegra_pmc_powergate_enable_clocks(
>> +               struct tegra_powergate *powergate)
>> +{
>> +       int i, err;
>> +
>> +       for (i = 0; i < MAX_CLK_NUM; i++) {
>> +               if (!powergate->clk[i])
>> +                       break;
>> +
>> +               err = clk_prepare_enable(powergate->clk[i]);
>> +               if (err)
>> +                       goto out;
>> +       }
>> +
>> +       return 0;
>> +
>> +out:
>> +       while(i--)
>> +               clk_disable_unprepare(powergate->clk[i]);
>> +       return err;
>> +}
>> +
>> +static void tegra_pmc_powergate_disable_clocks(
>> +               struct tegra_powergate *powergate)
>> +{
>> +       int i;
>> +
>> +       for (i = 0; i < MAX_CLK_NUM; i++) {
>> +               if (!powergate->clk[i])
>> +                       break;
> nit: shouldn't we disable the clocks in the reverse order of their enabling?
This is not a hard requirement AFAIK. Actually in the documentation, the 
order to disable
clocks is the same as the order when enabling them.

>
>> +
>> +               clk_disable_unprepare(powergate->clk[i]);
>> +       }
>> +}
>> +
>> +static int tegra_pmc_powergate_mc_flush(struct tegra_powergate *powergate)
>> +{
>> +       int i, err;
>> +
>> +       for (i = 0; i < MAX_SWGROUP_NUM; i++) {
>> +               if (!powergate->swgroup[i])
>> +                       break;
>> +
>> +               err = tegra_mc_flush(powergate->swgroup[i]);
>> +               if (err)
>> +                       return err;
>> +       }
>> +
>> +       return 0;
>> +}
>> +
>> +static int tegra_pmc_powergate_mc_flush_done(struct tegra_powergate *powergate)
>> +{
>> +       int i, err;
>> +
>> +       for (i = 0; i < MAX_SWGROUP_NUM; i++) {
>> +               if (!powergate->swgroup[i])
>> +                       break;
>> +
>> +               err = tegra_mc_flush_done(powergate->swgroup[i]);
>> +               if (err)
>> +                       return err;
>> +       }
>> +
>> +       return 0;
>> +
>> +}
>> +
>> +static int tegra_pmc_powergate_reset_assert(
>> +               struct tegra_powergate *powergate)
>> +{
>> +       int i, err;
>> +
>> +       for (i = 0; i < MAX_RESET_NUM; i++) {
>> +               if (!powergate->reset[i])
>> +                       break;
>> +
>> +               err = reset_control_assert(powergate->reset[i]);
>> +               if (err)
>> +                       return err;
>> +       }
>> +
>> +       return 0;
>> +}
>> +
>> +static int tegra_pmc_powergate_reset_deassert(
>> +               struct tegra_powergate *powergate)
>> +{
>> +       int i, err;
>> +
>> +       for (i = 0; i < MAX_RESET_NUM; i++) {
>> +               if (!powergate->reset[i])
>> +                       break;
>> +
>> +               err = reset_control_deassert(powergate->reset[i]);
>> +               if (err)
>> +                       return err;
>> +       }
> Same remark for the reset assert/deassert order.
Like the clk enable/disable order, this should be fine. :)

>
>> +
>> +       return 0;
>> +}
>> +
>> +static int get_regulator(struct tegra_powergate *powergate)
> Name confusingly close to regulator_get, please prefix.
OK.

>
>> +{
>> +       struct platform_device *pdev;
>> +
>> +       if (!powergate->need_vdd)
>> +               return -EINVAL;
>> +
>> +       if (powergate->vdd && !IS_ERR(powergate->vdd))
>> +               return 0;
>> +
>> +       pdev = of_find_device_by_node(powergate->of_node);
>> +       if (!pdev)
>> +               return -EINVAL;
>> +
>> +       powergate->vdd = devm_regulator_get_optional(&pdev->dev, "vdd");
>> +       if (IS_ERR(powergate->vdd))
>> +               return -EINVAL;
>> +
>> +       return 0;
>> +}
>> +
>> +static int tegra_pmc_powergate_power_on(struct generic_pm_domain *domain)
>> +{
>> +       struct tegra_powergate *powergate = to_powergate(domain);
>> +       struct tegra_pmc *pmc = powergate->pmc;
>> +       int err;
>> +
>> +       dev_dbg(pmc->dev, "> %s(domain=%p)\n", __func__, domain);
>> +       dev_dbg(pmc->dev, "  name: %s\n", domain->name);
>> +
>> +       if (powergate->need_vdd) {
>> +               err = get_regulator(powergate);
>> +               if (!err) {
>> +                       err = regulator_enable(powergate->vdd);
>> +               }
>> +       } else {
>> +               err = tegra_pmc_powergate_set(powergate, true);
>> +       }
>> +       if (err < 0)
>> +               goto out;
>> +       udelay(10);
>> +
>> +       err = tegra_pmc_powergate_enable_clocks(powergate);
>> +       if (err)
>> +               goto out;
>> +       udelay(10);
>> +
>> +       err = tegra_pmc_powergate_remove_clamping(powergate);
>> +       if (err)
>> +               goto out;
>> +       udelay(10);
>> +
>> +       err = tegra_pmc_powergate_reset_deassert(powergate);
>> +       if (err)
>> +               goto out;
>> +       udelay(10);
>> +
>> +       err = tegra_pmc_powergate_mc_flush_done(powergate);
>> +       if (err)
>> +               goto out;
>> +       udelay(10);
>> +
>> +       tegra_pmc_powergate_disable_clocks(powergate);
>> +
>> +       return 0;
>> +
>> +/* XXX more error handing */
>> +out:
>> +       dev_dbg(pmc->dev, "< %s() = %d\n", __func__, err);
>> +       return err;
>> +}
>> +
>> +static int tegra_pmc_powergate_power_off(struct generic_pm_domain *domain)
>> +{
>> +       struct tegra_powergate *powergate = to_powergate(domain);
>> +       struct tegra_pmc *pmc = powergate->pmc;
>> +       int err;
>> +
>> +       dev_dbg(pmc->dev, "> %s(domain=%p)\n", __func__, domain);
>> +       dev_dbg(pmc->dev, "  name: %s\n", domain->name);
>> +
>> +       /* never turn off this partition */
> s/this partition/these partitions
OK.

>
>> +       switch (powergate->id) {
>> +       case TEGRA_POWERGATE_CPU:
>> +       case TEGRA_POWERGATE_CPU1:
>> +       case TEGRA_POWERGATE_CPU2:
>> +       case TEGRA_POWERGATE_CPU3:
>> +       case TEGRA_POWERGATE_CPU0:
>> +       case TEGRA_POWERGATE_C0NC:
>> +       case TEGRA_POWERGATE_IRAM:
>> +               dev_dbg(pmc->dev, "not disabling always-on partition %s\n",
>> +                       domain->name);
>> +               err = -EINVAL;
>> +               goto out;
>> +       }
>> +
>> +       err = tegra_pmc_powergate_enable_clocks(powergate);
>> +       if (err)
>> +               goto out;
>> +       udelay(10);
>> +
>> +       err = tegra_pmc_powergate_mc_flush(powergate);
>> +       if (err)
>> +               goto out;
>> +       udelay(10);
>> +
>> +       err = tegra_pmc_powergate_reset_assert(powergate);
>> +       if (err)
>> +               goto out;
>> +       udelay(10);
>> +
>> +       tegra_pmc_powergate_disable_clocks(powergate);
>> +       udelay(10);
>> +
>> +       if (powergate->vdd)
>> +               err = regulator_disable(powergate->vdd);
>> +       else
>> +               err = tegra_pmc_powergate_set(powergate, false);
>> +       if (err)
>> +               goto out;
>> +
>> +       return 0;
>> +
>> +/* XXX more error handling */
>> +out:
>> +       dev_dbg(pmc->dev, "< %s() = %d\n", __func__, err);
>> +       return err;
>> +}
>> +
>>   static int powergate_show(struct seq_file *s, void *data)
>>   {
>>          unsigned int i;
>> @@ -429,6 +783,231 @@ static int tegra_powergate_debugfs_init(void)
>>          return 0;
>>   }
>>
>> +static struct generic_pm_domain *
>> +tegra_powergate_of_xlate(struct of_phandle_args *args, void *data)
>> +{
>> +       struct tegra_pmc *pmc = data;
>> +       struct tegra_powergate *powergate;
>> +
>> +       dev_dbg(pmc->dev, "> %s(args=%p, data=%p)\n", __func__, args, data);
>> +
>> +       list_for_each_entry(powergate, &pmc->powergate_list, head) {
>> +               if (!powergate->base.name)
>> +                       continue;
>> +
>> +               if (powergate->id == args->args[0]) {
>> +                       dev_dbg(pmc->dev, "< %s() = %p\n", __func__, powergate);
>> +                       return &powergate->base;
>> +               }
>> +       }
>> +
>> +       dev_dbg(pmc->dev, "< %s() = -ENOENT\n", __func__);
>> +       return ERR_PTR(-ENOENT);
>> +}
>> +
>> +static int of_get_clks(struct tegra_powergate *powergate)
> This function (and the few following ones) should also get a less
> ambiguous name.
OK.

>
>> +{
>> +       struct clk *clk;
>> +       int i;
>> +
>> +       for (i = 0; i < MAX_CLK_NUM; i++) {
>> +               clk = of_clk_get(powergate->of_node, i);
>> +               if (IS_ERR(clk)) {
>> +                       if (PTR_ERR(clk) == -ENOENT)
>> +                               break;
>> +                       else
>> +                               return PTR_ERR(clk);
> ... they should also probably free the resources they managed to
> acquire in case of error.
Will add a error path to handle the error and clk_put the clocks.

Thanks,
Vince

>
> I cannot comment on the DT bindings - Peter and Thierry are probably
> the best persons for that. I just wonder if we should not define them
> all under a global power-domains node that would be the only one to
> match the compatible property whereas the childs would define the
> individual domains (a bit like pinmux definitions). But globally the
> idea seems sound to me.

Re: [RFC PATCH 3/9] memory: tegra: add flush operation for Tegra124 memory clients

[Alexandre Courbot]

On Mon, Mar 2, 2015 at 5:54 PM, Vince Hsu <vinceh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> wrote:
>>>          {
>>>                  .id = 0x00,
>>>                  .name = "ptcr",
>>> @@ -959,7 +961,108 @@ static const struct tegra_smmu_swgroup
>>> tegra124_swgroups[] = {
>>>          { .swgroup = TEGRA_SWGROUP_VI,        .reg = 0x280 },
>>>   };
>>>
>>> +static struct tegra_mc_hotreset tegra124_mc_hotreset[] = {
>>> +       {TEGRA_SWGROUP_AFI,        0x200, 0x204,  0},
>>> +       {TEGRA_SWGROUP_AVPC,       0x200, 0x204,  1},
>>> +       {TEGRA_SWGROUP_DC,         0x200, 0x204,  2},
>>> +       {TEGRA_SWGROUP_DCB,        0x200, 0x204,  3},
>>> +       {TEGRA_SWGROUP_HC,         0x200, 0x204,  6},
>>> +       {TEGRA_SWGROUP_HDA,        0x200, 0x204,  7},
>>> +       {TEGRA_SWGROUP_ISP2,       0x200, 0x204,  8},
>>> +       {TEGRA_SWGROUP_MPCORE,     0x200, 0x204,  9},
>>> +       {TEGRA_SWGROUP_MPCORELP,   0x200, 0x204, 10},
>>> +       {TEGRA_SWGROUP_MSENC,      0x200, 0x204, 11},
>>> +       {TEGRA_SWGROUP_PPCS,       0x200, 0x204, 14},
>>> +       {TEGRA_SWGROUP_SATA,       0x200, 0x204, 15},
>>> +       {TEGRA_SWGROUP_VDE,        0x200, 0x204, 16},
>>> +       {TEGRA_SWGROUP_VI,         0x200, 0x204, 17},
>>> +       {TEGRA_SWGROUP_VIC,        0x200, 0x204, 18},
>>> +       {TEGRA_SWGROUP_XUSB_HOST,  0x200, 0x204, 19},
>>> +       {TEGRA_SWGROUP_XUSB_DEV,   0x200, 0x204, 20},
>>> +       {TEGRA_SWGROUP_TSEC,       0x200, 0x204, 22},
>>> +       {TEGRA_SWGROUP_SDMMC1A,    0x200, 0x204, 29},
>>> +       {TEGRA_SWGROUP_SDMMC2A,    0x200, 0x204, 30},
>>> +       {TEGRA_SWGROUP_SDMMC3A,    0x200, 0x204, 31},
>>> +       {TEGRA_SWGROUP_SDMMC4A,    0x970, 0x974,  0},
>>> +       {TEGRA_SWGROUP_ISP2B,      0x970, 0x974,  1},
>>> +       {TEGRA_SWGROUP_GPU,        0x970, 0x974,  2},
>>> +};
>>> +
>>>   #ifdef CONFIG_ARCH_TEGRA_124_SOC
>>> +
>>> +static bool tegra124_stable_hotreset_check(struct tegra_mc *mc,
>>> +               u32 reg, u32 *stat)
>>> +{
>>> +       int i;
>>> +       u32 cur_stat;
>>> +       u32 prv_stat;
>>> +
>>> +       /*
>>> +        * There might be a glitch seen with the status register if we
>>> program
>>> +        * the control register and then read the status register in a
>>> short
>>> +        * window due to a HW bug. So here we poll for a stable status
>>> read.
>>> +        */
>>> +       prv_stat = mc_readl(mc, reg);
>>> +       for (i = 0; i < 5; i++) {
>>
>> Why 5 times?
>
> This is a magic number from downstream. It should be enough to reach a
> stable state.

Most people don't like arbitrary numbers without any explanation. And
sentences like "should be enough" make me nervous. :) Could you try to
document this number so we at least know why *this* value instead of
one of the possible other values an integer can take?

>
>>> +               cur_stat = mc_readl(mc, reg);
>>> +               if (cur_stat != prv_stat)
>>> +                       return false;
>>> +       }
>>> +       *stat = cur_stat;
>>> +       return true;
>>> +}
>>> +
>>> +static int tegra124_mc_flush(struct tegra_mc *mc,
>>> +               const struct tegra_mc_hotreset *hotreset)
>>> +{
>>> +       u32 val;
>>> +
>>> +       if (!mc || !hotreset)
>>> +               return -EINVAL;
>>> +
>>> +       /* XXX add mutex */
>>
>> I guess this is a TODO for when this series exists the RFC state. :)
>
> Oh, yes.
>
>>> +
>>> +       val = mc_readl(mc, hotreset->ctrl);
>>> +       val |= BIT(hotreset->bit);
>>> +       mc_writel(mc, val, hotreset->ctrl);
>>> +       mc_readl(mc, hotreset->ctrl);
>>> +
>>> +       /* poll till the flush is done */
>>> +       do {
>>> +               udelay(10);
>>> +               val = 0;
>>> +               if (!tegra124_stable_hotreset_check(mc, hotreset->status,
>>> &val))
>>> +                       continue;
>>> +       } while (!(val & BIT(hotreset->bit)));
>>
>> So here you are waiting for the right bit in hotreset->status to turn
>> to 1. Why is the whole thing with tegra124_stable_hotreset_check()
>> needed? Could it switch from 1 to 0 to 1 again, with the first window
>> at 1 being invalid?
>
> There is a comment which describes why we have to check this in
> tegra124_stable_hotreset_check().
>
>> In that case isn't there a risk that
>> tegra124_stable_hotreset_check() might do 5 consecutive reads on that
>> invalid state and let us continue before the flush is completed?
>
> So there is another check in while(). :)

Yeah, I guess the issue is reduced to ensuring that 6 consecutive
identical reads are enough to guarantee that the value of this
register is stable.

Re: [RFC PATCH 2/9] memory: tegra: add mc flush support

[Alexandre Courbot]

On Mon, Mar 2, 2015 at 5:54 PM, Vince Hsu <vinceh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> wrote:
>>> +int tegra_mc_flush(struct tegra_mc_swgroup *sg)
>>> +{
>>> +       struct tegra_mc *mc;
>>> +       const struct tegra_mc_hotreset *client;
>>> +       int i;
>>> +
>>> +       if (!sg || !sg->mc)
>>> +               return -EINVAL;;
>>> +
>>> +       mc = sg->mc;
>>> +       if (!mc->soc->ops || !mc->soc->ops->flush)
>>> +               return -EINVAL;;
>>> +
>>> +       client = mc->soc->hotresets;
>>> +
>>> +       for (i = 0; i < mc->soc->num_hotresets; i++, client++) {
>>> +               if (sg->id == client->swgroup)
>>> +                       return mc->soc->ops->flush(mc, client);
>>> +       }
>>> +
>>> +       return -EINVAL;
>>> +}
>>> +EXPORT_SYMBOL(tegra_mc_flush);
>>> +
>>> +int tegra_mc_flush_done(struct tegra_mc_swgroup *sg)
>>> +{
>>> +       struct tegra_mc *mc;
>>> +       const struct tegra_mc_hotreset *client;
>>> +       int i;
>>> +
>>> +       if (!sg || !sg->mc)
>>> +               return -EINVAL;;
>>> +
>>> +       mc = sg->mc;
>>> +       if (!mc->soc->ops || !mc->soc->ops->flush)
>>> +               return -EINVAL;;
>>> +
>>> +       client = mc->soc->hotresets;
>>> +
>>> +       for (i = 0; i < mc->soc->num_hotresets; i++, client++) {
>>> +               if (sg->id == client->swgroup)
>>> +                       return mc->soc->ops->flush_done(mc, client);
>>> +       }
>>> +
>>> +       return -EINVAL;
>>> +}
>>> +EXPORT_SYMBOL(tegra_mc_flush_done);
>>
>> These functions are identical, excepted for the callback they are
>> invoking. Could you merge the common part into a function that returns
>> the right client to call the callback on, or ERR_PTR(-EINVAL) in case
>> of failure?
>
> I couldn't think of a clever way to do this. Any ideas? :)

How about something like this (warning: might now be that great, untested):

/* Have this in your .h and use it in your tegra_mc_ops struct */
typedef int (*mc_op)(struct tegra_mc *mc,
             const struct tegra_mc_hotreset *hotreset)

static int __tegra_mc_op(struct tegra_mc_swgroup *sg, mc_op op)
{
       struct tegra_mc *mc;
       const struct tegra_mc_hotreset *client;
       int i;

       mc = sg->mc;
       client = mc->soc->hotresets;

       for (i = 0; i < mc->soc->num_hotresets; i++, client++) {
               if (sg->id == client->swgroup)
                       return op(mc, client);
       }

       return -EINVAL;
}

#define tegra_mc_op(sg, op)                                               \
    ((!sg || !sg->mc || !sg->mc->soc->ops || sg->mc->soc->ops->op) ?  \
        -EINVAL :                                                 \
        __tegra_mc_op(sg, sg->mc->soc->ops->op));

int tegra_mc_flush(struct tegra_mc_swgroup *sg)
{
       return tegra_mc_op(sg, flush);
}
EXPORT_SYMBOL(tegra_mc_flush);

int tegra_mc_flush_done(struct tegra_mc_swgroup *sg)
{
       return tegra_mc_op(sg, flush_done);
}
EXPORT_SYMBOL(tegra_mc_flush_done);


Actually when writing this code I found two other issues:

>>> +int tegra_mc_flush_done(struct tegra_mc_swgroup *sg)
>>> +{
>>> +       struct tegra_mc *mc;
>>> +       const struct tegra_mc_hotreset *client;
>>> +       int i;
>>> +
>>> +       if (!sg || !sg->mc)
>>> +               return -EINVAL;;

Double ; (also in tegra_mc_flush)

>>> +
>>> +       mc = sg->mc;
>>> +       if (!mc->soc->ops || !mc->soc->ops->flush)

Should be ops->flush_done?

Re: [RFC PATCH 2/9] memory: tegra: add mc flush support

[Vince Hsu]

Hi,

On 03/02/2015 05:29 PM, Alexandre Courbot wrote:
> On Mon, Mar 2, 2015 at 5:54 PM, Vince Hsu <vinceh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> wrote:
>>>> +int tegra_mc_flush(struct tegra_mc_swgroup *sg)
>>>> +{
>>>> +       struct tegra_mc *mc;
>>>> +       const struct tegra_mc_hotreset *client;
>>>> +       int i;
>>>> +
>>>> +       if (!sg || !sg->mc)
>>>> +               return -EINVAL;;
>>>> +
>>>> +       mc = sg->mc;
>>>> +       if (!mc->soc->ops || !mc->soc->ops->flush)
>>>> +               return -EINVAL;;
>>>> +
>>>> +       client = mc->soc->hotresets;
>>>> +
>>>> +       for (i = 0; i < mc->soc->num_hotresets; i++, client++) {
>>>> +               if (sg->id == client->swgroup)
>>>> +                       return mc->soc->ops->flush(mc, client);
>>>> +       }
>>>> +
>>>> +       return -EINVAL;
>>>> +}
>>>> +EXPORT_SYMBOL(tegra_mc_flush);
>>>> +
>>>> +int tegra_mc_flush_done(struct tegra_mc_swgroup *sg)
>>>> +{
>>>> +       struct tegra_mc *mc;
>>>> +       const struct tegra_mc_hotreset *client;
>>>> +       int i;
>>>> +
>>>> +       if (!sg || !sg->mc)
>>>> +               return -EINVAL;;
>>>> +
>>>> +       mc = sg->mc;
>>>> +       if (!mc->soc->ops || !mc->soc->ops->flush)
>>>> +               return -EINVAL;;
>>>> +
>>>> +       client = mc->soc->hotresets;
>>>> +
>>>> +       for (i = 0; i < mc->soc->num_hotresets; i++, client++) {
>>>> +               if (sg->id == client->swgroup)
>>>> +                       return mc->soc->ops->flush_done(mc, client);
>>>> +       }
>>>> +
>>>> +       return -EINVAL;
>>>> +}
>>>> +EXPORT_SYMBOL(tegra_mc_flush_done);
>>> These functions are identical, excepted for the callback they are
>>> invoking. Could you merge the common part into a function that returns
>>> the right client to call the callback on, or ERR_PTR(-EINVAL) in case
>>> of failure?
>> I couldn't think of a clever way to do this. Any ideas? :)
> How about something like this (warning: might now be that great, untested):
>
> /* Have this in your .h and use it in your tegra_mc_ops struct */
> typedef int (*mc_op)(struct tegra_mc *mc,
>               const struct tegra_mc_hotreset *hotreset)
>
> static int __tegra_mc_op(struct tegra_mc_swgroup *sg, mc_op op)
> {
>         struct tegra_mc *mc;
>         const struct tegra_mc_hotreset *client;
>         int i;
>
>         mc = sg->mc;
>         client = mc->soc->hotresets;
>
>         for (i = 0; i < mc->soc->num_hotresets; i++, client++) {
>                 if (sg->id == client->swgroup)
>                         return op(mc, client);
>         }
>
>         return -EINVAL;
> }
>
> #define tegra_mc_op(sg, op)                                               \
>      ((!sg || !sg->mc || !sg->mc->soc->ops || sg->mc->soc->ops->op) ?  \
>          -EINVAL :                                                 \
>          __tegra_mc_op(sg, sg->mc->soc->ops->op));
>
> int tegra_mc_flush(struct tegra_mc_swgroup *sg)
> {
>         return tegra_mc_op(sg, flush);
> }
> EXPORT_SYMBOL(tegra_mc_flush);
>
> int tegra_mc_flush_done(struct tegra_mc_swgroup *sg)
> {
>         return tegra_mc_op(sg, flush_done);
> }
> EXPORT_SYMBOL(tegra_mc_flush_done);
Thanks for the example! Will use this idea and remove the double 
semicolon below.

Thanks,
Vince

>
>
> Actually when writing this code I found two other issues:
>
>>>> +int tegra_mc_flush_done(struct tegra_mc_swgroup *sg)
>>>> +{
>>>> +       struct tegra_mc *mc;
>>>> +       const struct tegra_mc_hotreset *client;
>>>> +       int i;
>>>> +
>>>> +       if (!sg || !sg->mc)
>>>> +               return -EINVAL;;
> Double ; (also in tegra_mc_flush)
>
>>>> +
>>>> +       mc = sg->mc;
>>>> +       if (!mc->soc->ops || !mc->soc->ops->flush)
> Should be ops->flush_done?

Re: [RFC PATCH 2/9] memory: tegra: add mc flush support

[Alexandre Courbot]

On Mon, Mar 2, 2015 at 6:29 PM, Alexandre Courbot <gnurou-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>>> These functions are identical, excepted for the callback they are
>>> invoking. Could you merge the common part into a function that returns
>>> the right client to call the callback on, or ERR_PTR(-EINVAL) in case
>>> of failure?
>>
>> I couldn't think of a clever way to do this. Any ideas? :)
>
> How about something like this (warning: might now be that great, untested):
>
> /* Have this in your .h and use it in your tegra_mc_ops struct */
> typedef int (*mc_op)(struct tegra_mc *mc,
>              const struct tegra_mc_hotreset *hotreset)

This type should be named tegra_mc_op, since the header that defines
it is in include/linux.

Re: [RFC PATCH 2/9] memory: tegra: add mc flush support

[Vince Hsu]

On 03/03/2015 04:03 PM, Alexandre Courbot wrote:
> On Mon, Mar 2, 2015 at 6:29 PM, Alexandre Courbot <gnurou-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>>>> These functions are identical, excepted for the callback they are
>>>> invoking. Could you merge the common part into a function that returns
>>>> the right client to call the callback on, or ERR_PTR(-EINVAL) in case
>>>> of failure?
>>> I couldn't think of a clever way to do this. Any ideas? :)
>> How about something like this (warning: might now be that great, untested):
>>
>> /* Have this in your .h and use it in your tegra_mc_ops struct */
>> typedef int (*mc_op)(struct tegra_mc *mc,
>>               const struct tegra_mc_hotreset *hotreset)
> This type should be named tegra_mc_op, since the header that defines
> it is in include/linux.
Can we just leave it in this C file? I see no reason to place it in some 
other
header file. :)

Thanks,
Vince

Re: [RFC PATCH 2/9] memory: tegra: add mc flush support

[Alexandre Courbot]

On Tue, Mar 3, 2015 at 5:09 PM, Vince Hsu <vinceh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> wrote:
>
> On 03/03/2015 04:03 PM, Alexandre Courbot wrote:
>>
>> On Mon, Mar 2, 2015 at 6:29 PM, Alexandre Courbot <gnurou-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>> wrote:
>>>>>
>>>>> These functions are identical, excepted for the callback they are
>>>>> invoking. Could you merge the common part into a function that returns
>>>>> the right client to call the callback on, or ERR_PTR(-EINVAL) in case
>>>>> of failure?
>>>>
>>>> I couldn't think of a clever way to do this. Any ideas? :)
>>>
>>> How about something like this (warning: might now be that great,
>>> untested):
>>>
>>> /* Have this in your .h and use it in your tegra_mc_ops struct */
>>> typedef int (*mc_op)(struct tegra_mc *mc,
>>>               const struct tegra_mc_hotreset *hotreset)
>>
>> This type should be named tegra_mc_op, since the header that defines
>> it is in include/linux.
>
> Can we just leave it in this C file? I see no reason to place it in some
> other
> header file. :)

Can you also move tegra_mc_ops's definition into the C file? If so I
agree. Otherwise I prefer to make sure that the same type is used
everywhere.

Re: [RFC PATCH 2/9] memory: tegra: add mc flush support

[Vince Hsu]

On 03/03/2015 04:14 PM, Alexandre Courbot wrote:
> On Tue, Mar 3, 2015 at 5:09 PM, Vince Hsu <vinceh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> wrote:
>> On 03/03/2015 04:03 PM, Alexandre Courbot wrote:
>>> On Mon, Mar 2, 2015 at 6:29 PM, Alexandre Courbot <gnurou-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>>> wrote:
>>>>>> These functions are identical, excepted for the callback they are
>>>>>> invoking. Could you merge the common part into a function that returns
>>>>>> the right client to call the callback on, or ERR_PTR(-EINVAL) in case
>>>>>> of failure?
>>>>> I couldn't think of a clever way to do this. Any ideas? :)
>>>> How about something like this (warning: might now be that great,
>>>> untested):
>>>>
>>>> /* Have this in your .h and use it in your tegra_mc_ops struct */
>>>> typedef int (*mc_op)(struct tegra_mc *mc,
>>>>                const struct tegra_mc_hotreset *hotreset)
>>> This type should be named tegra_mc_op, since the header that defines
>>> it is in include/linux.
>> Can we just leave it in this C file? I see no reason to place it in some
>> other
>> header file. :)
> Can you also move tegra_mc_ops's definition into the C file? If so I
> agree. Otherwise I prefer to make sure that the same type is used
> everywhere.
No. Let's keep that in the include/soc/tegra/mc.h.

Thanks,
Vince

Re: [RFC PATCH 2/9] memory: tegra: add mc flush support

[Alexandre Courbot]

On Tue, Mar 3, 2015 at 5:18 PM, Vince Hsu <vinceh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> wrote:
>
> On 03/03/2015 04:14 PM, Alexandre Courbot wrote:
>>
>> On Tue, Mar 3, 2015 at 5:09 PM, Vince Hsu <vinceh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> wrote:
>>>
>>> On 03/03/2015 04:03 PM, Alexandre Courbot wrote:
>>>>
>>>> On Mon, Mar 2, 2015 at 6:29 PM, Alexandre Courbot <gnurou-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>>>> wrote:
>>>>>>>
>>>>>>> These functions are identical, excepted for the callback they are
>>>>>>> invoking. Could you merge the common part into a function that
>>>>>>> returns
>>>>>>> the right client to call the callback on, or ERR_PTR(-EINVAL) in case
>>>>>>> of failure?
>>>>>>
>>>>>> I couldn't think of a clever way to do this. Any ideas? :)
>>>>>
>>>>> How about something like this (warning: might now be that great,
>>>>> untested):
>>>>>
>>>>> /* Have this in your .h and use it in your tegra_mc_ops struct */
>>>>> typedef int (*mc_op)(struct tegra_mc *mc,
>>>>>                const struct tegra_mc_hotreset *hotreset)
>>>>
>>>> This type should be named tegra_mc_op, since the header that defines
>>>> it is in include/linux.
>>>
>>> Can we just leave it in this C file? I see no reason to place it in some
>>> other
>>> header file. :)
>>
>> Can you also move tegra_mc_ops's definition into the C file? If so I
>> agree. Otherwise I prefer to make sure that the same type is used
>> everywhere.
>
> No. Let's keep that in the include/soc/tegra/mc.h.

Actually, couldn't some of these definitions be moved into
drivers/memory/tegra/mc.h? I don't see any reference to e.g. struct
tegra_mc_soc outside of drivers/memory/tegra/. These doesn't seem to
be a need to make these available to everybody.

Re: [RFC PATCH 2/9] memory: tegra: add mc flush support

[Vince Hsu]

On 03/03/2015 04:31 PM, Alexandre Courbot wrote:
> On Tue, Mar 3, 2015 at 5:18 PM, Vince Hsu <vinceh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> wrote:
>> On 03/03/2015 04:14 PM, Alexandre Courbot wrote:
>>> On Tue, Mar 3, 2015 at 5:09 PM, Vince Hsu <vinceh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> wrote:
>>>> On 03/03/2015 04:03 PM, Alexandre Courbot wrote:
>>>>> On Mon, Mar 2, 2015 at 6:29 PM, Alexandre Courbot <gnurou-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>>>>> wrote:
>>>>>>>> These functions are identical, excepted for the callback they are
>>>>>>>> invoking. Could you merge the common part into a function that
>>>>>>>> returns
>>>>>>>> the right client to call the callback on, or ERR_PTR(-EINVAL) in case
>>>>>>>> of failure?
>>>>>>> I couldn't think of a clever way to do this. Any ideas? :)
>>>>>> How about something like this (warning: might now be that great,
>>>>>> untested):
>>>>>>
>>>>>> /* Have this in your .h and use it in your tegra_mc_ops struct */
>>>>>> typedef int (*mc_op)(struct tegra_mc *mc,
>>>>>>                 const struct tegra_mc_hotreset *hotreset)
>>>>> This type should be named tegra_mc_op, since the header that defines
>>>>> it is in include/linux.
>>>> Can we just leave it in this C file? I see no reason to place it in some
>>>> other
>>>> header file. :)
>>> Can you also move tegra_mc_ops's definition into the C file? If so I
>>> agree. Otherwise I prefer to make sure that the same type is used
>>> everywhere.
>> No. Let's keep that in the include/soc/tegra/mc.h.
> Actually, couldn't some of these definitions be moved into
> drivers/memory/tegra/mc.h? I don't see any reference to e.g. struct
> tegra_mc_soc outside of drivers/memory/tegra/. These doesn't seem to
> be a need to make these available to everybody.
It seems all the definitions were in the 
drivers/memory/tegra/tegra-mc.h. And
then due to some reason, Thierry moved them under include/soc/tegra.
And we do need the struct tegra_mc_soc in the include/soc/tegra/tegra/mc.h
because struct tegra_mc refers to it and iommu/tegra-smmu.c refers to
struct tegra_mc.

Thanks,
Vince