Skip to contentSkip to conversation

memory: tegra20-emc: Poll EMC-CaR handshake instead of waiting for interrupt

PATCH Applied Dmitry Osipenko 2 participants 4 messages Mar 19, 2020 – May 6, 2020 2 messages not in the archive

Subsystems (2)
Tegra architecture support Memory controller drivers
lore
Share & tools
lore

Dmitry Osipenko

author
#0 lore raw

PATCH 1/2 memory: tegra20-emc: Poll EMC-CaR handshake instead of waiting for interrupt

The memory clock-rate change could be running on a non-boot CPU, while the boot CPU handles the EMC interrupt. This introduces an unnecessary latency since boot CPU should handle the interrupt and then notify the sibling CPU about clock-rate change completion. In some rare cases boot CPU could be in uninterruptible state for a significant time (like in a case of KASAN + NFS root), it could get to the point that completion timeouts before boot CPU gets a chance to handle interrupt. The solution is to get rid of the completion and replace it with interrupt-status polling.

Signed-off-by Dmitry Osipenko <[email protected]>
---
 drivers/memory/tegra/tegra20-emc.c | 28 +++++++++++-----------------
 1 file changed, 11 insertions(+), 17 deletions(-)
drivers/memory/tegra/tegra20-emc.c +11 -17
index f74dd417c874..60b048ae9982 100644
@@ -7,11 +7,11 @@
 
 #include <linux/clk.h>
 #include <linux/clk/tegra.h>
-#include <linux/completion.h>
 #include <linux/debugfs.h>
 #include <linux/err.h>
 #include <linux/interrupt.h>
 #include <linux/io.h>
+#include <linux/iopoll.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/of.h>

