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

[Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>]

On 5/22/25 7:48 AM, Maulik Shah (mkshah) wrote:
> 
> 
> On 5/21/2025 10:31 PM, Konrad Dybcio wrote:
>> On 5/21/25 10:32 AM, Maulik Shah wrote:
>>> DDR statistic provide different DDR LPM and DDR frequency statistic.
>>> Add support to read from MSGRAM and display via debugfs.
>>>
>>> Signed-off-by: Maulik Shah <maulik.shah@oss.qualcomm.com>
>>> ---
>>
>> [...]
>>
>>> +	case 0:
>>> +		seq_printf(s, "DDR LPM Stat Name:0x%x\tcount:%u\tDuration (ticks):%llu\n",
>>> +			   DDR_STATS_LPM_NAME(data->name), data->count, data->duration);
>>> +		break;
>>> +	case 1:
>>> +		if (!data->count || !DDR_STATS_FREQ(data->name))
>>> +			return;
>>> +
>>> +		cp_idx = DDR_STATS_CP_IDX(data->name);
>>> +		seq_printf(s, "DDR Freq %uMhz:\tCP IDX:%u\tcount:%u\tDuration (ticks):%llu\n",
>>> +			   DDR_STATS_FREQ(data->name), cp_idx, data->count, data->duration);
>>
>> clang complains about both prints:
>>
>> drivers/soc/qcom/qcom_stats.c:173:7: warning: format specifies type 'unsigned int' but the argument has type 'unsigned long' [-Wformat]
>>   172 |                 seq_printf(s, "DDR LPM Stat Name:0x%x\tcount:%u\tDuration (ticks):%llu\n",
>>       |                                                    ~~
>>       |                                                    %lx
>>   173 |                            DDR_STATS_LPM_NAME(data->name), data->count, data->duration);
>>       |                            ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>
>> drivers/soc/qcom/qcom_stats.c:181:7: warning: format specifies type 'unsigned int' but the argument has type 'unsigned long' [-Wformat]
>>   180 |                 seq_printf(s, "DDR Freq %uMhz:\tCP IDX:%u\tcount:%u\tDuration (ticks):%llu\n",
>>       |                                         ~~
>>       |                                         %lu
>>   181 |                            DDR_STATS_FREQ(data->name), cp_idx, data->count, data->duration);
>>       |                            ^~~~~~~~~~~~~~~~~~~~~~~~~~
>>
>>
>>
> 
> I will update correct format specifiers in v3.
> 
>>
>>> +
>>> +	key = readl_relaxed(reg + config->ddr_stats_offset + DDR_STATS_MAGIC_KEY_ADDR);
>>> +	if (key == DDR_STATS_MAGIC_KEY)
>>> +		debugfs_create_file("ddr_stats", 0400, root,
>>> +				    (__force void *)reg + config->ddr_stats_offset,
>>> +				    &qcom_ddr_stats_fops);
>>
>> else
>> 	pr_err("Found invalid DDR stats magic\n");
>>
>> (because through the compatible, we much expect it to be present)
> 
> The qcom,rpmh-stats compatible does not guarantee the DDR stats presence. DDR stats is only present if
> magic value matches. The ddr stats was incrementally added over time so older SoCs like SM8150, QCS615
> will not have the ddr stats and would end up printing this error during boot up but yes all almost all
> rpmh targets do have the DDR stats present. If we are ok to print this error for older SoCs i can add it
> or how about using pr_warn instead of pr_err?

OK, Let's not print anything then

Konrad