| Message ID | 20220415055916.28350-1-rex-bc.chen@mediatek.com |
|---|---|
| Headers | show |
| Series | cpufreq: mediatek: Cleanup and support MT8183 and MT8186 | expand |
On Fri, Apr 15, 2022 at 1:59 PM Rex-BC Chen <rex-bc.chen@mediatek.com> wrote: > > Add MediaTek CCI devfreq node for MT8183. > > Signed-off-by: Andrew-sh.Cheng <andrew-sh.cheng@mediatek.com> > Signed-off-by: Rex-BC Chen <rex-bc.chen@mediatek.com> > --- > arch/arm64/boot/dts/mediatek/mt8183-evb.dts | 4 ++++ > arch/arm64/boot/dts/mediatek/mt8183-kukui.dtsi | 4 ++++ > arch/arm64/boot/dts/mediatek/mt8183.dtsi | 7 +++++++ > 3 files changed, 15 insertions(+) > > diff --git a/arch/arm64/boot/dts/mediatek/mt8183-evb.dts b/arch/arm64/boot/dts/mediatek/mt8183-evb.dts > index 8953dbf84f3e..7ac9864db9de 100644 > --- a/arch/arm64/boot/dts/mediatek/mt8183-evb.dts > +++ b/arch/arm64/boot/dts/mediatek/mt8183-evb.dts > @@ -412,6 +412,10 @@ > > }; > > +&cci { > + proc-supply = <&mt6358_vproc12_reg>; > +}; > + > &cpu0 { > proc-supply = <&mt6358_vproc12_reg>; > }; > diff --git a/arch/arm64/boot/dts/mediatek/mt8183-kukui.dtsi b/arch/arm64/boot/dts/mediatek/mt8183-kukui.dtsi > index 0f9480f91261..4786a32ee975 100644 > --- a/arch/arm64/boot/dts/mediatek/mt8183-kukui.dtsi > +++ b/arch/arm64/boot/dts/mediatek/mt8183-kukui.dtsi > @@ -230,6 +230,10 @@ > status = "okay"; > }; > > +&cci { > + proc-supply = <&mt6358_vproc12_reg>; > +}; > + > &cpu0 { > proc-supply = <&mt6358_vproc12_reg>; > }; > diff --git a/arch/arm64/boot/dts/mediatek/mt8183.dtsi b/arch/arm64/boot/dts/mediatek/mt8183.dtsi > index 4ae3305d16d2..334728413582 100644 > --- a/arch/arm64/boot/dts/mediatek/mt8183.dtsi > +++ b/arch/arm64/boot/dts/mediatek/mt8183.dtsi > @@ -280,6 +280,13 @@ > }; > }; > > + cci: cci { > + compatible = "mediatek,mt8183-cci"; > + clocks = <&apmixedsys CLK_APMIXED_CCIPLL>; > + clock-names = "cci_clock"; > + operating-points-v2 = <&cci_opp>; hi Rex, cci_opp is not defined in dts. > + }; > + > cpus { > #address-cells = <1>; > #size-cells = <0>; > -- > 2.18.0 >
On Fri, Apr 15, 2022 at 2:06 PM Hsin-Yi Wang <hsinyi@google.com> wrote: > > On Fri, Apr 15, 2022 at 1:59 PM Rex-BC Chen <rex-bc.chen@mediatek.com> wrote: > > > > Add MediaTek CCI devfreq node for MT8183. > > > > Signed-off-by: Andrew-sh.Cheng <andrew-sh.cheng@mediatek.com> > > Signed-off-by: Rex-BC Chen <rex-bc.chen@mediatek.com> > > --- > > arch/arm64/boot/dts/mediatek/mt8183-evb.dts | 4 ++++ > > arch/arm64/boot/dts/mediatek/mt8183-kukui.dtsi | 4 ++++ > > arch/arm64/boot/dts/mediatek/mt8183.dtsi | 7 +++++++ > > 3 files changed, 15 insertions(+) > > > > diff --git a/arch/arm64/boot/dts/mediatek/mt8183-evb.dts b/arch/arm64/boot/dts/mediatek/mt8183-evb.dts > > index 8953dbf84f3e..7ac9864db9de 100644 > > --- a/arch/arm64/boot/dts/mediatek/mt8183-evb.dts > > +++ b/arch/arm64/boot/dts/mediatek/mt8183-evb.dts > > @@ -412,6 +412,10 @@ > > > > }; > > > > +&cci { > > + proc-supply = <&mt6358_vproc12_reg>; > > +}; > > + > > &cpu0 { > > proc-supply = <&mt6358_vproc12_reg>; > > }; > > diff --git a/arch/arm64/boot/dts/mediatek/mt8183-kukui.dtsi b/arch/arm64/boot/dts/mediatek/mt8183-kukui.dtsi > > index 0f9480f91261..4786a32ee975 100644 > > --- a/arch/arm64/boot/dts/mediatek/mt8183-kukui.dtsi > > +++ b/arch/arm64/boot/dts/mediatek/mt8183-kukui.dtsi > > @@ -230,6 +230,10 @@ > > status = "okay"; > > }; > > > > +&cci { > > + proc-supply = <&mt6358_vproc12_reg>; > > +}; > > + > > &cpu0 { > > proc-supply = <&mt6358_vproc12_reg>; > > }; > > diff --git a/arch/arm64/boot/dts/mediatek/mt8183.dtsi b/arch/arm64/boot/dts/mediatek/mt8183.dtsi > > index 4ae3305d16d2..334728413582 100644 > > --- a/arch/arm64/boot/dts/mediatek/mt8183.dtsi > > +++ b/arch/arm64/boot/dts/mediatek/mt8183.dtsi > > @@ -280,6 +280,13 @@ > > }; > > }; > > > > + cci: cci { > > + compatible = "mediatek,mt8183-cci"; > > + clocks = <&apmixedsys CLK_APMIXED_CCIPLL>; > > + clock-names = "cci_clock"; > > + operating-points-v2 = <&cci_opp>; > > hi Rex, > > cci_opp is not defined in dts. > It's in the previous patch. Please ignore this comment. > > + }; > > + > > cpus { > > #address-cells = <1>; > > #size-cells = <0>; > > -- > > 2.18.0 > >
On Fri, Apr 15, 2022 at 1:59 PM Rex-BC Chen <rex-bc.chen@mediatek.com> wrote: > > To prevent infinite loop when tracking voltage, we calculate the maximum > value for each platform data. > We assume min voltage is 0 and tracking target voltage using > min_volt_shift for each iteration. > The retry_max is 3 times of expeted iteration count. > > Signed-off-by: Rex-BC Chen <rex-bc.chen@mediatek.com> > --- > drivers/cpufreq/mediatek-cpufreq.c | 16 ++++++++++++++++ > 1 file changed, 16 insertions(+) > > diff --git a/drivers/cpufreq/mediatek-cpufreq.c b/drivers/cpufreq/mediatek-cpufreq.c > index cc44a7a9427a..d4c00237e862 100644 > --- a/drivers/cpufreq/mediatek-cpufreq.c > +++ b/drivers/cpufreq/mediatek-cpufreq.c > @@ -86,6 +86,16 @@ static int mtk_cpufreq_voltage_tracking(struct mtk_cpu_dvfs_info *info, > struct regulator *proc_reg = info->proc_reg; > struct regulator *sram_reg = info->sram_reg; > int pre_vproc, pre_vsram, new_vsram, vsram, vproc, ret; > + int retry_max; > + > + /* > + * We assume min voltage is 0 and tracking target voltage using > + * min_volt_shift for each iteration. > + * The retry_max is 3 times of expeted iteration count. > + */ > + retry_max = 3 * DIV_ROUND_UP(max(info->soc_data->sram_max_volt, > + info->soc_data->proc_max_volt), > + info->soc_data->min_volt_shift); mtk_cpufreq_voltage_tracking() will be called very frequently. retry_max is the same every time mtk_cpufreq_voltage_tracking() is called. Is it better to calculate before and store in mtk_cpu_dvfs_info? > > pre_vproc = regulator_get_voltage(proc_reg); > if (pre_vproc < 0) { > @@ -151,6 +161,12 @@ static int mtk_cpufreq_voltage_tracking(struct mtk_cpu_dvfs_info *info, > > pre_vproc = vproc; > pre_vsram = vsram; > + > + if (--retry_max < 0) { > + dev_err(info->cpu_dev, > + "over loop count, failed to set voltage\n"); > + return -EINVAL; > + } > } while (vproc != new_vproc || vsram != new_vsram); > > return 0; > -- > 2.18.0 >
On Fri, 2022-04-15 at 14:14 +0800, Hsin-Yi Wang wrote: > On Fri, Apr 15, 2022 at 1:59 PM Rex-BC Chen <rex-bc.chen@mediatek.com > > wrote: > > > > To prevent infinite loop when tracking voltage, we calculate the > > maximum > > value for each platform data. > > We assume min voltage is 0 and tracking target voltage using > > min_volt_shift for each iteration. > > The retry_max is 3 times of expeted iteration count. > > > > Signed-off-by: Rex-BC Chen <rex-bc.chen@mediatek.com> > > --- > > drivers/cpufreq/mediatek-cpufreq.c | 16 ++++++++++++++++ > > 1 file changed, 16 insertions(+) > > > > diff --git a/drivers/cpufreq/mediatek-cpufreq.c > > b/drivers/cpufreq/mediatek-cpufreq.c > > index cc44a7a9427a..d4c00237e862 100644 > > --- a/drivers/cpufreq/mediatek-cpufreq.c > > +++ b/drivers/cpufreq/mediatek-cpufreq.c > > @@ -86,6 +86,16 @@ static int mtk_cpufreq_voltage_tracking(struct > > mtk_cpu_dvfs_info *info, > > struct regulator *proc_reg = info->proc_reg; > > struct regulator *sram_reg = info->sram_reg; > > int pre_vproc, pre_vsram, new_vsram, vsram, vproc, ret; > > + int retry_max; > > + > > + /* > > + * We assume min voltage is 0 and tracking target voltage > > using > > + * min_volt_shift for each iteration. > > + * The retry_max is 3 times of expeted iteration count. > > + */ > > + retry_max = 3 * DIV_ROUND_UP(max(info->soc_data- > > >sram_max_volt, > > + info->soc_data- > > >proc_max_volt), > > + info->soc_data- > > >min_volt_shift); > > mtk_cpufreq_voltage_tracking() will be called very frequently. > retry_max is the same every time mtk_cpufreq_voltage_tracking() is > called. Is it better to calculate before and store in > mtk_cpu_dvfs_info? > Hello Hsin-Yi, Thanks for your reviwew. I will do this in next version. BRs, Rex > > > > pre_vproc = regulator_get_voltage(proc_reg); > > if (pre_vproc < 0) { > > @@ -151,6 +161,12 @@ static int mtk_cpufreq_voltage_tracking(struct > > mtk_cpu_dvfs_info *info, > > > > pre_vproc = vproc; > > pre_vsram = vsram; > > + > > + if (--retry_max < 0) { > > + dev_err(info->cpu_dev, > > + "over loop count, failed to set > > voltage\n"); > > + return -EINVAL; > > + } > > } while (vproc != new_vproc || vsram != new_vsram); > > > > return 0; > > -- > > 2.18.0 > >
Il 15/04/22 07:59, Rex-BC Chen ha scritto: > Add mediatek,cci property to support MediaTek CCI feature. > > Signed-off-by: Rex-BC Chen <rex-bc.chen@mediatek.com> Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
Il 15/04/22 07:59, Rex-BC Chen ha scritto: > Add MediaTek CCI devfreq node for MT8183. > > Signed-off-by: Andrew-sh.Cheng <andrew-sh.cheng@mediatek.com> > Signed-off-by: Rex-BC Chen <rex-bc.chen@mediatek.com> Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
Il 15/04/22 07:59, Rex-BC Chen ha scritto: > - Add cpufreq opp table. > - Add MediaTek cci opp table. > - Add property of opp table and clock fro cpufreq. > > Signed-off-by: Andrew-sh.Cheng <andrew-sh.cheng@mediatek.com> > Signed-off-by: Rex-BC Chen <rex-bc.chen@mediatek.com> > --- > arch/arm64/boot/dts/mediatek/mt8183-evb.dts | 32 +++ > arch/arm64/boot/dts/mediatek/mt8183.dtsi | 270 ++++++++++++++++++++ > 2 files changed, 302 insertions(+) > > diff --git a/arch/arm64/boot/dts/mediatek/mt8183-evb.dts b/arch/arm64/boot/dts/mediatek/mt8183-evb.dts > index f3fd3cca23e9..8953dbf84f3e 100644 > --- a/arch/arm64/boot/dts/mediatek/mt8183-evb.dts > +++ b/arch/arm64/boot/dts/mediatek/mt8183-evb.dts ..snip.. > diff --git a/arch/arm64/boot/dts/mediatek/mt8183.dtsi b/arch/arm64/boot/dts/mediatek/mt8183.dtsi > index 4b08691ed39e..4ae3305d16d2 100644 > --- a/arch/arm64/boot/dts/mediatek/mt8183.dtsi > +++ b/arch/arm64/boot/dts/mediatek/mt8183.dtsi > @@ -42,6 +42,244 @@ > rdma1 = &rdma1; > }; > > + cluster0_opp: cluster_opp_table0 { Please don't use underscores in devicetree for anything that's not a phandle. cluster0_opp: cluster0-opp-table { > + compatible = "operating-points-v2"; > + opp-shared; > + opp0_00 { Apart from the underscore being invalid, across the kernel, you can find many instances of opp tables, containing names related to the frequencies, so, for consistency (this is not a rule!), perhaps it would be a good idea to do so. opp-793000000 { > + opp-hz = /bits/ 64 <793000000>; > + opp-microvolt = <650000>; > + required-opps = <&opp2_00>; > + }; > + opp0_01 { opp-910000000 { > + opp-hz = /bits/ 64 <910000000>; > + opp-microvolt = <687500>; > + required-opps = <&opp2_01>; > + }; > + opp0_02 { ..snip.. > + opp0_15 { > + opp-hz = /bits/ 64 <1989000000>; > + opp-microvolt = <1050000>; > + required-opps = <&opp2_15>; P.S.: Please fix the typo below! > + }; }; > + > + ...also... > + cci_opp: opp_table2 { cci_opp: cci-opp-table { > + compatible = "operating-points-v2"; > + opp-shared; > + opp2_00: opp-273000000 { This one is fine instead :)) Cheers, Angelo
Il 15/04/22 07:59, Rex-BC Chen ha scritto: > From: Jia-Wei Chang <jia-wei.chang@mediatek.com> > > In some MediaTek SoCs, like MT8183, CPU and CCI share the same power > supplies. Cpufreq needs to check if CCI devfreq exists and wait until > CCI devfreq ready before scaling frequency. > > Before CCI devfreq is ready, we record the voltage when booting to > kernel and use the max(cpu target voltage, booting voltage) to > prevent cpufreq adjust to the lower voltage which will cause the CCI > crash because of high frequency and low voltage. > > - Add is_ccifreq_ready() to link CCI device to CPI, and CPU will start > DVFS when CCI is ready. > - Add platform data for MT8183. > > Signed-off-by: Jia-Wei Chang <jia-wei.chang@mediatek.com> > Signed-off-by: Rex-BC Chen <rex-bc.chen@mediatek.com> I am enthusiast to see that the solution that I've proposed was welcome! I only have one nit on this patch, check below: > --- > drivers/cpufreq/mediatek-cpufreq.c | 80 +++++++++++++++++++++++++++++- > 1 file changed, 79 insertions(+), 1 deletion(-) > > diff --git a/drivers/cpufreq/mediatek-cpufreq.c b/drivers/cpufreq/mediatek-cpufreq.c > index d4c00237e862..dd3f739fede1 100644 > --- a/drivers/cpufreq/mediatek-cpufreq.c > +++ b/drivers/cpufreq/mediatek-cpufreq.c ..snip.. > @@ -225,6 +251,14 @@ static int mtk_cpufreq_set_target(struct cpufreq_policy *policy, > vproc = dev_pm_opp_get_voltage(opp); > dev_pm_opp_put(opp); > > + /* > + * If MediaTek cci is supported but is not ready, we will use the value > + * of max(target cpu voltage, booting voltage) to prevent high freqeuncy > + * low voltage crash. > + */ > + if (info->soc_data->ccifreq_supported && !is_ccifreq_ready(info)) > + vproc = max(vproc, info->vproc_on_boot); > + > /* > * If the new voltage or the intermediate voltage is higher than the > * current voltage, scale up voltage first. ..snip.. > @@ -423,6 +484,13 @@ static int mtk_cpu_dvfs_info_init(struct mtk_cpu_dvfs_info *info, int cpu) > if (ret) > goto out_disable_mux_clock; > > + info->vproc_on_boot = regulator_get_voltage(info->proc_reg); This result is used only if we use ccifreq, so this should be enclosed in an if condition: this will spare us some (yes, small) time on devices that don't use it. if (info->soc_data->ccifreq_supported) { info->vproc_on_boot = regulator_get_voltage(info->proc_reg); if (info->vproc_on_boot < 0) { dev_err(.... goto .. } } P.S.: While at it, since the maximum width is 100 columns, the dev_err() call fits, so don't break that line! After the requested change: Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
Il 15/04/22 08:14, Hsin-Yi Wang ha scritto: > On Fri, Apr 15, 2022 at 1:59 PM Rex-BC Chen <rex-bc.chen@mediatek.com> wrote: >> >> To prevent infinite loop when tracking voltage, we calculate the maximum >> value for each platform data. >> We assume min voltage is 0 and tracking target voltage using >> min_volt_shift for each iteration. >> The retry_max is 3 times of expeted iteration count. >> >> Signed-off-by: Rex-BC Chen <rex-bc.chen@mediatek.com> I'm sorry Rex, but this commit has to be squashed with 09/15, as the logic is that each commit has to be acceptable, and 09/15 is not, without this fix. Besides, as Hsin-Yi suggested, calculating this every time may hit performance, but at the same time I don't want to lose this explicit calculation... >> --- >> drivers/cpufreq/mediatek-cpufreq.c | 16 ++++++++++++++++ >> 1 file changed, 16 insertions(+) >> >> diff --git a/drivers/cpufreq/mediatek-cpufreq.c b/drivers/cpufreq/mediatek-cpufreq.c >> index cc44a7a9427a..d4c00237e862 100644 >> --- a/drivers/cpufreq/mediatek-cpufreq.c >> +++ b/drivers/cpufreq/mediatek-cpufreq.c >> @@ -86,6 +86,16 @@ static int mtk_cpufreq_voltage_tracking(struct mtk_cpu_dvfs_info *info, >> struct regulator *proc_reg = info->proc_reg; >> struct regulator *sram_reg = info->sram_reg; >> int pre_vproc, pre_vsram, new_vsram, vsram, vproc, ret; >> + int retry_max; >> + >> + /* >> + * We assume min voltage is 0 and tracking target voltage using >> + * min_volt_shift for each iteration. >> + * The retry_max is 3 times of expeted iteration count. >> + */ >> + retry_max = 3 * DIV_ROUND_UP(max(info->soc_data->sram_max_volt, >> + info->soc_data->proc_max_volt), >> + info->soc_data->min_volt_shift); > > mtk_cpufreq_voltage_tracking() will be called very frequently. > retry_max is the same every time mtk_cpufreq_voltage_tracking() is > called. Is it better to calculate before and store in > mtk_cpu_dvfs_info? > ...so I agree with this solution: perhaps you can add a "vtrack_max" variable to mtk_cpu_dvfs_info as suggested, and fill in that one in function mtk_cpu_dvfs_info_init(), where we effectively initialize all-the-things. Cheers, Angelo
Il 15/04/22 07:59, Rex-BC Chen ha scritto: > From: Jia-Wei Chang <jia-wei.chang@mediatek.com> > > Voltages and shifts are defined as macros originally. > There are different requirements of these values for each MediaTek SoCs. > Therefore, we add the platform data and move these values into it. > > Signed-off-by: Jia-Wei Chang <jia-wei.chang@mediatek.com> > Signed-off-by: Rex-BC Chen <rex-bc.chen@mediatek.com> Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
Il 15/04/22 07:59, Rex-BC Chen ha scritto: > From: "Andrew-sh.Cheng" <andrew-sh.cheng@mediatek.com> > > From this opp notifier, cpufreq should listen to opp notification and do > proper actions when receiving events of disable and voltage adjustment. > > One of the user for this opp notifier is MediaTek SVS. > The MediaTek Smart Voltage Scaling (SVS) is a hardware which calculates > suitable SVS bank voltages to OPP voltage table. > > Signed-off-by: Andrew-sh.Cheng <andrew-sh.cheng@mediatek.com> > Signed-off-by: Jia-Wei Chang <jia-wei.chang@mediatek.com> > Signed-off-by: Rex-BC Chen <rex-bc.chen@mediatek.com> > --- > drivers/cpufreq/mediatek-cpufreq.c | 93 +++++++++++++++++++++++++++--- > 1 file changed, 85 insertions(+), 8 deletions(-) > > diff --git a/drivers/cpufreq/mediatek-cpufreq.c b/drivers/cpufreq/mediatek-cpufreq.c > index fa8b193bf27b..221f249f8d21 100644 > --- a/drivers/cpufreq/mediatek-cpufreq.c > +++ b/drivers/cpufreq/mediatek-cpufreq.c > @@ -41,6 +41,11 @@ struct mtk_cpu_dvfs_info { > int intermediate_voltage; > bool need_voltage_tracking; > int pre_vproc; > + /* Avoid race condition for regulators between notify and policy */ > + struct mutex reg_lock; > + struct notifier_block opp_nb; > + int opp_cpu; This should be unsigned int because: 1. A negative CPU number is impossible; 2. The only usage is as a parameter of cpufreq_cpu_get(unsigned int cpu). Please change this to unsigned int, after which: Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
Il 15/04/22 07:59, Rex-BC Chen ha scritto: > From: Jia-Wei Chang <jia-wei.chang@mediatek.com> > > We found the buck voltage may not be exactly the same with what we set > because CPU may share the same buck with other module. > Therefore, we need to record the previous desired value instead of reading > it from regulators. > > Signed-off-by: Andrew-sh.Cheng <andrew-sh.cheng@mediatek.com> > Signed-off-by: Jia-Wei Chang <jia-wei.chang@mediatek.com> > Signed-off-by: Rex-BC Chen <rex-bc.chen@mediatek.com> > --- > drivers/cpufreq/mediatek-cpufreq.c | 17 +++++++++++++---- > 1 file changed, 13 insertions(+), 4 deletions(-) > > diff --git a/drivers/cpufreq/mediatek-cpufreq.c b/drivers/cpufreq/mediatek-cpufreq.c > index ff27f77e8ee6..fa8b193bf27b 100644 > --- a/drivers/cpufreq/mediatek-cpufreq.c > +++ b/drivers/cpufreq/mediatek-cpufreq.c > @@ -40,6 +40,7 @@ struct mtk_cpu_dvfs_info { > struct list_head list_head; > int intermediate_voltage; > bool need_voltage_tracking; > + int pre_vproc; > }; > > static LIST_HEAD(dvfs_info_list); > @@ -191,11 +192,17 @@ static int mtk_cpufreq_voltage_tracking(struct mtk_cpu_dvfs_info *info, > > static int mtk_cpufreq_set_voltage(struct mtk_cpu_dvfs_info *info, int vproc) > { > + int ret; > + > if (info->need_voltage_tracking) > - return mtk_cpufreq_voltage_tracking(info, vproc); > + ret = mtk_cpufreq_voltage_tracking(info, vproc); > else > - return regulator_set_voltage(info->proc_reg, vproc, > - vproc + VOLT_TOL); > + ret = regulator_set_voltage(info->proc_reg, vproc, > + MAX_VOLT_LIMIT); > + if (!ret) > + info->pre_vproc = vproc; > + > + return ret; > } > > static int mtk_cpufreq_set_target(struct cpufreq_policy *policy, > @@ -213,7 +220,9 @@ static int mtk_cpufreq_set_target(struct cpufreq_policy *policy, > inter_vproc = info->intermediate_voltage; > > pre_freq_hz = clk_get_rate(cpu_clk); > - pre_vproc = regulator_get_voltage(info->proc_reg); > + pre_vproc = info->pre_vproc; > + if (pre_vproc <= 0) > + pre_vproc = regulator_get_voltage(info->proc_reg); I would do it like that, instead: if (unlikely(info->pre_vproc <= 0)) pre_vproc = regulator_get_voltage(info->proc_reg); else pre_vproc = info->pre_vproc; ....as even though it is indeed possible that info->pre_vproc is <= 0, it is very unlikely to happen ;-) This also solves a 'pre_vproc' double assignment issue, by the way. Cheers, Angelo
Il 15/04/22 07:59, Rex-BC Chen ha scritto: > To make driver more readable, replace old_* with pre_*. > > Signed-off-by: Rex-BC Chen <rex-bc.chen@mediatek.com> Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com
Il 15/04/22 07:59, Rex-BC Chen ha scritto: > - Replace pr_* with dev_* to show logs. > - Remove usage of __func__. > > Signed-off-by: Rex-BC Chen <rex-bc.chen@mediatek.com> Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com
On Fri, 2022-04-15 at 14:24 +0200, AngeloGioacchino Del Regno wrote: > Il 15/04/22 07:59, Rex-BC Chen ha scritto: > > From: Jia-Wei Chang <jia-wei.chang@mediatek.com> > > > > We found the buck voltage may not be exactly the same with what we > > set > > because CPU may share the same buck with other module. > > Therefore, we need to record the previous desired value instead of > > reading > > it from regulators. > > > > Signed-off-by: Andrew-sh.Cheng <andrew-sh.cheng@mediatek.com> > > Signed-off-by: Jia-Wei Chang <jia-wei.chang@mediatek.com> > > Signed-off-by: Rex-BC Chen <rex-bc.chen@mediatek.com> > > --- > > drivers/cpufreq/mediatek-cpufreq.c | 17 +++++++++++++---- > > 1 file changed, 13 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/cpufreq/mediatek-cpufreq.c > > b/drivers/cpufreq/mediatek-cpufreq.c > > index ff27f77e8ee6..fa8b193bf27b 100644 > > --- a/drivers/cpufreq/mediatek-cpufreq.c > > +++ b/drivers/cpufreq/mediatek-cpufreq.c > > @@ -40,6 +40,7 @@ struct mtk_cpu_dvfs_info { > > struct list_head list_head; > > int intermediate_voltage; > > bool need_voltage_tracking; > > + int pre_vproc; > > }; > > > > static LIST_HEAD(dvfs_info_list); > > @@ -191,11 +192,17 @@ static int > > mtk_cpufreq_voltage_tracking(struct mtk_cpu_dvfs_info *info, > > > > static int mtk_cpufreq_set_voltage(struct mtk_cpu_dvfs_info > > *info, int vproc) > > { > > + int ret; > > + > > if (info->need_voltage_tracking) > > - return mtk_cpufreq_voltage_tracking(info, vproc); > > + ret = mtk_cpufreq_voltage_tracking(info, vproc); > > else > > - return regulator_set_voltage(info->proc_reg, vproc, > > - vproc + VOLT_TOL); > > + ret = regulator_set_voltage(info->proc_reg, vproc, > > + MAX_VOLT_LIMIT); > > + if (!ret) > > + info->pre_vproc = vproc; > > + > > + return ret; > > } > > > > static int mtk_cpufreq_set_target(struct cpufreq_policy *policy, > > @@ -213,7 +220,9 @@ static int mtk_cpufreq_set_target(struct > > cpufreq_policy *policy, > > inter_vproc = info->intermediate_voltage; > > > > pre_freq_hz = clk_get_rate(cpu_clk); > > - pre_vproc = regulator_get_voltage(info->proc_reg); > > + pre_vproc = info->pre_vproc; > > + if (pre_vproc <= 0) > > + pre_vproc = regulator_get_voltage(info->proc_reg); > > I would do it like that, instead: > > if (unlikely(info->pre_vproc <= 0)) > pre_vproc = regulator_get_voltage(info->proc_reg); > else > pre_vproc = info->pre_vproc; > > ....as even though it is indeed possible that info->pre_vproc is <= > 0, it is > very unlikely to happen ;-) > This also solves a 'pre_vproc' double assignment issue, by the way. > > Cheers, > Angelo > > > Hello Angelo, OK, I will add this in next version. Thanks for your suggestion. BRs, Rex
On Fri, 2022-04-15 at 14:24 +0200, AngeloGioacchino Del Regno wrote: > Il 15/04/22 08:14, Hsin-Yi Wang ha scritto: > > On Fri, Apr 15, 2022 at 1:59 PM Rex-BC Chen < > > rex-bc.chen@mediatek.com> wrote: > > > > > > To prevent infinite loop when tracking voltage, we calculate the > > > maximum > > > value for each platform data. > > > We assume min voltage is 0 and tracking target voltage using > > > min_volt_shift for each iteration. > > > The retry_max is 3 times of expeted iteration count. > > > > > > Signed-off-by: Rex-BC Chen <rex-bc.chen@mediatek.com> > > I'm sorry Rex, but this commit has to be squashed with 09/15, as the > logic is > that each commit has to be acceptable, and 09/15 is not, without this > fix. > > Besides, as Hsin-Yi suggested, calculating this every time may hit > performance, > but at the same time I don't want to lose this explicit > calculation... > Hello Angelo, I will squash thius patch into 9/15 and will use info->vtrack_max to record the value in probe function. Thanks! BRs, Rex > > > --- > > > drivers/cpufreq/mediatek-cpufreq.c | 16 ++++++++++++++++ > > > 1 file changed, 16 insertions(+) > > > > > > diff --git a/drivers/cpufreq/mediatek-cpufreq.c > > > b/drivers/cpufreq/mediatek-cpufreq.c > > > index cc44a7a9427a..d4c00237e862 100644 > > > --- a/drivers/cpufreq/mediatek-cpufreq.c > > > +++ b/drivers/cpufreq/mediatek-cpufreq.c > > > @@ -86,6 +86,16 @@ static int mtk_cpufreq_voltage_tracking(struct > > > mtk_cpu_dvfs_info *info, > > > struct regulator *proc_reg = info->proc_reg; > > > struct regulator *sram_reg = info->sram_reg; > > > int pre_vproc, pre_vsram, new_vsram, vsram, vproc, ret; > > > + int retry_max; > > > + > > > + /* > > > + * We assume min voltage is 0 and tracking target voltage > > > using > > > + * min_volt_shift for each iteration. > > > + * The retry_max is 3 times of expeted iteration count. > > > + */ > > > + retry_max = 3 * DIV_ROUND_UP(max(info->soc_data- > > > >sram_max_volt, > > > + info->soc_data- > > > >proc_max_volt), > > > + info->soc_data- > > > >min_volt_shift); > > > > mtk_cpufreq_voltage_tracking() will be called very frequently. > > retry_max is the same every time mtk_cpufreq_voltage_tracking() is > > called. Is it better to calculate before and store in > > mtk_cpu_dvfs_info? > > > > ...so I agree with this solution: perhaps you can add a "vtrack_max" > variable to > mtk_cpu_dvfs_info as suggested, and fill in that one in function > mtk_cpu_dvfs_info_init(), where we effectively initialize all-the- > things. > > Cheers, > Angelo
On Fri, 2022-04-15 at 14:24 +0200, AngeloGioacchino Del Regno wrote: > Il 15/04/22 07:59, Rex-BC Chen ha scritto: > > From: Jia-Wei Chang <jia-wei.chang@mediatek.com> > > > > In some MediaTek SoCs, like MT8183, CPU and CCI share the same > > power > > supplies. Cpufreq needs to check if CCI devfreq exists and wait > > until > > CCI devfreq ready before scaling frequency. > > > > Before CCI devfreq is ready, we record the voltage when booting to > > kernel and use the max(cpu target voltage, booting voltage) to > > prevent cpufreq adjust to the lower voltage which will cause the > > CCI > > crash because of high frequency and low voltage. > > > > - Add is_ccifreq_ready() to link CCI device to CPI, and CPU will > > start > > DVFS when CCI is ready. > > - Add platform data for MT8183. > > > > Signed-off-by: Jia-Wei Chang <jia-wei.chang@mediatek.com> > > Signed-off-by: Rex-BC Chen <rex-bc.chen@mediatek.com> > > I am enthusiast to see that the solution that I've proposed was > welcome! > > I only have one nit on this patch, check below: > Hello Angelo, Thanks for your advice for this modification. I also reply to Kevin and describe this solution. Let's wait for Kevin's feedback and other suggestion. > > --- > > drivers/cpufreq/mediatek-cpufreq.c | 80 > > +++++++++++++++++++++++++++++- > > 1 file changed, 79 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/cpufreq/mediatek-cpufreq.c > > b/drivers/cpufreq/mediatek-cpufreq.c > > index d4c00237e862..dd3f739fede1 100644 > > --- a/drivers/cpufreq/mediatek-cpufreq.c > > +++ b/drivers/cpufreq/mediatek-cpufreq.c > > ..snip.. > > > @@ -225,6 +251,14 @@ static int mtk_cpufreq_set_target(struct > > cpufreq_policy *policy, > > vproc = dev_pm_opp_get_voltage(opp); > > dev_pm_opp_put(opp); > > > > + /* > > + * If MediaTek cci is supported but is not ready, we will use > > the value > > + * of max(target cpu voltage, booting voltage) to prevent high > > freqeuncy > > + * low voltage crash. > > + */ > > + if (info->soc_data->ccifreq_supported && > > !is_ccifreq_ready(info)) > > + vproc = max(vproc, info->vproc_on_boot); > > + > > /* > > * If the new voltage or the intermediate voltage is higher > > than the > > * current voltage, scale up voltage first. > > ..snip.. > > > @@ -423,6 +484,13 @@ static int mtk_cpu_dvfs_info_init(struct > > mtk_cpu_dvfs_info *info, int cpu) > > if (ret) > > goto out_disable_mux_clock; > > > > + info->vproc_on_boot = regulator_get_voltage(info->proc_reg); > > This result is used only if we use ccifreq, so this should be > enclosed in an if > condition: this will spare us some (yes, small) time on devices that > don't use it. > > if (info->soc_data->ccifreq_supported) { > info->vproc_on_boot = regulator_get_voltage(info- > >proc_reg); > if (info->vproc_on_boot < 0) { > dev_err(.... > goto .. > } > } > > P.S.: While at it, since the maximum width is 100 columns, the > dev_err() call fits, > so don't break that line! > > After the requested change: > > Reviewed-by: AngeloGioacchino Del Regno < > angelogioacchino.delregno@collabora.com> I will add this in next version if there is no any suggestion for this patch. Thanks! BRs, Rex
On Fri, 2022-04-15 at 14:24 +0200, AngeloGioacchino Del Regno wrote: > Il 15/04/22 07:59, Rex-BC Chen ha scritto: > > - Add cpufreq opp table. > > - Add MediaTek cci opp table. > > - Add property of opp table and clock fro cpufreq. > > > > Signed-off-by: Andrew-sh.Cheng <andrew-sh.cheng@mediatek.com> > > Signed-off-by: Rex-BC Chen <rex-bc.chen@mediatek.com> > > --- > > arch/arm64/boot/dts/mediatek/mt8183-evb.dts | 32 +++ > > arch/arm64/boot/dts/mediatek/mt8183.dtsi | 270 > > ++++++++++++++++++++ > > 2 files changed, 302 insertions(+) > > > > diff --git a/arch/arm64/boot/dts/mediatek/mt8183-evb.dts > > b/arch/arm64/boot/dts/mediatek/mt8183-evb.dts > > index f3fd3cca23e9..8953dbf84f3e 100644 > > --- a/arch/arm64/boot/dts/mediatek/mt8183-evb.dts > > +++ b/arch/arm64/boot/dts/mediatek/mt8183-evb.dts > > ..snip.. > > > diff --git a/arch/arm64/boot/dts/mediatek/mt8183.dtsi > > b/arch/arm64/boot/dts/mediatek/mt8183.dtsi > > index 4b08691ed39e..4ae3305d16d2 100644 > > --- a/arch/arm64/boot/dts/mediatek/mt8183.dtsi > > +++ b/arch/arm64/boot/dts/mediatek/mt8183.dtsi > > @@ -42,6 +42,244 @@ > > rdma1 = &rdma1; > > }; > > > > + cluster0_opp: cluster_opp_table0 { > > Please don't use underscores in devicetree for anything that's not a > phandle. > > cluster0_opp: cluster0-opp-table { > > > + compatible = "operating-points-v2"; > > + opp-shared; > > + opp0_00 { > > Apart from the underscore being invalid, across the kernel, you can > find many > instances of opp tables, containing names related to the frequencies, > so, for > consistency (this is not a rule!), perhaps it would be a good idea to > do so. > > opp-793000000 { > > > + opp-hz = /bits/ 64 <793000000>; > > + opp-microvolt = <650000>; > > + required-opps = <&opp2_00>; > > + }; > > + opp0_01 { > > opp-910000000 { > > > + opp-hz = /bits/ 64 <910000000>; > > + opp-microvolt = <687500>; > > + required-opps = <&opp2_01>; > > + }; > > + opp0_02 { > > ..snip.. > > > + opp0_15 { > > + opp-hz = /bits/ 64 <1989000000>; > > + opp-microvolt = <1050000>; > > + required-opps = <&opp2_15>; > > P.S.: Please fix the typo below! > > > + }; }; > > + > > + > > ...also... > > > + cci_opp: opp_table2 { > > cci_opp: cci-opp-table { > > > + compatible = "operating-points-v2"; > > + opp-shared; > > + opp2_00: opp-273000000 { > > This one is fine instead :)) > > Cheers, > Angelo Hello Angelo, OK, I will modify this issue in next version. Thanks! BRs, Rex
Hi Rex, Rex-BC Chen <rex-bc.chen@mediatek.com> writes: > From: Jia-Wei Chang <jia-wei.chang@mediatek.com> > > In some MediaTek SoCs, like MT8183, CPU and CCI share the same power > supplies. Cpufreq needs to check if CCI devfreq exists and wait until > CCI devfreq ready before scaling frequency. > > Before CCI devfreq is ready, we record the voltage when booting to > kernel and use the max(cpu target voltage, booting voltage) to > prevent cpufreq adjust to the lower voltage which will cause the CCI > crash because of high frequency and low voltage. > > - Add is_ccifreq_ready() to link CCI device to CPI, and CPU will start > DVFS when CCI is ready. > - Add platform data for MT8183. > > Signed-off-by: Jia-Wei Chang <jia-wei.chang@mediatek.com> > Signed-off-by: Rex-BC Chen <rex-bc.chen@mediatek.com> The solution of keeping the max of the CPU voltage from OPP and boot-up voltage makes sense until CCI is ready. Thank you for the rework and the detailed technical explanations. Reviewed-by: Kevin Hilman <khilman@baylibre.com> Kevin
Cpufreq is a DVFS driver used for power saving to scale the clock frequency and supply the voltage for CPUs. This series do some cleanup for MediaTek cpufreq drivers and add support for MediaTek SVS[2] and MediaTek CCI devfreq[3] which are supported in MT8183 and MT8186. Changes for V3: 1. Rebased to linux-next-20220414. 2. Drop accepted patches. 3. Drop "cpufreq: mediatek: Use maximum voltage in init stage" because we make sure the voltage we set is safe for both mediatek cci and cpufreq. 4. Rename cci property to mediatek,cci. 5. Adjust order of cleanup patches. 6. Add new patches for cleanup, handle infinite loop and MT8183 dts. 7. Revise drivers from reviewers' suggestion. 8. Revise commit message of some patches to avoid confusion and misunderstand. 9. Revise "cpufreq: mediatek: Link CCI device to CPU". We do not return successful to pretend we set the target frequency done when cci is not ready. Instead, we find and set a safe voltage so that we can set the target cpufrequency. Changes for V2: 1. Drop the modification of transforming cpufreq-mediatek into yaml and only add the MediaTek CCI property for MediaTek cpufreq. 2. Split the original patches into several patches. Reference series: [1]: V1 of this series is present by Jia-Wei Chang. message-id:20220307122151.11666-1-jia-wei.chang@mediatek.com [2]: The MediaTek CCI devfreq driver is introduced in another series. message-id:20220408052150.22536-1-johnson.wang@mediatek.com [3]: The MediaTek SVS driver is introduced in another series. message-id:20220221063939.14969-1-roger.lu@mediatek.com Andrew-sh.Cheng (1): cpufreq: mediatek: Add opp notification support Jia-Wei Chang (8): dt-bindings: cpufreq: mediatek: Add MediaTek CCI property cpufreq: mediatek: Record previous target vproc value cpufreq: mediatek: Move voltage limits to platform data cpufreq: mediatek: Add .get function cpufreq: mediatek: Make sram regulator optional cpufreq: mediatek: Refine mtk_cpufreq_voltage_tracking() cpufreq: mediatek: Link CCI device to CPU cpufreq: mediatek: Add support for MT8186 Rex-BC Chen (6): cpufreq: mediatek: Use device print to show logs cpufreq: mediatek: Replace old_* with pre_* cpufreq: mediatek: Add counter to prevent infinite loop when tracking voltage arm64: dts: mediatek: Add opp table and clock property for MT8183 cpufreq arm64: dts: mediatek: Add MediaTek CCI node for MT8183 arm64: dts: mediatek: Add mediatek,cci property for MT8183 cpufreq .../bindings/cpufreq/cpufreq-mediatek.txt | 5 + arch/arm64/boot/dts/mediatek/mt8183-evb.dts | 36 ++ .../arm64/boot/dts/mediatek/mt8183-kukui.dtsi | 4 + arch/arm64/boot/dts/mediatek/mt8183.dtsi | 285 ++++++++++ drivers/cpufreq/mediatek-cpufreq.c | 497 ++++++++++++------ 5 files changed, 664 insertions(+), 163 deletions(-)