Re: [PATCH V4 01/14] dt-bindings: cpufreq: mediatek: Add MediaTek CCI property

[Rex-BC Chen <rex-bc.chen@mediatek.com>]

On Mon, 2022-04-25 at 10:55 +0200, Krzysztof Kozlowski wrote:
> On 25/04/2022 08:19, Rex-BC Chen wrote:
> > On Fri, 2022-04-22 at 19:34 +0200, Krzysztof Kozlowski wrote:
> > > On 22/04/2022 19:26, Krzysztof Kozlowski wrote:
> > > > On 22/04/2022 09:52, Rex-BC Chen wrote:
> > > > > MediaTek Cache Coherent Interconnect (CCI) uses software
> > > > > devfreq
> > > > > module
> > > > > for scaling clock frequency and adjust voltage.
> > > > > The phandle could be linked between CPU and MediaTek CCI for
> > > > > some
> > > > > MediaTek SoCs, like MT8183 and MT8186.
> > > > > Therefore, we add this property in cpufreq-mediatek.txt.
> > > > > 
> > > > > Signed-off-by: Rex-BC Chen <rex-bc.chen@mediatek.com>
> > > > > ---
> > > > >  .../devicetree/bindings/cpufreq/cpufreq-
> > > > > mediatek.txt         | 5
> > > > > +++++
> > > > >  1 file changed, 5 insertions(+)
> > > > > 
> > > > > diff --git
> > > > > a/Documentation/devicetree/bindings/cpufreq/cpufreq-
> > > > > mediatek.txt
> > > > > b/Documentation/devicetree/bindings/cpufreq/cpufreq-
> > > > > mediatek.txt
> > > > > index b8233ec91d3d..3387e1e2a2df 100644
> > > > > --- a/Documentation/devicetree/bindings/cpufreq/cpufreq-
> > > > > mediatek.txt
> > > > > +++ b/Documentation/devicetree/bindings/cpufreq/cpufreq-
> > > > > mediatek.txt
> > > > > @@ -20,6 +20,11 @@ Optional properties:
> > > > >  	       Vsram to fit SoC specific needs. When absent,
> > > > > the
> > > > > voltage scaling
> > > > >  	       flow is handled by hardware, hence no software
> > > > > "voltage
> > > > > tracking" is
> > > > >  	       needed.
> > > > > +- mediatek,cci:
> > > > > +	MediaTek Cache Coherent Interconnect (CCI) uses the
> > > > > software
> > > > > devfreq module to
> > > > > +	scale the clock frequency and adjust the voltage.
> > > > 
> > > > Devfreq is a SW mechanism, it should not be part of bindings
> > > > description.
> > 
> > Hello Krzysztof,
> > 
> > The reason we want to get the "mediatek,cci":
> > We need to check the mediatek cci is ready and probed done.
> > Because cpufreq and mediatek cci are sharing the same regulator in
> > little core cpus.
> > Therefore, to prevent high frequency low voltage issue, we need to
> > make
> > sure the mediatek cci is ready.
> > 
> > If mediatek cci is ready, cpufreq and mediatek cci will register
> > the
> > same regulator and from regulator's implementation, if there are
> > two
> > device using the same regulator, the framwork will make sure it's
> > using
> > the max voltage.
> 
> Thanks for explanation. The property should be described with what
> you
> said here. The property and description should match hardware, so
> there
> is no place for devfreq. Instead mention that power rail is shared or
> voltage regulators are common.
> 

Hello Krzysztof,

I will modify the description to the reason why we need mediatek,cci.

> However I am not sure if you solved your problem... see below:
> 
> > For example:
> > mediatek cci set 1.2V originally. When cpufreq want to adjust lower
> > frequency adn set voltage to 1.0V.
> > The framework will remain using 1.2V to prevent crash of mediatek
> > cci.
> 
> No, regulator_set_voltage() for proc_reg says:
> "NOTE: If the regulator is shared between several devices then the
> lowest
>  request voltage that meets the system constraints will be used."
> 
> Not the highest. So when your devfreq and cpufreq boots, calling
> regulator_set_voltage will still cause high frequency and low
> voltage.
> 

From the driver comment, I think it still needs to match "meets the
system constraints".

From drivers, we can trace the driver and it finally to
regulator_get_optimal_voltage().
In [1], the framework will get max voltage while finding each device's
voltage.

[1]: 
https://elixir.bootlin.com/linux/latest/source/drivers/regulator/core.c#L3815

> > 
> > Therefore, we need to confirm the mediatek cci is ready and
> > register
> > the regulator.
> > 
> > > > 
> > > > > +	For details, please refer to
> > > > > +	Documentation/devicetree/bindings/interconnect/mediatek
> > > > > ,cci.yam
> > > > > l
> > > > 
> > > > Since the file does not exist, I have troubles reviewing it.
> > > > First
> > > > of
> > > > all, you already have "mediatek,cci-control" property in DT, so
> > > > why
> > > > using different name?
> > 
> > I am not sure where is "mediatek,cci-control". I think this name is
> > not
> > used before.
> > 
> 
> Documentation/devicetree/bindings/net/mediatek-net.txt
> 
> > > > 
> > > > Second, it looks like you want to put devfreq into bindings
> > > > instead
> > > > of
> > > > using proper interconnect bindings.
> > > 
> > > Actually judging by the driver this looks like some
> > > device-boot-time-ordering, so I wonder whether this is a proper
> > > way
> > > to
> > > express it.
> > 
> > Yes, we need to get the mediatek cci node and let cpufreq and
> > mediatek
> > cci link succefully. In that case, we can know the mediatek cci is
> > ready. And we can set the voltage using the regulator framwork.
> > 
> > [1]: 
> > 
https://urldefense.com/v3/__https://patchwork.kernel.org/project/linux-mediatek/patch/20220422075239.16437-11-rex-bc.chen@mediatek.com/__;!!CTRNKA9wMg0ARbw!xOuKKyjBosmRcUseQXU9SiPu8msBXrrQAASdxwVbR0SU2inuXUtO180Y0Erkpy-JmOwu$
> >  
> 
> Yes, I see the use case. I am not convinced yet whether this is
> proper
> approach...
> 

When mediatek cci is ready (probe done and register regulator done), we
can confirm that regulator framwork will make sure the voltage setting
is safe.

BRs,
Rex

> 
> Best regards,
> Krzysztof