[PATCH v2 0/5] Tegra20 External Memory Controller driver

[PATCH v2 0/5] Tegra20 External Memory Controller driver

[Dmitry Osipenko]

Hello,

Couple years ago the Tegra20 EMC driver was removed from the kernel
due to incompatible changes in the Tegra's clock driver. This patchset
introduces a modernized EMC driver. Currently the sole purpose of the
driver is to initialize DRAM frequency to maximum rate during of the
kernels boot-up. Later we may consider implementing dynamic memory
frequency scaling, utilizing functionality provided by this driver.

Changelog:

v2:
	- Minor code cleanups like consistent use of writel_relaxed instead
	  of non-relaxed version, reworded error messages, etc.

	- Factored out use_pllm_ud bit checking into a standalone patch for
	  consistency.

Dmitry Osipenko (5):
  dt: bindings: tegra20-emc: Document interrupt property
  ARM: dts: tegra20: Add interrupt to External Memory Controller
  clk: tegra20: Turn EMC clock gate into divider
  clk: tegra20: Check whether direct PLLM sourcing is turned off for EMC
  memory: tegra: Introduce Tegra20 EMC driver

 .../bindings/arm/tegra/nvidia,tegra20-emc.txt |   2 +
 arch/arm/boot/dts/tegra20.dtsi                |   1 +
 drivers/clk/tegra/clk-tegra20.c               |  46 +-
 drivers/memory/tegra/Kconfig                  |  10 +
 drivers/memory/tegra/Makefile                 |   1 +
 drivers/memory/tegra/tegra20-emc.c            | 586 ++++++++++++++++++
 6 files changed, 636 insertions(+), 10 deletions(-)
 create mode 100644 drivers/memory/tegra/tegra20-emc.c

-- 
2.17.0

[PATCH v2 1/5] dt: bindings: tegra20-emc: Document interrupt property

[Dmitry Osipenko]

EMC has a dedicated interrupt that is used to notify about completion of
HW operations. Document the interrupt property.

Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
 .../devicetree/bindings/arm/tegra/nvidia,tegra20-emc.txt        | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-emc.txt b/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-emc.txt
index 4c33b29dc660..a6fe401d0d48 100644
--- a/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-emc.txt
+++ b/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-emc.txt
@@ -10,6 +10,7 @@ Properties:
   and chosen using the ramcode board selector. If omitted, only one
   set of tables can be present and said tables will be used
   irrespective of ram-code configuration.
+- interrupts : Should contain EMC General interrupt.
 
 Child device nodes describe the memory settings for different configurations and clock rates.
 
@@ -20,6 +21,7 @@ Example:
 		#size-cells = < 0 >;
 		compatible = "nvidia,tegra20-emc";
 		reg = <0x7000f4000 0x200>;
+		interrupts = <0 78 0x04>;
 	}
 
 
-- 
2.17.0

Re: [PATCH v2 1/5] dt: bindings: tegra20-emc: Document interrupt property

[Rob Herring]

On Mon, Jun 04, 2018 at 01:36:50AM +0300, Dmitry Osipenko wrote:
> EMC has a dedicated interrupt that is used to notify about completion of
> HW operations. Document the interrupt property.
> 
> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> ---
>  .../devicetree/bindings/arm/tegra/nvidia,tegra20-emc.txt        | 2 ++
>  1 file changed, 2 insertions(+)

Acked-by: Rob Herring <robh@kernel.org>

[PATCH v2 2/5] ARM: dts: tegra20: Add interrupt to External Memory Controller

[Dmitry Osipenko]

Add interrupt entry into the EMC DT node.

Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
 arch/arm/boot/dts/tegra20.dtsi | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm/boot/dts/tegra20.dtsi b/arch/arm/boot/dts/tegra20.dtsi
index 983dd5c14794..3cd3cb28cfd9 100644
--- a/arch/arm/boot/dts/tegra20.dtsi
+++ b/arch/arm/boot/dts/tegra20.dtsi
@@ -609,6 +609,7 @@
 	memory-controller@7000f400 {
 		compatible = "nvidia,tegra20-emc";
 		reg = <0x7000f400 0x200>;
+		interrupts = <GIC_SPI 78 IRQ_TYPE_LEVEL_HIGH>;
 		#address-cells = <1>;
 		#size-cells = <0>;
 	};
-- 
2.17.0

[PATCH v2 3/5] clk: tegra20: Turn EMC clock gate into divider

[Dmitry Osipenko]

Kernel should never gate the EMC clock as it causes immediate lockup, so
removing clk-gate functionality doesn't affect anything. Turning EMC clk
gate into divider allows to implement glitch-less EMC scaling, avoiding
reparenting to a backup clock.

Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
 drivers/clk/tegra/clk-tegra20.c | 36 ++++++++++++++++++++++++---------
 1 file changed, 26 insertions(+), 10 deletions(-)

diff --git a/drivers/clk/tegra/clk-tegra20.c b/drivers/clk/tegra/clk-tegra20.c
index cc857d4d4a86..2bd35418716a 100644
--- a/drivers/clk/tegra/clk-tegra20.c
+++ b/drivers/clk/tegra/clk-tegra20.c
@@ -578,7 +578,6 @@ static struct tegra_clk tegra20_clks[tegra_clk_max] __initdata = {
 	[tegra_clk_afi] = { .dt_id = TEGRA20_CLK_AFI, .present = true },
 	[tegra_clk_fuse] = { .dt_id = TEGRA20_CLK_FUSE, .present = true },
 	[tegra_clk_kfuse] = { .dt_id = TEGRA20_CLK_KFUSE, .present = true },
-	[tegra_clk_emc] = { .dt_id = TEGRA20_CLK_EMC, .present = true },
 };
 
 static unsigned long tegra20_clk_measure_input_freq(void)
