[PATCH 0/4] ARM: tegra: re-enable EMC scaling function for Tegra20

[PATCH 0/4] ARM: tegra: re-enable EMC scaling function for Tegra20

[Joseph Lo]

The EMC scaling function was lost after Tegra deprecated the legacy
clock driver and switched to CCF driver. In this series, we add the EMC
clock driver in the Tegra CCF driver. And re-register the EMC driver to
the EMC clock driver to support EMC scaling again.

Verified on Seaboard.

Joseph Lo (4):
  ARM: tegra: moving tegra_bct_strapping to tegra-soc.h for global
    visibility
  clk: tegra: add EMC clock driver
  memory: tegra20-emc: move out Tegra20 EMC driver from mach-tegra
  clk: tegra20: enable EMC clock driver

 arch/arm/mach-tegra/Makefile                       |   1 -
 arch/arm/mach-tegra/fuse.h                         |   2 -
 arch/arm/mach-tegra/tegra20_speedo.c               |   1 +
 arch/arm/mach-tegra/tegra2_emc.h                   |  24 ---
 drivers/clk/tegra/Makefile                         |   1 +
 drivers/clk/tegra/clk-emc.c                        | 183 +++++++++++++++++++++
 drivers/clk/tegra/clk-tegra20.c                    |  25 +--
 drivers/clk/tegra/clk.h                            |  19 +++
 drivers/memory/Kconfig                             |  10 ++
 drivers/memory/Makefile                            |   1 +
 .../tegra2_emc.c => drivers/memory/tegra20-emc.c   |  16 +-
 include/linux/platform_data/tegra_emc.h            |   7 +
 include/linux/tegra-soc.h                          |   2 +
 13 files changed, 250 insertions(+), 42 deletions(-)
 delete mode 100644 arch/arm/mach-tegra/tegra2_emc.h
 create mode 100644 drivers/clk/tegra/clk-emc.c
 rename arch/arm/mach-tegra/tegra2_emc.c => drivers/memory/tegra20-emc.c (95%)

-- 
1.8.5

[PATCH 1/4] ARM: tegra: moving tegra_bct_strapping to tegra-soc.h for global visibility

[Joseph Lo]

This patch moves the tegra_btc_strapping variable to the tegra-soc.h for
the global visibility that the other Tegra device driver can access it.
It also a preparation that we can move out the Tegra20 EMC driver from
mach-tegra to the drivers folder.

Signed-off-by: Joseph Lo <josephl@nvidia.com>
---
 arch/arm/mach-tegra/fuse.h           | 2 --
 arch/arm/mach-tegra/tegra20_speedo.c | 1 +
 arch/arm/mach-tegra/tegra2_emc.c     | 2 +-
 include/linux/tegra-soc.h            | 2 ++
 4 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/arch/arm/mach-tegra/fuse.h b/arch/arm/mach-tegra/fuse.h
index c01d04785d67..44945c8b5d17 100644
--- a/arch/arm/mach-tegra/fuse.h
+++ b/arch/arm/mach-tegra/fuse.h
@@ -50,8 +50,6 @@ extern int tegra_cpu_speedo_id;		/* only exist in Tegra30 and later */
 extern int tegra_soc_speedo_id;
 extern enum tegra_revision tegra_revision;
 
-extern int tegra_bct_strapping;
-
 unsigned long long tegra_chip_uid(void);
 void tegra_init_fuse(void);
 bool tegra_spare_fuse(int bit);
diff --git a/arch/arm/mach-tegra/tegra20_speedo.c b/arch/arm/mach-tegra/tegra20_speedo.c
index fa6eb570623f..3b1bb5360a92 100644
--- a/arch/arm/mach-tegra/tegra20_speedo.c
+++ b/arch/arm/mach-tegra/tegra20_speedo.c
@@ -16,6 +16,7 @@
 
 #include <linux/kernel.h>
 #include <linux/bug.h>
+#include <linux/tegra-soc.h>
 
 #include "fuse.h"
 
diff --git a/arch/arm/mach-tegra/tegra2_emc.c b/arch/arm/mach-tegra/tegra2_emc.c
index 3ae4a7f1a2fb..26e4edbd8a6a 100644
--- a/arch/arm/mach-tegra/tegra2_emc.c
+++ b/arch/arm/mach-tegra/tegra2_emc.c
@@ -24,9 +24,9 @@
 #include <linux/of.h>
 #include <linux/platform_device.h>
 #include <linux/platform_data/tegra_emc.h>
+#include <linux/tegra-soc.h>
 
 #include "tegra2_emc.h"
-#include "fuse.h"
 
 #ifdef CONFIG_TEGRA_EMC_SCALING_ENABLE
 static bool emc_enable = true;
diff --git a/include/linux/tegra-soc.h b/include/linux/tegra-soc.h
index 95f611d78f3a..2e02a9a033c5 100644
--- a/include/linux/tegra-soc.h
+++ b/include/linux/tegra-soc.h
@@ -17,6 +17,8 @@
 #ifndef __LINUX_TEGRA_SOC_H_
 #define __LINUX_TEGRA_SOC_H_
 
+extern int tegra_bct_strapping;
+
 u32 tegra_read_chipid(void);
 
 #endif /* __LINUX_TEGRA_SOC_H_ */
-- 
1.8.5

[PATCH 2/4] clk: tegra: add EMC clock driver

[Joseph Lo]

Add External Memory Controller (EMC) clock interface for the Tegra CCF
driver to support EMC scaling.

Signed-off-by: Joseph Lo <josephl@nvidia.com>
---
Cc: Mike Turquette <mturquette@linaro.org>
---
 drivers/clk/tegra/Makefile              |   1 +
 drivers/clk/tegra/clk-emc.c             | 183 ++++++++++++++++++++++++++++++++
 drivers/clk/tegra/clk.h                 |  19 ++++
 include/linux/platform_data/tegra_emc.h |   7 ++
 4 files changed, 210 insertions(+)
 create mode 100644 drivers/clk/tegra/clk-emc.c

diff --git a/drivers/clk/tegra/Makefile b/drivers/clk/tegra/Makefile
index f7dfb72884a4..c493ba9ad531 100644
--- a/drivers/clk/tegra/Makefile
+++ b/drivers/clk/tegra/Makefile
@@ -1,6 +1,7 @@
 obj-y					+= clk.o
 obj-y					+= clk-audio-sync.o
 obj-y					+= clk-divider.o
+obj-y					+= clk-emc.o
 obj-y					+= clk-periph.o
 obj-y					+= clk-periph-gate.o
 obj-y					+= clk-pll.o
