[PATCH V2 RESEND 1/4] PM / devfreq: Use more accurate returned new_freq as resume_freq

[Dong Aisheng]

Use the more accurate returned new_freq as resume_freq.
It's the same as how devfreq->previous_freq was updated.

Fixes: 83f8ca45afbf0 ("PM / devfreq: add support for suspend/resume of a
devfreq device")
Signed-off-by: Dong Aisheng 
---
 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 b6d3e7db0b09..85927bd7ee76 100644
--- a/drivers/devfreq/devfreq.c
+++ b/drivers/devfreq/devfreq.c
@@ -388,7 +388,7 @@ static int devfreq_set_target(struct devfreq *devfreq, 
unsigned long new_freq,
devfreq->previous_freq = new_freq;
 
if (devfreq->suspend_freq)
-   devfreq->resume_freq = cur_freq;
+   devfreq->resume_freq = new_freq;
 
return err;
 }
-- 
2.25.1

[PATCH V2 RESEND 3/4] PM / devfreq: bail out early if no freq changes in devfreq_set_target

[Dong Aisheng]

It's unnecessary to set the same freq again and run notifier calls.

Signed-off-by: Dong Aisheng 
---
 drivers/devfreq/devfreq.c | 9 ++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
index 85927bd7ee76..a6ee91dd17bd 100644
--- a/drivers/devfreq/devfreq.c
+++ b/drivers/devfreq/devfreq.c
@@ -352,13 +352,16 @@ static int devfreq_set_target(struct devfreq *devfreq, 
unsigned long new_freq,
 {
struct devfreq_freqs freqs;
unsigned long cur_freq;
-   int err = 0;
+   int err;
 
if (devfreq->profile->get_cur_freq)
devfreq->profile->get_cur_freq(devfreq->dev.parent, _freq);
else
cur_freq = devfreq->previous_freq;
 
+   if (new_freq == cur_freq)
+   return 0;
+
freqs.old = cur_freq;
freqs.new = new_freq;
devfreq_notify_transition(devfreq, , DEVFREQ_PRECHANGE);
@@ -375,7 +378,7 @@ static int devfreq_set_target(struct devfreq *devfreq, 
unsigned long new_freq,
 * and DEVFREQ_POSTCHANGE because for showing the correct frequency
 * change order of between devfreq device and passive devfreq device.
 */
-   if (trace_devfreq_frequency_enabled() && new_freq != cur_freq)
+   if (trace_devfreq_frequency_enabled())
trace_devfreq_frequency(devfreq, new_freq, cur_freq);
 
freqs.new = new_freq;
@@ -390,7 +393,7 @@ static int devfreq_set_target(struct devfreq *devfreq, 
unsigned long new_freq,
if (devfreq->suspend_freq)
devfreq->resume_freq = new_freq;
 
-   return err;
+   return 0;
 }
 
 /**
-- 
2.25.1

[PATCH V2 RESEND 4/4] PM / devfreq: imx8m-ddrc: remove imx8m_ddrc_get_dev_status

[Dong Aisheng]

Current driver actually does not support simple ondemand governor
as it's unable to provide device load information. So removing
the unnecessary callback to avoid confusing.
Right now the driver is using userspace governor by default.

polling_ms was also dropped as it's not needed for non-ondemand
governor.

Signed-off-by: Dong Aisheng 
---
 drivers/devfreq/imx8m-ddrc.c | 14 --
 1 file changed, 14 deletions(-)

diff --git a/drivers/devfreq/imx8m-ddrc.c b/drivers/devfreq/imx8m-ddrc.c
index bc82d3653bff..ecb9375aa877 100644
--- a/drivers/devfreq/imx8m-ddrc.c
+++ b/drivers/devfreq/imx8m-ddrc.c
@@ -280,18 +280,6 @@ static int imx8m_ddrc_get_cur_freq(struct device *dev, 
unsigned long *freq)
return 0;
 }
 
-static int imx8m_ddrc_get_dev_status(struct device *dev,
-struct devfreq_dev_status *stat)
-{
-   struct imx8m_ddrc *priv = dev_get_drvdata(dev);
-
-   stat->busy_time = 0;
-   stat->total_time = 0;
-   stat->current_frequency = clk_get_rate(priv->dram_core);
-
-   return 0;
-}
-
 static int imx8m_ddrc_init_freq_info(struct device *dev)
 {
struct imx8m_ddrc *priv = dev_get_drvdata(dev);
@@ -429,9 +417,7 @@ static int imx8m_ddrc_probe(struct platform_device *pdev)
if (ret < 0)
goto err;
 
-   priv->profile.polling_ms = 1000;
priv->profile.target = imx8m_ddrc_target;
-   priv->profile.get_dev_status = imx8m_ddrc_get_dev_status;
priv->profile.exit = imx8m_ddrc_exit;
priv->profile.get_cur_freq = imx8m_ddrc_get_cur_freq;
priv->profile.initial_freq = clk_get_rate(priv->dram_core);
-- 
2.25.1

[PATCH V2 RESEND 2/4] PM / devfreq: Remove the invalid description for get_target_freq

[Dong Aisheng]

First of all, no_central_polling was removed since
commit 7e6fdd4bad03 ("PM / devfreq: Core updates to support devices
which can idle")
Secondly, get_target_freq() is not only called only with update_devfreq()
notified by OPP now, but also min/max freq qos notifier.

So remove this invalid description now to avoid confusing.

Signed-off-by: Dong Aisheng 
---
 Documentation/ABI/testing/sysfs-class-devfreq | 5 +
 drivers/devfreq/governor.h| 2 --
 2 files changed, 1 insertion(+), 6 deletions(-)

diff --git a/Documentation/ABI/testing/sysfs-class-devfreq 
b/Documentation/ABI/testing/sysfs-class-devfreq
index 386bc230a33d..5e6b74f30406 100644
--- a/Documentation/ABI/testing/sysfs-class-devfreq
+++ b/Documentation/ABI/testing/sysfs-class-devfreq
@@ -97,10 +97,7 @@ Description:
object. The values are represented in ms. If the value is
less than 1 jiffy, it is considered to be 0, which means
no polling. This value is meaningless if the governor is
-   not polling; thus. If the governor is not using
-   devfreq-provided central polling
-   (/sys/class/devfreq/.../central_polling is 0), this value
-   may be useless.
+   not polling.
 
A list of governors that support the node:
- simple_ondmenad
diff --git a/drivers/devfreq/governor.h b/drivers/devfreq/governor.h
index 244634465170..2d69a0ce6291 100644
--- a/drivers/devfreq/governor.h
+++ b/drivers/devfreq/governor.h
@@ -57,8 +57,6 @@
  * Basically, get_target_freq will run
  * devfreq_dev_profile.get_dev_status() to get the
  * status of the device (load = busy_time / total_time).
- * If no_central_polling is set, this callback is called
- * only with update_devfreq() notified by OPP.
  * @event_handler:  Callback for devfreq core framework to notify events
  *  to governors. Events include per device governor
  *  init and exit, opp changes out of devfreq, suspend
-- 
2.25.1

[PATCH V2 RESEND 0/4] PM / devfreq: a few small fixes and improvements

[Dong Aisheng]

A few small fixes and improvements

ChangeLog:
v2 Resend:
 * rebase to devfreq-next
 * drop original patch 1 & 5.
   Patch 5 will be re-sent later when dependent patches merged.
v1->v2:
 * squash a few patches
 * rebase to devfreq-testing


Dong Aisheng (4):
  PM / devfreq: Use more accurate returned new_freq as resume_freq
  PM / devfreq: Remove the invalid description for get_target_freq
  PM / devfreq: bail out early if no freq changes in devfreq_set_target
  PM / devfreq: imx8m-ddrc: remove imx8m_ddrc_get_dev_status

 Documentation/ABI/testing/sysfs-class-devfreq |  5 +
 drivers/devfreq/devfreq.c | 11 +++
 drivers/devfreq/governor.h|  2 --
 drivers/devfreq/imx8m-ddrc.c  | 14 --
 4 files changed, 8 insertions(+), 24 deletions(-)

-- 
2.25.1

Re: [PATCH V2 0/6] PM / devfreq: a few small fixes and improvements

[Dong Aisheng]

Hi Chanwoo,

On Tue, Mar 23, 2021 at 11:13 AM Dong Aisheng  wrote:
>
> A few small fixes and improvements
>
> ChangeLog:
> v1->v2:
>  * squash a few patches
>  * rebase to devfreq-testing

I have to rebase to devfreq-testing instead of devfreq-next because
below two patches
only exist in devfreq-testing.
5cc75e9252e9 (devfreq/devfreq-testing) PM / devfreq: Add
devfreq_transitions debugfs file
dc9e557845c1 PM / devfreq: Add new up_threshold and down_differential
sysfs attrs
My patch 5 needs change based on it according to your suggestion. So i have to
rebase to that branch.

However, i found devfreq-testing can't build with GOV_PASSVIE enabled.
Patch 1 fixed it. You can squash to the original one when apply.

Please help take a look at this new series.
Thanks

Regards
Aisheng

>  * drop two patches which are already in devfreq-next
>
> Dong Aisheng (6):
>   PM / devfreq: fix build error when DEVFREQ_GOV_PASSIVE enabled
>   PM / devfreq: Use more accurate returned new_freq as resume_freq
>   PM / devfreq: Remove the invalid description for get_target_freq
>   PM / devfreq: bail out early if no freq changes in devfreq_set_target
>   PM / devfreq: governor: optimize simpleondemand get_target_freq
>   PM / devfreq: imx8m-ddrc: remove imx8m_ddrc_get_dev_status
>
>  Documentation/ABI/testing/sysfs-class-devfreq |  5 +--
>  drivers/devfreq/devfreq.c | 37 +++
>  drivers/devfreq/governor.h|  2 -
>  drivers/devfreq/governor_simpleondemand.c | 31 ++--
>  drivers/devfreq/imx8m-ddrc.c  | 14 ---
>  5 files changed, 35 insertions(+), 54 deletions(-)
>
> --
> 2.25.1
>

[PATCH V2 6/6] PM / devfreq: imx8m-ddrc: remove imx8m_ddrc_get_dev_status

[Dong Aisheng]

Current driver actually does not support simple ondemand governor
as it's unable to provide device load information. So removing
the unnecessary callback to avoid confusing.
Right now the driver is using userspace governor by default.

polling_ms was also dropped as it's not needed for non-ondemand
governor.

Signed-off-by: Dong Aisheng 
---
 drivers/devfreq/imx8m-ddrc.c | 14 --
 1 file changed, 14 deletions(-)

diff --git a/drivers/devfreq/imx8m-ddrc.c b/drivers/devfreq/imx8m-ddrc.c
index bc82d3653bff..ecb9375aa877 100644
--- a/drivers/devfreq/imx8m-ddrc.c
+++ b/drivers/devfreq/imx8m-ddrc.c
@@ -280,18 +280,6 @@ static int imx8m_ddrc_get_cur_freq(struct device *dev, 
unsigned long *freq)
return 0;
 }
 
-static int imx8m_ddrc_get_dev_status(struct device *dev,
-struct devfreq_dev_status *stat)
-{
-   struct imx8m_ddrc *priv = dev_get_drvdata(dev);
-
-   stat->busy_time = 0;
-   stat->total_time = 0;
-   stat->current_frequency = clk_get_rate(priv->dram_core);
-
-   return 0;
-}
-
 static int imx8m_ddrc_init_freq_info(struct device *dev)
 {
struct imx8m_ddrc *priv = dev_get_drvdata(dev);
@@ -429,9 +417,7 @@ static int imx8m_ddrc_probe(struct platform_device *pdev)
if (ret < 0)
goto err;
 
-   priv->profile.polling_ms = 1000;
priv->profile.target = imx8m_ddrc_target;
-   priv->profile.get_dev_status = imx8m_ddrc_get_dev_status;
priv->profile.exit = imx8m_ddrc_exit;
priv->profile.get_cur_freq = imx8m_ddrc_get_cur_freq;
priv->profile.initial_freq = clk_get_rate(priv->dram_core);
-- 
2.25.1

[PATCH V2 5/6] PM / devfreq: governor: optimize simpleondemand get_target_freq

[Dong Aisheng]

The device profile up_threshold/down_differential only needs to be
initialized once when calling devm_devfreq_add_device. It's unnecessary
to put the data check logic in the hot path (.get_target_freq()) where it
will be called all the time during polling. Instead, we only check and
initialize it one time during DEVFREQ_GOV_START.

This also helps check data validability in advance during DEVFREQ_GOV_START
rather than checking it later when running .get_target_freq().

Signed-off-by: Dong Aisheng 
---
Change Log:
v1->v2:
 * rebase to devfreq-testing
---
 drivers/devfreq/devfreq.c | 24 +-
 drivers/devfreq/governor_simpleondemand.c | 31 +++
 2 files changed, 26 insertions(+), 29 deletions(-)

diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
index cacda7d1f858..270e51f5318f 100644
--- a/drivers/devfreq/devfreq.c
+++ b/drivers/devfreq/devfreq.c
@@ -1908,9 +1908,7 @@ static ssize_t up_threshold_show(struct device *dev,
if (!df->profile)
return -EINVAL;
 
-   return sprintf(buf, "%d\n", df->profile->up_threshold
-   ? df->profile->up_threshold
-   : DEVFREQ_UP_THRESHOLD);
+   return sprintf(buf, "%d\n", df->profile->up_threshold);
 }
 
 static ssize_t up_threshold_store(struct device *dev,
@@ -1934,9 +1932,7 @@ static ssize_t up_threshold_store(struct device *dev,
if (value > 100)
value = 100;
 
-   down_differential = df->profile->down_differential
-   ? df->profile->down_differential
-   : DEVFREQ_DOWN_DIFFERENCTIAL;
+   down_differential = df->profile->down_differential;
if (value < down_differential)
value = down_differential;
 
@@ -1961,9 +1957,7 @@ static ssize_t down_differential_show(struct device *dev,
if (!df->profile)
return -EINVAL;
 
-   return sprintf(buf, "%d\n", df->profile->down_differential
-   ? df->profile->down_differential
-   : DEVFREQ_DOWN_DIFFERENCTIAL);
+   return sprintf(buf, "%d\n", df->profile->down_differential);
 }
 
 static ssize_t down_differential_store(struct device *dev,
@@ -1984,9 +1978,7 @@ static ssize_t down_differential_store(struct device *dev,
if (ret != 1)
return -EINVAL;
 
-   up_threshold = df->profile->up_threshold
-   ? df->profile->up_threshold
-   : DEVFREQ_UP_THRESHOLD;
+   up_threshold = df->profile->up_threshold;
if (value > up_threshold)
value = up_threshold;
 
@@ -2113,16 +2105,12 @@ static int devfreq_summary_show(struct seq_file *s, 
void *data)
polling_ms = 0;
 
if (IS_SUPPORTED_ATTR(devfreq->governor->attrs, UP_THRESHOLD))
-   up_threshold = devfreq->profile->up_threshold
-   ? devfreq->profile->up_threshold
-   : DEVFREQ_UP_THRESHOLD;
+   up_threshold = devfreq->profile->up_threshold;
else
up_threshold = 0;
 
if (IS_SUPPORTED_ATTR(devfreq->governor->attrs, DOWN_DIFF))
-   down_differential = devfreq->profile->down_differential
-   ? devfreq->profile->down_differential
-   : DEVFREQ_DOWN_DIFFERENCTIAL;
+   down_differential = devfreq->profile->down_differential;
else
down_differential = 0;
mutex_unlock(>lock);
diff --git a/drivers/devfreq/governor_simpleondemand.c 
b/drivers/devfreq/governor_simpleondemand.c
index 93f6061667d9..3e195b46554a 100644
--- a/drivers/devfreq/governor_simpleondemand.c
+++ b/drivers/devfreq/governor_simpleondemand.c
@@ -18,8 +18,8 @@ static int devfreq_simple_ondemand_func(struct devfreq *df,
int err;
struct devfreq_dev_status *stat;
unsigned long long a, b;
-   unsigned int dfso_upthreshold = DEVFREQ_UP_THRESHOLD;
-   unsigned int dfso_downdifferential = DEVFREQ_DOWN_DIFFERENCTIAL;
+   unsigned int dfso_upthreshold = df->profile->up_threshold;
+   unsigned int dfso_downdifferential = df->profile->down_differential;
 
err = devfreq_update_stats(df);
if (err)
@@ -27,15 +27,6 @@ static int devfreq_simple_ondemand_func(struct devfreq *df,
 
stat = >last_status;
 
-   if (df->profile->up_threshold)
-   dfso_upthreshold = df->profile->up_threshold;
-   if (df->profile->d

[PATCH V2 1/6] PM / devfreq: fix build error when DEVFREQ_GOV_PASSIVE enabled

[Dong Aisheng]

drivers/devfreq/devfreq.c: In function ‘devfreq_transitions_show’:
drivers/devfreq/devfreq.c:2188:25: error: ‘struct devfreq’ has no member named 
‘governor_name’; did you mean ‘governor’?
 2188 |   if (!strncmp(devfreq->governor_name,
  | ^
  | governor

Fixes: 5cc75e9252e9 ("PM / devfreq: Add devfreq_transitions debugfs file")
Signed-off-by: Dong Aisheng 
---
 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 d625b268adf2..107badfc6b2b 100644
--- a/drivers/devfreq/devfreq.c
+++ b/drivers/devfreq/devfreq.c
@@ -2185,7 +2185,7 @@ static int devfreq_transitions_show(struct seq_file *s, 
void *data)
continue;
 
 #if IS_ENABLED(CONFIG_DEVFREQ_GOV_PASSIVE)
-   if (!strncmp(devfreq->governor_name,
+   if (!strncmp(devfreq->governor->name,
DEVFREQ_GOV_PASSIVE, DEVFREQ_NAME_LEN)) {
struct devfreq_passive_data *data = devfreq->data;
 
-- 
2.25.1

[PATCH V2 3/6] PM / devfreq: Remove the invalid description for get_target_freq

[Dong Aisheng]

First of all, no_central_polling was removed since
commit 7e6fdd4bad03 ("PM / devfreq: Core updates to support devices
which can idle")
Secondly, get_target_freq() is not only called only with update_devfreq()
notified by OPP now, but also min/max freq qos notifier.

So remove this invalid description now to avoid confusing.

Signed-off-by: Dong Aisheng 
---
 Documentation/ABI/testing/sysfs-class-devfreq | 5 +
 drivers/devfreq/governor.h| 2 --
 2 files changed, 1 insertion(+), 6 deletions(-)

diff --git a/Documentation/ABI/testing/sysfs-class-devfreq 
b/Documentation/ABI/testing/sysfs-class-devfreq
index 22408cfa7ed2..e71595c84f22 100644
--- a/Documentation/ABI/testing/sysfs-class-devfreq
+++ b/Documentation/ABI/testing/sysfs-class-devfreq
@@ -97,10 +97,7 @@ Description:
object. The values are represented in ms. If the value is
less than 1 jiffy, it is considered to be 0, which means
no polling. This value is meaningless if the governor is
-   not polling; thus. If the governor is not using
-   devfreq-provided central polling
-   (/sys/class/devfreq/.../central_polling is 0), this value
-   may be useless.
+   not polling.
 
A list of governors that support the node:
- simple_ondmenad
diff --git a/drivers/devfreq/governor.h b/drivers/devfreq/governor.h
index 48736a2ae970..9d72f5b66bfa 100644
--- a/drivers/devfreq/governor.h
+++ b/drivers/devfreq/governor.h
@@ -67,8 +67,6 @@
  * Basically, get_target_freq will run
  * devfreq_dev_profile.get_dev_status() to get the
  * status of the device (load = busy_time / total_time).
- * If no_central_polling is set, this callback is called
- * only with update_devfreq() notified by OPP.
  * @event_handler:  Callback for devfreq core framework to notify events
  *  to governors. Events include per device governor
  *  init and exit, opp changes out of devfreq, suspend
-- 
2.25.1

[PATCH V2 0/6] PM / devfreq: a few small fixes and improvements

[Dong Aisheng]

A few small fixes and improvements

ChangeLog:
v1->v2:
 * squash a few patches
 * rebase to devfreq-testing
 * drop two patches which are already in devfreq-next

Dong Aisheng (6):
  PM / devfreq: fix build error when DEVFREQ_GOV_PASSIVE enabled
  PM / devfreq: Use more accurate returned new_freq as resume_freq
  PM / devfreq: Remove the invalid description for get_target_freq
  PM / devfreq: bail out early if no freq changes in devfreq_set_target
  PM / devfreq: governor: optimize simpleondemand get_target_freq
  PM / devfreq: imx8m-ddrc: remove imx8m_ddrc_get_dev_status

 Documentation/ABI/testing/sysfs-class-devfreq |  5 +--
 drivers/devfreq/devfreq.c | 37 +++
 drivers/devfreq/governor.h|  2 -
 drivers/devfreq/governor_simpleondemand.c | 31 ++--
 drivers/devfreq/imx8m-ddrc.c  | 14 ---
 5 files changed, 35 insertions(+), 54 deletions(-)

-- 
2.25.1

[PATCH V2 2/6] PM / devfreq: Use more accurate returned new_freq as resume_freq

[Dong Aisheng]

Use the more accurate returned new_freq as resume_freq.
It's the same as how devfreq->previous_freq was updated.

Fixes: 83f8ca45afbf0 ("PM / devfreq: add support for suspend/resume of a
devfreq device")
Signed-off-by: Dong Aisheng 
---
 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 107badfc6b2b..b537fd9602cd 100644
--- a/drivers/devfreq/devfreq.c
+++ b/drivers/devfreq/devfreq.c
@@ -443,7 +443,7 @@ static int devfreq_set_target(struct devfreq *devfreq, 
unsigned long new_freq,
devfreq->previous_freq = new_freq;
 
if (devfreq->suspend_freq)
-   devfreq->resume_freq = cur_freq;
+   devfreq->resume_freq = new_freq;
 
return err;
 }
-- 
2.25.1

[PATCH V2 4/6] PM / devfreq: bail out early if no freq changes in devfreq_set_target

[Dong Aisheng]

It's unnecessary to set the same freq again and run notifier calls.

Signed-off-by: Dong Aisheng 
---
 drivers/devfreq/devfreq.c | 9 ++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
index b537fd9602cd..cacda7d1f858 100644
--- a/drivers/devfreq/devfreq.c
+++ b/drivers/devfreq/devfreq.c
@@ -403,13 +403,16 @@ static int devfreq_set_target(struct devfreq *devfreq, 
unsigned long new_freq,
 {
struct devfreq_freqs freqs;
unsigned long cur_freq;
-   int err = 0;
+   int err;
 
if (devfreq->profile->get_cur_freq)
devfreq->profile->get_cur_freq(devfreq->dev.parent, _freq);
else
cur_freq = devfreq->previous_freq;
 
+   if (new_freq == cur_freq)
+   return 0;
+
freqs.old = cur_freq;
freqs.new = new_freq;
devfreq_notify_transition(devfreq, , DEVFREQ_PRECHANGE);
@@ -426,7 +429,7 @@ static int devfreq_set_target(struct devfreq *devfreq, 
unsigned long new_freq,
 * and DEVFREQ_POSTCHANGE because for showing the correct frequency
 * change order of between devfreq device and passive devfreq device.
 */
-   if (trace_devfreq_frequency_enabled() && new_freq != cur_freq)
+   if (trace_devfreq_frequency_enabled())
trace_devfreq_frequency(devfreq, new_freq, cur_freq);
 
devfreq_update_transitions(devfreq, cur_freq, new_freq,
@@ -445,7 +448,7 @@ static int devfreq_set_target(struct devfreq *devfreq, 
unsigned long new_freq,
if (devfreq->suspend_freq)
devfreq->resume_freq = new_freq;
 
-   return err;
+   return 0;
 }
 
 /**
-- 
2.25.1

Re: [PATCH 07/11] PM / devfreq: check get_dev_status before start monitor

[Dong Aisheng]

On Thu, Mar 18, 2021 at 5:42 PM Chanwoo Choi  wrote:
>
> On 3/18/21 6:54 PM, Chanwoo Choi wrote:
> > On 3/18/21 5:03 PM, Dong Aisheng wrote:
> >> Hi Chanwoo,
> >>
> >> On Sat, Mar 13, 2021 at 2:45 PM Dong Aisheng  wrote:
> >>>
> >>> On Sat, Mar 13, 2021 at 12:09 AM Chanwoo Choi  wrote:
> >>>>
> >>>> On 21. 3. 12. 오후 7:57, Dong Aisheng wrote:
> >>>>> On Thu, Mar 11, 2021 at 2:54 PM Chanwoo Choi  
> >>>>> wrote:
> >>>>>>
> >>>>>> On 3/10/21 1:56 PM, Dong Aisheng wrote:
> >>>>>>> On Wed, Mar 10, 2021 at 11:08 AM Chanwoo Choi  
> >>>>>>> wrote:
> >>>>>>>>
> >>>>>>>> On 3/10/21 11:56 AM, Dong Aisheng wrote:
> >>>>>>>>> On Wed, Mar 10, 2021 at 12:12 AM Chanwoo Choi  
> >>>>>>>>> wrote:
> >>>>>>>>>>
> >>>>>>>>>> On 21. 3. 10. 오전 12:58, Chanwoo Choi wrote:
> >>>>>>>>>>> On 21. 3. 9. 오후 9:58, Dong Aisheng wrote:
> >>>>>>>>>>>> The devfreq monitor depends on the device to provide load 
> >>>>>>>>>>>> information
> >>>>>>>>>>>> by .get_dev_status() to calculate the next target freq.
> >>>>>>>>>>>>
> >>>>>>>>>>>> And this will cause changing governor to simple ondemand fail
> >>>>>>>>>>>> if device can't support.
> >>>>>>>>>>>>
> >>>>>>>>>>>> Signed-off-by: Dong Aisheng 
> >>>>>>>>>>>> ---
> >>>>>>>>>>>>drivers/devfreq/devfreq.c | 10 +++---
> >>>>>>>>>>>>drivers/devfreq/governor.h|  2 +-
> >>>>>>>>>>>>drivers/devfreq/governor_simpleondemand.c |  3 +--
> >>>>>>>>>>>>3 files changed, 9 insertions(+), 6 deletions(-)
> >>>>>>>>>>>>
> >>>>>>>>>>>> diff --git a/drivers/devfreq/devfreq.c 
> >>>>>>>>>>>> b/drivers/devfreq/devfreq.c
> >>>>>>>>>>>> index 7231fe6862a2..d1787b6c7d7c 100644
> >>>>>>>>>>>> --- a/drivers/devfreq/devfreq.c
> >>>>>>>>>>>> +++ b/drivers/devfreq/devfreq.c
> >>>>>>>>>>>> @@ -482,10 +482,13 @@ static void devfreq_monitor(struct 
> >>>>>>>>>>>> work_struct
> >>>>>>>>>>>> *work)
> >>>>>>>>>>>> * to be called from governor in response to DEVFREQ_GOV_START
> >>>>>>>>>>>> * event when device is added to devfreq framework.
> >>>>>>>>>>>> */
> >>>>>>>>>>>> -void devfreq_monitor_start(struct devfreq *devfreq)
> >>>>>>>>>>>> +int devfreq_monitor_start(struct devfreq *devfreq)
> >>>>>>>>>>>>{
> >>>>>>>>>>>>if (IS_SUPPORTED_FLAG(devfreq->governor->flags, 
> >>>>>>>>>>>> IRQ_DRIVEN))
> >>>>>>>>>>>> -return;
> >>>>>>>>>>>> +return 0;
> >>>>>>>>>>>> +
> >>>>>>>>>>>> +if (!devfreq->profile->get_dev_status)
> >>>>>>>>>>>> +return -EINVAL;
> >>>>>>>>>>
> >>>>>>>>>> Again, I think that get_dev_status is not used for all governors.
> >>>>>>>>>> So that it cause the governor start fail. Don't check whether
> >>>>>>>>>> .get_dev_status is NULL or not.
> >>>>>>>>>>
> >>>>>>>>>
> >>>>>>>>> I'm not quite understand your point.
> >>>>>>>>> it is used by governor_simpleondemand.c and tegra_devfreq_governor.
> >>>>>>>>> get_target_freq -> devfreq_update_stats ->

Re: [PATCH 07/11] PM / devfreq: check get_dev_status before start monitor

[Dong Aisheng]

Hi Chanwoo,

On Sat, Mar 13, 2021 at 2:45 PM Dong Aisheng  wrote:
>
> On Sat, Mar 13, 2021 at 12:09 AM Chanwoo Choi  wrote:
> >
> > On 21. 3. 12. 오후 7:57, Dong Aisheng wrote:
> > > On Thu, Mar 11, 2021 at 2:54 PM Chanwoo Choi  
> > > wrote:
> > >>
> > >> On 3/10/21 1:56 PM, Dong Aisheng wrote:
> > >>> On Wed, Mar 10, 2021 at 11:08 AM Chanwoo Choi  
> > >>> wrote:
> > >>>>
> > >>>> On 3/10/21 11:56 AM, Dong Aisheng wrote:
> > >>>>> On Wed, Mar 10, 2021 at 12:12 AM Chanwoo Choi  
> > >>>>> wrote:
> > >>>>>>
> > >>>>>> On 21. 3. 10. 오전 12:58, Chanwoo Choi wrote:
> > >>>>>>> On 21. 3. 9. 오후 9:58, Dong Aisheng wrote:
> > >>>>>>>> The devfreq monitor depends on the device to provide load 
> > >>>>>>>> information
> > >>>>>>>> by .get_dev_status() to calculate the next target freq.
> > >>>>>>>>
> > >>>>>>>> And this will cause changing governor to simple ondemand fail
> > >>>>>>>> if device can't support.
> > >>>>>>>>
> > >>>>>>>> Signed-off-by: Dong Aisheng 
> > >>>>>>>> ---
> > >>>>>>>>drivers/devfreq/devfreq.c | 10 +++---
> > >>>>>>>>drivers/devfreq/governor.h|  2 +-
> > >>>>>>>>drivers/devfreq/governor_simpleondemand.c |  3 +--
> > >>>>>>>>3 files changed, 9 insertions(+), 6 deletions(-)
> > >>>>>>>>
> > >>>>>>>> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
> > >>>>>>>> index 7231fe6862a2..d1787b6c7d7c 100644
> > >>>>>>>> --- a/drivers/devfreq/devfreq.c
> > >>>>>>>> +++ b/drivers/devfreq/devfreq.c
> > >>>>>>>> @@ -482,10 +482,13 @@ static void devfreq_monitor(struct 
> > >>>>>>>> work_struct
> > >>>>>>>> *work)
> > >>>>>>>> * to be called from governor in response to DEVFREQ_GOV_START
> > >>>>>>>> * event when device is added to devfreq framework.
> > >>>>>>>> */
> > >>>>>>>> -void devfreq_monitor_start(struct devfreq *devfreq)
> > >>>>>>>> +int devfreq_monitor_start(struct devfreq *devfreq)
> > >>>>>>>>{
> > >>>>>>>>if (IS_SUPPORTED_FLAG(devfreq->governor->flags, IRQ_DRIVEN))
> > >>>>>>>> -return;
> > >>>>>>>> +return 0;
> > >>>>>>>> +
> > >>>>>>>> +if (!devfreq->profile->get_dev_status)
> > >>>>>>>> +return -EINVAL;
> > >>>>>>
> > >>>>>> Again, I think that get_dev_status is not used for all governors.
> > >>>>>> So that it cause the governor start fail. Don't check whether
> > >>>>>> .get_dev_status is NULL or not.
> > >>>>>>
> > >>>>>
> > >>>>> I'm not quite understand your point.
> > >>>>> it is used by governor_simpleondemand.c and tegra_devfreq_governor.
> > >>>>> get_target_freq -> devfreq_update_stats -> get_dev_status
> > >>>>
> > >>>> The devfreq can add the new governor by anyone.
> > >>>> So these functions like devfreq_monitor_* have to support
> > >>>> the governors and also must support the governor to be added
> > >>>> in the future.
> > >>>
> > >>> Yes, but devfreq_monitor_* is only used by polling mode, right?
> > >>> The governor using it has to implement get_dev_status unless
> > >>> there's an exception in the future.
> > >>>
> > >>> Currently this patch wants to address the issue that user can switch
> > >>> to ondemand governor (polling mode) by sysfs even devices does
> > >>> not support it (no get_dev_status implemented).
> > >>
> > >> As I commented, I'll fix this issue. If devfreq driver doesn't 

Re: [PATCH 07/11] PM / devfreq: check get_dev_status before start monitor

[Dong Aisheng]

On Sat, Mar 13, 2021 at 12:09 AM Chanwoo Choi  wrote:
>
> On 21. 3. 12. 오후 7:57, Dong Aisheng wrote:
> > On Thu, Mar 11, 2021 at 2:54 PM Chanwoo Choi  wrote:
> >>
> >> On 3/10/21 1:56 PM, Dong Aisheng wrote:
> >>> On Wed, Mar 10, 2021 at 11:08 AM Chanwoo Choi  
> >>> wrote:
> >>>>
> >>>> On 3/10/21 11:56 AM, Dong Aisheng wrote:
> >>>>> On Wed, Mar 10, 2021 at 12:12 AM Chanwoo Choi  
> >>>>> wrote:
> >>>>>>
> >>>>>> On 21. 3. 10. 오전 12:58, Chanwoo Choi wrote:
> >>>>>>> On 21. 3. 9. 오후 9:58, Dong Aisheng wrote:
> >>>>>>>> The devfreq monitor depends on the device to provide load information
> >>>>>>>> by .get_dev_status() to calculate the next target freq.
> >>>>>>>>
> >>>>>>>> And this will cause changing governor to simple ondemand fail
> >>>>>>>> if device can't support.
> >>>>>>>>
> >>>>>>>> Signed-off-by: Dong Aisheng 
> >>>>>>>> ---
> >>>>>>>>drivers/devfreq/devfreq.c | 10 +++---
> >>>>>>>>drivers/devfreq/governor.h|  2 +-
> >>>>>>>>drivers/devfreq/governor_simpleondemand.c |  3 +--
> >>>>>>>>3 files changed, 9 insertions(+), 6 deletions(-)
> >>>>>>>>
> >>>>>>>> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
> >>>>>>>> index 7231fe6862a2..d1787b6c7d7c 100644
> >>>>>>>> --- a/drivers/devfreq/devfreq.c
> >>>>>>>> +++ b/drivers/devfreq/devfreq.c
> >>>>>>>> @@ -482,10 +482,13 @@ static void devfreq_monitor(struct work_struct
> >>>>>>>> *work)
> >>>>>>>> * to be called from governor in response to DEVFREQ_GOV_START
> >>>>>>>> * event when device is added to devfreq framework.
> >>>>>>>> */
> >>>>>>>> -void devfreq_monitor_start(struct devfreq *devfreq)
> >>>>>>>> +int devfreq_monitor_start(struct devfreq *devfreq)
> >>>>>>>>{
> >>>>>>>>if (IS_SUPPORTED_FLAG(devfreq->governor->flags, IRQ_DRIVEN))
> >>>>>>>> -return;
> >>>>>>>> +return 0;
> >>>>>>>> +
> >>>>>>>> +if (!devfreq->profile->get_dev_status)
> >>>>>>>> +return -EINVAL;
> >>>>>>
> >>>>>> Again, I think that get_dev_status is not used for all governors.
> >>>>>> So that it cause the governor start fail. Don't check whether
> >>>>>> .get_dev_status is NULL or not.
> >>>>>>
> >>>>>
> >>>>> I'm not quite understand your point.
> >>>>> it is used by governor_simpleondemand.c and tegra_devfreq_governor.
> >>>>> get_target_freq -> devfreq_update_stats -> get_dev_status
> >>>>
> >>>> The devfreq can add the new governor by anyone.
> >>>> So these functions like devfreq_monitor_* have to support
> >>>> the governors and also must support the governor to be added
> >>>> in the future.
> >>>
> >>> Yes, but devfreq_monitor_* is only used by polling mode, right?
> >>> The governor using it has to implement get_dev_status unless
> >>> there's an exception in the future.
> >>>
> >>> Currently this patch wants to address the issue that user can switch
> >>> to ondemand governor (polling mode) by sysfs even devices does
> >>> not support it (no get_dev_status implemented).
> >>
> >> As I commented, I'll fix this issue. If devfreq driver doesn't implement
> >> the .get_dev_status, don't show it via available_governors. I think that
> >> it is fundamental solution to fix this issue.
> >
> > Sounds good
> >
> >> So on this version,
> >> don't add the this conditional statement on this function
> >>
> >
> > Almost all this patch did is adding a checking for get_dev_status.
> > So do you mean drop this patch?
> > I wonder it's still a necessary checking to explicitly 

Re: [PATCH 07/11] PM / devfreq: check get_dev_status before start monitor

[Dong Aisheng]

On Thu, Mar 11, 2021 at 2:54 PM Chanwoo Choi  wrote:
>
> On 3/10/21 1:56 PM, Dong Aisheng wrote:
> > On Wed, Mar 10, 2021 at 11:08 AM Chanwoo Choi  wrote:
> >>
> >> On 3/10/21 11:56 AM, Dong Aisheng wrote:
> >>> On Wed, Mar 10, 2021 at 12:12 AM Chanwoo Choi  wrote:
> >>>>
> >>>> On 21. 3. 10. 오전 12:58, Chanwoo Choi wrote:
> >>>>> On 21. 3. 9. 오후 9:58, Dong Aisheng wrote:
> >>>>>> The devfreq monitor depends on the device to provide load information
> >>>>>> by .get_dev_status() to calculate the next target freq.
> >>>>>>
> >>>>>> And this will cause changing governor to simple ondemand fail
> >>>>>> if device can't support.
> >>>>>>
> >>>>>> Signed-off-by: Dong Aisheng 
> >>>>>> ---
> >>>>>>   drivers/devfreq/devfreq.c | 10 +++---
> >>>>>>   drivers/devfreq/governor.h|  2 +-
> >>>>>>   drivers/devfreq/governor_simpleondemand.c |  3 +--
> >>>>>>   3 files changed, 9 insertions(+), 6 deletions(-)
> >>>>>>
> >>>>>> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
> >>>>>> index 7231fe6862a2..d1787b6c7d7c 100644
> >>>>>> --- a/drivers/devfreq/devfreq.c
> >>>>>> +++ b/drivers/devfreq/devfreq.c
> >>>>>> @@ -482,10 +482,13 @@ static void devfreq_monitor(struct work_struct
> >>>>>> *work)
> >>>>>>* to be called from governor in response to DEVFREQ_GOV_START
> >>>>>>* event when device is added to devfreq framework.
> >>>>>>*/
> >>>>>> -void devfreq_monitor_start(struct devfreq *devfreq)
> >>>>>> +int devfreq_monitor_start(struct devfreq *devfreq)
> >>>>>>   {
> >>>>>>   if (IS_SUPPORTED_FLAG(devfreq->governor->flags, IRQ_DRIVEN))
> >>>>>> -return;
> >>>>>> +return 0;
> >>>>>> +
> >>>>>> +if (!devfreq->profile->get_dev_status)
> >>>>>> +return -EINVAL;
> >>>>
> >>>> Again, I think that get_dev_status is not used for all governors.
> >>>> So that it cause the governor start fail. Don't check whether
> >>>> .get_dev_status is NULL or not.
> >>>>
> >>>
> >>> I'm not quite understand your point.
> >>> it is used by governor_simpleondemand.c and tegra_devfreq_governor.
> >>> get_target_freq -> devfreq_update_stats -> get_dev_status
> >>
> >> The devfreq can add the new governor by anyone.
> >> So these functions like devfreq_monitor_* have to support
> >> the governors and also must support the governor to be added
> >> in the future.
> >
> > Yes, but devfreq_monitor_* is only used by polling mode, right?
> > The governor using it has to implement get_dev_status unless
> > there's an exception in the future.
> >
> > Currently this patch wants to address the issue that user can switch
> > to ondemand governor (polling mode) by sysfs even devices does
> > not support it (no get_dev_status implemented).
>
> As I commented, I'll fix this issue. If devfreq driver doesn't implement
> the .get_dev_status, don't show it via available_governors. I think that
> it is fundamental solution to fix this issue.

Sounds good

> So on this version,
> don't add the this conditional statement on this function
>

Almost all this patch did is adding a checking for get_dev_status.
So do you mean drop this patch?
I wonder it's still a necessary checking to explicitly tell devfreq monitor
users that get_dev_status is needed during governor startup.

> And on next version, please use the capital letter for first character
> on patch title as following:
>
> - PM / devfreq: Check get_dev_status before start monitor
>

Okay to me.
Thanks for the suggestion.

Regards
Aisheng

> >
> > Regards
> > Aisheng
> >
> >>
> >>>
> >>> Without checking, device can switch to ondemand governor if it does not 
> >>> support.
> >>>
> >>> Am i missed something?
> >>>
> >>> Regards
> >>> Aisheng
> >>>
> >>>>>>   switch (devfreq->profile->timer) {
> >>>>>>   case DEVFREQ_TIMER_DEF

Re: [PATCH 07/11] PM / devfreq: check get_dev_status before start monitor

[Dong Aisheng]

On Wed, Mar 10, 2021 at 11:08 AM Chanwoo Choi  wrote:
>
> On 3/10/21 11:56 AM, Dong Aisheng wrote:
> > On Wed, Mar 10, 2021 at 12:12 AM Chanwoo Choi  wrote:
> >>
> >> On 21. 3. 10. 오전 12:58, Chanwoo Choi wrote:
> >>> On 21. 3. 9. 오후 9:58, Dong Aisheng wrote:
> >>>> The devfreq monitor depends on the device to provide load information
> >>>> by .get_dev_status() to calculate the next target freq.
> >>>>
> >>>> And this will cause changing governor to simple ondemand fail
> >>>> if device can't support.
> >>>>
> >>>> Signed-off-by: Dong Aisheng 
> >>>> ---
> >>>>   drivers/devfreq/devfreq.c | 10 +++---
> >>>>   drivers/devfreq/governor.h|  2 +-
> >>>>   drivers/devfreq/governor_simpleondemand.c |  3 +--
> >>>>   3 files changed, 9 insertions(+), 6 deletions(-)
> >>>>
> >>>> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
> >>>> index 7231fe6862a2..d1787b6c7d7c 100644
> >>>> --- a/drivers/devfreq/devfreq.c
> >>>> +++ b/drivers/devfreq/devfreq.c
> >>>> @@ -482,10 +482,13 @@ static void devfreq_monitor(struct work_struct
> >>>> *work)
> >>>>* to be called from governor in response to DEVFREQ_GOV_START
> >>>>* event when device is added to devfreq framework.
> >>>>*/
> >>>> -void devfreq_monitor_start(struct devfreq *devfreq)
> >>>> +int devfreq_monitor_start(struct devfreq *devfreq)
> >>>>   {
> >>>>   if (IS_SUPPORTED_FLAG(devfreq->governor->flags, IRQ_DRIVEN))
> >>>> -return;
> >>>> +return 0;
> >>>> +
> >>>> +if (!devfreq->profile->get_dev_status)
> >>>> +return -EINVAL;
> >>
> >> Again, I think that get_dev_status is not used for all governors.
> >> So that it cause the governor start fail. Don't check whether
> >> .get_dev_status is NULL or not.
> >>
> >
> > I'm not quite understand your point.
> > it is used by governor_simpleondemand.c and tegra_devfreq_governor.
> > get_target_freq -> devfreq_update_stats -> get_dev_status
>
> The devfreq can add the new governor by anyone.
> So these functions like devfreq_monitor_* have to support
> the governors and also must support the governor to be added
> in the future.

Yes, but devfreq_monitor_* is only used by polling mode, right?
The governor using it has to implement get_dev_status unless
there's an exception in the future.

Currently this patch wants to address the issue that user can switch
to ondemand governor (polling mode) by sysfs even devices does
not support it (no get_dev_status implemented).

Regards
Aisheng

>
> >
> > Without checking, device can switch to ondemand governor if it does not 
> > support.
> >
> > Am i missed something?
> >
> > Regards
> > Aisheng
> >
> >>>>   switch (devfreq->profile->timer) {
> >>>>   case DEVFREQ_TIMER_DEFERRABLE:
> >>>> @@ -495,12 +498,13 @@ void devfreq_monitor_start(struct devfreq *devfreq)
> >>>>   INIT_DELAYED_WORK(>work, devfreq_monitor);
> >>>>   break;
> >>>>   default:
> >>>> -return;
> >>>> +return -EINVAL;
> >>>>   }
> >>>>   if (devfreq->profile->polling_ms)
> >>>>   queue_delayed_work(devfreq_wq, >work,
> >>>>   msecs_to_jiffies(devfreq->profile->polling_ms));
> >>>> +return 0;
> >>>>   }
> >>>>   EXPORT_SYMBOL(devfreq_monitor_start);
> >>>> diff --git a/drivers/devfreq/governor.h b/drivers/devfreq/governor.h
> >>>> index 5cee3f64fe2b..31af6d072a10 100644
> >>>> --- a/drivers/devfreq/governor.h
> >>>> +++ b/drivers/devfreq/governor.h
> >>>> @@ -75,7 +75,7 @@ struct devfreq_governor {
> >>>>   unsigned int event, void *data);
> >>>>   };
> >>>> -void devfreq_monitor_start(struct devfreq *devfreq);
> >>>> +int devfreq_monitor_start(struct devfreq *devfreq);
> >>>>   void devfreq_monitor_stop(struct devfreq *devfreq);
> >>>>   void devfreq_monitor_suspend(struct devfreq *devfreq);
> >>>>   void devfreq_monitor_resume(struct devfre

Re: [PATCH 05/11] PM / devfreq: use more accurate returned new_freq as resume_freq

[Dong Aisheng]

On Wed, Mar 10, 2021 at 10:50 AM Chanwoo Choi  wrote:
>
> On 3/10/21 11:43 AM, Dong Aisheng wrote:
> > On Tue, Mar 9, 2021 at 11:53 PM Chanwoo Choi  wrote:
> >>
> >> On 21. 3. 9. 오후 9:58, Dong Aisheng wrote:
> >>> Use the more accurate returned new_freq as resume_freq.
> >>> It's the same as how devfreq->previous_freq was updated.
> >>>
> >>> Signed-off-by: Dong Aisheng 
> >>> ---
> >>>   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 6e80bf70e7b3..ce569bd9adfa 100644
> >>> --- a/drivers/devfreq/devfreq.c
> >>> +++ b/drivers/devfreq/devfreq.c
> >>> @@ -390,7 +390,7 @@ static int devfreq_set_target(struct devfreq 
> >>> *devfreq, unsigned long new_freq,
> >>>   devfreq->previous_freq = new_freq;
> >>>
> >>>   if (devfreq->suspend_freq)
> >>> - devfreq->resume_freq = cur_freq;
> >>> + devfreq->resume_freq = new_freq;
> >>>
> >>>   return err;
> >>>   }
> >>>
> >>
> >> This patch fixes the previous patch[1]. So that you need to
> >> add 'Fixes' tag as following:
> >>
> >> Fixes: 83f8ca45afbf0 ("PM / devfreq: add support for suspend/resume of a
> >> devfreq device")
> >>
> >
> > Will add Fixes tag in next version.
>
>
> On next version, recommend to place this patch at the first.

Yes, good practice as it's a fix.

Regards
Aisheng

>
> >
> >> commit 83f8ca45afbf041e312909f442128b99657d90b7
> >> Refs: v4.20-rc6-2-g83f8ca45afbf
> >> Author: Lukasz Luba 
> >> AuthorDate: Wed Dec 5 12:05:53 2018 +0100
> >> Commit: MyungJoo Ham 
> >> CommitDate: Tue Dec 11 11:09:47 2018 +0900
> >>
> >>  PM / devfreq: add support for suspend/resume of a devfreq device
> >>
> >>
> >> --
> >> Best Regards,
> >> Samsung Electronics
> >> Chanwoo Choi
> >
> >
>
>
> --
> Best Regards,
> Chanwoo Choi
> Samsung Electronics

Re: [PATCH 11/11] PM / devfreq: imx8m-ddrc: drop polling_ms

[Dong Aisheng]

On Wed, Mar 10, 2021 at 12:23 AM Chanwoo Choi  wrote:
>
> On 21. 3. 9. 오후 9:58, Dong Aisheng wrote:
> > polling_ms is only used by simple ondemand governor which
> > this driver can't support. Drop it to avoid confusing.
> >
> > Signed-off-by: Dong Aisheng 
> > ---
> >   drivers/devfreq/imx8m-ddrc.c | 1 -
> >   1 file changed, 1 deletion(-)
> >
> > diff --git a/drivers/devfreq/imx8m-ddrc.c b/drivers/devfreq/imx8m-ddrc.c
> > index 0a6b7a1c829d..ecb9375aa877 100644
> > --- a/drivers/devfreq/imx8m-ddrc.c
> > +++ b/drivers/devfreq/imx8m-ddrc.c
> > @@ -417,7 +417,6 @@ static int imx8m_ddrc_probe(struct platform_device 
> > *pdev)
> >   if (ret < 0)
> >   goto err;
> >
> > - priv->profile.polling_ms = 1000;
> >   priv->profile.target = imx8m_ddrc_target;
> >   priv->profile.exit = imx8m_ddrc_exit;
> >   priv->profile.get_cur_freq = imx8m_ddrc_get_cur_freq;
> >
>
> You can squash this patch with patch10 because polling_ms
> is related to .get_dev_status.

Sure i can do it.

Regards
Aisheng

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

Re: [PATCH 09/11] PM / devfreq: governor: optimize simpleondemand get_target_freq

[Dong Aisheng]

On Wed, Mar 10, 2021 at 12:09 AM Chanwoo Choi  wrote:
>
> On 21. 3. 9. 오후 9:58, Dong Aisheng wrote:
> > devfreq_simple_ondemand_data only needs to be initialized once when
> > calling devm_devfreq_add_device. It's unnecessary to put the data
> > check logic in the hot path (.get_target_freq()) where it will be
> > called all the time during polling. Instead, we only check and initialize
> > it one time during DEVFREQ_GOV_START.
> >
> > This also helps check data validability in advance during DEVFREQ_GOV_START
> > rather than checking it later when running .get_target_freq().
> >
> > Signed-off-by: Dong Aisheng 
> > ---
> >   drivers/devfreq/governor_simpleondemand.c | 50 +++
> >   1 file changed, 34 insertions(+), 16 deletions(-)
> >
> > diff --git a/drivers/devfreq/governor_simpleondemand.c 
> > b/drivers/devfreq/governor_simpleondemand.c
> > index ea287b57cbf3..341eb7e9dc04 100644
> > --- a/drivers/devfreq/governor_simpleondemand.c
> > +++ b/drivers/devfreq/governor_simpleondemand.c
> > @@ -15,15 +15,19 @@
> >   /* Default constants for DevFreq-Simple-Ondemand (DFSO) */
> >   #define DFSO_UPTHRESHOLD(90)
> >   #define DFSO_DOWNDIFFERENCTIAL  (5)
> > +
> > +static struct devfreq_simple_ondemand_data od_default = {
> > + .upthreshold = DFSO_UPTHRESHOLD,
> > + .downdifferential = DFSO_DOWNDIFFERENCTIAL,
> > +};
> > +
> >   static int devfreq_simple_ondemand_func(struct devfreq *df,
> >   unsigned long *freq)
> >   {
> >   int err;
> >   struct devfreq_dev_status *stat;
> >   unsigned long long a, b;
> > - unsigned int dfso_upthreshold = DFSO_UPTHRESHOLD;
> > - unsigned int dfso_downdifferential = DFSO_DOWNDIFFERENCTIAL;
> > - struct devfreq_simple_ondemand_data *data = df->data;
> > + struct devfreq_simple_ondemand_data *od = df->data;
> >
> >   err = devfreq_update_stats(df);
> >   if (err)
> > @@ -31,16 +35,6 @@ static int devfreq_simple_ondemand_func(struct devfreq 
> > *df,
> >
> >   stat = >last_status;
> >
> > - if (data) {
> > - if (data->upthreshold)
> > - dfso_upthreshold = data->upthreshold;
> > - if (data->downdifferential)
> > - dfso_downdifferential = data->downdifferential;
> > - }
> > - if (dfso_upthreshold > 100 ||
> > - dfso_upthreshold < dfso_downdifferential)
> > - return -EINVAL;
> > -
> >   /* Assume MAX if it is going to be divided by zero */
> >   if (stat->total_time == 0) {
> >   *freq = DEVFREQ_MAX_FREQ;
> > @@ -55,7 +49,7 @@ static int devfreq_simple_ondemand_func(struct devfreq 
> > *df,
> >
> >   /* Set MAX if it's busy enough */
> >   if (stat->busy_time * 100 >
> > - stat->total_time * dfso_upthreshold) {
> > + stat->total_time * od->upthreshold) {
> >   *freq = DEVFREQ_MAX_FREQ;
> >   return 0;
> >   }
> > @@ -68,7 +62,7 @@ static int devfreq_simple_ondemand_func(struct devfreq 
> > *df,
> >
> >   /* Keep the current frequency */
> >   if (stat->busy_time * 100 >
> > - stat->total_time * (dfso_upthreshold - dfso_downdifferential)) {
> > + stat->total_time * (od->upthreshold - od->downdifferential)) {
> >   *freq = stat->current_frequency;
> >   return 0;
> >   }
> > @@ -78,17 +72,41 @@ static int devfreq_simple_ondemand_func(struct devfreq 
> > *df,
> >   a *= stat->current_frequency;
> >   b = div_u64(a, stat->total_time);
> >   b *= 100;
> > - b = div_u64(b, (dfso_upthreshold - dfso_downdifferential / 2));
> > + b = div_u64(b, (od->upthreshold - od->downdifferential / 2));
> >   *freq = (unsigned long) b;
> >
> >   return 0;
> >   }
> >
> > +static int devfreq_simple_ondemand_check_od(struct devfreq *devfreq)
> > +{
> > + struct devfreq_simple_ondemand_data *od = devfreq->data;
> > +
> > + if (od) {
> > + if (!od->upthreshold)
> > + od->upthreshold = DFSO_UPTHRESHOLD;
> > +
> > + if (!od->downdifferential)
> > + od->downdifferential = DFSO_DOWNDIFFERENCTIAL;
> > +
> > + if (od->upthreshold > 100 ||
> > +   

Re: [PATCH 08/11] PM / devfreq: check get_dev_status in devfreq_update_stats

[Dong Aisheng]

On Wed, Mar 10, 2021 at 12:20 AM Chanwoo Choi  wrote:
>
> On 21. 3. 9. 오후 9:58, Dong Aisheng wrote:
> > Check .get_dev_status() in devfreq_update_stats in case it's abused
> > when a device does not provide it.
> >
> > Signed-off-by: Dong Aisheng 
> > ---
> >   drivers/devfreq/governor.h | 3 +++
> >   1 file changed, 3 insertions(+)
> >
> > diff --git a/drivers/devfreq/governor.h b/drivers/devfreq/governor.h
> > index 31af6d072a10..67a6dbdd5d23 100644
> > --- a/drivers/devfreq/governor.h
> > +++ b/drivers/devfreq/governor.h
> > @@ -89,6 +89,9 @@ int devfreq_update_target(struct devfreq *devfreq, 
> > unsigned long freq);
> >
> >   static inline int devfreq_update_stats(struct devfreq *df)
> >   {
> > + if (!df->profile->get_dev_status)
> > + return -EINVAL;
> > +
>
> I'm considering the following method instead of returning the error
> when .get_dev_status is NULL.
>
> if (!df->profile->get_dev_status) {
> df->last_status.total_time = 0;
> df->last_status.busy_time = 0;
> df->last_status.current_frequency = 0;
> return 0;
> }

I might  suggest not cause it's meaningless for ondemand governor but
introducing confusing. Simply return error could make the life a bit easier.
does it make sense to you?

Regards
Aisheng

>
> >   return df->profile->get_dev_status(df->dev.parent, >last_status);
> >   }
> >   #endif /* _GOVERNOR_H */
> >
>
>
> --
> Best Regards,
> Samsung Electronics
> Chanwoo Choi

Re: [PATCH 07/11] PM / devfreq: check get_dev_status before start monitor

[Dong Aisheng]

On Wed, Mar 10, 2021 at 12:12 AM Chanwoo Choi  wrote:
>
> On 21. 3. 10. 오전 12:58, Chanwoo Choi wrote:
> > On 21. 3. 9. 오후 9:58, Dong Aisheng wrote:
> >> The devfreq monitor depends on the device to provide load information
> >> by .get_dev_status() to calculate the next target freq.
> >>
> >> And this will cause changing governor to simple ondemand fail
> >> if device can't support.
> >>
> >> Signed-off-by: Dong Aisheng 
> >> ---
> >>   drivers/devfreq/devfreq.c | 10 +++---
> >>   drivers/devfreq/governor.h|  2 +-
> >>   drivers/devfreq/governor_simpleondemand.c |  3 +--
> >>   3 files changed, 9 insertions(+), 6 deletions(-)
> >>
> >> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
> >> index 7231fe6862a2..d1787b6c7d7c 100644
> >> --- a/drivers/devfreq/devfreq.c
> >> +++ b/drivers/devfreq/devfreq.c
> >> @@ -482,10 +482,13 @@ static void devfreq_monitor(struct work_struct
> >> *work)
> >>* to be called from governor in response to DEVFREQ_GOV_START
> >>* event when device is added to devfreq framework.
> >>*/
> >> -void devfreq_monitor_start(struct devfreq *devfreq)
> >> +int devfreq_monitor_start(struct devfreq *devfreq)
> >>   {
> >>   if (IS_SUPPORTED_FLAG(devfreq->governor->flags, IRQ_DRIVEN))
> >> -return;
> >> +return 0;
> >> +
> >> +if (!devfreq->profile->get_dev_status)
> >> +return -EINVAL;
>
> Again, I think that get_dev_status is not used for all governors.
> So that it cause the governor start fail. Don't check whether
> .get_dev_status is NULL or not.
>

I'm not quite understand your point.
it is used by governor_simpleondemand.c and tegra_devfreq_governor.
get_target_freq -> devfreq_update_stats -> get_dev_status

Without checking, device can switch to ondemand governor if it does not support.

Am i missed something?

Regards
Aisheng

> >>   switch (devfreq->profile->timer) {
> >>   case DEVFREQ_TIMER_DEFERRABLE:
> >> @@ -495,12 +498,13 @@ void devfreq_monitor_start(struct devfreq *devfreq)
> >>   INIT_DELAYED_WORK(>work, devfreq_monitor);
> >>   break;
> >>   default:
> >> -return;
> >> +return -EINVAL;
> >>   }
> >>   if (devfreq->profile->polling_ms)
> >>   queue_delayed_work(devfreq_wq, >work,
> >>   msecs_to_jiffies(devfreq->profile->polling_ms));
> >> +return 0;
> >>   }
> >>   EXPORT_SYMBOL(devfreq_monitor_start);
> >> diff --git a/drivers/devfreq/governor.h b/drivers/devfreq/governor.h
> >> index 5cee3f64fe2b..31af6d072a10 100644
> >> --- a/drivers/devfreq/governor.h
> >> +++ b/drivers/devfreq/governor.h
> >> @@ -75,7 +75,7 @@ struct devfreq_governor {
> >>   unsigned int event, void *data);
> >>   };
> >> -void devfreq_monitor_start(struct devfreq *devfreq);
> >> +int devfreq_monitor_start(struct devfreq *devfreq);
> >>   void devfreq_monitor_stop(struct devfreq *devfreq);
> >>   void devfreq_monitor_suspend(struct devfreq *devfreq);
> >>   void devfreq_monitor_resume(struct devfreq *devfreq);
> >> diff --git a/drivers/devfreq/governor_simpleondemand.c
> >> b/drivers/devfreq/governor_simpleondemand.c
> >> index d57b82a2b570..ea287b57cbf3 100644
> >> --- a/drivers/devfreq/governor_simpleondemand.c
> >> +++ b/drivers/devfreq/governor_simpleondemand.c
> >> @@ -89,8 +89,7 @@ static int devfreq_simple_ondemand_handler(struct
> >> devfreq *devfreq,
> >>   {
> >>   switch (event) {
> >>   case DEVFREQ_GOV_START:
> >> -devfreq_monitor_start(devfreq);
> >> -break;
> >> +return devfreq_monitor_start(devfreq);
> >>   case DEVFREQ_GOV_STOP:
> >>   devfreq_monitor_stop(devfreq);
> >>
> >
> > Need to handle the all points of devfreq_monitor_start() usage.
> > please check the tegra30-devfreq.c for this update.
> >
> > $ grep -rn "devfreq_monitor_start" drivers/
> > drivers/devfreq/governor_simpleondemand.c:92:
> > devfreq_monitor_start(devfreq);
> > drivers/devfreq/tegra30-devfreq.c:744:
> > devfreq_monitor_start(devfreq);
> > ..
> >
>
>
> --
> Best Regards,
> Samsung Electronics
> Chanwoo Choi

Re: [PATCH 07/11] PM / devfreq: check get_dev_status before start monitor

[Dong Aisheng]

On Tue, Mar 9, 2021 at 11:58 PM Chanwoo Choi  wrote:
>
> On 21. 3. 9. 오후 9:58, Dong Aisheng wrote:
> > The devfreq monitor depends on the device to provide load information
> > by .get_dev_status() to calculate the next target freq.
> >
> > And this will cause changing governor to simple ondemand fail
> > if device can't support.
> >
> > Signed-off-by: Dong Aisheng 
> > ---
> >   drivers/devfreq/devfreq.c | 10 +++---
> >   drivers/devfreq/governor.h|  2 +-
> >   drivers/devfreq/governor_simpleondemand.c |  3 +--
> >   3 files changed, 9 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
> > index 7231fe6862a2..d1787b6c7d7c 100644
> > --- a/drivers/devfreq/devfreq.c
> > +++ b/drivers/devfreq/devfreq.c
> > @@ -482,10 +482,13 @@ static void devfreq_monitor(struct work_struct *work)
> >* to be called from governor in response to DEVFREQ_GOV_START
> >* event when device is added to devfreq framework.
> >*/
> > -void devfreq_monitor_start(struct devfreq *devfreq)
> > +int devfreq_monitor_start(struct devfreq *devfreq)
> >   {
> >   if (IS_SUPPORTED_FLAG(devfreq->governor->flags, IRQ_DRIVEN))
> > - return;
> > + return 0;
> > +
> > + if (!devfreq->profile->get_dev_status)
> > + return -EINVAL;
> >
> >   switch (devfreq->profile->timer) {
> >   case DEVFREQ_TIMER_DEFERRABLE:
> > @@ -495,12 +498,13 @@ void devfreq_monitor_start(struct devfreq *devfreq)
> >   INIT_DELAYED_WORK(>work, devfreq_monitor);
> >   break;
> >   default:
> > - return;
> > + return -EINVAL;
> >   }
> >
> >   if (devfreq->profile->polling_ms)
> >   queue_delayed_work(devfreq_wq, >work,
> >   msecs_to_jiffies(devfreq->profile->polling_ms));
> > + return 0;
> >   }
> >   EXPORT_SYMBOL(devfreq_monitor_start);
> >
> > diff --git a/drivers/devfreq/governor.h b/drivers/devfreq/governor.h
> > index 5cee3f64fe2b..31af6d072a10 100644
> > --- a/drivers/devfreq/governor.h
> > +++ b/drivers/devfreq/governor.h
> > @@ -75,7 +75,7 @@ struct devfreq_governor {
> >   unsigned int event, void *data);
> >   };
> >
> > -void devfreq_monitor_start(struct devfreq *devfreq);
> > +int devfreq_monitor_start(struct devfreq *devfreq);
> >   void devfreq_monitor_stop(struct devfreq *devfreq);
> >   void devfreq_monitor_suspend(struct devfreq *devfreq);
> >   void devfreq_monitor_resume(struct devfreq *devfreq);
> > diff --git a/drivers/devfreq/governor_simpleondemand.c 
> > b/drivers/devfreq/governor_simpleondemand.c
> > index d57b82a2b570..ea287b57cbf3 100644
> > --- a/drivers/devfreq/governor_simpleondemand.c
> > +++ b/drivers/devfreq/governor_simpleondemand.c
> > @@ -89,8 +89,7 @@ static int devfreq_simple_ondemand_handler(struct devfreq 
> > *devfreq,
> >   {
> >   switch (event) {
> >   case DEVFREQ_GOV_START:
> > - devfreq_monitor_start(devfreq);
> > - break;
> > + return devfreq_monitor_start(devfreq);
> >
> >   case DEVFREQ_GOV_STOP:
> >   devfreq_monitor_stop(devfreq);
> >
>
> Need to handle the all points of devfreq_monitor_start() usage.
> please check the tegra30-devfreq.c for this update.
>
> $ grep -rn "devfreq_monitor_start" drivers/
> drivers/devfreq/governor_simpleondemand.c:92:
> devfreq_monitor_start(devfreq);
> drivers/devfreq/tegra30-devfreq.c:744:  
> devfreq_monitor_start(devfreq);

I can add error check for tegra in the next versions.
Thanks

Regards
Aisheng

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

Re: [PATCH 05/11] PM / devfreq: use more accurate returned new_freq as resume_freq

[Dong Aisheng]

On Tue, Mar 9, 2021 at 11:53 PM Chanwoo Choi  wrote:
>
> On 21. 3. 9. 오후 9:58, Dong Aisheng wrote:
> > Use the more accurate returned new_freq as resume_freq.
> > It's the same as how devfreq->previous_freq was updated.
> >
> > Signed-off-by: Dong Aisheng 
> > ---
> >   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 6e80bf70e7b3..ce569bd9adfa 100644
> > --- a/drivers/devfreq/devfreq.c
> > +++ b/drivers/devfreq/devfreq.c
> > @@ -390,7 +390,7 @@ static int devfreq_set_target(struct devfreq *devfreq, 
> > unsigned long new_freq,
> >   devfreq->previous_freq = new_freq;
> >
> >   if (devfreq->suspend_freq)
> > - devfreq->resume_freq = cur_freq;
> > + devfreq->resume_freq = new_freq;
> >
> >   return err;
> >   }
> >
>
> This patch fixes the previous patch[1]. So that you need to
> add 'Fixes' tag as following:
>
> Fixes: 83f8ca45afbf0 ("PM / devfreq: add support for suspend/resume of a
> devfreq device")
>

Will add Fixes tag in next version.

> commit 83f8ca45afbf041e312909f442128b99657d90b7
> Refs: v4.20-rc6-2-g83f8ca45afbf
> Author: Lukasz Luba 
> AuthorDate: Wed Dec 5 12:05:53 2018 +0100
> Commit: MyungJoo Ham 
> CommitDate: Tue Dec 11 11:09:47 2018 +0900
>
>  PM / devfreq: add support for suspend/resume of a devfreq device
>
>
> --
> Best Regards,
> Samsung Electronics
> Chanwoo Choi

Re: [PATCH 04/11] PM / devfreq: bail out early if no freq changes in devfreq_set_target

[Dong Aisheng]

On Tue, Mar 9, 2021 at 11:47 PM Chanwoo Choi  wrote:
>
> On 21. 3. 9. 오후 9:58, Dong Aisheng wrote:
> > It's unnecessary to set the same freq again and run notifier calls.
> >
> > Signed-off-by: Dong Aisheng 
> > ---
> >   drivers/devfreq/devfreq.c | 5 -
> >   1 file changed, 4 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
> > index bf3047896e41..6e80bf70e7b3 100644
> > --- a/drivers/devfreq/devfreq.c
> > +++ b/drivers/devfreq/devfreq.c
> > @@ -358,6 +358,9 @@ static int devfreq_set_target(struct devfreq *devfreq, 
> > unsigned long new_freq,
> >   else
> >   cur_freq = devfreq->previous_freq;
> >
> > + if (new_freq == cur_freq)
> > + return 0;
> > +
> >   freqs.old = cur_freq;
> >   freqs.new = new_freq;
> >   devfreq_notify_transition(devfreq, , DEVFREQ_PRECHANGE);
> > @@ -374,7 +377,7 @@ static int devfreq_set_target(struct devfreq *devfreq, 
> > unsigned long new_freq,
> >* and DEVFREQ_POSTCHANGE because for showing the correct frequency
> >* change order of between devfreq device and passive devfreq device.
> >*/
> > - if (trace_devfreq_frequency_enabled() && new_freq != cur_freq)
> > + if (trace_devfreq_frequency_enabled())
> >   trace_devfreq_frequency(devfreq, new_freq, cur_freq);
> >
> >   freqs.new = new_freq;
> >
>
> I'd like you to squash patch4 with patch6 because actually patch6
> is too minor clean-up. I think it is possible.

Got it, will squash when re-send.

Regards
Aisheng

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

Re: [PATCH 03/11] PM / devfreq: fix the wrong set_freq path for userspace governor

[Dong Aisheng]

On Tue, Mar 9, 2021 at 11:13 PM Chanwoo Choi  wrote:
>
> On 21. 3. 9. 오후 9:58, Dong Aisheng wrote:
> > Fix the wrong set_freq path for userspace governor.
> >
> > Signed-off-by: Dong Aisheng 
> > ---
> >   drivers/devfreq/Kconfig | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/devfreq/Kconfig b/drivers/devfreq/Kconfig
> > index 00704efe6398..20373a893b44 100644
> > --- a/drivers/devfreq/Kconfig
> > +++ b/drivers/devfreq/Kconfig
> > @@ -62,7 +62,7 @@ config DEVFREQ_GOV_USERSPACE
> >   help
> > Sets the frequency at the user specified one.
> > This governor returns the user configured frequency if there
> > -   has been an input to /sys/devices/.../power/devfreq_set_freq.
> > +   has been an input to /sys/devices/.../userspace/set_freq.
> > Otherwise, the governor does not change the frequency
> > given at the initialization.
> >
> >
>
> Looks good. But this patch just fix the information in Kconfig
> instead of fixing the wrong operation. To clarify the commit message,
> I'll change the patch title and commit message as following:

Looks like more accurate. Thanks.

Regards
Aisheng

>
>  PM / devfreq: Fix the wrong set_freq path for userspace governor in
> Kconfig
>
>  Fix the wrong set_freq path for userspace governor in Kconfig.
>
>  Signed-off-by: Dong Aisheng 
>
>
> Applied it. But, if you have any other objection, please let me know.
>
>
> --
> Best Regards,
> Samsung Electronics
> Chanwoo Choi

Re: [PATCH 02/11] PM / devfreq: remove the invalid description for get_target_freq

[Dong Aisheng]

On Tue, Mar 9, 2021 at 11:02 PM Chanwoo Choi  wrote:
>
> On 21. 3. 9. 오후 9:58, Dong Aisheng wrote:
> > First of all, no_central_polling was removed since
> > commit 7e6fdd4bad03 ("PM / devfreq: Core updates to support devices
> > which can idle")
> > Secondly, get_target_freq() is not only called only with update_devfreq()
> > notified by OPP now, but also min/max freq qos notifier.
> >
> > So remove this invalid description now to avoid confusing.
> >
> > Signed-off-by: Dong Aisheng 
> > ---
> >   drivers/devfreq/governor.h | 2 --
> >   1 file changed, 2 deletions(-)
> >
> > diff --git a/drivers/devfreq/governor.h b/drivers/devfreq/governor.h
> > index 70f44b3ca42e..5cee3f64fe2b 100644
> > --- a/drivers/devfreq/governor.h
> > +++ b/drivers/devfreq/governor.h
> > @@ -57,8 +57,6 @@
> >*  Basically, get_target_freq will run
> >*  devfreq_dev_profile.get_dev_status() to get the
> >*  status of the device (load = busy_time / total_time).
> > - *   If no_central_polling is set, this callback is called
> > - *   only with update_devfreq() notified by OPP.
> >* @event_handler:  Callback for devfreq core framework to notify 
> > events
> >*  to governors. Events include per device governor
> >*  init and exit, opp changes out of devfreq, suspend
> >
>
> As I replied from patch1, I recommend that squash it with patch1.

Got it, i will squash when sending the next version.
Thanks

Regards
Aisheng

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

Re: [RFC 05/19] devfreq: imx8m-ddrc: Change governor to powersave

[Dong Aisheng]

On Sat, Feb 20, 2021 at 12:03 AM Abel Vesa  wrote:
>
> By switching to powersave governor, we allow the imx8m-ddrc to always
> run at minimum rate needed by all the running masters.
>
> Signed-off-by: Abel Vesa 

Would you please help clarify a bit more why need use powersave by default?

Regards
Aisheng

> ---
>  drivers/devfreq/imx8m-ddrc.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/devfreq/imx8m-ddrc.c b/drivers/devfreq/imx8m-ddrc.c
> index bc82d3653bff..3a6c04ba4f2e 100644
> --- a/drivers/devfreq/imx8m-ddrc.c
> +++ b/drivers/devfreq/imx8m-ddrc.c
> @@ -379,7 +379,7 @@ static int imx8m_ddrc_probe(struct platform_device *pdev)
>  {
> struct device *dev = >dev;
> struct imx8m_ddrc *priv;
> -   const char *gov = DEVFREQ_GOV_USERSPACE;
> +   const char *gov = DEVFREQ_GOV_POWERSAVE;
> int ret;
>
> priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> --
> 2.29.2
>

[PATCH 11/11] PM / devfreq: imx8m-ddrc: drop polling_ms

[Dong Aisheng]

polling_ms is only used by simple ondemand governor which
this driver can't support. Drop it to avoid confusing.

Signed-off-by: Dong Aisheng 
---
 drivers/devfreq/imx8m-ddrc.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/devfreq/imx8m-ddrc.c b/drivers/devfreq/imx8m-ddrc.c
index 0a6b7a1c829d..ecb9375aa877 100644
--- a/drivers/devfreq/imx8m-ddrc.c
+++ b/drivers/devfreq/imx8m-ddrc.c
@@ -417,7 +417,6 @@ static int imx8m_ddrc_probe(struct platform_device *pdev)
if (ret < 0)
goto err;
 
-   priv->profile.polling_ms = 1000;
priv->profile.target = imx8m_ddrc_target;
priv->profile.exit = imx8m_ddrc_exit;
priv->profile.get_cur_freq = imx8m_ddrc_get_cur_freq;
-- 
2.25.1

[PATCH 06/11] PM / devfreq: drop the unnecessary low variable initialization

[Dong Aisheng]

drop the unnecessary low variable initialization and make the return
more straightforward.

Signed-off-by: Dong Aisheng 
---
 drivers/devfreq/devfreq.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
index ce569bd9adfa..7231fe6862a2 100644
--- a/drivers/devfreq/devfreq.c
+++ b/drivers/devfreq/devfreq.c
@@ -351,7 +351,7 @@ static int devfreq_set_target(struct devfreq *devfreq, 
unsigned long new_freq,
 {
struct devfreq_freqs freqs;
unsigned long cur_freq;
-   int err = 0;
+   int err;
 
if (devfreq->profile->get_cur_freq)
devfreq->profile->get_cur_freq(devfreq->dev.parent, _freq);
@@ -392,7 +392,7 @@ static int devfreq_set_target(struct devfreq *devfreq, 
unsigned long new_freq,
if (devfreq->suspend_freq)
devfreq->resume_freq = new_freq;
 
-   return err;
+   return 0;
 }
 
 /**
-- 
2.25.1

[PATCH 07/11] PM / devfreq: check get_dev_status before start monitor

[Dong Aisheng]

The devfreq monitor depends on the device to provide load information
by .get_dev_status() to calculate the next target freq.

And this will cause changing governor to simple ondemand fail
if device can't support.

Signed-off-by: Dong Aisheng 
---
 drivers/devfreq/devfreq.c | 10 +++---
 drivers/devfreq/governor.h|  2 +-
 drivers/devfreq/governor_simpleondemand.c |  3 +--
 3 files changed, 9 insertions(+), 6 deletions(-)

diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
index 7231fe6862a2..d1787b6c7d7c 100644
--- a/drivers/devfreq/devfreq.c
+++ b/drivers/devfreq/devfreq.c
@@ -482,10 +482,13 @@ static void devfreq_monitor(struct work_struct *work)
  * to be called from governor in response to DEVFREQ_GOV_START
  * event when device is added to devfreq framework.
  */
-void devfreq_monitor_start(struct devfreq *devfreq)
+int devfreq_monitor_start(struct devfreq *devfreq)
 {
if (IS_SUPPORTED_FLAG(devfreq->governor->flags, IRQ_DRIVEN))
-   return;
+   return 0;
+
+   if (!devfreq->profile->get_dev_status)
+   return -EINVAL;
 
switch (devfreq->profile->timer) {
case DEVFREQ_TIMER_DEFERRABLE:
@@ -495,12 +498,13 @@ void devfreq_monitor_start(struct devfreq *devfreq)
INIT_DELAYED_WORK(>work, devfreq_monitor);
break;
default:
-   return;
+   return -EINVAL;
}
 
if (devfreq->profile->polling_ms)
queue_delayed_work(devfreq_wq, >work,
msecs_to_jiffies(devfreq->profile->polling_ms));
+   return 0;
 }
 EXPORT_SYMBOL(devfreq_monitor_start);
 
diff --git a/drivers/devfreq/governor.h b/drivers/devfreq/governor.h
index 5cee3f64fe2b..31af6d072a10 100644
--- a/drivers/devfreq/governor.h
+++ b/drivers/devfreq/governor.h
@@ -75,7 +75,7 @@ struct devfreq_governor {
unsigned int event, void *data);
 };
 
-void devfreq_monitor_start(struct devfreq *devfreq);
+int devfreq_monitor_start(struct devfreq *devfreq);
 void devfreq_monitor_stop(struct devfreq *devfreq);
 void devfreq_monitor_suspend(struct devfreq *devfreq);
 void devfreq_monitor_resume(struct devfreq *devfreq);
diff --git a/drivers/devfreq/governor_simpleondemand.c 
b/drivers/devfreq/governor_simpleondemand.c
index d57b82a2b570..ea287b57cbf3 100644
--- a/drivers/devfreq/governor_simpleondemand.c
+++ b/drivers/devfreq/governor_simpleondemand.c
@@ -89,8 +89,7 @@ static int devfreq_simple_ondemand_handler(struct devfreq 
*devfreq,
 {
switch (event) {
case DEVFREQ_GOV_START:
-   devfreq_monitor_start(devfreq);
-   break;
+   return devfreq_monitor_start(devfreq);
 
case DEVFREQ_GOV_STOP:
devfreq_monitor_stop(devfreq);
-- 
2.25.1

[PATCH 08/11] PM / devfreq: check get_dev_status in devfreq_update_stats

[Dong Aisheng]

Check .get_dev_status() in devfreq_update_stats in case it's abused
when a device does not provide it.

Signed-off-by: Dong Aisheng 
---
 drivers/devfreq/governor.h | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/devfreq/governor.h b/drivers/devfreq/governor.h
index 31af6d072a10..67a6dbdd5d23 100644
--- a/drivers/devfreq/governor.h
+++ b/drivers/devfreq/governor.h
@@ -89,6 +89,9 @@ int devfreq_update_target(struct devfreq *devfreq, unsigned 
long freq);
 
 static inline int devfreq_update_stats(struct devfreq *df)
 {
+   if (!df->profile->get_dev_status)
+   return -EINVAL;
+
return df->profile->get_dev_status(df->dev.parent, >last_status);
 }
 #endif /* _GOVERNOR_H */
-- 
2.25.1

[PATCH 09/11] PM / devfreq: governor: optimize simpleondemand get_target_freq

[Dong Aisheng]

devfreq_simple_ondemand_data only needs to be initialized once when
calling devm_devfreq_add_device. It's unnecessary to put the data
check logic in the hot path (.get_target_freq()) where it will be
called all the time during polling. Instead, we only check and initialize
it one time during DEVFREQ_GOV_START.

This also helps check data validability in advance during DEVFREQ_GOV_START
rather than checking it later when running .get_target_freq().

Signed-off-by: Dong Aisheng 
---
 drivers/devfreq/governor_simpleondemand.c | 50 +++
 1 file changed, 34 insertions(+), 16 deletions(-)

diff --git a/drivers/devfreq/governor_simpleondemand.c 
b/drivers/devfreq/governor_simpleondemand.c
index ea287b57cbf3..341eb7e9dc04 100644
--- a/drivers/devfreq/governor_simpleondemand.c
+++ b/drivers/devfreq/governor_simpleondemand.c
@@ -15,15 +15,19 @@
 /* Default constants for DevFreq-Simple-Ondemand (DFSO) */
 #define DFSO_UPTHRESHOLD   (90)
 #define DFSO_DOWNDIFFERENCTIAL (5)
+
+static struct devfreq_simple_ondemand_data od_default = {
+   .upthreshold = DFSO_UPTHRESHOLD,
+   .downdifferential = DFSO_DOWNDIFFERENCTIAL,
+};
+
 static int devfreq_simple_ondemand_func(struct devfreq *df,
unsigned long *freq)
 {
int err;
struct devfreq_dev_status *stat;
unsigned long long a, b;
-   unsigned int dfso_upthreshold = DFSO_UPTHRESHOLD;
-   unsigned int dfso_downdifferential = DFSO_DOWNDIFFERENCTIAL;
-   struct devfreq_simple_ondemand_data *data = df->data;
+   struct devfreq_simple_ondemand_data *od = df->data;
 
err = devfreq_update_stats(df);
if (err)
@@ -31,16 +35,6 @@ static int devfreq_simple_ondemand_func(struct devfreq *df,
 
stat = >last_status;
 
-   if (data) {
-   if (data->upthreshold)
-   dfso_upthreshold = data->upthreshold;
-   if (data->downdifferential)
-   dfso_downdifferential = data->downdifferential;
-   }
-   if (dfso_upthreshold > 100 ||
-   dfso_upthreshold < dfso_downdifferential)
-   return -EINVAL;
-
/* Assume MAX if it is going to be divided by zero */
if (stat->total_time == 0) {
*freq = DEVFREQ_MAX_FREQ;
@@ -55,7 +49,7 @@ static int devfreq_simple_ondemand_func(struct devfreq *df,
 
/* Set MAX if it's busy enough */
if (stat->busy_time * 100 >
-   stat->total_time * dfso_upthreshold) {
+   stat->total_time * od->upthreshold) {
*freq = DEVFREQ_MAX_FREQ;
return 0;
}
@@ -68,7 +62,7 @@ static int devfreq_simple_ondemand_func(struct devfreq *df,
 
/* Keep the current frequency */
if (stat->busy_time * 100 >
-   stat->total_time * (dfso_upthreshold - dfso_downdifferential)) {
+   stat->total_time * (od->upthreshold - od->downdifferential)) {
*freq = stat->current_frequency;
return 0;
}
@@ -78,17 +72,41 @@ static int devfreq_simple_ondemand_func(struct devfreq *df,
a *= stat->current_frequency;
b = div_u64(a, stat->total_time);
b *= 100;
-   b = div_u64(b, (dfso_upthreshold - dfso_downdifferential / 2));
+   b = div_u64(b, (od->upthreshold - od->downdifferential / 2));
*freq = (unsigned long) b;
 
return 0;
 }
 
+static int devfreq_simple_ondemand_check_od(struct devfreq *devfreq)
+{
+   struct devfreq_simple_ondemand_data *od = devfreq->data;
+
+   if (od) {
+   if (!od->upthreshold)
+   od->upthreshold = DFSO_UPTHRESHOLD;
+
+   if (!od->downdifferential)
+   od->downdifferential = DFSO_DOWNDIFFERENCTIAL;
+
+   if (od->upthreshold > 100 ||
+   od->upthreshold < od->downdifferential)
+   return -EINVAL;
+   } else {
+   od = _default;
+   }
+
+   return 0;
+}
+
 static int devfreq_simple_ondemand_handler(struct devfreq *devfreq,
unsigned int event, void *data)
 {
switch (event) {
case DEVFREQ_GOV_START:
+   if (devfreq_simple_ondemand_check_od(devfreq))
+   return -EINVAL;
+
return devfreq_monitor_start(devfreq);
 
case DEVFREQ_GOV_STOP:
-- 
2.25.1

[PATCH 10/11] PM / devfreq: imx8m-ddrc: remove imx8m_ddrc_get_dev_status

[Dong Aisheng]

Current driver actually does not support simple ondemand governor
as it's unable to provide device load information. So removing
the unnecessary callback to avoid confusing.
Right now the driver is using userspace governor by default.

Signed-off-by: Dong Aisheng 
---
 drivers/devfreq/imx8m-ddrc.c | 13 -
 1 file changed, 13 deletions(-)

diff --git a/drivers/devfreq/imx8m-ddrc.c b/drivers/devfreq/imx8m-ddrc.c
index bc82d3653bff..0a6b7a1c829d 100644
--- a/drivers/devfreq/imx8m-ddrc.c
+++ b/drivers/devfreq/imx8m-ddrc.c
@@ -280,18 +280,6 @@ static int imx8m_ddrc_get_cur_freq(struct device *dev, 
unsigned long *freq)
return 0;
 }
 
-static int imx8m_ddrc_get_dev_status(struct device *dev,
-struct devfreq_dev_status *stat)
-{
-   struct imx8m_ddrc *priv = dev_get_drvdata(dev);
-
-   stat->busy_time = 0;
-   stat->total_time = 0;
-   stat->current_frequency = clk_get_rate(priv->dram_core);
-
-   return 0;
-}
-
 static int imx8m_ddrc_init_freq_info(struct device *dev)
 {
struct imx8m_ddrc *priv = dev_get_drvdata(dev);
@@ -431,7 +419,6 @@ static int imx8m_ddrc_probe(struct platform_device *pdev)
 
priv->profile.polling_ms = 1000;
priv->profile.target = imx8m_ddrc_target;
-   priv->profile.get_dev_status = imx8m_ddrc_get_dev_status;
priv->profile.exit = imx8m_ddrc_exit;
priv->profile.get_cur_freq = imx8m_ddrc_get_cur_freq;
priv->profile.initial_freq = clk_get_rate(priv->dram_core);
-- 
2.25.1

[PATCH 05/11] PM / devfreq: use more accurate returned new_freq as resume_freq

[Dong Aisheng]

Use the more accurate returned new_freq as resume_freq.
It's the same as how devfreq->previous_freq was updated.

Signed-off-by: Dong Aisheng 
---
 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 6e80bf70e7b3..ce569bd9adfa 100644
--- a/drivers/devfreq/devfreq.c
+++ b/drivers/devfreq/devfreq.c
@@ -390,7 +390,7 @@ static int devfreq_set_target(struct devfreq *devfreq, 
unsigned long new_freq,
devfreq->previous_freq = new_freq;
 
if (devfreq->suspend_freq)
-   devfreq->resume_freq = cur_freq;
+   devfreq->resume_freq = new_freq;
 
return err;
 }
-- 
2.25.1

[PATCH 04/11] PM / devfreq: bail out early if no freq changes in devfreq_set_target

[Dong Aisheng]

It's unnecessary to set the same freq again and run notifier calls.

Signed-off-by: Dong Aisheng 
---
 drivers/devfreq/devfreq.c | 5 -
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
index bf3047896e41..6e80bf70e7b3 100644
--- a/drivers/devfreq/devfreq.c
+++ b/drivers/devfreq/devfreq.c
@@ -358,6 +358,9 @@ static int devfreq_set_target(struct devfreq *devfreq, 
unsigned long new_freq,
else
cur_freq = devfreq->previous_freq;
 
+   if (new_freq == cur_freq)
+   return 0;
+
freqs.old = cur_freq;
freqs.new = new_freq;
devfreq_notify_transition(devfreq, , DEVFREQ_PRECHANGE);
@@ -374,7 +377,7 @@ static int devfreq_set_target(struct devfreq *devfreq, 
unsigned long new_freq,
 * and DEVFREQ_POSTCHANGE because for showing the correct frequency
 * change order of between devfreq device and passive devfreq device.
 */
-   if (trace_devfreq_frequency_enabled() && new_freq != cur_freq)
+   if (trace_devfreq_frequency_enabled())
trace_devfreq_frequency(devfreq, new_freq, cur_freq);
 
freqs.new = new_freq;
-- 
2.25.1

[PATCH 11/11] PM / devfreq: imx8m-ddrc: drop polling_ms

[Dong Aisheng]

polling_ms is only used by simple ondemand governor which
this driver can't support. Drop it to avoid confusing.

Signed-off-by: Dong Aisheng 
---
 drivers/devfreq/imx8m-ddrc.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/devfreq/imx8m-ddrc.c b/drivers/devfreq/imx8m-ddrc.c
index 0a6b7a1c829d..ecb9375aa877 100644
--- a/drivers/devfreq/imx8m-ddrc.c
+++ b/drivers/devfreq/imx8m-ddrc.c
@@ -417,7 +417,6 @@ static int imx8m_ddrc_probe(struct platform_device *pdev)
if (ret < 0)
goto err;
 
-   priv->profile.polling_ms = 1000;
priv->profile.target = imx8m_ddrc_target;
priv->profile.exit = imx8m_ddrc_exit;
priv->profile.get_cur_freq = imx8m_ddrc_get_cur_freq;
-- 
2.25.1

[PATCH 01/11] doc: ABI: devfreq: remove invalid central_polling description

[Dong Aisheng]

no_central_polling has been removed since
commit 7e6fdd4bad03 ("PM / devfreq: Core updates to support devices
which can idle")

Signed-off-by: Dong Aisheng 
---
 Documentation/ABI/testing/sysfs-class-devfreq | 5 +
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/Documentation/ABI/testing/sysfs-class-devfreq 
b/Documentation/ABI/testing/sysfs-class-devfreq
index 386bc230a33d..5e6b74f30406 100644
--- a/Documentation/ABI/testing/sysfs-class-devfreq
+++ b/Documentation/ABI/testing/sysfs-class-devfreq
@@ -97,10 +97,7 @@ Description:
object. The values are represented in ms. If the value is
less than 1 jiffy, it is considered to be 0, which means
no polling. This value is meaningless if the governor is
-   not polling; thus. If the governor is not using
-   devfreq-provided central polling
-   (/sys/class/devfreq/.../central_polling is 0), this value
-   may be useless.
+   not polling.
 
A list of governors that support the node:
- simple_ondmenad
-- 
2.25.1

[PATCH 02/11] PM / devfreq: remove the invalid description for get_target_freq

[Dong Aisheng]

First of all, no_central_polling was removed since
commit 7e6fdd4bad03 ("PM / devfreq: Core updates to support devices
which can idle")
Secondly, get_target_freq() is not only called only with update_devfreq()
notified by OPP now, but also min/max freq qos notifier.

So remove this invalid description now to avoid confusing.

Signed-off-by: Dong Aisheng 
---
 drivers/devfreq/governor.h | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/devfreq/governor.h b/drivers/devfreq/governor.h
index 70f44b3ca42e..5cee3f64fe2b 100644
--- a/drivers/devfreq/governor.h
+++ b/drivers/devfreq/governor.h
@@ -57,8 +57,6 @@
  * Basically, get_target_freq will run
  * devfreq_dev_profile.get_dev_status() to get the
  * status of the device (load = busy_time / total_time).
- * If no_central_polling is set, this callback is called
- * only with update_devfreq() notified by OPP.
  * @event_handler:  Callback for devfreq core framework to notify events
  *  to governors. Events include per device governor
  *  init and exit, opp changes out of devfreq, suspend
-- 
2.25.1

[PATCH 00/11] PM / devfreq: a few small fixes and improvements

[Dong Aisheng]

A few small fixes and improvements

Dong Aisheng (11):
  doc: ABI: devfreq: remove invalid central_polling description
  PM / devfreq: remove the invalid description for get_target_freq
  PM / devfreq: fix the wrong set_freq path for userspace governor
  PM / devfreq: bail out early if no freq changes in devfreq_set_target
  PM / devfreq: use more accurate returned new_freq as resume_freq
  PM / devfreq: drop the unnecessary low variable initialization
  PM / devfreq: check get_dev_status before start monitor
  PM / devfreq: check get_dev_status in devfreq_update_stats
  PM / devfreq: governor: optimize simpleondemand get_target_freq
  PM / devfreq: imx8m-ddrc: remove imx8m_ddrc_get_dev_status
  PM / devfreq: imx8m-ddrc: drop polling_ms

 Documentation/ABI/testing/sysfs-class-devfreq |  5 +-
 drivers/devfreq/Kconfig   |  2 +-
 drivers/devfreq/devfreq.c | 21 +---
 drivers/devfreq/governor.h|  7 +--
 drivers/devfreq/governor_simpleondemand.c | 53 ---
 drivers/devfreq/imx8m-ddrc.c  | 14 -
 6 files changed, 55 insertions(+), 47 deletions(-)

-- 
2.25.1

[PATCH 03/11] PM / devfreq: fix the wrong set_freq path for userspace governor

[Dong Aisheng]

Fix the wrong set_freq path for userspace governor.

Signed-off-by: Dong Aisheng 
---
 drivers/devfreq/Kconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/devfreq/Kconfig b/drivers/devfreq/Kconfig
index 00704efe6398..20373a893b44 100644
--- a/drivers/devfreq/Kconfig
+++ b/drivers/devfreq/Kconfig
@@ -62,7 +62,7 @@ config DEVFREQ_GOV_USERSPACE
help
  Sets the frequency at the user specified one.
  This governor returns the user configured frequency if there
- has been an input to /sys/devices/.../power/devfreq_set_freq.
+ has been an input to /sys/devices/.../userspace/set_freq.
  Otherwise, the governor does not change the frequency
  given at the initialization.
 
-- 
2.25.1

[PATCH 10/11] PM / devfreq: imx8m-ddrc: remove imx8m_ddrc_get_dev_status

[Dong Aisheng]

Current driver actually does not support simple ondemand governor
as it's unable to provide device load information. So removing
the unnecessary callback to avoid confusing.
Right now the driver is using userspace governor by default.

Signed-off-by: Dong Aisheng 
---
 drivers/devfreq/imx8m-ddrc.c | 13 -
 1 file changed, 13 deletions(-)

diff --git a/drivers/devfreq/imx8m-ddrc.c b/drivers/devfreq/imx8m-ddrc.c
index bc82d3653bff..0a6b7a1c829d 100644
--- a/drivers/devfreq/imx8m-ddrc.c
+++ b/drivers/devfreq/imx8m-ddrc.c
@@ -280,18 +280,6 @@ static int imx8m_ddrc_get_cur_freq(struct device *dev, 
unsigned long *freq)
return 0;
 }
 
-static int imx8m_ddrc_get_dev_status(struct device *dev,
-struct devfreq_dev_status *stat)
-{
-   struct imx8m_ddrc *priv = dev_get_drvdata(dev);
-
-   stat->busy_time = 0;
-   stat->total_time = 0;
-   stat->current_frequency = clk_get_rate(priv->dram_core);
-
-   return 0;
-}
-
 static int imx8m_ddrc_init_freq_info(struct device *dev)
 {
struct imx8m_ddrc *priv = dev_get_drvdata(dev);
@@ -431,7 +419,6 @@ static int imx8m_ddrc_probe(struct platform_device *pdev)
 
priv->profile.polling_ms = 1000;
priv->profile.target = imx8m_ddrc_target;
-   priv->profile.get_dev_status = imx8m_ddrc_get_dev_status;
priv->profile.exit = imx8m_ddrc_exit;
priv->profile.get_cur_freq = imx8m_ddrc_get_cur_freq;
priv->profile.initial_freq = clk_get_rate(priv->dram_core);
-- 
2.25.1

[PATCH 08/11] PM / devfreq: check get_dev_status in devfreq_update_stats

[Dong Aisheng]

Check .get_dev_status() in devfreq_update_stats in case it's abused
when a device does not provide it.

Signed-off-by: Dong Aisheng 
---
 drivers/devfreq/governor.h | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/devfreq/governor.h b/drivers/devfreq/governor.h
index 31af6d072a10..67a6dbdd5d23 100644
--- a/drivers/devfreq/governor.h
+++ b/drivers/devfreq/governor.h
@@ -89,6 +89,9 @@ int devfreq_update_target(struct devfreq *devfreq, unsigned 
long freq);
 
 static inline int devfreq_update_stats(struct devfreq *df)
 {
+   if (!df->profile->get_dev_status)
+   return -EINVAL;
+
return df->profile->get_dev_status(df->dev.parent, >last_status);
 }
 #endif /* _GOVERNOR_H */
-- 
2.25.1

[PATCH 07/11] PM / devfreq: check get_dev_status before start monitor

[Dong Aisheng]

The devfreq monitor depends on the device to provide load information
by .get_dev_status() to calculate the next target freq.

And this will cause changing governor to simple ondemand fail
if device can't support.

Signed-off-by: Dong Aisheng 
---
 drivers/devfreq/devfreq.c | 10 +++---
 drivers/devfreq/governor.h|  2 +-
 drivers/devfreq/governor_simpleondemand.c |  3 +--
 3 files changed, 9 insertions(+), 6 deletions(-)

diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
index 7231fe6862a2..d1787b6c7d7c 100644
--- a/drivers/devfreq/devfreq.c
+++ b/drivers/devfreq/devfreq.c
@@ -482,10 +482,13 @@ static void devfreq_monitor(struct work_struct *work)
  * to be called from governor in response to DEVFREQ_GOV_START
  * event when device is added to devfreq framework.
  */
-void devfreq_monitor_start(struct devfreq *devfreq)
+int devfreq_monitor_start(struct devfreq *devfreq)
 {
if (IS_SUPPORTED_FLAG(devfreq->governor->flags, IRQ_DRIVEN))
-   return;
+   return 0;
+
+   if (!devfreq->profile->get_dev_status)
+   return -EINVAL;
 
switch (devfreq->profile->timer) {
case DEVFREQ_TIMER_DEFERRABLE:
@@ -495,12 +498,13 @@ void devfreq_monitor_start(struct devfreq *devfreq)
INIT_DELAYED_WORK(>work, devfreq_monitor);
break;
default:
-   return;
+   return -EINVAL;
}
 
if (devfreq->profile->polling_ms)
queue_delayed_work(devfreq_wq, >work,
msecs_to_jiffies(devfreq->profile->polling_ms));
+   return 0;
 }
 EXPORT_SYMBOL(devfreq_monitor_start);
 
diff --git a/drivers/devfreq/governor.h b/drivers/devfreq/governor.h
index 5cee3f64fe2b..31af6d072a10 100644
--- a/drivers/devfreq/governor.h
+++ b/drivers/devfreq/governor.h
@@ -75,7 +75,7 @@ struct devfreq_governor {
unsigned int event, void *data);
 };
 
-void devfreq_monitor_start(struct devfreq *devfreq);
+int devfreq_monitor_start(struct devfreq *devfreq);
 void devfreq_monitor_stop(struct devfreq *devfreq);
 void devfreq_monitor_suspend(struct devfreq *devfreq);
 void devfreq_monitor_resume(struct devfreq *devfreq);
diff --git a/drivers/devfreq/governor_simpleondemand.c 
b/drivers/devfreq/governor_simpleondemand.c
index d57b82a2b570..ea287b57cbf3 100644
--- a/drivers/devfreq/governor_simpleondemand.c
+++ b/drivers/devfreq/governor_simpleondemand.c
@@ -89,8 +89,7 @@ static int devfreq_simple_ondemand_handler(struct devfreq 
*devfreq,
 {
switch (event) {
case DEVFREQ_GOV_START:
-   devfreq_monitor_start(devfreq);
-   break;
+   return devfreq_monitor_start(devfreq);
 
case DEVFREQ_GOV_STOP:
devfreq_monitor_stop(devfreq);
-- 
2.25.1

[PATCH 09/11] PM / devfreq: governor: optimize simpleondemand get_target_freq

[Dong Aisheng]

devfreq_simple_ondemand_data only needs to be initialized once when
calling devm_devfreq_add_device. It's unnecessary to put the data
check logic in the hot path (.get_target_freq()) where it will be
called all the time during polling. Instead, we only check and initialize
it one time during DEVFREQ_GOV_START.

This also helps check data validability in advance during DEVFREQ_GOV_START
rather than checking it later when running .get_target_freq().

Signed-off-by: Dong Aisheng 
---
 drivers/devfreq/governor_simpleondemand.c | 50 +++
 1 file changed, 34 insertions(+), 16 deletions(-)

diff --git a/drivers/devfreq/governor_simpleondemand.c 
b/drivers/devfreq/governor_simpleondemand.c
index ea287b57cbf3..341eb7e9dc04 100644
--- a/drivers/devfreq/governor_simpleondemand.c
+++ b/drivers/devfreq/governor_simpleondemand.c
@@ -15,15 +15,19 @@
 /* Default constants for DevFreq-Simple-Ondemand (DFSO) */
 #define DFSO_UPTHRESHOLD   (90)
 #define DFSO_DOWNDIFFERENCTIAL (5)
+
+static struct devfreq_simple_ondemand_data od_default = {
+   .upthreshold = DFSO_UPTHRESHOLD,
+   .downdifferential = DFSO_DOWNDIFFERENCTIAL,
+};
+
 static int devfreq_simple_ondemand_func(struct devfreq *df,
unsigned long *freq)
 {
int err;
struct devfreq_dev_status *stat;
unsigned long long a, b;
-   unsigned int dfso_upthreshold = DFSO_UPTHRESHOLD;
-   unsigned int dfso_downdifferential = DFSO_DOWNDIFFERENCTIAL;
-   struct devfreq_simple_ondemand_data *data = df->data;
+   struct devfreq_simple_ondemand_data *od = df->data;
 
err = devfreq_update_stats(df);
if (err)
@@ -31,16 +35,6 @@ static int devfreq_simple_ondemand_func(struct devfreq *df,
 
stat = >last_status;
 
-   if (data) {
-   if (data->upthreshold)
-   dfso_upthreshold = data->upthreshold;
-   if (data->downdifferential)
-   dfso_downdifferential = data->downdifferential;
-   }
-   if (dfso_upthreshold > 100 ||
-   dfso_upthreshold < dfso_downdifferential)
-   return -EINVAL;
-
/* Assume MAX if it is going to be divided by zero */
if (stat->total_time == 0) {
*freq = DEVFREQ_MAX_FREQ;
@@ -55,7 +49,7 @@ static int devfreq_simple_ondemand_func(struct devfreq *df,
 
/* Set MAX if it's busy enough */
if (stat->busy_time * 100 >
-   stat->total_time * dfso_upthreshold) {
+   stat->total_time * od->upthreshold) {
*freq = DEVFREQ_MAX_FREQ;
return 0;
}
@@ -68,7 +62,7 @@ static int devfreq_simple_ondemand_func(struct devfreq *df,
 
/* Keep the current frequency */
if (stat->busy_time * 100 >
-   stat->total_time * (dfso_upthreshold - dfso_downdifferential)) {
+   stat->total_time * (od->upthreshold - od->downdifferential)) {
*freq = stat->current_frequency;
return 0;
}
@@ -78,17 +72,41 @@ static int devfreq_simple_ondemand_func(struct devfreq *df,
a *= stat->current_frequency;
b = div_u64(a, stat->total_time);
b *= 100;
-   b = div_u64(b, (dfso_upthreshold - dfso_downdifferential / 2));
+   b = div_u64(b, (od->upthreshold - od->downdifferential / 2));
*freq = (unsigned long) b;
 
return 0;
 }
 
+static int devfreq_simple_ondemand_check_od(struct devfreq *devfreq)
+{
+   struct devfreq_simple_ondemand_data *od = devfreq->data;
+
+   if (od) {
+   if (!od->upthreshold)
+   od->upthreshold = DFSO_UPTHRESHOLD;
+
+   if (!od->downdifferential)
+   od->downdifferential = DFSO_DOWNDIFFERENCTIAL;
+
+   if (od->upthreshold > 100 ||
+   od->upthreshold < od->downdifferential)
+   return -EINVAL;
+   } else {
+   od = _default;
+   }
+
+   return 0;
+}
+
 static int devfreq_simple_ondemand_handler(struct devfreq *devfreq,
unsigned int event, void *data)
 {
switch (event) {
case DEVFREQ_GOV_START:
+   if (devfreq_simple_ondemand_check_od(devfreq))
+   return -EINVAL;
+
return devfreq_monitor_start(devfreq);
 
case DEVFREQ_GOV_STOP:
-- 
2.25.1

[PATCH 06/11] PM / devfreq: drop the unnecessary low variable initialization

[Dong Aisheng]

drop the unnecessary low variable initialization and make the return
more straightforward.

Signed-off-by: Dong Aisheng 
---
 drivers/devfreq/devfreq.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
index ce569bd9adfa..7231fe6862a2 100644
--- a/drivers/devfreq/devfreq.c
+++ b/drivers/devfreq/devfreq.c
@@ -351,7 +351,7 @@ static int devfreq_set_target(struct devfreq *devfreq, 
unsigned long new_freq,
 {
struct devfreq_freqs freqs;
unsigned long cur_freq;
-   int err = 0;
+   int err;
 
if (devfreq->profile->get_cur_freq)
devfreq->profile->get_cur_freq(devfreq->dev.parent, _freq);
@@ -392,7 +392,7 @@ static int devfreq_set_target(struct devfreq *devfreq, 
unsigned long new_freq,
if (devfreq->suspend_freq)
devfreq->resume_freq = new_freq;
 
-   return err;
+   return 0;
 }
 
 /**
-- 
2.25.1

[PATCH 04/11] PM / devfreq: bail out early if no freq changes in devfreq_set_target

[Dong Aisheng]

It's unnecessary to set the same freq again and run notifier calls.

Signed-off-by: Dong Aisheng 
---
 drivers/devfreq/devfreq.c | 5 -
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
index bf3047896e41..6e80bf70e7b3 100644
--- a/drivers/devfreq/devfreq.c
+++ b/drivers/devfreq/devfreq.c
@@ -358,6 +358,9 @@ static int devfreq_set_target(struct devfreq *devfreq, 
unsigned long new_freq,
else
cur_freq = devfreq->previous_freq;
 
+   if (new_freq == cur_freq)
+   return 0;
+
freqs.old = cur_freq;
freqs.new = new_freq;
devfreq_notify_transition(devfreq, , DEVFREQ_PRECHANGE);
@@ -374,7 +377,7 @@ static int devfreq_set_target(struct devfreq *devfreq, 
unsigned long new_freq,
 * and DEVFREQ_POSTCHANGE because for showing the correct frequency
 * change order of between devfreq device and passive devfreq device.
 */
-   if (trace_devfreq_frequency_enabled() && new_freq != cur_freq)
+   if (trace_devfreq_frequency_enabled())
trace_devfreq_frequency(devfreq, new_freq, cur_freq);
 
freqs.new = new_freq;
-- 
2.25.1

[PATCH 05/11] PM / devfreq: use more accurate returned new_freq as resume_freq

[Dong Aisheng]

Use the more accurate returned new_freq as resume_freq.
It's the same as how devfreq->previous_freq was updated.

Signed-off-by: Dong Aisheng 
---
 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 6e80bf70e7b3..ce569bd9adfa 100644
--- a/drivers/devfreq/devfreq.c
+++ b/drivers/devfreq/devfreq.c
@@ -390,7 +390,7 @@ static int devfreq_set_target(struct devfreq *devfreq, 
unsigned long new_freq,
devfreq->previous_freq = new_freq;
 
if (devfreq->suspend_freq)
-   devfreq->resume_freq = cur_freq;
+   devfreq->resume_freq = new_freq;
 
return err;
 }
-- 
2.25.1

[PATCH 02/11] PM / devfreq: remove the invalid description for get_target_freq

[Dong Aisheng]

First of all, no_central_polling was removed since
commit 7e6fdd4bad03 ("PM / devfreq: Core updates to support devices
which can idle")
Secondly, get_target_freq() is not only called only with update_devfreq()
notified by OPP now, but also min/max freq qos notifier.

So remove this invalid description now to avoid confusing.

Signed-off-by: Dong Aisheng 
---
 drivers/devfreq/governor.h | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/devfreq/governor.h b/drivers/devfreq/governor.h
index 70f44b3ca42e..5cee3f64fe2b 100644
--- a/drivers/devfreq/governor.h
+++ b/drivers/devfreq/governor.h
@@ -57,8 +57,6 @@
  * Basically, get_target_freq will run
  * devfreq_dev_profile.get_dev_status() to get the
  * status of the device (load = busy_time / total_time).
- * If no_central_polling is set, this callback is called
- * only with update_devfreq() notified by OPP.
  * @event_handler:  Callback for devfreq core framework to notify events
  *  to governors. Events include per device governor
  *  init and exit, opp changes out of devfreq, suspend
-- 
2.25.1

[PATCH 03/11] PM / devfreq: fix the wrong set_freq path for userspace governor

[Dong Aisheng]

Fix the wrong set_freq path for userspace governor.

Signed-off-by: Dong Aisheng 
---
 drivers/devfreq/Kconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/devfreq/Kconfig b/drivers/devfreq/Kconfig
index 00704efe6398..20373a893b44 100644
--- a/drivers/devfreq/Kconfig
+++ b/drivers/devfreq/Kconfig
@@ -62,7 +62,7 @@ config DEVFREQ_GOV_USERSPACE
help
  Sets the frequency at the user specified one.
  This governor returns the user configured frequency if there
- has been an input to /sys/devices/.../power/devfreq_set_freq.
+ has been an input to /sys/devices/.../userspace/set_freq.
  Otherwise, the governor does not change the frequency
  given at the initialization.
 
-- 
2.25.1

[PATCH 01/11] doc: ABI: devfreq: remove invalid central_polling description

[Dong Aisheng]

no_central_polling has been removed since
commit 7e6fdd4bad03 ("PM / devfreq: Core updates to support devices
which can idle")

Signed-off-by: Dong Aisheng 
---
 Documentation/ABI/testing/sysfs-class-devfreq | 5 +
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/Documentation/ABI/testing/sysfs-class-devfreq 
b/Documentation/ABI/testing/sysfs-class-devfreq
index 386bc230a33d..5e6b74f30406 100644
--- a/Documentation/ABI/testing/sysfs-class-devfreq
+++ b/Documentation/ABI/testing/sysfs-class-devfreq
@@ -97,10 +97,7 @@ Description:
object. The values are represented in ms. If the value is
less than 1 jiffy, it is considered to be 0, which means
no polling. This value is meaningless if the governor is
-   not polling; thus. If the governor is not using
-   devfreq-provided central polling
-   (/sys/class/devfreq/.../central_polling is 0), this value
-   may be useless.
+   not polling.
 
A list of governors that support the node:
- simple_ondmenad
-- 
2.25.1

[PATCH 00/11] PM / devfreq: a few small fixes and improvements

[Dong Aisheng]

A few small fixes and improvements

Dong Aisheng (11):
  doc: ABI: devfreq: remove invalid central_polling description
  PM / devfreq: remove the invalid description for get_target_freq
  PM / devfreq: fix the wrong set_freq path for userspace governor
  PM / devfreq: bail out early if no freq changes in devfreq_set_target
  PM / devfreq: use more accurate returned new_freq as resume_freq
  PM / devfreq: drop the unnecessary low variable initialization
  PM / devfreq: check get_dev_status before start monitor
  PM / devfreq: check get_dev_status in devfreq_update_stats
  PM / devfreq: governor: optimize simpleondemand get_target_freq
  PM / devfreq: imx8m-ddrc: remove imx8m_ddrc_get_dev_status
  PM / devfreq: imx8m-ddrc: drop polling_ms

 Documentation/ABI/testing/sysfs-class-devfreq |  5 +-
 drivers/devfreq/Kconfig   |  2 +-
 drivers/devfreq/devfreq.c | 21 +---
 drivers/devfreq/governor.h|  7 +--
 drivers/devfreq/governor_simpleondemand.c | 53 ---
 drivers/devfreq/imx8m-ddrc.c  | 14 -
 6 files changed, 55 insertions(+), 47 deletions(-)

-- 
2.25.1

[PATCH 2/3] of: property: add debug info for supplier devices still unavailable

[Dong Aisheng]

It's normal that supplier devices may still unavaiable when parse DT to
create dependency. e.g. supplier devices populated by drivers.
Add debug info for this case.

Cc: devicet...@vger.kernel.org
Cc: Saravana Kannan 
Cc: Rob Herring 
Cc: Greg Kroah-Hartman 
Signed-off-by: Dong Aisheng 
---
 drivers/of/property.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/of/property.c b/drivers/of/property.c
index 408a7b5f06a9..21a854e85234 100644
--- a/drivers/of/property.c
+++ b/drivers/of/property.c
@@ -1150,6 +1150,8 @@ static int of_link_to_phandle(struct device *dev, struct 
device_node *sup_np,
 * Can't check for cycles or no cycles. So let's try
 * again later.
 */
+   dev_dbg(dev, "Not linking to %pOFP - device may still 
unavailable\n",
+   sup_np);
ret = -EAGAIN;
}
 
-- 
2.23.0

[PATCH 3/3] of: property: fix document of of_get_next_parent_dev

[Dong Aisheng]

Fix document of of_get_next_parent_dev.

Cc: devicet...@vger.kernel.org
Cc: Saravana Kannan 
Cc: Rob Herring 
Cc: Greg Kroah-Hartman 
Signed-off-by: Dong Aisheng 
---
 drivers/of/property.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/of/property.c b/drivers/of/property.c
index 21a854e85234..5bd4a9bead47 100644
--- a/drivers/of/property.c
+++ b/drivers/of/property.c
@@ -1038,7 +1038,7 @@ static bool of_is_ancestor_of(struct device_node 
*test_ancestor,
 }
 
 /**
- * of_get_next_parent_dev - Add device link to supplier from supplier phandle
+ * of_get_next_parent_dev - Get the closest ancestor device of a device node
  * @np: device tree node
  *
  * Given a device tree node (@np), this function finds its closest ancestor
-- 
2.23.0

[PATCH 1/3] driver core: simply go out if the same device_link is added again

[Dong Aisheng]

It's possible that the same device link may be added by parsing the
function dependecy in DT. e.g. clock/gpio/regulators.
Simply go out for this case.

Cc: Greg Kroah-Hartman 
Cc: "Rafael J. Wysocki" 
Cc: Saravana Kannan 
Signed-off-by: Dong Aisheng 
---
 drivers/base/core.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/base/core.c b/drivers/base/core.c
index 4c03bdd3a268..7d91d4074136 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -567,6 +567,9 @@ struct device_link *device_link_add(struct device *consumer,
if (link->consumer != consumer)
continue;
 
+   if (flags == link->flags)
+   goto out;
+
if (flags & DL_FLAG_PM_RUNTIME) {
if (!(link->flags & DL_FLAG_PM_RUNTIME)) {
pm_runtime_new_link(consumer);
-- 
2.23.0

Re: [PATCH v5 10/14] clk: imx: Add generic blk-ctl driver

[Dong Aisheng]

On Tue, Nov 3, 2020 at 7:22 PM Abel Vesa  wrote:
...
> +static int imx_blk_ctl_reset_set(struct reset_controller_dev *rcdev,
> + unsigned long id, bool assert)
> +{
> +   struct imx_blk_ctl_drvdata *drvdata = container_of(rcdev,
> +   struct imx_blk_ctl_drvdata, rcdev);
> +   unsigned int offset = drvdata->rst_hws[id].offset;
> +   unsigned int shift = drvdata->rst_hws[id].shift;
> +   unsigned int mask = drvdata->rst_hws[id].mask;
> +   void __iomem *reg_addr = drvdata->base + offset;
> +   unsigned long flags;
> +   u32 reg;
> +
> +   if (!assert && !test_bit(1, >rst_hws[id].asserted))
> +   return -ENODEV;

What if consumers call deassert first in probe which seems common in kernel?
It seems will fail.
e.g.
probe() {
reset_control_get()
reset_control_deassert()
}

Regards
Aisheng

> +
> +   if (assert && !test_and_set_bit(1, >rst_hws[id].asserted))
> +   pm_runtime_get_sync(rcdev->dev)
> +
> +   spin_lock_irqsave(>lock, flags);
> +
> +   reg = readl(reg_addr);
> +   if (assert)
> +   writel(reg & ~(mask << shift), reg_addr);
> +   else
> +   writel(reg | (mask << shift), reg_addr);
> +
> +   spin_unlock_irqrestore(>lock, flags);
> +
> +   if (!assert && test_and_clear_bit(1, >rst_hws[id].asserted))
> +   pm_runtime_put(rcdev->dev)
> +
> +   return 0;
> +}
> +
> +static int imx_blk_ctl_reset_assert(struct reset_controller_dev *rcdev,
> +  unsigned long id)
> +{
> +   return imx_blk_ctl_reset_set(rcdev, id, true);
> +}
> +
> +static int imx_blk_ctl_reset_deassert(struct reset_controller_dev *rcdev,
> +unsigned long id)
> +{
> +   return imx_blk_ctl_reset_set(rcdev, id, false);
> +}
> +
> +static const struct reset_control_ops imx_blk_ctl_reset_ops = {
> +   .assert = imx_blk_ctl_reset_assert,
> +   .deassert   = imx_blk_ctl_reset_deassert,
> +};
> +
> +static int imx_blk_ctl_register_reset_controller(struct device *dev)
> +{
> +   const struct imx_blk_ctl_dev_data *dev_data = 
> of_device_get_match_data(dev);
> +   struct imx_blk_ctl_drvdata *drvdata = dev_get_drvdata(dev);
> +   int max = dev_data->resets_max;
> +   struct imx_reset_hw *hws;
> +   int i;
> +
> +   spin_lock_init(>lock);
> +
> +   drvdata->rcdev.owner = THIS_MODULE;
> +   drvdata->rcdev.nr_resets = max;
> +   drvdata->rcdev.ops   = _blk_ctl_reset_ops;
> +   drvdata->rcdev.of_node   = dev->of_node;
> +   drvdata->rcdev.dev   = dev;
> +
> +   drvdata->rst_hws = devm_kcalloc(dev, max, sizeof(struct imx_reset_hw),
> +   GFP_KERNEL);
> +   hws = drvdata->rst_hws;
> +
> +   for (i = 0; i < dev_data->hws_num; i++) {
> +   struct imx_blk_ctl_hw *hw = _data->hws[i];
> +
> +   if (hw->type != BLK_CTL_RESET)
> +   continue;
> +
> +   hws[hw->id].offset = hw->offset;
> +   hws[hw->id].shift = hw->shift;
> +   hws[hw->id].mask = hw->mask;
> +   }
> +
> +   return devm_reset_controller_register(dev, >rcdev);
> +}
> +static struct clk_hw *imx_blk_ctl_register_one_clock(struct device *dev,
> +   struct imx_blk_ctl_hw *hw)
> +{
> +   struct imx_blk_ctl_drvdata *drvdata = dev_get_drvdata(dev);
> +   void __iomem *base = drvdata->base;
> +   struct clk_hw *clk_hw = NULL;
> +
> +   switch (hw->type) {
> +   case BLK_CTL_CLK_MUX:
> +   clk_hw = imx_dev_clk_hw_mux_flags(dev, hw->name,
> + base + hw->offset,
> + hw->shift, hw->width,
> + hw->parents,
> + hw->parents_count,
> + hw->flags);
> +   break;
> +   case BLK_CTL_CLK_GATE:
> +   clk_hw = imx_dev_clk_hw_gate(dev, hw->name, hw->parents,
> +base + hw->offset, hw->shift);
> +   break;
> +   case BLK_CTL_CLK_SHARED_GATE:
> +   clk_hw = imx_dev_clk_hw_gate_shared(dev, hw->name,
> +   hw->parents,
> +   base + hw->offset,
> +   hw->shift,
> +   hw->shared_count);
> +   break;
> +   case BLK_CTL_CLK_PLL14XX:
> +   clk_hw = imx_dev_clk_hw_pll14xx(dev, hw->name, hw->parents,
> +   base + hw->offset, 
> hw->pll_tbl);
> +   break;
> +   };
> +
> +   

Re: [PATCH v3 10/14] clk: imx: Add generic blk-ctl driver

[Dong Aisheng]

On Tue, Sep 8, 2020 at 6:27 PM Abel Vesa  wrote:
>
> The i.MX8MP platform introduces a new type of IP which is called BLK_CTL in
> RM and usually is comprised of some GPRs that are considered too
> generic to be part of any dedicated IP from that specific subsystem.
>
> In general, some of the GPRs have some clock bits, some have reset bits,
> so in order to be able to use the imx clock API, this needs to be
> in a clock driver. From there it can use the reset controller API and
> leave the rest to the syscon.
>
> Signed-off-by: Abel Vesa 
> ---
>  drivers/clk/imx/Makefile  |   2 +-
>  drivers/clk/imx/clk-blk-ctl.c | 297 
> ++
>  drivers/clk/imx/clk-blk-ctl.h |  80 
>  3 files changed, 378 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/clk/imx/clk-blk-ctl.c
>  create mode 100644 drivers/clk/imx/clk-blk-ctl.h
>
> diff --git a/drivers/clk/imx/Makefile b/drivers/clk/imx/Makefile
> index 79e53f2..105c117 100644
> --- a/drivers/clk/imx/Makefile
> +++ b/drivers/clk/imx/Makefile
> @@ -23,7 +23,7 @@ obj-$(CONFIG_MXC_CLK) += mxc-clk.o
>
>  obj-$(CONFIG_CLK_IMX8MM) += clk-imx8mm.o
>  obj-$(CONFIG_CLK_IMX8MN) += clk-imx8mn.o
> -obj-$(CONFIG_CLK_IMX8MP) += clk-imx8mp.o
> +obj-$(CONFIG_CLK_IMX8MP) += clk-imx8mp.o clk-blk-ctl.o
>  obj-$(CONFIG_CLK_IMX8MQ) += clk-imx8mq.o
>
>  obj-$(CONFIG_MXC_CLK_SCU) += clk-imx-scu.o clk-imx-lpcg-scu.o
> diff --git a/drivers/clk/imx/clk-blk-ctl.c b/drivers/clk/imx/clk-blk-ctl.c
> new file mode 100644
> index ..1a6f1eb
> --- /dev/null
> +++ b/drivers/clk/imx/clk-blk-ctl.c
> @@ -0,0 +1,297 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright 2020 NXP.
> + */
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#include "clk.h"
> +#include "clk-blk-ctl.h"
> +
> +struct imx_reset_hw {
> +   u32 offset;
> +   u32 shift;
> +   u32 mask;
> +   volatile unsigned long asserted;

Could you clarify a bit why need 'volatile' here?

> +};
> +
> +struct imx_pm_safekeep_info {
> +   uint32_t *regs_values;
> +   uint32_t *regs_offsets;
> +   uint32_t regs_num;
> +};
> +
> +struct imx_blk_ctl_drvdata {
> +   void __iomem *base;
> +   struct reset_controller_dev rcdev;
> +   struct imx_reset_hw *rst_hws;
> +   struct imx_pm_safekeep_info pm_info;
> +
> +   spinlock_t lock;
> +};
> +
> +static void __maybe_unused imx_blk_ctl_read_write(struct device *dev,
> +   bool write)
> +{
> +   struct imx_blk_ctl_drvdata *drvdata = dev_get_drvdata(dev);
> +   struct imx_pm_safekeep_info *pm_info = >pm_info;
> +   void __iomem *base = drvdata->base;
> +   int i;
> +
> +   if (!pm_info->regs_num)
> +   return;
> +
> +   for (i = 0; i < pm_info->regs_num; i++) {
> +   u32 offset = pm_info->regs_offsets[i];
> +
> +   if (write)
> +   writel(pm_info->regs_values[i], base + offset);
> +   else
> +   pm_info->regs_values[i] = readl(base + offset);
> +   }
> +
> +}
> +
> +static int __maybe_unused imx_blk_ctl_runtime_suspend(struct device *dev)
> +{
> +   imx_blk_ctl_read_write(dev, false);
> +
> +   return 0;
> +}
> +
> +static int __maybe_unused imx_blk_ctl_runtime_resume(struct device *dev)
> +{
> +   imx_blk_ctl_read_write(dev, true);
> +
> +   return 0;
> +}
> +
> +const struct dev_pm_ops imx_blk_ctl_pm_ops = {
> +   SET_RUNTIME_PM_OPS(imx_blk_ctl_runtime_suspend,
> +  imx_blk_ctl_runtime_resume, NULL)
> +   SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
> +  pm_runtime_force_resume)
> +};
> +EXPORT_SYMBOL_GPL(imx_blk_ctl_pm_ops);
> +
> +static int imx_blk_ctl_reset_set(struct reset_controller_dev *rcdev,
> + unsigned long id, bool assert)
> +{
> +   struct imx_blk_ctl_drvdata *drvdata = container_of(rcdev,
> +   struct imx_blk_ctl_drvdata, rcdev);
> +   unsigned int offset = drvdata->rst_hws[id].offset;
> +   unsigned int shift = drvdata->rst_hws[id].shift;
> +   unsigned int mask = drvdata->rst_hws[id].mask;
> +   void __iomem *reg_addr = drvdata->base + offset;
> +   unsigned long flags;
> +   u32 reg;
> +
> +   if (assert && !test_and_set_bit(1, >rst_hws[id].asserted))
> +   pm_runtime_get_sync(rcdev->dev);
> +

i'm a bit confused because each reset hw has a field 'asserted' which means
you're doing bit operations on each separate private variable.
Is that what we want?

BTW, what if user calling deassert first?
Will that cause writing registers without enabling power domain?

> +   spin_lock_irqsave(>lock, flags);
> +
> +   reg = readl(reg_addr);
> +   if (assert)
> +   writel(reg & ~(mask << 

Re: [PATCH v1] driver core: Fix device_pm_lock() locking for device links

[Dong Aisheng]

Hi Saravana

On Tue, Sep 1, 2020 at 6:10 AM Saravana Kannan  wrote:
>
> This commit fixes two issues:
>
> 1. The lockdep warning reported by Dong Aisheng  [1].
>
> It is a warning about a cycle (dpm_list_mtx --> kn->active#3 --> fw_lock)
> that was introduced when device-link devices were added to expose device
> link information in sysfs.
>
> The patch that "introduced" this cycle can't be reverted because it's fixes
> a real SRCU issue and also ensures that the device-link device is deleted
> as soon as the device-link is deleted. This is important to avoid sysfs
> name collisions if the device-link is create again immediately (this can
> happen a lot with deferred probing).
>
> 2. device_link_drop_managed() is not grabbing device_pm_lock().
>
> When device_link_del() calls __device_link_del() (device_link_del() ->
> device_link_put_kref() kref_put() -> __device_link_del()) it grabs the
> device_pm_lock().
>
> However, when device_link_drop_managed() calls __device_link_del()
> (device_link_drop_managed() -> kref_put() -> __device_link_del()) it
> doesn't grab device_pm_lock(). There's nothing special about managed
> device-links that remove the need for grabbing device_pm_lock(). So, this
> patch makes sure device_pm_lock() is always held when deleting managed
> links.
>
> And thanks to Stephen Boyd for helping me understand the lockdep splat.
>
> Fixes: 843e600b8a2b ("driver core: Fix sleeping in invalid context during 
> device link deletion")
> Fixes: 515db266a9da ("driver core: Remove device link creation limitation")
> [1] - 
> https://lore.kernel.org/lkml/CAA+hA=S4eAreb7vo69LAXSk2t5=deknxhaiy1wspk4xtp9u...@mail.gmail.com/
> Reported-by: Dong Aisheng 
> Signed-off-by: Saravana Kannan 

Thanks a lot for the quick fix. It worked for me.

Tested-by: Dong Aisheng 

Regards
Aisheng

> ---
>
> Rafael,
>
> A bigger question I had is why we need to grab device_pm_lock() around
> device_link_del() in the first place. I understand the need to grab it
> during device_link_add() -- it's because we are checking the supplier is
> in the dpm_list and because we are reordering devices on the dpm_list.
>
> But during deletion, we don't need to do either one of those.  So, why
> do we even need to grab the device_pm_lock() in the first place. The
> device_links_write_lock() that we already grab before deleting a device
> link seems like it'd be sufficient. If you agree we don't need to grab
> device_pm_lock() during deletion, then I can change this patch to just
> delete that locking.
>
> -Saravana
>
>  drivers/base/core.c | 8 
>  1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/base/core.c b/drivers/base/core.c
> index f6f620aa9408..de1935e21d97 100644
> --- a/drivers/base/core.c
> +++ b/drivers/base/core.c
> @@ -766,8 +766,10 @@ static void __device_link_del(struct kref *kref)
> if (link->flags & DL_FLAG_PM_RUNTIME)
> pm_runtime_drop_link(link->consumer);
>
> +   device_pm_lock();
> list_del_rcu(>s_node);
> list_del_rcu(>c_node);
> +   device_pm_unlock();
> device_unregister(>link_dev);
>  }
>  #else /* !CONFIG_SRCU */
> @@ -781,8 +783,10 @@ static void __device_link_del(struct kref *kref)
> if (link->flags & DL_FLAG_PM_RUNTIME)
> pm_runtime_drop_link(link->consumer);
>
> +   device_pm_lock();
> list_del(>s_node);
> list_del(>c_node);
> +   device_pm_unlock();
> device_unregister(>link_dev);
>  }
>  #endif /* !CONFIG_SRCU */
> @@ -807,9 +811,7 @@ static void device_link_put_kref(struct device_link *link)
>  void device_link_del(struct device_link *link)
>  {
> device_links_write_lock();
> -   device_pm_lock();
> device_link_put_kref(link);
> -   device_pm_unlock();
> device_links_write_unlock();
>  }
>  EXPORT_SYMBOL_GPL(device_link_del);
> @@ -830,7 +832,6 @@ void device_link_remove(void *consumer, struct device 
> *supplier)
> return;
>
> device_links_write_lock();
> -   device_pm_lock();
>
> list_for_each_entry(link, >links.consumers, s_node) {
> if (link->consumer == consumer) {
> @@ -839,7 +840,6 @@ void device_link_remove(void *consumer, struct device 
> *supplier)
> }
> }
>
> -   device_pm_unlock();
> device_links_write_unlock();
>  }
>  EXPORT_SYMBOL_GPL(device_link_remove);
> --
> 2.28.0.402.g5ffc5be6b7-goog
>

Re: [PATCH 21/22] arm64: dts: imx8qxp: Remove i.MX7 compatible from USDHC

[Dong Aisheng]

On Mon, Aug 24, 2020 at 6:32 PM Krzysztof Kozlowski  wrote:
>
> On Mon, Aug 24, 2020 at 10:31:31AM +, Bough Chen wrote:
> > > -Original Message-----
> > > From: Dong Aisheng [mailto:donga...@gmail.com]
> > > Sent: 2020年8月24日 17:45
> > > To: Krzysztof Kozlowski 
> > > Cc: Aisheng Dong ; devicet...@vger.kernel.org;
> > > linux-ser...@vger.kernel.org; Anson Huang ;
> > > linux-g...@vger.kernel.org; Fabio Estevam ; Linus
> > > Walleij ; linux...@vger.kernel.org;
> > > linux-...@vger.kernel.org; linux-kernel@vger.kernel.org;
> > > linux-...@vger.kernel.org; Bartosz Golaszewski
> > > ; Rob Herring ;
> > > linux-...@lists.infradead.org; dl-linux-imx ; 
> > > Pengutronix
> > > Kernel Team ; Thierry Reding
> > > ; Shawn Guo ; Sascha
> > > Hauer ; linux-arm-ker...@lists.infradead.org;
> > > linux-watch...@vger.kernel.org; Bough Chen 
> > > Subject: Re: [PATCH 21/22] arm64: dts: imx8qxp: Remove i.MX7 compatible
> > > from USDHC
> > >
> > > On Mon, Aug 24, 2020 at 5:15 PM Krzysztof Kozlowski 
> > > wrote:
> > > >
> > > > On Mon, Aug 24, 2020 at 09:00:19AM +, Aisheng Dong wrote:
> > > > > > From: Krzysztof Kozlowski 
> > > > > > Sent: Monday, August 24, 2020 12:16 AM
> > > > > >
> > > > > > The USDHC on i.MX 8QXP has its own compatible described in
> > > > > > bindings and used in the driver (with its own quirks).  Remove
> > > > > > additional fsl,imx7d-usdhc compatible to fix dtbs_check warnings 
> > > > > > like:
> > > > > >
> > > > > >   arch/arm64/boot/dts/freescale/imx8qxp-ai_ml.dt.yaml:
> > > mmc@5b01:
> > > > > > compatible: ['fsl,imx8qxp-usdhc', 'fsl,imx7d-usdhc'] is too long
> > > > > > From schema:
> > > > > > /ocumentation/devicetree/bindings/mmc/fsl-imx-esdhc.yaml
> > > > > >
> > > > > >   arch/arm64/boot/dts/freescale/imx8qxp-ai_ml.dt.yaml:
> > > mmc@5b01:
> > > > > > compatible: Additional items are not allowed
> > > > > > ('fsl,imx7d-usdhc' was
> > > > > > unexpected)
> > > > > >
> > > > > > Signed-off-by: Krzysztof Kozlowski 
> > > > >
> > > > > For Patch 19-22, I think we should fix dt binding doc.
> > > >
> > > > Are you sure that these USDHC controllers are compatible with i.MX 7D?
> > > > Could they really run with fsl,imx7d-usdhc compatible?
> > >
> > > AFAIK uSDHC on QXP is derived from the former platforms with adding a few
> > > more new features. e.g. HS400ES/CMDQ.
> > > Let me loop in uSDHC driver owner Haibo Chen to double confirm.
> >
> > Yes, usdhc of imx8qxp can work by using the compatible "fsl, imx7d-usdhc", 
> > but will not support HS400ES/Command Queue any more. Also imx8qxp support 
> > Auto CMD23, but imx7d not.
> > And imx8qxp need to re-config the clock rate after system PM, imx7d do not 
> > need to do this.
>
> Then we can leave the compatible in DTS and I will correct the device
> tree schema.

Haibo,

As Krzysztof is helping fix uSDHC binding doc in Patch 12/22,
please double check with IC for the whole uSDHC derive relationships on i.MX
and feedback to Krzysztof if anything is wrong.

Regards
Aisheng

>
> Best regards,
> Krzysztof

Re: [PATCH 21/22] arm64: dts: imx8qxp: Remove i.MX7 compatible from USDHC

[Dong Aisheng]

On Mon, Aug 24, 2020 at 5:15 PM Krzysztof Kozlowski  wrote:
>
> On Mon, Aug 24, 2020 at 09:00:19AM +, Aisheng Dong wrote:
> > > From: Krzysztof Kozlowski 
> > > Sent: Monday, August 24, 2020 12:16 AM
> > >
> > > The USDHC on i.MX 8QXP has its own compatible described in bindings and
> > > used in the driver (with its own quirks).  Remove additional 
> > > fsl,imx7d-usdhc
> > > compatible to fix dtbs_check warnings like:
> > >
> > >   arch/arm64/boot/dts/freescale/imx8qxp-ai_ml.dt.yaml: mmc@5b01:
> > > compatible: ['fsl,imx8qxp-usdhc', 'fsl,imx7d-usdhc'] is too long
> > > From schema:
> > > /ocumentation/devicetree/bindings/mmc/fsl-imx-esdhc.yaml
> > >
> > >   arch/arm64/boot/dts/freescale/imx8qxp-ai_ml.dt.yaml: mmc@5b01:
> > > compatible: Additional items are not allowed ('fsl,imx7d-usdhc' was
> > > unexpected)
> > >
> > > Signed-off-by: Krzysztof Kozlowski 
> >
> > For Patch 19-22, I think we should fix dt binding doc.
>
> Are you sure that these USDHC controllers are compatible with i.MX 7D?
> Could they really run with fsl,imx7d-usdhc compatible?

AFAIK uSDHC on QXP is derived from the former platforms with adding a few
more new features. e.g. HS400ES/CMDQ.
Let me loop in uSDHC driver owner Haibo Chen to double confirm.

Regards
Aisheng

> The implementation (Linux kernel driver) is different, I guess on
> purpose...
>
> Best regards,
> Krzysztof
>
> >
> > Regards
> > Aisheng
> >
> > > ---
> > >  arch/arm64/boot/dts/freescale/imx8qxp.dtsi | 6 +++---
> > >  1 file changed, 3 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/arch/arm64/boot/dts/freescale/imx8qxp.dtsi
> > > b/arch/arm64/boot/dts/freescale/imx8qxp.dtsi
> > > index 61bccb69f09e..26c4fcdfe290 100644
> > > --- a/arch/arm64/boot/dts/freescale/imx8qxp.dtsi
> > > +++ b/arch/arm64/boot/dts/freescale/imx8qxp.dtsi
> > > @@ -362,7 +362,7 @@
> > > };
> > >
> > > usdhc1: mmc@5b01 {
> > > -   compatible = "fsl,imx8qxp-usdhc", "fsl,imx7d-usdhc";
> > > +   compatible = "fsl,imx8qxp-usdhc";
> > > interrupts = ;
> > > reg = <0x5b01 0x1>;
> > > clocks = <_lpcg IMX_CONN_LPCG_SDHC0_IPG_CLK>, @@
> > > -374,7 +374,7 @@
> > > };
> > >
> > > usdhc2: mmc@5b02 {
> > > -   compatible = "fsl,imx8qxp-usdhc", "fsl,imx7d-usdhc";
> > > +   compatible = "fsl,imx8qxp-usdhc";
> > > interrupts = ;
> > > reg = <0x5b02 0x1>;
> > > clocks = <_lpcg IMX_CONN_LPCG_SDHC1_IPG_CLK>, @@
> > > -388,7 +388,7 @@
> > > };
> > >
> > > usdhc3: mmc@5b03 {
> > > -   compatible = "fsl,imx8qxp-usdhc", "fsl,imx7d-usdhc";
> > > +   compatible = "fsl,imx8qxp-usdhc";
> > > interrupts = ;
> > > reg = <0x5b03 0x1>;
> > > clocks = <_lpcg IMX_CONN_LPCG_SDHC2_IPG_CLK>,
> > > --
> > > 2.17.1
> >
>
> ___
> linux-arm-kernel mailing list
> linux-arm-ker...@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Lockdep warning caused by "driver core: Fix sleeping in invalid context during device link deletion"

[Dong Aisheng]

Hi ALL,

We met the below WARNING during system suspend on an iMX6Q SDB board
with the latest linus/master branch (v5.9-rc1+) and next-20200820.
v5.8 kernel is ok. So i did bisect and finally found it's caused by
the patch below.
Reverting it can get rid of the warning, but I wonder if there may be
other potential issues.
Any ideas?

Defconfig used is: imx_v6_v7_defconfig

commit 843e600b8a2b01463c4d873a90b2c2ea8033f1f6
Author: Saravana Kannan 
Date:   Thu Jul 16 14:45:23 2020 -0700

driver core: Fix sleeping in invalid context during device link deletion

Marek and Guenter reported that commit 287905e68dd2 ("driver core:
Expose device link details in sysfs") caused sleeping/scheduling while
atomic warnings.

BUG: sleeping function called from invalid context at
kernel/locking/mutex.c:935
in_atomic(): 1, irqs_disabled(): 0, non_block: 0, pid: 12, name: kworker/0:1
2 locks held by kworker/0:1/12:
  #0: ee8074a8 ((wq_completion)rcu_gp){+.+.}-{0:0}, at:
process_one_work+0x174/0x7dc
  #1: ee921f20 ((work_completion)(>work)){+.+.}-{0:0}, at:
process_one_work+0x174/0x7dc
Preemption disabled at:
[] srcu_invoke_callbacks+0xc0/0x154
- 8< - SNIP
[] (device_del) from [] (device_unregister+0x24/0x64)
[] (device_unregister) from []
(srcu_invoke_callbacks+0xcc/0x154)
[] (srcu_invoke_callbacks) from []
(process_one_work+0x234/0x7dc)
[] (process_one_work) from [] (worker_thread+0x44/0x51c)
[] (worker_thread) from [] (kthread+0x158/0x1a0)
[] (kthread) from [] (ret_from_fork+0x14/0x20)
Exception stack(0xee921fb0 to 0xee921ff8)

This was caused by the device link device being released in the context
of srcu_invoke_callbacks().  There is no need to wait till the RCU
callback to release the device link device.  So release the device
earlier and move the call_srcu() into the device release code. That way,
the memory will get freed only after the device is released AND the RCU
callback is called.

Fixes: 287905e68dd2 ("driver core: Expose device link details in sysfs")
Reported-by: Marek Szyprowski 
Reported-by: Guenter Roeck 
Reported-by: Naresh Kamboju 
Signed-off-by: Saravana Kannan 
Tested-by: Marek Szyprowski 
Tested-by: Guenter Roeck 
Link: 
https://lore.kernel.org/r/20200716214523.2924704-1-sarava...@google.com
Signed-off-by: Greg Kroah-Hartman 

Error log:
# echo mem > /sys/power/state
[   39.111865] PM: suspend entry (deep)
[   39.148650] Filesystems sync: 0.032 seconds
[   39.154034]
[   39.155537] ==
[   39.161723] WARNING: possible circular locking dependency detected
[   39.167911] 5.9.0-rc1-00103-g7eac66d0456f #37 Not tainted
[   39.173315] --
[   39.179500] sh/647 is trying to acquire lock:
[   39.183862] c15a310c (dpm_list_mtx){+.+.}-{3:3}, at:
dpm_for_each_dev+0x20/0x5c
[   39.191200]
[   39.191200] but task is already holding lock:
[   39.197036] c15a37e4 (fw_lock){+.+.}-{3:3}, at: fw_pm_notify+0x90/0xd4
[   39.203582]
[   39.203582] which lock already depends on the new lock.
[   39.203582]
[   39.211763]
[   39.211763] the existing dependency chain (in reverse order) is:
[   39.219249]
[   39.219249] -> #2 (fw_lock){+.+.}-{3:3}:
[   39.224673]mutex_lock_nested+0x1c/0x24
[   39.229126]firmware_uevent+0x18/0xa0
[   39.233411]dev_uevent+0xc4/0x1f8
[   39.237343]uevent_show+0x98/0x114
[   39.241362]dev_attr_show+0x18/0x48
[   39.245472]sysfs_kf_seq_show+0x84/0xec
[   39.249927]seq_read+0x138/0x550
[   39.253774]vfs_read+0x94/0x164
[   39.257529]ksys_read+0x60/0xe8
[   39.261288]ret_fast_syscall+0x0/0x28
[   39.265564]0xbed7c808
[   39.268538]
[   39.268538] -> #1 (kn->active#3){}-{0:0}:
[   39.274391]kernfs_remove_by_name_ns+0x40/0x94
[   39.279450]device_del+0x144/0x3fc
[   39.283467]__device_link_del+0x4c/0x70
[   39.287919]device_link_remove+0x5c/0x8c
[   39.292464]_regulator_put.part.0+0x104/0x1dc
[   39.297436]regulator_put+0x2c/0x3c
[   39.299731] regulator regulator.5: Failed to increase supply voltage: -110
[   39.301544]release_nodes+0x1b4/0x204
[   39.301553]really_probe+0x104/0x3b4
[   39.316881]driver_probe_device+0x58/0xb4
[   39.321506]device_driver_attach+0x58/0x60
[   39.326217]__driver_attach+0x58/0xd0
[   39.330499]bus_for_each_dev+0x74/0xbc
[   39.334863]bus_add_driver+0x150/0x1dc
[   39.339227]driver_register+0x74/0x108
[   39.343599]i2c_register_driver+0x38/0x8c
[   39.348227]do_one_initcall+0x84/0x3b4
[   39.352598]kernel_init_freeable+0x154/0x1e4
[   39.357485]kernel_init+0x8/0x118
[   39.361415]ret_from_fork+0x14/0x20
[   39.365518]0x0
[   39.367883]
[   39.367883] -> #0 (dpm_list_mtx){+.+.}-{3:3}:

Re: [PATCH v2 11/17] clk: imx: Add blk_ctrl combo driver

[Dong Aisheng]

On Thu, Aug 20, 2020 at 4:31 AM Abel Vesa  wrote:

> > > +extern const struct imx_blk_ctrl_dev_data imx8mp_audio_blk_ctrl_dev_data 
> > > __initconst;
> > > +extern const struct imx_blk_ctrl_dev_data imx8mp_media_blk_ctrl_dev_data 
> > > __initconst;
> > > +extern const struct imx_blk_ctrl_dev_data imx8mp_hdmi_blk_ctrl_dev_data 
> > > __initconst;
> > > +
> >
> > If no special reasons, i may prefer to put those data in either
> > clk-blk-ctrl.c or separate clk-blk-ctrl-data.c
> > because there seems to be no code level dependency on the CCM
> > driver(clk-imx8mq.c) for clk_blk_ctrl drivers.
> > Then we can save those extern variables.
> >
>
> The rationale here is to have the SoC specific definitions in the SoC specific
> clock provider driver. Otherwise with every new SoC that will use the blk_ctl
> IPs we will increase the size of clk-blk-ctrl driver. Plus the kernel names of
> the clocks used by each blk_ctl IP as parents are also defined in the SoC
> specific clock provider driver.
>
> I'll find a way, though, to move those so that they would not be shared 
> between
> all the clock drivers that needs a blk_ctl implementation.
>

Please refer to pinctrl-imx.c to see if we can do similar things for blk-ctrl.

Regards
Aisheng

> > Regards
> > Aisheng
> >
> > > +#endif
> > > +
> > > --
> > > 2.7.4
> > >

Re: [PATCH v2 16/17] arm64: dts: imx8mp: Add media_blk_ctrl node

[Dong Aisheng]

Hi Rob, Stephen,

On Thu, Aug 20, 2020 at 4:37 AM Abel Vesa  wrote:
>
> On 20-08-18 19:34:14, Dong Aisheng wrote:
> > On Fri, Aug 14, 2020 at 8:12 PM Abel Vesa  wrote:
> > >
> > > Some of the features of the media_ctrl will be used by some
> > > different drivers in a way those drivers will know best, so adding the
> > > syscon compatible we allow those to do just that. Only the resets
> > > and the clocks are registered bit the clk-blk-ctrl driver.
> > >
> > > Signed-off-by: Abel Vesa 
> > > ---
> > >  arch/arm64/boot/dts/freescale/imx8mp.dtsi | 16 
> > >  1 file changed, 16 insertions(+)
> > >
> > > diff --git a/arch/arm64/boot/dts/freescale/imx8mp.dtsi 
> > > b/arch/arm64/boot/dts/freescale/imx8mp.dtsi
> > > index dede0ae..2d6d213 100644
> > > --- a/arch/arm64/boot/dts/freescale/imx8mp.dtsi
> > > +++ b/arch/arm64/boot/dts/freescale/imx8mp.dtsi
> > > @@ -736,6 +736,22 @@
> > > };
> > > };
> > >
> > > +   aips4: bus@32c0 {
> > > +   compatible = "simple-bus";
> > > +   reg = <0x32c0 0x40>;
> > > +   #address-cells = <1>;
> > > +   #size-cells = <1>;
> > > +   ranges;
> > > +
> > > +   media_blk_ctrl: clock-controller@32ec {
> >
> > For this combo device, maybe we can directly name it as blk-ctrl@32ec.
> > Rob, do you think if we can do that?
> >
>
> I think it was Stephen who suggested we change it to clock-controller in the
> last's version thread.
>
> TBH, I agree with you here, since it makes more sense to be called blk-ctrl
> provided that this is not really just a clock controller.
>

How do you think?

Regards
Aisheng

> > > +   compatible = "fsl,imx8mp-media-blk-ctrl", 
> > > "syscon";
> > > +   reg = <0x32ec 0x1>;
> > > +
> >
> > Remove unnecessary blank line
> >
>
> Will do.
>
> > Otherwise:
> > Reviewed-by: Dong Aisheng 
> >
> > Regards
> > Aisheng
> >
> > > +   #clock-cells = <1>;
> > > +   #reset-cells = <1>;
> > > +   };
> > > +   };
> > > +
> > > aips5: bus@30c0 {
> > > compatible = "fsl,aips-bus", "simple-bus";
> > > reg = <0x30c0 0x40>;
> > > --
> > > 2.7.4
> > >

Re: [PATCH v2 16/17] arm64: dts: imx8mp: Add media_blk_ctrl node

[Dong Aisheng]

On Fri, Aug 14, 2020 at 8:12 PM Abel Vesa  wrote:
>
> Some of the features of the media_ctrl will be used by some
> different drivers in a way those drivers will know best, so adding the
> syscon compatible we allow those to do just that. Only the resets
> and the clocks are registered bit the clk-blk-ctrl driver.
>
> Signed-off-by: Abel Vesa 
> ---
>  arch/arm64/boot/dts/freescale/imx8mp.dtsi | 16 
>  1 file changed, 16 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/freescale/imx8mp.dtsi 
> b/arch/arm64/boot/dts/freescale/imx8mp.dtsi
> index dede0ae..2d6d213 100644
> --- a/arch/arm64/boot/dts/freescale/imx8mp.dtsi
> +++ b/arch/arm64/boot/dts/freescale/imx8mp.dtsi
> @@ -736,6 +736,22 @@
> };
> };
>
> +   aips4: bus@32c0 {
> +   compatible = "simple-bus";
> +   reg = <0x32c0 0x40>;
> +   #address-cells = <1>;
> +   #size-cells = <1>;
> +   ranges;
> +
> +   media_blk_ctrl: clock-controller@32ec {

For this combo device, maybe we can directly name it as blk-ctrl@32ec.
Rob, do you think if we can do that?

> +   compatible = "fsl,imx8mp-media-blk-ctrl", 
> "syscon";
> +   reg = <0x32ec 0x1>;
> +

Remove unnecessary blank line

Otherwise:
Reviewed-by: Dong Aisheng 

Regards
Aisheng

> +   #clock-cells = <1>;
> +   #reset-cells = <1>;
> +   };
> +   };
> +
> aips5: bus@30c0 {
> compatible = "fsl,aips-bus", "simple-bus";
> reg = <0x30c0 0x40>;
> --
> 2.7.4
>

Re: [PATCH v2 15/17] arm64: dts: imx8mp: Add audio_blk_ctrl node

[Dong Aisheng]

On Fri, Aug 14, 2020 at 8:12 PM Abel Vesa  wrote:
>
> Some of the features of the audio_ctrl will be used by some
> different drivers in a way those drivers will know best, so adding the
> syscon compatible we allow those to do just that. Only the resets
> and the clocks are registered bit the clk-blk-ctrl driver.
>
> Signed-off-by: Abel Vesa 
> ---
>  arch/arm64/boot/dts/freescale/imx8mp.dtsi | 16 
>  1 file changed, 16 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/freescale/imx8mp.dtsi 
> b/arch/arm64/boot/dts/freescale/imx8mp.dtsi
> index daa1769..dede0ae 100644
> --- a/arch/arm64/boot/dts/freescale/imx8mp.dtsi
> +++ b/arch/arm64/boot/dts/freescale/imx8mp.dtsi
> @@ -736,6 +736,22 @@
> };
> };
>
> +   aips5: bus@30c0 {
> +   compatible = "fsl,aips-bus", "simple-bus";
> +   reg = <0x30c0 0x40>;
> +   #address-cells = <1>;
> +   #size-cells = <1>;
> +   ranges;
> +
> +   audio_blk_ctrl: clock-controller@30e2 {
> +   compatible = "fsl,imx8mp-audio-blk-ctrl", 
> "syscon";
> +   reg = <0x30e2 0x50C>;

0x50c

> +

remove unnecessary blank line

Otherwise:
Reviewed-by: Dong Aisheng 

Regards
Aisheng

> +   #clock-cells = <1>;
> +   #reset-cells = <1>;
> +   };
> +   };
> +
> gic: interrupt-controller@3880 {
> compatible = "arm,gic-v3";
> reg = <0x3880 0x1>,
> --
> 2.7.4
>

Re: [PATCH v2 12/17] clk: imx8mp: Add audio blk_ctrl clocks and resets

[Dong Aisheng]

On Fri, Aug 14, 2020 at 8:12 PM Abel Vesa  wrote:
>
> Add audio blk_ctrl clocks and resets in the i.MX8MP clock
> driver to be picked up by the clk-blk-ctrl driver.
>
> Signed-off-by: Abel Vesa 
> ---
>  drivers/clk/imx/clk-blk-ctrl.c |   4 ++
>  drivers/clk/imx/clk-imx8mp.c   | 138 
> +

As i mentioned in Patch 11, I wonder we probably better to put blk
ctrl clk data in blk ctrl driver.
Otherwise, i'm okay with the code.

Regards
Aisheng

>  2 files changed, 142 insertions(+)
>
> diff --git a/drivers/clk/imx/clk-blk-ctrl.c b/drivers/clk/imx/clk-blk-ctrl.c
> index 1672646..1c2991c 100644
> --- a/drivers/clk/imx/clk-blk-ctrl.c
> +++ b/drivers/clk/imx/clk-blk-ctrl.c
> @@ -312,6 +312,10 @@ static const struct dev_pm_ops imx_blk_ctrl_pm_ops = {
>  };
>
>  static const struct of_device_id imx_blk_ctrl_of_match[] = {
> +   {
> +   .compatible = "fsl,imx8mp-audio-blk-ctrl",
> +   .data = _audio_blk_ctrl_dev_data
> +   },
> { /* Sentinel */ }
>  };
>  MODULE_DEVICE_TABLE(of, imx_blk_ctrl_of_match);
> diff --git a/drivers/clk/imx/clk-imx8mp.c b/drivers/clk/imx/clk-imx8mp.c
> index 462c558..00e7f5e 100644
> --- a/drivers/clk/imx/clk-imx8mp.c
> +++ b/drivers/clk/imx/clk-imx8mp.c
> @@ -4,6 +4,7 @@
>   */
>
>  #include 
> +#include 
>  #include 
>  #include 
>  #include 
> @@ -14,11 +15,148 @@
>  #include 
>
>  #include "clk.h"
> +#include "clk-blk-ctrl.h"
> +
> +#defineIMX_AUDIO_BLK_CTRL_CLKEN0   0x0
> +#defineIMX_AUDIO_BLK_CTRL_CLKEN1   0x4
> +#defineIMX_AUDIO_BLK_CTRL_EARC 0x200
> +#defineIMX_AUDIO_BLK_CTRL_SAI1_MCLK_SEL0x300
> +#defineIMX_AUDIO_BLK_CTRL_SAI2_MCLK_SEL0x304
> +#defineIMX_AUDIO_BLK_CTRL_SAI3_MCLK_SEL0x308
> +#defineIMX_AUDIO_BLK_CTRL_SAI5_MCLK_SEL0x30C
> +#defineIMX_AUDIO_BLK_CTRL_SAI6_MCLK_SEL0x310
> +#defineIMX_AUDIO_BLK_CTRL_SAI7_MCLK_SEL0x314
> +#defineIMX_AUDIO_BLK_CTRL_PDM_CLK  0x318
> +#defineIMX_AUDIO_BLK_CTRL_SAI_PLL_GNRL_CTL 0x400
> +#defineIMX_AUDIO_BLK_CTRL_SAI_PLL_FDIVL_CTL0   0x404
> +#defineIMX_AUDIO_BLK_CTRL_SAI_PLL_FDIVL_CTL1   0x408
> +#defineIMX_AUDIO_BLK_CTRL_SAI_PLL_SSCG_CTL 0x40C
> +#defineIMX_AUDIO_BLK_CTRL_SAI_PLL_MNIT_CTL 0x410
> +#defineIMX_AUDIO_BLK_CTRL_IPG_LP_CTRL  0x504
> +
> +#define IMX_MEDIA_BLK_CTRL_SFT_RSTN0x0
> +#define IMX_MEDIA_BLK_CTRL_CLK_EN  0x4
>
>  static u32 share_count_nand;
>  static u32 share_count_media;
>  static u32 share_count_audio;
>
> +static int shared_count_pdm;
> +
> +static const struct imx_pll14xx_rate_table imx_blk_ctrl_sai_pll_tbl[] = {
> +   PLL_1443X_RATE(65000U, 325, 3, 2, 0),
> +};
> +
> +static const struct imx_pll14xx_clk imx_blk_ctrl_sai_pll = {
> +   .type = PLL_1443X,
> +   .rate_table = imx_blk_ctrl_sai_pll_tbl,
> +};
> +
> +static const char * const imx_sai_mclk2_sels[] = {"sai1_root", "sai2_root", 
> "sai3_root", "dummy",
> +  "sai5_root", "sai6_root", 
> "sai7_root", "sai1_mclk",
> +  "sai2_mclk", "sai3_mclk", "dummy",
> +  "sai5_mclk", "sai6_mclk", 
> "sai7_mclk", "spdif1_ext_clk"};
> +static const char * const imx_sai1_mclk1_sels[] = {"sai1_root", "sai1_mclk", 
> };
> +static const char * const imx_sai2_mclk1_sels[] = {"sai2_root", "sai2_mclk", 
> };
> +static const char * const imx_sai3_mclk1_sels[] = {"sai3_root", "sai3_mclk", 
> };
> +static const char * const imx_sai5_mclk1_sels[] = {"sai5_root", "sai5_mclk", 
> };
> +static const char * const imx_sai6_mclk1_sels[] = {"sai6_root", "sai6_mclk", 
> };
> +static const char * const imx_sai7_mclk1_sels[] = {"sai7_root", "sai7_mclk", 
> };
> +static const char * const imx_pdm_sels[] = {"pdm_root", "sai_pll_div2", 
> "dummy", "dummy" };
> +static const char * const imx_sai_pll_ref_sels[] = {"osc_24m", "dummy", 
> "dummy", "dummy", };
> +static const char * const imx_sai_pll_bypass_sels[] = {"sai_pll", 
> "sai_pll_ref_sel", };
> +
> +static struct imx_blk_ctrl_hw imx8mp_audio_blk_ctrl_hws[] = {
> +   /* clocks */
> +   IMX_BLK_CTRL_CLK_MUX("sai_pll_ref_sel", 
> IMX8MP_CLK_AUDIO_BLK_CTRL_SAI_PLL_REF_SEL, 0x400, 0, 2, imx_sai_pll_ref_sels),
> +   IMX_BLK_CTRL_CLK_PLL14XX("sai_pll", 
> IMX8MP_CLK_AUDIO_BLK_CTRL_SAI_PLL, 0x400, "sai_pll_ref_sel", 
> _blk_ctrl_sai_pll),
> +   IMX_BLK_CTRL_CLK_MUX_FLAGS("sai_pll_bypass", 
> IMX8MP_CLK_AUDIO_BLK_CTRL_SAI_PLL_BYPASS, 0x400, 4, 1, 
> imx_sai_pll_bypass_sels, CLK_SET_RATE_PARENT),
> +   IMX_BLK_CTRL_CLK_GATE("sai_pll_out", 
> IMX8MP_CLK_AUDIO_BLK_CTRL_SAI_PLL_OUT, 0x400, 13, "sai_pll_bypass"),
> +   IMX_BLK_CTRL_CLK_MUX_FLAGS("sai1_mclk1_sel", 
> IMX8MP_CLK_AUDIO_BLK_CTRL_SAI1_MCLK1_SEL, 0x300, 0, 1, imx_sai1_mclk1_sels, 
> 

Re: [PATCH v2 11/17] clk: imx: Add blk_ctrl combo driver

[Dong Aisheng]

On Fri, Aug 14, 2020 at 8:12 PM Abel Vesa  wrote:
>
> On i.MX8MP, there is a new type of IP which is called BLK_CTRL in
> RM and usually is comprised of some GPRs that are considered too
> generic to be part of any dedicated IP from that specific subsystem.
>
> In general, some of the GPRs have some clock bits, some have reset bits,
> so in order to be able to use the imx clock API, this needs to be
> in a clock driver. From there it can use the reset controller API and
> leave the rest to the syscon.
>
> This driver is intended to work with the following BLK_CTRL IPs found in
> i.MX8MP (but it might be reused by the future i.MX platforms that
> have this kind of IP in their design):
>  - Audio
>  - Media
>  - HDMI
>
> Signed-off-by: Abel Vesa 

The code mostly looks good to me.
Only a few minor comments.

> ---
>  drivers/clk/imx/Makefile   |   2 +-
>  drivers/clk/imx/clk-blk-ctrl.c | 327 
> +
>  drivers/clk/imx/clk-blk-ctrl.h |  81 ++
>  3 files changed, 409 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/clk/imx/clk-blk-ctrl.c
>  create mode 100644 drivers/clk/imx/clk-blk-ctrl.h
>
> diff --git a/drivers/clk/imx/Makefile b/drivers/clk/imx/Makefile
> index 928f874c..7afe1df 100644
> --- a/drivers/clk/imx/Makefile
> +++ b/drivers/clk/imx/Makefile
> @@ -27,7 +27,7 @@ obj-$(CONFIG_MXC_CLK_SCU) += \
>
>  obj-$(CONFIG_CLK_IMX8MM) += clk-imx8mm.o
>  obj-$(CONFIG_CLK_IMX8MN) += clk-imx8mn.o
> -obj-$(CONFIG_CLK_IMX8MP) += clk-imx8mp.o
> +obj-$(CONFIG_CLK_IMX8MP) += clk-imx8mp.o clk-blk-ctrl.o
>  obj-$(CONFIG_CLK_IMX8MQ) += clk-imx8mq.o
>  obj-$(CONFIG_CLK_IMX8QXP) += clk-imx8qxp.o clk-imx8qxp-lpcg.o
>
> diff --git a/drivers/clk/imx/clk-blk-ctrl.c b/drivers/clk/imx/clk-blk-ctrl.c
> new file mode 100644
> index ..1672646
> --- /dev/null
> +++ b/drivers/clk/imx/clk-blk-ctrl.c
> @@ -0,0 +1,327 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright 2020 NXP.
> + */
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#include "clk.h"
> +#include "clk-blk-ctrl.h"
> +
> +struct reset_hw {

s/reset_hw/imx_reset_hw
It helps the reader to quickly understand it's not core structure

> +   u32 offset;
> +   u32 shift;
> +   u32 mask;
> +   bool asserted;
> +};
> +
> +struct pm_safekeep_info {

imx_pm_safekeep_info

> +   uint32_t *regs_values;
> +   uint32_t *regs_offsets;
> +   uint32_t regs_num;
> +};
> +
> +struct imx_blk_ctrl_drvdata {
> +   void __iomem *base;
> +   struct reset_controller_dev rcdev;
> +   struct reset_hw *rst_hws;
> +   struct pm_safekeep_info pm_info;
> +
> +   spinlock_t lock;
> +};
> +
> +static int imx_blk_ctrl_reset_set(struct reset_controller_dev *rcdev,
> + unsigned long id, bool assert)
> +{
> +   struct imx_blk_ctrl_drvdata *drvdata = container_of(rcdev,
> +   struct imx_blk_ctrl_drvdata, rcdev);
> +   unsigned int offset = drvdata->rst_hws[id].offset;
> +   unsigned int shift = drvdata->rst_hws[id].shift;
> +   unsigned int mask = drvdata->rst_hws[id].mask;
> +   void __iomem *reg_addr = drvdata->base + offset;
> +   unsigned long flags;
> +   unsigned int asserted_before = 0, asserted_after = 0;

swap above two lines from long to short

> +   u32 reg;
> +   int i;
> +
> +   spin_lock_irqsave(>lock, flags);
> +
> +   for (i = 0; i < drvdata->rcdev.nr_resets; i++)
> +   if (drvdata->rst_hws[i].asserted)
> +   asserted_before++;
> +
> +   if (asserted_before == 0 && assert)
> +   pm_runtime_get(rcdev->dev);
> +
> +   if (assert) {
> +   reg = readl(reg_addr);
> +   writel(reg & ~(mask << shift), reg_addr);
> +   drvdata->rst_hws[id].asserted = true;
> +   } else {
> +   reg = readl(reg_addr);
> +   writel(reg | (mask << shift), reg_addr);
> +   drvdata->rst_hws[id].asserted = false;
> +   }
> +
> +   for (i = 0; i < drvdata->rcdev.nr_resets; i++)
> +   if (drvdata->rst_hws[i].asserted)
> +   asserted_after++;

I guess the logic may be able to be simplified.
For example, put assert ref count in the private structure.
Then
call pm_runtime_get when ref count is 0 and assert is true.
call pm_runtime_put when ref ount is 0 and assert is false.
No need to go through twice for loop each time.

> +
> +   if (asserted_before == 1 && asserted_after == 0)
> +   pm_runtime_put(rcdev->dev);
> +
> +   spin_unlock_irqrestore(>lock, flags);
> +
> +   return 0;
> +}
> +
> +static int imx_blk_ctrl_reset_assert(struct reset_controller_dev *rcdev,
> +  unsigned long id)
> +{
> +   return imx_blk_ctrl_reset_set(rcdev, id, true);
> +}

Re: [PATCH v2 10/17] Documentation: bindings: clk: Add bindings for i.MX BLK_CTRL

[Dong Aisheng]

On Fri, Aug 14, 2020 at 8:14 PM Abel Vesa  wrote:
>
> Document the i.MX BLK_CTRL with its devicetree properties.
>
> Signed-off-by: Abel Vesa 

Reviewed-by: Dong Aisheng 

Regards
Aisheng

Re: [PATCH v2 09/17] arm64: dts: Remove imx-hdmimix-reset header file

[Dong Aisheng]

On Fri, Aug 14, 2020 at 8:13 PM Abel Vesa  wrote:
>
> The hdmi BLK_CTRL ids have been moved to imx8mp-reset.h
>
> Signed-off-by: Abel Vesa 

The change seems do not comply with the patch title?

Regards
Aisheng

> ---
>  arch/arm64/boot/dts/freescale/imx8mp.dtsi | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/arch/arm64/boot/dts/freescale/imx8mp.dtsi 
> b/arch/arm64/boot/dts/freescale/imx8mp.dtsi
> index 9de2aa1..daa1769 100644
> --- a/arch/arm64/boot/dts/freescale/imx8mp.dtsi
> +++ b/arch/arm64/boot/dts/freescale/imx8mp.dtsi
> @@ -4,6 +4,7 @@
>   */
>
>  #include 
> +#include 
>  #include 
>  #include 
>  #include 
> --
> 2.7.4
>

Re: [PATCH v2 08/17] clk: imx8mp: Add audio shared gate

[Dong Aisheng]

On Fri, Aug 14, 2020 at 8:12 PM Abel Vesa  wrote:
>
> According to the RM, the CCGR101 is shared for the following root clocks:
> - AUDIO_AHB_CLK_ROOT
> - AUDIO_AXI_CLK_ROOT
> - SAI2_CLK_ROOT
> - SAI3_CLK_ROOT
> - SAI5_CLK_ROOT
> - SAI6_CLK_ROOT
> - SAI7_CLK_ROOT
> - PDM_CLK_ROOT
>
> Signed-off-by: Abel Vesa 

Reviewed-by: Dong Aisheng 

Regards
Aisheng

Re: [PATCH v2 07/17] dt-bindings: reset: imx8mp: Add hdmi blk_ctrl reset IDs

[Dong Aisheng]

On Fri, Aug 14, 2020 at 8:13 PM Abel Vesa  wrote:
>
> These will be used imx8mp for blk-ctrl driver.
>
> Signed-off-by: Abel Vesa 
> Acked-by: Rob Herring 

Reviewed-by: Dong Aisheng 

Regards
Aisheng

Re: [PATCH v2 05/17] dt-bindings: reset: imx8mp: Add media blk_ctrl reset IDs

[Dong Aisheng]

On Fri, Aug 14, 2020 at 8:13 PM Abel Vesa  wrote:
>
> These will be used by the imx8mp for blk-ctrl driver.
>
> Signed-off-by: Abel Vesa 
> Acked-by: Rob Herring 

Reviewed-by: Dong Aisheng 

Regards
Aisheng

Re: [PATCH v2 04/17] dt-bindings: clock: imx8mp: Add media blk_ctrl clock IDs

[Dong Aisheng]

On Fri, Aug 14, 2020 at 8:14 PM Abel Vesa  wrote:
>
> These will be used by the imx8mp for blk-ctrl driver.
>
> Signed-off-by: Abel Vesa 
> Acked-by: Rob Herring 

Reviewed-by: Dong Aisheng 

Regards
Aisheng

Re: [PATCH v2 03/17] dt-bindings: clock: imx8mp: Add ids for the audio shared gate

[Dong Aisheng]

On Fri, Aug 14, 2020 at 8:13 PM Abel Vesa  wrote:
>
> All these IDs are for one single HW gate (CCGR101) that is shared
> between these root clocks.
>
> Signed-off-by: Abel Vesa 
> Acked-by: Rob Herring 

Reviewed-by: Dong Aisheng 

Regards
Aisheng

Re: [PATCH v2 02/17] dt-bindings: reset: imx8mp: Add audio blk_ctrl reset IDs

[Dong Aisheng]

On Fri, Aug 14, 2020 at 8:13 PM Abel Vesa  wrote:
>
> These will be used by the imx8mp for blk-ctrl driver.
>
> Signed-off-by: Abel Vesa 

Reviewed-by: Dong Aisheng 

Regards
Aisheng

Re: [PATCH v2 01/17] dt-bindings: clocks: imx8mp: Rename audiomix ids clocks to audio_blk_ctrl

[Dong Aisheng]

On Fri, Aug 14, 2020 at 8:14 PM Abel Vesa  wrote:
>
> In the reference manual the actual name is Audio BLK_CTRL.
> Lets make it more obvious here by renaming from audiomix to audio_blk_ctrl.
>
> Signed-off-by: Abel Vesa 

As there's still no users of the old definitions, it's okay for me to
change it now.
Reviewed-by: Dong Aisheng 

Regards
Aisheng

> ---
>  include/dt-bindings/clock/imx8mp-clock.h | 120 
> +++
>  1 file changed, 60 insertions(+), 60 deletions(-)
>
> diff --git a/include/dt-bindings/clock/imx8mp-clock.h 
> b/include/dt-bindings/clock/imx8mp-clock.h
> index 7a23f28..6008f32 100644
> --- a/include/dt-bindings/clock/imx8mp-clock.h
> +++ b/include/dt-bindings/clock/imx8mp-clock.h
> @@ -324,66 +324,66 @@
>
>  #define IMX8MP_CLK_END 313
>
> -#define IMX8MP_CLK_AUDIOMIX_SAI1_IPG   0
> -#define IMX8MP_CLK_AUDIOMIX_SAI1_MCLK1 1
> -#define IMX8MP_CLK_AUDIOMIX_SAI1_MCLK2 2
> -#define IMX8MP_CLK_AUDIOMIX_SAI1_MCLK3 3
> -#define IMX8MP_CLK_AUDIOMIX_SAI2_IPG   4
> -#define IMX8MP_CLK_AUDIOMIX_SAI2_MCLK1 5
> -#define IMX8MP_CLK_AUDIOMIX_SAI2_MCLK2 6
> -#define IMX8MP_CLK_AUDIOMIX_SAI2_MCLK3 7
> -#define IMX8MP_CLK_AUDIOMIX_SAI3_IPG   8
> -#define IMX8MP_CLK_AUDIOMIX_SAI3_MCLK1 9
> -#define IMX8MP_CLK_AUDIOMIX_SAI3_MCLK2 10
> -#define IMX8MP_CLK_AUDIOMIX_SAI3_MCLK3 11
> -#define IMX8MP_CLK_AUDIOMIX_SAI5_IPG   12
> -#define IMX8MP_CLK_AUDIOMIX_SAI5_MCLK1 13
> -#define IMX8MP_CLK_AUDIOMIX_SAI5_MCLK2 14
> -#define IMX8MP_CLK_AUDIOMIX_SAI5_MCLK3 15
> -#define IMX8MP_CLK_AUDIOMIX_SAI6_IPG   16
> -#define IMX8MP_CLK_AUDIOMIX_SAI6_MCLK1 17
> -#define IMX8MP_CLK_AUDIOMIX_SAI6_MCLK2 18
> -#define IMX8MP_CLK_AUDIOMIX_SAI6_MCLK3 19
> -#define IMX8MP_CLK_AUDIOMIX_SAI7_IPG   20
> -#define IMX8MP_CLK_AUDIOMIX_SAI7_MCLK1 21
> -#define IMX8MP_CLK_AUDIOMIX_SAI7_MCLK2 22
> -#define IMX8MP_CLK_AUDIOMIX_SAI7_MCLK3 23
> -#define IMX8MP_CLK_AUDIOMIX_ASRC_IPG   24
> -#define IMX8MP_CLK_AUDIOMIX_PDM_IPG25
> -#define IMX8MP_CLK_AUDIOMIX_SDMA2_ROOT 26
> -#define IMX8MP_CLK_AUDIOMIX_SDMA3_ROOT 27
> -#define IMX8MP_CLK_AUDIOMIX_SPBA2_ROOT 28
> -#define IMX8MP_CLK_AUDIOMIX_DSP_ROOT   29
> -#define IMX8MP_CLK_AUDIOMIX_DSPDBG_ROOT30
> -#define IMX8MP_CLK_AUDIOMIX_EARC_IPG   31
> -#define IMX8MP_CLK_AUDIOMIX_OCRAMA_IPG 32
> -#define IMX8MP_CLK_AUDIOMIX_AUD2HTX_IPG33
> -#define IMX8MP_CLK_AUDIOMIX_EDMA_ROOT  34
> -#define IMX8MP_CLK_AUDIOMIX_AUDPLL_ROOT35
> -#define IMX8MP_CLK_AUDIOMIX_MU2_ROOT   36
> -#define IMX8MP_CLK_AUDIOMIX_MU3_ROOT   37
> -#define IMX8MP_CLK_AUDIOMIX_EARC_PHY   38
> -#define IMX8MP_CLK_AUDIOMIX_PDM_ROOT   39
> -#define IMX8MP_CLK_AUDIOMIX_SAI1_MCLK1_SEL 40
> -#define IMX8MP_CLK_AUDIOMIX_SAI1_MCLK2_SEL 41
> -#define IMX8MP_CLK_AUDIOMIX_SAI2_MCLK1_SEL 42
> -#define IMX8MP_CLK_AUDIOMIX_SAI2_MCLK2_SEL 43
> -#define IMX8MP_CLK_AUDIOMIX_SAI3_MCLK1_SEL 44
> -#define IMX8MP_CLK_AUDIOMIX_SAI3_MCLK2_SEL 45
> -#define IMX8MP_CLK_AUDIOMIX_SAI4_MCLK1_SEL 46
> -#define IMX8MP_CLK_AUDIOMIX_SAI4_MCLK2_SEL 47
> -#define IMX8MP_CLK_AUDIOMIX_SAI5_MCLK1_SEL 48
> -#define IMX8MP_CLK_AUDIOMIX_SAI5_MCLK2_SEL 49
> -#define IMX8MP_CLK_AUDIOMIX_SAI6_MCLK1_SEL 50
> -#define IMX8MP_CLK_AUDIOMIX_SAI6_MCLK2_SEL 51
> -#define IMX8MP_CLK_AUDIOMIX_SAI7_MCLK1_SEL 52
> -#define IMX8MP_CLK_AUDIOMIX_SAI7_MCLK2_SEL 53
> -#define IMX8MP_CLK_AUDIOMIX_PDM_SEL54
> -#define IMX8MP_CLK_AUDIOMIX_SAI_PLL_REF_SEL55
> -#define IMX8MP_CLK_AUDIOMIX_SAI_PLL56
> -#define IMX8MP_CLK_AUDIOMIX_SAI_PLL_BYPASS 57
> -#define IMX8MP_CLK_AUDIOMIX_SAI_PLL_OUT58
> +#define IMX8MP_CLK_AUDIO_BLK_CTRL_SAI1_IPG 0
> +#define IMX8MP_CLK_AUDIO_BLK_CTRL_SAI1_MCLK1   1
> +#define IMX8MP_CLK_AUDIO_BLK_CTRL_SAI1_MCLK2   2
> +#define IMX8MP_CLK_AUDIO_BLK_CTRL_SAI1_MCLK3   3
> +#define IMX8MP_CLK_AUDIO_BLK_CTRL_SAI2_IPG 4
> +#define IMX8MP_CLK_AUDIO_BLK_CTRL_SAI2_MCLK1   5
> +#define IMX8MP_CLK_AUDIO_BLK_CTRL_SAI2_MCLK2   6
> +#define IMX8MP_CLK_AUDIO_BLK_CTRL_SAI2_MCLK3   7
> +#define IMX8MP_CLK_AUDIO_BLK_CTRL_SAI3_IPG 8
> +#define IMX8MP_CLK_AUDIO_BLK_CTRL_SAI3_MCLK1   9
> +#define IMX8MP_CLK_AUDIO_BLK_CTRL_SAI3_MCLK2   10
> +#define IMX8MP_CLK_AUDIO_BLK_CTRL_SAI3_MCLK3   11
> +#define IMX8MP_CLK_AUDIO_BLK_CTRL_SAI5_

Re: [PATCH V4 3/5] clk: imx: Support building i.MX common clock driver as module

[Dong Aisheng]

On Thu, Jul 2, 2020 at 2:11 PM Anson Huang  wrote:
>
> > Subject: Re: [PATCH V4 3/5] clk: imx: Support building i.MX common clock
> > driver as module
> >
> > On Thu, Jul 2, 2020 at 11:26 AM Anson Huang 
> > wrote:
> > [...]
> > > > > @@ -143,16 +148,18 @@ void imx_cscmr1_fixup(u32 *val)  static int
> > > > > imx_keep_uart_clocks;  static struct clk ** const
> > > > > *imx_uart_clocks;
> > > > >
> > > > > -static int __init imx_keep_uart_clocks_param(char *str)
> > > > > +static int __maybe_unused imx_keep_uart_clocks_param(char *str)
> > > > >  {
> > > > > imx_keep_uart_clocks = 1;
> > > > >
> > > > > return 0;
> > > > >  }
> > > > > +#ifndef MODULE
> > > > >  __setup_param("earlycon", imx_keep_uart_earlycon,
> > > > >   imx_keep_uart_clocks_param, 0);
> > > > > __setup_param("earlyprintk", imx_keep_uart_earlyprintk,
> > > > >   imx_keep_uart_clocks_param, 0);
> > > >
> > > > I feel not only the __setup_param, the whole logic of
> > > > keep_uart_clocks are not needed for Module case. Is it true?
> > >
> > > Yes, but the 'keep_uart_clocks' is false by default and the function
> > > imx_keep_uart_clocks_param() already has '__maybe_unused', it does NOT
> > > impact anything if it is for module build, so I did NOT add the #ifndef 
> > > check
> > for them, just to keep code easy and clean.
> > >
> >
> > IMHO do not compile them is a more easy and clean way. Then users don't
> > have to look into the code logic which is meaingless for Module case.
> >
> > BTW, it really does not make any sense to only condionally compile
> > __setup_parm() but left
> > the param functions definition to be handled by __maybe_unnused.
> > They're together part of code, aren't they?
>
> I am fine of adding the '#ifndef MODULE' to imx_clk_disable_uart() and 
> imx_keep_uart_clocks_param()
> as well in next patch series. Others like ' imx_keep_uart_clocks ' and 
> imx_register_uart_clocks() need to
> be kept always built, since they are used by each clock driver no matter 
> built-in or module build.
>
> So that means I have to add another 'ifndef MODULE' or I need to adjust some 
> code sequence to make
> those code can be built-out in same block and just use single 'ifndef 
> MODULE', I think adjust the code
> sequence should be better, will go with this way.

What if we  condionally compile it in clk.h? Will that be easiser?

Regards
Aisheng

>
> Anson

Re: [PATCH V4 3/5] clk: imx: Support building i.MX common clock driver as module

[Dong Aisheng]

On Thu, Jul 2, 2020 at 11:26 AM Anson Huang  wrote:
[...]
> > > @@ -143,16 +148,18 @@ void imx_cscmr1_fixup(u32 *val)
> > >  static int imx_keep_uart_clocks;
> > >  static struct clk ** const *imx_uart_clocks;
> > >
> > > -static int __init imx_keep_uart_clocks_param(char *str)
> > > +static int __maybe_unused imx_keep_uart_clocks_param(char *str)
> > >  {
> > > imx_keep_uart_clocks = 1;
> > >
> > > return 0;
> > >  }
> > > +#ifndef MODULE
> > >  __setup_param("earlycon", imx_keep_uart_earlycon,
> > >   imx_keep_uart_clocks_param, 0);
> > >  __setup_param("earlyprintk", imx_keep_uart_earlyprintk,
> > >   imx_keep_uart_clocks_param, 0);
> >
> > I feel not only the __setup_param, the whole logic of keep_uart_clocks
> > are not needed for Module case. Is it true?
>
> Yes, but the 'keep_uart_clocks' is false by default and the function 
> imx_keep_uart_clocks_param()
> already has '__maybe_unused', it does NOT impact anything if it is for module 
> build, so I did NOT
> add the #ifndef check for them, just to keep code easy and clean.
>

IMHO do not compile them is a more easy and clean way. Then users
don't have to look into the code logic
which is meaingless for Module case.

BTW, it really does not make any sense to only condionally compile
__setup_parm() but left
the param functions definition to be handled by __maybe_unnused.
They're together part of code, aren't they?

Regards
Aisheng

Regards
Aisheng

> Thanks,
> Anson
>

Re: [PATCH V4 5/5] clk: imx8qxp: Support building i.MX8QXP clock driver as module

[Dong Aisheng]

On Thu, Jul 2, 2020 at 11:55 AM Anson Huang  wrote:
[...]
> > > +{
> > > +   return platform_driver_register(_lpcg_clk_driver);
> > > +}
> > > +device_initcall(imx8qxp_lpcg_clk_init);
> >
> > Any reason to change to device_initcall which looks a bit strange?
> > Is it because the following line?
> > +obj-$(CONFIG_MXC_CLK_SCU) += clk-imx-scu.o clk-imx-lpcg-scu.o
> > But it looks to me they're still two modules. Aren't they?
>
> It is suggested by Arnd to NOT use builtin_platform_driver(), in order to 
> support module
> unload, although the clock driver normally does NOT support remove, but it is 
> better to
> follow the right way.
>

By expanding builtin_platform_driver() marcro, you will find your
patch is exactly doing the same thing
as buildin_platform_driver() which obivously is unneccesary.

#define builtin_platform_driver(__platform_driver) \
builtin_driver(__platform_driver, platform_driver_register)
#define builtin_driver(__driver, __register, ...) \

static int __init __driver##_init(void) \
{ \
return __register(&(__driver) , ##__VA_ARGS__); \
} \
device_initcall(__driver##_init);

If we want to support unload, we need a .remove() callback as current
clocks are not allocated by devm_().
If don't support,  we probably can use builtin_platform_driver() first
and switch to module_platform_driver()
in the future once the driver supports release resource properly.

Regards
Aisheng

> The change in Makefile is just to link scu/lpcg library to i.MX8QXP clk 
> driver, so that we can
> get rid of exports and below 2 .ko are needed for all i.MX SoCs with SCU 
> inside as per your
> saying of i.MX8QXP clock driver can be extended for future SoCs with SCU.
>
> clk-imx-lpcg-scu.ko
> clk-imx-scu.ko
>
> Thanks,
> Anson

Re: [PATCH V4 5/5] clk: imx8qxp: Support building i.MX8QXP clock driver as module

[Dong Aisheng]

On Thu, Jul 2, 2020 at 10:20 AM Anson Huang  wrote:
>
> Change configuration to "tristate", use device_initcall() instead
> of builtin_platform_driver(), add module author, description and
> license to support building i.MX8QXP clock drivers as module.
>
> Signed-off-by: Anson Huang 
> ---
> Changes since V3:
> - use device_initcall() instead of builtin_platform_driver();
> - add module author/description;
> - link common scu/lpcg clock driver to i.MX8QXP clock driver, then
>   no need to have exports.
> ---
>  drivers/clk/imx/Kconfig|  6 +++---
>  drivers/clk/imx/Makefile   |  9 -
>  drivers/clk/imx/clk-imx8qxp-lpcg.c | 10 +-
>  drivers/clk/imx/clk-imx8qxp.c  | 11 ++-
>  4 files changed, 26 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/clk/imx/Kconfig b/drivers/clk/imx/Kconfig
> index 1867111..8340dfe 100644
> --- a/drivers/clk/imx/Kconfig
> +++ b/drivers/clk/imx/Kconfig
> @@ -5,8 +5,8 @@ config MXC_CLK
> depends on ARCH_MXC
>
>  config MXC_CLK_SCU
> -   bool
> -   depends on IMX_SCU
> +   tristate "IMX SCU clock"
> +   depends on ARCH_MXC && IMX_SCU
>
>  config CLK_IMX1
>   bool "IMX1 CCM Clock Driver"
> @@ -127,7 +127,7 @@ config CLK_IMX8MQ
> Build the driver for i.MX8MQ CCM Clock Driver
>
>  config CLK_IMX8QXP
> -   bool "IMX8QXP SCU Clock"
> +   tristate "IMX8QXP SCU Clock"
> depends on ARCH_MXC && IMX_SCU && ARM64
> select MXC_CLK_SCU
> help
> diff --git a/drivers/clk/imx/Makefile b/drivers/clk/imx/Makefile
> index 17f5d12..79e53f2 100644
> --- a/drivers/clk/imx/Makefile
> +++ b/drivers/clk/imx/Makefile
> @@ -21,15 +21,14 @@ mxc-clk-objs += clk-pll14xx.o
>  mxc-clk-objs += clk-sscg-pll.o
>  obj-$(CONFIG_MXC_CLK) += mxc-clk.o
>
> -obj-$(CONFIG_MXC_CLK_SCU) += \
> -   clk-scu.o \
> -   clk-lpcg-scu.o
> -
>  obj-$(CONFIG_CLK_IMX8MM) += clk-imx8mm.o
>  obj-$(CONFIG_CLK_IMX8MN) += clk-imx8mn.o
>  obj-$(CONFIG_CLK_IMX8MP) += clk-imx8mp.o
>  obj-$(CONFIG_CLK_IMX8MQ) += clk-imx8mq.o
> -obj-$(CONFIG_CLK_IMX8QXP) += clk-imx8qxp.o clk-imx8qxp-lpcg.o
> +
> +obj-$(CONFIG_MXC_CLK_SCU) += clk-imx-scu.o clk-imx-lpcg-scu.o
> +clk-imx-scu-$(CONFIG_CLK_IMX8QXP) += clk-scu.o clk-imx8qxp.o
> +clk-imx-lpcg-scu-$(CONFIG_CLK_IMX8QXP) += clk-lpcg-scu.o clk-imx8qxp-lpcg.o
>
>  obj-$(CONFIG_CLK_IMX1)   += clk-imx1.o
>  obj-$(CONFIG_CLK_IMX21)  += clk-imx21.o
> diff --git a/drivers/clk/imx/clk-imx8qxp-lpcg.c 
> b/drivers/clk/imx/clk-imx8qxp-lpcg.c
> index 04c8ee3..5b6648e 100644
> --- a/drivers/clk/imx/clk-imx8qxp-lpcg.c
> +++ b/drivers/clk/imx/clk-imx8qxp-lpcg.c
> @@ -231,4 +231,12 @@ static struct platform_driver imx8qxp_lpcg_clk_driver = {
> .probe = imx8qxp_lpcg_clk_probe,
>  };
>
> -builtin_platform_driver(imx8qxp_lpcg_clk_driver);
> +static int __init imx8qxp_lpcg_clk_init(void)

Does __init work for module?

> +{
> +   return platform_driver_register(_lpcg_clk_driver);
> +}
> +device_initcall(imx8qxp_lpcg_clk_init);

Any reason to change to device_initcall which looks a bit strange?
Is it because the following line?
+obj-$(CONFIG_MXC_CLK_SCU) += clk-imx-scu.o clk-imx-lpcg-scu.o
But it looks to me they're still two modules. Aren't they?

Regards
Aisheng

> +
> +MODULE_AUTHOR("Aisheng Dong ");
> +MODULE_DESCRIPTION("NXP i.MX8QXP LPCG clock driver");
> +MODULE_LICENSE("GPL v2");
> diff --git a/drivers/clk/imx/clk-imx8qxp.c b/drivers/clk/imx/clk-imx8qxp.c
> index 5e2903e..9bcf0d1 100644
> --- a/drivers/clk/imx/clk-imx8qxp.c
> +++ b/drivers/clk/imx/clk-imx8qxp.c
> @@ -151,4 +151,13 @@ static struct platform_driver imx8qxp_clk_driver = {
> },
> .probe = imx8qxp_clk_probe,
>  };
> -builtin_platform_driver(imx8qxp_clk_driver);
> +
> +static int __init imx8qxp_clk_init(void)
> +{
> +   return platform_driver_register(_clk_driver);
> +}
> +device_initcall(imx8qxp_clk_init);
> +
> +MODULE_AUTHOR("Aisheng Dong ");
> +MODULE_DESCRIPTION("NXP i.MX8QXP clock driver");
> +MODULE_LICENSE("GPL v2");
> --
> 2.7.4
>

Re: [PATCH V4 4/5] clk: imx8m: Support module build

[Dong Aisheng]

On Thu, Jul 2, 2020 at 10:18 AM Anson Huang  wrote:
>
> Change configuration to "tristate", add module author, description
> and license to support building i.MX8M SoCs clock driver as module.
>
> Signed-off-by: Anson Huang 

Reviewed-by: Dong Aisheng 

Regards
Aisheng

> ---
> Changes since V3:
> - add module author/description, and merge all i.MX8M SoCs patch into
>   one patch.
> ---
>  drivers/clk/imx/Kconfig  | 8 
>  drivers/clk/imx/clk-imx8mm.c | 4 
>  drivers/clk/imx/clk-imx8mn.c | 4 
>  drivers/clk/imx/clk-imx8mp.c | 4 
>  drivers/clk/imx/clk-imx8mq.c | 4 
>  5 files changed, 20 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/clk/imx/Kconfig b/drivers/clk/imx/Kconfig
> index f6ddf76..1867111 100644
> --- a/drivers/clk/imx/Kconfig
> +++ b/drivers/clk/imx/Kconfig
> @@ -99,28 +99,28 @@ config CLK_VF610
>   select MXC_CLK
>
>  config CLK_IMX8MM
> -   bool "IMX8MM CCM Clock Driver"
> +   tristate "IMX8MM CCM Clock Driver"
> depends on ARCH_MXC
> select MXC_CLK
> help
> Build the driver for i.MX8MM CCM Clock Driver
>
>  config CLK_IMX8MN
> -   bool "IMX8MN CCM Clock Driver"
> +   tristate "IMX8MN CCM Clock Driver"
> depends on ARCH_MXC
> select MXC_CLK
> help
> Build the driver for i.MX8MN CCM Clock Driver
>
>  config CLK_IMX8MP
> -   bool "IMX8MP CCM Clock Driver"
> +   tristate "IMX8MP CCM Clock Driver"
> depends on ARCH_MXC
> select MXC_CLK
> help
> Build the driver for i.MX8MP CCM Clock Driver
>
>  config CLK_IMX8MQ
> -   bool "IMX8MQ CCM Clock Driver"
> +   tristate "IMX8MQ CCM Clock Driver"
> depends on ARCH_MXC
> select MXC_CLK
> help
> diff --git a/drivers/clk/imx/clk-imx8mm.c b/drivers/clk/imx/clk-imx8mm.c
> index b793264..0de0be0 100644
> --- a/drivers/clk/imx/clk-imx8mm.c
> +++ b/drivers/clk/imx/clk-imx8mm.c
> @@ -657,3 +657,7 @@ static struct platform_driver imx8mm_clk_driver = {
> },
>  };
>  module_platform_driver(imx8mm_clk_driver);
> +
> +MODULE_AUTHOR("Bai Ping ");
> +MODULE_DESCRIPTION("NXP i.MX8MM clock driver");
> +MODULE_LICENSE("GPL v2");
> diff --git a/drivers/clk/imx/clk-imx8mn.c b/drivers/clk/imx/clk-imx8mn.c
> index 213cc37..e984de5 100644
> --- a/drivers/clk/imx/clk-imx8mn.c
> +++ b/drivers/clk/imx/clk-imx8mn.c
> @@ -608,3 +608,7 @@ static struct platform_driver imx8mn_clk_driver = {
> },
>  };
>  module_platform_driver(imx8mn_clk_driver);
> +
> +MODULE_AUTHOR("Anson Huang ");
> +MODULE_DESCRIPTION("NXP i.MX8MN clock driver");
> +MODULE_LICENSE("GPL v2");
> diff --git a/drivers/clk/imx/clk-imx8mp.c b/drivers/clk/imx/clk-imx8mp.c
> index ca74771..f3cedf2 100644
> --- a/drivers/clk/imx/clk-imx8mp.c
> +++ b/drivers/clk/imx/clk-imx8mp.c
> @@ -773,3 +773,7 @@ static struct platform_driver imx8mp_clk_driver = {
> },
>  };
>  module_platform_driver(imx8mp_clk_driver);
> +
> +MODULE_AUTHOR("Anson Huang ");
> +MODULE_DESCRIPTION("NXP i.MX8MP clock driver");
> +MODULE_LICENSE("GPL v2");
> diff --git a/drivers/clk/imx/clk-imx8mq.c b/drivers/clk/imx/clk-imx8mq.c
> index a64aace..a06cc21 100644
> --- a/drivers/clk/imx/clk-imx8mq.c
> +++ b/drivers/clk/imx/clk-imx8mq.c
> @@ -643,3 +643,7 @@ static struct platform_driver imx8mq_clk_driver = {
> },
>  };
>  module_platform_driver(imx8mq_clk_driver);
> +
> +MODULE_AUTHOR("Abel Vesa ");
> +MODULE_DESCRIPTION("NXP i.MX8MQ clock driver");
> +MODULE_LICENSE("GPL v2");
> --
> 2.7.4
>

Re: [PATCH V4 3/5] clk: imx: Support building i.MX common clock driver as module

[Dong Aisheng]

On Thu, Jul 2, 2020 at 10:19 AM Anson Huang  wrote:
>
> There are more and more requirements of building SoC specific drivers
> as modules, add support for building i.MX common clock driver as module
> to meet the requirement.
>
> Signed-off-by: Anson Huang 
> ---
> Changes since V3:
> - ONLY include __setup_param() build for built-in, module build no 
> need
>   to have it.
> ---
>  drivers/clk/imx/Kconfig|  8 ++--
>  drivers/clk/imx/Makefile   | 40 
> +++---
>  drivers/clk/imx/clk-composite-8m.c |  2 ++
>  drivers/clk/imx/clk-cpu.c  |  2 ++
>  drivers/clk/imx/clk-frac-pll.c |  2 ++
>  drivers/clk/imx/clk-gate2.c|  2 ++
>  drivers/clk/imx/clk-pll14xx.c  |  5 +
>  drivers/clk/imx/clk-sscg-pll.c |  2 ++
>  drivers/clk/imx/clk.c  | 22 +++--
>  9 files changed, 57 insertions(+), 28 deletions(-)
>
> diff --git a/drivers/clk/imx/Kconfig b/drivers/clk/imx/Kconfig
> index 09fc8ad..f6ddf76 100644
> --- a/drivers/clk/imx/Kconfig
> +++ b/drivers/clk/imx/Kconfig
> @@ -1,8 +1,8 @@
>  # SPDX-License-Identifier: GPL-2.0
>  # common clock support for NXP i.MX SoC family.
>  config MXC_CLK
> -   bool
> -   def_bool ARCH_MXC
> +   tristate "IMX clock"
> +   depends on ARCH_MXC
>
>  config MXC_CLK_SCU
> bool
> @@ -101,24 +101,28 @@ config CLK_VF610
>  config CLK_IMX8MM
> bool "IMX8MM CCM Clock Driver"
> depends on ARCH_MXC
> +   select MXC_CLK
> help
> Build the driver for i.MX8MM CCM Clock Driver
>
>  config CLK_IMX8MN
> bool "IMX8MN CCM Clock Driver"
> depends on ARCH_MXC
> +   select MXC_CLK
> help
> Build the driver for i.MX8MN CCM Clock Driver
>
>  config CLK_IMX8MP
> bool "IMX8MP CCM Clock Driver"
> depends on ARCH_MXC
> +   select MXC_CLK
> help
> Build the driver for i.MX8MP CCM Clock Driver
>
>  config CLK_IMX8MQ
> bool "IMX8MQ CCM Clock Driver"
> depends on ARCH_MXC
> +   select MXC_CLK
> help
> Build the driver for i.MX8MQ CCM Clock Driver
>
> diff --git a/drivers/clk/imx/Makefile b/drivers/clk/imx/Makefile
> index 394ade7..17f5d12 100644
> --- a/drivers/clk/imx/Makefile
> +++ b/drivers/clk/imx/Makefile
> @@ -1,25 +1,25 @@
>  # SPDX-License-Identifier: GPL-2.0
>
> -obj-$(CONFIG_MXC_CLK) += \
> -   clk.o \
> -   clk-busy.o \
> -   clk-composite-8m.o \
> -   clk-cpu.o \
> -   clk-composite-7ulp.o \
> -   clk-divider-gate.o \
> -   clk-fixup-div.o \
> -   clk-fixup-mux.o \
> -   clk-frac-pll.o \
> -   clk-gate-exclusive.o \
> -   clk-gate2.o \
> -   clk-pfd.o \
> -   clk-pfdv2.o \
> -   clk-pllv1.o \
> -   clk-pllv2.o \
> -   clk-pllv3.o \
> -   clk-pllv4.o \
> -   clk-sscg-pll.o \
> -   clk-pll14xx.o
> +mxc-clk-objs += clk.o
> +mxc-clk-objs += clk-busy.o
> +mxc-clk-objs += clk-composite-7ulp.o
> +mxc-clk-objs += clk-composite-8m.o
> +mxc-clk-objs += clk-cpu.o
> +mxc-clk-objs += clk-divider-gate.o
> +mxc-clk-objs += clk-fixup-div.o
> +mxc-clk-objs += clk-fixup-mux.o
> +mxc-clk-objs += clk-frac-pll.o
> +mxc-clk-objs += clk-gate2.o
> +mxc-clk-objs += clk-gate-exclusive.o
> +mxc-clk-objs += clk-pfd.o
> +mxc-clk-objs += clk-pfdv2.o
> +mxc-clk-objs += clk-pllv1.o
> +mxc-clk-objs += clk-pllv2.o
> +mxc-clk-objs += clk-pllv3.o
> +mxc-clk-objs += clk-pllv4.o
> +mxc-clk-objs += clk-pll14xx.o
> +mxc-clk-objs += clk-sscg-pll.o
> +obj-$(CONFIG_MXC_CLK) += mxc-clk.o
>
>  obj-$(CONFIG_MXC_CLK_SCU) += \
> clk-scu.o \
> diff --git a/drivers/clk/imx/clk-composite-8m.c 
> b/drivers/clk/imx/clk-composite-8m.c
> index d2b5af8..78fb7e5 100644
> --- a/drivers/clk/imx/clk-composite-8m.c
> +++ b/drivers/clk/imx/clk-composite-8m.c
> @@ -5,6 +5,7 @@
>
>  #include 
>  #include 
> +#include 
>  #include 
>  #include 
>
> @@ -243,3 +244,4 @@ struct clk_hw *imx8m_clk_hw_composite_flags(const char 
> *name,
> kfree(mux);
> return ERR_CAST(hw);
>  }
> +EXPORT_SYMBOL_GPL(imx8m_clk_hw_composite_flags);
> diff --git a/drivers/clk/imx/clk-cpu.c b/drivers/clk/imx/clk-cpu.c
> index cb182be..cb6ca4c 100644
> --- a/drivers/clk/imx/clk-cpu.c
> +++ b/drivers/clk/imx/clk-cpu.c
> @@ -5,6 +5,7 @@
>
>  #include 
>  #include 
> +#include 
>  #include 
>  #include "clk.h"
>
> @@ -104,3 +105,4 @@ struct clk_hw *imx_clk_hw_cpu(const char *name, const 
> char *parent_name,
>
> return hw;
>  }
> +EXPORT_SYMBOL_GPL(imx_clk_hw_cpu);
> diff --git a/drivers/clk/imx/clk-frac-pll.c b/drivers/clk/imx/clk-frac-pll.c
> index 101e0a3..c703056 100644
> --- a/drivers/clk/imx/clk-frac-pll.c
> +++ b/drivers/clk/imx/clk-frac-pll.c
> @@ -10,6 +10,7 @@
>
>  #include 
>  #include 
> +#include 
>  #include 
>  #include 
>  #include 
> @@ -233,3 +234,4 @@ struct clk_hw *imx_clk_hw_frac_pll(const char *name,
>
> return hw;
>  }
> 

Re: [PATCH V3 04/10] clk: imx: Support building SCU clock driver as module

[Dong Aisheng]

On Tue, Jun 30, 2020 at 5:16 AM Anson Huang  wrote:
>
> Hi, Arnd
>
>
> > Subject: Re: [PATCH V3 04/10] clk: imx: Support building SCU clock driver as
> > module
> >
> > On Mon, Jun 29, 2020 at 4:52 PM Anson Huang 
> > wrote:
> > > > Subject: Re: [PATCH V3 04/10] clk: imx: Support building SCU clock
> > > > driver as module On Mon, Jun 29, 2020 at 2:53 PM Anson Huang
> >  wrote:
> > > >
> > > > Sorry, I misread the patch in multiple ways. First of all, you
> > > > already put clk-scu.o and clk-lpcg-scu.o files into a combined
> > > > loadable module, and I had only looked at clk-scu.c.
> > > >
> > > > What I actually meant here was to link clk-scu.o together with
> > > > clk-imx8qxp.o (and possibly future chip-specific files) into a
> > > > loadable module and drop the export.
> > >
> > > Sorry, could you please advise more details about how to do it in 
> > > Makefile?
> > > I tried below but it looks like NOT working. multiple definition of
> > module_init() error reported.
> > >
> > > obj-$(CONFIG_MXC_CLK_SCU) := clk-imx.o clk-imx-y += clk-scu.o
> > > clk-lpcg-scu.o
> > > clk-imx-$(CONFIG_CLK_IMX8QXP) += clk-imx8qxp.o clk-imx8qxp-lpcg.o
> >
> > Right, you can't have multiple module_init() in a module, so what I 
> > suggested
> > earlier won't work any more as soon as you add a second chip that uses the
> > clk-scu driver, and then you have to use separate modules, or some hack that
> > calls the init functions one at a time, which is probably worse.
> >
> > If it's only imx8qxp, you can do it like this:
> >
> > obj-$(CONFIG_MXC_CLK_SCU) := clk-imx-scu.o clk-imx-lpcg.o
> > clk-imx-scu-y += clk-scu.o clk-imx8qxp.o
> > clk-imx-lpcq-scu-y += clk-lpcg-scu.o clk-imx8qxp-lpcg.o
> >
> > If you already know that the scu driver is going to be used in future 
> > chips, then
> > just stay with what you have now, using a separate module per file, 
> > exporting
> > the symbols as needed.
> >
>
> Thanks, and yes, I know that scu clk driver will be used for future i.MX8X 
> chips with
> SCU inside, the current i.MX8QXP clock driver can NOT cover all i.MX8X chips, 
> so
> I will stay with the exporting symbols.
>

SCU clock driver is a common driver for all SCU based platforms.
Current i.MX8QXP SCU clock driver will be extended to support all
future SCU based platforms.
So theoretically clk-scu.o and clk-imx8qxp.o can be combined. LPCG  is similar.
Maybe you can give a try as Arnd suggested.

Regards
Aisheng

> Thanks,
> Anson

Re: [PATCH V3 04/10] clk: imx: Support building SCU clock driver as module

[Dong Aisheng]

On Tue, Jun 30, 2020 at 3:36 AM Arnd Bergmann  wrote:
>
> On Mon, Jun 29, 2020 at 2:53 PM Anson Huang  wrote:
> > > Subject: Re: [PATCH V3 04/10] clk: imx: Support building SCU clock driver 
> > > as
> > > module
> > >
> > > On Mon, Jun 29, 2020 at 8:06 AM Anson Huang 
> > > wrote:
> > >
> > > > --- a/drivers/clk/imx/Makefile
> > > > +++ b/drivers/clk/imx/Makefile
> > > > @@ -21,9 +21,9 @@ obj-$(CONFIG_MXC_CLK) += \
> > > > clk-sscg-pll.o \
> > > > clk-pll14xx.o
> > > >
> > > > -obj-$(CONFIG_MXC_CLK_SCU) += \
> > > > -   clk-scu.o \
> > > > -   clk-lpcg-scu.o
> > > > +mxc-clk-scu-objs += clk-lpcg-scu.o
> > > > +mxc-clk-scu-objs += clk-scu.o
> > > > +obj-$(CONFIG_MXC_CLK_SCU) += mxc-clk-scu.o
> > >
> > > It looks like the two modules are tightly connected, one is useless 
> > > without the
> > > other. How about linking them into a combined module and dropping the
> > > export statement?
> > >
> >
> > From HW perspective, the SCU clock driver and LPCG SCU clock driver are 
> > different,
> > SCU clock driver is for those clocks controlled by system controller (M4 
> > which runs a firmware),
> > while LPCG SCU clock is for those clock gates inside module, which means AP 
> > core can
> > control it directly via register access, no need to via SCU API.
>
> Sorry, I misread the patch in multiple ways. First of all, you already put
> clk-scu.o and clk-lpcg-scu.o files into a combined loadable module, and
> I had only looked at clk-scu.c.
>
> What I actually meant here was to link clk-scu.o together with clk-imx8qxp.o
> (and possibly future chip-specific files) into a loadable module and drop
> the export.

It sounds like a good idea to me.
Actually I planned to combine them into one driver in the future.

Regards
Aisheng

>
> > So, I think it is NOT that tightly connected, it is because they are both 
> > for i.MX8 SoCs with SCU
> > inside, so they are put together in the Makefile.
> >
> > If the export statement is acceptable, I think it is better to just keep 
> > it, make sense?
>
> There is nothing wrong with the export as such, this was just an
> idea to simplify the logic.
>
>   Arnd

Re: [PATCH V2 3/9] clk: imx: Support building SCU clock driver as module

[Dong Aisheng]

On Thu, Jun 25, 2020 at 6:43 AM Stephen Boyd  wrote:
>
> Quoting Aisheng Dong (2020-06-23 19:59:09)
> > > > > > -   bool
> > > > > > -   def_bool ARCH_MXC
> > > > > > +   tristate "IMX clock"
> > > > > > +   depends on ARCH_MXC
> > > > > >
> > > > > > But user can still set MXC_CLK to be m, either via make menuconfig
> > > > > > or
> > > > > defconfig.
> > > > >
> > > > > Isn't that what we want?
> > > >
> > > > No, if user set MXC_CLK to m, the build will break for i.MX6&7.
> > > >
> > > > > Why does ARCH_MXC being enabled mandate that it is builtin? Is some
> > > > > architecture level code calling into the clk driver?
> > > >
> > > >
> > > > It's mainly because there's no Kconfig options for i.MX6 &7 clock 
> > > > drivers.
> > > > It just reuses ARCH config CONFIG_SOC_XXX which can only be y.
> > > > e.g.
> > > > obj-$(CONFIG_SOC_IMX6Q)  += clk-imx6q.o
> > > > obj-$(CONFIG_SOC_IMX6SL) += clk-imx6sl.o
> > > > obj-$(CONFIG_SOC_IMX7ULP) += clk-imx7ulp.o
> > > > obj-$(CONFIG_SOC_VF610)  += clk-vf610.o ..
> > > >
> > > > If setting MXC_CLK to m, the platform clock drivers will fail to build
> > > > due to miss to find symbols defined in the common clock library by
> > > > CONFIG_MXC_CLK.
> > > > So we have to avoid users to be able to config MXC_CLK to m for i.MX6&7.
> > > > Only depends on ARCH_MXC mean user still can set it to m.
> > >
> > > I think for i.MX6/7, although MXC_CLK is tristate, but it is selected by
> > > ARCH_MXC which is always "y", so MXC_CLK can ONLY be "y" even it is 
> > > explicitly
> > > set to "m" in imx_v6_v7_defconfig file. So that means MXC_CLK can ONLY
> > > support built-in for i.MX6/7 SoCs, and that is what we want?
> > >
> >
> > Yes, I'm trying to explain to Stephen why we have to select MXC_CLK in 
> > ARCH_MXC
> > And what issues we will met if not select it.
> >
>
> Why aren't there options to enable clk-imx6q and clk-imx6sl in the
> clk/imx/Kconfig file? Those can be bool or tristate depending on if the
> SoC drivers use CLK_OF_DECLARE or not and depend on the mxc-clk library
> and SoC config we have in the makefile today.

Yes, we can do that in clk/imx/Kconfig as follows theoretically.
config CLK_IMX6Q
bool
def_bool ARCH_MXC && ARM
select MXC_CLK

But we have totally 15 platforms that need to change.
e.g.
drivers/clk/imx/Makefile
obj-$(CONFIG_SOC_IMX1)   += clk-imx1.o
obj-$(CONFIG_SOC_IMX21)  += clk-imx21.o
obj-$(CONFIG_SOC_IMX25)  += clk-imx25.o
obj-$(CONFIG_SOC_IMX27)  += clk-imx27.o
obj-$(CONFIG_SOC_IMX31)  += clk-imx31.o
obj-$(CONFIG_SOC_IMX35)  += clk-imx35.o
obj-$(CONFIG_SOC_IMX5)   += clk-imx5.o
obj-$(CONFIG_SOC_IMX6Q)  += clk-imx6q.o
obj-$(CONFIG_SOC_IMX6SL) += clk-imx6sl.o
obj-$(CONFIG_SOC_IMX6SLL) += clk-imx6sll.o
obj-$(CONFIG_SOC_IMX6SX) += clk-imx6sx.o
obj-$(CONFIG_SOC_IMX6UL) += clk-imx6ul.o
obj-$(CONFIG_SOC_IMX7D)  += clk-imx7d.o
obj-$(CONFIG_SOC_IMX7ULP) += clk-imx7ulp.o
obj-$(CONFIG_SOC_VF610)  += clk-vf610.o

Not sure if it's really worth to do that.
The easiest way to address this issue is just select it in
arch/arm/mach-imx/Kconfig,
then no need to change those 15 clock config options or just builtin
clk libraries.

Stephen,
which one do you prefer?

Regards
Aisheng

Re: [PATCH] clk: imx: lpcg: write twice when writing lpcg regs

[Dong Aisheng]

On Sat, Sep 7, 2019 at 9:47 PM Stephen Boyd  wrote:
>
> Quoting Peng Fan (2019-08-27 01:17:50)
> > From: Peng Fan 
> >
> > There is hardware issue that:
> > The output clock the LPCG cell will not turn back on as expected,
> > even though a read of the IPG registers in the LPCG indicates that
> > the clock should be enabled.
> >
> > The software workaround is to write twice to enable the LPCG clock
> > output.
> >
> > Signed-off-by: Peng Fan 
>
> Does this need a Fixes tag?

Not sure as it's not code logic issue but a hardware bug.
And 4.19 LTS still have not this driver support.

Regards
Aisheng

>

Re: [PATCH] clk: imx: lpcg: write twice when writing lpcg regs

[Dong Aisheng]

On Tue, Aug 27, 2019 at 4:19 PM Peng Fan  wrote:
>
> From: Peng Fan 
>
> There is hardware issue that:
> The output clock the LPCG cell will not turn back on as expected,
> even though a read of the IPG registers in the LPCG indicates that
> the clock should be enabled.
>
> The software workaround is to write twice to enable the LPCG clock
> output.
>
> Signed-off-by: Peng Fan 

Reviewed-by: Dong Aisheng 

Regards
Aisheng

Re: [EXT] Re: [PATCH] mailbox: imx: add support for imx v1 mu

[Dong Aisheng]

On Mon, Jul 29, 2019 at 11:03 AM Richard Zhu  wrote:
>
> Hi Aisheng:
> Thanks for your comments.
>
> > -Original Message-----
> > From: Dong Aisheng 
> > Sent: 2019年7月29日 10:35
> > To: Richard Zhu 
> > Cc: jassisinghb...@gmail.com; Oleksij Rempel ;
> > Aisheng Dong ; open list
> > ; moderated list:ARM/FREESCALE IMX / MXC
> > ARM ARCHITECTURE 
> > Subject: [EXT] Re: [PATCH] mailbox: imx: add support for imx v1 mu
> >
> > On Mon, Jul 29, 2019 at 10:36 AM Richard Zhu 
> > wrote:
> > >
> > > There is a version1.0 MU on i.MX7ULP platform.
> > > One new version ID register is added, and it's offset is 0.
> > > TRn registers are defined at the offset 0x20 ~ 0x2C.
> > > RRn registers are defined at the offset 0x40 ~ 0x4C.
> > > SR/CR registers are defined at 0x60/0x64.
> > > Extend this driver to support it.
> > >
> >
> > If only the register base offset is different, there's probably a more 
> > simple way.
> > Please refer to:
> >
> [Richard Zhu] TRx, RRx and the CR/SR have the different offset addresses.
> That means three different offset addresses should be manipulated if the 
> solution listed above is used.
> It seems that it's a little complex, and maybe introduce bugs when different 
> offset address is manipulated.
> According, the current method suggested by Oleksij is much clear, and easy to 
> extend for future extension.
>

I missed that.
Maybe the patch title should be V2 and add Suggested-by: tag to
reminder reviewer
it's a new version?

If there're multiple offset differences. I'm fine with this way.
Feel free to add my tag.
Reviewed-by: Dong Aisheng 

Regards
Aisheng

> Hi Olekiji:
> How do you think about?
>
> Best Regards
> Richard Zhu
>
> > Regards
> > Aisheng
> >
> > > Signed-off-by: Richard Zhu 
> > > ---
> > >  drivers/mailbox/imx-mailbox.c | 67
> > > ---
> > >  1 file changed, 50 insertions(+), 17 deletions(-)
> > >
> > > diff --git a/drivers/mailbox/imx-mailbox.c
> > > b/drivers/mailbox/imx-mailbox.c index 25be8bb..8423a38 100644
> > > --- a/drivers/mailbox/imx-mailbox.c
> > > +++ b/drivers/mailbox/imx-mailbox.c
> > > @@ -12,19 +12,11 @@
> > >  #include 
> > >  #include 
> > >
> > > -/* Transmit Register */
> > > -#define IMX_MU_xTRn(x) (0x00 + 4 * (x))
> > > -/* Receive Register */
> > > -#define IMX_MU_xRRn(x) (0x10 + 4 * (x))
> > > -/* Status Register */
> > > -#define IMX_MU_xSR 0x20
> > >  #define IMX_MU_xSR_GIPn(x) BIT(28 + (3 - (x)))
> > >  #define IMX_MU_xSR_RFn(x)  BIT(24 + (3 - (x)))
> > >  #define IMX_MU_xSR_TEn(x)  BIT(20 + (3 - (x)))
> > >  #define IMX_MU_xSR_BRDIP   BIT(9)
> > >
> > > -/* Control Register */
> > > -#define IMX_MU_xCR 0x24
> > >  /* General Purpose Interrupt Enable */
> > >  #define IMX_MU_xCR_GIEn(x) BIT(28 + (3 - (x)))
> > >  /* Receive Interrupt Enable */
> > > @@ -44,6 +36,13 @@ enum imx_mu_chan_type {
> > > IMX_MU_TYPE_RXDB,   /* Rx doorbell */
> > >  };
> > >
> > > +struct imx_mu_dcfg {
> > > +   u32 xTR[4]; /* Transmit Registers */
> > > +   u32 xRR[4]; /* Receive Registers */
> > > +   u32 xSR;/* Status Register */
> > > +   u32 xCR;/* Control Register */
> > > +};
> > > +
> > >  struct imx_mu_con_priv {
> > > unsigned intidx;
> > > char
> > irq_desc[IMX_MU_CHAN_NAME_SIZE];
> > > @@ -61,12 +60,39 @@ struct imx_mu_priv {
> > > struct mbox_chanmbox_chans[IMX_MU_CHANS];
> > >
> > > struct imx_mu_con_priv  con_priv[IMX_MU_CHANS];
> > > +   const struct imx_mu_dcfg*dcfg;
> > > struct clk  *clk;
> > > int irq;
> > >
> > > boolside_b;
> > >  };
> > >
> > > +static const struct imx_mu_dcfg imx_mu_cfg_imx6sx = {
> > > +   .xTR[0] = 0x0,
> > > +   .xTR[1] = 0x4,
> > > +   .xTR[2] = 0x8,
> > > +   .xTR[3] = 0xC,
> > > +   .xRR[0] = 0x10,
> > > +   .xRR[1] = 0x14,
> > > +   .xRR[2] = 0x18,
> > > +   .xRR[3] = 0x1C,
> > > +   .xSR= 0x20,
> > > +   .xCR

Re: [PATCH] mailbox: imx: add support for imx v1 mu

[Dong Aisheng]

On Mon, Jul 29, 2019 at 10:36 AM Richard Zhu  wrote:
>
> There is a version1.0 MU on i.MX7ULP platform.
> One new version ID register is added, and it's offset is 0.
> TRn registers are defined at the offset 0x20 ~ 0x2C.
> RRn registers are defined at the offset 0x40 ~ 0x4C.
> SR/CR registers are defined at 0x60/0x64.
> Extend this driver to support it.
>

If only the register base offset is different, there's probably a more
simple way.
Please refer to:
https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/drivers/tty/serial/fsl_lpuart.c?id=24b1e5f0e83c2aced8096473d20c4cf6c1355f30

Regards
Aisheng

> Signed-off-by: Richard Zhu 
> ---
>  drivers/mailbox/imx-mailbox.c | 67 
> ---
>  1 file changed, 50 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/mailbox/imx-mailbox.c b/drivers/mailbox/imx-mailbox.c
> index 25be8bb..8423a38 100644
> --- a/drivers/mailbox/imx-mailbox.c
> +++ b/drivers/mailbox/imx-mailbox.c
> @@ -12,19 +12,11 @@
>  #include 
>  #include 
>
> -/* Transmit Register */
> -#define IMX_MU_xTRn(x) (0x00 + 4 * (x))
> -/* Receive Register */
> -#define IMX_MU_xRRn(x) (0x10 + 4 * (x))
> -/* Status Register */
> -#define IMX_MU_xSR 0x20
>  #define IMX_MU_xSR_GIPn(x) BIT(28 + (3 - (x)))
>  #define IMX_MU_xSR_RFn(x)  BIT(24 + (3 - (x)))
>  #define IMX_MU_xSR_TEn(x)  BIT(20 + (3 - (x)))
>  #define IMX_MU_xSR_BRDIP   BIT(9)
>
> -/* Control Register */
> -#define IMX_MU_xCR 0x24
>  /* General Purpose Interrupt Enable */
>  #define IMX_MU_xCR_GIEn(x) BIT(28 + (3 - (x)))
>  /* Receive Interrupt Enable */
> @@ -44,6 +36,13 @@ enum imx_mu_chan_type {
> IMX_MU_TYPE_RXDB,   /* Rx doorbell */
>  };
>
> +struct imx_mu_dcfg {
> +   u32 xTR[4]; /* Transmit Registers */
> +   u32 xRR[4]; /* Receive Registers */
> +   u32 xSR;/* Status Register */
> +   u32 xCR;/* Control Register */
> +};
> +
>  struct imx_mu_con_priv {
> unsigned intidx;
> charirq_desc[IMX_MU_CHAN_NAME_SIZE];
> @@ -61,12 +60,39 @@ struct imx_mu_priv {
> struct mbox_chanmbox_chans[IMX_MU_CHANS];
>
> struct imx_mu_con_priv  con_priv[IMX_MU_CHANS];
> +   const struct imx_mu_dcfg*dcfg;
> struct clk  *clk;
> int irq;
>
> boolside_b;
>  };
>
> +static const struct imx_mu_dcfg imx_mu_cfg_imx6sx = {
> +   .xTR[0] = 0x0,
> +   .xTR[1] = 0x4,
> +   .xTR[2] = 0x8,
> +   .xTR[3] = 0xC,
> +   .xRR[0] = 0x10,
> +   .xRR[1] = 0x14,
> +   .xRR[2] = 0x18,
> +   .xRR[3] = 0x1C,
> +   .xSR= 0x20,
> +   .xCR= 0x24,
> +};
> +
> +static const struct imx_mu_dcfg imx_mu_cfg_imx7ulp = {
> +   .xTR[0] = 0x20,
> +   .xTR[1] = 0x24,
> +   .xTR[2] = 0x28,
> +   .xTR[3] = 0x2C,
> +   .xRR[0] = 0x40,
> +   .xRR[1] = 0x44,
> +   .xRR[2] = 0x48,
> +   .xRR[3] = 0x4C,
> +   .xSR= 0x60,
> +   .xCR= 0x64,
> +};
> +
>  static struct imx_mu_priv *to_imx_mu_priv(struct mbox_controller *mbox)
>  {
> return container_of(mbox, struct imx_mu_priv, mbox);
> @@ -88,10 +114,10 @@ static u32 imx_mu_xcr_rmw(struct imx_mu_priv *priv, u32 
> set, u32 clr)
> u32 val;
>
> spin_lock_irqsave(>xcr_lock, flags);
> -   val = imx_mu_read(priv, IMX_MU_xCR);
> +   val = imx_mu_read(priv, priv->dcfg->xCR);
> val &= ~clr;
> val |= set;
> -   imx_mu_write(priv, val, IMX_MU_xCR);
> +   imx_mu_write(priv, val, priv->dcfg->xCR);
> spin_unlock_irqrestore(>xcr_lock, flags);
>
> return val;
> @@ -111,8 +137,8 @@ static irqreturn_t imx_mu_isr(int irq, void *p)
> struct imx_mu_con_priv *cp = chan->con_priv;
> u32 val, ctrl, dat;
>
> -   ctrl = imx_mu_read(priv, IMX_MU_xCR);
> -   val = imx_mu_read(priv, IMX_MU_xSR);
> +   ctrl = imx_mu_read(priv, priv->dcfg->xCR);
> +   val = imx_mu_read(priv, priv->dcfg->xSR);
>
> switch (cp->type) {
> case IMX_MU_TYPE_TX:
> @@ -138,10 +164,10 @@ static irqreturn_t imx_mu_isr(int irq, void *p)
> imx_mu_xcr_rmw(priv, 0, IMX_MU_xCR_TIEn(cp->idx));
> mbox_chan_txdone(chan, 0);
> } else if (val == IMX_MU_xSR_RFn(cp->idx)) {
> -   dat = imx_mu_read(priv, IMX_MU_xRRn(cp->idx));
> +   dat = imx_mu_read(priv, priv->dcfg->xRR[cp->idx]);
> mbox_chan_received_data(chan, (void *));
> } else if (val == IMX_MU_xSR_GIPn(cp->idx)) {
> -   imx_mu_write(priv, IMX_MU_xSR_GIPn(cp->idx), IMX_MU_xSR);
> +   imx_mu_write(priv, IMX_MU_xSR_GIPn(cp->idx), priv->dcfg->xSR);
> mbox_chan_received_data(chan, NULL);
> } else {
> dev_warn_ratelimited(priv->dev, "Not handled 

Re: [PATCH V2 3/4] irq: imx-irqsteer: change to use reg_num instead of irq_group

[Dong Aisheng]

On Wed, Jan 30, 2019 at 9:23 PM Lucas Stach  wrote:
[...]
> > > +   /* one register bit map represents 32 input interrupts */
> > > +   data->reg_num /= 32;
> > +
> > > if (IS_ENABLED(CONFIG_PM_SLEEP)) {
> > > data->saved_reg = devm_kzalloc(>dev,
> > > -   sizeof(u32) * data->irq_groups * 2,
> > > +   sizeof(u32) * data->reg_num,
> >   GFP_KERNEL);
>
> Does this last parameter now fit on the line above?
>

No, 81 now. :)

Regards
Dong Aisheng

Re: [PATCH V2 4/4] irq: imx: irqsteer: add multi output interrupts support

[Dong Aisheng]

On Wed, Jan 30, 2019 at 9:33 PM Lucas Stach  wrote:
>
> Am Mittwoch, den 30.01.2019, 13:06 + schrieb Aisheng Dong:
> > One irqsteer channel can support up to 8 output interrupts.
> >
> > > Cc: Marc Zyngier 
> > > Cc: Lucas Stach 
> > > Cc: Shawn Guo 
> > > Signed-off-by: Dong Aisheng 
> > ---
> > ChangeLog:
> > v1->v2:
> >  * calculate irq_count by fsl,num-irqs instead of parsing interrupts
> >property from devicetree to match the input interrupts and outputs
> >  * improve output interrupt handler by searching only two registers
> >withint the same group
> > ---
> >  drivers/irqchip/irq-imx-irqsteer.c | 76 
> > +-
> >  1 file changed, 59 insertions(+), 17 deletions(-)
> >
> > diff --git a/drivers/irqchip/irq-imx-irqsteer.c 
> > b/drivers/irqchip/irq-imx-irqsteer.c
> > index 67ed862..cc40039 100644
> > --- a/drivers/irqchip/irq-imx-irqsteer.c
> > +++ b/drivers/irqchip/irq-imx-irqsteer.c
> > @@ -10,6 +10,7 @@
> >  #include 
> >  #include 
> >  #include 
> > +#include 
> >  #include 
> >  #include 
> >
> > @@ -21,10 +22,13 @@
> > >  #define CHAN_MINTDIS(t)(CTRL_STRIDE_OFF(t, 3) + 0x4)
> > >  #define CHAN_MASTRSTAT(t)  (CTRL_STRIDE_OFF(t, 3) + 0x8)
> >
> > > +#define CHAN_MAX_OUTPUT_INT0x8
> > +
> >  struct irqsteer_data {
> > > >   void __iomem*regs;
> > > >   struct clk  *ipg_clk;
> > > > - int irq;
> > > > + int irq[CHAN_MAX_OUTPUT_INT];
> > > > + int irq_count;
> > > >   raw_spinlock_t  lock;
> > > >   int reg_num;
> > > >   int channel;
> > @@ -87,26 +91,45 @@ static const struct irq_domain_ops 
> > imx_irqsteer_domain_ops = {
> > > >   .xlate  = irq_domain_xlate_onecell,
> >  };
> >
> > +static int imx_irqsteer_get_hwirq_base(struct irqsteer_data *data, u32 irq)
> > +{
> > > +   int i;
> > +
> > > +   for (i = 0; i < data->irq_count; i++) {
> > > +   if (data->irq[i] == irq)
> > + break;
>
> return i * 64; here...
> > + }
> > +
> > + return i * 64;
>
> ... and -EINVAL or something here, so we don't return a out of bounds
> hwirq base if the loop ever doesn't match something?
>

Good suggestion, will add it.

> > +}
> > +
> >  static void imx_irqsteer_irq_handler(struct irq_desc *desc)
> >  {
> > > struct irqsteer_data *data = irq_desc_get_handler_data(desc);
> > > +   int hwirq;
> > > int i;
> >
> > > chained_irq_enter(irq_desc_get_chip(desc), desc);
> >
> > > -   for (i = 0; i < data->reg_num * 32; i += 32) {
> > > -   int idx = imx_irqsteer_get_reg_index(data, i);
> > > +   hwirq = imx_irqsteer_get_hwirq_base(data, irq_desc_get_irq(desc));
> > +
> > > +   for (i = 0; i < 2; i++) {
> > > +   int idx = imx_irqsteer_get_reg_index(data, hwirq);
> > > unsigned long irqmap;
> > > int pos, virq;
> >
> > > +   if (hwirq >= data->reg_num * 32)
> > > +   break;
> > +
> > > irqmap = readl_relaxed(data->regs +
> > >CHANSTATUS(idx, data->reg_num));
> >
> > > for_each_set_bit(pos, , 32) {
> > > -   virq = irq_find_mapping(data->domain, pos + i);
> > + virq = irq_find_mapping(data->domain, pos + hwirq);
>
> The irq index calculation need to be "pos + i * 32 + hwirq", otherwise
> this will map to the wrong virqs for the second register in each group.
>

For second register map, hwirq will plus 32 in next round.
So i can't see this will map a wrong virqs.
And it looks to me ""pos + i * 32 + hwirq" is equal to "hwirq + 32".
Am i missed something?

> >   if (virq)
> > > generic_handle_irq(virq);
> > > }
> > + hwirq += 32;
>
> Could be folded into the loop head.
>

You mean “for (i = 0; i < 2; i++, hwirq +=32)” ?
I feel that's not quite necessary.

> >   }
> >
> > > chained_irq_exit(irq_desc_get_chip(desc), desc);
> > @@ -117,7 +140,8 @@ static int imx_irqsteer_probe(struct platform_device 
> > *

Re: [PATCH V2 2/4] dt-bindings: irq: imx-irqsteer: add multi output interrupts support

[Dong Aisheng]

On Wed, Jan 30, 2019 at 9:21 PM Lucas Stach  wrote:
>
> Am Mittwoch, den 30.01.2019, 13:05 + schrieb Aisheng Dong:
> > One irqsteer channel can support up to 8 output interrupts.
> >
> > > Cc: Marc Zyngier 
> > > Cc: Rob Herring 
> > > Cc: Lucas Stach 
> > > Cc: Shawn Guo 
> > Cc: devicet...@vger.kernel.org
> > > Signed-off-by: Dong Aisheng 
> > ---
> > ChangeLog:
> > v1->v2:
> >  * remove one unnecessary note.
> > ---
> >  .../devicetree/bindings/interrupt-controller/fsl,irqsteer.txt| 5 
> > +++--
> >  1 file changed, 3 insertions(+), 2 deletions(-)
> >
> > diff --git 
> > a/Documentation/devicetree/bindings/interrupt-controller/fsl,irqsteer.txt 
> > b/Documentation/devicetree/bindings/interrupt-controller/fsl,irqsteer.txt
> > index 6d0a41b..984ab92 100644
> > --- 
> > a/Documentation/devicetree/bindings/interrupt-controller/fsl,irqsteer.txt
> > +++ 
> > b/Documentation/devicetree/bindings/interrupt-controller/fsl,irqsteer.txt
> > @@ -6,8 +6,9 @@ Required properties:
> > > - "fsl,imx8m-irqsteer"
> > > - "fsl,imx-irqsteer"
> >  - reg: Physical base address and size of registers.
> > -- interrupts: Should contain the parent interrupt line used to multiplex 
> > the
> > -  input interrupts.
> > +- interrupts: Should contain the up to 8 parent interrupt lines used to
> > +  multiplex the input interrupts. They should be sepcified seqencially
>
> Typos here, should be: "specified sequentially"
>

Nice catch. Will update in new version.
Thanks

Regards
Dong Aisheng

> > +  from output 0 to 7.
> >  - clocks: Should contain one clock for entry in clock-names
> >see Documentation/devicetree/bindings/clock/clock-bindings.txt
> >  - clock-names:

Re: [PATCH v2] firmware: imx: Add support to start/stop a CPU

[Dong Aisheng]

On Wed, Jan 30, 2019 at 9:12 PM Daniel Baluta  wrote:
>
> Thanks Aisheng for the comments!
>
> 
>
> +int imx_sc_pm_cpu_start(struct imx_sc_ipc *ipc, u32 resource,
> > > +   bool enable, u64 address)
> > > +{
> > > +   struct imx_sc_msg_req_cpu_start msg;
> > > +   struct imx_sc_rpc_msg *hdr = 
> > > +
> > > +   hdr->ver = IMX_SC_RPC_VERSION;
> > > +   hdr->svc = (uint8_t)IMX_SC_RPC_SVC_PM;
> > > +   hdr->func = (uint8_t)IMX_SC_PM_FUNC_CPU_START;
> >
> > pls drop the unneccesary unit8_t
>
> Ok, I can do that. I had borrowed it from the already existing
> functions imx_sc_misc_get_control / imx_sc_misc_set_control
>

Yes, i know, we need remove them later to avoid confusing.

Regards
Dong Aisheng

> >
> > > +   hdr->size = 4;
> > > +
> > > +   msg.address_hi = address >> 32;
> > > +   msg.address_lo = address;
> > > +   msg.resource = resource;
> > > +   msg.enable = enable;
> > > +
> > > +   return imx_scu_call_rpc(ipc, , false);
> >
> > s/false/true
>
> Nice catch!
>
> Inded, the old API had a different semantic for this parameter.
> >
> > > +}
> > > +EXPORT_SYMBOL(imx_sc_pm_cpu_start);
> > > diff --git a/include/linux/firmware/imx/svc/misc.h
> > > b/include/linux/firmware/imx/svc/misc.h
> > > index e21c49aba92f..c03bf2a23add 100644
> > > --- a/include/linux/firmware/imx/svc/misc.h
> > > +++ b/include/linux/firmware/imx/svc/misc.h
> > > @@ -52,4 +52,7 @@ int imx_sc_misc_set_control(struct imx_sc_ipc
> > > *ipc, u32 resource,
> > >  int imx_sc_misc_get_control(struct imx_sc_ipc *ipc, u32 resource,
> > > u8 ctrl, u32 *val);
> > >
> > > +int imx_sc_pm_cpu_start(struct imx_sc_ipc *ipc, u32 resource,
> > > +   bool enable, u64 address);
> >
> > Nitpick: phy_addr
>
> Ok, will fix.
>
>

Re: [PATCH v2] firmware: imx: Add support to start/stop a CPU

[Dong Aisheng]

On Wed, Jan 30, 2019 at 5:59 PM Daniel Baluta  wrote:
>
> This is done via RPC call to SCU.
>
> Signed-off-by: Daniel Baluta 
> ---
> Changes since v1:
> - remove unused variable ret
> - add documentation for imx_sc_pm_cpu_start function
>
>  drivers/firmware/imx/misc.c   | 38 +++
>  include/linux/firmware/imx/svc/misc.h |  3 +++
>  2 files changed, 41 insertions(+)
>
> diff --git a/drivers/firmware/imx/misc.c b/drivers/firmware/imx/misc.c
> index 97f5424dbac9..f511d6f624fd 100644
> --- a/drivers/firmware/imx/misc.c
> +++ b/drivers/firmware/imx/misc.c
> @@ -18,6 +18,14 @@ struct imx_sc_msg_req_misc_set_ctrl {
> u16 resource;
>  } __packed;
>
> +struct imx_sc_msg_req_cpu_start {
> +   struct imx_sc_rpc_msg hdr;
> +   u32 address_hi;
> +   u32 address_lo;
> +   u16 resource;
> +   u8 enable;
> +} __packed;
> +
>  struct imx_sc_msg_req_misc_get_ctrl {
> struct imx_sc_rpc_msg hdr;
> u32 ctrl;
> @@ -97,3 +105,33 @@ int imx_sc_misc_get_control(struct imx_sc_ipc *ipc, u32 
> resource,
> return 0;
>  }
>  EXPORT_SYMBOL(imx_sc_misc_get_control);
> +
> +/*
> + * This function starts/stops a CPU identified by @resource
> + *
> + * @param[in] ipc IPC handle
> + * @param[in] resourceresource the control is associated with
> + * @param[in] enable  true for start, false for stop
> + * @param[out]address initial instruction address to be executed
> + *
> + * @return Returns 0 for success and < 0 for errors.
> + */
> +int imx_sc_pm_cpu_start(struct imx_sc_ipc *ipc, u32 resource,
> +   bool enable, u64 address)
> +{
> +   struct imx_sc_msg_req_cpu_start msg;
> +   struct imx_sc_rpc_msg *hdr = 
> +
> +   hdr->ver = IMX_SC_RPC_VERSION;
> +   hdr->svc = (uint8_t)IMX_SC_RPC_SVC_PM;
> +   hdr->func = (uint8_t)IMX_SC_PM_FUNC_CPU_START;

pls drop the unneccesary unit8_t

> +   hdr->size = 4;
> +
> +   msg.address_hi = address >> 32;
> +   msg.address_lo = address;
> +   msg.resource = resource;
> +   msg.enable = enable;
> +
> +   return imx_scu_call_rpc(ipc, , false);

s/false/true

> +}
> +EXPORT_SYMBOL(imx_sc_pm_cpu_start);
> diff --git a/include/linux/firmware/imx/svc/misc.h 
> b/include/linux/firmware/imx/svc/misc.h
> index e21c49aba92f..c03bf2a23add 100644
> --- a/include/linux/firmware/imx/svc/misc.h
> +++ b/include/linux/firmware/imx/svc/misc.h
> @@ -52,4 +52,7 @@ int imx_sc_misc_set_control(struct imx_sc_ipc *ipc, u32 
> resource,
>  int imx_sc_misc_get_control(struct imx_sc_ipc *ipc, u32 resource,
> u8 ctrl, u32 *val);
>
> +int imx_sc_pm_cpu_start(struct imx_sc_ipc *ipc, u32 resource,
> +   bool enable, u64 address);

Nitpick: phy_addr

Otherwise, looks good to me.

Regards
Dong Aisheng

> +
>  #endif /* _SC_MISC_API_H */
> --
> 2.17.1
>

Re: [PATCH 2/4] dt-bindings: irq: imx-irqsteer: add multi output interrupts support

[Dong Aisheng]

On Fri, Jan 25, 2019 at 6:47 PM Lucas Stach  wrote:
>
> Am Freitag, den 18.01.2019, 07:53 + schrieb Aisheng Dong:
> > One irqsteer channel can support up to 8 output interrupts.
> >
> > > Cc: Marc Zyngier 
> > > Cc: Rob Herring 
> > > Cc: Lucas Stach 
> > > Cc: Shawn Guo 
> > Cc: devicet...@vger.kernel.org
> > > Signed-off-by: Dong Aisheng 
> > ---
> >  .../devicetree/bindings/interrupt-controller/fsl,irqsteer.txt| 5 
> > +++--
> >  1 file changed, 3 insertions(+), 2 deletions(-)
> >
> > diff --git 
> > a/Documentation/devicetree/bindings/interrupt-controller/fsl,irqsteer.txt 
> > b/Documentation/devicetree/bindings/interrupt-controller/fsl,irqsteer.txt
> > index eaabcda..beeed4a 100644
> > --- 
> > a/Documentation/devicetree/bindings/interrupt-controller/fsl,irqsteer.txt
> > +++ 
> > b/Documentation/devicetree/bindings/interrupt-controller/fsl,irqsteer.txt
> > @@ -6,8 +6,9 @@ Required properties:
> > > - "fsl,imx8m-irqsteer"
> > > - "fsl,imx-irqsteer"
> >  - reg: Physical base address and size of registers.
> > -- interrupts: Should contain the parent interrupt line used to multiplex 
> > the
> > -  input interrupts.
> > +- interrupts: Should contain the up to 8 parent interrupt lines used to
> > +  multiplex the input interrupts. They should be sepcified seqencially
> > +  from output 0 to 7. NOTE at least one output interrupt has to be 
> > specified.
>
> I don't think the note clarifies anything. Given that the thing is a
> irq multiplexer I don't think anyone expects that you could get away
> with no output irq.
>

Sound reasonable to me.
I will remove the note and keep other as it is.

Regards
Dong Aisheng

Re: [PATCH 4/4] irq: imx: irqsteer: add multi output interrupts support

[Dong Aisheng]

On Fri, Jan 25, 2019 at 6:55 PM Lucas Stach  wrote:
>
> Am Freitag, den 18.01.2019, 07:53 + schrieb Aisheng Dong:
> > One irqsteer channel can support up to 8 output interrupts.
> >
> > > Cc: Marc Zyngier 
> > > Cc: Lucas Stach 
> > > Cc: Shawn Guo 
> > > Signed-off-by: Dong Aisheng 
> > ---
> >  drivers/irqchip/irq-imx-irqsteer.c | 39 
> > +++---
> >  1 file changed, 28 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/irqchip/irq-imx-irqsteer.c 
> > b/drivers/irqchip/irq-imx-irqsteer.c
> > index 1bebf0a..54802fa 100644
> > --- a/drivers/irqchip/irq-imx-irqsteer.c
> > +++ b/drivers/irqchip/irq-imx-irqsteer.c
> > @@ -10,6 +10,7 @@
> >  #include 
> >  #include 
> >  #include 
> > +#include 
> >  #include 
> >  #include 
> >
> > @@ -21,10 +22,13 @@
> > >  #define CHAN_MINTDIS(t)(CTRL_STRIDE_OFF(t, 3) + 0x4)
> > >  #define CHAN_MASTRSTAT(t)  (CTRL_STRIDE_OFF(t, 3) + 0x8)
> >
> > > +#define CHAN_MAX_OUTPUT_INT0x8
> > +
> >  struct irqsteer_data {
> > > >   void __iomem*regs;
> > > >   struct clk  *ipg_clk;
> > > > - int irq;
> > > > + int irq[CHAN_MAX_OUTPUT_INT];
> > > > + int irq_count;
> > > >   raw_spinlock_t  lock;
> > > >   int reg_num;
> > > >   int channel;
> > @@ -117,7 +121,7 @@ static int imx_irqsteer_probe(struct platform_device 
> > *pdev)
> > > struct device_node *np = pdev->dev.of_node;
> > > struct irqsteer_data *data;
> > > struct resource *res;
> > > -   int ret;
> > > +   int i, ret;
> >
> > > data = devm_kzalloc(>dev, sizeof(*data), GFP_KERNEL);
> > > if (!data)
> > @@ -130,12 +134,6 @@ static int imx_irqsteer_probe(struct platform_device 
> > *pdev)
> > > return PTR_ERR(data->regs);
> > > }
> >
> > > -   data->irq = platform_get_irq(pdev, 0);
> > > -   if (data->irq <= 0) {
> > > -   dev_err(>dev, "failed to get irq\n");
> > > -   return -ENODEV;
> > > -   }
> > -
> > > data->ipg_clk = devm_clk_get(>dev, "ipg");
> > > if (IS_ERR(data->ipg_clk)) {
> > > ret = PTR_ERR(data->ipg_clk);
> > @@ -177,8 +175,23 @@ static int imx_irqsteer_probe(struct platform_device 
> > *pdev)
> > > return -ENOMEM;
> > > }
> >
> > > -   irq_set_chained_handler_and_data(data->irq, imx_irqsteer_irq_handler,
> > > -data);
> > + data->irq_count = of_irq_count(np);
>
> We normally don't validate stuff that comes from the DT, but I guess it
> might be helpful to validate that the number of output irqs specified
> matches what the number of input irqs, i.e. there is one output irqs
> specified for each group of 64 inputs.
>

Sound like a good idea.
I think we can calculate the irq_count from irqs number instead of of_irq_count.
Then the following irq_of_parse_and_map() will validate them automatically.

However, i think we'd better to keep the irq_count range check as well.

> > + if (!data->irq_count || data->irq_count > CHAN_MAX_OUTPUT_INT) {
> > > +   clk_disable_unprepare(data->ipg_clk);
> > > +   return -EINVAL;
> > > +   }
> > +
> > > +   for (i = 0; i < data->irq_count; i++) {
> > > +   data->irq[i] = irq_of_parse_and_map(np, i);
> > > +   if (!data->irq[i]) {
> > > +   clk_disable_unprepare(data->ipg_clk);
> > > +   return -EINVAL;
> > > +   }
> > +
> > > +   irq_set_chained_handler_and_data(data->irq[i],
> > > +imx_irqsteer_irq_handler,
> > +  data);
>
> We really want some data about the output irq being passed here, so we
> can cut down the number of register reads in the irq handler to the
> maximum of 2 status registers per group.
>

Will implement it and resend.

Regards
Dong Aisheng

> Regards,
> Lucas

Re: [PATCH 4/4] irq: imx: irqsteer: add multi output interrupts support

[Dong Aisheng]

Hi Lucas,

[...]

> > > The fsl,irq-groups property is exactly your NINT32 parameter above. I just
> > > wrongly assumed that it's always in multiples of 64, as that's what the
> > > i.MX8MQ DCSS irqsteer module looks like. We should fix this and be done 
> > > with
> > > it.
> > >
> >
> > No, not exactly the same thing. Using group will confuse people that the 
> > group is 32.
> > However, internally Group is fixed 64 interrupts although it may not use 
> > all the
> > 64 interrupts. E.g. 32 interrupts.
> > See CHn_MINTDIS register which is also defined fixed to 64.
> >
> > The two HW parameter for integration is already very clear. We should use 
> > interrupts
> > Number for the channel. Not group.
>
> Okay, I see that the name irq-groups is confusing for you. But then I
> find the -per-chan naming confusing.
>
> So given that we seem to agree to split each channel into it's own DT
> node, there is no need to name the property "something-per-chan", as
> it's implied by the split DT nodes that all properties in one node are
> referencing a channel.
>

Double checked with IP module owner by looking into the RTL code,
writes to CHANnCTL registers have no effect.
This register is reserved and the doc will be updated.
Verified on both MX8QXP and MX8MQ.

All channels are isolated and have independent programming model.
So it will be okay to define them separately in device tree.

> May I suggest to name the property "fsl,num-irqs"?

Sounds good to me.

Regards
Dong Aisheng

>
> Regards,
> Lucas
>

[PATCH V2 1/1] MAINTAINERS: imx: include drivers/firmware/imx path

[Dong Aisheng]

Due to newly added IMX SCU firmware support, let's add
drivers/firmware/imx into maintainership.

Cc: Shawn Guo 
Cc: Arnd Bergmann 
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Dong Aisheng 
---
v2:
 * add missing include/linux/firmware/imx
---
 MAINTAINERS | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 9ad052a..eee17f2 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1462,7 +1462,9 @@ F:arch/arm/mach-mxs/
 F: arch/arm/boot/dts/imx*
 F: arch/arm/configs/imx*_defconfig
 F: drivers/clk/imx/
+F: drivers/firmware/imx/
 F: drivers/soc/imx/
+F: include/linux/firmware/imx/
 F: include/soc/imx/
 
 ARM/FREESCALE VYBRID ARM ARCHITECTURE
-- 
2.7.4

[PATCH V2 1/1] MAINTAINERS: imx: include drivers/firmware/imx path

[Dong Aisheng]

Due to newly added IMX SCU firmware support, let's add
drivers/firmware/imx into maintainership.

Cc: Shawn Guo 
Cc: Arnd Bergmann 
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Dong Aisheng 
---
v2:
 * add missing include/linux/firmware/imx
---
 MAINTAINERS | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 9ad052a..eee17f2 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1462,7 +1462,9 @@ F:arch/arm/mach-mxs/
 F: arch/arm/boot/dts/imx*
 F: arch/arm/configs/imx*_defconfig
 F: drivers/clk/imx/
+F: drivers/firmware/imx/
 F: drivers/soc/imx/
+F: include/linux/firmware/imx/
 F: include/soc/imx/
 
 ARM/FREESCALE VYBRID ARM ARCHITECTURE
-- 
2.7.4