@@ -799,6 +798,31 @@ 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 void __init tegra20_emc_clk_init(void)
+{
+	struct clk *clk;
+
+	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 = tegra_clk_register_mc("mc", "emc_mux", clk_base + CLK_SOURCE_EMC,
+				    &emc_lock);
+	clks[TEGRA20_CLK_MC] = clk;
+
+	/*
+	 * Note that 'emc_mux' source and 'emc' rate shouldn't be changed at
+	 * the same time due to a HW bug, this won't happen because we're
+	 * defining 'emc_mux' and 'emc' as a distinct clocks.
+	 */
+	clk = clk_register_divider(NULL, "emc", "emc_mux", CLK_IS_CRITICAL,
+				   clk_base + CLK_SOURCE_EMC, 0, 7,
+				   0, &emc_lock);
+	clks[TEGRA20_CLK_EMC] = clk;
+}
+
 static void __init tegra20_periph_clk_init(void)
 {
 	struct tegra_periph_init_data *data;
@@ -812,15 +836,7 @@ static void __init tegra20_periph_clk_init(void)
 	clks[TEGRA20_CLK_AC97] = 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 = tegra_clk_register_mc("mc", "emc_mux", clk_base + CLK_SOURCE_EMC,
-				    &emc_lock);
-	clks[TEGRA20_CLK_MC] = clk;
+	tegra20_emc_clk_init();
 
 	/* dsi */
 	clk = tegra_clk_register_periph_gate("dsi", "pll_d", 0, clk_base, 0,
-- 
2.17.0

[PATCH v2 4/5] clk: tegra20: Check whether direct PLLM sourcing is turned off for EMC

[Dmitry Osipenko]

Ensure that direct PLLM sourcing is turned off for EMC as we don't support
that configuration in the clk driver.

Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
 drivers/clk/tegra/clk-tegra20.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/clk/tegra/clk-tegra20.c b/drivers/clk/tegra/clk-tegra20.c
index 2bd35418716a..ca4eadb9520e 100644
--- a/drivers/clk/tegra/clk-tegra20.c
+++ b/drivers/clk/tegra/clk-tegra20.c
@@ -800,7 +800,9 @@ static struct tegra_periph_init_data tegra_periph_nodiv_clk_list[] = {
 
 static void __init tegra20_emc_clk_init(void)
 {
+	const u32 use_pllm_ud = BIT(29);
 	struct clk *clk;
+	u32 emc_reg;
 
 	clk = clk_register_mux(NULL, "emc_mux", mux_pllmcp_clkm,
 			       ARRAY_SIZE(mux_pllmcp_clkm),
@@ -812,6 +814,14 @@ static void __init tegra20_emc_clk_init(void)
 				    &emc_lock);
 	clks[TEGRA20_CLK_MC] = clk;
 
+	/* un-divided pll_m_out0 is currently unsupported */
+	emc_reg = readl_relaxed(clk_base + CLK_SOURCE_EMC);
+	if (emc_reg & use_pllm_ud) {
+		pr_err("%s: un-divided PllM_out0 used as clock source\n",
+		       __func__);
+		return;
+	}
+
 	/*
 	 * Note that 'emc_mux' source and 'emc' rate shouldn't be changed at
 	 * the same time due to a HW bug, this won't happen because we're
-- 
2.17.0

[PATCH v2 5/5] memory: tegra: Introduce Tegra20 EMC driver

[Dmitry Osipenko]

Introduce driver for the External Memory Controller (EMC) found on Tegra20
chips, which 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 <digetx@gmail.com>
---
 drivers/memory/tegra/Kconfig       |  10 +
 drivers/memory/tegra/Makefile      |   1 +
 drivers/memory/tegra/tegra20-emc.c | 586 +++++++++++++++++++++++++++++
 3 files changed, 597 insertions(+)
 create mode 100644 drivers/memory/tegra/tegra20-emc.c

diff --git a/drivers/memory/tegra/Kconfig b/drivers/memory/tegra/Kconfig
index 6d74e499e18d..34e0b70f5c5f 100644
--- a/drivers/memory/tegra/Kconfig
+++ b/drivers/memory/tegra/Kconfig
@@ -6,6 +6,16 @@ config TEGRA_MC
 	  This driver supports the Memory Controller (MC) hardware found on
 	  NVIDIA Tegra SoCs.
 
+config TEGRA20_EMC
+	bool "NVIDIA Tegra20 External Memory Controller driver"
+	default y
+	depends on ARCH_TEGRA_2x_SOC
+	help
+	  This driver is for the External Memory Controller (EMC) found on
+	  Tegra20 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
diff --git a/drivers/memory/tegra/Makefile b/drivers/memory/tegra/Makefile
index 94ab16ba075b..3971a6b7c487 100644
--- a/drivers/memory/tegra/Makefile
+++ b/drivers/memory/tegra/Makefile
@@ -10,5 +10,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_TEGRA124_EMC) += tegra124-emc.o
 obj-$(CONFIG_ARCH_TEGRA_186_SOC) += tegra186.o
diff --git a/drivers/memory/tegra/tegra20-emc.c b/drivers/memory/tegra/tegra20-emc.c
new file mode 100644
index 000000000000..26a18b5e7941
--- /dev/null
+++ b/drivers/memory/tegra/tegra20-emc.c
@@ -0,0 +1,586 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Tegra20 External Memory Controller driver
+ *
+ * Author: Dmitry Osipenko <digetx@gmail.com>
+ */
+
+#include <linux/clk.h>
+#include <linux/completion.h>
+#include <linux/err.h>
+#include <linux/interrupt.h>
+#include <linux/iopoll.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/sort.h>
+#include <linux/types.h>
+
+#include <soc/tegra/fuse.h>
+
+#define EMC_INTSTATUS				0x000
+#define EMC_INTMASK				0x004
+#define EMC_TIMING_CONTROL			0x028
+#define EMC_RC					0x02c
+#define EMC_RFC					0x030
+#define EMC_RAS					0x034
+#define EMC_RP					0x038
+#define EMC_R2W					0x03c
+#define EMC_W2R					0x040
+#define EMC_R2P					0x044
+#define EMC_W2P					0x048
+#define EMC_RD_RCD				0x04c
+#define EMC_WR_RCD				0x050
+#define EMC_RRD					0x054
+#define EMC_REXT				0x058
+#define EMC_WDV					0x05c
+#define EMC_QUSE				0x060
+#define EMC_QRST				0x064
+#define EMC_QSAFE				0x068
+#define EMC_RDV					0x06c
+#define EMC_REFRESH				0x070
+#define EMC_BURST_REFRESH_NUM			0x074
+#define EMC_PDEX2WR				0x078
+#define EMC_PDEX2RD				0x07c
+#define EMC_PCHG2PDEN				0x080
+#define EMC_ACT2PDEN				0x084
+#define EMC_AR2PDEN				0x088
+#define EMC_RW2PDEN				0x08c
+#define EMC_TXSR				0x090
+#define EMC_TCKE				0x094
+#define EMC_TFAW				0x098
+#define EMC_TRPAB				0x09c
+#define EMC_TCLKSTABLE				0x0a0
+#define EMC_TCLKSTOP				0x0a4
+#define EMC_TREFBW				0x0a8
+#define EMC_QUSE_EXTRA				0x0ac
+#define EMC_ODT_WRITE				0x0b0
+#define EMC_ODT_READ				0x0b4
+#define EMC_FBIO_CFG5				0x104
+#define EMC_FBIO_CFG6				0x114
+#define EMC_AUTO_CAL_INTERVAL			0x2a8
+#define EMC_CFG_2				0x2b8
+#define EMC_CFG_DIG_DLL				0x2bc
+#define EMC_DLL_XFORM_DQS			0x2c0
+#define EMC_DLL_XFORM_QUSE			0x2c4
+#define EMC_ZCAL_REF_CNT			0x2e0
+#define EMC_ZCAL_WAIT_CNT			0x2e4
+#define EMC_CFG_CLKTRIM_0			0x2d0
+#define EMC_CFG_CLKTRIM_1			0x2d4
+#define EMC_CFG_CLKTRIM_2			0x2d8
+
+#define EMC_CLKCHANGE_REQ_ENABLE		BIT(0)
+#define EMC_CLKCHANGE_PD_ENABLE			BIT(1)
+#define EMC_CLKCHANGE_SR_ENABLE			BIT(2)
+
+#define EMC_TIMING_UPDATE			BIT(0)
+
+#define EMC_CLKCHANGE_COMPLETE_INT		BIT(4)
+
+static const unsigned long emc_timing_registers[] = {
+	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_WDV,
+	EMC_QUSE,
+	EMC_QRST,
+	EMC_QSAFE,
+	EMC_RDV,
+	EMC_REFRESH,
+	EMC_BURST_REFRESH_NUM,
+	EMC_PDEX2WR,
+	EMC_PDEX2RD,
+	EMC_PCHG2PDEN,
+	EMC_ACT2PDEN,
+	EMC_AR2PDEN,
+	EMC_RW2PDEN,
+	EMC_TXSR,
+	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_DLL_XFORM_DQS,
+	EMC_DLL_XFORM_QUSE,
+	EMC_ZCAL_REF_CNT,
+	EMC_ZCAL_WAIT_CNT,
+	EMC_AUTO_CAL_INTERVAL,
+	EMC_CFG_CLKTRIM_0,
+	EMC_CFG_CLKTRIM_1,
+	EMC_CFG_CLKTRIM_2,
+};
+
+struct emc_timing {
+	unsigned long rate;
+	u32 emc_registers_data[ARRAY_SIZE(emc_timing_registers)];
+};
+
+struct tegra_emc {
+	struct device *dev;
+	struct notifier_block clk_nb;
+	struct clk *backup_clk;
+	struct clk *emc_mux;
+	struct clk *pll_m;
+	struct clk *clk;
+	void __iomem *regs;
+
+	struct completion clk_handshake_complete;
+	int irq;
+
+	struct emc_timing *timings;
+	unsigned int num_timings;
+};
+
+static irqreturn_t tegra_emc_isr(int irq, void *data)
+{
+	struct tegra_emc *emc = data;
+	u32 intmask = EMC_CLKCHANGE_COMPLETE_INT;
+	u32 status;
+
+	status = readl_relaxed(emc->regs + EMC_INTSTATUS) & intmask;
+	if (!status)
+		return IRQ_NONE;
+
+	/* clear interrupts */
+	writel_relaxed(status, emc->regs + EMC_INTSTATUS);
+
+	/* notify about EMC-CAR handshake completion */
+	complete(&emc->clk_handshake_complete);
+
+	return IRQ_HANDLED;
+}
+
+static struct emc_timing *tegra_emc_find_timing(struct tegra_emc *emc,
+						unsigned long rate)
+{
+	struct emc_timing *timing = NULL;
+	unsigned int i;
+
+	for (i = 0; i < emc->num_timings; i++) {
+		if (emc->timings[i].rate >= rate) {
+			timing = &emc->timings[i];
+			break;
+		}
+	}
+
+	if (!timing) {
+		dev_err(emc->dev, "no timing for rate %lu\n", rate);
+		return NULL;
+	}
+
+	return timing;
+}
+
+static int emc_prepare_timing_change(struct tegra_emc *emc, unsigned long rate)
+{
+	struct emc_timing *timing = tegra_emc_find_timing(emc, rate);
+	unsigned int i;
+
+	if (!timing)
+		return -ENOENT;
+
+	dev_dbg(emc->dev, "%s: timing rate %lu emc rate %lu\n",
+		__func__, timing->rate, rate);
+
+	/* program shadow registers */
+	for (i = 0; i < ARRAY_SIZE(timing->emc_registers_data); i++)
+		writel_relaxed(timing->emc_registers_data[i],
+			       emc->regs + emc_timing_registers[i]);
+
+	/* wait until programming has settled */
+	readl_relaxed(emc->regs + emc_timing_registers[0]);
+
+	if (emc->irq < 0)
+		writel_relaxed(EMC_CLKCHANGE_COMPLETE_INT,
+			       emc->regs + EMC_INTMASK);
+	else
+		reinit_completion(&emc->clk_handshake_complete);
+
+	return 0;
+}
+
+static int emc_complete_timing_change(struct tegra_emc *emc, bool flush)
+{
+	long timeout;
+	u32 value;
+	int err;
+
+	dev_dbg(emc->dev, "%s: flush %d\n", __func__, flush);
+
+	if (flush) {
+		/* manually initiate memory timing update */
+		writel_relaxed(EMC_TIMING_UPDATE,
+			       emc->regs + EMC_TIMING_CONTROL);
+		return 0;
+	}
+
+	if (emc->irq < 0) {
+		/* poll interrupt status if IRQ isn't available */
+		err = readl_relaxed_poll_timeout(emc->regs + EMC_INTSTATUS,
+				value, value & EMC_CLKCHANGE_COMPLETE_INT,
+				1, 100);
+		if (err) {
+			dev_err(emc->dev, "EMC-CAR handshake failed\n");
+			return -EIO;
+		}
+
+		return 0;
+	}
+
+	timeout = wait_for_completion_timeout(&emc->clk_handshake_complete,
+					      usecs_to_jiffies(100));
+	if (timeout == 0) {
+		dev_err(emc->dev, "EMC handshake failed\n");
+		return -EIO;
+	} else if (timeout < 0) {
+		dev_err(emc->dev, "failed to wait for EMC-CAR handshake: %ld\n",
+			timeout);
+		return timeout;
+	}
+
+	return 0;
+}
+
+static int load_one_timing_from_dt(struct tegra_emc *emc,
+				   struct emc_timing *timing,
+				   struct device_node *node)
+{
+	u32 rate;
+	int err;
+
+	if (!of_device_is_compatible(node, "nvidia,tegra20-emc-table")) {
+		dev_err(emc->dev, "incompatible DT node \"%s\"\n",
+			node->name);
+		return -EINVAL;
+	}
+
+	err = of_property_read_u32(node, "clock-frequency", &rate);
+	if (err) {
+		dev_err(emc->dev, "timing %s: failed to read rate: %d\n",
+			node->name, err);
+		return err;
+	}
+
+	err = of_property_read_u32_array(node, "nvidia,emc-registers",
+					 timing->emc_registers_data,
+					 ARRAY_SIZE(emc_timing_registers));
+	if (err) {
+		dev_err(emc->dev,
+			"timing %s: failed to read emc timing data: %d\n",
+			node->name, err);
+		return err;
+	}
+
+	/*
+	 * The EMC clock rate is twice the bus rate, and the bus rate is
+	 * measured in kHz.
+	 */
+	timing->rate = rate * 2 * 1000;
+
+	dev_dbg(emc->dev, "%s: emc rate %ld\n", __func__, timing->rate);
+
+	return 0;
+}
+
+static int cmp_timings(const void *_a, const void *_b)
+{
+	const struct emc_timing *a = _a;
+	const struct emc_timing *b = _b;
+
+	if (a->rate < b->rate)
+		return -1;
+	else if (a->rate == b->rate)
+		return 0;
+	else
+		return 1;
+}
+
+static int tegra_emc_load_timings_from_dt(struct tegra_emc *emc,
+					  struct device_node *node)
+{
+	struct device_node *child;
+	struct emc_timing *timing;
+	int child_count;
+	int err;
+
+	child_count = of_get_child_count(node);
+	if (!child_count) {
+		dev_err(emc->dev, "no memory timings in DT node\n");
+		return -ENOENT;
+	}
+
+	emc->timings = devm_kcalloc(emc->dev, child_count, sizeof(*timing),
+				    GFP_KERNEL);
+	if (!emc->timings)
+		return -ENOMEM;
+
+	emc->num_timings = child_count;
+	timing = emc->timings;
+
+	for_each_child_of_node(node, child) {
+		err = load_one_timing_from_dt(emc, timing++, child);
+		if (err) {
+			of_node_put(child);
+			return err;
+		}
+	}
+
+	sort(emc->timings, emc->num_timings, sizeof(*timing), cmp_timings,
+	     NULL);
+
+	return 0;
+}
+
+static struct device_node *
+tegra_emc_find_node_by_ram_code(struct tegra_emc *emc, u32 ram_code)
+{
+	struct device_node *np;
+	int err;
+
+	for_each_child_of_node(emc->dev->of_node, np) {
+		u32 value;
+
+		err = of_property_read_u32(np, "nvidia,ram-code", &value);
+		if (err || value != ram_code)
+			continue;
+
+		return np;
+	}
+
+	dev_info(emc->dev, "no memory timings for RAM code %u found in DT\n",
+		 ram_code);
+
+	return NULL;
+}
+
+static int tegra_emc_clk_change_notify(struct notifier_block *nb,
+				       unsigned long msg, void *data)
+{
+	struct tegra_emc *emc = container_of(nb, struct tegra_emc, clk_nb);
+	struct clk_notifier_data *cnd = data;
+	int err;
+
+	switch (msg) {
+	case PRE_RATE_CHANGE:
+		err = emc_prepare_timing_change(emc, cnd->new_rate);
+		break;
+
+	case ABORT_RATE_CHANGE:
+		err = emc_prepare_timing_change(emc, cnd->old_rate);
+		if (err)
+			break;
+
+		err = emc_complete_timing_change(emc, true);
+		break;
+
+	case POST_RATE_CHANGE:
+		err = emc_complete_timing_change(emc, false);
+		break;
+
+	default:
+		return NOTIFY_DONE;
+	}
+
+	return notifier_from_errno(err);
+}
+
+static int emc_setup_hw(struct tegra_emc *emc)
+{
+	u32 emc_cfg;
+
+	emc_cfg = readl_relaxed(emc->regs + EMC_CFG_2);
+
+	/*
+	 * Depending on a memory type, DRAM should enter either self-refresh
+	 * or power-down state on EMC clock change.
+	 */
+	if (!(emc_cfg & EMC_CLKCHANGE_PD_ENABLE) &&
+	    !(emc_cfg & EMC_CLKCHANGE_SR_ENABLE))
+	{
+		dev_err(emc->dev,
+			"bootloader didn't specify DRAM auto-suspend mode\n");
+		return -EINVAL;
+	}
+
+	/* allow EMC and CAR to handshake on PLL divider/source changes */
+	emc_cfg |= EMC_CLKCHANGE_REQ_ENABLE;
+	writel_relaxed(emc_cfg, emc->regs + EMC_CFG_2);
+
+	/* initialize interrupt */
+	writel_relaxed(EMC_CLKCHANGE_COMPLETE_INT, emc->regs + EMC_INTMASK);
+	writel_relaxed(EMC_CLKCHANGE_COMPLETE_INT, emc->regs + EMC_INTSTATUS);
+
+	return 0;
+}
+
+static int emc_init(struct tegra_emc *emc, unsigned long rate)
+{
+	int err;
+
+	err = clk_set_parent(emc->emc_mux, emc->backup_clk);
+	if (err) {
+		dev_err(emc->dev,
+			"failed to reparent to backup source: %d\n", err);
+		return err;
+	}
+
+	err = clk_set_rate(emc->pll_m, rate);
+	if (err)
+		dev_err(emc->dev,
+			"failed to change pll_m rate: %d\n", err);
+
+	err = clk_set_parent(emc->emc_mux, emc->pll_m);
+	if (err) {
+		dev_err(emc->dev,
+			"failed to reparent to pll_m: %d\n", err);
+		return err;
+	}
+
+	return 0;
+}
+
+static int tegra_emc_probe(struct platform_device *pdev)
+{
+	struct device_node *np;
+	struct tegra_emc *emc;
+	struct resource *res;
+	u32 ram_code;
+	int err;
+
+	emc = devm_kzalloc(&pdev->dev, sizeof(*emc), GFP_KERNEL);
+	if (!emc)
+		return -ENOMEM;
+
+	emc->dev = &pdev->dev;
+
+	ram_code = tegra_read_ram_code();
+
+	np = tegra_emc_find_node_by_ram_code(emc, ram_code);
+	if (!np)
+		return -ENOENT;
+
+	err = tegra_emc_load_timings_from_dt(emc, np);
+	of_node_put(np);
+	if (err)
+		return err;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	emc->regs = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(emc->regs))
+		return PTR_ERR(emc->regs);
+
+	err = emc_setup_hw(emc);
+	if (err)
+		return err;
+
+	emc->irq = platform_get_irq(pdev, 0);
+	if (emc->irq < 0) {
+		dev_warn(&pdev->dev, "interrupt not specified\n");
+		dev_warn(&pdev->dev, "continuing, but please update your DT\n");
+	} else {
+		init_completion(&emc->clk_handshake_complete);
+
+		err = devm_request_irq(&pdev->dev, emc->irq, tegra_emc_isr, 0,
+				       dev_name(&pdev->dev), emc);
+		if (err < 0) {
+			dev_err(&pdev->dev, "failed to request IRQ#%u: %d\n",
+				emc->irq, err);
+			return err;
+		}
+	}
+
+	emc->pll_m = clk_get_sys(NULL, "pll_m");
+	if (IS_ERR(emc->pll_m)) {
+		err = PTR_ERR(emc->pll_m);
+		dev_err(&pdev->dev, "failed to get pll_m: %d\n", err);
+		return err;
+	}
+
+	emc->backup_clk = clk_get_sys(NULL, "pll_p");
+	if (IS_ERR(emc->backup_clk)) {
+		err = PTR_ERR(emc->backup_clk);
+		dev_err(&pdev->dev, "failed to get pll_p: %d\n", err);
+		goto put_pll_m;
+	}
+
+	emc->clk = clk_get_sys(NULL, "emc");
+	if (IS_ERR(emc->clk)) {
+		err = PTR_ERR(emc->clk);
+		dev_err(&pdev->dev, "failed to get emc: %d\n", err);
+		goto put_backup;
+	}
+
+	emc->emc_mux = clk_get_parent(emc->clk);
+	if (IS_ERR(emc->emc_mux)) {
+		err = PTR_ERR(emc->emc_mux);
+		dev_err(&pdev->dev, "failed to get emc_mux: %d\n", err);
+		goto put_emc;
+	}
+
+	emc->clk_nb.notifier_call = tegra_emc_clk_change_notify;
+
+	err = clk_notifier_register(emc->clk, &emc->clk_nb);
+	if (err) {
+		dev_err(&pdev->dev, "failed to register clk notifier: %d\n",
+			err);
+		goto put_emc;
+	}
+
+	/* set DRAM clock rate to maximum */
+	err = emc_init(emc, emc->timings[emc->num_timings - 1].rate);
+	if (err) {
+		dev_err(&pdev->dev, "failed to initialize clk rate: %d\n",
+			err);
+		goto unreg_notifier;
+	}
+
+	return 0;
+
+unreg_notifier:
+	clk_notifier_unregister(emc->emc_mux, &emc->clk_nb);
+put_emc:
+	clk_put(emc->clk);
+put_backup:
+	clk_put(emc->backup_clk);
+put_pll_m:
+	clk_put(emc->pll_m);
+
+	return err;
+}
+
+static const struct of_device_id tegra_emc_of_match[] = {
+	{ .compatible = "nvidia,tegra20-emc", },
+	{},
+};
+
+static struct platform_driver tegra_emc_driver = {
+	.probe = tegra_emc_probe,
+	.driver = {
+		.name = "tegra20-emc",
+		.of_match_table = tegra_emc_of_match,
+		.suppress_bind_attrs = true,
+	},
+};
+
+static int __init tegra_emc_init(void)
+{
+	return platform_driver_register(&tegra_emc_driver);
+}
+subsys_initcall(tegra_emc_init);
-- 
2.17.0

Re: [PATCH v2 5/5] memory: tegra: Introduce Tegra20 EMC driver

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 19214 bytes --]

On Mon, Jun 04, 2018 at 01:36:54AM +0300, Dmitry Osipenko wrote:
> Introduce driver for the External Memory Controller (EMC) found on Tegra20
> chips, which 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 <digetx@gmail.com>
> ---
>  drivers/memory/tegra/Kconfig       |  10 +
>  drivers/memory/tegra/Makefile      |   1 +
>  drivers/memory/tegra/tegra20-emc.c | 586 +++++++++++++++++++++++++++++
>  3 files changed, 597 insertions(+)
>  create mode 100644 drivers/memory/tegra/tegra20-emc.c
> 
> diff --git a/drivers/memory/tegra/Kconfig b/drivers/memory/tegra/Kconfig
> index 6d74e499e18d..34e0b70f5c5f 100644
> --- a/drivers/memory/tegra/Kconfig
> +++ b/drivers/memory/tegra/Kconfig
> @@ -6,6 +6,16 @@ config TEGRA_MC
>  	  This driver supports the Memory Controller (MC) hardware found on
>  	  NVIDIA Tegra SoCs.
>  
> +config TEGRA20_EMC
> +	bool "NVIDIA Tegra20 External Memory Controller driver"
> +	default y
> +	depends on ARCH_TEGRA_2x_SOC
> +	help
> +	  This driver is for the External Memory Controller (EMC) found on
> +	  Tegra20 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
> diff --git a/drivers/memory/tegra/Makefile b/drivers/memory/tegra/Makefile
> index 94ab16ba075b..3971a6b7c487 100644
> --- a/drivers/memory/tegra/Makefile
> +++ b/drivers/memory/tegra/Makefile
> @@ -10,5 +10,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_TEGRA124_EMC) += tegra124-emc.o
>  obj-$(CONFIG_ARCH_TEGRA_186_SOC) += tegra186.o
> diff --git a/drivers/memory/tegra/tegra20-emc.c b/drivers/memory/tegra/tegra20-emc.c
> new file mode 100644
> index 000000000000..26a18b5e7941
> --- /dev/null
> +++ b/drivers/memory/tegra/tegra20-emc.c
> @@ -0,0 +1,586 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Tegra20 External Memory Controller driver
> + *
> + * Author: Dmitry Osipenko <digetx@gmail.com>
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/completion.h>
> +#include <linux/err.h>
> +#include <linux/interrupt.h>
> +#include <linux/iopoll.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/sort.h>
> +#include <linux/types.h>
> +
> +#include <soc/tegra/fuse.h>
> +
> +#define EMC_INTSTATUS				0x000
> +#define EMC_INTMASK				0x004
> +#define EMC_TIMING_CONTROL			0x028
> +#define EMC_RC					0x02c
> +#define EMC_RFC					0x030
> +#define EMC_RAS					0x034
> +#define EMC_RP					0x038
> +#define EMC_R2W					0x03c
> +#define EMC_W2R					0x040
> +#define EMC_R2P					0x044
> +#define EMC_W2P					0x048
> +#define EMC_RD_RCD				0x04c
> +#define EMC_WR_RCD				0x050
> +#define EMC_RRD					0x054
> +#define EMC_REXT				0x058
> +#define EMC_WDV					0x05c
> +#define EMC_QUSE				0x060
> +#define EMC_QRST				0x064
> +#define EMC_QSAFE				0x068
> +#define EMC_RDV					0x06c
> +#define EMC_REFRESH				0x070
> +#define EMC_BURST_REFRESH_NUM			0x074
> +#define EMC_PDEX2WR				0x078
> +#define EMC_PDEX2RD				0x07c
> +#define EMC_PCHG2PDEN				0x080
> +#define EMC_ACT2PDEN				0x084
> +#define EMC_AR2PDEN				0x088
> +#define EMC_RW2PDEN				0x08c
> +#define EMC_TXSR				0x090
> +#define EMC_TCKE				0x094
> +#define EMC_TFAW				0x098
> +#define EMC_TRPAB				0x09c
> +#define EMC_TCLKSTABLE				0x0a0
> +#define EMC_TCLKSTOP				0x0a4
> +#define EMC_TREFBW				0x0a8
> +#define EMC_QUSE_EXTRA				0x0ac
> +#define EMC_ODT_WRITE				0x0b0
> +#define EMC_ODT_READ				0x0b4
> +#define EMC_FBIO_CFG5				0x104
> +#define EMC_FBIO_CFG6				0x114
> +#define EMC_AUTO_CAL_INTERVAL			0x2a8
> +#define EMC_CFG_2				0x2b8
> +#define EMC_CFG_DIG_DLL				0x2bc
> +#define EMC_DLL_XFORM_DQS			0x2c0
> +#define EMC_DLL_XFORM_QUSE			0x2c4
> +#define EMC_ZCAL_REF_CNT			0x2e0
> +#define EMC_ZCAL_WAIT_CNT			0x2e4
> +#define EMC_CFG_CLKTRIM_0			0x2d0
> +#define EMC_CFG_CLKTRIM_1			0x2d4
> +#define EMC_CFG_CLKTRIM_2			0x2d8
> +
> +#define EMC_CLKCHANGE_REQ_ENABLE		BIT(0)
> +#define EMC_CLKCHANGE_PD_ENABLE			BIT(1)
> +#define EMC_CLKCHANGE_SR_ENABLE			BIT(2)
> +
> +#define EMC_TIMING_UPDATE			BIT(0)
> +
> +#define EMC_CLKCHANGE_COMPLETE_INT		BIT(4)
> +
> +static const unsigned long emc_timing_registers[] = {
> +	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_WDV,
> +	EMC_QUSE,
> +	EMC_QRST,
> +	EMC_QSAFE,
> +	EMC_RDV,
> +	EMC_REFRESH,
> +	EMC_BURST_REFRESH_NUM,
> +	EMC_PDEX2WR,
> +	EMC_PDEX2RD,
> +	EMC_PCHG2PDEN,
> +	EMC_ACT2PDEN,
> +	EMC_AR2PDEN,
> +	EMC_RW2PDEN,
> +	EMC_TXSR,
> +	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_DLL_XFORM_DQS,
> +	EMC_DLL_XFORM_QUSE,
> +	EMC_ZCAL_REF_CNT,
> +	EMC_ZCAL_WAIT_CNT,
> +	EMC_AUTO_CAL_INTERVAL,
> +	EMC_CFG_CLKTRIM_0,
> +	EMC_CFG_CLKTRIM_1,
> +	EMC_CFG_CLKTRIM_2,
> +};
> +
> +struct emc_timing {
> +	unsigned long rate;
> +	u32 emc_registers_data[ARRAY_SIZE(emc_timing_registers)];
> +};

Nit: this seems like a very long variable name for something that is
really just "values" or "data" written into a set of registers.

> +struct tegra_emc {
> +	struct device *dev;
> +	struct notifier_block clk_nb;
> +	struct clk *backup_clk;
> +	struct clk *emc_mux;
> +	struct clk *pll_m;
> +	struct clk *clk;
> +	void __iomem *regs;
> +
> +	struct completion clk_handshake_complete;
> +	int irq;
> +
> +	struct emc_timing *timings;
> +	unsigned int num_timings;
> +};
> +
> +static irqreturn_t tegra_emc_isr(int irq, void *data)
> +{
> +	struct tegra_emc *emc = data;
> +	u32 intmask = EMC_CLKCHANGE_COMPLETE_INT;
> +	u32 status;
> +
> +	status = readl_relaxed(emc->regs + EMC_INTSTATUS) & intmask;
> +	if (!status)
> +		return IRQ_NONE;
> +
> +	/* clear interrupts */
> +	writel_relaxed(status, emc->regs + EMC_INTSTATUS);

Do we really want to just clear the handshake complete interrupt or do
we want to clear all of them? Perhaps we should also warn if there are
other interrupts that we're not handling? Currently we'd only get some
warning if another interrupt triggered without the handshake complete
one triggering at the same time, but couldn't there be others asserted
along with the handshake complete interrupt? In which case we'd just
be ignoring them. Or perhaps not clearing it would get the ISR run
immediately again and produce the "nobody cared" warning?

> +
> +	/* notify about EMC-CAR handshake completion */
> +	complete(&emc->clk_handshake_complete);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static struct emc_timing *tegra_emc_find_timing(struct tegra_emc *emc,
> +						unsigned long rate)
> +{
> +	struct emc_timing *timing = NULL;
> +	unsigned int i;
> +
> +	for (i = 0; i < emc->num_timings; i++) {
> +		if (emc->timings[i].rate >= rate) {
> +			timing = &emc->timings[i];
> +			break;
> +		}
> +	}
> +
> +	if (!timing) {
> +		dev_err(emc->dev, "no timing for rate %lu\n", rate);
> +		return NULL;
> +	}
> +
> +	return timing;
> +}
> +
> +static int emc_prepare_timing_change(struct tegra_emc *emc, unsigned long rate)
> +{
> +	struct emc_timing *timing = tegra_emc_find_timing(emc, rate);
> +	unsigned int i;
> +
> +	if (!timing)
> +		return -ENOENT;
> +
> +	dev_dbg(emc->dev, "%s: timing rate %lu emc rate %lu\n",
> +		__func__, timing->rate, rate);
> +
> +	/* program shadow registers */
> +	for (i = 0; i < ARRAY_SIZE(timing->emc_registers_data); i++)
> +		writel_relaxed(timing->emc_registers_data[i],
> +			       emc->regs + emc_timing_registers[i]);
> +
> +	/* wait until programming has settled */
> +	readl_relaxed(emc->regs + emc_timing_registers[0]);
> +
> +	if (emc->irq < 0)
> +		writel_relaxed(EMC_CLKCHANGE_COMPLETE_INT,
> +			       emc->regs + EMC_INTMASK);
> +	else
> +		reinit_completion(&emc->clk_handshake_complete);
> +
> +	return 0;
> +}
> +
> +static int emc_complete_timing_change(struct tegra_emc *emc, bool flush)
> +{
> +	long timeout;
> +	u32 value;
> +	int err;
> +
> +	dev_dbg(emc->dev, "%s: flush %d\n", __func__, flush);
> +
> +	if (flush) {
> +		/* manually initiate memory timing update */
> +		writel_relaxed(EMC_TIMING_UPDATE,
> +			       emc->regs + EMC_TIMING_CONTROL);
> +		return 0;
> +	}
> +
> +	if (emc->irq < 0) {
> +		/* poll interrupt status if IRQ isn't available */
> +		err = readl_relaxed_poll_timeout(emc->regs + EMC_INTSTATUS,
> +				value, value & EMC_CLKCHANGE_COMPLETE_INT,
> +				1, 100);
> +		if (err) {
> +			dev_err(emc->dev, "EMC-CAR handshake failed\n");
> +			return -EIO;
> +		}
> +
> +		return 0;
> +	}
> +
> +	timeout = wait_for_completion_timeout(&emc->clk_handshake_complete,
> +					      usecs_to_jiffies(100));
> +	if (timeout == 0) {
> +		dev_err(emc->dev, "EMC handshake failed\n");
> +		return -EIO;
> +	} else if (timeout < 0) {
> +		dev_err(emc->dev, "failed to wait for EMC-CAR handshake: %ld\n",
> +			timeout);
> +		return timeout;
> +	}
> +
> +	return 0;
> +}
> +
> +static int load_one_timing_from_dt(struct tegra_emc *emc,
> +				   struct emc_timing *timing,
> +				   struct device_node *node)
> +{
> +	u32 rate;
> +	int err;
> +
> +	if (!of_device_is_compatible(node, "nvidia,tegra20-emc-table")) {
> +		dev_err(emc->dev, "incompatible DT node \"%s\"\n",
> +			node->name);
> +		return -EINVAL;
> +	}
> +
> +	err = of_property_read_u32(node, "clock-frequency", &rate);
> +	if (err) {
> +		dev_err(emc->dev, "timing %s: failed to read rate: %d\n",
> +			node->name, err);
> +		return err;
> +	}
> +
> +	err = of_property_read_u32_array(node, "nvidia,emc-registers",
> +					 timing->emc_registers_data,
> +					 ARRAY_SIZE(emc_timing_registers));
> +	if (err) {
> +		dev_err(emc->dev,
> +			"timing %s: failed to read emc timing data: %d\n",
> +			node->name, err);
> +		return err;
> +	}
> +
> +	/*
> +	 * The EMC clock rate is twice the bus rate, and the bus rate is
> +	 * measured in kHz.
> +	 */
> +	timing->rate = rate * 2 * 1000;
> +
> +	dev_dbg(emc->dev, "%s: emc rate %ld\n", __func__, timing->rate);

Nit: %lu for timing->rate?

> +
> +	return 0;
> +}
> +
> +static int cmp_timings(const void *_a, const void *_b)
> +{
> +	const struct emc_timing *a = _a;
> +	const struct emc_timing *b = _b;
> +
> +	if (a->rate < b->rate)
> +		return -1;
> +	else if (a->rate == b->rate)
> +		return 0;
> +	else
> +		return 1;

Nit, I tend to 

> +}
> +
> +static int tegra_emc_load_timings_from_dt(struct tegra_emc *emc,
> +					  struct device_node *node)
> +{
> +	struct device_node *child;
> +	struct emc_timing *timing;
> +	int child_count;
> +	int err;
> +
> +	child_count = of_get_child_count(node);

It's unfortunate that of_get_child_count() doesn't return unsigned int,
there's no reason why this would have to be signed.

> +	if (!child_count) {
> +		dev_err(emc->dev, "no memory timings in DT node\n");
> +		return -ENOENT;
> +	}
> +
> +	emc->timings = devm_kcalloc(emc->dev, child_count, sizeof(*timing),
> +				    GFP_KERNEL);
> +	if (!emc->timings)
> +		return -ENOMEM;
> +
> +	emc->num_timings = child_count;
> +	timing = emc->timings;
> +
> +	for_each_child_of_node(node, child) {
> +		err = load_one_timing_from_dt(emc, timing++, child);
> +		if (err) {
> +			of_node_put(child);
> +			return err;
> +		}
> +	}
> +
> +	sort(emc->timings, emc->num_timings, sizeof(*timing), cmp_timings,
> +	     NULL);
> +
> +	return 0;
> +}
> +
> +static struct device_node *
> +tegra_emc_find_node_by_ram_code(struct tegra_emc *emc, u32 ram_code)
> +{
> +	struct device_node *np;
> +	int err;
> +
> +	for_each_child_of_node(emc->dev->of_node, np) {
> +		u32 value;
> +
> +		err = of_property_read_u32(np, "nvidia,ram-code", &value);
> +		if (err || value != ram_code)
> +			continue;
> +
> +		return np;
> +	}
> +
> +	dev_info(emc->dev, "no memory timings for RAM code %u found in DT\n",
> +		 ram_code);

This seems like it should be dev_warn() or perhaps even dev_err() given
that the result of it is the driver failing to probe. dev_info() may go
unnoticed.

> +
> +	return NULL;
> +}
> +
> +static int tegra_emc_clk_change_notify(struct notifier_block *nb,
> +				       unsigned long msg, void *data)
> +{
> +	struct tegra_emc *emc = container_of(nb, struct tegra_emc, clk_nb);
> +	struct clk_notifier_data *cnd = data;
> +	int err;
> +
> +	switch (msg) {
> +	case PRE_RATE_CHANGE:
> +		err = emc_prepare_timing_change(emc, cnd->new_rate);
> +		break;
> +
> +	case ABORT_RATE_CHANGE:
> +		err = emc_prepare_timing_change(emc, cnd->old_rate);
> +		if (err)
> +			break;
> +
> +		err = emc_complete_timing_change(emc, true);
> +		break;
> +
> +	case POST_RATE_CHANGE:
> +		err = emc_complete_timing_change(emc, false);
> +		break;
> +
> +	default:
> +		return NOTIFY_DONE;
> +	}
> +
> +	return notifier_from_errno(err);
> +}
> +
> +static int emc_setup_hw(struct tegra_emc *emc)
> +{
> +	u32 emc_cfg;
> +
> +	emc_cfg = readl_relaxed(emc->regs + EMC_CFG_2);
> +
> +	/*
> +	 * Depending on a memory type, DRAM should enter either self-refresh
> +	 * or power-down state on EMC clock change.
> +	 */
> +	if (!(emc_cfg & EMC_CLKCHANGE_PD_ENABLE) &&
> +	    !(emc_cfg & EMC_CLKCHANGE_SR_ENABLE))
> +	{
> +		dev_err(emc->dev,
> +			"bootloader didn't specify DRAM auto-suspend mode\n");
> +		return -EINVAL;
> +	}
> +
> +	/* allow EMC and CAR to handshake on PLL divider/source changes */
> +	emc_cfg |= EMC_CLKCHANGE_REQ_ENABLE;
> +	writel_relaxed(emc_cfg, emc->regs + EMC_CFG_2);
> +
> +	/* initialize interrupt */
> +	writel_relaxed(EMC_CLKCHANGE_COMPLETE_INT, emc->regs + EMC_INTMASK);
> +	writel_relaxed(EMC_CLKCHANGE_COMPLETE_INT, emc->regs + EMC_INTSTATUS);
> +
> +	return 0;
> +}
> +
> +static int emc_init(struct tegra_emc *emc, unsigned long rate)
> +{
> +	int err;
> +
> +	err = clk_set_parent(emc->emc_mux, emc->backup_clk);
> +	if (err) {
> +		dev_err(emc->dev,
> +			"failed to reparent to backup source: %d\n", err);
> +		return err;
> +	}
> +
> +	err = clk_set_rate(emc->pll_m, rate);
> +	if (err)
> +		dev_err(emc->dev,
> +			"failed to change pll_m rate: %d\n", err);
> +
> +	err = clk_set_parent(emc->emc_mux, emc->pll_m);
> +	if (err) {
> +		dev_err(emc->dev,
> +			"failed to reparent to pll_m: %d\n", err);
> +		return err;
> +	}
> +
> +	return 0;
> +}
> +
> +static int tegra_emc_probe(struct platform_device *pdev)
> +{
> +	struct device_node *np;
> +	struct tegra_emc *emc;
> +	struct resource *res;
> +	u32 ram_code;
> +	int err;
> +
> +	emc = devm_kzalloc(&pdev->dev, sizeof(*emc), GFP_KERNEL);
> +	if (!emc)
> +		return -ENOMEM;
> +
> +	emc->dev = &pdev->dev;
> +
> +	ram_code = tegra_read_ram_code();
> +
> +	np = tegra_emc_find_node_by_ram_code(emc, ram_code);
> +	if (!np)
> +		return -ENOENT;
> +
> +	err = tegra_emc_load_timings_from_dt(emc, np);
> +	of_node_put(np);
> +	if (err)
> +		return err;
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	emc->regs = devm_ioremap_resource(&pdev->dev, res);
> +	if (IS_ERR(emc->regs))
> +		return PTR_ERR(emc->regs);
> +
> +	err = emc_setup_hw(emc);
> +	if (err)
> +		return err;
> +
> +	emc->irq = platform_get_irq(pdev, 0);
> +	if (emc->irq < 0) {
> +		dev_warn(&pdev->dev, "interrupt not specified\n");
> +		dev_warn(&pdev->dev, "continuing, but please update your DT\n");

Do we really need this? I think this is a case where we don't have to
keep backwards-compatibility because this driver hasn't "worked" in a
very long time (because it was absent). Therefore, if we error out in
the absence of an interrupt we don't break anything.

There's a few places in this driver that are awkward just because the
interrupt isn't mandatory. I don't think it's warranted in this case.

> +	} else {
> +		init_completion(&emc->clk_handshake_complete);
> +
> +		err = devm_request_irq(&pdev->dev, emc->irq, tegra_emc_isr, 0,
> +				       dev_name(&pdev->dev), emc);
> +		if (err < 0) {
> +			dev_err(&pdev->dev, "failed to request IRQ#%u: %d\n",
> +				emc->irq, err);
> +			return err;
> +		}
> +	}
> +
> +	emc->pll_m = clk_get_sys(NULL, "pll_m");
> +	if (IS_ERR(emc->pll_m)) {
> +		err = PTR_ERR(emc->pll_m);
> +		dev_err(&pdev->dev, "failed to get pll_m: %d\n", err);
> +		return err;
> +	}
> +
> +	emc->backup_clk = clk_get_sys(NULL, "pll_p");
> +	if (IS_ERR(emc->backup_clk)) {
> +		err = PTR_ERR(emc->backup_clk);
> +		dev_err(&pdev->dev, "failed to get pll_p: %d\n", err);
> +		goto put_pll_m;
> +	}
> +
> +	emc->clk = clk_get_sys(NULL, "emc");
> +	if (IS_ERR(emc->clk)) {
> +		err = PTR_ERR(emc->clk);
> +		dev_err(&pdev->dev, "failed to get emc: %d\n", err);
> +		goto put_backup;
> +	}

Instead of using clk_get_sys(), why not specify these in the DT with
proper names for context ("emc", "pll", "backup")? Again, I don't think
we have to worry about backwards-compatibility here since there can be
no regression.

> +
> +	emc->emc_mux = clk_get_parent(emc->clk);
> +	if (IS_ERR(emc->emc_mux)) {
> +		err = PTR_ERR(emc->emc_mux);
> +		dev_err(&pdev->dev, "failed to get emc_mux: %d\n", err);
> +		goto put_emc;
> +	}
> +
> +	emc->clk_nb.notifier_call = tegra_emc_clk_change_notify;
> +
> +	err = clk_notifier_register(emc->clk, &emc->clk_nb);
> +	if (err) {
> +		dev_err(&pdev->dev, "failed to register clk notifier: %d\n",
> +			err);
> +		goto put_emc;
> +	}
> +
> +	/* set DRAM clock rate to maximum */
> +	err = emc_init(emc, emc->timings[emc->num_timings - 1].rate);
> +	if (err) {
> +		dev_err(&pdev->dev, "failed to initialize clk rate: %d\n",
> +			err);
> +		goto unreg_notifier;
> +	}
> +
> +	return 0;
> +
> +unreg_notifier:
> +	clk_notifier_unregister(emc->emc_mux, &emc->clk_nb);
> +put_emc:
> +	clk_put(emc->clk);
> +put_backup:
> +	clk_put(emc->backup_clk);
> +put_pll_m:
> +	clk_put(emc->pll_m);
> +
> +	return err;
> +}
> +
> +static const struct of_device_id tegra_emc_of_match[] = {
> +	{ .compatible = "nvidia,tegra20-emc", },
> +	{},
> +};
> +
> +static struct platform_driver tegra_emc_driver = {
> +	.probe = tegra_emc_probe,
> +	.driver = {
> +		.name = "tegra20-emc",
> +		.of_match_table = tegra_emc_of_match,
> +		.suppress_bind_attrs = true,
> +	},
> +};
> +
> +static int __init tegra_emc_init(void)
> +{
> +	return platform_driver_register(&tegra_emc_driver);
> +}
> +subsys_initcall(tegra_emc_init);
> -- 
> 2.17.0

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 5/5] memory: tegra: Introduce Tegra20 EMC driver

[Dmitry Osipenko]

On 06.06.2018 14:02, Thierry Reding wrote:
> On Mon, Jun 04, 2018 at 01:36:54AM +0300, Dmitry Osipenko wrote:
>> Introduce driver for the External Memory Controller (EMC) found on Tegra20
>> chips, which 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 <digetx@gmail.com>
>> ---
>>  drivers/memory/tegra/Kconfig       |  10 +
>>  drivers/memory/tegra/Makefile      |   1 +
>>  drivers/memory/tegra/tegra20-emc.c | 586 +++++++++++++++++++++++++++++
>>  3 files changed, 597 insertions(+)
>>  create mode 100644 drivers/memory/tegra/tegra20-emc.c
>>
>> diff --git a/drivers/memory/tegra/Kconfig b/drivers/memory/tegra/Kconfig
>> index 6d74e499e18d..34e0b70f5c5f 100644
>> --- a/drivers/memory/tegra/Kconfig
>> +++ b/drivers/memory/tegra/Kconfig
>> @@ -6,6 +6,16 @@ config TEGRA_MC
>>  	  This driver supports the Memory Controller (MC) hardware found on
>>  	  NVIDIA Tegra SoCs.
>>  
>> +config TEGRA20_EMC
>> +	bool "NVIDIA Tegra20 External Memory Controller driver"
>> +	default y
>> +	depends on ARCH_TEGRA_2x_SOC
>> +	help
>> +	  This driver is for the External Memory Controller (EMC) found on
>> +	  Tegra20 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
>> diff --git a/drivers/memory/tegra/Makefile b/drivers/memory/tegra/Makefile
>> index 94ab16ba075b..3971a6b7c487 100644
>> --- a/drivers/memory/tegra/Makefile
>> +++ b/drivers/memory/tegra/Makefile
>> @@ -10,5 +10,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_TEGRA124_EMC) += tegra124-emc.o
>>  obj-$(CONFIG_ARCH_TEGRA_186_SOC) += tegra186.o
>> diff --git a/drivers/memory/tegra/tegra20-emc.c b/drivers/memory/tegra/tegra20-emc.c
>> new file mode 100644
>> index 000000000000..26a18b5e7941
>> --- /dev/null
>> +++ b/drivers/memory/tegra/tegra20-emc.c
>> @@ -0,0 +1,586 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Tegra20 External Memory Controller driver
>> + *
>> + * Author: Dmitry Osipenko <digetx@gmail.com>
>> + */
>> +
>> +#include <linux/clk.h>
>> +#include <linux/completion.h>
>> +#include <linux/err.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/iopoll.h>
>> +#include <linux/kernel.h>
>> +#include <linux/module.h>
>> +#include <linux/of.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/sort.h>
>> +#include <linux/types.h>
>> +
>> +#include <soc/tegra/fuse.h>
>> +
>> +#define EMC_INTSTATUS				0x000
>> +#define EMC_INTMASK				0x004
>> +#define EMC_TIMING_CONTROL			0x028
>> +#define EMC_RC					0x02c
>> +#define EMC_RFC					0x030
>> +#define EMC_RAS					0x034
>> +#define EMC_RP					0x038
>> +#define EMC_R2W					0x03c
>> +#define EMC_W2R					0x040
>> +#define EMC_R2P					0x044
>> +#define EMC_W2P					0x048
>> +#define EMC_RD_RCD				0x04c
>> +#define EMC_WR_RCD				0x050
>> +#define EMC_RRD					0x054
>> +#define EMC_REXT				0x058
>> +#define EMC_WDV					0x05c
>> +#define EMC_QUSE				0x060
>> +#define EMC_QRST				0x064
>> +#define EMC_QSAFE				0x068
>> +#define EMC_RDV					0x06c
>> +#define EMC_REFRESH				0x070
>> +#define EMC_BURST_REFRESH_NUM			0x074
>> +#define EMC_PDEX2WR				0x078
>> +#define EMC_PDEX2RD				0x07c
>> +#define EMC_PCHG2PDEN				0x080
>> +#define EMC_ACT2PDEN				0x084
>> +#define EMC_AR2PDEN				0x088
>> +#define EMC_RW2PDEN				0x08c
>> +#define EMC_TXSR				0x090
>> +#define EMC_TCKE				0x094
>> +#define EMC_TFAW				0x098
>> +#define EMC_TRPAB				0x09c
>> +#define EMC_TCLKSTABLE				0x0a0
>> +#define EMC_TCLKSTOP				0x0a4
>> +#define EMC_TREFBW				0x0a8
>> +#define EMC_QUSE_EXTRA				0x0ac
>> +#define EMC_ODT_WRITE				0x0b0
>> +#define EMC_ODT_READ				0x0b4
>> +#define EMC_FBIO_CFG5				0x104
>> +#define EMC_FBIO_CFG6				0x114
>> +#define EMC_AUTO_CAL_INTERVAL			0x2a8
>> +#define EMC_CFG_2				0x2b8
>> +#define EMC_CFG_DIG_DLL				0x2bc
>> +#define EMC_DLL_XFORM_DQS			0x2c0
>> +#define EMC_DLL_XFORM_QUSE			0x2c4
>> +#define EMC_ZCAL_REF_CNT			0x2e0
>> +#define EMC_ZCAL_WAIT_CNT			0x2e4
>> +#define EMC_CFG_CLKTRIM_0			0x2d0
>> +#define EMC_CFG_CLKTRIM_1			0x2d4
>> +#define EMC_CFG_CLKTRIM_2			0x2d8
>> +
>> +#define EMC_CLKCHANGE_REQ_ENABLE		BIT(0)
>> +#define EMC_CLKCHANGE_PD_ENABLE			BIT(1)
>> +#define EMC_CLKCHANGE_SR_ENABLE			BIT(2)
>> +
>> +#define EMC_TIMING_UPDATE			BIT(0)
>> +
>> +#define EMC_CLKCHANGE_COMPLETE_INT		BIT(4)
>> +
>> +static const unsigned long emc_timing_registers[] = {
>> +	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_WDV,
>> +	EMC_QUSE,
>> +	EMC_QRST,
>> +	EMC_QSAFE,
>> +	EMC_RDV,
>> +	EMC_REFRESH,
>> +	EMC_BURST_REFRESH_NUM,
>> +	EMC_PDEX2WR,
>> +	EMC_PDEX2RD,
>> +	EMC_PCHG2PDEN,
>> +	EMC_ACT2PDEN,
>> +	EMC_AR2PDEN,
>> +	EMC_RW2PDEN,
>> +	EMC_TXSR,
>> +	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_DLL_XFORM_DQS,
>> +	EMC_DLL_XFORM_QUSE,
>> +	EMC_ZCAL_REF_CNT,
>> +	EMC_ZCAL_WAIT_CNT,
>> +	EMC_AUTO_CAL_INTERVAL,
>> +	EMC_CFG_CLKTRIM_0,
>> +	EMC_CFG_CLKTRIM_1,
>> +	EMC_CFG_CLKTRIM_2,
>> +};
>> +
>> +struct emc_timing {
>> +	unsigned long rate;
>> +	u32 emc_registers_data[ARRAY_SIZE(emc_timing_registers)];
>> +};
> 
> Nit: this seems like a very long variable name for something that is
> really just "values" or "data" written into a set of registers.
> 

