Skip to contentSkip to conversation

memory: tegra114-emc: Fix bus width and pad auto-calibration

PATCH New Patrick Fischer 2 participants 8 messages Nov 11

Subsystems (4)
lore
Share & tools
lore
#0 lore raw

cover memory: tegra114-emc: Fix bus width and pad auto-calibration

On a Tegra Note 7 (Tegra114, P1640) I had kernel panics, bit errors and WLAN firmware hangs under load, and two kernel panics on EMC rate changes from pll_p to 624 MHz on pll_c. Looking at the EMC I found three problems in the Tegra114 EMC driver:

1. EMC_AUTO_CAL_CONFIG is masked down to the self-clearing start bit before it is written, which clears AUTO_CAL_ENABLE on the first rate change. The bootloader leaves calibration enabled, with channel 0 as master and channel 1 as slave. Patch 1 stops clearing the register. Programming the table values needs the channel registers, which patches 4 to 6 describe and use.
2. emc_icc_set() assumes a 32-bit DRAM bus, but Tegra114 has two 32-bit channels. Every interconnect request asks for twice the EMC rate, so actmon at 312 MHz moves the EMC to 624 MHz (patch 2).
3. Pad auto-calibration is disabled for every clock change, but the interval is only restored if EMC_CTT_TERM_CTRL changes, so it stays off after the first change (patch 3).

Patches 1 to 3 are small, do not depend on the device tree change and are marked for stable. Patch 1 comes first on purpose: patch 2 and the clk patch mentioned below make the EMC run at a real 624 MHz on pll_c more often, and both panics I saw on a change to that rate were with the calibration cleared. Patches 4 to 6 are not marked: they need the binding and the device tree. Patches 1 to 5 are for the memory controller tree, patch 6 for the Tegra tree. The driver has been there since v7.2, but the first in-tree Tegra114 device tree with EMC timings (TF701T) came with v7.4-rc1, so the stable tags matter for v7.4 and for out-of-tree device trees.

A separate patch for clk-tegra124-emc.c deals with a related problem:
PLLM is fixed on Tegra114, but the device trees have timings at PLLM rates the bootloader did not set up, and those are loaded with the EMC at another rate. I am sending it to the clk maintainers at the same time. It does not need this series to build, but it should not land in a release without patch 1, for the reason above, so it is not marked for stable and asks to be left out of AUTOSEL.

The Tegra Note 7 device tree is not in mainline yet; I used Svyatoslav's tegratab series on top of v7.3 with this series. Results on the P1640 (Micron MT41K128M16, 1 GB, RAM code 0), with the channel registers in the device tree unless noted:

- EMC rate follows the actmon level (102, 204 and 624 MHz). The timing registers match the table at each rate.
- EMC_AUTO_CAL_CONFIG reads 0x20f10000 and 0x28f10000 in the two channels after rate changes (before: 0), the interval is set (before:
0), and EMC_AUTO_CAL_STATUS keeps changing (before: stuck).
- A memory test of 3 h 20 min on a wall charger (1700 passes of checksums and copies over 160 MB, memory sensor up to 65 degC, EMC at 624 MHz in 1208 of the 1700 passes) ran without errors, the first two hours together with a WLAN upload of 36 GB. That kernel had this series, an unrelated wlcore fix and a debug print.
- With an earlier version that mapped the channel registers at fixed addresses, a memory test of about an hour with the EMC switching between 408 MHz on pll_p and 624 MHz on pll_c, and a 32 minute hold test at 624 MHz (pattern left untouched for 150 s per round, then compared), ran without errors or panics.
- With the same kernel but without the channel registers in the device tree, so that the driver takes the path patch 1 leaves for existing device trees (what a stable kernel with patches 1 to 3 does):
EMC_AUTO_CAL_CONFIG keeps the bootloader values 0x20f10f0f and 0x28f10f0f in the two channels through rate changes between 204, 408 and 624 MHz, EMC_AUTO_CAL_STATUS keeps changing, and a 10 minute memory test switching between 408 MHz on pll_p and 624 MHz on pll_c on every pass (261 passes) ran without errors.