diff --git a/drivers/clk/tegra/clk-emc.c b/drivers/clk/tegra/clk-emc.c
new file mode 100644
index 000000000000..4403696a7dc2
--- /dev/null
+++ b/drivers/clk/tegra/clk-emc.c
@@ -0,0 +1,183 @@
+/*
+ * Copyright (c) 2013, NVIDIA CORPORATION.  All rights reserved.
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ */
+
+#include <linux/clk.h>
+#include <linux/clk-provider.h>
+#include <linux/platform_data/tegra_emc.h>
+
+#include "clk.h"
+
+static u8 clk_emc_get_parent(struct clk_hw *hw)
+{
+	struct tegra_clk_emc *emc = to_clk_emc(hw);
+	const struct clk_ops *mux_ops = emc->periph->mux_ops;
+	struct clk_hw *mux_hw = &emc->periph->mux.hw;
+
+	mux_hw->clk = hw->clk;
+	return mux_ops->get_parent(mux_hw);
+}
+
+static unsigned long clk_emc_recalc_rate(struct clk_hw *hw,
+					 unsigned long parent_rate)
+{
+	struct tegra_clk_emc *emc = to_clk_emc(hw);
+	struct tegra_clk_periph *periph = emc->periph;
+	const struct clk_ops *div_ops = periph->div_ops;
+	struct clk_hw *div_hw = &periph->divider.hw;
+
+	return div_ops->recalc_rate(div_hw, parent_rate);
+}
+
+static long clk_emc_round_rate(struct clk_hw *hw, unsigned long rate,
+			       unsigned long *prate)
+{
+	struct tegra_clk_emc *emc = to_clk_emc(hw);
+	struct clk *parent_clk = __clk_get_parent(hw->clk);
+	unsigned long parent_rate = __clk_get_rate(parent_clk);
+	unsigned long ret;
+
+	if (!emc->emc_ops)
+		return parent_rate;
+
+	ret = emc->emc_ops->emc_round_rate(rate);
+	if (!ret)
+		return parent_rate;
+
+	return ret;
+}
+
+static int clk_emc_set_rate(struct clk_hw *hw, unsigned long rate,
+			    unsigned long parent_rate)
+{
+	struct tegra_clk_emc *emc = to_clk_emc(hw);
+	struct tegra_clk_periph *periph = emc->periph;
+	const struct clk_ops *div_ops = periph->div_ops;
+	struct clk_hw *div_hw = &periph->divider.hw;
+	int ret = -EINVAL;
+
+	if (!emc->emc_ops)
+		goto out;
+
+	ret = emc->emc_ops->emc_set_rate(rate);
+	if (ret)
+		goto out;
+
+	div_ops->set_rate(div_hw, rate, parent_rate);
+
+out:
+	return ret;
+}
+
+static int clk_emc_is_enabled(struct clk_hw *hw)
+{
+	struct tegra_clk_emc *emc = to_clk_emc(hw);
+	const struct clk_ops *gate_ops = emc->periph->gate_ops;
+	struct clk_hw *gate_hw = &emc->periph->gate.hw;
+
+	gate_hw->clk = hw->clk;
+
+	return gate_ops->is_enabled(gate_hw);
+}
+
+static int clk_emc_enable(struct clk_hw *hw)
+{
+	struct tegra_clk_emc *emc = to_clk_emc(hw);
+	const struct clk_ops *gate_ops = emc->periph->gate_ops;
+	struct clk_hw *gate_hw = &emc->periph->gate.hw;
+
+	gate_hw->clk = hw->clk;
+
+	return gate_ops->enable(gate_hw);
+}
+
+static void clk_emc_disable(struct clk_hw *hw)
+{
+	struct tegra_clk_emc *emc = to_clk_emc(hw);
+	const struct clk_ops *gate_ops = emc->periph->gate_ops;
+	struct clk_hw *gate_hw = &emc->periph->gate.hw;
+
+	gate_ops->disable(gate_hw);
+}
+
+void tegra_register_emc_clk_ops(struct clk *emc_clk,
+				const struct emc_clk_ops *emc_ops)
+{
+	struct clk_hw *hw;
+	struct tegra_clk_emc *emc;
+
+	if (IS_ERR_OR_NULL(emc_clk))
+		return;
+	hw = __clk_get_hw(emc_clk);
+
+	emc = to_clk_emc(hw);
+	if (emc)
+		emc->emc_ops = emc_ops;
+}
+
+static const struct clk_ops tegra_clk_emc_ops = {
+	.get_parent = clk_emc_get_parent,
+	.recalc_rate = clk_emc_recalc_rate,
+	.round_rate = clk_emc_round_rate,
+	.set_rate = clk_emc_set_rate,
+	.is_enabled = clk_emc_is_enabled,
+	.enable = clk_emc_enable,
+	.disable = clk_emc_disable,
+};
+
+struct clk *tegra_clk_register_emc(const char *name, const char **parent_names,
+	int num_parents, struct tegra_clk_periph *periph,
+	void __iomem *clk_base, u32 offset, unsigned long flags)
+{
+	struct tegra_clk_emc *emc;
+	struct clk *clk;
+	struct clk_init_data init;
+	struct tegra_clk_periph_regs *bank;
+
+	emc = kzalloc(sizeof(*emc), GFP_KERNEL);
+	if (!emc) {
+		pr_err("%s: could not allocate emc clk\n", __func__);
+		return ERR_PTR(-ENOMEM);
+	}
+
+	init.name = name;
+	init.ops = &tegra_clk_emc_ops;
+	init.flags = flags;
+	init.parent_names = parent_names;
+	init.num_parents = num_parents;
+
+	bank = get_reg_bank(periph->gate.clk_num);
+	if (!bank)
+		return ERR_PTR(-EINVAL);
+
+	periph->magic = TEGRA_CLK_PERIPH_MAGIC;
+	periph->mux.reg = clk_base + offset;
+	periph->divider.reg = clk_base + offset;
+	periph->gate.clk_base = clk_base;
+	periph->gate.regs = bank;
+	periph->gate.enable_refcnt = periph_clk_enb_refcnt;
+
+	/* Data in .init is copied by clk_register(), so stack variable OK */
+	emc->hw.init = &init;
+	emc->periph = periph;
+	clk = clk_register(NULL, &emc->hw);
+	if (IS_ERR(clk)) {
+		kfree(emc);
+		return clk;
+	}
+
+	emc->periph->mux.hw.clk = clk;
+	emc->periph->divider.hw.clk = clk;
+	emc->periph->gate.hw.clk = clk;
+
+	return clk;
+}
diff --git a/drivers/clk/tegra/clk.h b/drivers/clk/tegra/clk.h
index 16ec8d6bb87f..381a9b486805 100644
--- a/drivers/clk/tegra/clk.h
+++ b/drivers/clk/tegra/clk.h
@@ -511,6 +511,25 @@ struct tegra_periph_init_data {
 			NULL, 0, NULL)
 
 /**
+ * struct clk-emc - emc clock
+ *
+ * @hw:         handle between common and hardware-specific interfaces
+ * @periph:     periph clock
+ * @emc_ops:    emc ops
+ */
+struct tegra_clk_emc {
+	struct clk_hw			hw;
+	struct tegra_clk_periph		*periph;
+	const struct emc_clk_ops	*emc_ops;
+};
+
+#define to_clk_emc(_hw) container_of(_hw, struct tegra_clk_emc, hw)
+
+struct clk *tegra_clk_register_emc(const char *name, const char **parent_names,
+		int num_parents, struct tegra_clk_periph *periph,
+		void __iomem *clk_base, u32 offset, unsigned long flags);
+
+/**
  * struct clk_super_mux - super clock
  *
  * @hw:		handle between common and hardware-specific interfaces
diff --git a/include/linux/platform_data/tegra_emc.h b/include/linux/platform_data/tegra_emc.h
index df67505e98f8..f36cb58932d2 100644
--- a/include/linux/platform_data/tegra_emc.h
+++ b/include/linux/platform_data/tegra_emc.h
@@ -31,4 +31,11 @@ struct tegra_emc_pdata {
 	struct tegra_emc_table *tables;
 };
 
+struct emc_clk_ops {
+	long		(*emc_round_rate)(unsigned long);
+	int		(*emc_set_rate)(unsigned long);
+};
+
+void tegra_register_emc_clk_ops(struct clk *emc_clk,
+				const struct emc_clk_ops *emc_ops);
 #endif
-- 
1.8.5

[PATCH 3/4] memory: tegra20-emc: move out Tegra20 EMC driver from mach-tegra

[Joseph Lo]

This patch moves out the Tegra20 EMC driver from mach-tegra to the
drivers/memory folder. And register the EMC driver to the EMC interface
of the Tegra CCF driver.

Signed-off-by: Joseph Lo <josephl@nvidia.com>
---
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 arch/arm/mach-tegra/Makefile                       |  1 -
 arch/arm/mach-tegra/tegra2_emc.h                   | 24 ----------------------
 drivers/memory/Kconfig                             | 10 +++++++++
 drivers/memory/Makefile                            |  1 +
 .../tegra2_emc.c => drivers/memory/tegra20-emc.c   | 14 +++++++++----
 5 files changed, 21 insertions(+), 29 deletions(-)
 delete mode 100644 arch/arm/mach-tegra/tegra2_emc.h
 rename arch/arm/mach-tegra/tegra2_emc.c => drivers/memory/tegra20-emc.c (95%)

diff --git a/arch/arm/mach-tegra/Makefile b/arch/arm/mach-tegra/Makefile
index 019bb1758662..6fbfbb77dcd9 100644
--- a/arch/arm/mach-tegra/Makefile
+++ b/arch/arm/mach-tegra/Makefile
@@ -14,7 +14,6 @@ obj-y					+= sleep.o
 obj-y					+= tegra.o
 obj-$(CONFIG_CPU_IDLE)			+= cpuidle.o
 obj-$(CONFIG_ARCH_TEGRA_2x_SOC)		+= tegra20_speedo.o
-obj-$(CONFIG_ARCH_TEGRA_2x_SOC)		+= tegra2_emc.o
 obj-$(CONFIG_ARCH_TEGRA_2x_SOC)		+= sleep-tegra20.o
 obj-$(CONFIG_ARCH_TEGRA_2x_SOC)		+= pm-tegra20.o
 ifeq ($(CONFIG_CPU_IDLE),y)
diff --git a/arch/arm/mach-tegra/tegra2_emc.h b/arch/arm/mach-tegra/tegra2_emc.h
deleted file mode 100644
index f61409b54cb7..000000000000
--- a/arch/arm/mach-tegra/tegra2_emc.h
+++ /dev/null
@@ -1,24 +0,0 @@
-/*
- * Copyright (C) 2011 Google, Inc.
- *
- * Author:
- *	Colin Cross <ccross@android.com>
- *
- * This software is licensed under the terms of the GNU General Public
- * License version 2, as published by the Free Software Foundation, and
- * may be copied, distributed, and modified under those terms.
- *
- * This program is distributed in the hope that it will be useful,
- * but WITHOUT ANY WARRANTY; without even the implied warranty of
- * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
- * GNU General Public License for more details.
- *
- */
-
-#ifndef __MACH_TEGRA_TEGRA2_EMC_H_
-#define __MACH_TEGRA_TEGRA2_EMC_H
-
-int tegra_emc_set_rate(unsigned long rate);
-long tegra_emc_round_rate(unsigned long rate);
-
-#endif
diff --git a/drivers/memory/Kconfig b/drivers/memory/Kconfig
index 29a11db365bc..55017b9bdb6b 100644
--- a/drivers/memory/Kconfig
+++ b/drivers/memory/Kconfig
@@ -40,6 +40,16 @@ config TEGRA20_MC
 	  analysis, especially for IOMMU/GART(Graphics Address
 	  Relocation Table) module.
 