Ok.

>> +struct tegra_emc {
>> +	struct device *dev;
>> +	struct notifier_block clk_nb;
>> +	struct clk *backup_clk;
>> +	struct clk *emc_mux;
>> +	struct clk *pll_m;
>> +	struct clk *clk;
>> +	void __iomem *regs;
>> +
>> +	struct completion clk_handshake_complete;
>> +	int irq;
>> +
>> +	struct emc_timing *timings;
>> +	unsigned int num_timings;
>> +};
>> +
>> +static irqreturn_t tegra_emc_isr(int irq, void *data)
>> +{
>> +	struct tegra_emc *emc = data;
>> +	u32 intmask = EMC_CLKCHANGE_COMPLETE_INT;
>> +	u32 status;
>> +
>> +	status = readl_relaxed(emc->regs + EMC_INTSTATUS) & intmask;
>> +	if (!status)
>> +		return IRQ_NONE;
>> +
>> +	/* clear interrupts */
>> +	writel_relaxed(status, emc->regs + EMC_INTSTATUS);
> 
> Do we really want to just clear the handshake complete interrupt or do
> we want to clear all of them? Perhaps we should also warn if there are
> other interrupts that we're not handling? Currently we'd only get some
> warning if another interrupt triggered without the handshake complete
> one triggering at the same time, but couldn't there be others asserted
> along with the handshake complete interrupt? In which case we'd just
> be ignoring them. Or perhaps not clearing it would get the ISR run
> immediately again and produce the "nobody cared" warning?
> 

Yes, we really want to just clear the handshake-complete interrupt. No, we
shouldn't warn about other interrupts because IRQ subsys does it for us. Other
interrupts shouldn't be asserted because we disabled them with the interrupts
masking in emc_setup_hw(). Other interrupts can only be asserted in a case of a
bug, there will be a "nobody cared" warning and interrupt will be disabled, this
is exactly what we want in that case.

>> +
>> +	/* notify about EMC-CAR handshake completion */
>> +	complete(&emc->clk_handshake_complete);
>> +
>> +	return IRQ_HANDLED;
>> +}
>> +
>> +static struct emc_timing *tegra_emc_find_timing(struct tegra_emc *emc,
>> +						unsigned long rate)
>> +{
>> +	struct emc_timing *timing = NULL;
>> +	unsigned int i;
>> +
>> +	for (i = 0; i < emc->num_timings; i++) {
>> +		if (emc->timings[i].rate >= rate) {
>> +			timing = &emc->timings[i];
>> +			break;
>> +		}
>> +	}
>> +
>> +	if (!timing) {
>> +		dev_err(emc->dev, "no timing for rate %lu\n", rate);
>> +		return NULL;
>> +	}
>> +
>> +	return timing;
>> +}
>> +
>> +static int emc_prepare_timing_change(struct tegra_emc *emc, unsigned long rate)
>> +{
>> +	struct emc_timing *timing = tegra_emc_find_timing(emc, rate);
>> +	unsigned int i;
>> +
>> +	if (!timing)
>> +		return -ENOENT;
>> +
>> +	dev_dbg(emc->dev, "%s: timing rate %lu emc rate %lu\n",
>> +		__func__, timing->rate, rate);
>> +
>> +	/* program shadow registers */
>> +	for (i = 0; i < ARRAY_SIZE(timing->emc_registers_data); i++)
>> +		writel_relaxed(timing->emc_registers_data[i],
>> +			       emc->regs + emc_timing_registers[i]);
>> +
>> +	/* wait until programming has settled */
>> +	readl_relaxed(emc->regs + emc_timing_registers[0]);
>> +
>> +	if (emc->irq < 0)
>> +		writel_relaxed(EMC_CLKCHANGE_COMPLETE_INT,
>> +			       emc->regs + EMC_INTMASK);
>> +	else
>> +		reinit_completion(&emc->clk_handshake_complete);
>> +
>> +	return 0;
>> +}
>> +
>> +static int emc_complete_timing_change(struct tegra_emc *emc, bool flush)
>> +{
>> +	long timeout;
>> +	u32 value;
>> +	int err;
>> +
>> +	dev_dbg(emc->dev, "%s: flush %d\n", __func__, flush);
>> +
>> +	if (flush) {
>> +		/* manually initiate memory timing update */
>> +		writel_relaxed(EMC_TIMING_UPDATE,
>> +			       emc->regs + EMC_TIMING_CONTROL);
>> +		return 0;
>> +	}
>> +
>> +	if (emc->irq < 0) {
>> +		/* poll interrupt status if IRQ isn't available */
>> +		err = readl_relaxed_poll_timeout(emc->regs + EMC_INTSTATUS,
>> +				value, value & EMC_CLKCHANGE_COMPLETE_INT,
>> +				1, 100);
>> +		if (err) {
>> +			dev_err(emc->dev, "EMC-CAR handshake failed\n");
>> +			return -EIO;
>> +		}
>> +
>> +		return 0;
>> +	}
>> +
>> +	timeout = wait_for_completion_timeout(&emc->clk_handshake_complete,
>> +					      usecs_to_jiffies(100));
>> +	if (timeout == 0) {
>> +		dev_err(emc->dev, "EMC handshake failed\n");
>> +		return -EIO;
>> +	} else if (timeout < 0) {
>> +		dev_err(emc->dev, "failed to wait for EMC-CAR handshake: %ld\n",
>> +			timeout);
>> +		return timeout;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static int load_one_timing_from_dt(struct tegra_emc *emc,
>> +				   struct emc_timing *timing,
>> +				   struct device_node *node)
>> +{
>> +	u32 rate;
>> +	int err;
>> +
>> +	if (!of_device_is_compatible(node, "nvidia,tegra20-emc-table")) {
>> +		dev_err(emc->dev, "incompatible DT node \"%s\"\n",
>> +			node->name);
>> +		return -EINVAL;
>> +	}
>> +
>> +	err = of_property_read_u32(node, "clock-frequency", &rate);
>> +	if (err) {
>> +		dev_err(emc->dev, "timing %s: failed to read rate: %d\n",
>> +			node->name, err);
>> +		return err;
>> +	}
>> +
>> +	err = of_property_read_u32_array(node, "nvidia,emc-registers",
>> +					 timing->emc_registers_data,
>> +					 ARRAY_SIZE(emc_timing_registers));
>> +	if (err) {
>> +		dev_err(emc->dev,
>> +			"timing %s: failed to read emc timing data: %d\n",
>> +			node->name, err);
>> +		return err;
>> +	}
>> +
>> +	/*
>> +	 * The EMC clock rate is twice the bus rate, and the bus rate is
>> +	 * measured in kHz.
>> +	 */
>> +	timing->rate = rate * 2 * 1000;
>> +
>> +	dev_dbg(emc->dev, "%s: emc rate %ld\n", __func__, timing->rate);
> 
> Nit: %lu for timing->rate?
> 

Yes.

>> +
>> +	return 0;
>> +}
>> +
>> +static int cmp_timings(const void *_a, const void *_b)
>> +{
>> +	const struct emc_timing *a = _a;
>> +	const struct emc_timing *b = _b;
>> +
>> +	if (a->rate < b->rate)
>> +		return -1;
>> +	else if (a->rate == b->rate)
>> +		return 0;
>> +	else
>> +		return 1;
> 
> Nit, I tend to 
> 

Tend to..?

>> +}
>> +
>> +static int tegra_emc_load_timings_from_dt(struct tegra_emc *emc,
>> +					  struct device_node *node)
>> +{
>> +	struct device_node *child;
>> +	struct emc_timing *timing;
>> +	int child_count;
>> +	int err;
>> +
>> +	child_count = of_get_child_count(node);
> 
> It's unfortunate that of_get_child_count() doesn't return unsigned int,
> there's no reason why this would have to be signed.
> 

Patches are welcome ;)

>> +	if (!child_count) {
>> +		dev_err(emc->dev, "no memory timings in DT node\n");
>> +		return -ENOENT;
>> +	}
>> +
>> +	emc->timings = devm_kcalloc(emc->dev, child_count, sizeof(*timing),
>> +				    GFP_KERNEL);
>> +	if (!emc->timings)
>> +		return -ENOMEM;
>> +
>> +	emc->num_timings = child_count;
>> +	timing = emc->timings;
>> +
>> +	for_each_child_of_node(node, child) {
>> +		err = load_one_timing_from_dt(emc, timing++, child);
>> +		if (err) {
>> +			of_node_put(child);
>> +			return err;
>> +		}
>> +	}
>> +
>> +	sort(emc->timings, emc->num_timings, sizeof(*timing), cmp_timings,
>> +	     NULL);
>> +
>> +	return 0;
>> +}
>> +
>> +static struct device_node *
>> +tegra_emc_find_node_by_ram_code(struct tegra_emc *emc, u32 ram_code)
>> +{
>> +	struct device_node *np;
>> +	int err;
>> +
>> +	for_each_child_of_node(emc->dev->of_node, np) {
>> +		u32 value;
>> +
>> +		err = of_property_read_u32(np, "nvidia,ram-code", &value);
>> +		if (err || value != ram_code)
>> +			continue;
>> +
>> +		return np;
>> +	}
>> +
>> +	dev_info(emc->dev, "no memory timings for RAM code %u found in DT\n",
>> +		 ram_code);
> 
> This seems like it should be dev_warn() or perhaps even dev_err() given
> that the result of it is the driver failing to probe. dev_info() may go
> unnoticed.
> 

Absence of memory timings is a valid case, hence dev_info() suit well here.

I can't see anything wrong with returning a errno if driver has nothing to do
and prefer to keep it because in that case managed resources would be free'd by
the driver core, though returning '0' also would work.

>> +
>> +	return NULL;
>> +}
>> +
>> +static int tegra_emc_clk_change_notify(struct notifier_block *nb,
>> +				       unsigned long msg, void *data)
>> +{
>> +	struct tegra_emc *emc = container_of(nb, struct tegra_emc, clk_nb);
>> +	struct clk_notifier_data *cnd = data;
>> +	int err;
>> +
>> +	switch (msg) {
>> +	case PRE_RATE_CHANGE:
>> +		err = emc_prepare_timing_change(emc, cnd->new_rate);
>> +		break;
>> +
>> +	case ABORT_RATE_CHANGE:
>> +		err = emc_prepare_timing_change(emc, cnd->old_rate);
>> +		if (err)
>> +			break;
>> +
>> +		err = emc_complete_timing_change(emc, true);
>> +		break;
>> +
>> +	case POST_RATE_CHANGE:
>> +		err = emc_complete_timing_change(emc, false);
>> +		break;
>> +
>> +	default:
>> +		return NOTIFY_DONE;
>> +	}
>> +
>> +	return notifier_from_errno(err);
>> +}
>> +
>> +static int emc_setup_hw(struct tegra_emc *emc)
>> +{
>> +	u32 emc_cfg;
>> +
>> +	emc_cfg = readl_relaxed(emc->regs + EMC_CFG_2);
>> +
>> +	/*
>> +	 * Depending on a memory type, DRAM should enter either self-refresh
>> +	 * or power-down state on EMC clock change.
>> +	 */
>> +	if (!(emc_cfg & EMC_CLKCHANGE_PD_ENABLE) &&
>> +	    !(emc_cfg & EMC_CLKCHANGE_SR_ENABLE))
>> +	{
>> +		dev_err(emc->dev,
>> +			"bootloader didn't specify DRAM auto-suspend mode\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	/* allow EMC and CAR to handshake on PLL divider/source changes */
>> +	emc_cfg |= EMC_CLKCHANGE_REQ_ENABLE;
>> +	writel_relaxed(emc_cfg, emc->regs + EMC_CFG_2);
>> +
>> +	/* initialize interrupt */
>> +	writel_relaxed(EMC_CLKCHANGE_COMPLETE_INT, emc->regs + EMC_INTMASK);
>> +	writel_relaxed(EMC_CLKCHANGE_COMPLETE_INT, emc->regs + EMC_INTSTATUS);
>> +
>> +	return 0;
>> +}
>> +
>> +static int emc_init(struct tegra_emc *emc, unsigned long rate)
>> +{
>> +	int err;
>> +
>> +	err = clk_set_parent(emc->emc_mux, emc->backup_clk);
>> +	if (err) {
>> +		dev_err(emc->dev,
>> +			"failed to reparent to backup source: %d\n", err);
>> +		return err;
>> +	}
>> +
>> +	err = clk_set_rate(emc->pll_m, rate);
>> +	if (err)
>> +		dev_err(emc->dev,
>> +			"failed to change pll_m rate: %d\n", err);
>> +
>> +	err = clk_set_parent(emc->emc_mux, emc->pll_m);
>> +	if (err) {
>> +		dev_err(emc->dev,
>> +			"failed to reparent to pll_m: %d\n", err);
>> +		return err;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static int tegra_emc_probe(struct platform_device *pdev)
>> +{
>> +	struct device_node *np;
>> +	struct tegra_emc *emc;
>> +	struct resource *res;
>> +	u32 ram_code;
>> +	int err;
>> +
>> +	emc = devm_kzalloc(&pdev->dev, sizeof(*emc), GFP_KERNEL);
>> +	if (!emc)
>> +		return -ENOMEM;
>> +
>> +	emc->dev = &pdev->dev;
>> +
>> +	ram_code = tegra_read_ram_code();
>> +
>> +	np = tegra_emc_find_node_by_ram_code(emc, ram_code);
>> +	if (!np)
>> +		return -ENOENT;
>> +
>> +	err = tegra_emc_load_timings_from_dt(emc, np);
>> +	of_node_put(np);
>> +	if (err)
>> +		return err;
>> +
>> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> +	emc->regs = devm_ioremap_resource(&pdev->dev, res);
>> +	if (IS_ERR(emc->regs))
>> +		return PTR_ERR(emc->regs);
>> +
>> +	err = emc_setup_hw(emc);
>> +	if (err)
>> +		return err;
>> +
>> +	emc->irq = platform_get_irq(pdev, 0);
>> +	if (emc->irq < 0) {
>> +		dev_warn(&pdev->dev, "interrupt not specified\n");
>> +		dev_warn(&pdev->dev, "continuing, but please update your DT\n");
> 
> Do we really need this? I think this is a case where we don't have to
> keep backwards-compatibility because this driver hasn't "worked" in a
> very long time (because it was absent). Therefore, if we error out in
> the absence of an interrupt we don't break anything.
> 
> There's a few places in this driver that are awkward just because the
> interrupt isn't mandatory. I don't think it's warranted in this case.
> 

Backwards compatibility is always nice to have, but I don't really mind dropping it.

>> +	} else {
>> +		init_completion(&emc->clk_handshake_complete);
>> +
>> +		err = devm_request_irq(&pdev->dev, emc->irq, tegra_emc_isr, 0,
>> +				       dev_name(&pdev->dev), emc);
>> +		if (err < 0) {
>> +			dev_err(&pdev->dev, "failed to request IRQ#%u: %d\n",
>> +				emc->irq, err);
>> +			return err;
>> +		}
>> +	}
>> +
>> +	emc->pll_m = clk_get_sys(NULL, "pll_m");
>> +	if (IS_ERR(emc->pll_m)) {
>> +		err = PTR_ERR(emc->pll_m);
>> +		dev_err(&pdev->dev, "failed to get pll_m: %d\n", err);
>> +		return err;
>> +	}
>> +
>> +	emc->backup_clk = clk_get_sys(NULL, "pll_p");
>> +	if (IS_ERR(emc->backup_clk)) {
>> +		err = PTR_ERR(emc->backup_clk);
>> +		dev_err(&pdev->dev, "failed to get pll_p: %d\n", err);
>> +		goto put_pll_m;
>> +	}
>> +
>> +	emc->clk = clk_get_sys(NULL, "emc");
>> +	if (IS_ERR(emc->clk)) {
>> +		err = PTR_ERR(emc->clk);
>> +		dev_err(&pdev->dev, "failed to get emc: %d\n", err);
>> +		goto put_backup;
>> +	}
> 
> Instead of using clk_get_sys(), why not specify these in the DT with
> proper names for context ("emc", "pll", "backup")? Again, I don't think
> we have to worry about backwards-compatibility here since there can be
> no regression.
> 

I don't think that "pll" and "backup" could be placed in DT because it is a pure
software-driver description.

"emc" could be retrieved from DT if we don't care about backwards compatibility.

Re: [PATCH v2 5/5] memory: tegra: Introduce Tegra20 EMC driver

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 12670 bytes --]

On Wed, Jun 06, 2018 at 04:42:01PM +0300, Dmitry Osipenko wrote:
> On 06.06.2018 14:02, Thierry Reding wrote:
> > On Mon, Jun 04, 2018 at 01:36:54AM +0300, Dmitry Osipenko wrote:
[...]
> >> +struct tegra_emc {
> >> +	struct device *dev;
> >> +	struct notifier_block clk_nb;
> >> +	struct clk *backup_clk;
> >> +	struct clk *emc_mux;
> >> +	struct clk *pll_m;
> >> +	struct clk *clk;
> >> +	void __iomem *regs;
> >> +
> >> +	struct completion clk_handshake_complete;
> >> +	int irq;
> >> +
> >> +	struct emc_timing *timings;
> >> +	unsigned int num_timings;
> >> +};
> >> +
> >> +static irqreturn_t tegra_emc_isr(int irq, void *data)
> >> +{
> >> +	struct tegra_emc *emc = data;
> >> +	u32 intmask = EMC_CLKCHANGE_COMPLETE_INT;
> >> +	u32 status;
> >> +
> >> +	status = readl_relaxed(emc->regs + EMC_INTSTATUS) & intmask;
> >> +	if (!status)
> >> +		return IRQ_NONE;
> >> +
> >> +	/* clear interrupts */
> >> +	writel_relaxed(status, emc->regs + EMC_INTSTATUS);
> > 
> > Do we really want to just clear the handshake complete interrupt or do
> > we want to clear all of them? Perhaps we should also warn if there are
> > other interrupts that we're not handling? Currently we'd only get some
> > warning if another interrupt triggered without the handshake complete
> > one triggering at the same time, but couldn't there be others asserted
> > along with the handshake complete interrupt? In which case we'd just
> > be ignoring them. Or perhaps not clearing it would get the ISR run
> > immediately again and produce the "nobody cared" warning?
> > 
> 
> Yes, we really want to just clear the handshake-complete interrupt. No, we
> shouldn't warn about other interrupts because IRQ subsys does it for us. Other
> interrupts shouldn't be asserted because we disabled them with the interrupts
> masking in emc_setup_hw(). Other interrupts can only be asserted in a case of a
> bug, there will be a "nobody cared" warning and interrupt will be disabled, this
> is exactly what we want in that case.

Okay, that's what I was suspecting might happen. Seems fine, then.

> >> +static int cmp_timings(const void *_a, const void *_b)
> >> +{
> >> +	const struct emc_timing *a = _a;
> >> +	const struct emc_timing *b = _b;
> >> +
> >> +	if (a->rate < b->rate)
> >> +		return -1;
> >> +	else if (a->rate == b->rate)
> >> +		return 0;
> >> +	else
> >> +		return 1;
> > 
> > Nit, I tend to 
> > 
> 
> Tend to..?

Looks like I got distracted at this point. =) What I meant to say is
that I tend to not use if ... else if ... else for things that do
immediately return. The above is equivalent to the below, which I think
is much easier to read:

	if (a->rate < b->rate)
		return -1;

	if (a->rate > b->rate)
		return 1;

	return 0;

> >> +}
> >> +
> >> +static int tegra_emc_load_timings_from_dt(struct tegra_emc *emc,
> >> +					  struct device_node *node)
> >> +{
> >> +	struct device_node *child;
> >> +	struct emc_timing *timing;
> >> +	int child_count;
> >> +	int err;
> >> +
> >> +	child_count = of_get_child_count(node);
> > 
> > It's unfortunate that of_get_child_count() doesn't return unsigned int,
> > there's no reason why this would have to be signed.
> > 
> 
> Patches are welcome ;)

Yeah, just a random drive-by comment. Please ignore. =)

> 
> >> +	if (!child_count) {
> >> +		dev_err(emc->dev, "no memory timings in DT node\n");
> >> +		return -ENOENT;
> >> +	}
> >> +
> >> +	emc->timings = devm_kcalloc(emc->dev, child_count, sizeof(*timing),
> >> +				    GFP_KERNEL);
> >> +	if (!emc->timings)
> >> +		return -ENOMEM;
> >> +
> >> +	emc->num_timings = child_count;
> >> +	timing = emc->timings;
> >> +
> >> +	for_each_child_of_node(node, child) {
> >> +		err = load_one_timing_from_dt(emc, timing++, child);
> >> +		if (err) {
> >> +			of_node_put(child);
> >> +			return err;
> >> +		}
> >> +	}
> >> +
> >> +	sort(emc->timings, emc->num_timings, sizeof(*timing), cmp_timings,
> >> +	     NULL);
> >> +
> >> +	return 0;
> >> +}
> >> +
> >> +static struct device_node *
> >> +tegra_emc_find_node_by_ram_code(struct tegra_emc *emc, u32 ram_code)
> >> +{
> >> +	struct device_node *np;
> >> +	int err;
> >> +
> >> +	for_each_child_of_node(emc->dev->of_node, np) {
> >> +		u32 value;
> >> +
> >> +		err = of_property_read_u32(np, "nvidia,ram-code", &value);
> >> +		if (err || value != ram_code)
> >> +			continue;
> >> +
> >> +		return np;
> >> +	}
> >> +
> >> +	dev_info(emc->dev, "no memory timings for RAM code %u found in DT\n",
> >> +		 ram_code);
> > 
> > This seems like it should be dev_warn() or perhaps even dev_err() given
> > that the result of it is the driver failing to probe. dev_info() may go
> > unnoticed.
> > 
> 
> Absence of memory timings is a valid case, hence dev_info() suit well here.
> 
> I can't see anything wrong with returning a errno if driver has nothing to do
> and prefer to keep it because in that case managed resources would be free'd by
> the driver core, though returning '0' also would work.

I disagree. A driver failing to probe will show up as a kernel log entry
and is something that people will have to whitelist if they're filtering
for error messages in the boot log.

I think it's more user-friendly to just let the driver succeed the probe
in an expected case, even if that means there's really nothing to do. If
you're really concerned about the managed resources staying around, I
think you could probably get rid of them explicitly. By the looks of it
devres_release_all() isn't an exported symbol, so it can't be called
from driver code, but perhaps that's something that we can change.

> 
> >> +
> >> +	return NULL;
> >> +}
> >> +
> >> +static int tegra_emc_clk_change_notify(struct notifier_block *nb,
> >> +				       unsigned long msg, void *data)
> >> +{
> >> +	struct tegra_emc *emc = container_of(nb, struct tegra_emc, clk_nb);
> >> +	struct clk_notifier_data *cnd = data;
> >> +	int err;
> >> +
> >> +	switch (msg) {
> >> +	case PRE_RATE_CHANGE:
> >> +		err = emc_prepare_timing_change(emc, cnd->new_rate);
> >> +		break;
> >> +
> >> +	case ABORT_RATE_CHANGE:
> >> +		err = emc_prepare_timing_change(emc, cnd->old_rate);
> >> +		if (err)
> >> +			break;
> >> +
> >> +		err = emc_complete_timing_change(emc, true);
> >> +		break;
> >> +
> >> +	case POST_RATE_CHANGE:
> >> +		err = emc_complete_timing_change(emc, false);
> >> +		break;
> >> +
> >> +	default:
> >> +		return NOTIFY_DONE;
> >> +	}
> >> +
> >> +	return notifier_from_errno(err);
> >> +}
> >> +
> >> +static int emc_setup_hw(struct tegra_emc *emc)
> >> +{
> >> +	u32 emc_cfg;
> >> +
> >> +	emc_cfg = readl_relaxed(emc->regs + EMC_CFG_2);
> >> +
> >> +	/*
> >> +	 * Depending on a memory type, DRAM should enter either self-refresh
> >> +	 * or power-down state on EMC clock change.
> >> +	 */
> >> +	if (!(emc_cfg & EMC_CLKCHANGE_PD_ENABLE) &&
> >> +	    !(emc_cfg & EMC_CLKCHANGE_SR_ENABLE))
> >> +	{
> >> +		dev_err(emc->dev,
> >> +			"bootloader didn't specify DRAM auto-suspend mode\n");
> >> +		return -EINVAL;
> >> +	}
> >> +
> >> +	/* allow EMC and CAR to handshake on PLL divider/source changes */
> >> +	emc_cfg |= EMC_CLKCHANGE_REQ_ENABLE;
> >> +	writel_relaxed(emc_cfg, emc->regs + EMC_CFG_2);
> >> +
> >> +	/* initialize interrupt */
> >> +	writel_relaxed(EMC_CLKCHANGE_COMPLETE_INT, emc->regs + EMC_INTMASK);
> >> +	writel_relaxed(EMC_CLKCHANGE_COMPLETE_INT, emc->regs + EMC_INTSTATUS);
> >> +
> >> +	return 0;
> >> +}
> >> +
> >> +static int emc_init(struct tegra_emc *emc, unsigned long rate)
> >> +{
> >> +	int err;
> >> +
> >> +	err = clk_set_parent(emc->emc_mux, emc->backup_clk);
> >> +	if (err) {
> >> +		dev_err(emc->dev,
> >> +			"failed to reparent to backup source: %d\n", err);
> >> +		return err;
> >> +	}
> >> +
> >> +	err = clk_set_rate(emc->pll_m, rate);
> >> +	if (err)
> >> +		dev_err(emc->dev,
> >> +			"failed to change pll_m rate: %d\n", err);
> >> +
> >> +	err = clk_set_parent(emc->emc_mux, emc->pll_m);
> >> +	if (err) {
> >> +		dev_err(emc->dev,
> >> +			"failed to reparent to pll_m: %d\n", err);
> >> +		return err;
> >> +	}
> >> +
> >> +	return 0;
> >> +}
> >> +
> >> +static int tegra_emc_probe(struct platform_device *pdev)
> >> +{
> >> +	struct device_node *np;
> >> +	struct tegra_emc *emc;
> >> +	struct resource *res;
> >> +	u32 ram_code;
> >> +	int err;
> >> +
> >> +	emc = devm_kzalloc(&pdev->dev, sizeof(*emc), GFP_KERNEL);
> >> +	if (!emc)
> >> +		return -ENOMEM;
> >> +
> >> +	emc->dev = &pdev->dev;
> >> +
> >> +	ram_code = tegra_read_ram_code();
> >> +
> >> +	np = tegra_emc_find_node_by_ram_code(emc, ram_code);
> >> +	if (!np)
> >> +		return -ENOENT;
> >> +
> >> +	err = tegra_emc_load_timings_from_dt(emc, np);
> >> +	of_node_put(np);
> >> +	if (err)
> >> +		return err;
> >> +
> >> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> >> +	emc->regs = devm_ioremap_resource(&pdev->dev, res);
> >> +	if (IS_ERR(emc->regs))
> >> +		return PTR_ERR(emc->regs);
> >> +
> >> +	err = emc_setup_hw(emc);
> >> +	if (err)
> >> +		return err;
> >> +
> >> +	emc->irq = platform_get_irq(pdev, 0);
> >> +	if (emc->irq < 0) {
> >> +		dev_warn(&pdev->dev, "interrupt not specified\n");
> >> +		dev_warn(&pdev->dev, "continuing, but please update your DT\n");
> > 
> > Do we really need this? I think this is a case where we don't have to
> > keep backwards-compatibility because this driver hasn't "worked" in a
> > very long time (because it was absent). Therefore, if we error out in
> > the absence of an interrupt we don't break anything.
> > 
> > There's a few places in this driver that are awkward just because the
> > interrupt isn't mandatory. I don't think it's warranted in this case.
> > 
> 
> Backwards compatibility is always nice to have, but I don't really mind dropping it.

So the Tegra20 EMC driver was removed in commit a7cbe92cef27 ("ARM:
tegra: remove tegra EMC scaling driver"), which is dated August 20,
2013. So it's been almost 5 years since EMC frequency scaling has
worked on Tegra20 (that is, it hasn't worked since v3.15). I very
much doubt that anyone is still running a kernel or DTB from that
time, given how little used to be supported and how much they'd be
missing out on if they stuck to that DTB.

I don't think backwards compatibility will hold up as an argument
in this case.

> 
> >> +	} else {
> >> +		init_completion(&emc->clk_handshake_complete);
> >> +
> >> +		err = devm_request_irq(&pdev->dev, emc->irq, tegra_emc_isr, 0,
> >> +				       dev_name(&pdev->dev), emc);
> >> +		if (err < 0) {
> >> +			dev_err(&pdev->dev, "failed to request IRQ#%u: %d\n",
> >> +				emc->irq, err);
> >> +			return err;
> >> +		}
> >> +	}
> >> +
> >> +	emc->pll_m = clk_get_sys(NULL, "pll_m");
> >> +	if (IS_ERR(emc->pll_m)) {
> >> +		err = PTR_ERR(emc->pll_m);
> >> +		dev_err(&pdev->dev, "failed to get pll_m: %d\n", err);
> >> +		return err;
> >> +	}
> >> +
> >> +	emc->backup_clk = clk_get_sys(NULL, "pll_p");
> >> +	if (IS_ERR(emc->backup_clk)) {
> >> +		err = PTR_ERR(emc->backup_clk);
> >> +		dev_err(&pdev->dev, "failed to get pll_p: %d\n", err);
> >> +		goto put_pll_m;
> >> +	}
> >> +
> >> +	emc->clk = clk_get_sys(NULL, "emc");
> >> +	if (IS_ERR(emc->clk)) {
> >> +		err = PTR_ERR(emc->clk);
> >> +		dev_err(&pdev->dev, "failed to get emc: %d\n", err);
> >> +		goto put_backup;
> >> +	}
> > 
> > Instead of using clk_get_sys(), why not specify these in the DT with
> > proper names for context ("emc", "pll", "backup")? Again, I don't think
> > we have to worry about backwards-compatibility here since there can be
> > no regression.
> > 
> 
> I don't think that "pll" and "backup" could be placed in DT because it is a pure
> software-driver description.
> 
> "emc" could be retrieved from DT if we don't care about backwards compatibility.

I think both PLL and backup clocks would be appropriate to describe in
DT. There are other cases, like for display, where we list clocks in the
DT that aren't strictly inputs to a hardware block. But they are related
to the functionality of the block, so I think it makes sense to list
them as well.

In this particular case, the PLL is what drives the memory banks, which
is the think that the EMC controls, right? So that itself is reason
enough, in my opinion, to list it in DT. Much the same goes for the
backup clock, which is really just the PLL for some transient state
where the normal PLL is being reconfigured.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 5/5] memory: tegra: Introduce Tegra20 EMC driver

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 3151 bytes --]

On Mon, Jun 11, 2018 at 01:35:03PM +0200, Thierry Reding wrote:
> On Wed, Jun 06, 2018 at 04:42:01PM +0300, Dmitry Osipenko wrote:
> > On 06.06.2018 14:02, Thierry Reding wrote:
> > > On Mon, Jun 04, 2018 at 01:36:54AM +0300, Dmitry Osipenko wrote:
[...]
> > >> +	if (!child_count) {
> > >> +		dev_err(emc->dev, "no memory timings in DT node\n");
> > >> +		return -ENOENT;
> > >> +	}
> > >> +
> > >> +	emc->timings = devm_kcalloc(emc->dev, child_count, sizeof(*timing),
> > >> +				    GFP_KERNEL);
> > >> +	if (!emc->timings)
> > >> +		return -ENOMEM;
> > >> +
> > >> +	emc->num_timings = child_count;
> > >> +	timing = emc->timings;
> > >> +
> > >> +	for_each_child_of_node(node, child) {
> > >> +		err = load_one_timing_from_dt(emc, timing++, child);
> > >> +		if (err) {
> > >> +			of_node_put(child);
> > >> +			return err;
> > >> +		}
> > >> +	}
> > >> +
> > >> +	sort(emc->timings, emc->num_timings, sizeof(*timing), cmp_timings,
> > >> +	     NULL);
> > >> +
> > >> +	return 0;
> > >> +}
> > >> +
> > >> +static struct device_node *
> > >> +tegra_emc_find_node_by_ram_code(struct tegra_emc *emc, u32 ram_code)
> > >> +{
> > >> +	struct device_node *np;
> > >> +	int err;
> > >> +
> > >> +	for_each_child_of_node(emc->dev->of_node, np) {
> > >> +		u32 value;
> > >> +
> > >> +		err = of_property_read_u32(np, "nvidia,ram-code", &value);
> > >> +		if (err || value != ram_code)
> > >> +			continue;
> > >> +
> > >> +		return np;
> > >> +	}
> > >> +
> > >> +	dev_info(emc->dev, "no memory timings for RAM code %u found in DT\n",
> > >> +		 ram_code);
> > > 
> > > This seems like it should be dev_warn() or perhaps even dev_err() given
> > > that the result of it is the driver failing to probe. dev_info() may go
> > > unnoticed.
> > > 
> > 
> > Absence of memory timings is a valid case, hence dev_info() suit well here.
> > 
> > I can't see anything wrong with returning a errno if driver has nothing to do
> > and prefer to keep it because in that case managed resources would be free'd by
> > the driver core, though returning '0' also would work.
> 
> I disagree. A driver failing to probe will show up as a kernel log entry
> and is something that people will have to whitelist if they're filtering
> for error messages in the boot log.
> 
> I think it's more user-friendly to just let the driver succeed the probe
> in an expected case, even if that means there's really nothing to do. If
> you're really concerned about the managed resources staying around, I
> think you could probably get rid of them explicitly. By the looks of it
> devres_release_all() isn't an exported symbol, so it can't be called
> from driver code, but perhaps that's something that we can change.

Maybe an easier way to avoid keeping the managed resources around would
be to move the check a little further up. That way, we can abort earlier
if no EMC timings are available, before any resources are even
allocated.

The tegra_emc_find_node_by_ram_code() function would need to take a
struct device * instead of struct tegra_emc *, but otherwise it should
work fine.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 5/5] memory: tegra: Introduce Tegra20 EMC driver

[Dmitry Osipenko]

On Monday, 11 June 2018 14:41:33 MSK Thierry Reding wrote:
> On Mon, Jun 11, 2018 at 01:35:03PM +0200, Thierry Reding wrote:
> > On Wed, Jun 06, 2018 at 04:42:01PM +0300, Dmitry Osipenko wrote:
> > > On 06.06.2018 14:02, Thierry Reding wrote:
> > > > On Mon, Jun 04, 2018 at 01:36:54AM +0300, Dmitry Osipenko wrote:
> [...]
> 
> > > >> +	if (!child_count) {
> > > >> +		dev_err(emc->dev, "no memory timings in DT node\n");
> > > >> +		return -ENOENT;
> > > >> +	}
> > > >> +
> > > >> +	emc->timings = devm_kcalloc(emc->dev, child_count, 
sizeof(*timing),
> > > >> +				    GFP_KERNEL);
> > > >> +	if (!emc->timings)
> > > >> +		return -ENOMEM;
> > > >> +
> > > >> +	emc->num_timings = child_count;
> > > >> +	timing = emc->timings;
> > > >> +
> > > >> +	for_each_child_of_node(node, child) {
> > > >> +		err = load_one_timing_from_dt(emc, timing++, child);
> > > >> +		if (err) {
> > > >> +			of_node_put(child);
> > > >> +			return err;
> > > >> +		}
> > > >> +	}
> > > >> +
> > > >> +	sort(emc->timings, emc->num_timings, sizeof(*timing), 
cmp_timings,
> > > >> +	     NULL);
> > > >> +
> > > >> +	return 0;
> > > >> +}
> > > >> +
> > > >> +static struct device_node *
> > > >> +tegra_emc_find_node_by_ram_code(struct tegra_emc *emc, u32 ram_code)
> > > >> +{
> > > >> +	struct device_node *np;
> > > >> +	int err;
> > > >> +
> > > >> +	for_each_child_of_node(emc->dev->of_node, np) {
> > > >> +		u32 value;
> > > >> +
> > > >> +		err = of_property_read_u32(np, "nvidia,ram-code", &value);
> > > >> +		if (err || value != ram_code)
> > > >> +			continue;
> > > >> +
> > > >> +		return np;
> > > >> +	}
> > > >> +
> > > >> +	dev_info(emc->dev, "no memory timings for RAM code %u found in
> > > >> DT\n",
> > > >> +		 ram_code);
> > > > 
> > > > This seems like it should be dev_warn() or perhaps even dev_err()
> > > > given
> > > > that the result of it is the driver failing to probe. dev_info() may
> > > > go
> > > > unnoticed.
> > > 
> > > Absence of memory timings is a valid case, hence dev_info() suit well
> > > here.
> > > 
> > > I can't see anything wrong with returning a errno if driver has nothing
> > > to do and prefer to keep it because in that case managed resources
> > > would be free'd by the driver core, though returning '0' also would
> > > work.
> > 
> > I disagree. A driver failing to probe will show up as a kernel log entry
> > and is something that people will have to whitelist if they're filtering
> > for error messages in the boot log.
> > 
> > I think it's more user-friendly to just let the driver succeed the probe
> > in an expected case, even if that means there's really nothing to do. If
> > you're really concerned about the managed resources staying around, I
> > think you could probably get rid of them explicitly. By the looks of it
> > devres_release_all() isn't an exported symbol, so it can't be called
> > from driver code, but perhaps that's something that we can change.
> 
> Maybe an easier way to avoid keeping the managed resources around would
> be to move the check a little further up. That way, we can abort earlier
> if no EMC timings are available, before any resources are even
> allocated.
> 
> The tegra_emc_find_node_by_ram_code() function would need to take a
> struct device * instead of struct tegra_emc *, but otherwise it should
> work fine.

Good point, thank you!

Re: [PATCH v2 5/5] memory: tegra: Introduce Tegra20 EMC driver

[Dmitry Osipenko]

On Monday, 11 June 2018 14:35:03 MSK Thierry Reding wrote:
> On Wed, Jun 06, 2018 at 04:42:01PM +0300, Dmitry Osipenko wrote:
> > On 06.06.2018 14:02, Thierry Reding wrote:
> > > On Mon, Jun 04, 2018 at 01:36:54AM +0300, Dmitry Osipenko wrote:
> [...]
> 
> > >> +struct tegra_emc {
> > >> +	struct device *dev;
> > >> +	struct notifier_block clk_nb;
> > >> +	struct clk *backup_clk;
> > >> +	struct clk *emc_mux;
> > >> +	struct clk *pll_m;
> > >> +	struct clk *clk;
> > >> +	void __iomem *regs;
> > >> +
> > >> +	struct completion clk_handshake_complete;
> > >> +	int irq;
> > >> +
> > >> +	struct emc_timing *timings;
> > >> +	unsigned int num_timings;
> > >> +};
> > >> +
> > >> +static irqreturn_t tegra_emc_isr(int irq, void *data)
> > >> +{
> > >> +	struct tegra_emc *emc = data;
> > >> +	u32 intmask = EMC_CLKCHANGE_COMPLETE_INT;
> > >> +	u32 status;
> > >> +
> > >> +	status = readl_relaxed(emc->regs + EMC_INTSTATUS) & intmask;
> > >> +	if (!status)
> > >> +		return IRQ_NONE;
> > >> +
> > >> +	/* clear interrupts */
> > >> +	writel_relaxed(status, emc->regs + EMC_INTSTATUS);
> > > 
> > > Do we really want to just clear the handshake complete interrupt or do
> > > we want to clear all of them? Perhaps we should also warn if there are
> > > other interrupts that we're not handling? Currently we'd only get some
> > > warning if another interrupt triggered without the handshake complete
> > > one triggering at the same time, but couldn't there be others asserted
> > > along with the handshake complete interrupt? In which case we'd just
> > > be ignoring them. Or perhaps not clearing it would get the ISR run
> > > immediately again and produce the "nobody cared" warning?
> > 
> > Yes, we really want to just clear the handshake-complete interrupt. No, we
> > shouldn't warn about other interrupts because IRQ subsys does it for us.
> > Other interrupts shouldn't be asserted because we disabled them with the
> > interrupts masking in emc_setup_hw(). Other interrupts can only be
> > asserted in a case of a bug, there will be a "nobody cared" warning and
> > interrupt will be disabled, this is exactly what we want in that case.
> 
> Okay, that's what I was suspecting might happen. Seems fine, then.
> 
> > >> +static int cmp_timings(const void *_a, const void *_b)
> > >> +{
> > >> +	const struct emc_timing *a = _a;
> > >> +	const struct emc_timing *b = _b;
> > >> +
> > >> +	if (a->rate < b->rate)
> > >> +		return -1;
> > >> +	else if (a->rate == b->rate)
> > >> +		return 0;
> > >> +	else
> > >> +		return 1;
> > > 
> > > Nit, I tend to
> > 
> > Tend to..?
> 
> Looks like I got distracted at this point. =) What I meant to say is
> that I tend to not use if ... else if ... else for things that do
> immediately return. The above is equivalent to the below, which I think
> is much easier to read:
> 
> 	if (a->rate < b->rate)
> 		return -1;
> 
> 	if (a->rate > b->rate)
> 		return 1;
> 
> 	return 0;
> 

Okay.

> > >> +}
> > >> +
> > >> +static int tegra_emc_load_timings_from_dt(struct tegra_emc *emc,
> > >> +					  struct device_node *node)
> > >> +{
> > >> +	struct device_node *child;
> > >> +	struct emc_timing *timing;
> > >> +	int child_count;
> > >> +	int err;
> > >> +
> > >> +	child_count = of_get_child_count(node);
> > > 
> > > It's unfortunate that of_get_child_count() doesn't return unsigned int,
> > > there's no reason why this would have to be signed.
> > 
> > Patches are welcome ;)
> 
> Yeah, just a random drive-by comment. Please ignore. =)
> 
> > >> +	if (!child_count) {
> > >> +		dev_err(emc->dev, "no memory timings in DT node\n");
> > >> +		return -ENOENT;
> > >> +	}
> > >> +
> > >> +	emc->timings = devm_kcalloc(emc->dev, child_count, 
sizeof(*timing),
> > >> +				    GFP_KERNEL);
> > >> +	if (!emc->timings)
> > >> +		return -ENOMEM;
> > >> +
> > >> +	emc->num_timings = child_count;
> > >> +	timing = emc->timings;
> > >> +
> > >> +	for_each_child_of_node(node, child) {
> > >> +		err = load_one_timing_from_dt(emc, timing++, child);
> > >> +		if (err) {
> > >> +			of_node_put(child);
> > >> +			return err;
> > >> +		}
> > >> +	}
> > >> +
> > >> +	sort(emc->timings, emc->num_timings, sizeof(*timing), 
cmp_timings,
> > >> +	     NULL);
> > >> +
> > >> +	return 0;
> > >> +}
> > >> +
> > >> +static struct device_node *
> > >> +tegra_emc_find_node_by_ram_code(struct tegra_emc *emc, u32 ram_code)
> > >> +{
> > >> +	struct device_node *np;
> > >> +	int err;
> > >> +
> > >> +	for_each_child_of_node(emc->dev->of_node, np) {
> > >> +		u32 value;
> > >> +
> > >> +		err = of_property_read_u32(np, "nvidia,ram-code", &value);
> > >> +		if (err || value != ram_code)
> > >> +			continue;
> > >> +
> > >> +		return np;
> > >> +	}
> > >> +
> > >> +	dev_info(emc->dev, "no memory timings for RAM code %u found in DT
\n",
> > >> +		 ram_code);
> > > 
> > > This seems like it should be dev_warn() or perhaps even dev_err() given
> > > that the result of it is the driver failing to probe. dev_info() may go
> > > unnoticed.
> > 
> > Absence of memory timings is a valid case, hence dev_info() suit well
> > here.
> > 
> > I can't see anything wrong with returning a errno if driver has nothing to
> > do and prefer to keep it because in that case managed resources would be
> > free'd by the driver core, though returning '0' also would work.
> 
> I disagree. A driver failing to probe will show up as a kernel log entry
> and is something that people will have to whitelist if they're filtering
> for error messages in the boot log.
> 
> I think it's more user-friendly to just let the driver succeed the probe
> in an expected case, even if that means there's really nothing to do. If
> you're really concerned about the managed resources staying around, I
> think you could probably get rid of them explicitly. By the looks of it
> devres_release_all() isn't an exported symbol, so it can't be called
> from driver code, but perhaps that's something that we can change.
> 
> > >> +
> > >> +	return NULL;
> > >> +}
> > >> +
> > >> +static int tegra_emc_clk_change_notify(struct notifier_block *nb,
> > >> +				       unsigned long msg, void *data)
> > >> +{
> > >> +	struct tegra_emc *emc = container_of(nb, struct tegra_emc, 
clk_nb);
> > >> +	struct clk_notifier_data *cnd = data;
> > >> +	int err;
> > >> +
> > >> +	switch (msg) {
> > >> +	case PRE_RATE_CHANGE:
> > >> +		err = emc_prepare_timing_change(emc, cnd->new_rate);
> > >> +		break;
> > >> +
> > >> +	case ABORT_RATE_CHANGE:
> > >> +		err = emc_prepare_timing_change(emc, cnd->old_rate);
> > >> +		if (err)
> > >> +			break;
> > >> +
> > >> +		err = emc_complete_timing_change(emc, true);
> > >> +		break;
> > >> +
> > >> +	case POST_RATE_CHANGE:
> > >> +		err = emc_complete_timing_change(emc, false);
> > >> +		break;
> > >> +
> > >> +	default:
> > >> +		return NOTIFY_DONE;
> > >> +	}
> > >> +
> > >> +	return notifier_from_errno(err);
> > >> +}
> > >> +
> > >> +static int emc_setup_hw(struct tegra_emc *emc)
> > >> +{
> > >> +	u32 emc_cfg;
> > >> +
> > >> +	emc_cfg = readl_relaxed(emc->regs + EMC_CFG_2);
> > >> +
> > >> +	/*
> > >> +	 * Depending on a memory type, DRAM should enter either self-
refresh
> > >> +	 * or power-down state on EMC clock change.
> > >> +	 */
> > >> +	if (!(emc_cfg & EMC_CLKCHANGE_PD_ENABLE) &&
> > >> +	    !(emc_cfg & EMC_CLKCHANGE_SR_ENABLE))
> > >> +	{
> > >> +		dev_err(emc->dev,
> > >> +			"bootloader didn't specify DRAM auto-suspend mode\n");
> > >> +		return -EINVAL;
> > >> +	}
> > >> +
> > >> +	/* allow EMC and CAR to handshake on PLL divider/source changes 
*/
> > >> +	emc_cfg |= EMC_CLKCHANGE_REQ_ENABLE;
> > >> +	writel_relaxed(emc_cfg, emc->regs + EMC_CFG_2);
> > >> +
> > >> +	/* initialize interrupt */
> > >> +	writel_relaxed(EMC_CLKCHANGE_COMPLETE_INT, emc->regs + 
EMC_INTMASK);
> > >> +	writel_relaxed(EMC_CLKCHANGE_COMPLETE_INT, emc->regs +
> > >> EMC_INTSTATUS);
> > >> +
> > >> +	return 0;
> > >> +}
> > >> +
> > >> +static int emc_init(struct tegra_emc *emc, unsigned long rate)
> > >> +{
> > >> +	int err;
> > >> +
> > >> +	err = clk_set_parent(emc->emc_mux, emc->backup_clk);
> > >> +	if (err) {
> > >> +		dev_err(emc->dev,
> > >> +			"failed to reparent to backup source: %d\n", err);
> > >> +		return err;
> > >> +	}
> > >> +
> > >> +	err = clk_set_rate(emc->pll_m, rate);
> > >> +	if (err)
> > >> +		dev_err(emc->dev,
> > >> +			"failed to change pll_m rate: %d\n", err);
> > >> +
> > >> +	err = clk_set_parent(emc->emc_mux, emc->pll_m);
> > >> +	if (err) {
> > >> +		dev_err(emc->dev,
> > >> +			"failed to reparent to pll_m: %d\n", err);
> > >> +		return err;
> > >> +	}
> > >> +
> > >> +	return 0;
> > >> +}
> > >> +
> > >> +static int tegra_emc_probe(struct platform_device *pdev)
> > >> +{
> > >> +	struct device_node *np;
> > >> +	struct tegra_emc *emc;
> > >> +	struct resource *res;
> > >> +	u32 ram_code;
> > >> +	int err;
> > >> +
> > >> +	emc = devm_kzalloc(&pdev->dev, sizeof(*emc), GFP_KERNEL);
> > >> +	if (!emc)
> > >> +		return -ENOMEM;
> > >> +
> > >> +	emc->dev = &pdev->dev;
> > >> +
> > >> +	ram_code = tegra_read_ram_code();
> > >> +
> > >> +	np = tegra_emc_find_node_by_ram_code(emc, ram_code);
> > >> +	if (!np)
> > >> +		return -ENOENT;
> > >> +
> > >> +	err = tegra_emc_load_timings_from_dt(emc, np);
> > >> +	of_node_put(np);
> > >> +	if (err)
> > >> +		return err;
> > >> +
> > >> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > >> +	emc->regs = devm_ioremap_resource(&pdev->dev, res);
> > >> +	if (IS_ERR(emc->regs))
> > >> +		return PTR_ERR(emc->regs);
> > >> +
> > >> +	err = emc_setup_hw(emc);
> > >> +	if (err)
> > >> +		return err;
> > >> +
> > >> +	emc->irq = platform_get_irq(pdev, 0);
> > >> +	if (emc->irq < 0) {
> > >> +		dev_warn(&pdev->dev, "interrupt not specified\n");
> > >> +		dev_warn(&pdev->dev, "continuing, but please update your DT
\n");
> > > 
> > > Do we really need this? I think this is a case where we don't have to
> > > keep backwards-compatibility because this driver hasn't "worked" in a
> > > very long time (because it was absent). Therefore, if we error out in
> > > the absence of an interrupt we don't break anything.
> > > 
> > > There's a few places in this driver that are awkward just because the
> > > interrupt isn't mandatory. I don't think it's warranted in this case.
> > 
> > Backwards compatibility is always nice to have, but I don't really mind
> > dropping it.
> So the Tegra20 EMC driver was removed in commit a7cbe92cef27 ("ARM:
> tegra: remove tegra EMC scaling driver"), which is dated August 20,
> 2013. So it's been almost 5 years since EMC frequency scaling has
> worked on Tegra20 (that is, it hasn't worked since v3.15). I very
> much doubt that anyone is still running a kernel or DTB from that
> time, given how little used to be supported and how much they'd be
> missing out on if they stuck to that DTB.
> 
> I don't think backwards compatibility will hold up as an argument
> in this case.
> 

Okay.

> > >> +	} else {
> > >> +		init_completion(&emc->clk_handshake_complete);
> > >> +
> > >> +		err = devm_request_irq(&pdev->dev, emc->irq, tegra_emc_isr, 
0,
> > >> +				       dev_name(&pdev->dev), emc);
> > >> +		if (err < 0) {
> > >> +			dev_err(&pdev->dev, "failed to request IRQ#%u: %d\n",
> > >> +				emc->irq, err);
> > >> +			return err;
> > >> +		}
> > >> +	}
> > >> +
> > >> +	emc->pll_m = clk_get_sys(NULL, "pll_m");
> > >> +	if (IS_ERR(emc->pll_m)) {
> > >> +		err = PTR_ERR(emc->pll_m);
> > >> +		dev_err(&pdev->dev, "failed to get pll_m: %d\n", err);
> > >> +		return err;
> > >> +	}
> > >> +
> > >> +	emc->backup_clk = clk_get_sys(NULL, "pll_p");
> > >> +	if (IS_ERR(emc->backup_clk)) {
> > >> +		err = PTR_ERR(emc->backup_clk);
> > >> +		dev_err(&pdev->dev, "failed to get pll_p: %d\n", err);
> > >> +		goto put_pll_m;
> > >> +	}
> > >> +
> > >> +	emc->clk = clk_get_sys(NULL, "emc");
> > >> +	if (IS_ERR(emc->clk)) {
> > >> +		err = PTR_ERR(emc->clk);
> > >> +		dev_err(&pdev->dev, "failed to get emc: %d\n", err);
> > >> +		goto put_backup;
> > >> +	}
> > > 
> > > Instead of using clk_get_sys(), why not specify these in the DT with
> > > proper names for context ("emc", "pll", "backup")? Again, I don't think
> > > we have to worry about backwards-compatibility here since there can be
> > > no regression.
> > 
> > I don't think that "pll" and "backup" could be placed in DT because it is
> > a pure software-driver descriptio     in
> DT. There are other cases, like for display, where we list clocks in the
> DT that aren't strictly inputs to a hardware block. But they are related
> to the functionality of the block, so I think it makes sense to list
> them as well.
> 
> In this particular case, the PLL is what drives the memory banks, which
> is the think that the EMC controls, right? So that itself is reason
> enough, in my opinion, to list it in DT. Much the same goes for the
> backup clock, which is really just the PLL for some transient state
> where the normal PLL is being reconfigured.

PLL itself shouldn't really matter. EMC doesn't control PLL, but only 
interacts with the Clock-and-Reset controller. This means that we could use 
PLL_C instead of PLL_M if we wanted. Though PLL_M is meant to be a "memory 
PLL".

To me a selection of a parent clock is a pure software description that is 
wrong to be placed in DT.

I'm not sure that specifying parent clock for display in DT is correct. That 
simply doesn't make sense because there are four possible parent clocks for 
the display. Selection of a parent clock in DT is a pure software description 
that is only relevant for the upstream Linux DRM driver, it's a mistake to me.

Moreover PLL isn't a "device" clock, but specifically a "system" clock. So 
describing it in DT as a "device" clock is wrong to me too.

Putting all together, the PLL's should be left as-is they are now in v2, 
please let me know if you disagree.

The "emc" clock could be placed in DT since we won't bother with the DT 
backwards compatibility, I'll change it in the next revision of the patchset.

Re: [PATCH v2 5/5] memory: tegra: Introduce Tegra20 EMC driver

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 6381 bytes --]

On Mon, Jun 11, 2018 at 04:06:41PM +0300, Dmitry Osipenko wrote:
> On Monday, 11 June 2018 14:35:03 MSK Thierry Reding wrote:
> > On Wed, Jun 06, 2018 at 04:42:01PM +0300, Dmitry Osipenko wrote:
> > > On 06.06.2018 14:02, Thierry Reding wrote:
> > > > On Mon, Jun 04, 2018 at 01:36:54AM +0300, Dmitry Osipenko wrote:
[...]
> > > >> +	} else {
> > > >> +		init_completion(&emc->clk_handshake_complete);
> > > >> +
> > > >> +		err = devm_request_irq(&pdev->dev, emc->irq, tegra_emc_isr, 
> 0,
> > > >> +				       dev_name(&pdev->dev), emc);
> > > >> +		if (err < 0) {
> > > >> +			dev_err(&pdev->dev, "failed to request IRQ#%u: %d\n",
> > > >> +				emc->irq, err);
> > > >> +			return err;
> > > >> +		}
> > > >> +	}
> > > >> +
> > > >> +	emc->pll_m = clk_get_sys(NULL, "pll_m");
> > > >> +	if (IS_ERR(emc->pll_m)) {
> > > >> +		err = PTR_ERR(emc->pll_m);
> > > >> +		dev_err(&pdev->dev, "failed to get pll_m: %d\n", err);
> > > >> +		return err;
> > > >> +	}
> > > >> +
> > > >> +	emc->backup_clk = clk_get_sys(NULL, "pll_p");
> > > >> +	if (IS_ERR(emc->backup_clk)) {
> > > >> +		err = PTR_ERR(emc->backup_clk);
> > > >> +		dev_err(&pdev->dev, "failed to get pll_p: %d\n", err);
> > > >> +		goto put_pll_m;
> > > >> +	}
> > > >> +
> > > >> +	emc->clk = clk_get_sys(NULL, "emc");
> > > >> +	if (IS_ERR(emc->clk)) {
> > > >> +		err = PTR_ERR(emc->clk);
> > > >> +		dev_err(&pdev->dev, "failed to get emc: %d\n", err);
> > > >> +		goto put_backup;
> > > >> +	}
> > > > 
> > > > Instead of using clk_get_sys(), why not specify these in the DT with
> > > > proper names for context ("emc", "pll", "backup")? Again, I don't think
> > > > we have to worry about backwards-compatibility here since there can be
> > > > no regression.
> > > 
> > > I don't think that "pll" and "backup" could be placed in DT because it is
> > > a pure software-driver descriptio     in
> > DT. There are other cases, like for display, where we list clocks in the
> > DT that aren't strictly inputs to a hardware block. But they are related
> > to the functionality of the block, so I think it makes sense to list
> > them as well.
> > 
> > In this particular case, the PLL is what drives the memory banks, which
> > is the think that the EMC controls, right? So that itself is reason
> > enough, in my opinion, to list it in DT. Much the same goes for the
> > backup clock, which is really just the PLL for some transient state
> > where the normal PLL is being reconfigured.
> 
> PLL itself shouldn't really matter. EMC doesn't control PLL, but only 
> interacts with the Clock-and-Reset controller. This means that we could use 
> PLL_C instead of PLL_M if we wanted. Though PLL_M is meant to be a "memory 
> PLL".
> 
> To me a selection of a parent clock is a pure software description that is 
> wrong to be placed in DT.

I think of it as configuration rather than software description. The
problem that we're trying to solve with DT is primarily one of
configuration and parameterization. The idea is that we describe the
interfaces of some hardware module and then specify which resources
to use as inputs and/or outputs for those interfaces.

So in this case we can define that in order to perform its function the
EMC driver needs to somehow control a PLL that drives the memory. Even
if the EMC hardware itself doesn't control a PLL, not controlling a PLL
would make the EMC driver rather pointless. Actually, if the hardware
did control the PLL itself, there'd be no need to describe any aspect of
that PLL in the DT. We only need to describe it in DT because we've got
several possibilities and we want to make sure we pick one that is best
for our specific use-case.

> I'm not sure that specifying parent clock for display in DT is correct. That 
> simply doesn't make sense because there are four possible parent clocks for 
> the display. Selection of a parent clock in DT is a pure software description 
> that is only relevant for the upstream Linux DRM driver, it's a mistake to me.

I disagree. While it is certainly true that there are multiple possible
PLLs for display, which one to use depends on the use-case. In most SoC
generations we have two display PLLs where pll_d2 is "more capable" than
pll_d because it can run at higher frequencies. pll_d for example can in
many cases not be used to drive an HDMI output at 1080p resolutions. For
DisplayPort output, pll_d can usually also not be used. It therefore
makes sense to define in DT which PLL to use for a particular use-case.

Now, I'll grant you that this somewhat blurs the lines of hardware
descriptions vs. software description. But the alternative would be that
we don't describe the PLLs in DT, which would imply that we have to have
some logic in the driver that either knows which PLLs exist on the given
SoC and can query their capabilities in order to determine which one to
use for any given use-case.

It also means that the driver becomes much less generic, because we have
to put a lot of SoC specific knowledge into the driver. This is perhaps
not a big issue for SoCs like Tegra that are very closely integrated
anyway, but imagine if you take the same approach with IP that can be
licensed from a third party and therefore could appear in many more
different combinations.

I think having the parent PLL defined in DT is a good compromise. If you
say that strictly only hardware interfaces can be represented in DT, you
take away a lot of the flexibility and in turn put a lot of the data
back into drivers.

> Moreover PLL isn't a "device" clock, but specifically a "system" clock. So 
> describing it in DT as a "device" clock is wrong to me too.
> 
> Putting all together, the PLL's should be left as-is they are now in v2, 
> please let me know if you disagree.

I don't care very strongly in this case because the driver is unlikely
to ever be reused anywhere other than on Tegra20. But looking up the
system clock by a string is not something that is typically encouraged
because it limits portability.

> The "emc" clock could be placed in DT since we won't bother with the DT 
> backwards compatibility, I'll change it in the next revision of the patchset.

Frankly, if you don't have the other clocks in DT there's little sense
in having the EMC clock in DT.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 5/5] memory: tegra: Introduce Tegra20 EMC driver

[Dmitry Osipenko]

On Monday, 11 June 2018 18:53:43 MSK Thierry Reding wrote:
> On Mon, Jun 11, 2018 at 04:06:41PM +0300, Dmitry Osipenko wrote:
> > On Monday, 11 June 2018 14:35:03 MSK Thierry Reding wrote:
> > > On Wed, Jun 06, 2018 at 04:42:01PM +0300, Dmitry Osipenko wrote:
> > > > On 06.06.2018 14:02, Thierry Reding wrote:
> > > > > On Mon, Jun 04, 2018 at 01:36:54AM +0300, Dmitry Osipenko wrote:
> [...]
> 
> > > > >> +	} else {
> > > > >> +		init_completion(&emc->clk_handshake_complete);
> > > > >> +
> > > > >> +		err = devm_request_irq(&pdev->dev, emc->irq, tegra_emc_isr,
> > 
> > 0,
> > 
> > > > >> +				       dev_name(&pdev->dev), emc);
> > > > >> +		if (err < 0) {
> > > > >> +			dev_err(&pdev->dev, "failed to request IRQ#%u: %d\n",
> > > > >> +				emc->irq, err);
> > > > >> +			return err;
> > > > >> +		}
> > > > >> +	}
> > > > >> +
> > > > >> +	emc->pll_m = clk_get_sys(NULL, "pll_m");
> > > > >> +	if (IS_ERR(emc->pll_m)) {
> > > > >> +		err = PTR_ERR(emc->pll_m);
> > > > >> +		dev_err(&pdev->dev, "failed to get pll_m: %d\n", err);
> > > > >> +		return err;
> > > > >> +	}
> > > > >> +
> > > > >> +	emc->backup_clk = clk_get_sys(NULL, "pll_p");
> > > > >> +	if (IS_ERR(emc->backup_clk)) {
> > > > >> +		err = PTR_ERR(emc->backup_clk);
> > > > >> +		dev_err(&pdev->dev, "failed to get pll_p: %d\n", err);
> > > > >> +		goto put_pll_m;
> > > > >> +	}
> > > > >> +
> > > > >> +	emc->clk = clk_get_sys(NULL, "emc");
> > > > >> +	if (IS_ERR(emc->clk)) {
> > > > >> +		err = PTR_ERR(emc->clk);
> > > > >> +		dev_err(&pdev->dev, "failed to get emc: %d\n", err);
> > > > >> +		goto put_backup;
> > > > >> +	}
> > > > > 
> > > > > Instead of using clk_get_sys(), why not specify these in the DT with
> > > > > proper names for context ("emc", "pll", "backup")? Again, I don't
> > > > > think
> > > > > we have to worry about backwards-compatibility here since there can
> > > > > be
> > > > > no regression.
> > > > 
> > > > I don't think that "pll" and "backup" could be placed in DT because it
> > > > is
> > > > a pure software-driver descriptio     in
> > > 
> > > DT. There are other cases, like for display, where we list clocks in the
> > > DT that aren't strictly inputs to a hardware block. But they are related
> > > to the functionality of the block, so I think it makes sense to list
> > > them as well.
> > > 
> > > In this particular case, the PLL is what drives the memory banks, which
> > > is the think that the EMC controls, right? So that itself is reason
> > > enough, in my opinion, to list it in DT. Much the same goes for the
> > > backup clock, which is really just the PLL for some transient state
> > > where the normal PLL is being reconfigured.
> > 
> > PLL itself shouldn't really matter. EMC doesn't control PLL, but only
> > interacts with the Clock-and-Reset controller. This means that we could
> > use
> > PLL_C instead of PLL_M if we wanted. Though PLL_M is meant to be a "memory
> > PLL".
> > 
> > To me a selection of a parent clock is a pure software description that is
> > wrong to be placed in DT.
> 
> I think of it as configuration rather than software description. The
> problem that we're trying to solve with DT is primarily one of
> configuration and parameterization. The idea is that we describe the
> interfaces of some hardware module and then specify which resources
> to use as inputs and/or outputs for those interfaces.
> 
> So in this case we can define that in order to perform its function the
> EMC driver needs to somehow control a PLL that drives the memory. Even
> if the EMC hardware itself doesn't control a PLL, not controlling a PLL
> would make the EMC driver rather pointless. Actually, if the hardware
> did control the PLL itself, there'd be no need to describe any aspect of
> that PLL in the DT. We only need to describe it in DT because we've got
> several possibilities and we want to make sure we pick one that is best
> for our specific use-case.
> 
> > I'm not sure that specifying parent clock for display in DT is correct.
> > That simply doesn't make sense because there are four possible parent
> > clocks for the display. Selection of a parent clock in DT is a pure
> > software description that is only relevant for the upstream Linux DRM
> > driver, it's a mistake to me.
> I disagree. While it is certainly true that there are multiple possible
> PLLs for display, which one to use depends on the use-case. In most SoC
> generations we have two display PLLs where pll_d2 is "more capable" than
> pll_d because it can run at higher frequencies. pll_d for example can in
> many cases not be used to drive an HDMI output at 1080p resolutions. For
> DisplayPort output, pll_d can usually also not be used. It therefore
> makes sense to define in DT which PLL to use for a particular use-case.
> 

Okay.

> Now, I'll grant you that this somewhat blurs the lines of hardware
> descriptions vs. software description. But the alternative would be that
> we don't describe the PLLs in DT, which would imply that we have to have
> some logic in the driver that either knows which PLLs exist on the given
> SoC and can query their capabilities in order to determine which one to
> use for any given use-case.
> 
> It also means that the driver becomes much less generic, because we have
> to put a lot of SoC specific knowledge into the driver. This is perhaps
> not a big issue for SoCs like Tegra that are very closely integrated
> anyway, but imagine if you take the same approach with IP that can be
> licensed from a third party and therefore could appear in many more
> different combinations.
> 

Having some device-specific configuration in DT isn't a problem if there is a 
machine-specific compatibility property for a device, so that device driver 
could override a broken DT if needed.

> I think having the parent PLL defined in DT is a good compromise. If you
> say that strictly only hardware interfaces can be represented in DT, you
> take away a lot of the flexibility and in turn put a lot of the data
> back into drivers.
> 

Problem is that this flexibility is "written in stone".

For example DT may specify PLL_C as the main parent clock for which clock 
driver seems allow a dynamic clock rate change. The HW configuration in DT 
doesn't make sense in this case, it's a SW configuration which is specific to 
upstream Linux kernel.

> > Moreover PLL isn't a "device" clock, but specifically a "system" clock. So
> > describing it in DT as a "device" clock is wrong to me too.
> > 
> > Putting all together, the PLL's should be left as-is they are now in v2,
> > please let me know if you disagree.
> 
> I don't care very strongly in this case because the driver is unlikely
> to ever be reused anywhere other than on Tegra20. But looking up the
> system clock by a string is not something that is typically encouraged
> because it limits portability.
> 

No, in this case it only increases portability because we won't have to deal 
with unsupported/broken configurations.

On the other hand we could just place the static clocks configuration into the 
DT, because realistically we won't ever have to deal with an unsupported 
configurations. I'll consider this variant for v3.

> > The "emc" clock could be placed in DT since we won't bother with the DT
> > backwards compatibility, I'll change it in the next revision of the
> > patchset.
> Frankly, if you don't have the other clocks in DT there's little sense
> in having the EMC clock in DT.

In any case having the EMC clock in DT would make the DT description more 
consistent.

Thank you very much for the review!

Re: [PATCH v2 0/5] Tegra20 External Memory Controller driver

[Peter De Schrijver]

On Mon, Jun 04, 2018 at 01:36:49AM +0300, Dmitry Osipenko wrote:
> Hello,
> 
> Couple years ago the Tegra20 EMC driver was removed from the kernel
> due to incompatible changes in the Tegra's clock driver. This patchset
> introduces a modernized EMC driver. Currently the sole purpose of the
> driver is to initialize DRAM frequency to maximum rate during of the
> kernels boot-up. Later we may consider implementing dynamic memory
> frequency scaling, utilizing functionality provided by this driver.
> 
> Changelog:
> 
> v2:
> 	- Minor code cleanups like consistent use of writel_relaxed instead
> 	  of non-relaxed version, reworded error messages, etc.
> 
> 	- Factored out use_pllm_ud bit checking into a standalone patch for
> 	  consistency.
> 
> Dmitry Osipenko (5):
>   dt: bindings: tegra20-emc: Document interrupt property
>   ARM: dts: tegra20: Add interrupt to External Memory Controller
>   clk: tegra20: Turn EMC clock gate into divider
>   clk: tegra20: Check whether direct PLLM sourcing is turned off for EMC
>   memory: tegra: Introduce Tegra20 EMC driver
> 

Series Acked-By: Peter De Schrijver <pdeschrijver@nvidia.com>

Re: [PATCH v2 0/5] Tegra20 External Memory Controller driver

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 1256 bytes --]