These runs do not prove that the missing calibration caused the earlier panics, since those runs also had the wrong bus width, but I have not seen a panic on a 624 MHz change since. I also do not claim that this series fixes the WLAN hangs: on 7 November the WLAN chip stopped transmitting for about a day with these patches applied and then worked again, for reasons I could not find, and I have not been able to reproduce it. Not tested: Dalmore, Roth, TF701T, suspend and resume.

Mikko, tegra124-emc.c has the same masking of EMC_AUTO_CAL_CONFIG (since 73a7f0a90641). I cannot test on Tegra124 and do not know whether its channels need the same per-channel setup, so I left it alone. Is it worth a look there?

Tool use (see Documentation/process/generated-content.rst): the patches, their changelogs and this cover letter were prepared with an LLM coding assistant (Claude Code). It compared the driver with the downstream tegra11_emc.c and the Tegra 4 TRM, read the EMC registers on the device, wrote the code changes and drafted the texts from my measurements and instructions. I reviewed the changes and ran all tests on the device.

---
Patrick Fischer (6):
memory: tegra114-emc: Stop clearing EMC_AUTO_CAL_CONFIG
memory: tegra114-emc: Use the full DRAM bus width for interconnect requests
memory: tegra114-emc: Always restore the auto-calibration interval
dt-bindings: memory: tegra: Add Tegra114 EMC channel registers
memory: tegra114-emc: Program pad auto-calibration per channel
ARM: tegra: Add EMC channel registers on Tegra114

 .../memory-controllers/nvidia,tegra124-emc.yaml    | 11 ++++++-
 arch/arm/boot/dts/nvidia/tegra114.dtsi             |  3 +-
 drivers/memory/tegra/tegra114-emc.c                | 37 ++++++++++++++++------
 3 files changed, 39 insertions(+), 12 deletions(-)

---
base-commit: 1e88944ecbe9def8aeef898a2e9ce29cf509a9de
change-id: 20261107-upstream-emc-t114-4d53a2a5001d

Best regards,

Signature · 1 line
Patrick Fischer <[email protected]>
#1 lore raw

PATCH 1/6 memory: tegra114-emc: Stop clearing EMC_AUTO_CAL_CONFIG

tegra114_emc_prepare_timing_change() masks the EMC_AUTO_CAL_CONFIG value of the timing with AUTO_CAL_START before writing it, which leaves only the self-clearing start bit. As last_timing starts out zeroed, the first rate change already does this write. From then on AUTO_CAL_ENABLE is off and, according to the Tegra 4 TRM, the pads no longer use the pull-up and pull-down codes from the calibration logic but the static settings from the pad control registers. On a Tegra Note 7 (P1640) the bootloader leaves 0x20f10f0f in channel 0 and 0x28f10f0f in channel 1; after the first rate change both read 0.

The full value cannot be written through the broadcast registers
either: channel 0 runs the calibration and channel 1 is its slave (AUTO_CAL_SLAVE_MODE, bit 27), so a broadcast write would make both channels masters. Leave the register alone, which keeps the calibration the way the bootloader set it up. That value is not necessarily the one in the timing tables: on the P1640 the bootloader uses the pull-up and pull-down offsets that the tables only have at 792 MHz (0xf0f). A later patch programs the table values per channel where the device tree describes the channel registers.

Tested on a Tegra Note 7 (P1640) with this series but without the channel registers in the device tree, which takes the path this patch leaves for existing device trees: both channels keep the bootloader values through rate changes between 204, 408 and 624 MHz, and a 10 minute memory test switching between 408 and 624 MHz ran without errors.

Fixes dce208b5405f ("memory: tegra: Add Tegra114 EMC driver")
---
 drivers/memory/tegra/tegra114-emc.c | 6 ------
 1 file changed, 6 deletions(-)
