Re: [PATCH v3 1/3] soc: qcom: smem: Expose DDR data from SMEM
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
>>
>> 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
Re: [PATCH v3 1/3] soc: qcom: smem: Expose DDR data from SMEM
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/[email protected]/config) compiler: m68k-linux-gcc (GCC) 15.2.0 reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20260115/[email protected]/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 | Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/ 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
Re: [PATCH v3 1/3] soc: qcom: smem: Expose DDR data from SMEM
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))
> +
Re: [PATCH v3 1/3] soc: qcom: smem: Expose DDR data from SMEM
On Thu, Jan 08, 2026 at 03:21:50PM +0100, Konrad Dybcio wrote:
> From: Konrad Dybcio
>
> 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
> Signed-off-by: Konrad Dybcio
> ---
> 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
> #include
> #include
> #include
> @@ -16,6 +17,8 @@
> #include
> #include
>
> +#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 ..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
> +
> +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 ..017bb894a91b
> --- /dev/null
> +++ b/drivers/soc/qcom/smem_dramc
