Skip to contentSkip to conversation

memory: tegra: Introduce Tegra30 EMC driver

PATCH Superseded Dmitry Osipenko 3 participants 17 messages Apr 14, 2019 – May 16, 2019

Subsystems (5)
Device tree bindings Common clk Tegra architecture support Memory controller drivers +1 more subsystems

v2 of 10 Superseded Latest is v10 →

lore
Share & tools
lore

Dmitry Osipenko

author
#0 lore raw

v2 cover memory: tegra: Introduce Tegra30 EMC driver

Hello,

This series introduces driver for the External Memory Controller (EMC) found on Tegra30 chips, it controls the external DRAM on the board. The purpose of this driver is to program memory timing for external memory on the EMC clock rate change. The driver was tested using the ACTMON devfreq driver that performs memory frequency scaling based on memory-usage load.

Please also note that this patchset is based on this series:

https://www.spinics.net/lists/linux-tegra/msg39687.html

Changelog:

v2: - Added support for changing MC clock diver configuration based on
Memory Controller (MC) configuration which is part of the memory timing.

- Merged the "Add custom EMC clock implementation" patch into this series because the "Introduce Tegra30 EMC driver" patch directly depends on it. Please note that Tegra20 EMC driver will need to be adapted for the clock changes as well, I'll send out the Tegra20 patches after this series will be applied because of some other dependencies (devfreq) and because the temporary breakage won't be critical (driver will just error out on probe).

- EMC driver now performs MC configuration validation by checking that the number of MC / EMC timings matches and that the timings rate is the same.

- EMC driver now supports timings that want to change the MC clock configuration.

- Other minor prettifying changes of the code.

Dmitry Osipenko (4):
clk: tegra20/30: Add custom EMC clock implementation
dt-bindings: memory: Add binding for NVIDIA Tegra30 External Memory
Controller
memory: tegra: Introduce Tegra30 EMC driver
ARM: dts: tegra30: Add External Memory Controller node

 .../memory-controllers/nvidia,tegra30-emc.txt |  257 ++++
 arch/arm/boot/dts/tegra30.dtsi                |   11 +
 drivers/clk/tegra/Makefile                    |    2 +
 drivers/clk/tegra/clk-tegra20-emc.c           |  307 +++++
 drivers/clk/tegra/clk-tegra20.c               |   51 +-
 drivers/clk/tegra/clk-tegra30.c               |   35 +-
 drivers/clk/tegra/clk.h                       |    6 +
 drivers/memory/tegra/Kconfig                  |   10 +
 drivers/memory/tegra/Makefile                 |    1 +
 drivers/memory/tegra/mc.c                     |    3 -
 drivers/memory/tegra/mc.h                     |   30 +-
 drivers/memory/tegra/tegra30-emc.c            | 1159 +++++++++++++++++
 drivers/memory/tegra/tegra30.c                |   44 +
 include/linux/clk/tegra.h                     |    6 +
 14 files changed, 1860 insertions(+), 62 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/memory-controllers/nvidia,tegra30-emc.txt
 create mode 100644 drivers/clk/tegra/clk-tegra20-emc.c
 create mode 100644 drivers/memory/tegra/tegra30-emc.c
Signature · 1 line
2.21.0
author
#1 lore raw

PATCH v2 1/4 clk: tegra20/30: Add custom EMC clock implementation

A proper External Memory Controller clock rounding and parent selection functionality is required by the EMC drivers. It is not available using the generic clock implementation, hence add a custom one. The clock rate rounding shall be done by the EMC drivers because they have information about available memory timings, so the drivers will have to register a callback that will round the requested rate. EMC clock users won't be able to request EMC clock by getting -EPROBE_DEFER until EMC driver is probed and the callback is set up. The functionality is somewhat similar to the clk-emc.c which serves Tegra124+ SoC's, the later HW generations support more parent clock sources and the HW configuration and integration with the EMC drivers differs a tad from the older gens, hence it's not really worth to try to squash everything into a single source file.

Signed-off-by Dmitry Osipenko <[email protected]>
---
 drivers/clk/tegra/Makefile          |   2 +
 drivers/clk/tegra/clk-tegra20-emc.c | 307 ++++++++++++++++++++++++++++
 drivers/clk/tegra/clk-tegra20.c     |  51 ++---
 drivers/clk/tegra/clk-tegra30.c     |  35 +++-
 drivers/clk/tegra/clk.h             |   6 +
 include/linux/clk/tegra.h           |   6 +
 6 files changed, 357 insertions(+), 50 deletions(-)
 create mode 100644 drivers/clk/tegra/clk-tegra20-emc.c
drivers/clk/tegra/Makefile +2 -0
index 4812e45c2214..df966ca06788 100644
@@ -17,7 +17,9 @@ obj-y					+= clk-tegra-fixed.o
 obj-y					+= clk-tegra-super-gen4.o
 obj-$(CONFIG_TEGRA_CLK_EMC)		+= clk-emc.o
 obj-$(CONFIG_ARCH_TEGRA_2x_SOC)         += clk-tegra20.o
+obj-$(CONFIG_ARCH_TEGRA_2x_SOC)		+= clk-tegra20-emc.o
 obj-$(CONFIG_ARCH_TEGRA_3x_SOC)         += clk-tegra30.o
+obj-$(CONFIG_ARCH_TEGRA_3x_SOC)		+= clk-tegra20-emc.o
 obj-$(CONFIG_ARCH_TEGRA_114_SOC)	+= clk-tegra114.o
 obj-$(CONFIG_ARCH_TEGRA_124_SOC)	+= clk-tegra124.o
 obj-$(CONFIG_TEGRA_CLK_DFLL)		+= clk-tegra124-dfll-fcpu.o
drivers/clk/tegra/clk-tegra20-emc.c new file +307 -0

307 lines not on this page · raw message

drivers/clk/tegra/clk-tegra20.c +12 -39

81 lines not on this page · raw message

drivers/clk/tegra/clk-tegra30.c +24 -11
index fa8d573ac626..9b5ffa0a6ed8 100644
@@ -162,7 +162,6 @@ static unsigned long input_freq;
 
 static DEFINE_SPINLOCK(cml_lock);
 static DEFINE_SPINLOCK(pll_d_lock);
-static DEFINE_SPINLOCK(emc_lock);
 
 #define TEGRA_INIT_DATA_MUX(_name, _parents, _offset,	\
 			    _clk_num, _gate_flags, _clk_id)	\

@@ -819,7 +818,7 @@ static struct tegra_clk tegra30_clks[tegra_clk_max] __initdata = {
 	[tegra_clk_pll_a] = { .dt_id = TEGRA30_CLK_PLL_A, .present = true },
 	[tegra_clk_pll_a_out0] = { .dt_id = TEGRA30_CLK_PLL_A_OUT0, .present = true },
 	[tegra_clk_cec] = { .dt_id = TEGRA30_CLK_CEC, .present = true },
-	[tegra_clk_emc] = { .dt_id = TEGRA30_CLK_EMC, .present = true },
+	[tegra_clk_emc] = { .dt_id = TEGRA30_CLK_EMC, .present = false },
 };
 
 static const char *pll_e_parents[] = { "pll_ref", "pll_p" };

@@ -1006,7 +1005,6 @@ static void __init tegra30_super_clk_init(void)
 static const char *mux_pllacp_clkm[] = { "pll_a_out0", "unused", "pll_p",
 					 "clk_m" };
 static const char *mux_pllpcm_clkm[] = { "pll_p", "pll_c", "pll_m", "clk_m" };
-static const char *mux_pllmcp_clkm[] = { "pll_m", "pll_c", "pll_p", "clk_m" };
 static const char *spdif_out_parents[] = { "pll_a_out0", "spdif_2x", "pll_p",
 					   "clk_m" };
 static const char *mux_pllmcpa[] = { "pll_m", "pll_c", "pll_p", "pll_a_out0" };

@@ -1055,14 +1053,12 @@ static void __init tegra30_periph_clk_init(void)
 	clks[TEGRA30_CLK_AFI] = 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, &emc_lock);
+	clk = tegra30_clk_register_emc(clk_base + CLK_SOURCE_EMC);
+
+	clks[TEGRA30_CLK_EMC] = clk;
 
-	clk = tegra_clk_register_mc("mc", "emc_mux", clk_base + CLK_SOURCE_EMC,
-				    &emc_lock);
+	clk = tegra_clk_register_mc("mc", "emc", clk_base + CLK_SOURCE_EMC,
+				    NULL);
 	clks[TEGRA30_CLK_MC] = clk;
 
 	/* cml0 */

@@ -1313,6 +1309,23 @@ static struct tegra_audio_clk_info tegra30_audio_plls[] = {
 	{ "pll_a", &pll_a_params, tegra_clk_pll_a, "pll_p_out1" },
 };
 
