[PATCH v3 09/27] dt-bindings: clock: mediatek: Describe MT8196 clock controllers

[AngeloGioacchino Del Regno]

Il 04/08/25 16:33, Krzysztof Kozlowski ha scritto:
> On 04/08/2025 16:31, AngeloGioacchino Del Regno wrote:
>> Il 04/08/25 16:19, Krzysztof Kozlowski ha scritto:
>>> On 04/08/2025 15:58, Krzysztof Kozlowski wrote:
>>>>>
>>>>> So, what should we do then?
>>>>>
>>>>> Change it to "mediatek,clock-hw-refcounter", and adding a comment to the binding
>>>>> saying that this is called "Hardware Voter (HWV)" in the datasheets?
>>>>>
>>>>> Or is using the "interconnect" property without any driver in the interconnect API
>>>>> actually legit? - Because to me it doesn't look like being legit (and if it is, it
>>>>> shouldn't be, as I'm sure that everyone would expect an interconnect API driver
>>>>> when encountering an "interconnect" property in DT), and if so, we should just add
>>>>
>>>> Why you would not add any interconnect driver for interconnect API?
>>>> Look, the current phandle allows you to poke in some other MMIO space
>>>> for the purpose of enabling the clock FOO? So interconnect or power
>>>> domains or whatever allows you to have existing or new driver to receive
>>>> xlate() and, when requested resources associated with clock FOO.
>>>
>>> Something got here cut. Last sentence is supposed to be:
>>>
>>> "So interconnect or power
>>> domains or whatever allows you to have existing or new driver to receive
>>> xlate() and, when requested, toggle the resources associated with clock
>>> FOO."
>>>
>>>>
>>>> Instead of the FOO clock driver poking resources, you do
>>>> clk_prepare_enable() or pm_domain or icc_enable().
>>>
>>> I looked now at the driver and see your clock drivers poking via regmap
>>> to other MMIO. That's exactly usecase of syscon and exactly the pattern
>>> *we are usually discouraging*. It's limited, non-scalable and vendor-driven.
>>>
>>
>> If the HWV wasn't BROKEN, I'd be the first one to go for generic stuff, but
>> since it is what it is, adding bloat to generic, non vendor-driven APIs would
>> be bad.
>>
>>> If this was a power domain provider then:
>>> 1. Your clock drivers would only do runtime PM.
>>
>> The clock drivers would have to get a list of power domain that is *equal to*
>> (in their amount) the list of clocks.
>> But then those are not power domains, as those registers in the MCU are ONLY
>> ungating a clock and nothing else in the current state of the hardware.
>>
>>> 2. Your MCU would be the power domain controller doing whatever is
>>> necessary - toggling these set/clr bits - when given clock is enabled.
>>
>> That MCU does support power domain voting (for two power domains in the main
>> PD Controller, and for all power domains in the multimedia PD controller), and
>> this is something completely separated from the *clock* controllers.
>>
>> Just to make the picture complete for you: the power domains that this MCU can
>> manage are not in any way related to the clocks that it can manage. At all.
> 
> 
> OK, thanks for explanations. Please rephrase commit msg and property
> description in v4. I am fine in using "hardware voter" terminology in
> some places, so it will match datasheet, but I want to make it clear
> that it is not voting for resources how we usually understand it. It's
> just syscon stuff, poking in other system-like device registers because
> hardware is like that.
> 

I'm happy that we finally reached a conclusion that works for both of us, and
I am sorry that all this went on for weeks with (very) long discussions.

Thanks for that.

Regards,
Angelo

> 
> Best regards,
> Krzysztof