Re: [PATCH 06/13] perf: stm32: introduce DDRPERFM driver

[Clement LE GOFFIC]

On 6/25/25 10:48, Krzysztof Kozlowski wrote:
> On 25/06/2025 10:33, Clement LE GOFFIC wrote:
> > On 6/25/25 08:35, Krzysztof Kozlowski wrote:
> > > On 24/06/2025 12:43, Clement LE GOFFIC wrote:
> > > > On 6/23/25 11:45, Krzysztof Kozlowski wrote:
> > > > [...]
> > > >
> > > > Hi Krzysztof,
> > > >
> > > > Sorry I forgot to address comments below.
> > > >
> > > > > > +
> > > > > > +static const struct stm32_ddr_pmu_cfg stm32_ddr_pmu_cfg_mp1 = {
> > > > > > +	.regs = &stm32_ddr_pmu_regspec_mp1,
> > > > > > +	.attribute = stm32_ddr_pmu_attr_groups_mp1,
> > > > > > +	.counters_nb = MP1_CNT_NB,
> > > > > > +	.evt_counters_nb = MP1_CNT_NB - 1, /* Time counter is not an event counter */
> > > > > > +	.time_cnt_idx = MP1_TIME_CNT_IDX,
> > > > > > +	.get_counter = stm32_ddr_pmu_get_event_counter_mp1,
> > > > > > +};
> > > > > > +
> > > > > > +static const struct stm32_ddr_pmu_cfg stm32_ddr_pmu_cfg_mp2 = {
> > > > > > +	.regs = &stm32_ddr_pmu_regspec_mp2,
> > > > > > +	.attribute = stm32_ddr_pmu_attr_groups_mp2,
> > > > > > +	.counters_nb = MP2_CNT_NB,
> > > > > > +	.evt_counters_nb = MP2_CNT_NB - 1, /* Time counter is an event counter */
> > > > > > +	.time_cnt_idx = MP2_TIME_CNT_IDX,
> > > > > > +	.get_counter = stm32_ddr_pmu_get_event_counter_mp2,
> > > > > > +};
> > > > > > +
> > > > > > +static const struct dev_pm_ops stm32_ddr_pmu_pm_ops = {
> > > > > > +	SET_SYSTEM_SLEEP_PM_OPS(NULL, stm32_ddr_pmu_device_resume)
> > > > > > +};
> > > > > > +
> > > > > > +static const struct of_device_id stm32_ddr_pmu_of_match[] = {
> > > > > > +	{
> > > > > > +		.compatible = "st,stm32mp131-ddr-pmu",
> > > > > > +		.data = &stm32_ddr_pmu_cfg_mp1
> > > > > > +	},
> > > > > > +	{
> > > > > > +		.compatible = "st,stm32mp151-ddr-pmu",
> > > > > > +		.data = &stm32_ddr_pmu_cfg_mp1
> > > > >
> > > > > So devices are compatible, thus express it correctly and drop this.
> > > >
> > > > Ok so I assume this comes with your comment in the bindings and
> > > > basically don't get you point here.
> > > > Can you please be more precise ?
> > >
> > > Express compatibility in the bindings, like 90% of SoCs are doing, so
> > > with proper fallback and drop this entry in the table. My comment was
> > > pretty precise, because this is completely standard pattern, also used
> > > already in stm32.
> >
> > Ok I remember your discussion with Alex in my V1 of pinctrl-hdp :
> > https://lore.kernel.org/all/1de58672-5355-4b75-99f4-c48687017d2f@xxxxxxxxxx/
> >
> > Does it suits you :
> > In the SoC DT:
> > MP13: compatible = "st,stm32mp131-ddr-pmu", "st,stm32mp1-ddr-pmu";
> > MP15: compatible = "st,stm32mp151-ddr-pmu", "st,stm32mp1-ddr-pmu";
>
> No, because I did not say to change other entry in the table. Please
> read again what I asked: drop this. "This" means ONLY this entry. "Drop
> this" does not mean "change something else". Do not change other entries
> by introducing some generic compatible. That's not the pattern ever
> endorsed by DT maintainers. Add front compatible and you are done,
> smallest amount of changes, most obvious code.

Ok so in the SoC DT I'll keep:
MP13: compatible = "st,stm32mp131-ddr-pmu";
MP15: compatible = "st,stm32mp151-ddr-pmu", "st,stm32mp131-ddr-pmu";

Thanks for clarifying.

Best regards,
Clément

> Best regards,
> Krzysztof