+config TEGRA20_EMC
+	bool "Tegra20 External Memory Controller(EMC) driver"
+	default y
+	depends on ARCH_TEGRA_2x_SOC
+	help
+	  This driver is for the External Memory Controller(EMC) module
+	  available in Tegra20 SoCs. It was used for setting up the timing
+	  in the EMC registers when scaling the memory frequency. To enable
+	  the function, you should also say Y in TEGRA_EMC_SCALING_ENABLE.
+
 config TEGRA30_MC
 	bool "Tegra30 Memory Controller(MC) driver"
 	default y
diff --git a/drivers/memory/Makefile b/drivers/memory/Makefile
index 969d923dad93..23e920dd4ebf 100644
--- a/drivers/memory/Makefile
+++ b/drivers/memory/Makefile
@@ -7,5 +7,6 @@ obj-$(CONFIG_OF)		+= of_memory.o
 endif
 obj-$(CONFIG_TI_EMIF)		+= emif.o
 obj-$(CONFIG_MVEBU_DEVBUS)	+= mvebu-devbus.o
+obj-$(CONFIG_TEGRA20_EMC)	+= tegra20-emc.o
 obj-$(CONFIG_TEGRA20_MC)	+= tegra20-mc.o
 obj-$(CONFIG_TEGRA30_MC)	+= tegra30-mc.o
diff --git a/arch/arm/mach-tegra/tegra2_emc.c b/drivers/memory/tegra20-emc.c
similarity index 95%
rename from arch/arm/mach-tegra/tegra2_emc.c
rename to drivers/memory/tegra20-emc.c
index 26e4edbd8a6a..de2a89089b1a 100644
--- a/arch/arm/mach-tegra/tegra2_emc.c
+++ b/drivers/memory/tegra20-emc.c
@@ -26,8 +26,6 @@
 #include <linux/platform_data/tegra_emc.h>
 #include <linux/tegra-soc.h>
 
-#include "tegra2_emc.h"
-
 #ifdef CONFIG_TEGRA_EMC_SCALING_ENABLE
 static bool emc_enable = true;
 #else
@@ -98,7 +96,7 @@ static const unsigned long emc_reg_addr[TEGRA_EMC_NUM_REGS] = {
 };
 
 /* Select the closest EMC rate that is higher than the requested rate */
