From: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>

Most modern Qualcomm platforms (>= SM8150) expose information about the
DDR memory present on the system via SMEM.

Details from this information is used in various scenarios, such as
multimedia drivers configuring the hardware based on the "Highest Bank
address Bit" (hbb), or the list of valid frequencies in validation
scenarios...

Add support for parsing v3-v7 version of the structs. Unforunately,
they are not versioned, so some elbow grease is necessary to determine
which one is present. See for reference:

ver 3: https://git.codelinaro.org/clo/la/abl/tianocore/edk2/-/commit/1d11897d2cfcc7b85f28ff74c445018dbbecac7a
ver 4: https://git.codelinaro.org/clo/la/abl/tianocore/edk2/-/commit/f6e9aa549260bbc0bdcb156c2b05f48dc5963203
ver 5: https://git.codelinaro.org/clo/la/abl/tianocore/edk2/-/commit/617d3297abe8b1b8dd3de3d1dd69c3961e6f343f
ver 5 with 6regions: https://git.codelinaro.org/clo/la/abl/tianocore/edk2/-/commit/d770e009f9bae58d56d926f7490bbfb45af8341f
ver 6: https://git.codelinaro.org/clo/la/abl/tianocore/edk2/-/commit/62659b557fdb1551b20fae8073d1d701dfa8a62e
ver 7: https://git.codelinaro.org/clo/la/abl/tianocore/edk2/-/commit/734d95599c5ebb1ca0d4e1639142e65c590532b7

Reviewed-by: Bjorn Andersson <andersson@kernel.org>
Signed-off-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
---
 drivers/soc/qcom/Makefile     |   3 +-
 drivers/soc/qcom/smem.c       |  14 +-
 drivers/soc/qcom/smem.h       |   9 +
 drivers/soc/qcom/smem_dramc.c | 408 ++++++++++++++++++++++++++++++++++++++++++
 include/linux/soc/qcom/smem.h |   4 +
 5 files changed, 436 insertions(+), 2 deletions(-)

diff --git a/drivers/soc/qcom/Makefile b/drivers/soc/qcom/Makefile
index b7f1d2a57367..798643be3590 100644
--- a/drivers/soc/qcom/Makefile
+++ b/drivers/soc/qcom/Makefile
@@ -23,7 +23,8 @@ obj-$(CONFIG_QCOM_RPMH)		+= qcom_rpmh.o
 qcom_rpmh-y			+= rpmh-rsc.o
 qcom_rpmh-y			+= rpmh.o
 obj-$(CONFIG_QCOM_SMD_RPM)	+= rpm-proc.o smd-rpm.o
-obj-$(CONFIG_QCOM_SMEM) +=	smem.o
+qcom_smem-y			+= smem.o smem_dramc.o
+obj-$(CONFIG_QCOM_SMEM) +=	qcom_smem.o
 obj-$(CONFIG_QCOM_SMEM_STATE) += smem_state.o
 CFLAGS_smp2p.o := -I$(src)
 obj-$(CONFIG_QCOM_SMP2P)	+= smp2p.o
diff --git a/drivers/soc/qcom/smem.c b/drivers/soc/qcom/smem.c
index 088b2bbee9e6..a53bf9ed8e92 100644
--- a/drivers/soc/qcom/smem.c
+++ b/drivers/soc/qcom/smem.c
@@ -4,6 +4,7 @@
  * Copyright (c) 2012-2013, The Linux Foundation. All rights reserved.
  */
 
+#include <linux/debugfs.h>
 #include <linux/hwspinlock.h>
 #include <linux/io.h>
 #include <linux/module.h>
@@ -16,6 +17,8 @@
 #include <linux/soc/qcom/smem.h>
 #include <linux/soc/qcom/socinfo.h>
 