On Mon, Jun 04, 2018 at 01:36:49AM +0300, Dmitry Osipenko wrote:
> Hello,
> 
> Couple years ago the Tegra20 EMC driver was removed from the kernel
> due to incompatible changes in the Tegra's clock driver. This patchset
> introduces a modernized EMC driver. Currently the sole purpose of the
> driver is to initialize DRAM frequency to maximum rate during of the
> kernels boot-up. Later we may consider implementing dynamic memory
> frequency scaling, utilizing functionality provided by this driver.
> 
> Changelog:
> 
> v2:
> 	- Minor code cleanups like consistent use of writel_relaxed instead
> 	  of non-relaxed version, reworded error messages, etc.
> 
> 	- Factored out use_pllm_ud bit checking into a standalone patch for
> 	  consistency.
> 
> Dmitry Osipenko (5):
>   dt: bindings: tegra20-emc: Document interrupt property
>   ARM: dts: tegra20: Add interrupt to External Memory Controller
>   clk: tegra20: Turn EMC clock gate into divider
>   clk: tegra20: Check whether direct PLLM sourcing is turned off for EMC
>   memory: tegra: Introduce Tegra20 EMC driver

I took a brief look and didn't spot any dependencies between the clk and
memory patches. Is it correct that these can be applied separately?

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 0/5] Tegra20 External Memory Controller driver

