On Wed, Oct 29, 2025 at 10:42:15PM +0200, Ville Syrjälä wrote:
From: Ville Syrjälä <[email protected]>
Unfortunately the MAD_DIMM DIMM_S and DIMM_L bits on ICL are
not idential, so we are currently decoding DIMM_S incorrectly.
Fix the problem by defining the DIMM_S and DIMM_L bits separately.
And for consistency do that same for SKL, even though there the
bits do match between the two DIMMs. The result is rather
repetitive in places, but I didn't feel like obfuscatign things
with cpp macros/etc.
Broken decoding on Dell XPS 13 7390 2-in-1:
CH0 DIMM L size: 32 Gb, width: X16, ranks: 2, 16Gb+ DIMMs: no
CH0 DIMM S size: 32 Gb, width: X32, ranks: 3, 16Gb+ DIMMs: no
CH0 ranks: 2, 16Gb+ DIMMs: no
CH1 DIMM L size: 32 Gb, width: X16, ranks: 2, 16Gb+ DIMMs: no
CH1 DIMM S size: 32 Gb, width: X32, ranks: 3, 16Gb+ DIMMs: no
CH1 ranks: 2, 16Gb+ DIMMs: no
Memory configuration is symmetric? no
Fixed decoding on Dell XPS 13 7390 2-in-1:
CH0 DIMM L size: 32 Gb, width: X16, ranks: 2, 16Gb+ DIMMs: no
CH0 DIMM S size: 32 Gb, width: X16, ranks: 2, 16Gb+ DIMMs: no
CH0 ranks: 2, 16Gb+ DIMMs: no
CH1 DIMM L size: 32 Gb, width: X16, ranks: 2, 16Gb+ DIMMs: no
CH1 DIMM S size: 32 Gb, width: X16, ranks: 2, 16Gb+ DIMMs: no
CH1 ranks: 2, 16Gb+ DIMMs: no
Memory configuration is symmetric? yes
Signed-off-by: Ville Syrjälä <[email protected]>
---
drivers/gpu/drm/i915/intel_mchbar_regs.h | 53 +++++---
drivers/gpu/drm/i915/soc/intel_dram.c | 166 +++++++++++++++++------
2 files changed, 155 insertions(+), 64 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_mchbar_regs.h
b/drivers/gpu/drm/i915/intel_mchbar_regs.h
index a46a45b9d2e1..614d4017b57b 100644
--- a/drivers/gpu/drm/i915/intel_mchbar_regs.h
+++ b/drivers/gpu/drm/i915/intel_mchbar_regs.h
@@ -160,25 +160,40 @@
#define SKL_MAD_DIMM_CH0_0_0_0_MCHBAR_MCMAIN _MMIO(MCHBAR_MIRROR_BASE_SNB +
0x500C)
#define SKL_MAD_DIMM_CH1_0_0_0_MCHBAR_MCMAIN _MMIO(MCHBAR_MIRROR_BASE_SNB +
0x5010)
-#define SKL_DRAM_S_SHIFT 16
-#define SKL_DRAM_RANK_MASK REG_GENMASK(10, 10)
-#define SKL_DRAM_RANK_1
REG_FIELD_PREP(SKL_DRAM_RANK_MASK, 0)
-#define SKL_DRAM_RANK_2
REG_FIELD_PREP(SKL_DRAM_RANK_MASK, 1)
-#define SKL_DRAM_WIDTH_MASK REG_GENMASK(9, 8)
-#define SKL_DRAM_WIDTH_X8
REG_FIELD_PREP(SKL_DRAM_WIDTH_MASK, 0)
-#define SKL_DRAM_WIDTH_X16
REG_FIELD_PREP(SKL_DRAM_WIDTH_MASK, 1)
-#define SKL_DRAM_WIDTH_X32
REG_FIELD_PREP(SKL_DRAM_WIDTH_MASK, 2)
-#define SKL_DRAM_SIZE_MASK REG_GENMASK(5, 0)
-#define ICL_DRAM_RANK_MASK REG_GENMASK(10, 9)
-#define ICL_DRAM_RANK_1
REG_FIELD_PREP(ICL_DRAM_RANK_MASK, 0)
-#define ICL_DRAM_RANK_2
REG_FIELD_PREP(ICL_DRAM_RANK_MASK, 1)
-#define ICL_DRAM_RANK_3
REG_FIELD_PREP(ICL_DRAM_RANK_MASK, 2)
-#define ICL_DRAM_RANK_4
REG_FIELD_PREP(ICL_DRAM_RANK_MASK, 3)
-#define ICL_DRAM_WIDTH_MASK REG_GENMASK(8, 7)
-#define ICL_DRAM_WIDTH_X8
REG_FIELD_PREP(ICL_DRAM_WIDTH_MASK, 0)
-#define ICL_DRAM_WIDTH_X16
REG_FIELD_PREP(ICL_DRAM_WIDTH_MASK, 1)
-#define ICL_DRAM_WIDTH_X32
REG_FIELD_PREP(ICL_DRAM_WIDTH_MASK, 2)
-#define ICL_DRAM_SIZE_MASK REG_GENMASK(6, 0)
+#define SKL_DIMM_S_RANK_MASK REG_GENMASK(26, 26)
+#define SKL_DIMM_S_RANK_1
REG_FIELD_PREP(SKL_DIMM_S_RANK_MASK, 0)
+#define SKL_DIMM_S_RANK_2
REG_FIELD_PREP(SKL_DIMM_S_RANK_MASK, 1)
+#define SKL_DIMM_S_WIDTH_MASK REG_GENMASK(25, 24)
+#define SKL_DIMM_S_WIDTH_X8
REG_FIELD_PREP(SKL_DIMM_S_WIDTH_MASK, 0)
+#define SKL_DIMM_S_WIDTH_X16
REG_FIELD_PREP(SKL_DIMM_S_WIDTH_MASK, 1)
+#define SKL_DIMM_S_WIDTH_X32
REG_FIELD_PREP(SKL_DIMM_S_WIDTH_MASK, 2)
+#define SKL_DIMM_S_SIZE_MASK REG_GENMASK(21, 16)
+#define SKL_DIMM_L_RANK_MASK REG_GENMASK(10, 10)
+#define SKL_DIMM_L_RANK_1
REG_FIELD_PREP(SKL_DIMM_L_RANK_MASK, 0)
+#define SKL_DIMM_L_RANK_2
REG_FIELD_PREP(SKL_DIMM_L_RANK_MASK, 1)
+#define SKL_DIMM_L_WIDTH_MASK REG_GENMASK(9, 8)
+#define SKL_DIMM_L_WIDTH_X8
REG_FIELD_PREP(SKL_DIMM_L_WIDTH_MASK, 0)
+#define SKL_DIMM_L_WIDTH_X16
REG_FIELD_PREP(SKL_DIMM_L_WIDTH_MASK, 1)
+#define SKL_DIMM_L_WIDTH_X32
REG_FIELD_PREP(SKL_DIMM_L_WIDTH_MASK, 2)
+#define SKL_DIMM_L_SIZE_MASK REG_GENMASK(5, 0)
+#define ICL_DIMM_S_RANK_MASK REG_GENMASK(27, 26)
+#define ICL_DIMM_S_RANK_1
REG_FIELD_PREP(ICL_DIMM_S_RANK_MASK, 0)
+#define ICL_DIMM_S_RANK_2
REG_FIELD_PREP(ICL_DIMM_S_RANK_MASK, 1)
+#define ICL_DIMM_S_WIDTH_MASK REG_GENMASK(25, 24)
+#define ICL_DIMM_S_WIDTH_X8
REG_FIELD_PREP(ICL_DIMM_S_WIDTH_MASK, 0)
+#define ICL_DIMM_S_WIDTH_X16
REG_FIELD_PREP(ICL_DIMM_S_WIDTH_MASK, 1)
+#define ICL_DIMM_S_WIDTH_X32
REG_FIELD_PREP(ICL_DIMM_S_WIDTH_MASK, 2)
+#define ICL_DIMM_S_SIZE_MASK REG_GENMASK(22, 16)
+#define ICL_DIMM_L_RANK_MASK REG_GENMASK(10, 9)
+#define ICL_DIMM_L_RANK_1
REG_FIELD_PREP(ICL_DIMM_L_RANK_MASK, 0)
+#define ICL_DIMM_L_RANK_2
REG_FIELD_PREP(ICL_DIMM_L_RANK_MASK, 1)
+#define ICL_DIMM_L_RANK_3
REG_FIELD_PREP(ICL_DIMM_L_RANK_MASK, 2)
+#define ICL_DIMM_L_RANK_4
REG_FIELD_PREP(ICL_DIMM_L_RANK_MASK, 3)
+#define ICL_DIMM_L_WIDTH_MASK REG_GENMASK(8, 7)
+#define ICL_DIMM_L_WIDTH_X8
REG_FIELD_PREP(ICL_DIMM_L_WIDTH_MASK, 0)
+#define ICL_DIMM_L_WIDTH_X16
REG_FIELD_PREP(ICL_DIMM_L_WIDTH_MASK, 1)
+#define ICL_DIMM_L_WIDTH_X32
REG_FIELD_PREP(ICL_DIMM_L_WIDTH_MASK, 2)
+#define ICL_DIMM_L_SIZE_MASK REG_GENMASK(6, 0)
so we have:
ICL_DIMM_L_RANK_MASK REG_GENMASK(10 , 9)
ICL_DIMM_L_WIDTH_MASK REG_GENMASK(8 , 7)
ICL_DIMM_L_SIZE_MASK REG_GENMASK(6 , 0)
vs
ICL_DIMM_S_RANK_MASK REG_GENMASK(11 + 16, 10 + 16)
ICL_DIMM_S_WIDTH_MASK REG_GENMASK(9 + 16, 8 + 16)
ICL_DIMM_S_SIZE_MASK REG_GENMASK(6 + 16, 0 + 16)
I can't really check the soc spec right now, but this fix seems very
verbose. Maybe the first patch could fix the bug in the simple way and
then have these refactors on top?
u16 lval = val & 0xffff;
u16 sval = val >> 16;
/*
* Some cursing here for format change - "fix" it up by making it compatible
* with the lower bits by doing shr where appropriate
*/
sval = (sval & ICL_DIMM_L_SIZE_MASK) |
((sval >> 1) & ICL_DIMM_L_WIDTH_MASK) |
((sval >> 1) & ICL_DIMM_L_RANK_MASK);
...
static int
skl_dram_get_channel_info(struct drm_i915_private *i915,
struct dram_channel_info *ch,
int channel, u32 val)
{
- skl_dram_get_dimm_info(i915, &ch->dimm_l,
- channel, 'L', val & 0xffff);
- skl_dram_get_dimm_info(i915, &ch->dimm_s,
- channel, 'S', val >> 16);
Which would mean basically changing this function to derive sval and
lval as above (untested). This could easily propagate to through stable.
Lucas De Marchi
