Message-ID: <0fe4165d-f198-42cc-9c2f-f1e51bd96716@kernel.org>
Date: Mon, 4 Aug 2025 16:33:46 +0200
From: Krzysztof Kozlowski <krzk@...nel.org>
To: AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>,
 Laura Nao <laura.nao@...labora.com>, wenst@...omium.org
Cc: conor+dt@...nel.org, devicetree@...r.kernel.org,
 guangjie.song@...iatek.com, kernel@...labora.com, krzk+dt@...nel.org,
 linux-arm-kernel@...ts.infradead.org, linux-clk@...r.kernel.org,
 linux-kernel@...r.kernel.org, linux-mediatek@...ts.infradead.org,
 matthias.bgg@...il.com, mturquette@...libre.com, netdev@...r.kernel.org,
 nfraprado@...labora.com, p.zabel@...gutronix.de, richardcochran@...il.com,
 robh@...nel.org, sboyd@...nel.org
Subject: Re: [PATCH v3 09/27] dt-bindings: clock: mediatek: Describe MT8196
 clock controllers

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.


Best regards,
Krzysztof

Powered by blists - more mailing lists