[Dmitry Osipenko]

On 06.06.2018 13:45, Thierry Reding wrote:
> On Mon, Jun 04, 2018 at 01:36:49AM +0300, Dmitry Osipenko wrote:
>> Hello,
>>
>> Couple years ago the Tegra20 EMC driver was removed from the kernel
>> due to incompatible changes in the Tegra's clock driver. This patchset
>> introduces a modernized EMC driver. Currently the sole purpose of the
>> driver is to initialize DRAM frequency to maximum rate during of the
>> kernels boot-up. Later we may consider implementing dynamic memory
>> frequency scaling, utilizing functionality provided by this driver.
>>
>> Changelog:
>>
>> v2:
>> 	- Minor code cleanups like consistent use of writel_relaxed instead
>> 	  of non-relaxed version, reworded error messages, etc.
>>
>> 	- Factored out use_pllm_ud bit checking into a standalone patch for
>> 	  consistency.
>>
>> Dmitry Osipenko (5):
>>   dt: bindings: tegra20-emc: Document interrupt property
>>   ARM: dts: tegra20: Add interrupt to External Memory Controller
>>   clk: tegra20: Turn EMC clock gate into divider
>>   clk: tegra20: Check whether direct PLLM sourcing is turned off for EMC
>>   memory: tegra: Introduce Tegra20 EMC driver
> 
> I took a brief look and didn't spot any dependencies between the clk and
> memory patches. Is it correct that these can be applied separately?

Yes, it is correct.