-long tegra_emc_round_rate(unsigned long rate)
+static long tegra20_emc_round_rate(unsigned long rate)
 {
 	struct tegra_emc_pdata *pdata;
 	int i;
@@ -142,7 +140,7 @@ long tegra_emc_round_rate(unsigned long rate)
  * and relies on the clock lock on the emc clock to avoid races between
  * multiple frequency changes
  */
-int tegra_emc_set_rate(unsigned long rate)
+static int tegra20_emc_set_rate(unsigned long rate)
 {
 	struct tegra_emc_pdata *pdata;
 	int i;
@@ -296,6 +294,11 @@ static struct tegra_emc_pdata *tegra_emc_fill_pdata(struct platform_device *pdev
 	return pdata;
 }
 
+static const struct emc_clk_ops tegra20_emc_clk_ops = {
+	.emc_round_rate = tegra20_emc_round_rate,
+	.emc_set_rate = tegra20_emc_set_rate,
+};
+
 static int tegra_emc_probe(struct platform_device *pdev)
 {
 	struct tegra_emc_pdata *pdata;
@@ -323,6 +326,9 @@ static int tegra_emc_probe(struct platform_device *pdev)
 
 	emc_pdev = pdev;
 
+	tegra_register_emc_clk_ops(clk_get_sys(NULL, "emc"),
+				   &tegra20_emc_clk_ops);
+
 	return 0;
 }
 
-- 
1.8.5

[PATCH 4/4] clk: tegra20: enable EMC clock driver

[Joseph Lo]

Re-register the EMC clock to the EMC clock driver in the Tegra CCF
driver to support EMC scaling.

Signed-off-by: Joseph Lo <josephl@nvidia.com>
---
Cc: Mike Turquette <mturquette@linaro.org>
---
 drivers/clk/tegra/clk-tegra20.c | 25 +++++++++++++++----------
 1 file changed, 15 insertions(+), 10 deletions(-)

diff --git a/drivers/clk/tegra/clk-tegra20.c b/drivers/clk/tegra/clk-tegra20.c
index dbace152b2fa..ecae0f6bb7bf 100644
--- a/drivers/clk/tegra/clk-tegra20.c
+++ b/drivers/clk/tegra/clk-tegra20.c
@@ -795,6 +795,20 @@ static struct tegra_periph_init_data tegra_periph_nodiv_clk_list[] = {
 	TEGRA_INIT_DATA_NODIV("disp2",	mux_pllpdc_clkm, CLK_SOURCE_DISP2, 30, 2, 26,  0, TEGRA20_CLK_DISP2),
 };
 
+static struct tegra_clk_periph tegra_emc_periph =
+	TEGRA_CLK_PERIPH(30, 3, 0, 0, 8, 1, 0, TEGRA20_CLK_EMC, 0, NULL, NULL);
+
+static __init void tegra20_emc_clk_init(void __iomem *clk_base)
+{
+	struct clk *clk;
+
+	clk = tegra_clk_register_emc("emc", mux_pllmcp_clkm,
+		ARRAY_SIZE(mux_pllmcp_clkm), &tegra_emc_periph, clk_base,
+		CLK_SOURCE_EMC, CLK_IGNORE_UNUSED | CLK_GET_RATE_NOCACHE);
+
+	clks[TEGRA20_CLK_EMC] = clk;
+}
+
 static void __init tegra20_periph_clk_init(void)
 {
 	struct tegra_periph_init_data *data;
@@ -812,16 +826,6 @@ static void __init tegra20_periph_clk_init(void)
 				    0, 34, periph_clk_enb_refcnt);
 	clks[TEGRA20_CLK_APBDMA] = clk;
 
-	/* emc */
-	clk = clk_register_mux(NULL, "emc_mux", mux_pllmcp_clkm,
-			       ARRAY_SIZE(mux_pllmcp_clkm),
-			       CLK_SET_RATE_NO_REPARENT,
-			       clk_base + CLK_SOURCE_EMC,
-			       30, 2, 0, NULL);
-	clk = tegra_clk_register_periph_gate("emc", "emc_mux", 0, clk_base, 0,
-				    57, periph_clk_enb_refcnt);
-	clks[TEGRA20_CLK_EMC] = clk;
-
 	/* dsi */
 	clk = tegra_clk_register_periph_gate("dsi", "pll_d", 0, clk_base, 0,
 				    48, periph_clk_enb_refcnt);
@@ -1114,6 +1118,7 @@ static void __init tegra20_clock_init(struct device_node *np)
 	tegra20_super_clk_init();
 	tegra_super_clk_gen4_init(clk_base, pmc_base, tegra20_clks, NULL);
 	tegra20_periph_clk_init();
+	tegra20_emc_clk_init(clk_base);
 	tegra20_audio_clk_init();
 	tegra_pmc_clk_init(pmc_base, tegra20_clks);
 
-- 
1.8.5

[PATCH 1/4] ARM: tegra: moving tegra_bct_strapping to tegra-soc.h for global visibility

[Stephen Warren]

On 12/17/2013 02:26 AM, Joseph Lo wrote:
> This patch moves the tegra_btc_strapping variable to the tegra-soc.h for
> the global visibility that the other Tegra device driver can access it.
> It also a preparation that we can move out the Tegra20 EMC driver from
> mach-tegra to the drivers folder.

Rather than introduce yet more dependencies between the Tegra tree and
other trees for 3.14, I'd like to see the two clock changes in this
series taken into the clock tree for 3.14, and the other 2 patches
deferred for 3.15.

Joseph, I assume that if patches 2/4 and 4/4 are applied without patches
1/4 and 3/4, there will be no issues?

This patch looks fine, I think ...

[PATCH 2/4] clk: tegra: add EMC clock driver

[Stephen Warren]

On 12/17/2013 02:26 AM, Joseph Lo wrote:
> Add External Memory Controller (EMC) clock interface for the Tegra CCF
> driver to support EMC scaling.

> diff --git a/drivers/clk/tegra/clk-emc.c b/drivers/clk/tegra/clk-emc.c

> +static long clk_emc_round_rate(struct clk_hw *hw, unsigned long rate,
> +			       unsigned long *prate)
> +{
> +	struct tegra_clk_emc *emc = to_clk_emc(hw);
> +	struct clk *parent_clk = __clk_get_parent(hw->clk);
> +	unsigned long parent_rate = __clk_get_rate(parent_clk);
> +	unsigned long ret;
> +
> +	if (!emc->emc_ops)
> +		return parent_rate;
> +
> +	ret = emc->emc_ops->emc_round_rate(rate);
> +	if (!ret)
> +		return parent_rate;
> +
> +	return ret;
> +}

Rather than implementing this custom "emc_ops" feature, isn't there a
standard clock notifier feature that the EMC driver can use?

Isn't the EMC driver the only thing that will be changing the EMC clock
rate? If so, why not just have the EMC driver perform the appropriate
pre-/post-rate-change actions before/after calling clk_set_rate()?

> +void tegra_register_emc_clk_ops(struct clk *emc_clk,
> +				const struct emc_clk_ops *emc_ops)
> +{
> +	struct clk_hw *hw;
> +	struct tegra_clk_emc *emc;
> +
> +	if (IS_ERR_OR_NULL(emc_clk))
> +		return;

IS_ERROR_OR_NULL() shouldn't be used. "struct clk *" is either IS_ERR()
on error, or it's NULL on error, not both. The body of clk_get() implies
you need to use IS_ERR().

[PATCH 1/4] ARM: tegra: moving tegra_bct_strapping to tegra-soc.h for global visibility

[Stephen Warren]

On 12/17/2013 02:26 AM, Joseph Lo wrote:
> This patch moves the tegra_btc_strapping variable to the tegra-soc.h for

s/btc/bct/

[PATCH 4/4] clk: tegra20: enable EMC clock driver

[Stephen Warren]

On 12/17/2013 02:26 AM, Joseph Lo wrote:
> Re-register the EMC clock to the EMC clock driver in the Tegra CCF
> driver to support EMC scaling.

Can't this be squashed into patch 2/4?

[PATCH 1/4] ARM: tegra: moving tegra_bct_strapping to tegra-soc.h for global visibility

[Joseph Lo]

On Wed, 2013-12-18 at 06:53 +0800, Stephen Warren wrote:
> On 12/17/2013 02:26 AM, Joseph Lo wrote:
> > This patch moves the tegra_btc_strapping variable to the tegra-soc.h for
> > the global visibility that the other Tegra device driver can access it.
> > It also a preparation that we can move out the Tegra20 EMC driver from
> > mach-tegra to the drivers folder.
> 
> Rather than introduce yet more dependencies between the Tegra tree and
> other trees for 3.14, I'd like to see the two clock changes in this
> series taken into the clock tree for 3.14, and the other 2 patches
> deferred for 3.15.
> 
OK, sounds fine to me. So I will just update 2/4 in this series after
squashing 2/4 and 4/4 into one patch. And resend the other two patches
when 3.14-rc1 shows up.

> Joseph, I assume that if patches 2/4 and 4/4 are applied without patches
> 1/4 and 3/4, there will be no issues?
Yes. It's OK.
> 
> This patch looks fine, I think ...

[PATCH 2/4] clk: tegra: add EMC clock driver

[Joseph Lo]

On Wed, 2013-12-18 at 06:58 +0800, Stephen Warren wrote:
> On 12/17/2013 02:26 AM, Joseph Lo wrote:
> > Add External Memory Controller (EMC) clock interface for the Tegra CCF
> > driver to support EMC scaling.
> 
> > diff --git a/drivers/clk/tegra/clk-emc.c b/drivers/clk/tegra/clk-emc.c
> 
> > +static long clk_emc_round_rate(struct clk_hw *hw, unsigned long rate,
> > +			       unsigned long *prate)
> > +{
> > +	struct tegra_clk_emc *emc = to_clk_emc(hw);
> > +	struct clk *parent_clk = __clk_get_parent(hw->clk);
> > +	unsigned long parent_rate = __clk_get_rate(parent_clk);
> > +	unsigned long ret;
> > +
> > +	if (!emc->emc_ops)
> > +		return parent_rate;
> > +
> > +	ret = emc->emc_ops->emc_round_rate(rate);
> > +	if (!ret)
> > +		return parent_rate;
> > +
> > +	return ret;
> > +}
> 
> Rather than implementing this custom "emc_ops" feature, isn't there a
> standard clock notifier feature that the EMC driver can use?
> 
> Isn't the EMC driver the only thing that will be changing the EMC clock
> rate? If so, why not just have the EMC driver perform the appropriate
> pre-/post-rate-change actions before/after calling clk_set_rate()?

We have two HW components needs to be updated when EMC rate change. One
is the EMC clock for tuning the frequency, the other is the EMC for
updating the timing and configuration for external memory (DRAM). So
this question looks like to me is that can we separate the operation of
the two components when rate changing?

We have two modes that depend on what memory type (DDR2, LPDDR2,
DDR3/LP) we used on the platform to support EMC scaling.
1. power down mode (Tegra20 used this mode only.)
In this mode, we update the EMC timing and configurations to EMC shadow
registers. Then updating the rate in the EMC clock register. The HW will
trigger the rate changing and timing/configurations updating for EMC.
2. self-refresh mode (Tegra30/114/124 if DDR3)
More complicate in this mode. Putting DRAM in self-refresh, updating EMC
settings, updating EMC clock, then we still need auto calibration before
restores DRAM from the self-refresh mode. So the difference was the EMC
clock operation was part of EMC scaling procedures.

I guess using the clk_notifier may be OK for the 1st case.

I suggest create a EMC clock driver to work closely with EMC driver.
That would help us to represent fully clock function of EMC clock.

> 
> > +void tegra_register_emc_clk_ops(struct clk *emc_clk,
> > +				const struct emc_clk_ops *emc_ops)
> > +{
> > +	struct clk_hw *hw;
> > +	struct tegra_clk_emc *emc;
> > +
> > +	if (IS_ERR_OR_NULL(emc_clk))
> > +		return;
> 
> IS_ERROR_OR_NULL() shouldn't be used. "struct clk *" is either IS_ERR()
> on error, or it's NULL on error, not both. The body of clk_get() implies
> you need to use IS_ERR().

True, will fix.

[PATCH 2/4] clk: tegra: add EMC clock driver

[Stephen Warren]

On 12/18/2013 02:42 AM, Joseph Lo wrote:
> On Wed, 2013-12-18 at 06:58 +0800, Stephen Warren wrote:
>> On 12/17/2013 02:26 AM, Joseph Lo wrote:
>>> Add External Memory Controller (EMC) clock interface for the Tegra CCF
>>> driver to support EMC scaling.
>>
>>> diff --git a/drivers/clk/tegra/clk-emc.c b/drivers/clk/tegra/clk-emc.c
>>
>>> +static long clk_emc_round_rate(struct clk_hw *hw, unsigned long rate,
>>> +			       unsigned long *prate)
>>> +{
>>> +	struct tegra_clk_emc *emc = to_clk_emc(hw);
>>> +	struct clk *parent_clk = __clk_get_parent(hw->clk);
>>> +	unsigned long parent_rate = __clk_get_rate(parent_clk);
>>> +	unsigned long ret;
>>> +
>>> +	if (!emc->emc_ops)
>>> +		return parent_rate;
>>> +
>>> +	ret = emc->emc_ops->emc_round_rate(rate);
>>> +	if (!ret)
>>> +		return parent_rate;
>>> +
>>> +	return ret;
>>> +}
>>
>> Rather than implementing this custom "emc_ops" feature, isn't there a
>> standard clock notifier feature that the EMC driver can use?
>>
>> Isn't the EMC driver the only thing that will be changing the EMC clock
>> rate? If so, why not just have the EMC driver perform the appropriate
>> pre-/post-rate-change actions before/after calling clk_set_rate()?
> 
> We have two HW components needs to be updated when EMC rate change. One
> is the EMC clock for tuning the frequency, the other is the EMC for
> updating the timing and configuration for external memory (DRAM). So
> this question looks like to me is that can we separate the operation of
> the two components when rate changing?
> 
> We have two modes that depend on what memory type (DDR2, LPDDR2,
> DDR3/LP) we used on the platform to support EMC scaling.
> 1. power down mode (Tegra20 used this mode only.)
> In this mode, we update the EMC timing and configurations to EMC shadow
> registers. Then updating the rate in the EMC clock register. The HW will
> trigger the rate changing and timing/configurations updating for EMC.
> 2. self-refresh mode (Tegra30/114/124 if DDR3)
> More complicate in this mode. Putting DRAM in self-refresh, updating EMC
> settings, updating EMC clock, then we still need auto calibration before
> restores DRAM from the self-refresh mode. So the difference was the EMC
> clock operation was part of EMC scaling procedures.
> 
> I guess using the clk_notifier may be OK for the 1st case.

In both cases, isn't the overall operation something like:

a) Do some work before changing the EMC clock
b) Change the EMC clock
c) Do some work after changing the EMC clock

Admittedly, the exact definition of "some work" is different for your
cases (1) and (2) above, but the overall structure is the same. As such,
can't the EMC scaling driver do (a), then do (b) i.e. call
clk_set_rate(), then do (c)? Or, in your case (2), do we need to do
funny tricks like running from IRAM since we can't access SDRAM during
the clock change? If so, I'm not sure how having the EMC clock changing
code is going to help your case (2) anyway, since we'll presumably have
to code up a custom stub in assembly for the part of the code that runs
from IRAM...

[PATCH 2/4] clk: tegra: add EMC clock driver

[Mike Turquette]

Quoting Stephen Warren (2013-12-18 10:28:32)
> On 12/18/2013 02:42 AM, Joseph Lo wrote:
> > On Wed, 2013-12-18 at 06:58 +0800, Stephen Warren wrote:
> >> On 12/17/2013 02:26 AM, Joseph Lo wrote:
> >>> Add External Memory Controller (EMC) clock interface for the Tegra CCF
> >>> driver to support EMC scaling.
> >>
> >>> diff --git a/drivers/clk/tegra/clk-emc.c b/drivers/clk/tegra/clk-emc.c
> >>
> >>> +static long clk_emc_round_rate(struct clk_hw *hw, unsigned long rate,
> >>> +                          unsigned long *prate)
> >>> +{
> >>> +   struct tegra_clk_emc *emc = to_clk_emc(hw);
> >>> +   struct clk *parent_clk = __clk_get_parent(hw->clk);
> >>> +   unsigned long parent_rate = __clk_get_rate(parent_clk);
> >>> +   unsigned long ret;
> >>> +
> >>> +   if (!emc->emc_ops)
> >>> +           return parent_rate;
> >>> +
> >>> +   ret = emc->emc_ops->emc_round_rate(rate);
> >>> +   if (!ret)
> >>> +           return parent_rate;
> >>> +
> >>> +   return ret;
> >>> +}
> >>
> >> Rather than implementing this custom "emc_ops" feature, isn't there a
> >> standard clock notifier feature that the EMC driver can use?
> >>
> >> Isn't the EMC driver the only thing that will be changing the EMC clock
> >> rate? If so, why not just have the EMC driver perform the appropriate
> >> pre-/post-rate-change actions before/after calling clk_set_rate()?
> > 
> > We have two HW components needs to be updated when EMC rate change. One
> > is the EMC clock for tuning the frequency, the other is the EMC for
> > updating the timing and configuration for external memory (DRAM). So
> > this question looks like to me is that can we separate the operation of
> > the two components when rate changing?
> > 
> > We have two modes that depend on what memory type (DDR2, LPDDR2,
> > DDR3/LP) we used on the platform to support EMC scaling.
> > 1. power down mode (Tegra20 used this mode only.)
> > In this mode, we update the EMC timing and configurations to EMC shadow
> > registers. Then updating the rate in the EMC clock register. The HW will
> > trigger the rate changing and timing/configurations updating for EMC.
> > 2. self-refresh mode (Tegra30/114/124 if DDR3)
> > More complicate in this mode. Putting DRAM in self-refresh, updating EMC
> > settings, updating EMC clock, then we still need auto calibration before
> > restores DRAM from the self-refresh mode. So the difference was the EMC
> > clock operation was part of EMC scaling procedures.
> > 
> > I guess using the clk_notifier may be OK for the 1st case.
> 
> In both cases, isn't the overall operation something like:
> 
> a) Do some work before changing the EMC clock
> b) Change the EMC clock
> c) Do some work after changing the EMC clock
> 
> Admittedly, the exact definition of "some work" is different for your
> cases (1) and (2) above, but the overall structure is the same. As such,
> can't the EMC scaling driver do (a), then do (b) i.e. call
> clk_set_rate(), then do (c)? Or, in your case (2), do we need to do
> funny tricks like running from IRAM since we can't access SDRAM during
> the clock change? If so, I'm not sure how having the EMC clock changing
> code is going to help your case (2) anyway, since we'll presumably have
> to code up a custom stub in assembly for the part of the code that runs
> from IRAM...