drivers/memory/tegra/tegra114-emc.c +0 -6
index e50d71842d3a..eb1360627ece 100644
@@ -609,12 +609,6 @@ static int tegra114_emc_prepare_timing_change(struct tegra_emc *emc,
 		emc_ccfifo_writel(emc, timing->emc_auto_cal_config3,
 				  EMC_AUTO_CAL_CONFIG3);
 
-	if (timing->emc_auto_cal_config != last->emc_auto_cal_config) {
-		val = timing->emc_auto_cal_config;
-		val &= EMC_AUTO_CAL_CONFIG_AUTO_CAL_START;
-		emc_ccfifo_writel(emc, val, EMC_AUTO_CAL_CONFIG);
-	}
-
 	/* DDR3: predict MRS long wait count */
 	if (emc->dram_type == DRAM_TYPE_DDR3 &&
 	    dll_change == DLL_CHANGE_ON) {
Signature · 1 line
2.56.0
#2 lore raw

PATCH 2/6 memory: tegra114-emc: Use the full DRAM bus width for interconnect requests

In reply to Patrick Fischer

Tegra114 has two 32-bit EMC channels, so the DRAM data bus is 64 bits
wide: the Tegra 4 TRM describes a "dual-channel (2x 32-bit)" DRAM interface with separate EMC0 and EMC1 register blocks, and the downstream kernel says "On Tegra11 dual channel MC effectively operates as 64-bit bus" and returns 8 from tegra_mc_get_effective_bytes_width() (arch/arm/mach-tegra/include/mach/mc.h, rel-tegranote-r8).

emc_icc_set() divides the requested bandwidth by 4 bytes instead, so every interconnect request asks for twice the EMC rate it needs. The actmon OPP table in tegra114-peripherals-opp.dtsi already uses kBps = kHz x 16, which matches the 64-bit bus: with the current driver actmon at 312 MHz requests an EMC rate of 624 MHz, and actmon at 624 MHz requests 1248 MHz, which ends up at the highest table.

Use 8 bytes. On a Tegra Note 7 (P1640) the EMC rate now follows the actmon level (102 -> 102 MHz, 204 -> 204 MHz, 624 -> 624 MHz).

With the correct width the EMC ends up at a real 624 MHz on pll_c more often. Both kernel panics I saw on a rate change to 624 MHz were with the auto-calibration cleared, so this should not be backported without "memory: tegra114-emc: Stop clearing EMC_AUTO_CAL_CONFIG".

Fixes dce208b5405f ("memory: tegra: Add Tegra114 EMC driver")
---
 drivers/memory/tegra/tegra114-emc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/memory/tegra/tegra114-emc.c +1 -1
index eb1360627ece..6ec1bb2c3ed0 100644
@@ -1163,7 +1163,7 @@ static int emc_icc_set(struct icc_node *src, struct icc_node *dst)
 	unsigned long long peak_bw = icc_units_to_bps(dst->peak_bw);
 	unsigned long long avg_bw = icc_units_to_bps(dst->avg_bw);
 	unsigned long long rate = max(avg_bw, peak_bw);
-	unsigned int dram_data_bus_width_bytes = 4;
+	unsigned int dram_data_bus_width_bytes = 8;
 	const unsigned int ddr = 2;
 	int err;
 
Signature · 1 line
2.56.0
#3 lore raw

PATCH 3/6 memory: tegra114-emc: Always restore the auto-calibration interval

In reply to Patrick Fischer

tegra114_emc_prepare_timing_change() disables pad auto-calibration for every clock change by writing 0 to EMC_AUTO_CAL_INTERVAL. The interval is only written back if EMC_CTT_TERM_CTRL differs between the old and the new timing, which is not the case with the usual tables, so after the first rate change auto-calibration stays off for good. Read back on a Tegra Note 7 (P1640) after a few rate changes, EMC_AUTO_CAL_INTERVAL is 0 and EMC_AUTO_CAL_STATUS no longer changes.

The Tegra124 driver this code is based on writes the interval back unconditionally for DDR3 as part of its BGBIAS handling, which was left out for Tegra114. The downstream Tegra114 driver (tegra11_emc.c, rel-tegranote-r8) also restores it after every clock change.

Restore the interval unconditionally.

Fixes dce208b5405f ("memory: tegra: Add Tegra114 EMC driver")
---
 drivers/memory/tegra/tegra114-emc.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)
drivers/memory/tegra/tegra114-emc.c +3 -5
index 6ec1bb2c3ed0..9de3f2e728ea 100644
@@ -706,7 +706,6 @@ static void tegra114_emc_complete_timing_change(struct tegra_emc *emc,
 						unsigned long rate)
 {
 	struct emc_timing *timing = tegra114_emc_find_timing(emc, rate);
-	struct emc_timing *last = &emc->last_timing;
 
 	if (!timing)
 		return;
@@ -714,10 +713,9 @@ static void tegra114_emc_complete_timing_change(struct tegra_emc *emc,
 	/* Wait until the state machine has settled */
 	emc_seq_wait_clkchange(emc);
 
-	/* Restore AUTO_CAL */
-	if (timing->emc_ctt_term_ctrl != last->emc_ctt_term_ctrl)
-		writel(timing->emc_auto_cal_interval,
-		       emc->regs + EMC_AUTO_CAL_INTERVAL);
+	/* Restore AUTO_CAL, it was disabled for the clock change */
+	writel(timing->emc_auto_cal_interval,
+	       emc->regs + EMC_AUTO_CAL_INTERVAL);
 
 	/* Restore dynamic self-refresh */
 	if (timing->emc_cfg & EMC_CFG_PWR_MASK)
Signature · 1 line
2.56.0
#4 lore raw

PATCH 4/6 dt-bindings: memory: tegra: Add Tegra114 EMC channel registers

In reply to Patrick Fischer

The Tegra114 EMC has two channels of 32 bits. Besides the register block that writes to both channels, each channel has its own block, EMC0 at 0x7001a000 and EMC1 at 0x7001a800 (Tegra 4 TRM address map).

Some settings have to differ between the channels: for pad auto-calibration, channel 0 is the master and channel 1 is the slave (EMC_AUTO_CAL_CONFIG bit 27, AUTO_CAL_SLAVE_MODE), which cannot be programmed through the broadcast block. Allow the two channel blocks as second and third reg entries for Tegra114: a device tree has either only the broadcast block, as the existing ones do, or all three. The meaning follows from the position, so no reg-names are needed. Tegra124 keeps a single entry.

---
 .../bindings/memory-controllers/nvidia,tegra124-emc.yaml      | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)
Documentation/devicetree/bindings/memory-controllers/nvidia,tegra124-emc.yaml +10 -1
index 8e0c67d671f9..c9335483ba85 100644
@@ -25,7 +25,13 @@ properties:
           - const: nvidia,tegra124-emc
 
   reg:
-    maxItems: 1
+    oneOf:
+      - items:
+          - description: EMC registers, writes go to all channels
+      - items:
+          - description: EMC registers, writes go to all channels
+          - description: EMC channel 0 registers
+          - description: EMC channel 1 registers
 
   clocks:
     items:
@@ -218,6 +224,9 @@ allOf:
             enum:
               - nvidia,tegra124-emc
     then:
+      properties:
+        reg:
+          maxItems: 1
       patternProperties:
         "^emc-timings-[0-9]+$":
           patternProperties:
Signature · 1 line
2.56.0
#5 lore raw

Re: [PATCH 4/6] dt-bindings: memory: tegra: Add Tegra114 EMC channel registers

Thank you for your contribution! Sashiko AI review found 1 potential issue to consider: - [Low] dt-bindings: memory: tegra: piling Tegra114 conditionals in tegra124-emc.yaml --

Thank you for your contribution! Sashiko AI review found 1 potential issue to consider:
- [Low] dt-bindings: memory: tegra: piling Tegra114 conditionals in tegra124-emc.yaml
--

commit d5391965f848eab5127dd4add5fff242e933a0a0
Author: Patrick Fischer <[email protected]>

dt-bindings: memory: tegra: Add Tegra114 EMC channel registers

This commit updates the Tegra124 EMC binding schema to allow two optional channel register blocks in the reg property for Tegra114 while restricting Tegra124 to a single reg entry.

Quoted 20 lines
Documentation/devicetree/bindings/memory-controllers/nvidia,tegra124-emc.yaml +7 -1
index 8e0c67d671f91..c9335483ba850 100644
@@ -25,7 +25,13 @@ properties:
           - const: nvidia,tegra124-emc
 
   reg:
-    maxItems: 1
+    oneOf:
+      - items:
+          - description: EMC registers, writes go to all channels
+      - items:
+          - description: EMC registers, writes go to all channels
+          - description: EMC channel 0 registers
+          - description: EMC channel 1 registers
 
   clocks:
     items:

Documentation/devicetree/bindings/memory-controllers/nvidia,tegra124-emc.yaml allOf: · line 232

Quoted 9 lines
Documentation/devicetree/bindings/memory-controllers/nvidia,tegra124-emc.yaml +3 -0
@@ -218,6 +224,9 @@ allOf:
             enum:
               - nvidia,tegra124-emc
     then:
+      properties:
+        reg:
+          maxItems: 1
       patternProperties:
         "^emc-timings-[0-9]+$":
           patternProperties:

[Severity: Low]
This isn't a bug, but would it be cleaner to split nvidia,tegra114-emc into its own schema file rather than adding more per-compatible conditionals to nvidia,tegra124-emc.yaml?

nvidia,tegra124-emc.yaml already relaxes nvidia,emc-configuration to a 97 to 143 entry range and uses an if-then block under allOf to require nvidia,emc-bgbias-ctl0 and nvidia,emc-cfg-2 only on Tegra124. Widening the top-level reg property and constraining it back in the conditional further diverges the two variants, whereas Tegra20, Tegra30, and Tegra210 each use a separate EMC binding schema.

Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=4
#6 lore raw

PATCH 5/6 memory: tegra114-emc: Program pad auto-calibration per channel

In reply to Patrick Fischer

The timing tables carry a value for EMC_AUTO_CAL_CONFIG, but the driver cannot apply it through the broadcast registers: channel 0 runs the calibration and channel 1 takes over its results as a slave (AUTO_CAL_SLAVE_MODE, bit 27), as the values the bootloader leaves show (0x20f10f0f and 0x28f10f0f on a Tegra Note 7, P1640). The downstream driver (tegra11_emc.c, rel-tegranote-r8) programs this register per channel for the same reason.

Map the channel registers when the device tree describes them and write the table value to channel 0 and the same value with the slave bit to channel 1. As in the downstream driver, this happens directly on every rate change, with the AUTO_CAL_START bit from the table. It does not need the CCFIFO: the Tegra 4 TRM lists EMC_AUTO_CAL_CONFIG among the shadow registers that are programmed for the new frequency before the clock change. With an older device tree the register stays as the bootloader set it.

Tested on a Tegra Note 7 (P1640) together with the previous patches:
the channels read 0x20f10000 and 0x28f10000 after rate changes and EMC_AUTO_CAL_STATUS keeps changing. Before, two out of two rate changes from pll_p at 408 MHz to pll_c at 624 MHz ended in a kernel panic; with this series, about an hour of memory testing that switched between 408 and 624 MHz ran without errors. That points at the calibration but does not prove it, as the old runs also had the wrong bus width.

---
 drivers/memory/tegra/tegra114-emc.c | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)
drivers/memory/tegra/tegra114-emc.c +25 -0
index 9de3f2e728ea..d5c3142a0671 100644
@@ -142,6 +142,7 @@
 
 #define EMC_AUTO_CAL_CONFIG			0x2a4
 #define EMC_AUTO_CAL_CONFIG_AUTO_CAL_START	BIT(31)
+#define EMC_AUTO_CAL_CONFIG_SLAVE_MODE		BIT(27)
 #define EMC_AUTO_CAL_INTERVAL			0x2a8
 #define EMC_AUTO_CAL_STATUS			0x2ac
 #define EMC_AUTO_CAL_STATUS_ACTIVE		BIT(31)
@@ -393,6 +394,7 @@ struct tegra_emc {
 	struct device *dev;
 	struct tegra_mc *mc;
 	void __iomem *regs;
+	void __iomem *ch_regs[2];
 	unsigned int irq;
 	struct clk *clk;
 
@@ -609,6 +611,19 @@ static int tegra114_emc_prepare_timing_change(struct tegra_emc *emc,
 		emc_ccfifo_writel(emc, timing->emc_auto_cal_config3,
 				  EMC_AUTO_CAL_CONFIG3);
 
+	/*
+	 * Channel 0 runs the pad calibration and channel 1 takes over its
+	 * results, so the two channels need different values. Without the
+	 * channel registers in the DT, keep what the bootloader set up.
+	 */
+	if (emc->ch_regs[0]) {
+		writel(timing->emc_auto_cal_config,
+		       emc->ch_regs[0] + EMC_AUTO_CAL_CONFIG);
+		writel(timing->emc_auto_cal_config |
+		       EMC_AUTO_CAL_CONFIG_SLAVE_MODE,
+		       emc->ch_regs[1] + EMC_AUTO_CAL_CONFIG);
+	}
+
 	/* DDR3: predict MRS long wait count */
 	if (emc->dram_type == DRAM_TYPE_DDR3 &&
 	    dll_change == DLL_CHANGE_ON) {
@@ -1253,6 +1268,16 @@ static int tegra114_emc_probe(struct platform_device *pdev)
 	if (IS_ERR(emc->regs))
 		return PTR_ERR(emc->regs);
 
+	/* Older device trees only describe the broadcast registers */
+	if (platform_get_resource(pdev, IORESOURCE_MEM, 2)) {
+		for (unsigned int i = 0; i < ARRAY_SIZE(emc->ch_regs); i++) {
+			emc->ch_regs[i] =
+				devm_platform_ioremap_resource(pdev, i + 1);
+			if (IS_ERR(emc->ch_regs[i]))
+				return PTR_ERR(emc->ch_regs[i]);
+		}
+	}
+
 	emc->mc = devm_tegra_memory_controller_get(dev);
 	if (IS_ERR(emc->mc))
 		return PTR_ERR(emc->mc);
Signature · 1 line
2.56.0
#7 lore raw

PATCH 6/6 ARM: tegra: Add EMC channel registers on Tegra114

In reply to Patrick Fischer

Describe the EMC0 and EMC1 register blocks next to the broadcast registers, so that the EMC driver can program the pad auto-calibration of each channel.

---
 arch/arm/boot/dts/nvidia/tegra114.dtsi | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
arch/arm/boot/dts/nvidia/tegra114.dtsi +2 -1
index a5958f3a965b..cf3d98274b36 100644
@@ -804,7 +804,8 @@ mc: memory-controller@70019000 {
 
 	emc: external-memory-controller@7001b000 {
 		compatible = "nvidia,tegra114-emc";
-		reg = <0x7001b000 0x800>;
+		reg = <0x7001b000 0x800>, <0x7001a000 0x800>,
+		      <0x7001a800 0x800>;
 		interrupts = <GIC_SPI 78 IRQ_TYPE_LEVEL_HIGH>;
 		clocks = <&tegra_car TEGRA114_CLK_EMC>;
 		clock-names = "emc";
Signature · 1 line
2.56.0