RE: [PATCH] devfreq: Make log message more explicit when devfreq device already exists

[MyungJoo Ham]

>Before creating a new devfreq device devfreq_add_device() checks
>if there is already a devfreq dev associated with the requesting
>device (parent). If that's the case the function rejects to create
>another devfreq dev for that parent and logs an error. The error
>message is very unspecific, make it a bit more explicit.
>
>Signed-off-by: Matthias Kaehlcke 
>---
> drivers/devfreq/devfreq.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
>diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
>index ab22bf8a12d6..0e2dd734ab58 100644
>--- a/drivers/devfreq/devfreq.c
>+++ b/drivers/devfreq/devfreq.c
>@@ -625,7 +625,7 @@ struct devfreq *devfreq_add_device(struct device *dev,
>   devfreq = find_device_devfreq(dev);
>   mutex_unlock(_list_lock);
>   if (!IS_ERR(devfreq)) {
>-  dev_err(dev, "%s: Unable to create devfreq for the device.\n",
>+  dev_err(dev, "%s: devfreq device already exists!\n",

Yes, this is more helpful! Thanks!

Acked-by: MyungJoo Ham 

Re: linux-next: build warning after merge of the devfreq tree

[MyungJoo Ham]

Thank you for pointing this out!

I've added a fix to the tree:
https://git.kernel.org/pub/scm/linux/kernel/git/mzx/devfreq.git/log/?h=for-next
(and shared the patch in a previous reply)

Rafael, could you please pull the fix from the git repo above?


Cheers,
MyungJoo

On Mon, Aug 26, 2019 at 8:51 PM Stephen Rothwell  wrote:
>
> Hi all,
>
> After merging the devfreq tree, today's linux-next build (x86_64
> allmodconfig) produced this warning:
>
> drivers/devfreq/governor_passive.c: In function 
> 'devfreq_passive_event_handler':
> drivers/devfreq/governor_passive.c:152:17: warning: unused variable 'dev' 
> [-Wunused-variable]
>   struct device *dev = devfreq->dev.parent;
>  ^~~
>
> Introduced by commit
>
>   0ef7c7cce43f ("PM / devfreq: passive: Use non-devm notifiers")
>
> --
> Cheers,
> Stephen Rothwell



-- 
MyungJoo Ham, Ph.D.
S/W Center, Samsung Electronics

[PATCH] PM / devfreq: passive: fix compiler warning

[MyungJoo Ham]

The recent commit of
PM / devfreq: passive: Use non-devm notifiers
had incurred compiler warning, "unused variable 'dev'".

Reported-by: Stephen Rothwell 
Signed-off-by: MyungJoo Ham 
---
 drivers/devfreq/governor_passive.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/devfreq/governor_passive.c
b/drivers/devfreq/governor_passive.c
index da48547..be6eeab 100644
--- a/drivers/devfreq/governor_passive.c
+++ b/drivers/devfreq/governor_passive.c
@@ -149,7 +149,6 @@ static int devfreq_passive_notifier_call(struct
notifier_block *nb,
 static int devfreq_passive_event_handler(struct devfreq *devfreq,
 unsigned int event, void *data)
 {
-struct device *dev = devfreq->dev.parent;
 struct devfreq_passive_data *p_data
 = (struct devfreq_passive_data *)devfreq->data;
 struct devfreq *parent = (struct devfreq *)p_data->parent;
-- 
2.7.4

RE: Re: [PATCH] devfreq: tegra20: add COMMON_CLK dependency

[MyungJoo Ham]

On 19. 6. 28. 오후 7:32, Arnd Bergmann wrote:
> Compile-testing the new driver on platforms without CONFIG_COMMON_CLK
> leads to a link error:
> 
> drivers/devfreq/tegra20-devfreq.o: In function `tegra_devfreq_target':
> tegra20-devfreq.c:(.text+0x288): undefined reference to `clk_set_min_rate'
> 
> Add a dependency on COMMON_CLK to avoid this.
> 
> Fixes: 1d39ee8dad6d ("PM / devfreq: Introduce driver for NVIDIA Tegra20")
> Signed-off-by: Arnd Bergmann 
> ---
>  drivers/devfreq/Kconfig | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/devfreq/Kconfig b/drivers/devfreq/Kconfig
> index f3b242987fd9..defe1d438710 100644
> --- a/drivers/devfreq/Kconfig
> +++ b/drivers/devfreq/Kconfig
> @@ -107,6 +107,7 @@ config ARM_TEGRA_DEVFREQ
>  config ARM_TEGRA20_DEVFREQ
>   tristate "NVIDIA Tegra20 DEVFREQ Driver"
>   depends on (TEGRA_MC && TEGRA20_EMC) || COMPILE_TEST
> + depends on COMMON_CLK
>   select DEVFREQ_GOV_SIMPLE_ONDEMAND
>   select PM_OPP
>   help
> 

Acked-by: MyungJoo Ham 

Thanks!


Cheers,
MyungJoo.

RE: [PATCH v3 02/22] PM / devfreq: tegra30: Keep interrupt disabled while governor is stopped

[MyungJoo Ham]

>There is no real need to keep interrupt always-enabled, will be nicer
>to keep it disabled while governor is inactive.
>
>Suggested-by: Thierry Reding 
>Signed-off-by: Dmitry Osipenko 
>---
> drivers/devfreq/tegra30-devfreq.c | 43 ---
> 1 file changed, 22 insertions(+), 21 deletions(-)
>
>diff --git a/drivers/devfreq/tegra30-devfreq.c 
>b/drivers/devfreq/tegra30-devfreq.c
>index a27300f40b0b..5e2b133babdd 100644
>--- a/drivers/devfreq/tegra30-devfreq.c
>+++ b/drivers/devfreq/tegra30-devfreq.c
[]
>@@ -416,8 +417,6 @@ static void tegra_actmon_start(struct tegra_devfreq *tegra)
> {
>   unsigned int i;
> 
>-  disable_irq(tegra->irq);
>-
>   actmon_writel(tegra, ACTMON_SAMPLING_PERIOD - 1,
> ACTMON_GLB_PERIOD_CTRL);
> 

I think this has nothing to do with
"keep it disabled while governor is inactive."

And this looks dangerous because it disables the safety measure
of disabling interrupt while you touch some looking-critical registers.
Anyway, as I do not know the internals of Tegra SoC, I cannot sure.

Cheers,
MyungJoo

RE: [PATCH v2 4/4] PM / devfreq: Add required OPPs support to passive governor

[MyungJoo Ham]

>Look at the required OPPs of the "parent" device to determine the OPP that
>is required from the slave device managed by the passive governor. This
>allows having mappings between a parent device and a slave device even when
>they don't have the same number of OPPs.
>
>Signed-off-by: Saravana Kannan 
>---
> drivers/devfreq/governor_passive.c | 20 +++-
> 1 file changed, 15 insertions(+), 5 deletions(-)

Acked-by: MyungJoo Ham 

Cheers,
MyungJoo

RE: [PATCH v2 3/4] PM / devfreq: Cache OPP table reference in devfreq

[MyungJoo Ham]

>The OPP table can be used often in devfreq. Trying to get it each time can
>be expensive, so cache it in the devfreq struct.
>
>Signed-off-by: Saravana Kannan 
>---
> drivers/devfreq/devfreq.c | 6 ++
> include/linux/devfreq.h   | 1 +
> 2 files changed, 7 insertions(+)

Acked-by: MyungJoo Ham 


Cheers,
MyungJoo

RE: Re: [PATCH v4 13/16] PM / devfreq: tegra: Support Tegra30

[MyungJoo Ham]

Sender : Dmitry Osipenko 
>24.06.2019 14:11, MyungJoo Ham пишет:
>>>
>>> - Original Message -
>>> Sender : Dmitry Osipenko 
>>>
>>> 24.06.2019 10:34, MyungJoo Ham пишет:
>>>>>
>>>>> A question:
>>>>>
>>>>> Does this driver support Tegra20 as well?
>>>>> I'm asking this because ARCH_TEGRA includes ARCH_TEGRA_2x_SOC
>>>>> according to /drivers/soc/tegra/Kconfig.
>>>>>
>>>>
>>>> For this matter, how about updating your 13/16 patch as follows?
>>>>
>> []
>>>
>>> Good call! I'll update this patch following yours suggestion, thanks.
>> 
>> Or, you may approve the modified commits here:
>> https://git.kernel.org/pub/scm/linux/kernel/git/mzx/devfreq.git/log/?h=for-next
> 
>Looks almost good to me!
> 
>I just recalled that there is also a 64bit variant of Tegra124, the Tegra132. 
>Hence
>the Tegra30+ Kconfig entry should look like this (it's also worthy to break 
>the lines
>for readability):
> 
>diff --git a/drivers/devfreq/Kconfig b/drivers/devfreq/Kconfig
>index ccb1a68c4b51..bd2efbc27725 100644
>--- a/drivers/devfreq/Kconfig
>+++ b/drivers/devfreq/Kconfig
>@@ -94,7 +94,10 @@ config ARM_EXYNOS_BUS_DEVFREQ
> 
> config ARM_TEGRA_DEVFREQ
>tristate "NVIDIA Tegra30/114/124/210 DEVFREQ Driver"
>-   depends on ARCH_TEGRA || COMPILE_TEST
>+   depends on ARCH_TEGRA_3x_SOC  || ARCH_TEGRA_114_SOC || \
>+  ARCH_TEGRA_132_SOC || ARCH_TEGRA_124_SOC || \
>+  ARCH_TEGRA_210_SOC || \
>+  COMPILE_TEST
>select PM_OPP
>help
>  This adds the DEVFREQ driver for the Tegra family of SoCs.
> 
>Could you please adjust the patches like I'm suggesting? I'll approve yours 
>change
>then and won't re-spin the first batch of the patches.

I've adjusted as you suggested. It's pushed to the git repo as well.

Cheers,
MyungJoo.

RE: Re: [PATCH v4 13/16] PM / devfreq: tegra: Support Tegra30

[MyungJoo Ham]

> 
>- Original Message -
>Sender : Dmitry Osipenko 
> 
>24.06.2019 10:34, MyungJoo Ham пишет:
>>>
>>> A question:
>>>
>>> Does this driver support Tegra20 as well?
>>> I'm asking this because ARCH_TEGRA includes ARCH_TEGRA_2x_SOC
>>> according to /drivers/soc/tegra/Kconfig.
>>>
>> 
>> For this matter, how about updating your 13/16 patch as follows?
>> 
[]
> 
>Good call! I'll update this patch following yours suggestion, thanks.

Or, you may approve the modified commits here:
https://git.kernel.org/pub/scm/linux/kernel/git/mzx/devfreq.git/log/?h=for-next


Cheers,
MyungJoo

Re: [PATCH v4 13/16] PM / devfreq: tegra: Support Tegra30

[MyungJoo Ham]

> 
>A question:
> 
>Does this driver support Tegra20 as well?
>I'm asking this because ARCH_TEGRA includes ARCH_TEGRA_2x_SOC
>according to /drivers/soc/tegra/Kconfig.
> 

For this matter, how about updating your 13/16 patch as follows?

---
 drivers/devfreq/Kconfig | 4 ++--
 drivers/devfreq/tegra-devfreq.c | 1 +
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/devfreq/Kconfig b/drivers/devfreq/Kconfig
index 7dd46d44579d..78c4b436aad8 100644
--- a/drivers/devfreq/Kconfig
+++ b/drivers/devfreq/Kconfig
@@ -93,8 +93,8 @@ config ARM_EXYNOS_BUS_DEVFREQ
  This does not yet operate with optimal voltages.
 
 config ARM_TEGRA_DEVFREQ
-   tristate "Tegra DEVFREQ Driver"
-   depends on ARCH_TEGRA_124_SOC
+   tristate "NVIDIA Tegra30/114/124/210 DEVFREQ Driver"
+   depends on ARCH_TEGRA_3x_SOC || ARCH_TEGRA_114_SOC || 
ARCH_TEGRA_124_SOC || ARCH_TEGRA_210_SOC
select PM_OPP
help
  This adds the DEVFREQ driver for the Tegra family of SoCs.
diff --git a/drivers/devfreq/tegra-devfreq.c b/drivers/devfreq/tegra-devfreq.c
index 5cddf2199c4e..a6ba75f4106d 100644
--- a/drivers/devfreq/tegra-devfreq.c
+++ b/drivers/devfreq/tegra-devfreq.c
@@ -726,6 +726,7 @@ static int tegra_devfreq_remove(struct platform_device 
*pdev)
 }
 
 static const struct of_device_id tegra_devfreq_of_match[] = {
+   { .compatible = "nvidia,tegra30-actmon" },
{ .compatible = "nvidia,tegra124-actmon" },
{ },
 };
-- 
2.17.1

Re: [PATCH v4 13/16] PM / devfreq: tegra: Support Tegra30

[MyungJoo Ham]

> On Thu, May 02, 2019 at 02:38:12AM +0300, Dmitry Osipenko wrote:
> > The devfreq driver can be used on Tegra30 without any code change and
> > it works perfectly fine, the default Tegra124 parameters are good enough
> > for Tegra30.
> > 
> > Reviewed-by: Chanwoo Choi 
> > Signed-off-by: Dmitry Osipenko 
> > ---
> >  drivers/devfreq/Kconfig | 4 ++--
> >  drivers/devfreq/tegra-devfreq.c | 1 +
> >  2 files changed, 3 insertions(+), 2 deletions(-)
> 
> Acked-by: Thierry Reding 
> 
> diff --git a/drivers/devfreq/Kconfig b/drivers/devfreq/Kconfig
> index 7dd46d4..b291803 100644
> --- a/drivers/devfreq/Kconfig
> +++ b/drivers/devfreq/Kconfig
> @@ -93,8 +93,8 @@ config ARM_EXYNOS_BUS_DEVFREQ
> This does not yet operate with optimal voltages.
>  
>  config ARM_TEGRA_DEVFREQ
> - tristate "Tegra DEVFREQ Driver"
> - depends on ARCH_TEGRA_124_SOC
> + tristate "NVIDIA Tegra30/114/124/210 DEVFREQ Driver"
> + depends on ARCH_TEGRA
>   select PM_OPP
>   help
> This adds the DEVFREQ driver for the Tegra family of SoCs.

A question:

Does this driver support Tegra20 as well?
I'm asking this because ARCH_TEGRA includes ARCH_TEGRA_2x_SOC
according to /drivers/soc/tegra/Kconfig.

Cheers,
MyungJoo.

RE: [PATCH v1 01/11] PM / devfreq: tegra30: Change irq type to unsigned int

[MyungJoo Ham]

>IRQ numbers are always positive, hence the corresponding variable should
>be unsigned to keep types consistent. This is a minor change that cleans
>up code a tad more.
>
>Suggested-by: Thierry Reding 
>Signed-off-by: Dmitry Osipenko 

Acked-by: MyungJoo Ham 


Cheers,
MyungJoo

RE: [PATCH v2 4/4] devfreq: add mediatek cci devfreq

[MyungJoo Ham]

>This adds a devfreq driver for the Cache Coherent Interconnect (CCI)
>of the Mediatek MT8183.
>
>On the MT8183 the CCI is supplied by the same regulator as the LITTLE
>cores. The driver is notified when the regulator voltage changes
>(driven by cpufreq) and adjusts the CCI frequency to the maximum
>possible value.
>
>Signed-off-by: Andrew-sh.Cheng 
>---
> drivers/devfreq/Kconfig  |  10 ++
> drivers/devfreq/Makefile |   1 +
> drivers/devfreq/mt8183-cci-devfreq.c | 235 +++
> 3 files changed, 246 insertions(+)
> create mode 100644 drivers/devfreq/mt8183-cci-devfreq.c
>

1. It appears that proc_reg_uV might be used before initialization.
It would be appropriate to initialize it at the probe function.

2. Because you are already using OPP, why don't you rely on
OPP fully? (use OPP from CPUFreq drivers as well in order to get
OPP events automatically.) Anyway, this is a minor item that does
not need to be corrected.

Cheers
MyungJoo

RE: [PATCH v2 2/4] opp: add API which get max freq by voltage

[MyungJoo Ham]

>This API will get voltage as input parameter.
>Search all opp items for the item which with max frequency,
>and the voltae is smaller than provided voltage.
>
>Signed-off-by: Andrew-sh.Cheng 
>---
> drivers/opp/core.c | 55 ++
> include/linux/pm_opp.h |  8 
> 2 files changed, 63 insertions(+)
>
>diff --git a/include/linux/pm_opp.h b/include/linux/pm_opp.h
>index 24c757a..57deef9 100644
>--- a/include/linux/pm_opp.h
>+++ b/include/linux/pm_opp.h
>@@ -102,6 +102,8 @@ struct dev_pm_opp *dev_pm_opp_find_freq_exact(struct 
>device *dev,
> 
> struct dev_pm_opp *dev_pm_opp_find_freq_floor(struct device *dev,
> unsigned long *freq);
>+struct dev_pm_opp *dev_pm_opp_find_max_freq_by_volt(struct device *dev,
>+unsigned long u_volt);

For the symmetricity, wouldn't it be better to name it
dev_pm_opp_find_volt_ceiling(dev, u_volt); ?

Cheers,
MyungJoo

RE: [PATCH v3 3/5] devfreq: rk3399_dmc: Pass ODT and auto power down parameters to TF-A.

[MyungJoo Ham]

>From: Enric Balletbo i Serra 
>
>Trusted Firmware-A (TF-A) for rk3399 implements a SiP call to get the
>on-die termination (ODT) and auto power down parameters from kernel,
>this patch adds the functionality to do this. Also, if DDR clock
>frequency is lower than the on-die termination (ODT) disable frequency
>this driver should disable the DDR ODT.
>
>Signed-off-by: Enric Balletbo i Serra 
>Reviewed-by: Chanwoo Choi 
>Signed-off-by: Gael PORTAY 

Acked-by: MyungJoo Ham 


Cheers!
MyungJoo

>---
>
>Changes in v3:
>- [PATCH v2 3/5] Add Signed-off-by: Gael PORTAY .
>Remove comments.
>Move pmu dt parsing after dt-parsing of timings to fix
> data->odt_dis_freq value.
>
>Changes in v2: None
>
>Changes in v1:
>- [RFC 3/10] Add an explanation for platform SIP calls.
>- [RFC 3/10] Change if statement for a switch.
>- [RFC 3/10] Rename ddr_flag to odt_enable to be more clear.
>
> drivers/devfreq/rk3399_dmc.c| 71 -
> include/soc/rockchip/rockchip_sip.h |  1 +
> 2 files changed, 71 insertions(+), 1 deletion(-)

RE: [PATCH v3 2/5] dt-bindings: devfreq: rk3399_dmc: Add rockchip,pmu phandle.

[MyungJoo Ham]

>From: Enric Balletbo i Serra 
>
>The Rockchip DMC (Dynamic Memory Interface) needs to access to the PMU
>general register files to know the DRAM type, so add a phandle to the
>syscon that manages these registers.
>
>Signed-off-by: Enric Balletbo i Serra 
>Reviewed-by: Chanwoo Choi 
>Acked-by: Rob Herring 
>Signed-off-by: Gael PORTAY 
>---
>
>Changes in v3:
>- [PATCH v2 2/5] Add Signed-off-by: Gael PORTAY .
>
>Changes in v2: None
>
>Changes in v1:
>- [RFC 2/10] Add reviewed and acked tags from Chanwoo Choi and Rob Herring
>
> Documentation/devicetree/bindings/devfreq/rk3399_dmc.txt | 2 ++
> 1 file changed, 2 insertions(+)

Acked-by: MyungJoo Ham 

RE: [PATCH v3 1/5] devfreq: rockchip-dfi: Move GRF definitions to a common place.

[MyungJoo Ham]

>From: Enric Balletbo i Serra 
>
>Some rk3399 GRF (Generic Register Files) definitions can be used for
>different drivers. Move these definitions to a common include so we
>don't need to duplicate these definitions.
>
>Signed-off-by: Enric Balletbo i Serra 
>Acked-by: Chanwoo Choi 
>Signed-off-by: Gael PORTAY 
>---
>
>Changes in v3:
>- [PATCH v2 1/5] Add Signed-off-by: Gael PORTAY .
>
>Changes in v2:
>- [PATCH 1/8] Really add Acked-by: Chanwoo Choi .
>
>Changes in v1:
>- [RFC 1/10] Add Acked-by: Chanwoo Choi 
>- [RFC 1/10] s/Generic/General/ (Robin Murphy)
>- [RFC 4/10] Removed from the series. I did not found a use case where not 
>holding the mutex causes the issue.
>- [RFC 7/10] Removed from the series. I did not found a use case where this 
>matters.
>
> drivers/devfreq/event/rockchip-dfi.c | 23 +++
> include/soc/rockchip/rk3399_grf.h| 21 +
> 2 files changed, 28 insertions(+), 16 deletions(-)
> create mode 100644 include/soc/rockchip/rk3399_grf.h
>

Acked-by: MyungJoo Ham 

RE: Re: [PATCH] PM / devfreq: exynos-bus: Suspend all devices on system shutdown

[MyungJoo Ham]

>Hi Chanwoo,
>
>On 2019-03-21 10:19, Chanwoo Choi wrote:
>> On 19. 3. 21. 오후 6:01, Marek Szyprowski wrote:
>>> Force all Exynos buses to safe operation points before doing the system
>>> reboot operation. There are board on which some aggressive power saving
>>> operation points are behind the capabilities of the bootloader to properly
>>> reset the hardware and boot the board. This way one can avoid board crash
>>> early after reboot.
>>>
>>> This fixes reboot issue on OdroidU3 board both with eMMC and SD boot.
>>>
>>> Reported-by: Markus Reichl 
>>> Signed-off-by: Marek Szyprowski 
>>> ---
>>> This is an alternative to https://patchwork.kernel.org/patch/10781433/
>>> limited only to Exynos-bus driver.
>>> ---
>>>   drivers/devfreq/exynos-bus.c | 8 
>>>   1 file changed, 8 insertions(+)
>>>
>>> diff --git a/drivers/devfreq/exynos-bus.c b/drivers/devfreq/exynos-bus.c
>>> index c25658b26598..486cc5b422f1 100644
>>> --- a/drivers/devfreq/exynos-bus.c
>>> +++ b/drivers/devfreq/exynos-bus.c
>>> @@ -514,6 +514,13 @@ static int exynos_bus_probe(struct platform_device 
>>> *pdev)
>>> return ret;
>>>   }
>>>   
>>> +static void exynos_bus_shutdown(struct platform_device *pdev)
>>> +{
>>> +   struct exynos_bus *bus = dev_get_drvdata(>dev);
>>> +
>>> +   devfreq_suspend_device(bus->devfreq);
>>> +}
>>> +
>>>   #ifdef CONFIG_PM_SLEEP
>>>   static int exynos_bus_resume(struct device *dev)
>>>   {
>>> @@ -556,6 +563,7 @@ MODULE_DEVICE_TABLE(of, exynos_bus_of_match);
>>>   
>>>   static struct platform_driver exynos_bus_platdrv = {
>>> .probe  = exynos_bus_probe,
>>> +   .shutdown       = exynos_bus_shutdown,
>>> .driver = {
>>> .name   = "exynos-bus",
>>> .pm = _bus_pm,
>>>
>> Actually, I already agreed the previous patch.
>> Also, it looks good to me.
>
>Yes, I know, but MyungJoo had some objections, that's why I prepared 
>alternative version.
>
>> Acked-by: Chanwoo Choi 
>>

This has become looking good now :)


Acked-by: MyungJoo Ham 


Cheers,
MyungJoo

RE: [RESEND PATCH] PM / devfreq: Fix static checker warning in try_then_request_governor

[MyungJoo Ham]

>The patch 23c7b54ca1cd: "PM / devfreq: Fix devfreq_add_device() when
>drivers are built as modules." leads to the following static checker
>warning:
>
>drivers/devfreq/devfreq.c:1043 governor_store()
>warn: 'governor' can also be NULL
>
>The reason is that the try_then_request_governor() function returns both
>error pointers and NULL. It should just return error pointers, so fix
>this by returning a ERR_PTR to the error intead of returning NULL.
>
>Fixes: 23c7b54ca1cd ("PM / devfreq: Fix devfreq_add_device() when drivers are 
>built as modules.")
>Reported-by: Dan Carpenter 
>Signed-off-by: Enric Balletbo i Serra 
>Reviewed-by: Chanwoo Choi 
>---
>Hi,
>
>This is a resend of [1] as seems that got lost at some point and I just
>noticed that was never merged.
>
>Thanks,
> Enric

Acked-by: MyungJoo Ham 

Thanks!


CHeers,
MyungJoo

RE: [PATCH v5] PM / devfreq: Restart previous governor if new governor fails to start

[MyungJoo Ham]

>From: Saravana Kannan 
>
>If the new governor fails to start, switch back to old governor so that the
>devfreq state is not left in some weird limbo.
>
>[Mjungjoo: assume fatal on revert failure and set df->governor to NULL]
>Signed-off-by: Sibi Sankar 
>Signed-off-by: Saravana Kannan 
>Reviewed-by: Chanwoo Choi 

I'll modify WARN->ERROR for the case when it's fatal:

>+  if (ret) {
>+  dev_warn(dev,
>+   "%s: reverting to Governor %s failed (%d)\n",
>+   __func__, df->governor_name, ret);
>+      df->governor = NULL;
>+  }

Acked-by: MyungJoo Ham 


>---
>V5:
>* assume fatal on revert failure and set df->governor to NULL
>
>V4:
>* Removed prev_governor check.
>
>V3:
>* Fix NULL deref for real this time.
>* Addressed some style preferences.
>
>V2:
>* Fixed typo in commit text
>* Fixed potential NULL deref
>
> drivers/devfreq/devfreq.c | 16 ++--
> 1 file changed, 14 insertions(+), 2 deletions(-)
>

RE: Re: [PATCH v4] PM / devfreq: Restart previous governor if new governor fails to start

[MyungJoo Ham]

>Hey MyungJoo, Kyungmin
>Did you get a chance to think about how you
>want this fix implemented?
>
>On 2019-02-19 10:42, Sibi Sankar wrote:
>> Hey MyungJoo,
>> 
>> On 12/14/18 7:15 AM, MyungJoo Ham wrote:
>>>> From: Saravana Kannan 
>>>> 
>>>> If the new governor fails to start, switch back to old governor so 
>>>> that the
>>>> devfreq state is not left in some weird limbo.
>>>> 
>>>> Signed-off-by: Sibi Sankar 
>>>> Signed-off-by: Saravana Kannan 
>>>> Reviewed-by: Chanwoo Choi 
>>> 
>>> Hello,
>>> 
>>> In overall, the idea and the implementation looks good.
>>> 
>>> However, I have a question:
>>> 
>>> What if the following line fails?
>>> 
>>> +   df->governor->event_handler(df, DEVFREQ_GOV_START,
>>> +   NULL);
>>> 
>>> Don't we still need something to handle for such events?
>> 
>> The original discussion went as follows:
>> governor_store is expected to be used only on cases
>> where devfreq_add_device() succeeded i.e prev->governor
>> is expected to be present and DEVFREQ_GOV_START is
>> expected to succeed. Hence falling back to the previous
>> governor seems like a sensible idea.
>> 
>> This would also prevent DEVFREQ_GOV_STOP from being called
>> on a governor were DEVFREQ_GOV_START had failed which is
>> ideal.
>> 
>> That being said DEVFREQ_GOV_START can still fail for the
>> prev-governor due to some change in state of the system.
>> Do you want to handle this case by clearing the state of
>> governor rather than switching to previous governor?
>> 

If moving back to previous governor fails after
failing for "next" governor, we may assume it's fatal
and stop the device; we can simply return errors.

In such a case, df->governor may need to be NULL as well.


Cheers,
MyungJoo

RE: Re: [PATCH v3 5/7] drivers: devfreq: add longer polling interval in idle

[MyungJoo Ham]

>> 
>> There are some requirements that you need to consider:
>> 
>> Is 30% really applicable to ALL devfreq devices?
>The 30% load while the device is on lowest OPP is to filter some noise.
>It might be tunable over sysfs for each device if you like.
>>  - What if some devices do not want such behaviors?
>They can set polling_idle_ms and polling_ms the same value.
>>  - What if some devices want different values (change behavors)?
>Need of sysfs tunable here.
>>  - What if some manufactures want different default values?
>Like above (sysfs).
>>  - What if some devices want to let the framework know that it's in idle?
>There might be a filed in devfreq->state which could handle this.
>>  - What if some other kernel context, device (drivers),
>>  or userspace process want to notify that it's no more idling?This issue 
>> is more related to the new movement in the 'interconnect'
>development. They have a goal for this kind of interactions and QoS
>between devices or their clients. In devfreq it would be possible
>to tackle this, but would require a lot of changes (notification chain,
>state machines in devices,
>
>> 
>> As mentioned in the internal thread (tizen.org),
>> I'm not convinced by the idea of assuming that a device can be considered 
>> "idling"
>> if it has simply "low" utilization.
>> 
>> You are going to deteriorate the UI response time of mobile devices 
>> significantly.
>Current devfreq wake-up also does not guarantee that, maybe on a single
>CPU platform does.

Yes, the current devfreq does not enhance UI response time in the sense
that it would keep the same reponse time anyway.

However, your current approach will surely deteriorate it by lengthen
the polling latency.

For mobile and wearable devices, in many cases I've been witnessing,
the device idles right before the user's input (launching an app,
scrolling messages or web pages, or press a play button).
For mitigation, we often relay UI inputs to DVFS mechanisms so that
we either increase frequency for any UI inputs for a short period or
shorten the polling latency for a short period.
When a highly user-interactive device is idling or operating a low frequency,
we should assume that it's going to be highly performing anytime;
loosening the checking period is not a good solution in that sense
although it is probable for servers or workstations.
But, I don't think servers/workstations do care power consumption of
DVFS checking loops anyway.



>
>I will try to address your and Chanwoo's comments that the devfreq still
>needs deferred polling in some platforms.
>Would it be OK if we have two options: deferred and delayed work while
>registering a wakeup for a device?
>That would be a function like: polling_mode_init(devfreq) instead of
>simple INIT_DEFERRED_WORK(), which will check the device's preference.
>The device driver could set a filed in 'polling_mode' to enum:
>POWER_EFFICIENT or RELIABLE_INTERVAL. For compatibility with old drivers
>where the polling_mode = 0, SYSTEM_DEFAULT_POLLING_MODE (which is one
>of these two) would be used.
>Then the two-phase-polling-interval from this patch could only be used
>for the RELIABLE_INTERVAL configuration or even dropped.

One thing I want to add is: let's not over-complicate it.
Do you have any experimental results on how much power is saved by doing this?
(and user response time losses)


Cheers,
MyungJoo

RE: [PATCH v3 2/2] drivers: devfreq: add tracing for scheduling work

[MyungJoo Ham]

>This patch add basic tracing of the devfreq workqueue and delayed work.
>It aims to capture changes of the polling intervals and device state.
>
>Signed-off-by: Lukasz Luba 
>---
> drivers/devfreq/devfreq.c | 5 +
> 1 file changed, 5 insertions(+)
>
>diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
>index 0ae3de7..660365a 100644
>--- a/drivers/devfreq/devfreq.c
>+++ b/drivers/devfreq/devfreq.c

Acked-by: MyungJoo Ham 

RE: [PATCH v3 1/2] trace: events: add devfreq trace event file

[MyungJoo Ham]

>The patch adds a new file for with trace events for devfreq
>framework. They are used for performance analysis of the framework.
>It also contains updates in MAINTAINERS file adding new entry for
>devfreq maintainers.
>
>Signed-off-by: Lukasz Luba 
>---
> MAINTAINERS|  1 +
> include/trace/events/devfreq.h | 40 
> 2 files changed, 41 insertions(+)
> create mode 100644 include/trace/events/devfreq.h

Acked-by: MyungJoo Ham 

Thanks!


Cheers,
MyungJoo

Re: [PATCH 3/3] PM / devfreq: tegra: remove unneeded variable

[MyungJoo Ham]

On Mon, Feb 18, 2019 at 6:58 PM Jon Hunter  wrote:
>
>
> On 18/02/2019 00:38, Chanwoo Choi wrote:
> > On 19. 2. 17. 오전 12:18, Yangtao Li wrote:
> >> This variable is not used after initialization, so
> >> remove it. And in order to unify the code style,
> >> move the location where the dev_get_drvdata is called
> >> by the way.
> >>
> >> Signed-off-by: Yangtao Li 
> >> ---
> >>  drivers/devfreq/tegra-devfreq.c | 7 ++-
> >>  1 file changed, 2 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/drivers/devfreq/tegra-devfreq.c 
> >> b/drivers/devfreq/tegra-devfreq.c
> >> index c59d2eee5d30..c89ba7b834ff 100644
> >> --- a/drivers/devfreq/tegra-devfreq.c
> >> +++ b/drivers/devfreq/tegra-devfreq.c
> >> @@ -573,10 +573,7 @@ static int tegra_governor_get_target(struct devfreq 
> >> *devfreq,
> >>  static int tegra_governor_event_handler(struct devfreq *devfreq,
> >>  unsigned int event, void *data)
> >>  {
> >> -struct tegra_devfreq *tegra;
> >> -int ret = 0;
> >> -
> >> -tegra = dev_get_drvdata(devfreq->dev.parent);
> >> +struct tegra_devfreq *tegra = dev_get_drvdata(devfreq->dev.parent);
> >>
> >>  switch (event) {
> >>  case DEVFREQ_GOV_START:
> >> @@ -600,7 +597,7 @@ static int tegra_governor_event_handler(struct devfreq 
> >> *devfreq,
> >>  break;
> >>  }
> >>
> >> -return ret;
> >> +return 0;
> >>  }
> >>
> >>  static struct devfreq_governor tegra_devfreq_governor = {
> >>
> >
> > Reviewed-by: Chanwoo Choi 
> >
>
> Acked-by: Jon Hunter 
>
> Thanks!
> Jon

Acked-by: MyungJoo Ham 

Merged. Thanks!

>
> --
> nvpublic



-- 
MyungJoo Ham, Ph.D.
S/W Center, Samsung Electronics

Re: [PATCH 1/3] PM / devfreq: rk3399_dmc: remove unneeded semicolon

[MyungJoo Ham]

On Mon, Feb 18, 2019 at 10:09 AM Chanwoo Choi  wrote:
>
> Hi,
>
> On 19. 2. 17. 오전 12:18, Yangtao Li wrote:
> > The semicolon is unneeded, so remove it.
> >
> > Signed-off-by: Yangtao Li 
> > ---
> >  drivers/devfreq/rk3399_dmc.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/devfreq/rk3399_dmc.c b/drivers/devfreq/rk3399_dmc.c
> > index e795ad2b3f6b..a228dad2bee4 100644
> > --- a/drivers/devfreq/rk3399_dmc.c
> > +++ b/drivers/devfreq/rk3399_dmc.c
> > @@ -322,7 +322,7 @@ static int rk3399_dmcfreq_probe(struct platform_device 
> > *pdev)
> >
> >   dev_err(dev, "Cannot get the clk dmc_clk\n");
> >   return PTR_ERR(data->dmc_clk);
> > - };
> > + }
> >
> >   data->edev = devfreq_event_get_edev_by_phandle(dev, 0);
> >   if (IS_ERR(data->edev))
> >
>
> Reviewed-by: Chanwoo Choi 

Acked-by: MyungJoo Ham 

Merged.

>
> --
> Best Regards,
> Chanwoo Choi
> Samsung Electronics


-- 
MyungJoo Ham, Ph.D.
S/W Center, Samsung Electronics

Re: [PATCH 2/3] PM / devfreq: rockchip-dfi: remove unneeded semicolon

[MyungJoo Ham]

On Mon, Feb 18, 2019 at 9:41 AM Chanwoo Choi  wrote:
>
> Hi,
>
> On 19. 2. 17. 오전 12:18, Yangtao Li wrote:
> > The semicolon is unneeded, so remove it.
> >
> > Signed-off-by: Yangtao Li 
> > ---
> >  drivers/devfreq/event/rockchip-dfi.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/devfreq/event/rockchip-dfi.c 
> > b/drivers/devfreq/event/rockchip-dfi.c
> > index 22b113363ffc..fcbf76ebf55d 100644
> > --- a/drivers/devfreq/event/rockchip-dfi.c
> > +++ b/drivers/devfreq/event/rockchip-dfi.c
> > @@ -211,7 +211,7 @@ static int rockchip_dfi_probe(struct platform_device 
> > *pdev)
> >   if (IS_ERR(data->clk)) {
> >   dev_err(dev, "Cannot get the clk dmc_clk\n");
> >   return PTR_ERR(data->clk);
> > - };
> > + }
> >
> >   /* try to find the optional reference to the pmu syscon */
> >   node = of_parse_phandle(np, "rockchip,pmu", 0);
> >
>
> Reviewed-by: Chanwoo Choi 

Acked-by: MyungJoo Ham 

Merged. Thanks.

>
> --
> Best Regards,
> Chanwoo Choi
> Samsung Electronics



-- 
MyungJoo Ham, Ph.D.
S/W Center, Samsung Electronics

RE: [PATCH v3 5/7] drivers: devfreq: add longer polling interval in idle

[MyungJoo Ham]

> This patch adds new mechanism for devfreq devices which changes polling
> interval. The system should sleep longer when the devfreq device is almost
> not used. The devfreq framework will not schedule the work too often.
> This low-load state is recognised when the device is operating at the lowest
> frequency and has low busy_time/total_time factor (< 30%).
> When the frequency is different then min, the device is under normal polling
> which is the value defined in driver's 'polling_ms'.
> When the device is getting more pressure, the framework is able to catch it
> based on 'load' in lowest frequency and will start polling more frequently.
> The second scenario is when the governor recognised heavy load at minimum
> frequency and increases the frequency. The devfreq framework will start
> polling in shorter intervals.
> The polling interval, when the device is not heavily, can also be changed
> from userspace of defined by the driver's author.
> 
> Signed-off-by: Lukasz Luba 
> ---
>  drivers/devfreq/devfreq.c | 151 
> +++---
>  drivers/devfreq/governor.h|   3 +-
>  drivers/devfreq/governor_simpleondemand.c |   6 +-
>  3 files changed, 145 insertions(+), 15 deletions(-)
> 

There are some requirements that you need to consider:

Is 30% really applicable to ALL devfreq devices?
- What if some devices do not want such behaviors?
- What if some devices want different values (change behavors)?
- What if some manufactures want different default values?
- What if some devices want to let the framework know that it's in idle?
- What if some other kernel context, device (drivers),
or userspace process want to notify that it's no more idling?

As mentioned in the internal thread (tizen.org),
I'm not convinced by the idea of assuming that a device can be considered 
"idling"
if it has simply "low" utilization.

You are going to deteriorate the UI response time of mobile devices 
significantly.

Cheers,
MyungJoo.

RE: [PATCH 3/3] devfreq: add mediatek cci devfreq

[MyungJoo Ham]

>From: "Andrew-sh.Cheng" 
>
>For big/little cpu cluster architecture,
>not only CPU frequency, but CCI frequency will also affect performance.
>
>Little cores and cci share the same buck in Mediatek mt8183 IC,
>so we add a CCI devfreq which will get notification when buck voltage
>is changed, then CCI devfreq can set cci frequency as high as possible.
>
>Signed-off-by: Andrew-sh.Cheng 

Please correct the coding style first in the .c file.

- empty last line?
- inconsistent indentation (line26? match the first lines for all variable 
declarations OR don't match them at all)
- Need proper boilerplate and try to comply with C89.

Cheers,
MyungJoo

RE: [PATCH] devfreq: Suspend all devices on system shutdown

[MyungJoo Ham]

>This way devfreq core ensures that all its devices will be set to safe
>operation points before reboot operation. There are board on which some
>aggressive power saving operation points are behind the capabilities of
>the bootloader to properly reset the hardware and boot the board. This
>way one can avoid board crash early after reboot.
>
>Similar pattern is used in CPUfreq subsystem.
>
>Reported-by: Markus Reichl 
>Signed-off-by: Marek Szyprowski 
>---

You are invoking ALL devfreq suspend callbacks at shutdown
with this commit.

Can you make it invoke only devices explicitly saying their needs
to handle "SHUTDOWN" event?

For example, we can add a flag at struct devfreq_dev_profile:
"uint32_t requirement"
, where
0x1: need to operate at the initial frequency for suspend
0x2: need to operate at the initial frequency for shutdown
0x4: it forgets its status at resume, reconfigure frequency at resume.
(or reverse 0x1's semantics for the backward compatibility)
...

Cheers,
MyungJoo

RE: Re: [PATCH] drivers: extcon: Add support for ptn5150

[MyungJoo Ham]

>Hi Chanwoo Choi,
>
>This is the first time I am sending a driver to LKML. I have a few doubts. Can
>you please clarify them when you are free?

Although I'm not Chanwoo, but a guy who's about 50ft away from his cubicle,
as he appears to be busy today... :)

>
>1. I have developed and tested this patch on 4.14.89 kernel. When trying to 
>mainline the driver should I rebase and send the patch on top of current 
>tree(v5.0-rc2)?

Yes, you are supposed to be based on the most recent RC.

>
>2. Is there any specific git on which I should test this driver on? Like below?
>https://git.kernel.org/pub/scm/linux/kernel/git/chanwoo/extcon.git

I'd recommend to pull torvald's master with RC tag.


Cheers,
MyungJoo

>
>Thanks,
>Vijai Kumar K

[PATCH] PM / devfreq: consistent indentation

[MyungJoo Ham]

Following up with complaints on inconsistent indentation from
Yangtao Li, this fixes indentation inconsistency.

In principle, this tries to put arguments aligned to the left
including the first argument except for in the case where
the first argument is on the far-right side.

Signed-off-by: MyungJoo Ham 
---
 drivers/devfreq/devfreq.c | 49 +++
 1 file changed, 24 insertions(+), 25 deletions(-)

diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
index 4af608a..428a1de 100644
--- a/drivers/devfreq/devfreq.c
+++ b/drivers/devfreq/devfreq.c
@@ -528,7 +528,7 @@ void devfreq_interval_update(struct devfreq *devfreq, 
unsigned int *delay)
mutex_lock(>lock);
if (!devfreq->stop_polling)
queue_delayed_work(devfreq_wq, >work,
- msecs_to_jiffies(devfreq->profile->polling_ms));
+   msecs_to_jiffies(devfreq->profile->polling_ms));
}
 out:
mutex_unlock(>lock);
@@ -537,7 +537,7 @@ EXPORT_SYMBOL(devfreq_interval_update);
 
 /**
  * devfreq_notifier_call() - Notify that the device frequency requirements
- *has been changed out of devfreq framework.
+ *  has been changed out of devfreq framework.
  * @nb:the notifier_block (supposed to be devfreq->nb)
  * @type:  not used
  * @devp:  not used
@@ -683,12 +683,11 @@ struct devfreq *devfreq_add_device(struct device *dev,
goto err_out;
}
 
-   devfreq->trans_table =
-   devm_kzalloc(>dev,
-array3_size(sizeof(unsigned int),
-devfreq->profile->max_state,
-devfreq->profile->max_state),
-GFP_KERNEL);
+   devfreq->trans_table = devm_kzalloc(>dev,
+   array3_size(sizeof(unsigned int),
+   devfreq->profile->max_state,
+   devfreq->profile->max_state),
+   GFP_KERNEL);
if (!devfreq->trans_table) {
mutex_unlock(>lock);
err = -ENOMEM;
@@ -696,9 +695,9 @@ struct devfreq *devfreq_add_device(struct device *dev,
}
 
devfreq->time_in_state = devm_kcalloc(>dev,
-   devfreq->profile->max_state,
-   sizeof(unsigned long),
-   GFP_KERNEL);
+   devfreq->profile->max_state,
+   sizeof(unsigned long),
+   GFP_KERNEL);
if (!devfreq->time_in_state) {
mutex_unlock(>lock);
err = -ENOMEM;
@@ -1184,7 +1183,7 @@ static ssize_t available_governors_show(struct device *d,
 */
if (df->governor->immutable) {
count = scnprintf([count], DEVFREQ_NAME_LEN,
-  "%s ", df->governor_name);
+ "%s ", df->governor_name);
/*
 * The devfreq device shows the registered governor except for
 * immutable governors such as passive governor .
@@ -1497,8 +1496,8 @@ EXPORT_SYMBOL(devfreq_recommended_opp);
 
 /**
  * devfreq_register_opp_notifier() - Helper function to get devfreq notified
- *for any changes in the OPP availability
- *changes
+ *  for any changes in the OPP availability
+ *  changes
  * @dev:   The devfreq user device. (parent of devfreq)
  * @devfreq:   The devfreq object.
  */
@@ -1510,8 +1509,8 @@ EXPORT_SYMBOL(devfreq_register_opp_notifier);
 
 /**
  * devfreq_unregister_opp_notifier() - Helper function to stop getting devfreq
- *  notified for any changes in the OPP
- *  availability changes anymore.
+ *notified for any changes in the OPP
+ *availability changes anymore.
  * @dev:   The devfreq user device. (parent of devfreq)
  * @devfreq:   The devfreq object.
  *
@@ -1530,8 +1529,8 @@ static void devm_devfreq_opp_release(struct device *dev, 
void *res)
 }
 
 /**
- * devm_ devfreq_register_opp_notifier()
- * - Resource-managed devfreq_register_opp_notifier()
+ * devm_devfreq_register_opp_notifier() - Resource-managed
+ *   devfreq_register_opp_notifier()
  * @dev:   The devfreq user device. (parent of devfreq)
  * @devfreq:   The devfreq object.
  */
@@ -1559,8 +1558,8 @@ int devm_devfreq_register_opp_notifier(struct device *dev,
 EX

RE: [PATCH 2/3] PM / devfreq: fix missing check of return value in devfreq_add_device()

[MyungJoo Ham]

>devm_kzalloc() could fail, so insert a check of its return value. And
>if it fails, returns -ENOMEM.
>
>Signed-off-by: Yangtao Li 
>---
> drivers/devfreq/devfreq.c | 14 +-
> 1 file changed, 13 insertions(+), 1 deletion(-)
>
>diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
>index 076b1c2f40a4..923889229a0b 100644
>--- a/drivers/devfreq/devfreq.c
>+++ b/drivers/devfreq/devfreq.c

Acked-by: MyungJoo Ham 

RE: [PATCH 3/3] PM / devfreq: fix mem leak in devfreq_add_device()

[MyungJoo Ham]

>'devfreq' is malloced in devfreq_add_device() and should be freed in
>the error handling cases, otherwise it will cause memory leak.
>
>Signed-off-by: Yangtao Li 

Acked-by: MyungJoo Ham 


>---
> drivers/devfreq/devfreq.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
>diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
>index 923889229a0b..fe6c157cb7e0 100644
>--- a/drivers/devfreq/devfreq.c
>+++ b/drivers/devfreq/devfreq.c
>@@ -651,7 +651,7 @@ struct devfreq *devfreq_add_device(struct device *dev,
>   mutex_unlock(>lock);
>   err = set_freq_table(devfreq);
>   if (err < 0)
>-  goto err_out;
>+  goto err_dev;
>   mutex_lock(>lock);
>   }
> 

RE: [PATCH] PM / devfreq: fix mem leak and missing check of return value in devfreq_add_device()

[MyungJoo Ham]

> 'devfreq' is malloced in devfreq_add_device() and should be freed in
> the error handling cases, otherwise it will cause memory leak.
> 
> devm_kzalloc() could fail, so insert a check of its return value. And
> if it fails, returns -ENOMEM.
> 
> Signed-off-by: Yangtao Li 

Dear Yangtao,

Could you please make your patch without indentation style changes?


The label, "err_devfreq", would fit more if it's renamed "err_kzalloc".


Cheers,
MyungJoo.

RE: Re: [PATCH v3 4/5] drivers: power: suspend: call devfreq suspend/resume

[MyungJoo Ham]

>On 2018-12-05 12:05, Lukasz Luba wrote:
>> Devfreq framework supports suspend of its devices.
>> Call the the devfreq interface and allow devfreq devices preserve/restore
>> their states during suspend/resume.
>>
>> Suggested-by: Tobias Jakobi 
>> Reviewed-by: Chanwoo Choi 
>> Signed-off-by: Lukasz Luba 

This looks all good to me.

Acked-by: MyungJoo Ham 

>> ---
>>  drivers/base/power/main.c | 3 +++
>>  1 file changed, 3 insertions(+)
>>
>> diff --git a/drivers/base/power/main.c b/drivers/base/power/main.c
>> index a690fd4..0992e67 100644
>> --- a/drivers/base/power/main.c
>> +++ b/drivers/base/power/main.c
>> @@ -32,6 +32,7 @@
>>  #include 
>>  #include 
>>  #include 
>> +#include 
>>  #include 
>>  
>>  #include "../base.h"
>> @@ -1078,6 +1079,7 @@ void dpm_resume(pm_message_t state)
>>  dpm_show_time(starttime, state, 0, NULL);
>>  
>>  cpufreq_resume();
>> +devfreq_resume();
>>  trace_suspend_resume(TPS("dpm_resume"), state.event, false);
>>  }
>>  
>> @@ -1852,6 +1854,7 @@ int dpm_suspend(pm_message_t state)
>>  trace_suspend_resume(TPS("dpm_suspend"), state.event, true);
>>  might_sleep();
>>  
>> +devfreq_suspend();
>>  cpufreq_suspend();
>>  
>>  mutex_lock(_list_mtx);

Re: [PATCH] devfreq: Use of_node_name_eq for node name comparisons

[MyungJoo Ham]

On Thu, Dec 6, 2018 at 4:54 AM Rob Herring  wrote:
>
> Convert string compares of DT node names to use of_node_name_eq helper
> instead. This removes direct access to the node name pointer.
>
> For instances using of_node_cmp, this has the side effect of now using
> case sensitive comparisons. This should not matter for any FDT based
> system which all of these are.
>
> Cc: Chanwoo Choi 
> Cc: MyungJoo Ham 
> Cc: Kyungmin Park 
> Cc: Kukjin Kim 
> Cc: Krzysztof Kozlowski 
> Cc: linux...@vger.kernel.org
> Cc: linux-arm-ker...@lists.infradead.org
> Cc: linux-samsung-...@vger.kernel.org
> Signed-off-by: Rob Herring 

Acked-by: MyungJoo Ham 

> ---
>  drivers/devfreq/devfreq-event.c | 2 +-
>  drivers/devfreq/event/exynos-ppmu.c | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)

RE: [PATCH v4] PM / devfreq: Restart previous governor if new governor fails to start

[MyungJoo Ham]

> From: Saravana Kannan 
> 
> If the new governor fails to start, switch back to old governor so that the
> devfreq state is not left in some weird limbo.
> 
> Signed-off-by: Sibi Sankar 
> Signed-off-by: Saravana Kannan 
> Reviewed-by: Chanwoo Choi 

Hello,

In overall, the idea and the implementation looks good.

However, I have a question:

What if the following line fails?

+   df->governor->event_handler(df, DEVFREQ_GOV_START,
+   NULL);

Don't we still need something to handle for such events?

Cheers,
MyungJoo

RE: Re: [PATCH v3] PM / devfreq: Restart previous governor if new governor fails to start

[MyungJoo Ham]

> Hi Saravana,
> 
> The devfreq git repo is maintained by Myungjoo Ham.
> you can check it on MAINTAINERS file.
> 
> I think that you better to resend mail to mainline
> with my reviewed tag because the devfreq core could be modified
> and then merge conflict might be happen when apply this patch.
> 
> Regards,
> Chanwoo Choi
> 
> On 2018년 12월 08일 05:29, Saravana Kannan wrote:
> > 
> > On 11/9/16 4:10 PM, Chanwoo Choi wrote:
> >> After fixing the bug of devfreq_add_device(),
> >> I'll remove the unneeded 'if statement' of prev_governor in governor_store.
> > 
> > It's been 2 years and this patch still hasn't been picked up. Can we please 
> > pick this up and get this into the next kernel release?
> > 
> > Thanks,
> > 
> > Saravana
> > 

If that's already 2 years old, please rebase, test to see if that's
still valid with today's kernel, and resubmit.

Sorry for missing it, right now, after 2 years,
I just can't remember this one.

Cheers,
MyungJoo

RE: Re: [PATCH v3 3/5] devfreq: add devfreq_suspend/resume() functions

[MyungJoo Ham]

> Hi Lukasz,
> 
> On 2018년 12월 05일 20:05, Lukasz Luba wrote:
> > This patch adds implementation for global suspend/resume for
> > devfreq framework. System suspend will next use these functions.
> > 
> > Suggested-by: Tobias Jakobi 
> > Suggested-by: Chanwoo Choi 
> > Signed-off-by: Lukasz Luba 
> > ---
> >  drivers/devfreq/devfreq.c | 44 
> >  include/linux/devfreq.h   |  6 ++
> >  2 files changed, 50 insertions(+)
> 
> Reviewed-by: Chanwoo Choi 

Acked-by: MyungJoo Ham 

The patches from 1 to 3 are being applied and tested.
I'll probably send pull-request to Rafael today after some testing targetting 
4.21+

Cheers,
MyungJoo

RE: [PATCH v3 1/5] devfreq: refactor set_target frequency function

[MyungJoo Ham]

> The refactoring is needed for the new client in devfreq: suspend.
> To avoid code duplication, move it to the new local function
> devfreq_set_target.
> 
> Suggested-by: Tobias Jakobi 
> Suggested-by: Chanwoo Choi 
> Reviewed-by: Chanwoo Choi 
> Signed-off-by: Lukasz Luba 
> ---

Acked-by: MyungJoo Ham 

RE: Re: [PATCH v3 2/5] devfreq: add support for suspend/resume of a devfreq device

[MyungJoo Ham]

> Hi Lukasz,
> 
> On 2018년 12월 05일 20:05, Lukasz Luba wrote:
> > The patch prepares devfreq device for handling suspend/resume
> > functionality. The new fields will store needed information during this
> > process. Devfreq framework handles opp-suspend DT entry and there is no
> > need of modyfications in the drivers code. It uses atomic variables to
> > make sure no race condition affects the process.
> > 
> > Suggested-by: Tobias Jakobi 
> > Suggested-by: Chanwoo Choi 
> > Signed-off-by: Lukasz Luba 
> > ---
> >  drivers/devfreq/devfreq.c | 47 
> > +--
> >  include/linux/devfreq.h   |  7 +++
> >  2 files changed, 48 insertions(+), 6 deletions(-)
> 
> Reviewed-by: Chanwoo Choi 
> 

Looks goot do me as well.

Acked-by: MyungJoo Ham 


Anyway, for the sake of curiosity...

Having suspend-frequency is usually required when
the frequency configuration is reset with suspend-resume
as older Exynos's CPU did (I don't know whether it still does).

Does GPU do this as well?
(memory-bus won't do this because they are kept turned on during suspend)


Cheers,
MyungJoo

RE: [PATCH 1/6] devfreq: add basic fileds supporting suspend functionality

[MyungJoo Ham]

>The patch prepares devfreq device for handling suspend/resume functionality.
>The new fields will store needed information during this process.
>Devfreq framework handles opp-suspend DT entry and there is no need of
>modyfications in the drivers code.
>
>The patch draws on Tobias Jakobi's work posted ~2 years ago, who tried to
>solve issue with devfreq device's frequency during suspend/resume.
>During the discussion on LKML some corner cases and comments appeared
>related to the design. This patch address them keeping in mind suggestions
>from Chanwoo Choi.
>
>Suggested-by: Tobias Jakobi 
>Suggested-by: Chanwoo Choi 
>Signed-off-by: Lukasz Luba 

When you add new elements in a common struct (i.e., struct devfreq),
please describe kindly in the doxygen entries so that developers
may understand before reading all places where the new elements are
used.

You have added three new elements and there is no explanations on them.


Cheers,
MyungJoo

RE: [PATCH 1/6] devfreq: add basic fileds supporting suspend functionality

[MyungJoo Ham]

>The patch prepares devfreq device for handling suspend/resume functionality.
>The new fields will store needed information during this process.
>Devfreq framework handles opp-suspend DT entry and there is no need of
>modyfications in the drivers code.
>
>The patch draws on Tobias Jakobi's work posted ~2 years ago, who tried to
>solve issue with devfreq device's frequency during suspend/resume.
>During the discussion on LKML some corner cases and comments appeared
>related to the design. This patch address them keeping in mind suggestions
>from Chanwoo Choi.
>
>Suggested-by: Tobias Jakobi 
>Suggested-by: Chanwoo Choi 
>Signed-off-by: Lukasz Luba 

When you add new elements in a common struct (i.e., struct devfreq),
please describe kindly in the doxygen entries so that developers
may understand before reading all places where the new elements are
used.

You have added three new elements and there is no explanations on them.


Cheers,
MyungJoo

RE: [PATCH v2] PM / devfreq: Stop the governor before device_unregister()

[MyungJoo Ham]

> From: Vincent Donnefort 
> 
> device_release() is freeing the resources before calling the device
> specific release callback which is, in the case of devfreq, stopping
> the governor.
> 
> It is a problem as some governors are using the device resources. e.g.
> simpleondemand which is using the devfreq deferrable monitoring work. If it
> is not stopped before the resources are freed, it might lead to a use after
> free.
> 
> Signed-off-by: Vincent Donnefort 
> Reviewed-by: John Einar Reitan 
> Reviewed-by: Chanwoo Choi 

Acked-by: MyungJoo Ham 


Cheers,
MyungJoo

RE: [PATCH v2] PM / devfreq: Stop the governor before device_unregister()

[MyungJoo Ham]

> From: Vincent Donnefort 
> 
> device_release() is freeing the resources before calling the device
> specific release callback which is, in the case of devfreq, stopping
> the governor.
> 
> It is a problem as some governors are using the device resources. e.g.
> simpleondemand which is using the devfreq deferrable monitoring work. If it
> is not stopped before the resources are freed, it might lead to a use after
> free.
> 
> Signed-off-by: Vincent Donnefort 
> Reviewed-by: John Einar Reitan 
> Reviewed-by: Chanwoo Choi 

Acked-by: MyungJoo Ham 


Cheers,
MyungJoo

RE: [PATCH] PM / devfreq: remove redundant null pointer check before kfree

[MyungJoo Ham]

 
> kfree has taken the null pointer into account. hence it is safe
> to remove the redundant null pointer check before kfree.
> 
> Signed-off-by: zhong jiang 

Acked-by: MyungJoo Ham 

> ---
>  drivers/devfreq/devfreq.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
> index 4c49bb1..c37021d 100644
> --- a/drivers/devfreq/devfreq.c
> +++ b/drivers/devfreq/devfreq.c
> @@ -675,8 +675,7 @@ struct devfreq *devfreq_add_device(struct device *dev,
>   device_unregister(>dev);
>   devfreq = NULL;
>  err_dev:
> - if (devfreq)
> - kfree(devfreq);
> + kfree(devfreq);
>  err_out:
>   return ERR_PTR(err);
>  }
> -- 
> 1.7.12.4

 
--
MyungJoo Ham (함명주), Ph.D.
Autonomous Machine Lab., AI Center, Samsung Research.
Cell: +82-10-6714-2858

RE: [PATCH] PM / devfreq: remove redundant null pointer check before kfree

[MyungJoo Ham]

 
> kfree has taken the null pointer into account. hence it is safe
> to remove the redundant null pointer check before kfree.
> 
> Signed-off-by: zhong jiang 

Acked-by: MyungJoo Ham 

> ---
>  drivers/devfreq/devfreq.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
> index 4c49bb1..c37021d 100644
> --- a/drivers/devfreq/devfreq.c
> +++ b/drivers/devfreq/devfreq.c
> @@ -675,8 +675,7 @@ struct devfreq *devfreq_add_device(struct device *dev,
>   device_unregister(>dev);
>   devfreq = NULL;
>  err_dev:
> - if (devfreq)
> - kfree(devfreq);
> + kfree(devfreq);
>  err_out:
>   return ERR_PTR(err);
>  }
> -- 
> 1.7.12.4

 
--
MyungJoo Ham (함명주), Ph.D.
Autonomous Machine Lab., AI Center, Samsung Research.
Cell: +82-10-6714-2858

RE: Re: [PATCH] devfreq: Convert to using %pOFn instead of device_node.name

[MyungJoo Ham]

> Dear Rob,
> 
> On 2018년 08월 28일 10:52, Rob Herring wrote:
> > In preparation to remove the node name pointer from struct device_node,
> > convert printf users to use the %pOFn format specifier.
> > 
> > Cc: Chanwoo Choi 
> > Cc: MyungJoo Ham 
> > Cc: Kyungmin Park 
> > Cc: Kukjin Kim 
> > Cc: Krzysztof Kozlowski 
> > Cc: linux...@vger.kernel.org
> > Cc: linux-arm-ker...@lists.infradead.org
> > Cc: linux-samsung-...@vger.kernel.org
> > Signed-off-by: Rob Herring 
> > ---
> >  drivers/devfreq/event/exynos-ppmu.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
[]
> 
> Acked-by: Chanwoo Choi 

Acked-by: MyungJoo Ham 

RE: Re: [PATCH] devfreq: Convert to using %pOFn instead of device_node.name

[MyungJoo Ham]

> Dear Rob,
> 
> On 2018년 08월 28일 10:52, Rob Herring wrote:
> > In preparation to remove the node name pointer from struct device_node,
> > convert printf users to use the %pOFn format specifier.
> > 
> > Cc: Chanwoo Choi 
> > Cc: MyungJoo Ham 
> > Cc: Kyungmin Park 
> > Cc: Kukjin Kim 
> > Cc: Krzysztof Kozlowski 
> > Cc: linux...@vger.kernel.org
> > Cc: linux-arm-ker...@lists.infradead.org
> > Cc: linux-samsung-...@vger.kernel.org
> > Signed-off-by: Rob Herring 
> > ---
> >  drivers/devfreq/event/exynos-ppmu.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
[]
> 
> Acked-by: Chanwoo Choi 

Acked-by: MyungJoo Ham 

RE: [PATCH v3 2/2] PM / devfreq: Add devfreq driver for interconnect bandwidth voting

[MyungJoo Ham]

>This driver registers itself as a devfreq device that allows devfreq
>governors to make bandwidth votes for an interconnect path. This allows
>applying various policies for different interconnect paths using devfreq
>governors.
>

First of all, the name, "devfreq_icbw", is not appropriate for a
devfreq device driver. It confuses; it looks like a part of the
framework itself.

>diff --git a/drivers/devfreq/devfreq_icbw.c b/drivers/devfreq/devfreq_icbw.c
>new file mode 100644
>index 000..231fb21
>--- /dev/null
>+++ b/drivers/devfreq/devfreq_icbw.c
>@@ -0,0 +1,116 @@
>+// SPDX-License-Identifier: GPL-2.0
>+/*
>+ * Copyright (c) 2013-2014, 2018, The Linux Foundation. All rights reserved.
>+ */
>+
>+#define pr_fmt(fmt) "icbw: " fmt
>+
>+#include 
>+#include 
>+#include 
>+#include 
>+#include 
>+#include 
>+#include 
>+#include 
>+#include 
>+#include 

Where can I find this file?

>+
>+struct dev_data {
>+  struct icc_path *path;
>+  u32 cur_ab;
>+  u32 cur_pb;
>+  unsigned long gov_ab;
>+  struct devfreq *df;
>+  struct devfreq_dev_profile dp;
>+};
>+
>+static int icbw_target(struct device *dev, unsigned long *freq, u32 flags)
>+{
>+  struct dev_data *d = dev_get_drvdata(dev);
>+  int ret;
>+  u32 new_pb = *freq, new_ab = d->gov_ab;
>+
>+  if (d->cur_pb == new_pb && d->cur_ab == new_ab)
>+  return 0;
>+
>+  dev_dbg(dev, "BW KBps: AB: %u PB: %u\n", new_ab, new_pb);
>+
>+  ret = icc_set(d->path, new_ab, new_pb);

I'm not sure if icc_set is available.


Cheers,
MyungJoo

RE: [PATCH v3 2/2] PM / devfreq: Add devfreq driver for interconnect bandwidth voting

[MyungJoo Ham]

>This driver registers itself as a devfreq device that allows devfreq
>governors to make bandwidth votes for an interconnect path. This allows
>applying various policies for different interconnect paths using devfreq
>governors.
>

First of all, the name, "devfreq_icbw", is not appropriate for a
devfreq device driver. It confuses; it looks like a part of the
framework itself.

>diff --git a/drivers/devfreq/devfreq_icbw.c b/drivers/devfreq/devfreq_icbw.c
>new file mode 100644
>index 000..231fb21
>--- /dev/null
>+++ b/drivers/devfreq/devfreq_icbw.c
>@@ -0,0 +1,116 @@
>+// SPDX-License-Identifier: GPL-2.0
>+/*
>+ * Copyright (c) 2013-2014, 2018, The Linux Foundation. All rights reserved.
>+ */
>+
>+#define pr_fmt(fmt) "icbw: " fmt
>+
>+#include 
>+#include 
>+#include 
>+#include 
>+#include 
>+#include 
>+#include 
>+#include 
>+#include 
>+#include 

Where can I find this file?

>+
>+struct dev_data {
>+  struct icc_path *path;
>+  u32 cur_ab;
>+  u32 cur_pb;
>+  unsigned long gov_ab;
>+  struct devfreq *df;
>+  struct devfreq_dev_profile dp;
>+};
>+
>+static int icbw_target(struct device *dev, unsigned long *freq, u32 flags)
>+{
>+  struct dev_data *d = dev_get_drvdata(dev);
>+  int ret;
>+  u32 new_pb = *freq, new_ab = d->gov_ab;
>+
>+  if (d->cur_pb == new_pb && d->cur_ab == new_ab)
>+  return 0;
>+
>+  dev_dbg(dev, "BW KBps: AB: %u PB: %u\n", new_ab, new_pb);
>+
>+  ret = icc_set(d->path, new_ab, new_pb);

I'm not sure if icc_set is available.


Cheers,
MyungJoo

RE: [PATCH v3 1/2] PM / devfreq: Generic CPU frequency to device frequency mapping governor

[MyungJoo Ham]

>Many CPU architectures have caches that can scale independent of the CPUs.
>Frequency scaling of the caches is necessary to make sure the cache is not
>a performance bottleneck that leads to poor performance and power. The same
>idea applies for RAM/DDR.
>
>To achieve this, this patch adds a generic devfreq governor that takes the
>current frequency of each CPU frequency domain and then adjusts the
>frequency of the cache (or any devfreq device) based on the frequency of
>the CPUs. It listens to CPU frequency transition notifiers to keep itself
>up to date on the current CPU frequency.
>
>To decide the frequency of the device, the governor does one of the
>following:

This exactly has the same purpose with "passive" governor except for the
single part: passive governor depends on another devfreq driver, not
cpufreq driver.

If both governors have many features in common, can you merge them into one?
Passive governor also has "get_target_freq", which allows driver authors
to define the mapping.

Probably, you will need to add one more notifier call to get events from
cpufreq instead of devfreq.

>
>* Uses a CPU frequency to device frequency mapping table
>  - Either one mapping table used for all CPU freq policies (typically used
>for system with homogeneous cores/clusters that have the same OPPs).
>  - One mapping table per CPU freq policy (typically used for ASMP systems
>with heterogeneous CPUs with different OPPs)
>
>OR
>
>* Scales the device frequency in proportion to the CPU frequency. So, if
>  the CPUs are running at their max frequency, the device runs at its max
>  frequency.  If the CPUs are running at their min frequency, the device
>  runs at its min frequency. And interpolated for frequencies in between.

If you want to satisfy these two cases to the "modified" passive governors,
you may need to supply two "preloaded" get_target_freq functions for
struct devfreq_passive_data. If that's impossible, requiring a bit more
modifications, you may need to write alternative routines in
update_devfreq_passive().

Cheers,
MyungJoo

>
>Signed-off-by: Saravana Kannan 
>---
> .../bindings/devfreq/devfreq-cpufreq-map.txt   |  53 ++
> drivers/devfreq/Kconfig|   8 +
> drivers/devfreq/Makefile   |   1 +
> drivers/devfreq/governor_cpufreq_map.c | 583 +
> 4 files changed, 645 insertions(+)
> create mode 100644 
> Documentation/devicetree/bindings/devfreq/devfreq-cpufreq-map.txt
> create mode 100644 drivers/devfreq/governor_cpufreq_map.c
>
[]
>diff --git a/drivers/devfreq/governor_cpufreq_map.c 
>b/drivers/devfreq/governor_cpufreq_map.c
>new file mode 100644
>index 000..084a3ff
>--- /dev/null
>+++ b/drivers/devfreq/governor_cpufreq_map.c
>@@ -0,0 +1,583 @@
>+// SPDX-License-Identifier: GPL-2.0
>+/*
>+ * Copyright (c) 2014-2015, 2018, The Linux Foundation. All rights reserved.
>+ */

Is this boilerplate fine? ( using // )

RE: [PATCH v3 1/2] PM / devfreq: Generic CPU frequency to device frequency mapping governor

[MyungJoo Ham]

>Many CPU architectures have caches that can scale independent of the CPUs.
>Frequency scaling of the caches is necessary to make sure the cache is not
>a performance bottleneck that leads to poor performance and power. The same
>idea applies for RAM/DDR.
>
>To achieve this, this patch adds a generic devfreq governor that takes the
>current frequency of each CPU frequency domain and then adjusts the
>frequency of the cache (or any devfreq device) based on the frequency of
>the CPUs. It listens to CPU frequency transition notifiers to keep itself
>up to date on the current CPU frequency.
>
>To decide the frequency of the device, the governor does one of the
>following:

This exactly has the same purpose with "passive" governor except for the
single part: passive governor depends on another devfreq driver, not
cpufreq driver.

If both governors have many features in common, can you merge them into one?
Passive governor also has "get_target_freq", which allows driver authors
to define the mapping.

Probably, you will need to add one more notifier call to get events from
cpufreq instead of devfreq.

>
>* Uses a CPU frequency to device frequency mapping table
>  - Either one mapping table used for all CPU freq policies (typically used
>for system with homogeneous cores/clusters that have the same OPPs).
>  - One mapping table per CPU freq policy (typically used for ASMP systems
>with heterogeneous CPUs with different OPPs)
>
>OR
>
>* Scales the device frequency in proportion to the CPU frequency. So, if
>  the CPUs are running at their max frequency, the device runs at its max
>  frequency.  If the CPUs are running at their min frequency, the device
>  runs at its min frequency. And interpolated for frequencies in between.

If you want to satisfy these two cases to the "modified" passive governors,
you may need to supply two "preloaded" get_target_freq functions for
struct devfreq_passive_data. If that's impossible, requiring a bit more
modifications, you may need to write alternative routines in
update_devfreq_passive().

Cheers,
MyungJoo

>
>Signed-off-by: Saravana Kannan 
>---
> .../bindings/devfreq/devfreq-cpufreq-map.txt   |  53 ++
> drivers/devfreq/Kconfig|   8 +
> drivers/devfreq/Makefile   |   1 +
> drivers/devfreq/governor_cpufreq_map.c | 583 +
> 4 files changed, 645 insertions(+)
> create mode 100644 
> Documentation/devicetree/bindings/devfreq/devfreq-cpufreq-map.txt
> create mode 100644 drivers/devfreq/governor_cpufreq_map.c
>
[]
>diff --git a/drivers/devfreq/governor_cpufreq_map.c 
>b/drivers/devfreq/governor_cpufreq_map.c
>new file mode 100644
>index 000..084a3ff
>--- /dev/null
>+++ b/drivers/devfreq/governor_cpufreq_map.c
>@@ -0,0 +1,583 @@
>+// SPDX-License-Identifier: GPL-2.0
>+/*
>+ * Copyright (c) 2014-2015, 2018, The Linux Foundation. All rights reserved.
>+ */

Is this boilerplate fine? ( using // )

RE: Re: Re: devfreq relation with pm qos

[MyungJoo Ham]

>MyungJo, I am trying to understand the relationship of PM QOS with
>devfreq drivers. I don't see any relation of PM QOS api's with devfreq drivers
>directly as there are no QOS apis used in the devfreq framework.
>
>The only explanation I have is that PM QOS has direct relationship
>with CPUFREQ driver and as CPUFREQ driver works with CPU
>frequency and that CPU frequency directly affects DEVFREQ drivers
>as devfreq driver maps CPU frequency to device frequency and
>that is why PM QOS doesn’t have direct relation with PM QOS.
>
>Is my understanding right?

Your understanding on the fact that there is no relation between
PM QOS and Devfreq framework of today's code is correct.

The reason on why so is not.
In general, you don't make a peripheral device be directly throttled
by CPUFreq values. There exist such cases, but they are not
"general" or inherit characteristics of peripheral devices.

Cheers,
MyungJoo.

RE: Re: Re: devfreq relation with pm qos

[MyungJoo Ham]

>MyungJo, I am trying to understand the relationship of PM QOS with
>devfreq drivers. I don't see any relation of PM QOS api's with devfreq drivers
>directly as there are no QOS apis used in the devfreq framework.
>
>The only explanation I have is that PM QOS has direct relationship
>with CPUFREQ driver and as CPUFREQ driver works with CPU
>frequency and that CPU frequency directly affects DEVFREQ drivers
>as devfreq driver maps CPU frequency to device frequency and
>that is why PM QOS doesn’t have direct relation with PM QOS.
>
>Is my understanding right?

Your understanding on the fact that there is no relation between
PM QOS and Devfreq framework of today's code is correct.

The reason on why so is not.
In general, you don't make a peripheral device be directly throttled
by CPUFreq values. There exist such cases, but they are not
"general" or inherit characteristics of peripheral devices.

Cheers,
MyungJoo.

RE: Re: devfreq relation with pm qos

[MyungJoo Ham]

> + dev freq maintainters.
> 
> On Mon, Jul 9, 2018 at 3:37 AM, noman pouigt  wrote:
> > folks,
> >
> > I am trying to figure out the relationship between PM QOS
> > with devfreq framework. I see this thread[1] where MyungJoo
> > talks about QOS and devfreq but that control is through
> > sysfs but I don't see any relation of pm qos (kernel/power/qos.c)
> > with devfreq directly as devfreq is not calling any of the QOS
> > api's. Is this intended?
> >
> > Isn't QOS value update using pm_qos_update_request has a
> > direct relation with devfreq drivers i.e. setting the value to
> > high or low selects the corresponding voltage and frequency
> > setting in devfreq framework? I went through the devfreq
> > drivers and couldn't find that relationship. Is this by design or
> > I am missing something very obvious?
> >
> > [1] https://lwn.net/Articles/484161/

Hello,

Unfortunately, the suggested concept in the referred article was not ever
accepted in the mainline.

Cheers,
MyungJo

RE: Re: devfreq relation with pm qos

[MyungJoo Ham]

> + dev freq maintainters.
> 
> On Mon, Jul 9, 2018 at 3:37 AM, noman pouigt  wrote:
> > folks,
> >
> > I am trying to figure out the relationship between PM QOS
> > with devfreq framework. I see this thread[1] where MyungJoo
> > talks about QOS and devfreq but that control is through
> > sysfs but I don't see any relation of pm qos (kernel/power/qos.c)
> > with devfreq directly as devfreq is not calling any of the QOS
> > api's. Is this intended?
> >
> > Isn't QOS value update using pm_qos_update_request has a
> > direct relation with devfreq drivers i.e. setting the value to
> > high or low selects the corresponding voltage and frequency
> > setting in devfreq framework? I went through the devfreq
> > drivers and couldn't find that relationship. Is this by design or
> > I am missing something very obvious?
> >
> > [1] https://lwn.net/Articles/484161/

Hello,

Unfortunately, the suggested concept in the referred article was not ever
accepted in the mainline.

Cheers,
MyungJo

RE: [PATCH v5 01/12] PM / devfreq: Init user limits from OPP limits, not viceversa

[MyungJoo Ham]

>Commit ab8f58ad72c4 ("PM / devfreq: Set min/max_freq when adding
>the devfreq device") introduced the initialization of the user
>limits min/max_freq from the lowest/highest available OPPs. Later
>commit f1d981eaecf8 ("PM / devfreq: Use the available min/max
>frequency") added scaling_min/max_freq, which actually represent
>the frequencies of the lowest/highest available OPP. scaling_min/
>max_freq are initialized with the values from min/max_freq, which
>is totally correct in the context, but a bit awkward to read.
>
>Swap the initialization and assign scaling_min/max_freq with the
>OPP freqs and then the user limts min/max_freq with scaling_min/
>max_freq.
>
>Needless to say that this change is a NOP, intended to improve
>readability.
>
>Signed-off-by: Matthias Kaehlcke 
>Reviewed-by: Chanwoo Choi 
>Reviewed-by: Brian Norris 
>---
>Changes in v5:
>- none
>
>Changes in v4:
>- added 'Reviewed-by: Brian Norris ' tag
>
>Changes in v3:
>- none
>
>Changes in v2:
>- added 'Reviewed-by: Chanwoo Choi ' tag
>---
> drivers/devfreq/devfreq.c | 12 ++--
> 1 file changed, 6 insertions(+), 6 deletions(-)

Acked-by: MyungJoo Ham 

This can be applied independently from other commits in this series.

RE: [PATCH v5 01/12] PM / devfreq: Init user limits from OPP limits, not viceversa

[MyungJoo Ham]

>Commit ab8f58ad72c4 ("PM / devfreq: Set min/max_freq when adding
>the devfreq device") introduced the initialization of the user
>limits min/max_freq from the lowest/highest available OPPs. Later
>commit f1d981eaecf8 ("PM / devfreq: Use the available min/max
>frequency") added scaling_min/max_freq, which actually represent
>the frequencies of the lowest/highest available OPP. scaling_min/
>max_freq are initialized with the values from min/max_freq, which
>is totally correct in the context, but a bit awkward to read.
>
>Swap the initialization and assign scaling_min/max_freq with the
>OPP freqs and then the user limts min/max_freq with scaling_min/
>max_freq.
>
>Needless to say that this change is a NOP, intended to improve
>readability.
>
>Signed-off-by: Matthias Kaehlcke 
>Reviewed-by: Chanwoo Choi 
>Reviewed-by: Brian Norris 
>---
>Changes in v5:
>- none
>
>Changes in v4:
>- added 'Reviewed-by: Brian Norris ' tag
>
>Changes in v3:
>- none
>
>Changes in v2:
>- added 'Reviewed-by: Chanwoo Choi ' tag
>---
> drivers/devfreq/devfreq.c | 12 ++--
> 1 file changed, 6 insertions(+), 6 deletions(-)

Acked-by: MyungJoo Ham 

This can be applied independently from other commits in this series.

RE: [PATCH v4] PM / devfreq: Fix devfreq_add_device() when drivers are built as modules.

[MyungJoo Ham]

>+  if (!strncmp(name, DEVFREQ_GOV_SIMPLE_ONDEMAND,
>+   DEVFREQ_NAME_LEN))
>+  err = request_module("governor_%s", "simpleondemand");
>+  else
>+  err = request_module("governor_%s", name);
>+  if (err)
>+  return NULL;

You are returning without the lock acquired..

RE: [PATCH v4] PM / devfreq: Fix devfreq_add_device() when drivers are built as modules.

[MyungJoo Ham]

>+  if (!strncmp(name, DEVFREQ_GOV_SIMPLE_ONDEMAND,
>+   DEVFREQ_NAME_LEN))
>+  err = request_module("governor_%s", "simpleondemand");
>+  else
>+  err = request_module("governor_%s", name);
>+  if (err)
>+  return NULL;

You are returning without the lock acquired..

RE: [PATCH v3] PM / devfreq: Fix devfreq_add_device() when drivers are built as modules.

[MyungJoo Ham]

>@@ -988,12 +1030,13 @@ static ssize_t governor_store(struct device *dev, 
>struct device_attribute *attr,
>   if (ret != 1)
>   return -EINVAL;
> 
>-  mutex_lock(_list_lock);
>-  governor = find_devfreq_governor(str_governor);
>+  governor = try_then_request_governor(str_governor);
>   if (IS_ERR(governor)) {
>-  ret = PTR_ERR(governor);
>-  goto out;
>+  return PTR_ERR(governor);
>   }
>+
>+  mutex_lock(_list_lock);
>+
>   if (df->governor == governor) {
>   ret = 0;
>   goto out;
>-- 

The possibility that the result of try_then_request_governor is
not kept protected until the line of
"if (df->governor == governor)" is itching.

Can you make it kept "locked" from the return of
find_devfreq_governor() (either in try_then... or in this function)
to the unlock of governor_store()?


Cheers,
MyungJoo

RE: [PATCH v3] PM / devfreq: Fix devfreq_add_device() when drivers are built as modules.

[MyungJoo Ham]

>@@ -988,12 +1030,13 @@ static ssize_t governor_store(struct device *dev, 
>struct device_attribute *attr,
>   if (ret != 1)
>   return -EINVAL;
> 
>-  mutex_lock(_list_lock);
>-  governor = find_devfreq_governor(str_governor);
>+  governor = try_then_request_governor(str_governor);
>   if (IS_ERR(governor)) {
>-  ret = PTR_ERR(governor);
>-  goto out;
>+  return PTR_ERR(governor);
>   }
>+
>+  mutex_lock(_list_lock);
>+
>   if (df->governor == governor) {
>   ret = 0;
>   goto out;
>-- 

The possibility that the result of try_then_request_governor is
not kept protected until the line of
"if (df->governor == governor)" is itching.

Can you make it kept "locked" from the return of
find_devfreq_governor() (either in try_then... or in this function)
to the unlock of governor_store()?


Cheers,
MyungJoo

RE: Re: [PATCH v3] PM / devfreq: Fix devfreq_add_device() when drivers are built as modules.

[MyungJoo Ham]

> >> Adding to Ezequiel's point, shouldn't we take more granular lock
> >> (devfreq->lock) first and then call devfreq_list_lock at the time of
> >> adding to the list?
> >> 
> > 
> > Not sure why we should do that. devfreq->lock should be used to
> > protect the struct devfreq state, while the devfreq_list_lock
> > is apparently protecting the two lists (which seem unrelated
> > lists).

Correct.

devfreq->lock protects an instance of devfreq.
devfreq_list_lock protects the global devfreq data (list of devfreq / governors)

> > 
> > So, the two locks are not correlated.
> > 
> > Regards,
> > Eze
> In governor_store(), we do 'df->governor = governor;' without taking 
> df->lock. So it is possible to switch governor while update_devfreq() is 
> in progress.

Yup. that's possible.

> I smell trouble there. Don't you think so?
> I am assuming df->lock protects 'struct devfreq' and devfreq_list_lock 
> protects both device and governor lists.

devfreq_list_lock is not supposed to protect a device.

Assuming a memory read of a word is atomic (I'm not aware of one that's not
unless in a case where the address is unaligned in some archtectures),
update_devfreq won't cause such issues because it reads "devfreq->governor"
only one in its execution except for the null check.

Thus, if there could be an error, it'd be a case where someone else is
doing "devfreq->governor = NULL" without devfreq->lock.
And, find_devfreq_governor() does not return NULL.


Cheers,
MyungJoo

> 
> -Akhil.
> 

RE: Re: [PATCH v3] PM / devfreq: Fix devfreq_add_device() when drivers are built as modules.

[MyungJoo Ham]

> >> Adding to Ezequiel's point, shouldn't we take more granular lock
> >> (devfreq->lock) first and then call devfreq_list_lock at the time of
> >> adding to the list?
> >> 
> > 
> > Not sure why we should do that. devfreq->lock should be used to
> > protect the struct devfreq state, while the devfreq_list_lock
> > is apparently protecting the two lists (which seem unrelated
> > lists).

Correct.

devfreq->lock protects an instance of devfreq.
devfreq_list_lock protects the global devfreq data (list of devfreq / governors)

> > 
> > So, the two locks are not correlated.
> > 
> > Regards,
> > Eze
> In governor_store(), we do 'df->governor = governor;' without taking 
> df->lock. So it is possible to switch governor while update_devfreq() is 
> in progress.

Yup. that's possible.

> I smell trouble there. Don't you think so?
> I am assuming df->lock protects 'struct devfreq' and devfreq_list_lock 
> protects both device and governor lists.

devfreq_list_lock is not supposed to protect a device.

Assuming a memory read of a word is atomic (I'm not aware of one that's not
unless in a case where the address is unaligned in some archtectures),
update_devfreq won't cause such issues because it reads "devfreq->governor"
only one in its execution except for the null check.

Thus, if there could be an error, it'd be a case where someone else is
doing "devfreq->governor = NULL" without devfreq->lock.
And, find_devfreq_governor() does not return NULL.


Cheers,
MyungJoo

> 
> -Akhil.
> 

Re: [RFC] PM / devfreq: Add support for alerts

[MyungJoo Ham]

Hi Akhil,

Actually, this provides custom events for governors, with different
behaviors per each governor, while Matthias's give you the effects not
targeted for a specific governor.

Could you please show me why (the usage cases) you need this
additional event for governors? (You need to implement an event
handler in each governor for this approach).


Cheers,
MyungJoo


On Fri, Jun 1, 2018 at 8:43 PM, Akhil P Oommen  wrote:
>
>
> On 5/31/2018 11:47 AM, MyungJoo Ham wrote:
>>>
>>> Currently, DEVFREQ reevaluates the device state periodically and/or
>>> based on the OPP list changes. Private API has to be exposed to allow
>>> the device driver to alert/notify the governor to reevaluate when a new
>>> set of data is available. This makes the governor more coupled to a
>>> particular device driver. We can improve here by exposing a DEVFREQ API
>>> to allow the device drivers to send generic alerts to the governor.
>>>
>>> Signed-off-by: Akhil P Oommen 
>>> ---
>>>   drivers/devfreq/devfreq.c  | 21 +
>>>   drivers/devfreq/governor.h |  1 +
>>>   include/linux/devfreq.h|  5 +
>>>   3 files changed, 27 insertions(+)
>>>
>> Hello Akhil,
>>
>> It appears that this will have the same effect with
>> "[PATCH 08/11] PM / devfreq: Make update_devfreq() public" from Matthias
>> Kaehlcke, doesn't it?
>>
>>
>> Cheers,
>> MyungJoo
>>
> Hi MyngJoo,
>
> The patch you mentioned is a step in the right direction. But this patch
> allows:
> 1. the governor to decide whether to reevaluate or not. I feel it would be a
> better architecture (better Separation of Concern) if that decision is left
> to the governor alone.
> 2. the devices to share multiple types of alerts. A governor may use these
> alerts for internal bookkeeping/algorithm and decide to reevaluate policy
> when it is necessary. Since we are opening up a new devfreq API for devices,
> isn't it better to go for a generic one?
>
> Regards,
> Akhil.



-- 
MyungJoo Ham, Ph.D.
S/W Center, Samsung Electronics

Re: [RFC] PM / devfreq: Add support for alerts

[MyungJoo Ham]

Hi Akhil,

Actually, this provides custom events for governors, with different
behaviors per each governor, while Matthias's give you the effects not
targeted for a specific governor.

Could you please show me why (the usage cases) you need this
additional event for governors? (You need to implement an event
handler in each governor for this approach).


Cheers,
MyungJoo


On Fri, Jun 1, 2018 at 8:43 PM, Akhil P Oommen  wrote:
>
>
> On 5/31/2018 11:47 AM, MyungJoo Ham wrote:
>>>
>>> Currently, DEVFREQ reevaluates the device state periodically and/or
>>> based on the OPP list changes. Private API has to be exposed to allow
>>> the device driver to alert/notify the governor to reevaluate when a new
>>> set of data is available. This makes the governor more coupled to a
>>> particular device driver. We can improve here by exposing a DEVFREQ API
>>> to allow the device drivers to send generic alerts to the governor.
>>>
>>> Signed-off-by: Akhil P Oommen 
>>> ---
>>>   drivers/devfreq/devfreq.c  | 21 +
>>>   drivers/devfreq/governor.h |  1 +
>>>   include/linux/devfreq.h|  5 +
>>>   3 files changed, 27 insertions(+)
>>>
>> Hello Akhil,
>>
>> It appears that this will have the same effect with
>> "[PATCH 08/11] PM / devfreq: Make update_devfreq() public" from Matthias
>> Kaehlcke, doesn't it?
>>
>>
>> Cheers,
>> MyungJoo
>>
> Hi MyngJoo,
>
> The patch you mentioned is a step in the right direction. But this patch
> allows:
> 1. the governor to decide whether to reevaluate or not. I feel it would be a
> better architecture (better Separation of Concern) if that decision is left
> to the governor alone.
> 2. the devices to share multiple types of alerts. A governor may use these
> alerts for internal bookkeeping/algorithm and decide to reevaluate policy
> when it is necessary. Since we are opening up a new devfreq API for devices,
> isn't it better to go for a generic one?
>
> Regards,
> Akhil.



-- 
MyungJoo Ham, Ph.D.
S/W Center, Samsung Electronics

RE: [RFC] PM / devfreq: Add support for alerts

[MyungJoo Ham]

> Currently, DEVFREQ reevaluates the device state periodically and/or
> based on the OPP list changes. Private API has to be exposed to allow
> the device driver to alert/notify the governor to reevaluate when a new
> set of data is available. This makes the governor more coupled to a
> particular device driver. We can improve here by exposing a DEVFREQ API
> to allow the device drivers to send generic alerts to the governor.
> 
> Signed-off-by: Akhil P Oommen 
> ---
>  drivers/devfreq/devfreq.c  | 21 +
>  drivers/devfreq/governor.h |  1 +
>  include/linux/devfreq.h|  5 +
>  3 files changed, 27 insertions(+)
> 

Hello Akhil,

It appears that this will have the same effect with
"[PATCH 08/11] PM / devfreq: Make update_devfreq() public" from Matthias 
Kaehlcke, doesn't it?


Cheers,
MyungJoo

RE: [RFC] PM / devfreq: Add support for alerts

[MyungJoo Ham]

> Currently, DEVFREQ reevaluates the device state periodically and/or
> based on the OPP list changes. Private API has to be exposed to allow
> the device driver to alert/notify the governor to reevaluate when a new
> set of data is available. This makes the governor more coupled to a
> particular device driver. We can improve here by exposing a DEVFREQ API
> to allow the device drivers to send generic alerts to the governor.
> 
> Signed-off-by: Akhil P Oommen 
> ---
>  drivers/devfreq/devfreq.c  | 21 +
>  drivers/devfreq/governor.h |  1 +
>  include/linux/devfreq.h|  5 +
>  3 files changed, 27 insertions(+)
> 

Hello Akhil,

It appears that this will have the same effect with
"[PATCH 08/11] PM / devfreq: Make update_devfreq() public" from Matthias 
Kaehlcke, doesn't it?


Cheers,
MyungJoo

RE: [PATCH 2/2] PM / devfreq: Generic cpufreq governor

[MyungJoo Ham]

>Many CPU architectures have caches that can scale independent of the CPUs.
>Frequency scaling of the caches is necessary to make sure the cache is not
>a performance bottleneck that leads to poor performance and power. The same
>idea applies for RAM/DDR.
>
>To achieve this, this patch series adds a generic devfreq governor that can
>listen to the frequency transitions of each CPU frequency domain and then
>adjusts the frequency of the cache (or any devfreq device) based on the
>frequency of the CPUs.

I agree that we have some hardware pieces that want to configure
frequencies based on the CPUfreq.

Creating a devfreq governor that configures devfreq-freq
based on incoming events (CPUFreq-transition-event in this case)
is indeed a good idea.

However, I would like to ask the followings:
The overall code appears to be overly complex compared what you need.
- Do you really need to revive "CPUFREQ POLICY" events for this?
especially when you are going to look at "first CPU" only?


Cheers,
MyungJoo

RE: [PATCH 2/2] PM / devfreq: Generic cpufreq governor

[MyungJoo Ham]

>Many CPU architectures have caches that can scale independent of the CPUs.
>Frequency scaling of the caches is necessary to make sure the cache is not
>a performance bottleneck that leads to poor performance and power. The same
>idea applies for RAM/DDR.
>
>To achieve this, this patch series adds a generic devfreq governor that can
>listen to the frequency transitions of each CPU frequency domain and then
>adjusts the frequency of the cache (or any devfreq device) based on the
>frequency of the CPUs.

I agree that we have some hardware pieces that want to configure
frequencies based on the CPUfreq.

Creating a devfreq governor that configures devfreq-freq
based on incoming events (CPUFreq-transition-event in this case)
is indeed a good idea.

However, I would like to ask the followings:
The overall code appears to be overly complex compared what you need.
- Do you really need to revive "CPUFREQ POLICY" events for this?
especially when you are going to look at "first CPU" only?


Cheers,
MyungJoo

RE: [PATCH 08/11] PM / devfreq: Make update_devfreq() public

[MyungJoo Ham]

>Currently update_devfreq() is only visible to devfreq governors outside
>of devfreq.c. Make it public to allow drivers that adjust devfreq policies
>to cause a re-evaluation of the frequency after a policy change.
>
>Signed-off-by: Matthias Kaehlcke <m...@chromium.org>
>---
> drivers/devfreq/governor.h | 3 ---
> include/linux/devfreq.h| 8 
> 2 files changed, 8 insertions(+), 3 deletions(-)

With the requirement from patch 9/11, this commit is reasonable enough.

Acked-by: MyungJoo Ham <myungjoo@samsung.com>

RE: [PATCH 08/11] PM / devfreq: Make update_devfreq() public

[MyungJoo Ham]

>Currently update_devfreq() is only visible to devfreq governors outside
>of devfreq.c. Make it public to allow drivers that adjust devfreq policies
>to cause a re-evaluation of the frequency after a policy change.
>
>Signed-off-by: Matthias Kaehlcke 
>---
> drivers/devfreq/governor.h | 3 ---
> include/linux/devfreq.h| 8 
> 2 files changed, 8 insertions(+), 3 deletions(-)

With the requirement from patch 9/11, this commit is reasonable enough.

Acked-by: MyungJoo Ham 

RE: [PATCH 07/11] PM / devfreg: Add support policy notifiers

[MyungJoo Ham]

>Policy notifiers are called before a frequency change and may narrow
>the min/max frequency range in devfreq_policy, which is used to adjust
>the target frequency if it is beyond this range.
>
>Also add a few helpers:
> - devfreq_verify_within_[dev_]limits()
>- should be used by the notifiers for policy adjustments.
> - dev_to_devfreq()
>- lookup a devfreq strict from a device pointer
>
>Signed-off-by: Matthias Kaehlcke 
>---
> drivers/devfreq/devfreq.c | 47 +---
> include/linux/devfreq.h   | 66 +++
> 2 files changed, 102 insertions(+), 11 deletions(-)

Hello Matthias,


Why should we have yet another notifier from an instance of devfreq?
Wouldn't it better to let the current notifier (transition notifier)
handle new events as well by adding possible event states to it?

Anyway, is this the reason why you've separated some data of devfreq
into "policy" struct? (I was wondering why while reading commit 6/11).


Cheers
MyungJoo

RE: [PATCH 07/11] PM / devfreg: Add support policy notifiers

[MyungJoo Ham]

>Policy notifiers are called before a frequency change and may narrow
>the min/max frequency range in devfreq_policy, which is used to adjust
>the target frequency if it is beyond this range.
>
>Also add a few helpers:
> - devfreq_verify_within_[dev_]limits()
>- should be used by the notifiers for policy adjustments.
> - dev_to_devfreq()
>- lookup a devfreq strict from a device pointer
>
>Signed-off-by: Matthias Kaehlcke 
>---
> drivers/devfreq/devfreq.c | 47 +---
> include/linux/devfreq.h   | 66 +++
> 2 files changed, 102 insertions(+), 11 deletions(-)

Hello Matthias,


Why should we have yet another notifier from an instance of devfreq?
Wouldn't it better to let the current notifier (transition notifier)
handle new events as well by adding possible event states to it?

Anyway, is this the reason why you've separated some data of devfreq
into "policy" struct? (I was wondering why while reading commit 6/11).


Cheers
MyungJoo

RE: [PATCH 05/11] PM / devfreq: governors: Return device frequency limits instead of user limits

[MyungJoo Ham]

>The performance, powersave and simpleondemand governors can return
>df->min/max_freq, which are the user defined frequency limits.
>update_devfreq() already takes care of adjusting the target frequency
>with the user limits if necessary, therefore we can return
>df->scaling_min/max_freq instead, which is the min/max frequency
>supported by the device at a given time (depending on the
>enabled/disabled OPPs)
>
>Signed-off-by: Matthias Kaehlcke 
>---
> drivers/devfreq/governor_performance.c| 2 +-
> drivers/devfreq/governor_powersave.c  | 2 +-
> drivers/devfreq/governor_simpleondemand.c | 6 +++---
> 3 files changed, 5 insertions(+), 5 deletions(-)
>

Actually, even scaling_max_freq and scaling_min_freq are
covered centerally at devfreq.c:update_devfreq();

Wouldn't it be sufficient to return UINT_MAX for performance
and return UINT_MIN (0) for powersave, if the purpose is to
remove redundancy?

In the same sense, we may return UINT_MAX for freq-increasing
case for simpleondemand as well, because they are filtered
centrally anyway.

(This commit might be better merged to 4/11 in that case as well.)


Cheers,
MyungJoo

RE: [PATCH 05/11] PM / devfreq: governors: Return device frequency limits instead of user limits

[MyungJoo Ham]

>The performance, powersave and simpleondemand governors can return
>df->min/max_freq, which are the user defined frequency limits.
>update_devfreq() already takes care of adjusting the target frequency
>with the user limits if necessary, therefore we can return
>df->scaling_min/max_freq instead, which is the min/max frequency
>supported by the device at a given time (depending on the
>enabled/disabled OPPs)
>
>Signed-off-by: Matthias Kaehlcke 
>---
> drivers/devfreq/governor_performance.c| 2 +-
> drivers/devfreq/governor_powersave.c  | 2 +-
> drivers/devfreq/governor_simpleondemand.c | 6 +++---
> 3 files changed, 5 insertions(+), 5 deletions(-)
>

Actually, even scaling_max_freq and scaling_min_freq are
covered centerally at devfreq.c:update_devfreq();

Wouldn't it be sufficient to return UINT_MAX for performance
and return UINT_MIN (0) for powersave, if the purpose is to
remove redundancy?

In the same sense, we may return UINT_MAX for freq-increasing
case for simpleondemand as well, because they are filtered
centrally anyway.

(This commit might be better merged to 4/11 in that case as well.)


Cheers,
MyungJoo

RE: [PATCH 04/11] PM / devfreq: Remove redundant frequency adjustment from governors

[MyungJoo Ham]

> The userspace and simpleondemand governor determine a target frequency and
> then adjust it according to the df->min/max_freq limits that might have
> been set by user space. This adjustment is redundant, it is done in
> update_devfreq() for any governor, right after returning from
> governor->get_target_freq().
> 
> Signed-off-by: Matthias Kaehlcke <m...@chromium.org>
> ---
>  drivers/devfreq/governor_simpleondemand.c |  5 -
>  drivers/devfreq/governor_userspace.c  | 16 
>  2 files changed, 4 insertions(+), 17 deletions(-)
> 

Yes, indeed. Governors are no longer required to be aware of min/max freq.

Acked-by: MyungJoo Ham <myungjoo@samsung.com>

RE: [PATCH 04/11] PM / devfreq: Remove redundant frequency adjustment from governors

[MyungJoo Ham]

> The userspace and simpleondemand governor determine a target frequency and
> then adjust it according to the df->min/max_freq limits that might have
> been set by user space. This adjustment is redundant, it is done in
> update_devfreq() for any governor, right after returning from
> governor->get_target_freq().
> 
> Signed-off-by: Matthias Kaehlcke 
> ---
>  drivers/devfreq/governor_simpleondemand.c |  5 -
>  drivers/devfreq/governor_userspace.c  | 16 
>  2 files changed, 4 insertions(+), 17 deletions(-)
> 

Yes, indeed. Governors are no longer required to be aware of min/max freq.

Acked-by: MyungJoo Ham 

RE: [PATCH 03/11] PM / devfreq: Remove check for df->max_freq == 0 from governors

[MyungJoo Ham]

>Commit "PM / devfreq: Fix handling of min/max_freq == 0" ensures that
>df->max_freq is not 0, remove unnecessary checks.
>
>Signed-off-by: Matthias Kaehlcke <m...@chromium.org>
>---
> drivers/devfreq/governor_performance.c| 5 +
> drivers/devfreq/governor_simpleondemand.c | 7 +++
> 2 files changed, 4 insertions(+), 8 deletions(-)
>
>diff --git a/drivers/devfreq/governor_performance.c 
>b/drivers/devfreq/governor_performance.c
>index 4d23ecfbd948..1c990cb45098 100644
>

Thanks!

Acked-by: MyungJoo Ham <myungjoo@samsung.com>

RE: [PATCH 03/11] PM / devfreq: Remove check for df->max_freq == 0 from governors

[MyungJoo Ham]

>Commit "PM / devfreq: Fix handling of min/max_freq == 0" ensures that
>df->max_freq is not 0, remove unnecessary checks.
>
>Signed-off-by: Matthias Kaehlcke 
>---
> drivers/devfreq/governor_performance.c| 5 +
> drivers/devfreq/governor_simpleondemand.c | 7 +++
> 2 files changed, 4 insertions(+), 8 deletions(-)
>
>diff --git a/drivers/devfreq/governor_performance.c 
>b/drivers/devfreq/governor_performance.c
>index 4d23ecfbd948..1c990cb45098 100644
>

Thanks!

Acked-by: MyungJoo Ham 

RE: [PATCH 02/11] PM / devfreq: Fix handling of min/max_freq == 0

[MyungJoo Ham]

> Commit ab8f58ad72c4 ("PM / devfreq: Set min/max_freq when adding the
> devfreq device") initializes df->min/max_freq with the min/max OPP when
> the device is added. Later commit f1d981eaecf8 ("PM / devfreq: Use the
> available min/max frequency") adds df->scaling_min/max_freq and the
> following to the frequency adjustment code:
> 
>   max_freq = MIN(devfreq->scaling_max_freq, devfreq->max_freq);
> 
> With the current handling of min/max_freq this is incorrect:
> 
> Even though df->max_freq is now initialized to a value != 0 user space
> can still set it to 0, in this case max_freq would be 0 instead of
> df->scaling_max_freq as intended. In consequence the frequency adjustment
> is not performed:
> 
>   if (max_freq && freq > max_freq) {
>   freq = max_freq;
> 
> To fix this set df->min/max freq to the min/max OPP in max/max_freq_store,
> when the user passes a value of 0. This also prevents df->max_freq from
> being set below the min OPP when df->min_freq is 0, and similar for
> min_freq. Since it is now guaranteed that df->min/max_freq can't be 0 the
> checks for this case can be removed.
> 
> Fixes: f1d981eaecf8 ("PM / devfreq: Use the available min/max frequency")
> Signed-off-by: Matthias Kaehlcke <m...@chromium.org>
> ---
>  drivers/devfreq/devfreq.c | 30 ++
>  1 file changed, 18 insertions(+), 12 deletions(-)

Thanks a lot! Nice Catch.

Acked-by: MyungJoo Ham <myunngjoo@samsung.com>

Cheers,
MyungJoo.

RE: [PATCH 02/11] PM / devfreq: Fix handling of min/max_freq == 0

[MyungJoo Ham]

> Commit ab8f58ad72c4 ("PM / devfreq: Set min/max_freq when adding the
> devfreq device") initializes df->min/max_freq with the min/max OPP when
> the device is added. Later commit f1d981eaecf8 ("PM / devfreq: Use the
> available min/max frequency") adds df->scaling_min/max_freq and the
> following to the frequency adjustment code:
> 
>   max_freq = MIN(devfreq->scaling_max_freq, devfreq->max_freq);
> 
> With the current handling of min/max_freq this is incorrect:
> 
> Even though df->max_freq is now initialized to a value != 0 user space
> can still set it to 0, in this case max_freq would be 0 instead of
> df->scaling_max_freq as intended. In consequence the frequency adjustment
> is not performed:
> 
>   if (max_freq && freq > max_freq) {
>   freq = max_freq;
> 
> To fix this set df->min/max freq to the min/max OPP in max/max_freq_store,
> when the user passes a value of 0. This also prevents df->max_freq from
> being set below the min OPP when df->min_freq is 0, and similar for
> min_freq. Since it is now guaranteed that df->min/max_freq can't be 0 the
> checks for this case can be removed.
> 
> Fixes: f1d981eaecf8 ("PM / devfreq: Use the available min/max frequency")
> Signed-off-by: Matthias Kaehlcke 
> ---
>  drivers/devfreq/devfreq.c | 30 ++
>  1 file changed, 18 insertions(+), 12 deletions(-)

Thanks a lot! Nice Catch.

Acked-by: MyungJoo Ham 

Cheers,
MyungJoo.

RE: [PATCH v4 0/6] devfreq: rk3399_dmc: improve rk3399_dmc driver and it's documentation

[MyungJoo Ham]

>Dear all,
>
>These patches is an attempt to improve a little bit the rk3399_dmc
>driver and it's documentation in order to have all in a better shape for
>a future work I am doing. My final intention is add ddrfreq support for
>rockchip drm driver, but the patches for this are still
>work-in-progress. So let's start with this first patchset that is
>basically some fixes/improvements for the rk3399_dmc driver.
>
>The patches apply on top of current 4.17-rc3.
>
>Best regards,
> Enric
>

Dear Enric,

It appears that this patchset is ready to be applied.

Is "dt-binding" part going to be merged separatedly?
Or may I handle all the 6 commits? (assuming the three dt-binding got acks from 
dt maintainers)

Cheers,
MyungJoo

RE: [PATCH v4 0/6] devfreq: rk3399_dmc: improve rk3399_dmc driver and it's documentation

[MyungJoo Ham]

>Dear all,
>
>These patches is an attempt to improve a little bit the rk3399_dmc
>driver and it's documentation in order to have all in a better shape for
>a future work I am doing. My final intention is add ddrfreq support for
>rockchip drm driver, but the patches for this are still
>work-in-progress. So let's start with this first patchset that is
>basically some fixes/improvements for the rk3399_dmc driver.
>
>The patches apply on top of current 4.17-rc3.
>
>Best regards,
> Enric
>

Dear Enric,

It appears that this patchset is ready to be applied.

Is "dt-binding" part going to be merged separatedly?
Or may I handle all the 6 commits? (assuming the three dt-binding got acks from 
dt maintainers)

Cheers,
MyungJoo

RE: [PATCH v4 3/6] devfreq: rk3399_dmc: remove wait for dcf irq event.

[MyungJoo Ham]

>We have already wait dcf done in ATF, so don't need wait dcf irq
>in kernel, besides, clear dcf irq in kernel will import competiton
>between kernel and ATF, only handle dcf irq in ATF is a better way.
>
>Signed-off-by: Lin Huang <h...@rock-chips.com>
>Signed-off-by: Enric Balletbo i Serra <enric.balle...@collabora.com>
>Reviewed-by: Chanwoo Choi <cw00.c...@samsung.com>
>---
>
>Changes in v4: None
>Changes in v3: None
>Changes in v2:
>- [3/6] Add Reviewed-by Chanwoo Choi
>
> drivers/devfreq/rk3399_dmc.c | 53 +---
> 1 file changed, 1 insertion(+), 52 deletions(-)
>

Acked-by: MyungJoo Ham <myungjoo@samsung.com>

RE: [PATCH v4 3/6] devfreq: rk3399_dmc: remove wait for dcf irq event.

[MyungJoo Ham]

>We have already wait dcf done in ATF, so don't need wait dcf irq
>in kernel, besides, clear dcf irq in kernel will import competiton
>between kernel and ATF, only handle dcf irq in ATF is a better way.
>
>Signed-off-by: Lin Huang 
>Signed-off-by: Enric Balletbo i Serra 
>Reviewed-by: Chanwoo Choi 
>---
>
>Changes in v4: None
>Changes in v3: None
>Changes in v2:
>- [3/6] Add Reviewed-by Chanwoo Choi
>
> drivers/devfreq/rk3399_dmc.c | 53 +---
> 1 file changed, 1 insertion(+), 52 deletions(-)
>

Acked-by: MyungJoo Ham 

RE: [PATCH v4 6/6] devfreq: rk3399_dmc: fix spelling mistakes.

[MyungJoo Ham]

>Fix some spelling mistakes in error and debug messages.
>
>Signed-off-by: Enric Balletbo i Serra <enric.balle...@collabora.com>
>---
>
>Changes in v4:
>- [6/6] Introduce this new patch that fixes some spelling.
>
>Changes in v3: None
>Changes in v2: None
>
> drivers/devfreq/rk3399_dmc.c | 12 ++--
> 1 file changed, 6 insertions(+), 6 deletions(-)

Acked-by: MyungJoo Ham <myungjoo@samsung.com>

RE: [PATCH v4 6/6] devfreq: rk3399_dmc: fix spelling mistakes.

[MyungJoo Ham]

>Fix some spelling mistakes in error and debug messages.
>
>Signed-off-by: Enric Balletbo i Serra 
>---
>
>Changes in v4:
>- [6/6] Introduce this new patch that fixes some spelling.
>
>Changes in v3: None
>Changes in v2: None
>
> drivers/devfreq/rk3399_dmc.c | 12 ++--
> 1 file changed, 6 insertions(+), 6 deletions(-)

Acked-by: MyungJoo Ham 

RE: Re: [PATCH 1/3] PM / devfreq: Actually support providing freq_table

[MyungJoo Ham]

>On Mon 23 Apr 19:48 PDT 2018, Chanwoo Choi wrote:
>
>> Hi,
>> 
>> On 2018??? 04??? 24??? 09:20, Bjorn Andersson wrote:
>> > The code in devfreq_add_device() handles the case where a freq_table is
>> > passed by the client, but then requests min and max frequences from
>> > the, in this case absent, opp tables.
>> > 
>> > Read the min and max frequencies from the frequency table, which has
>> > been built from the opp table if one exists, instead of querying the
>> > opp table.
>> > 
>> > Signed-off-by: Bjorn Andersson 
>> > ---
>> > 
>> > An alternative approach is to clarify in the devfreq code that it's not
>> > possible to pass a freq_table and then in patch 3 create an opp table for 
>> > the
>> > device in runtime; although the error handling of this becomes non-trivial.
>> > 
>> > Transitioning the UFSHCD to use opp tables directly is hindered by the fact
>> > that the Qualcomm UFS hardware has two different clocks that needs to be
>> > running at different rates, so we would need a way to describe the two 
>> > rates in
>> > the opp table. (And would force us to change the DT binding)
>> > 
>> >  drivers/devfreq/devfreq.c | 22 --
>> >  1 file changed, 4 insertions(+), 18 deletions(-)
>> > 
>> > diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
>> > index fe2af6aa88fc..086ced50a13d 100644
>> > --- a/drivers/devfreq/devfreq.c
>> > +++ b/drivers/devfreq/devfreq.c
>> > @@ -74,30 +74,16 @@ static struct devfreq *find_device_devfreq(struct 
>> > device *dev)
>> >  
>> >  static unsigned long find_available_min_freq(struct devfreq *devfreq)
>> >  {
>> > -  struct dev_pm_opp *opp;
>> > -  unsigned long min_freq = 0;
>> > -
>> > -  opp = dev_pm_opp_find_freq_ceil(devfreq->dev.parent, _freq);
>> > -  if (IS_ERR(opp))
>> > -  min_freq = 0;
>> > -  else
>> > -  dev_pm_opp_put(opp);
>> > +  struct devfreq_dev_profile *profile = devfreq->profile;
>> >  
>> > -  return min_freq;
>> > +  return profile->freq_table[0];
>> 
>> It is wrong. The thermal framework support the devfreq-cooling device
>> which uses the dev_pm_opp_enable/disable().
>> 
>
>Okay, that makes sense. So rather than registering a custom freq_table I
>should register the min and max frequency using dev_pm_opp_add().
>
>> In order to find the correct available min frequency,
>> the devfreq have to use the OPP function instead of using the first entry
>> of the freq_table array.
>> 
>
>Based on this there seems to be room for cleaning out the freq_table
>from devfreq, to reduce the confusion. I will review this further.

Could you please check if the bug suffering you gets resolved by
replacing 0 with ULONG_MAX in the function find_available_max_freq?

-max_freq = 0;
+max_freq = ULONG_MAX;

Even if you are not using OPP, these functions should provide somewhat
"compatible" values.

Cheers,
MyungJoo


>
>Thanks,
>Bjorn
>

RE: Re: [PATCH 1/3] PM / devfreq: Actually support providing freq_table

[MyungJoo Ham]

>On Mon 23 Apr 19:48 PDT 2018, Chanwoo Choi wrote:
>
>> Hi,
>> 
>> On 2018??? 04??? 24??? 09:20, Bjorn Andersson wrote:
>> > The code in devfreq_add_device() handles the case where a freq_table is
>> > passed by the client, but then requests min and max frequences from
>> > the, in this case absent, opp tables.
>> > 
>> > Read the min and max frequencies from the frequency table, which has
>> > been built from the opp table if one exists, instead of querying the
>> > opp table.
>> > 
>> > Signed-off-by: Bjorn Andersson 
>> > ---
>> > 
>> > An alternative approach is to clarify in the devfreq code that it's not
>> > possible to pass a freq_table and then in patch 3 create an opp table for 
>> > the
>> > device in runtime; although the error handling of this becomes non-trivial.
>> > 
>> > Transitioning the UFSHCD to use opp tables directly is hindered by the fact
>> > that the Qualcomm UFS hardware has two different clocks that needs to be
>> > running at different rates, so we would need a way to describe the two 
>> > rates in
>> > the opp table. (And would force us to change the DT binding)
>> > 
>> >  drivers/devfreq/devfreq.c | 22 --
>> >  1 file changed, 4 insertions(+), 18 deletions(-)
>> > 
>> > diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
>> > index fe2af6aa88fc..086ced50a13d 100644
>> > --- a/drivers/devfreq/devfreq.c
>> > +++ b/drivers/devfreq/devfreq.c
>> > @@ -74,30 +74,16 @@ static struct devfreq *find_device_devfreq(struct 
>> > device *dev)
>> >  
>> >  static unsigned long find_available_min_freq(struct devfreq *devfreq)
>> >  {
>> > -  struct dev_pm_opp *opp;
>> > -  unsigned long min_freq = 0;
>> > -
>> > -  opp = dev_pm_opp_find_freq_ceil(devfreq->dev.parent, _freq);
>> > -  if (IS_ERR(opp))
>> > -  min_freq = 0;
>> > -  else
>> > -  dev_pm_opp_put(opp);
>> > +  struct devfreq_dev_profile *profile = devfreq->profile;
>> >  
>> > -  return min_freq;
>> > +  return profile->freq_table[0];
>> 
>> It is wrong. The thermal framework support the devfreq-cooling device
>> which uses the dev_pm_opp_enable/disable().
>> 
>
>Okay, that makes sense. So rather than registering a custom freq_table I
>should register the min and max frequency using dev_pm_opp_add().
>
>> In order to find the correct available min frequency,
>> the devfreq have to use the OPP function instead of using the first entry
>> of the freq_table array.
>> 
>
>Based on this there seems to be room for cleaning out the freq_table
>from devfreq, to reduce the confusion. I will review this further.

Could you please check if the bug suffering you gets resolved by
replacing 0 with ULONG_MAX in the function find_available_max_freq?

-max_freq = 0;
+max_freq = ULONG_MAX;

Even if you are not using OPP, these functions should provide somewhat
"compatible" values.

Cheers,
MyungJoo


>
>Thanks,
>Bjorn
>

RE: [PATCH 6/6] devfreq: rk3399_dmc: register devfreq notification to dmc driver.

[MyungJoo Ham]

>From: Lin Huang 
>
>Because dmc may also access the PMU_BUS_IDLE_REQ register, we need to
>ensure that the pd driver and the dmc driver will not access at this
>register at the same time.
>
>Signed-off-by: Lin Huang 
>Signed-off-by: Enric Balletbo i Serra 
>---
>
> drivers/devfreq/rk3399_dmc.c  | 47 +--
> drivers/soc/rockchip/pm_domains.c | 31 +++
> include/soc/rockchip/rk3399_dmc.h | 63 +++
> 3 files changed, 96 insertions(+), 45 deletions(-)
> create mode 100644 include/soc/rockchip/rk3399_dmc.h
>
>diff --git a/drivers/devfreq/rk3399_dmc.c b/drivers/devfreq/rk3399_dmc.c
>index 5bfca028eaaf..a1f320634d69 100644
>--- a/drivers/devfreq/rk3399_dmc.c
>+++ b/drivers/devfreq/rk3399_dmc.c
[]
>diff --git a/drivers/soc/rockchip/pm_domains.c 
>b/drivers/soc/rockchip/pm_domains.c
>index 53efc386b1ad..7acc836e7eb7 100644
>--- a/drivers/soc/rockchip/pm_domains.c
>+++ b/drivers/soc/rockchip/pm_domains.c
[]
>+static int dmc_notify(struct notifier_block *nb, unsigned long event,
>+void *data)
>+{
>+  if (event == DEVFREQ_PRECHANGE)
>+  mutex_lock(_pmu->mutex);
>+  else if (event == DEVFREQ_POSTCHANGE)
>+  mutex_unlock(_pmu->mutex);
>+
>+  return NOTIFY_OK;
>+}
>+

Doesn't this incur a deadlock?

1. devfreq.c:update_freq calls devfreq_notify_transition(DEVFREQ_PRECHANGE)
2. pm_domain.c:dmc_notify calls mutex_lock(dmc_pmu->mutex)
3. devfreq.c:update_freq calls target callback
4. rk3399_dmc.c:rk3399_dmcfreq_target calls mutex_lock(>lock)
   >> update_freq cannot proceed. 


Cheers,
MyungJoo

RE: [PATCH 6/6] devfreq: rk3399_dmc: register devfreq notification to dmc driver.

[MyungJoo Ham]

>From: Lin Huang 
>
>Because dmc may also access the PMU_BUS_IDLE_REQ register, we need to
>ensure that the pd driver and the dmc driver will not access at this
>register at the same time.
>
>Signed-off-by: Lin Huang 
>Signed-off-by: Enric Balletbo i Serra 
>---
>
> drivers/devfreq/rk3399_dmc.c  | 47 +--
> drivers/soc/rockchip/pm_domains.c | 31 +++
> include/soc/rockchip/rk3399_dmc.h | 63 +++
> 3 files changed, 96 insertions(+), 45 deletions(-)
> create mode 100644 include/soc/rockchip/rk3399_dmc.h
>
>diff --git a/drivers/devfreq/rk3399_dmc.c b/drivers/devfreq/rk3399_dmc.c
>index 5bfca028eaaf..a1f320634d69 100644
>--- a/drivers/devfreq/rk3399_dmc.c
>+++ b/drivers/devfreq/rk3399_dmc.c
[]
>diff --git a/drivers/soc/rockchip/pm_domains.c 
>b/drivers/soc/rockchip/pm_domains.c
>index 53efc386b1ad..7acc836e7eb7 100644
>--- a/drivers/soc/rockchip/pm_domains.c
>+++ b/drivers/soc/rockchip/pm_domains.c
[]
>+static int dmc_notify(struct notifier_block *nb, unsigned long event,
>+void *data)
>+{
>+  if (event == DEVFREQ_PRECHANGE)
>+  mutex_lock(_pmu->mutex);
>+  else if (event == DEVFREQ_POSTCHANGE)
>+  mutex_unlock(_pmu->mutex);
>+
>+  return NOTIFY_OK;
>+}
>+

Doesn't this incur a deadlock?

1. devfreq.c:update_freq calls devfreq_notify_transition(DEVFREQ_PRECHANGE)
2. pm_domain.c:dmc_notify calls mutex_lock(dmc_pmu->mutex)
3. devfreq.c:update_freq calls target callback
4. rk3399_dmc.c:rk3399_dmcfreq_target calls mutex_lock(>lock)
   >> update_freq cannot proceed. 


Cheers,
MyungJoo

RE: [PATCH 5/6] devfreq: rk3399_dmc: do not print error when get supply and clk defer.

[MyungJoo Ham]

> From: Lin Huang <h...@rock-chips.com>
> 
> We just return -EPROBE_DEFER error code to caller and do not
> print error message when try to get center logic regulator
> and DMC clock defer.
> 
> Signed-off-by: Lin Huang <h...@rock-chips.com>
> Signed-off-by: Enric Balletbo i Serra <enric.balle...@collabora.com>

Acked-by: MyungJoo Ham <myungjoo@samsung.com>

I'll wait for the rebase of 3/6.

> ---
> 
>  drivers/devfreq/rk3399_dmc.c | 6 ++
>  1 file changed, 6 insertions(+)
> 

RE: [PATCH 5/6] devfreq: rk3399_dmc: do not print error when get supply and clk defer.

[MyungJoo Ham]

> From: Lin Huang 
> 
> We just return -EPROBE_DEFER error code to caller and do not
> print error message when try to get center logic regulator
> and DMC clock defer.
> 
> Signed-off-by: Lin Huang 
> Signed-off-by: Enric Balletbo i Serra 

Acked-by: MyungJoo Ham 

I'll wait for the rebase of 3/6.

> ---
> 
>  drivers/devfreq/rk3399_dmc.c | 6 ++
>  1 file changed, 6 insertions(+)
> 

RE: [PATCH 3/6] devfreq: rk3399_dmc: remove wait for dcf irq event.

[MyungJoo Ham]

> We have already wait dcf done in ATF, so don't need wait dcf irq
> in kernel, besides, clear dcf irq in kernel will import competiton
> between kernel and ATF, only handle dcf irq in ATF is a better way.
> 
> Signed-off-by: Lin Huang 
> Signed-off-by: Enric Balletbo i Serra 

This does not apply to 4.17-rc2 or a little less recent rc's.

Would you please rebase to the latest?


Cheers,
MyungJoo

> ---
> 
>  drivers/devfreq/rk3399_dmc.c | 53 +---
>  1 file changed, 1 insertion(+), 52 deletions(-)
> 
> diff --git a/drivers/devfreq/rk3399_dmc.c b/drivers/devfreq/rk3399_dmc.c
> index 5dfbfa3cc878..44a379657cd5 100644
> 

RE: [PATCH 3/6] devfreq: rk3399_dmc: remove wait for dcf irq event.

[MyungJoo Ham]

> We have already wait dcf done in ATF, so don't need wait dcf irq
> in kernel, besides, clear dcf irq in kernel will import competiton
> between kernel and ATF, only handle dcf irq in ATF is a better way.
> 
> Signed-off-by: Lin Huang 
> Signed-off-by: Enric Balletbo i Serra 

This does not apply to 4.17-rc2 or a little less recent rc's.

Would you please rebase to the latest?


Cheers,
MyungJoo

> ---
> 
>  drivers/devfreq/rk3399_dmc.c | 53 +---
>  1 file changed, 1 insertion(+), 52 deletions(-)
> 
> diff --git a/drivers/devfreq/rk3399_dmc.c b/drivers/devfreq/rk3399_dmc.c
> index 5dfbfa3cc878..44a379657cd5 100644
> 

RE: [PATCH 1/6] dt-bindings: devfreq: rk3399_dmc: improve binding documentation.

[MyungJoo Ham]

>From: Nick Milner <nick.mil...@collabora.com>
>
>There are several typos, references to non existent files, grammar and
>punctuation mistakes in the rk3399_dmc.txt binding. This patch tries
>to improve the binding documentation and fix these mistakes.
>
>Signed-off-by: Nick Milner <nick.mil...@collabora.com>
>Signed-off-by: Enric Balletbo i Serra <enric.balle...@collabora.com>

Acked-by: MyungJoo Ham <myungjoo@samsung.com>

CC (The original author of this doc): Lin Huang <h...@rock-chips.com>


Cheers,
MyungJoo.

>---
>
> .../bindings/devfreq/rk3399_dmc.txt   | 207 +-
> 1 file changed, 105 insertions(+), 102 deletions(-)
>
>diff --git a/Documentation/devicetree/bindings/devfreq/rk3399_dmc.txt 
>b/Documentation/devicetree/bindings/devfreq/rk3399_dmc.txt

RE: [PATCH 1/6] dt-bindings: devfreq: rk3399_dmc: improve binding documentation.

[MyungJoo Ham]

>From: Nick Milner 
>
>There are several typos, references to non existent files, grammar and
>punctuation mistakes in the rk3399_dmc.txt binding. This patch tries
>to improve the binding documentation and fix these mistakes.
>
>Signed-off-by: Nick Milner 
>Signed-off-by: Enric Balletbo i Serra 

Acked-by: MyungJoo Ham 

CC (The original author of this doc): Lin Huang 


Cheers,
MyungJoo.

>---
>
> .../bindings/devfreq/rk3399_dmc.txt   | 207 +-
> 1 file changed, 105 insertions(+), 102 deletions(-)
>
>diff --git a/Documentation/devicetree/bindings/devfreq/rk3399_dmc.txt 
>b/Documentation/devicetree/bindings/devfreq/rk3399_dmc.txt