Joseph,

Just as a reference, check out how the smp_twd stuff is updated based on
a clock rate-change notifier in arch/arm/kernel/smp_twd.c, lines
103-142.

Regards,
Mike

> 

[PATCH 2/4] clk: tegra: add EMC clock driver

[Joseph Lo]

On Thu, 2013-12-19 at 03:30 +0800, Mike Turquette wrote:
> Quoting Stephen Warren (2013-12-18 10:28:32)
> > On 12/18/2013 02:42 AM, Joseph Lo wrote:
> > > On Wed, 2013-12-18 at 06:58 +0800, Stephen Warren wrote:
> > >> On 12/17/2013 02:26 AM, Joseph Lo wrote:
> > >>> Add External Memory Controller (EMC) clock interface for the Tegra CCF
> > >>> driver to support EMC scaling.
> > >>
> > >>> diff --git a/drivers/clk/tegra/clk-emc.c b/drivers/clk/tegra/clk-emc.c
> > >>
> > >>> +static long clk_emc_round_rate(struct clk_hw *hw, unsigned long rate,
> > >>> +                          unsigned long *prate)
> > >>> +{
> > >>> +   struct tegra_clk_emc *emc = to_clk_emc(hw);
> > >>> +   struct clk *parent_clk = __clk_get_parent(hw->clk);
> > >>> +   unsigned long parent_rate = __clk_get_rate(parent_clk);
> > >>> +   unsigned long ret;
> > >>> +
> > >>> +   if (!emc->emc_ops)
> > >>> +           return parent_rate;
> > >>> +
> > >>> +   ret = emc->emc_ops->emc_round_rate(rate);
> > >>> +   if (!ret)
> > >>> +           return parent_rate;
> > >>> +
> > >>> +   return ret;
> > >>> +}
> > >>
> > >> Rather than implementing this custom "emc_ops" feature, isn't there a
> > >> standard clock notifier feature that the EMC driver can use?
> > >>
> > >> Isn't the EMC driver the only thing that will be changing the EMC clock
> > >> rate? If so, why not just have the EMC driver perform the appropriate
> > >> pre-/post-rate-change actions before/after calling clk_set_rate()?
> > > 
> > > We have two HW components needs to be updated when EMC rate change. One
> > > is the EMC clock for tuning the frequency, the other is the EMC for
> > > updating the timing and configuration for external memory (DRAM). So
> > > this question looks like to me is that can we separate the operation of
> > > the two components when rate changing?
> > > 
> > > We have two modes that depend on what memory type (DDR2, LPDDR2,
> > > DDR3/LP) we used on the platform to support EMC scaling.
> > > 1. power down mode (Tegra20 used this mode only.)
> > > In this mode, we update the EMC timing and configurations to EMC shadow
> > > registers. Then updating the rate in the EMC clock register. The HW will
> > > trigger the rate changing and timing/configurations updating for EMC.
> > > 2. self-refresh mode (Tegra30/114/124 if DDR3)
> > > More complicate in this mode. Putting DRAM in self-refresh, updating EMC
> > > settings, updating EMC clock, then we still need auto calibration before
> > > restores DRAM from the self-refresh mode. So the difference was the EMC
> > > clock operation was part of EMC scaling procedures.
> > > 
> > > I guess using the clk_notifier may be OK for the 1st case.
> > 
> > In both cases, isn't the overall operation something like:
> > 
> > a) Do some work before changing the EMC clock
> > b) Change the EMC clock
> > c) Do some work after changing the EMC clock
> > 
> > Admittedly, the exact definition of "some work" is different for your
> > cases (1) and (2) above, but the overall structure is the same. As such,
> > can't the EMC scaling driver do (a), then do (b) i.e. call
> > clk_set_rate(), then do (c)? Or, in your case (2), do we need to do
> > funny tricks like running from IRAM since we can't access SDRAM during
> > the clock change? If so, I'm not sure how having the EMC clock changing
> > code is going to help your case (2) anyway, since we'll presumably have
> > to code up a custom stub in assembly for the part of the code that runs
> > from IRAM...
> 
> Joseph,
> 
> Just as a reference, check out how the smp_twd stuff is updated based on
> a clock rate-change notifier in arch/arm/kernel/smp_twd.c, lines
> 103-142.
> 
Thanks.

