Re: [PATCH 0/8] Add cpufreq and cci devfreq for mt8183, and SVS support

Re: [PATCH 0/8] Add cpufreq and cci devfreq for mt8183, and SVS support

[Viresh Kumar]

On 16-05-19, 17:08, Andrew-sh.Cheng wrote:
> From: "Andrew-sh.Cheng" <andrew-sh.cheng@mediatek.com>
> 
> MT8183 supports CPU DVFS and CCI DVFS, and LITTLE cpus and CCI are in the same voltage domain.
> So, this series is to add drivers to handle the voltage coupling between CPU and CCI DVFS.
> 
> For SVS support, add OPP_EVENT_ADJUST_VOLTAGE and corresponding reaction.

No version information here or what has changed. That doesn't help.

I believe this is V3.

Don't resend it but please mention the changes in reply now. Thanks.

> Andrew-sh.Cheng (7):
>   cpufreq: mediatek: change to regulator_get_optional
>   cpufreq: mediatek: add clock enable for intermediate clock
>   cpufreq: mediatek: Add support for mt8183
>   dt-bindings: devfreq: add compatible for mt8183 cci devfreq
>   devfreq: add mediatek cci devfreq
>   cpufreq: mediatek: add opp notification for SVS support
>   devfreq: cci devfreq register opp notification for SVS support
> 
> Stephen Boyd (1):
>   PM / OPP: Support adjusting OPP voltages at runtime
> 
>  .../bindings/devfreq/mt8183-cci-devfreq.txt        |  20 ++
>  drivers/cpufreq/cpufreq-dt-platdev.c               |   1 +
>  drivers/cpufreq/mediatek-cpufreq.c                 |  88 +++++-
>  drivers/devfreq/Kconfig                            |  10 +
>  drivers/devfreq/Makefile                           |   1 +
>  drivers/devfreq/mt8183-cci-devfreq.c               | 310 +++++++++++++++++++++
>  drivers/opp/core.c                                 |  78 ++++++
>  include/linux/pm_opp.h                             |  11 +
>  8 files changed, 517 insertions(+), 2 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/devfreq/mt8183-cci-devfreq.txt
>  create mode 100644 drivers/devfreq/mt8183-cci-devfreq.c
> 
> -- 
> 2.12.5

-- 
viresh

Re: [PATCH 4/8] dt-bindings: devfreq: add compatible for mt8183 cci devfreq

[Viresh Kumar]

On 16-05-19, 17:08, Andrew-sh.Cheng wrote:
> From: "Andrew-sh.Cheng" <andrew-sh.cheng@mediatek.com>
> 
> This adds dt-binding documentation of cci devfreq
> for Mediatek MT8183 SoC platform.
> 
> Signed-off-by: Andrew-sh.Cheng <andrew-sh.cheng@mediatek.com>
> ---
>  .../bindings/devfreq/mt8183-cci-devfreq.txt          | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/devfreq/mt8183-cci-devfreq.txt
> 
> diff --git a/Documentation/devicetree/bindings/devfreq/mt8183-cci-devfreq.txt b/Documentation/devicetree/bindings/devfreq/mt8183-cci-devfreq.txt
> new file mode 100644
> index 000000000000..3189902902e0
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/devfreq/mt8183-cci-devfreq.txt
> @@ -0,0 +1,20 @@
> +* Mediatek Cache Coherent Interconnect(CCI) frequency device
> +
> +Required properties:
> +- compatible: should contain "mediatek,mt8183-cci" for frequency scaling of CCI

Example doesn't have this compatible .

> +- clocks: for frequency scaling of CCI
> +- clock-names: for frequency scaling of CCI driver to reference
> +- regulator: for voltage scaling of CCI
> +- operating-points-v2: for frequency scaling of CCI opp table
> +
> +Example:
> +	cci: cci {
> +		compatible = "mediatek,cci";
> +		clocks = <&apmixedsys CLK_APMIXED_CCIPLL>;
> +		clock-names = "cci_clock";
> +		operating-points-v2 = <&cci_opp>;
> +	};
> +
> +	&cci {
> +		proc-supply = <&mt6358_vproc12_reg>;
> +	};
> \ No newline at end of file
> -- 
> 2.12.5

