Re: [PATCH] drivers/perf: hisi: Add identifier sysfs file

John Garry
2020-06-18T09:20:32Z

On 18/06/2020 02:40, Shaokun Zhang wrote:
>> }
>>
>> + hha_pmu->identifier = readl(hha_pmu->base + HHA_VERSION);
> Since we are now refactoring the PMU framework, the PMU version offset
> is always the same except DDRC PMU and other uncore PMU modules will
> also use this, how about we do it as the common code:
>
> #define HISI_PMU_VERSION_REG 0x1CF0
> int hisi_uncore_pmu_version(struct hisi_pmu *hisi_pmu)
> {
> return readl(hisi_pmu->base + HISI_PMU_VERSION_REG);
> }
> EXPORT_SYMBOL_GPL(hisi_uncore_pmu_version);

Hi Shaokun,

Some points to make:

- It's hardly worth having a separate function to do this 1-line readl()
call, especially since it not even generic (DDRC is different)

- We would have to export it (or put in a common header file with static
inline keywords) - less exports are good

- with factoring out common code, it's good to reduce total code - this
change would increase it, AFAICS

- This is HW specific. The driver is currently layered such that all HW
specific stuff is in the HW driver (like hisi_uncore_ddrc_pmu.c), and
not library code (hisi_uncore_pmu.c). I don't see why you want to mix
that, like you're proposing in the framework revision you proposed
internally.

Thanks,
John

>
> hha_pmu->identifier = hisi_uncore_pmu_version(hha_pmu);
> we can remove the duplicated PMU version register definition in each module.
>
> Thanks,
> Shaokun
>
>> +
>> return 0;
>> }
>>
>> @@ -320,10 +323,23 @@ static const struct attribute_group hisi_hha_pmu_cpumask_attr_group = {
>> .attrs = hisi_hha_pmu_cpumask_attrs,
>> };