Joseph

[PATCH 2/4] clk: tegra: add EMC clock driver

[Joseph Lo]

On Thu, 2013-12-19 at 02:28 +0800, Stephen Warren wrote:
> On 12/18/2013 02:42 AM, Joseph Lo wrote:
> > On Wed, 2013-12-18 at 06:58 +0800, Stephen Warren wrote:
> >> On 12/17/2013 02:26 AM, Joseph Lo wrote:
> >>> Add External Memory Controller (EMC) clock interface for the Tegra CCF
> >>> driver to support EMC scaling.
> >>
> >>> diff --git a/drivers/clk/tegra/clk-emc.c b/drivers/clk/tegra/clk-emc.c
> >>
> >>> +static long clk_emc_round_rate(struct clk_hw *hw, unsigned long rate,
> >>> +			       unsigned long *prate)
> >>> +{
> >>> +	struct tegra_clk_emc *emc = to_clk_emc(hw);
> >>> +	struct clk *parent_clk = __clk_get_parent(hw->clk);
> >>> +	unsigned long parent_rate = __clk_get_rate(parent_clk);
> >>> +	unsigned long ret;
> >>> +
> >>> +	if (!emc->emc_ops)
> >>> +		return parent_rate;
> >>> +
> >>> +	ret = emc->emc_ops->emc_round_rate(rate);
> >>> +	if (!ret)
> >>> +		return parent_rate;
> >>> +
> >>> +	return ret;
> >>> +}
> >>
> >> Rather than implementing this custom "emc_ops" feature, isn't there a
> >> standard clock notifier feature that the EMC driver can use?
> >>
> >> Isn't the EMC driver the only thing that will be changing the EMC clock
> >> rate? If so, why not just have the EMC driver perform the appropriate
> >> pre-/post-rate-change actions before/after calling clk_set_rate()?
> > 
> > We have two HW components needs to be updated when EMC rate change. One
> > is the EMC clock for tuning the frequency, the other is the EMC for
> > updating the timing and configuration for external memory (DRAM). So
> > this question looks like to me is that can we separate the operation of
> > the two components when rate changing?
> > 
> > We have two modes that depend on what memory type (DDR2, LPDDR2,
> > DDR3/LP) we used on the platform to support EMC scaling.
> > 1. power down mode (Tegra20 used this mode only.)
> > In this mode, we update the EMC timing and configurations to EMC shadow
> > registers. Then updating the rate in the EMC clock register. The HW will
> > trigger the rate changing and timing/configurations updating for EMC.
> > 2. self-refresh mode (Tegra30/114/124 if DDR3)
> > More complicate in this mode. Putting DRAM in self-refresh, updating EMC
> > settings, updating EMC clock, then we still need auto calibration before
> > restores DRAM from the self-refresh mode. So the difference was the EMC
> > clock operation was part of EMC scaling procedures.
> > 
> > I guess using the clk_notifier may be OK for the 1st case.
> 
> In both cases, isn't the overall operation something like:
> 
> a) Do some work before changing the EMC clock
> b) Change the EMC clock
> c) Do some work after changing the EMC clock
> 
Roughly say, yes. But ...

