| Message ID | 20220422075239.16437-2-rex-bc.chen@mediatek.com |
|---|---|
| State | New |
| Headers | show |
| Series | cpufreq: mediatek: Cleanup and support MT8183 and MT8186 | expand |
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. > + For details, please refer to > + Documentation/devicetree/bindings/interconnect/mediatek,cci.yaml 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? Second, it looks like you want to put devfreq into bindings instead of using proper interconnect bindings. Best regards, Krzysztof
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. 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. 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. > > > > 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://patchwork.kernel.org/project/linux-mediatek/patch/20220422075239.16437-11-rex-bc.chen@mediatek.com/ BRs, Rex > > > Best regards, > Krzysztof
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. 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. > > 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://patchwork.kernel.org/project/linux-mediatek/patch/20220422075239.16437-11-rex-bc.chen@mediatek.com/ Yes, I see the use case. I am not convinced yet whether this is proper approach... Best regards, Krzysztof
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. >
On 25/04/2022 12:20, Rex-BC Chen wrote: >> 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 Right, actually in your case it's the regulator_check_consumers() above that line, because you I think it's quite generic problem, so would be worth solving for more regulator consumers, but your approach is fine. At least I do not have anything smarter, at the moment. Best regards, Krzysztof
On Mon, 2022-04-25 at 12:52 +0200, Krzysztof Kozlowski wrote: > On 25/04/2022 12:20, Rex-BC Chen wrote: > > > 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://urldefense.com/v3/__https://elixir.bootlin.com/linux/latest/source/drivers/regulator/core.c*L3815__;Iw!!CTRNKA9wMg0ARbw!35L9ISPWDpbGPsLEc0935D2nWTLtaLSNfPipvteddPaxwO2i_KaN0wfWegpkyaPjjagW$ > > > > Right, actually in your case it's the regulator_check_consumers() > above > that line, because you > > I think it's quite generic problem, so would be worth solving for > more > regulator consumers, but your approach is fine. At least I do not > have > anything smarter, at the moment. > > Best regards, > Krzysztof Hello Krzysztof, Thanks for your review! I will send next version to modify the description of mediatek,cci when the driver part is explained clear. BRs, Rex
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. + For details, please refer to + Documentation/devicetree/bindings/interconnect/mediatek,cci.yaml - #cooling-cells: For details, please refer to Documentation/devicetree/bindings/thermal/thermal-cooling-devices.yaml
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(+)