[GIT PULL] clk fixes for v4.20-rc6

2018-12-07 Thread Stephen Boyd
The following changes since commit 1aefa98b010e9cc7a07046cbcb1237ddad85b708:

  clk: qcom: gcc: Fix board clock node name (2018-11-09 14:13:55 -0800)

are available in the Git repository at:

  https://git.kernel.org/pub/scm/linux/kernel/git/clk/linux.git 
tags/clk-fixes-for-linus

for you to fetch changes up to 9a43be9cedd516f188e6333d3b43402386723eff:

  clk: zynqmp: Off by one in zynqmp_is_valid_clock() (2018-12-03 09:54:48 -0800)


A few clk driver fixes this time:

 - Introduce protected-clock DT binding to fix breakage on qcom sdm845-mtp
   boards where the qspi clks introduced this merge window cause the
   firmware on those boards to take down the system if we try to read
   the clk registers

 - Fix a couple off-by-one errors found by Dan Carpenter

 - Handle failure in zynq fixed factor clk driver to avoid using
   uninitialized data


Bjorn Andersson (1):
  arm64: dts: qcom: sdm845-mtp: Mark protected gcc clocks

Dan Carpenter (3):
  clk: mvebu: Off by one bugs in cp110_of_clk_get()
  clk: mmp: Off by one in mmp_clk_add()
  clk: zynqmp: Off by one in zynqmp_is_valid_clock()

Rajan Vaja (1):
  clk: zynqmp: handle fixed factor param query error

Stephen Boyd (3):
  dt-bindings: clk: Introduce 'protected-clocks' property
  clk: qcom: Support 'protected-clocks' property
  Merge branch 'clk-protected-binding' into clk-fixes

 .../devicetree/bindings/clock/clock-bindings.txt   | 16 
 arch/arm64/boot/dts/qcom/sdm845-mtp.dts|  6 ++
 drivers/clk/mmp/clk.c  |  2 +-
 drivers/clk/mvebu/cp110-system-controller.c|  4 ++--
 drivers/clk/qcom/common.c  | 18 ++
 drivers/clk/zynqmp/clkc.c  |  5 -
 6 files changed, 47 insertions(+), 4 deletions(-)

-- 
Sent by a computer through tubes


Re: [PATCH] Revert "clk: fix __clk_init_parent() for single parent clocks"

2018-12-06 Thread Stephen Boyd
Sorry this email is long.

TL;DR is that I don't think we need to revert the patch as you suggest.
Instead, we should fix the driver to indicate NULL as the parent name of
the index it tells the framework about.

Quoting Jerome Brunet (2018-12-05 09:20:09)
> On Tue, 2018-12-04 at 23:54 -0800, Stephen Boyd wrote:
> > 
> > Ok. Thanks for the explanation. What do you do to fix this problem in
> > the 'ao_cts_cec' clk implementation?
> 
> IMO, there is nothing to fix, the implementation is correct. 
> 
> >  Sounds like you just rely on
> > clk_set_rate() to fix this all up for you when the consumer changes the
> > rate?
> 
> To setup the rate as the customer expect, yes.

Ok.

> 
> > 
> > If we have something that is default parented to something we're not
> > telling the framework about for some reason then one solution would be
> > to have some init code reparent the clk in hardware before registering
> > it with the framework. Basically "fix up" the clk tree in hardware to be
> > consistent with what software can reason about. 
> 
> Should we really ? Something the framework does not know about is not
> necessarily something wrong.
> 
> It would best if we could model the tree completly, but most of the time, we
> just approximate it the best we can.
> 
> The framework just knows how to setup rates, it has no idea what rate needs to
> be set or when - And I think it is best if does not make any assumption about
> that.
> 
> Pushing it a bit, this unknown parent might important to the boot sequence.
> How would you know when it is safe to change the setting ?
> What would change it to ? It Seems obvious when there is only one known
> parent, but what if there two ? which one do you pick ? Is it really the role
> of CCF to make this kind of choice ? 

Agreed. We can't mandate this requirement because of what you say. There
are pitfalls such as not always knowing if it's safe to change the clk
tree before clks are registered.

> 
> > 
> > This also reminds me of some audio clks I've seen before where the
> > default parent is some external pin and it can be reparented to an
> > internal clk with clk_set_rate/parent. In that case, I think we forced
> > the parent over to the internal clk before registering so that it would
> > always get parented properly in the framework. Right now, this is going
> > to cause problems for plans to probe defer clk_get() on orphan clks.
> 
> clk_get() on orphaned clock should probably return an error if it is not the
> case already ? or 0 maybe ?

I'd prefer that it return EPROBE_DEFER if the clk is an orphan, except
that it causes problems for this case where muxes move away from
something unknown to something known.

> 
> > 
> > Maybe this could work better if we allowed
> > 'assigned-clock-parents/rates' to reparent clks regardless of orphan
> > status and/or had some flag that could be set on clks to indicate that
> > we're OK if clk_get() is called on it when it's an orphan because this
> > isn't a problem?
> 
> Not sure I understand your point here.

There is a problem with assigned-clock DT properties and orphan clks and
making orphan clks defer driver probes.

> 
> > 
> > Or we can make the defer on orphan code only defer if the clk has a
> > single parent and it's an orphan and return the clk if there is at least
> > one parent of the clk that has been registered and isn't marked as an
> > orphan. That would need to be recursively applied, so I guess we would
> > update our orphan marking code to indicate that clk_get() on the clk
> > should probe defer or not instead of indicating the clk's orphan status.
> > Doing this would also side-step the problem Rockchip was having where
> > certain parents never appeared but the clk was parented to it in
> > hardware, essentially blocking the clk from ever being touched by
> > consumers.
> 
> ... and now you lost me completly :)

Hmm alright.

> 
> > 
> > On the other hand, not having the clk there causes us to do a global
> > search of the clk name space each time we look for parents by the
> > "unknown" index, which in the Rockchip case was quite often because the
> > clk was changing between two other parents with a third one that never
> > got registered. So it may be better to just register an "unknown" clk as
> > a root clk with a rate of 0 that you can then hook everything up to that
> > is hardware reset to this unknown input. That way nothing is orphaned
> > and everyone is happy. We could do this for the audio case I talked
> > about earlier too so when the external pin is left unconnected we could
> > regi

[PATCH 2/8] clk: st: Remove usage of CLK_IS_BASIC

2018-12-06 Thread Stephen Boyd
This flag doesn't look to be used by any code, just set in various clk
init structures and then never tested again. Remove it from these
drivers as it doesn't provide any benefit.

Signed-off-by: Stephen Boyd 
---
 drivers/clk/st/clk-flexgen.c | 2 +-
 drivers/clk/st/clkgen-fsyn.c | 4 ++--
 drivers/clk/st/clkgen-pll.c  | 2 +-
 3 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/clk/st/clk-flexgen.c b/drivers/clk/st/clk-flexgen.c