+static struct clk *tegra30_clk_src_onecell_get(struct of_phandle_args *clkspec,
+					       void *data)
+{
+	struct clk *clk;
+
+	clk = of_clk_src_onecell_get(clkspec, data);
+	if (IS_ERR(clk))
+		return clk;
+
+	if (clkspec->args[0] == TEGRA30_CLK_EMC) {
+		if (!tegra30_clk_emc_driver_available())
+			return ERR_PTR(-EPROBE_DEFER);
+	}
+
+	return clk;
+}
+
 static void __init tegra30_clock_init(struct device_node *np)
 {
 	struct device_node *node;

@@ -1356,7 +1369,7 @@ static void __init tegra30_clock_init(struct device_node *np)
 
 	tegra_init_dup_clks(tegra_clk_duplicates, clks, TEGRA30_CLK_CLK_MAX);
 
-	tegra_add_of_provider(np, of_clk_src_onecell_get);
+	tegra_add_of_provider(np, tegra30_clk_src_onecell_get);
 	tegra_register_devclks(devclks, ARRAY_SIZE(devclks));
 
 	tegra_clk_apply_init_table = tegra30_clock_apply_init_table;
drivers/clk/tegra/clk.h +6 -0
index 09bccbb9640c..197cc1122e20 100644
@@ -849,4 +849,10 @@ int div_frac_get(unsigned long rate, unsigned parent_rate, u8 width,
 		udelay(delay);		\
 	} while (0)
 
+bool tegra20_clk_emc_driver_available(void);
+struct clk *tegra20_clk_register_emc(void __iomem *ioaddr);
+
+bool tegra30_clk_emc_driver_available(void);
+struct clk *tegra30_clk_register_emc(void __iomem *ioaddr);
+
 #endif /* TEGRA_CLK_H */
include/linux/clk/tegra.h +6 -0
index afb9edfa5d58..8e86951b2759 100644
@@ -130,4 +130,10 @@ extern void tegra210_put_utmipll_in_iddq(void);
 extern void tegra210_put_utmipll_out_iddq(void);
 extern int tegra210_clk_handle_mbist_war(unsigned int id);
 
+struct clk;
+
+void tegra20_clk_set_emc_round_callback(void *round_cb, void *arg_cb);
+void tegra30_clk_set_emc_round_callback(void *round_cb, void *arg_cb);
+int tegra30_clk_prepare_emc_mc_same_freq(struct clk *emc_clk, bool same);
+
 #endif /* __LINUX_CLK_TEGRA_H_ */
Signature · 1 line
2.21.0
maintainer
#2 lore raw

Re: [PATCH v2 1/4] clk: tegra20/30: Add custom EMC clock implementation

Quoting Dmitry Osipenko (2019-04-14 13:20:06)

drivers/clk/tegra/clk-tegra20-emc.c line 4

Quoted 10 lines
drivers/clk/tegra/clk-tegra20-emc.c new file +4 -0
new file mode 100644
index 000000000000..35b67a9989c8
@@ -0,0 +1,307 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include <linux/bits.h>
+#include <linux/clk/tegra.h>

Include clk-provider.h as this is a clk provider driver.

Quoted 9 lines
drivers/clk/tegra/clk-tegra20-emc.c new file +9 -0
+#include <linux/err.h>
+#include <linux/io.h>
+#include <linux/kernel.h>
+#include <linux/slab.h>
+
+#include "clk.h"
+
+#define CLK_SOURCE_EMC_2X_CLK_DIVISOR_MASK     GENMASK(7, 0)
+#define CLK_SOURCE_EMC_2X_CLK_SRC_MASK         GENMASK(31, 30)

[...]

drivers/clk/tegra/clk-tegra20-emc.c line 207

Quoted 11 lines
drivers/clk/tegra/clk-tegra20-emc.c new file +11 -0
+
+static const struct clk_ops tegra_clk_emc_ops = {
+       .recalc_rate = emc_recalc_rate,
+       .get_parent = emc_get_parent,
+       .set_parent = emc_set_parent,
+       .set_rate = emc_set_rate,
+       .set_rate_and_parent = emc_set_rate_and_parent,
+       .determine_rate = emc_determine_rate,
+};
+
+void tegra20_clk_set_emc_round_callback(void *round_cb, void *arg_cb)

Why can't we have type safety on these function pointers?

Quoted 7 lines
drivers/clk/tegra/clk-tegra20-emc.c new file +7 -0
+{
+       struct clk *clk = __clk_lookup("emc");
+       struct tegra_clk_emc *emc;
+       struct clk_hw *hw;
+
+       if (clk) {
+               hw = __clk_get_hw(clk);

drivers/clk/tegra/clk-tegra20-emc.c line 224

Quoted 10 lines
drivers/clk/tegra/clk-tegra20-emc.c new file +10 -0
+               emc = to_tegra_clk_emc(hw);
+
+               emc->round_cb = round_cb;
+               emc->arg_cb = arg_cb;
+       }
+}
+
+bool tegra20_clk_emc_driver_available(void)
+{
+       struct clk *clk = __clk_lookup("emc");

Can we avoid using __clk_lookup()? Maybe by having the clk_hw pointer in this driver somehow?

drivers/clk/tegra/clk-tegra20-emc.c line 214

Quoted 5 lines
drivers/clk/tegra/clk-tegra20-emc.c new file +5 -0
+       struct tegra_clk_emc *emc;
+       struct clk_hw *hw;
+
+       if (clk) {
+               hw = __clk_get_hw(clk);

This gets further to the point, we don't prefer to see drivers use __clk_get_hw() unless they absolutely need to. Maybe I don't understand the driver structure, but it seems like maybe the driver that's providing the callbacks could be the same driver that's registering these clks, and thus everything could be inside one file so that we don't have to pass around callbacks and clk_hw pointers? Commit text says "this is how it's been" but that's not a reason to keep doing it.

Quoted 2 lines
drivers/clk/tegra/clk-tegra20-emc.c new file +2 -0
+               emc = to_tegra_clk_emc(hw);
+
author
#3 lore raw

Re: [PATCH v2 1/4] clk: tegra20/30: Add custom EMC clock implementation

25.04.2019 22:07, Stephen Boyd пишет:

Quoted 13 lines

Quoting Dmitry Osipenko (2019-04-14 13:20:06)

drivers/clk/tegra/clk-tegra20-emc.c line 4

Quoted 10 lines
drivers/clk/tegra/clk-tegra20-emc.c new file +4 -0
new file mode 100644
index 000000000000..35b67a9989c8
@@ -0,0 +1,307 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include <linux/bits.h>
+#include <linux/clk/tegra.h>

Include clk-provider.h as this is a clk provider driver.

Okay, although clk-provider.h is also included by clk.h below.

Quoted 10 lines
Quoted 9 lines
drivers/clk/tegra/clk-tegra20-emc.c new file +9 -0
+#include <linux/err.h>
+#include <linux/io.h>
+#include <linux/kernel.h>
+#include <linux/slab.h>
+
+#include "clk.h"
+
+#define CLK_SOURCE_EMC_2X_CLK_DIVISOR_MASK     GENMASK(7, 0)
+#define CLK_SOURCE_EMC_2X_CLK_SRC_MASK         GENMASK(31, 30)

[...]

Quoted 13 lines

drivers/clk/tegra/clk-tegra20-emc.c line 207

Quoted 11 lines
drivers/clk/tegra/clk-tegra20-emc.c new file +11 -0
+
+static const struct clk_ops tegra_clk_emc_ops = {
+       .recalc_rate = emc_recalc_rate,
+       .get_parent = emc_get_parent,
+       .set_parent = emc_set_parent,
+       .set_rate = emc_set_rate,
+       .set_rate_and_parent = emc_set_rate_and_parent,
+       .determine_rate = emc_determine_rate,
+};
+
+void tegra20_clk_set_emc_round_callback(void *round_cb, void *arg_cb)

Why can't we have type safety on these function pointers?

It is probably not really necessary since it's a platform-specific API. But I'll add an explicit type in v3 for consistency, thanks.

Quoted 12 lines
Quoted 12 lines
drivers/clk/tegra/clk-tegra20-emc.c new file +12 -0
+{
+       struct clk *clk = __clk_lookup("emc");
+       struct tegra_clk_emc *emc;
+       struct clk_hw *hw;
+
+       if (clk) {
+               hw = __clk_get_hw(clk);
+               emc = to_tegra_clk_emc(hw);
+
+               emc->round_cb = round_cb;
+               emc->arg_cb = arg_cb;
+       }
Quoted 8 lines

drivers/clk/tegra/clk-tegra20-emc.c line 224

Quoted 5 lines
drivers/clk/tegra/clk-tegra20-emc.c new file +5 -0
+}
+
+bool tegra20_clk_emc_driver_available(void)
+{
+       struct clk *clk = __clk_lookup("emc");

Can we avoid using __clk_lookup()? Maybe by having the clk_hw pointer in this driver somehow?

tegra20_clk_emc_driver_available() is a private function of the Tegra's clk-core. Please note that prototype of this function is added to the local "drivers/clk/tegra/clk.h" file.

It is indeed possible to use clk pointer instead of __clk_lookup() and I'll change that in v3 since it's causing some confusion.

Quoted 6 lines

drivers/clk/tegra/clk-tegra20-emc.c line 214

Quoted 5 lines
drivers/clk/tegra/clk-tegra20-emc.c new file +5 -0
+       struct tegra_clk_emc *emc;
+       struct clk_hw *hw;
+
+       if (clk) {
+               hw = __clk_get_hw(clk);
Quoted 7 lines

This gets further to the point, we don't prefer to see drivers use __clk_get_hw() unless they absolutely need to. Maybe I don't understand the driver structure, but it seems like maybe the driver that's providing the callbacks could be the same driver that's registering these clks, and thus everything could be inside one file so that we don't have to pass around callbacks and clk_hw pointers? Commit text says "this is how it's been" but that's not a reason to keep doing it.

Again, tegra20_clk_emc_driver_available() is for the Terga's clk-core and not for the EMC (External Memory Controller) driver. This function is used to determine whether EMC driver is ready to handle clock-rate changes (PRE/POST rate-change notifications), clk users can't get EMC clock until driver is ready (i.e. the "round" callback is registered by the driver).

Please see tegra20_clk_src_onecell_get() changes in this patch and drivers/memory/tegra/tegra20-emc.c for clarity.

It is not possible to register clk from the EMC driver without having some custom integration with the clk-core because CLK and EMC are different hardware units and hence EMC driver doesn't have direct access to the CLK registers. I think it is better to keep CLK and memory-timing programming logically separated simply for consistency, although I'm open to suggestions.

maintainer
#4 lore raw

Re: [PATCH v2 1/4] clk: tegra20/30: Add custom EMC clock implementation

Quoting Dmitry Osipenko (2019-04-25 14:42:19)

Quoted 15 lines

25.04.2019 22:07, Stephen Boyd пишет:

Quoted 13 lines

Quoting Dmitry Osipenko (2019-04-14 13:20:06)

drivers/clk/tegra/clk-tegra20-emc.c line 4

Quoted 10 lines
drivers/clk/tegra/clk-tegra20-emc.c new file +4 -0
new file mode 100644
index 000000000000..35b67a9989c8
@@ -0,0 +1,307 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include <linux/bits.h>
+#include <linux/clk/tegra.h>

Include clk-provider.h as this is a clk provider driver.

Quoted 1 line

Okay, although clk-provider.h is also included by clk.h below.

We prefer explicit includes instead of implicit ones. Failing to do that leads to changes like the one I'm making now to push out linux/io.h to all the users who are relying on clk-provider.h to provide it for them.

Quoted 22 lines
Quoted 13 lines
Quoted 13 lines
drivers/clk/tegra/clk-tegra20-emc.c new file +12 -0
+{
+       struct clk *clk = __clk_lookup("emc");
+       struct tegra_clk_emc *emc;
+       struct clk_hw *hw;
+
+       if (clk) {
+               hw = __clk_get_hw(clk);
+               emc = to_tegra_clk_emc(hw);
+
+               emc->round_cb = round_cb;
+               emc->arg_cb = arg_cb;
+       }
Quoted 8 lines

drivers/clk/tegra/clk-tegra20-emc.c line 224

Quoted 5 lines
drivers/clk/tegra/clk-tegra20-emc.c new file +5 -0
+}
+
+bool tegra20_clk_emc_driver_available(void)
+{
+       struct clk *clk = __clk_lookup("emc");

Can we avoid using __clk_lookup()? Maybe by having the clk_hw pointer in this driver somehow?

Quoted 6 lines

tegra20_clk_emc_driver_available() is a private function of the Tegra's clk-core. Please note that prototype of this function is added to the local "drivers/clk/tegra/clk.h" file.

It is indeed possible to use clk pointer instead of __clk_lookup() and I'll change that in v3 since it's causing some confusion.

Can you use a clk_hw pointer directly? I'd like to see clk provider drivers only use struct clk_hw, and clk consumers only use struct clk.

Quoted 15 lines
Quoted 7 lines

drivers/clk/tegra/clk-tegra20-emc.c line 214

Quoted 6 lines
drivers/clk/tegra/clk-tegra20-emc.c new file +5 -0
+       struct tegra_clk_emc *emc;
+       struct clk_hw *hw;
+
+       if (clk) {
+               hw = __clk_get_hw(clk);
Quoted 7 lines

This gets further to the point, we don't prefer to see drivers use __clk_get_hw() unless they absolutely need to. Maybe I don't understand the driver structure, but it seems like maybe the driver that's providing the callbacks could be the same driver that's registering these clks, and thus everything could be inside one file so that we don't have to pass around callbacks and clk_hw pointers? Commit text says "this is how it's been" but that's not a reason to keep doing it.

Quoted 6 lines

Again, tegra20_clk_emc_driver_available() is for the Terga's clk-core and not for the EMC (External Memory Controller) driver. This function is used to determine whether EMC driver is ready to handle clock-rate changes (PRE/POST rate-change notifications), clk users can't get EMC clock until driver is ready (i.e. the "round" callback is registered by the driver).

Who are the other users of the EMC clk? Why does the EMC driver and the clk driver need to be in sync in such a way that requires us to check if some code in the EMC driver is ready before we can hand out clks from this clk provider? It looks like some tight coupling between these two pieces of code on the clk_get() path which is setting off alarms for me.

Quoted 10 lines

Please see tegra20_clk_src_onecell_get() changes in this patch and drivers/memory/tegra/tegra20-emc.c for clarity.

It is not possible to register clk from the EMC driver without having some custom integration with the clk-core because CLK and EMC are different hardware units and hence EMC driver doesn't have direct access to the CLK registers. I think it is better to keep CLK and memory-timing programming logically separated simply for consistency, although I'm open to suggestions.

Sorry, I'm really confused and it's probably because I'm not taking the time to read all the Tegra code right now. I'm trying to understand the overall memory controller frequency logic.

I understand that there's a memory controller, and a clk controller, and they're different hardware blocks. Presumably the clk controller can't change frequencies without informing the memory controller that it's going to change something so the memory controller can fix timings in the EMC register space.

Is there some other entity that's needing to be deferred while the EMC and clk drivers tell each other that they're probed and ready so that these notifiers all work? Is that entity using clks directly instead of going through the EMC to change frequencies? If so, why can't we funnel that logic through EMC which then goes to the clk driver? That way it's clearly "thing that changes frequency" -> EMC -> clk controller call chain. It might even allow us to get rid of the rate notifiers?

author
#5 lore raw

Re: [PATCH v2 1/4] clk: tegra20/30: Add custom EMC clock implementation

26.04.2019 1:25, Stephen Boyd пишет:

Quoted 16 lines

Quoting Dmitry Osipenko (2019-04-25 14:42:19)

Quoted 15 lines

25.04.2019 22:07, Stephen Boyd пишет:

Quoted 13 lines

Quoting Dmitry Osipenko (2019-04-14 13:20:06)

drivers/clk/tegra/clk-tegra20-emc.c line 4

Quoted 10 lines
drivers/clk/tegra/clk-tegra20-emc.c new file +4 -0
new file mode 100644
index 000000000000..35b67a9989c8
@@ -0,0 +1,307 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include <linux/bits.h>
+#include <linux/clk/tegra.h>

Include clk-provider.h as this is a clk provider driver.

Quoted 5 lines
Quoted 1 line

Okay, although clk-provider.h is also included by clk.h below.

We prefer explicit includes instead of implicit ones. Failing to do that leads to changes like the one I'm making now to push out linux/io.h to all the users who are relying on clk-provider.h to provide it for them.

Ok.

Quoted 26 lines
Quoted 26 lines
Quoted 13 lines
Quoted 13 lines
drivers/clk/tegra/clk-tegra20-emc.c new file +12 -0
+{
+       struct clk *clk = __clk_lookup("emc");
+       struct tegra_clk_emc *emc;
+       struct clk_hw *hw;
+
+       if (clk) {
+               hw = __clk_get_hw(clk);
+               emc = to_tegra_clk_emc(hw);
+
+               emc->round_cb = round_cb;
+               emc->arg_cb = arg_cb;
+       }
Quoted 8 lines

drivers/clk/tegra/clk-tegra20-emc.c line 224

Quoted 5 lines
drivers/clk/tegra/clk-tegra20-emc.c new file +5 -0
+}
+
+bool tegra20_clk_emc_driver_available(void)
+{
+       struct clk *clk = __clk_lookup("emc");

Can we avoid using __clk_lookup()? Maybe by having the clk_hw pointer in this driver somehow?

tegra20_clk_emc_driver_available() is a private function of the Tegra's clk-core. Please note that prototype of this function is added to the local "drivers/clk/tegra/clk.h" file.

Quoted 5 lines
Quoted 2 lines

It is indeed possible to use clk pointer instead of __clk_lookup() and I'll change that in v3 since it's causing some confusion.

Can you use a clk_hw pointer directly? I'd like to see clk provider drivers only use struct clk_hw, and clk consumers only use struct clk.

The clk_hw pointers are not stored directly anywhere in the Tegra's clk-driver, it will be a quite massive change to make the clk driver's code to use clk_hw pointers instead of struct clk.

I can add a global variable for the tegra_clk_emc pointer, but it is not really necessary since there is __clk_get_hw.

Could you please explain what's the problem with __clk_get_hw?

Quoted 22 lines
Quoted 15 lines
Quoted 7 lines

drivers/clk/tegra/clk-tegra20-emc.c line 214

Quoted 6 lines
drivers/clk/tegra/clk-tegra20-emc.c new file +5 -0
+       struct tegra_clk_emc *emc;
+       struct clk_hw *hw;
+
+       if (clk) {
+               hw = __clk_get_hw(clk);
Quoted 7 lines

This gets further to the point, we don't prefer to see drivers use __clk_get_hw() unless they absolutely need to. Maybe I don't understand the driver structure, but it seems like maybe the driver that's providing the callbacks could be the same driver that's registering these clks, and thus everything could be inside one file so that we don't have to pass around callbacks and clk_hw pointers? Commit text says "this is how it's been" but that's not a reason to keep doing it.

Quoted 6 lines

Again, tegra20_clk_emc_driver_available() is for the Terga's clk-core and not for the EMC (External Memory Controller) driver. This function is used to determine whether EMC driver is ready to handle clock-rate changes (PRE/POST rate-change notifications), clk users can't get EMC clock until driver is ready (i.e. the "round" callback is registered by the driver).

Quoted 5 lines

Who are the other users of the EMC clk? Why does the EMC driver and the clk driver need to be in sync in such a way that requires us to check if some code in the EMC driver is ready before we can hand out clks from this clk provider? It looks like some tight coupling between these two pieces of code on the clk_get() path which is setting off alarms for me.

The other EMC clk user is the devfreq driver. We don't want the devfreq driver to get and use the EMC clock if memory timing can't be programmed.

Quoted 11 lines
Quoted 10 lines

Please see tegra20_clk_src_onecell_get() changes in this patch and drivers/memory/tegra/tegra20-emc.c for clarity.

It is not possible to register clk from the EMC driver without having some custom integration with the clk-core because CLK and EMC are different hardware units and hence EMC driver doesn't have direct access to the CLK registers. I think it is better to keep CLK and memory-timing programming logically separated simply for consistency, although I'm open to suggestions.

Quoted 3 lines

Sorry, I'm really confused and it's probably because I'm not taking the time to read all the Tegra code right now. I'm trying to understand the overall memory controller frequency logic.

No problems, sorry for the confusion.

Quoted 5 lines

I understand that there's a memory controller, and a clk controller, and they're different hardware blocks. Presumably the clk controller can't change frequencies without informing the memory controller that it's going to change something so the memory controller can fix timings in the EMC register space.

That is correct.

Quoted 8 lines

Is there some other entity that's needing to be deferred while the EMC and clk drivers tell each other that they're probed and ready so that these notifiers all work? Is that entity using clks directly instead of going through the EMC to change frequencies? If so, why can't we funnel that logic through EMC which then goes to the clk driver? That way it's clearly "thing that changes frequency" -> EMC -> clk controller call chain. It might even allow us to get rid of the rate notifiers?

That entity is the devfreq driver which uses EMC clk directly right now.

https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/devfreq/tegra-devfreq.c#n484

I think we can funnel all the rate-changing logic through the EMC drivers using the PM memory bandwidth QoS API, which is currently unsupported by the drivers. And then indeed the rate notifiers won't be needed.

	https://github.com/grate-driver/linux/commit/2aa2390dd0cb8bca98e4cea6a29d7bda64a51bb3

Actually it will be the next step to wire up the PM QoS support to all relevant drivers that I'm going to implement after the current CLK/EMC/DEVFREQ patch sets will land, it's just not realistic to do everything in a single hop.

maintainer
#6 lore raw

Re: [PATCH v2 1/4] clk: tegra20/30: Add custom EMC clock implementation

Quoting Dmitry Osipenko (2019-04-25 17:03:11)

Quoted 32 lines

26.04.2019 1:25, Stephen Boyd пишет:

Quoted 31 lines

Quoting Dmitry Osipenko (2019-04-25 14:42:19)

Quoted 29 lines

25.04.2019 22:07, Stephen Boyd пишет:

Quoted 13 lines

Quoting Dmitry Osipenko (2019-04-14 13:20:06)

Quoted 12 lines
drivers/clk/tegra/clk-tegra20-emc.c new file +12 -0
+{
+       struct clk *clk = __clk_lookup("emc");
+       struct tegra_clk_emc *emc;
+       struct clk_hw *hw;
+
+       if (clk) {
+               hw = __clk_get_hw(clk);
+               emc = to_tegra_clk_emc(hw);
+
+               emc->round_cb = round_cb;
+               emc->arg_cb = arg_cb;
+       }
Quoted 8 lines

drivers/clk/tegra/clk-tegra20-emc.c line 224

Quoted 5 lines
drivers/clk/tegra/clk-tegra20-emc.c new file +5 -0
+}
+
+bool tegra20_clk_emc_driver_available(void)
+{
+       struct clk *clk = __clk_lookup("emc");

Can we avoid using __clk_lookup()? Maybe by having the clk_hw pointer in this driver somehow?

tegra20_clk_emc_driver_available() is a private function of the Tegra's clk-core. Please note that prototype of this function is added to the local "drivers/clk/tegra/clk.h" file.

It is indeed possible to use clk pointer instead of __clk_lookup() and I'll change that in v3 since it's causing some confusion.

Quoted 6 lines
Quoted 2 lines

Can you use a clk_hw pointer directly? I'd like to see clk provider drivers only use struct clk_hw, and clk consumers only use struct clk.

The clk_hw pointers are not stored directly anywhere in the Tegra's clk-driver, it will be a quite massive change to make the clk driver's code to use clk_hw pointers instead of struct clk.

Well they must be stored in the clk driver somewhere because that's how the whole clk framework works.

Quoted 5 lines

I can add a global variable for the tegra_clk_emc pointer, but it is not really necessary since there is __clk_get_hw.

Could you please explain what's the problem with __clk_get_hw?

If it's a large invasive change then no worries. The thin that's wrong with __clk_get_hw() is that it's using a struct clk, meaing that most likely this is a clk consumer driver doing something weird.

Quoted 28 lines
Quoted 21 lines
Quoted 14 lines
Quoted 5 lines
Quoted 4 lines

+ struct tegra_clk_emc *emc; + struct clk_hw *hw; + +
if (clk) { + hw = __clk_get_hw(clk);

Quoted 8 lines

This gets further to the point, we don't prefer to see drivers use __clk_get_hw() unless they absolutely need to. Maybe I don't understand the driver structure, but it seems like maybe the driver that's providing the callbacks could be the same driver that's registering these clks, and thus everything could be inside one file so that we don't have to pass around callbacks and clk_hw pointers? Commit text says "this is how it's been" but that's not a reason to keep doing it.

Quoted 6 lines

Again, tegra20_clk_emc_driver_available() is for the Terga's clk-core and not for the EMC (External Memory Controller) driver. This function is used to determine whether EMC driver is ready to handle clock-rate changes (PRE/POST rate-change notifications), clk users can't get EMC clock until driver is ready (i.e. the "round" callback is registered by the driver).

Quoted 6 lines

Who are the other users of the EMC clk? Why does the EMC driver and the clk driver need to be in sync in such a way that requires us to check if some code in the EMC driver is ready before we can hand out clks from this clk provider? It looks like some tight coupling between these two pieces of code on the clk_get() path which is setting off alarms for me.

Quoted 3 lines

The other EMC clk user is the devfreq driver. We don't want the devfreq driver to get and use the EMC clock if memory timing can't be programmed.

Alright, got it!

Quoted 15 lines

That is correct.

Quoted 9 lines

Is there some other entity that's needing to be deferred while the EMC and clk drivers tell each other that they're probed and ready so that these notifiers all work? Is that entity using clks directly instead of going through the EMC to change frequencies? If so, why can't we funnel that logic through EMC which then goes to the clk driver? That way it's clearly "thing that changes frequency" -> EMC -> clk controller call chain. It might even allow us to get rid of the rate notifiers?

That entity is the devfreq driver which uses EMC clk directly right now.

Quoted 6 lines

https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/devfreq/tegra-devfreq.c#n484

I think we can funnel all the rate-changing logic through the EMC drivers using the PM memory bandwidth QoS API, which is currently unsupported by the drivers. And then indeed the rate notifiers won't be needed.

Why does PM QoS matter here? I'm mostly asking for the chain to be stacked and the devfreq driver to be the same as the EMC driver and then have the EMC/devfreq combo driver use clk APIs to set rates and do things before and after.

Quoted 7 lines
        https://github.com/grate-driver/linux/commit/2aa2390dd0cb8bca98e4cea6a29d7bda64a51bb3

Actually it will be the next step to wire up the PM QoS support to all relevant drivers that I'm going to implement after the current CLK/EMC/DEVFREQ patch sets will land, it's just not realistic to do everything in a single hop.

Ok. It doesn't make me feel great but I suppose if you don't disappear after this code is merged things will work out in the end.

author
#7 lore raw

Re: [PATCH v2 1/4] clk: tegra20/30: Add custom EMC clock implementation

26.04.2019 3:41, Stephen Boyd пишет:

Quoted 36 lines

Quoting Dmitry Osipenko (2019-04-25 17:03:11)

Quoted 35 lines

26.04.2019 1:25, Stephen Boyd пишет:

Quoted 33 lines

Quoting Dmitry Osipenko (2019-04-25 14:42:19)

Quoted 23 lines

25.04.2019 22:07, Stephen Boyd пишет:

Quoted 13 lines

Quoting Dmitry Osipenko (2019-04-14 13:20:06)

Quoted 12 lines
drivers/clk/tegra/clk-tegra20-emc.c new file +12 -0
+{
+       struct clk *clk = __clk_lookup("emc");
+       struct tegra_clk_emc *emc;
+       struct clk_hw *hw;
+
+       if (clk) {
+               hw = __clk_get_hw(clk);
+               emc = to_tegra_clk_emc(hw);
+
+               emc->round_cb = round_cb;
+               emc->arg_cb = arg_cb;
+       }
Quoted 8 lines

drivers/clk/tegra/clk-tegra20-emc.c line 224

Quoted 5 lines
drivers/clk/tegra/clk-tegra20-emc.c new file +5 -0
+}
+
+bool tegra20_clk_emc_driver_available(void)
+{
+       struct clk *clk = __clk_lookup("emc");

Can we avoid using __clk_lookup()? Maybe by having the clk_hw pointer in this driver somehow?

Quoted 6 lines

tegra20_clk_emc_driver_available() is a private function of the Tegra's clk-core. Please note that prototype of this function is added to the local "drivers/clk/tegra/clk.h" file.

It is indeed possible to use clk pointer instead of __clk_lookup() and I'll change that in v3 since it's causing some confusion.

Can you use a clk_hw pointer directly? I'd like to see clk provider drivers only use struct clk_hw, and clk consumers only use struct clk.

Quoted 6 lines
Quoted 3 lines

The clk_hw pointers are not stored directly anywhere in the Tegra's clk-driver, it will be a quite massive change to make the clk driver's code to use clk_hw pointers instead of struct clk.

Well they must be stored in the clk driver somewhere because that's how the whole clk framework works.

AFAIK, Tegra stores them only in a form of clk->core->hw.

Quoted 66 lines
Quoted 5 lines

I can add a global variable for the tegra_clk_emc pointer, but it is not really necessary since there is __clk_get_hw.

Could you please explain what's the problem with __clk_get_hw?

If it's a large invasive change then no worries. The thin that's wrong with __clk_get_hw() is that it's using a struct clk, meaing that most likely this is a clk consumer driver doing something weird.

Quoted 28 lines
Quoted 21 lines
Quoted 14 lines
Quoted 5 lines
Quoted 4 lines

+ struct tegra_clk_emc *emc; + struct clk_hw *hw; + +
if (clk) { + hw = __clk_get_hw(clk);

Quoted 8 lines

This gets further to the point, we don't prefer to see drivers use __clk_get_hw() unless they absolutely need to. Maybe I don't understand the driver structure, but it seems like maybe the driver that's providing the callbacks could be the same driver that's registering these clks, and thus everything could be inside one file so that we don't have to pass around callbacks and clk_hw pointers? Commit text says "this is how it's been" but that's not a reason to keep doing it.

Quoted 6 lines

Again, tegra20_clk_emc_driver_available() is for the Terga's clk-core and not for the EMC (External Memory Controller) driver. This function is used to determine whether EMC driver is ready to handle clock-rate changes (PRE/POST rate-change notifications), clk users can't get EMC clock until driver is ready (i.e. the "round" callback is registered by the driver).

Quoted 6 lines

Who are the other users of the EMC clk? Why does the EMC driver and the clk driver need to be in sync in such a way that requires us to check if some code in the EMC driver is ready before we can hand out clks from this clk provider? It looks like some tight coupling between these two pieces of code on the clk_get() path which is setting off alarms for me.

Quoted 3 lines

The other EMC clk user is the devfreq driver. We don't want the devfreq driver to get and use the EMC clock if memory timing can't be programmed.

Alright, got it!

Quoted 15 lines

That is correct.

Quoted 9 lines

Is there some other entity that's needing to be deferred while the EMC and clk drivers tell each other that they're probed and ready so that these notifiers all work? Is that entity using clks directly instead of going through the EMC to change frequencies? If so, why can't we funnel that logic through EMC which then goes to the clk driver? That way it's clearly "thing that changes frequency" -> EMC -> clk controller call chain. It might even allow us to get rid of the rate notifiers?

That entity is the devfreq driver which uses EMC clk directly right now.

Quoted 6 lines

https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/devfreq/tegra-devfreq.c#n484

I think we can funnel all the rate-changing logic through the EMC drivers using the PM memory bandwidth QoS API, which is currently unsupported by the drivers. And then indeed the rate notifiers won't be needed.

Quoted 4 lines

Why does PM QoS matter here? I'm mostly asking for the chain to be stacked and the devfreq driver to be the same as the EMC driver and then have the EMC/devfreq combo driver use clk APIs to set rates and do things before and after.

It could be implemented either way, the result will be the same. The final decision probably will be made after wiring up the PM QoS support and doing some more testing. Also note that EMC and devfreq are actually separate hardware blocks that are integrated.

So it all could look like this (current variant that in works):

 +-------+   PM QoS   +-+
 |DEVICE1+----------->+E|    CLK API
 +-------+   PM QoS   |M+<----------+   +-+
             +------->+C|           +--->C|
             |        +-+               |L|
 +-------+   |                       +-->K|
 |DEVICE2+---+    +-------+  CLK API |  +-+
 +-------+        |DEVFREQ+<---------+
                  +-------+

And like this:

+-------+ PM QoS  +-+
|DEVICE1+---------+ |
+-------+         | |       +-+
+-------+ PM QoS  |E|  CLK  |C|
|DEVICE2+-------->+M+<----->+L|
+-------+         |C|  API  |K|
+-------+ PM QoS  | |       +++
|DEVFREQ+---------+ |        |
+---+---+         +-+        |
    ^                        |
    +------------------------+

CLK API

Or like this:

+-------+ PM QoS  +-+
|DEVICE1+---------+ |
+-------+         | |       +-+
+-------+ PM QoS  |E|  CLK  |C|
|DEVICE2+-------->+M+<----->+L|
+-------+         |C|  API  |K|
+-------+ PM QoS  | |       +-+
|DEVFREQ+---------+ |
+---+---+         +++
    ^              |
    +--------------+

PM QoS

As you can see there will be other drivers that will demand memory bandwidth too. For example the display controller performs isochronous memory transfers and hence may require higher bandwidth than devfreq suggests because devfreq judges the bandwidth demand by taking average of bandwidth consumption over a period of time.

The other thing is that drivers should be abstracted from the knowledge about the EMC because:

a) memory/device hardware units do not know about each other

b) it is common to express memory bandwidth in Mbs and not in a clock rate

It's only the devfreq driver that is a bit special because devfreq hardware is actually integrated with the EMC.

Quoted 11 lines
Quoted 7 lines
        https://github.com/grate-driver/linux/commit/2aa2390dd0cb8bca98e4cea6a29d7bda64a51bb3

Actually it will be the next step to wire up the PM QoS support to all relevant drivers that I'm going to implement after the current CLK/EMC/DEVFREQ patch sets will land, it's just not realistic to do everything in a single hop.

Ok. It doesn't make me feel great but I suppose if you don't disappear after this code is merged things will work out in the end.

Correct.

author
#8 lore raw

PATCH v2 2/4 dt-bindings: memory: Add binding for NVIDIA Tegra30 External Memory Controller

In reply to Dmitry Osipenko

Add device-tree binding for NVIDIA Tegra30 External Memory Controller. The binding is based on the Tegra124 EMC binding since hardware is similar, although there are couple significant differences.

Signed-off-by Dmitry Osipenko <[email protected]>
---
 .../memory-controllers/nvidia,tegra30-emc.txt | 257 ++++++++++++++++++
 1 file changed, 257 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/memory-controllers/nvidia,tegra30-emc.txt
Documentation/devicetree/bindings/memory-controllers/nvidia,tegra30-emc.txt new file +257 -0
new file mode 100644
index 000000000000..dffe396c5d79
@@ -0,0 +1,257 @@
+NVIDIA Tegra30 SoC EMC (external memory controller)
+====================================================
+
+Required properties :
+- compatible : Should be "nvidia,tegra30-emc".
+- reg : physical base address and length of the controller's registers.
+- #address-cells : Should be 1
+- #size-cells : Should be 0
+- interrupts : Should contain EMC General interrupt.
+- clocks : Should contain EMC clock.
+- nvidia,memory-controller : phandle of the MC driver.
+
+The node should contain a "emc-timings" subnode for each supported RAM type
+(see field RAM_CODE in register PMC_STRAPPING_OPT_A), with its unit address
+being its RAM_CODE.
+
+Required properties for "emc-timings" nodes :
+- nvidia,ram-code : Should contain the value of RAM_CODE this timing set is
+used for.
+
+Each "emc-timings" node should contain a "timing" subnode for every supported
+EMC clock rate. The "timing" subnodes should have the clock rate in Hz as
+their unit address.
+
+Required properties for "timing" nodes :
+- clock-frequency : Should contain the memory clock rate in Hz.
+- The following properties contain EMC timing characterization values
+(specified in the board documentation) :
+  - nvidia,emc-auto-cal-interval : EMC_AUTO_CAL_INTERVAL
+  - nvidia,emc-mode-1 : Mode Register 1
+  - nvidia,emc-mode-2 : Mode Register 2
+  - nvidia,emc-mode-reset : Mode Register 0
+  - nvidia,emc-zcal-cnt-long : EMC_ZCAL_WAIT_CNT after clock change
+  - nvidia,emc-cfg-dyn-self-ref : dynamic self-refresh enabled
+  - nvidia,emc-cfg-periodic-qrst : FBIO "read" FIFO periodic resetting enabled
+- nvidia,emc-configuration : EMC timing characterization data. These are the
+registers (see section "18.13.2 EMC Registers" in the TRM) whose values need to
+be specified, according to the board documentation:
+
+	EMC_RC
+	EMC_RFC
+	EMC_RAS
+	EMC_RP
+	EMC_R2W
+	EMC_W2R
+	EMC_R2P
+	EMC_W2P
+	EMC_RD_RCD
+	EMC_WR_RCD
+	EMC_RRD
+	EMC_REXT
+	EMC_WEXT
+	EMC_WDV
+	EMC_QUSE
+	EMC_QRST
+	EMC_QSAFE
+	EMC_RDV
+	EMC_REFRESH
+	EMC_BURST_REFRESH_NUM
+	EMC_PRE_REFRESH_REQ_CNT
+	EMC_PDEX2WR
+	EMC_PDEX2RD
+	EMC_PCHG2PDEN
+	EMC_ACT2PDEN
+	EMC_AR2PDEN
+	EMC_RW2PDEN
+	EMC_TXSR
+	EMC_TXSRDLL
+	EMC_TCKE
+	EMC_TFAW
+	EMC_TRPAB
+	EMC_TCLKSTABLE
+	EMC_TCLKSTOP
+	EMC_TREFBW
+	EMC_QUSE_EXTRA
+	EMC_FBIO_CFG6
+	EMC_ODT_WRITE
+	EMC_ODT_READ
+	EMC_FBIO_CFG5
+	EMC_CFG_DIG_DLL
+	EMC_CFG_DIG_DLL_PERIOD
+	EMC_DLL_XFORM_DQS0
+	EMC_DLL_XFORM_DQS1
+	EMC_DLL_XFORM_DQS2
+	EMC_DLL_XFORM_DQS3
+	EMC_DLL_XFORM_DQS4
+	EMC_DLL_XFORM_DQS5
+	EMC_DLL_XFORM_DQS6
+	EMC_DLL_XFORM_DQS7
+	EMC_DLL_XFORM_QUSE0
+	EMC_DLL_XFORM_QUSE1
+	EMC_DLL_XFORM_QUSE2
+	EMC_DLL_XFORM_QUSE3
+	EMC_DLL_XFORM_QUSE4
+	EMC_DLL_XFORM_QUSE5
+	EMC_DLL_XFORM_QUSE6
+	EMC_DLL_XFORM_QUSE7
+	EMC_DLI_TRIM_TXDQS0
+	EMC_DLI_TRIM_TXDQS1
+	EMC_DLI_TRIM_TXDQS2
+	EMC_DLI_TRIM_TXDQS3
+	EMC_DLI_TRIM_TXDQS4
+	EMC_DLI_TRIM_TXDQS5
+	EMC_DLI_TRIM_TXDQS6
+	EMC_DLI_TRIM_TXDQS7
+	EMC_DLL_XFORM_DQ0
+	EMC_DLL_XFORM_DQ1
+	EMC_DLL_XFORM_DQ2
+	EMC_DLL_XFORM_DQ3
+	EMC_XM2CMDPADCTRL
+	EMC_XM2DQSPADCTRL2
+	EMC_XM2DQPADCTRL2
+	EMC_XM2CLKPADCTRL
+	EMC_XM2COMPPADCTRL
+	EMC_XM2VTTGENPADCTRL
+	EMC_XM2VTTGENPADCTRL2
+	EMC_XM2QUSEPADCTRL
+	EMC_XM2DQSPADCTRL3
+	EMC_CTT_TERM_CTRL
+	EMC_ZCAL_INTERVAL
+	EMC_ZCAL_WAIT_CNT
+	EMC_MRS_WAIT_CNT
+	EMC_AUTO_CAL_CONFIG
+	EMC_CTT
+	EMC_CTT_DURATION
+	EMC_DYN_SELF_REF_CONTROL
+	EMC_FBIO_SPARE
+	EMC_CFG_RSV
+
+Example SoC include file:
+
+/ {
+	emc@7000f400 {
+		compatible = "nvidia,tegra30-emc";
+		reg = <0x7000f400 0x400>;
+		interrupts = <GIC_SPI 78 IRQ_TYPE_LEVEL_HIGH>;
+		clocks = <&tegra_car TEGRA30_CLK_EMC>;
+		#address-cells = <1>;
+		#size-cells = <0>;
+
+		nvidia,memory-controller = <&mc>;
+	};
+};
+
+Example board file:
+
+/ {
+	emc@7000f400 {
+		emc-timings-1 {
+			nvidia,ram-code = <1>;
+
+			timing-667000000 {
+				clock-frequency = <667000000>;
+
+				nvidia,emc-auto-cal-interval = <0x001fffff>;
+				nvidia,emc-mode-1 = <0x80100002>;
+				nvidia,emc-mode-2 = <0x80200018>;
+				nvidia,emc-mode-reset = <0x80000b71>;
+				nvidia,emc-zcal-cnt-long = <0x00000040>;
+				nvidia,emc-cfg-dyn-self-ref = <0x00000000>;
+				nvidia,emc-cfg-periodic-qrst = <0x00000001>;
+
+				nvidia,emc-configuration = <
+					0x00000020 /* EMC_RC */
+					0x0000006a /* EMC_RFC */
+					0x00000017 /* EMC_RAS */
+					0x00000007 /* EMC_RP */
+					0x00000005 /* EMC_R2W */
+					0x0000000c /* EMC_W2R */
+					0x00000003 /* EMC_R2P */
+					0x00000011 /* EMC_W2P */
+					0x00000007 /* EMC_RD_RCD */
+					0x00000007 /* EMC_WR_RCD */
+					0x00000002 /* EMC_RRD */
+					0x00000001 /* EMC_REXT */
+					0x00000000 /* EMC_WEXT */
+					0x00000007 /* EMC_WDV */
+					0x0000000a /* EMC_QUSE */
+					0x00000009 /* EMC_QRST */
+					0x0000000b /* EMC_QSAFE */
+					0x00000011 /* EMC_RDV */
+					0x00001412 /* EMC_REFRESH */
+					0x00000000 /* EMC_BURST_REFRESH_NUM */
+					0x00000504 /* EMC_PRE_REFRESH_REQ_CNT */
+					0x00000002 /* EMC_PDEX2WR */
+					0x0000000e /* EMC_PDEX2RD */
+					0x00000001 /* EMC_PCHG2PDEN */
+					0x00000000 /* EMC_ACT2PDEN */
+					0x0000000c /* EMC_AR2PDEN */
+					0x00000016 /* EMC_RW2PDEN */
+					0x00000072 /* EMC_TXSR */
+					0x00000200 /* EMC_TXSRDLL */
+					0x00000005 /* EMC_TCKE */
+					0x00000015 /* EMC_TFAW */
+					0x00000000 /* EMC_TRPAB */
+					0x00000006 /* EMC_TCLKSTABLE */
+					0x00000007 /* EMC_TCLKSTOP */
+					0x00001453 /* EMC_TREFBW */
+					0x0000000b /* EMC_QUSE_EXTRA */
+					0x00000006 /* EMC_FBIO_CFG6 */
+					0x00000000 /* EMC_ODT_WRITE */
+					0x00000000 /* EMC_ODT_READ */
+					0x00005088 /* EMC_FBIO_CFG5 */
+					0xf00b0191 /* EMC_CFG_DIG_DLL */
+					0x00008000 /* EMC_CFG_DIG_DLL_PERIOD */
+					0x00000008 /* EMC_DLL_XFORM_DQS0 */
+					0x00000008 /* EMC_DLL_XFORM_DQS1 */
+					0x00000008 /* EMC_DLL_XFORM_DQS2 */
+					0x00000008 /* EMC_DLL_XFORM_DQS3 */
+					0x0000000a /* EMC_DLL_XFORM_DQS4 */
+					0x0000000a /* EMC_DLL_XFORM_DQS5 */
+					0x0000000a /* EMC_DLL_XFORM_DQS6 */
+					0x0000000a /* EMC_DLL_XFORM_DQS7 */
+					0x00018000 /* EMC_DLL_XFORM_QUSE0 */
+					0x00018000 /* EMC_DLL_XFORM_QUSE1 */
+					0x00018000 /* EMC_DLL_XFORM_QUSE2 */
+					0x00018000 /* EMC_DLL_XFORM_QUSE3 */
+					0x00000000 /* EMC_DLL_XFORM_QUSE4 */
+					0x00000000 /* EMC_DLL_XFORM_QUSE5 */
+					0x00000000 /* EMC_DLL_XFORM_QUSE6 */
+					0x00000000 /* EMC_DLL_XFORM_QUSE7 */
+					0x00000000 /* EMC_DLI_TRIM_TXDQS0 */
+					0x00000000 /* EMC_DLI_TRIM_TXDQS1 */
+					0x00000000 /* EMC_DLI_TRIM_TXDQS2 */
+					0x00000000 /* EMC_DLI_TRIM_TXDQS3 */
+					0x00000000 /* EMC_DLI_TRIM_TXDQS4 */
+					0x00000000 /* EMC_DLI_TRIM_TXDQS5 */
+					0x00000000 /* EMC_DLI_TRIM_TXDQS6 */
+					0x00000000 /* EMC_DLI_TRIM_TXDQS7 */
+					0x0000000a /* EMC_DLL_XFORM_DQ0 */
+					0x0000000a /* EMC_DLL_XFORM_DQ1 */
+					0x0000000a /* EMC_DLL_XFORM_DQ2 */
+					0x0000000a /* EMC_DLL_XFORM_DQ3 */
+					0x000002a0 /* EMC_XM2CMDPADCTRL */
+					0x0800013d /* EMC_XM2DQSPADCTRL2 */
+					0x22220000 /* EMC_XM2DQPADCTRL2 */
+					0x77fff884 /* EMC_XM2CLKPADCTRL */
+					0x01f1f501 /* EMC_XM2COMPPADCTRL */
+					0x07077404 /* EMC_XM2VTTGENPADCTRL */
+					0x54000000 /* EMC_XM2VTTGENPADCTRL2 */
+					0x080001e8 /* EMC_XM2QUSEPADCTRL */
+					0x0c000021 /* EMC_XM2DQSPADCTRL3 */
+					0x00000802 /* EMC_CTT_TERM_CTRL */
+					0x00020000 /* EMC_ZCAL_INTERVAL */
+					0x00000100 /* EMC_ZCAL_WAIT_CNT */
+					0x0155000c /* EMC_MRS_WAIT_CNT */
+					0xa0f10000 /* EMC_AUTO_CAL_CONFIG */
+					0x00000000 /* EMC_CTT */
+					0x00000000 /* EMC_CTT_DURATION */
+					0x800028a5 /* EMC_DYN_SELF_REF_CONTROL */
+					0xe8000000 /* EMC_FBIO_SPARE */
+					0xff00ff49 /* EMC_CFG_RSV */
+				>;
+			};
+		};
+	};
+};
Signature · 1 line
2.21.0
maintainer
#9 lore raw

Re: [PATCH v2 2/4] dt-bindings: memory: Add binding for NVIDIA Tegra30 External Memory Controller

Quoted 3 lines · Dmitry Osipenko

Add device-tree binding for NVIDIA Tegra30 External Memory Controller. The binding is based on the Tegra124 EMC binding since hardware is similar, although there are couple significant differences.

My comments on Tegra124 binding apply here.

Quoted 6 lines
Signed-off-by Dmitry Osipenko <[email protected]>
---
 .../memory-controllers/nvidia,tegra30-emc.txt | 257 ++++++++++++++++++
 1 file changed, 257 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/memory-controllers/nvidia,tegra30-emc.txt
author
#10 lore raw

Re: [PATCH v2 2/4] dt-bindings: memory: Add binding for NVIDIA Tegra30 External Memory Controller

30.04.2019 1:05, Rob Herring пишет:

Quoted 6 lines · Dmitry Osipenko
Quoted 3 lines

Add device-tree binding for NVIDIA Tegra30 External Memory Controller. The binding is based on the Tegra124 EMC binding since hardware is similar, although there are couple significant differences.

My comments on Tegra124 binding apply here.

The common timing definition doesn't fully match the definition that is used by Tegra's Memory Controller, thus the DQS (data strobe) timing parameter is comprised of multiple sub-parameters that describe how to generate the strobe in hardware. There are also more additional parameters that are specific to Tegra and they are individually characterized for each memory model and clock rate. Hence the common timing definition isn't usable.

maintainer
#11 lore raw

Re: [PATCH v2 2/4] dt-bindings: memory: Add binding for NVIDIA Tegra30 External Memory Controller

Quoted 9 lines · Dmitry Osipenko

30.04.2019 1:05, Rob Herring пишет:

Quoted 6 lines · Dmitry Osipenko
Quoted 3 lines

Add device-tree binding for NVIDIA Tegra30 External Memory Controller. The binding is based on the Tegra124 EMC binding since hardware is similar, although there are couple significant differences.

My comments on Tegra124 binding apply here.

Quoted 7 lines

The common timing definition doesn't fully match the definition that is used by Tegra's Memory Controller, thus the DQS (data strobe) timing parameter is comprised of multiple sub-parameters that describe how to generate the strobe in hardware. There are also more additional parameters that are specific to Tegra and they are individually characterized for each memory model and clock rate. Hence the common timing definition isn't usable.

I don't understand. Every PC in the world can work with any DIMM (within a given generation) just with SPD data. Why is that not sufficient here?

In any case, it seems for Tegra124 a different approach is going to be taken. Seems like an "avoid DT" solution to me, but if it's contained within the firmware it's not my problem.

Rob

author
#12 lore raw

Re: [PATCH v2 2/4] dt-bindings: memory: Add binding for NVIDIA Tegra30 External Memory Controller

02.05.2019 3:17, Rob Herring пишет:

Quoted 18 lines · Dmitry Osipenko
Quoted 9 lines

30.04.2019 1:05, Rob Herring пишет:

Quoted 6 lines · Dmitry Osipenko
Quoted 3 lines

Add device-tree binding for NVIDIA Tegra30 External Memory Controller. The binding is based on the Tegra124 EMC binding since hardware is similar, although there are couple significant differences.

My comments on Tegra124 binding apply here.

Quoted 7 lines

The common timing definition doesn't fully match the definition that is used by Tegra's Memory Controller, thus the DQS (data strobe) timing parameter is comprised of multiple sub-parameters that describe how to generate the strobe in hardware. There are also more additional parameters that are specific to Tegra and they are individually characterized for each memory model and clock rate. Hence the common timing definition isn't usable.

Quoted 3 lines

I don't understand. Every PC in the world can work with any DIMM (within a given generation) just with SPD data. Why is that not sufficient here?

Because this is not a standard PC, but a custom embedded hardware that is simpler and also doesn't fully follow the standards in some cases.

Quoted 3 lines

In any case, it seems for Tegra124 a different approach is going to be taken. Seems like an "avoid DT" solution to me, but if it's contained within the firmware it's not my problem.

My above comment really applies to all Terga's.

The Tegra210 is also a bit more complicated case because of the proprietary signed firmware that can't be easily replaced with opensource alternative without special hacks, but AFAIK the unofficial opensource firmware will be available in some form for at least one consumer device (Nintendo Switch).

Please write a detailed comment to the Tegra210's patch, saying what you would want to see changed. I'm sure Joseph will try to do his best.

Note that it is always possible to define a proper device tree binding and then also "unofficially" support the downstream binding, IIRC that's what some drivers are already doing in upstream kernel. So I think you could just demand for the proper binding regardless of the firmware situation.

author
#13 lore raw

Re: [PATCH v2 2/4] dt-bindings: memory: Add binding for NVIDIA Tegra30 External Memory Controller

02.05.2019 3:52, Dmitry Osipenko пишет:

Quoted 19 lines

02.05.2019 3:17, Rob Herring пишет:

Quoted 18 lines · Dmitry Osipenko
Quoted 16 lines

30.04.2019 1:05, Rob Herring пишет:

Quoted 6 lines · Dmitry Osipenko
Quoted 3 lines

Add device-tree binding for NVIDIA Tegra30 External Memory Controller. The binding is based on the Tegra124 EMC binding since hardware is similar, although there are couple significant differences.

My comments on Tegra124 binding apply here.

The common timing definition doesn't fully match the definition that is used by Tegra's Memory Controller, thus the DQS (data strobe) timing parameter is comprised of multiple sub-parameters that describe how to generate the strobe in hardware. There are also more additional parameters that are specific to Tegra and they are individually characterized for each memory model and clock rate. Hence the common timing definition isn't usable.

Quoted 6 lines
Quoted 3 lines

I don't understand. Every PC in the world can work with any DIMM (within a given generation) just with SPD data. Why is that not sufficient here?

Because this is not a standard PC, but a custom embedded hardware that is simpler and also doesn't fully follow the standards in some cases.

Even if there is a way to derive at least some of required parameters from the common timing, I don't have information about how to do it and there is pretty much no chance to get it into public.

Rob, please let me know if you're okay with this binding.

author
#14 lore raw

Re: [PATCH v2 2/4] dt-bindings: memory: Add binding for NVIDIA Tegra30 External Memory Controller

02.05.2019 4:39, Dmitry Osipenko пишет:

Quoted 24 lines

02.05.2019 3:52, Dmitry Osipenko пишет:

Quoted 23 lines

02.05.2019 3:17, Rob Herring пишет:

Quoted 21 lines · Dmitry Osipenko
Quoted 9 lines

30.04.2019 1:05, Rob Herring пишет:

Quoted 6 lines · Dmitry Osipenko
Quoted 3 lines

Add device-tree binding for NVIDIA Tegra30 External Memory Controller. The binding is based on the Tegra124 EMC binding since hardware is similar, although there are couple significant differences.

My comments on Tegra124 binding apply here.

Quoted 7 lines

The common timing definition doesn't fully match the definition that is used by Tegra's Memory Controller, thus the DQS (data strobe) timing parameter is comprised of multiple sub-parameters that describe how to generate the strobe in hardware. There are also more additional parameters that are specific to Tegra and they are individually characterized for each memory model and clock rate. Hence the common timing definition isn't usable.

I don't understand. Every PC in the world can work with any DIMM (within a given generation) just with SPD data. Why is that not sufficient here?

Quoted 9 lines
Quoted 2 lines

Because this is not a standard PC, but a custom embedded hardware that is simpler and also doesn't fully follow the standards in some cases.

Even if there is a way to derive at least some of required parameters from the common timing, I don't have information about how to do it and there is pretty much no chance to get it into public.

Rob, please let me know if you're okay with this binding.

I looked through the Technical Reference Manual a bit more closely and it actually has comments about how to convert some of the common timing parameters into register values. Some of those parameters are not trivial to convert and the doc could be incomplete, and some of the parameters are not documented at all.

The other much more minor reason against the common timing description is that all boards come with the memory timing definition in a platform-specific form in downstream kernel. We're re-using that definition in the device-tree, hence it is easy to port the timings from downstream into upstream.

author
#15 lore raw

PATCH v2 3/4 memory: tegra: Introduce Tegra30 EMC driver

In reply to Dmitry Osipenko

Introduce driver for the External Memory Controller (EMC) found on Tegra30 chips, it controls the external DRAM on the board. The purpose of this driver is to program memory timing for external memory on the EMC clock rate change.

Signed-off-by Dmitry Osipenko <[email protected]>
---
 drivers/memory/tegra/Kconfig       |   10 +
 drivers/memory/tegra/Makefile      |    1 +
 drivers/memory/tegra/mc.c          |    3 -
 drivers/memory/tegra/mc.h          |   30 +-
 drivers/memory/tegra/tegra30-emc.c | 1159 ++++++++++++++++++++++++++++
 drivers/memory/tegra/tegra30.c     |   44 ++
 6 files changed, 1235 insertions(+), 12 deletions(-)
 create mode 100644 drivers/memory/tegra/tegra30-emc.c
drivers/memory/tegra/Kconfig +10 -0
index 34e0b70f5c5f..89c02a368c65 100644
@@ -16,6 +16,16 @@ config TEGRA20_EMC
 	  This driver is required to change memory timings / clock rate for
 	  external memory.
 
+config TEGRA30_EMC
+	bool "NVIDIA Tegra30 External Memory Controller driver"
+	default y
+	depends on TEGRA_MC && ARCH_TEGRA_3x_SOC
+	help
+	  This driver is for the External Memory Controller (EMC) found on
+	  Tegra30 chips. The EMC controls the external DRAM on the board.
+	  This driver is required to change memory timings / clock rate for
+	  external memory.
+
 config TEGRA124_EMC
 	bool "NVIDIA Tegra124 External Memory Controller driver"
 	default y
drivers/memory/tegra/Makefile +1 -0
index 3971a6b7c487..3d23c4261104 100644
@@ -11,5 +11,6 @@ tegra-mc-$(CONFIG_ARCH_TEGRA_210_SOC) += tegra210.o
 obj-$(CONFIG_TEGRA_MC) += tegra-mc.o
 
 obj-$(CONFIG_TEGRA20_EMC)  += tegra20-emc.o
+obj-$(CONFIG_TEGRA30_EMC)  += tegra30-emc.o
 obj-$(CONFIG_TEGRA124_EMC) += tegra124-emc.o
 obj-$(CONFIG_ARCH_TEGRA_186_SOC) += tegra186.o
drivers/memory/tegra/mc.c +0 -3
index 31b47459c84d..52ede43f1e07 100644
@@ -51,9 +51,6 @@
 #define MC_EMEM_ADR_CFG 0x54
 #define MC_EMEM_ADR_CFG_EMEM_NUMDEV BIT(0)
 
-#define MC_TIMING_CONTROL		0xfc
-#define MC_TIMING_UPDATE		BIT(0)
-
 static const struct of_device_id tegra_mc_of_match[] = {
 #ifdef CONFIG_ARCH_TEGRA_2x_SOC
 	{ .compatible = "nvidia,tegra20-mc-gart", .data = &tegra20_mc_soc },
drivers/memory/tegra/mc.h +21 -9
index 887a3b07334f..90da9f62ce2b 100644
@@ -9,20 +9,32 @@
 #ifndef MEMORY_TEGRA_MC_H
 #define MEMORY_TEGRA_MC_H
 
+#include <linux/bits.h>
 #include <linux/io.h>
 #include <linux/types.h>
 
 #include <soc/tegra/mc.h>
 
-#define MC_INT_DECERR_MTS (1 << 16)
-#define MC_INT_SECERR_SEC (1 << 13)
-#define MC_INT_DECERR_VPR (1 << 12)
-#define MC_INT_INVALID_APB_ASID_UPDATE (1 << 11)
-#define MC_INT_INVALID_SMMU_PAGE (1 << 10)
-#define MC_INT_ARBITRATION_EMEM (1 << 9)
-#define MC_INT_SECURITY_VIOLATION (1 << 8)
-#define MC_INT_INVALID_GART_PAGE (1 << 7)
-#define MC_INT_DECERR_EMEM (1 << 6)
+#define MC_INT_DECERR_MTS				BIT(16)
+#define MC_INT_SECERR_SEC				BIT(13)
+#define MC_INT_DECERR_VPR				BIT(12)
+#define MC_INT_INVALID_APB_ASID_UPDATE			BIT(11)
+#define MC_INT_INVALID_SMMU_PAGE			BIT(10)
+#define MC_INT_ARBITRATION_EMEM				BIT(9)
+#define MC_INT_SECURITY_VIOLATION			BIT(8)
+#define MC_INT_INVALID_GART_PAGE			BIT(7)
+#define MC_INT_DECERR_EMEM				BIT(6)
+
+#define MC_EMEM_ARB_OUTSTANDING_REQ			0x94
+#define MC_EMEM_ARB_OUTSTANDING_REQ_MAX_MASK		0x1ff
+#define MC_EMEM_ARB_OUTSTANDING_REQ_HOLDOFF_OVERRIDE	BIT(30)
+#define MC_EMEM_ARB_OUTSTANDING_REQ_LIMIT_ENABLE	BIT(31)
+
+#define MC_EMEM_ARB_OVERRIDE				0xe8
+#define MC_EMEM_ARB_OVERRIDE_EACK_MASK			0x3
+
+#define MC_TIMING_CONTROL				0xfc
+#define MC_TIMING_UPDATE				BIT(0)
 
 static inline u32 mc_readl(struct tegra_mc *mc, unsigned long offset)
 {
drivers/memory/tegra/tegra30-emc.c new file +1159 -0

1,159 lines not on this page · raw message

drivers/memory/tegra/tegra30.c +44 -0

56 lines not on this page · raw message

Signature · 1 line
2.21.0
author
#16 lore raw

PATCH v2 4/4 ARM: dts: tegra30: Add External Memory Controller node

In reply to Dmitry Osipenko

Add External Memory Controller node to the device-tree.

Signed-off-by Dmitry Osipenko <[email protected]>
---
 arch/arm/boot/dts/tegra30.dtsi | 11 +++++++++++
 1 file changed, 11 insertions(+)
arch/arm/boot/dts/tegra30.dtsi +11 -0
index e074258d4518..92c4aeafab29 100644
@@ -732,6 +732,17 @@
 		#reset-cells = <1>;
 	};
 
+	memory-controller@7000f400 {
+		compatible = "nvidia,tegra30-emc";
+		reg = <0x7000f400 0x400>;
+		interrupts = <GIC_SPI 78 IRQ_TYPE_LEVEL_HIGH>;
+		clocks = <&tegra_car TEGRA30_CLK_EMC>;
+		#address-cells = <1>;
+		#size-cells = <0>;
+
+		nvidia,memory-controller = <&mc>;
+	};
+
 	fuse@7000f800 {
 		compatible = "nvidia,tegra30-efuse";
 		reg = <0x7000f800 0x400>;
Signature · 1 line
2.21.0