+#include "smem.h"
+
 /*
  * The Qualcomm shared memory system is a allocate only heap structure that
  * consists of one of more memory areas that can be accessed by the processors
@@ -284,6 +287,8 @@ struct qcom_smem {
 	struct smem_partition global_partition;
 	struct smem_partition partitions[SMEM_HOST_COUNT];
 
+	struct dentry *debugfs_dir;
+
 	unsigned num_regions;
 	struct smem_region regions[] __counted_by(num_regions);
 };
@@ -1236,17 +1241,24 @@ static int qcom_smem_probe(struct platform_device *pdev)
 
 	__smem = smem;
 
+	smem->debugfs_dir = smem_dram_parse(smem->dev);
+
 	smem->socinfo = platform_device_register_data(&pdev->dev, "qcom-socinfo",
 						      PLATFORM_DEVID_NONE, NULL,
 						      0);
-	if (IS_ERR(smem->socinfo))
+	if (IS_ERR(smem->socinfo)) {
+		debugfs_remove_recursive(smem->debugfs_dir);
+
 		dev_dbg(&pdev->dev, "failed to register socinfo device\n");
+	}
 
 	return 0;
 }
 
 static void qcom_smem_remove(struct platform_device *pdev)
 {
+	debugfs_remove_recursive(__smem->debugfs_dir);
+
 	platform_device_unregister(__smem->socinfo);
 
 	__smem = NULL;
diff --git a/drivers/soc/qcom/smem.h b/drivers/soc/qcom/smem.h
new file mode 100644
index 000000000000..8bf3f606e1ae
--- /dev/null
+++ b/drivers/soc/qcom/smem.h
@@ -0,0 +1,9 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef __QCOM_SMEM_INTERNAL__
+#define __QCOM_SMEM_INTERNAL__
+
+#include <linux/device.h>
+
+struct dentry *smem_dram_parse(struct device *dev);
+
+#endif
diff --git a/drivers/soc/qcom/smem_dramc.c b/drivers/soc/qcom/smem_dramc.c
new file mode 100644
index 000000000000..017bb894a91b
--- /dev/null
+++ b/drivers/soc/qcom/smem_dramc.c
@@ -0,0 +1,408 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (c) 2025 Qualcomm Innovation Center, Inc. All rights reserved.
+ */
+
+#include <linux/debugfs.h>
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/of_device.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/soc/qcom/smem.h>
+#include <linux/units.h>
+#include <linux/soc/qcom/smem.h>
+
+#include "smem.h"
+
+#define SMEM_DDR_INFO_ID		603
+
+#define MAX_DDR_FREQ_NUM_V3		13
+#define MAX_DDR_FREQ_NUM_V5		14
+
+#define MAX_CHAN_NUM			8
+#define MAX_RANK_NUM			2
+
+#define DDR_HBB_MIN			13
+#define DDR_HBB_MAX			19
+
+#define MAX_SHUB_ENTRIES		8
+
+static struct smem_dram *__dram;
+
+enum ddr_info_version {
+	INFO_UNKNOWN,
+	INFO_V3,
+	INFO_V3_WITH_14_FREQS,
+	INFO_V4,
+	INFO_V5,
+	INFO_V5_WITH_6_REGIONS,
+	INFO_V6, /* INFO_V6 seems to only have shipped with 6 DDR regions, unlike V7 */
+	INFO_V7,
+	INFO_V7_WITH_6_REGIONS,
+};
+
+struct smem_dram {
+	unsigned long frequencies[MAX_DDR_FREQ_NUM_V5];
+	u32 num_frequencies;
+	u8 hbb;
+};
+
+enum ddr_type {
+	DDR_TYPE_NODDR = 0,
+	DDR_TYPE_LPDDR1 = 1,
+	DDR_TYPE_LPDDR2 = 2,
+	DDR_TYPE_PCDDR2 = 3,
+	DDR_TYPE_PCDDR3 = 4,
+	DDR_TYPE_LPDDR3 = 5,
+	DDR_TYPE_LPDDR4 = 6,
+	DDR_TYPE_LPDDR4X = 7,
+	DDR_TYPE_LPDDR5 = 8,
+	DDR_TYPE_LPDDR5X = 9,
+};
+
+/* The data structures below are NOT __packed on purpose! */
+
+/* Structs used across multiple versions */
+struct ddr_part_details {
+	__le16 revision_id1;
+	__le16 revision_id2;
+	__le16 width;
+	__le16 density;
+};
+
+struct ddr_freq_table {
+	u32 freq_khz;
+	u8 enabled;
+};
+
+/* V3 */
+struct ddr_freq_plan_v3 {
+	struct ddr_freq_table ddr_freq[MAX_DDR_FREQ_NUM_V3]; /* NOTE: some have 14 like v5 */
+	u8 num_ddr_freqs;
+	phys_addr_t clk_period_address;
+};
+
+struct ddr_details_v3 {
+	u8 manufacturer_id;
+	u8 device_type;
+	struct ddr_part_details ddr_params[MAX_CHAN_NUM];
+	struct ddr_freq_plan_v3 ddr_freq_tbl;
+	u8 num_channels;
+};
+
+/* V4 */
+struct ddr_details_v4 {
+	u8 manufacturer_id;
+	u8 device_type;
+	struct ddr_part_details ddr_params[MAX_CHAN_NUM];
+	struct ddr_freq_plan_v3 ddr_freq_tbl;
+	u8 num_channels;
+	u8 num_ranks[MAX_CHAN_NUM];
+	u8 highest_bank_addr_bit[MAX_CHAN_NUM][MAX_RANK_NUM];
+};
+
+/* V5 */
+struct shub_freq_table {
+	u8 enable;
+	u32 freq_khz;
+};
+
+struct shub_freq_plan_entry {
+	u8 num_shub_freqs;
+	struct shub_freq_table shub_freq[MAX_SHUB_ENTRIES];
+};
+
+struct ddr_xbl2quantum_smem_data {
+	phys_addr_t ssr_cookie_addr;
+	u32 reserved[10];
+};
+
+struct ddr_freq_plan_v5 {
+	struct ddr_freq_table ddr_freq[MAX_DDR_FREQ_NUM_V5];
+	u8 num_ddr_freqs;
+	phys_addr_t clk_period_address;
+	u32 max_nom_ddr_freq;
+};
+
+struct ddr_region_v5 {
+	u64 start_address;
+	u64 size;
+	u64 mem_controller_address;
+	u32 granule_size; /* MiB */
+	u8  ddr_rank;
+#define DDR_RANK_0	BIT(0)
+#define DDR_RANK_1	BIT(1)
+	u8  segments_start_index;
+	u64 segments_start_offset;
+};
+
+struct ddr_regions_v5 {
+	u32 ddr_region_num; /* We expect this to always be 4 or 6 */
+	u64 ddr_rank0_size;
+	u64 ddr_rank1_size;
+	u64 ddr_cs0_start_addr;
+	u64 ddr_cs1_start_addr;
+	u32 highest_bank_addr_bit;
+	struct ddr_region_v5 ddr_region[] __counted_by(ddr_region_num);
+};
+
+struct ddr_details_v5 {
+	u8 manufacturer_id;
+	u8 device_type;
+	struct ddr_part_details ddr_params[MAX_CHAN_NUM];
+	struct ddr_freq_plan_v5 ddr_freq_tbl;
+	u8 num_channels;
+	u8 _padding;
+	struct ddr_regions_v5 ddr_regions;
+};
+
+/* V6 */
+struct ddr_misc_info_v6 {
+	u32 dsf_version;
+	u32 reserved[10];
+};
+
+/* V7 */
+struct ddr_details_v7 {
+	u8 manufacturer_id;
+	u8 device_type;
+	struct ddr_part_details ddr_params[MAX_CHAN_NUM];
+	struct ddr_freq_plan_v5 ddr_freq_tbl;
+	u8 num_channels;
+	u8 sct_config;
+	struct ddr_regions_v5 ddr_regions;
+};
+
+/**
+ * qcom_smem_dram_get_hbb(): Get the Highest bank address bit
+ *
+ * Context: Check qcom_smem_is_available() before calling this function.
+ * Because __dram * is initialized by smem_dram_parse(), which is in turn
+ * called from * qcom_smem_probe(), __dram will only be NULL if the data
+ * couldn't have been found/interpreted correctly.
+ *
+ * Return: 0 on success, -ENODATA on failure.
+ */
+int qcom_smem_dram_get_hbb(void)
+{
+	int hbb;
+
+	if (!__dram)
+		return -ENODATA;
+
+	hbb = __dram->hbb;
+	if (hbb == 0)
+		return -ENODATA;
+	else if (hbb < DDR_HBB_MIN || hbb > DDR_HBB_MAX)
+		return -EINVAL;
+
+	return hbb;
+}
+EXPORT_SYMBOL_GPL(qcom_smem_dram_get_hbb);
+
+static void smem_dram_parse_v3_data(struct smem_dram *dram, void *data, bool additional_freq_entry)
+{
+	/* This may be 13 or 14 */
+	int num_freq_entries = MAX_DDR_FREQ_NUM_V3;
+	struct ddr_details_v3 *details = data;
+
+	if (additional_freq_entry)
+		num_freq_entries++;
+
+	for (int i = 0; i < num_freq_entries; i++) {
+		struct ddr_freq_table *freq_entry = &details->ddr_freq_tbl.ddr_freq[i];
+
+		if (freq_entry->freq_khz && freq_entry->enabled)
+			dram->frequencies[dram->num_frequencies++] = 1000 * freq_entry->freq_khz;
+	}
+}
+
+static void smem_dram_parse_v4_data(struct smem_dram *dram, void *data)
+{
+	struct ddr_details_v4 *details = data;
+
+	/* Rank 0 channel 0 entry holds the correct value */
+	dram->hbb = details->highest_bank_addr_bit[0][0];
+
+	for (int i = 0; i < MAX_DDR_FREQ_NUM_V3; i++) {
+		struct ddr_freq_table *freq_entry = &details->ddr_freq_tbl.ddr_freq[i];
+
+		if (freq_entry->freq_khz && freq_entry->enabled)
+			dram->frequencies[dram->num_frequencies++] = 1000 * freq_entry->freq_khz;
+	}
+}
+
+static void smem_dram_parse_v5_data(struct smem_dram *dram, void *data)
+{
+	struct ddr_details_v5 *details = data;
+	struct ddr_regions_v5 *region = &details->ddr_regions;
+
+	dram->hbb = region[0].highest_bank_addr_bit;
+
+	for (int i = 0; i < MAX_DDR_FREQ_NUM_V5; i++) {
+		struct ddr_freq_table *freq_entry = &details->ddr_freq_tbl.ddr_freq[i];
+
+		if (freq_entry->freq_khz && freq_entry->enabled)
+			dram->frequencies[dram->num_frequencies++] = 1000 * freq_entry->freq_khz;
+	}
+}
+
+static void smem_dram_parse_v7_data(struct smem_dram *dram, void *data)
+{
+	struct ddr_details_v7 *details = data;
+	struct ddr_regions_v5 *region = &details->ddr_regions;
+
+	dram->hbb = region[0].highest_bank_addr_bit;
+
+	for (int i = 0; i < MAX_DDR_FREQ_NUM_V5; i++) {
+		struct ddr_freq_table *freq_entry = &details->ddr_freq_tbl.ddr_freq[i];
+
+		if (freq_entry->freq_khz && freq_entry->enabled)
+			dram->frequencies[dram->num_frequencies++] = 1000 * freq_entry->freq_khz;
+	}
+}
+
+/* The structure contains no version field, so we have to perform some guesswork.. */
+static int smem_dram_infer_struct_version(size_t size)
+{
+	/* Some early versions provided less bytes of less useful data */
+	if (size < sizeof(struct ddr_details_v3))
+		return -EINVAL;
+
+	if (size == sizeof(struct ddr_details_v3))
+		return INFO_V3;
+
+	if (size == sizeof(struct ddr_details_v3)
+		 + sizeof(struct ddr_freq_table))
+		return INFO_V3_WITH_14_FREQS;
+
+	if (size == sizeof(struct ddr_details_v4))
+		return INFO_V4;
+
+	if (size == sizeof(struct ddr_details_v5)
+		 + 4 * sizeof(struct ddr_region_v5))
+		return INFO_V5;
+
+	if (size == sizeof(struct ddr_details_v5)
+		 + 4 * sizeof(struct ddr_region_v5)
+		 + sizeof(struct ddr_xbl2quantum_smem_data)
+		 + sizeof(struct shub_freq_plan_entry))
+		return INFO_V5;
+
+	if (size == sizeof(struct ddr_details_v5)
+		 + 6 * sizeof(struct ddr_region_v5))
+		return INFO_V5_WITH_6_REGIONS;
+
+	if (size == sizeof(struct ddr_details_v5)
+		 + 6 * sizeof(struct ddr_region_v5)
+		 + sizeof(struct ddr_xbl2quantum_smem_data)
+		 + sizeof(struct shub_freq_plan_entry))
+		return INFO_V5_WITH_6_REGIONS;
+
+	if (size == sizeof(struct ddr_details_v5)
+		 + 6 * sizeof(struct ddr_region_v5)
+		 + sizeof(struct ddr_misc_info_v6)
+		 + sizeof(struct shub_freq_plan_entry))
+		return INFO_V6;
+
+	if (size == sizeof(struct ddr_details_v7)
+		 + 4 * sizeof(struct ddr_region_v5)
+		 + sizeof(struct ddr_misc_info_v6)
+		 + sizeof(struct shub_freq_plan_entry))
+		return INFO_V7;
+
+	if (size == sizeof(struct ddr_details_v7)
+		 + 6 * sizeof(struct ddr_region_v5)
+		 + sizeof(struct ddr_misc_info_v6)
+		 + sizeof(struct shub_freq_plan_entry))
+		return INFO_V7_WITH_6_REGIONS;
+
+	return INFO_UNKNOWN;
+}
+
+static int smem_dram_frequencies_show(struct seq_file *s, void *unused)
+{
+	struct smem_dram *dram = s->private;
+
+	for (int i = 0; i < dram->num_frequencies; i++)
+		seq_printf(s, "%lu\n", dram->frequencies[i]);
+
+	return 0;
+}
+DEFINE_SHOW_ATTRIBUTE(smem_dram_frequencies);
+
+static int smem_hbb_show(struct seq_file *s, void *unused)
+{
+	struct smem_dram *dram = s->private;
+
+	if (!dram->hbb)
+		return -EINVAL;
+
+	seq_printf(s, "%d\n", dram->hbb);
+
+	return 0;
+}
+DEFINE_SHOW_ATTRIBUTE(smem_hbb);
+
+struct dentry *smem_dram_parse(struct device *dev)
+{
+	struct dentry *debugfs_dir;
+	enum ddr_info_version ver;
+	struct smem_dram *dram;
+	size_t actual_size;
+	void *data = NULL;
+
+	/* No need to check qcom_smem_is_available(), this func is called by the SMEM driver */
+	data = qcom_smem_get(QCOM_SMEM_HOST_ANY, SMEM_DDR_INFO_ID, &actual_size);
+	if (IS_ERR_OR_NULL(data))
+		return ERR_PTR(-ENODATA);
+
+	ver = smem_dram_infer_struct_version(actual_size);
+	if (ver < 0) {
+		/* Some SoCs don't provide data that's useful for us */
+		return ERR_PTR(-ENODATA);
+	} else if (ver == INFO_UNKNOWN) {
+		/* In other cases, we may not have added support for a newer struct revision */
+		pr_err("Found an unknown type of DRAM info struct (size = %zu)\n", actual_size);
+		return ERR_PTR(-EINVAL);
+	}
+
+	dram = devm_kzalloc(dev, sizeof(*dram), GFP_KERNEL);
+	if (!dram)
+		return ERR_PTR(-ENOMEM);
+
+	switch (ver) {
+	case INFO_V3:
+		smem_dram_parse_v3_data(dram, data, false);
+		break;
+	case INFO_V3_WITH_14_FREQS:
+		smem_dram_parse_v3_data(dram, data, true);
+		break;
+	case INFO_V4:
+		smem_dram_parse_v4_data(dram, data);
+		break;
+	case INFO_V5:
+	case INFO_V5_WITH_6_REGIONS:
+	case INFO_V6:
+		smem_dram_parse_v5_data(dram, data);
+		break;
+	case INFO_V7:
+	case INFO_V7_WITH_6_REGIONS:
+		smem_dram_parse_v7_data(dram, data);
+		break;
+	default:
+		return ERR_PTR(-EINVAL);
+	}
+
+	/* Both the entry and its parent dir will be cleaned up by debugfs_remove_recursive */
+	debugfs_dir = debugfs_create_dir("qcom_smem", NULL);
+	debugfs_create_file("dram_frequencies", 0444, debugfs_dir, dram,
+			    &smem_dram_frequencies_fops);
+	debugfs_create_file("hbb", 0444, debugfs_dir, dram, &smem_hbb_fops);
+
+	/* If there was no failure so far, assign the global variable */
+	__dram = dram;
+
+	return debugfs_dir;
+}
diff --git a/include/linux/soc/qcom/smem.h b/include/linux/soc/qcom/smem.h
index f946e3beca21..223cd5090a2a 100644
--- a/include/linux/soc/qcom/smem.h
+++ b/include/linux/soc/qcom/smem.h
@@ -2,6 +2,8 @@
 #ifndef __QCOM_SMEM_H__
 #define __QCOM_SMEM_H__
 