We still need an EMC clock driver. Currently there is no clock function
in Tegra clock driver can just support it.

For example,

* the set_rate function
We had an EMC table that includes whole the rates, EMC settings and EMC
clock settings that we should run at the rate of MC/2 or same with MC.
We can't just set the EMC clock rate without the info came from the
table. It's pre-define, pre-setup for the platform and SDRAM module. So
the operation can't be separated into PRE_RATE_CHANGE or
POST_RATE_CHANGE.

* the round_rate function
We also need to get a supported rate for the EMC/EMC clock from the EMC
table.

If we ignore the case above, then we need a totally new EMC table that
only supports fixed rate of MC rate and only support one mode for rate
changing. That means only Tegra20 support it currently.

> Admittedly, the exact definition of "some work" is different for your
> cases (1) and (2) above, but the overall structure is the same. As such,
> can't the EMC scaling driver do (a), then do (b) i.e. call
> clk_set_rate(), then do (c)? Or, in your case (2), do we need to do
> funny tricks like running from IRAM since we can't access SDRAM during
> the clock change? If so, I'm not sure how having the EMC clock changing
> code is going to help your case (2) anyway, since we'll presumably have
> to code up a custom stub in assembly for the part of the code that runs
> from IRAM...
> 
When updating EMC rate, we also need flow controller support to lock HW
to access SDRAM (self-refresh mode). Running these codes on IRAM can't
block other HW to do that.

[PATCH 2/4] clk: tegra: add EMC clock driver

[Peter De Schrijver]

> > I guess using the clk_notifier may be OK for the 1st case.
> 
> In both cases, isn't the overall operation something like:
> 
> a) Do some work before changing the EMC clock
> b) Change the EMC clock
> c) Do some work after changing the EMC clock
> 

There is hw interaction between the EMC controller and the CAR module.
Writing to the EMC clock source register in CAR will trigger a state machine
in the EMC controller to handle the change of memory clock. The details
of the state machine and how it needs to be setup are different for
each Tegra variant. This means we need to be sure of the exact sequence
of CAR and EMC register accesses for this to work. In theory this could
be done with pre and post rate change notifiers, but I'm afraid this would
be quite fragile.

> Admittedly, the exact definition of "some work" is different for your
> cases (1) and (2) above, but the overall structure is the same. As such,
> can't the EMC scaling driver do (a), then do (b) i.e. call
> clk_set_rate(), then do (c)? Or, in your case (2), do we need to do

We would need to be very sure clk_set_rate() doesn't access any other register
besides the EMC clock source register before returning to the EMC scaling
driver.

> funny tricks like running from IRAM since we can't access SDRAM during
> the clock change? If so, I'm not sure how having the EMC clock changing
> code is going to help your case (2) anyway, since we'll presumably have
> to code up a custom stub in assembly for the part of the code that runs
> from IRAM...
> 

There is no need for assembler or running from IRAM (at least from Tegra30
onwards, I don't know about Tegra20).

Cheers,

Peter.

[PATCH 2/4] clk: tegra: add EMC clock driver

[Lucas Stach]

Am Donnerstag, den 19.12.2013, 12:05 +0200 schrieb Peter De Schrijver:
[...]
> 
> > funny tricks like running from IRAM since we can't access SDRAM during
> > the clock change? If so, I'm not sure how having the EMC clock changing
> > code is going to help your case (2) anyway, since we'll presumably have
> > to code up a custom stub in assembly for the part of the code that runs
> > from IRAM...
> > 
> 
> There is no need for assembler or running from IRAM (at least from Tegra30
> onwards, I don't know about Tegra20).
> 
Tegra20 doesn't need any IRAM trickery, the sequence is just:
1. set up EMC shadow registers for new clock frequency
2. change EMC divider in CAR module

This was known working on Colibri T20 before the CCF change.

Regards,
Lucas

[PATCH 2/4] clk: tegra: add EMC clock driver

[Peter De Schrijver]

On Thu, Dec 19, 2013 at 12:43:28PM +0100, Lucas Stach wrote:
> Am Donnerstag, den 19.12.2013, 12:05 +0200 schrieb Peter De Schrijver:
> [...]
> > 
> > > funny tricks like running from IRAM since we can't access SDRAM during
> > > the clock change? If so, I'm not sure how having the EMC clock changing
> > > code is going to help your case (2) anyway, since we'll presumably have
> > > to code up a custom stub in assembly for the part of the code that runs
> > > from IRAM...
> > > 
> > 
> > There is no need for assembler or running from IRAM (at least from Tegra30
> > onwards, I don't know about Tegra20).
> > 
> Tegra20 doesn't need any IRAM trickery, the sequence is just:
> 1. set up EMC shadow registers for new clock frequency
> 2. change EMC divider in CAR module
> 

Nothing needs to be done after the divider change?

Cheers,

Peter.

[PATCH 2/4] clk: tegra: add EMC clock driver

[Stephen Warren]

On 12/19/2013 02:43 AM, Joseph Lo wrote:
> On Thu, 2013-12-19 at 02:28 +0800, Stephen Warren wrote:
>> On 12/18/2013 02:42 AM, Joseph Lo wrote:
>>> On Wed, 2013-12-18 at 06:58 +0800, Stephen Warren wrote:
>>>> On 12/17/2013 02:26 AM, Joseph Lo wrote:
>>>>> Add External Memory Controller (EMC) clock interface for the Tegra CCF
>>>>> driver to support EMC scaling.
>>>>
>>>>> diff --git a/drivers/clk/tegra/clk-emc.c b/drivers/clk/tegra/clk-emc.c
>>>>
>>>>> +static long clk_emc_round_rate(struct clk_hw *hw, unsigned long rate,
>>>>> +			       unsigned long *prate)
>>>>> +{
>>>>> +	struct tegra_clk_emc *emc = to_clk_emc(hw);
>>>>> +	struct clk *parent_clk = __clk_get_parent(hw->clk);
>>>>> +	unsigned long parent_rate = __clk_get_rate(parent_clk);
>>>>> +	unsigned long ret;
>>>>> +
>>>>> +	if (!emc->emc_ops)
>>>>> +		return parent_rate;
>>>>> +
>>>>> +	ret = emc->emc_ops->emc_round_rate(rate);
>>>>> +	if (!ret)
>>>>> +		return parent_rate;
>>>>> +
>>>>> +	return ret;
>>>>> +}
>>>>
>>>> Rather than implementing this custom "emc_ops" feature, isn't there a
>>>> standard clock notifier feature that the EMC driver can use?
>>>>
>>>> Isn't the EMC driver the only thing that will be changing the EMC clock
>>>> rate? If so, why not just have the EMC driver perform the appropriate
>>>> pre-/post-rate-change actions before/after calling clk_set_rate()?
>>>
>>> We have two HW components needs to be updated when EMC rate change. One
>>> is the EMC clock for tuning the frequency, the other is the EMC for
>>> updating the timing and configuration for external memory (DRAM). So
>>> this question looks like to me is that can we separate the operation of
>>> the two components when rate changing?
>>>
>>> We have two modes that depend on what memory type (DDR2, LPDDR2,
>>> DDR3/LP) we used on the platform to support EMC scaling.
>>> 1. power down mode (Tegra20 used this mode only.)
>>> In this mode, we update the EMC timing and configurations to EMC shadow
>>> registers. Then updating the rate in the EMC clock register. The HW will
>>> trigger the rate changing and timing/configurations updating for EMC.
>>> 2. self-refresh mode (Tegra30/114/124 if DDR3)
>>> More complicate in this mode. Putting DRAM in self-refresh, updating EMC
>>> settings, updating EMC clock, then we still need auto calibration before
>>> restores DRAM from the self-refresh mode. So the difference was the EMC
>>> clock operation was part of EMC scaling procedures.
>>>
>>> I guess using the clk_notifier may be OK for the 1st case.
>>
>> In both cases, isn't the overall operation something like:
>>
>> a) Do some work before changing the EMC clock
>> b) Change the EMC clock
>> c) Do some work after changing the EMC clock
>>
> Roughly say, yes. But ...
> 
> We still need an EMC clock driver. Currently there is no clock function
> in Tegra clock driver can just support it.
> 
> For example,
> 
> * the set_rate function
> We had an EMC table that includes whole the rates, EMC settings and EMC
> clock settings that we should run at the rate of MC/2 or same with MC.
> We can't just set the EMC clock rate without the info came from the
> table. It's pre-define, pre-setup for the platform and SDRAM module. So
> the operation can't be separated into PRE_RATE_CHANGE or
> POST_RATE_CHANGE.
> 
> * the round_rate function
> We also need to get a supported rate for the EMC/EMC clock from the EMC
> table.
> 
> If we ignore the case above, then we need a totally new EMC table that
> only supports fixed rate of MC rate and only support one mode for rate
> changing. That means only Tegra20 support it currently.