-- 
viresh

Re: [PATCH 6/8] PM / OPP: Support adjusting OPP voltages at runtime

[Viresh Kumar]

On 16-05-19, 17:08, Andrew-sh.Cheng wrote:
> From: Stephen Boyd <sboyd@codeaurora.org>
> 
> On some SoCs the Adaptive Voltage Scaling (AVS) technique is
> employed to optimize the operating voltage of a device. At a
> given frequency, the hardware monitors dynamic factors and either
> makes a suggestion for how much to adjust a voltage for the
> current frequency, or it automatically adjusts the voltage
> without software intervention. Add an API to the OPP library for
> the former case, so that AVS type devices can update the voltages
> for an OPP when the hardware determines the voltage should
> change. The assumption is that drivers like CPUfreq or devfreq
> will register for the OPP notifiers and adjust the voltage
> according to suggestions that AVS makes.
> 
> This patch is devired from [1] submitted by Stephen.
> [1] https://lore.kernel.org/patchwork/patch/599279/
> 
> Signed-off-by: Stephen Boyd <sboyd@codeaurora.org>
> Signed-off-by: Roger Lu <roger.lu@mediatek.com>
> ---
>  drivers/opp/core.c     | 78 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/pm_opp.h | 11 +++++++
>  2 files changed, 89 insertions(+)

This is an rcu implementation which got removed long back from OPP core. Please
align this with the latest changes.

-- 
viresh

Re: [PATCH 8/8] devfreq: mediatek: cci devfreq register opp notification for SVS support

[Hsin-Yi Wang]

