Re: [PATCH 1/3] soc: qcom: qcom_stats: Add support to read DDR statistic

["Maulik Shah (mkshah)" <maulik.shah@oss.qualcomm.com>]

On 4/29/2025 4:04 PM, Konrad Dybcio wrote:
> On 4/29/25 5:52 AM, Maulik Shah wrote:
>> DDR statistic provide different DDR LPM and DDR frequency statistic.
>> +#define DDR_STATS_MAGIC_KEY		0xA1157A75
>> +#define DDR_STATS_MAX_NUM_MODES		0x14
> 
> Numbers (count) make more sense in decimal
> 

I will change to decimal.

>> +#define DDR_STATS_MAGIC_KEY_ADDR	0x0
>> +#define DDR_STATS_NUM_MODES_ADDR	0x4
>> +#define DDR_STATS_ENTRY_START_ADDR	0x8
> 
> [...]
> 

>> +	 */
>> +	name = DDR_STATS_TYPE(data->name);
>> +	if (name == 0x0) {
>> +		name = DDR_STATS_LPM_NAME(data->name);
> 
> I'm not super keen on reusing the 'name' variable, maybe turn the
> outer if-condition to switch(DDR_STATS_TYPE(data->name))

Okay. I will use switch-case.

> 

> 
>> +}
>> +
>> +static int qcom_ddr_stats_show(struct seq_file *s, void *d)
>> +{
>> +	struct ddr_stats_entry data[DDR_STATS_MAX_NUM_MODES];
>> +	void __iomem *reg = (void __iomem *)s->private;
>> +	u32 entry_count;
>> +	int i;
>> +
>> +	entry_count = readl_relaxed(reg + DDR_STATS_NUM_MODES_ADDR);
>> +	if (entry_count > DDR_STATS_MAX_NUM_MODES)
>> +		return 0;
> 
> -EINVAL
> 
> Konrad

I kept this return as success from details given in commit message of [1] 
which made the qcom_subsystem_sleep_stats_show() function return 0
in order to run command like below to collect the stats without interspersed errors
grep ^ /sys/kernel/debug/qcom_stats/*

The same may break if return error from ddr stats too.

[1] https://lore.kernel.org/r/20230119032329.2909383-1-swboyd@chromium.org 

Thanks,
Maulik