Isn't the EMC scaling driver the only driver that will call
clk_set_rate() on the EMC clock? As such, can't we assume that any value
passed to set_rate/round_rate for this one clock have already been
validated by the EMC driver, and hence the clock driver should just
blindly accept/apply it?

[PATCH 2/4] clk: tegra: add EMC clock driver

[Stephen Warren]

On 12/19/2013 03:05 AM, Peter De Schrijver wrote:
>>> I guess using the clk_notifier may be OK for the 1st case.
>>
>> In both cases, isn't the overall operation something like:
>>
>> a) Do some work before changing the EMC clock
>> b) Change the EMC clock
>> c) Do some work after changing the EMC clock
>>
> 
> There is hw interaction between the EMC controller and the CAR module.
> Writing to the EMC clock source register in CAR will trigger a state machine
> in the EMC controller to handle the change of memory clock. The details
> of the state machine and how it needs to be setup are different for
> each Tegra variant. This means we need to be sure of the exact sequence
> of CAR and EMC register accesses for this to work. In theory this could
> be done with pre and post rate change notifiers, but I'm afraid this would
> be quite fragile.

Isn't the state machine setup something that the EMC driver does through
EMC registers (or perhaps also using flow controller registers?). If all
the clock driver does for this clock is program the requested rate,
which triggers the HW state machine, then I think we can isolate
everything else into the EMC driver.

In such a case, we don't need to use pre-/post-rate change notifiers at
all; just have the EMC driver call into the clock driver at the
appropriate step in the process where the clock registers should be written.

> 
>> Admittedly, the exact definition of "some work" is different for your
>> cases (1) and (2) above, but the overall structure is the same. As such,
>> can't the EMC scaling driver do (a), then do (b) i.e. call
>> clk_set_rate(), then do (c)? Or, in your case (2), do we need to do
> 
> We would need to be very sure clk_set_rate() doesn't access any other register
> besides the EMC clock source register before returning to the EMC scaling
> driver.

Well, we can certainly make that happen. We're writing the code behind
clk_set_rate() for the EMC clock after all...

But I'm not sure why that is a requirement. If it is, we're pretty
screwed, because surely there's nothing stopping some other driver
that's running on a different CPU from coming along and changing some
completely unrelated clock, which will then touch "some other register".

[PATCH 2/4] clk: tegra: add EMC clock driver

[Peter De Schrijver]

On Thu, Dec 19, 2013 at 08:44:43PM +0100, Stephen Warren wrote:
> On 12/19/2013 03:05 AM, Peter De Schrijver wrote:
> >>> I guess using the clk_notifier may be OK for the 1st case.
> >>
> >> In both cases, isn't the overall operation something like:
> >>
> >> a) Do some work before changing the EMC clock
> >> b) Change the EMC clock
> >> c) Do some work after changing the EMC clock
> >>
> > 
> > There is hw interaction between the EMC controller and the CAR module.
> > Writing to the EMC clock source register in CAR will trigger a state machine
> > in the EMC controller to handle the change of memory clock. The details
> > of the state machine and how it needs to be setup are different for
> > each Tegra variant. This means we need to be sure of the exact sequence
> > of CAR and EMC register accesses for this to work. In theory this could
> > be done with pre and post rate change notifiers, but I'm afraid this would
> > be quite fragile.
> 
> Isn't the state machine setup something that the EMC driver does through
> EMC registers (or perhaps also using flow controller registers?). If all
> the clock driver does for this clock is program the requested rate,
> which triggers the HW state machine, then I think we can isolate
> everything else into the EMC driver.
> 
> In such a case, we don't need to use pre-/post-rate change notifiers at
> all; just have the EMC driver call into the clock driver at the
> appropriate step in the process where the clock registers should be written.
> 
> > 
> >> Admittedly, the exact definition of "some work" is different for your
> >> cases (1) and (2) above, but the overall structure is the same. As such,
> >> can't the EMC scaling driver do (a), then do (b) i.e. call
> >> clk_set_rate(), then do (c)? Or, in your case (2), do we need to do
> > 
> > We would need to be very sure clk_set_rate() doesn't access any other register
> > besides the EMC clock source register before returning to the EMC scaling
> > driver.
> 
> Well, we can certainly make that happen. We're writing the code behind
> clk_set_rate() for the EMC clock after all...
> 

CCF propagates clock changes if you do a set_rate. In this case, I think it
will work out, but in general clk_set_rate() does a lot more then just calling
the clock's set_rate method.

> But I'm not sure why that is a requirement. If it is, we're pretty
> screwed, because surely there's nothing stopping some other driver
> that's running on a different CPU from coming along and changing some
> completely unrelated clock, which will then touch "some other register".
> 

It's unlikely any other CPU will execute much code, because as soon as it does
an SDRAM access, it will be stuck until the EMC has finalized its rate change.
I think the requirement is more like 'do not touch any EMC register and possibly
also the EMC divider register before the EMC state machine has finished'. At
the moment this ensured by locks to prevent other CPUs from doing an EMC rate
change and by a 'barrier read' of a EMC register which will only complete once
the state machine has finished. On Tegra30 and later, some more work needs to
be done before the entire rate change is finished.

Cheers,

Peter.