index 918ba3164da9..ca8d5350f94e 100644
--- a/drivers/clk/st/clk-flexgen.c
+++ b/drivers/clk/st/clk-flexgen.c
@@ -210,7 +210,7 @@ static struct clk *clk_register_flexgen(const char *name,
 
init.name = name;
init.ops = _ops;
-   init.flags = CLK_IS_BASIC | CLK_GET_RATE_NOCACHE | flexgen_flags;
+   init.flags = CLK_GET_RATE_NOCACHE | flexgen_flags;
init.parent_names = parent_names;
init.num_parents = num_parents;
 
diff --git a/drivers/clk/st/clkgen-fsyn.c b/drivers/clk/st/clkgen-fsyn.c
index cfa07622..946ceb14dbf7 100644
--- a/drivers/clk/st/clkgen-fsyn.c
+++ b/drivers/clk/st/clkgen-fsyn.c
@@ -404,7 +404,7 @@ static struct clk * __init st_clk_register_quadfs_pll(
 
init.name = name;
init.ops = quadfs->pll_ops;
-   init.flags = CLK_IS_BASIC | CLK_GET_RATE_NOCACHE;
+   init.flags = CLK_GET_RATE_NOCACHE;
init.parent_names = _name;
init.num_parents = 1;
 
@@ -843,7 +843,7 @@ static struct clk * __init st_clk_register_quadfs_fsynth(
 
init.name = name;
init.ops = _quadfs_ops;
-   init.flags = flags | CLK_GET_RATE_NOCACHE | CLK_IS_BASIC;
+   init.flags = flags | CLK_GET_RATE_NOCACHE;
init.parent_names = _name;
init.num_parents = 1;
 
diff --git a/drivers/clk/st/clkgen-pll.c b/drivers/clk/st/clkgen-pll.c
index 7a7106dc80bf..6930348ce843 100644
--- a/drivers/clk/st/clkgen-pll.c
+++ b/drivers/clk/st/clkgen-pll.c
@@ -613,7 +613,7 @@ static struct clk * __init clkgen_pll_register(const char 
*parent_name,
init.name = clk_name;
init.ops = pll_data->ops;
 
-   init.flags = pll_flags | CLK_IS_BASIC | CLK_GET_RATE_NOCACHE;
+   init.flags = pll_flags | CLK_GET_RATE_NOCACHE;
init.parent_names = _name;
init.num_parents  = 1;
 
-- 
Sent by a computer through tubes



[PATCH 4/8] clk: h8300: Remove usage of CLK_IS_BASIC

2018-12-06 Thread Stephen Boyd
This flag doesn't look to be used by any code, just set in various clk
init structures and then never tested again. Remove it from these
drivers as it doesn't provide any benefit.

Cc: Yoshinori Sato 
Cc: 
Signed-off-by: Stephen Boyd 
---
 drivers/clk/h8300/clk-h8s2678.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/clk/h8300/clk-h8s2678.c b/drivers/clk/h8300/clk-h8s2678.c
index b68045d8b921..c7ae653c8a16 100644
--- a/drivers/clk/h8300/clk-h8s2678.c
+++ b/drivers/clk/h8300/clk-h8s2678.c
@@ -117,7 +117,7 @@ static void __init h8s2678_pll_clk_setup(struct device_node 
*node)
parent_name = of_clk_get_parent_name(node, 0);
init.name = clk_name;
init.ops = _ops;
-   init.flags = CLK_IS_BASIC;
+   init.flags = 0;
init.parent_names = _name;
init.num_parents = 1;
pll_clock->hw.init = 
-- 
Sent by a computer through tubes



[PATCH 3/8] clk: axm5516: Remove usage of CLK_IS_BASIC

2018-12-06 Thread Stephen Boyd
This flag doesn't look to be used by any code, just set in various clk
init structures and then never tested again. Remove it from these
drivers as it doesn't provide any benefit.

Cc: Anders Berg 
Signed-off-by: Stephen Boyd 
---
 drivers/clk/clk-axm5516.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/clk/clk-axm5516.c b/drivers/clk/clk-axm5516.c
index 5d7ae333257e..98e0c9ba7b61 100644
--- a/drivers/clk/clk-axm5516.c
+++ b/drivers/clk/clk-axm5516.c
@@ -311,7 +311,6 @@ static struct axxia_divclk clk_per_div = {
"clk_sm1_pll"
},
.num_parents = 1,
-   .flags = CLK_IS_BASIC,
.ops = _divclk_ops,
},
.reg   = 0x1000c,
@@ -326,7 +325,6 @@ static struct axxia_divclk clk_mmc_div = {
"clk_sm1_pll"
},
.num_parents = 1,
-   .flags = CLK_IS_BASIC,
.ops = _divclk_ops,
},
.reg   = 0x1000c,
-- 
Sent by a computer through tubes



[PATCH 1/8] clk: renesas: Remove usage of CLK_IS_BASIC

2018-12-06 Thread Stephen Boyd
This flag doesn't look to be used by any code, just set in various clk
init structures and then never tested again. Remove it from these
drivers as it doesn't provide any benefit.

Cc: Geert Uytterhoeven 
Cc: 
Signed-off-by: Stephen Boyd 
---
 drivers/clk/renesas/clk-div6.c | 2 +-
 drivers/clk/renesas/clk-mstp.c | 2 +-
 drivers/clk/renesas/r9a06g032-clocks.c | 8 
 drivers/clk/renesas/rcar-gen3-cpg.c| 2 +-
 drivers/clk/renesas/renesas-cpg-mssr.c | 2 +-
 5 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/clk/renesas/clk-div6.c b/drivers/clk/renesas/clk-div6.c
index 57c934164306..e98a9f5b3c90 100644
--- a/drivers/clk/renesas/clk-div6.c
+++ b/drivers/clk/renesas/clk-div6.c
@@ -274,7 +274,7 @@ struct clk * __init cpg_div6_register(const char *name,
/* Register the clock. */
init.name = name;
init.ops = _div6_clock_ops;
-   init.flags = CLK_IS_BASIC;
+   init.flags = 0;
init.parent_names = parent_names;
init.num_parents = valid_parents;
 
diff --git a/drivers/clk/renesas/clk-mstp.c b/drivers/clk/renesas/clk-mstp.c
index 1c1768c2cc82..2ba6105937e3 100644
--- a/drivers/clk/renesas/clk-mstp.c
+++ b/drivers/clk/renesas/clk-mstp.c
@@ -158,7 +158,7 @@ static struct clk * __init cpg_mstp_clock_register(const 
char *name,
 
init.name = name;
init.ops = _mstp_clock_ops;
-   init.flags = CLK_IS_BASIC | CLK_SET_RATE_PARENT;
+   init.flags = CLK_SET_RATE_PARENT;
/* INTC-SYS is the module clock of the GIC, and must not be disabled */
if (!strcmp(name, "intc-sys")) {
pr_debug("MSTP %s setting CLK_IS_CRITICAL\n", name);
diff --git a/drivers/clk/renesas/r9a06g032-clocks.c 
b/drivers/clk/renesas/r9a06g032-clocks.c
index 6d2b56891559..658cb11b6f55 100644
--- a/drivers/clk/renesas/r9a06g032-clocks.c
+++ b/drivers/clk/renesas/r9a06g032-clocks.c
@@ -424,7 +424,7 @@ r9a06g032_register_gate(struct r9a06g032_priv *clocks,
 
init.name = desc->name;
init.ops = _clk_gate_ops;
-   init.flags = CLK_IS_BASIC | CLK_SET_RATE_PARENT;
+   init.flags = CLK_SET_RATE_PARENT;
init.parent_names = parent_name ? _name : NULL;
init.num_parents = parent_name ? 1 : 0;
 
@@ -595,7 +595,7 @@ r9a06g032_register_div(struct r9a06g032_priv *clocks,
 
init.name = desc->name;
init.ops = _clk_div_ops;
-   init.flags = CLK_IS_BASIC | CLK_SET_RATE_PARENT;
+   init.flags = CLK_SET_RATE_PARENT;
init.parent_names = parent_name ? _name : NULL;
init.num_parents = parent_name ? 1 : 0;
 
@@ -683,7 +683,7 @@ r9a06g032_register_bitsel(struct r9a06g032_priv *clocks,
 
init.name = desc->name;
init.ops = _bitselect_ops;
-   init.flags = CLK_IS_BASIC | CLK_SET_RATE_PARENT;
+   init.flags = CLK_SET_RATE_PARENT;
init.parent_names = names;
init.num_parents = 2;
 
@@ -777,7 +777,7 @@ r9a06g032_register_dualgate(struct r9a06g032_priv *clocks,
 
init.name = desc->name;
init.ops = _clk_dualgate_ops;
-   init.flags = CLK_IS_BASIC | CLK_SET_RATE_PARENT;
+   init.flags = CLK_SET_RATE_PARENT;
init.parent_names = _name;
init.num_parents = 1;
g->hw.init = 
diff --git a/drivers/clk/renesas/rcar-gen3-cpg.c 
b/drivers/clk/renesas/rcar-gen3-cpg.c
index 4ba38f98cc7b..48e003c69cd1 100644
--- a/drivers/clk/renesas/rcar-gen3-cpg.c
+++ b/drivers/clk/renesas/rcar-gen3-cpg.c
@@ -368,7 +368,7 @@ static struct clk * __init cpg_sd_clk_register(const struct 
cpg_core_clk *core,
 
init.name = core->name;
init.ops = _sd_clock_ops;
-   init.flags = CLK_IS_BASIC | CLK_SET_RATE_PARENT;
+   init.flags = CLK_SET_RATE_PARENT;
init.parent_names = _name;
init.num_parents = 1;
 
diff --git a/drivers/clk/renesas/renesas-cpg-mssr.c 
b/drivers/clk/renesas/renesas-cpg-mssr.c
index f7bb817420b4..30df0dc853f0 100644
--- a/drivers/clk/renesas/renesas-cpg-mssr.c
+++ b/drivers/clk/renesas/renesas-cpg-mssr.c
@@ -412,7 +412,7 @@ static void __init cpg_mssr_register_mod_clk(const struct 
mssr_mod_clk *mod,
 
init.name = mod->name;
init.ops = _mstp_clock_ops;
-   init.flags = CLK_IS_BASIC | CLK_SET_RATE_PARENT;
+   init.flags = CLK_SET_RATE_PARENT;
for (i = 0; i < info->num_crit_mod_clks; i++)
if (id == info->crit_mod_clks[i]) {
dev_dbg(dev, "MSTP %s setting CLK_IS_CRITICAL\n",
-- 
Sent by a computer through tubes



[PATCH 5/8] clk: hisilicon: Remove usage of CLK_IS_BASIC

2018-12-06 Thread Stephen Boyd
This flag doesn't look to be used by any code, just set in various clk
init structures and then never tested again. Remove it from these
drivers as it doesn't provide any benefit.

Cc: Jiancheng Xue 
Cc: Leo Yan 
Cc: Jianguo Sun 
Cc: Wei Yongjun 
Signed-off-by: Stephen Boyd 
---
 drivers/clk/hisilicon/clk-hi3620.c| 2 +-
 drivers/clk/hisilicon/clk-hisi-phase.c| 2 +-
 drivers/clk/hisilicon/clk-hix5hd2.c   | 2 +-
 drivers/clk/hisilicon/clkgate-separated.c | 2 +-
 4 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/clk/hisilicon/clk-hi3620.c 
b/drivers/clk/hisilicon/clk-hi3620.c
index 77072c7778b9..2eda9bdf6d03 100644
--- a/drivers/clk/hisilicon/clk-hi3620.c
+++ b/drivers/clk/hisilicon/clk-hi3620.c
@@ -435,7 +435,7 @@ static struct clk *hisi_register_clk_mmc(struct 
hisi_mmc_clock *mmc_clk,
 
init.name = mmc_clk->name;
init.ops = _mmc_ops;
-   init.flags = mmc_clk->flags | CLK_IS_BASIC;
+   init.flags = mmc_clk->flags;
init.parent_names = (mmc_clk->parent_name ? _clk->parent_name : 
NULL);
init.num_parents = (mmc_clk->parent_name ? 1 : 0);
mclk->hw.init = 
diff --git a/drivers/clk/hisilicon/clk-hisi-phase.c 
b/drivers/clk/hisilicon/clk-hisi-phase.c
index 5bce9297b78b..5fdc267bb2da 100644
--- a/drivers/clk/hisilicon/clk-hisi-phase.c
+++ b/drivers/clk/hisilicon/clk-hisi-phase.c
@@ -103,7 +103,7 @@ struct clk *clk_register_hisi_phase(struct device *dev,
 
init.name = clks->name;
init.ops = _phase_ops;
-   init.flags = clks->flags | CLK_IS_BASIC;
+   init.flags = clks->flags;
init.parent_names = clks->parent_names ? >parent_names : NULL;
init.num_parents = clks->parent_names ? 1 : 0;
 
diff --git a/drivers/clk/hisilicon/clk-hix5hd2.c 
b/drivers/clk/hisilicon/clk-hix5hd2.c
index 9584f0c32dda..659bd5f493b8 100644
--- a/drivers/clk/hisilicon/clk-hix5hd2.c
+++ b/drivers/clk/hisilicon/clk-hix5hd2.c
@@ -274,7 +274,7 @@ hix5hd2_clk_register_complex(struct hix5hd2_complex_clock 
*clks, int nums,
else
init.ops = _complex_ops;
 
-   init.flags = CLK_IS_BASIC;
+   init.flags = 0;
init.parent_names =
(clks[i].parent_name ? [i].parent_name : NULL);
init.num_parents = (clks[i].parent_name ? 1 : 0);
diff --git a/drivers/clk/hisilicon/clkgate-separated.c 
b/drivers/clk/hisilicon/clkgate-separated.c
index f36bdef91831..ae84884dc749 100644
--- a/drivers/clk/hisilicon/clkgate-separated.c
+++ b/drivers/clk/hisilicon/clkgate-separated.c
@@ -110,7 +110,7 @@ struct clk *hisi_register_clkgate_sep(struct device *dev, 
const char *name,
 
init.name = name;
init.ops = _separated_ops;
-   init.flags = flags | CLK_IS_BASIC;
+   init.flags = flags;
init.parent_names = (parent_name ? _name : NULL);
init.num_parents = (parent_name ? 1 : 0);
 
-- 
Sent by a computer through tubes



[PATCH 7/8] clk: samsung: s3c2410: Remove usage of CLK_IS_BASIC

2018-12-06 Thread Stephen Boyd
This flag doesn't look to be used by any code, just set in the clk init
structure and then never tested again. Remove it from this drivers as it
doesn't provide any benefit.

Cc: Kukjin Kim 
Cc: Krzysztof Kozlowski 
Cc: Sylwester Nawrocki 
Signed-off-by: Stephen Boyd 
---
 drivers/clk/samsung/clk-s3c2410-dclk.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/clk/samsung/clk-s3c2410-dclk.c 
b/drivers/clk/samsung/clk-s3c2410-dclk.c
index 0d92f3e5e3d9..82f8ae22fd34 100644
--- a/drivers/clk/samsung/clk-s3c2410-dclk.c
+++ b/drivers/clk/samsung/clk-s3c2410-dclk.c
@@ -105,7 +105,7 @@ static struct clk_hw *s3c24xx_register_clkout(struct device 
*dev,
 
init.name = name;
init.ops = _clkout_ops;
-   init.flags = CLK_IS_BASIC;
+   init.flags = 0;
init.parent_names = parent_names;
init.num_parents = num_parents;
 
-- 
Sent by a computer through tubes



[PATCH 8/8] clk: Loongson1: Remove usage of CLK_IS_BASIC

2018-12-06 Thread Stephen Boyd
This flag doesn't look to be used by any code, just set in the clk init
structure and then never tested again. Remove it from this driver as it
doesn't provide any benefit. Also remove parenthesis nearby that are not
needed and include clk.h to fix a sparse warning about static function
definition.

Signed-off-by: Stephen Boyd 
---
 drivers/clk/loongson1/clk.c | 8 +---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/clk/loongson1/clk.c b/drivers/clk/loongson1/clk.c
index cfcfd143fccb..983ce9f6edbb 100644
--- a/drivers/clk/loongson1/clk.c
+++ b/drivers/clk/loongson1/clk.c
@@ -10,6 +10,8 @@
 #include 
 #include 
 
+#include "clk.h"
+
 struct clk_hw *__init clk_hw_register_pll(struct device *dev,
  const char *name,
  const char *parent_name,
@@ -27,9 +29,9 @@ struct clk_hw *__init clk_hw_register_pll(struct device *dev,
 
init.name = name;
init.ops = ops;
-   init.flags = flags | CLK_IS_BASIC;
-   init.parent_names = (parent_name ? _name : NULL);
-   init.num_parents = (parent_name ? 1 : 0);
+   init.flags = flags;
+   init.parent_names = parent_name ? _name : NULL;
+   init.num_parents = parent_name ? 1 : 0;
hw->init = 
 
/* register the clock */
-- 
Sent by a computer through tubes



[PATCH 6/8] clk: versatile: sp810: Remove usage of CLK_IS_BASIC

2018-12-06 Thread Stephen Boyd
This flag doesn't look to be used by any code, just set in the clk init
structure and then never tested again. Remove it from this driver as it
doesn't provide any benefit.

Cc: Linus Walleij 
Signed-off-by: Stephen Boyd 
---
 drivers/clk/versatile/clk-sp810.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/clk/versatile/clk-sp810.c 
b/drivers/clk/versatile/clk-sp810.c
index 1fe1e8d970cf..c2b6bb814742 100644
--- a/drivers/clk/versatile/clk-sp810.c
+++ b/drivers/clk/versatile/clk-sp810.c
@@ -111,7 +111,7 @@ static void __init clk_sp810_of_setup(struct device_node 
*node)
 
init.name = name;
init.ops = _sp810_timerclken_ops;
-   init.flags = CLK_IS_BASIC;
+   init.flags = 0;
init.parent_names = parent_names;
init.num_parents = num;
 
-- 
Sent by a computer through tubes



Re: [PATCH] clk: qcom: smd: Add support for MSM8998 rpm clocks

2018-12-06 Thread Stephen Boyd
Quoting Jeffrey Hugo (2018-12-06 13:11:06)
> Add rpm smd clocks, PMIC and bus clocks which are required on MSM8998
> for clients to vote on.
> 
> Signed-off-by: Jeffrey Hugo 
> ---
>  .../devicetree/bindings/clock/qcom,rpmcc.txt   |  1 +
>  drivers/clk/qcom/clk-smd-rpm.c | 62 
> ++
>  include/dt-bindings/clock/qcom,rpmcc.h |  6 +++
>  3 files changed, 69 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/clock/qcom,rpmcc.txt 
> b/Documentation/devicetree/bindings/clock/qcom,rpmcc.txt
> index 87b4949..16c4293 100644
> --- a/Documentation/devicetree/bindings/clock/qcom,rpmcc.txt
> +++ b/Documentation/devicetree/bindings/clock/qcom,rpmcc.txt
> @@ -17,6 +17,7 @@ Required properties :
> "qcom,rpmcc-apq8064", "qcom,rpmcc"
> "qcom,rpmcc-msm8996", "qcom,rpmcc"
> "qcom,rpmcc-qcs404", "qcom,rpmcc"
> +   "qcom,rpmcc-msm8998", "qcom,rpmcc"

Can you keep this sorted on the first compatible?

>  
>  - #clock-cells : shall contain 1
>  

Rob may prefer this file is split from the driver part into a different
patch.

> diff --git a/drivers/clk/qcom/clk-smd-rpm.c b/drivers/clk/qcom/clk-smd-rpm.c
> index d3aadae..269304e 100644
> --- a/drivers/clk/qcom/clk-smd-rpm.c
> +++ b/drivers/clk/qcom/clk-smd-rpm.c
> @@ -655,11 +655,73 @@ static int clk_smd_rpm_enable_scaling(struct 
> qcom_smd_rpm *rpm)
> .num_clks = ARRAY_SIZE(qcs404_clks),
>  };
>  
> +/* msm8998 */
> +DEFINE_CLK_SMD_RPM(msm8998, snoc_clk, snoc_a_clk, QCOM_SMD_RPM_BUS_CLK, 1);
> +DEFINE_CLK_SMD_RPM(msm8998, cnoc_clk, cnoc_a_clk, QCOM_SMD_RPM_BUS_CLK, 2);
> +DEFINE_CLK_SMD_RPM(msm8998, ce1_clk, ce1_a_clk, QCOM_SMD_RPM_CE_CLK, 0);
> +DEFINE_CLK_SMD_RPM_XO_BUFFER(msm8998, div_clk1, div_clk1_a, 0xb);
> +DEFINE_CLK_SMD_RPM(msm8998, ipa_clk, ipa_a_clk, QCOM_SMD_RPM_IPA_CLK, 0);
> +DEFINE_CLK_SMD_RPM_XO_BUFFER(msm8998, bb_clk1, bb_clk1_a, 1);
> +DEFINE_CLK_SMD_RPM_XO_BUFFER(msm8998, bb_clk2, bb_clk2_a, 2);
> +DEFINE_CLK_SMD_RPM_XO_BUFFER_PINCTRL(msm8998, bb_clk3_pin, bb_clk3_a_pin, 3);
> +DEFINE_CLK_SMD_RPM(msm8998, mmssnoc_axi_rpm_clk, mmssnoc_axi_rpm_a_clk,
> +  QCOM_SMD_RPM_MMAXI_CLK, 0);
> +DEFINE_CLK_SMD_RPM(msm8998, aggre1_noc_clk, aggre1_noc_a_clk,
> +  QCOM_SMD_RPM_AGGR_CLK, 1);
> +DEFINE_CLK_SMD_RPM(msm8998, aggre2_noc_clk, aggre2_noc_a_clk,
> +  QCOM_SMD_RPM_AGGR_CLK, 2);
> +DEFINE_CLK_SMD_RPM_QDSS(msm8998, qdss_clk, qdss_a_clk,
> +   QCOM_SMD_RPM_MISC_CLK, 1);
> +DEFINE_CLK_SMD_RPM_XO_BUFFER(msm8998, rf_clk1, rf_clk1_a, 4);
> +DEFINE_CLK_SMD_RPM_XO_BUFFER_PINCTRL(msm8998, rf_clk2_pin, rf_clk2_a_pin, 5);
> +DEFINE_CLK_SMD_RPM_XO_BUFFER(msm8998, rf_clk3, rf_clk3_a, 6);
> +DEFINE_CLK_SMD_RPM_XO_BUFFER_PINCTRL(msm8998, rf_clk3_pin, rf_clk3_a_pin, 6);
> +static struct clk_smd_rpm *msm8998_clks[] = {
> +   [RPM_SMD_SNOC_CLK] = _snoc_clk,
> +   [RPM_SMD_SNOC_A_CLK] = _snoc_a_clk,
> +   [RPM_SMD_CNOC_CLK] = _cnoc_clk,
> +   [RPM_SMD_CNOC_A_CLK] = _cnoc_a_clk,
> +   [RPM_SMD_CE1_CLK] = _ce1_clk,
> +   [RPM_SMD_CE1_A_CLK] = _ce1_a_clk,
> +   [RPM_SMD_DIV_CLK1] = _div_clk1,
> +   [RPM_SMD_DIV_A_CLK1] = _div_clk1_a,
> +   [RPM_SMD_IPA_CLK] = _ipa_clk,
> +   [RPM_SMD_IPA_A_CLK] = _ipa_a_clk,
> +   [RPM_SMD_BB_CLK1] = _bb_clk1,
> +   [RPM_SMD_BB_CLK1_A] = _bb_clk1_a,
> +   [RPM_SMD_BB_CLK2] = _bb_clk2,
> +   [RPM_SMD_BB_CLK2_A] = _bb_clk2_a,
> +   [RPM_SMD_BB_CLK3_PIN] = _bb_clk3_pin,
> +   [RPM_SMD_BB_CLK3_A_PIN] = _bb_clk3_a_pin,
> +   [RPM_SMD_MMAXI_CLK] = _mmssnoc_axi_rpm_clk,
> +   [RPM_SMD_MMAXI_A_CLK] = _mmssnoc_axi_rpm_a_clk,
> +   [RPM_SMD_AGGR1_NOC_CLK] = _aggre1_noc_clk,
> +   [RPM_SMD_AGGR1_NOC_A_CLK] = _aggre1_noc_a_clk,
> +   [RPM_SMD_AGGR2_NOC_CLK] = _aggre2_noc_clk,
> +   [RPM_SMD_AGGR2_NOC_A_CLK] = _aggre2_noc_a_clk,
> +   [RPM_SMD_QDSS_CLK] = _qdss_clk,
> +   [RPM_SMD_QDSS_A_CLK] = _qdss_a_clk,
> +   [RPM_SMD_RF_CLK1] = _rf_clk1,
> +   [RPM_SMD_RF_CLK1_A] = _rf_clk1_a,
> +   [RPM_SMD_RF_CLK2_PIN] = _rf_clk2_pin,
> +   [RPM_SMD_RF_CLK2_A_PIN] = _rf_clk2_a_pin,
> +   [RPM_SMD_RF_CLK3] = _rf_clk3,
> +   [RPM_SMD_RF_CLK3_A] = _rf_clk3_a,
> +   [RPM_SMD_RF_CLK3_PIN] = _rf_clk3_pin,
> +   [RPM_SMD_RF_CLK3_A_PIN] = _rf_clk3_a_pin,
> +};
> +
> +static const struct rpm_smd_clk_desc rpm_clk_msm8998 = {
> +   .clks = msm8998_clks,
> +   .num_clks = ARRAY_SIZE(msm8998_clks),
> +};
> +
>  static const struct of_device_id rpm_smd_clk_match_table[] = {
> { .compatible = "qcom,rpmcc-msm8916", .data = _clk_msm8916 },
> { .compatible = "qcom,rpmcc-msm8974", .data = _clk_msm8974 },
> { .compatible = "qcom,rpmcc-msm8996", .data = _clk_msm8996 },
> { .compatible = "qcom,rpmcc-qcs404",  .data = _clk_qcs404  },
> +   { .compatible = "qcom,rpmcc-msm8998", .data = _clk_msm8998 

Re: [PATCH v2] clk: npcm7xx: get fixed clocks from DT

2018-12-06 Thread Stephen Boyd
Quoting Tali Perry (2018-12-06 00:44:31)
> diff --git a/drivers/clk/clk-npcm7xx.c b/drivers/clk/clk-npcm7xx.c
> index 27a86b7a34db..4bd2e40997d4 100644
> --- a/drivers/clk/clk-npcm7xx.c
> +++ b/drivers/clk/clk-npcm7xx.c
> @@ -8,13 +8,19 @@
>   */
>  
>  #include 
> +#include 
>  #include 
> +#include 
>  #include 
>  #include 
>  #include 
> +#include 
> +#include 
>  #include 
> +#include 
>  #include 
>  #include 
> +#include 

Why?

>  #include 
>  
>  #include 
> @@ -568,6 +575,31 @@ static void __init npcm7xx_clk_init(struct device_node 
> *clk_np)
> for (i = 0; i < NPCM7XX_NUM_CLOCKS; i++)
> npcm7xx_clk_data->hws[i] = ERR_PTR(-EPROBE_DEFER);
>  
> +   /* Read fixed clocks. These 3 clocks must be defined in DT */
> +   clk = of_clk_get_by_name(clk_np, NPCM7XX_CLK_S_REFCLK);
> +   if (IS_ERR(clk)) {
> +   pr_err("failed to find external REFCLK on device tree, 
> err=%ld\n",
> +   PTR_ERR(clk));
> +   clk_put(clk);
> +   goto npcm7xx_init_fail_no_clk_on_dt;
> +   }
> +
> +   clk = of_clk_get_by_name(clk_np, NPCM7XX_CLK_S_SYSBYPCK);
> +   if (IS_ERR(clk)) {
> +   pr_err("failed to find external SYSBYPCK on device tree, 
> err=%ld\n",
> +   PTR_ERR(clk));
> +   clk_put(clk);
> +   goto npcm7xx_init_fail_no_clk_on_dt;
> +   }
> +
> +   clk = of_clk_get_by_name(clk_np, NPCM7XX_CLK_S_MCBYPCK);
> +   if (IS_ERR(clk)) {
> +   pr_err("failed to find external MCBYPCK on device tree, 
> err=%ld\n",
> +   PTR_ERR(clk));
> +   clk_put(clk);
> +   goto npcm7xx_init_fail_no_clk_on_dt;
> +   }

Now this looks like a DT validator in the kernel. DT folks are working
on a schema and validator, which should be able to make sure that the
DTS file has the proper set of clks specified for this drivers' node so
that we don't need to check in the kernel.

So it looks like nothing needs to change here?



Re: [RFC PATCH] clk: qcom: clk-rpmh: Add IPA clock support

2018-12-06 Thread Stephen Boyd
Quoting David Dai (2018-12-05 17:24:18)
> 
> 
> On 12/4/2018 11:15 PM, Stephen Boyd wrote:
> > Quoting David Dai (2018-12-04 17:14:10)
> >> On 12/4/2018 2:34 PM, Stephen Boyd wrote:
> >>> Quoting Alex Elder (2018-12-04 13:41:47)
> >>>>
> >>> But then we translate that clock rate into a bandwidth request to the
> >>> BCM hardware? Seems really weird because it's doing the opposite of what
> >>> you say is abusive. What does the IPA driver plan to do with this clk?
> >>> Calculate a frequency by knowing that it really boils down to some
> >>> bandwidth that then gets converted back into some clock frequency? Do we
> >>> have the user somewhere that can be pointed to?
> >> The clock rate is translated into a unitless threshold value sent as
> >> part of the rpmh msg
> >> that BCM takes to select a performance. In this case, the unit
> >> conversion is based on
> >> the unit value read from the aux data which is in Khz. I understand that
> >> this wasn't
> >> explicitly mentioned anywhere and I'll improve on that next patch.
> > How is this different from bus bandwidth requests? In those cases the
> > bandwidth is calculated in bits per second or something like that, and
> > written to the hardware so it can convert that bandwidth into kHz and
> > set a bus clk frequency in the clock controller? So in the IPA case
> > we've skipped the bps to kHz conversion step and gone straight to the
> > clk frequency setting part? Is a BCM able to aggregate units of
> > bandwidth or kHz depending on how it's configured and this BCM is
> > configured for kHz?
> 
> The data written to the hardware is just a 14bit scalar value that it 
> takes to select a performance/frequency from a preset table. It's not 
> really doing any sort of conversion in hardware in this case, instead 
> the value is computed by software based on the aux data given. Think of 
> it as a generic aggregator as opposed to being strictly bandwidth and 
> the aggregator itself does not care what type of value it is(be it Khz 
> or BW/s).
> 

Got it. Thanks!



Re: [PATCH v2 3/3] clk: qcom: gcc-msm8998: Add clkref clocks

2018-12-05 Thread Stephen Boyd
Quoting Bjorn Andersson (2018-12-03 10:33:30)
> Add clkref clocks for usb3, hdmi, ufs, pcie, and usb2. They are all
> sourced off CXO_IN, so parent them off "xo" until a proper link to the
> rpmcc can be described in DT.
> 
> Signed-off-by: Bjorn Andersson 
> ---

Applied to clk-next



Re: [PATCH v2 2/3] clk: qcom: gcc-msm8998: Disable halt check of UFS clocks

2018-12-05 Thread Stephen Boyd
Quoting Bjorn Andersson (2018-12-03 10:33:29)
> Drop the halt check of the UFS symbol clocks, in accordance with other
> platforms. This makes clk_disable_unused() happy and makes it possible
> to turn the clocks on again without an error.
> 
> Signed-off-by: Bjorn Andersson 
> ---

Applied to clk-next



Re: [PATCH v2 1/3] clk: qcom: gcc-msm8998: Drop hmss_dvm and lpass_at

2018-12-05 Thread Stephen Boyd
Quoting Bjorn Andersson (2018-12-03 10:33:28)
> Disabling gcc_hmss_dvm_bus_clk and gcc_lpass_at_clk causes the board to
> lock up, and by that preventing the kernel to boot without
> clk_ignore_unused.
> 
> gcc_hmss_dvm_bus_clk is marked always-on downstream, but not referenced,
> and gcc_lpass_at_clk isn't mentioned. So let's remove them until they
> are needed by some client.
> 
> Signed-off-by: Bjorn Andersson 
> ---

Applied to clk-next



Re: [PATCH] clk: qcom: Enumerate remaining msm8998 resets

2018-12-05 Thread Stephen Boyd
Quoting Jeffrey Hugo (2018-12-04 07:13:22)
> The current list of defined resets is incomplete compared to what the
> hardware implements.  Enumerate the remaining resets according to the
> hardware documentation.
> 
> Signed-off-by: Jeffrey Hugo 
> ---

Applied to clk-next



Re: [PATCH v5 4/8] soc: qcom: rpmpd: Add support for get/set performance state

2018-12-05 Thread Stephen Boyd
Quoting Rajendra Nayak (2018-12-05 02:11:22)
> 
> On 12/5/2018 12:33 PM, Rajendra Nayak wrote:
> >>
> >>> +   return 0;
> >>> +   }
> >>> +
> >>> +   of_node_put(np);
> >>
> >> This same code exists twice. Perhaps a helper needs to exist for
> >> qcom_rpm_get_performance() to pull the number out of the DT.
> > 
> > Sure I can make both drivers use a common helper instead of duplicating it.
> 
> which would mean I will need to create a new file just to define the
> common helper. Does that seem like an overkill?

Maybe put it in the genpd code and let it take a const char *name
argument that picks out the property that drivers want to look at? That
way other OPP properties can be picked out with a simple call to the
function but it's generic enough to be used in other places.



Re: [PATCH v1 1/1] clk: npcm: get fixed input clks from DT

2018-12-05 Thread Stephen Boyd
Quoting Tali Perry (2018-12-05 11:47:02)
> ---

No commit text? No Signed-off-by?

Sorry, I don't understand what this patch is for or what's going on.

>  drivers/clk/clk-npcm7xx.c | 64 
> +++
>  1 file changed, 53 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/clk/clk-npcm7xx.c b/drivers/clk/clk-npcm7xx.c
> index 27a86b7a34db..25f8ccaa3d2f 100644
> --- a/drivers/clk/clk-npcm7xx.c
> +++ b/drivers/clk/clk-npcm7xx.c
> @@ -8,17 +8,23 @@
>   */
>  
>  #include 
> +#include 
>  #include 
> +#include 
>  #include 
>  #include 
>  #include 
> +#include 
> +#include 
>  #include 
> +#include 
>  #include 
>  #include 
> +#include 
>  #include 
> -
>  #include 
>  
> +

I don't get it. Please don't send random whitespace changes in a patch.

>  struct npcm7xx_clk_pll {
> struct clk_hw   hw;
> void __iomem*pllcon;
> @@ -44,7 +50,8 @@ static unsigned long npcm7xx_clk_pll_recalc_rate(struct 
> clk_hw *hw,
> u64 ret;
>  
> if (parent_rate == 0) {
> -   pr_err("%s: parent rate is zero", __func__);
> +   pr_err("%s: parent rate is zero. reg=%s\n", __func__,
> +  hw->init->name);

Why do we care?

> return 0;
> }
>  
> @@ -544,12 +552,14 @@ static void __init npcm7xx_clk_init(struct device_node 
> *clk_np)
> void __iomem *clk_base;
> struct resource res;
> struct clk_hw *hw;
> +   struct clk *clk;
> int ret;
> int i;
>  
> +   clk_base = NULL;
> ret = of_address_to_resource(clk_np, 0, );
> if (ret) {
> -   pr_err("%pOFn: failed to get resource, ret %d\n", clk_np,
> +   pr_err("%s: failed to get resource, ret %d\n", clk_np->name,
> ret);
> return;
> }

Drop this change as it's making things worse.

> @@ -558,9 +568,10 @@ static void __init npcm7xx_clk_init(struct device_node 
> *clk_np)
> if (!clk_base)
> goto npcm7xx_init_error;
>  
> -   npcm7xx_clk_data = kzalloc(struct_size(npcm7xx_clk_data, hws,
> -  NPCM7XX_NUM_CLOCKS), GFP_KERNEL);
> -   if (!npcm7xx_clk_data)
> +   npcm7xx_clk_data = kzalloc(sizeof(*npcm7xx_clk_data->hws) *
> +   NPCM7XX_NUM_CLOCKS + sizeof(npcm7xx_clk_data), GFP_KERNEL);
> +
> +   if (!npcm7xx_clk_data->hws)
> goto npcm7xx_init_np_err;
>  
> npcm7xx_clk_data->num = NPCM7XX_NUM_CLOCKS;

Drop this hunk.



Re: [PATCH] clk: mediatek: fix the PCIe MAC clock parent

2018-12-05 Thread Stephen Boyd
Quoting Ryder Lee (2018-12-04 22:41:10)
> The PCIe function doesn't work as the clock tree of MAC layer is wrong.
> Hence fix the clock table.
> 
> Fixes: 3b5e748615e7 ("clk: mediatek: add clock support for MT7629 SoC")
> Signed-off-by: Ryder Lee 
> ---

Applied to clk-next



RE: [PATCH V6 0/9] clk: add imx7ulp clk support

2018-12-05 Thread Stephen Boyd
Quoting Aisheng DONG (2018-12-03 17:29:29)
> > -Original Message-
> > From: Stephen Boyd [mailto:sb...@kernel.org]
> > Sent: Tuesday, December 4, 2018 3:32 AM
> > To: Aisheng DONG ; linux-...@vger.kernel.org
> > Cc: linux-kernel@vger.kernel.org; linux-arm-ker...@lists.infradead.org;
> > mturque...@baylibre.com; shawn...@kernel.org; Anson Huang
> > ; Jacky Bai ; dl-linux-imx
> > ; Aisheng DONG 
> > Subject: Re: [PATCH V6 0/9] clk: add imx7ulp clk support
> > 
> > Quoting A.s. Dong (2018-11-14 05:01:31)
> > > This patch series intends to add imx7ulp clk support.
> > >
> > > i.MX7ULP Clock functions are under joint control of the System Clock
> > > Generation (SCG) modules, Peripheral Clock Control (PCC) modules, and
> > > Core Mode Controller (CMC)1 blocks
> > >
> > > The clocking scheme provides clear separation between M4 domain and A7
> > > domain. Except for a few clock sources shared between two domains,
> > > such as the System Oscillator clock, the Slow IRC (SIRC), and and the
> > > Fast IRC clock (FIRCLK), clock sources and clock management are
> > > separated and contained within each domain.
> > >
> > > M4 clock management consists of SCG0, PCC0, PCC1, and CMC0 modules.
> > > A7 clock management consists of SCG1, PCC2, PCC3, and CMC1 modules.
> > >
> > > Note: this series only adds A7 clock domain support as M4 clock domain
> > > will be handled by M4 seperately.
> > >
> > 
> > I got:
> > 
> > drivers/clk/imx/clk-pllv4.c:152:15: warning: symbol 'imx_clk_pllv4' was not
> > declared. Should it be static?
> > drivers/clk/imx/clk-pfdv2.c:166:15: warning: symbol 'imx_clk_pfdv2' was not
> > declared. Should it be static?
> > drivers/clk/imx/clk-divider-gate.c:174:15: warning: symbol
> > 'imx_clk_divider_gate' was not declared. Should it be static?
> > drivers/clk/imx/clk-composite-7ulp.c:22:15: warning: symbol
> > 'imx7ulp_clk_composite' was not declared. Should it be static?
> > 
> > which I can fix easily by throwing in clk.h into each file.
> 
> Thanks, I will double check it when I back to office.
> 

No worries, I fixed it up and merged to clk-next.



Re: [PATCH v6 07/10] clk: rk808: use managed version of of_provider registration

2018-12-05 Thread Stephen Boyd
Quoting Matti Vaittinen (2018-12-04 03:38:03)
> Simplify clean-up for rk808 by using managed version of of_provider
> registration.
> 
> Signed-off-by: Matti Vaittinen 
> ---

Applied to clk-next



Re: [PATCH v11 2/2] cpufreq: qcom-hw: Add support for QCOM cpufreq HW driver

2018-12-05 Thread Stephen Boyd
Quoting Viresh Kumar (2018-12-04 22:16:00)
> On 05-12-18, 09:07, Taniya Das wrote:
> > Hello Stephen, Viresh
> > 
> > Thanks for the code and suggestions.
> > 
> > Having a NR_DOMAINS '2' makes the driver not scalable for re-use.
> 
> Sure, I didn't like it either and that wasn't really what I was suggesting in
> the first place. I didn't wanted to write the code myself and pass it on, but 
> I
> have it now anyway :)
> 
> It may have a bug or two in there, but compiles just fine and is rebased over
> your patch Taniya.

If we move the ioremap of registers to the cpufreq_driver::init path
then we need to unmap it on cpufreq_driver::exit. In fact, all the
devm_*() code in the init path needs to change to non-devm. Otherwise
it's a nice simplification!

> +static int qcom_cpufreq_hw_read_lut(struct device *dev,
> +   struct cpufreq_policy *policy,
> +   void __iomem *base)
>  {
> u32 data, src, lval, i, core_count, prev_cc = 0, prev_freq = 0, freq;
> -   unsigned int max_cores = cpumask_weight(>related_cpus);
> +   unsigned int max_cores = cpumask_weight(policy->cpus);
> +   struct cpufreq_frequency_table  *table;
>  
> -   c->table = devm_kcalloc(dev, LUT_MAX_ENTRIES + 1,
> -   sizeof(*c->table), GFP_KERNEL);
> -   if (!c->table)
> +   table = devm_kcalloc(dev, LUT_MAX_ENTRIES + 1,
> +   sizeof(*table), GFP_KERNEL);

This one.

> +   if (!table)
> return -ENOMEM;
>  
> for (i = 0; i < LUT_MAX_ENTRIES; i++) {
> @@ -194,22 +154,29 @@ static void qcom_get_related_cpus(int index, struct 
> cpumask *m)
> }
>  }
>  
> -static int qcom_cpu_resources_init(struct platform_device *pdev,
> -  unsigned int cpu, int index,
> -  unsigned long xo_rate,
> -  unsigned long cpu_hw_rate)
> +static int qcom_cpufreq_hw_cpu_init(struct cpufreq_policy *policy)
>  {
> -   struct cpufreq_qcom *c;
> +   struct device *dev = _pdev->dev;
> +   struct of_phandle_args args;
> +   struct device_node *cpu_np;
> struct resource *res;
> -   struct device *dev = >dev;
> void __iomem *base;
> -   int ret, cpu_r;
> +   int ret, index;
>  
> -   c = devm_kzalloc(dev, sizeof(*c), GFP_KERNEL);
> -   if (!c)
> -   return -ENOMEM;
> +   cpu_np = of_cpu_device_node_get(policy->cpu);
> +   if (!cpu_np)
> +   return -EINVAL;
> +
> +   ret = of_parse_phandle_with_args(cpu_np, "qcom,freq-domain",
> +   "#freq-domain-cells", 0, );
> +   of_node_put(cpu_np);
>  
> -   res = platform_get_resource(pdev, IORESOURCE_MEM, index);
> +   if (ret)
> +   return ret;
> +
> +   index = args.args[0];
> +
> +   res = platform_get_resource(global_pdev, IORESOURCE_MEM, index);
> base = devm_ioremap_resource(dev, res);

And this one.

> if (IS_ERR(base))
> return PTR_ERR(base);

Here's a patch to squash in to fix those tiny problems. I left it as
devm_ioremap_resource() because that has all the nice features of
resource requesting inside and it didn't seem too bad to devm_iounmap()
on the exit path.

-8<--

 drivers/cpufreq/qcom-cpufreq-hw.c | 37 ++---
 1 file changed, 26 insertions(+), 11 deletions(-)

diff --git a/drivers/cpufreq/qcom-cpufreq-hw.c 
b/drivers/cpufreq/qcom-cpufreq-hw.c
index bcf9bb37298a..1e865e216839 100644
--- a/drivers/cpufreq/qcom-cpufreq-hw.c
+++ b/drivers/cpufreq/qcom-cpufreq-hw.c
@@ -10,6 +10,7 @@
 #include 
 #include 
 #include 
+#include 
 
 #define LUT_MAX_ENTRIES40U
 #define LUT_SRCGENMASK(31, 30)
@@ -77,8 +78,7 @@ static int qcom_cpufreq_hw_read_lut(struct device *dev,
unsigned int max_cores = cpumask_weight(policy->cpus);
struct cpufreq_frequency_table  *table;
 
-   table = devm_kcalloc(dev, LUT_MAX_ENTRIES + 1,
-   sizeof(*table), GFP_KERNEL);
+   table = kcalloc(LUT_MAX_ENTRIES + 1, sizeof(*table), GFP_KERNEL);
if (!table)
return -ENOMEM;
 
@@ -144,7 +144,7 @@ static void qcom_get_related_cpus(int index, struct cpumask 
*m)
 
ret = of_parse_phandle_with_args(cpu_np, "qcom,freq-domain",
 "#freq-domain-cells", 0,
- );
+);
of_node_put(cpu_np);
if (ret < 0)
continue;
@@ -170,7 +170,6 @@ static int qcom_cpufreq_hw_cpu_init(struct cpufreq_policy 
*policy)
ret = of_parse_phandle_with_args(cpu_np, "qcom,freq-domain",
"#freq-domain-cells", 0, );
of_node_put(cpu_np);
-

Re: [PATCH v3 5/5] clk: samsung: exynos5433: add imem clocks

2018-12-05 Thread Stephen Boyd
Quoting Sylwester Nawrocki (2018-12-05 02:57:32)
> On 12/4/18 19:40, Stephen Boyd wrote:
> > Quoting Kamil Konieczny (2018-12-04 08:52:48)
> >> +
> >> +static const unsigned long imem_clk_regs[] __initconst = {
> >> +   ENABLE_ACLK_IMEM,
> >> +   ENABLE_ACLK_IMEM_INT_MEM,
> >> +   ENABLE_ACLK_IMEM_SSS,
> >> +   ENABLE_ACLK_IMEM_SLIMSSS,
> >> +   ENABLE_ACLK_IMEM_RTIC,
> >> +   ENABLE_ACLK_IMEM_SMMU_SSS,
> >> +   ENABLE_ACLK_IMEM_SMMU_SLIMSSS,
> >> +   ENABLE_ACLK_IMEM_SMMU_RTIC,
> >> +   ENABLE_ACLK_IMEM_ARBG_TX,
> >> +   ENABLE_ACLK_IMEM_SMMU_ARBG_TX,
> >> +   ENABLE_PCLK_IMEM,
> >> +   ENABLE_PCLK_IMEM_SSS,
> >> +   ENABLE_PCLK_IMEM_SLIMSSS,
> >> +   ENABLE_PCLK_IMEM_RTIC,
> >> +   ENABLE_PCLK_IMEM_SMMU_SSS,
> >> +   ENABLE_PCLK_IMEM_SMMU_SLIMSSS,
> >> +   ENABLE_PCLK_IMEM_SMMU_RTIC,
> >> +   ENABLE_PCLK_IMEM_SMMU_ARGB_TX,
> >> +};
> >> +
> >> +static const struct samsung_gate_clock imem_gate_clks[] __initconst = {
> >> +   /* ENABLE_ACLK_IMEM */
> >> +   GATE(CLK_ACLK_AXI2AHB_IMEMH, "aclk_axi2ahb_imemh", "aclk_imem_200",
> >> +   ENABLE_ACLK_IMEM, 24, 0, 0),
> 
> I don't think that clock will ever need to be disabled/enabled, so I would
> drop this definition. The clock will remain in its default state after reset
> (enabled).
> 
> >> +   GATE(CLK_ACLK_AXIDS_SROMC, "aclk_axids_sromc", "aclk_imem_200",
> >> +   ENABLE_ACLK_IMEM, 23, CLK_IGNORE_UNUSED, 0),
> > 
> > Why is there so much use of CLK_IGNORE_UNUSED in this file?
> 
> I suppose CLK_IGNORE_UNUSED is needed because there is no drivers that
> would enable required clocks. For some clocks the flag could probably
> indeed just be omitted, e.g. SLIMSSS clocks. 
> 
> I'm inclined to just define clocks that we are confident about and which
> are needed now. i.e. the SSS IP block clocks. So in include/dt-bindings/
> clock/exynos5433.h we would have something like:

Agreed, it doesn't make much sense to add clk support for clks that
you'll never need to modify one way or the other.

>  
> +/* CMU_IMEM */
> +#define CLK_ACLK_SSS   1
> +#define CLK_PCLK_SSS   40
> 
> +#define IMEM_NR_CLK41
> 
> The other clocks could be added later as needed by someone who has 
> detailed knowledge about respective peripheral blocks.
> 

The slow addition of new clks to the binding header file makes for an
integration problem, so can we try to expose any clks that we know about
now as defines and make them not work if the driver isn't implementing
support for those clks? That way the binding is not changing but the
implementation can decide to support or not support certain clks.



Re: [PATCH] clk: qcom: Remove LPASS_CC config for GCC lpass clocks

2018-12-05 Thread Stephen Boyd
Quoting Taniya Das (2018-12-05 00:02:00)
> The GCC lpass clocks are updated as protected, so clean up the ifdefers.
> 
> Signed-off-by: Taniya Das 
> ---

Ok. But this will have to wait for a few months until everything is
merged together. Was that the intention of sending this now instead of
later?


Re: [PATCH] clk: socfpga: Don't have get_parent for single parent ops

2018-12-05 Thread Stephen Boyd
Quoting Dinh Nguyen (2018-12-05 07:17:41)
> Hi Stephen,
> 
> On 12/5/18 1:17 AM, Stephen Boyd wrote:
> > (Adding Dinh's korg email)
> > 
> > I also wonder if this driver is even used anymore or maybe we can delete
> > it?
> > 
> 
> The armv7 SoCFPGA platforms are using this driver.
> 

Ok and those platforms are booting in kernelci.org as
https://kernelci.org/boot/socfpga_cyclone5_de0_sockit/ perhaps?



Re: [PATCH v11 2/2] cpufreq: qcom-hw: Add support for QCOM cpufreq HW driver

2018-12-05 Thread Stephen Boyd
Quoting Taniya Das (2018-12-04 19:37:00)
> Hello Stephen, Viresh
> 
> Thanks for the code and suggestions.
> 
> Having a NR_DOMAINS '2' makes the driver not scalable for re-use.
> This assumption is only true for the current version of the HW and do 
> not intend to update/clean-up this logic again. So want to stick keeping 
> current logic of having the *qcom_freq_domain_map[NR_CPUS].

Ok, so let's just count the number of reg properties there are in the
node and use that to know how many frequency domains exist. That already
maps exactly to how many frequency domains this driver needs to worry
about. Alternatively, we could count the number of named register
regions like 'freq_domain0', 'freq_domain1', etc. are listed in
reg-names so that other register regions in this device could be exposed
in DT when/if they are added. Here's the reg counting patch.

diff --git a/drivers/cpufreq/qcom-cpufreq-hw.c 
b/drivers/cpufreq/qcom-cpufreq-hw.c
index 8dc6b73c2f22..c0f6a07eb3c5 100644
--- a/drivers/cpufreq/qcom-cpufreq-hw.c
+++ b/drivers/cpufreq/qcom-cpufreq-hw.c
@@ -29,8 +29,6 @@ struct cpufreq_qcom {
cpumask_t related_cpus;
 };
 
-static struct cpufreq_qcom *qcom_freq_domain_map[NR_CPUS];
-
 static int qcom_cpufreq_hw_target_index(struct cpufreq_policy *policy,
unsigned int index)
 {
@@ -76,9 +74,26 @@ static unsigned int qcom_cpufreq_hw_fast_switch(struct 
cpufreq_policy *policy,
 
 static int qcom_cpufreq_hw_cpu_init(struct cpufreq_policy *policy)
 {
-   struct cpufreq_qcom *c;
+   struct cpufreq_qcom *freq_domains, *c;
+   struct device_node *cpu_np;
+   struct of_phandle_args args;
+   int ret;
+
+   freq_domains = cpufreq_get_driver_data();
+   if (!freq_domains)
+   return -ENODEV;
+
+   cpu_np = of_cpu_device_node_get(policy->cpu);
+   if (!cpu_np)
+   return -ENODEV;
+
+   ret = of_parse_phandle_with_args(cpu_np, "qcom,freq-domain",
+"#freq-domain-cells", 0, );
+   of_node_put(cpu_np);
+   if (ret)
+   return ret;
 
-   c = qcom_freq_domain_map[policy->cpu];
+   c = _domains[args.args[0]];
if (!c) {
pr_err("No scaling support for CPU%d\n", policy->cpu);
return -ENODEV;
@@ -171,43 +186,15 @@ static int qcom_cpufreq_hw_read_lut(struct device *dev, 
struct cpufreq_qcom *c,
return 0;
 }
 
-static void qcom_get_related_cpus(int index, struct cpumask *m)
-{
-   struct device_node *cpu_np;
-   struct of_phandle_args args;
-   int cpu, ret;
-
-   for_each_possible_cpu(cpu) {
-   cpu_np = of_cpu_device_node_get(cpu);
-   if (!cpu_np)
-   continue;
-
-   ret = of_parse_phandle_with_args(cpu_np, "qcom,freq-domain",
-"#freq-domain-cells", 0,
- );
-   of_node_put(cpu_np);
-   if (ret < 0)
-   continue;
-
-   if (index == args.args[0])
-   cpumask_set_cpu(cpu, m);
-   }
-}
-
 static int qcom_cpu_resources_init(struct platform_device *pdev,
-  unsigned int cpu, int index,
-  unsigned long xo_rate,
-  unsigned long cpu_hw_rate)
+  int index, unsigned long xo_rate,
+  unsigned long cpu_hw_rate,
+  struct cpufreq_qcom *c)
 {
-   struct cpufreq_qcom *c;
struct resource *res;
struct device *dev = >dev;
void __iomem *base;
-   int ret, cpu_r;
-
-   c = devm_kzalloc(dev, sizeof(*c), GFP_KERNEL);
-   if (!c)
-   return -ENOMEM;
+   int ret;
 
res = platform_get_resource(pdev, IORESOURCE_MEM, index);
base = devm_ioremap_resource(dev, res);
@@ -220,12 +207,6 @@ static int qcom_cpu_resources_init(struct platform_device 
*pdev,
return -ENODEV;
}
 
-   qcom_get_related_cpus(index, >related_cpus);
-   if (!cpumask_weight(>related_cpus)) {
-   dev_err(dev, "Domain-%d failed to get related CPUs\n", index);
-   return -ENOENT;
-   }
-
c->perf_state_reg = base + REG_PERF_STATE;
 
ret = qcom_cpufreq_hw_read_lut(dev, c, base, xo_rate, cpu_hw_rate);
@@ -234,9 +215,6 @@ static int qcom_cpu_resources_init(struct platform_device 
*pdev,
return ret;
}
 
-   for_each_cpu(cpu_r, >related_cpus)
-   qcom_freq_domain_map[cpu_r] = c;
-
return 0;
 }
 
@@ -245,9 +223,20 @@ static int qcom_cpufreq_hw_driver_probe(struct 
platform_device *pdev)
struct device_node *cpu_np;
struct of_phandle_args args;
struct clk *clk;
-   unsigned int cpu;
+   unsigned int cpu, domain, 

Re: [PATCH] Revert "clk: fix __clk_init_parent() for single parent clocks"

2018-12-04 Thread Stephen Boyd
Quoting Jerome Brunet (2018-12-04 11:51:17)
> On Tue, 2018-12-04 at 10:05 -0800, Stephen Boyd wrote:
> > Quoting Jerome Brunet (2018-12-04 08:32:57)
> > > This reverts commit 2430a94d1e719b7b4af2a65b781a4c036eb22e64.
> > > 
> > > From the original commit message:
> > >   It turned out a problem because there are some single-parent clocks
> > >   that implement .get_parent() callback and return non-zero index.
> > >   The SOCFPGA clock is the case; the commit broke the SOCFPGA boards.
> > > 
> > > It is wrong for a clock to return an index >= num_parents. CCF checks
> > > for this condition in clk_core_get_parent_by_index(). This function sets
> > > the parent to NULL if index is incoherent, which seems like the only
> > > sane choice.
> > > 
> > > commit 2430a94d1e71 ("clk: fix __clk_init_parent() for single parent
> > > clocks")
> > > appears to be a work around installed in the core framework for a problem
> > > that is platform specific, and should probably be fixed in the platform
> > > code.
> > 
> > Ouch. I see that I even pointed that out in 2016 but never got a reply
> > or a fix patch[1].
> > 
> > > Further more, it introduces a problem in a corner case of the mux clock.
> > > Take mux with multiple parents, but only one is known, the rest being
> > > undocumented. The register reset has one of unknown parent set.
> > > 
> > > Before commit 2430a94d1e71 ("clk: fix __clk_init_parent() for single
> > > parent clocks"):
> > >  * get_parent() is called, register is read and give an unknown index.
> > >-> the mux is orphaned.
> > >  * a call to set_rate() will reparent the mux to the only known parent.
> > > 
> > > With commit 2430a94d1e71 ("clk: fix __clk_init_parent() for single parent
> > > clocks"):
> > >  * the register is never read.
> > >  * parent is wrongly assumed to be the only known one.
> > >As a consequence, all the calculation deriving from the mux will be
> > >wrong
> > >  * Since we believe the only know parent to be set, set_parent() won't
> > >ever be called and the register won't be set with the correct value.
> > 
> > Isn't this the broken bad case all over again? Why register a clk as a
> > mux and then only say it has one parent?
> 
> I understand it is a bit odd but as I explained it is a corner case.
> 
> We are really trying to drive a mux here, applying a values will change the
> clock signal we get. Documentation being what it is, we only know one the
> parent. The other parent could anything or maybe not connected at all, who
> know. That is not the important part actually
> 
> If such mux was already set to the known entry by default, it would be OK to
> ignore it. But if it is not, then we CCF to realise that and change the
> setting accordingly.
> 
> This the case of the 'ao_cts_cec' clock in the following patch:
> https://lore.kernel.org/patchwork/patch/1021028/
> 
> by default the value in the register is 0, but the only one that makes sense
> for us is 1.

Ok. Thanks for the explanation. What do you do to fix this problem in
the 'ao_cts_cec' clk implementation? Sounds like you just rely on
clk_set_rate() to fix this all up for you when the consumer changes the
rate?

If we have something that is default parented to something we're not
telling the framework about for some reason then one solution would be
to have some init code reparent the clk in hardware before registering
it with the framework. Basically "fix up" the clk tree in hardware to be
consistent with what software can reason about. 

This also reminds me of some audio clks I've seen before where the
default parent is some external pin and it can be reparented to an
internal clk with clk_set_rate/parent. In that case, I think we forced
the parent over to the internal clk before registering so that it would
always get parented properly in the framework. Right now, this is going
to cause problems for plans to probe defer clk_get() on orphan clks.

Maybe this could work better if we allowed
'assigned-clock-parents/rates' to reparent clks regardless of orphan
status and/or had some flag that could be set on clks to indicate that
we're OK if clk_get() is called on it when it's an orphan because this
isn't a problem?

Or we can make the defer on orphan code only defer if the clk has a
single parent and it's an orphan and return the clk if there is at least
one parent of the clk that has been registered and isn't marked as an
orphan. That would need to be recursively applied, so I guess we would
update our orphan marking code to indicate that c

Re: [PATCH] clk: socfpga: Don't have get_parent for single parent ops

2018-12-04 Thread Stephen Boyd
(Adding Dinh's korg email)

I also wonder if this driver is even used anymore or maybe we can delete
it?

Quoting Stephen Boyd (2018-12-04 11:34:16)
> This driver creates a gate clk with the possibility to have multiple
> parents. That can cause problems if the common clk framework tries to
> call the get_parent() op and gets back a number that's larger than the
> number of parents the clk says it supports in
> clk_init_data::num_parents. Let's duplicate the clk_ops structure each
> time this function is called and drop the get/set parent ops when there
> is only one parent. This allows the framework to consider a number
> larger than clk_init_data::num_parents as an error condition of the
> get_parent() clk op, clearing the way for proper code.
> 
> Cc: Jerome Brunet 
> Cc: Masahiro Yamada 
> Cc: Dinh Nguyen 
> Signed-off-by: Stephen Boyd 
> ---
>  drivers/clk/socfpga/clk-gate.c | 22 +-
>  1 file changed, 13 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/clk/socfpga/clk-gate.c b/drivers/clk/socfpga/clk-gate.c
> index aa7a6e6a15b6..73e03328d5c5 100644
> --- a/drivers/clk/socfpga/clk-gate.c
> +++ b/drivers/clk/socfpga/clk-gate.c
> @@ -176,8 +176,7 @@ static struct clk_ops gateclk_ops = {
> .set_parent = socfpga_clk_set_parent,
>  };
>  
> -static void __init __socfpga_gate_init(struct device_node *node,
> -   const struct clk_ops *ops)
> +void __init socfpga_gate_init(struct device_node *node)
>  {
> u32 clk_gate[2];
> u32 div_reg[3];
> @@ -188,12 +187,17 @@ static void __init __socfpga_gate_init(struct 
> device_node *node,
> const char *clk_name = node->name;
> const char *parent_name[SOCFPGA_MAX_PARENTS];
> struct clk_init_data init;
> +   struct clk_ops *ops;
> int rc;
>  
> socfpga_clk = kzalloc(sizeof(*socfpga_clk), GFP_KERNEL);
> if (WARN_ON(!socfpga_clk))
> return;
>  
> +   ops = kmemdup(_ops, sizeof(gateclk_ops), GFP_KERNEL);
> +   if (WARN_ON(!ops))
> +   return;
> +
> rc = of_property_read_u32_array(node, "clk-gate", clk_gate, 2);
> if (rc)
> clk_gate[0] = 0;
> @@ -202,8 +206,8 @@ static void __init __socfpga_gate_init(struct device_node 
> *node,
> socfpga_clk->hw.reg = clk_mgr_base_addr + clk_gate[0];
> socfpga_clk->hw.bit_idx = clk_gate[1];
>  
> -   gateclk_ops.enable = clk_gate_ops.enable;
> -   gateclk_ops.disable = clk_gate_ops.disable;
> +   ops->enable = clk_gate_ops.enable;
> +   ops->disable = clk_gate_ops.disable;
> }
>  
> rc = of_property_read_u32(node, "fixed-divider", _div);
> @@ -234,6 +238,11 @@ static void __init __socfpga_gate_init(struct 
> device_node *node,
> init.flags = 0;
>  
> init.num_parents = of_clk_parent_fill(node, parent_name, 
> SOCFPGA_MAX_PARENTS);
> +   if (init.num_parents < 2) {
> +   ops->get_parent = NULL;
> +   ops->set_parent = NULL;
> +   }
> +
> init.parent_names = parent_name;
> socfpga_clk->hw.hw.init = 
>  
> @@ -246,8 +255,3 @@ static void __init __socfpga_gate_init(struct device_node 
> *node,
> if (WARN_ON(rc))
> return;
>  }
> -
> -void __init socfpga_gate_init(struct device_node *node)
> -{
> -   __socfpga_gate_init(node, _ops);
> -}


Re: [RFC PATCH] clk: qcom: clk-rpmh: Add IPA clock support

2018-12-04 Thread Stephen Boyd
Quoting David Dai (2018-12-04 17:14:10)
> 
> On 12/4/2018 2:34 PM, Stephen Boyd wrote:
> > Quoting Alex Elder (2018-12-04 13:41:47)
> >> On 12/4/18 1:24 PM, Stephen Boyd wrote:
> >>> Quoting David Dai (2018-12-03 19:50:13)
> >>>> Add IPA clock support by extending the current clk rpmh driver to support
> >>>> clocks that are managed by a different type of RPMh resource known as
> >>>> Bus Clock Manager(BCM).
> >>> Yes, but why? Does the IPA driver need to set clk rates and that somehow
> >>> doesn't work as a bandwidth request?
> >> The IPA core clock is a *clock*, not a bus.  Representing it as if
> >> it were a bus, abusing the interconnect interface--pretending a bandwidth
> >> request is really a clock rate request--is kind of kludgy.  I think Bjorn
> >> and David (and maybe Georgi? I don't know) decided a long time ago that
> >> exposing this as a clock is the right way to do it.  I agree with that.
> >>
> > But then we translate that clock rate into a bandwidth request to the
> > BCM hardware? Seems really weird because it's doing the opposite of what
> > you say is abusive. What does the IPA driver plan to do with this clk?
> > Calculate a frequency by knowing that it really boils down to some
> > bandwidth that then gets converted back into some clock frequency? Do we
> > have the user somewhere that can be pointed to?
> The clock rate is translated into a unitless threshold value sent as 
> part of the rpmh msg
> that BCM takes to select a performance. In this case, the unit 
> conversion is based on
> the unit value read from the aux data which is in Khz. I understand that 
> this wasn't
> explicitly mentioned anywhere and I'll improve on that next patch. 

How is this different from bus bandwidth requests? In those cases the
bandwidth is calculated in bits per second or something like that, and
written to the hardware so it can convert that bandwidth into kHz and
set a bus clk frequency in the clock controller? So in the IPA case
we've skipped the bps to kHz conversion step and gone straight to the
clk frequency setting part? Is a BCM able to aggregate units of
bandwidth or kHz depending on how it's configured and this BCM is
configured for kHz?

> Here's a link to
> the IPA driver implementation: https://lkml.org/lkml/2018/11/7/220

Thanks for the link. It looks like the IPA driver hard codes a rate of
75 MHz.



Re: [PATCH 07/14] clock: milbeaut: Add Milbeaut M10V clock control

2018-12-04 Thread Stephen Boyd
Quoting Masahiro Yamada (2018-12-04 20:26:06)
> On Wed, Dec 5, 2018 at 3:14 AM Stephen Boyd  wrote:
> >
> > Quoting Masahiro Yamada (2018-12-04 03:03:53)
> > > Hi Stephen,
> > >
> > >
> > > On Fri, Nov 30, 2018 at 5:31 PM Stephen Boyd  wrote:
> > > >
> > > > Quoting Sugaya Taichi (2018-11-18 17:01:12)
> > > > > Add Milbeaut M10V clock ( including PLL ) control.
> > > >
> > > > Please give some more details here.
> > > >
> > > > >
> > > > > Signed-off-by: Sugaya Taichi 
> > > > > ---
> > > > >  drivers/clk/Makefile   |   1 +
> > > > >  drivers/clk/clk-m10v.c | 671 
> > > > > +
> > > >
> > > > And this is different from Uniphier? Maybe we need a socionext
> > > > directory under drivers/clk/.
> > >
> > >
> > >
> > > This is always a difficult question,
> > > and I do not have a strong opinion.
> > >
> > >
> > > I am fine with moving the files to drivers/clk/socionext
> > > although no file would be shared.
> > >
> > >
> > > FYI
> > >
> > > UniPhier and Milbeaut are completely different platforms
> > > developed/maintained by different teams.
> > >
> > > They happen to live in the same company now
> > > just because Socionext merged the LSI business from Panasonic and Fujitsu.
> > >
> > > UniPhier originates in Panasonic, while Milbeaut in Fujitsu.
> > >
> >
> > Thanks for the background info. I'd prefer to defer to however the dts
> > files are getting split up into directories. If they're all put under
> > arch/arm64/boot/dts/socionext/ then I would say combine the two clk
> > drivers into a socionext directory. Otherwise, keep them split out.
> 
> 
> If you want to align with the DT directory structure,
> the answer is clear.
> 
> 
> Milbeaut DT files will be put together with UniPhier ones
> into socionext directory.
> 
> 
> For arm64, DT directories are already sorted out by vendors.
> 
> Even 32-bit ARM is going to that way.
> 
> Rob Herring just posted a python script
> to move all DT files in arch/arm/boot/dts/
> into vendor subdirectories.
> 
> 
> Please let me know if you want me to
> move drivers/clk/uniphier/* to drivers/clk/socionext/*.
> 

Maybe the dts needs to be split up instead? Looks like the gpio drivers
are in a uniphier directory and there is some precedence to keep the
"taken over" company name when vendors are merged into other vendors.
Maybe that's how things have happened here? It would be nice to be
consistent, but I leave the decision up to you to figure out if that
really matters to you. I'll be fine either way.



Re: [PATCH V2 5/5] PM / Domains: Propagate performance state updates

2018-12-04 Thread Stephen Boyd
Quoting Ulf Hansson (2018-12-03 05:38:35)
> + Stephen, Mike, Graham
> 
> On Fri, 30 Nov 2018 at 12:06, Viresh Kumar  wrote:
> >
> > On 30-11-18, 11:18, Ulf Hansson wrote:
> There is one a big difference while comparing with clocks, which make
> this more difficult.
> 
> That is, in dev_pm_genpd_set_performance_state(), we are *not* calling
> ->the set_performance_state() callback of the genpd, unless the genpd
> is already powered on. Instead, for that case, we are only aggregating
> the performance states votes, to defer to invoke
> ->set_performance_state() until the genpd becomes powered on. In some
> way this makes sense, but for clock_set_rate(), the clock's rate can
> be changed, no matter if the clock has been prepared/enabled or not.
> 
> I recall we discussed this behavior of genpd, while introducing the
> performance states support to it. Reaching this point, introducing the
> master-domain propagation of performance states votes, we may need to
> re-consider the behavior, as there is evidently an overhead that grows
> along with the hierarchy.
> 
> As a matter of fact, what I think this boils to, is to consider if we
> want to temporary drop the performance state vote for a device from
> genpd's ->runtime_suspend() callback. Thus, also restore the vote from
> genpd's ->runtime_resume() callback. That's because, this is directly
> related to whether genpd should care about whether it's powered on or
> off, when calling the ->set_performance_state(). We have had
> discussions at LKML already around this topic. It seems like we need
> to pick them up to reach a consensus, before we can move forward with
> this.

What are we trying to gain consensus on exactly?

I've been thinking that we would want there to be a performance state
'target' for the 'devices are runtime suspended' state that we apply
when the genpd is powered down from runtime PM suspend of all devices in
the domain. This would then be overwritten with whatever the aggregated
performance state is when the genpd is powered back on due to some
device runtime resuming. Don't we already sort of have support for this
right now in genpd with commit fc5cbf0c94b6 ("PM / Domains: Support for
multiple states")? So I would think that we either let that state_idx
member be 0 or some implementation defined 'off' state index of the
genpd that can be achieved.

I was also thinking that the genpd_power_state was more than just a
bunch of idle states of a device so that we could combine
on/off/idle/active(performance states?) into one genpd_power_state
space that is affected by idle and on/off runtime PM status.

In the Qualcomm use-case, the state of the clk, either on or off, is
factored into the decision to consider the voltage constraint that the
clk has on the CX domain. So when the clk is disabled, the voltage
constraint on CX can be removed/ignored in the aggregation equation
because the hardware isn't clocking. This needs to work properly so that
devices can disable their clks and save power.

I hope that we can move clk on/off/rate control to be implemented with
clk domains based upon genpds so that we can generically reason about
genpds being on/off and at some performance state (i.e. a frequency)
instead of making something clk specific in the genpd code. If we can do
that, then this can be stated as a slave genpd (the clk domain) that's
linked to a master genpd (the voltage/corner domain) can only affect the
performance state requirement of the master genpd when the slave genpd
is on. When the slave genpd is off, the performance state requirement is
dropped and the master domain settles to a lower requirement based on
the other domains linked to it that are on. The clk domain would turn on
and off when the set of devices it is attached to are all suspended and
that would "do the right thing" by bubbling up the requirements to the
master domain this way.



Re: [PATCH v5 7/8] arm64: dts: sdm845: Add rpmh powercontroller node

2018-12-04 Thread Stephen Boyd
Quoting Rajendra Nayak (2018-12-03 21:21:18)
> diff --git a/arch/arm64/boot/dts/qcom/sdm845.dtsi 
> b/arch/arm64/boot/dts/qcom/sdm845.dtsi
> index b72bdb0a31a5..a6d0cd8d17b0 100644
> --- a/arch/arm64/boot/dts/qcom/sdm845.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi
> @@ -1324,6 +1325,56 @@
> compatible = "qcom,sdm845-rpmh-clk";
> #clock-cells = <1>;
> };
> +
> +   rpmhpd: power-controller {
> +   compatible = "qcom,sdm845-rpmhpd";
> +   #power-domain-cells = <1>;
> +   operating-points-v2 = <_opp_table>;
> +   };
> +
> +   rpmhpd_opp_table: opp-table {

This table should go somewhere else? I don't understand why it's in the
rpmh node because it's not an rpmh device. Does it go to the root? Or
does it go under rpmhpd itself? I'm not sure.



Re: [PATCH v5 4/8] soc: qcom: rpmpd: Add support for get/set performance state

2018-12-04 Thread Stephen Boyd
Quoting Rajendra Nayak (2018-12-03 21:21:15)
> @@ -221,6 +224,47 @@ static int rpmpd_power_off(struct generic_pm_domain 
> *domain)
> return ret;
>  }
>  
> +static int rpmpd_set_performance(struct generic_pm_domain *domain,
> +unsigned int state)
> +{
> +   int ret = 0;
> +   struct rpmpd *pd = domain_to_rpmpd(domain);
> +
> +   mutex_lock(_lock);
> +
> +   if (state > MAX_RPMPD_STATE)
> +   goto out;

Does this need to be under the mutex lock? Doesn't look like it.

> +
> +   pd->corner = state;
> +
> +   if (!pd->enabled && (pd->key != KEY_FLOOR_CORNER))

Please drop useless parenthesis.

> +   goto out;
> +
> +   ret = rpmpd_aggregate_corner(pd);
> +
> +out:
> +   mutex_unlock(_lock);
> +
> +   return ret;
> +}
> +
> +static unsigned int rpmpd_get_performance(struct generic_pm_domain *genpd,
> + struct dev_pm_opp *opp)
> +{
> +   struct device_node *np;
> +   unsigned int corner = 0;
> +
> +   np = dev_pm_opp_get_of_node(opp);
> +   if (of_property_read_u32(np, "qcom,level", )) {
> +   pr_err("%s: missing 'qcom,level' property\n", __func__);

We leak np reference here, assuming dev_pm_opp_get_of_node() returns an
of_node_get() pointer to the caller.

> +   return 0;
> +   }
> +
> +   of_node_put(np);

This same code exists twice. Perhaps a helper needs to exist for
qcom_rpm_get_performance() to pull the number out of the DT.

> +
> +   return corner;
> +}


Re: [PATCH v5 3/8] soc: qcom: rpmpd: Add a Power domain driver to model corners

2018-12-04 Thread Stephen Boyd
Overall looks good to me, just some nitpicks around modules and
includes.

Quoting Rajendra Nayak (2018-12-03 21:21:14)
> The Power domains for corners just pass the performance state set by the
> consumers to the RPM (Remote Power manager) which then takes care
> of setting the appropriate voltage on the corresponding rails to
> meet the performance needs.
> 
> We add all Power domain data needed on msm8996 here. This driver can easily
> be extended by adding data for other qualcomm SoCs as well.
> 

Why is "Power" capitalized in power domain?

> diff --git a/drivers/soc/qcom/Makefile b/drivers/soc/qcom/Makefile
> index f25b54cd6cf8..f1b25fdcf2ad 100644
> --- a/drivers/soc/qcom/Makefile
> +++ b/drivers/soc/qcom/Makefile
> @@ -21,3 +21,4 @@ obj-$(CONFIG_QCOM_WCNSS_CTRL) += wcnss_ctrl.o
>  obj-$(CONFIG_QCOM_APR) += apr.o
>  obj-$(CONFIG_QCOM_LLCC) += llcc-slice.o
>  obj-$(CONFIG_QCOM_SDM845_LLCC) += llcc-sdm845.o
> +obj-$(CONFIG_QCOM_RPMPD) += rpmpd.o
> diff --git a/drivers/soc/qcom/rpmpd.c b/drivers/soc/qcom/rpmpd.c
> new file mode 100644
> index ..a0b9f122d793
> --- /dev/null
> +++ b/drivers/soc/qcom/rpmpd.c
> @@ -0,0 +1,294 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* Copyright (c) 2017-2018, The Linux Foundation. All rights reserved. */
> +
> +#include 
> +#include 

And what is exported?

> +#include 
> +#include 
> +#include 

Is it a module? It's only bool so I don't think so.

> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#include 
> +#include 
> +
> +#define domain_to_rpmpd(domain) container_of(domain, struct rpmpd, pd)
> +
> +/* Resource types */
> +#define RPMPD_SMPA 0x61706d73
> +#define RPMPD_LDOA 0x616f646c
> +
> +/* Operation Keys */
> +#define KEY_CORNER 0x6e726f63 /* corn */
> +#define KEY_ENABLE 0x6e657773 /* swen */
> +#define KEY_FLOOR_CORNER   0x636676   /* vfc */
> +
> +#define DEFINE_RPMPD_CORN_SMPA(_platform, _name, _active, r_id)  
>   \
> +   static struct rpmpd _platform##_##_active;  \
> +   static struct rpmpd _platform##_##_name = { \
> +   .pd = { .name = #_name, },  \
> +   .peer = &_platform##_##_active, \
> +   .res_type = RPMPD_SMPA, \
> +   .res_id = r_id, \
> +   .key = KEY_CORNER,  \
> +   };  \
> +   static struct rpmpd _platform##_##_active = {   \
> +   .pd = { .name = #_active, },\
> +   .peer = &_platform##_##_name,   \
> +   .active_only = true,\
> +   .res_type = RPMPD_SMPA, \
> +   .res_id = r_id, \
> +   .key = KEY_CORNER,  \
> +   }
> +
> +#define DEFINE_RPMPD_CORN_LDOA(_platform, _name, r_id) \
> +   static struct rpmpd _platform##_##_name = { \
> +   .pd = { .name = #_name, },  \
> +   .res_type = RPMPD_LDOA, \
> +   .res_id = r_id, \
> +   .key = KEY_CORNER,  \
> +   }
> +
> +#define DEFINE_RPMPD_VFC(_platform, _name, r_id, r_type)   \
> +   static struct rpmpd _platform##_##_name = { \
> +   .pd = { .name = #_name, },  \
> +   .res_type = r_type, \
> +   .res_id = r_id, \
> +   .key = KEY_FLOOR_CORNER,\
> +   }
> +
> +#define DEFINE_RPMPD_VFC_SMPA(_platform, _name, r_id)  \
> +   DEFINE_RPMPD_VFC(_platform, _name, r_id, RPMPD_SMPA)
> +
> +#define DEFINE_RPMPD_VFC_LDOA(_platform, _name, r_id)  \
> +   DEFINE_RPMPD_VFC(_platform, _name, r_id, RPMPD_LDOA)
> +
> +struct rpmpd_req {
> +   __le32 key;
> +   __le32 nbytes;
> +   __le32 value;
> +};
> +
> +struct rpmpd {
> +   struct generic_pm_domain pd;
> +   struct rpmpd *peer;
> +   const bool active_only;
> +   unsigned int corner;
> +   bool enabled;
> +   const char *res_name;
> +   const int res_type;
> +   const int res_id;
> +   struct qcom_smd_rpm *rpm;
> +   __le32 key;
> +};
> +
> +struct rpmpd_desc {
> +   struct rpmpd **rpmpds;
> +   size_t num_pds;
> +};
> +
> +static DEFINE_MUTEX(rpmpd_lock);
> +
> +/* msm8996 RPM Power domains */
> 

Re: [PATCH v5 1/8] dt-bindings: opp: Introduce qcom-opp bindings

2018-12-04 Thread Stephen Boyd
Quoting Rajendra Nayak (2018-12-03 21:21:12)
> On Qualcomm Technologies, Inc. platforms, an OPP node needs
> to describe an additional level/corner value that is then communicated
> to a remote microprocessor by the CPU, which then takes some
> actions (like adjusting voltage values across various rails)
> based on the value passed.
> 
> Describe these bindings in the qcom-opp bindings document.
> 
> Signed-off-by: Rajendra Nayak 
> Acked-by: Viresh Kumar 
> ---

Reviewed-by: Stephen Boyd 



Re: [PATCH v11 2/2] cpufreq: qcom-hw: Add support for QCOM cpufreq HW driver

2018-12-04 Thread Stephen Boyd
Quoting Stephen Boyd (2018-12-04 14:28:11)
> Quoting Viresh Kumar (2018-12-03 21:12:31)
> > Hi Taniya,
> > 
> > Sorry that I haven't been reviewing it much from last few iterations as I 
> > was
> > letting others get this into a better shape. Thanks for your efforts..
> > 
> > On 02-12-18, 09:25, Taniya Das wrote:
> > > +++ b/drivers/cpufreq/qcom-cpufreq-hw.c
> > 
> > > +struct cpufreq_qcom {
> > > + struct cpufreq_frequency_table *table;
> > > + void __iomem *perf_state_reg;
> > > + cpumask_t related_cpus;
> > > +};
> > > +
> > > +static struct cpufreq_qcom *qcom_freq_domain_map[NR_CPUS];
> > 
> > Now that the code is much more simplified, I am not sure if you need this
> > per-cpu structure at all. The only place where you are using it is in
> > qcom_cpufreq_hw_cpu_init() and probe(). Why not merge 
> > qcom_cpu_resources_init()
> > completely into qcom_cpufreq_hw_cpu_init() and get rid of this structure
> > entirely ?
> > 
> 
> Good point. Here's an untested patch to handle that. It removes the
> related functionality and makes an array of pointers to the domains that
> are created each time qcom_cpu_resources_init() is called.
> 
> ---8<
> 
> diff --git a/drivers/cpufreq/qcom-cpufreq-hw.c 
> b/drivers/cpufreq/qcom-cpufreq-hw.c
> index 8dc6b73c2f22..04e7cfd70316 100644
> --- a/drivers/cpufreq/qcom-cpufreq-hw.c
> +++ b/drivers/cpufreq/qcom-cpufreq-hw.c
> @@ -23,14 +23,14 @@
>  #define REG_LUT_TABLE  0x110
>  #define REG_PERF_STATE 0x920
>  
> +#define MAX_FREQ_DOMAINS   2
> +
>  struct cpufreq_qcom {
> struct cpufreq_frequency_table *table;
> void __iomem *perf_state_reg;
> cpumask_t related_cpus;
>  };
>  
> -static struct cpufreq_qcom *qcom_freq_domain_map[NR_CPUS];
> -
>  static int qcom_cpufreq_hw_target_index(struct cpufreq_policy *policy,
> unsigned int index)
>  {
> @@ -76,9 +76,14 @@ static unsigned int qcom_cpufreq_hw_fast_switch(struct 
> cpufreq_policy *policy,
>  
>  static int qcom_cpufreq_hw_cpu_init(struct cpufreq_policy *policy)
>  {
> +   struct cpufreq_qcom **freq_domain_map;
> struct cpufreq_qcom *c;
>  
> -   c = qcom_freq_domain_map[policy->cpu];
> +   freq_domain_map = cpufreq_get_driver_data();
> +   if (!freq_domain_map)
> +   return -ENODEV;
> +
> +   c = freq_domain_map[policy->cpu];

And this fails now because it indexes based on cpu number. We have to
parse the frequency domain out of the cpunode again to fix that.

Here's the updated (still untested) patch and I also just allocated the
freq domain array up front instead of in each domain init routine to
simplify some more.

diff --git a/drivers/cpufreq/qcom-cpufreq-hw.c 
b/drivers/cpufreq/qcom-cpufreq-hw.c
index 8dc6b73c2f22..bc0d734f7e3c 100644
--- a/drivers/cpufreq/qcom-cpufreq-hw.c
+++ b/drivers/cpufreq/qcom-cpufreq-hw.c
@@ -23,14 +23,14 @@
 #define REG_LUT_TABLE  0x110
 #define REG_PERF_STATE 0x920
 
+#define MAX_FREQ_DOMAINS   2
+
 struct cpufreq_qcom {
struct cpufreq_frequency_table *table;
void __iomem *perf_state_reg;
cpumask_t related_cpus;
 };
 
-static struct cpufreq_qcom *qcom_freq_domain_map[NR_CPUS];
-
 static int qcom_cpufreq_hw_target_index(struct cpufreq_policy *policy,
unsigned int index)
 {
@@ -76,9 +76,26 @@ static unsigned int qcom_cpufreq_hw_fast_switch(struct 
cpufreq_policy *policy,
 
 static int qcom_cpufreq_hw_cpu_init(struct cpufreq_policy *policy)
 {
-   struct cpufreq_qcom *c;
+   struct cpufreq_qcom *freq_domain_map, *c;
+   struct device_node *cpu_np;
+   struct of_phandle_args args;
+   int ret;
+
+   freq_domain_map = cpufreq_get_driver_data();
+   if (!freq_domain_map)
+   return -ENODEV;
+
+   cpu_np = of_cpu_device_node_get(policy->cpu);
+   if (!cpu_np)
+   return -ENODEV;
+
+   ret = of_parse_phandle_with_args(cpu_np, "qcom,freq-domain",
+"#freq-domain-cells", 0, );
+   of_node_put(cpu_np);
+   if (ret)
+   return ret;
 
-   c = qcom_freq_domain_map[policy->cpu];
+   c = _domain_map[args.args[0]];
if (!c) {
pr_err("No scaling support for CPU%d\n", policy->cpu);
return -ENODEV;
@@ -171,43 +188,15 @@ static int qcom_cpufreq_hw_read_lut(struct device *dev, 
struct cpufreq_qcom *c,
return 0;
 }
 
-static void qcom_get_related_cpus(int index, struct cpumask *m)
-{
-   struct device_node *cpu_np;
-

Re: [PATCH] clk: fix clk_mux_val_to_index() error value

2018-12-04 Thread Stephen Boyd
Quoting Jerome Brunet (2018-12-04 11:55:10)
> On Tue, 2018-12-04 at 10:43 -0800, Stephen Boyd wrote:
> > Quoting Jerome Brunet (2018-12-04 08:34:03)
> > > clk_mux_val_to_index() is meant to be used by .get_parent(), which
> > > returns a u8, so when the value provided does not map to any valid index,
> > > it is not a good idea to return a negative error value.
> > > 
> > > Instead, return num_parents which we know is an invalid index and let
> > > CCF deal with it.
> > > 
> > > Fixes: 77deb66d262f ("clk: mux: add helper function for index/value
> > > translation")
> > > Signed-off-by: Jerome Brunet 
> > > ---
> > 
> > Thanks!
> > 
> > > diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
> > > index 60c51871b04b..fc20886ef069 100644
> > > --- a/include/linux/clk-provider.h
> > > +++ b/include/linux/clk-provider.h
> > > @@ -550,8 +550,8 @@ struct clk_hw *clk_hw_register_mux_table(struct device
> > > *dev, const char *name,
> > > void __iomem *reg, u8 shift, u32 mask,
> > > u8 clk_mux_flags, u32 *table, spinlock_t *lock);
> > >  
> > > -int clk_mux_val_to_index(struct clk_hw *hw, u32 *table, unsigned int
> > > flags,
> > > -unsigned int val);
> > > +u8 clk_mux_val_to_index(struct clk_hw *hw, u32 *table, unsigned int
> > > flags,
> > 
> > I wonder if we should just make this unsigned int? Does it hurt at all
> > to have it be a wider type even though it doesn't match the CCF decision
> > to make this a u8 for the parent index number space?
> > 
> 
> I also wondered about this but since the target is get_parent(), I just
> aligned the two.
> 
> In the end, I don't really care, as you prefer. Just let me know if you would
> like a v2 with this change
> 

Ok I may just make it unsigned int when applying then.



Re: [RFC PATCH] clk: qcom: clk-rpmh: Add IPA clock support

2018-12-04 Thread Stephen Boyd
Quoting Alex Elder (2018-12-04 13:41:47)
> On 12/4/18 1:24 PM, Stephen Boyd wrote:
> > Quoting David Dai (2018-12-03 19:50:13)
> >> Add IPA clock support by extending the current clk rpmh driver to support
> >> clocks that are managed by a different type of RPMh resource known as
> >> Bus Clock Manager(BCM).
> > 
> > Yes, but why? Does the IPA driver need to set clk rates and that somehow
> > doesn't work as a bandwidth request?
> 
> The IPA core clock is a *clock*, not a bus.  Representing it as if
> it were a bus, abusing the interconnect interface--pretending a bandwidth
> request is really a clock rate request--is kind of kludgy.  I think Bjorn
> and David (and maybe Georgi? I don't know) decided a long time ago that
> exposing this as a clock is the right way to do it.  I agree with that.
> 

But then we translate that clock rate into a bandwidth request to the
BCM hardware? Seems really weird because it's doing the opposite of what
you say is abusive. What does the IPA driver plan to do with this clk?
Calculate a frequency by knowing that it really boils down to some
bandwidth that then gets converted back into some clock frequency? Do we
have the user somewhere that can be pointed to?

Of course, none of these details are in the commit text so it's really
hard for me as a bystander to figure this all out.  So again, please add
these sorts of details to the commit text so we can be "sold" on the
idea of the patch instead of stating what the patch does.



Re: [PATCH v11 2/2] cpufreq: qcom-hw: Add support for QCOM cpufreq HW driver

2018-12-04 Thread Stephen Boyd
Quoting Viresh Kumar (2018-12-03 21:12:31)
> Hi Taniya,
> 
> Sorry that I haven't been reviewing it much from last few iterations as I was
> letting others get this into a better shape. Thanks for your efforts..
> 
> On 02-12-18, 09:25, Taniya Das wrote:
> > +++ b/drivers/cpufreq/qcom-cpufreq-hw.c
> 
> > +struct cpufreq_qcom {
> > + struct cpufreq_frequency_table *table;
> > + void __iomem *perf_state_reg;
> > + cpumask_t related_cpus;
> > +};
> > +
> > +static struct cpufreq_qcom *qcom_freq_domain_map[NR_CPUS];
> 
> Now that the code is much more simplified, I am not sure if you need this
> per-cpu structure at all. The only place where you are using it is in
> qcom_cpufreq_hw_cpu_init() and probe(). Why not merge 
> qcom_cpu_resources_init()
> completely into qcom_cpufreq_hw_cpu_init() and get rid of this structure
> entirely ?
> 

Good point. Here's an untested patch to handle that. It removes the
related functionality and makes an array of pointers to the domains that
are created each time qcom_cpu_resources_init() is called.

---8<

diff --git a/drivers/cpufreq/qcom-cpufreq-hw.c 
b/drivers/cpufreq/qcom-cpufreq-hw.c
index 8dc6b73c2f22..04e7cfd70316 100644
--- a/drivers/cpufreq/qcom-cpufreq-hw.c
+++ b/drivers/cpufreq/qcom-cpufreq-hw.c
@@ -23,14 +23,14 @@
 #define REG_LUT_TABLE  0x110
 #define REG_PERF_STATE 0x920
 
+#define MAX_FREQ_DOMAINS   2
+
 struct cpufreq_qcom {
struct cpufreq_frequency_table *table;
void __iomem *perf_state_reg;
cpumask_t related_cpus;
 };
 
-static struct cpufreq_qcom *qcom_freq_domain_map[NR_CPUS];
-
 static int qcom_cpufreq_hw_target_index(struct cpufreq_policy *policy,
unsigned int index)
 {
@@ -76,9 +76,14 @@ static unsigned int qcom_cpufreq_hw_fast_switch(struct 
cpufreq_policy *policy,
 
 static int qcom_cpufreq_hw_cpu_init(struct cpufreq_policy *policy)
 {
+   struct cpufreq_qcom **freq_domain_map;
struct cpufreq_qcom *c;
 
-   c = qcom_freq_domain_map[policy->cpu];
+   freq_domain_map = cpufreq_get_driver_data();
+   if (!freq_domain_map)
+   return -ENODEV;
+
+   c = freq_domain_map[policy->cpu];
if (!c) {
pr_err("No scaling support for CPU%d\n", policy->cpu);
return -ENODEV;
@@ -171,39 +176,17 @@ static int qcom_cpufreq_hw_read_lut(struct device *dev, 
struct cpufreq_qcom *c,
return 0;
 }
 
-static void qcom_get_related_cpus(int index, struct cpumask *m)
-{
-   struct device_node *cpu_np;
-   struct of_phandle_args args;
-   int cpu, ret;
-
-   for_each_possible_cpu(cpu) {
-   cpu_np = of_cpu_device_node_get(cpu);
-   if (!cpu_np)
-   continue;
-
-   ret = of_parse_phandle_with_args(cpu_np, "qcom,freq-domain",
-"#freq-domain-cells", 0,
- );
-   of_node_put(cpu_np);
-   if (ret < 0)
-   continue;
-
-   if (index == args.args[0])
-   cpumask_set_cpu(cpu, m);
-   }
-}
-
 static int qcom_cpu_resources_init(struct platform_device *pdev,
   unsigned int cpu, int index,
   unsigned long xo_rate,
-  unsigned long cpu_hw_rate)
+  unsigned long cpu_hw_rate,
+  struct cpufreq_qcom **freq_domain_map)
 {
struct cpufreq_qcom *c;
struct resource *res;
struct device *dev = >dev;
void __iomem *base;
-   int ret, cpu_r;
+   int ret;
 
c = devm_kzalloc(dev, sizeof(*c), GFP_KERNEL);
if (!c)
@@ -220,12 +203,6 @@ static int qcom_cpu_resources_init(struct platform_device 
*pdev,
return -ENODEV;
}
 
-   qcom_get_related_cpus(index, >related_cpus);
-   if (!cpumask_weight(>related_cpus)) {
-   dev_err(dev, "Domain-%d failed to get related CPUs\n", index);
-   return -ENOENT;
-   }
-
c->perf_state_reg = base + REG_PERF_STATE;
 
ret = qcom_cpufreq_hw_read_lut(dev, c, base, xo_rate, cpu_hw_rate);
@@ -234,8 +211,7 @@ static int qcom_cpu_resources_init(struct platform_device 
*pdev,
return ret;
}
 
-   for_each_cpu(cpu_r, >related_cpus)
-   qcom_freq_domain_map[cpu_r] = c;
+   freq_domain_map[index] = c;
 
return 0;
 }
@@ -245,9 +221,16 @@ static int qcom_cpufreq_hw_driver_probe(struct 
platform_device *pdev)
struct device_node *cpu_np;
struct of_phandle_args args;
struct clk *clk;
-   unsigned int cpu;
+   unsigned int cpu, domain;
unsigned long xo_rate, cpu_hw_rate;
int ret;
+   struct cpufreq_qcom **freq_domain_map, *freq_domain;
+
+   

Re: [PATCH] clk: meson-gxbb: switch video clocks mux tables to static

2018-12-04 Thread Stephen Boyd
Quoting Jerome Brunet (2018-12-04 12:05:02)
> On Tue, 2018-12-04 at 20:33 +0100, Neil Armstrong wrote:
> > Le 04/12/2018 18:43, Stephen Boyd a écrit :
> > > Quoting Neil Armstrong (2018-12-04 04:58:39)
> > > > @@ -1983,7 +1983,7 @@ static struct clk_fixed_factor gxbb_vclk2_div12 =
> > > > {
> > > >  };
> > > >  
> > > >  static u32 mux_table_cts_sel[] = { 0, 1, 2, 3, 4, 8, 9, 10, 11, 12 };
> > > 
> > > I was talking about this mux_table_cts_sel array. Can this be const?
> > > 
> > 
> > Other mux tables aren't const, Jerome so you see a reason why they aren't
> > const ?
> 
> I'm surpised we missed that. The PLL params tables of amlogic are all 'static
> const' but all the mux tables are just 'static' , humm
> 
> [...] 5 min later [...]
> 
> Actually the table field of clk_mux is not const, so giving it a const table
> would produce a warning about discarding const qualifier
> 
> I guess there should no problem adding const to the table field of struct
> clk_mux. What do you think Stephen  ?

Yes it would need to be marked const in the struct clk_mux as well. Go
for it to move more things to the RO section.



[PATCH] clk: socfpga: Don't have get_parent for single parent ops

2018-12-04 Thread Stephen Boyd
This driver creates a gate clk with the possibility to have multiple
parents. That can cause problems if the common clk framework tries to
call the get_parent() op and gets back a number that's larger than the
number of parents the clk says it supports in
clk_init_data::num_parents. Let's duplicate the clk_ops structure each
time this function is called and drop the get/set parent ops when there
is only one parent. This allows the framework to consider a number
larger than clk_init_data::num_parents as an error condition of the
get_parent() clk op, clearing the way for proper code.

Cc: Jerome Brunet 
Cc: Masahiro Yamada 
Cc: Dinh Nguyen 
Signed-off-by: Stephen Boyd 
---
 drivers/clk/socfpga/clk-gate.c | 22 +-
 1 file changed, 13 insertions(+), 9 deletions(-)

diff --git a/drivers/clk/socfpga/clk-gate.c b/drivers/clk/socfpga/clk-gate.c
index aa7a6e6a15b6..73e03328d5c5 100644
--- a/drivers/clk/socfpga/clk-gate.c
+++ b/drivers/clk/socfpga/clk-gate.c
@@ -176,8 +176,7 @@ static struct clk_ops gateclk_ops = {
.set_parent = socfpga_clk_set_parent,
 };
 
-static void __init __socfpga_gate_init(struct device_node *node,
-   const struct clk_ops *ops)
+void __init socfpga_gate_init(struct device_node *node)
 {
u32 clk_gate[2];
u32 div_reg[3];
@@ -188,12 +187,17 @@ static void __init __socfpga_gate_init(struct device_node 
*node,
const char *clk_name = node->name;
const char *parent_name[SOCFPGA_MAX_PARENTS];
struct clk_init_data init;
+   struct clk_ops *ops;
int rc;
 
socfpga_clk = kzalloc(sizeof(*socfpga_clk), GFP_KERNEL);
if (WARN_ON(!socfpga_clk))
return;
 
+   ops = kmemdup(_ops, sizeof(gateclk_ops), GFP_KERNEL);
+   if (WARN_ON(!ops))
+   return;
+
rc = of_property_read_u32_array(node, "clk-gate", clk_gate, 2);
if (rc)
clk_gate[0] = 0;
@@ -202,8 +206,8 @@ static void __init __socfpga_gate_init(struct device_node 
*node,
socfpga_clk->hw.reg = clk_mgr_base_addr + clk_gate[0];
socfpga_clk->hw.bit_idx = clk_gate[1];
 
-   gateclk_ops.enable = clk_gate_ops.enable;
-   gateclk_ops.disable = clk_gate_ops.disable;
+   ops->enable = clk_gate_ops.enable;
+   ops->disable = clk_gate_ops.disable;
}
 
rc = of_property_read_u32(node, "fixed-divider", _div);
@@ -234,6 +238,11 @@ static void __init __socfpga_gate_init(struct device_node 
*node,
init.flags = 0;
 
init.num_parents = of_clk_parent_fill(node, parent_name, 
SOCFPGA_MAX_PARENTS);
+   if (init.num_parents < 2) {
+   ops->get_parent = NULL;
+   ops->set_parent = NULL;
+   }
+
init.parent_names = parent_name;
socfpga_clk->hw.hw.init = 
 
@@ -246,8 +255,3 @@ static void __init __socfpga_gate_init(struct device_node 
*node,
if (WARN_ON(rc))
return;
 }
-
-void __init socfpga_gate_init(struct device_node *node)
-{
-   __socfpga_gate_init(node, _ops);
-}
-- 
Sent by a computer through tubes



Re: [PATCH v7 2/4] clk: meson: add DT documentation for emmc clock controller

2018-12-04 Thread Stephen Boyd
Quoting Jianxin Pan (2018-12-03 18:39:34)
> Hi Stephen,
> 
> On 2018/12/4 6:45, Stephen Boyd wrote:
> > Quoting Jianxin Pan (2018-11-15 04:18:30)
> >> diff --git a/include/dt-bindings/clock/amlogic,mmc-clkc.h 
> >> b/include/dt-bindings/clock/amlogic,mmc-clkc.h
> >> new file mode 100644
> >> index 000..162b949
> >> --- /dev/null
> >> +++ b/include/dt-bindings/clock/amlogic,mmc-clkc.h
> >> @@ -0,0 +1,17 @@
> >> +/* SPDX-License-Identifier: (GPL-2.0+ OR MIT) */
> >> +/*
> >> + * Meson MMC sub clock tree IDs
> >> + *
> >> + * Copyright (c) 2018 Amlogic, Inc. All rights reserved.
> >> + * Author: Yixun Lan 
> >> + */
> >> +
> >> +#ifndef __MMC_CLKC_H
> >> +#define __MMC_CLKC_H
> >> +
> >> +#define CLKID_MMC_DIV  1
> > 
> > Why does the define numbering start with 1 instead of 0?
> >
> The Clock ID 0 is used by  CLKID_MMC_MUX.
> CLKID_MMC_MUX is an internal clock which defined in 
> drivers/clk/meson/mmc-clkc.c, and it's the parent of CLKID_MMC_DIV.

Ok, thanks.



Re: [RFC PATCH] Add IPA clock support for clk-rpmh

2018-12-04 Thread Stephen Boyd
Quoting David Dai (2018-12-03 19:50:12)
> This patch extends the existing clk-rpmh driver to support a different
> type of RPMh resource known as Bus Clock Manager(BCM) in order to scale
> performance for the Qualcomm IP Accelerator(IPA) core clock.

Please don't send cover letters for single patches.



Re: [RFC PATCH] clk: qcom: clk-rpmh: Add IPA clock support

2018-12-04 Thread Stephen Boyd
Quoting David Dai (2018-12-03 19:50:13)
> Add IPA clock support by extending the current clk rpmh driver to support
> clocks that are managed by a different type of RPMh resource known as
> Bus Clock Manager(BCM).

Yes, but why? Does the IPA driver need to set clk rates and that somehow
doesn't work as a bandwidth request?

> 
> Signed-off-by: David Dai 
> ---
> diff --git a/drivers/clk/qcom/clk-rpmh.c b/drivers/clk/qcom/clk-rpmh.c
> index 9f4fc77..42e2cd2 100644
> --- a/drivers/clk/qcom/clk-rpmh.c
> +++ b/drivers/clk/qcom/clk-rpmh.c
> @@ -18,6 +18,32 @@
>  #define CLK_RPMH_ARC_EN_OFFSET 0
>  #define CLK_RPMH_VRM_EN_OFFSET 4
>  
> +#define BCM_TCS_CMD_COMMIT_MASK0x4000
> +#define BCM_TCS_CMD_VALID_SHIFT29
> +#define BCM_TCS_CMD_VOTE_MASK  0x3fff
> +#define BCM_TCS_CMD_VOTE_SHIFT 0
> +
> +#define BCM_TCS_CMD(valid, vote) \
> +   (BCM_TCS_CMD_COMMIT_MASK |\

Nitpick: Add space before the \ and align them all up on the right side
of the page.

> +   ((valid) << BCM_TCS_CMD_VALID_SHIFT) |\
> +   ((cpu_to_le32(vote) &\
> +   BCM_TCS_CMD_VOTE_MASK) << BCM_TCS_CMD_VOTE_SHIFT))
> +
> +/**
> + * struct bcm_db - Auxiliary data pertaining to each Bus Clock Manager(BCM)
> + * @unit: divisor used to convert Hz value to an RPMh msg
> + * @width: multiplier used to convert Hz value to an RPMh msg
> + * @vcd: virtual clock domain that this bcm belongs to
> + * @reserved: reserved to pad the struct
> + */
> +

Nitpick: Please remove the newline between comment and struct.

> +struct bcm_db {
> +   u32 unit;
> +   u16 width;
> +   u8 vcd;
> +   u8 reserved;

These would need __le32 and __le16 for the byte swap.

> +};
> +
>  /**
>   * struct clk_rpmh - individual rpmh clock data structure
>   * @hw:handle between common and hardware-specific 
> interfaces
> @@ -210,6 +249,91 @@ static unsigned long clk_rpmh_recalc_rate(struct clk_hw 
> *hw,
> .recalc_rate= clk_rpmh_recalc_rate,
>  };
>  
> +static int clk_rpmh_bcm_send_cmd(struct clk_rpmh *c, bool enable)
> +{
> +   struct tcs_cmd cmd = { 0 };
> +   u32 cmd_state;
> +   int ret;
> +
> +   cmd_state = enable ? (c->aggr_state ? c->aggr_state : 1) : 0;
> +
> +   if (c->last_sent_aggr_state == cmd_state)
> +   return 0;
> +
> +   cmd.addr = c->res_addr;
> +   cmd.data = BCM_TCS_CMD(enable, cmd_state);
> +
> +   ret = rpmh_write_async(c->dev, RPMH_ACTIVE_ONLY_STATE, , 1);
> +   if (ret) {
> +   dev_err(c->dev, "set active state of %s failed: (%d)\n",
> +   c->res_name, ret);
> +   return ret;
> +   }
> +
> +   c->last_sent_aggr_state = cmd_state;
> +
> +   return 0;
> +}
> +
> +static int clk_rpmh_bcm_prepare(struct clk_hw *hw)
> +{
> +   struct clk_rpmh *c = to_clk_rpmh(hw);
> +   int ret = 0;

Don't initialize variables and then reassign them right after.

> +
> +   mutex_lock(_clk_lock);
> +   ret = clk_rpmh_bcm_send_cmd(c, true);
> +   mutex_unlock(_clk_lock);
> +
> +   return ret;
> +};
> +
> +static void clk_rpmh_bcm_unprepare(struct clk_hw *hw)
> +{
> +   struct clk_rpmh *c = to_clk_rpmh(hw);
> +
> +   mutex_lock(_clk_lock);
> +   clk_rpmh_bcm_send_cmd(c, false);
> +   mutex_unlock(_clk_lock);
> +};
> +
> +static int clk_rpmh_bcm_set_rate(struct clk_hw *hw, unsigned long rate,
> +unsigned long parent_rate)
> +{
> +   struct clk_rpmh *c = to_clk_rpmh(hw);
> +
> +   c->aggr_state = rate / (c->aux_data.unit * 1000);
> +
> +   if (clk_hw_is_prepared(hw)) {

Why do we need to check is_prepared? Add a comment indicating we can't
send the request when the clk is disabled?

> +   mutex_lock(_clk_lock);
> +   clk_rpmh_bcm_send_cmd(c, true);

This function is always inside mutex_lock()/unlock() pair, so push the
lock into the function?

> +   mutex_unlock(_clk_lock);
> +   }
> +
> +   return 0;
> +};
> +
> +static long clk_rpmh_round_rate(struct clk_hw *hw, unsigned long rate,
> +   unsigned long *parent_rate)
> +{
> +   return rate;
> +}
> +
> +static unsigned long clk_rpmh_bcm_recalc_rate(struct clk_hw *hw,
> +   unsigned long prate)
> +{
> +   struct clk_rpmh *c = to_clk_rpmh(hw);
> +
> +   return c->aggr_state * c->aux_data.unit * 1000;

What is 1000 for?

> +}
> +
> +static const struct clk_ops clk_rpmh_bcm_ops = {
> +   .prepare= clk_rpmh_bcm_prepare,
> +   .unprepare  = clk_rpmh_bcm_unprepare,
> +   .set_rate   = clk_rpmh_bcm_set_rate,
> +   .round_rate = clk_rpmh_round_rate,
> +   .recalc_rate= clk_rpmh_bcm_recalc_rate,
> +};
> +
>  /* Resource name must match resource id present in cmd-db. */
>  DEFINE_CLK_RPMH_ARC(sdm845, bi_tcxo, bi_tcxo_ao, "xo.lvl", 0x3, 2);
>  DEFINE_CLK_RPMH_VRM(sdm845, 

Re: [PATCH] clk: fix clk_mux_val_to_index() error value

2018-12-04 Thread Stephen Boyd
Quoting Jerome Brunet (2018-12-04 08:34:03)
> clk_mux_val_to_index() is meant to be used by .get_parent(), which
> returns a u8, so when the value provided does not map to any valid index,
> it is not a good idea to return a negative error value.
> 
> Instead, return num_parents which we know is an invalid index and let
> CCF deal with it.
> 
> Fixes: 77deb66d262f ("clk: mux: add helper function for index/value 
> translation")
> Signed-off-by: Jerome Brunet 
> ---

Thanks!

> diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
> index 60c51871b04b..fc20886ef069 100644
> --- a/include/linux/clk-provider.h
> +++ b/include/linux/clk-provider.h
> @@ -550,8 +550,8 @@ struct clk_hw *clk_hw_register_mux_table(struct device 
> *dev, const char *name,
> void __iomem *reg, u8 shift, u32 mask,
> u8 clk_mux_flags, u32 *table, spinlock_t *lock);
>  
> -int clk_mux_val_to_index(struct clk_hw *hw, u32 *table, unsigned int flags,
> -unsigned int val);
> +u8 clk_mux_val_to_index(struct clk_hw *hw, u32 *table, unsigned int flags,

I wonder if we should just make this unsigned int? Does it hurt at all
to have it be a wider type even though it doesn't match the CCF decision
to make this a u8 for the parent index number space?



Re: [PATCH v3 5/5] clk: samsung: exynos5433: add imem clocks

2018-12-04 Thread Stephen Boyd
Quoting Kamil Konieczny (2018-12-04 08:52:48)
> +
> +static const unsigned long imem_clk_regs[] __initconst = {
> +   ENABLE_ACLK_IMEM,
> +   ENABLE_ACLK_IMEM_INT_MEM,
> +   ENABLE_ACLK_IMEM_SSS,
> +   ENABLE_ACLK_IMEM_SLIMSSS,
> +   ENABLE_ACLK_IMEM_RTIC,
> +   ENABLE_ACLK_IMEM_SMMU_SSS,
> +   ENABLE_ACLK_IMEM_SMMU_SLIMSSS,
> +   ENABLE_ACLK_IMEM_SMMU_RTIC,
> +   ENABLE_ACLK_IMEM_ARBG_TX,
> +   ENABLE_ACLK_IMEM_SMMU_ARBG_TX,
> +   ENABLE_PCLK_IMEM,
> +   ENABLE_PCLK_IMEM_SSS,
> +   ENABLE_PCLK_IMEM_SLIMSSS,
> +   ENABLE_PCLK_IMEM_RTIC,
> +   ENABLE_PCLK_IMEM_SMMU_SSS,
> +   ENABLE_PCLK_IMEM_SMMU_SLIMSSS,
> +   ENABLE_PCLK_IMEM_SMMU_RTIC,
> +   ENABLE_PCLK_IMEM_SMMU_ARGB_TX,
> +};
> +
> +static const struct samsung_gate_clock imem_gate_clks[] __initconst = {
> +   /* ENABLE_ACLK_IMEM */
> +   GATE(CLK_ACLK_AXI2AHB_IMEMH, "aclk_axi2ahb_imemh", "aclk_imem_200",
> +   ENABLE_ACLK_IMEM, 24, 0, 0),
> +   GATE(CLK_ACLK_AXIDS_SROMC, "aclk_axids_sromc", "aclk_imem_200",
> +   ENABLE_ACLK_IMEM, 23, CLK_IGNORE_UNUSED, 0),

Why is there so much use of CLK_IGNORE_UNUSED in this file?

> +   GATE(CLK_ACLK_SROMC, "aclk_sromc", "aclk_imem_200",


Re: [PATCH 07/14] clock: milbeaut: Add Milbeaut M10V clock control

2018-12-04 Thread Stephen Boyd
Quoting Sugaya, Taichi (2018-12-04 00:26:16)
> On 2018/11/30 17:31, Stephen Boyd wrote:
> > Quoting Sugaya Taichi (2018-11-18 17:01:12)
> >> +void __init m10v_clk_mux_setup(struct device_node *node)
> >> +{
> >> +   const char *clk_name = node->name;
> >> +   struct clk_init_data init;
> >> +   const char **parent_names;
> >> +   struct m10v_mux *mcm;
> >> +   struct clk *clk;
> >> +   int i, parents;
> >> +
> >> +   if (!m10v_clk_iomap())
> >> +   return;
> >> +
> >> +   of_property_read_string(node, "clock-output-names", _name);
> >> +
> >> +   parents = of_clk_get_parent_count(node);
> >> +   if (parents < 2) {
> >> +   pr_err("%s: not a mux\n", clk_name);
> > 
> > How is this possible?
> 
> When the node has more than 1 clks...
> Or I am misunderstanding your question?

This looks like code that's checking DT for correctness. We don't
typically do that in the kernel because the kernel isn't a DT validator.
That's all I'm saying. I think this comment is not useful if the driver
design is done to specify parent linkages in C code instead of DT, so
don't worry about this too much.



Re: [PATCH 07/14] clock: milbeaut: Add Milbeaut M10V clock control

2018-12-04 Thread Stephen Boyd
Quoting Masahiro Yamada (2018-12-04 03:03:53)
> Hi Stephen,
> 
> 
> On Fri, Nov 30, 2018 at 5:31 PM Stephen Boyd  wrote:
> >
> > Quoting Sugaya Taichi (2018-11-18 17:01:12)
> > > Add Milbeaut M10V clock ( including PLL ) control.
> >
> > Please give some more details here.
> >
> > >
> > > Signed-off-by: Sugaya Taichi 
> > > ---
> > >  drivers/clk/Makefile   |   1 +
> > >  drivers/clk/clk-m10v.c | 671 
> > > +
> >
> > And this is different from Uniphier? Maybe we need a socionext
> > directory under drivers/clk/.
> 
> 
> 
> This is always a difficult question,
> and I do not have a strong opinion.
> 
> 
> I am fine with moving the files to drivers/clk/socionext
> although no file would be shared.
> 
> 
> FYI
> 
> UniPhier and Milbeaut are completely different platforms
> developed/maintained by different teams.
> 
> They happen to live in the same company now
> just because Socionext merged the LSI business from Panasonic and Fujitsu.
> 
> UniPhier originates in Panasonic, while Milbeaut in Fujitsu.
> 

Thanks for the background info. I'd prefer to defer to however the dts
files are getting split up into directories. If they're all put under
arch/arm64/boot/dts/socionext/ then I would say combine the two clk
drivers into a socionext directory. Otherwise, keep them split out.



Re: [PATCH] Revert "clk: fix __clk_init_parent() for single parent clocks"

2018-12-04 Thread Stephen Boyd
Quoting Jerome Brunet (2018-12-04 08:32:57)
> This reverts commit 2430a94d1e719b7b4af2a65b781a4c036eb22e64.
> 
> From the original commit message:
>   It turned out a problem because there are some single-parent clocks
>   that implement .get_parent() callback and return non-zero index.
>   The SOCFPGA clock is the case; the commit broke the SOCFPGA boards.
> 
> It is wrong for a clock to return an index >= num_parents. CCF checks
> for this condition in clk_core_get_parent_by_index(). This function sets
> the parent to NULL if index is incoherent, which seems like the only
> sane choice.
> 
> commit 2430a94d1e71 ("clk: fix __clk_init_parent() for single parent clocks")
> appears to be a work around installed in the core framework for a problem
> that is platform specific, and should probably be fixed in the platform code.

Ouch. I see that I even pointed that out in 2016 but never got a reply
or a fix patch[1].

> 
> Further more, it introduces a problem in a corner case of the mux clock.
> Take mux with multiple parents, but only one is known, the rest being
> undocumented. The register reset has one of unknown parent set.
> 
> Before commit 2430a94d1e71 ("clk: fix __clk_init_parent() for single parent 
> clocks"):
>  * get_parent() is called, register is read and give an unknown index.
>-> the mux is orphaned.
>  * a call to set_rate() will reparent the mux to the only known parent.
> 
> With commit 2430a94d1e71 ("clk: fix __clk_init_parent() for single parent 
> clocks"):
>  * the register is never read.
>  * parent is wrongly assumed to be the only known one.
>As a consequence, all the calculation deriving from the mux will be
>wrong
>  * Since we believe the only know parent to be set, set_parent() won't
>ever be called and the register won't be set with the correct value.

Isn't this the broken bad case all over again? Why register a clk as a
mux and then only say it has one parent?

> 
> Signed-off-by: Jerome Brunet 
> ---

Is this related to the other patch you sent? Can you send series for
related patches please?

[1] https://lkml.kernel.org/r/20160209181833.ga24...@codeaurora.org


Re: [PATCH] clk: meson-gxbb: switch video clocks mux tables to static

2018-12-04 Thread Stephen Boyd
Quoting Neil Armstrong (2018-12-04 04:58:39)
> Makes the following sparse warnings silent:
> drivers/clk/meson/vid-pll-div.c:58:26: warning: symbol '_get_table_val' was 
> not declared. Should it be static?
> drivers/clk/meson/gxbb.c:1585:12: warning: symbol 'gxbb_vid_pll_parent_names' 
> was not declared. Should it be static?
> drivers/clk/meson/gxbb.c:1620:12: warning: symbol 'gxbb_vclk_parent_names' 
> was not declared. Should it be static?
> drivers/clk/meson/gxbb.c:1980:12: warning: symbol 'gxbb_cts_parent_names' was 
> not declared. Should it be static?
> drivers/clk/meson/gxbb.c:2036:12: warning: symbol 
> 'gxbb_cts_hdmi_tx_parent_names' was not declared. Should it be static?

I fixed all of these already in the patch I sent you.

> @@ -1983,7 +1983,7 @@ static struct clk_fixed_factor gxbb_vclk2_div12 = {
>  };
>  
>  static u32 mux_table_cts_sel[] = { 0, 1, 2, 3, 4, 8, 9, 10, 11, 12 };

I was talking about this mux_table_cts_sel array. Can this be const?



Re: [PATCH v11 1/2] dt-bindings: cpufreq: Introduce QCOM CPUFREQ Firmware bindings

2018-12-03 Thread Stephen Boyd
Quoting Rob Herring (2018-12-03 15:09:07)
> On Sun, Dec 02, 2018 at 09:25:02AM +0530, Taniya Das wrote:
> > Add QCOM cpufreq firmware device bindings for Qualcomm Technology Inc's
> > SoCs. This is required for managing the cpu frequency transitions which are
> > controlled by the hardware engine.
> > 
> > Signed-off-by: Taniya Das 
> > ---
> >  .../bindings/cpufreq/cpufreq-qcom-hw.txt   | 172 
> > +
> >  1 file changed, 172 insertions(+)
> >  create mode 100644 
> > Documentation/devicetree/bindings/cpufreq/cpufreq-qcom-hw.txt
> > 
> > diff --git a/Documentation/devicetree/bindings/cpufreq/cpufreq-qcom-hw.txt 
> > b/Documentation/devicetree/bindings/cpufreq/cpufreq-qcom-hw.txt
> > new file mode 100644
> > index 000..2b82965
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/cpufreq/cpufreq-qcom-hw.txt
> > @@ -0,0 +1,172 @@
> > +Qualcomm Technologies, Inc. CPUFREQ Bindings
> > +
> > +CPUFREQ HW is a hardware engine used by some Qualcomm Technologies, Inc. 
> > (QTI)
> > +SoCs to manage frequency in hardware. It is capable of controlling 
> > frequency
> > +for multiple clusters.
> > +
> > +Properties:
> > +- compatible
> > + Usage:  required
> > + Value type: 
> > + Definition: must be "qcom,cpufreq-hw".
> > +
> > +- clocks
> > + Usage:  required
> > + Value type:  From common clock binding.
> > + Definition: clock handle for XO clock and GPLL0 clock.
> > +
> > +- clock-names
> > + Usage:  required
> > + Value type:  From common clock binding.
> > + Definition: must be "xo", "alternate".
> > +
> > +- reg
> > + Usage:  required
> > + Value type: 
> > + Definition: Addresses and sizes for the memory of the HW bases in
> > + each frequency domain.
> > +- reg-names
> > + Usage:  Optional
> > + Value type: 
> > + Definition: Frequency domain name i.e.
> > + "freq-domain0", "freq-domain1".
> > +
> > +- freq-domain-cells:
> 
> #freq-domain-cells
> 
> Otherwise,
> 
> Reviewed-by: Rob Herring 

Or should it be #qcom,freq-domain-cells? That would match the same stem
of the property used in the cpu node.




Re: [PATCH] arm64: dts: qcom: msm8998: Fix compatible of scm node

2018-12-03 Thread Stephen Boyd
Quoting Bjorn Andersson (2018-11-29 22:56:55)
> The scm binding and driver was updated to rely on the fallback to the
> default qcom,scm for any modern SoC and as such both are required. Add
> the default compatible to make the scm instance probe.
> 
> Fixes: d850156a226a ("arm64: dts: qcom: msm8998: Add firmware node")
> Signed-off-by: Bjorn Andersson 
> ---

Reviewed-by: Stephen Boyd 



Re: [RFC v3 2/3] dt-bindings: sdm845-pinctrl: add wakeup interrupt parent for GPIO

2018-12-03 Thread Stephen Boyd
Quoting Lina Iyer (2018-11-30 10:33:17)
> On Thu, Nov 29 2018 at 14:45 -0700, Lina Iyer wrote:
> >On Wed, Nov 28 2018 at 17:25 -0700, Bjorn Andersson wrote:
> >>On Wed 28 Nov 09:39 PST 2018, Lina Iyer wrote:
> >>
> >>>On Tue, Nov 27 2018 at 14:45 -0700, Stephen Boyd wrote:
> >>>> Quoting Lina Iyer (2018-11-27 10:21:23)
> >>>> > On Tue, Nov 27 2018 at 02:12 -0700, Stephen Boyd wrote:
> 
> [...]
> >BTW, I am discussing with the internal folks here on if we need to mask
> >TLMM when the wakeup-parent is MPM. If we don't have to, we should be
> >able to follow the same model as we done in this patch and don't even
> >have to check the compatible or use the approach suggested by Stephen.
> >
> The TLMM and the MPM are not active at the same time. However, there is
> a small chance they might be (a few clock cycles) when the system is
> going down, but even then, since we replay the interrupt from the MPM
> driver before the interrupts are serviced by Linux, we would not see
> multiple GPIO interrupts.
> 
> The way we have MPM working downstream, for a wakeup GPIO IRQ -
> 
> a. Application cores gets a wakeup interrupt either from RPM or GIC (if
> TLMM was not powered down) while still in the interrupt locked context.
> 
> b. In the hardware, apps core handshakes with the RPM and then starts
> resuming from the platform's system idle driver.
> 
> c. The first CPU to wake up calls MPM driver from the idle driver, which
> reads the shared memory to find the MPM pins that are set. Converts the
> MPM pins to their corresponding linux interrupt and replays the
> interrupt.
> 
> d. Idle driver exits and wakeup GPIO interrupt is handled.
> 
> The MPM pins are not updated after the RPM lets the application core to
> run. Since TLMM is functional after the RPM handshake, it takes over.
> 

Thanks for the background info. I don't think it really changes anything
that we've discussed though. We still need to mask the IRQ in TLMM all
the time when we're using the PDC and we need to leave it unmasked and
replay edges that the MPM sees when we use the MPM. Should I clean up my
RFC patch and post it to the list? I'd like to see hierarchical gpio
irqs work in general for this problem and also the SSBI/SPMI gpio irq
problem that Linus pointed out last week.



Re: [PATCH v4 1/8] clk: clkdev/of_clk - add managed lookup and provider registrations

2018-12-03 Thread Stephen Boyd
Quoting Matti Vaittinen (2018-11-30 02:50:22)
> Hello Stephen,
> 
> Thanks a bunch for taking the time and reviewing this!
> 
> On Fri, Nov 30, 2018 at 12:54:10AM -0800, Stephen Boyd wrote:
> > Quoting Matti Vaittinen (2018-11-13 03:55:58)
> > > With MFD devices the clk properties may be contained in MFD (parent) DT
> > > node. Current devm_of_clk_add_hw_provider assumes the clk is bound to MFD
> > > subdevice not to MFD device (parent). Add
> > > devm_of_clk_add_hw_provider_parent to tackle this issue.
> > > 
> > > Also clkdev registration lacks of managed registration functions and it
> > > seems few drivers do not drop clkdev lookups at exit. Add
> > > devm_clk_hw_register_clkdev and devm_clk_release_clkdev to ease lookup
> > > releasing at exit.
> > 
> > Please split this into clkdev and non-clkdev devm functionality.
> Allright. I'll split this to two patches.
> 
> > > --- a/Documentation/driver-model/devres.txt
> > > +++ b/Documentation/driver-model/devres.txt
> > > @@ -238,6 +238,9 @@ CLOCK
> > >devm_clk_put()
> > >devm_clk_hw_register()
> > >devm_of_clk_add_hw_provider()
> > > +  devm_of_clk_add_parent_hw_provider()
> > > +  devm_clk_hw_register_clkdev()
> > > +  devm_clk_release_clkdev()
> > 
> > The 'release' or non-common functions shouldn't be documented here.
> So I will drop the line mentioning devm_clk_release_clkdev()
> 
> > > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> > > index af011974d4ec..9bb921eb90f6 100644
> > > --- a/drivers/clk/clk.c
> > > +++ b/drivers/clk/clk.c
> > > @@ -3893,12 +3893,12 @@ static void devm_of_clk_release_provider(struct 
> > > device *dev, void *res)
> > > of_clk_del_provider(*(struct device_node **)res);
> > >  }
> > >  
> > > -int devm_of_clk_add_hw_provider(struct device *dev,
> > > +static int __devm_of_clk_add_hw_provider(struct device *dev,
> > > struct clk_hw *(*get)(struct of_phandle_args 
> > > *clkspec,
> > >   void *data),
> > > -   void *data)
> > > +   struct device_node *of_node, void *data)
> > >  {
> > > -   struct device_node **ptr, *np;
> > > +   struct device_node **ptr;
> > > int ret;
> > >  
> > > ptr = devres_alloc(devm_of_clk_release_provider, sizeof(*ptr),
> > > @@ -3906,10 +3906,9 @@ int devm_of_clk_add_hw_provider(struct device *dev,
> > > if (!ptr)
> > > return -ENOMEM;
> > >  
> > > -   np = dev->of_node;
> > > -   ret = of_clk_add_hw_provider(np, get, data);
> > > +   *ptr = of_node;
> > > +   ret = of_clk_add_hw_provider(of_node, get, data);
> > > if (!ret) {
> > > -   *ptr = np;
> > 
> > Why is this moved outside of the if condition?
> I completely removed the local variable np and just unconditionally set
> the allocated devres to point at the node (if allocation succeeded). We
> could of course only do this if the provider registration succeeded and
> save one assignment - but I guess I intended to remove the curly braces
> and thus decided to go for one liner after if. But apparently I didn't
> remove the braces O_o. Well, I can put the assignment inside the
> condition if you prefer that.
> 
> > In fact, why isn't just
> > the first line in this hunk deleted and passed to this function as
> > struct device_node *np?
> 
> I am sorry but I don't quite follow your suggestion here. Do you mean we
> could just pass the struct device_node *np in devres_add()? I thought
> the pointer passed to devress_add() should be allocated using
> devres_alloc. Can you please elaborate what you mean?

I'm just trying to reduce the diff in the patch.

> 
> > 
> > > devres_add(dev, ptr);
> > > } else {
> > > devres_free(ptr);
[..]
> > 
> > > +int devm_of_clk_add_hw_provider(struct device *dev,
> > > +   struct clk_hw *(*get)(struct of_phandle_args 
> > > *clkspec,
> > > + void *data),
> > > +   void *data)
> > > +{
> > > +   return __devm_of_clk_add_hw_provider(dev, get, dev->of_node, 
> > > data);
> > > +}
> > >  EXPORT_SYMBOL_GPL(devm_of_clk_add_hw_provider);
> > >  
> > > +int dev

Re: [PATCH v7 2/4] clk: meson: add DT documentation for emmc clock controller

2018-12-03 Thread Stephen Boyd
Quoting Jianxin Pan (2018-11-15 04:18:30)
> diff --git a/include/dt-bindings/clock/amlogic,mmc-clkc.h 
> b/include/dt-bindings/clock/amlogic,mmc-clkc.h
> new file mode 100644
> index 000..162b949
> --- /dev/null
> +++ b/include/dt-bindings/clock/amlogic,mmc-clkc.h
> @@ -0,0 +1,17 @@
> +/* SPDX-License-Identifier: (GPL-2.0+ OR MIT) */
> +/*
> + * Meson MMC sub clock tree IDs
> + *
> + * Copyright (c) 2018 Amlogic, Inc. All rights reserved.
> + * Author: Yixun Lan 
> + */
> +
> +#ifndef __MMC_CLKC_H
> +#define __MMC_CLKC_H
> +
> +#define CLKID_MMC_DIV  1

Why does the define numbering start with 1 instead of 0?

> +#define CLKID_MMC_PHASE_CORE   2
> +#define CLKID_MMC_PHASE_TX 3
> +#define CLKID_MMC_PHASE_RX 4
> +


Re: [PATCH v1 4/4] phy: qcom-qmp: Expose provided clocks to DT

2018-12-03 Thread Stephen Boyd
Quoting Evan Green (2018-11-29 14:13:57)
> Register a simple clock provider for the PHY pipe clock sources so that
> device tree users can point at these clocks via phandles to the lane
> nodes.
> 
> Signed-off-by: Evan Green 
> ---

Reviewed-by: Stephen Boyd 



Re: [PATCH v1 3/4] arm64: dts: qcom: sdm845: Fix QMP PHY #clock-cells

2018-12-03 Thread Stephen Boyd
Quoting Evan Green (2018-11-29 14:13:56)
> Move #clock-cells into the child node for instances of the qcom-qmp-phy
> nodes, and set it to zero, in accordance with the proper bindings. PHYs
> that don't provide clocks don't have #clock-cells, and so are left alone.
> 
> Signed-off-by: Evan Green 
> ---

Reviewed-by: Stephen Boyd 



Re: [PATCH v1 2/4] arm64: dts: qcom: msm8996: Fix QMP PHY #clock-cells

2018-12-03 Thread Stephen Boyd
Quoting Evan Green (2018-11-29 14:13:55)
> Move #clock-cells into the child node and set it to 0 to conform to the
> proper binding specification.
> 
> Signed-off-by: Evan Green 
> ---

Reviewed-by: Stephen Boyd 



Re: [PATCH v1 1/4] dt-bindings: phy-qcom-qmp: Move #clock-cells to child

2018-12-03 Thread Stephen Boyd
Quoting Evan Green (2018-11-29 14:13:54)
> The phy-qcom-qmp bindings specified #clock-cells as 1. This was never used
> because of_clk_add_provider() was never called, so there was no way anybody
> could reference these clocks from DT. Furthermore, even if they could be
> accessed, the bindings never specified what should go in that additional
> cell.
> 
> Fix these incomplete and broken bindings. Move the #clock-cells into the
> child node, since that is the actual clock provider, and not all
> instances of qcom-qmp-phy are clock providers. Also set #clock-cells to
> zero, since there's nothing to pass to it.
> 
> Signed-off-by: Evan Green 
> ---

Reviewed-by: Stephen Boyd 



Re: [PATCH 2/2] clk: core: link consumer with clock driver

2018-12-03 Thread Stephen Boyd
Quoting Miquel Raynal (2018-12-03 14:16:55)
> Stephen Boyd  wrote on Mon, 03 Dec 2018 11:20:31
> -0800:
> > Quoting Miquel Raynal (2018-11-30 02:20:52)
> > > Stephen Boyd  wrote on Fri, 30 Nov 2018 01:26:20
> > > -0800:
> > > > Quoting Miquel Raynal (2018-11-23 01:11:32)  
> > > 
> > > Because we need the caller's 'struct device' to make the link and
> > > this is not available in __clk_get(). I tried to ad it as parameter but
> > > I don't think it is possible to retrieve a 'struct device' from the
> > > device name. The functions where this is problematic are:
> > > * clk.c:__of_clk_get_from_provider()
> > > * clkdev.c:clk_get_sys()
> > > 
> > > By the way in my new version I called the helpers:
> > > * clk_{link,unlink}_hierarchy()
> > > * clk_{link,unlink}_consumer()
> > > 
> > > I will send a new version with these helpers, but if you have anything
> > > in mind to help me achieve the above request, I will welcome the idea.
> > >   
> > 
> > We can do the linking in __clk_get() and __clk_put() if we poke into the
> > struct clk -> struct clk_core and bury the struct device into each
> > clk_core structure.
> > 
> 
> I meant the consumer device's structure. Yes, from a struct clk, the
> first change in patch 1/2 let's us do clk->core->dev to get the clock
> device. But for linking I need both the clock device and the consumer
> device; and the latter will be missing in __clk_get().
> 

Ah ok, sounds like this is how it has to be then.



Re: [PATCH V6 3/9] clk: imx: add pllv4 support

2018-12-03 Thread Stephen Boyd
Quoting A.s. Dong (2018-11-14 05:01:43)
> pllv4 is designed for System Clock Generation (SCG) module observed
> in IMX ULP SoC series. e.g. i.MX7ULP.
> 
> The SCG modules generates clock used to derive processor, system,
> peripheral bus and external memory interface clocks while this patch
> intends to support the PLL part.
> 
> Cc: Stephen Boyd 
> Cc: Michael Turquette 
> Cc: Shawn Guo 
> Cc: Anson Huang 
> Cc: Bai Ping 
> Signed-off-by: Dong Aisheng 
> 
> ---

Applied to clk-next



Re: [PATCH V6 8/9] clk: imx: implement new clk_hw based APIs

2018-12-03 Thread Stephen Boyd
Quoting A.s. Dong (2018-11-14 05:02:04)
> Clock providers are recommended to use the new struct clk_hw based API,
> so implement IMX clk_hw based provider helpers functions to the new
> approach.
> 
> Signed-off-by: Dong Aisheng 
> 
> ---

Applied to clk-next



Re: [PATCH V6 9/9] clk: imx: add imx7ulp clk driver

2018-12-03 Thread Stephen Boyd
Quoting A.s. Dong (2018-11-14 05:02:08)
> i.MX7ULP Clock functions are under joint control of the System
> Clock Generation (SCG) modules, Peripheral Clock Control (PCC)
> modules, and Core Mode Controller (CMC)1 blocks
> 
> The clocking scheme provides clear separation between M4 domain
> and A7 domain. Except for a few clock sources shared between two
> domains, such as the System Oscillator clock, the Slow IRC (SIRC),
> and and the Fast IRC clock (FIRCLK), clock sources and clock
> management are separated and contained within each domain.
> 
> M4 clock management consists of SCG0, PCC0, PCC1, and CMC0 modules.
> A7 clock management consists of SCG1, PCC2, PCC3, and CMC1 modules.
> 
> This driver only adds clock support in A7 domain.
> 
> Note that most clocks required to be operated when gated, e.g. pll,
> pfd, pcc. And more special cases that scs/ddr/nic mux selecting
> different clock source requires that clock to be enabled first,
> then we need set CLK_OPS_PARENT_ENABLE flag for them properly.
> 
> Cc: Stephen Boyd 
> Cc: Michael Turquette 
> Cc: Shawn Guo 
> Cc: Anson Huang 
> Cc: Bai Ping 
> Signed-off-by: Dong Aisheng 
> 
> ---

Applied to clk-next



Re: [PATCH V6 6/9] dt-bindings: clock: add imx7ulp clock binding doc

2018-12-03 Thread Stephen Boyd
Quoting A.s. Dong (2018-11-14 05:01:56)
> i.MX7ULP Clock functions are under joint control of the System
> Clock Generation (SCG) modules, Peripheral Clock Control (PCC)
> modules, and Core Mode Controller (CMC)1 blocks
> 
> Note IMX7ULP has two clock domains: M4 and A7. This binding doc
> is only for A7 clock domain.
> 
> Cc: Rob Herring 
> Cc: Mark Rutland 
> Cc: Stephen Boyd 
> Cc: Michael Turquette 
> Cc: Shawn Guo 
> Cc: Anson Huang 
> Cc: Bai Ping 
> Signed-off-by: Dong Aisheng 
> 
> ---

Applied to clk-next



Re: [PATCH V6 7/9] clk: imx: make mux parent strings const

2018-12-03 Thread Stephen Boyd
Quoting A.s. Dong (2018-11-14 05:02:00)
> As the commit 2893c379461a ("clk: make strings in parent name arrays
> const"), let's make the parent strings const, otherwise we may meet
> the following warning when compiling:
> 
> drivers/clk/imx/clk-imx7ulp.c: In function 'imx7ulp_clocks_init':
> drivers/clk/imx/clk-imx7ulp.c:73:35: warning: passing argument 5 of
> 'imx_clk_mux_flags' discards 'const' qualifier from pointer target 
> type
> 
>   clks[IMX7ULP_CLK_APLL_PRE_SEL] = imx_clk_mux_flags("apll_pre_sel", base + 
> 0x508, 0,
> 1, pll_pre_sels, ARRAY_SIZE(pll_pre_sels), CLK_SET_PARENT_GATE);
>^
> In file included from drivers/clk/imx/clk-imx7ulp.c:23:0:
> drivers/clk/imx/clk.h:200:27: note: expected 'const char **' but argument is
>  of type 'const char * const*'
> ...
> 
> Cc: Stephen Boyd 
> Cc: Michael Turquette 
> Cc: Shawn Guo 
> Signed-off-by: Dong Aisheng 
> 
> ---

Applied to clk-next



Re: [PATCH V6 4/9] clk: imx: add pfdv2 support

2018-12-03 Thread Stephen Boyd
Quoting A.s. Dong (2018-11-14 05:01:47)
> The pfdv2 is designed for PLL Fractional Divide (PFD) observed in System
> Clock Generation (SCG) module in IMX ULP SoC series. e.g. i.MX7ULP.
> 
> NOTE pfdv2 can only be operated when clk is gated.
> 
> Cc: Stephen Boyd 
> Cc: Michael Turquette 
> Cc: Shawn Guo 
> Cc: Anson Huang 
> Cc: Bai Ping 
> Signed-off-by: Dong Aisheng 
> 
> ---

Applied to clk-next



Re: [PATCH V6 5/9] clk: imx: add imx7ulp composite clk support

2018-12-03 Thread Stephen Boyd
Quoting A.s. Dong (2018-11-14 05:01:51)
> The imx composite clk is designed for Peripheral Clock Control (PCC)
> module observed in IMX ULP SoC series.
> 
> NOTE pcc can only be operated when clk is gated.
> 
> Cc: Stephen Boyd 
> Cc: Michael Turquette 
> Cc: Shawn Guo 
> Cc: Anson Huang 
> Cc: Bai Ping 
> Signed-off-by: Dong Aisheng 
> 
> ---

Applied to clk-next



Re: [PATCH V6 2/9] clk: fractional-divider: add CLK_FRAC_DIVIDER_ZERO_BASED flag support

2018-12-03 Thread Stephen Boyd
Quoting A.s. Dong (2018-11-14 05:01:39)
> Adding CLK_FRAC_DIVIDER_ZERO_BASED flag to indicate the numerator and
> denominator value in register are start from 0.
> 
> This can be used to support frac dividers like below:
> Divider output clock = Divider input clock x [(frac +1) / (div +1)]
> where frac/div in register is:
> 000b - Divide by 1.
> 001b - Divide by 2.
> 010b - Divide by 3.
> 
> Cc: Stephen Boyd 
> Cc: Michael Turquette 
> Signed-off-by: Dong Aisheng 
> 
> ---

Applied to clk-next



Re: [PATCH V6 0/9] clk: add imx7ulp clk support

2018-12-03 Thread Stephen Boyd
Quoting A.s. Dong (2018-11-14 05:01:31)
> This patch series intends to add imx7ulp clk support.
> 
> i.MX7ULP Clock functions are under joint control of the System
> Clock Generation (SCG) modules, Peripheral Clock Control (PCC)
> modules, and Core Mode Controller (CMC)1 blocks
> 
> The clocking scheme provides clear separation between M4 domain
> and A7 domain. Except for a few clock sources shared between two
> domains, such as the System Oscillator clock, the Slow IRC (SIRC),
> and and the Fast IRC clock (FIRCLK), clock sources and clock
> management are separated and contained within each domain.
> 
> M4 clock management consists of SCG0, PCC0, PCC1, and CMC0 modules.
> A7 clock management consists of SCG1, PCC2, PCC3, and CMC1 modules.
> 
> Note: this series only adds A7 clock domain support as M4 clock
> domain will be handled by M4 seperately.
> 

I got:

drivers/clk/imx/clk-pllv4.c:152:15: warning: symbol 'imx_clk_pllv4' was not 
declared. Should it be static?
drivers/clk/imx/clk-pfdv2.c:166:15: warning: symbol 'imx_clk_pfdv2' was not 
declared. Should it be static?
drivers/clk/imx/clk-divider-gate.c:174:15: warning: symbol 
'imx_clk_divider_gate' was not declared. Should it be static?
drivers/clk/imx/clk-composite-7ulp.c:22:15: warning: symbol 
'imx7ulp_clk_composite' was not declared. Should it be static?

which I can fix easily by throwing in clk.h into each file.



Re: [PATCH V6 1/9] clk: imx: add gatable clock divider support

2018-12-03 Thread Stephen Boyd
Quoting A.s. Dong (2018-11-14 05:01:35)
> For dividers with zero indicating clock is disabled, instead of giving a
> warning each time like "clkx: Zero divisor and CLK_DIVIDER_ALLOW_ZERO not
> set" in exist code, we'd like to introduce enable/disable function for it.
> e.g.
> 000b - Clock disabled
> 001b - Divide by 1
> 010b - Divide by 2
> ...
> 
> Set rate when the clk is disabled will cache the rate request and only
> when the clk is enabled will the driver actually program the hardware to
> have the requested divider value. Similarly, when the clk is disabled we'll
> write a 0 there, but when the clk is enabled we'll restore whatever rate
> (divider) was chosen last.
> 
> It does mean that recalc rate will be sort of odd, because when the clk is
> off it will return 0, and when the clk is on it will return the right rate.
> So to make things work, we'll need to return the cached rate in recalc rate
> when the clk is off and read the hardware when the clk is on.
> 
> NOTE for the default off divider, the recalc rate will still return 0 as
> there's still no proper preset rate. Enable such divider will give user
> a reminder error message.
> 
> Cc: Stephen Boyd 
> Cc: Michael Turquette 
> Cc: Shawn Guo 
> Signed-off-by: Dong Aisheng 
> 
> ---

Applied to clk-next



Re: [PATCH 2/2] clk: core: link consumer with clock driver

2018-12-03 Thread Stephen Boyd
Quoting Miquel Raynal (2018-11-30 02:20:52)
> Hi Stephen,
> 
> Stephen Boyd  wrote on Fri, 30 Nov 2018 01:26:20
> -0800:
> 
> > Quoting Miquel Raynal (2018-11-23 01:11:32)
> > > Would you agree with me adding dummy functions in the #else section
> > > like:
> > > 
> > > static inline void __clk_device_link(struct device *consumer, struct clk 
> > > *clk)
> > > {
> > >return;
> > > }
> > > 
> > > static inline void __clk_device_unlink(struct clk *clk)
> > > {
> > >return;
> > > }
> > > 
> > > Do you want me to also declare these functions in the #if section
> > > (with the external keyword) to balance the above declarations?  
> > 
> > Why can't we do the linking in __clk_get() and __clk_put()?
> > 
> 
> Because we need the caller's 'struct device' to make the link and
> this is not available in __clk_get(). I tried to ad it as parameter but
> I don't think it is possible to retrieve a 'struct device' from the
> device name. The functions where this is problematic are:
> * clk.c:__of_clk_get_from_provider()
> * clkdev.c:clk_get_sys()
> 
> By the way in my new version I called the helpers:
> * clk_{link,unlink}_hierarchy()
> * clk_{link,unlink}_consumer()
> 
> I will send a new version with these helpers, but if you have anything
> in mind to help me achieve the above request, I will welcome the idea.
> 

We can do the linking in __clk_get() and __clk_put() if we poke into the
struct clk -> struct clk_core and bury the struct device into each
clk_core structure.



Re: [PATCH 05/12] PCI: aardvark: add suspend to RAM support

2018-12-03 Thread Stephen Boyd
Quoting Lorenzo Pieralisi (2018-12-03 09:18:59)
> [+Stephen, Mike]
> 
> On Mon, Dec 03, 2018 at 04:38:46PM +0100, Miquel Raynal wrote:
> > Hi Lorenzo,
> > 
> > Lorenzo Pieralisi  wrote on Mon, 3 Dec 2018
> > 10:27:08 +:
> > 
> > > [+Rafael, Sudeep]
> > > 
> > > On Fri, Nov 23, 2018 at 03:18:24PM +0100, Miquel Raynal wrote:
> > > > Add suspend and resume callbacks. The priority of these are
> > > > "_noirq()", to workaround early access to the registers done by the
> > > > PCI core through the ->read()/->write() callbacks at resume time.
> > > > 
> > > > Signed-off-by: Miquel Raynal 
> > > > ---
> > > >  drivers/pci/controller/pci-aardvark.c | 52 +++
> > > >  1 file changed, 52 insertions(+)
> > > > 
> > > > diff --git a/drivers/pci/controller/pci-aardvark.c 
> > > > b/drivers/pci/controller/pci-aardvark.c
> > > > index 108b3f15c410..7ecf1ac4036b 100644
> > > > --- a/drivers/pci/controller/pci-aardvark.c
> > > > +++ b/drivers/pci/controller/pci-aardvark.c
> > > > @@ -1108,6 +1108,55 @@ static int advk_pcie_setup_clk(struct advk_pcie 
> > > > *pcie)
> > > >   return ret;
> > > >  }
> > > >  
> > > > +static int __maybe_unused advk_pcie_suspend(struct device *dev)
> > > > +{
> > > > + struct advk_pcie *pcie = dev_get_drvdata(dev);
> > > > +
> > > > + advk_pcie_disable_phy(pcie);
> > > > +
> > > > + clk_disable_unprepare(pcie->clk);  
> > > 
> > > I have noticed it is common practice, still, I would like to check whether
> > > it is allowed to call functions that may sleep in a NOIRQ suspend/resume
> > > callback ?
> > 
> > You are right this is weird. I double checked and for instance,
> > pcie-mediatek.c, pci-tegra.c and pci-imx6.c do the exact same thing.
> > There are probably other cases where drivers call functions that may
> > sleep from a NOIRQ context. I am interested to know if this is valid
> > and if not, what is the alternative?
> 
> I added Stephen and Mike, who along with Rafael can help us shed some
> light into this, I do not have the necessary bits of info myself, I just
> noticed.
> 

Is the noirq phase of system suspend run with irqs disabled? Or just run
with the device irqs disabled? I thought it was the latter, which is
fine for this scenario because it's still running in a schedulable
context.



Re: [PATCH v15 5/5] clk: imx: Add clock driver for i.MX8MQ CCM

2018-12-03 Thread Stephen Boyd
Quoting Abel Vesa (2018-12-01 02:52:15)
> Add driver for the Clock Control Module found on i.MX8MQ.
> 
> Signed-off-by: Anson Huang 
> Signed-off-by: Bai Ping 
> Signed-off-by: Lucas Stach 
> Signed-off-by: Abel Vesa 
> Reviewed-by: Sascha Hauer 
> ---

Applied to clk-next

but checkpatch complains about the parent arrays being const char *
const instead of const char *. Can you send a followup to fix that?



Re: [PATCH v15 4/5] clk: imx: Add imx composite clock

2018-12-03 Thread Stephen Boyd
Quoting Abel Vesa (2018-12-01 02:52:14)
> Since a lot of clocks on imx8m are formed by a mux, gate, predivider and
> divider, the idea here is to combine all of those into one composite clock,
> but we need to deal with both predivider and divider at the same time and
> therefore we add the imx8m_clk_composite_divider_ops and register
> the composite clock with those.
> 
> Signed-off-by: Abel Vesa 
> Suggested-by: Sascha Hauer 
> Reviewed-by: Sascha Hauer 
> ---

Applied to clk-next



Re: [PATCH v15 3/5] clk: imx: Add SCCG PLL type

2018-12-03 Thread Stephen Boyd
Quoting Abel Vesa (2018-12-01 02:52:13)
> From: Lucas Stach 
> 
> The SCCG is a new PLL type introduced on i.MX8.
> 
> The description of this SCCG clock can be found here:
> 
> https://www.nxp.com/docs/en/reference-manual/IMX8MDQLQRM.pdf#page=834
> 
> Signed-off-by: Lucas Stach 
> Signed-off-by: Abel Vesa 
> Reviewed-by: Sascha Hauer 
> ---

Applied to clk-next



Re: [PATCH v15 2/5] clk: imx: Add fractional PLL output clock

2018-12-03 Thread Stephen Boyd
Quoting Abel Vesa (2018-12-01 02:52:11)
> From: Lucas Stach 
> 
> This is a new fractional clock type introduced on i.MX8.
> 
> The description of this fractional clock can be found here:
> 
> https://www.nxp.com/docs/en/reference-manual/IMX8MDQLQRM.pdf#page=834
> 
> Signed-off-by: Lucas Stach 
> Signed-off-by: Abel Vesa 
> Reviewed-by: Sascha Hauer 
> ---

Applied to clk-next



Re: [PATCH v15 1/5] dt-bindings: Add binding for i.MX8MQ CCM

2018-12-03 Thread Stephen Boyd
Quoting Abel Vesa (2018-12-01 02:52:10)
> From: Lucas Stach 
> 
> This adds the binding for the i.MX8MQ Clock Controller Module.
> 
> Signed-off-by: Lucas Stach 
> Signed-off-by: Abel Vesa 
> Reviewed-by: Rob Herring 
> ---

Applied to clk-next



Re: [PATCH v2] clk: qcom: Fix MSM8998 resets

2018-12-03 Thread Stephen Boyd
Quoting Jeffrey Hugo (2018-12-03 08:13:43)
> The offsets for the defined BCR reset registers does not match the hardware
> documentation.  Update the values to match the hardware documentation.
> 
> Fixes: b5f5f525c547 (clk: qcom: Add MSM8998 Global Clock Control (GCC) driver)
> Signed-off-by: Jeffrey Hugo 
> Reviewed-by: Bjorn Andersson 
> ---

Applied to clk-next



Re: [PATCH v3 2/3] dt-bindings: clk: meson: add main controller clock input

2018-12-03 Thread Stephen Boyd
Quoting Jerome Brunet (2018-12-03 09:16:39)
> Add the clock input of the main clock controller
> 
> Signed-off-by: Jerome Brunet 
> ---

Reviewed-by: Stephen Boyd 



Re: [PATCH v3 1/3] dt-bindings: clk: meson: add ao controller clock inputs

2018-12-03 Thread Stephen Boyd
Quoting Jerome Brunet (2018-12-03 09:16:38)
> Add the clock inputs of amlogic AO clock controller
> 
> Signed-off-by: Jerome Brunet 
> ---

Reviewed-by: Stephen Boyd 



Re: [PATCH] clk: qcom: Fix MSM8998 resets

2018-12-03 Thread Stephen Boyd
Quoting Jeffrey Hugo (2018-12-03 09:19:20)
> On 12/3/2018 10:02 AM, Stephen Boyd wrote:
> > Quoting Jeffrey Hugo (2018-12-03 08:08:46)
> >> On 12/3/2018 8:55 AM, Bjorn Andersson wrote:
> >>> On Mon 03 Dec 07:34 PST 2018, Jeffrey Hugo wrote:
> >>>
> >>>> The offsets for the defined BCR reset registers does not match the 
> >>>> hardware
> >>>> documentation.  Update the values to match the hardware documentation.
> >>>>
> >>>
> >>> Sorry for not spotting this before.
> >>>
> >>>> Fixes: b5f5f525c547 (clk: qcom: Add MSM8998 Global Clock Control (GCC) 
> >>>> driver)
> >>>> Signed-off-by: Jeffrey Hugo 
> >>>> ---
> >>>>drivers/clk/qcom/gcc-msm8998.c | 38 
> >>>> +++---
> >>>>1 file changed, 19 insertions(+), 19 deletions(-)
> >>>>
> >>>> diff --git a/drivers/clk/qcom/gcc-msm8998.c 
> >>>> b/drivers/clk/qcom/gcc-msm8998.c
> >>>> index 9f0ae40..01cc555 100644
> >>>> --- a/drivers/clk/qcom/gcc-msm8998.c
> >>>> +++ b/drivers/clk/qcom/gcc-msm8998.c
> >>>> @@ -2742,25 +2742,25 @@ enum {
> >>>>};
> >>>>
> >>>>static const struct qcom_reset_map gcc_msm8998_resets[] = {
> >>>> -[GCC_BLSP1_QUP1_BCR] = { 0x102400 },
> >>>> -[GCC_BLSP1_QUP2_BCR] = { 0x110592 },
> >>>> -[GCC_BLSP1_QUP3_BCR] = { 0x118784 },
> >>>> -[GCC_BLSP1_QUP4_BCR] = { 0x126976 },
> >>>> -[GCC_BLSP1_QUP5_BCR] = { 0x135168 },
> >>>> -[GCC_BLSP1_QUP6_BCR] = { 0x143360 },
> >>>> -[GCC_BLSP2_QUP1_BCR] = { 0x155648 },
> >>>> -[GCC_BLSP2_QUP2_BCR] = { 0x163840 },
> >>>> -[GCC_BLSP2_QUP3_BCR] = { 0x172032 },
> >>>> -[GCC_BLSP2_QUP4_BCR] = { 0x180224 },
> >>>> -[GCC_BLSP2_QUP5_BCR] = { 0x188416 },
> >>>> -[GCC_BLSP2_QUP6_BCR] = { 0x196608 },
> >>>> -[GCC_PCIE_0_BCR] = { 0x438272 },
> >>>> -[GCC_PDM_BCR] = { 0x208896 },
> >>>> -[GCC_SDCC2_BCR] = { 0x81920 },
> >>>> -[GCC_SDCC4_BCR] = { 0x90112 },
> >>>> -[GCC_TSIF_BCR] = { 0x221184 },
> >>>> -[GCC_UFS_BCR] = { 0x479232 },
> >>>> -[GCC_USB_30_BCR] = { 0x61440 },
> >>>> +[GCC_BLSP1_QUP1_BCR] = { 0x19000 },
> >>>> +[GCC_BLSP1_QUP2_BCR] = { 0x1b000 },
> >>>> +[GCC_BLSP1_QUP3_BCR] = { 0x1d000 },
> >>>> +[GCC_BLSP1_QUP4_BCR] = { 0x1f000 },
> >>>> +[GCC_BLSP1_QUP5_BCR] = { 0x21000 },
> >>>> +[GCC_BLSP1_QUP6_BCR] = { 0x23000 },
> >>>> +[GCC_BLSP2_QUP1_BCR] = { 0x26000 },
> >>>> +[GCC_BLSP2_QUP2_BCR] = { 0x28000 },
> >>>> +[GCC_BLSP2_QUP3_BCR] = { 0x2a000 },
> >>>> +[GCC_BLSP2_QUP4_BCR] = { 0x2c000 },
> >>>> +[GCC_BLSP2_QUP5_BCR] = { 0x2e000 },
> >>>> +[GCC_BLSP2_QUP6_BCR] = { 0x3 },
> >>>> +[GCC_PCIE_0_BCR] = { 0x6c01c },
> >>>
> >>> I find GCC_PCIE_0_BCR at 0x6b000 and then GCC_PCIE_0_PHY_BCR at 0x6c01c.
> >>
> >> Doh.  Thanks for the double check.  GCC_PCIE_0_PHY_BCR is not defined in
> >> include/dt-bindings/clock/qcom,gcc-msm8998.h so I plan to leave it out
> >> until later.
> >>
> >> Expect a v2 shortly.
> > 
> > Will you add GCC_PCIE0_PHY_BCR shortly so we don't have to add it later
> > on when it becomes critical?
> > 
> 
> My plan was to let it sit until it becomes necessary.  I'm working on 
> USB and found that GCC_QUSB2PHY_PRIM_BCR, GCC_USB3_PHY_BCR, and 
> GCC_USB3PHY_PHY_BCR are also missing, so I suspect there are others.
> 
> Would you prefer I send a follow up that adds the PCIE phy and the USB 
> resets?

Yes please send a followup patch to add all the possible defines and
implementations that you can find. It makes future cross-tree merges
simpler.



Re: [PATCH v11 3/3] clk: qcom: Add lpass clock controller driver for SDM845

2018-12-03 Thread Stephen Boyd
Quoting Taniya Das (2018-11-30 10:21:29)
> Add support for the lpass clock controller found on SDM845 based devices.
> This would allow lpass peripheral loader drivers to control the clocks to
> bring the subsystem out of reset.
> LPASS clocks present on the global clock controller would be registered
> with the clock framework based on the protected-clock flag. Also do not
> gate these clocks if they are left unused, as the lpass clocks require
> the global clock controller lpass clocks to be enabled before they are
> accessed. Mark the GCC lpass clocks as CRITICAL, for the LPASS clock
> access.
> 
> Signed-off-by: Taniya Das 
> ---

Applied to clk-next + added another #ifdef around unused clks to silence
unused warning.



Re: [PATCH v11 2/3] dt-bindings: clock: Introduce QCOM LPASS clock bindings

2018-12-03 Thread Stephen Boyd
Quoting Taniya Das (2018-11-30 10:21:28)
> Add device tree bindings for Low Power Audio subsystem clock controller for
> Qualcomm Technology Inc's SDM845 SoCs.
> 
> Reviewed-by: Rob Herring 
> Signed-off-by: Taniya Das 
> ---

Applied to clk-next

I noticed that a #define was lost after some revisions for some always
on clk, but I suspect we never care and will never need to turn it on so
I just went with it and dropped it too. Let's cross fingers and hope
that doesn't come back to haunt us!



Re: [PATCH v11 1/3] dt-bindings: clock: Update GCC bindings for protected-clocks

2018-12-03 Thread Stephen Boyd
Quoting Taniya Das (2018-11-30 10:21:27)
> Add protected-clocks list which could used to specify the clocks to be
> bypassed on certain devices.
> 
> Reviewed-by: Rob Herring 
> Signed-off-by: Taniya Das 
> ---

Applied to clk-next



Re: [PATCH v2 2/3] clk: ti: check clock type before doing autoidle ops

2018-12-03 Thread Stephen Boyd
Quoting Tony Lindgren (2018-12-03 07:39:10)
> * Stephen Boyd  [181130 23:52]:
> > Quoting Tony Lindgren (2018-11-30 07:37:29)
> > > * Tero Kristo  [181130 09:21]:
> > > > On 30/11/2018 09:57, Stephen Boyd wrote:
> > > ...
> > > 
> > > And that way it would just propagate to the parent clock
> > > domain driver and the clock framework does not need to know
> > > about clockdomains. A clockdomain could be just a genpd
> > > domain.
> > > 
> > > Or do you guys have better ideas?
> > > 
> > 
> > Wouldn't the device link in clk framework patches do this for you if we
> > had the RUNTIME_PM flag passed in. If this is about keeping the clock
> > controller active when a consumer device is using it then I think it may
> > work.
> 
> The consumer device stays active just fine with PM runtime
> calls. So yes, the problem is keeping a clock controller forced
> active for the period of consumer device reset. Other than
> that typically autoidle can be just kept enabled.
> 
> Below is a clarified suggested example usage if we wanted to
> use PM runtime on a clock controller device from a consumer
> device reset driver:
> 
> error = pm_runtime_get_dev()
> ...
> cdev = clk_get_device(clk);
> ...
> error = pm_runtime_get(cdev);
> ...
> /* Do the consumer device reset here */
> ...
> pm_runtime_put(cdev);
> pm_runtime_put(dev);
> 

Does the consumer reset use the reset framework or something else? If
the driver is using the reset framework, I would expect the reset
framework to _also_ have device links and keep the clock controller,
i.e. reset provider, active while the reset is being toggled. And this
assumes the reset controller and clock controller code is all rolled up
together in a single driver that can tell itself to deny idle for
certain clks that are associated with whatever resets they affect.



Re: [PATCH] clk: qcom: Fix MSM8998 resets

2018-12-03 Thread Stephen Boyd
Quoting Jeffrey Hugo (2018-12-03 08:08:46)
> On 12/3/2018 8:55 AM, Bjorn Andersson wrote:
> > On Mon 03 Dec 07:34 PST 2018, Jeffrey Hugo wrote:
> > 
> >> The offsets for the defined BCR reset registers does not match the hardware
> >> documentation.  Update the values to match the hardware documentation.
> >>
> > 
> > Sorry for not spotting this before.
> > 
> >> Fixes: b5f5f525c547 (clk: qcom: Add MSM8998 Global Clock Control (GCC) 
> >> driver)
> >> Signed-off-by: Jeffrey Hugo 
> >> ---
> >>   drivers/clk/qcom/gcc-msm8998.c | 38 
> >> +++---
> >>   1 file changed, 19 insertions(+), 19 deletions(-)
> >>
> >> diff --git a/drivers/clk/qcom/gcc-msm8998.c 
> >> b/drivers/clk/qcom/gcc-msm8998.c
> >> index 9f0ae40..01cc555 100644
> >> --- a/drivers/clk/qcom/gcc-msm8998.c
> >> +++ b/drivers/clk/qcom/gcc-msm8998.c
> >> @@ -2742,25 +2742,25 @@ enum {
> >>   };
> >>   
> >>   static const struct qcom_reset_map gcc_msm8998_resets[] = {
> >> -[GCC_BLSP1_QUP1_BCR] = { 0x102400 },
> >> -[GCC_BLSP1_QUP2_BCR] = { 0x110592 },
> >> -[GCC_BLSP1_QUP3_BCR] = { 0x118784 },
> >> -[GCC_BLSP1_QUP4_BCR] = { 0x126976 },
> >> -[GCC_BLSP1_QUP5_BCR] = { 0x135168 },
> >> -[GCC_BLSP1_QUP6_BCR] = { 0x143360 },
> >> -[GCC_BLSP2_QUP1_BCR] = { 0x155648 },
> >> -[GCC_BLSP2_QUP2_BCR] = { 0x163840 },
> >> -[GCC_BLSP2_QUP3_BCR] = { 0x172032 },
> >> -[GCC_BLSP2_QUP4_BCR] = { 0x180224 },
> >> -[GCC_BLSP2_QUP5_BCR] = { 0x188416 },
> >> -[GCC_BLSP2_QUP6_BCR] = { 0x196608 },
> >> -[GCC_PCIE_0_BCR] = { 0x438272 },
> >> -[GCC_PDM_BCR] = { 0x208896 },
> >> -[GCC_SDCC2_BCR] = { 0x81920 },
> >> -[GCC_SDCC4_BCR] = { 0x90112 },
> >> -[GCC_TSIF_BCR] = { 0x221184 },
> >> -[GCC_UFS_BCR] = { 0x479232 },
> >> -[GCC_USB_30_BCR] = { 0x61440 },
> >> +[GCC_BLSP1_QUP1_BCR] = { 0x19000 },
> >> +[GCC_BLSP1_QUP2_BCR] = { 0x1b000 },
> >> +[GCC_BLSP1_QUP3_BCR] = { 0x1d000 },
> >> +[GCC_BLSP1_QUP4_BCR] = { 0x1f000 },
> >> +[GCC_BLSP1_QUP5_BCR] = { 0x21000 },
> >> +[GCC_BLSP1_QUP6_BCR] = { 0x23000 },
> >> +[GCC_BLSP2_QUP1_BCR] = { 0x26000 },
> >> +[GCC_BLSP2_QUP2_BCR] = { 0x28000 },
> >> +[GCC_BLSP2_QUP3_BCR] = { 0x2a000 },
> >> +[GCC_BLSP2_QUP4_BCR] = { 0x2c000 },
> >> +[GCC_BLSP2_QUP5_BCR] = { 0x2e000 },
> >> +[GCC_BLSP2_QUP6_BCR] = { 0x3 },
> >> +[GCC_PCIE_0_BCR] = { 0x6c01c },
> > 
> > I find GCC_PCIE_0_BCR at 0x6b000 and then GCC_PCIE_0_PHY_BCR at 0x6c01c.
> 
> Doh.  Thanks for the double check.  GCC_PCIE_0_PHY_BCR is not defined in 
> include/dt-bindings/clock/qcom,gcc-msm8998.h so I plan to leave it out 
> until later.
> 
> Expect a v2 shortly.

Will you add GCC_PCIE0_PHY_BCR shortly so we don't have to add it later
on when it becomes critical?



Re: [PATCH v11 2/2] cpufreq: qcom-hw: Add support for QCOM cpufreq HW driver

2018-12-03 Thread Stephen Boyd
Quoting Taniya Das (2018-12-01 19:55:03)
> The CPUfreq HW present in some QCOM chipsets offloads the steps necessary
> for changing the frequency of CPUs. The driver implements the cpufreq
> driver interface for this hardware engine.
> 
> Signed-off-by: Saravana Kannan 
> Signed-off-by: Stephen Boyd 
> Signed-off-by: Taniya Das 
> ---

Reviewed-by: Stephen Boyd 



Re: [PATCH v11 1/2] dt-bindings: cpufreq: Introduce QCOM CPUFREQ Firmware bindings

2018-12-03 Thread Stephen Boyd
Quoting Taniya Das (2018-12-01 19:55:02)
> Add QCOM cpufreq firmware device bindings for Qualcomm Technology Inc's
> SoCs. This is required for managing the cpu frequency transitions which are
> controlled by the hardware engine.
> 
> Signed-off-by: Taniya Das 
> ---

Reviewed-by: Stephen Boyd 



Re: [PATCH v3] tty: serial: qcom_geni_serial: Fix softlock

2018-12-03 Thread Stephen Boyd
Quoting Ryan Case (2018-11-29 18:18:40)
> Transfers were being divided into device FIFO sized (64 byte max)
> operations which would poll for completion within a spin_lock_irqsave /
> spin_unlock_irqrestore block. This both made things slow by waiting for
> the FIFO to completely drain before adding further data and would also
> result in softlocks on large transmissions.
> 
> This patch allows larger transfers with continuous FIFO additions as
> space becomes available and removes polling from the interrupt handler.
> 
> Signed-off-by: Ryan Case 
> ---

Reviewed-by: Stephen Boyd 



Re: [PATCH v2 2/3] clk: ti: check clock type before doing autoidle ops

2018-11-30 Thread Stephen Boyd
Quoting Tony Lindgren (2018-11-30 07:37:29)
> Hi,
> 
> * Tero Kristo  [181130 09:21]:
> > On 30/11/2018 09:57, Stephen Boyd wrote:
> > > No that is not preferred. Can the omap2_clk_deny_idle() function be
> > > integrated closer into the clk framework in some way that allows it to
> > > be part of the clk_ops structure? And then have that take a clk_hw
> > > structure instead of a struct clk? I haven't looked at this in any
> > > detail whatsoever so I may be way off right now.
> > 
> > It could be added under the main clk_ops struct, however this would
> > introduce two new func pointers to it which are not used by anything else
> > but OMAP. Are you aware of any other platforms requiring similar feature?
> 
> From consumer usage point of view, I'm still wondering about
> the relationship of clk_deny_idle() and clkdm_deny_idle().
> 
> It seems that we need to allow reset control drivers call
> clk_deny_idle() for the duration of reset. And it seems the
> clk_deny_idle() should propagate to also up to the related
> clock domain driver to do clkdm_deny_idle().
> 
> So maybe clk_deny_idle() is could just be something like:
> 
> dev = clk_get_device(clk);
> ...
> error = pm_runtime_get(dev);
> ...
> pm_runtime_put(dev);
> ...
> 
> And that way it would just propagate to the parent clock
> domain driver and the clock framework does not need to know
> about clockdomains. A clockdomain could be just a genpd
> domain.
> 
> Or do you guys have better ideas?
> 

Wouldn't the device link in clk framework patches do this for you if we
had the RUNTIME_PM flag passed in. If this is about keeping the clock
controller active when a consumer device is using it then I think it may
work.



Re: [PATCH 2/2] clk: Add Fixed MMIO clock driver

2018-11-30 Thread Stephen Boyd
Quoting Janek Kotas (2018-11-30 01:57:09)
> 
> I tried it. io_remap doesn’t allow RAM to be mapped,
> even if it’s marked as a reserved memory.
> I got an error, the address couldn’t be mapped.
> 
> ARM64’s memory management has an explicit check for this here:
> https://elixir.bootlin.com/linux/latest/source/arch/arm64/mm/ioremap.c#L55
> 

Ok. Thanks!



Re: [PATCH v8] clk: Add (devm_)clk_get_optional() functions

2018-11-30 Thread Stephen Boyd
Quoting Russell King - ARM Linux (2018-11-30 03:04:46)
> On Fri, Nov 30, 2018 at 10:25:37AM +, Phil Edworthy wrote:
> > On 30 November 2018 09:09 Stephen Boyd wrote:
> > > Quoting Phil Edworthy (2018-11-20 06:14:45)
> > > > This adds clk_get_optional() and devm_clk_get_optional() functions to
> > > > get optional clocks.
> > > > They behave the same as (devm_)clk_get except where there is no clock
> > > > producer. In this case, instead of returning -ENOENT, the function
> > > > returns NULL. This makes error checking simpler and allows
> > > > clk_prepare_enable, etc to be called on the returned reference without
> > > > additional checks.
> > > 
> > > Ok. I guess that works by virtue of how -ENOENT is returned by various
> > > functions that are called deeper in the clk_get() path? I'm cautiously
> > > optimistic. So cautious, we should probably add a comment to these 
> > > optional
> > > functions that indicate they rely on the functions they call to return 
> > > -ENOENT
> > > under the various conditions that make a clk optional.
> > Yes, it does indeed rely on how clk_get() is implemented.
> > Specifically, that if __of_clk_get_by_name() returns -EINVAL, the error is
> > superseded by clk_get_sys() returning -ENOENT.
> > As you say, a comment may help here.
> 
> Each time the question of the optional clk_get() stuff comes up, we go
> around the same discussions time and time again.  So far, each time
> has ended up flopping.
> 
> Yes, clk_get() can only ever return -ENOENT if it falls back to the
> non-DT methods, because it assumes that the clk tables are complete
> (it can do nothing else.)
> 
> I don't think it needs a comment because it's obvious from the code
> and also from the implementation point of view.

Ok. I would still suggest a comment on of_clk_get_by_name() that
indicates what types of return values happen in there. Otherwise it's
obvious from the code after reading 3 or 4 functions deep
(__of_clk_get_by_name -> __of_clk_get -> of_parse_phandle_with_args and
__of_clk_get_from_provider) that this is what happens.

> 
> > > > +static inline struct clk *clk_get_optional(struct device *dev, const
> > > > +char *id)
> > > 
> > > Any kernel doc for this function?
> > I took my cue from the surrounding functions, let me know if I have to add 
> > it.
> 
> I don't see you need to - this is an internal function by way of the
> "static inline" you have before it.  It's not an API function.

It's static inline in a header file. That is an API function as far as I
can tell.



Re: [PATCH 2/2] clk: core: link consumer with clock driver

2018-11-30 Thread Stephen Boyd
Quoting Miquel Raynal (2018-11-23 01:11:32)
> Would you agree with me adding dummy functions in the #else section
> like:
> 
> static inline void __clk_device_link(struct device *consumer, struct clk *clk)
> {
>return;
> }
> 
> static inline void __clk_device_unlink(struct clk *clk)
> {
>return;
> }
> 
> Do you want me to also declare these functions in the #if section
> (with the external keyword) to balance the above declarations?

Why can't we do the linking in __clk_get() and __clk_put()?



Re: [PATCH 0/2] Link consumer with clock driver

2018-11-30 Thread Stephen Boyd
Quoting Miquel Raynal (2018-11-22 13:22:10)
> Hello,
> 
> While working on suspend to RAM feature, I ran into troubles multiple
> times when clocks where not suspending/resuming at the desired time. I
> had a look at the core and I think the same logic as in the
> regulator's core may be applied here to (very easily) fix this issue:
> using device links.

Thanks! I've wanted to add device links to the clk framework for a while
now but haven't gotten around to it. Can you expand on the scenario that
you've run into where you need this?

> 
> The only additional change I had to do was to always (when available)
> populate the device entry of the core clock structure so that it could
> be used later. This is the purpose of patch 1. Patch 2 actually adds
> support for device links.

That's fine. We should do it into clk_core all the time for other
reasons too.

> 
> As I am not used to hack into the clock subsystem I might have missed
> something big preventing such change but so far I could not see
> anything wrong with it. As this touches core code, I am of course
> entirely open to suggestions.
> 

Two questions:

 1) Do device links work if we make a loop between devices? I'm thinking
 of the case where we have two clock controllers that provide clks to
 each other and could potentially register some clks and then clk_get()
 the ones they depend on. I suspect loops don't work and so we need to
 be aware of this and somehow prevent it.

 2) Do we need to link clk consumers to anything besides the device
 providing the clk they get from clk_get()? Put another way, do we need
 to make links through the clk tree and any potential parent clks of
 whatever the device is using at the time. Would clk tree changing
 topology because some new parent is chosen need to affect
 suspend/resume ordering? Or would that all need to be handled by clk
 framework figuring out the ordering of the tree at suspend/resume time
 and suspending clock controller drivers in the right order?




Re: [PATCH v8] clk: Add (devm_)clk_get_optional() functions

2018-11-30 Thread Stephen Boyd
Quoting Phil Edworthy (2018-11-20 06:14:45)
> This adds clk_get_optional() and devm_clk_get_optional() functions to get
> optional clocks.
> They behave the same as (devm_)clk_get except where there is no clock
> producer. In this case, instead of returning -ENOENT, the function
> returns NULL. This makes error checking simpler and allows
> clk_prepare_enable, etc to be called on the returned reference
> without additional checks.

Ok. I guess that works by virtue of how -ENOENT is returned by various
functions that are called deeper in the clk_get() path? I'm cautiously
optimistic. So cautious, we should probably add a comment to these
optional functions that indicate they rely on the functions they call to
return -ENOENT under the various conditions that make a clk optional.

> 
> diff --git a/include/linux/clk.h b/include/linux/clk.h
> index a7773b5c0b9f..3ea3c78f62dd 100644
> --- a/include/linux/clk.h
> +++ b/include/linux/clk.h
> @@ -383,6 +383,17 @@ int __must_check devm_clk_bulk_get_all(struct device 
> *dev,
>   */
>  struct clk *devm_clk_get(struct device *dev, const char *id);
>  
> +/**
> + * devm_clk_get_optional - lookup and obtain a managed reference to an 
> optional
> + *clock producer.
> + * @dev: device for clock "consumer"
> + * @id: clock consumer ID
> + *
> + * Behaves the same as devm_clk_get except where there is no clock producer. 
> In

Please add () around devm_clk_get() so we know it's a function.

> + * this case, instead of returning -ENOENT, the function returns NULL.
> + */
> +struct clk *devm_clk_get_optional(struct device *dev, const char *id);
> +
>  /**
>   * devm_get_clk_from_child - lookup and obtain a managed reference to a
>   *  clock producer from child node.
> @@ -718,6 +729,12 @@ static inline struct clk *devm_clk_get(struct device 
> *dev, const char *id)
> return NULL;
>  }
>  
> +static inline struct clk *devm_clk_get_optional(struct device *dev,
> +   const char *id)
> +{
> +   return NULL;
> +}
> +
>  static inline int __must_check devm_clk_bulk_get(struct device *dev, int 
> num_clks,
>  struct clk_bulk_data *clks)
>  {
> @@ -862,6 +879,16 @@ static inline void clk_bulk_disable_unprepare(int 
> num_clks,
> clk_bulk_unprepare(num_clks, clks);
>  }
>  
> +static inline struct clk *clk_get_optional(struct device *dev, const char 
> *id)

Any kernel doc for this function?

> +{
> +   struct clk *clk = clk_get(dev, id);
> +
> +   if (clk == ERR_PTR(-ENOENT))
> +   clk = NULL;
> +
> +   return clk;
> +}
> +


Re: [PATCH] dt-bindings: clock: Require #reset-cells in sdm845-videocc

2018-11-30 Thread Stephen Boyd
Quoting Douglas Anderson (2018-11-27 11:24:43)
> The #reset-cells was listed as optional in the bindings for
> qcom,sdm845-videocc.  There's no reason for it to be optional.  As per
> Stephen [1]:
> 
> > We should update the binding to make it a required property. It
> > doesn't make any sense why that property would be optional given
> > that the hardware either has support for resets or it doesn't.
> 
> sdm845 support is still in its infancy so we shouldn't be affecting
> any real device trees by modifying the bindings here.
> 
> [1] 
> https://lkml.kernel.org/r/154330186815.88331.12720647562079303...@swboyd.mtv.corp.google.com
> 
> Fixes: 84b66b211603 ("dt-bindings: clock: Introduce QCOM Video clock 
> bindings")
> Suggested-by: Stephen Boyd 
> Signed-off-by: Douglas Anderson 
> ---

Applied to clk-next



Re: [PATCH 2/2] clk: Add Fixed MMIO clock driver

2018-11-30 Thread Stephen Boyd
Quoting Janek Kotas (2018-11-30 00:52:16)
> 
> We have a single register in a fixed location which contains 
> the frequency of the main system clock.
> This allows us to use the same image in emulation, FPGA
> and simulation without any changes.
> We usually don’t use a full bootloader, just a simple wrapper,
> which initializes the necessary stuff and jumps to the kernel.
> 
> 
> So the hardware team has decided to throw a frequency register in there?
> Alright! Does that fixed rate clk generate its fixed rate from some
> other clk? I’m curious if this fixed rate clk has a parent source.
> 
> 
> It depends, in simulation and emulation we can just generate 
> a clock source with a given frequency.
> On FPGA it’s usually an external oscillator, sometimes a fixed PLL.
> The driver is not intended to be used in full, complex SoCs.

Ok, sounds like it isn't worth modeling this then.

> 
> 
> It would also be good to make sure that any clks registered from this
> driver can't be populated from regions of memory like DDR. Can you
> confirm? I think it will fail, but it would be worth checking
> 

Please try this.

> Yes I'd prefer a platform driver unless there's some reason it can't be
> done. We may need to have both in case this needs to be populated very
> early, but if that isn’t the case then just a platform driver for now.
> 
> 
> I played with it a bit, and it and it looks like I need both.
> We use this clock source with some of our components, 
> and for some of them the platform driver was initialized too late.
> 

Ok. So do both then.



[PATCH 1/2] clk: mediatek: Drop __init from mtk_clk_register_cpumuxes()

2018-11-30 Thread Stephen Boyd
This function is used from more places than just __init code. Removing
__init silences a section mismatch warning here.

Cc: Sean Wang 
Cc: Ryder Lee 
Cc: Rob Herring 
Cc: Wenzhen Yu 
Cc: Weiyi Lu 
Signed-off-by: Stephen Boyd 
---
 drivers/clk/mediatek/clk-cpumux.c | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/clk/mediatek/clk-cpumux.c 
b/drivers/clk/mediatek/clk-cpumux.c
index 16e56772d280..6c7eaa21e662 100644
--- a/drivers/clk/mediatek/clk-cpumux.c
+++ b/drivers/clk/mediatek/clk-cpumux.c
@@ -53,7 +53,7 @@ static const struct clk_ops clk_cpumux_ops = {
.set_parent = clk_cpumux_set_parent,
 };
 
-static struct clk __init *
+static struct clk *
 mtk_clk_register_cpumux(const struct mtk_composite *mux,
struct regmap *regmap)
 {
@@ -84,9 +84,9 @@ mtk_clk_register_cpumux(const struct mtk_composite *mux,
return clk;
 }
 
-int __init mtk_clk_register_cpumuxes(struct device_node *node,
-const struct mtk_composite *clks, int num,
-struct clk_onecell_data *clk_data)
+int mtk_clk_register_cpumuxes(struct device_node *node,
+ const struct mtk_composite *clks, int num,
+ struct clk_onecell_data *clk_data)
 {
int i;
struct clk *clk;
-- 
Sent by a computer through tubes



[PATCH 2/2] clk: mediatek: Drop more __init markings for driver probe

2018-11-30 Thread Stephen Boyd
This function is called from driver probe, which isn't the same as
__init code because driver probe can happen later. Drop the __init
marking here to fix this potential problem.

Cc: Sean Wang 
Cc: Ryder Lee 
Cc: Rob Herring 
Cc: Wenzhen Yu 
Cc: Weiyi Lu 
Fixes: 2fc0a509e4ee ("clk: mediatek: add clock support for MT7622 SoC")
Signed-off-by: Stephen Boyd 
---
 drivers/clk/mediatek/clk-mt7622.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/clk/mediatek/clk-mt7622.c 
b/drivers/clk/mediatek/clk-mt7622.c
index 92f7e32770c6..a8aecef1ba89 100644
--- a/drivers/clk/mediatek/clk-mt7622.c
+++ b/drivers/clk/mediatek/clk-mt7622.c
@@ -513,7 +513,7 @@ static const struct mtk_gate peri_clks[] = {
GATE_PERI1(CLK_PERI_IRTX_PD, "peri_irtx_pd", "irtx_sel", 2),
 };
 
-static struct mtk_composite infra_muxes[] __initdata = {
+static struct mtk_composite infra_muxes[] = {
MUX(CLK_INFRA_MUX1_SEL, "infra_mux1_sel", infra_mux1_parents,
0x000, 2, 2),
 };
@@ -652,7 +652,7 @@ static int mtk_topckgen_init(struct platform_device *pdev)
return of_clk_add_provider(node, of_clk_src_onecell_get, clk_data);
 }
 
-static int __init mtk_infrasys_init(struct platform_device *pdev)
+static int mtk_infrasys_init(struct platform_device *pdev)
 {
struct device_node *node = pdev->dev.of_node;
struct clk_onecell_data *clk_data;
-- 
Sent by a computer through tubes



  1   2   3   4   5   6   7   8   9   10   >