On Thu, May 16, 2019 at 9:08 AM Andrew-sh.Cheng
<andrew-sh.cheng@mediatek.com> wrote:
>
> From: "Andrew-sh.Cheng" <andrew-sh.cheng@mediatek.com>
>
> SVS will change the voltage of opp item.
> CCI devfreq need to react to change frequency.
>
> Signed-off-by: Andrew-sh.Cheng <andrew-sh.cheng@mediatek.com>
> ---
>  drivers/devfreq/mt8183-cci-devfreq.c | 63 ++++++++++++++++++++++++++++++++++++
>  1 file changed, 63 insertions(+)
>
> diff --git a/drivers/devfreq/mt8183-cci-devfreq.c b/drivers/devfreq/mt8183-cci-devfreq.c
> index 818a167c442f..250c963789f3 100644
> --- a/drivers/devfreq/mt8183-cci-devfreq.c
> +++ b/drivers/devfreq/mt8183-cci-devfreq.c
> @@ -19,7 +19,10 @@ struct cci_devfreq {
>         struct regulator *proc_reg;
>         unsigned long proc_reg_uV;
>         struct clk *cci_clk;
> +       unsigned long freq;
>         struct notifier_block nb;
> +       struct notifier_block opp_nb;
> +       int cci_min_freq;
>  };
>
>  static int cci_devfreq_regulator_notifier(struct notifier_block *nb,
> @@ -65,17 +68,62 @@ static int cci_devfreq_regulator_notifier(struct notifier_block *nb,
>         return 0;
>  }
>
> +static int ccidevfreq_opp_notifier(struct notifier_block *nb,
> +unsigned long event, void *data)
> +{
> +       int ret;
> +       struct dev_pm_opp *opp = data;
> +       struct cci_devfreq *cci_df = container_of(nb, struct cci_devfreq,
> +                                                 opp_nb);
> +       unsigned long   freq, volt, cur_volt;
> +
> +       if (event == OPP_EVENT_ADJUST_VOLTAGE) {
> +               freq = dev_pm_opp_get_freq(opp);
> +               /* current opp item is changed */
> +               if (freq == cci_df->freq) {
> +                       volt = dev_pm_opp_get_voltage(opp);
> +                       cur_volt = regulator_get_voltage(cci_df->proc_reg);
> +
> +                       if (volt > cur_volt) {
> +                               /* need reduce freq */
> +                               mutex_lock(&cci_df->devfreq->lock);
> +                               ret = update_devfreq(cci_df->devfreq);
> +                               if (ret)
> +                                       pr_err("Fail to reduce cci frequency by opp notification: %d\n",
This line is too long
> +                                              ret);
> +                               mutex_unlock(&cci_df->devfreq->lock);
> +                       }
> +               }
> +
> +               if (freq == cci_df->cci_min_freq) {
> +                       volt = dev_pm_opp_get_voltage(opp);
> +                       regulator_set_voltage(cci_df->proc_reg, volt, INT_MAX);
> +               }
> +       } else if (event == OPP_EVENT_DISABLE) {
Is this case handled? If not, is this branch needed?
> +       }
> +
> +       return 0;
> +}
> +
> +
>  static int mtk_cci_governor_get_target(struct devfreq *devfreq,
>                                        unsigned long *freq)
>  {
>         struct cci_devfreq *cci_df;
>         struct dev_pm_opp *opp;
> +       int ret;
>
>         cci_df = dev_get_drvdata(devfreq->dev.parent);
>
>         /* find available frequency */
>         opp = dev_pm_opp_find_freq_ceil_by_volt(devfreq->dev.parent,
>                                                 cci_df->proc_reg_uV);
> +       ret = PTR_ERR_OR_ZERO(opp);
> +       if (ret) {
> +               pr_err("%s[%d], cannot find opp with voltage=%d: %d\n",
> +                      __func__, __LINE__, cci_df->proc_reg_uV, ret);
> +               return ret;
> +       }
>         *freq = dev_pm_opp_get_freq(opp);
>
>         return 0;
> @@ -87,9 +135,11 @@ static int mtk_cci_governor_event_handler(struct devfreq *devfreq,
>         int ret;
>         struct cci_devfreq *cci_df;
>         struct notifier_block *nb;
> +       struct notifier_block *opp_nb;
>
>         cci_df = dev_get_drvdata(devfreq->dev.parent);
>         nb = &cci_df->nb;
> +       opp_nb = &cci_df->opp_nb;
>
>         switch (event) {
>         case DEVFREQ_GOV_START:
> @@ -100,6 +150,8 @@ static int mtk_cci_governor_event_handler(struct devfreq *devfreq,
>                 if (ret)
>                         pr_err("%s: failed to add governor: %d\n", __func__,
>                                ret);
> +               opp_nb->notifier_call = ccidevfreq_opp_notifier;
> +               dev_pm_opp_register_notifier(devfreq->dev.parent, opp_nb);
>                 break;
>
>         case DEVFREQ_GOV_STOP:
> @@ -141,6 +193,8 @@ static int mtk_cci_devfreq_target(struct device *dev, unsigned long *freq,
>                 return ret;
>         }
>
> +       cci_df->freq = *freq;
> +
>         return 0;
>  }
>
> @@ -152,6 +206,8 @@ static int mtk_cci_devfreq_probe(struct platform_device *pdev)
>  {
>         struct device *cci_dev = &pdev->dev;
>         struct cci_devfreq *cci_df;
> +       unsigned long freq, volt;
> +       struct dev_pm_opp *opp;
>         int ret;
>
>         cci_df = devm_kzalloc(cci_dev, sizeof(*cci_df), GFP_KERNEL);
> @@ -181,6 +237,13 @@ static int mtk_cci_devfreq_probe(struct platform_device *pdev)
>                 return ret;
>         }
>
> +       /* set voltage lower bound */
> +       freq = 1;
> +       opp = dev_pm_opp_find_freq_ceil(cci_dev, &freq);
> +       cci_df->cci_min_freq = dev_pm_opp_get_freq(opp);
> +       volt = dev_pm_opp_get_voltage(opp);
> +       dev_pm_opp_put(opp);
> +
>         platform_set_drvdata(pdev, cci_df);
>
>         cci_df->devfreq = devm_devfreq_add_device(cci_dev,

Re: [PATCH 7/8] cpufreq: mediatek: add opp notification for SVS support

[Hsin-Yi Wang]

On Thu, May 16, 2019 at 9:08 AM Andrew-sh.Cheng
<andrew-sh.cheng@mediatek.com> wrote:
>         bool need_voltage_tracking;
> +       struct mutex lock; /* avoid notify and policy race condition */
> +       struct notifier_block opp_nb;
> +       int opp_cpu;
> +       unsigned long opp_freq;
>  };
>
>  static LIST_HEAD(dvfs_info_list);
> @@ -239,6 +243,7 @@ static int mtk_cpufreq_set_target(struct cpufreq_policy *policy,
>         vproc = dev_pm_opp_get_voltage(opp);
>         dev_pm_opp_put(opp);
>
> +       mutex_lock(&info->lock);

Should init mutex, otherwise it'll get lockdep warning:
[    0.587055] Call trace:
[    0.587069]  dump_backtrace+0x0/0x168
[    0.587077]  show_stack+0x20/0x2c
[    0.587086]  dump_stack+0xe4/0x134
[    0.587095]  register_lock_class+0x3e8/0x4b0
[    0.587103]  __lock_acquire+0xac/0x14e8
[    0.587110]  lock_acquire+0x1d0/0x208
[    0.587118]  __mutex_lock_common+0xc0/0xb40
[    0.587126]  mutex_lock_nested+0x40/0x50
[    0.587135]  mtk_cpufreq_set_target+0xcc/0x2a8
[    0.587143]  __cpufreq_driver_target+0x438/0x4d8
[    0.587150]  cpufreq_online+0x5b4/0x6e0
[    0.587156]  cpufreq_add_dev+0x4c/0x84
[    0.587164]  subsys_interface_register+0xb8/0x10c
[    0.587171]  cpufreq_register_driver+0x11c/0x1c0
[    0.587178]  mtk_cpufreq_probe+0x378/0x4b8
[    0.587185]  platform_drv_probe+0x80/0xb0
[    0.587192]  really_probe+0x114/0x28c
[    0.587198]  driver_probe_device+0x64/0xfc
[    0.587205]  __device_attach_driver+0xb8/0xd0
[    0.587211]  bus_for_each_drv+0x88/0xd0
[    0.587218]  __device_attach+0xb0/0x134
[    0.587224]  device_initial_probe+0x20/0x2c
[    0.587230]  bus_probe_device+0x34/0x94
[    0.587238]  device_add+0x520/0x5b4
[    0.587245]  platform_device_add+0x17c/0x208
[    0.587252]  platform_device_register_full+0xc0/0x100
[    0.587261]  mtk_cpufreq_driver_init+0x8c/0xdc
[    0.587268]  do_one_initcall+0x1c0/0x3e0
[    0.587276]  do_initcall_level+0x1f4/0x224
[    0.587282]  do_basic_setup+0x34/0x4c
[    0.587288]  kernel_init_freeable+0x10c/0x194
[    0.587295]  kernel_init+0x14/0x100
[    0.587302]  ret_from_fork+0x10/0x18
[    0.587510] cpufreq: cpufreq_online: CPU4: Unlisted initial
frequency changed to: 1248000 KHz

Re: [PATCH 6/8] PM / OPP: Support adjusting OPP voltages at runtime

[Roger Lu]

Dear Stephen Boyd,

This patch is derived from [1]. Please kindly shares the suggestion to
us. Thanks very much.

[1]: https://lore.kernel.org/patchwork/patch/599279/

Dear Viresh,

I followed _opp_set_availability() coding style to refine
dev_pm_opp_adjust_voltage() from this patch. Is this refinement suitable
for OPP core? Thanks a lot.

On Mon, 2019-05-20 at 12:47 +0800, Viresh Kumar wrote:
> On 16-05-19, 17:08, Andrew-sh.Cheng wrote:
> > From: Stephen Boyd <sboyd@codeaurora.org>
> > 
> > On some SoCs the Adaptive Voltage Scaling (AVS) technique is
> > employed to optimize the operating voltage of a device. At a
> > given frequency, the hardware monitors dynamic factors and either
> > makes a suggestion for how much to adjust a voltage for the
> > current frequency, or it automatically adjusts the voltage
> > without software intervention. Add an API to the OPP library for
> > the former case, so that AVS type devices can update the voltages
> > for an OPP when the hardware determines the voltage should
> > change. The assumption is that drivers like CPUfreq or devfreq
> > will register for the OPP notifiers and adjust the voltage
> > according to suggestions that AVS makes.
> > 
> > This patch is devired from [1] submitted by Stephen.
> > [1] https://lore.kernel.org/patchwork/patch/599279/
> > 
> > Signed-off-by: Stephen Boyd <sboyd@codeaurora.org>
> > Signed-off-by: Roger Lu <roger.lu@mediatek.com>
> > ---
> >  drivers/opp/core.c     | 78 ++++++++++++++++++++++++++++++++++++++++++++++++++
> >  include/linux/pm_opp.h | 11 +++++++
> >  2 files changed, 89 insertions(+)
> 
> This is an rcu implementation which got removed long back from OPP core. Please
> align this with the latest changes.
> 


/**
 * dev_pm_opp_adjust_voltage() - helper to change the voltage of an OPP
 * @dev:		device for which we do this operation
 * @freq:		OPP frequency to adjust voltage of
 * @u_volt:		new OPP voltage
 *
 * Return: -EINVAL for bad pointers, -ENOMEM if no memory available for
the
 * copy operation, returns 0 if no modifcation was done OR modification
was
 * successful.
 */
int dev_pm_opp_adjust_voltage(struct device *dev, unsigned long freq,
			      unsigned long u_volt)
{
	struct opp_table *opp_table;
	struct dev_pm_opp *tmp_opp, *opp = ERR_PTR(-ENODEV);
	int r = 0;

	/* Find the opp_table */
	opp_table = _find_opp_table(dev);
	if (IS_ERR(opp_table)) {
		r = PTR_ERR(opp_table);
		dev_warn(dev, "%s: Device OPP not found (%d)\n", __func__, r);
		return r;
	}

	mutex_lock(&opp_table->lock);

	/* Do we have the frequency? */
	list_for_each_entry(tmp_opp, &opp_table->opp_list, node) {
		if (tmp_opp->rate == freq) {
			opp = tmp_opp;
			break;
		}
	}

	if (IS_ERR(opp)) {
		r = PTR_ERR(opp);
		goto adjust_unlock;
	}

	/* Is update really needed? */
	if (opp->supplies->u_volt == u_volt)
		goto adjust_unlock;

	opp->supplies->u_volt = u_volt;

	dev_pm_opp_get(opp);
	mutex_unlock(&opp_table->lock);

	/* Notify the voltage change of the OPP */
	blocking_notifier_call_chain(&opp_table->head,
OPP_EVENT_ADJUST_VOLTAGE,
				     opp);

	dev_pm_opp_put(opp);
	goto adjust_put_table;

adjust_unlock:
	mutex_unlock(&opp_table->lock);
adjust_put_table:
	dev_pm_opp_put_opp_table(opp_table);
	return r;
}

Sincerely,
Roger Lu.

Re: [PATCH 6/8] PM / OPP: Support adjusting OPP voltages at runtime

[Viresh Kumar]

On 29-07-19, 11:39, Roger Lu wrote:
> Dear Stephen Boyd,
> 
> This patch is derived from [1]. Please kindly shares the suggestion to
> us. Thanks very much.
> 
> [1]: https://lore.kernel.org/patchwork/patch/599279/
> 
> Dear Viresh,
> 
> I followed _opp_set_availability() coding style to refine
> dev_pm_opp_adjust_voltage() from this patch. Is this refinement suitable
> for OPP core? Thanks a lot.

Looks okay from a quick look.

-- 
viresh