@@ -144,7 +144,6 @@ struct emc_timing {
 
 struct tegra_emc {
 	struct device *dev;
-	struct completion clk_handshake_complete;
 	struct notifier_block clk_nb;
 	struct clk *clk;
 	void __iomem *regs;

@@ -162,17 +161,13 @@ struct tegra_emc {
 static irqreturn_t tegra_emc_isr(int irq, void *data)
 {
 	struct tegra_emc *emc = data;
-	u32 intmask = EMC_REFRESH_OVERFLOW_INT | EMC_CLKCHANGE_COMPLETE_INT;
+	u32 intmask = EMC_REFRESH_OVERFLOW_INT;
 	u32 status;
 
 	status = readl_relaxed(emc->regs + EMC_INTSTATUS) & intmask;
 	if (!status)
 		return IRQ_NONE;
 
-	/* notify about EMC-CAR handshake completion */
-	if (status & EMC_CLKCHANGE_COMPLETE_INT)
-		complete(&emc->clk_handshake_complete);
-
 	/* notify about HW problem */
 	if (status & EMC_REFRESH_OVERFLOW_INT)
 		dev_err_ratelimited(emc->dev,

@@ -224,14 +219,13 @@ static int emc_prepare_timing_change(struct tegra_emc *emc, unsigned long rate)
 	/* wait until programming has settled */
 	readl_relaxed(emc->regs + emc_timing_registers[i - 1]);
 
-	reinit_completion(&emc->clk_handshake_complete);
-
 	return 0;
 }
 
 static int emc_complete_timing_change(struct tegra_emc *emc, bool flush)
 {
-	unsigned long timeout;
+	u32 val;
+	int err;
 
 	dev_dbg(emc->dev, "%s: flush %d\n", __func__, flush);
 

@@ -242,11 +236,12 @@ static int emc_complete_timing_change(struct tegra_emc *emc, bool flush)
 		return 0;
 	}
 
-	timeout = wait_for_completion_timeout(&emc->clk_handshake_complete,
-					      msecs_to_jiffies(100));
-	if (timeout == 0) {
-		dev_err(emc->dev, "EMC-CAR handshake failed\n");
-		return -EIO;
+	err = readl_relaxed_poll_timeout_atomic(emc->regs + EMC_INTSTATUS, val,
+						val & EMC_CLKCHANGE_COMPLETE_INT,
+						1, 100);
+	if (err) {
+		dev_err(emc->dev, "emc-car handshake timeout: %d\n", err);
+		return err;
 	}
 
 	return 0;

@@ -412,7 +407,7 @@ tegra_emc_find_node_by_ram_code(struct device *dev)
 
 static int emc_setup_hw(struct tegra_emc *emc)
 {
-	u32 intmask = EMC_REFRESH_OVERFLOW_INT | EMC_CLKCHANGE_COMPLETE_INT;
+	u32 intmask = EMC_REFRESH_OVERFLOW_INT;
 	u32 emc_cfg, emc_dbg;
 
 	emc_cfg = readl_relaxed(emc->regs + EMC_CFG_2);

@@ -686,7 +681,6 @@ static int tegra_emc_probe(struct platform_device *pdev)
 		return -ENOMEM;
 	}
 
-	init_completion(&emc->clk_handshake_complete);
 	emc->clk_nb.notifier_call = tegra_emc_clk_change_notify;
 	emc->dev = &pdev->dev;
Signature · 1 line
2.25.1
author
#2 lore raw

PATCH 2/2 memory: tegra30-emc: Poll EMC-CaR handshake instead of waiting for interrupt

In reply to a message not in the archive

The memory clock-rate change could be running on a non-boot CPU, while the boot CPU handles the EMC interrupt. This introduces an unnecessary latency since boot CPU should handle the interrupt and then notify the sibling CPU about clock-rate change completion. In some rare cases boot CPU could be in uninterruptible state for a significant time (like in a case of KASAN + NFS root), it could get to the point that completion timeouts before boot CPU gets a chance to handle interrupt. The solution is to get rid of the completion and replace it with interrupt-status polling.

Signed-off-by Dmitry Osipenko <[email protected]>
---
 drivers/memory/tegra/tegra30-emc.c | 116 +++++++++++------------------
 1 file changed, 44 insertions(+), 72 deletions(-)
drivers/memory/tegra/tegra30-emc.c +44 -72
index 727dc7153390..900a291803ca 100644
@@ -11,7 +11,6 @@
 
 #include <linux/clk.h>
 #include <linux/clk/tegra.h>
-#include <linux/completion.h>
 #include <linux/debugfs.h>
 #include <linux/delay.h>
 #include <linux/err.h>

@@ -327,7 +326,6 @@ struct emc_timing {
 struct tegra_emc {
 	struct device *dev;
 	struct tegra_mc *mc;
-	struct completion clk_handshake_complete;
 	struct notifier_block clk_nb;
 	struct clk *clk;
 	void __iomem *regs;

@@ -374,52 +372,10 @@ static int emc_seq_update_timing(struct tegra_emc *emc)
 	return 0;
 }
 
-static void emc_complete_clk_change(struct tegra_emc *emc)
-{
-	struct emc_timing *timing = emc->new_timing;
-	unsigned int dram_num;
-	bool failed = false;
-	int err;
-
-	/* re-enable auto-refresh */
-	dram_num = tegra_mc_get_emem_device_count(emc->mc);
-	writel_relaxed(EMC_REFCTRL_ENABLE_ALL(dram_num),
-		       emc->regs + EMC_REFCTRL);
-
-	/* restore auto-calibration */
-	if (emc->vref_cal_toggle)
-		writel_relaxed(timing->emc_auto_cal_interval,
-			       emc->regs + EMC_AUTO_CAL_INTERVAL);
-
-	/* restore dynamic self-refresh */
-	if (timing->emc_cfg_dyn_self_ref) {
-		emc->emc_cfg |= EMC_CFG_DYN_SREF_ENABLE;
-		writel_relaxed(emc->emc_cfg, emc->regs + EMC_CFG);
-	}
-
-	/* set number of clocks to wait after each ZQ command */
-	if (emc->zcal_long)
-		writel_relaxed(timing->emc_zcal_cnt_long,
-			       emc->regs + EMC_ZCAL_WAIT_CNT);
-
-	/* wait for writes to settle */
-	udelay(2);
-
-	/* update restored timing */
-	err = emc_seq_update_timing(emc);
-	if (err)
-		failed = true;
-
-	/* restore early ACK */
-	mc_writel(emc->mc, emc->mc_override, MC_EMEM_ARB_OVERRIDE);
-
-	WRITE_ONCE(emc->bad_state, failed);
-}
-
 static irqreturn_t tegra_emc_isr(int irq, void *data)
 {
 	struct tegra_emc *emc = data;
-	u32 intmask = EMC_REFRESH_OVERFLOW_INT | EMC_CLKCHANGE_COMPLETE_INT;
+	u32 intmask = EMC_REFRESH_OVERFLOW_INT;
 	u32 status;
 
 	status = readl_relaxed(emc->regs + EMC_INTSTATUS) & intmask;

@@ -434,18 +390,6 @@ static irqreturn_t tegra_emc_isr(int irq, void *data)
 	/* clear interrupts */
 	writel_relaxed(status, emc->regs + EMC_INTSTATUS);
 
-	/* notify about EMC-CAR handshake completion */
-	if (status & EMC_CLKCHANGE_COMPLETE_INT) {
-		if (completion_done(&emc->clk_handshake_complete)) {
-			dev_err_ratelimited(emc->dev,
-					    "bogus handshake interrupt\n");
-			return IRQ_NONE;
-		}
-
-		emc_complete_clk_change(emc);
-		complete(&emc->clk_handshake_complete);
-	}
-
 	return IRQ_HANDLED;
 }
 

@@ -801,29 +745,58 @@ static int emc_prepare_timing_change(struct tegra_emc *emc, unsigned long rate)
 	 */
 	mc_readl(emc->mc, MC_EMEM_ARB_OVERRIDE);
 
-	reinit_completion(&emc->clk_handshake_complete);
-
-	emc->new_timing = timing;
-
 	return 0;
 }
 
 static int emc_complete_timing_change(struct tegra_emc *emc,
 				      unsigned long rate)
 {
-	unsigned long timeout;
+	struct emc_timing *timing = emc_find_timing(emc, rate);
+	unsigned int dram_num;
+	u32 val;
+	int err;
 
-	timeout = wait_for_completion_timeout(&emc->clk_handshake_complete,
-					      msecs_to_jiffies(100));
-	if (timeout == 0) {
-		dev_err(emc->dev, "emc-car handshake failed\n");
-		return -EIO;
+	err = readl_relaxed_poll_timeout_atomic(emc->regs + EMC_INTSTATUS, val,
+						val & EMC_CLKCHANGE_COMPLETE_INT,
+						1, 100);
+	if (err) {
+		dev_err(emc->dev, "emc-car handshake timeout: %d\n", err);
+		return err;
 	}
 
-	if (READ_ONCE(emc->bad_state))
-		return -EIO;
+	/* re-enable auto-refresh */
+	dram_num = tegra_mc_get_emem_device_count(emc->mc);
+	writel_relaxed(EMC_REFCTRL_ENABLE_ALL(dram_num),
+		       emc->regs + EMC_REFCTRL);
+
+	/* restore auto-calibration */
+	if (emc->vref_cal_toggle)
+		writel_relaxed(timing->emc_auto_cal_interval,
+			       emc->regs + EMC_AUTO_CAL_INTERVAL);
 
-	return 0;
+	/* restore dynamic self-refresh */
+	if (timing->emc_cfg_dyn_self_ref) {
+		emc->emc_cfg |= EMC_CFG_DYN_SREF_ENABLE;
+		writel_relaxed(emc->emc_cfg, emc->regs + EMC_CFG);
+	}
+
+	/* set number of clocks to wait after each ZQ command */
+	if (emc->zcal_long)
+		writel_relaxed(timing->emc_zcal_cnt_long,
+			       emc->regs + EMC_ZCAL_WAIT_CNT);
+
+	/* wait for writes to settle */
+	udelay(2);
+
+	/* update restored timing */
+	err = emc_seq_update_timing(emc);
+	if (!err)
+		emc->bad_state = false;
+
+	/* restore early ACK */
+	mc_writel(emc->mc, emc->mc_override, MC_EMEM_ARB_OVERRIDE);
+
+	return err;
 }
 
 static int emc_unprepare_timing_change(struct tegra_emc *emc,

@@ -1033,7 +1006,7 @@ static struct device_node *emc_find_node_by_ram_code(struct device *dev)
 
 static int emc_setup_hw(struct tegra_emc *emc)
 {
-	u32 intmask = EMC_REFRESH_OVERFLOW_INT | EMC_CLKCHANGE_COMPLETE_INT;
+	u32 intmask = EMC_REFRESH_OVERFLOW_INT;
 	u32 fbio_cfg5, emc_cfg, emc_dbg;
 	enum emc_dram_type dram_type;
 

@@ -1321,7 +1294,6 @@ static int tegra_emc_probe(struct platform_device *pdev)
 	if (!emc->mc)
 		return -EPROBE_DEFER;
 
-	init_completion(&emc->clk_handshake_complete);
 	emc->clk_nb.notifier_call = emc_clk_change_notify;
 	emc->dev = &pdev->dev;
Signature · 1 line
2.25.1
Applied
#5 lore raw
In reply to a message not in the archive
Quoted 9 lines · Dmitry Osipenko

The memory clock-rate change could be running on a non-boot CPU, while the boot CPU handles the EMC interrupt. This introduces an unnecessary latency since boot CPU should handle the interrupt and then notify the sibling CPU about clock-rate change completion. In some rare cases boot CPU could be in uninterruptible state for a significant time (like in a case of KASAN + NFS root), it could get to the point that completion timeouts before boot CPU gets a chance to handle interrupt. The solution is to get rid of the completion and replace it with interrupt-status polling.

Quoted 4 lines
Signed-off-by Dmitry Osipenko <[email protected]>
---
 drivers/memory/tegra/tegra20-emc.c | 28 +++++++++++-----------------
 1 file changed, 11 insertions(+), 17 deletions(-)

Applied to for-5.8/memory, thanks.

Thierry

Applied
#4 lore raw

Re: [PATCH v1 2/2] memory: tegra30-emc: Poll EMC-CaR handshake instead of waiting for interrupt

In reply to a message not in the archive
Quoted 9 lines · Dmitry Osipenko

The memory clock-rate change could be running on a non-boot CPU, while the boot CPU handles the EMC interrupt. This introduces an unnecessary latency since boot CPU should handle the interrupt and then notify the sibling CPU about clock-rate change completion. In some rare cases boot CPU could be in uninterruptible state for a significant time (like in a case of KASAN + NFS root), it could get to the point that completion timeouts before boot CPU gets a chance to handle interrupt. The solution is to get rid of the completion and replace it with interrupt-status polling.

Quoted 4 lines
Signed-off-by Dmitry Osipenko <[email protected]>
---
 drivers/memory/tegra/tegra30-emc.c | 116 +++++++++++------------------
 1 file changed, 44 insertions(+), 72 deletions(-)

Applied to for-5.8/memory, thanks.

Thierry