+#include <linux/platform_device.h>
+
 #define QCOM_SMEM_HOST_ANY -1
 
 bool qcom_smem_is_available(void);
@@ -17,4 +19,6 @@ int qcom_smem_get_feature_code(u32 *code);
 
 int qcom_smem_bust_hwspin_lock_by_host(unsigned int host);
 
+int qcom_smem_dram_get_hbb(void);
+
 #endif

-- 
2.52.0

Hi Konrad,

kernel test robot noticed the following build warnings:

[auto build test WARNING on fc4e91c639c0af93d63c3d5bc0ee45515dd7504a]

url:    https://github.com/intel-lab-lkp/linux/commits/Konrad-Dybcio/soc-qcom-smem-Expose-DDR-data-from-SMEM/20260108-222445
base:   fc4e91c639c0af93d63c3d5bc0ee45515dd7504a
patch link:    https://lore.kernel.org/r/20260108-topic-smem_dramc-v3-1-6b64df58a017%40oss.qualcomm.com
patch subject: [PATCH v3 1/3] soc: qcom: smem: Expose DDR data from SMEM
config: m68k-allyesconfig (https://download.01.org/0day-ci/archive/20260115/202601150105.Pod3agMP-lkp@intel.com/config)
compiler: m68k-linux-gcc (GCC) 15.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20260115/202601150105.Pod3agMP-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202601150105.Pod3agMP-lkp@intel.com/

All warnings (new ones prefixed by >>):

   In function 'smem_dram_parse_v3_data',
       inlined from 'smem_dram_parse' at drivers/soc/qcom/smem_dramc.c:380:3:
>> drivers/soc/qcom/smem_dramc.c:216:31: warning: iteration 13 invokes undefined behavior [-Waggressive-loop-optimizations]
     216 |                 if (freq_entry->freq_khz && freq_entry->enabled)
         |                     ~~~~~~~~~~^~~~~~~~~~
   drivers/soc/qcom/smem_dramc.c:213:27: note: within this loop
     213 |         for (int i = 0; i < num_freq_entries; i++) {
         |                         ~~^~~~~~~~~~~~~~~~~~
--
>> Warning: drivers/soc/qcom/smem.c:293 struct member 'debugfs_dir' not described in 'qcom_smem'
>> Warning: drivers/soc/qcom/smem.c:293 struct member 'debugfs_dir' not described in 'qcom_smem'


vim +216 drivers/soc/qcom/smem_dramc.c

   203	
   204	static void smem_dram_parse_v3_data(struct smem_dram *dram, void *data, bool additional_freq_entry)
   205	{
   206		/* This may be 13 or 14 */
   207		int num_freq_entries = MAX_DDR_FREQ_NUM_V3;
   208		struct ddr_details_v3 *details = data;
   209	
   210		if (additional_freq_entry)
   211			num_freq_entries++;
   212	
   213		for (int i = 0; i < num_freq_entries; i++) {
   214			struct ddr_freq_table *freq_entry = &details->ddr_freq_tbl.ddr_freq[i];
   215	
 > 216			if (freq_entry->freq_khz && freq_entry->enabled)
   217				dram->frequencies[dram->num_frequencies++] = 1000 * freq_entry->freq_khz;
   218		}
   219	}
   220	
   221	static void smem_dram_parse_v4_data(struct smem_dram *dram, void *data)
   222	{
   223		struct ddr_details_v4 *details = data;
   224	
   225		/* Rank 0 channel 0 entry holds the correct value */
   226		dram->hbb = details->highest_bank_addr_bit[0][0];
   227	
   228		for (int i = 0; i < MAX_DDR_FREQ_NUM_V3; i++) {
   229			struct ddr_freq_table *freq_entry = &details->ddr_freq_tbl.ddr_freq[i];
   230	
   231			if (freq_entry->freq_khz && freq_entry->enabled)
   232				dram->frequencies[dram->num_frequencies++] = 1000 * freq_entry->freq_khz;
   233		}
   234	}
   235	
   236	static void smem_dram_parse_v5_data(struct smem_dram *dram, void *data)
   237	{
   238		struct ddr_details_v5 *details = data;
   239		struct ddr_regions_v5 *region = &details->ddr_regions;
   240	
   241		dram->hbb = region[0].highest_bank_addr_bit;
   242	
   243		for (int i = 0; i < MAX_DDR_FREQ_NUM_V5; i++) {
   244			struct ddr_freq_table *freq_entry = &details->ddr_freq_tbl.ddr_freq[i];
   245	
   246			if (freq_entry->freq_khz && freq_entry->enabled)
   247				dram->frequencies[dram->num_frequencies++] = 1000 * freq_entry->freq_khz;
   248		}
   249	}
   250	
   251	static void smem_dram_parse_v7_data(struct smem_dram *dram, void *data)
   252	{
   253		struct ddr_details_v7 *details = data;
   254		struct ddr_regions_v5 *region = &details->ddr_regions;
   255	
   256		dram->hbb = region[0].highest_bank_addr_bit;
   257	
   258		for (int i = 0; i < MAX_DDR_FREQ_NUM_V5; i++) {
   259			struct ddr_freq_table *freq_entry = &details->ddr_freq_tbl.ddr_freq[i];
   260	
   261			if (freq_entry->freq_khz && freq_entry->enabled)
   262				dram->frequencies[dram->num_frequencies++] = 1000 * freq_entry->freq_khz;
   263		}
   264	}
   265	
   266	/* The structure contains no version field, so we have to perform some guesswork.. */
   267	static int smem_dram_infer_struct_version(size_t size)
   268	{
   269		/* Some early versions provided less bytes of less useful data */
   270		if (size < sizeof(struct ddr_details_v3))
   271			return -EINVAL;
   272	
   273		if (size == sizeof(struct ddr_details_v3))
   274			return INFO_V3;
   275	
   276		if (size == sizeof(struct ddr_details_v3)
   277			 + sizeof(struct ddr_freq_table))
   278			return INFO_V3_WITH_14_FREQS;
   279	
   280		if (size == sizeof(struct ddr_details_v4))
   281			return INFO_V4;
   282	
   283		if (size == sizeof(struct ddr_details_v5)
   284			 + 4 * sizeof(struct ddr_region_v5))
   285			return INFO_V5;
   286	
   287		if (size == sizeof(struct ddr_details_v5)
   288			 + 4 * sizeof(struct ddr_region_v5)
   289			 + sizeof(struct ddr_xbl2quantum_smem_data)
   290			 + sizeof(struct shub_freq_plan_entry))
   291			return INFO_V5;
   292	
   293		if (size == sizeof(struct ddr_details_v5)
   294			 + 6 * sizeof(struct ddr_region_v5))
   295			return INFO_V5_WITH_6_REGIONS;
   296	
   297		if (size == sizeof(struct ddr_details_v5)
   298			 + 6 * sizeof(struct ddr_region_v5)
   299			 + sizeof(struct ddr_xbl2quantum_smem_data)
   300			 + sizeof(struct shub_freq_plan_entry))
   301			return INFO_V5_WITH_6_REGIONS;
   302	
   303		if (size == sizeof(struct ddr_details_v5)
   304			 + 6 * sizeof(struct ddr_region_v5)
   305			 + sizeof(struct ddr_misc_info_v6)
   306			 + sizeof(struct shub_freq_plan_entry))
   307			return INFO_V6;
   308	
   309		if (size == sizeof(struct ddr_details_v7)
   310			 + 4 * sizeof(struct ddr_region_v5)
   311			 + sizeof(struct ddr_misc_info_v6)
   312			 + sizeof(struct shub_freq_plan_entry))
   313			return INFO_V7;
   314	
   315		if (size == sizeof(struct ddr_details_v7)
   316			 + 6 * sizeof(struct ddr_region_v5)
   317			 + sizeof(struct ddr_misc_info_v6)
   318			 + sizeof(struct shub_freq_plan_entry))
   319			return INFO_V7_WITH_6_REGIONS;
   320	
   321		return INFO_UNKNOWN;
   322	}
   323	
   324	static int smem_dram_frequencies_show(struct seq_file *s, void *unused)
   325	{
   326		struct smem_dram *dram = s->private;
   327	
   328		for (int i = 0; i < dram->num_frequencies; i++)
   329			seq_printf(s, "%lu\n", dram->frequencies[i]);
   330	
   331		return 0;
   332	}
   333	DEFINE_SHOW_ATTRIBUTE(smem_dram_frequencies);
   334	
   335	static int smem_hbb_show(struct seq_file *s, void *unused)
   336	{
   337		struct smem_dram *dram = s->private;
   338	
   339		if (!dram->hbb)
   340			return -EINVAL;
   341	
   342		seq_printf(s, "%d\n", dram->hbb);
   343	
   344		return 0;
   345	}
   346	DEFINE_SHOW_ATTRIBUTE(smem_hbb);
   347	
   348	struct dentry *smem_dram_parse(struct device *dev)
   349	{
   350		struct dentry *debugfs_dir;
   351		enum ddr_info_version ver;
   352		struct smem_dram *dram;
   353		size_t actual_size;
   354		void *data = NULL;
   355	
   356		/* No need to check qcom_smem_is_available(), this func is called by the SMEM driver */
   357		data = qcom_smem_get(QCOM_SMEM_HOST_ANY, SMEM_DDR_INFO_ID, &actual_size);
   358		if (IS_ERR_OR_NULL(data))
   359			return ERR_PTR(-ENODATA);
   360	
   361		ver = smem_dram_infer_struct_version(actual_size);
   362		if (ver < 0) {
   363			/* Some SoCs don't provide data that's useful for us */
   364			return ERR_PTR(-ENODATA);
   365		} else if (ver == INFO_UNKNOWN) {
   366			/* In other cases, we may not have added support for a newer struct revision */
   367			pr_err("Found an unknown type of DRAM info struct (size = %zu)\n", actual_size);
   368			return ERR_PTR(-EINVAL);
   369		}
   370	
   371		dram = devm_kzalloc(dev, sizeof(*dram), GFP_KERNEL);
   372		if (!dram)
   373			return ERR_PTR(-ENOMEM);
   374	
   375		switch (ver) {
   376		case INFO_V3:
   377			smem_dram_parse_v3_data(dram, data, false);
   378			break;
   379		case INFO_V3_WITH_14_FREQS:
 > 380			smem_dram_parse_v3_data(dram, data, true);

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

On Thu, Jan 08, 2026 at 03:21:50PM +0100, Konrad Dybcio wrote:
> diff --git a/drivers/soc/qcom/smem_dramc.c b/drivers/soc/qcom/smem_dramc.c
[..]
> +struct ddr_regions_v5 {
> +	u32 ddr_region_num; /* We expect this to always be 4 or 6 */
> +	u64 ddr_rank0_size;
> +	u64 ddr_rank1_size;
> +	u64 ddr_cs0_start_addr;
> +	u64 ddr_cs1_start_addr;
> +	u32 highest_bank_addr_bit;

Aren't all these structs encoded in little endian? __leXX?

> +	struct ddr_region_v5 ddr_region[] __counted_by(ddr_region_num);

Was going to joke about this one, but realized that there's a
__counted_by_le()

> +};
> +
> +struct ddr_details_v5 {
> +	u8 manufacturer_id;
> +	u8 device_type;
> +	struct ddr_part_details ddr_params[MAX_CHAN_NUM];
> +	struct ddr_freq_plan_v5 ddr_freq_tbl;
> +	u8 num_channels;
> +	u8 _padding;
> +	struct ddr_regions_v5 ddr_regions;
> +};
> +
> +/* V6 */
> +struct ddr_misc_info_v6 {
> +	u32 dsf_version;
> +	u32 reserved[10];
> +};
> +
> +/* V7 */
> +struct ddr_details_v7 {
> +	u8 manufacturer_id;
> +	u8 device_type;
> +	struct ddr_part_details ddr_params[MAX_CHAN_NUM];
> +	struct ddr_freq_plan_v5 ddr_freq_tbl;
> +	u8 num_channels;
> +	u8 sct_config;
> +	struct ddr_regions_v5 ddr_regions;
> +};
> +
> +/**
> + * qcom_smem_dram_get_hbb(): Get the Highest bank address bit
> + *
> + * Context: Check qcom_smem_is_available() before calling this function.
> + * Because __dram * is initialized by smem_dram_parse(), which is in turn
> + * called from * qcom_smem_probe(), __dram will only be NULL if the data
> + * couldn't have been found/interpreted correctly.
> + *
> + * Return: 0 on success, -ENODATA on failure.

Seems more like "highest bank bit on success, -ENODATA on failure.

> + */
> +int qcom_smem_dram_get_hbb(void)
> +{
> +	int hbb;
> +
> +	if (!__dram)
> +		return -ENODATA;
> +
> +	hbb = __dram->hbb;
> +	if (hbb == 0)
> +		return -ENODATA;
> +	else if (hbb < DDR_HBB_MIN || hbb > DDR_HBB_MAX)
> +		return -EINVAL;

Not really "Invalid argument", -ENODATA is probably better here as well.

> +
> +	return hbb;
> +}
> +EXPORT_SYMBOL_GPL(qcom_smem_dram_get_hbb);
> +
[..]
> +/* The structure contains no version field, so we have to perform some guesswork.. */
> +static int smem_dram_infer_struct_version(size_t size)
> +{
> +	/* Some early versions provided less bytes of less useful data */
> +	if (size < sizeof(struct ddr_details_v3))
> +		return -EINVAL;
> +
> +	if (size == sizeof(struct ddr_details_v3))
> +		return INFO_V3;
> +
> +	if (size == sizeof(struct ddr_details_v3)
> +		 + sizeof(struct ddr_freq_table))

Don't you find it weird to have the + after the wrap?

> +		return INFO_V3_WITH_14_FREQS;
> +
> +	if (size == sizeof(struct ddr_details_v4))
> +		return INFO_V4;
> +
> +	if (size == sizeof(struct ddr_details_v5)
> +		 + 4 * sizeof(struct ddr_region_v5))
> +		return INFO_V5;
> +
> +	if (size == sizeof(struct ddr_details_v5)
> +		 + 4 * sizeof(struct ddr_region_v5)
> +		 + sizeof(struct ddr_xbl2quantum_smem_data)
> +		 + sizeof(struct shub_freq_plan_entry))
> +		return INFO_V5;
> +
> +	if (size == sizeof(struct ddr_details_v5)
> +		 + 6 * sizeof(struct ddr_region_v5))
> +		return INFO_V5_WITH_6_REGIONS;
> +
> +	if (size == sizeof(struct ddr_details_v5)
> +		 + 6 * sizeof(struct ddr_region_v5)
> +		 + sizeof(struct ddr_xbl2quantum_smem_data)
> +		 + sizeof(struct shub_freq_plan_entry))
> +		return INFO_V5_WITH_6_REGIONS;
> +
> +	if (size == sizeof(struct ddr_details_v5)
> +		 + 6 * sizeof(struct ddr_region_v5)
> +		 + sizeof(struct ddr_misc_info_v6)
> +		 + sizeof(struct shub_freq_plan_entry))
> +		return INFO_V6;
> +
> +	if (size == sizeof(struct ddr_details_v7)
> +		 + 4 * sizeof(struct ddr_region_v5)
> +		 + sizeof(struct ddr_misc_info_v6)
> +		 + sizeof(struct shub_freq_plan_entry))
> +		return INFO_V7;
> +
> +	if (size == sizeof(struct ddr_details_v7)
> +		 + 6 * sizeof(struct ddr_region_v5)
> +		 + sizeof(struct ddr_misc_info_v6)
> +		 + sizeof(struct shub_freq_plan_entry))
> +		return INFO_V7_WITH_6_REGIONS;
> +
> +	return INFO_UNKNOWN;
> +}
> +
[..]
> +
> +struct dentry *smem_dram_parse(struct device *dev)
> +{
> +	struct dentry *debugfs_dir;
> +	enum ddr_info_version ver;
> +	struct smem_dram *dram;
> +	size_t actual_size;
> +	void *data = NULL;
> +
> +	/* No need to check qcom_smem_is_available(), this func is called by the SMEM driver */
> +	data = qcom_smem_get(QCOM_SMEM_HOST_ANY, SMEM_DDR_INFO_ID, &actual_size);
> +	if (IS_ERR_OR_NULL(data))
> +		return ERR_PTR(-ENODATA);
> +
> +	ver = smem_dram_infer_struct_version(actual_size);
> +	if (ver < 0) {
> +		/* Some SoCs don't provide data that's useful for us */
> +		return ERR_PTR(-ENODATA);
> +	} else if (ver == INFO_UNKNOWN) {
> +		/* In other cases, we may not have added support for a newer struct revision */
> +		pr_err("Found an unknown type of DRAM info struct (size = %zu)\n", actual_size);

Is there a reason why this isn't dev_err(dev, ...)?

> +		return ERR_PTR(-EINVAL);
> +	}
> +
> +	dram = devm_kzalloc(dev, sizeof(*dram), GFP_KERNEL);
> +	if (!dram)
> +		return ERR_PTR(-ENOMEM);
> +
> +	switch (ver) {
> +	case INFO_V3:
> +		smem_dram_parse_v3_data(dram, data, false);
> +		break;
> +	case INFO_V3_WITH_14_FREQS:
> +		smem_dram_parse_v3_data(dram, data, true);
> +		break;
> +	case INFO_V4:
> +		smem_dram_parse_v4_data(dram, data);
> +		break;
> +	case INFO_V5:
> +	case INFO_V5_WITH_6_REGIONS:
> +	case INFO_V6:
> +		smem_dram_parse_v5_data(dram, data);
> +		break;
> +	case INFO_V7:
> +	case INFO_V7_WITH_6_REGIONS:
> +		smem_dram_parse_v7_data(dram, data);
> +		break;
> +	default:
> +		return ERR_PTR(-EINVAL);
> +	}
> +
> +	/* Both the entry and its parent dir will be cleaned up by debugfs_remove_recursive */
> +	debugfs_dir = debugfs_create_dir("qcom_smem", NULL);
> +	debugfs_create_file("dram_frequencies", 0444, debugfs_dir, dram,
> +			    &smem_dram_frequencies_fops);
> +	debugfs_create_file("hbb", 0444, debugfs_dir, dram, &smem_hbb_fops);
> +
> +	/* If there was no failure so far, assign the global variable */
> +	__dram = dram;
> +
> +	return debugfs_dir;
> +}
> diff --git a/include/linux/soc/qcom/smem.h b/include/linux/soc/qcom/smem.h
> index f946e3beca21..223cd5090a2a 100644
> --- a/include/linux/soc/qcom/smem.h
> +++ b/include/linux/soc/qcom/smem.h
> @@ -2,6 +2,8 @@
>  #ifndef __QCOM_SMEM_H__
>  #define __QCOM_SMEM_H__
>  
> +#include <linux/platform_device.h>

I'm not able to see why.

Regards,
Bjorn

> +
>  #define QCOM_SMEM_HOST_ANY -1
>  
>  bool qcom_smem_is_available(void);
> @@ -17,4 +19,6 @@ int qcom_smem_get_feature_code(u32 *code);
>  
>  int qcom_smem_bust_hwspin_lock_by_host(unsigned int host);
>  
> +int qcom_smem_dram_get_hbb(void);
> +
>  #endif
> 
> -- 
> 2.52.0
>

On Thu, Jan 08, 2026 at 03:21:50PM +0100, Konrad Dybcio wrote:
> From: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
> 
> Most modern Qualcomm platforms (>= SM8150) expose information about the
> DDR memory present on the system via SMEM.
> 
> Details from this information is used in various scenarios, such as
> multimedia drivers configuring the hardware based on the "Highest Bank
> address Bit" (hbb), or the list of valid frequencies in validation
> scenarios...
> 
> Add support for parsing v3-v7 version of the structs. Unforunately,
> they are not versioned, so some elbow grease is necessary to determine
> which one is present. See for reference:
> 
> ver 3: https://git.codelinaro.org/clo/la/abl/tianocore/edk2/-/commit/1d11897d2cfcc7b85f28ff74c445018dbbecac7a
> ver 4: https://git.codelinaro.org/clo/la/abl/tianocore/edk2/-/commit/f6e9aa549260bbc0bdcb156c2b05f48dc5963203
> ver 5: https://git.codelinaro.org/clo/la/abl/tianocore/edk2/-/commit/617d3297abe8b1b8dd3de3d1dd69c3961e6f343f
> ver 5 with 6regions: https://git.codelinaro.org/clo/la/abl/tianocore/edk2/-/commit/d770e009f9bae58d56d926f7490bbfb45af8341f
> ver 6: https://git.codelinaro.org/clo/la/abl/tianocore/edk2/-/commit/62659b557fdb1551b20fae8073d1d701dfa8a62e
> ver 7: https://git.codelinaro.org/clo/la/abl/tianocore/edk2/-/commit/734d95599c5ebb1ca0d4e1639142e65c590532b7
> 
> Reviewed-by: Bjorn Andersson <andersson@kernel.org>
> Signed-off-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
> ---
>  drivers/soc/qcom/Makefile     |   3 +-
>  drivers/soc/qcom/smem.c       |  14 +-
>  drivers/soc/qcom/smem.h       |   9 +
>  drivers/soc/qcom/smem_dramc.c | 408 ++++++++++++++++++++++++++++++++++++++++++
>  include/linux/soc/qcom/smem.h |   4 +
>  5 files changed, 436 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/soc/qcom/Makefile b/drivers/soc/qcom/Makefile
> index b7f1d2a57367..798643be3590 100644
> --- a/drivers/soc/qcom/Makefile
> +++ b/drivers/soc/qcom/Makefile
> @@ -23,7 +23,8 @@ obj-$(CONFIG_QCOM_RPMH)		+= qcom_rpmh.o
>  qcom_rpmh-y			+= rpmh-rsc.o
>  qcom_rpmh-y			+= rpmh.o
>  obj-$(CONFIG_QCOM_SMD_RPM)	+= rpm-proc.o smd-rpm.o
> -obj-$(CONFIG_QCOM_SMEM) +=	smem.o
> +qcom_smem-y			+= smem.o smem_dramc.o
> +obj-$(CONFIG_QCOM_SMEM) +=	qcom_smem.o
>  obj-$(CONFIG_QCOM_SMEM_STATE) += smem_state.o
>  CFLAGS_smp2p.o := -I$(src)
>  obj-$(CONFIG_QCOM_SMP2P)	+= smp2p.o
> diff --git a/drivers/soc/qcom/smem.c b/drivers/soc/qcom/smem.c
> index 088b2bbee9e6..a53bf9ed8e92 100644
> --- a/drivers/soc/qcom/smem.c
> +++ b/drivers/soc/qcom/smem.c
> @@ -4,6 +4,7 @@
>   * Copyright (c) 2012-2013, The Linux Foundation. All rights reserved.
>   */
>  
> +#include <linux/debugfs.h>
>  #include <linux/hwspinlock.h>
>  #include <linux/io.h>
>  #include <linux/module.h>
> @@ -16,6 +17,8 @@
>  #include <linux/soc/qcom/smem.h>
>  #include <linux/soc/qcom/socinfo.h>
>  
> +#include "smem.h"
> +
>  /*
>   * The Qualcomm shared memory system is a allocate only heap structure that
>   * consists of one of more memory areas that can be accessed by the processors
> @@ -284,6 +287,8 @@ struct qcom_smem {
>  	struct smem_partition global_partition;
>  	struct smem_partition partitions[SMEM_HOST_COUNT];
>  
> +	struct dentry *debugfs_dir;
> +
>  	unsigned num_regions;
>  	struct smem_region regions[] __counted_by(num_regions);
>  };
> @@ -1236,17 +1241,24 @@ static int qcom_smem_probe(struct platform_device *pdev)
>  
>  	__smem = smem;
>  
> +	smem->debugfs_dir = smem_dram_parse(smem->dev);

Is it possible, even after calling qcom_smem_is_available() before calling 
qcom_smem_dram_get_hbb() we are getting __dram as NULL.

is it good to move __smem assignment to the end with barrier so all the
changes before the assignment are seen when somebody checking qcom_smem_is_available()
with a pair smp store/release pair.

> +
>  	smem->socinfo = platform_device_register_data(&pdev->dev, "qcom-socinfo",
>  						      PLATFORM_DEVID_NONE, NULL,
>  						      0);
> -	if (IS_ERR(smem->socinfo))
> +	if (IS_ERR(smem->socinfo)) {
> +		debugfs_remove_recursive(smem->debugfs_dir);
> +
>  		dev_dbg(&pdev->dev, "failed to register socinfo device\n");
> +	}
>  
>  	return 0;
>  }
>  
>  static void qcom_smem_remove(struct platform_device *pdev)
>  {
> +	debugfs_remove_recursive(__smem->debugfs_dir);
> +
>  	platform_device_unregister(__smem->socinfo);
>  
>  	__smem = NULL;
> diff --git a/drivers/soc/qcom/smem.h b/drivers/soc/qcom/smem.h
> new file mode 100644
> index 000000000000..8bf3f606e1ae
> --- /dev/null
> +++ b/drivers/soc/qcom/smem.h
> @@ -0,0 +1,9 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#ifndef __QCOM_SMEM_INTERNAL__
> +#define __QCOM_SMEM_INTERNAL__
> +
> +#include <linux/device.h>
> +
> +struct dentry *smem_dram_parse(struct device *dev);
> +
> +#endif
> diff --git a/drivers/soc/qcom/smem_dramc.c b/drivers/soc/qcom/smem_dramc.c
> new file mode 100644
> index 000000000000..017bb894a91b
> --- /dev/null
> +++ b/drivers/soc/qcom/smem_dramc.c
> @@ -0,0 +1,408 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2025 Qualcomm Innovation Center, Inc. All rights reserved.

Year less.. ??

> + */
> +
> +#include <linux/debugfs.h>
> +#include <linux/io.h>
> +#include <linux/module.h>
> +#include <linux/of_device.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/soc/qcom/smem.h>
> +#include <linux/units.h>
> +#include <linux/soc/qcom/smem.h>

2nd time..

> +
> +#include "smem.h"
> +
> +#define SMEM_DDR_INFO_ID		603
> +
> +#define MAX_DDR_FREQ_NUM_V3		13
> +#define MAX_DDR_FREQ_NUM_V5		14
> +
> +#define MAX_CHAN_NUM			8
> +#define MAX_RANK_NUM			2
> +
> +#define DDR_HBB_MIN			13
> +#define DDR_HBB_MAX			19
> +
> +#define MAX_SHUB_ENTRIES		8
> +
> +static struct smem_dram *__dram;
> +
> +enum ddr_info_version {
> +	INFO_UNKNOWN,
> +	INFO_V3,
> +	INFO_V3_WITH_14_FREQS,
> +	INFO_V4,
> +	INFO_V5,
> +	INFO_V5_WITH_6_REGIONS,
> +	INFO_V6, /* INFO_V6 seems to only have shipped with 6 DDR regions, unlike V7 */
> +	INFO_V7,
> +	INFO_V7_WITH_6_REGIONS,
> +};
> +
> +struct smem_dram {
> +	unsigned long frequencies[MAX_DDR_FREQ_NUM_V5];
> +	u32 num_frequencies;

freq and num_freq_entries ? since you have used freq at various places..

> +	u8 hbb;
> +};
> +
> +enum ddr_type {
> +	DDR_TYPE_NODDR = 0,
> +	DDR_TYPE_LPDDR1 = 1,
> +	DDR_TYPE_LPDDR2 = 2,
> +	DDR_TYPE_PCDDR2 = 3,
> +	DDR_TYPE_PCDDR3 = 4,
> +	DDR_TYPE_LPDDR3 = 5,
> +	DDR_TYPE_LPDDR4 = 6,
> +	DDR_TYPE_LPDDR4X = 7,
> +	DDR_TYPE_LPDDR5 = 8,
> +	DDR_TYPE_LPDDR5X = 9,
> +};
> +
> +/* The data structures below are NOT __packed on purpose! */
> +
> +/* Structs used across multiple versions */
> +struct ddr_part_details {
> +	__le16 revision_id1;
> +	__le16 revision_id2;
> +	__le16 width;
> +	__le16 density;
> +};
> +
> +struct ddr_freq_table {
> +	u32 freq_khz;
> +	u8 enabled;
> +};
> +
> +/* V3 */
> +struct ddr_freq_plan_v3 {
> +	struct ddr_freq_table ddr_freq[MAX_DDR_FREQ_NUM_V3]; /* NOTE: some have 14 like v5 */
> +	u8 num_ddr_freqs;
> +	phys_addr_t clk_period_address;
> +};
> +
> +struct ddr_details_v3 {
> +	u8 manufacturer_id;
> +	u8 device_type;
> +	struct ddr_part_details ddr_params[MAX_CHAN_NUM];
> +	struct ddr_freq_plan_v3 ddr_freq_tbl;
> +	u8 num_channels;
> +};
> +
> +/* V4 */
> +struct ddr_details_v4 {
> +	u8 manufacturer_id;
> +	u8 device_type;
> +	struct ddr_part_details ddr_params[MAX_CHAN_NUM];
> +	struct ddr_freq_plan_v3 ddr_freq_tbl;
> +	u8 num_channels;
> +	u8 num_ranks[MAX_CHAN_NUM];
> +	u8 highest_bank_addr_bit[MAX_CHAN_NUM][MAX_RANK_NUM];
> +};
> +
> +/* V5 */
> +struct shub_freq_table {
> +	u8 enable;
> +	u32 freq_khz;
> +};
> +
> +struct shub_freq_plan_entry {
> +	u8 num_shub_freqs;
> +	struct shub_freq_table shub_freq[MAX_SHUB_ENTRIES];
> +};
> +
> +struct ddr_xbl2quantum_smem_data {
> +	phys_addr_t ssr_cookie_addr;
> +	u32 reserved[10];
> +};
> +
> +struct ddr_freq_plan_v5 {
> +	struct ddr_freq_table ddr_freq[MAX_DDR_FREQ_NUM_V5];
> +	u8 num_ddr_freqs;
> +	phys_addr_t clk_period_address;
> +	u32 max_nom_ddr_freq;
> +};
> +
> +struct ddr_region_v5 {
> +	u64 start_address;
> +	u64 size;
> +	u64 mem_controller_address;
> +	u32 granule_size; /* MiB */
> +	u8  ddr_rank;
> +#define DDR_RANK_0	BIT(0)
> +#define DDR_RANK_1	BIT(1)
> +	u8  segments_start_index;
> +	u64 segments_start_offset;
> +};
> +
> +struct ddr_regions_v5 {
> +	u32 ddr_region_num; /* We expect this to always be 4 or 6 */
> +	u64 ddr_rank0_size;
> +	u64 ddr_rank1_size;
> +	u64 ddr_cs0_start_addr;
> +	u64 ddr_cs1_start_addr;
> +	u32 highest_bank_addr_bit;
> +	struct ddr_region_v5 ddr_region[] __counted_by(ddr_region_num);
> +};
> +
> +struct ddr_details_v5 {
> +	u8 manufacturer_id;
> +	u8 device_type;
> +	struct ddr_part_details ddr_params[MAX_CHAN_NUM];
> +	struct ddr_freq_plan_v5 ddr_freq_tbl;
> +	u8 num_channels;
> +	u8 _padding;
> +	struct ddr_regions_v5 ddr_regions;
> +};
> +
> +/* V6 */
> +struct ddr_misc_info_v6 {
> +	u32 dsf_version;
> +	u32 reserved[10];
> +};
> +
> +/* V7 */
> +struct ddr_details_v7 {
> +	u8 manufacturer_id;
> +	u8 device_type;
> +	struct ddr_part_details ddr_params[MAX_CHAN_NUM];
> +	struct ddr_freq_plan_v5 ddr_freq_tbl;
> +	u8 num_channels;
> +	u8 sct_config;
> +	struct ddr_regions_v5 ddr_regions;
> +};
> +
> +/**
> + * qcom_smem_dram_get_hbb(): Get the Highest bank address bit
> + *
> + * Context: Check qcom_smem_is_available() before calling this function.
> + * Because __dram * is initialized by smem_dram_parse(), which is in turn
> + * called from * qcom_smem_probe(), __dram will only be NULL if the data
> + * couldn't have been found/interpreted correctly.
> + *
> + * Return: 0 on success, -ENODATA on failure.
> + */
> +int qcom_smem_dram_get_hbb(void)
> +{
> +	int hbb;
> +
> +	if (!__dram)
> +		return -ENODATA;
> +
> +	hbb = __dram->hbb;
> +	if (hbb == 0)
> +		return -ENODATA;

Incase, you want to consider to save some lines..

if (!__dram || !__dram->hbb)
	return -ENODATA;


> +	else if (hbb < DDR_HBB_MIN || hbb > DDR_HBB_MAX)
> +		return -EINVAL;
> +
> +	return hbb;
> +}
> +EXPORT_SYMBOL_GPL(qcom_smem_dram_get_hbb);
> +
> +static void smem_dram_parse_v3_data(struct smem_dram *dram, void *data, bool additional_freq_entry)
> +{
> +	/* This may be 13 or 14 */
> +	int num_freq_entries = MAX_DDR_FREQ_NUM_V3;
> +	struct ddr_details_v3 *details = data;
> +
> +	if (additional_freq_entry)
> +		num_freq_entries++;
> +
> +	for (int i = 0; i < num_freq_entries; i++) {
> +		struct ddr_freq_table *freq_entry = &details->ddr_freq_tbl.ddr_freq[i];
> +
> +		if (freq_entry->freq_khz && freq_entry->enabled)
> +			dram->frequencies[dram->num_frequencies++] = 1000 * freq_entry->freq_khz;
> +	}
> +}
> +
> +static void smem_dram_parse_v4_data(struct smem_dram *dram, void *data)
> +{
> +	struct ddr_details_v4 *details = data;
> +
> +	/* Rank 0 channel 0 entry holds the correct value */
> +	dram->hbb = details->highest_bank_addr_bit[0][0];
> +
> +	for (int i = 0; i < MAX_DDR_FREQ_NUM_V3; i++) {
> +		struct ddr_freq_table *freq_entry = &details->ddr_freq_tbl.ddr_freq[i];
> +
> +		if (freq_entry->freq_khz && freq_entry->enabled)
> +			dram->frequencies[dram->num_frequencies++] = 1000 * freq_entry->freq_khz;
> +	}
> +}
> +
> +static void smem_dram_parse_v5_data(struct smem_dram *dram, void *data)
> +{
> +	struct ddr_details_v5 *details = data;
> +	struct ddr_regions_v5 *region = &details->ddr_regions;
> +
> +	dram->hbb = region[0].highest_bank_addr_bit;
> +
> +	for (int i = 0; i < MAX_DDR_FREQ_NUM_V5; i++) {
> +		struct ddr_freq_table *freq_entry = &details->ddr_freq_tbl.ddr_freq[i];
> +
> +		if (freq_entry->freq_khz && freq_entry->enabled)
> +			dram->frequencies[dram->num_frequencies++] = 1000 * freq_entry->freq_khz;
> +	}
> +}
> +
> +static void smem_dram_parse_v7_data(struct smem_dram *dram, void *data)
> +{
> +	struct ddr_details_v7 *details = data;
> +	struct ddr_regions_v5 *region = &details->ddr_regions;
> +
> +	dram->hbb = region[0].highest_bank_addr_bit;
> +
> +	for (int i = 0; i < MAX_DDR_FREQ_NUM_V5; i++) {
> +		struct ddr_freq_table *freq_entry = &details->ddr_freq_tbl.ddr_freq[i];
> +
> +		if (freq_entry->freq_khz && freq_entry->enabled)
> +			dram->frequencies[dram->num_frequencies++] = 1000 * freq_entry->freq_khz;
> +	}
> +}
> +
> +/* The structure contains no version field, so we have to perform some guesswork.. */
> +static int smem_dram_infer_struct_version(size_t size)
> +{
> +	/* Some early versions provided less bytes of less useful data */
> +	if (size < sizeof(struct ddr_details_v3))
> +		return -EINVAL;
> +
> +	if (size == sizeof(struct ddr_details_v3))
> +		return INFO_V3;
> +
> +	if (size == sizeof(struct ddr_details_v3)
> +		 + sizeof(struct ddr_freq_table))
> +		return INFO_V3_WITH_14_FREQS;
> +
> +	if (size == sizeof(struct ddr_details_v4))
> +		return INFO_V4;
> +
> +	if (size == sizeof(struct ddr_details_v5)
> +		 + 4 * sizeof(struct ddr_region_v5))
> +		return INFO_V5;
> +
> +	if (size == sizeof(struct ddr_details_v5)
> +		 + 4 * sizeof(struct ddr_region_v5)
> +		 + sizeof(struct ddr_xbl2quantum_smem_data)
> +		 + sizeof(struct shub_freq_plan_entry))
> +		return INFO_V5;

Why this does not have separate name ?

> +
> +	if (size == sizeof(struct ddr_details_v5)
> +		 + 6 * sizeof(struct ddr_region_v5))
> +		return INFO_V5_WITH_6_REGIONS;
> +
> +	if (size == sizeof(struct ddr_details_v5)
> +		 + 6 * sizeof(struct ddr_region_v5)
> +		 + sizeof(struct ddr_xbl2quantum_smem_data)
> +		 + sizeof(struct shub_freq_plan_entry))
> +		return INFO_V5_WITH_6_REGIONS;
> +
> +	if (size == sizeof(struct ddr_details_v5)
> +		 + 6 * sizeof(struct ddr_region_v5)
> +		 + sizeof(struct ddr_misc_info_v6)
> +		 + sizeof(struct shub_freq_plan_entry))
> +		return INFO_V6;
> +
> +	if (size == sizeof(struct ddr_details_v7)
> +		 + 4 * sizeof(struct ddr_region_v5)
> +		 + sizeof(struct ddr_misc_info_v6)
> +		 + sizeof(struct shub_freq_plan_entry))
> +		return INFO_V7;
> +
> +	if (size == sizeof(struct ddr_details_v7)
> +		 + 6 * sizeof(struct ddr_region_v5)
> +		 + sizeof(struct ddr_misc_info_v6)
> +		 + sizeof(struct shub_freq_plan_entry))
> +		return INFO_V7_WITH_6_REGIONS;
> +
> +	return INFO_UNKNOWN;
> +}
> +
> +static int smem_dram_frequencies_show(struct seq_file *s, void *unused)
> +{
> +	struct smem_dram *dram = s->private;
> +
> +	for (int i = 0; i < dram->num_frequencies; i++)
> +		seq_printf(s, "%lu\n", dram->frequencies[i]);
> +
> +	return 0;
> +}
> +DEFINE_SHOW_ATTRIBUTE(smem_dram_frequencies);
> +
> +static int smem_hbb_show(struct seq_file *s, void *unused)
> +{
> +	struct smem_dram *dram = s->private;
> +
> +	if (!dram->hbb)
> +		return -EINVAL;
> +
> +	seq_printf(s, "%d\n", dram->hbb);
> +
> +	return 0;
> +}
> +DEFINE_SHOW_ATTRIBUTE(smem_hbb);
> +
> +struct dentry *smem_dram_parse(struct device *dev)
> +{
> +	struct dentry *debugfs_dir;
> +	enum ddr_info_version ver;
> +	struct smem_dram *dram;
> +	size_t actual_size;
> +	void *data = NULL;
> +
> +	/* No need to check qcom_smem_is_available(), this func is called by the SMEM driver */

This comment seems redundant..

> +	data = qcom_smem_get(QCOM_SMEM_HOST_ANY, SMEM_DDR_INFO_ID, &actual_size);
> +	if (IS_ERR_OR_NULL(data))
> +		return ERR_PTR(-ENODATA);
> +
> +	ver = smem_dram_infer_struct_version(actual_size);
> +	if (ver < 0) {
> +		/* Some SoCs don't provide data that's useful for us */
> +		return ERR_PTR(-ENODATA);
> +	} else if (ver == INFO_UNKNOWN) {
> +		/* In other cases, we may not have added support for a newer struct revision */
> +		pr_err("Found an unknown type of DRAM info struct (size = %zu)\n", actual_size);
> +		return ERR_PTR(-EINVAL);
> +	}
> +
> +	dram = devm_kzalloc(dev, sizeof(*dram), GFP_KERNEL);
> +	if (!dram)
> +		return ERR_PTR(-ENOMEM);
> +
> +	switch (ver) {
> +	case INFO_V3:
> +		smem_dram_parse_v3_data(dram, data, false);
> +		break;
> +	case INFO_V3_WITH_14_FREQS:
> +		smem_dram_parse_v3_data(dram, data, true);
> +		break;
> +	case INFO_V4:
> +		smem_dram_parse_v4_data(dram, data);
> +		break;
> +	case INFO_V5:
> +	case INFO_V5_WITH_6_REGIONS:
> +	case INFO_V6:
> +		smem_dram_parse_v5_data(dram, data);
> +		break;
> +	case INFO_V7:
> +	case INFO_V7_WITH_6_REGIONS:
> +		smem_dram_parse_v7_data(dram, data);
> +		break;
> +	default:
> +		return ERR_PTR(-EINVAL);
> +	}
> +
> +	/* Both the entry and its parent dir will be cleaned up by debugfs_remove_recursive */
> +	debugfs_dir = debugfs_create_dir("qcom_smem", NULL);
> +	debugfs_create_file("dram_frequencies", 0444, debugfs_dir, dram,
> +			    &smem_dram_frequencies_fops);
> +	debugfs_create_file("hbb", 0444, debugfs_dir, dram, &smem_hbb_fops);
> +
> +	/* If there was no failure so far, assign the global variable */

nit: I agree that we should write comments, but sometimes it is too obvious.
This is a simple assignment, not something complex like a barrier that requires
detailed explanation.

> +	__dram = dram;
> +
> +	return debugfs_dir;
> +}
> diff --git a/include/linux/soc/qcom/smem.h b/include/linux/soc/qcom/smem.h
> index f946e3beca21..223cd5090a2a 100644
> --- a/include/linux/soc/qcom/smem.h
> +++ b/include/linux/soc/qcom/smem.h
> @@ -2,6 +2,8 @@
>  #ifndef __QCOM_SMEM_H__
>  #define __QCOM_SMEM_H__
>  
> +#include <linux/platform_device.h>
> +

Why this ?

>  #define QCOM_SMEM_HOST_ANY -1
>  
>  bool qcom_smem_is_available(void);
> @@ -17,4 +19,6 @@ int qcom_smem_get_feature_code(u32 *code);
>  
>  int qcom_smem_bust_hwspin_lock_by_host(unsigned int host);
>  
> +int qcom_smem_dram_get_hbb(void);
> +
>  #endif
> 
> -- 
> 2.52.0
> 

-- 
-Mukesh Ojha

On 1/9/26 2:36 PM, Mukesh Ojha wrote:
> On Thu, Jan 08, 2026 at 03:21:50PM +0100, Konrad Dybcio wrote:
>> From: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
>>
>> Most modern Qualcomm platforms (>= SM8150) expose information about the
>> DDR memory present on the system via SMEM.

[...]

>> @@ -1236,17 +1241,24 @@ static int qcom_smem_probe(struct platform_device *pdev)
>>  
>>  	__smem = smem;
>>  
>> +	smem->debugfs_dir = smem_dram_parse(smem->dev);
> 
> Is it possible, even after calling qcom_smem_is_available() before calling 
> qcom_smem_dram_get_hbb() we are getting __dram as NULL.
> 
> is it good to move __smem assignment to the end with barrier so all the
> changes before the assignment are seen when somebody checking qcom_smem_is_available()
> with a pair smp store/release pair.

I think just moving the __smem assignment down will be enough, no?

What scenario do you have in mind that would require SMP barriers?

[...]

>> +struct smem_dram {
>> +	unsigned long frequencies[MAX_DDR_FREQ_NUM_V5];
>> +	u32 num_frequencies;
> 
> freq and num_freq_entries ? since you have used freq at various places..

The names in structs come from internal shmem definitions that I didn't
want to stray away from

Making the kernel-side struct fields named the same feels like added
confusion to me

[...]

>> +	if (size == sizeof(struct ddr_details_v5)
>> +		 + 4 * sizeof(struct ddr_region_v5)
>> +		 + sizeof(struct ddr_xbl2quantum_smem_data)
>> +		 + sizeof(struct shub_freq_plan_entry))
>> +		return INFO_V5;
> 
> Why this does not have separate name ?

Because it's the same DDR info structure as "normal v5", with trailing
extras that we don't really care about

[...]

>> +struct dentry *smem_dram_parse(struct device *dev)
>> +{
>> +	struct dentry *debugfs_dir;
>> +	enum ddr_info_version ver;
>> +	struct smem_dram *dram;
>> +	size_t actual_size;
>> +	void *data = NULL;
>> +
>> +	/* No need to check qcom_smem_is_available(), this func is called by the SMEM driver */
> 
> This comment seems redundant..

With this one specifically, I don't agree it's obvious..

Konrad